Re: [Qemu-devel] [PATCH 1/2] target-ppc: Add helpers for updating a CPU's SDR1 and external HPT

[David Gibson]

On Fri, Mar 04, 2016 at 10:59:17AM +0100, Thomas Huth wrote:
> On 04.03.2016 06:35, David Gibson wrote:
> > When a Power cpu with 64-bit hash MMU has it's hash page table (HPT)
> > pointer updated by a write to the SDR1 register we need to update some
> > derived variables.  Likewise, when the cpu is configured for an external
> > HPT (one not in the guest memory space) some derived variables need to be
> > updated.
> > 
> > Currently the logic for this is (partially) duplicated in ppc_store_sdr1()
> > and in spapr_cpu_reset().  In future we're going to need it in some other
> > places, so make some common helpers for this update.
> > 
> > In addition the new ppc_hash64_set_external_hpt() helper also updates
> > SDR1 in KVM - it's not updated by the normal runtime KVM <-> qemu CPU
> > synchronization.  In a sense this belongs logically in the
> > ppc_hash64_set_sdr1() helper, but that is called from
> > kvm_arch_get_registers() so can't itself call cpu_synchronize_state()
> > without infinite recursion.  In practice this doesn't matter because
> > the only other caller is TCG specific.
> > 
> > Currently there aren't situations where updating SDR1 at runtime in KVM
> > matters, but there are going to be in future.
> > 
> > Signed-off-by: David Gibson <address@hidden>
> > ---
> >  hw/ppc/spapr.c          | 13 ++-----------
> >  target-ppc/kvm.c        | 15 +++++++++++++++
> >  target-ppc/kvm_ppc.h    |  6 ++++++
> >  target-ppc/mmu-hash64.c | 42 ++++++++++++++++++++++++++++++++++++++++++
> >  target-ppc/mmu-hash64.h |  6 ++++++
> >  target-ppc/mmu_helper.c | 13 ++++++-------
> >  6 files changed, 77 insertions(+), 18 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index e9d4abf..a88e3af 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1196,17 +1196,8 @@ static void spapr_cpu_reset(void *opaque)
> >  
> >      env->spr[SPR_HIOR] = 0;
> >  
> > -    env->external_htab = (uint8_t *)spapr->htab;
> > -    env->htab_base = -1;
> > -    /*
> > -     * htab_mask is the mask used to normalize hash value to PTEG index.
> > -     * htab_shift is log2 of hash table size.
> > -     * We have 8 hpte per group, and each hpte is 16 bytes.
> > -     * ie have 128 bytes per hpte entry.
> > -     */
> > -    env->htab_mask = (1ULL << (spapr->htab_shift - 7)) - 1;
> > -    env->spr[SPR_SDR1] = (target_ulong)(uintptr_t)spapr->htab |
> > -        (spapr->htab_shift - 18);
> > +    ppc_hash64_set_external_hpt(cpu, spapr->htab, spapr->htab_shift,
> > +                                &error_fatal);
> >  }
> >  
> >  static void spapr_create_nvram(sPAPRMachineState *spapr)
> > diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> > index d67c169..99b3231 100644
> > --- a/target-ppc/kvm.c
> > +++ b/target-ppc/kvm.c
> > @@ -2537,3 +2537,18 @@ int kvmppc_enable_hwrng(void)
> >  
> >      return kvmppc_enable_hcall(kvm_state, H_RANDOM);
> >  }
> > +
> > +int kvmppc_update_sdr1(PowerPCCPU *cpu)
> > +{
> > +    CPUState *cs = CPU(cpu);
> > +
> > +    if (!kvm_enabled()) {
> > +        return 0; /* nothing to do */
> > +    }
> 
> Since you're updating more than SDR1 below ... Could you maybe add a
> "assert(cs->kvm_vcpu_dirty)" here, just to make sure that nobody
> accidentally ever calls this function without synchronizing the
> registers first?

Hmm.. that's a good idea, but it doesn't work so well with the one below.

> 
> > +    /* The normal KVM_PUT_RUNTIME_STATE doesn't include SDR1, which is
> > +     * why we need an explicit update for it.  KVM_PUT_RESET_STATE is
> > +     * overkill, but this is a pretty rare operation, so it's simpler
> > +     * than writing a special purpose updater */
> > +    return kvm_arch_put_registers(cs, KVM_PUT_RESET_STATE);
> > +}
> 
> That indeed looks like a big overkill ... what about extracting the
> block from kvm_arch_put_registers() which sets the the "struct
> kvm_sregs" via KVM_SET_SREGS into a separate function, and then call
> that function here instead?
> kvm_arch_put_registers() is IMHO way too big anyway, so separating some
> code into external functions there would improve readability there, too.

Yeah, fair enough.  I'll split that up in a prelim patch for v2.

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

signature.asc
Description: PGP signature