[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-ppc] [PATCHv2 3/3] target-ppc: Eliminate kvmppc_kern_htab glob
From: |
David Gibson |
Subject: |
Re: [Qemu-ppc] [PATCHv2 3/3] target-ppc: Eliminate kvmppc_kern_htab global |
Date: |
Tue, 8 Mar 2016 11:36:11 +1100 |
User-agent: |
Mutt/1.5.24 (2015-08-30) |
On Mon, Mar 07, 2016 at 02:41:34PM +0100, Greg Kurz wrote:
> On Mon, 7 Mar 2016 13:26:26 +1100
> David Gibson <address@hidden> 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 <address@hidden>
> > Signed-off-by: David Gibson <address@hidden>
> > Reviewed-by: Thomas Huth <address@hidden>
> > ---
>
> Typo in comment in target-ppc/mmu-hash64.c (see below).
>
> Apart from that, you get:
>
> Reviewed-by: Greg Kurz <address@hidden>
>
> 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 <address@hidden>
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
- [Qemu-ppc] [PATCHv2 2/3] target-ppc: Add helpers for updating a CPU's SDR1 and external HPT, (continued)
[Qemu-ppc] [PATCHv2 3/3] target-ppc: Eliminate kvmppc_kern_htab global, David Gibson, 2016/03/06
[Qemu-ppc] [PATCHv2 1/3] target-ppc: Split out SREGS get/put functions, David Gibson, 2016/03/06
Re: [Qemu-ppc] [PATCHv2 1/3] target-ppc: Split out SREGS get/put functions, Greg Kurz, 2016/03/08