Re: [PATCH RFCv2 02/15] xen/arm: lpae: Use the generic helpers to defined the Xen PT helpers

[Julien Grall <julien@xxxxxxx>]

Hi Stefano,

On 11/05/2021 23:26, Stefano Stabellini wrote:
> On Sun, 25 Apr 2021, Julien Grall wrote:
> > From: Julien Grall <jgrall@xxxxxxxxxx>
> >
> > Currently, Xen PT helpers are only working with 4KB page granularity
> > and open-code the generic helpers. To allow more flexibility, we can
> > re-use the generic helpers and pass Xen's page granularity
> > (PAGE_SHIFT).
> >
> > As Xen PT helpers are used in both C and assembly, we need to move
> > the generic helpers definition outside of the !__ASSEMBLY__ section.
> >
> > Note the aliases for each level are still kept for the time being so we
> > can avoid a massive patch to change all the callers.
> >
> > Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
> The patch is OK as is. I have a couple of suggestions for improvement
> below. If you feel like making them, good, otherwise I am also OK if you
> don't want to change anything.
>
>
> > ---
> >      Changes in v2:
> >          - New patch
> > ---
> >   xen/include/asm-arm/lpae.h | 71 +++++++++++++++++++++-----------------
> >   1 file changed, 40 insertions(+), 31 deletions(-)
> >
> > diff --git a/xen/include/asm-arm/lpae.h b/xen/include/asm-arm/lpae.h
> > index 4fb9a40a4ca9..310f5225e056 100644
> > --- a/xen/include/asm-arm/lpae.h
> > +++ b/xen/include/asm-arm/lpae.h
> > @@ -159,6 +159,17 @@ static inline bool lpae_is_superpage(lpae_t pte, unsigned 
> > int level)
> >   #define lpae_get_mfn(pte)    (_mfn((pte).walk.base))
> >   #define lpae_set_mfn(pte, mfn)  ((pte).walk.base = mfn_x(mfn))
> >   
> > +/* Generate an array @var containing the offset for each level from @addr */
> > +#define DECLARE_OFFSETS(var, addr)          \
> > +    const unsigned int var[4] = {           \
> > +        zeroeth_table_offset(addr),         \
> > +        first_table_offset(addr),           \
> > +        second_table_offset(addr),          \
> > +        third_table_offset(addr)            \
> > +    }
> > +
> > +#endif /* __ASSEMBLY__ */
> > +
> >   /*
> >    * AArch64 supports pages with different sizes (4K, 16K, and 64K).
> >    * Provide a set of generic helpers that will compute various
> > @@ -190,17 +201,6 @@ static inline bool lpae_is_superpage(lpae_t pte, unsigned 
> > int level)
> >   #define LPAE_TABLE_INDEX_GS(gs, lvl, addr)   \
> >       (((addr) >> LEVEL_SHIFT_GS(gs, lvl)) & LPAE_ENTRY_MASK_GS(gs))
> >   
> > -/* Generate an array @var containing the offset for each level from @addr */
> > -#define DECLARE_OFFSETS(var, addr)          \
> > -    const unsigned int var[4] = {           \
> > -        zeroeth_table_offset(addr),         \
> > -        first_table_offset(addr),           \
> > -        second_table_offset(addr),          \
> > -        third_table_offset(addr)            \
> > -    }
> > -
> > -#endif /* __ASSEMBLY__ */
> > -
> >   /*
> >    * These numbers add up to a 48-bit input address space.
> >    *
> > @@ -211,26 +211,35 @@ static inline bool lpae_is_superpage(lpae_t pte, unsigned 
> > int level)
> >    * therefore 39-bits are sufficient.
> >    */
> >   
> > -#define LPAE_SHIFT      9
> > -#define LPAE_ENTRIES    (_AC(1,U) << LPAE_SHIFT)
> > -#define LPAE_ENTRY_MASK (LPAE_ENTRIES - 1)
> > -
> > -#define THIRD_SHIFT    (PAGE_SHIFT)
> > -#define THIRD_ORDER    (THIRD_SHIFT - PAGE_SHIFT)
> > -#define THIRD_SIZE     (_AT(paddr_t, 1) << THIRD_SHIFT)
> > -#define THIRD_MASK     (~(THIRD_SIZE - 1))
> > -#define SECOND_SHIFT   (THIRD_SHIFT + LPAE_SHIFT)
> > -#define SECOND_ORDER   (SECOND_SHIFT - PAGE_SHIFT)
> > -#define SECOND_SIZE    (_AT(paddr_t, 1) << SECOND_SHIFT)
> > -#define SECOND_MASK    (~(SECOND_SIZE - 1))
> > -#define FIRST_SHIFT    (SECOND_SHIFT + LPAE_SHIFT)
> > -#define FIRST_ORDER    (FIRST_SHIFT - PAGE_SHIFT)
> > -#define FIRST_SIZE     (_AT(paddr_t, 1) << FIRST_SHIFT)
> > -#define FIRST_MASK     (~(FIRST_SIZE - 1))
> > -#define ZEROETH_SHIFT  (FIRST_SHIFT + LPAE_SHIFT)
> > -#define ZEROETH_ORDER  (ZEROETH_SHIFT - PAGE_SHIFT)
> > -#define ZEROETH_SIZE   (_AT(paddr_t, 1) << ZEROETH_SHIFT)
> > -#define ZEROETH_MASK   (~(ZEROETH_SIZE - 1))
> Should we add a one-line in-code comment saying that the definitions
> below are for 4KB pages? It is not immediately obvious any longer.
Because they are not meant to be for 4KB pages. They are meant to be for 
Xen page size.
Today, it is always 4KB but I would like the Xen code to not rely on that.

I can clarify it in an in-code comment.

> > +#define LPAE_SHIFT          LPAE_SHIFT_GS(PAGE_SHIFT)
> > +#define LPAE_ENTRIES        LPAE_ENTRIES_GS(PAGE_SHIFT)
> > +#define LPAE_ENTRY_MASK     LPAE_ENTRY_MASK_GS(PAGE_SHIFT)
> >
> > +#define LEVEL_SHIFT(lvl)    LEVEL_SHIFT_GS(PAGE_SHIFT, lvl)
> > +#define LEVEL_ORDER(lvl)    LEVEL_ORDER_GS(PAGE_SHIFT, lvl)
> > +#define LEVEL_SIZE(lvl)     LEVEL_SIZE_GS(PAGE_SHIFT, lvl)
> > +#define LEVEL_MASK(lvl)     (~(LEVEL_SIZE(lvl) - 1))
> I would avoid adding these 4 macros. It would be OK if they were just
> used within this file but lpae.h is a header: they could end up be used
> anywhere in the xen/ code and they have a very generic name. My
> suggestion would be to skip them and just do:
Those macros will be used in follow-up patches. They are pretty useful 
to avoid introduce static array with the different information for each 
level.
Would prefix them with XEN_ be better?

> #define THIRD_SHIFT         LEVEL_SHIFT_GS(PAGE_SHIFT, 3)
>
> etc.
>
>
> > +/* Convenience aliases */
> > +#define THIRD_SHIFT         LEVEL_SHIFT(3)
> > +#define THIRD_ORDER         LEVEL_ORDER(3)
> > +#define THIRD_SIZE          LEVEL_SIZE(3)
> > +#define THIRD_MASK          LEVEL_MASK(3)
> > +
> > +#define SECOND_SHIFT        LEVEL_SHIFT(2)
> > +#define SECOND_ORDER        LEVEL_ORDER(2)
> > +#define SECOND_SIZE         LEVEL_SIZE(2)
> > +#define SECOND_MASK         LEVEL_MASK(2)
> > +
> > +#define FIRST_SHIFT         LEVEL_SHIFT(1)
> > +#define FIRST_ORDER         LEVEL_ORDER(1)
> > +#define FIRST_SIZE          LEVEL_SIZE(1)
> > +#define FIRST_MASK          LEVEL_MASK(1)
> > +
> > +#define ZEROETH_SHIFT       LEVEL_SHIFT(0)
> > +#define ZEROETH_ORDER       LEVEL_ORDER(0)
> > +#define ZEROETH_SIZE        LEVEL_SIZE(0)
> > +#define ZEROETH_MASK        LEVEL_MASK(0)
> >   
> >   /* Calculate the offsets into the pagetables for a given VA */
> >   #define zeroeth_linear_offset(va) ((va) >> ZEROETH_SHIFT)
> > --
> > 2.17.1
Cheers,

--
Julien Grall