| Message ID | 1479168677-23633-2-git-send-email-athorlton@sgi.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
* Alex Thorlton <athorlton@sgi.com> wrote: > On systems with sufficiently large e820 tables, and several IOAPICs, it > is possible for the XENMEM_machine_memory_map callback (and its > counterpart, XENMEM_memory_map) to attempt to return an e820 table with > more than 128 entries. This callback adds entries to the BIOS-provided > e820 table to account for IOAPIC registers, which, on sufficiently large > systems, can result in an e820 table that is too large to copy back into > xen_e820_map. > > This change simply increases the size of xen_e820_map to E820_X_MAX to > ensure that there is enough room to store the entire e820 map returned > from this callback. This means: #ifdef CONFIG_EFI #include <linux/numa.h> #define E820_X_MAX (E820MAX + 3 * MAX_NUMNODES) #else /* ! CONFIG_EFI */ #define E820_X_MAX E820MAX #endif It's a bit weird to key it off of CONFIG_EFI - why isn't it unconditional? But I have no objections to the principle if it fixes a crash. Thanks, Ingo
On 15/11/16 01:11, Alex Thorlton wrote: > On systems with sufficiently large e820 tables, and several IOAPICs, it > is possible for the XENMEM_machine_memory_map callback (and its > counterpart, XENMEM_memory_map) to attempt to return an e820 table with > more than 128 entries. This callback adds entries to the BIOS-provided > e820 table to account for IOAPIC registers, which, on sufficiently large > systems, can result in an e820 table that is too large to copy back into > xen_e820_map. > > This change simply increases the size of xen_e820_map to E820_X_MAX to > ensure that there is enough room to store the entire e820 map returned > from this callback. > > Signed-off-by: Alex Thorlton <athorlton@sgi.com> > Suggested-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> > Cc: Russ Anderson <rja@sgi.com> > Cc: David Vrabel <david.vrabel@citrix.com> > Cc: Juergen Gross <jgross@suse.com> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Ingo Molnar <mingo@redhat.com> > Cc: "H. Peter Anvin" <hpa@zytor.com> > Cc: x86@kernel.org > Cc: xen-devel@lists.xenproject.org > --- > arch/x86/xen/setup.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/arch/x86/xen/setup.c b/arch/x86/xen/setup.c > index f8960fc..5e1ecc7 100644 > --- a/arch/x86/xen/setup.c > +++ b/arch/x86/xen/setup.c > @@ -41,7 +41,7 @@ > unsigned long xen_released_pages; > > /* E820 map used during setting up memory. */ > -static struct e820entry xen_e820_map[E820MAX] __initdata; > +static struct e820entry xen_e820_map[E820_X_MAX] __initdata; > static u32 xen_e820_map_entries __initdata; > > /* > @@ -750,7 +750,7 @@ char * __init xen_memory_setup(void) > max_pfn = min(max_pfn, xen_start_info->nr_pages); > mem_end = PFN_PHYS(max_pfn); > > - memmap.nr_entries = E820MAX; > + memmap.nr_entries = E820_X_MAX; Please use ARRAY_SIZE(xen_e820_map) here and ... > set_xen_guest_handle(memmap.buffer, xen_e820_map); > > op = xen_initial_domain() ? > @@ -923,7 +923,7 @@ char * __init xen_auto_xlated_memory_setup(void) > int i; > int rc; > > - memmap.nr_entries = E820MAX; > + memmap.nr_entries = E820_X_MAX; ... here. Juergen
On Tue, Nov 15, 2016 at 07:30:35AM +0100, Ingo Molnar wrote: > > * Alex Thorlton <athorlton@sgi.com> wrote: > > > On systems with sufficiently large e820 tables, and several IOAPICs, it > > is possible for the XENMEM_machine_memory_map callback (and its > > counterpart, XENMEM_memory_map) to attempt to return an e820 table with > > more than 128 entries. This callback adds entries to the BIOS-provided > > e820 table to account for IOAPIC registers, which, on sufficiently large > > systems, can result in an e820 table that is too large to copy back into > > xen_e820_map. > > > > This change simply increases the size of xen_e820_map to E820_X_MAX to > > ensure that there is enough room to store the entire e820 map returned > > from this callback. > > This means: > > #ifdef CONFIG_EFI > #include <linux/numa.h> > #define E820_X_MAX (E820MAX + 3 * MAX_NUMNODES) > #else /* ! CONFIG_EFI */ > #define E820_X_MAX E820MAX > #endif > > It's a bit weird to key it off of CONFIG_EFI - why isn't it unconditional? > > But I have no objections to the principle if it fixes a crash. I originally thought that Juergen and Jan's alternative solution eliminated the need for my change, but it appears that I was incorrect, at least if we want to get the kernel booting on the older HV without having to modify it. I'll remove the ifdef here to make this unconditional. Thanks, Ingo! - Alex
On Wed, Nov 16, 2016 at 07:09:01AM +0100, Juergen Gross wrote: > On 15/11/16 01:11, Alex Thorlton wrote: > > On systems with sufficiently large e820 tables, and several IOAPICs, it > > is possible for the XENMEM_machine_memory_map callback (and its > > counterpart, XENMEM_memory_map) to attempt to return an e820 table with > > more than 128 entries. This callback adds entries to the BIOS-provided > > e820 table to account for IOAPIC registers, which, on sufficiently large > > systems, can result in an e820 table that is too large to copy back into > > xen_e820_map. > > > > This change simply increases the size of xen_e820_map to E820_X_MAX to > > ensure that there is enough room to store the entire e820 map returned > > from this callback. > > > > Signed-off-by: Alex Thorlton <athorlton@sgi.com> > > Suggested-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> > > Cc: Russ Anderson <rja@sgi.com> > > Cc: David Vrabel <david.vrabel@citrix.com> > > Cc: Juergen Gross <jgross@suse.com> > > Cc: Thomas Gleixner <tglx@linutronix.de> > > Cc: Ingo Molnar <mingo@redhat.com> > > Cc: "H. Peter Anvin" <hpa@zytor.com> > > Cc: x86@kernel.org > > Cc: xen-devel@lists.xenproject.org > > --- > > arch/x86/xen/setup.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/arch/x86/xen/setup.c b/arch/x86/xen/setup.c > > index f8960fc..5e1ecc7 100644 > > --- a/arch/x86/xen/setup.c > > +++ b/arch/x86/xen/setup.c > > @@ -41,7 +41,7 @@ > > unsigned long xen_released_pages; > > > > /* E820 map used during setting up memory. */ > > -static struct e820entry xen_e820_map[E820MAX] __initdata; > > +static struct e820entry xen_e820_map[E820_X_MAX] __initdata; > > static u32 xen_e820_map_entries __initdata; > > > > /* > > @@ -750,7 +750,7 @@ char * __init xen_memory_setup(void) > > max_pfn = min(max_pfn, xen_start_info->nr_pages); > > mem_end = PFN_PHYS(max_pfn); > > > > - memmap.nr_entries = E820MAX; > > + memmap.nr_entries = E820_X_MAX; > > Please use ARRAY_SIZE(xen_e820_map) here and ... > > > set_xen_guest_handle(memmap.buffer, xen_e820_map); > > > > op = xen_initial_domain() ? > > @@ -923,7 +923,7 @@ char * __init xen_auto_xlated_memory_setup(void) > > int i; > > int rc; > > > > - memmap.nr_entries = E820MAX; > > + memmap.nr_entries = E820_X_MAX; > > ... here. Got it. Thanks, Juergen! - Alex
diff --git a/arch/x86/xen/setup.c b/arch/x86/xen/setup.c index f8960fc..5e1ecc7 100644 --- a/arch/x86/xen/setup.c +++ b/arch/x86/xen/setup.c @@ -41,7 +41,7 @@ unsigned long xen_released_pages; /* E820 map used during setting up memory. */ -static struct e820entry xen_e820_map[E820MAX] __initdata; +static struct e820entry xen_e820_map[E820_X_MAX] __initdata; static u32 xen_e820_map_entries __initdata; /* @@ -750,7 +750,7 @@ char * __init xen_memory_setup(void) max_pfn = min(max_pfn, xen_start_info->nr_pages); mem_end = PFN_PHYS(max_pfn); - memmap.nr_entries = E820MAX; + memmap.nr_entries = E820_X_MAX; set_xen_guest_handle(memmap.buffer, xen_e820_map); op = xen_initial_domain() ? @@ -923,7 +923,7 @@ char * __init xen_auto_xlated_memory_setup(void) int i; int rc; - memmap.nr_entries = E820MAX; + memmap.nr_entries = E820_X_MAX; set_xen_guest_handle(memmap.buffer, xen_e820_map); rc = HYPERVISOR_memory_op(XENMEM_memory_map, &memmap);
On systems with sufficiently large e820 tables, and several IOAPICs, it is possible for the XENMEM_machine_memory_map callback (and its counterpart, XENMEM_memory_map) to attempt to return an e820 table with more than 128 entries. This callback adds entries to the BIOS-provided e820 table to account for IOAPIC registers, which, on sufficiently large systems, can result in an e820 table that is too large to copy back into xen_e820_map. This change simply increases the size of xen_e820_map to E820_X_MAX to ensure that there is enough room to store the entire e820 map returned from this callback. Signed-off-by: Alex Thorlton <athorlton@sgi.com> Suggested-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> Cc: Russ Anderson <rja@sgi.com> Cc: David Vrabel <david.vrabel@citrix.com> Cc: Juergen Gross <jgross@suse.com> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Ingo Molnar <mingo@redhat.com> Cc: "H. Peter Anvin" <hpa@zytor.com> Cc: x86@kernel.org Cc: xen-devel@lists.xenproject.org --- arch/x86/xen/setup.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)