| Message ID | 11752259.O9o76ZdvQC@kreacher |
|---|---|
| State | New |
| Headers | show |
| Series | ACPI: x86: Call acpi_boot_table_init() after acpi_table_upgrade() | expand |
On Tue, Apr 13, 2021 at 7:43 PM David Hildenbrand <david@redhat.com> wrote: > > On 13.04.21 16:01, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > Commit 1a1c130ab757 ("ACPI: tables: x86: Reserve memory occupied by > > ACPI tables") attempted to address an issue with reserving the memory > > occupied by ACPI tables, but it broke the initrd-based table override > > mechanism relied on by multiple users. > > > > To restore the initrd-based ACPI table override functionality, move > > the acpi_boot_table_init() invocation in setup_arch() on x86 after > > the acpi_table_upgrade() one. > > > > Fixes: 1a1c130ab757 ("ACPI: tables: x86: Reserve memory occupied by ACPI tables") > > Reported-by: Hans de Goede <hdegoede@redhat.com> > > Tested-by: Hans de Goede <hdegoede@redhat.com> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > --- > > > > George, can you please check if this reintroduces the issue addressed by > > the above commit for you? > > > > --- > > arch/x86/kernel/setup.c | 5 ++--- > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > Index: linux-pm/arch/x86/kernel/setup.c > > =================================================================== > > --- linux-pm.orig/arch/x86/kernel/setup.c > > +++ linux-pm/arch/x86/kernel/setup.c > > @@ -1045,9 +1045,6 @@ void __init setup_arch(char **cmdline_p) > > > > cleanup_highmap(); > > > > - /* Look for ACPI tables and reserve memory occupied by them. */ > > - acpi_boot_table_init(); > > - > > memblock_set_current_limit(ISA_END_ADDRESS); > > e820__memblock_setup(); > > > > @@ -1132,6 +1129,8 @@ void __init setup_arch(char **cmdline_p) > > reserve_initrd(); > > > > acpi_table_upgrade(); > > + /* Look for ACPI tables and reserve memory occupied by them. */ > > + acpi_boot_table_init(); > > > > vsmp_init(); > > This is fairly late; especially, it's after actual allocations -- see > e820__memblock_alloc_reserved_mpc_new(). > > Can't the table upgrade mechanism fix up when adjusting something? Not at this point of the cycle I'm afraid. > Some details on what actually breaks would be helpful. Generally speaking, the table overrides that come from the initrd are not taken into account if acpi_boot_table_init() runs before acpi_table_upgrade() and the latter cannot run before reserve_initrd(). Honestly, I'm not sure how much effort it would take to untangle this ATM.
On 13.04.21 19:53, Rafael J. Wysocki wrote: > On Tue, Apr 13, 2021 at 7:43 PM David Hildenbrand <david@redhat.com> wrote: >> >> On 13.04.21 16:01, Rafael J. Wysocki wrote: >>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >>> >>> Commit 1a1c130ab757 ("ACPI: tables: x86: Reserve memory occupied by >>> ACPI tables") attempted to address an issue with reserving the memory >>> occupied by ACPI tables, but it broke the initrd-based table override >>> mechanism relied on by multiple users. >>> >>> To restore the initrd-based ACPI table override functionality, move >>> the acpi_boot_table_init() invocation in setup_arch() on x86 after >>> the acpi_table_upgrade() one. >>> >>> Fixes: 1a1c130ab757 ("ACPI: tables: x86: Reserve memory occupied by ACPI tables") >>> Reported-by: Hans de Goede <hdegoede@redhat.com> >>> Tested-by: Hans de Goede <hdegoede@redhat.com> >>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >>> --- >>> >>> George, can you please check if this reintroduces the issue addressed by >>> the above commit for you? >>> >>> --- >>> arch/x86/kernel/setup.c | 5 ++--- >>> 1 file changed, 2 insertions(+), 3 deletions(-) >>> >>> Index: linux-pm/arch/x86/kernel/setup.c >>> =================================================================== >>> --- linux-pm.orig/arch/x86/kernel/setup.c >>> +++ linux-pm/arch/x86/kernel/setup.c >>> @@ -1045,9 +1045,6 @@ void __init setup_arch(char **cmdline_p) >>> >>> cleanup_highmap(); >>> >>> - /* Look for ACPI tables and reserve memory occupied by them. */ >>> - acpi_boot_table_init(); >>> - >>> memblock_set_current_limit(ISA_END_ADDRESS); >>> e820__memblock_setup(); >>> >>> @@ -1132,6 +1129,8 @@ void __init setup_arch(char **cmdline_p) >>> reserve_initrd(); >>> >>> acpi_table_upgrade(); >>> + /* Look for ACPI tables and reserve memory occupied by them. */ >>> + acpi_boot_table_init(); >>> >>> vsmp_init(); >> >> This is fairly late; especially, it's after actual allocations -- see >> e820__memblock_alloc_reserved_mpc_new(). >> >> Can't the table upgrade mechanism fix up when adjusting something? > > Not at this point of the cycle I'm afraid. > >> Some details on what actually breaks would be helpful. > > Generally speaking, the table overrides that come from the initrd are > not taken into account if acpi_boot_table_init() runs before > acpi_table_upgrade() and the latter cannot run before > reserve_initrd(). I see. (looking at Documentation/acpi/initrd_table_override.txt I understand what acpi table overrides are for :) ) > > Honestly, I'm not sure how much effort it would take to untangle this ATM. Also true; ideally, we wouldn't have any allocations (find+reserve) before ordinary reservations are done. However, I have no idea if we can move e820__memblock_alloc_reserved_mpc_new() and reserve_real_mode() around easily. Also, reserve_initrd()->relocate_initrd() does allocations. This is a mess. -- Thanks, David / dhildenb
On Wed, Apr 14, 2021 at 09:42:01AM +0200, David Hildenbrand wrote: > On 13.04.21 19:53, Rafael J. Wysocki wrote: > > On Tue, Apr 13, 2021 at 7:43 PM David Hildenbrand <david@redhat.com> wrote: > > > > > > On 13.04.21 16:01, Rafael J. Wysocki wrote: > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > > > Commit 1a1c130ab757 ("ACPI: tables: x86: Reserve memory occupied by > > > > ACPI tables") attempted to address an issue with reserving the memory > > > > occupied by ACPI tables, but it broke the initrd-based table override > > > > mechanism relied on by multiple users. > > > > > > > > To restore the initrd-based ACPI table override functionality, move > > > > the acpi_boot_table_init() invocation in setup_arch() on x86 after > > > > the acpi_table_upgrade() one. > > > > > > > > Fixes: 1a1c130ab757 ("ACPI: tables: x86: Reserve memory occupied by ACPI tables") > > > > Reported-by: Hans de Goede <hdegoede@redhat.com> > > > > Tested-by: Hans de Goede <hdegoede@redhat.com> > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > --- > > > > > > > > George, can you please check if this reintroduces the issue addressed by > > > > the above commit for you? > > > > > > > > --- > > > > arch/x86/kernel/setup.c | 5 ++--- > > > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > > > > > Index: linux-pm/arch/x86/kernel/setup.c > > > > =================================================================== > > > > --- linux-pm.orig/arch/x86/kernel/setup.c > > > > +++ linux-pm/arch/x86/kernel/setup.c > > > > @@ -1045,9 +1045,6 @@ void __init setup_arch(char **cmdline_p) > > > > > > > > cleanup_highmap(); > > > > > > > > - /* Look for ACPI tables and reserve memory occupied by them. */ > > > > - acpi_boot_table_init(); > > > > - > > > > memblock_set_current_limit(ISA_END_ADDRESS); > > > > e820__memblock_setup(); > > > > > > > > @@ -1132,6 +1129,8 @@ void __init setup_arch(char **cmdline_p) > > > > reserve_initrd(); > > > > > > > > acpi_table_upgrade(); > > > > + /* Look for ACPI tables and reserve memory occupied by them. */ > > > > + acpi_boot_table_init(); > > > > > > > > vsmp_init(); > > > > > > This is fairly late; especially, it's after actual allocations -- see > > > e820__memblock_alloc_reserved_mpc_new(). > > > > > > Can't the table upgrade mechanism fix up when adjusting something? > > > > Not at this point of the cycle I'm afraid. > > > > > Some details on what actually breaks would be helpful. > > > > Generally speaking, the table overrides that come from the initrd are > > not taken into account if acpi_boot_table_init() runs before > > acpi_table_upgrade() and the latter cannot run before > > reserve_initrd(). > > I see. (looking at Documentation/acpi/initrd_table_override.txt I understand > what acpi table overrides are for :) ) > > > > > Honestly, I'm not sure how much effort it would take to untangle this ATM. > > Also true; ideally, we wouldn't have any allocations (find+reserve) before > ordinary reservations are done. > > However, I have no idea if we can move > e820__memblock_alloc_reserved_mpc_new() and reserve_real_mode() around > easily. Also, reserve_initrd()->relocate_initrd() does allocations. Even if we can move e820__memblock_alloc_reserved_mpc_new() and reserve_real_mode(), the allocation in reserve_initrd() has to be before the tables override, we would only reduce the probability of allocating an ACPI page. I think what we can do is to override the ACPI tables separately from their initial parsing. Rafael, what do you say? > This is a mess. True :( -- Sincerely yours, Mike.
On Wed, Apr 14, 2021 at 10:13 AM Mike Rapoport <rppt@linux.ibm.com> wrote: > > On Wed, Apr 14, 2021 at 09:42:01AM +0200, David Hildenbrand wrote: > > On 13.04.21 19:53, Rafael J. Wysocki wrote: > > > On Tue, Apr 13, 2021 at 7:43 PM David Hildenbrand <david@redhat.com> wrote: > > > > > > > > On 13.04.21 16:01, Rafael J. Wysocki wrote: > > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > > > > > Commit 1a1c130ab757 ("ACPI: tables: x86: Reserve memory occupied by > > > > > ACPI tables") attempted to address an issue with reserving the memory > > > > > occupied by ACPI tables, but it broke the initrd-based table override > > > > > mechanism relied on by multiple users. > > > > > > > > > > To restore the initrd-based ACPI table override functionality, move > > > > > the acpi_boot_table_init() invocation in setup_arch() on x86 after > > > > > the acpi_table_upgrade() one. > > > > > > > > > > Fixes: 1a1c130ab757 ("ACPI: tables: x86: Reserve memory occupied by ACPI tables") > > > > > Reported-by: Hans de Goede <hdegoede@redhat.com> > > > > > Tested-by: Hans de Goede <hdegoede@redhat.com> > > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > --- > > > > > > > > > > George, can you please check if this reintroduces the issue addressed by > > > > > the above commit for you? > > > > > > > > > > --- > > > > > arch/x86/kernel/setup.c | 5 ++--- > > > > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > > > > > > > Index: linux-pm/arch/x86/kernel/setup.c > > > > > =================================================================== > > > > > --- linux-pm.orig/arch/x86/kernel/setup.c > > > > > +++ linux-pm/arch/x86/kernel/setup.c > > > > > @@ -1045,9 +1045,6 @@ void __init setup_arch(char **cmdline_p) > > > > > > > > > > cleanup_highmap(); > > > > > > > > > > - /* Look for ACPI tables and reserve memory occupied by them. */ > > > > > - acpi_boot_table_init(); > > > > > - > > > > > memblock_set_current_limit(ISA_END_ADDRESS); > > > > > e820__memblock_setup(); > > > > > > > > > > @@ -1132,6 +1129,8 @@ void __init setup_arch(char **cmdline_p) > > > > > reserve_initrd(); > > > > > > > > > > acpi_table_upgrade(); > > > > > + /* Look for ACPI tables and reserve memory occupied by them. */ > > > > > + acpi_boot_table_init(); > > > > > > > > > > vsmp_init(); > > > > > > > > This is fairly late; especially, it's after actual allocations -- see > > > > e820__memblock_alloc_reserved_mpc_new(). > > > > > > > > Can't the table upgrade mechanism fix up when adjusting something? > > > > > > Not at this point of the cycle I'm afraid. > > > > > > > Some details on what actually breaks would be helpful. > > > > > > Generally speaking, the table overrides that come from the initrd are > > > not taken into account if acpi_boot_table_init() runs before > > > acpi_table_upgrade() and the latter cannot run before > > > reserve_initrd(). > > > > I see. (looking at Documentation/acpi/initrd_table_override.txt I understand > > what acpi table overrides are for :) ) > > > > > > > > Honestly, I'm not sure how much effort it would take to untangle this ATM. > > > > Also true; ideally, we wouldn't have any allocations (find+reserve) before > > ordinary reservations are done. > > > > However, I have no idea if we can move > > e820__memblock_alloc_reserved_mpc_new() and reserve_real_mode() around > > easily. Also, reserve_initrd()->relocate_initrd() does allocations. > > Even if we can move e820__memblock_alloc_reserved_mpc_new() and > reserve_real_mode(), the allocation in reserve_initrd() has to be before > the tables override, we would only reduce the probability of allocating an > ACPI page. > > I think what we can do is to override the ACPI tables separately from their > initial parsing. Rafael, what do you say? Well, possibly. With one caveat that parsing a table that's going to be overridden subsequently may not be a good idea. Anyway, things like that can only be done in the next cycle or later.
Rafael & Mike, Do you still need this patch tested in the failing scenario? Have not seen any recent activity on this. Thank you, George On 4/13/2021 10:01 AM, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Commit 1a1c130ab757 ("ACPI: tables: x86: Reserve memory occupied by > ACPI tables") attempted to address an issue with reserving the memory > occupied by ACPI tables, but it broke the initrd-based table override > mechanism relied on by multiple users. > > To restore the initrd-based ACPI table override functionality, move > the acpi_boot_table_init() invocation in setup_arch() on x86 after > the acpi_table_upgrade() one. > > Fixes: 1a1c130ab757 ("ACPI: tables: x86: Reserve memory occupied by ACPI tables") > Reported-by: Hans de Goede <hdegoede@redhat.com> > Tested-by: Hans de Goede <hdegoede@redhat.com> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > > George, can you please check if this reintroduces the issue addressed by > the above commit for you? > > --- > arch/x86/kernel/setup.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > Index: linux-pm/arch/x86/kernel/setup.c > =================================================================== > --- linux-pm.orig/arch/x86/kernel/setup.c > +++ linux-pm/arch/x86/kernel/setup.c > @@ -1045,9 +1045,6 @@ void __init setup_arch(char **cmdline_p) > > cleanup_highmap(); > > - /* Look for ACPI tables and reserve memory occupied by them. */ > - acpi_boot_table_init(); > - > memblock_set_current_limit(ISA_END_ADDRESS); > e820__memblock_setup(); > > @@ -1132,6 +1129,8 @@ void __init setup_arch(char **cmdline_p) > reserve_initrd(); > > acpi_table_upgrade(); > + /* Look for ACPI tables and reserve memory occupied by them. */ > + acpi_boot_table_init(); > > vsmp_init(); > > > >
On Mon, Apr 26, 2021 at 5:51 PM George Kennedy <george.kennedy@oracle.com> wrote: > > Rafael & Mike, > > Do you still need this patch tested in the failing scenario? Well, it's been merged into the mainline, so please test 5.12. Thanks!
Index: linux-pm/arch/x86/kernel/setup.c =================================================================== --- linux-pm.orig/arch/x86/kernel/setup.c +++ linux-pm/arch/x86/kernel/setup.c @@ -1045,9 +1045,6 @@ void __init setup_arch(char **cmdline_p) cleanup_highmap(); - /* Look for ACPI tables and reserve memory occupied by them. */ - acpi_boot_table_init(); - memblock_set_current_limit(ISA_END_ADDRESS); e820__memblock_setup(); @@ -1132,6 +1129,8 @@ void __init setup_arch(char **cmdline_p) reserve_initrd(); acpi_table_upgrade(); + /* Look for ACPI tables and reserve memory occupied by them. */ + acpi_boot_table_init(); vsmp_init();