| Message ID | 20210425201318.15447-3-julien@xen.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/arm: mm: Remove open-coding mappings | expand |
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 >
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,
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 >
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,
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.
Hi Stefano, Sorry for the late answer. On 13/05/2021 23:44, Stefano Stabellini wrote: > 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. I am thinking to use the following naming (diff on top of this patch): -#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 XEN_PT_SHIFT LPAE_SHIFT_GS(PAGE_SHIFT) +#define XEN_PT_ENTRIES LPAE_ENTRIES_GS(PAGE_SHIFT) +#define XEN_PT_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)) +#define XEN_PT_LEVEL_SHIFT(lvl) LEVEL_SHIFT_GS(PAGE_SHIFT, lvl) +#define XEN_PT_LEVEL_ORDER(lvl) LEVEL_ORDER_GS(PAGE_SHIFT, lvl) +#define XEN_PT_LEVEL_SIZE(lvl) LEVEL_SIZE_GS(PAGE_SHIFT, lvl) +#define XEN_PT_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) +#define THIRD_SHIFT XEN_PT_LEVEL_SHIFT(3) +#define THIRD_ORDER XEN_PT_LEVEL_ORDER(3) +#define THIRD_SIZE XEN_PT_LEVEL_SIZE(3) +#define THIRD_MASK XEN_PT_LEVEL_MASK(3) + +#define SECOND_SHIFT XEN_PT_LEVEL_SHIFT(2) +#define SECOND_ORDER XEN_PT_LEVEL_ORDER(2) +#define SECOND_SIZE XEN_PT_LEVEL_SIZE(2) +#define SECOND_MASK XEN_PT_LEVEL_MASK(2) + +#define FIRST_SHIFT XEN_PT_LEVEL_SHIFT(1) +#define FIRST_ORDER XEN_PT_LEVEL_ORDER(1) +#define FIRST_SIZE XEN_PT_LEVEL_SIZE(1) +#define FIRST_MASK XEN_PT_LEVEL_MASK(1) + +#define ZEROETH_SHIFT XEN_PT_LEVEL_SHIFT(0) +#define ZEROETH_ORDER XEN_PT_LEVEL_ORDER(0) +#define ZEROETH_SIZE XEN_PT_LEVEL_SIZE(0) +#define ZEROETH_MASK XEN_PT_LEVEL_MASK(0) I don't plan to modify the nameing for ZEROETH*, FIRST*, SECOND*, THIRD*. Let me know if you prefer it over the currrent naming. Cheers,
On Sat, 3 Jul 2021, Julien Grall wrote: > Hi Stefano, > > Sorry for the late answer. > > On 13/05/2021 23:44, Stefano Stabellini wrote: > > 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. > > I am thinking to use the following naming (diff on top of this patch): > > -#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 XEN_PT_SHIFT LPAE_SHIFT_GS(PAGE_SHIFT) > +#define XEN_PT_ENTRIES LPAE_ENTRIES_GS(PAGE_SHIFT) > +#define XEN_PT_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)) > +#define XEN_PT_LEVEL_SHIFT(lvl) LEVEL_SHIFT_GS(PAGE_SHIFT, lvl) > +#define XEN_PT_LEVEL_ORDER(lvl) LEVEL_ORDER_GS(PAGE_SHIFT, lvl) > +#define XEN_PT_LEVEL_SIZE(lvl) LEVEL_SIZE_GS(PAGE_SHIFT, lvl) > +#define XEN_PT_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) > +#define THIRD_SHIFT XEN_PT_LEVEL_SHIFT(3) > +#define THIRD_ORDER XEN_PT_LEVEL_ORDER(3) > +#define THIRD_SIZE XEN_PT_LEVEL_SIZE(3) > +#define THIRD_MASK XEN_PT_LEVEL_MASK(3) > + > +#define SECOND_SHIFT XEN_PT_LEVEL_SHIFT(2) > +#define SECOND_ORDER XEN_PT_LEVEL_ORDER(2) > +#define SECOND_SIZE XEN_PT_LEVEL_SIZE(2) > +#define SECOND_MASK XEN_PT_LEVEL_MASK(2) > + > +#define FIRST_SHIFT XEN_PT_LEVEL_SHIFT(1) > +#define FIRST_ORDER XEN_PT_LEVEL_ORDER(1) > +#define FIRST_SIZE XEN_PT_LEVEL_SIZE(1) > +#define FIRST_MASK XEN_PT_LEVEL_MASK(1) > + > +#define ZEROETH_SHIFT XEN_PT_LEVEL_SHIFT(0) > +#define ZEROETH_ORDER XEN_PT_LEVEL_ORDER(0) > +#define ZEROETH_SIZE XEN_PT_LEVEL_SIZE(0) > +#define ZEROETH_MASK XEN_PT_LEVEL_MASK(0) > > I don't plan to modify the nameing for ZEROETH*, FIRST*, SECOND*, THIRD*. > > Let me know if you prefer it over the currrent naming. Yes, I think it is better, thanks!
Hi Stefano, On 13/07/2021 21:53, Stefano Stabellini wrote: > On Sat, 3 Jul 2021, Julien Grall wrote: >> Hi Stefano, >> >> Sorry for the late answer. >> >> On 13/05/2021 23:44, Stefano Stabellini wrote: >>> 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. >> >> I am thinking to use the following naming (diff on top of this patch): >> >> -#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 XEN_PT_SHIFT LPAE_SHIFT_GS(PAGE_SHIFT) >> +#define XEN_PT_ENTRIES LPAE_ENTRIES_GS(PAGE_SHIFT) >> +#define XEN_PT_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)) >> +#define XEN_PT_LEVEL_SHIFT(lvl) LEVEL_SHIFT_GS(PAGE_SHIFT, lvl) >> +#define XEN_PT_LEVEL_ORDER(lvl) LEVEL_ORDER_GS(PAGE_SHIFT, lvl) >> +#define XEN_PT_LEVEL_SIZE(lvl) LEVEL_SIZE_GS(PAGE_SHIFT, lvl) >> +#define XEN_PT_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) >> +#define THIRD_SHIFT XEN_PT_LEVEL_SHIFT(3) >> +#define THIRD_ORDER XEN_PT_LEVEL_ORDER(3) >> +#define THIRD_SIZE XEN_PT_LEVEL_SIZE(3) >> +#define THIRD_MASK XEN_PT_LEVEL_MASK(3) >> + >> +#define SECOND_SHIFT XEN_PT_LEVEL_SHIFT(2) >> +#define SECOND_ORDER XEN_PT_LEVEL_ORDER(2) >> +#define SECOND_SIZE XEN_PT_LEVEL_SIZE(2) >> +#define SECOND_MASK XEN_PT_LEVEL_MASK(2) >> + >> +#define FIRST_SHIFT XEN_PT_LEVEL_SHIFT(1) >> +#define FIRST_ORDER XEN_PT_LEVEL_ORDER(1) >> +#define FIRST_SIZE XEN_PT_LEVEL_SIZE(1) >> +#define FIRST_MASK XEN_PT_LEVEL_MASK(1) >> + >> +#define ZEROETH_SHIFT XEN_PT_LEVEL_SHIFT(0) >> +#define ZEROETH_ORDER XEN_PT_LEVEL_ORDER(0) >> +#define ZEROETH_SIZE XEN_PT_LEVEL_SIZE(0) >> +#define ZEROETH_MASK XEN_PT_LEVEL_MASK(0) >> >> I don't plan to modify the nameing for ZEROETH*, FIRST*, SECOND*, THIRD*. >> >> Let me know if you prefer it over the currrent naming. > > Yes, I think it is better, thanks! Ok. I will try to respin the series soon. Cheers,
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)