diff mbox series

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

Message ID 20210425201318.15447-3-julien@xen.org (mailing list archive)
State New
Headers show
Series xen/arm: mm: Remove open-coding mappings | expand

Commit Message

Julien Grall April 25, 2021, 8:13 p.m. UTC
From: Julien Grall <jgrall@amazon.com>

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@amazon.com>

---
    Changes in v2:
        - New patch
---
 xen/include/asm-arm/lpae.h | 71 +++++++++++++++++++++-----------------
 1 file changed, 40 insertions(+), 31 deletions(-)

Comments

Stefano Stabellini May 11, 2021, 10:26 p.m. UTC | #1
On Sun, 25 Apr 2021, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> 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@amazon.com>

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.


> +#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:

#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
>
Julien Grall May 12, 2021, 2:26 p.m. UTC | #2
Hi Stefano,

On 11/05/2021 23:26, Stefano Stabellini wrote:
> On Sun, 25 Apr 2021, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> 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@amazon.com>
> 
> 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,
Stefano Stabellini May 12, 2021, 9:30 p.m. UTC | #3
On Wed, 12 May 2021, Julien Grall wrote:
> Hi Stefano,
> 
> On 11/05/2021 23:26, Stefano Stabellini wrote:
> > On Sun, 25 Apr 2021, Julien Grall wrote:
> > > From: Julien Grall <jgrall@amazon.com>
> > > 
> > > 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@amazon.com>
> > 
> > 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.

That would help I think


> > > +#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?

Maybe. The concern I have is that there are multiple page granularities
(4kb, 16kb, etc) and multiple page sizes (4kb, 2mb, etc). If I just see
LEVEL_ORDER it is not immediately obvious what granularity and what size
we are talking about.

I think using a name that makes it clear that they are referring to Xen
pages, currently 4kb, it would make sense. Or maybe a in-code comment
would be sufficient.

I don't have a great suggestion here so I'll leave it to you. I am also
OK to keep them as is.


> > #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
>
Julien Grall May 12, 2021, 10:16 p.m. UTC | #4
Hi Stefano,

On 12/05/2021 22:30, Stefano Stabellini wrote:
> On Wed, 12 May 2021, Julien Grall wrote:
>>>> +#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?
> 
> Maybe. The concern I have is that there are multiple page granularities
> (4kb, 16kb, etc) and multiple page sizes (4kb, 2mb, etc). If I just see
> LEVEL_ORDER it is not immediately obvious what granularity and what size
> we are talking about.

I am a bit puzzled with your answer. AFAIU, you are happy with the 
existing macros (THIRD_*, SECOND_*) but not with the new macros.

In reality, there is no difference because THIRD_* doesn't tell you the 
exact size but only "this is a level 3 mapping".

So can you clarify what you are after? IOW is it reworking the current 
naming scheme?

Cheers,
Stefano Stabellini May 13, 2021, 10:44 p.m. UTC | #5
On Wed, 12 May 2021, Julien Grall wrote:
> Hi Stefano,
> 
> On 12/05/2021 22:30, Stefano Stabellini wrote:
> > On Wed, 12 May 2021, Julien Grall wrote:
> > > > > +#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?
> > 
> > Maybe. The concern I have is that there are multiple page granularities
> > (4kb, 16kb, etc) and multiple page sizes (4kb, 2mb, etc). If I just see
> > LEVEL_ORDER it is not immediately obvious what granularity and what size
> > we are talking about.
> 
> I am a bit puzzled with your answer. AFAIU, you are happy with the existing
> macros (THIRD_*, SECOND_*) but not with the new macros.
>
> In reality, there is no difference because THIRD_* doesn't tell you the exact
> size but only "this is a level 3 mapping".
> 
> So can you clarify what you are after? IOW is it reworking the current naming
> scheme?

You are right -- there is no real difference between THIRD_*, SECOND_*
and LEVEL_*.

The original reason for my comments is that I hadn't read the following
patches, and the definition of LEVEL_* macros is simple, they could be
open coded. It looked like they were only going to be used to make the
definition of THIRD_*, SECOND_* a bit easier. So, at first, I was
wondering if they were needed at all.

Secondly, I realized that they were going to be used in *.c files by
other patches. That's why they are there. But I started thinking whether
we should find a way to make it a bit clearer that they are for Xen
pages, currently at 4KB granularity. THIRD_*, SECOND_*, etc. are already
generic names which don't convey the granularity or whether they are Xen
pages at all. But LEVEL_* seem even more generic.

As I mentioned, I don't have any good suggestions for changes to make
here, so unless you can come up with a good idea let's keep it as is.
diff mbox series

Patch

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))
+#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))
+
+/* 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)