On Mon, Mar 07, 2016 at 02:41:34PM +0100, Greg Kurz wrote: > On Mon, 7 Mar 2016 13:26:26 +1100 > David Gibson <da...@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 <gk...@linux.vnet.ibm.com> > > Signed-off-by: David Gibson <da...@gibson.dropbear.id.au> > > Reviewed-by: Thomas Huth <th...@redhat.com> > > --- > > Typo in comment in target-ppc/mmu-hash64.c (see below). > > Apart from that, you get: > > Reviewed-by: Greg Kurz <gk...@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 <gk...@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
signature.asc
Description: PGP signature
