Re: [Xen-devel] [PATCH v4 2/9] arm/mem_access: Add defines supporting PTs with varying page sizes

[Julien Grall <julien.grall@xxxxxxx>]

Hi Sergej,

On 20/06/17 21:33, Sergej Proskurin wrote:
> The ARMv8 architecture supports pages with different (4K, 16K, and 64K) sizes.
> To enable guest page table walks for various configurations, this commit
> extends the defines and helpers of the current implementation.
>
> Signed-off-by: Sergej Proskurin <proskurin@xxxxxxxxxxxxx>
> ---
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Cc: Julien Grall <julien.grall@xxxxxxx>
> ---
> v3: Eliminate redundant macro definitions by introducing generic macros.
>
> v4: Replace existing macros with ones that generate static inline
>     helpers as to ease the readability of the code.
>
>     Move the introduced code into lpae.h
> ---
>  xen/include/asm-arm/lpae.h | 67 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 67 insertions(+)
>
> diff --git a/xen/include/asm-arm/lpae.h b/xen/include/asm-arm/lpae.h
> index 6fbf7c606c..2913428e96 100644
> --- a/xen/include/asm-arm/lpae.h
> +++ b/xen/include/asm-arm/lpae.h
> @@ -151,6 +151,73 @@ static inline bool lpae_is_superpage(lpae_t pte, unsigned 
> int level)
>      return (level < 3) && lpae_mapping(pte);
>  }
>
> +/*
> + * The ARMv8 architecture supports pages with different sizes (4K, 16K, and
> + * 64K). To enable guest page table walks for various configurations, the
> + * following helpers enable walking the guest's translation table with varying
> + * page size granularities.
> + */
> +
> +#define LPAE_SHIFT_4K           (9)
> +#define LPAE_SHIFT_16K          (11)
> +#define LPAE_SHIFT_64K          (13)
> +
> +#define lpae_entries(gran)      (_AC(1,U) << LPAE_SHIFT_##gran)
> +#define lpae_entry_mask(gran)   (lpae_entries(gran) - 1)
> +
> +#define PAGE_SHIFT_4K           (12)
> +#define PAGE_SHIFT_16K          (14)
> +#define PAGE_SHIFT_64K          (16)
We already define PAGE_SHIFT_* in xen/iommu.h. You probably want to 
consolidate them in a single common place (maybe xen/paging.h or 
xen/lib.h?).
> +
> +#define third_shift(gran)       (PAGE_SHIFT_##gran)
> +#define third_size(gran)        ((paddr_t)1 << third_shift(gran))
> +
> +#define second_shift(gran)      (third_shift(gran) + LPAE_SHIFT_##gran)
> +#define second_size(gran)       ((paddr_t)1 << second_shift(gran))
> +
> +#define first_shift(gran)       (second_shift(gran) + LPAE_SHIFT_##gran)
> +#define first_size(gran)        ((paddr_t)1 << first_shift(gran))
> +
> +/* Note that there is no zeroeth lookup level with a 64K granule size. */
> +#define zeroeth_shift(gran)     (first_shift(gran) + LPAE_SHIFT_##gran)
> +#define zeroeth_size(gran)      ((paddr_t)1 << zeroeth_shift(gran))
> +
> +#define GUEST_TABLE_OFFSET(offs, gran)          ((paddr_t)(offs) & 
> lpae_entry_mask(gran))
> +#define third_guest_table_offset(gva, gran)     GUEST_TABLE_OFFSET((gva >> 
> third_shift(gran)), gran)
> +#define second_guest_table_offset(gva, gran)    GUEST_TABLE_OFFSET((gva >> 
> second_shift(gran)), gran)
> +#define first_guest_table_offset(gva, gran)     GUEST_TABLE_OFFSET((gva >> 
> first_shift(gran)), gran)
> +#define zeroeth_guest_table_offset(gva, gran)   GUEST_TABLE_OFFSET((gva >> 
> zeroeth_shift(gran)), gran)
I don't think we should expose those macros and AFAICT you only use them 
once. So I would prefer if you move open-code them in the helpers below.
> +
> +#define GUEST_TABLE_OFFSET_HELPERS(gran)                                \
> +static inline vaddr_t third_guest_table_offset_##gran##K(vaddr_t gva)   \
> +{                                                                       \
> +    return third_guest_table_offset(gva, gran##K);                      \
> +}                                                                       \
> +                                                                        \
> +static inline vaddr_t second_guest_table_offset_##gran##K(vaddr_t gva)  \
> +{                                                                       \
> +    return second_guest_table_offset(gva, gran##K);                     \
> +}                                                                       \
> +                                                                        \
> +static inline vaddr_t first_guest_table_offset_##gran##K(vaddr_t gva)   \
> +{                                                                       \
> +    return first_guest_table_offset(gva, gran##K);                      \
> +}                                                                       \
> +                                                                        \
> +static inline vaddr_t zeroeth_guest_table_offset_##gran##K(vaddr_t gva) \
> +{                                                                       \
> +    if ( gran == 64 )                                                   \
Please add a comment saying 64K does not have zeroeth page-table.

> +        return 0;                                                       \
> +    else                                                                \
> +        return zeroeth_guest_table_offset((paddr_t)gva, gran##K);       \
> +}                                                                       \
> +
> +GUEST_TABLE_OFFSET_HELPERS(4);
> +#ifdef CONFIG_ARM_64
> +GUEST_TABLE_OFFSET_HELPERS(16);
> +GUEST_TABLE_OFFSET_HELPERS(64);
> +#endif
Please undef both GUEST_TABLE_OFFSET and GUEST_TABLE_OFFSET_HELPERS.

> +
>  #endif /* __ASSEMBLY__ */
>
>  /*
Cheers,

--
Julien Grall

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