KVM: Add KVM_CAP_IRQ_ARCH capability
Subsystems (3)
PATCH 1/1 KVM: Add KVM_CAP_IRQ_ARCH capability
Setting this capability on a vcpu connects that vcpu to an interrupt controller device. The args[0] field of the argument kvm_enable_cap struct specifies the overall architecture of the interrupt controller. The args[1] field specifies the CPU number for the vcpu from the interrupt controller's point of view.
--- arch/powerpc/include/asm/kvm_host.h | 3 +++ arch/powerpc/kvm/powerpc.c | 29 +++++++++++++++++++++++++++++ include/uapi/linux/kvm.h | 1 + 3 files changed, 33 insertions(+)
arch/powerpc/include/asm/kvm_host.h +3 -0
@@ -373,6 +373,9 @@ struct kvmppc_booke_debug_reg {struct kvm_vcpu_arch {ulong host_stack;u32 host_pid; + +u32 intr_ctrler; +#ifdef CONFIG_PPC_BOOK3S struct kvmppc_slb slb[64 ];int slb_max;/* 1 + index of last valid entry in slb[] */
arch/powerpc/kvm/powerpc.c +29 -0
@@ -459,6 +459,12 @@ void kvm_arch_vcpu_free(struct kvm_vcpu *vcpu)hrtimer_cancel (&vcpu->arch.dec_timer);tasklet_kill (&vcpu->arch.tasklet); +/* Release any per-vcpu irq controller state */ +switch (vcpu->arch.intr_ctrler) { +default : +break ; + } +kvmppc_remove_vcpu_debugfs (vcpu);kvmppc_core_vcpu_free (vcpu); }
@@ -791,6 +797,29 @@ static int kvm_vcpu_ioctl_enable_cap(struct kvm_vcpu *vcpu, break; }#endif +case KVM_CAP_IRQ_ARCH : + r = -EBUSY ; +mutex_lock (&vcpu->kvm->lock); +/* disallow changing once set */ +if (!vcpu->arch.intr_ctrler) { + r =0 ; +switch (cap->args[0 ]) { +case 0 :/* no interrupt controller */ +break ; +default : + r = -EINVAL ; + } +if (!r) { +/* +* Make sure any state set up by the interrupt +* controller init routine is seen before this. +*/ +smp_wmb (); + vcpu->arch.intr_ctrler = cap->args[0 ]; + } + } +mutex_unlock (&vcpu->kvm->lock); +break ;default : r = -EINVAL ;break ;
include/uapi/linux/kvm.h +1 -0
@@ -663,6 +663,7 @@ struct kvm_ppc_smmu_info {#define KVM_CAP_S390_CSS_SUPPORT 85#define KVM_CAP_PPC_EPR 86#define KVM_CAP_DEVICE_CTRL 87 +#define KVM_CAP_IRQ_ARCH 88#ifdef KVM_CAP_IRQ_ROUTING
Signature · 1 line
1.7.10.4
Quoted 19 lines · Paul Mackerras
Setting this capability on a vcpu connects that vcpu to an interrupt controller device. The args[0] field of the argument
kvm_enable_capstruct specifies the overall architecture of the interrupt controller. The args[1] field specifies the CPU number for the vcpu from the interrupt controller's point of view.Signed-off-by Paul Mackerras <[email protected]>--- arch/powerpc/include/asm/kvm_host.h | 3 +++ arch/powerpc/kvm/powerpc.c | 29 +++++++++++++++++++++++++++++ include/uapi/linux/kvm.h | 1 + 3 files changed, 33 insertions(+)arch/powerpc/include/asm/kvm_host.h +0
-0
arch/powerpc/include/asm/kvm_host.h struct kvmppc_booke_debug_reg { · line 378
Quoted 6 lines
arch/powerpc/include/asm/kvm_host.h +3
-0@@ -373,6 +373,9 @@ struct kvmppc_booke_debug_reg {struct kvm_vcpu_arch {ulong host_stack;u32 host_pid; + +u32 intr_ctrler; +
That abbreviation seems a bit awkward, and we should also have a private-data pointer.
How about:
u32 irq_arch; void *irq_priv;
arch/powerpc/include/asm/kvm_host.h struct kvmppc_booke_debug_reg {
Quoted 10 lines
arch/powerpc/include/asm/kvm_host.h +9
-0#endif +case KVM_CAP_IRQ_ARCH : + r = -EBUSY ; +mutex_lock (&vcpu->kvm->lock); +/* disallow changing once set */ +if (!vcpu->arch.intr_ctrler) { + r =0 ; +switch (cap->args[0 ]) { +case 0 :/* no interrupt controller */ +break ;
s/0/KVM_IRQ_ARCH_NONE/
...at least so that this patch makes it clear where other type ids should be defined.
arch/powerpc/include/asm/kvm_host.h
Quoted 13 lines
arch/powerpc/include/asm/kvm_host.h +6
-0+default : + r = -EINVAL ; + } +if (!r) { +/* +* Make sure any state set up by the interrupt
arch/powerpc/include/asm/kvm_host.h +1
-0+* controller init routine is seen before this.
arch/powerpc/include/asm/kvm_host.h +4
-0+*/ +smp_wmb (); + vcpu->arch.intr_ctrler = cap->args[0 ]; + }
Do we really need that wmb()? We're in vcpu context, right? If the vcpu migrates to another host cpu, that involves rescheduling which already has a sync. If the interrupt controller code we call here modifies data that will be seen from outside the vcpu, it's the responsibility of that code to use whatever locks, barriers, etc. are needed (and it's unlikely that vcpu->arch.intr_ctrler will be the relevant thing that it needs to order with).
This patch should also add a hook at vcpu destruction to call into the irq code.
-Scott
Quoted 27 lines · Scott Wood
Quoted 16 lines · Paul Mackerras
Setting this capability on a vcpu connects that vcpu to an interrupt controller device. The args[0] field of the argument
kvm_enable_capstruct specifies the overall architecture of the interrupt controller. The args[1] field specifies the CPU number for the vcpu from the interrupt controller's point of view.Signed-off-by Paul Mackerras <[email protected]>---
arch/powerpc/include/asm/kvm_host.h | 3 +++ arch/powerpc/kvm/powerpc.c | 29 +++++++++++++++++++++++++++++ include/uapi/linux/kvm.h | 1 + 3 files changed, 33 insertions(+)arch/powerpc/include/asm/kvm_host.h +0
-0arch/powerpc/include/asm/kvm_host.h struct kvmppc_booke_debug_reg { · line 378
Quoted 6 lines
arch/powerpc/include/asm/kvm_host.h +3
-0@@ -373,6 +373,9 @@ struct kvmppc_booke_debug_reg {struct kvm_vcpu_arch {ulong host_stack;u32 host_pid; + +u32 intr_ctrler; +That abbreviation seems a bit awkward, and we should also have a private-data pointer.
Quoted 3 lines
How about:
u32 irq_arch;
We also want int pic_fd, no?
Alex
Quoted 35 lines · Alexander Graf
Quoted 2 lines · Scott Wood
Quoted 1 line · Paul Mackerras
Setting this capability on a vcpu connects that vcpu to an
interrupt
Quoted 1 line
Quoted 1 line
controller device. The args[0] field of the argument
kvm_enable_capQuoted 2 lines
Quoted 2 lines
struct specifies the overall architecture of the interrupt controller. The args[1] field specifies the CPU number for the
vcpu
Quoted 5 lines
Quoted 5 lines
from the interrupt controller's point of view.
Signed-off-by Paul Mackerras <[email protected]>---
arch/powerpc/include/asm/kvm_host.h| 3 +++arch/powerpc/kvm/powerpc.c| 29+++++++++++++++++++++++++++++
Quoted 3 lines
Review
Quoted 3 lines
include/uapi/linux/kvm.h| 1 +3 files changed, 33 insertions(+)(quoted diff) +0
-0
b/arch/powerpc/include/asm/kvm_host.hQuoted 14 lines
arch/powerpc/include/asm/kvm_host.h struct kvmppc_booke_debug_reg { · line 378
Quoted 10 lines
index f4ba881..dd167e4 100644
arch/powerpc/include/asm/kvm_host.h +3
-0@@ -373,6 +373,9 @@ struct kvmppc_booke_debug_reg {struct kvm_vcpu_arch {ulong host_stack;u32 host_pid; + +u32 intr_ctrler; +That abbreviation seems a bit awkward, and we should also have a private-data pointer.
Quoted 5 lines
Quoted 3 lines
How about:
u32 irq_arch;We also want int
pic_fd, no?
Not sure we really need that on the vcpu. We'll need it on the vm unless we add it as an arg to the vcpu cap enable.
-Scott
Quoted 29 lines · Scott Wood
Quoted 28 lines · Alexander Graf
Quoted 27 lines · Scott Wood
Quoted 16 lines · Paul Mackerras
Setting this capability on a vcpu connects that vcpu to an interrupt controller device. The args[0] field of the argument
kvm_enable_capstruct specifies the overall architecture of the interrupt controller. The args[1] field specifies the CPU number for the vcpu from the interrupt controller's point of view.Signed-off-by Paul Mackerras <[email protected]>---
arch/powerpc/include/asm/kvm_host.h | 3 +++ arch/powerpc/kvm/powerpc.c | 29 +++++++++++++++++++++++++++++ include/uapi/linux/kvm.h | 1 + 3 files changed, 33 insertions(+)arch/powerpc/include/asm/kvm_host.h +0
-0arch/powerpc/include/asm/kvm_host.h struct kvmppc_booke_debug_reg { · line 378
Quoted 6 lines
arch/powerpc/include/asm/kvm_host.h +3
-0@@ -373,6 +373,9 @@ struct kvmppc_booke_debug_reg {struct kvm_vcpu_arch {ulong host_stack;u32 host_pid; + +u32 intr_ctrler; +That abbreviation seems a bit awkward, and we should also have a private-data pointer.
Quoted 6 lines
Quoted 4 lines
Quoted 3 lines
How about:
u32 irq_arch;We also want int
pic_fd, no?Not sure we really need that on the vcpu. We'll need it on the vm unless we add it as an arg to the vcpu cap enable.
I don't think we need anything vm global for the cpu <-> PIC connections. Also, if you want to deregister a CPU (hotplug remove), you probably want to tell the PIC that the CPU has gone.
Alex
Quoted 39 lines · Alexander Graf
Quoted 4 lines · Scott Wood
Quoted 3 lines · Alexander Graf
Quoted 2 lines · Scott Wood
Quoted 1 line · Paul Mackerras
Setting this capability on a vcpu connects that vcpu to an
interrupt
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
controller device. The args[0] field of the argument
kvm_enable_capQuoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
struct specifies the overall architecture of the interrupt controller. The args[1] field specifies the CPU number for the
vcpu
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
from the interrupt controller's point of view.
Signed-off-by Paul Mackerras <[email protected]>---
arch/powerpc/include/asm/kvm_host.h| 3 +++arch/powerpc/kvm/powerpc.c| 29+++++++++++++++++++++++++++++
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Review
Quoted 3 lines
include/uapi/linux/kvm.h| 1 +3 files changed, 33 insertions(+)(quoted diff) +0
-0
b/arch/powerpc/include/asm/kvm_host.hQuoted 16 lines
Quoted 16 lines
Quoted 16 lines
arch/powerpc/include/asm/kvm_host.h struct kvmppc_booke_debug_reg { · line 378
Quoted 10 lines
index f4ba881..dd167e4 100644
arch/powerpc/include/asm/kvm_host.h +3
-0@@ -373,6 +373,9 @@ struct kvmppc_booke_debug_reg {struct kvm_vcpu_arch {ulong host_stack;u32 host_pid; + +u32 intr_ctrler; +That abbreviation seems a bit awkward, and we should also have a private-data pointer.
How about:
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
Quoted 1 line
u32 irq_arch;We also want int
pic_fd, no?Not sure we really need that on the vcpu. We'll need it on the vm
unless we add it as an arg to the vcpu cap enable.
I don't think we need anything vm global for the cpu <-> PIC connections.
The two components have to link up somehow. If the vcpu cap enable ioctl doesn't specify it, then the device creation will have to stick a reference to itself somewhere in the vm.
Quoted 1 line
Also, if you want to deregister a CPU (hotplug remove), you probably want to tell the PIC that the CPU has gone.
Yes, I mentioned that in my previous e-mail.
-Scott
Quoted 33 lines · Scott Wood
Quoted 32 lines · Alexander Graf
Quoted 31 lines · Scott Wood
Quoted 30 lines · Alexander Graf
Quoted 29 lines · Scott Wood
Quoted 16 lines · Paul Mackerras
Setting this capability on a vcpu connects that vcpu to an interrupt controller device. The args[0] field of the argument
kvm_enable_capstruct specifies the overall architecture of the interrupt controller. The args[1] field specifies the CPU number for the vcpu from the interrupt controller's point of view.Signed-off-by Paul Mackerras <[email protected]>---
arch/powerpc/include/asm/kvm_host.h | 3 +++ arch/powerpc/kvm/powerpc.c | 29 +++++++++++++++++++++++++++++ include/uapi/linux/kvm.h | 1 + 3 files changed, 33 insertions(+)arch/powerpc/include/asm/kvm_host.h +0
-0arch/powerpc/include/asm/kvm_host.h struct kvmppc_booke_debug_reg { · line 378
Quoted 6 lines
arch/powerpc/include/asm/kvm_host.h +3
-0@@ -373,6 +373,9 @@ struct kvmppc_booke_debug_reg {struct kvm_vcpu_arch {ulong host_stack;u32 host_pid; + +u32 intr_ctrler; +That abbreviation seems a bit awkward, and we should also have a private-data pointer.
How about:
Quoted 7 lines
Quoted 5 lines
Quoted 4 lines
Quoted 2 lines
Quoted 1 line
u32 irq_arch;We also want int
pic_fd, no?Not sure we really need that on the vcpu. We'll need it on the vm unless we add it as an arg to the vcpu cap enable.
I don't think we need anything vm global for the cpu <-> PIC connections.
The two components have to link up somehow. If the vcpu cap enable ioctl doesn't specify it, then the device creation will have to stick a reference to itself somewhere in the vm.
Yes, and I think it's a mistake to do it in the vm. We should rather specify the PIC fd in the cap enable ioctl.
Quoted 4 lines
Quoted 2 lines
Also, if you want to deregister a CPU (hotplug remove), you probably want to tell the PIC that the CPU has gone.
Yes, I mentioned that in my previous e-mail.
Yup :). No disagreement with you so far ;)
Alex
Quoted 9 lines · Alexander Graf
Quoted 4 lines · Scott Wood
Quoted 1 line · Alexander Graf
We also want int
pic_fd, no?Not sure we really need that on the vcpu. We'll need it on the vm unless we add it as an arg to the vcpu cap enable.
I don't think we need anything vm global for the cpu <-> PIC connections. Also, if you want to deregister a CPU (hotplug remove), you probably want to tell the PIC that the CPU has gone.
I had kvm_arch_vcpu_free() calling a release function for the selected PIC, which should be enough to let the PIC know the CPU has gone away, I would think.
I agree we don't need anything vm global. The only vm ioctl which needs to care about interrupt controllers is KVM_IRQ_LINE, and the approach I take in my patchset is just to call all of the compiled-in interrupt controllers until one of them takes it. That assumes that each irq architecture adds its own private data pointer to kvm->arch if it's compiled in, which is feasible provided there are only a few architectures supported, and gives the advantages of strong typing. I did it this way because with the irq architecture being specified per vcpu, there is no overall vm global irq architecture.
Paul.
Quoted 8 lines · Paul Mackerras
Quoted 7 lines · Alexander Graf
Quoted 4 lines · Scott Wood
Quoted 1 line · Alexander Graf
We also want int
pic_fd, no?Not sure we really need that on the vcpu. We'll need it on the
Quoted 8 lines
vm unless we add it as an arg to the vcpu cap enable.
Quoted 2 lines
I don't think we need anything vm global for the cpu <-> PIC
connections. Also, if you want to deregister a CPU (hotplug remove), you probably want to tell the PIC that the CPU has gone.
I had
kvm_arch_vcpu_free()calling a release function for the selected PIC, which should be enough to let the PIC know the CPU has gone away, I would think.
Ah, sorry, I missed that somehow.
Quoted 4 lines
I agree we don't need anything vm global. The only vm ioctl which needs to care about interrupt controllers is
KVM_IRQ_LINE, and the approach I take in my patchset is just to call all of the compiled-in interrupt controllers until one of them takes it.
KVM_IRQ_LINE is (eventually) supposed to go through the IRQ routing layer that determines which irqchip it goes to. Otherwise, how will we do things like generate MSIs through irqfd? At least, for those of us who haven't already mapped MSIs to normal interrupts inside QEMU via hcalls. :-)
-Scott
Quoted 15 lines · Scott Wood
arch/powerpc/include/asm/kvm_host.h struct kvmppc_booke_debug_reg { · line 378
Quoted 9 lines · Paul Mackerras
arch/powerpc/include/asm/kvm_host.h +3
-0@@ -373,6 +373,9 @@ struct kvmppc_booke_debug_reg {struct kvm_vcpu_arch {ulong host_stack;u32 host_pid; + +u32 intr_ctrler; +That abbreviation seems a bit awkward, and we should also have a private-data pointer.
Quoted 4 lines
How about:
u32 irq_arch; void *irq_priv;
Regarding the irq_priv - in my patchset the XICS code adds its own private data pointer. That has the advantage that it can be strongly typed, and if it is non-NULL then I know it points to XICS data, not the data for some other type of controller. As long as we are only going to have a small number of IRQ architectures then it's feasible to allow each to have its own data pointer, and we get the advantages of strong typing.
Quoted 9 lines
arch/powerpc/include/asm/kvm_host.h struct kvmppc_booke_debug_reg { · line 381
Quoted 3 lines
arch/powerpc/include/asm/kvm_host.h +3
-0+switch (cap->args[0 ]) { +case 0 :/* no interrupt controller */ +break ;s/0/KVM_IRQ_ARCH_NONE/
...at least so that this patch makes it clear where other type ids should
be defined.
OK, whatever.
Quoted 13 lines
arch/powerpc/include/asm/kvm_host.h struct kvmppc_booke_debug_reg {
Quoted 11 lines
arch/powerpc/include/asm/kvm_host.h +11
-0+default : + r = -EINVAL ; + } +if (!r) { +/* +* Make sure any state set up by the interrupt +* controller init routine is seen before this. +*/ +smp_wmb (); + vcpu->arch.intr_ctrler = cap->args[0 ]; + }Do we really need that
wmb()? We're in vcpu context, right? If the
Quoted 8 lines
vcpu
migrates to another host cpu, that involves rescheduling which already has a sync. If the interrupt controller code we call here modifies data that will be seen from outside the vcpu, it's the responsibility of that code to use whatever locks, barriers, etc. are needed (and it's unlikely
thatvcpu->arch.intr_ctrlerwill be the relevant thing that it needs to order with).
OK, you're right, the generic KVM code serializes most vcpu ioctls, including KVM_RUN and KVM_ENABLE_CAP, so the barrier isn't in fact needed.
Quoted 2 lines
This patch should also add a hook at vcpu destruction to call into the irq code.
You appear to have missed this hunk:
arch/powerpc/kvm/powerpc.c +6 -0
@@ -459,6 +459,12 @@ void kvm_arch_vcpu_free(struct kvm_vcpu *vcpu)hrtimer_cancel (&vcpu->arch.dec_timer);tasklet_kill (&vcpu->arch.tasklet); +/* Release any per-vcpu irq controller state */ +switch (vcpu->arch.intr_ctrler) { +default : +break ; + } +kvmppc_remove_vcpu_debugfs (vcpu);kvmppc_core_vcpu_free (vcpu); }
Paul.
Quoted 21 lines · Paul Mackerras
Quoted 15 lines · Scott Wood
arch/powerpc/include/asm/kvm_host.h struct kvmppc_booke_debug_reg { · line 378
Quoted 9 lines · Paul Mackerras
arch/powerpc/include/asm/kvm_host.h +3
-0@@ -373,6 +373,9 @@ struct kvmppc_booke_debug_reg {struct kvm_vcpu_arch {ulong host_stack;u32 host_pid; + +u32 intr_ctrler; +That abbreviation seems a bit awkward, and we should also have a private-data pointer.
Quoted 4 lines
How about:
u32 irq_arch; void *irq_priv;
Quoted 7 lines
Regarding the
irq_priv- in my patchset the XICS code adds its own private data pointer. That has the advantage that it can be strongly typed, and if it is non-NULL then I know it points to XICS data, not the data for some other type of controller. As long as we are only going to have a small number of IRQ architectures then it's feasible to allow each to have its own data pointer, and we get the advantages of strong typing.
OK.
Quoted 11 lines
Quoted 9 lines
arch/powerpc/include/asm/kvm_host.h struct kvmppc_booke_debug_reg { · line 381
Quoted 3 lines
arch/powerpc/include/asm/kvm_host.h +3
-0+switch (cap->args[0 ]) { +case 0 :/* no interrupt controller */ +break ;s/0/KVM_IRQ_ARCH_NONE/
...at least so that this patch makes it clear where other type ids should
be defined.OK, whatever.
Well, it wouldn't be good for MPIC to end up defined in one place and XICS in another, especially if adding a definition is how new IDs are allocated...
-Scott