| Message ID | 20120703090115.27746.55688.stgit@ubuntu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 3 July 2012 10:01, Christoffer Dall <c.dall@virtualopensystems.com> wrote: > From: Christoffer Dall <cdall@cs.columbia.edu> > > 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 << 1 > FIQ: (vcpu_index << 1) | 1 > > This is documented in Documentation/kvm/api.txt. It occurred to me that rather than encoding the CPU index in the IRQ field value, maybe we should just use the per-vcpu version of KVM_IRQ_LINE the same way we do for injecting the per-CPU lines of the in-kernel (V)GIC ? (The subtext here is that it would be cool to have QEMU's generic interrupt handling code for KVM be able to say "if you do a cpu_interrupt()/cpu_reset_interrupt() and async interrupt delivery is enabled then just do the per-vcpu ioctl". Then there wouldn't need to be any kvm-specific code in hw/arm_pic.c at all...) -- PMM -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/06/2012 08:20 PM, Peter Maydell wrote: > On 3 July 2012 10:01, Christoffer Dall <c.dall@virtualopensystems.com> wrote: >> From: Christoffer Dall <cdall@cs.columbia.edu> >> >> 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 << 1 >> FIQ: (vcpu_index << 1) | 1 >> >> This is documented in Documentation/kvm/api.txt. > > It occurred to me that rather than encoding the CPU index in the IRQ > field value, maybe we should just use the per-vcpu version of > KVM_IRQ_LINE the same way we do for injecting the per-CPU lines > of the in-kernel (V)GIC ? What do you mean by "per-vcpu version of KVM_IRQ_LINE"? > > (The subtext here is that it would be cool to have QEMU's > generic interrupt handling code for KVM be able to say "if > you do a cpu_interrupt()/cpu_reset_interrupt() and async > interrupt delivery is enabled then just do the per-vcpu ioctl". > Then there wouldn't need to be any kvm-specific code in > hw/arm_pic.c at all...) If you mean "vcpu ioctl", then no, vcpu ioctls must be called from the vcpu thread.
On 7 August 2012 14:59, Avi Kivity <avi@redhat.com> wrote: > On 08/06/2012 08:20 PM, Peter Maydell wrote: >> On 3 July 2012 10:01, Christoffer Dall <c.dall@virtualopensystems.com> wrote: >>> From: Christoffer Dall <cdall@cs.columbia.edu> >>> >>> 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 << 1 >>> FIQ: (vcpu_index << 1) | 1 >>> >>> This is documented in Documentation/kvm/api.txt. >> >> It occurred to me that rather than encoding the CPU index in the IRQ >> field value, maybe we should just use the per-vcpu version of >> KVM_IRQ_LINE the same way we do for injecting the per-CPU lines >> of the in-kernel (V)GIC ? > > What do you mean by "per-vcpu version of KVM_IRQ_LINE"? The ARM VGIC implementation implements "I need to raise per-CPU interrupt X" by providing a vcpu ioctl KVM_IRQ_LINE (this is in addition to the vm ioctl KVM_IRQ_LINE which it uses for "I need to raise the external interrupt X"). The patch updating the API documentation is this one: https://lists.cs.columbia.edu/pipermail/kvmarm/2012-July/001206.html >> (The subtext here is that it would be cool to have QEMU's >> generic interrupt handling code for KVM be able to say "if >> you do a cpu_interrupt()/cpu_reset_interrupt() and async >> interrupt delivery is enabled then just do the per-vcpu ioctl". >> Then there wouldn't need to be any kvm-specific code in >> hw/arm_pic.c at all...) > > If you mean "vcpu ioctl", then no, vcpu ioctls must be called from the > vcpu thread. That's a shame, because it's the obvious interface for "do something to this specific CPU". -- PMM -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/07/2012 05:12 PM, Peter Maydell wrote: > On 7 August 2012 14:59, Avi Kivity <avi@redhat.com> wrote: >> On 08/06/2012 08:20 PM, Peter Maydell wrote: >>> On 3 July 2012 10:01, Christoffer Dall <c.dall@virtualopensystems.com> wrote: >>>> From: Christoffer Dall <cdall@cs.columbia.edu> >>>> >>>> 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 << 1 >>>> FIQ: (vcpu_index << 1) | 1 >>>> >>>> This is documented in Documentation/kvm/api.txt. >>> >>> It occurred to me that rather than encoding the CPU index in the IRQ >>> field value, maybe we should just use the per-vcpu version of >>> KVM_IRQ_LINE the same way we do for injecting the per-CPU lines >>> of the in-kernel (V)GIC ? >> >> What do you mean by "per-vcpu version of KVM_IRQ_LINE"? > > The ARM VGIC implementation implements "I need to raise per-CPU > interrupt X" by providing a vcpu ioctl KVM_IRQ_LINE (this is > in addition to the vm ioctl KVM_IRQ_LINE which it uses for > "I need to raise the external interrupt X"). > The patch updating the API documentation is this one: > https://lists.cs.columbia.edu/pipermail/kvmarm/2012-July/001206.html Yikes. "vm ioctl (and vcpu_ioctl on ARM)" is just horrible. First, use a different ioctl. Second, vcpu ioctls are synchronous, you must call them from a vcpu thread (think syscall operating on current). You want something asynchronous. How about using the vm ioctl, with a range of numbers allocated for per-processor interrupts? In fact the documentation appears to say you already did this. > >>> (The subtext here is that it would be cool to have QEMU's >>> generic interrupt handling code for KVM be able to say "if >>> you do a cpu_interrupt()/cpu_reset_interrupt() and async >>> interrupt delivery is enabled then just do the per-vcpu ioctl". >>> Then there wouldn't need to be any kvm-specific code in >>> hw/arm_pic.c at all...) >> >> If you mean "vcpu ioctl", then no, vcpu ioctls must be called from the >> vcpu thread. > > That's a shame, because it's the obvious interface for "do something > to this specific CPU". We can do something new if needed. So far all vcpu operations were synchronous (mostly, get/set state).
On 7 August 2012 15:28, Avi Kivity <avi@redhat.com> wrote: > On 08/07/2012 05:12 PM, Peter Maydell wrote: >> On 7 August 2012 14:59, Avi Kivity <avi@redhat.com> wrote: >>> What do you mean by "per-vcpu version of KVM_IRQ_LINE"? >> >> The ARM VGIC implementation implements "I need to raise per-CPU >> interrupt X" by providing a vcpu ioctl KVM_IRQ_LINE (this is >> in addition to the vm ioctl KVM_IRQ_LINE which it uses for >> "I need to raise the external interrupt X"). >> The patch updating the API documentation is this one: >> https://lists.cs.columbia.edu/pipermail/kvmarm/2012-July/001206.html > > Yikes. "vm ioctl (and vcpu_ioctl on ARM)" is just horrible. First, use > a different ioctl. Second, vcpu ioctls are synchronous, you must call > them from a vcpu thread (think syscall operating on current). You want > something asynchronous. > > How about using the vm ioctl, with a range of numbers allocated for > per-processor interrupts? In fact the documentation appears to say you > already did this. That's how the non-VGIC interrupt delivery works. Where this thread started was me saying "that looks pretty ugly because you have to encode the CPU number in the interrupt number somehow, why not just use a vcpu ioctl instead"... Whatever we do I do think (a) we should be consistent between VGIC and non-VGIC (b) ideally it shouldn't be an ARM-specific way of doing things. -- PMM -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt index 310fe50..79c10fc 100644 --- a/Documentation/virtual/kvm/api.txt +++ b/Documentation/virtual/kvm/api.txt @@ -614,15 +614,19 @@ 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 Sets the level of a GSI input to the interrupt controller model in the kernel. -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. +On some architectures it is required 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. + +ARM uses two types of interrupt lines per CPU: IRQ and FIQ. The value of the +irq field should be (vcpu_index << 1) for IRQs and ((vcpu_index << 1) | 1) for +FIQs. Level is used to raise/lower the line. struct kvm_irq_level { union { diff --git a/arch/arm/include/asm/kvm.h b/arch/arm/include/asm/kvm.h index 1d0d8f1..54f301d 100644 --- a/arch/arm/include/asm/kvm.h +++ b/arch/arm/include/asm/kvm.h @@ -22,6 +22,15 @@ #include <asm/types.h> #define __KVM_HAVE_GUEST_DEBUG +#define __KVM_HAVE_IRQ_LINE + +/* + * 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 diff --git a/arch/arm/include/asm/kvm_arm.h b/arch/arm/include/asm/kvm_arm.h index 56f5c85..220f241 100644 --- a/arch/arm/include/asm/kvm_arm.h +++ b/arch/arm/include/asm/kvm_arm.h @@ -48,8 +48,10 @@ #define HCR_BSU_IS (1 << 10) #define HCR_FB (1 << 9) #define HCR_VA (1 << 8) -#define HCR_VI (1 << 7) -#define HCR_VF (1 << 6) +#define HCR_VI_BIT_NR 7 +#define HCR_VF_BIT_NR 6 +#define HCR_VI (1 << HCR_VI_BIT_NR) +#define HCR_VF (1 << HCR_VF_BIT_NR) #define HCR_AMO (1 << 5) #define HCR_IMO (1 << 4) #define HCR_FMO (1 << 3) @@ -73,6 +75,7 @@ #define HCR_GUEST_MASK (HCR_TSC | HCR_TSW | HCR_TWI | HCR_VM | HCR_BSU_IS | \ HCR_FB | HCR_TAC | 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) diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c index ce3d258..8b024ee 100644 --- a/arch/arm/kvm/arm.c +++ b/arch/arm/kvm/arm.c @@ -24,6 +24,7 @@ #include <linux/fs.h> #include <linux/mman.h> #include <linux/sched.h> +#include <linux/kvm.h> #include <trace/events/kvm.h> #define CREATE_TRACE_POINTS @@ -256,6 +257,7 @@ void kvm_arch_vcpu_uninit(struct kvm_vcpu *vcpu) void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu) { + vcpu->cpu = cpu; } void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu) @@ -296,6 +298,51 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) return -EINVAL; } +int kvm_vm_ioctl_irq_line(struct kvm *kvm, struct kvm_irq_level *irq_level) +{ + unsigned int vcpu_idx; + struct kvm_vcpu *vcpu; + unsigned long *ptr; + bool set; + int bit_nr; + + vcpu_idx = irq_level->irq >> 1; + if (vcpu_idx >= KVM_MAX_VCPUS) + return -EINVAL; + + vcpu = kvm_get_vcpu(kvm, vcpu_idx); + if (!vcpu) + return -EINVAL; + + trace_kvm_set_irq(irq_level->irq, irq_level->level, 0); + + if ((irq_level->irq & 1) == KVM_ARM_IRQ_LINE) + bit_nr = HCR_VI_BIT_NR; + else /* KVM_ARM_FIQ_LINE */ + bit_nr = HCR_VF_BIT_NR; + + ptr = (unsigned long *)&vcpu->arch.irq_lines; + if (irq_level->level) + set = test_and_set_bit(bit_nr, ptr); + else + set = test_and_clear_bit(bit_nr, ptr); + + /* + * If we didn't change anything, no need to wake up or kick other CPUs + */ + if (!!set == !!irq_level->level) + return 0; + + /* + * The vcpu irq_lines field was updated, wake up sleeping VCPUs and + * trigger a world-switch round on the running physical CPU to set the + * virtual IRQ/FIQ fields in the HCR appropriately. + */ + kvm_vcpu_kick(vcpu); + + return 0; +} + long kvm_arch_vcpu_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg) { diff --git a/include/linux/kvm.h b/include/linux/kvm.h index 2ce09aa..f978447 100644 --- a/include/linux/kvm.h +++ b/include/linux/kvm.h @@ -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;