| Message ID | 20241106122927.26461-2-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/mm: miscellaneous fixes | expand |
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()? 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. Jan
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.
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.
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
diff --git a/xen/arch/x86/include/asm/page.h b/xen/arch/x86/include/asm/page.h index e01af28916b0..6970916d61d5 100644 --- 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) +#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))) diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index d537a799bced..8f7c397a82d4 100644 --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -5249,9 +5249,7 @@ int map_pages_to_xen( L3T_LOCK(current_l3page); ol3e = *pl3e; - if ( cpu_has_page1gb && - !(((virt >> PAGE_SHIFT) | mfn_x(mfn)) & - ((1UL << (L3_PAGETABLE_SHIFT - PAGE_SHIFT)) - 1)) && + if ( cpu_has_page1gb && IS_L3E_ALIGNED(virt, mfn) && nr_mfns >= (1UL << (L3_PAGETABLE_SHIFT - PAGE_SHIFT)) && !(flags & (_PAGE_PAT | MAP_SMALL_PAGES)) ) { @@ -5370,8 +5368,7 @@ int map_pages_to_xen( if ( !pl2e ) goto out; - if ( ((((virt >> PAGE_SHIFT) | mfn_x(mfn)) & - ((1u << PAGETABLE_ORDER) - 1)) == 0) && + if ( IS_L2E_ALIGNED(virt, mfn) && (nr_mfns >= (1u << PAGETABLE_ORDER)) && !(flags & (_PAGE_PAT|MAP_SMALL_PAGES)) ) { @@ -5541,9 +5538,7 @@ int map_pages_to_xen( check_l3: if ( cpu_has_page1gb && (flags == PAGE_HYPERVISOR) && - ((nr_mfns == 0) || - !(((virt >> PAGE_SHIFT) | mfn_x(mfn)) & - ((1UL << (L3_PAGETABLE_SHIFT - PAGE_SHIFT)) - 1))) ) + ((nr_mfns == 0) || IS_L3E_ALIGNED(virt, mfn)) ) { unsigned long base_mfn; const l2_pgentry_t *l2t;
Split the code that detects whether the physical and linear address of a mapping request are suitable to be used in an L3 or L2 slot. No functional change intended. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- xen/arch/x86/include/asm/page.h | 6 ++++++ xen/arch/x86/mm.c | 11 +++-------- 2 files changed, 9 insertions(+), 8 deletions(-)