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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.11.2024 18:19, Roger Pau Monné wrote:
> 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?

Yes.

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

Not sure without actually seeing it in place. Without those wrapper macros,
having n be the first macro parameter may help reduce the visual difference
between both variants. Yet if you clearly feel better with the wrappers,
I'm not going to insist on omitting them.

Jan