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