[Qemu-devel] [PATCH 0/2] target-ppc: Clean up handling of SDR1 and external HPTs

[Qemu-devel] [PATCH 0/2] target-ppc: Clean up handling of SDR1 and external HPTs

[David Gibson]

This pair of patches cleans up handling of SDR1 (master page table
pointer register for Power) and related cases with an external
(i.e. managed by qemu or KVM, rather than the guest) hash page table
(HPT).

I wouldn't push 1/2 on its own after the soft freeze, except that it
simplifies 2/2 which fixes a real regression.

David Gibson (2):
  target-ppc: Add helpers for updating a CPU's SDR1 and external HPT
  target-ppc: Eliminate kvmppc_kern_htab global

 hw/ppc/spapr.c          | 16 ++--------
 hw/ppc/spapr_hcall.c    | 10 +++----
 target-ppc/kvm.c        | 15 ++++++++++
 target-ppc/kvm_ppc.h    |  6 ++++
 target-ppc/mmu-hash64.c | 80 ++++++++++++++++++++++++++++++++++++-------------
 target-ppc/mmu-hash64.h | 11 ++++---
 target-ppc/mmu_helper.c | 13 ++++----
 7 files changed, 101 insertions(+), 50 deletions(-)

-- 
2.5.0

[Qemu-devel] [PATCH 1/2] target-ppc: Add helpers for updating a CPU's SDR1 and external HPT

[David Gibson]

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        | 15 +++++++++++++++
 target-ppc/kvm_ppc.h    |  6 ++++++
 target-ppc/mmu-hash64.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 target-ppc/mmu-hash64.h |  6 ++++++
 target-ppc/mmu_helper.c | 13 ++++++-------
 6 files changed, 77 insertions(+), 18 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index e9d4abf..a88e3af 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 d67c169..99b3231 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -2537,3 +2537,18 @@ int kvmppc_enable_hwrng(void)
 
     return kvmppc_enable_hcall(kvm_state, H_RANDOM);
 }
+
+int kvmppc_update_sdr1(PowerPCCPU *cpu)
+{
+    CPUState *cs = CPU(cpu);
+
+    if (!kvm_enabled()) {
+        return 0; /* nothing to do */
+    }
+
+    /* The normal KVM_PUT_RUNTIME_STATE doesn't include SDR1, which is
+     * why we need an explicit update for it.  KVM_PUT_RESET_STATE is
+     * overkill, but this is a pretty rare operation, so it's simpler
+     * than writing a special purpose updater */
+    return kvm_arch_put_registers(cs, KVM_PUT_RESET_STATE);
+}
diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h
index fd64c44..5564988 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_update_sdr1(PowerPCCPU *cpu);
 
 #else
 
@@ -246,6 +247,11 @@ static inline int kvmppc_enable_hwrng(void)
 {
     return -1;
 }
+
+static inline int kvmppc_update_sdr1(PowerPCCPU *cpu)
+{
+    return 0; /* nothing to do */
+}
 #endif
 
 #ifndef CONFIG_KVM
diff --git a/target-ppc/mmu-hash64.c b/target-ppc/mmu-hash64.c
index 9c58fbf..5507781 100644
--- a/target-ppc/mmu-hash64.c
+++ b/target-ppc/mmu-hash64.c
@@ -258,6 +258,48 @@ 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 (kvmppc_update_sdr1(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) */
     {
-- 
2.5.0

Re: [Qemu-devel] [PATCH 1/2] target-ppc: Add helpers for updating a CPU's SDR1 and external HPT

[Thomas Huth]

On 04.03.2016 06:35, 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        | 15 +++++++++++++++
>  target-ppc/kvm_ppc.h    |  6 ++++++
>  target-ppc/mmu-hash64.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>  target-ppc/mmu-hash64.h |  6 ++++++
>  target-ppc/mmu_helper.c | 13 ++++++-------
>  6 files changed, 77 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index e9d4abf..a88e3af 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 d67c169..99b3231 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -2537,3 +2537,18 @@ int kvmppc_enable_hwrng(void)
>  
>      return kvmppc_enable_hcall(kvm_state, H_RANDOM);
>  }
> +
> +int kvmppc_update_sdr1(PowerPCCPU *cpu)
> +{
> +    CPUState *cs = CPU(cpu);
> +
> +    if (!kvm_enabled()) {
> +        return 0; /* nothing to do */
> +    }

Since you're updating more than SDR1 below ... Could you maybe add a
"assert(cs->kvm_vcpu_dirty)" here, just to make sure that nobody
accidentally ever calls this function without synchronizing the
registers first?

> +    /* The normal KVM_PUT_RUNTIME_STATE doesn't include SDR1, which is
> +     * why we need an explicit update for it.  KVM_PUT_RESET_STATE is
> +     * overkill, but this is a pretty rare operation, so it's simpler
> +     * than writing a special purpose updater */
> +    return kvm_arch_put_registers(cs, KVM_PUT_RESET_STATE);
> +}

That indeed looks like a big overkill ... what about extracting the
block from kvm_arch_put_registers() which sets the the "struct
kvm_sregs" via KVM_SET_SREGS into a separate function, and then call
that function here instead?
kvm_arch_put_registers() is IMHO way too big anyway, so separating some
code into external functions there would improve readability there, too.

Apart from that, the patch looks right to me.

 Thomas

Re: [Qemu-devel] [PATCH 1/2] target-ppc: Add helpers for updating a CPU's SDR1 and external HPT

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4501 bytes --]

On Fri, Mar 04, 2016 at 10:59:17AM +0100, Thomas Huth wrote:
> On 04.03.2016 06:35, 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        | 15 +++++++++++++++
> >  target-ppc/kvm_ppc.h    |  6 ++++++
> >  target-ppc/mmu-hash64.c | 42 ++++++++++++++++++++++++++++++++++++++++++
> >  target-ppc/mmu-hash64.h |  6 ++++++
> >  target-ppc/mmu_helper.c | 13 ++++++-------
> >  6 files changed, 77 insertions(+), 18 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index e9d4abf..a88e3af 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 d67c169..99b3231 100644
> > --- a/target-ppc/kvm.c
> > +++ b/target-ppc/kvm.c
> > @@ -2537,3 +2537,18 @@ int kvmppc_enable_hwrng(void)
> >  
> >      return kvmppc_enable_hcall(kvm_state, H_RANDOM);
> >  }
> > +
> > +int kvmppc_update_sdr1(PowerPCCPU *cpu)
> > +{
> > +    CPUState *cs = CPU(cpu);
> > +
> > +    if (!kvm_enabled()) {
> > +        return 0; /* nothing to do */
> > +    }
> 
> Since you're updating more than SDR1 below ... Could you maybe add a
> "assert(cs->kvm_vcpu_dirty)" here, just to make sure that nobody
> accidentally ever calls this function without synchronizing the
> registers first?

Hmm.. that's a good idea, but it doesn't work so well with the one below.

> 
> > +    /* The normal KVM_PUT_RUNTIME_STATE doesn't include SDR1, which is
> > +     * why we need an explicit update for it.  KVM_PUT_RESET_STATE is
> > +     * overkill, but this is a pretty rare operation, so it's simpler
> > +     * than writing a special purpose updater */
> > +    return kvm_arch_put_registers(cs, KVM_PUT_RESET_STATE);
> > +}
> 
> That indeed looks like a big overkill ... what about extracting the
> block from kvm_arch_put_registers() which sets the the "struct
> kvm_sregs" via KVM_SET_SREGS into a separate function, and then call
> that function here instead?
> kvm_arch_put_registers() is IMHO way too big anyway, so separating some
> code into external functions there would improve readability there, too.

Yeah, fair enough.  I'll split that up in a prelim patch for v2.

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[Qemu-devel] [PATCH 2/2] target-ppc: Eliminate kvmppc_kern_htab global

[David Gibson]

fa48b43 "target-ppc: Remove hack for ppc_hash64_load_hpte*() with HV KVM"
purports to remove a hack in the handling of hash page tables (HPTs)
managed by KVM instead of qemu.  However, it actually went in the wrong
direction.

That patch requires anything looking for an external HPT (that is one not
managed by the guest itself) to check both env->external_htab (for a qemu
managed HPT) and kvmppc_kern_htab (for a KVM managed HPT).  That's a
problem because kvmppc_kern_htab is local to mmu-hash64.c, but some places
which need to check for an external HPT are outside that, such as
kvm_arch_get_registers().  The latter was subtly broken by the earlier
patch such that gdbstub can no longer access memory.

Basically a KVM managed HPT is much more like a qemu managed HPT than it is
like a guest managed HPT, so the original "hack" was actually on the right
track.

This partially reverts fa48b43, so we again mark a KVM managed external HPT
by putting a special but non-NULL value in env->external_htab.  It then
goes further, using that marker to eliminate the kvmppc_kern_htab global
entirely.  The ppc_hash64_set_external_hpt() helper function is extended
to set that marker if passed a NULL value (if you're setting an external
HPT, but don't have an actual HPT to set, the assumption is that it must
be a KVM managed HPT).

This also has some flow-on changes to the HPT access helpers, required by
the above changes.

Reported-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr.c          |  3 +--
 hw/ppc/spapr_hcall.c    | 10 +++++-----
 target-ppc/mmu-hash64.c | 40 ++++++++++++++++++----------------------
 target-ppc/mmu-hash64.h |  9 +++------
 4 files changed, 27 insertions(+), 35 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index a88e3af..dc1f889 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1091,7 +1091,7 @@ static void spapr_reallocate_hpt(sPAPRMachineState *spapr, int shift,
         }
 
         spapr->htab_shift = shift;
-        kvmppc_kern_htab = true;
+        spapr->htab = NULL;
     } else {
         /* kernel-side HPT not needed, allocate in userspace instead */
         size_t size = 1ULL << shift;
@@ -1106,7 +1106,6 @@ static void spapr_reallocate_hpt(sPAPRMachineState *spapr, int shift,
 
         memset(spapr->htab, 0, size);
         spapr->htab_shift = shift;
-        kvmppc_kern_htab = false;
 
         for (i = 0; i < size / HASH_PTE_SIZE_64; i++) {
             DIRTY_HPTE(HPTE(spapr->htab, i));
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 1733482..b2b1b93 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -122,17 +122,17 @@ static target_ulong h_enter(PowerPCCPU *cpu, sPAPRMachineState *spapr,
                 break;
             }
         }
-        ppc_hash64_stop_access(token);
+        ppc_hash64_stop_access(cpu, token);
         if (index == 8) {
             return H_PTEG_FULL;
         }
     } else {
         token = ppc_hash64_start_access(cpu, pte_index);
         if (ppc_hash64_load_hpte0(cpu, token, 0) & HPTE64_V_VALID) {
-            ppc_hash64_stop_access(token);
+            ppc_hash64_stop_access(cpu, token);
             return H_PTEG_FULL;
         }
-        ppc_hash64_stop_access(token);
+        ppc_hash64_stop_access(cpu, token);
     }
 
     ppc_hash64_store_hpte(cpu, pte_index + index,
@@ -165,7 +165,7 @@ static RemoveResult remove_hpte(PowerPCCPU *cpu, target_ulong ptex,
     token = ppc_hash64_start_access(cpu, ptex);
     v = ppc_hash64_load_hpte0(cpu, token, 0);
     r = ppc_hash64_load_hpte1(cpu, token, 0);
-    ppc_hash64_stop_access(token);
+    ppc_hash64_stop_access(cpu, token);
 
     if ((v & HPTE64_V_VALID) == 0 ||
         ((flags & H_AVPN) && (v & ~0x7fULL) != avpn) ||
@@ -288,7 +288,7 @@ static target_ulong h_protect(PowerPCCPU *cpu, sPAPRMachineState *spapr,
     token = ppc_hash64_start_access(cpu, pte_index);
     v = ppc_hash64_load_hpte0(cpu, token, 0);
     r = ppc_hash64_load_hpte1(cpu, token, 0);
-    ppc_hash64_stop_access(token);
+    ppc_hash64_stop_access(cpu, token);
 
     if ((v & HPTE64_V_VALID) == 0 ||
         ((flags & H_AVPN) && (v & ~0x7fULL) != avpn)) {
diff --git a/target-ppc/mmu-hash64.c b/target-ppc/mmu-hash64.c
index 5507781..76510c3 100644
--- a/target-ppc/mmu-hash64.c
+++ b/target-ppc/mmu-hash64.c
@@ -36,10 +36,11 @@
 #endif
 
 /*
- * Used to indicate whether we have allocated htab in the
- * host kernel
+ * Used to indicate that a CPU has it's hash page table (HPT) managed
+ * within the host kernel
  */
-bool kvmppc_kern_htab;
+#define MMU_HASH64_KVM_MANAGED_HPT      ((void *)-1)
+
 /*
  * SLB handling
  */
@@ -283,7 +284,11 @@ void ppc_hash64_set_external_hpt(PowerPCCPU *cpu, void *hpt, int shift,
 
     cpu_synchronize_state(CPU(cpu));
 
-    env->external_htab = hpt;
+    if (hpt) {
+        env->external_htab = hpt;
+    } else {
+        env->external_htab = MMU_HASH64_KVM_MANAGED_HPT;
+    }
     ppc_hash64_set_sdr1(cpu, (target_ulong)(uintptr_t)hpt | (shift - 18),
                         &local_err);
     if (local_err) {
@@ -395,25 +400,16 @@ uint64_t ppc_hash64_start_access(PowerPCCPU *cpu, target_ulong pte_index)
     hwaddr pte_offset;
 
     pte_offset = pte_index * HASH_PTE_SIZE_64;
-    if (kvmppc_kern_htab) {
+    if (cpu->env.external_htab == MMU_HASH64_KVM_MANAGED_HPT) {
         /*
          * HTAB is controlled by KVM. Fetch the PTEG into a new buffer.
          */
         token = kvmppc_hash64_read_pteg(cpu, pte_index);
-        if (token) {
-            return token;
-        }
+    } else if (cpu->env.external_htab) {
         /*
-         * pteg read failed, even though we have allocated htab via
-         * kvmppc_reset_htab.
+         * HTAB is controlled by QEMU. Just point to the internally
+         * accessible PTEG.
          */
-        return 0;
-    }
-    /*
-     * HTAB is controlled by QEMU. Just point to the internally
-     * accessible PTEG.
-     */
-    if (cpu->env.external_htab) {
         token = (uint64_t)(uintptr_t) cpu->env.external_htab + pte_offset;
     } else if (cpu->env.htab_base) {
         token = cpu->env.htab_base + pte_offset;
@@ -421,9 +417,9 @@ uint64_t ppc_hash64_start_access(PowerPCCPU *cpu, target_ulong pte_index)
     return token;
 }
 
-void ppc_hash64_stop_access(uint64_t token)
+void ppc_hash64_stop_access(PowerPCCPU *cpu, uint64_t token)
 {
-    if (kvmppc_kern_htab) {
+    if (cpu->env.external_htab == MMU_HASH64_KVM_MANAGED_HPT) {
         kvmppc_hash64_free_pteg(token);
     }
 }
@@ -452,11 +448,11 @@ static hwaddr ppc_hash64_pteg_search(PowerPCCPU *cpu, hwaddr hash,
             && HPTE64_V_COMPARE(pte0, ptem)) {
             pte->pte0 = pte0;
             pte->pte1 = pte1;
-            ppc_hash64_stop_access(token);
+            ppc_hash64_stop_access(cpu, token);
             return (pte_index + i) * HASH_PTE_SIZE_64;
         }
     }
-    ppc_hash64_stop_access(token);
+    ppc_hash64_stop_access(cpu, token);
     /*
      * We didn't find a valid entry.
      */
@@ -771,7 +767,7 @@ void ppc_hash64_store_hpte(PowerPCCPU *cpu,
 {
     CPUPPCState *env = &cpu->env;
 
-    if (kvmppc_kern_htab) {
+    if (env->external_htab == MMU_HASH64_KVM_MANAGED_HPT) {
         kvmppc_hash64_write_pte(env, pte_index, pte0, pte1);
         return;
     }
diff --git a/target-ppc/mmu-hash64.h b/target-ppc/mmu-hash64.h
index 138cd7e..9bf8b9b 100644
--- a/target-ppc/mmu-hash64.h
+++ b/target-ppc/mmu-hash64.h
@@ -90,16 +90,13 @@ unsigned ppc_hash64_hpte_page_shift_noslb(PowerPCCPU *cpu,
 #define HPTE64_V_1TB_SEG        0x4000000000000000ULL
 #define HPTE64_V_VRMA_MASK      0x4001ffffff000000ULL
 
-
-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);
+void ppc_hash64_stop_access(PowerPCCPU *cpu, uint64_t token);
 
 static inline target_ulong ppc_hash64_load_hpte0(PowerPCCPU *cpu,
                                                  uint64_t token, int index)
@@ -108,7 +105,7 @@ static inline target_ulong ppc_hash64_load_hpte0(PowerPCCPU *cpu,
     uint64_t addr;
 
     addr = token + (index * HASH_PTE_SIZE_64);
-    if (kvmppc_kern_htab || env->external_htab) {
+    if (env->external_htab) {
         return  ldq_p((const void *)(uintptr_t)addr);
     } else {
         return ldq_phys(CPU(cpu)->as, addr);
@@ -122,7 +119,7 @@ static inline target_ulong ppc_hash64_load_hpte1(PowerPCCPU *cpu,
     uint64_t addr;
 
     addr = token + (index * HASH_PTE_SIZE_64) + HASH_PTE_SIZE_64/2;
-    if (kvmppc_kern_htab || env->external_htab) {
+    if (env->external_htab) {
         return  ldq_p((const void *)(uintptr_t)addr);
     } else {
         return ldq_phys(CPU(cpu)->as, addr);
-- 
2.5.0

Re: [Qemu-devel] [PATCH 2/2] target-ppc: Eliminate kvmppc_kern_htab global

[Thomas Huth]

On 04.03.2016 06:35, David Gibson wrote:
> fa48b43 "target-ppc: Remove hack for ppc_hash64_load_hpte*() with HV KVM"
> purports to remove a hack in the handling of hash page tables (HPTs)
> managed by KVM instead of qemu.  However, it actually went in the wrong
> direction.
> 
> That patch requires anything looking for an external HPT (that is one not
> managed by the guest itself) to check both env->external_htab (for a qemu
> managed HPT) and kvmppc_kern_htab (for a KVM managed HPT).  That's a
> problem because kvmppc_kern_htab is local to mmu-hash64.c, but some places
> which need to check for an external HPT are outside that, such as
> kvm_arch_get_registers().  The latter was subtly broken by the earlier
> patch such that gdbstub can no longer access memory.
> 
> Basically a KVM managed HPT is much more like a qemu managed HPT than it is
> like a guest managed HPT, so the original "hack" was actually on the right
> track.
> 
> This partially reverts fa48b43, so we again mark a KVM managed external HPT
> by putting a special but non-NULL value in env->external_htab.  It then
> goes further, using that marker to eliminate the kvmppc_kern_htab global
> entirely.  The ppc_hash64_set_external_hpt() helper function is extended
> to set that marker if passed a NULL value (if you're setting an external
> HPT, but don't have an actual HPT to set, the assumption is that it must
> be a KVM managed HPT).
> 
> This also has some flow-on changes to the HPT access helpers, required by
> the above changes.
> 
> Reported-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr.c          |  3 +--
>  hw/ppc/spapr_hcall.c    | 10 +++++-----
>  target-ppc/mmu-hash64.c | 40 ++++++++++++++++++----------------------
>  target-ppc/mmu-hash64.h |  9 +++------
>  4 files changed, 27 insertions(+), 35 deletions(-)

Patch looks fine to me.

Reviewed-by: Thomas Huth <thuth@redhat.com>