It has been 4 years since the default load address changed from 1M to 2M, and
_stext ceased residing in l2_xenmap[0]. We should not be inserting an unused
mapping.
To ensure we don't create/remove mappings accidentally, loop from 0 and obey
_PAGE_PRESENT on all entries.
Fixes: 7ed93f3a0dff ("x86: change default load address from 1 MiB to 2 MiB")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
I ought to have spotted this in c/s 52975142d154 ("x86/boot: Create the
l2_xenmap[] mappings dynamically") too.
---
xen/arch/x86/setup.c | 10 +++-------
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index da47cdea14a1..6f241048425c 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1279,16 +1279,12 @@ void __init noreturn __start_xen(unsigned long mbi_p)
/* The only data mappings to be relocated are in the Xen area. */
pl2e = __va(__pa(l2_xenmap));
- /*
- * Undo the temporary-hooking of the l1_directmap. __2M_text_start
- * is contained in this PTE.
- */
+
BUG_ON(using_2M_mapping() &&
l2_table_offset((unsigned long)_erodata) ==
l2_table_offset((unsigned long)_stext));
- *pl2e++ = l2e_from_pfn(xen_phys_start >> PAGE_SHIFT,
- PAGE_HYPERVISOR_RX | _PAGE_PSE);
- for ( i = 1; i < L2_PAGETABLE_ENTRIES; i++, pl2e++ )
+
+ for ( i = 0; i < L2_PAGETABLE_ENTRIES; i++, pl2e++ )
{
unsigned int flags;
--
2.11.0
On 29/11/2021 17:26, Andrew Cooper wrote: > It has been 4 years since the default load address changed from 1M to 2M, and > _stext ceased residing in l2_xenmap[0]. We should not be inserting an unused > mapping. > > To ensure we don't create/remove mappings accidentally, loop from 0 and obey > _PAGE_PRESENT on all entries. > > Fixes: 7ed93f3a0dff ("x86: change default load address from 1 MiB to 2 MiB") > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: Roger Pau Monné <roger.pau@citrix.com> > CC: Wei Liu <wl@xen.org> > > I ought to have spotted this in c/s 52975142d154 ("x86/boot: Create the > l2_xenmap[] mappings dynamically") too. > --- > xen/arch/x86/setup.c | 10 +++------- > 1 file changed, 3 insertions(+), 7 deletions(-) > > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c > index da47cdea14a1..6f241048425c 100644 > --- a/xen/arch/x86/setup.c > +++ b/xen/arch/x86/setup.c > @@ -1279,16 +1279,12 @@ void __init noreturn __start_xen(unsigned long mbi_p) > > /* The only data mappings to be relocated are in the Xen area. */ > pl2e = __va(__pa(l2_xenmap)); > - /* > - * Undo the temporary-hooking of the l1_directmap. __2M_text_start > - * is contained in this PTE. > - */ > + > BUG_ON(using_2M_mapping() && > l2_table_offset((unsigned long)_erodata) == > l2_table_offset((unsigned long)_stext)); > - *pl2e++ = l2e_from_pfn(xen_phys_start >> PAGE_SHIFT, > - PAGE_HYPERVISOR_RX | _PAGE_PSE); > - for ( i = 1; i < L2_PAGETABLE_ENTRIES; i++, pl2e++ ) > + > + for ( i = 0; i < L2_PAGETABLE_ENTRIES; i++, pl2e++ ) > { > unsigned int flags; > Actually, on further consideration, this hunk should be included too, to cross-check that we don't zap any of the dynamically created mappings. ~Andrew diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index 6f241048425c..5c8026b30e3e 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -1320,7 +1320,7 @@ void __init noreturn __start_xen(unsigned long mbi_p) } else { - *pl2e = l2e_empty(); + ASSERT_UNREACHABLE(); continue; }
On 29.11.2021 18:42, Andrew Cooper wrote: > On 29/11/2021 17:26, Andrew Cooper wrote: >> It has been 4 years since the default load address changed from 1M to 2M, and >> _stext ceased residing in l2_xenmap[0]. We should not be inserting an unused >> mapping. >> >> To ensure we don't create/remove mappings accidentally, loop from 0 and obey >> _PAGE_PRESENT on all entries. >> >> Fixes: 7ed93f3a0dff ("x86: change default load address from 1 MiB to 2 MiB") >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> --- >> CC: Jan Beulich <JBeulich@suse.com> >> CC: Roger Pau Monné <roger.pau@citrix.com> >> CC: Wei Liu <wl@xen.org> >> >> I ought to have spotted this in c/s 52975142d154 ("x86/boot: Create the >> l2_xenmap[] mappings dynamically") too. >> --- >> xen/arch/x86/setup.c | 10 +++------- >> 1 file changed, 3 insertions(+), 7 deletions(-) >> >> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c >> index da47cdea14a1..6f241048425c 100644 >> --- a/xen/arch/x86/setup.c >> +++ b/xen/arch/x86/setup.c >> @@ -1279,16 +1279,12 @@ void __init noreturn __start_xen(unsigned long mbi_p) >> >> /* The only data mappings to be relocated are in the Xen area. */ >> pl2e = __va(__pa(l2_xenmap)); >> - /* >> - * Undo the temporary-hooking of the l1_directmap. __2M_text_start >> - * is contained in this PTE. >> - */ >> + >> BUG_ON(using_2M_mapping() && >> l2_table_offset((unsigned long)_erodata) == >> l2_table_offset((unsigned long)_stext)); >> - *pl2e++ = l2e_from_pfn(xen_phys_start >> PAGE_SHIFT, >> - PAGE_HYPERVISOR_RX | _PAGE_PSE); >> - for ( i = 1; i < L2_PAGETABLE_ENTRIES; i++, pl2e++ ) >> + >> + for ( i = 0; i < L2_PAGETABLE_ENTRIES; i++, pl2e++ ) >> { >> unsigned int flags; >> > > Actually, on further consideration, this hunk should be included too, to > cross-check that we don't zap any of the dynamically created mappings. I agree in principle, but ... > --- a/xen/arch/x86/setup.c > +++ b/xen/arch/x86/setup.c > @@ -1320,7 +1320,7 @@ void __init noreturn __start_xen(unsigned long mbi_p) > } > else > { > - *pl2e = l2e_empty(); > + ASSERT_UNREACHABLE(); > continue; > } ... this isn't going to help non-debug builds. Dropping the zapping would mean release builds then run with an unadjusted PTE. I can see two options: Extract the flags from the existing PTE as fallback (i.e. keeping the ASSERT_UNREACHABLE()) or use BUG() instead. Jan
On 30/11/2021 07:28, Jan Beulich wrote: > On 29.11.2021 18:42, Andrew Cooper wrote: >> On 29/11/2021 17:26, Andrew Cooper wrote: >>> It has been 4 years since the default load address changed from 1M to 2M, and >>> _stext ceased residing in l2_xenmap[0]. We should not be inserting an unused >>> mapping. >>> >>> To ensure we don't create/remove mappings accidentally, loop from 0 and obey >>> _PAGE_PRESENT on all entries. >>> >>> Fixes: 7ed93f3a0dff ("x86: change default load address from 1 MiB to 2 MiB") >>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >>> --- >>> CC: Jan Beulich <JBeulich@suse.com> >>> CC: Roger Pau Monné <roger.pau@citrix.com> >>> CC: Wei Liu <wl@xen.org> >>> >>> I ought to have spotted this in c/s 52975142d154 ("x86/boot: Create the >>> l2_xenmap[] mappings dynamically") too. >>> --- >>> xen/arch/x86/setup.c | 10 +++------- >>> 1 file changed, 3 insertions(+), 7 deletions(-) >>> >>> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c >>> index da47cdea14a1..6f241048425c 100644 >>> --- a/xen/arch/x86/setup.c >>> +++ b/xen/arch/x86/setup.c >>> @@ -1279,16 +1279,12 @@ void __init noreturn __start_xen(unsigned long mbi_p) >>> >>> /* The only data mappings to be relocated are in the Xen area. */ >>> pl2e = __va(__pa(l2_xenmap)); >>> - /* >>> - * Undo the temporary-hooking of the l1_directmap. __2M_text_start >>> - * is contained in this PTE. >>> - */ >>> + >>> BUG_ON(using_2M_mapping() && >>> l2_table_offset((unsigned long)_erodata) == >>> l2_table_offset((unsigned long)_stext)); >>> - *pl2e++ = l2e_from_pfn(xen_phys_start >> PAGE_SHIFT, >>> - PAGE_HYPERVISOR_RX | _PAGE_PSE); >>> - for ( i = 1; i < L2_PAGETABLE_ENTRIES; i++, pl2e++ ) >>> + >>> + for ( i = 0; i < L2_PAGETABLE_ENTRIES; i++, pl2e++ ) >>> { >>> unsigned int flags; >>> >> Actually, on further consideration, this hunk should be included too, to >> cross-check that we don't zap any of the dynamically created mappings. > I agree in principle, but ... > >> --- a/xen/arch/x86/setup.c >> +++ b/xen/arch/x86/setup.c >> @@ -1320,7 +1320,7 @@ void __init noreturn __start_xen(unsigned long mbi_p) >> } >> else >> { >> - *pl2e = l2e_empty(); >> + ASSERT_UNREACHABLE(); >> continue; >> } > ... this isn't going to help non-debug builds. Dropping the zapping would > mean release builds then run with an unadjusted PTE. I can see two options: > Extract the flags from the existing PTE as fallback (i.e. keeping the > ASSERT_UNREACHABLE()) or use BUG() instead. I've actually dropped this entire block of code with a later cleanup, so I'll leave this hunk alone and post the whole series. ~Andrew
© 2016 - 2024 Red Hat, Inc.