Re: [Qemu-devel] [PATCH 1/2] ppc: Move HPTE size parsing code to target-ppc helper (and add 64kiB pages)

[Alexey Kardashevskiy <aik@ozlabs.ru>]

On 12/21/2015 01:41 PM, David Gibson wrote:
> The h_enter() hypercall implementation in spapr_hcall.c contains some code
> to determine the page size of the newly inserted hash PTE.  This is
> surprisingly complex in the general case, because POWER CPUs can have
> different implementation dependent ways of encoding page sizes.  The
> current version assumes POWER7 or POWER8 and doesn't support all the

s/and doesn't/doesn't/ ?


> possibilities of even those (but this is advertised correctly in the device
> tree and so works with guests).
>
> To support the possibility of future CPUs with different options, however,
> this really belongs in the core ppc mmu code, rather than the spapr
> ("pseries" machine type) specific code.
>
> So, move this code to a helper in target-ppc.
>
> At the same time, uncomment some code to allow 64kiB pages.  At the time
> that was added, I believe other parts of the ppc mmu code didn't handle
> 64kiB pages, but that's since been fixed.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>   hw/ppc/spapr_hcall.c    | 26 ++++++++------------------
>   target-ppc/mmu-hash64.c | 31 +++++++++++++++++++++++++++++++
>   target-ppc/mmu-hash64.h |  3 +++
>   3 files changed, 42 insertions(+), 18 deletions(-)
>
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index cebceea..c346e97 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -93,28 +93,18 @@ static target_ulong h_enter(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>       target_ulong pte_index = args[1];
>       target_ulong pteh = args[2];
>       target_ulong ptel = args[3];
> -    target_ulong page_shift = 12;
> +    int page_shift;
> +    uint64_t avpnm;
>       target_ulong raddr;
>       target_ulong index;
>       uint64_t token;
>
> -    /* only handle 4k and 16M pages for now */
> -    if (pteh & HPTE64_V_LARGE) {
> -#if 0 /* We don't support 64k pages yet */
> -        if ((ptel & 0xf000) == 0x1000) {
> -            /* 64k page */
> -        } else
> -#endif
> -        if ((ptel & 0xff000) == 0) {
> -            /* 16M page */
> -            page_shift = 24;
> -            /* lowest AVA bit must be 0 for 16M pages */
> -            if (pteh & 0x80) {
> -                return H_PARAMETER;
> -            }
> -        } else {
> -            return H_PARAMETER;
> -        }
> +    if (ppc_hash64_hpte_shift(pteh, ptel, &page_shift, &avpnm) < 0) {
> +        return H_PARAMETER;
> +    }
> +
> +    if (HPTE64_V_AVPN_VAL(pteh) & avpnm) {


HPTE64_V_AVPN_VAL(pteh) & ((1ULL << page_shift) >> 23)
should do the same thing, no?



> +        return H_PARAMETER;
>       }
>
>       raddr = (ptel & HPTE64_R_RPN) & ~((1ULL << page_shift) - 1);
> diff --git a/target-ppc/mmu-hash64.c b/target-ppc/mmu-hash64.c
> index 34e20fa..d3b895d 100644
> --- a/target-ppc/mmu-hash64.c
> +++ b/target-ppc/mmu-hash64.c
> @@ -399,6 +399,37 @@ static uint64_t ppc_hash64_page_shift(ppc_slb_t *slb)
>       return epnshift;
>   }
>
> +/* Returns the page shift of the given hpte.  This is the "base"
> + * shift, used for hashing.  Although we don't implement it yet, the
> + * architecture allows the actuall mapped page to be larger, but


s/actuall/actual/


> + * determining that size requires information from the slb. */
> +int ppc_hash64_hpte_shift(uint64_t pte0, uint64_t pte1,
> +                          int *shift, uint64_t *avpnm)
> +{
> +    *shift = 12; /* 4kiB default */
> +
> +    /* only handle 4k and 16M pages for now */
> +    if (pte0 & HPTE64_V_LARGE) {
> +        if ((pte1 & 0xf000) == 0x1000) {


Nit: it would improve readability of this code if HPTE64_R_KEY()&co was 
used. What does define these "key" bits meaning?


> +            *shift = 16; /* 64 kiB */
> +        } else if ((pte1 & 0xff000) == 0) {
> +            *shift = 24; /* 16MiB */
> +        } else {
> +            return -EINVAL;
> +        }
> +    }
> +
> +    if (avpnm) {
> +        if (*shift <= 23) {
> +            *avpnm = 0;
> +        } else {
> +            *avpnm = (1ULL << (*shift - 23)) - 1;
> +        }
> +    }


What does "avpnm" stand for? Abbreviated virtual page <something> mask?

If you dropped "if(avpnm)", you could move "*avpnm = 0;" to the beginning 
and "*avpnm = (1ULL << (*shift - 23)) - 1;" next to "*shift = 24;".



> +
> +    return 0;
> +}
> +
>   static hwaddr ppc_hash64_htab_lookup(CPUPPCState *env,
>                                        ppc_slb_t *slb, target_ulong eaddr,
>                                        ppc_hash_pte64_t *pte)
> diff --git a/target-ppc/mmu-hash64.h b/target-ppc/mmu-hash64.h
> index 291750f..71d2532 100644
> --- a/target-ppc/mmu-hash64.h
> +++ b/target-ppc/mmu-hash64.h
> @@ -117,6 +117,9 @@ typedef struct {
>       uint64_t pte0, pte1;
>   } ppc_hash_pte64_t;
>
> +int ppc_hash64_hpte_shift(uint64_t pte0, uint64_t pte1,
> +                          int *shift, uint64_t *avpnm);
> +
>   #endif /* CONFIG_USER_ONLY */
>
>   #endif /* !defined (__MMU_HASH64_H__) */
>


-- 
Alexey