* [PATCH] KVM: Add KVM_CAP_IRQ_ARCH capability @ 2013-03-14 1:20 ` Paul Mackerras 0 siblings, 0 replies; 22+ messages in thread From: Paul Mackerras @ 2013-03-14 1:20 UTC (permalink / raw) To: Alexander Graf, kvm; +Cc: kvm-ppc, Scott Wood 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. Signed-off-by: Paul Mackerras <paulus@samba.org> --- 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(+) diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h index f4ba881..dd167e4 100644 --- a/arch/powerpc/include/asm/kvm_host.h +++ b/arch/powerpc/include/asm/kvm_host.h @@ -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[] */ diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c index 934413c..891603e 100644 --- a/arch/powerpc/kvm/powerpc.c +++ b/arch/powerpc/kvm/powerpc.c @@ -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; diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h index 1f348e0..d875d37 100644 --- a/include/uapi/linux/kvm.h +++ b/include/uapi/linux/kvm.h @@ -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 -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH] KVM: Add KVM_CAP_IRQ_ARCH capability @ 2013-03-14 1:20 ` Paul Mackerras 0 siblings, 0 replies; 22+ messages in thread From: Paul Mackerras @ 2013-03-14 1:20 UTC (permalink / raw) To: Alexander Graf, kvm; +Cc: kvm-ppc, Scott Wood 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. Signed-off-by: Paul Mackerras <paulus@samba.org> --- 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(+) diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h index f4ba881..dd167e4 100644 --- a/arch/powerpc/include/asm/kvm_host.h +++ b/arch/powerpc/include/asm/kvm_host.h @@ -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[] */ diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c index 934413c..891603e 100644 --- a/arch/powerpc/kvm/powerpc.c +++ b/arch/powerpc/kvm/powerpc.c @@ -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; diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h index 1f348e0..d875d37 100644 --- a/include/uapi/linux/kvm.h +++ b/include/uapi/linux/kvm.h @@ -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 -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH] KVM: Add KVM_CAP_IRQ_ARCH capability 2013-03-14 1:20 ` Paul Mackerras @ 2013-03-14 18:20 ` Scott Wood -1 siblings, 0 replies; 22+ messages in thread From: Scott Wood @ 2013-03-14 18:20 UTC (permalink / raw) To: Paul Mackerras; +Cc: Alexander Graf, kvm, kvm-ppc On 03/13/2013 08:20:44 PM, Paul Mackerras wrote: > 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. > > Signed-off-by: Paul Mackerras <paulus@samba.org> > --- > 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(+) > > diff --git a/arch/powerpc/include/asm/kvm_host.h > b/arch/powerpc/include/asm/kvm_host.h > index f4ba881..dd167e4 100644 > --- a/arch/powerpc/include/asm/kvm_host.h > +++ b/arch/powerpc/include/asm/kvm_host.h > @@ -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; > #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. > + 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 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] KVM: Add KVM_CAP_IRQ_ARCH capability @ 2013-03-14 18:20 ` Scott Wood 0 siblings, 0 replies; 22+ messages in thread From: Scott Wood @ 2013-03-14 18:20 UTC (permalink / raw) To: Paul Mackerras; +Cc: Alexander Graf, kvm, kvm-ppc On 03/13/2013 08:20:44 PM, Paul Mackerras wrote: > 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. > > Signed-off-by: Paul Mackerras <paulus@samba.org> > --- > 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(+) > > diff --git a/arch/powerpc/include/asm/kvm_host.h > b/arch/powerpc/include/asm/kvm_host.h > index f4ba881..dd167e4 100644 > --- a/arch/powerpc/include/asm/kvm_host.h > +++ b/arch/powerpc/include/asm/kvm_host.h > @@ -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; > #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. > + 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 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] KVM: Add KVM_CAP_IRQ_ARCH capability 2013-03-14 18:20 ` Scott Wood @ 2013-03-14 18:33 ` Alexander Graf -1 siblings, 0 replies; 22+ messages in thread From: Alexander Graf @ 2013-03-14 18:33 UTC (permalink / raw) To: Scott Wood; +Cc: Paul Mackerras, kvm, kvm-ppc On 14.03.2013, at 19:20, Scott Wood wrote: > On 03/13/2013 08:20:44 PM, Paul Mackerras wrote: >> 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. >> Signed-off-by: Paul Mackerras <paulus@samba.org> >> --- >> 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(+) >> diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h >> index f4ba881..dd167e4 100644 >> --- a/arch/powerpc/include/asm/kvm_host.h >> +++ b/arch/powerpc/include/asm/kvm_host.h >> @@ -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; We also want int pic_fd, no? Alex ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] KVM: Add KVM_CAP_IRQ_ARCH capability @ 2013-03-14 18:33 ` Alexander Graf 0 siblings, 0 replies; 22+ messages in thread From: Alexander Graf @ 2013-03-14 18:33 UTC (permalink / raw) To: Scott Wood; +Cc: Paul Mackerras, kvm, kvm-ppc On 14.03.2013, at 19:20, Scott Wood wrote: > On 03/13/2013 08:20:44 PM, Paul Mackerras wrote: >> 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. >> Signed-off-by: Paul Mackerras <paulus@samba.org> >> --- >> 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(+) >> diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h >> index f4ba881..dd167e4 100644 >> --- a/arch/powerpc/include/asm/kvm_host.h >> +++ b/arch/powerpc/include/asm/kvm_host.h >> @@ -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; We also want int pic_fd, no? Alex ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] KVM: Add KVM_CAP_IRQ_ARCH capability 2013-03-14 18:33 ` Alexander Graf @ 2013-03-14 18:35 ` Scott Wood -1 siblings, 0 replies; 22+ messages in thread From: Scott Wood @ 2013-03-14 18:35 UTC (permalink / raw) To: Alexander Graf; +Cc: Paul Mackerras, kvm, kvm-ppc On 03/14/2013 01:33:30 PM, Alexander Graf wrote: > > On 14.03.2013, at 19:20, Scott Wood wrote: > > > On 03/13/2013 08:20:44 PM, Paul Mackerras wrote: > >> 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. > >> Signed-off-by: Paul Mackerras <paulus@samba.org> > >> --- > >> 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(+) > >> diff --git a/arch/powerpc/include/asm/kvm_host.h > b/arch/powerpc/include/asm/kvm_host.h > >> index f4ba881..dd167e4 100644 > >> --- a/arch/powerpc/include/asm/kvm_host.h > >> +++ b/arch/powerpc/include/asm/kvm_host.h > >> @@ -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; > > 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] KVM: Add KVM_CAP_IRQ_ARCH capability @ 2013-03-14 18:35 ` Scott Wood 0 siblings, 0 replies; 22+ messages in thread From: Scott Wood @ 2013-03-14 18:35 UTC (permalink / raw) To: Alexander Graf; +Cc: Paul Mackerras, kvm, kvm-ppc On 03/14/2013 01:33:30 PM, Alexander Graf wrote: > > On 14.03.2013, at 19:20, Scott Wood wrote: > > > On 03/13/2013 08:20:44 PM, Paul Mackerras wrote: > >> 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. > >> Signed-off-by: Paul Mackerras <paulus@samba.org> > >> --- > >> 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(+) > >> diff --git a/arch/powerpc/include/asm/kvm_host.h > b/arch/powerpc/include/asm/kvm_host.h > >> index f4ba881..dd167e4 100644 > >> --- a/arch/powerpc/include/asm/kvm_host.h > >> +++ b/arch/powerpc/include/asm/kvm_host.h > >> @@ -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; > > 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] KVM: Add KVM_CAP_IRQ_ARCH capability 2013-03-14 18:35 ` Scott Wood @ 2013-03-14 19:03 ` Alexander Graf -1 siblings, 0 replies; 22+ messages in thread From: Alexander Graf @ 2013-03-14 19:03 UTC (permalink / raw) To: Scott Wood; +Cc: Paul Mackerras, kvm, kvm-ppc On 14.03.2013, at 19:35, Scott Wood wrote: > On 03/14/2013 01:33:30 PM, Alexander Graf wrote: >> On 14.03.2013, at 19:20, Scott Wood wrote: >> > On 03/13/2013 08:20:44 PM, Paul Mackerras wrote: >> >> 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. >> >> Signed-off-by: Paul Mackerras <paulus@samba.org> >> >> --- >> >> 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(+) >> >> diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h >> >> index f4ba881..dd167e4 100644 >> >> --- a/arch/powerpc/include/asm/kvm_host.h >> >> +++ b/arch/powerpc/include/asm/kvm_host.h >> >> @@ -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; >> 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] KVM: Add KVM_CAP_IRQ_ARCH capability @ 2013-03-14 19:03 ` Alexander Graf 0 siblings, 0 replies; 22+ messages in thread From: Alexander Graf @ 2013-03-14 19:03 UTC (permalink / raw) To: Scott Wood; +Cc: Paul Mackerras, kvm, kvm-ppc On 14.03.2013, at 19:35, Scott Wood wrote: > On 03/14/2013 01:33:30 PM, Alexander Graf wrote: >> On 14.03.2013, at 19:20, Scott Wood wrote: >> > On 03/13/2013 08:20:44 PM, Paul Mackerras wrote: >> >> 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. >> >> Signed-off-by: Paul Mackerras <paulus@samba.org> >> >> --- >> >> 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(+) >> >> diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h >> >> index f4ba881..dd167e4 100644 >> >> --- a/arch/powerpc/include/asm/kvm_host.h >> >> +++ b/arch/powerpc/include/asm/kvm_host.h >> >> @@ -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; >> 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] KVM: Add KVM_CAP_IRQ_ARCH capability 2013-03-14 19:03 ` Alexander Graf @ 2013-03-14 19:10 ` Scott Wood -1 siblings, 0 replies; 22+ messages in thread From: Scott Wood @ 2013-03-14 19:10 UTC (permalink / raw) To: Alexander Graf; +Cc: Paul Mackerras, kvm, kvm-ppc On 03/14/2013 02:03:30 PM, Alexander Graf wrote: > > On 14.03.2013, at 19:35, Scott Wood wrote: > > > On 03/14/2013 01:33:30 PM, Alexander Graf wrote: > >> On 14.03.2013, at 19:20, Scott Wood wrote: > >> > On 03/13/2013 08:20:44 PM, Paul Mackerras wrote: > >> >> 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. > >> >> Signed-off-by: Paul Mackerras <paulus@samba.org> > >> >> --- > >> >> 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(+) > >> >> diff --git a/arch/powerpc/include/asm/kvm_host.h > b/arch/powerpc/include/asm/kvm_host.h > >> >> index f4ba881..dd167e4 100644 > >> >> --- a/arch/powerpc/include/asm/kvm_host.h > >> >> +++ b/arch/powerpc/include/asm/kvm_host.h > >> >> @@ -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; > >> 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. > 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] KVM: Add KVM_CAP_IRQ_ARCH capability @ 2013-03-14 19:10 ` Scott Wood 0 siblings, 0 replies; 22+ messages in thread From: Scott Wood @ 2013-03-14 19:10 UTC (permalink / raw) To: Alexander Graf; +Cc: Paul Mackerras, kvm, kvm-ppc On 03/14/2013 02:03:30 PM, Alexander Graf wrote: > > On 14.03.2013, at 19:35, Scott Wood wrote: > > > On 03/14/2013 01:33:30 PM, Alexander Graf wrote: > >> On 14.03.2013, at 19:20, Scott Wood wrote: > >> > On 03/13/2013 08:20:44 PM, Paul Mackerras wrote: > >> >> 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. > >> >> Signed-off-by: Paul Mackerras <paulus@samba.org> > >> >> --- > >> >> 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(+) > >> >> diff --git a/arch/powerpc/include/asm/kvm_host.h > b/arch/powerpc/include/asm/kvm_host.h > >> >> index f4ba881..dd167e4 100644 > >> >> --- a/arch/powerpc/include/asm/kvm_host.h > >> >> +++ b/arch/powerpc/include/asm/kvm_host.h > >> >> @@ -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; > >> 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. > 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] KVM: Add KVM_CAP_IRQ_ARCH capability 2013-03-14 19:10 ` Scott Wood @ 2013-03-14 21:46 ` Alexander Graf -1 siblings, 0 replies; 22+ messages in thread From: Alexander Graf @ 2013-03-14 21:46 UTC (permalink / raw) To: Scott Wood; +Cc: Paul Mackerras, kvm, kvm-ppc On 14.03.2013, at 20:10, Scott Wood wrote: > On 03/14/2013 02:03:30 PM, Alexander Graf wrote: >> On 14.03.2013, at 19:35, Scott Wood wrote: >> > On 03/14/2013 01:33:30 PM, Alexander Graf wrote: >> >> On 14.03.2013, at 19:20, Scott Wood wrote: >> >> > On 03/13/2013 08:20:44 PM, Paul Mackerras wrote: >> >> >> 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. >> >> >> Signed-off-by: Paul Mackerras <paulus@samba.org> >> >> >> --- >> >> >> 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(+) >> >> >> diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h >> >> >> index f4ba881..dd167e4 100644 >> >> >> --- a/arch/powerpc/include/asm/kvm_host.h >> >> >> +++ b/arch/powerpc/include/asm/kvm_host.h >> >> >> @@ -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; >> >> 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. > >> 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] KVM: Add KVM_CAP_IRQ_ARCH capability @ 2013-03-14 21:46 ` Alexander Graf 0 siblings, 0 replies; 22+ messages in thread From: Alexander Graf @ 2013-03-14 21:46 UTC (permalink / raw) To: Scott Wood; +Cc: Paul Mackerras, kvm, kvm-ppc On 14.03.2013, at 20:10, Scott Wood wrote: > On 03/14/2013 02:03:30 PM, Alexander Graf wrote: >> On 14.03.2013, at 19:35, Scott Wood wrote: >> > On 03/14/2013 01:33:30 PM, Alexander Graf wrote: >> >> On 14.03.2013, at 19:20, Scott Wood wrote: >> >> > On 03/13/2013 08:20:44 PM, Paul Mackerras wrote: >> >> >> 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. >> >> >> Signed-off-by: Paul Mackerras <paulus@samba.org> >> >> >> --- >> >> >> 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(+) >> >> >> diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h >> >> >> index f4ba881..dd167e4 100644 >> >> >> --- a/arch/powerpc/include/asm/kvm_host.h >> >> >> +++ b/arch/powerpc/include/asm/kvm_host.h >> >> >> @@ -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; >> >> 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. > >> 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] KVM: Add KVM_CAP_IRQ_ARCH capability 2013-03-14 19:03 ` Alexander Graf @ 2013-03-14 22:28 ` Paul Mackerras -1 siblings, 0 replies; 22+ messages in thread From: Paul Mackerras @ 2013-03-14 22:28 UTC (permalink / raw) To: Alexander Graf; +Cc: Scott Wood, kvm, kvm-ppc On Thu, Mar 14, 2013 at 08:03:30PM +0100, Alexander Graf wrote: > > On 14.03.2013, at 19:35, Scott Wood wrote: > > > On 03/14/2013 01:33:30 PM, Alexander Graf wrote: > >> 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] KVM: Add KVM_CAP_IRQ_ARCH capability @ 2013-03-14 22:28 ` Paul Mackerras 0 siblings, 0 replies; 22+ messages in thread From: Paul Mackerras @ 2013-03-14 22:28 UTC (permalink / raw) To: Alexander Graf; +Cc: Scott Wood, kvm, kvm-ppc On Thu, Mar 14, 2013 at 08:03:30PM +0100, Alexander Graf wrote: > > On 14.03.2013, at 19:35, Scott Wood wrote: > > > On 03/14/2013 01:33:30 PM, Alexander Graf wrote: > >> 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] KVM: Add KVM_CAP_IRQ_ARCH capability 2013-03-14 22:28 ` Paul Mackerras @ 2013-03-14 22:38 ` Scott Wood -1 siblings, 0 replies; 22+ messages in thread From: Scott Wood @ 2013-03-14 22:38 UTC (permalink / raw) To: Paul Mackerras; +Cc: Alexander Graf, kvm, kvm-ppc On 03/14/2013 05:28:03 PM, Paul Mackerras wrote: > On Thu, Mar 14, 2013 at 08:03:30PM +0100, Alexander Graf wrote: > > > > On 14.03.2013, at 19:35, Scott Wood wrote: > > > > > On 03/14/2013 01:33:30 PM, Alexander Graf wrote: > > >> 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. Ah, sorry, I missed that somehow. > 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] KVM: Add KVM_CAP_IRQ_ARCH capability @ 2013-03-14 22:38 ` Scott Wood 0 siblings, 0 replies; 22+ messages in thread From: Scott Wood @ 2013-03-14 22:38 UTC (permalink / raw) To: Paul Mackerras; +Cc: Alexander Graf, kvm, kvm-ppc On 03/14/2013 05:28:03 PM, Paul Mackerras wrote: > On Thu, Mar 14, 2013 at 08:03:30PM +0100, Alexander Graf wrote: > > > > On 14.03.2013, at 19:35, Scott Wood wrote: > > > > > On 03/14/2013 01:33:30 PM, Alexander Graf wrote: > > >> 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. Ah, sorry, I missed that somehow. > 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] KVM: Add KVM_CAP_IRQ_ARCH capability 2013-03-14 18:20 ` Scott Wood @ 2013-03-14 22:19 ` Paul Mackerras -1 siblings, 0 replies; 22+ messages in thread From: Paul Mackerras @ 2013-03-14 22:19 UTC (permalink / raw) To: Scott Wood; +Cc: Alexander Graf, kvm, kvm-ppc On Thu, Mar 14, 2013 at 01:20:38PM -0500, Scott Wood wrote: > On 03/13/2013 08:20:44 PM, Paul Mackerras wrote: > >--- a/arch/powerpc/include/asm/kvm_host.h > >+++ b/arch/powerpc/include/asm/kvm_host.h > >@@ -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; 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. > >+ 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. > >+ 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 > 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). 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. > This patch should also add a hook at vcpu destruction to call into the > irq code. You appear to have missed this hunk: diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c index 934413c..891603e 100644 --- a/arch/powerpc/kvm/powerpc.c +++ b/arch/powerpc/kvm/powerpc.c @@ -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. ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH] KVM: Add KVM_CAP_IRQ_ARCH capability @ 2013-03-14 22:19 ` Paul Mackerras 0 siblings, 0 replies; 22+ messages in thread From: Paul Mackerras @ 2013-03-14 22:19 UTC (permalink / raw) To: Scott Wood; +Cc: Alexander Graf, kvm, kvm-ppc On Thu, Mar 14, 2013 at 01:20:38PM -0500, Scott Wood wrote: > On 03/13/2013 08:20:44 PM, Paul Mackerras wrote: > >--- a/arch/powerpc/include/asm/kvm_host.h > >+++ b/arch/powerpc/include/asm/kvm_host.h > >@@ -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; 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. > >+ 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. > >+ 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 > 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). 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. > This patch should also add a hook at vcpu destruction to call into the > irq code. You appear to have missed this hunk: diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c index 934413c..891603e 100644 --- a/arch/powerpc/kvm/powerpc.c +++ b/arch/powerpc/kvm/powerpc.c @@ -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. ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH] KVM: Add KVM_CAP_IRQ_ARCH capability 2013-03-14 22:19 ` Paul Mackerras @ 2013-03-14 22:41 ` Scott Wood -1 siblings, 0 replies; 22+ messages in thread From: Scott Wood @ 2013-03-14 22:41 UTC (permalink / raw) To: Paul Mackerras; +Cc: Alexander Graf, kvm, kvm-ppc On 03/14/2013 05:19:17 PM, Paul Mackerras wrote: > On Thu, Mar 14, 2013 at 01:20:38PM -0500, Scott Wood wrote: > > On 03/13/2013 08:20:44 PM, Paul Mackerras wrote: > > > >--- a/arch/powerpc/include/asm/kvm_host.h > > >+++ b/arch/powerpc/include/asm/kvm_host.h > > >@@ -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; > > 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. > > >+ 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] KVM: Add KVM_CAP_IRQ_ARCH capability @ 2013-03-14 22:41 ` Scott Wood 0 siblings, 0 replies; 22+ messages in thread From: Scott Wood @ 2013-03-14 22:41 UTC (permalink / raw) To: Paul Mackerras; +Cc: Alexander Graf, kvm, kvm-ppc On 03/14/2013 05:19:17 PM, Paul Mackerras wrote: > On Thu, Mar 14, 2013 at 01:20:38PM -0500, Scott Wood wrote: > > On 03/13/2013 08:20:44 PM, Paul Mackerras wrote: > > > >--- a/arch/powerpc/include/asm/kvm_host.h > > >+++ b/arch/powerpc/include/asm/kvm_host.h > > >@@ -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; > > 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. > > >+ 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2013-03-14 22:41 UTC | newest] Thread overview: 22+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-03-14 1:20 [PATCH] KVM: Add KVM_CAP_IRQ_ARCH capability Paul Mackerras 2013-03-14 1:20 ` Paul Mackerras 2013-03-14 18:20 ` Scott Wood 2013-03-14 18:20 ` Scott Wood 2013-03-14 18:33 ` Alexander Graf 2013-03-14 18:33 ` Alexander Graf 2013-03-14 18:35 ` Scott Wood 2013-03-14 18:35 ` Scott Wood 2013-03-14 19:03 ` Alexander Graf 2013-03-14 19:03 ` Alexander Graf 2013-03-14 19:10 ` Scott Wood 2013-03-14 19:10 ` Scott Wood 2013-03-14 21:46 ` Alexander Graf 2013-03-14 21:46 ` Alexander Graf 2013-03-14 22:28 ` Paul Mackerras 2013-03-14 22:28 ` Paul Mackerras 2013-03-14 22:38 ` Scott Wood 2013-03-14 22:38 ` Scott Wood 2013-03-14 22:19 ` Paul Mackerras 2013-03-14 22:19 ` Paul Mackerras 2013-03-14 22:41 ` Scott Wood 2013-03-14 22:41 ` Scott Wood
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.