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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 5648 bytes --]

On Mon, Sep 11, 2017 at 02:04:37PM +0200, Greg Kurz wrote:
> On Sun, 10 Sep 2017 13:41:41 +1000
> David Gibson <david@gibson.dropbear.id.au> 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.

> 
> > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > ---
> > >  hw/ppc/spapr.c          |    9 +++++++++
> > >  hw/ppc/spapr_cpu_core.c |   12 +++++++-----
> > >  hw/ppc/spapr_hcall.c    |   14 ++++++++------
> > >  include/hw/ppc/spapr.h  |    1 +
> > >  4 files changed, 25 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index caffa1276328..bf24c26b756d 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -1290,6 +1290,15 @@ static void spapr_store_hpte(PPCVirtualHypervisor *vhyp, hwaddr ptex,
> > >      }
> > >  }
> > >  
> > > +target_ulong spapr_get_hpt_pointer(sPAPRMachineState *spapr)
> > > +{
> > > +    if (!spapr->htab) {
> > > +        return 0;
> > > +    }
> > > +
> > > +    return (target_ulong)(uintptr_t)spapr->htab | (spapr->htab_shift - 18);
> > > +}
> > > +
> > >  int spapr_hpt_shift_for_ramsize(uint64_t ramsize)
> > >  {
> > >      int shift;
> > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > index 85037ef71e27..581eb4d92de9 100644
> > > --- a/hw/ppc/spapr_cpu_core.c
> > > +++ b/hw/ppc/spapr_cpu_core.c
> > > @@ -93,11 +93,13 @@ static void spapr_cpu_reset(void *opaque)
> > >       * HPT
> > >       */
> > >      if (kvm_enabled()) {
> > > -        env->spr[SPR_SDR1] = (target_ulong)(uintptr_t)spapr->htab
> > > -            | (spapr->htab_shift - 18);
> > > -        if (kvmppc_put_books_sregs(cpu) < 0) {
> > > -            error_report("Unable to update SDR1 in KVM");
> > > -            exit(1);
> > > +        target_ulong sdr1 = spapr_get_hpt_pointer(spapr);
> > > +        if (sdr1) {
> > > +            env->spr[SPR_SDR1] = sdr1;
> > > +            if (kvmppc_put_books_sregs(cpu) < 0) {
> > > +                error_report("Unable to update SDR1 in KVM");
> > > +                exit(1);
> > > +            }
> > >          }
> > >      }
> > >  }
> > > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> > > index 6ab8c188f381..06059b44ab40 100644
> > > --- a/hw/ppc/spapr_hcall.c
> > > +++ b/hw/ppc/spapr_hcall.c
> > > @@ -735,9 +735,10 @@ static target_ulong h_resize_hpt_commit(PowerPCCPU *cpu,
> > >  
> > >          if (kvm_enabled()) {
> > >              /* For KVM PR, update the HPT pointer */
> > > -            target_ulong sdr1 = (target_ulong)(uintptr_t)spapr->htab
> > > -                | (spapr->htab_shift - 18);
> > > -            kvmppc_update_sdr1(sdr1);
> > > +            target_ulong sdr1 = spapr_get_hpt_pointer(spapr);
> > > +            if (sdr1) {
> > > +                kvmppc_update_sdr1(sdr1);
> > > +            }
> > >          }
> > >  
> > >          pending->hpt = NULL; /* so it's not free()d */
> > > @@ -1566,9 +1567,10 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
> > >              spapr_reallocate_hpt(spapr, maxshift, &error_fatal);
> > >              if (kvm_enabled()) {
> > >                  /* For KVM PR, update the HPT pointer */
> > > -                target_ulong sdr1 = (target_ulong)(uintptr_t)spapr->htab
> > > -                    | (spapr->htab_shift - 18);
> > > -                kvmppc_update_sdr1(sdr1);
> > > +                target_ulong sdr1 = spapr_get_hpt_pointer(spapr);
> > > +                if (sdr1) {
> > > +                    kvmppc_update_sdr1(sdr1);
> > > +                }
> > >              }
> > >          }
> > >      }
> > > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > > index c1b365f56431..a1f5edc15018 100644
> > > --- a/include/hw/ppc/spapr.h
> > > +++ b/include/hw/ppc/spapr.h
> > > @@ -709,4 +709,5 @@ void spapr_do_system_reset_on_cpu(CPUState *cs, run_on_cpu_data arg);
> > >  int spapr_vcpu_id(PowerPCCPU *cpu);
> > >  PowerPCCPU *spapr_find_cpu(int vcpu_id);
> > >  
> > > +target_ulong spapr_get_hpt_pointer(sPAPRMachineState *spapr);
> > >  #endif /* HW_SPAPR_H */
> > >   
> > 
> 



-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]