Hi Laurent, On Thu, 24 Mar 2016 09:41:59 +0100 Laurent Vivier <lviv...@redhat.com> 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 <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) */ > >>> { > >>> > >> > > >