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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html