| Message ID | 1463486765-31827-4-git-send-email-aleksey.makarov@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, May 17, 2016 at 03:06:03PM +0300, Aleksey Makarov wrote: > From: Jon Masters <jcm@redhat.com> > > This patch adds support for ACPI_TABLE_UPGRADE for ARM64 This feels like a tool to paper over problems rather than solve them. If vendors are shipping horrendously broken tables today, clearly no software has ever really supported them. So why add workarounds? Why is this necessary? Is there a particular case for this, or is this just for parity with x86? IMO it would be better to put pressure on vendors to fix their FW and provide sensible OS-agnostic update mechanisms. > To access initrd image we need to move initialization > of linear mapping a bit earlier. > > Signed-off-by: Jon Masters <jcm@redhat.com> > Signed-off-by: Aleksey Makarov <aleksey.makarov@linaro.org> > --- > arch/arm64/kernel/setup.c | 6 ++++-- > drivers/acpi/Kconfig | 2 +- > drivers/acpi/tables.c | 10 +++++++++- > 3 files changed, 14 insertions(+), 4 deletions(-) > > diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c > index feab2ee..1d5e24f 100644 > --- a/arch/arm64/kernel/setup.c > +++ b/arch/arm64/kernel/setup.c > @@ -261,11 +261,13 @@ void __init setup_arch(char **cmdline_p) > efi_init(); > arm64_memblock_init(); > > + paging_init(); > + > + early_initrd_acpi_init(); Do we actually need full paging up here? Why can we not use fixmap based copying as we do for the other cases prior to paging_init? > + > /* Parse the ACPI tables for possible boot-time configuration */ > acpi_boot_table_init(); > > - paging_init(); > - > if (acpi_disabled) > unflatten_device_tree(); > > diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig > index c204344..ec694c0 100644 > --- a/drivers/acpi/Kconfig > +++ b/drivers/acpi/Kconfig > @@ -313,7 +313,7 @@ config ACPI_CUSTOM_DSDT > > config ACPI_TABLE_UPGRADE > bool "Allow upgrading ACPI tables via initrd" > - depends on BLK_DEV_INITRD && X86 > + depends on BLK_DEV_INITRD && (X86 || ARM64) > default y > help Perhaps ARCH_HAS_ACPI_TABLE_UPGRADE? > This option provides functionality to upgrade arbitrary ACPI tables > diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c > index 82be84a..c35034d8 100644 > --- a/drivers/acpi/tables.c > +++ b/drivers/acpi/tables.c > @@ -482,6 +482,14 @@ static DECLARE_BITMAP(acpi_initrd_installed, NR_ACPI_INITRD_TABLES); > > #define MAP_CHUNK_SIZE (NR_FIX_BTMAPS << PAGE_SHIFT) > > +#if defined(CONFIG_X86) > +#define MAX_PHYS_ACPI_TABLES (max_low_pfn_mapped << PAGE_SHIFT) > +#elif defined(CONFIG_ARM64) > +#define MAX_PHYS_ACPI_TABLES PFN_PHYS(max_pfn) > +#else > +#error "MAX_PHYS_ACPI_TABLES is not defiend for this architecture" > +#endif s/defiend/defined/ This should be defined in an arch-specific header if it is actually arch-specific. Thanks, Mark. > + > static void __init acpi_table_initrd_init(void *data, size_t size) > { > int sig, no, table_nr = 0, total_offset = 0; > @@ -541,7 +549,7 @@ static void __init acpi_table_initrd_init(void *data, size_t size) > return; > > acpi_tables_addr = > - memblock_find_in_range(0, max_low_pfn_mapped << PAGE_SHIFT, > + memblock_find_in_range(0, MAX_PHYS_ACPI_TABLES, > all_tables_size, PAGE_SIZE); > if (!acpi_tables_addr) { > WARN_ON(1); > -- > 2.8.2 >
Hi Mark, On 05/17/2016 08:46 AM, Mark Rutland wrote: > On Tue, May 17, 2016 at 03:06:03PM +0300, Aleksey Makarov wrote: >> From: Jon Masters <jcm@redhat.com> >> >> This patch adds support for ACPI_TABLE_UPGRADE for ARM64 > > This feels like a tool to paper over problems rather than solve them. Generally, it's an arrow in the quiver used to triage problems, and then solve them by getting firmware updates made. > If vendors are shipping horrendously broken tables today, clearly no > software has ever really supported them. So why add workarounds? That's not (always) the case. These patches help with: 1). During development of a platform, it is much easier to debug problems with tables if you can test replacement ones without having to respin the firmware. In the server world, you usually don't have the firmware source code, so to get it respun could be days-weeks even if you are working with the authors closely. We have practically used this feature on a number of platforms already and it will continue. 2). They empower (advanced) users and developers to work around problems that they find on platforms. Sure, we want firmware to always be fixed and working well, but it is better if folks have the tools. > Why is this necessary? Is there a particular case for this, or is this > just for parity with x86? The use cases are as above. It's also required for parity with x86 functionality on servers. > IMO it would be better to put pressure on vendors to fix their FW and > provide sensible OS-agnostic update mechanisms. It's easier to put pressure on them after we know what the problems are. I agree that alternative update mechanisms are also good. We also have the ability (but it is not implemented on ARM) in GRUB2 to override ACPI tables, BUT it's kinda atrophied on all arches and requires that you rebuild GRUB with an additional module. The reality is that by incorporating this feature we are able to provide the same capability that already exists on x86 systems for ACPI table triage. Jon.
On 05/17/2016 12:44 PM, Jon Masters wrote: > 1). During development of a platform, it is much easier to debug > problems with tables if you can test replacement ones without having to > respin the firmware. In the server world, you usually don't have the > firmware source code, so to get it respun could be days-weeks even if > you are working with the authors closely. We have practically used this > feature on a number of platforms already and it will continue. For example, on one platform we were unable to fully boot RHEL(SA) due to a bug in one of the ACPI tables. But I was able to boot the system to a ramdisk containing a uuencode library and then write out the content of the tables over the serial port, then decompile/patch/recompile, and override replacement tables on the system. Then we beat the vendor up with the fixes and the official firmware was corrected.
On 05/17/2016 03:46 PM, Mark Rutland wrote: > On Tue, May 17, 2016 at 03:06:03PM +0300, Aleksey Makarov wrote: [...] >> diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c >> index feab2ee..1d5e24f 100644 >> --- a/arch/arm64/kernel/setup.c >> +++ b/arch/arm64/kernel/setup.c >> @@ -261,11 +261,13 @@ void __init setup_arch(char **cmdline_p) >> efi_init(); >> arm64_memblock_init(); >> >> + paging_init(); >> + >> + early_initrd_acpi_init(); > > Do we actually need full paging up here? > > Why can we not use fixmap based copying as we do for the other cases > prior to paging_init? The implementation of the feature acpi_table_initrd_init() (drivers/acpi/tables.c) works with initrd data represented as an array in virtual memory. It uses some library utility to find the redefined tables in that array and iterates over it to copy the data to new allocated memory. So we need to rewrite it considerably to access the initrd data via fixmap. In x86 arch, linear kernel memory is already mapped by the time when early_initrd_acpi_init() and acpi_boot_table_init() are called so I think that we can just move this mapping one function earlier too. Is it ok? I will address your other comments in the next version of the patchset. Thank you Aleksey Makarov >> + >> /* Parse the ACPI tables for possible boot-time configuration */ >> acpi_boot_table_init(); >> >> - paging_init(); >> - >> if (acpi_disabled) >> unflatten_device_tree(); >> >> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig >> index c204344..ec694c0 100644 >> --- a/drivers/acpi/Kconfig >> +++ b/drivers/acpi/Kconfig >> @@ -313,7 +313,7 @@ config ACPI_CUSTOM_DSDT >> >> config ACPI_TABLE_UPGRADE >> bool "Allow upgrading ACPI tables via initrd" >> - depends on BLK_DEV_INITRD && X86 >> + depends on BLK_DEV_INITRD && (X86 || ARM64) >> default y >> help > > Perhaps ARCH_HAS_ACPI_TABLE_UPGRADE? > >> This option provides functionality to upgrade arbitrary ACPI tables >> diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c >> index 82be84a..c35034d8 100644 >> --- a/drivers/acpi/tables.c >> +++ b/drivers/acpi/tables.c >> @@ -482,6 +482,14 @@ static DECLARE_BITMAP(acpi_initrd_installed, NR_ACPI_INITRD_TABLES); >> >> #define MAP_CHUNK_SIZE (NR_FIX_BTMAPS << PAGE_SHIFT) >> >> +#if defined(CONFIG_X86) >> +#define MAX_PHYS_ACPI_TABLES (max_low_pfn_mapped << PAGE_SHIFT) >> +#elif defined(CONFIG_ARM64) >> +#define MAX_PHYS_ACPI_TABLES PFN_PHYS(max_pfn) >> +#else >> +#error "MAX_PHYS_ACPI_TABLES is not defiend for this architecture" >> +#endif > > s/defiend/defined/ > > This should be defined in an arch-specific header if it is actually > arch-specific. > > Thanks, > Mark. > >> + >> static void __init acpi_table_initrd_init(void *data, size_t size) >> { >> int sig, no, table_nr = 0, total_offset = 0; >> @@ -541,7 +549,7 @@ static void __init acpi_table_initrd_init(void *data, size_t size) >> return; >> >> acpi_tables_addr = >> - memblock_find_in_range(0, max_low_pfn_mapped << PAGE_SHIFT, >> + memblock_find_in_range(0, MAX_PHYS_ACPI_TABLES, >> all_tables_size, PAGE_SIZE); >> if (!acpi_tables_addr) { >> WARN_ON(1); >> -- >> 2.8.2 >>
On Tue, May 17, 2016 at 12:44:02PM -0400, Jon Masters wrote: > Hi Mark, > > On 05/17/2016 08:46 AM, Mark Rutland wrote: > > On Tue, May 17, 2016 at 03:06:03PM +0300, Aleksey Makarov wrote: > >> From: Jon Masters <jcm@redhat.com> > >> > >> This patch adds support for ACPI_TABLE_UPGRADE for ARM64 > > > > This feels like a tool to paper over problems rather than solve them. > > Generally, it's an arrow in the quiver used to triage problems, and then > solve them by getting firmware updates made. Ok. The feature is _hideously_ misnamed for that purpose, however. > > If vendors are shipping horrendously broken tables today, clearly no > > software has ever really supported them. So why add workarounds? > > That's not (always) the case. These patches help with: > > 1). During development of a platform, it is much easier to debug > problems with tables if you can test replacement ones without having to > respin the firmware. In the server world, you usually don't have the > firmware source code, so to get it respun could be days-weeks even if > you are working with the authors closely. We have practically used this > feature on a number of platforms already and it will continue. I was under the impression that that was already possible with GRUB today (though I see below this may not be the case). > 2). They empower (advanced) users and developers to work around problems > that they find on platforms. Sure, we want firmware to always be fixed > and working well, but it is better if folks have the tools. > > > Why is this necessary? Is there a particular case for this, or is this > > just for parity with x86? > > The use cases are as above. It's also required for parity with x86 > functionality on servers. Parity for case 1 above, or is this used in other scenarios on x86 today? > > IMO it would be better to put pressure on vendors to fix their FW and > > provide sensible OS-agnostic update mechanisms. > > It's easier to put pressure on them after we know what the problems are. > I agree that alternative update mechanisms are also good. We also have > the ability (but it is not implemented on ARM) in GRUB2 to override ACPI > tables, BUT it's kinda atrophied on all arches and requires that you > rebuild GRUB with an additional module. This feels like something that could/should be rectified. > The reality is that by incorporating this feature we are able to > provide the same capability that already exists on x86 systems for > ACPI table triage. To be clear, I do not disagree that this feature can be useful for that case. I am just concerned that this is easily abused, and the description implies a more general set of use cases than we would like. Thanks, Mark.
On Tue, May 17, 2016 at 12:48:53PM -0400, Jon Masters wrote: > On 05/17/2016 12:44 PM, Jon Masters wrote: > > > 1). During development of a platform, it is much easier to debug > > problems with tables if you can test replacement ones without having to > > respin the firmware. In the server world, you usually don't have the > > firmware source code, so to get it respun could be days-weeks even if > > you are working with the authors closely. We have practically used this > > feature on a number of platforms already and it will continue. > > For example, on one platform we were unable to fully boot RHEL(SA) due > to a bug in one of the ACPI tables. But I was able to boot the system to > a ramdisk containing a uuencode library and then write out the content > of the tables over the serial port, then decompile/patch/recompile, and > override replacement tables on the system. Then we beat the vendor up > with the fixes and the official firmware was corrected. Can you explain to me please why you can't do it with GRUB ? I am using mainline GRUB and its acpi command all the time to update static ACPI tables for testing new features (ie IORT) and it works just fine for me (and you can still override the DSDT, which is likely to be the main source of bugs, through the CONFIG_ACPI_CUSTOM_DSDT config option, that works on ARM in mainline with no changes required). I just want to understand if there is really a compelling reason for adding this stuff when we can easily implement its features through something that is usable today without any kernel changes. Thanks, Lorenzo
On 05/23/2016 01:56 PM, Lorenzo Pieralisi wrote: > On Tue, May 17, 2016 at 12:48:53PM -0400, Jon Masters wrote: >> On 05/17/2016 12:44 PM, Jon Masters wrote: >> >>> 1). During development of a platform, it is much easier to debug >>> problems with tables if you can test replacement ones without having to >>> respin the firmware. In the server world, you usually don't have the >>> firmware source code, so to get it respun could be days-weeks even if >>> you are working with the authors closely. We have practically used this >>> feature on a number of platforms already and it will continue. >> >> For example, on one platform we were unable to fully boot RHEL(SA) due >> to a bug in one of the ACPI tables. But I was able to boot the system to >> a ramdisk containing a uuencode library and then write out the content >> of the tables over the serial port, then decompile/patch/recompile, and >> override replacement tables on the system. Then we beat the vendor up >> with the fixes and the official firmware was corrected. > > Can you explain to me please why you can't do it with GRUB ? It's doable with GRUB (if you rebuild GRUB modules to include it) > I am using mainline GRUB and its acpi command all the time to update > static ACPI tables for testing new features (ie IORT) and it works > just fine for me (and you can still override the DSDT, which is > likely to be the main source of bugs, through the CONFIG_ACPI_CUSTOM_DSDT > config option, that works on ARM in mainline with no changes required). > > I just want to understand if there is really a compelling reason > for adding this stuff when we can easily implement its features > through something that is usable today without any kernel changes. We're looking for x86 feature parity in the base platform. Every time there's a gratuitous differentiation it's a waste of time for folks trying to figure out what is different on an ARM system. So if we completely remove the ACPI override feature from the kernel and make everyone use GRUB instead, then fine, but ARM shouldn't be special. Jon.
diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c index feab2ee..1d5e24f 100644 --- a/arch/arm64/kernel/setup.c +++ b/arch/arm64/kernel/setup.c @@ -261,11 +261,13 @@ void __init setup_arch(char **cmdline_p) efi_init(); arm64_memblock_init(); + paging_init(); + + early_initrd_acpi_init(); + /* Parse the ACPI tables for possible boot-time configuration */ acpi_boot_table_init(); - paging_init(); - if (acpi_disabled) unflatten_device_tree(); diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig index c204344..ec694c0 100644 --- a/drivers/acpi/Kconfig +++ b/drivers/acpi/Kconfig @@ -313,7 +313,7 @@ config ACPI_CUSTOM_DSDT config ACPI_TABLE_UPGRADE bool "Allow upgrading ACPI tables via initrd" - depends on BLK_DEV_INITRD && X86 + depends on BLK_DEV_INITRD && (X86 || ARM64) default y help This option provides functionality to upgrade arbitrary ACPI tables diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c index 82be84a..c35034d8 100644 --- a/drivers/acpi/tables.c +++ b/drivers/acpi/tables.c @@ -482,6 +482,14 @@ static DECLARE_BITMAP(acpi_initrd_installed, NR_ACPI_INITRD_TABLES); #define MAP_CHUNK_SIZE (NR_FIX_BTMAPS << PAGE_SHIFT) +#if defined(CONFIG_X86) +#define MAX_PHYS_ACPI_TABLES (max_low_pfn_mapped << PAGE_SHIFT) +#elif defined(CONFIG_ARM64) +#define MAX_PHYS_ACPI_TABLES PFN_PHYS(max_pfn) +#else +#error "MAX_PHYS_ACPI_TABLES is not defiend for this architecture" +#endif + static void __init acpi_table_initrd_init(void *data, size_t size) { int sig, no, table_nr = 0, total_offset = 0; @@ -541,7 +549,7 @@ static void __init acpi_table_initrd_init(void *data, size_t size) return; acpi_tables_addr = - memblock_find_in_range(0, max_low_pfn_mapped << PAGE_SHIFT, + memblock_find_in_range(0, MAX_PHYS_ACPI_TABLES, all_tables_size, PAGE_SIZE); if (!acpi_tables_addr) { WARN_ON(1);