Re: [Qemu-devel] [PATCH 5/6] target-ppc: Remove hack for ppc_hash64_load_hpte*() with HV KVM

[Alexey Kardashevskiy <aik@ozlabs.ru>]

On 02/05/2016 01:13 PM, David Gibson wrote:
> With HV KVM, the guest's hash page table (HPT) is managed by the kernel and
> not directly accessible to QEMU.  This means that spapr->htab is NULL
> and normally env->external_htab would also be NULL for each cpu.
>
> However, that would cause ppc_hash64_load_hpte*() to do the wrong thing in
> the few cases where QEMU does need to load entries from the in-kernel HPT.
> Specifically, seeing external_htab is NULL, they would look for an HPT
> within the guest's address space instead.
>
> To stop that we have an ugly hack in the pseries machine type code to
> set external htab to (void *)1 instead.
>
> This patch removes that hack by having ppc_hash64_load_hpte*() explicitly
> check kvmppc_kern_htab instead, which makes more sense.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

To my limited knowledge of kvmppc_kern_htab&co, this looks good.

Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>



> ---
>   hw/ppc/spapr.c          | 7 -------
>   target-ppc/mmu-hash64.h | 4 ++--
>   2 files changed, 2 insertions(+), 9 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index d228375..77dd1b6 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1196,13 +1196,6 @@ static void spapr_cpu_reset(void *opaque)
>       env->spr[SPR_HIOR] = 0;
>
>       env->external_htab = (uint8_t *)spapr->htab;
> -    if (kvm_enabled() && !env->external_htab) {
> -        /*
> -         * HV KVM, set external_htab to 1 so our ppc_hash64_load_hpte*
> -         * functions do the right thing.
> -         */
> -        env->external_htab = (void *)1;
> -    }
>       env->htab_base = -1;
>       /*
>        * htab_mask is the mask used to normalize hash value to PTEG index.
> diff --git a/target-ppc/mmu-hash64.h b/target-ppc/mmu-hash64.h
> index ab0f86b..e7d9925 100644
> --- a/target-ppc/mmu-hash64.h
> +++ b/target-ppc/mmu-hash64.h
> @@ -102,7 +102,7 @@ static inline target_ulong ppc_hash64_load_hpte0(PowerPCCPU *cpu,
>       uint64_t addr;
>
>       addr = token + (index * HASH_PTE_SIZE_64);
> -    if (env->external_htab) {
> +    if (kvmppc_kern_htab || env->external_htab) {
>           return  ldq_p((const void *)(uintptr_t)addr);
>       } else {
>           return ldq_phys(CPU(cpu)->as, addr);


Out of curiosity - how does this work? ldq_p() reads from the userspace address


> @@ -116,7 +116,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 (env->external_htab) {
> +    if (kvmppc_kern_htab || env->external_htab) {
>           return  ldq_p((const void *)(uintptr_t)addr);
>       } else {
>           return ldq_phys(CPU(cpu)->as, addr);
>


-- 
Alexey