diff mbox

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

Message ID 1457317586-15122-3-git-send-email-david@gibson.dropbear.id.au
State New, archived
Headers show

Commit Message

David Gibson March 7, 2016, 2:26 a.m. UTC
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 <david@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(-)

Comments

Greg Kurz March 7, 2016, 1:37 p.m. UTC | #1
On Mon,  7 Mar 2016 13:26:25 +1100
David Gibson <david@gibson.dropbear.id.au> 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 <david@gibson.dropbear.id.au>
> ---

Reviewed-by: Greg Kurz <gkurz@linux.vnet.ibm.com>

>  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) */
>      {
Thomas Huth March 7, 2016, 3:56 p.m. UTC | #2
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 <david@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(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>
Laurent Vivier March 22, 2016, 4:33 p.m. UTC | #3
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 <david@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 March 24, 2016, 5:35 a.m. UTC | #4
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 <david@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) */
> >      {
> > 
>
Laurent Vivier March 24, 2016, 8:41 a.m. UTC | #5
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 <david@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) */
>>>      {
>>>
>>
>
Greg Kurz March 25, 2016, 9:13 a.m. UTC | #6
Hi Laurent,

On Thu, 24 Mar 2016 09:41:59 +0100
Laurent Vivier <lvivier@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 <david@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 March 29, 2016, 6:39 a.m. UTC | #7
On Fri, Mar 25, 2016 at 10:13:59AM +0100, Greg Kurz wrote:
> Hi Laurent,
> 
> On Thu, 24 Mar 2016 09:41:59 +0100
> Laurent Vivier <lvivier@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 ?

Ugh.  So, I think this is the simplest short term fix, and I've
applied a change to ppc-for-2.6 removing the cpu_synchronize_state().


But, longer term I don't think this is really right.  Removing the
cpu_synchronize_state() corrects using this in the reset path, but
breaks it if it is used outside the reset path.  At the moment we
don't do that, but I have code which will do so in my HPT resizing
branch.  Obviously I can fix that by putting cpu_synchronize_state()
in the caller, but it still seems a bit odd having a put_sregs().

Really, it seems to me that kvm_vcpu_dirty should be set to true
(either via cpu_synchronize_state() or directly) *before* the core CPU
reset path, in the same way that it is set to true in kvm_init_vcpu().
diff mbox

Patch

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