Re: [PATCH v2 1/4] x86/mm: introduce helpers to detect super page alignment

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Nov 07, 2024 at 05:07:34PM +0100, Roger Pau Monné wrote:
> On Thu, Nov 07, 2024 at 11:42:11AM +0100, Jan Beulich wrote:
> > On 06.11.2024 13:29, Roger Pau Monne wrote:
> > > --- a/xen/arch/x86/include/asm/page.h
> > > +++ b/xen/arch/x86/include/asm/page.h
> > > @@ -200,6 +200,12 @@ static inline l4_pgentry_t l4e_from_paddr(paddr_t 
> > > pa, unsigned int flags)
> > >  #define l4_table_offset(a)         \
> > >      (((a) >> L4_PAGETABLE_SHIFT) & (L4_PAGETABLE_ENTRIES - 1))
> > >  
> > > +/* Check if an address is aligned for a given slot level. */
> > > +#define SLOT_IS_ALIGNED(v, m, s) \
> > > +    IS_ALIGNED(PFN_DOWN(v) | mfn_x(m), (1UL << ((s) - PAGE_SHIFT)) - 1)
> > 
> > The check involving an address and an MFN, I think the comment would better
> > also reflect this. "Check if a (va,mfn) tuple is suitably aligned to be
> > mapped by a large page at a given page table level"?
> > 
> > As to the name of this helper macro - "SLOT" can mean about anything when
> > not further qualified. If the macro was local to ...
> > 
> > > +#define IS_L3E_ALIGNED(v, m) SLOT_IS_ALIGNED(v, m, L3_PAGETABLE_SHIFT)
> > > +#define IS_L2E_ALIGNED(v, m) SLOT_IS_ALIGNED(v, m, L2_PAGETABLE_SHIFT)
> > > +
> > >  /* Convert a pointer to a page-table entry into pagetable slot index. */
> > >  #define pgentry_ptr_to_slot(_p)    \
> > >      (((unsigned long)(_p) & ~PAGE_MASK) / sizeof(*(_p)))
> > > --- a/xen/arch/x86/mm.c
> > > +++ b/xen/arch/x86/mm.c
> > 
> > ... this (sole) file using the derived ones, that might be acceptable. If
> > it's to remain in page.h, how about e.g. IS_LnE_ALIGNED()?
> 
> Since you expressed further concerns in the next patch, I will move it
> to being local to mm.c.  I don't have any other use-case, but assumed
> the macros are generic enough to be useful in other contexts.
> 
> > I further wonder whether it wouldn't be neater if just the level was passed
> > into the helper. Doing so wouldn't even require token concatenation (which
> > iirc both you and Andrew don't like in situations like this one), as the
> > mask can be calculated from just level and PAGETABLE_ORDER. At which point
> > it may even make sense to leave out the wrapper macros.
> 
> I can see what I can do.

Would something like:

#define IS_LnE_ALIGNED(v, m, n) \
    IS_ALIGNED(PFN_DOWN(v) | mfn_x(m), (1UL << (PAGETABLE_ORDER * (n - 1))) - 1)

Defined only in the context of map_pages_to_xen() be OK with you?

I'm unsure whether it would be better if I still provided the
IS_L{2,3}E_ALIGNED() macros based on that, as IMO those macros made
the conditionals clearer to read.

Thanks, Roger.