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. 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 <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) */ >>> { >>> >> >