| Message ID | 20210914094108.22482-1-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/setup: call early_reserve_memory() earlier | expand |
On 14.09.2021 11:41, Juergen Gross wrote: > Commit a799c2bd29d19c565 ("x86/setup: Consolidate early memory > reservations") introduced early_reserve_memory() to do all needed > initial memblock_reserve() calls in one function. Unfortunately the > call of early_reserve_memory() is done too late for Xen dom0, as in > some cases a Xen hook called by e820__memory_setup() will need those > memory reservations to have happened already. > > Move the call of early_reserve_memory() to the beginning of > setup_arch() in order to avoid such problems. > > Cc: stable@vger.kernel.org > Fixes: a799c2bd29d19c565 ("x86/setup: Consolidate early memory reservations") > Reported-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> > Signed-off-by: Juergen Gross <jgross@suse.com> > --- > arch/x86/kernel/setup.c | 24 ++++++++++++------------ > 1 file changed, 12 insertions(+), 12 deletions(-) > > diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c > index 79f164141116..f369c51ec580 100644 > --- a/arch/x86/kernel/setup.c > +++ b/arch/x86/kernel/setup.c > @@ -757,6 +757,18 @@ dump_kernel_offset(struct notifier_block *self, unsigned long v, void *p) > > void __init setup_arch(char **cmdline_p) > { > + /* > + * Do some memory reservations *before* memory is added to > + * memblock, so memblock allocations won't overwrite it. > + * Do it after early param, so we could get (unlikely) panic from > + * serial. Hmm, this part of the comment is not only stale now, but gets actively undermined. No idea how likely such a panic() would be, and hence how relevant it is to retain this particular property. Jan > + * After this point everything still needed from the boot loader or > + * firmware or kernel text should be early reserved or marked not > + * RAM in e820. All other memory is free game. > + */ > + early_reserve_memory(); > + > #ifdef CONFIG_X86_32 > memcpy(&boot_cpu_data, &new_cpu_data, sizeof(new_cpu_data)); > > @@ -876,18 +888,6 @@ void __init setup_arch(char **cmdline_p) > > parse_early_param(); > > - /* > - * Do some memory reservations *before* memory is added to > - * memblock, so memblock allocations won't overwrite it. > - * Do it after early param, so we could get (unlikely) panic from > - * serial. > - * > - * After this point everything still needed from the boot loader or > - * firmware or kernel text should be early reserved or marked not > - * RAM in e820. All other memory is free game. > - */ > - early_reserve_memory(); > - > #ifdef CONFIG_MEMORY_HOTPLUG > /* > * Memory used by the kernel cannot be hot-removed because Linux >
On Tue, Sep 14, 2021 at 11:41:08AM +0200, Juergen Gross wrote: > Commit a799c2bd29d19c565 ("x86/setup: Consolidate early memory > reservations") introduced early_reserve_memory() to do all needed > initial memblock_reserve() calls in one function. Unfortunately the > call of early_reserve_memory() is done too late for Xen dom0, as in > some cases a Xen hook called by e820__memory_setup() will need those > memory reservations to have happened already. > > Move the call of early_reserve_memory() to the beginning of > setup_arch() in order to avoid such problems. > > Cc: stable@vger.kernel.org > Fixes: a799c2bd29d19c565 ("x86/setup: Consolidate early memory reservations") > Reported-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> > Signed-off-by: Juergen Gross <jgross@suse.com> Thanks, it works! Tested-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> > --- > arch/x86/kernel/setup.c | 24 ++++++++++++------------ > 1 file changed, 12 insertions(+), 12 deletions(-) > > diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c > index 79f164141116..f369c51ec580 100644 > --- a/arch/x86/kernel/setup.c > +++ b/arch/x86/kernel/setup.c > @@ -757,6 +757,18 @@ dump_kernel_offset(struct notifier_block *self, unsigned long v, void *p) > > void __init setup_arch(char **cmdline_p) > { > + /* > + * Do some memory reservations *before* memory is added to > + * memblock, so memblock allocations won't overwrite it. > + * Do it after early param, so we could get (unlikely) panic from > + * serial. > + * > + * After this point everything still needed from the boot loader or > + * firmware or kernel text should be early reserved or marked not > + * RAM in e820. All other memory is free game. > + */ > + early_reserve_memory(); > + > #ifdef CONFIG_X86_32 > memcpy(&boot_cpu_data, &new_cpu_data, sizeof(new_cpu_data)); > > @@ -876,18 +888,6 @@ void __init setup_arch(char **cmdline_p) > > parse_early_param(); > > - /* > - * Do some memory reservations *before* memory is added to > - * memblock, so memblock allocations won't overwrite it. > - * Do it after early param, so we could get (unlikely) panic from > - * serial. > - * > - * After this point everything still needed from the boot loader or > - * firmware or kernel text should be early reserved or marked not > - * RAM in e820. All other memory is free game. > - */ > - early_reserve_memory(); > - > #ifdef CONFIG_MEMORY_HOTPLUG > /* > * Memory used by the kernel cannot be hot-removed because Linux > -- > 2.26.2 >
On 14.09.21 12:03, Jan Beulich wrote: > On 14.09.2021 11:41, Juergen Gross wrote: >> Commit a799c2bd29d19c565 ("x86/setup: Consolidate early memory >> reservations") introduced early_reserve_memory() to do all needed >> initial memblock_reserve() calls in one function. Unfortunately the >> call of early_reserve_memory() is done too late for Xen dom0, as in >> some cases a Xen hook called by e820__memory_setup() will need those >> memory reservations to have happened already. >> >> Move the call of early_reserve_memory() to the beginning of >> setup_arch() in order to avoid such problems. >> >> Cc: stable@vger.kernel.org >> Fixes: a799c2bd29d19c565 ("x86/setup: Consolidate early memory reservations") >> Reported-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> >> Signed-off-by: Juergen Gross <jgross@suse.com> >> --- >> arch/x86/kernel/setup.c | 24 ++++++++++++------------ >> 1 file changed, 12 insertions(+), 12 deletions(-) >> >> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c >> index 79f164141116..f369c51ec580 100644 >> --- a/arch/x86/kernel/setup.c >> +++ b/arch/x86/kernel/setup.c >> @@ -757,6 +757,18 @@ dump_kernel_offset(struct notifier_block *self, unsigned long v, void *p) >> >> void __init setup_arch(char **cmdline_p) >> { >> + /* >> + * Do some memory reservations *before* memory is added to >> + * memblock, so memblock allocations won't overwrite it. >> + * Do it after early param, so we could get (unlikely) panic from >> + * serial. > > Hmm, this part of the comment is not only stale now, but gets actively > undermined. No idea how likely such a panic() would be, and hence how > relevant it is to retain this particular property. Ah, right. The alternative would be to split it up again. Let's let the x86 maintainers decide which way is the better one. Juergen > > Jan > >> + * After this point everything still needed from the boot loader or >> + * firmware or kernel text should be early reserved or marked not >> + * RAM in e820. All other memory is free game. >> + */ >> + early_reserve_memory(); >> + >> #ifdef CONFIG_X86_32 >> memcpy(&boot_cpu_data, &new_cpu_data, sizeof(new_cpu_data)); >> >> @@ -876,18 +888,6 @@ void __init setup_arch(char **cmdline_p) >> >> parse_early_param(); >> >> - /* >> - * Do some memory reservations *before* memory is added to >> - * memblock, so memblock allocations won't overwrite it. >> - * Do it after early param, so we could get (unlikely) panic from >> - * serial. >> - * >> - * After this point everything still needed from the boot loader or >> - * firmware or kernel text should be early reserved or marked not >> - * RAM in e820. All other memory is free game. >> - */ >> - early_reserve_memory(); >> - >> #ifdef CONFIG_MEMORY_HOTPLUG >> /* >> * Memory used by the kernel cannot be hot-removed because Linux >> >
You forgot to Cc Mike, lemme add him. And drop stable@ too. On Tue, Sep 14, 2021 at 01:06:22PM +0200, Juergen Gross wrote: > On 14.09.21 12:03, Jan Beulich wrote: > > On 14.09.2021 11:41, Juergen Gross wrote: > > > Commit a799c2bd29d19c565 ("x86/setup: Consolidate early memory > > > reservations") introduced early_reserve_memory() to do all needed > > > initial memblock_reserve() calls in one function. Unfortunately the > > > call of early_reserve_memory() is done too late for Xen dom0, as in > > > some cases a Xen hook called by e820__memory_setup() will need those > > > memory reservations to have happened already. > > > > > > Move the call of early_reserve_memory() to the beginning of > > > setup_arch() in order to avoid such problems. > > > > > > Cc: stable@vger.kernel.org > > > Fixes: a799c2bd29d19c565 ("x86/setup: Consolidate early memory reservations") > > > Reported-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> > > > Signed-off-by: Juergen Gross <jgross@suse.com> > > > --- > > > arch/x86/kernel/setup.c | 24 ++++++++++++------------ > > > 1 file changed, 12 insertions(+), 12 deletions(-) > > > > > > diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c > > > index 79f164141116..f369c51ec580 100644 > > > --- a/arch/x86/kernel/setup.c > > > +++ b/arch/x86/kernel/setup.c > > > @@ -757,6 +757,18 @@ dump_kernel_offset(struct notifier_block *self, unsigned long v, void *p) > > > void __init setup_arch(char **cmdline_p) > > > { > > > + /* > > > + * Do some memory reservations *before* memory is added to > > > + * memblock, so memblock allocations won't overwrite it. > > > + * Do it after early param, so we could get (unlikely) panic from > > > + * serial. > > > > Hmm, this part of the comment is not only stale now, but gets actively > > undermined. No idea how likely such a panic() would be, and hence how > > relevant it is to retain this particular property. > > Ah, right. > > The alternative would be to split it up again. Let's let the x86 > maintainers decide which way is the better one. > > > Juergen > > > > > Jan > > > > > + * After this point everything still needed from the boot loader or > > > + * firmware or kernel text should be early reserved or marked not > > > + * RAM in e820. All other memory is free game. > > > + */ > > > + early_reserve_memory(); > > > + > > > #ifdef CONFIG_X86_32 > > > memcpy(&boot_cpu_data, &new_cpu_data, sizeof(new_cpu_data)); > > > @@ -876,18 +888,6 @@ void __init setup_arch(char **cmdline_p) > > > parse_early_param(); > > > - /* > > > - * Do some memory reservations *before* memory is added to > > > - * memblock, so memblock allocations won't overwrite it. > > > - * Do it after early param, so we could get (unlikely) panic from > > > - * serial. > > > - * > > > - * After this point everything still needed from the boot loader or > > > - * firmware or kernel text should be early reserved or marked not > > > - * RAM in e820. All other memory is free game. > > > - */ > > > - early_reserve_memory(); > > > - > > > #ifdef CONFIG_MEMORY_HOTPLUG > > > /* > > > * Memory used by the kernel cannot be hot-removed because Linux > > > > > >
On Wed, Sep 15, 2021 at 01:00:20PM +0200, Borislav Petkov wrote: > You forgot to Cc Mike, lemme add him. > > And drop stable@ too. > > On Tue, Sep 14, 2021 at 01:06:22PM +0200, Juergen Gross wrote: > > On 14.09.21 12:03, Jan Beulich wrote: > > > On 14.09.2021 11:41, Juergen Gross wrote: > > > > Commit a799c2bd29d19c565 ("x86/setup: Consolidate early memory > > > > reservations") introduced early_reserve_memory() to do all needed > > > > initial memblock_reserve() calls in one function. Unfortunately the > > > > call of early_reserve_memory() is done too late for Xen dom0, as in > > > > some cases a Xen hook called by e820__memory_setup() will need those > > > > memory reservations to have happened already. > > > > > > > > Move the call of early_reserve_memory() to the beginning of > > > > setup_arch() in order to avoid such problems. > > > > > > > > Cc: stable@vger.kernel.org > > > > Fixes: a799c2bd29d19c565 ("x86/setup: Consolidate early memory reservations") > > > > Reported-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> > > > > Signed-off-by: Juergen Gross <jgross@suse.com> > > > > --- > > > > arch/x86/kernel/setup.c | 24 ++++++++++++------------ > > > > 1 file changed, 12 insertions(+), 12 deletions(-) > > > > > > > > diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c > > > > index 79f164141116..f369c51ec580 100644 > > > > --- a/arch/x86/kernel/setup.c > > > > +++ b/arch/x86/kernel/setup.c > > > > @@ -757,6 +757,18 @@ dump_kernel_offset(struct notifier_block *self, unsigned long v, void *p) > > > > void __init setup_arch(char **cmdline_p) > > > > { > > > > + /* > > > > + * Do some memory reservations *before* memory is added to > > > > + * memblock, so memblock allocations won't overwrite it. > > > > + * Do it after early param, so we could get (unlikely) panic from > > > > + * serial. > > > > > > Hmm, this part of the comment is not only stale now, but gets actively > > > undermined. No idea how likely such a panic() would be, and hence how > > > relevant it is to retain this particular property. > > > > Ah, right. > > > > The alternative would be to split it up again. Let's let the x86 > > maintainers decide which way is the better one. I think the first sentence about reserving memory before memblock allocations are possible is important and I think we should keep it. With that Acked-by: Mike Rapoport <rppt@linux.ibm.com> > > > > > > Juergen > > > > > > > > Jan > > > > > > > + * After this point everything still needed from the boot loader or > > > > + * firmware or kernel text should be early reserved or marked not > > > > + * RAM in e820. All other memory is free game. > > > > + */ > > > > + early_reserve_memory(); > > > > + > > > > #ifdef CONFIG_X86_32 > > > > memcpy(&boot_cpu_data, &new_cpu_data, sizeof(new_cpu_data)); > > > > @@ -876,18 +888,6 @@ void __init setup_arch(char **cmdline_p) > > > > parse_early_param(); > > > > - /* > > > > - * Do some memory reservations *before* memory is added to > > > > - * memblock, so memblock allocations won't overwrite it. > > > > - * Do it after early param, so we could get (unlikely) panic from > > > > - * serial. > > > > - * > > > > - * After this point everything still needed from the boot loader or > > > > - * firmware or kernel text should be early reserved or marked not > > > > - * RAM in e820. All other memory is free game. > > > > - */ > > > > - early_reserve_memory(); > > > > - > > > > #ifdef CONFIG_MEMORY_HOTPLUG > > > > /* > > > > * Memory used by the kernel cannot be hot-removed because Linux > > > > > > > > > > > > > > > > -- > Regards/Gruss, > Boris. > > https://people.kernel.org/tglx/notes-about-netiquette
On Thu, Sep 16, 2021 at 12:09:27PM +0300, Mike Rapoport wrote: > I think the first sentence about reserving memory before memblock > allocations are possible is important and I think we should keep it. I expanded that comment this way: /* * Do some memory reservations *before* memory is added to memblock, so * memblock allocations won't overwrite it. * * After this point, everything still needed from the boot loader or * firmware or kernel text should be early reserved or marked not RAM in * e820. All other memory is free game. * * This call needs to happen before e820__memory_setup() which calls the * xen_memory_setup() on Xen dom0 which relies on the fact that those * early reservations have happened already. */
On 16.09.21 12:29, Borislav Petkov wrote: > On Thu, Sep 16, 2021 at 12:09:27PM +0300, Mike Rapoport wrote: >> I think the first sentence about reserving memory before memblock >> allocations are possible is important and I think we should keep it. > > I expanded that comment this way: > > /* > * Do some memory reservations *before* memory is added to memblock, so > * memblock allocations won't overwrite it. > * > * After this point, everything still needed from the boot loader or > * firmware or kernel text should be early reserved or marked not RAM in > * e820. All other memory is free game. > * > * This call needs to happen before e820__memory_setup() which calls the > * xen_memory_setup() on Xen dom0 which relies on the fact that those > * early reservations have happened already. > */ > Yes, this seems to be an accurate description. Juergen
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c index 79f164141116..f369c51ec580 100644 --- a/arch/x86/kernel/setup.c +++ b/arch/x86/kernel/setup.c @@ -757,6 +757,18 @@ dump_kernel_offset(struct notifier_block *self, unsigned long v, void *p) void __init setup_arch(char **cmdline_p) { + /* + * Do some memory reservations *before* memory is added to + * memblock, so memblock allocations won't overwrite it. + * Do it after early param, so we could get (unlikely) panic from + * serial. + * + * After this point everything still needed from the boot loader or + * firmware or kernel text should be early reserved or marked not + * RAM in e820. All other memory is free game. + */ + early_reserve_memory(); + #ifdef CONFIG_X86_32 memcpy(&boot_cpu_data, &new_cpu_data, sizeof(new_cpu_data)); @@ -876,18 +888,6 @@ void __init setup_arch(char **cmdline_p) parse_early_param(); - /* - * Do some memory reservations *before* memory is added to - * memblock, so memblock allocations won't overwrite it. - * Do it after early param, so we could get (unlikely) panic from - * serial. - * - * After this point everything still needed from the boot loader or - * firmware or kernel text should be early reserved or marked not - * RAM in e820. All other memory is free game. - */ - early_reserve_memory(); - #ifdef CONFIG_MEMORY_HOTPLUG /* * Memory used by the kernel cannot be hot-removed because Linux
Commit a799c2bd29d19c565 ("x86/setup: Consolidate early memory reservations") introduced early_reserve_memory() to do all needed initial memblock_reserve() calls in one function. Unfortunately the call of early_reserve_memory() is done too late for Xen dom0, as in some cases a Xen hook called by e820__memory_setup() will need those memory reservations to have happened already. Move the call of early_reserve_memory() to the beginning of setup_arch() in order to avoid such problems. Cc: stable@vger.kernel.org Fixes: a799c2bd29d19c565 ("x86/setup: Consolidate early memory reservations") Reported-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> Signed-off-by: Juergen Gross <jgross@suse.com> --- arch/x86/kernel/setup.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-)