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

[Scott Wood <scottwood@freescale.com>]

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