Re: [RFC PATCH v3 5/6] kvm/ppc/mpic: in-kernel MPIC emulation

Scott Wood
2013-04-03T22:54:34Z

On 04/03/2013 05:12:06 PM, Alexander Graf wrote:
>
>
> Am 04.04.2013 um 00:07 schrieb Scott Wood <scottwood@freescale.com>:
>
> > On 04/03/2013 04:58:56 PM, Alexander Graf wrote:
> >> Am 03.04.2013 um 23:38 schrieb Scott Wood
> <scottwood@freescale.com>:
> >> > 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@freescale.com>
> >> >> > ---
> >> >> > 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