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

[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.

> >> > +    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