Am 04.04.2013 um 00:07 schrieb Scott Wood <[email protected]>:
> On 04/03/2013 04:58:56 PM, Alexander Graf wrote: >> Am 03.04.2013 um 23:38 schrieb Scott Wood <[email protected]>: >> > On 04/03/2013 11:19:42 AM, Alexander Graf wrote: >> >> On 03.04.2013, at 03:57, Scott Wood wrote: >> >> > Hook the MPIC code up to the KVM interfaces, add locking, etc. >> >> > >> >> > TODO: irqfd support, split up into multiple patches, KVM_IRQ_LINE >> >> > support >> >> > >> >> > Signed-off-by: Scott Wood <[email protected]> >> >> > --- >> >> > v3: mpic_put -> kvmppc_mpic_put >> >> > >> >> > >> >> [...] >> >> > +void kvmppc_mpic_set_epr(struct kvm_vcpu *vcpu); >> >> > + >> >> > int kvm_vcpu_ioctl_config_tlb(struct kvm_vcpu *vcpu, >> >> > struct kvm_config_tlb *cfg); >> >> > int kvm_vcpu_ioctl_dirty_tlb(struct kvm_vcpu *vcpu, >> >> > diff --git a/arch/powerpc/kvm/Kconfig b/arch/powerpc/kvm/Kconfig >> >> > index 63c67ec..a87139b 100644 >> >> > --- a/arch/powerpc/kvm/Kconfig >> >> > +++ b/arch/powerpc/kvm/Kconfig >> >> > @@ -151,6 +151,11 @@ config KVM_E500MC >> >> > >> >> > If unsure, say N. >> >> > >> >> > +config KVM_MPIC >> >> > + bool "KVM in-kernel MPIC emulation" >> >> > + depends on KVM >> >> This should probably depend on FSL KVM for now, until someone adds >> >> support for other MPIC revisions. >> > >> > I don't see a symbol specifically for "FSL KVM". What part of the MPIC >> > code depends on booke or any FSL-specific code? >> You support only FSL mpic device IDs :). So if someone on book3s goes along >> and sees this, he'd think "yes, I want an in-kernel MPIC", enables the >> option and wastes space. > > "Would this waste space" is not generally the criteria for kconfig > dependencies. Who is the kernel to get in the way of someone that wants an > FSL MPIC on a 4xx VM? :-) > > And again, there's no symbol for FSL KVM -- I'd have to use a list that could > get out of date. And it would reduce build testing in allyesconfig-type > configs. Ok, please indicate compatibility limitations in the Kconfig description at least then. > >> >> > + switch (attr->group) { >> >> > + case KVM_DEV_MPIC_GRP_MISC: >> >> > + switch (attr->attr) { >> >> > + case KVM_DEV_MPIC_BASE_ADDR: >> >> > + mutex_lock(&opp->kvm->slots_lock); >> >> > + attr64 = opp->reg_base; >> >> > + mutex_unlock(&opp->kvm->slots_lock); >> >> > + >> >> > + if (copy_to_user((u64 __user *)(long)attr->addr, >> >> > + &attr64, sizeof(u64))) >> >> u64 is tricky with put_user on 32bit hosts, so here copy_to_user makes >> >> sense >> > >> > What are the issues with put_user? It looks like it's supported with a >> > pair of "stw" instructions. >> Oh? Last time I tried to use get/put_user for one_reg it failed on ppc32. So >> maybe the u64 support is new? > > Not new according to git -- though I haven't tried to use it yet; maybe it's > broken. > >> >> > + case KVM_DEV_MPIC_GRP_IRQ_ACTIVE: >> >> > + if (attr->attr > MAX_SRC) >> >> > + return -EINVAL; >> >> > + >> >> > + attr32 = opp->src[attr->attr].pending; >> >> Isn't this missing a lock? >> > >> > I don't see why it needs one. If the pending status changes during the >> > ioctl, it's undefined which state you'll read back, and a lock wouldn't >> > change that (you could end up taking the lock before or after the change >> > gets made). >> > >> > reg_base above was a different situation -- it's 64-bit, so we can't read >> > it atomically on 32-bit. >> Ok, so this relies on 32bit read accesses being atomic and stale values ok. > > Not just 32-bit, but only two possible values, with one bitflip between > them... Even if GCC does the read a byte at a time it'd be OK. > >> That works for me, but deserves a comment. > > If I'm going to change it, might as well just put the lock in to be > consistent. Either way works, just want to make sure whoever reads the code knows that things were done on purpose :) Alex > > -Scott -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
