On 04/03/2013 04:58:56 PM, Alexander Graf wrote:
Am 03.04.2013 um 23:38 schrieb Scott Wood <scottwood@xxxxxxxxxxxxx>:
> 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 <scottwood@xxxxxxxxxxxxx>
>> > ---
>> > 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.
>> > + 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.
-Scott
--
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
