[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCHv2 3/3] target-ppc: Eliminate kvmppc_kern_htab gl
From: |
Greg Kurz |
Subject: |
Re: [Qemu-devel] [PATCHv2 3/3] target-ppc: Eliminate kvmppc_kern_htab global |
Date: |
Mon, 7 Mar 2016 14:41:34 +0100 |
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>
> 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
> + * 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) {
> @@ -396,25 +401,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;
> @@ -422,9 +418,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);
> }
> }
> @@ -453,11 +449,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.
> */
> @@ -772,7 +768,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);
[Qemu-devel] [PATCHv2 3/3] target-ppc: Eliminate kvmppc_kern_htab global, David Gibson, 2016/03/06
- Re: [Qemu-devel] [PATCHv2 3/3] target-ppc: Eliminate kvmppc_kern_htab global,
Greg Kurz <=
[Qemu-devel] [PATCHv2 1/3] target-ppc: Split out SREGS get/put functions, David Gibson, 2016/03/06
Re: [Qemu-devel] [PATCHv2 1/3] target-ppc: Split out SREGS get/put functions, Greg Kurz, 2016/03/08