Re: [Xen-devel] [RFC PATCH v3 05/10] arm/p2m: Make PTE helpers publicly available

[Julien Grall <julien.grall@xxxxxxx>]

Hi Sergej,

On 06/15/2017 12:05 PM, Sergej Proskurin wrote:
> In this commit we make the p2m_* helpers, which access PTE properties in
> a simplified way, publicly available. This is due to the fact that the
> helpers will be used in guest_walk.c in one of the following commits.
>
> Signed-off-by: Sergej Proskurin <proskurin@xxxxxxxxxxxxx>
> ---
> Cc:
> ---
>   xen/arch/arm/p2m.c        | 23 -----------------------
>   xen/include/asm-arm/p2m.h | 27 +++++++++++++++++++++++++++
>   2 files changed, 27 insertions(+), 23 deletions(-)
>
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index b7bbea1d81..eecbcdf870 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -46,29 +46,6 @@ static const paddr_t level_masks[] =
>   static const uint8_t level_orders[] =
>       { ZEROETH_ORDER, FIRST_ORDER, SECOND_ORDER, THIRD_ORDER };
>   
> -static inline bool_t p2m_valid(lpae_t pte)
> -{
> -    return pte.p2m.valid;
> -}
> -/*
> - * These two can only be used on L0..L2 ptes because L3 mappings set
> - * the table bit and therefore these would return the opposite to what
> - * you would expect.
> - */
> -static inline bool_t p2m_table(lpae_t pte)
> -{
> -    return p2m_valid(pte) && pte.p2m.table;
> -}
> -static inline bool_t p2m_mapping(lpae_t pte)
> -{
> -    return p2m_valid(pte) && !pte.p2m.table;
> -}
> -
> -static inline bool p2m_is_superpage(lpae_t pte, unsigned int level)
> -{
> -    return (level < 3) && p2m_mapping(pte);
> -}
> -
>   static void p2m_flush_tlb(struct p2m_domain *p2m);
>   
>   /* Unlock the flush and do a P2M TLB flush if necessary */
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index 18c57f936e..8053f2a0cf 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -198,6 +198,33 @@ static inline int p2m_is_write_locked(struct p2m_domain 
> *p2m)
>       return rw_is_write_locked(&p2m->lock);
>   }
>   
> +/*
> + * Helpers to lookup properties of ptes.
> + */
> +
> +static inline bool_t p2m_valid(lpae_t pte)
The name implies they should only be used for stage-2 page table. But 
you are using for other place such as for stage-1 page table.
This means that they are in the wrong header (they should go in page.h).

I would actually start to split page.h with lpae.h and 
short-descriptor.h to avoid mixing two type of page table in the same 
header. Note that I would be happy if you keep lpae in page.h for the 
time being. But short-descriptor should definitely go in a separate file.
I would also rename the helpers you move to lpae_*.

> +{
> +    return pte.p2m.valid;
As you plan to re-use for other things than stage-2 page-table, you use 
pte.walk rather than pte.p2m.
All those comments applies for the rest of the helpers.

> +}
> +/*
> + * These two can only be used on L0..L2 ptes because L3 mappings set
> + * the table bit and therefore these would return the opposite to what
> + * you would expect.
> + */
> +static inline bool_t p2m_table(lpae_t pte)
> +{
> +    return p2m_valid(pte) && pte.p2m.table;
> +}
> +static inline bool_t p2m_mapping(lpae_t pte)
> +{
> +    return p2m_valid(pte) && !pte.p2m.table;
> +}
> +
> +static inline bool p2m_is_superpage(lpae_t pte, unsigned int level)
> +{
> +    return (level < 3) && p2m_mapping(pte);
> +}
> +
>   /* Look up the MFN corresponding to a domain's GFN. */
>   mfn_t p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t);
>   
Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel