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

[Greg Kurz]

Hi Laurent,

On Thu, 24 Mar 2016 09:41:59 +0100
Laurent Vivier <address@hidden> wrote:

> On 24/03/2016 06:35, David Gibson wrote:
> > On Tue, Mar 22, 2016 at 05:33:45PM +0100, Laurent Vivier wrote:  
> >> Hi David,
> >>
> >> using kvm-unit-tests, I've found a side effect of your patches: the MSR
> >> is cleared (and perhaps some others).
> >>
> >> I was trying to test my patch on top of QEMU master:
> >>
> >> "ppc64: set MSR_SF bit"
> >> http://patchwork.ozlabs.org/patch/598198/
> >>
> >> and it was not working anymore.
> >>
> >> By bisecting, I've found this commit.
> >>
> >> I think "cpu_synchronize_state()" in "ppc_hash64_set_external_hpt()"
> >> restores the MSR from KVM whereas the one from QEMU has not been saved,
> >> because cpu_synchronize_all_post_reset() is called later.
> >>
> >> So it is cleared.
> >>
> >> You can test this by applying the MSR_SF patch and using the "emulator"
> >> test of kvm-unit-tests (the "emulator: 64bit" test case)  
> > 
> > Ugh, you're right of course.  But, I'm having a bit of trouble
> > figuring out how to fix it propertly.  
> 
> Perhaps you can just remove the cpu_synchronize_state()?
> 
> As this is in the reset phase (spapr_cpu_reset()), I think the content
> of the QEMU side registers are correct, and they will be synchronized at
> the end of the reset phase.
> 

I think this is right because qemu_system_reset() is called either:
- during system startup:

    cpu_synchronize_all_post_init();   => push regs to KVM, kvm_vcpu_dirty = 
false
...
    qemu_system_reset(VMRESET_SILENT);

QEMU still have good values for the registers though, since KVM hasn't run yet.

But ppc_hash64_set_external_hpt()->cpu_synchronize_state() will indeed pull
the registers from KVM and clear MSR_SF, since we have kvm_vcpu_dirty == false.

- or from main_loop_should_exit() and we have:

        cpu_synchronize_all_states();
        qemu_system_reset(VMRESET_REPORT);
and
        cpu_synchronize_all_states();
        qemu_system_reset(VMRESET_SILENT);

In which case ppc_hash64_set_external_hpt()->cpu_synchronize_state() isn't
needed.

Makes sense ?

Cheers.

--
Greg

> You can see in spapr_cpu_reset(), SPR_HIOR is updated without prior
> sync. I think spapr_cpu_reset() is called by qemu_device_reset() in
> qemu_system_reset(), which is always followed by a
> cpu_synchronize_all_post_reset(), which will call
> do_kvm_cpu_synchronize_post_reset() (kvm_arch_put_registers(cpu,
> KVM_PUT_RESET_STATE)) at the end.
> 
> 
> Laurent
> 
> >> On 07/03/2016 03:26, 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        |  2 +-
> >>>  target-ppc/kvm_ppc.h    |  6 ++++++
> >>>  target-ppc/mmu-hash64.c | 43 +++++++++++++++++++++++++++++++++++++++++++
> >>>  target-ppc/mmu-hash64.h |  6 ++++++
> >>>  target-ppc/mmu_helper.c | 13 ++++++-------
> >>>  6 files changed, 64 insertions(+), 19 deletions(-)
> >>>
> >>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >>> index 64c4acc..72a018b 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 8a762e8..380dff6 100644
> >>> --- a/target-ppc/kvm.c
> >>> +++ b/target-ppc/kvm.c
> >>> @@ -867,7 +867,7 @@ static int kvm_put_vpa(CPUState *cs)
> >>>  }
> >>>  #endif /* TARGET_PPC64 */
> >>>  
> >>> -static int kvmppc_put_books_sregs(PowerPCCPU *cpu)
> >>> +int kvmppc_put_books_sregs(PowerPCCPU *cpu)
> >>>  {
> >>>      CPUPPCState *env = &cpu->env;
> >>>      struct kvm_sregs sregs;
> >>> diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h
> >>> index fd64c44..fc79312 100644
> >>> --- a/target-ppc/kvm_ppc.h
> >>> +++ b/target-ppc/kvm_ppc.h
> >>> @@ -55,6 +55,7 @@ void kvmppc_hash64_write_pte(CPUPPCState *env, 
> >>> target_ulong pte_index,
> >>>                               target_ulong pte0, target_ulong pte1);
> >>>  bool kvmppc_has_cap_fixup_hcalls(void);
> >>>  int kvmppc_enable_hwrng(void);
> >>> +int kvmppc_put_books_sregs(PowerPCCPU *cpu);
> >>>  
> >>>  #else
> >>>  
> >>> @@ -246,6 +247,11 @@ static inline int kvmppc_enable_hwrng(void)
> >>>  {
> >>>      return -1;
> >>>  }
> >>> +
> >>> +static inline int kvmppc_put_books_sregs(PowerPCCPU *cpu)
> >>> +{
> >>> +    abort();
> >>> +}
> >>>  #endif
> >>>  
> >>>  #ifndef CONFIG_KVM
> >>> diff --git a/target-ppc/mmu-hash64.c b/target-ppc/mmu-hash64.c
> >>> index 9c58fbf..7b5200b 100644
> >>> --- a/target-ppc/mmu-hash64.c
> >>> +++ b/target-ppc/mmu-hash64.c
> >>> @@ -258,6 +258,49 @@ target_ulong helper_load_slb_vsid(CPUPPCState *env, 
> >>> target_ulong rb)
> >>>  /*
> >>>   * 64-bit hash table MMU handling
> >>>   */
> >>> +void ppc_hash64_set_sdr1(PowerPCCPU *cpu, target_ulong value,
> >>> +                         Error **errp)
> >>> +{
> >>> +    CPUPPCState *env = &cpu->env;
> >>> +    target_ulong htabsize = value & SDR_64_HTABSIZE;
> >>> +
> >>> +    env->spr[SPR_SDR1] = value;
> >>> +    if (htabsize > 28) {
> >>> +        error_setg(errp,
> >>> +                   "Invalid HTABSIZE 0x" TARGET_FMT_lx" stored in SDR1",
> >>> +                   htabsize);
> >>> +        htabsize = 28;
> >>> +    }
> >>> +    env->htab_mask = (1ULL << (htabsize + 18 - 7)) - 1;
> >>> +    env->htab_base = value & SDR_64_HTABORG;
> >>> +}
> >>> +
> >>> +void ppc_hash64_set_external_hpt(PowerPCCPU *cpu, void *hpt, int shift,
> >>> +                                 Error **errp)
> >>> +{
> >>> +    CPUPPCState *env = &cpu->env;
> >>> +    Error *local_err = NULL;
> >>> +
> >>> +    cpu_synchronize_state(CPU(cpu));
> >>> +
> >>> +    env->external_htab = hpt;
> >>> +    ppc_hash64_set_sdr1(cpu, (target_ulong)(uintptr_t)hpt | (shift - 18),
> >>> +                        &local_err);
> >>> +    if (local_err) {
> >>> +        error_propagate(errp, local_err);
> >>> +        return;
> >>> +    }
> >>> +
> >>> +    /* Not strictly necessary, but makes it clearer that an external
> >>> +     * htab is in use when debugging */
> >>> +    env->htab_base = -1;
> >>> +
> >>> +    if (kvm_enabled()) {
> >>> +        if (kvmppc_put_books_sregs(cpu) < 0) {
> >>> +            error_setg(errp, "Unable to update SDR1 in KVM");
> >>> +        }
> >>> +    }
> >>> +}
> >>>  
> >>>  static int ppc_hash64_pte_prot(PowerPCCPU *cpu,
> >>>                                 ppc_slb_t *slb, ppc_hash_pte64_t pte)
> >>> diff --git a/target-ppc/mmu-hash64.h b/target-ppc/mmu-hash64.h
> >>> index e7d9925..138cd7e 100644
> >>> --- a/target-ppc/mmu-hash64.h
> >>> +++ b/target-ppc/mmu-hash64.h
> >>> @@ -92,6 +92,12 @@ unsigned ppc_hash64_hpte_page_shift_noslb(PowerPCCPU 
> >>> *cpu,
> >>>  
> >>>  
> >>>  extern bool kvmppc_kern_htab;
> >>> +
> >>> +void ppc_hash64_set_sdr1(PowerPCCPU *cpu, target_ulong value,
> >>> +                         Error **errp);
> >>> +void ppc_hash64_set_external_hpt(PowerPCCPU *cpu, void *hpt, int shift,
> >>> +                                 Error **errp);
> >>> +
> >>>  uint64_t ppc_hash64_start_access(PowerPCCPU *cpu, target_ulong 
> >>> pte_index);
> >>>  void ppc_hash64_stop_access(uint64_t token);
> >>>  
> >>> diff --git a/target-ppc/mmu_helper.c b/target-ppc/mmu_helper.c
> >>> index e5ec8d6..fcb2cc5 100644
> >>> --- a/target-ppc/mmu_helper.c
> >>> +++ b/target-ppc/mmu_helper.c
> >>> @@ -2005,15 +2005,14 @@ void ppc_store_sdr1(CPUPPCState *env, 
> >>> target_ulong value)
> >>>      env->spr[SPR_SDR1] = value;
> >>>  #if defined(TARGET_PPC64)
> >>>      if (env->mmu_model & POWERPC_MMU_64) {
> >>> -        target_ulong htabsize = value & SDR_64_HTABSIZE;
> >>> +        PowerPCCPU *cpu = ppc_env_get_cpu(env);
> >>> +        Error *local_err = NULL;
> >>>  
> >>> -        if (htabsize > 28) {
> >>> -            fprintf(stderr, "Invalid HTABSIZE 0x" TARGET_FMT_lx
> >>> -                    " stored in SDR1\n", htabsize);
> >>> -            htabsize = 28;
> >>> +        ppc_hash64_set_sdr1(cpu, value, &local_err);
> >>> +        if (local_err) {
> >>> +            error_report_err(local_err);
> >>> +            error_free(local_err);
> >>>          }
> >>> -        env->htab_mask = (1ULL << (htabsize + 18 - 7)) - 1;
> >>> -        env->htab_base = value & SDR_64_HTABORG;
> >>>      } else
> >>>  #endif /* defined(TARGET_PPC64) */
> >>>      {
> >>>  
> >>  
> >   
>