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) 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 <da...@gibson.dropbear.id.au> > --- > 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) */ > { >