KVM/ARM Implementation

The following series implements KVM support for ARM processors, specifically on the Cortex A-15 platform.

Still on the to-do list:
 - Reuse VMIDs
 - Fix SMP host support
 - Fix SMP guest support
 - Support guest Thumb mode for MMIO emulation
 - Further testing
 - Performance improvements

Userspace can inject IRQs and FIQs through the KVM_IRQ_LINE VM ioctl. This ioctl is used since the sematics are in fact two lines that can be either raised or lowered on the VCPU - the IRQ and FIQ lines.

KVM needs to know which VCPU it must operate on and whether the FIQ or IRQ line is raised/lowered. Hence both pieces of information is packed in the kvm_irq_level->irq field. The irq fild value will be:
IRQ: vcpu_index * 2
FIQ: (vcpu_index * 2) + 1

This is documented in Documentation/kvm/api.txt.

The effect of the ioctl is simply to simply raise/lower the corresponding virt_irq field on the VCPU struct, which will cause the world-switch code to raise/lower virtual interrupts when running the guest on next switch. The wait_for_interrupt flag is also cleared for raised IRQs causing an idle VCPU to become active again.

Note: The custom trace_kvm_irq_line is used despite a generic definition of trace_kvm_set_irq, since the trace-Kvm_set_irq depends on the x86-specific define of __HAVE_IOAPIC. Either the trace event should be created regardless of this define or it should depend on another ifdef clause, common for both x86 and ARM. However, since the arguments don't really match those used in ARM, I am yet to be convinced why this is necessary.

Signed-off-by Christoffer Dall <[email protected]>

---
 Documentation/virtual/kvm/api.txt |   10 ++++++-
 arch/arm/include/asm/kvm.h        |    8 ++++++
 arch/arm/include/asm/kvm_arm.h    |    1 +
 arch/arm/kvm/arm.c                |   53 ++++++++++++++++++++++++++++++++++++-
 arch/arm/kvm/trace.h              |   21 +++++++++++++++
 include/linux/kvm.h               |    1 +
 6 files changed, 91 insertions(+), 3 deletions(-)

Documentation/virtual/kvm/api.txt +1 -1

index 7945b0b..4abaa67 100644
@@ -572,7 +572,7 @@ only go to the IOAPIC.  On ia64, a IOSAPIC is created.
 4.25 KVM_IRQ_LINE
 
 Capability: KVM_CAP_IRQCHIP
-Architectures: x86, ia64
+Architectures: x86, ia64, arm
 Type: vm ioctl
 Parameters: struct kvm_irq_level
 Returns: 0 on success, -1 on error

Documentation/virtual/kvm/api.txt +6 -0

+
+ARM uses two types of interrupt lines per CPU, ie. IRQ and FIQ. The value of the
+irq field should be (VCPU_INDEX * 2) for IRQs and ((VCPU_INDEX * 2) + 1) for
+FIQs. Level is used to raise/lower the line. See arch/arm/include/asm/kvm.h for
+convenience macros.
+
 struct kvm_irq_level {
 	union {
 		__u32 irq;     /* GSI */

arch/arm/include/asm/kvm.h +8 -0

index 87dc33b..8935062 100644
@@ -20,6 +20,14 @@
 #include <asm/types.h>
 
 /*
+ * KVM_IRQ_LINE macros to set/read IRQ/FIQ for specific VCPU index.
+ */
+enum KVM_ARM_IRQ_LINE_TYPE {
+	KVM_ARM_IRQ_LINE = 0,
+	KVM_ARM_FIQ_LINE = 1,
+};
+
+/*
  * Modes used for short-hand mode determinition in the world-switch code and
  * in emulation code.
  *

arch/arm/include/asm/kvm_arm.h +1 -0

index 835abd1..e378a37 100644
@@ -49,6 +49,7 @@
 #define HCR_VM		1
 #define HCR_GUEST_MASK (HCR_TSC | HCR_TWE | HCR_TWI | HCR_VM | HCR_AMO | \
 			HCR_AMO | HCR_IMO | HCR_FMO | HCR_SWIO)
+#define HCR_VIRT_EXCP_MASK (HCR_VA | HCR_VI | HCR_VF)
 
 /* Hyp System Control Register (HSCTLR) bits */
 #define HSCTLR_TE	(1 << 30)

arch/arm/kvm/arm.c +14 -0

index 89ba18d..fc0bd6b 100644
@@ -299,6 +299,43 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
 	return -EINVAL;
 }
 
+static int kvm_arch_vm_ioctl_irq_line(struct kvm *kvm,
+				      struct kvm_irq_level *irq_level)
+{
+	u32 mask;
+	unsigned int vcpu_idx;
+	struct kvm_vcpu *vcpu;
+
+	vcpu_idx = irq_level->irq / 2;
+	if (vcpu_idx >= KVM_MAX_VCPUS)
+		return -EINVAL;
+
+	vcpu = kvm_get_vcpu(kvm, vcpu_idx);
+	if (!vcpu)
+		return -EINVAL;

arch/arm/kvm/arm.c +18 -2

+
+	return 0;
+}
+
 long kvm_arch_vcpu_ioctl(struct file *filp,
 			 unsigned int ioctl, unsigned long arg)
 {

@@ -313,8 +350,20 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm, struct kvm_dirty_log *log)
 long kvm_arch_vm_ioctl(struct file *filp,
 		       unsigned int ioctl, unsigned long arg)
 {
-	printk(KERN_ERR "kvm_arch_vm_ioctl: Unsupported ioctl (%d)\n", ioctl);
-	return -EINVAL;
+	struct kvm *kvm = filp->private_data;
+	void __user *argp = (void __user *)arg;
+
+	switch (ioctl) {
+	case KVM_IRQ_LINE: {
+		struct kvm_irq_level irq_event;
+
+		if (copy_from_user(&irq_event, argp, sizeof irq_event))
+			return -EFAULT;
+		return kvm_arch_vm_ioctl_irq_line(kvm, &irq_event);
+	}
+	default:
+		return -EINVAL;
+	}
 }
 
 /**

arch/arm/kvm/trace.h +21 -0

index f8869c1..ac64e3a 100644
@@ -40,6 +40,27 @@ TRACE_EVENT(kvm_exit,
 );
 
 
+TRACE_EVENT(kvm_irq_line,
+	TP_PROTO(unsigned int type, unsigned int level, unsigned int vcpu_idx),
+	TP_ARGS(type, level, vcpu_idx),
+
+	TP_STRUCT__entry(
+		__field(	unsigned int,	type			)
+		__field(	unsigned int,	level			)
+		__field(	unsigned int,	vcpu_idx		)
+	),
+
+	TP_fast_assign(
+		__entry->type			= type;
+		__entry->level			= level;
+		__entry->vcpu_idx		= vcpu_idx;
+	),
+
+	TP_printk("KVM_IRQ_LINE: type: %s, level: %u, vcpu: %u",
+		(__entry->type == KVM_ARM_IRQ_LINE) ? "IRQ" : "FIQ",
+		__entry->level, __entry->vcpu_idx)
+);
+
 
 #endif /* _TRACE_KVM_H */
 

include/linux/kvm.h +1 -1

index c3892fc..679abbb 100644
@@ -111,6 +111,7 @@ struct kvm_irq_level {
 	 * ACPI gsi notion of irq.
 	 * For IA-64 (APIC model) IOAPIC0: irq 0-23; IOAPIC1: irq 24-47..
 	 * For X86 (standard AT mode) PIC0/1: irq 0-15. IOAPIC0: 0-23..
+	 * For ARM: IRQ: irq = (2*vcpu_index). FIQ: irq = (2*vcpu_indx + 1).
 	 */
 	union {
 		__u32 irq;
 
--

To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Just found two, maybe three nits while browsing by:

Documentation/virtual/kvm/api.txt Requires that an interrupt controller model has been previously created with

Quoted 3 lines · Christoffer Dall

Documentation/virtual/kvm/api.txt +3 -0

+ARM uses two types of interrupt lines per CPU, ie. IRQ and FIQ. The value of the
+irq field should be (VCPU_INDEX * 2) for IRQs and ((VCPU_INDEX * 2) + 1) for
+FIQs.

Quoted 18 lines

arch/arm/kvm/arm.c +18 -0

+static int kvm_arch_vm_ioctl_irq_line(struct kvm *kvm,
+                                   struct kvm_irq_level *irq_level)
+{
+     u32 mask;
+     unsigned int vcpu_idx;
+     struct kvm_vcpu *vcpu;
+
+     vcpu_idx = irq_level->irq / 2;
+     if (vcpu_idx >= KVM_MAX_VCPUS)
+             return -EINVAL;
+
+     vcpu = kvm_get_vcpu(kvm, vcpu_idx);
+     if (!vcpu)
+             return -EINVAL;
+
+     switch (irq_level->irq % 2) {
+     case KVM_ARM_IRQ_LINE:
+             mask = HCR_VI;

Quoted 6 lines

+             break;
+     case KVM_ARM_FIQ_LINE:
+             mask = HCR_VF;
+             break;
+     default:
+             return -EINVAL;

Due to % 2, default is unreachable. Remove the masking?

Just found two, maybe three nits while browsing by:

Quoted 52 lines · Christoffer Dall

Userspace can inject IRQs and FIQs through the KVM_IRQ_LINE VM ioctl. This ioctl is used since the sematics are in fact two lines that can be either raised or lowered on the VCPU - the IRQ and FIQ lines.

KVM needs to know which VCPU it must operate on and whether the FIQ or IRQ line is raised/lowered. Hence both pieces of information is packed in the kvm_irq_level->irq field. The irq fild value will be:
  IRQ: vcpu_index * 2
  FIQ: (vcpu_index * 2) + 1

This is documented in Documentation/kvm/api.txt.

The effect of the ioctl is simply to simply raise/lower the corresponding virt_irq field on the VCPU struct, which will cause the world-switch code to raise/lower virtual interrupts when running the guest on next switch. The wait_for_interrupt flag is also cleared for raised IRQs causing an idle VCPU to become active again.

Note: The custom trace_kvm_irq_line is used despite a generic definition of trace_kvm_set_irq, since the trace-Kvm_set_irq depends on the x86-specific define of __HAVE_IOAPIC. Either the trace event should be created regardless of this define or it should depend on another ifdef clause, common for both x86 and ARM. However, since the arguments don't really match those used in ARM, I am yet to be convinced why this is necessary.

Signed-off-by Christoffer Dall <[email protected]>

---
 Documentation/virtual/kvm/api.txt |   10 ++++++- arch/arm/include/asm/kvm.h        |    8 ++++++ arch/arm/include/asm/kvm_arm.h    |    1 +
 arch/arm/kvm/arm.c                |   53 ++++++++++++++++++++++++++++++++++++- arch/arm/kvm/trace.h              |   21 +++++++++++++++ include/linux/kvm.h               |    1 +

 6 files changed, 91 insertions(+), 3 deletions(-)

Documentation/virtual/kvm/api.txt +0 -0

index 7945b0b..4abaa67 100644
@@ -572,7 +572,7 @@ only go to the IOAPIC.  On ia64, a IOSAPIC is created.

 4.25 KVM_IRQ_LINE

 Capability: KVM_CAP_IRQCHIP

Documentation/virtual/kvm/api.txt +1 -1

-Architectures: x86, ia64
+Architectures: x86, ia64, arm

 Type: vm ioctl
 Parameters: struct kvm_irq_level
 Returns: 0 on success, -1 on error

Documentation/virtual/kvm/api.txt +0 -0

@@ -582,6 +582,14 @@ Requires that an interrupt controller model has been previously created with

 KVM_CREATE_IRQCHIP.  Note that edge-triggered interrupts require the level to be set to 1 and then back to 0.

Documentation/virtual/kvm/api.txt Requires that an interrupt controller model has been previously created with · line 586

Quoted 2 lines

Documentation/virtual/kvm/api.txt +2 -0

+KVM_CREATE_IRQCHIP (except for ARM).  Note that edge-triggered interrupts
+require the level to be set to 1 and then back to 0.

You probably wanted to replace the original lines with these two, no?

Quoted 66 lines

Documentation/virtual/kvm/api.txt +6 -0

+
+ARM uses two types of interrupt lines per CPU, ie. IRQ and FIQ. The value of the
+irq field should be (VCPU_INDEX * 2) for IRQs and ((VCPU_INDEX * 2) + 1) for
+FIQs. Level is used to raise/lower the line. See arch/arm/include/asm/kvm.h for
+convenience macros.
+

 struct kvm_irq_level {
      union {
              __u32 irq;     /* GSI */

arch/arm/include/asm/kvm.h +8 -0

index 87dc33b..8935062 100644
@@ -20,6 +20,14 @@
  #include <asm/types.h>
 
  /*
+ * KVM_IRQ_LINE macros to set/read IRQ/FIQ for specific VCPU index.
+ */
+enum KVM_ARM_IRQ_LINE_TYPE {
+     KVM_ARM_IRQ_LINE = 0,
+     KVM_ARM_FIQ_LINE = 1,
+};
+
+/*
   * Modes used for short-hand mode determinition in the world-switch code and
   * in emulation code.
   *

arch/arm/include/asm/kvm_arm.h +1 -0

index 835abd1..e378a37 100644
@@ -49,6 +49,7 @@
  #define HCR_VM               1
  #define HCR_GUEST_MASK (HCR_TSC | HCR_TWE | HCR_TWI | HCR_VM | HCR_AMO | \
                       HCR_AMO | HCR_IMO | HCR_FMO | HCR_SWIO)
+#define HCR_VIRT_EXCP_MASK (HCR_VA | HCR_VI | HCR_VF)
 
  /* Hyp System Control Register (HSCTLR) bits */
  #define HSCTLR_TE    (1 << 30)

arch/arm/kvm/arm.c +18 -0

index 89ba18d..fc0bd6b 100644
@@ -299,6 +299,43 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
       return -EINVAL;
  }
 
+static int kvm_arch_vm_ioctl_irq_line(struct kvm *kvm,
+                                   struct kvm_irq_level *irq_level)
+{
+     u32 mask;
+     unsigned int vcpu_idx;
+     struct kvm_vcpu *vcpu;
+
+     vcpu_idx = irq_level->irq / 2;
+     if (vcpu_idx >= KVM_MAX_VCPUS)
+             return -EINVAL;
+
+     vcpu = kvm_get_vcpu(kvm, vcpu_idx);
+     if (!vcpu)
+             return -EINVAL;
+
+     switch (irq_level->irq % 2) {
+     case KVM_ARM_IRQ_LINE:
+             mask = HCR_VI;

arch/arm/kvm/arm.c int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) · line 325

Quoted 6 lines

arch/arm/kvm/arm.c +6 -0

+             break;
+     case KVM_ARM_FIQ_LINE:
+             mask = HCR_VF;
+             break;
+     default:
+             return -EINVAL;

Due to % 2, default is unreachable. Remove the masking?

arch/arm/kvm/arm.c int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) · line 334

Quoted 9 lines

arch/arm/kvm/arm.c +9 -0

+     }
+
+     trace_kvm_irq_line(irq_level->irq % 2, irq_level->level, vcpu_idx);
+
+     if (irq_level->level) {
+             vcpu->arch.virt_irq |= mask;
+             vcpu->arch.wait_for_interrupts = 0;
+     } else
+             vcpu->arch.virt_irq &= ~mask;

No need to protect the bitops on virt_irq? Or what lock does this?

Quoted 62 lines

arch/arm/kvm/arm.c +18 -2

+
+     return 0;
+}
+
  long kvm_arch_vcpu_ioctl(struct file *filp,
                        unsigned int ioctl, unsigned long arg)
  {

@@ -313,8 +350,20 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm, struct kvm_dirty_log *log)
  long kvm_arch_vm_ioctl(struct file *filp,
                      unsigned int ioctl, unsigned long arg)
  {
-     printk(KERN_ERR "kvm_arch_vm_ioctl: Unsupported ioctl (%d)\n", ioctl);
-     return -EINVAL;
+     struct kvm *kvm = filp->private_data;
+     void __user *argp = (void __user *)arg;
+
+     switch (ioctl) {
+     case KVM_IRQ_LINE: {
+             struct kvm_irq_level irq_event;
+
+             if (copy_from_user(&irq_event, argp, sizeof irq_event))
+                     return -EFAULT;
+             return kvm_arch_vm_ioctl_irq_line(kvm, &irq_event);
+     }
+     default:
+             return -EINVAL;
+     }
  }
 
  /**

arch/arm/kvm/trace.h +21 -0

index f8869c1..ac64e3a 100644
@@ -40,6 +40,27 @@ TRACE_EVENT(kvm_exit,
  );
 
 
+TRACE_EVENT(kvm_irq_line,
+     TP_PROTO(unsigned int type, unsigned int level, unsigned int vcpu_idx),
+     TP_ARGS(type, level, vcpu_idx),
+
+     TP_STRUCT__entry(
+             __field(        unsigned int,   type                    )
+             __field(        unsigned int,   level                   )
+             __field(        unsigned int,   vcpu_idx                )
+     ),
+
+     TP_fast_assign(
+             __entry->type                   = type;
+             __entry->level                  = level;
+             __entry->vcpu_idx               = vcpu_idx;
+     ),
+
+     TP_printk("KVM_IRQ_LINE: type: %s, level: %u, vcpu: %u",
+             (__entry->type == KVM_ARM_IRQ_LINE) ? "IRQ" : "FIQ",
+             __entry->level, __entry->vcpu_idx)
+);
+
 
  #endif /* _TRACE_KVM_H */
 

include/linux/kvm.h struct kvm_irq_level { · line 117

Quoted 13 lines

include/linux/kvm.h +1 -0

index c3892fc..679abbb 100644
@@ -111,6 +111,7 @@ struct kvm_irq_level {
        * ACPI gsi notion of irq.
        * For IA-64 (APIC model) IOAPIC0: irq 0-23; IOAPIC1: irq 24-47..
        * For X86 (standard AT mode) PIC0/1: irq 0-15. IOAPIC0: 0-23..
+      * For ARM: IRQ: irq = (2*vcpu_index). FIQ: irq = (2*vcpu_indx + 1).
        */
       union {
               __u32 irq;

Quoted 10 lines · Christoffer Dall

The following series implements KVM support for ARM processors, specifically on the Cortex A-15 platform.

Still on the to-do list:
 - Reuse VMIDs
 - Fix SMP host support
 - Fix SMP guest support
 - Support guest Thumb mode for MMIO emulation
 - Further testing
 - Performance improvements

Other items for this list:
 - Support Neon/VFP in guests (the fpu regs struct is empty ATM)
 - Support guest debugging

I couldn't see any support for the TLS registers in your cp15 emulation:
did I miss it, or do we handle it without needing to trap?

by TLS you mean the cp15, c13 registers (tid and friends?) If so, I handle these in the world-switch code (look at read_cp15_state and write_cp15_state).

otherwise, help me out on the acronym...

Quoted 6 lines · Jan Kiszka

Just found two, maybe three nits while browsing by:

Documentation/virtual/kvm/api.txt Requires that an interrupt controller model has been previously created with

Quoted 3 lines · Christoffer Dall

Documentation/virtual/kvm/api.txt +3 -0

+ARM uses two types of interrupt lines per CPU, ie. IRQ and FIQ. The value of the
+irq field should be (VCPU_INDEX * 2) for IRQs and ((VCPU_INDEX * 2) + 1) for
+FIQs.

This seems to me a slightly obscure way of defining the two fields in this word (ie bits [31..1] cpu number, bit [0] irq-vs-fiq).

Quoted 18 lines

Quoted 18 lines

arch/arm/kvm/arm.c +18 -0

+static int kvm_arch_vm_ioctl_irq_line(struct kvm *kvm,
+                                   struct kvm_irq_level *irq_level)
+{
+     u32 mask;
+     unsigned int vcpu_idx;
+     struct kvm_vcpu *vcpu;
+
+     vcpu_idx = irq_level->irq / 2;
+     if (vcpu_idx >= KVM_MAX_VCPUS)
+             return -EINVAL;
+
+     vcpu = kvm_get_vcpu(kvm, vcpu_idx);
+     if (!vcpu)
+             return -EINVAL;
+
+     switch (irq_level->irq % 2) {
+     case KVM_ARM_IRQ_LINE:
+             mask = HCR_VI;

Quoted 8 lines

Quoted 6 lines

+             break;
+     case KVM_ARM_FIQ_LINE:
+             mask = HCR_VF;
+             break;
+     default:
+             return -EINVAL;

Due to % 2, default is unreachable. Remove the masking?

Removing the mask would be wrong since the irq field here is encoding both cpu number and irq-vs-fiq. The default is just an unreachable condition. (Why are we using % here rather than the obvious bit operation, incidentally?)

Quoted 5 lines · Peter Maydell

Removing the mask would be wrong since the irq field here is encoding both cpu number and irq-vs-fiq. The default is just an unreachable condition. (Why are we using % here rather than the obvious bit operation, incidentally?)

right, I will remove the default case.

I highly doubt that the difference in using a bitop will be measurably more efficient, but if you feel strongly about it, I can change it to a shift and bitwise and, which I assume is what you mean by the obvious bit operation? I think my CS background speaks for using %, but whatever.

Quoted 7 lines · Christoffer Dall

Quoted 5 lines · Peter Maydell

Removing the mask would be wrong since the irq field here is encoding both cpu number and irq-vs-fiq. The default is just an unreachable condition. (Why are we using % here rather than the obvious bit operation, incidentally?)

Quoted 7 lines

right, I will remove the default case.

I highly doubt that the difference in using a bitop will be measurably more efficient, but if you feel strongly about it, I can change it to a shift and bitwise and, which I assume is what you mean by the obvious bit operation? I think my CS background speaks for using %, but whatever.

Certainly the compiler ought to be able to figure out the two are the same thing; I just think "irq & 1" is more readable than "irq % 2" (because it's being clear that it's treating the variable as a pile of bits rather than an integer). This is bikeshedding rather, though, and style issues in kernel code are a matter for the kernel folk. So you can ignore me :-)

Quoted 1 line

cpu_idx) & 1" which I don't think is more clear.

Well, if it was just "irq & 1", then I hear you, but it would be "(irq

Quoted 1 line

Quoted 1 line

cpu_idx) & 1" which I don't think is more clear.

Quoted 2 lines · Christoffer Dall

Well, if it was just "irq & 1", then I hear you, but it would be "(irq

Quoted 1 line

Quoted 1 line

cpu_idx) & 1" which I don't think is more clear.

Er, what? The fields are [31..1] CPU index and [0] irqtype, right? So what you have now is:
    vcpu_idx = irq_level->irq / 2;
    irqtype = irq_level->irq % 2;
and the bitshifting equivalent is:
    vcpu_idx = irq_level->irq >> 1;
    irqtype = irq_level->irq & 1;
surely?

Shifting by the cpuindex is definitely wrong.

(Incidentally I fixed a bug in your QEMU-side code which wasn't feeding this field to the kernel in the way it expects:

http://git.linaro.org/gitweb?p=qemu/qemu-linaro.git;a=commitdiff;h=2502ba067e795e48d346f9816fad45177ca64bca

Sorry, I should have posted that to the list. I'll do that now.)

Quoted 14 lines · Peter Maydell

Quoted 2 lines · Christoffer Dall

Well, if it was just "irq & 1", then I hear you, but it would be "(irq

Quoted 1 line

Quoted 1 line

cpu_idx) & 1" which I don't think is more clear.

Er, what? The fields are [31..1] CPU index and [0] irqtype, right? So what you have now is:
    vcpu_idx = irq_level->irq / 2;
    irqtype = irq_level->irq % 2;
and the bitshifting equivalent is:
    vcpu_idx = irq_level->irq >> 1;
    irqtype = irq_level->irq & 1;
surely?

Quoted 1 line

Shifting by the cpuindex is definitely wrong.

actually, that's not how the irq_level field is defined.

If you look
in Documentation/virtual/kvm/api.txt:

"ARM uses two types of interrupt lines per CPU, ie. IRQ and FIQ. The value of the
irq field should be (VCPU_INDEX * 2) for IRQs and ((VCPU_INDEX * 2) + 1) for FIQs. Level is used to raise/lower the line. See arch/arm/include/asm/kvm.h for convenience macros."

we should definitely fix either side, and the only sane argument is that this is an irq_line field, so an index resembling an actual line seems more semantically in line with the field purpose rather than a bit encoding, but I am open to arguments and not married to the current implementation.

(Actually what would be clearest would be if the ioctl took the (interrupt-target, interrupt-line-for-that-target, value-of-line) tuple as three separate values rather than encoding two of them into a single integer, but I assume there's a reason we can't have that.)

Quoted 4 lines · Peter Maydell

(Actually what would be clearest would be if the ioctl took the (interrupt-target, interrupt-line-for-that-target, value-of-line) tuple as three separate values rather than encoding two of them into a single integer, but I assume there's a reason we can't have that.)

Have you thought about how this encoding scheme would be extended when we move to using the VGIC and an in-kernel interrupt controller implementation, incidentally? I haven't really looked into that at all, but I assume that then QEMU is going to start having to tell the kernel it wants to deliver interrupt 35 to the GIC, and so on...

Quoted 6 lines · Peter Maydell

Quoted 4 lines · Peter Maydell

(Actually what would be clearest would be if the ioctl took the (interrupt-target, interrupt-line-for-that-target, value-of-line) tuple as three separate values rather than encoding two of them into a single integer, but I assume there's a reason we can't have that.)

Quoted 7 lines

Have you thought about how this encoding scheme would be extended when we move to using the VGIC and an in-kernel interrupt controller implementation, incidentally? I haven't really looked into that at all, but I assume that then QEMU is going to start having to tell the kernel it wants to deliver interrupt 35 to the GIC, and so on...

no, I haven't looked into that at all. My plan was to decipher the common irq, ioapic stuff for x86 and see how much we can re-use and if there will be some nice way to either use what's there or change some bits to accomodate both existing archs and ARM. But the short answer is, no not really, I was focusing so far on getting a stable implementation upstream.

yes, we are going to have to have some interface with QEMU for this and if we need new features from what's already there that should probably be discussed in the same round as the mechanism for handing of CP15 stuff to QEMU that we touched upon earlier.

Quoted 7 lines · Christoffer Dall

Quoted 5 lines · Peter Maydell

Removing the mask would be wrong since the irq field here is encoding both cpu number and irq-vs-fiq. The default is just an unreachable condition. (Why are we using % here rather than the obvious bit operation, incidentally?)

Quoted 7 lines

right, I will remove the default case.

I highly doubt that the difference in using a bitop will be measurably more efficient, but if you feel strongly about it, I can change it to a shift and bitwise and, which I assume is what you mean by the obvious bit operation? I think my CS background speaks for using %, but whatever.

Certainly the compiler ought to be able to figure out the two are the same thing; I just think "irq & 1" is more readable than "irq % 2" (because it's being clear that it's treating the variable as a pile of bits rather than an integer). This is bikeshedding rather, though, and style issues in kernel code are a matter for the kernel folk. So you can ignore me :-)

Quoted 17 lines · Peter Maydell

Quoted 7 lines · Christoffer Dall

Quoted 5 lines · Peter Maydell

Removing the mask would be wrong since the irq field here is encoding both cpu number and irq-vs-fiq. The default is just an unreachable condition. (Why are we using % here rather than the obvious bit operation, incidentally?)

Quoted 7 lines

right, I will remove the default case.

I highly doubt that the difference in using a bitop will be measurably more efficient, but if you feel strongly about it, I can change it to a shift and bitwise and, which I assume is what you mean by the obvious bit operation? I think my CS background speaks for using %, but whatever.

Quoted 7 lines

Certainly the compiler ought to be able to figure out the two are the same thing; I just think "irq & 1" is more readable than "irq % 2" (because it's being clear that it's treating the variable as a pile of bits rather than an integer). This is bikeshedding rather, though, and style issues in kernel code are a matter for the kernel folk. So you can ignore me :-)

Well, if it was just "irq & 1", then I hear you, but it would be "(irq

Quoted 1 line

Quoted 1 line

cpu_idx) & 1" which I don't think is more clear.

But yes let's see what the kernel folks say.

Quoted 26 lines · Christoffer Dall

Quoted 17 lines · Peter Maydell

Quoted 7 lines · Christoffer Dall

Quoted 5 lines · Peter Maydell

Removing the mask would be wrong since the irq field here is encoding both cpu number and irq-vs-fiq. The default is just an unreachable condition. (Why are we using % here rather than the obvious bit operation, incidentally?)

Quoted 7 lines

right, I will remove the default case.

I highly doubt that the difference in using a bitop will be measurably more efficient, but if you feel strongly about it, I can change it to a shift and bitwise and, which I assume is what you mean by the obvious bit operation? I think my CS background speaks for using %, but whatever.

Quoted 7 lines

Certainly the compiler ought to be able to figure out the two are the same thing; I just think "irq & 1" is more readable than "irq % 2" (because it's being clear that it's treating the variable as a pile of bits rather than an integer). This is bikeshedding rather, though, and style issues in kernel code are a matter for the kernel folk. So you can ignore me :-)

Quoted 4 lines

Well, if it was just "irq & 1", then I hear you, but it would be "(irq

Quoted 1 line

Quoted 1 line

cpu_idx) & 1" which I don't think is more clear.

But yes let's see what the kernel folks say.

The general consensus is to use bit operations rather than arithmetic. The compiler will usually convert the "% 2" pattern into a shift, but I tend to agree with Peter on the readability of the thing. When encoding multiple information in a word, bit operations should be used, as they make it obvious which part of the word contains the bit you're interested in.

But I've probably been corrupted by working with HW guys for a bit too long... ;-)

Userspace can inject IRQs and FIQs through the KVM_IRQ_LINE VM ioctl. This ioctl is used since the sematics are in fact two lines that can be either raised or lowered on the VCPU - the IRQ and FIQ lines.

KVM needs to know which VCPU it must operate on and whether the FIQ or IRQ line is raised/lowered. Hence both pieces of information is packed in the kvm_irq_level->irq field. The irq fild value will be:
IRQ: vcpu_index * 2
FIQ: (vcpu_index * 2) + 1

This is documented in Documentation/kvm/api.txt.

The effect of the ioctl is simply to simply raise/lower the corresponding virt_irq field on the VCPU struct, which will cause the world-switch code to raise/lower virtual interrupts when running the guest on next switch. The wait_for_interrupt flag is also cleared for raised IRQs causing an idle VCPU to become active again.

Note: The custom trace_kvm_irq_line is used despite a generic definition of trace_kvm_set_irq, since the trace-Kvm_set_irq depends on the x86-specific define of __HAVE_IOAPIC. Either the trace event should be created regardless of this define or it should depend on another ifdef clause, common for both x86 and ARM. However, since the arguments don't really match those used in ARM, I am yet to be convinced why this is necessary.

arch/arm/kvm/arm.c +20 -0

+static int kvm_arch_vm_ioctl_irq_line(struct kvm *kvm,
+				      struct kvm_irq_level *irq_level)
+{
+	u32 mask;
+	unsigned int vcpu_idx;
+	struct kvm_vcpu *vcpu;
+
+	vcpu_idx = irq_level->irq / 2;
+	if (vcpu_idx >= KVM_MAX_VCPUS)
+		return -EINVAL;
+
+	vcpu = kvm_get_vcpu(kvm, vcpu_idx);
+	if (!vcpu)
+		return -EINVAL;
+
+	switch (irq_level->irq % 2) {
+	case KVM_ARM_IRQ_LINE:
+		mask = HCR_VI;
+		break;
+	case KVM_ARM_FIQ_LINE:

arch/arm/kvm/arm.c +6 -0

+	} else
+		vcpu->arch.virt_irq &= ~mask;
+
+	return 0;
+}
+

From: Christoffer Dall <[email protected]>

Adds a new important function in the main KVM/ARM code called handle_exit() which is called from kvm_arch_vcpu_ioctl_run() on returns from guest execution. This function examines the Hyp-Syndrome-Register (HSR), which contains information telling KVM what caused the exit from the guest.

Some of the reasons for an exit are CP15 accesses, which are not allowed from the guest and this commits handles these exits by

emulating the intented operation in software and skip the guest

instruction.

arch/arm/kvm/arm.c +42 -0

@@ -306,6 +307,62 @@ int kvm_arch_vcpu_runnable(struct kvm_vcpu *v)
 	return 0;
 }
 
+static inline int handle_exit(struct kvm_vcpu *vcpu, struct kvm_run *run,
+			      int exception_index)
+{
+	unsigned long hsr_ec;
+
+	if (exception_index == ARM_EXCEPTION_IRQ)
+		return 0;
+
+	if (exception_index != ARM_EXCEPTION_HVC) {
+		kvm_err(-EINVAL, "Unsupported exception type");
+		return -EINVAL;
+	}
+
+	hsr_ec = (vcpu->arch.hsr & HSR_EC) >> HSR_EC_SHIFT;
+	switch (hsr_ec) {
+	case HSR_EC_WFI:
+		return kvm_handle_wfi(vcpu, run);
+	case HSR_EC_CP15_32:
+	case HSR_EC_CP15_64:
+		return kvm_handle_cp15_access(vcpu, run);
+	case HSR_EC_CP14_MR:
+		return kvm_handle_cp14_access(vcpu, run);
+	case HSR_EC_CP14_LS:
+		return kvm_handle_cp14_load_store(vcpu, run);
+	case HSR_EC_CP14_64:
+		return kvm_handle_cp14_access(vcpu, run);
+	case HSR_EC_CP_0_13:
+		return kvm_handle_cp_0_13_access(vcpu, run);
+	case HSR_EC_CP10_ID:
+		return kvm_handle_cp10_id(vcpu, run);
+	case HSR_EC_SVC_HYP:
+		/* SVC called from Hyp mode should never get here */
+		kvm_msg("SVC called from Hyp mode shouldn't go here");
+		BUG();
+	case HSR_EC_HVC:
+		kvm_msg("hvc: %x (at %08x)", vcpu->arch.hsr & ((1 << 16) - 1),
+					     vcpu->arch.regs.pc);
+		kvm_msg("         HSR: %8x", vcpu->arch.hsr);
+		break;
+	case HSR_EC_IABT:
+	case HSR_EC_DABT:
+		return kvm_handle_guest_abort(vcpu, run);

arch/arm/kvm/arm.c +9 -0

+
+	return 0;
+}
+
 /**
  * kvm_arch_vcpu_ioctl_run - the main VCPU run function to execute guest code
  * @vcpu:	The VCPU pointer

@@ -333,6 +390,26 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
 		local_irq_enable();
 
 		trace_kvm_exit(vcpu->arch.regs.pc);
+
+		ret = handle_exit(vcpu, run, ret);
+		if (ret) {
+			kvm_err(ret, "Error in handle_exit");
+			break;

arch/arm/kvm/emulate.c +15 -0

+			break;
+		}
 	}
 
+
+/******************************************************************************
+ * Co-processor emulation
+ */
+
+struct coproc_params {
+	unsigned long CRm;
+	unsigned long CRn;
+	unsigned long Op1;
+	unsigned long Op2;
+	unsigned long Rt1;
+	unsigned long Rt2;
+	bool is_64bit;

From: Christoffer Dall <[email protected]>

When the guest accesses I/O memory this will create data abort exceptions and they are handled by decoding the HSR information (physical address, read/write, length, register) and forwarding reads and writes to QEMU which performs the device emulation.

Certain classes of load/store operations do not support the syndrome information provided in the HSR and we therefore must be able to fetch the offending instruction from guest memory and decode it manually.

This requires changing the general flow somewhat since new calls to run the VCPU must check if there's a pending MMIO load and perform the write after userspace has made the data available.

}

+/**
+ * kvm_handle_mmio_return -- Handle MMIO loads after user space emulation
+ * @vcpu: The VCPU pointer
+ * @run: The VCPU run struct containing the mmio data
+ *
+ * This should only be called after returning to QEMU for MMIO load emulation.

+ */

arch/arm/kvm/mmu.c +4 -0

+int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run)
+{
+	int *dest;
+	unsigned int len;

arch/arm/kvm/mmu.c +13 -0

+
+		trace_kvm_mmio(KVM_TRACE_MMIO_READ, len, run->mmio.phys_addr,
+				*((u64 *)run->mmio.data));
+
+		if (vcpu->arch.mmio_sign_extend && len < 4) {
+			mask = 1U << ((len * 8) - 1);
+			*dest = (*dest ^ mask) - mask;
+		}
+	}
+
+	return 0;
+}
+

From: Christoffer Dall <[email protected]>

When the guest executes a WFI instruction the operation is trapped to KVM, which emulates the instruction in software. There is no correlation between a guest executing a WFI instruction and actually puttin the

hardware into a low-power mode, since a KVM guest is essentially a process and the WFI instruction can be seen as 'sleep' call from this process. Therefore, we flag the VCPU to be in wait_for_interrupts mode and call the main KVM function kvm_vcpu_block() function. This function will put the thread on a wait-queue and call schedule.

When an interrupt comes in through KVM_IRQ_LINE (see previous patch) we signal the VCPU thread and unflag the VCPU to no longer wait for interrupts. All calls to kvm_arch_vcpu_ioctl_run() result in a call to kvm_vcpu_block() as long as the VCPU is in wfi-mode.

arch/arm/kvm/emulate.c +4 -0

+/**
+ * kvm_handle_wfi - handle a wait-for-interrupts instruction executed by a guest
+ * @vcpu:	the vcpu pointer
+ * @run:	the kvm_run structure pointer

In order to support KVM on a SMP host, it is necessary to initialize the hypervisor on all CPUs, mostly by making sure each CPU gets its own hypervisor stack and runs the HYP init code.

We also take care of some missing locking of modifications to the hypervisor page tables and ensure synchronized consistency between virtual IRQ masks and wait_for_interrupt flags on the VPUs.

Note that this code doesn't handle CPU hotplug yet. Note that this code doesn't support SMP guests.

WARNING: This code is in development and guests do not fully boot on SMP hosts yet.

arch/arm/kvm/arm.c +10 -0

@@ -464,13 +466,27 @@ static int kvm_arch_vm_ioctl_irq_line(struct kvm *kvm,
 
 	trace_kvm_irq_line(irq_level->irq % 2, irq_level->level, vcpu_idx);
 
+	spin_lock(&vcpu->arch.irq_lock);
 	if (irq_level->level) {
 		vcpu->arch.virt_irq |= mask;
+
+		/*
+		 * Note that we grab the wq.lock before clearing the wfi flag
+		 * since this ensures that a concurrent call to kvm_vcpu_block
+		 * will either sleep before we grab the lock, in which case we
+		 * wake it up, or will never sleep due to
+		 * kvm_arch_vcpu_runnable being true (iow. this avoids having
+		 * to grab the irq_lock in kvm_arch_vcpu_runnable).
+		 */

arch/arm/kvm/arm.c +12 -0

+
+static void cpu_init_hyp_mode(void *vector)
+{
+	unsigned long hyp_stack_ptr;
+	void *stack_page;
+
+	stack_page = __get_cpu_var(kvm_arm_hyp_stack_page);
+	hyp_stack_ptr = (unsigned long)stack_page + PAGE_SIZE;
+
+	cpu_set_vector(vector);
+
+	/*

arch/arm/kvm/arm.c +1 -1

@@ -522,47 +573,42 @@ static int init_hyp_mode(void)
 		return -ENOMEM;
 
 	/*
-	 * Allocate stack page for Hypervisor-mode
+	 * Allocate stack pages for Hypervisor-mode
 	 */

arch/arm/kvm/arm.c +8 -1

+		if (!stack_page) {
+			err = -ENOMEM;
+			goto out_free_pgd;
+		}
+
+		per_cpu(kvm_arm_hyp_stack_page, cpu) = stack_page;
+	}
 
 	init_phys_addr = virt_to_phys(__kvm_hyp_init);
 	init_end_phys_addr = virt_to_phys(__kvm_hyp_init_end);
+	BUG_ON(init_phys_addr & 0x1f);
 
 	/*
-	 * Create identity mapping

From: Marc Zyngier <[email protected]>

A guest may need to know which CPU it has booted on (and Linux does). Now that we can run KVM on a SMP host, QEMU may be running on any

CPU. In that case, directly reading MPIDR will give an inconsistent view on the guest CPU number (among other problems).

The solution is to use the VMPIDR register, which is computed by using the host MPIDR and overriding the low bits with KVM vcpu_id.

Signed-off-by Marc Zyngier <[email protected]>

Quoted 18 lines · Christoffer Dall

Userspace can inject IRQs and FIQs through the KVM_IRQ_LINE VM ioctl. This ioctl is used since the sematics are in fact two lines that can be either raised or lowered on the VCPU - the IRQ and FIQ lines.

KVM needs to know which VCPU it must operate on and whether the FIQ or IRQ line is raised/lowered. Hence both pieces of information is packed in the kvm_irq_level->irq field. The irq fild value will be:
  IRQ: vcpu_index * 2
  FIQ: (vcpu_index * 2) + 1

This is documented in Documentation/kvm/api.txt.

The effect of the ioctl is simply to simply raise/lower the corresponding virt_irq field on the VCPU struct, which will cause the world-switch code to raise/lower virtual interrupts when running the guest on next switch. The wait_for_interrupt flag is also cleared for raised IRQs causing an idle VCPU to become active again.

Quoted 6 lines

Note: The custom trace_kvm_irq_line is used despite a generic definition of trace_kvm_set_irq, since the trace-Kvm_set_irq depends on the x86-specific define of __HAVE_IOAPIC. Either the trace event should be created regardless of this define or it should depend on another ifdef clause, common for both x86 and ARM. However, since the arguments don't really match those used in ARM, I am yet to be convinced why this is necessary.

Why don't they match?  The assignment of lines to actual pins differs, but essentially it's the same thing (otherwise we'd use a different ioctl).

Quoted 11 lines

 Capability: KVM_CAP_IRQCHIP

Documentation/virtual/kvm/api.txt +1 -1

-Architectures: x86, ia64
+Architectures: x86, ia64, arm

 Type: vm ioctl
 Parameters: struct kvm_irq_level
 Returns: 0 on success, -1 on error

(quoted diff) +0 -0

@@ -582,6 +582,14 @@ Requires that an interrupt controller model has been previously created with

 KVM_CREATE_IRQCHIP.  Note that edge-triggered interrupts require the level to be set to 1 and then back to 0.

Documentation/virtual/kvm/api.txt Requires that an interrupt controller model has been previously created with · line 586

Quoted 2 lines

Documentation/virtual/kvm/api.txt +2 -0

+KVM_CREATE_IRQCHIP (except for ARM).  Note that edge-triggered interrupts
+require the level to be set to 1 and then back to 0.

Need to replace the previous line beginning with KVM_CREATE_IRQCHIP, not duplicate it.

Documentation/virtual/kvm/api.txt Requires that an interrupt controller model has been previously created with · line 591

Quoted 5 lines

Documentation/virtual/kvm/api.txt +5 -0

+
+ARM uses two types of interrupt lines per CPU, ie. IRQ and FIQ. The value of the
+irq field should be (VCPU_INDEX * 2) for IRQs and ((VCPU_INDEX * 2) + 1) for
+FIQs. Level is used to raise/lower the line. See arch/arm/include/asm/kvm.h for
+convenience macros.

Userspace only includes <linux/kvm.h>; also please name the macros here.

Quoted 26 lines

arch/arm/kvm/arm.c +25 -0

+static int kvm_arch_vm_ioctl_irq_line(struct kvm *kvm,
+                                   struct kvm_irq_level *irq_level)
+{
+     u32 mask;
+     unsigned int vcpu_idx;
+     struct kvm_vcpu *vcpu;
+
+     vcpu_idx = irq_level->irq / 2;
+     if (vcpu_idx >= KVM_MAX_VCPUS)
+             return -EINVAL;
+
+     vcpu = kvm_get_vcpu(kvm, vcpu_idx);
+     if (!vcpu)
+             return -EINVAL;
+
+     switch (irq_level->irq % 2) {
+     case KVM_ARM_IRQ_LINE:
+             mask = HCR_VI;
+             break;
+     case KVM_ARM_FIQ_LINE:
+             mask = HCR_VF;
+             break;
+     default:
+             return -EINVAL;
+     }

arch/arm/kvm/arm.c int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) · line 331

Quoted 5 lines

arch/arm/kvm/arm.c +5 -0

+
+     trace_kvm_irq_line(irq_level->irq % 2, irq_level->level, vcpu_idx);
+
+     if (irq_level->level) {
+             vcpu->arch.virt_irq |= mask;

racy - this is a vm ioctl, and so can (and usually is) invoked from outside the vcpu thread.

Quoted 1 line

+             vcpu->arch.wait_for_interrupts = 0;

Need an actual wakeup here (see x86's kvm_vcpu_kick() - should really be common code; it takes care of both the 'vcpu sleeping and needs a wakeup' and 'vcpu is in guest mode and needs to go to the host to evaluate interrupt state').

arch/arm/kvm/arm.c int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) · line 338

Quoted 7 lines

+     } else
+             vcpu->arch.virt_irq &= ~mask;

arch/arm/kvm/arm.c +4 -0

+
+     return 0;
+}
+

This commit introduces the framework for guest memory management through the use of 2nd stage translation. Each VM has a pointer to a level-1 tabled (the pgd field in struct kvm_arch) which is used for the 2nd stage translations. Entries are added when handling guest faults (later patch) and the table itself can be allocated and freed through the following functions implemented in arch/arm/kvm/arm_mmu.c:
- kvm_alloc_stage2_pgd(struct kvm *kvm);
- kvm_free_stage2_pgd(struct kvm *kvm);

Further, each entry in TLBs and caches are tagged with a VMID identifier in addition to ASIDs. The VMIDs are managed using a bitmap and assigned when creating the VM in kvm_arch_init_vm() where the 2nd stage pgd is also allocated. The table is freed in kvm_arch_destroy_vm(). Both functions are called from the main KVM code.

struct kvm_arch {

Quoted 4 lines

Why don't they match? The assignment of lines to actual pins differs, but essentially it's the same thing (otherwise we'd use a different ioctl).

because there is no notion of gsi and irq_source_id on ARM.

What's the
harm of this additional tracepoint?

If I should re-use the existing one, should I simply move it outside of __KVM_HAVE_IOAPIC?

Quoted 8 lines

arch/arm/kvm/arm.c int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) · line 331

Quoted 4 lines

arch/arm/kvm/arm.c +4 -0

+     trace_kvm_irq_line(irq_level->irq % 2, irq_level->level, vcpu_idx);
+
+     if (irq_level->level) {
+             vcpu->arch.virt_irq |= mask;

racy - this is a vm ioctl, and so can (and usually is) invoked from outside the vcpu thread.

this is taken care of in SMP host patch, but will be moved down the patches for next revision.

Quoted 8 lines

Quoted 2 lines

+ vcpu->arch.wait_for_interrupts = 0;

Need an actual wakeup here (see x86's kvm_vcpu_kick() - should really be common code; it takes care of both the 'vcpu sleeping and needs a wakeup' and 'vcpu is in guest mode and needs to go to the host to evaluate interrupt state').

the wakeup - same as above. Good point that we need to signal the other CPU. Will come, maybe not next revision, but the one after that.

Quoted 15 lines · Christoffer Dall

From: Christoffer Dall <[email protected]>

When the guest accesses I/O memory this will create data abort exceptions and they are handled by decoding the HSR information (physical address, read/write, length, register) and forwarding reads and writes to QEMU which performs the device emulation.

Certain classes of load/store operations do not support the syndrome information provided in the HSR and we therefore must be able to fetch the offending instruction from guest memory and decode it manually.

This requires changing the general flow somewhat since new calls to run the VCPU must check if there's a pending MMIO load and perform the write after userspace has made the data available.

Quoted 8 lines

 }

+/**
+ * kvm_handle_mmio_return -- Handle MMIO loads after user space emulation
+ * @vcpu: The VCPU pointer
+ * @run:  The VCPU run struct containing the mmio data
+ *
+ * This should only be called after returning to QEMU for MMIO load emulation.

s/to QEMU/from userspace/

Quoted 5 lines

+ */

arch/arm/kvm/mmu.c +4 -0

+int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run)
+{
+     int *dest;
+     unsigned int len;

arch/arm/kvm/mmu.c static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa, · line 306

Quoted 10 lines

arch/arm/kvm/mmu.c +10 -0

+     int mask;
+
+     if (!run->mmio.is_write) {
+             dest = vcpu_reg(vcpu, vcpu->arch.mmio_rd);
+             memset(dest, 0, sizeof(int));
+
+             if (run->mmio.len > 4) {
+                     kvm_err(-EINVAL, "Incorrect mmio length");
+                     return -EINVAL;
+             }

Time of check...

Quoted 3 lines

+
+             len = run->mmio.len;
+             memcpy(dest, run->mmio.data, len);

... time of use.  Anything in run-> is untrusted.  Best to use the kernel's copy of len.

From: Christoffer Dall <[email protected]>

Handles the guest faults in KVM by mapping in corresponding user pages in the 2nd stage page tables.

Introduces new ARM-specific kernel memory types, PAGE_KVM_GUEST and pgprot_guest variables used to map 2nd stage memory for KVM guests.

arch/arm/kvm/mmu.c +2 -0

+static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
+			  gfn_t gfn, struct kvm_memory_slot *memslot)

arch/arm/kvm/mmu.c +12 -0

+			return -ENOMEM;
+		}
+		pud_populate(NULL, pud, pmd);
+		pmd += pmd_index(fault_ipa);
+	} else
+		pmd = pmd_offset(pud, fault_ipa);
+
+	/* Create 2nd stage page table mapping - Level 2 */
+	if (pmd_none(*pmd)) {
+		pte = pte_alloc_one_kernel(NULL, fault_ipa);
+		if (!pte) {
+			kvm_err(-ENOMEM, "Cannot allocate 2nd stage pte");

Quoted 18 lines · Christoffer Dall

This commit introduces the framework for guest memory management through the use of 2nd stage translation. Each VM has a pointer to a level-1 tabled (the pgd field in struct kvm_arch) which is used for the 2nd stage translations. Entries are added when handling guest faults (later patch) and the table itself can be allocated and freed through the following functions implemented in arch/arm/kvm/arm_mmu.c:
 - kvm_alloc_stage2_pgd(struct kvm *kvm);
 - kvm_free_stage2_pgd(struct kvm *kvm);

Further, each entry in TLBs and caches are tagged with a VMID identifier in addition to ASIDs. The VMIDs are managed using a bitmap and assigned when creating the VM in kvm_arch_init_vm() where the 2nd stage pgd is also allocated. The table is freed in kvm_arch_destroy_vm(). Both functions are called from the main KVM code.

Quoted 7 lines

 struct kvm_arch {
-     pgd_t *pgd;     /* 1-level 2nd stage table */
+     u32    vmid;    /* The VMID used for the virt. memory system */
+     pgd_t *pgd;     /* 1-level 2nd stage table */
+     u64    vttbr;   /* VTTBR value associated with above pgd and vmid */ };

I can't say I have a solid grasp here, but my feeling is that vmid needs to be per-vcpu.  Otherwise vcpu 1 can migrate to a cpu that previously ran vcpu 0, and reuse its tlb since they have the same vmid.

Quoted 6 lines · Christoffer Dall

Quoted 4 lines

Why don't they match?  The assignment of lines to actual pins differs, but essentially it's the same thing (otherwise we'd use a different ioctl).

because there is no notion of gsi and irq_source_id on ARM.

gsi = number of irq line, just a bad name, but you do have it on ARM.

irq_source_id really shouldn't have been in kvm_set_irq(), it's an implementation detail rather than an architectural feature; just ignore it.

Quoted 2 lines

What's the
harm of this additional tracepoint?

If we get tools that use them, they have an additional difference to consider.  It's a weak argument but it's there.

Quoted 2 lines

If I should re-use the existing one, should I simply move it outside of __KVM_HAVE_IOAPIC?

Protect it with __KVM_HAVE_IRQ_LINE so we don't leak unused tracepoints for other archs.

Quoted 9 lines

Quoted 8 lines

Quoted 4 lines

+     trace_kvm_irq_line(irq_level->irq % 2, irq_level->level, vcpu_idx); +
+     if (irq_level->level) {
+             vcpu->arch.virt_irq |= mask;

racy - this is a vm ioctl, and so can (and usually is) invoked from outside the vcpu thread.

Quoted 2 lines

this is taken care of in SMP host patch, but will be moved down the patches for next revision.

Yes please.  It's hard to review this way.  Fold all the smp stuff into the patches which introduce the functionality.

Quoted 9 lines

Quoted 8 lines

Quoted 2 lines

+             vcpu->arch.wait_for_interrupts = 0;

Need an actual wakeup here (see x86's kvm_vcpu_kick() - should really be common code; it takes care of both the 'vcpu sleeping and needs a wakeup' and 'vcpu is in guest mode and needs to go to the host to evaluate interrupt state').

Quoted 2 lines

the wakeup - same as above. Good point that we need to signal the other CPU. Will come, maybe not next revision, but the one after that.

Ok.  I think you can reuse x86 concepts and even code.  I'll accept patches to make code arch independent ahead of the merge if it helps.

Quoted 27 lines · Avi Kivity

Quoted 20 lines · Christoffer Dall

This commit introduces the framework for guest memory management through the use of 2nd stage translation. Each VM has a pointer to a level-1 tabled (the pgd field in struct kvm_arch) which is used for the 2nd stage translations. Entries are added when handling guest faults (later patch) and the table itself can be allocated and freed through the following functions implemented in arch/arm/kvm/arm_mmu.c:
- kvm_alloc_stage2_pgd(struct kvm *kvm);
- kvm_free_stage2_pgd(struct kvm *kvm);

Further, each entry in TLBs and caches are tagged with a VMID identifier in addition to ASIDs. The VMIDs are managed using a bitmap and assigned when creating the VM in kvm_arch_init_vm() where the 2nd stage pgd is also allocated. The table is freed in kvm_arch_destroy_vm(). Both functions are called from the main KVM code.

struct kvm_arch {

arch/arm/include/asm/kvm_host.h struct kvm_vcpu; · line 37

Quoted 5 lines

arch/arm/include/asm/kvm_host.h +3 -1

-     pgd_t *pgd;     /* 1-level 2nd stage table */
+     u32    vmid;    /* The VMID used for the virt. memory system */
+     pgd_t *pgd;     /* 1-level 2nd stage table */
+     u64    vttbr;   /* VTTBR value associated with above pgd and vmid */
 };

Quoted 4 lines

I can't say I have a solid grasp here, but my feeling is that vmid needs to be per-vcpu. Otherwise vcpu 1 can migrate to a cpu that previously ran vcpu 0, and reuse its tlb since they have the same vmid.

According to the ARM guys, the same rules apply as for ASIDs which results into 1 vmid per VM. The vmid is checked along with a guest-specific asid, so they would only share TLB entries when running the same guest process, which I assume is fine then. Actually desirable. But, granted, my SMP knowledge is limited so far, so if Marc or Catalin feels like chipping in here...

Quoted 6 lines

Quoted 2 lines

If I should re-use the existing one, should I simply move it outside of __KVM_HAVE_IOAPIC?

Protect it with __KVM_HAVE_IRQ_LINE so we don't leak unused tracepoints for other archs.

ok. I used to be scared of touching your arch independent stuff, but maybe I should ease up there...

We need to differentiate in how Linux-as-a-guest acts and how the cpu is supposed to work.  A guest operating system can theoretically assign the ASID x to process A running on vcpu 0, and the same ASID x to process B running on vcpu 1

Quoted 4 lines · Avi Kivity

We need to differentiate in how Linux-as-a-guest acts and how the cpu is supposed to work. A guest operating system can theoretically assign the ASID x to process A running on vcpu 0, and the same ASID x to process B running on vcpu 1

That would be a guest bug. From the ARM ARM:
"For a symmetric multiprocessor cluster where a single operating system is running on the set of processing elements, ARMv7 requires all ASID values to be assigned uniquely within any single Inner Shareable domain. In other words, each ASID value must have the same meaning to all processing elements in the system."

Quoted 10 lines · Christoffer Dall

From: Christoffer Dall <[email protected]>

Adds a new important function in the main KVM/ARM code called handle_exit() which is called from kvm_arch_vcpu_ioctl_run() on returns from guest execution. This function examines the Hyp-Syndrome-Register (HSR), which contains information telling KVM what caused the exit from the guest.

Some of the reasons for an exit are CP15 accesses, which are not allowed from the guest and this commits handles these exits by

commit

Quoted 1 line

emulating the intented operation in software and skip the guest

intended

Quoted 52 lines

instruction.

arch/arm/kvm/arm.c +46 -0

@@ -306,6 +307,62 @@ int kvm_arch_vcpu_runnable(struct kvm_vcpu *v)
       return 0;
  }
 
+static inline int handle_exit(struct kvm_vcpu *vcpu, struct kvm_run *run,
+                           int exception_index)
+{
+     unsigned long hsr_ec;
+
+     if (exception_index == ARM_EXCEPTION_IRQ)
+             return 0;
+
+     if (exception_index != ARM_EXCEPTION_HVC) {
+             kvm_err(-EINVAL, "Unsupported exception type");
+             return -EINVAL;
+     }
+
+     hsr_ec = (vcpu->arch.hsr & HSR_EC) >> HSR_EC_SHIFT;
+     switch (hsr_ec) {
+     case HSR_EC_WFI:
+             return kvm_handle_wfi(vcpu, run);
+     case HSR_EC_CP15_32:
+     case HSR_EC_CP15_64:
+             return kvm_handle_cp15_access(vcpu, run);
+     case HSR_EC_CP14_MR:
+             return kvm_handle_cp14_access(vcpu, run);
+     case HSR_EC_CP14_LS:
+             return kvm_handle_cp14_load_store(vcpu, run);
+     case HSR_EC_CP14_64:
+             return kvm_handle_cp14_access(vcpu, run);
+     case HSR_EC_CP_0_13:
+             return kvm_handle_cp_0_13_access(vcpu, run);
+     case HSR_EC_CP10_ID:
+             return kvm_handle_cp10_id(vcpu, run);
+     case HSR_EC_SVC_HYP:
+             /* SVC called from Hyp mode should never get here */
+             kvm_msg("SVC called from Hyp mode shouldn't go here");
+             BUG();
+     case HSR_EC_HVC:
+             kvm_msg("hvc: %x (at %08x)", vcpu->arch.hsr & ((1 << 16) - 1),
+                                          vcpu->arch.regs.pc);
+             kvm_msg("         HSR: %8x", vcpu->arch.hsr);
+             break;
+     case HSR_EC_IABT:
+     case HSR_EC_DABT:
+             return kvm_handle_guest_abort(vcpu, run);
+     case HSR_EC_IABT_HYP:
+     case HSR_EC_DABT_HYP:
+             /* The hypervisor should never cause aborts */
+             kvm_msg("The hypervisor itself shouldn't cause aborts");

arch/arm/kvm/arm.c int kvm_arch_vcpu_runnable(struct kvm_vcpu *v) · line 361

Quoted 6 lines

arch/arm/kvm/arm.c +6 -0

+             BUG();
+     default:
+             kvm_msg("Unkown exception class: %08x (%08x)", hsr_ec,
+                             vcpu->arch.hsr);
+             BUG();
+     }

x86 uses a function table, which is slightly nicer.

Quoted 21 lines

arch/arm/kvm/arm.c +14 -0

+
+     return 0;
+}
+
  /**
   * kvm_arch_vcpu_ioctl_run - the main VCPU run function to execute guest code
   * @vcpu:    The VCPU pointer

@@ -333,6 +390,26 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
               local_irq_enable();
 
               trace_kvm_exit(vcpu->arch.regs.pc);
+
+             ret = handle_exit(vcpu, run, ret);
+             if (ret) {
+                     kvm_err(ret, "Error in handle_exit");
+                     break;
+             }
+
+             if (run->exit_reason == KVM_EXIT_MMIO)
+                     break;
+

arch/arm/kvm/arm.c int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) · line 407

Quoted 5 lines

arch/arm/kvm/arm.c +5 -0

+             if (need_resched()) {
+                     vcpu_put(vcpu);
+                     schedule();
+                     vcpu_load(vcpu);
+             }

I don't think you need the vcpu_put()/vcpu_load() here; you can replace the whole thing with cond_resched().

arch/arm/kvm/arm.c int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) · line 410

Quoted 3 lines

arch/arm/kvm/arm.c +3 -0

+
+             if (signal_pending(current) && !(run->exit_reason)) {
+                     run->exit_reason = KVM_EXIT_IRQ_WINDOW_OPEN;

This exit reason doesn't fit with KVM_IRQ_LINE.

With KVM_INTERRUPT, userspace is responsible for determining when to inject interrupts, so it needs to know when guest interrupts become enabled (this is KVM_EXIT_IRQ_WINDOWS_OPEN).  With KVM_IRQ_LINE, userspace just sets the line status, and the kernel takes care of everything.

Oh, and you need to exit to userspace unconditionally if a signal is pending.

Quoted 21 lines

arch/arm/kvm/emulate.c +19 -0

+                     break;
+             }
       }
 
+
+/******************************************************************************
+ * Co-processor emulation
+ */
+
+struct coproc_params {
+     unsigned long CRm;
+     unsigned long CRn;
+     unsigned long Op1;
+     unsigned long Op2;
+     unsigned long Rt1;
+     unsigned long Rt2;
+     bool is_64bit;
+     bool is_write;
+};
+
+#define CP15_OP(_vcpu, _params, _cp15_reg) \

arch/arm/kvm/emulate.c u32 *kvm_vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num, u32 mode) · line 151

Quoted 6 lines

arch/arm/kvm/emulate.c +6 -0

+do { \
+     if (_params->is_write) \
+             _vcpu->arch.cp15._cp15_reg = *vcpu_reg(_vcpu, _params->Rt1); \
+     else \
+             *vcpu_reg(_vcpu, _params->Rt1) = _vcpu->arch.cp15._cp15_reg; \
+} while (0);

Ugly.  How about an array of registers instead?

Quoted 5 lines · Christoffer Dall

From: Christoffer Dall <[email protected]>

When the guest executes a WFI instruction the operation is trapped to KVM, which emulates the instruction in software. There is no correlation between a guest executing a WFI instruction and actually puttin the

putting (puttin'? putin?)

Quoted 7 lines

hardware into a low-power mode, since a KVM guest is essentially a process and the WFI instruction can be seen as 'sleep' call from this process. Therefore, we flag the VCPU to be in wait_for_interrupts mode and call the main KVM function kvm_vcpu_block() function. This function will put the thread on a wait-queue and call schedule.

Quoted 4 lines

When an interrupt comes in through KVM_IRQ_LINE (see previous patch) we signal the VCPU thread and unflag the VCPU to no longer wait for interrupts. All calls to kvm_arch_vcpu_ioctl_run() result in a call to kvm_vcpu_block() as long as the VCPU is in wfi-mode.

Ah, this addresses my previous comment on this issue.

arch/arm/kvm/arm.c static int kvm_arch_vm_ioctl_irq_line(struct kvm *kvm, · line 473

Quoted 10 lines

      return ret;

arch/arm/kvm/arm.c +2 -0

@@ -454,6 +467,8 @@ static int kvm_arch_vm_ioctl_irq_line(struct kvm *kvm,
       if (irq_level->level) {
               vcpu->arch.virt_irq |= mask;
               vcpu->arch.wait_for_interrupts = 0;
 
 
+             if (waitqueue_active(&vcpu->wq))
+                     wake_up_interruptible(&vcpu->wq);

Not sufficient.  If the guest is running, you need to kick it out of guest mode and back into kvm, so that it samples the interrupt lines.

Also, racy:

racy:
  vcpu          host thread
                KVM_IRQ_LINE
  WFI
  if (!vcpu->arch.virt_irq) vcpu->arch.virt_irq = x vcpu->arch.wait_for_interrupts = 0 vcpu->arch.wait_for_interrupts = 1
                if (waitqueue_active()) (fails) schedule()

Quoted 10 lines

arch/arm/kvm/emulate.c +9 -0

+/**
+ * kvm_handle_wfi - handle a wait-for-interrupts instruction executed by a guest
+ * @vcpu:    the vcpu pointer
+ * @run:     the kvm_run structure pointer
+ *
+ * Simply sets the wait_for_interrupts flag on the vcpu structure, which will
+ * halt execution of world-switches and schedule other host processes until
+ * there is an incoming IRQ or FIQ to the VM.
+ */

Quoted 5 lines

 int kvm_handle_wfi(struct kvm_vcpu *vcpu, struct kvm_run *run) {
+     trace_kvm_wfi(vcpu->arch.regs.pc);
+     if (!vcpu->arch.virt_irq)
+             vcpu->arch.wait_for_interrupts = 1;