Patchwork KVM: Add KVM_CAP_IRQ_ARCH capability

login
register
mail settings
Submitter Paul Mackerras
Date March 14, 2013, 10:19 p.m.
Message ID <20130314221917.GI9841@iris.ozlabs.ibm.com>
Download mbox | patch
Permalink /patch/227825/
State New
Headers show

Comments

Paul Mackerras - March 14, 2013, 10:19 p.m.
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:


Paul.
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Scott Wood - March 14, 2013, 10:41 p.m.
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
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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