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 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.

Thanks, Roger.