Re: [PATCH] arm64/mm: Define PTE_SHIFT

[Mark Rutland]

On Fri, Mar 07, 2025 at 02:50:56PM +0530, Anshuman Khandual wrote:
> On 3/7/25 14:37, Ryan Roberts wrote:
> > On 07/03/2025 05:08, Anshuman Khandual wrote:

> >>  #define EARLY_LEVEL(lvl, lvls, vstart, vend, add)	\
> >> -	(lvls > lvl ? EARLY_ENTRIES(vstart, vend, SWAPPER_BLOCK_SHIFT + lvl * (PAGE_SHIFT - 3), add) : 0)
> >> +	(lvls > lvl ? EARLY_ENTRIES(vstart, vend, SWAPPER_BLOCK_SHIFT + \
> >> +	lvl * (PAGE_SHIFT - PTE_SHIFT), add) : 0)
> > 
> > nit: not sure what style guide says, but I would indent this continuation an
> > extra level.
> 
> IIUC - An indentation is not normally required with a line continuation although
> the starting letter should match the starting letter in the line above but after
> the '(' (if any).

Regardless of indenttation, the existing code is fairly hard to read,
and I reckon it'd be better to split up, e.g.

| /* Number of VA bits resolved by a single translation table level */
| #define PTDESC_TABLE_SHIFT	(PAGE_SHIFT - PTDESC_ORDER)
| 
| #define __EARLY_LEVEL(lvl, vstart, vend, add) \
| 	EARLY_ENTRIES(vstart, vend, SWAPPER_BLOCK_SHIFT + lvl * PTDESC_TABLE_SHIFT, add)
| 
| #define EARLY_LEVEL(lvl, lvls, vstart, vend, add) \
| 	((lvls) > (lvl) ? __EARLY_LEVEL(lvl, vstart, vend, add) : 0)

... and ignoring the use of _SHIFT vs _ORDER, I think that structure is
far more legible.

With that, we can fold EARLY_ENTRIES() and __EARLY_LEVEL() together and
move the 'add' into EARLY_LEVEL(), e.g.

| /* Number of VA bits resolved by a single translation table level */
| #define PTDESC_TABLE_SHIFT	(PAGE_SHIFT - PTDESC_ORDER)
| 
| #define EARLY_ENTRIES(lvl, vstart, vend) \
| 	(SPAN_NR_ENTRIES(vstart, vend, SWAPPER_BLOCK_SHIFT + lvl * PTDESC_TABLE_SHIFT))
| 
| #define EARLY_LEVEL(lvl, lvls, vstart, vend, add) \
| 	((lvls) > (lvl) ? EARLY_ENTRIES(lvl, vstart, vend) + (add) : 0)

... which I think makes the 'add' a bit easier to understand too.

Mark.