Re: [Qemu-devel] [PATCHv2 3/3] target-ppc: Eliminate kvmppc_kern_htab global

[David Gibson <david@gibson.dropbear.id.au>]

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

On Mon, Mar 07, 2016 at 02:41:34PM +0100, Greg Kurz wrote:
> On Mon,  7 Mar 2016 13:26:26 +1100
> David Gibson <david@gibson.dropbear.id.au> 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>
> > Reviewed-by: Thomas Huth <thuth@redhat.com>
> > ---
> 
> Typo in comment in target-ppc/mmu-hash64.c (see below).
> 
> Apart from that, you get:
> 
> Reviewed-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> 
> and also...
> 
> without this patch:
> 
> 0x00000000100009b8 in main (argc=<error reading variable: Cannot access memory at address 0x3fffc03ce270>, argv=<error reading variable: Cannot access memory at address 0x3fffc03ce278>) at fp.c:11
> 
> with this patch:
> 
> 0x00000000100009b8 in main (argc=4, argv=0x3fffc7fcfd18) at fp.c:11
> 
> Just to be sure, I've also tested with TCG: no regression.
> 
> Thanks for the fix !
> 
> Tested-by: Greg Kurz <gkurz@linux.vnet.ibm.com>

Thanks.

> >  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 72a018b..43708a2 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 7b5200b..a9ae0b3 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
> 
> s/it's/its

Oh dear.  That mispelling really annoys me, so it's an embarrassing
error.  Thanks for the catch.

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