On 04/03/2013 05:12:06 PM, Alexander Graf wrote:
Am 04.04.2013 um 00:07 schrieb Scott Wood <scottwood@xxxxxxxxxxxxx>:
> 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.
Ok, please indicate compatibility limitations in the Kconfig
description at least then.
OK -- not really a "compatibility" limitation so much as what models
are supported.
Note that mpc86xx has a 74xx-derived core, but also has an FSL MPIC...
Is 74xx/e600 supported by book3s_pr? Can't tell from the kconfig text.
:-)
>> >> > + 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 :)
OTOH, it looks like access_reg() *was* missing a lock. :-P
-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
