[PATCH 0/3] ACPI: ARM64: support for ACPI_TABLE_UPGRADE

[PATCH 0/3] ACPI: ARM64: support for ACPI_TABLE_UPGRADE

[Aleksey Makarov]

This patchset adds support for ACPI_TABLE_UPGRADE for ARM64

It refactors the code introduced by the patches by Lv Zheng [1], fixes access to
the destination of new ACPI tables as suggested by Mark Rutland [2] and enables
the feature for ARM64

It was first sent by Jon Masters [3] to linaro-acpi in December 2015

Tested on QEMU (arm64 and x86) and ThunderX
Should be applied to next-20160517

[1] https://lkml.kernel.org/g/cover.1460340514.git.lv.zheng@intel.com
[2] https://lists.linaro.org/pipermail/linaro-acpi/2015-December/006101.html
[3] https://lists.linaro.org/pipermail/linaro-acpi/2015-December/006099.html

Aleksey Makarov (2):
  ACPI: table upgrade: use cacheable map for tables
  ACPI: table upgrade: move early_initrd_acpi_init() to header file

Jon Masters (1):
  ACPI: ARM64: support for ACPI_TABLE_UPGRADE

 arch/arm64/kernel/setup.c |  6 ++++--
 arch/x86/kernel/setup.c   |  7 -------
 drivers/acpi/Kconfig      |  2 +-
 drivers/acpi/tables.c     | 21 ++++++++++++++++-----
 include/linux/acpi.h      |  4 ++--
 5 files changed, 23 insertions(+), 17 deletions(-)

-- 
2.8.2

[PATCH 1/3] ACPI: table upgrade: use cacheable map for tables

[Aleksey Makarov]

The new memory allocated in acpi_table_initrd_init() is used to
copy the upgraded tables to it.  So it should be mapped with
early_memunmap() instead of early_ioremap().

This is critical for ARM.

Signed-off-by: Aleksey Makarov <aleksey.makarov@linaro.org>
---
 drivers/acpi/tables.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
index a372f9e..449a649 100644
--- a/drivers/acpi/tables.c
+++ b/drivers/acpi/tables.c
@@ -578,10 +578,10 @@ static void __init acpi_table_initrd_init(void *data, size_t size)
 			clen = size;
 			if (clen > MAP_CHUNK_SIZE - slop)
 				clen = MAP_CHUNK_SIZE - slop;
-			dest_p = early_ioremap(dest_addr & PAGE_MASK,
+			dest_p = early_memremap(dest_addr & PAGE_MASK,
 						 clen + slop);
 			memcpy(dest_p + slop, src_p, clen);
-			early_iounmap(dest_p, clen + slop);
+			early_memunmap(dest_p, clen + slop);
 			src_p += clen;
 			dest_addr += clen;
 			size -= clen;
-- 
2.8.2

RE: [PATCH 1/3] ACPI: table upgrade: use cacheable map for tables

[Zheng, Lv]

Hi,

> From: Aleksey Makarov [mailto:aleksey.makarov@linaro.org]
> Subject: [PATCH 1/3] ACPI: table upgrade: use cacheable map for tables
> 
> The new memory allocated in acpi_table_initrd_init() is used to
> copy the upgraded tables to it.  So it should be mapped with
> early_memunmap() instead of early_ioremap().
> 
> This is critical for ARM.
> 
> Signed-off-by: Aleksey Makarov <aleksey.makarov@linaro.org>
[Lv Zheng] 
Acked-by: Lv Zheng <lv.zheng@intel.com>

Thanks
-Lv

> ---
>  drivers/acpi/tables.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
> index a372f9e..449a649 100644
> --- a/drivers/acpi/tables.c
> +++ b/drivers/acpi/tables.c
> @@ -578,10 +578,10 @@ static void __init acpi_table_initrd_init(void *data,
> size_t size)
>  			clen = size;
>  			if (clen > MAP_CHUNK_SIZE - slop)
>  				clen = MAP_CHUNK_SIZE - slop;
> -			dest_p = early_ioremap(dest_addr & PAGE_MASK,
> +			dest_p = early_memremap(dest_addr & PAGE_MASK,
>  						 clen + slop);
>  			memcpy(dest_p + slop, src_p, clen);
> -			early_iounmap(dest_p, clen + slop);
> +			early_memunmap(dest_p, clen + slop);
>  			src_p += clen;
>  			dest_addr += clen;
>  			size -= clen;
> --
> 2.8.2

[PATCH 2/3] ACPI: table upgrade: move early_initrd_acpi_init() to header file

[Aleksey Makarov]

Move early_initrd_acpi_init() to header file so that
it can be used with architectures other than X86

Signed-off-by: Aleksey Makarov <aleksey.makarov@linaro.org>
---
 arch/x86/kernel/setup.c | 7 -------
 drivers/acpi/tables.c   | 7 +++++--
 include/linux/acpi.h    | 4 ++--
 3 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index c4e7b39..5ddbbfd 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -399,10 +399,6 @@ static void __init reserve_initrd(void)
 	memblock_free(ramdisk_image, ramdisk_end - ramdisk_image);
 }
 
-static void __init early_initrd_acpi_init(void)
-{
-	early_acpi_table_init((void *)initrd_start, initrd_end - initrd_start);
-}
 #else
 static void __init early_reserve_initrd(void)
 {
@@ -410,9 +406,6 @@ static void __init early_reserve_initrd(void)
 static void __init reserve_initrd(void)
 {
 }
-static void __init early_initrd_acpi_init(void)
-{
-}
 #endif /* CONFIG_BLK_DEV_INITRD */
 
 static void __init parse_setup_data(void)
diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
index 449a649..82be84a 100644
--- a/drivers/acpi/tables.c
+++ b/drivers/acpi/tables.c
@@ -34,6 +34,7 @@
 #include <linux/bootmem.h>
 #include <linux/earlycpio.h>
 #include <linux/memblock.h>
+#include <linux/initrd.h>
 #include "internal.h"
 
 #ifdef CONFIG_ACPI_CUSTOM_DSDT
@@ -742,9 +743,11 @@ acpi_os_table_override(struct acpi_table_header *existing_table,
 	return AE_OK;
 }
 
-void __init early_acpi_table_init(void *data, size_t size)
+void __init early_initrd_acpi_init(void)
 {
-	acpi_table_initrd_init(data, size);
+#ifdef CONFIG_BLK_DEV_INITRD
+	acpi_table_initrd_init((void *)initrd_start, initrd_end - initrd_start);
+#endif
 }
 
 /*
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 288fac5..cb700c1 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -208,7 +208,7 @@ void acpi_boot_table_init (void);
 int acpi_mps_check (void);
 int acpi_numa_init (void);
 
-void early_acpi_table_init(void *data, size_t size);
+void early_initrd_acpi_init(void);
 int acpi_table_init (void);
 int acpi_table_parse(char *id, acpi_tbl_table_handler handler);
 int __init acpi_parse_entries(char *id, unsigned long table_size,
@@ -588,7 +588,7 @@ static inline const char *acpi_dev_name(struct acpi_device *adev)
 	return NULL;
 }
 
-static inline void early_acpi_table_init(void *data, size_t size) { }
+static inline void early_initrd_acpi_init(void) { }
 static inline void acpi_early_init(void) { }
 static inline void acpi_subsystem_init(void) { }
 
-- 
2.8.2

RE: [PATCH 2/3] ACPI: table upgrade: move early_initrd_acpi_init() to header file

[Zheng, Lv]

Hi,

> From: Aleksey Makarov [mailto:aleksey.makarov@linaro.org]
> Subject: [PATCH 2/3] ACPI: table upgrade: move early_initrd_acpi_init() to
> header file
> 
> Move early_initrd_acpi_init() to header file so that
> it can be used with architectures other than X86
[Lv Zheng] 
The patch looks OK to me.
Except 2 possible improvements.
You can feel free to add Acked-by: Lv Zheng <lv.zheng@intel.com>

> 
> Signed-off-by: Aleksey Makarov <aleksey.makarov@linaro.org>
> ---
>  arch/x86/kernel/setup.c | 7 -------
>  drivers/acpi/tables.c   | 7 +++++--
>  include/linux/acpi.h    | 4 ++--
>  3 files changed, 7 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index c4e7b39..5ddbbfd 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -399,10 +399,6 @@ static void __init reserve_initrd(void)
>  	memblock_free(ramdisk_image, ramdisk_end - ramdisk_image);
>  }
> 
> -static void __init early_initrd_acpi_init(void)
> -{
> -	early_acpi_table_init((void *)initrd_start, initrd_end - initrd_start);
> -}
>  #else
>  static void __init early_reserve_initrd(void)
>  {
> @@ -410,9 +406,6 @@ static void __init early_reserve_initrd(void)
>  static void __init reserve_initrd(void)
>  {
>  }
> -static void __init early_initrd_acpi_init(void)
> -{
> -}
>  #endif /* CONFIG_BLK_DEV_INITRD */
> 
>  static void __init parse_setup_data(void)
> diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
> index 449a649..82be84a 100644
> --- a/drivers/acpi/tables.c
> +++ b/drivers/acpi/tables.c
> @@ -34,6 +34,7 @@
>  #include <linux/bootmem.h>
>  #include <linux/earlycpio.h>
>  #include <linux/memblock.h>
> +#include <linux/initrd.h>
>  #include "internal.h"
> 
>  #ifdef CONFIG_ACPI_CUSTOM_DSDT
> @@ -742,9 +743,11 @@ acpi_os_table_override(struct acpi_table_header
> *existing_table,
>  	return AE_OK;
>  }
> 
> -void __init early_acpi_table_init(void *data, size_t size)
> +void __init early_initrd_acpi_init(void)
>  {
> -	acpi_table_initrd_init(data, size);
> +#ifdef CONFIG_BLK_DEV_INITRD
> +	acpi_table_initrd_init((void *)initrd_start, initrd_end - initrd_start);
> +#endif
>  }
[Lv Zheng] 
Please avoid to make #ifdef appearing in a function body.
You may consider a way to maintain #ifdef CONFIG_xxx at function boundary.
The point is:
#ifdef should be maintained at the function boundary, this is an implicit (maybe explicit in Documentation) programming rule in Linux.
This helps to split the "functionality" and the "dependency" to make the code cleaner and easier to follow.

For this case, you may:
#ifdef CONFIG_BLK_DEV_INTRD
#define ACPI_TABLE_INITRD_DATA	(initrd_start)
#define ACPI_TABLE_INITRD_SIZE	(initrd_end - initrd_start)
#else
#define ACPI_TABLE_INITRD_DATA	NULL
#define ACPI_TABLE_INITRD_SIZE	0
#endif

Or you can:
Use initrd_start, inird_end directly in acpi_table_initrd_init, and delete its arguments.
By doing so, you are able to achieve in-function #ifdef elimination by providing a stub for acpi_table_initrd_init().

> 
>  /*
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 288fac5..cb700c1 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -208,7 +208,7 @@ void acpi_boot_table_init (void);
>  int acpi_mps_check (void);
>  int acpi_numa_init (void);
> 
> -void early_acpi_table_init(void *data, size_t size);
> +void early_initrd_acpi_init(void);
>  int acpi_table_init (void);
>  int acpi_table_parse(char *id, acpi_tbl_table_handler handler);
>  int __init acpi_parse_entries(char *id, unsigned long table_size,
> @@ -588,7 +588,7 @@ static inline const char *acpi_dev_name(struct
> acpi_device *adev)
>  	return NULL;
>  }
> 
> -static inline void early_acpi_table_init(void *data, size_t size) { }
> +static inline void early_initrd_acpi_init(void) { }
>  static inline void acpi_early_init(void) { }
>  static inline void acpi_subsystem_init(void) { }
> 
[Lv Zheng] 
You can rename early_initrd_acpi_init to early_acpi_table_init.
We'd prefer not to export "initrd" awareness from acpi.h.

Thanks and best regards
-Lv

> --
> 2.8.2

[PATCH 3/3] ACPI: ARM64: support for ACPI_TABLE_UPGRADE

[Aleksey Makarov]

From: Jon Masters <jcm@redhat.com>

This patch adds support for ACPI_TABLE_UPGRADE for ARM64

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();
+
 	/* 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);
-- 
2.8.2

Re: [PATCH 3/3] ACPI: ARM64: support for ACPI_TABLE_UPGRADE

[Mark Rutland]

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
> 

Re: [PATCH 3/3] ACPI: ARM64: support for ACPI_TABLE_UPGRADE

[Jon Masters]

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.

-- 
Computer Architect | Sent from my Fedora powered laptop

Re: [PATCH 3/3] ACPI: ARM64: support for ACPI_TABLE_UPGRADE

[Jon Masters]

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.


-- 
Computer Architect | Sent from my Fedora powered laptop

Re: [PATCH 3/3] ACPI: ARM64: support for ACPI_TABLE_UPGRADE

[Lorenzo Pieralisi]

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

Re: [PATCH 3/3] ACPI: ARM64: support for ACPI_TABLE_UPGRADE

[Jon Masters]

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.

-- 
Computer Architect | Sent from my Fedora powered laptop

Re: [PATCH 3/3] ACPI: ARM64: support for ACPI_TABLE_UPGRADE

[Mark Rutland]

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.

Re: [PATCH 3/3] ACPI: ARM64: support for ACPI_TABLE_UPGRADE

[Aleksey Makarov]

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
>>