INVALID_MFN is ~0, so by it having all bits as 1s it doesn't fulfill the
super-page address alignment checks for L3 and L2 entries. Skip the alignment
checks if the new entry is a non-present one.
This fixes a regression introduced by 0b6b51a69f4d, where the switch from 0 to
INVALID_MFN caused all super-pages to be shattered when attempting to remove
mappings by passing INVALID_MFN instead of 0.
Fixes: 0b6b51a69f4d ('xen/mm: Switch map_pages_to_xen to use MFN typesafe')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
Changees since v2:
- Remove unneeded page present check.
Changes since v1:
- Detect non-present entries from the flags contents rather than checking the
mfn parameter.
---
xen/arch/x86/mm.c | 20 ++++++++++++++------
1 file changed, 14 insertions(+), 6 deletions(-)
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index ebb50a7836ac..b9a2234b53e1 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -5493,10 +5493,17 @@ int map_pages_to_xen(
} \
} while (0)
-/* Check if a (virt, mfn) tuple is aligned for a given slot level. */
-#define IS_LnE_ALIGNED(v, m, n) \
- IS_ALIGNED(PFN_DOWN(v) | mfn_x(m), \
- (1UL << (PAGETABLE_ORDER * ((n) - 1))) - 1)
+/*
+ * Check if a (virt, mfn) tuple is aligned for a given slot level. m must not
+ * be INVALID_MFN, since alignment is only relevant for present entries.
+ */
+#define IS_LnE_ALIGNED(v, m, n) ({ \
+ mfn_t m_ = m; \
+ \
+ ASSERT(!mfn_eq(m_, INVALID_MFN)); \
+ IS_ALIGNED(PFN_DOWN(v) | mfn_x(m_), \
+ (1UL << (PAGETABLE_ORDER * ((n) - 1))) - 1); \
+})
#define IS_L2E_ALIGNED(v, m) IS_LnE_ALIGNED(v, m, 2)
#define IS_L3E_ALIGNED(v, m) IS_LnE_ALIGNED(v, m, 3)
@@ -5517,7 +5524,8 @@ int map_pages_to_xen(
L3T_LOCK(current_l3page);
ol3e = *pl3e;
- if ( cpu_has_page1gb && IS_L3E_ALIGNED(virt, mfn) &&
+ if ( cpu_has_page1gb &&
+ (!(flags & _PAGE_PRESENT) || IS_L3E_ALIGNED(virt, mfn)) &&
nr_mfns >= (1UL << (L3_PAGETABLE_SHIFT - PAGE_SHIFT)) &&
!(flags & (_PAGE_PAT | MAP_SMALL_PAGES)) )
{
@@ -5636,7 +5644,7 @@ int map_pages_to_xen(
if ( !pl2e )
goto out;
- if ( IS_L2E_ALIGNED(virt, mfn) &&
+ if ( (!(flags & _PAGE_PRESENT) || IS_L2E_ALIGNED(virt, mfn)) &&
(nr_mfns >= (1u << PAGETABLE_ORDER)) &&
!(flags & (_PAGE_PAT|MAP_SMALL_PAGES)) )
{
--
2.46.0
On 14.11.2024 15:57, Roger Pau Monne wrote:
> @@ -5517,7 +5524,8 @@ int map_pages_to_xen(
> L3T_LOCK(current_l3page);
> ol3e = *pl3e;
>
> - if ( cpu_has_page1gb && IS_L3E_ALIGNED(virt, mfn) &&
> + if ( cpu_has_page1gb &&
> + (!(flags & _PAGE_PRESENT) || IS_L3E_ALIGNED(virt, mfn)) &&
> nr_mfns >= (1UL << (L3_PAGETABLE_SHIFT - PAGE_SHIFT)) &&
> !(flags & (_PAGE_PAT | MAP_SMALL_PAGES)) )
> {
> @@ -5636,7 +5644,7 @@ int map_pages_to_xen(
> if ( !pl2e )
> goto out;
>
> - if ( IS_L2E_ALIGNED(virt, mfn) &&
> + if ( (!(flags & _PAGE_PRESENT) || IS_L2E_ALIGNED(virt, mfn)) &&
> (nr_mfns >= (1u << PAGETABLE_ORDER)) &&
> !(flags & (_PAGE_PAT|MAP_SMALL_PAGES)) )
> {
Upon inspecting Andrew's report of crashes I noticed that this can't be quite
right. We can't entirely skip the alignment check when non-present mappings
are requested; we merely need to limit the check to the VA. In a reply to
the 1st v2 I actually had it arranged to match that requirement:
if ( (cpu_has_page1gb || !(flags & _PAGE_PRESENT)) &&
IS_L3E_ALIGNED(virt, flags & _PAGE_PRESENT ? mfn : _mfn(0)) &&
nr_mfns >= (1UL << (L3_PAGETABLE_SHIFT - PAGE_SHIFT)) &&
!(flags & (_PAGE_PAT | MAP_SMALL_PAGES)) )
Yet then I didn't pay attention to the difference when reviewing the 2nd v2
(that versioning issue of course isn't helping here either).
I'm afraid I can't (yet) connect the observed bad behavior with this issue,
though.
Jan
On 15.11.2024 10:09, Jan Beulich wrote:
> On 14.11.2024 15:57, Roger Pau Monne wrote:
>> @@ -5517,7 +5524,8 @@ int map_pages_to_xen(
>> L3T_LOCK(current_l3page);
>> ol3e = *pl3e;
>>
>> - if ( cpu_has_page1gb && IS_L3E_ALIGNED(virt, mfn) &&
>> + if ( cpu_has_page1gb &&
>> + (!(flags & _PAGE_PRESENT) || IS_L3E_ALIGNED(virt, mfn)) &&
>> nr_mfns >= (1UL << (L3_PAGETABLE_SHIFT - PAGE_SHIFT)) &&
>> !(flags & (_PAGE_PAT | MAP_SMALL_PAGES)) )
>> {
>> @@ -5636,7 +5644,7 @@ int map_pages_to_xen(
>> if ( !pl2e )
>> goto out;
>>
>> - if ( IS_L2E_ALIGNED(virt, mfn) &&
>> + if ( (!(flags & _PAGE_PRESENT) || IS_L2E_ALIGNED(virt, mfn)) &&
>> (nr_mfns >= (1u << PAGETABLE_ORDER)) &&
>> !(flags & (_PAGE_PAT|MAP_SMALL_PAGES)) )
>> {
>
> Upon inspecting Andrew's report of crashes I noticed that this can't be quite
> right. We can't entirely skip the alignment check when non-present mappings
> are requested; we merely need to limit the check to the VA. In a reply to
> the 1st v2 I actually had it arranged to match that requirement:
>
> if ( (cpu_has_page1gb || !(flags & _PAGE_PRESENT)) &&
> IS_L3E_ALIGNED(virt, flags & _PAGE_PRESENT ? mfn : _mfn(0)) &&
> nr_mfns >= (1UL << (L3_PAGETABLE_SHIFT - PAGE_SHIFT)) &&
> !(flags & (_PAGE_PAT | MAP_SMALL_PAGES)) )
>
> Yet then I didn't pay attention to the difference when reviewing the 2nd v2
> (that versioning issue of course isn't helping here either).
>
> I'm afraid I can't (yet) connect the observed bad behavior with this issue,
> though.
I think I now can: memnodemap is set using vmap_contig(). The VESA frame buffer
unmap, neglecting to check VA alignment, will have wrongly unmapped memnodemap.
Jan
On Fri, Nov 15, 2024 at 10:09:39AM +0100, Jan Beulich wrote:
> On 14.11.2024 15:57, Roger Pau Monne wrote:
> > @@ -5517,7 +5524,8 @@ int map_pages_to_xen(
> > L3T_LOCK(current_l3page);
> > ol3e = *pl3e;
> >
> > - if ( cpu_has_page1gb && IS_L3E_ALIGNED(virt, mfn) &&
> > + if ( cpu_has_page1gb &&
> > + (!(flags & _PAGE_PRESENT) || IS_L3E_ALIGNED(virt, mfn)) &&
> > nr_mfns >= (1UL << (L3_PAGETABLE_SHIFT - PAGE_SHIFT)) &&
> > !(flags & (_PAGE_PAT | MAP_SMALL_PAGES)) )
> > {
> > @@ -5636,7 +5644,7 @@ int map_pages_to_xen(
> > if ( !pl2e )
> > goto out;
> >
> > - if ( IS_L2E_ALIGNED(virt, mfn) &&
> > + if ( (!(flags & _PAGE_PRESENT) || IS_L2E_ALIGNED(virt, mfn)) &&
> > (nr_mfns >= (1u << PAGETABLE_ORDER)) &&
> > !(flags & (_PAGE_PAT|MAP_SMALL_PAGES)) )
> > {
>
> Upon inspecting Andrew's report of crashes I noticed that this can't be quite
> right. We can't entirely skip the alignment check when non-present mappings
> are requested; we merely need to limit the check to the VA. In a reply to
> the 1st v2 I actually had it arranged to match that requirement:
>
> if ( (cpu_has_page1gb || !(flags & _PAGE_PRESENT)) &&
> IS_L3E_ALIGNED(virt, flags & _PAGE_PRESENT ? mfn : _mfn(0)) &&
> nr_mfns >= (1UL << (L3_PAGETABLE_SHIFT - PAGE_SHIFT)) &&
> !(flags & (_PAGE_PAT | MAP_SMALL_PAGES)) )
>
> Yet then I didn't pay attention to the difference when reviewing the 2nd v2
> (that versioning issue of course isn't helping here either).
>
> I'm afraid I can't (yet) connect the observed bad behavior with this issue,
> though.
I think this might be caused by map_pages_to_xen() now freeing vmap
regions still under use, and that seems to manifest with the
memnodemap array being unintentionally unmapped (because it's
allocated with vmap_contig()). See the usage of mfn_to_nid() in
free_heap_pages().
Will prepare a patch, sorry.
Thanks, Roger.
© 2016 - 2024 Red Hat, Inc.