Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/4] spapr: introduce a helper to compute the address of the HPT

[Greg Kurz]

On Mon, 11 Sep 2017 22:50:45 +1000
David Gibson <address@hidden> wrote:

> On Mon, Sep 11, 2017 at 02:04:37PM +0200, Greg Kurz wrote:
> > On Sun, 10 Sep 2017 13:41:41 +1000
> > David Gibson <address@hidden> wrote:
> >   
> > > On Mon, Sep 04, 2017 at 11:47:05PM +0200, Greg Kurz wrote:  
> > > > The formula used to compute the address of the HPT allocated by QEMU is
> > > > open-coded in several places. This patch moves the magic to a dedicated
> > > > helper. While here, we also patch the callers to only pass the address
> > > > to KVM if we indeed have a userland HPT (ie, KVM PR).    
> > > 
> > > The helper function seems reasonable, though I'm not sure about the
> > > name (a. it's not just a pointer, since it includes the encoded size
> > > and b. the name doesn't indicate this is basically KVM PR specific).
> > >   
> > 
> > Sure, I'll come up with a better name.
> >   
> > > THe "only pass the address to KVM if we indeed have a userland HPT
> > > (ie, KVM PR)" bit doesn't really work.  You're doing it by testing for
> > > (sdr1 != 0), but that can only be true if the HPT is minimum size,
> > > which doesn't have much to do with anything meaningful.
> > >   
> > 
> > Hmmm... if QEMU has allocated an HPT in userspace then the helper
> > necessarily returns a non-null value, no matter the HPT size. Am
> > I missing something ?  
> 
> Yes, but the reverse is not true.  Even if qemu *hasn't* allocated an
> HPT in userspace, it will usually return a non-zero value - the only
> case it won't is when the HPT is minimum size.  That makes the test
> pretty pointless.
> 

The helper does return 0 if QEMU hasn't allocated an HPT in userspace.

[...]
> > > > +target_ulong spapr_get_hpt_pointer(sPAPRMachineState *spapr)
> > > > +{
> > > > +    if (!spapr->htab) {
> > > > +        return 0;
> > > > +    }
> > > > +
> > > > +    return (target_ulong)(uintptr_t)spapr->htab | (spapr->htab_shift - 
> > > > 18);
> > > > +}
> > > > +

but I agree the patch isn't good... for the comprehension at least. I'll
rework the patchset.

Also this causes a behavior change: we won't update SDR1 anymore with KVM HV,
which already handles it BTW.

void kvmppc_set_hpt(struct kvm *kvm, struct kvm_hpt_info *info)
{
        atomic64_set(&kvm->arch.mmio_update, 0);
        kvm->arch.hpt = *info;
        kvm->arch.sdr1 = __pa(info->virt) | (info->order - 18);

Maybe I should push this behavior change to a separate patch anyway...

pgpro0t9Vrzus.pgp
Description: OpenPGP digital signature