KVM/ARM Implementation

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 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 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...

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 6 lines · Peter Maydell

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

Quoted 6 lines

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."

Thanks.  So per-vm vmids should work.

Quoted 5 lines · Peter Maydell

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

Quoted 6 lines

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."

Thanks. So per-vm vmids should work.

Quoted 6 lines · Avi Kivity

Quoted 5 lines · Peter Maydell

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

Quoted 8 lines

Quoted 6 lines

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."

Thanks.  So per-vm vmids should work.

We where playing with a VMID recycling patch based on the assumption of per-cpu VMIDs being possible, which would have the advantage of recycling VMIDs without much complicated locking (inspired by the KVM SVM implementation). However we killed it with fire and hot plasma when it became clear it violated the ARM spec...

On the other hand, maybe we could do something with per vcpu VMIDs, but with proper synchronization accross physical CPUs in order to be compatible with the spec, but at the same time potentially allow a buggy guest to run? Since in practice a lot of CPUs will not share TLB (and instruction cache) structures, maybe it's possible that there is software out there that violates the spec, without having problems on the real hw.

Anyway VMID reuse will be available soon, and the difference between a per vm and per vcpu implementation is a couple of trivial lines of code.

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?

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.

Enabling in-kernel irqchips usually means "switching worlds". So the semantics of these particular IRQ inject interface details may change without breaking anything.

However, things might look different if there will be a need to inject also the CPU IRQs directly, not only the irqchip inputs. In that case, it may make some sense to reserve more space for interrupt types than just one bit and use a common encoding scheme.

Quoted 8 lines · Jan Kiszka

Enabling in-kernel irqchips usually means "switching worlds". So the semantics of these particular IRQ inject interface details may change without breaking anything.

However, things might look different if there will be a need to inject also the CPU IRQs directly, not only the irqchip inputs. In that case, it may make some sense to reserve more space for interrupt types than just one bit and use a common encoding scheme.

I think with an in-kernel GIC model you'd only need to be able to set one of the (256 including internal-to-the-CPU inputs) GIC input lines; the GIC itself then connects directly to the vcpu IRQ and FIQ.

So we could just have different semantics for the ioctl in the 'kernel GIC model enabled' config, as you suggest.

Quoted 14 lines · Peter Maydell

Quoted 8 lines · Jan Kiszka

Enabling in-kernel irqchips usually means "switching worlds". So the semantics of these particular IRQ inject interface details may change without breaking anything.

However, things might look different if there will be a need to inject also the CPU IRQs directly, not only the irqchip inputs. In that case, it may make some sense to reserve more space for interrupt types than just one bit and use a common encoding scheme.

I think with an in-kernel GIC model you'd only need to be able to set one of the (256 including internal-to-the-CPU inputs) GIC input lines; the GIC itself then connects directly to the vcpu IRQ and FIQ.

Quoted 2 lines

So we could just have different semantics for the ioctl in the 'kernel GIC model enabled' config, as you suggest.

btw, since we use the KVM_IRQ_LINE ioctl, it may make sense to require KVM_CREATE_IRQCHIP. To create a kernel GIC model, just call KVM_CREATE_IRQCHIP with a different parameter. This removes an "except for ARM" from the documentation.

Quoted 4 lines · Peter Maydell

I think with an in-kernel GIC model you'd only need to be able to set one of the (256 including internal-to-the-CPU inputs) GIC input lines; the GIC itself then connects directly to the vcpu IRQ and FIQ.

Quoted 2 lines

So we could just have different semantics for the ioctl in the 'kernel GIC model enabled' config, as you suggest.

btw, since we use the KVM_IRQ_LINE ioctl, it may make sense to require KVM_CREATE_IRQCHIP.  To create a kernel GIC model, just call KVM_CREATE_IRQCHIP with a different parameter.  This removes an "except for ARM" from the documentation.

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... ;-)

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 :-)

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;

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;
+}
+

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;
+}
+

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 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 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...

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;

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?

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 9 lines · Christoffer Dall

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 void kvm_free_stage2_pgd(struct kvm *kvm) · line 243

Quoted 12 lines

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

+static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
+                       gfn_t gfn, struct kvm_memory_slot *memslot)
+{
+     pfn_t pfn;
+     pgd_t *pgd;
+     pud_t *pud;
+     pmd_t *pmd;
+     pte_t *pte, new_pte;
+
+     pfn = gfn_to_pfn(vcpu->kvm, gfn);
+
+     if (is_error_pfn(pfn)) {

put_page()

Quoted 5 lines

+             kvm_err(-EFAULT, "Guest gfn %u (0x%08lx) does not have "
+                             "corresponding host mapping",
+                             gfn, gfn << PAGE_SHIFT);
+             return -EFAULT;
+     }

Quoted 8 lines

+
+     /* Create 2nd stage page table mapping - Level 1 */
+     pgd = vcpu->kvm->arch.pgd + pgd_index(fault_ipa);
+     pud = pud_offset(pgd, fault_ipa);
+     if (pud_none(*pud)) {
+             pmd = pmd_alloc_one(NULL, fault_ipa);
+             if (!pmd) {
+                     kvm_err(-ENOMEM, "Cannot allocate 2nd stage pmd");

put_page()

Quoted 12 lines

+                     return -ENOMEM;
+             }
+             pud_populate(NULL, pud, pmd);
+             pmd += pmd_index(fault_ipa);
+     } else
+             pmd = pmd_offset(pud, fault_ipa);

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

+
+     /* 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");

arch/arm/kvm/mmu.c void kvm_free_stage2_pgd(struct kvm *kvm) · line 278

Quoted 10 lines

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

+                     return -ENOMEM;
+             }
+             pmd_populate_kernel(NULL, pmd, pte);
+             pte += pte_index(fault_ipa);
+     } else
+             pte = pte_offset_kernel(pmd, fault_ipa);
+
+     /* Create 2nd stage page table mapping - Level 3 */
+     new_pte = pfn_pte(pfn, PAGE_KVM_GUEST);
+     set_pte_ext(pte, new_pte, 0);

With LPAE and 40-bit addresses, a guest can cause 2GBs worth of page tables to be pinned in host memory; this can be used as a denial of service attack.  x86 handles this by having a shrinker that can dynamically free page tables, see mmu_shrinker.

An alternative way may be to impose RLIMIT_AS on the sum of a guest's memory slots; though I prefer having a shrinker.

A bigger problem is that you pin all memory; what are the plans wrt mmu notifiers?

Quoted 4 lines

A bigger problem is that you pin all memory; what are the plans wrt mmu notifiers?

hmm, I have no plans (yet).

I haven't looked into neither MMU shrinker nor MMU notifier.

As I see it, the problems of consuming too much memory just for the page tables should be solved by somehow reclaiming pages used for the second stage mappings,

the question is just which mappings are the
most efficient to reclaim.

The other problem, the actual guest memory consuming too much memory, I assumed this limit would be set by the user when creating his/her VM, or can we do something smarter? (again, forgive my ignorance). What is the alternative to pinning actual guest pages

- as far as I
know it's not common to have swap space on ARM architectures, but I could be wrong.

Quoted 6 lines · Christoffer Dall

Quoted 4 lines

A bigger problem is that you pin all memory; what are the plans wrt mmu notifiers?

hmm, I have no plans (yet).

Quoted 5 lines

I haven't looked into neither MMU shrinker nor MMU notifier.

As I see it, the problems of consuming too much memory just for the page tables should be solved by somehow reclaiming pages used for the second stage mappings,

That's what the shrinker does.

Quoted 2 lines

the question is just which mappings are the
most efficient to reclaim.

Do you have accessed bits in those PTEs?

It's not really critical to have efficient reclaim here, since it happens so rarely.  It just needs to do something.

Quoted 4 lines

The other problem, the actual guest memory consuming too much memory, I assumed this limit would be set by the user when creating his/her VM, or can we do something smarter? (again, forgive my ignorance). What is the alternative to pinning actual guest pages

mmu notifiers - pages aren't pinned; instead, Linux calls back into kvm when modifying a host pte, and kvm responds by dropping or modifying its translation (second stage pte in your case).

Quoted 3 lines

 - as far as I
know it's not common to have swap space on ARM architectures, but I could be wrong.

It will become common once you start doing servers.

mmu notifiers are also useful for other optimizations, like ksm, ballooning, and transparent huge pages.

Quoted 5 lines · Avi Kivity

Quoted 3 lines · Christoffer Dall

- as far as I
know it's not common to have swap space on ARM architectures, but I could be wrong.

Quoted 2 lines

It will become common once you start doing servers.

I think so too, but I am not sure if it's completely supported for ARM. Is it all arch-independent or do we miss arm-specific pieces? Marc?

Quoted 5 lines

Quoted 2 lines

the question is just which mappings are the
most efficient to reclaim.

Do you have accessed bits in those PTEs?

nope. We can protect the underlying target pages though, but...

Quoted 3 lines

It's not really critical to have efficient reclaim here, since it happens so rarely. It just needs to do something.

when would you trigger it - when it reaches a certain limit, or? And then what, free the lot and re-allocate what's needed?

Quoted 9 lines

Quoted 4 lines

The other problem, the actual guest memory consuming too much memory, I assumed this limit would be set by the user when creating his/her VM, or can we do something smarter? (again, forgive my ignorance). What is the alternative to pinning actual guest pages

mmu notifiers - pages aren't pinned; instead, Linux calls back into kvm when modifying a host pte, and kvm responds by dropping or modifying its translation (second stage pte in your case).

ah ok, so this works across VM boundary. Based on hyper-calls I presume?

Quoted 3 lines

mmu notifiers are also useful for other optimizations, like ksm, ballooning, and transparent huge pages.

I know those features have to be supported eventually. The question is if all this must be in place before a merge upstream?

Quoted 3 lines · Christoffer Dall

Quoted 3 lines

Quoted 2 lines

the question is just which mappings are the
most efficient to reclaim.

Quoted 4 lines

Quoted 2 lines

Do you have accessed bits in those PTEs?

nope. We can protect the underlying target pages though, but...

Yeah, we have the same issue with one of the vendors.  Fortunately only 90% of the market is affected.

Quoted 4 lines

Quoted 3 lines

It's not really critical to have efficient reclaim here, since it happens so rarely.  It just needs to do something.

Quoted 2 lines

when would you trigger it - when it reaches a certain limit, or? And then what, free the lot and re-allocate what's needed?

The kernel triggers it based on internal pressure.  It tells you how much pressure to apply, so you just translate it to a number of pages to free.

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

The other problem, the actual guest memory consuming too much memory, I assumed this limit would be set by the user when creating his/her VM, or can we do something smarter? (again, forgive my ignorance). What is the alternative to pinning actual guest pages

Quoted 6 lines

Quoted 4 lines

mmu notifiers - pages aren't pinned; instead, Linux calls back into kvm when modifying a host pte, and kvm responds by dropping or modifying its translation (second stage pte in your case).

ah ok, so this works across VM boundary. Based on hyper-calls I presume?

No, it's completely internal to the host.

See for example kvm_mmu_notifier_invalidate_page() (in common code). It's called when Linux-as-host wants to change a pte (say to swap a page).  kvm responds by translating the host virtual address into a guest physical address (via the memory slots table), then zapping the relevant pte and flushing and TLBs which may have cached the pte.

Quoted 4 lines

Quoted 3 lines

mmu notifiers are also useful for other optimizations, like ksm, ballooning, and transparent huge pages.

Quoted 2 lines

I know those features have to be supported eventually. The question is if all this must be in place before a merge upstream?

It doesn't have to be there for the merge but I recommend giving it high priority.  At least read and understand the code so the addition will follow naturally.

Quoted 7 lines

Quoted 6 lines

Quoted 3 lines

It's not really critical to have efficient reclaim here, since it happens so rarely. It just needs to do something.

when would you trigger it - when it reaches a certain limit, or? And then what, free the lot and re-allocate what's needed?

Quoted 5 lines

The kernel triggers it based on internal pressure. It tells you how much pressure to apply, so you just translate it to a number of pages to free.

ok, so we pick those pages at random? (perhaps trying to avoid hitting the guest kernel at least for Linux, or...?)

Quoted 4 lines

It doesn't have to be there for the merge but I recommend giving it high priority. At least read and understand the code so the addition will follow naturally.

will do - I will make it a Christmas activity.

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;
+}
+

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]>

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

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;

Why not just block here?

Quoted 16 lines

Quoted 15 lines

arch/arm/kvm/emulate.c +12 -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.
+ */
 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;

Quoted 2 lines

Why not just block here?

well, if we block, but receive a signal that we want to go back into userspace for, and then come back but the guest should still be waiting, then I want that flag set, and I think it's the most logical control flow. Am I missing something completely?

Quoted 19 lines · Christoffer Dall

Quoted 18 lines

Quoted 15 lines

+/**
+ * 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.
+ */
 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;

Why not just block here?

Quoted 4 lines

well, if we block, but receive a signal that we want to go back into userspace for, and then come back but the guest should still be waiting, then I want that flag set, and I think it's the most logical control flow. Am I missing something completely?

That's just not the flow that the other archs use, I don't think that it really matters.  kvm_vcpu_block() checks for and wakes up on signals, so it's okay to call it from here directly.

Quoted 6 lines

Quoted 5 lines

well, if we block, but receive a signal that we want to go back into userspace for, and then come back but the guest should still be waiting, then I want that flag set, and I think it's the most logical control flow. Am I missing something completely?

Quoted 4 lines

That's just not the flow that the other archs use, I don't think that it really matters. kvm_vcpu_block() checks for and wakes up on signals, so it's okay to call it from here directly.

yes, but I still have to perform the check again in the main run function if it woke up for anything else than a guest interrupt, so why call it twice...