linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] Revert "ACPI: Add memory semantics to acpi_os_map_memory()"
@ 2021-09-10 12:28 Jia He
  2021-09-10 13:54 ` Lorenzo Pieralisi
  2021-09-10 14:32 ` [PATCH v2] " Jia He
  0 siblings, 2 replies; 23+ messages in thread
From: Jia He @ 2021-09-10 12:28 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: Will Deacon, Len Brown, Robert Moore, Erik Kaneda,
	linux-arm-kernel, linux-kernel, linux-acpi, devel, Jia He,
	stable, Ard Biesheuvel, Hanjun Guo, Catalin Marinas,
	Rafael J . Wysocki

This reverts commit 437b38c51162f8b87beb28a833c4d5dc85fa864e.

After this commit, a boot panic is alway hit on an Ampere EMAG server
with call trace as follows:
 Internal error: synchronous external abort: 96000410 [#1] SMP
 Modules linked in:
 CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.14.0+ #462
 Hardware name: MiTAC RAPTOR EV-883832-X3-0001/RAPTOR, BIOS 0.14 02/22/2019
 pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[...snip...]
 Call trace:
  acpi_ex_system_memory_space_handler+0x26c/0x2c8
  acpi_ev_address_space_dispatch+0x228/0x2c4
  acpi_ex_access_region+0x114/0x268
  acpi_ex_field_datum_io+0x128/0x1b8
  acpi_ex_extract_from_field+0x14c/0x2ac
  acpi_ex_read_data_from_field+0x190/0x1b8
  acpi_ex_resolve_node_to_value+0x1ec/0x288
  acpi_ex_resolve_to_value+0x250/0x274
  acpi_ds_evaluate_name_path+0xac/0x124
  acpi_ds_exec_end_op+0x90/0x410
  acpi_ps_parse_loop+0x4ac/0x5d8
  acpi_ps_parse_aml+0xe0/0x2c8
  acpi_ps_execute_method+0x19c/0x1ac
  acpi_ns_evaluate+0x1f8/0x26c
  acpi_ns_init_one_device+0x104/0x140
  acpi_ns_walk_namespace+0x158/0x1d0
  acpi_ns_initialize_devices+0x194/0x218
  acpi_initialize_objects+0x48/0x50
  acpi_init+0xe0/0x498

From the debugging, we're mapping something which is *not* described by
the EFI memory map, but *does* want PROT_NORMAL_NC.

Hence just revert it before everything is clear.

Fixes: 437b38c51162 ("ACPI: Add memory semantics to acpi_os_map_memory()")
Cc: stable@vger.kernel.org
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Ard Biesheuvel <ardb@kernel.org>
Cc: Hanjun Guo <guohanjun@huawei.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Signed-off-by: Jia He <justin.he@arm.com>
---
 arch/arm64/include/asm/acpi.h |  3 ---
 arch/arm64/kernel/acpi.c      | 19 +++----------------
 drivers/acpi/osl.c            | 23 +++++++----------------
 include/acpi/acpi_io.h        |  8 --------
 4 files changed, 10 insertions(+), 43 deletions(-)

diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
index 7535dc7cc5aa..bd68e1b7f29f 100644
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -50,9 +50,6 @@ pgprot_t __acpi_get_mem_attribute(phys_addr_t addr);
 void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size);
 #define acpi_os_ioremap acpi_os_ioremap
 
-void __iomem *acpi_os_memmap(acpi_physical_address phys, acpi_size size);
-#define acpi_os_memmap acpi_os_memmap
-
 typedef u64 phys_cpuid_t;
 #define PHYS_CPUID_INVALID INVALID_HWID
 
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
index 1c9c2f7a1c04..f3851724fe35 100644
--- a/arch/arm64/kernel/acpi.c
+++ b/arch/arm64/kernel/acpi.c
@@ -273,8 +273,7 @@ pgprot_t __acpi_get_mem_attribute(phys_addr_t addr)
 	return __pgprot(PROT_DEVICE_nGnRnE);
 }
 
-static void __iomem *__acpi_os_ioremap(acpi_physical_address phys,
-				       acpi_size size, bool memory)
+void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size)
 {
 	efi_memory_desc_t *md, *region = NULL;
 	pgprot_t prot;
@@ -300,11 +299,9 @@ static void __iomem *__acpi_os_ioremap(acpi_physical_address phys,
 	 * It is fine for AML to remap regions that are not represented in the
 	 * EFI memory map at all, as it only describes normal memory, and MMIO
 	 * regions that require a virtual mapping to make them accessible to
-	 * the EFI runtime services. Determine the region default
-	 * attributes by checking the requested memory semantics.
+	 * the EFI runtime services.
 	 */
-	prot = memory ? __pgprot(PROT_NORMAL_NC) :
-			__pgprot(PROT_DEVICE_nGnRnE);
+	prot = __pgprot(PROT_DEVICE_nGnRnE);
 	if (region) {
 		switch (region->type) {
 		case EFI_LOADER_CODE:
@@ -364,16 +361,6 @@ static void __iomem *__acpi_os_ioremap(acpi_physical_address phys,
 	return __ioremap(phys, size, prot);
 }
 
-void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size)
-{
-	return __acpi_os_ioremap(phys, size, false);
-}
-
-void __iomem *acpi_os_memmap(acpi_physical_address phys, acpi_size size)
-{
-	return __acpi_os_ioremap(phys, size, true);
-}
-
 /*
  * Claim Synchronous External Aborts as a firmware first notification.
  *
diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index a43f1521efe6..45c5c0e45e33 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -284,8 +284,7 @@ acpi_map_lookup_virt(void __iomem *virt, acpi_size size)
 #define should_use_kmap(pfn)   page_is_ram(pfn)
 #endif
 
-static void __iomem *acpi_map(acpi_physical_address pg_off, unsigned long pg_sz,
-			      bool memory)
+static void __iomem *acpi_map(acpi_physical_address pg_off, unsigned long pg_sz)
 {
 	unsigned long pfn;
 
@@ -295,8 +294,7 @@ static void __iomem *acpi_map(acpi_physical_address pg_off, unsigned long pg_sz,
 			return NULL;
 		return (void __iomem __force *)kmap(pfn_to_page(pfn));
 	} else
-		return memory ? acpi_os_memmap(pg_off, pg_sz) :
-				acpi_os_ioremap(pg_off, pg_sz);
+		return acpi_os_ioremap(pg_off, pg_sz);
 }
 
 static void acpi_unmap(acpi_physical_address pg_off, void __iomem *vaddr)
@@ -311,10 +309,9 @@ static void acpi_unmap(acpi_physical_address pg_off, void __iomem *vaddr)
 }
 
 /**
- * __acpi_os_map_iomem - Get a virtual address for a given physical address range.
+ * acpi_os_map_iomem - Get a virtual address for a given physical address range.
  * @phys: Start of the physical address range to map.
  * @size: Size of the physical address range to map.
- * @memory: true if remapping memory, false if IO
  *
  * Look up the given physical address range in the list of existing ACPI memory
  * mappings.  If found, get a reference to it and return a pointer to it (its
@@ -324,8 +321,8 @@ static void acpi_unmap(acpi_physical_address pg_off, void __iomem *vaddr)
  * During early init (when acpi_permanent_mmap has not been set yet) this
  * routine simply calls __acpi_map_table() to get the job done.
  */
-static void __iomem __ref
-*__acpi_os_map_iomem(acpi_physical_address phys, acpi_size size, bool memory)
+void __iomem __ref
+*acpi_os_map_iomem(acpi_physical_address phys, acpi_size size)
 {
 	struct acpi_ioremap *map;
 	void __iomem *virt;
@@ -356,7 +353,7 @@ static void __iomem __ref
 
 	pg_off = round_down(phys, PAGE_SIZE);
 	pg_sz = round_up(phys + size, PAGE_SIZE) - pg_off;
-	virt = acpi_map(phys, size, memory);
+	virt = acpi_map(phys, size);
 	if (!virt) {
 		mutex_unlock(&acpi_ioremap_lock);
 		kfree(map);
@@ -375,17 +372,11 @@ static void __iomem __ref
 	mutex_unlock(&acpi_ioremap_lock);
 	return map->virt + (phys - map->phys);
 }
-
-void __iomem *__ref
-acpi_os_map_iomem(acpi_physical_address phys, acpi_size size)
-{
-	return __acpi_os_map_iomem(phys, size, false);
-}
 EXPORT_SYMBOL_GPL(acpi_os_map_iomem);
 
 void *__ref acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
 {
-	return (void *)__acpi_os_map_iomem(phys, size, true);
+	return (void *)acpi_os_map_iomem(phys, size);
 }
 EXPORT_SYMBOL_GPL(acpi_os_map_memory);
 
diff --git a/include/acpi/acpi_io.h b/include/acpi/acpi_io.h
index a0212e67d6f4..027faa8883aa 100644
--- a/include/acpi/acpi_io.h
+++ b/include/acpi/acpi_io.h
@@ -14,14 +14,6 @@ static inline void __iomem *acpi_os_ioremap(acpi_physical_address phys,
 }
 #endif
 
-#ifndef acpi_os_memmap
-static inline void __iomem *acpi_os_memmap(acpi_physical_address phys,
-					    acpi_size size)
-{
-	return ioremap_cache(phys, size);
-}
-#endif
-
 extern bool acpi_permanent_mmap;
 
 void __iomem __ref
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [PATCH] Revert "ACPI: Add memory semantics to acpi_os_map_memory()"
  2021-09-10 12:28 [PATCH] Revert "ACPI: Add memory semantics to acpi_os_map_memory()" Jia He
@ 2021-09-10 13:54 ` Lorenzo Pieralisi
  2021-09-10 14:32 ` [PATCH v2] " Jia He
  1 sibling, 0 replies; 23+ messages in thread
From: Lorenzo Pieralisi @ 2021-09-10 13:54 UTC (permalink / raw)
  To: Jia He
  Cc: Will Deacon, Len Brown, Robert Moore, Erik Kaneda,
	linux-arm-kernel, linux-kernel, linux-acpi, devel,
	Ard Biesheuvel, Hanjun Guo, Catalin Marinas, Rafael J . Wysocki,
	harb

[dropped CC stable, +CC Harb]

On Fri, Sep 10, 2021 at 08:28:20PM +0800, Jia He wrote:
> This reverts commit 437b38c51162f8b87beb28a833c4d5dc85fa864e.
> 
> After this commit, a boot panic is alway hit on an Ampere EMAG server
> with call trace as follows:
>  Internal error: synchronous external abort: 96000410 [#1] SMP
>  Modules linked in:
>  CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.14.0+ #462
>  Hardware name: MiTAC RAPTOR EV-883832-X3-0001/RAPTOR, BIOS 0.14 02/22/2019
>  pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [...snip...]
>  Call trace:
>   acpi_ex_system_memory_space_handler+0x26c/0x2c8
>   acpi_ev_address_space_dispatch+0x228/0x2c4
>   acpi_ex_access_region+0x114/0x268
>   acpi_ex_field_datum_io+0x128/0x1b8
>   acpi_ex_extract_from_field+0x14c/0x2ac
>   acpi_ex_read_data_from_field+0x190/0x1b8
>   acpi_ex_resolve_node_to_value+0x1ec/0x288
>   acpi_ex_resolve_to_value+0x250/0x274
>   acpi_ds_evaluate_name_path+0xac/0x124
>   acpi_ds_exec_end_op+0x90/0x410
>   acpi_ps_parse_loop+0x4ac/0x5d8
>   acpi_ps_parse_aml+0xe0/0x2c8
>   acpi_ps_execute_method+0x19c/0x1ac
>   acpi_ns_evaluate+0x1f8/0x26c
>   acpi_ns_init_one_device+0x104/0x140
>   acpi_ns_walk_namespace+0x158/0x1d0
>   acpi_ns_initialize_devices+0x194/0x218
>   acpi_initialize_objects+0x48/0x50
>   acpi_init+0xe0/0x498
> 
> From the debugging, we're mapping something which is *not* described by
> the EFI memory map, but *does* want PROT_NORMAL_NC.

"Does not" you mean. We are forcing memory semantics mappings to
PROT_NORMAL_NC, which eMAG does not like at all and I'd need to
understand why.

It looks like the issue happen in SystemMemory Opregion handler.

> 
> Hence just revert it before everything is clear.
> 
> Fixes: 437b38c51162 ("ACPI: Add memory semantics to acpi_os_map_memory()")
> Cc: stable@vger.kernel.org

No need, it is not even in an -rc yet (and stable should not be CCed in
the addressees CC list).

Thanks,
Lorenzo

> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Ard Biesheuvel <ardb@kernel.org>
> Cc: Hanjun Guo <guohanjun@huawei.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Signed-off-by: Jia He <justin.he@arm.com>
> ---
>  arch/arm64/include/asm/acpi.h |  3 ---
>  arch/arm64/kernel/acpi.c      | 19 +++----------------
>  drivers/acpi/osl.c            | 23 +++++++----------------
>  include/acpi/acpi_io.h        |  8 --------
>  4 files changed, 10 insertions(+), 43 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
> index 7535dc7cc5aa..bd68e1b7f29f 100644
> --- a/arch/arm64/include/asm/acpi.h
> +++ b/arch/arm64/include/asm/acpi.h
> @@ -50,9 +50,6 @@ pgprot_t __acpi_get_mem_attribute(phys_addr_t addr);
>  void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size);
>  #define acpi_os_ioremap acpi_os_ioremap
>  
> -void __iomem *acpi_os_memmap(acpi_physical_address phys, acpi_size size);
> -#define acpi_os_memmap acpi_os_memmap
> -
>  typedef u64 phys_cpuid_t;
>  #define PHYS_CPUID_INVALID INVALID_HWID
>  
> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
> index 1c9c2f7a1c04..f3851724fe35 100644
> --- a/arch/arm64/kernel/acpi.c
> +++ b/arch/arm64/kernel/acpi.c
> @@ -273,8 +273,7 @@ pgprot_t __acpi_get_mem_attribute(phys_addr_t addr)
>  	return __pgprot(PROT_DEVICE_nGnRnE);
>  }
>  
> -static void __iomem *__acpi_os_ioremap(acpi_physical_address phys,
> -				       acpi_size size, bool memory)
> +void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size)
>  {
>  	efi_memory_desc_t *md, *region = NULL;
>  	pgprot_t prot;
> @@ -300,11 +299,9 @@ static void __iomem *__acpi_os_ioremap(acpi_physical_address phys,
>  	 * It is fine for AML to remap regions that are not represented in the
>  	 * EFI memory map at all, as it only describes normal memory, and MMIO
>  	 * regions that require a virtual mapping to make them accessible to
> -	 * the EFI runtime services. Determine the region default
> -	 * attributes by checking the requested memory semantics.
> +	 * the EFI runtime services.
>  	 */
> -	prot = memory ? __pgprot(PROT_NORMAL_NC) :
> -			__pgprot(PROT_DEVICE_nGnRnE);
> +	prot = __pgprot(PROT_DEVICE_nGnRnE);
>  	if (region) {
>  		switch (region->type) {
>  		case EFI_LOADER_CODE:
> @@ -364,16 +361,6 @@ static void __iomem *__acpi_os_ioremap(acpi_physical_address phys,
>  	return __ioremap(phys, size, prot);
>  }
>  
> -void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size)
> -{
> -	return __acpi_os_ioremap(phys, size, false);
> -}
> -
> -void __iomem *acpi_os_memmap(acpi_physical_address phys, acpi_size size)
> -{
> -	return __acpi_os_ioremap(phys, size, true);
> -}
> -
>  /*
>   * Claim Synchronous External Aborts as a firmware first notification.
>   *
> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> index a43f1521efe6..45c5c0e45e33 100644
> --- a/drivers/acpi/osl.c
> +++ b/drivers/acpi/osl.c
> @@ -284,8 +284,7 @@ acpi_map_lookup_virt(void __iomem *virt, acpi_size size)
>  #define should_use_kmap(pfn)   page_is_ram(pfn)
>  #endif
>  
> -static void __iomem *acpi_map(acpi_physical_address pg_off, unsigned long pg_sz,
> -			      bool memory)
> +static void __iomem *acpi_map(acpi_physical_address pg_off, unsigned long pg_sz)
>  {
>  	unsigned long pfn;
>  
> @@ -295,8 +294,7 @@ static void __iomem *acpi_map(acpi_physical_address pg_off, unsigned long pg_sz,
>  			return NULL;
>  		return (void __iomem __force *)kmap(pfn_to_page(pfn));
>  	} else
> -		return memory ? acpi_os_memmap(pg_off, pg_sz) :
> -				acpi_os_ioremap(pg_off, pg_sz);
> +		return acpi_os_ioremap(pg_off, pg_sz);
>  }
>  
>  static void acpi_unmap(acpi_physical_address pg_off, void __iomem *vaddr)
> @@ -311,10 +309,9 @@ static void acpi_unmap(acpi_physical_address pg_off, void __iomem *vaddr)
>  }
>  
>  /**
> - * __acpi_os_map_iomem - Get a virtual address for a given physical address range.
> + * acpi_os_map_iomem - Get a virtual address for a given physical address range.
>   * @phys: Start of the physical address range to map.
>   * @size: Size of the physical address range to map.
> - * @memory: true if remapping memory, false if IO
>   *
>   * Look up the given physical address range in the list of existing ACPI memory
>   * mappings.  If found, get a reference to it and return a pointer to it (its
> @@ -324,8 +321,8 @@ static void acpi_unmap(acpi_physical_address pg_off, void __iomem *vaddr)
>   * During early init (when acpi_permanent_mmap has not been set yet) this
>   * routine simply calls __acpi_map_table() to get the job done.
>   */
> -static void __iomem __ref
> -*__acpi_os_map_iomem(acpi_physical_address phys, acpi_size size, bool memory)
> +void __iomem __ref
> +*acpi_os_map_iomem(acpi_physical_address phys, acpi_size size)
>  {
>  	struct acpi_ioremap *map;
>  	void __iomem *virt;
> @@ -356,7 +353,7 @@ static void __iomem __ref
>  
>  	pg_off = round_down(phys, PAGE_SIZE);
>  	pg_sz = round_up(phys + size, PAGE_SIZE) - pg_off;
> -	virt = acpi_map(phys, size, memory);
> +	virt = acpi_map(phys, size);
>  	if (!virt) {
>  		mutex_unlock(&acpi_ioremap_lock);
>  		kfree(map);
> @@ -375,17 +372,11 @@ static void __iomem __ref
>  	mutex_unlock(&acpi_ioremap_lock);
>  	return map->virt + (phys - map->phys);
>  }
> -
> -void __iomem *__ref
> -acpi_os_map_iomem(acpi_physical_address phys, acpi_size size)
> -{
> -	return __acpi_os_map_iomem(phys, size, false);
> -}
>  EXPORT_SYMBOL_GPL(acpi_os_map_iomem);
>  
>  void *__ref acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
>  {
> -	return (void *)__acpi_os_map_iomem(phys, size, true);
> +	return (void *)acpi_os_map_iomem(phys, size);
>  }
>  EXPORT_SYMBOL_GPL(acpi_os_map_memory);
>  
> diff --git a/include/acpi/acpi_io.h b/include/acpi/acpi_io.h
> index a0212e67d6f4..027faa8883aa 100644
> --- a/include/acpi/acpi_io.h
> +++ b/include/acpi/acpi_io.h
> @@ -14,14 +14,6 @@ static inline void __iomem *acpi_os_ioremap(acpi_physical_address phys,
>  }
>  #endif
>  
> -#ifndef acpi_os_memmap
> -static inline void __iomem *acpi_os_memmap(acpi_physical_address phys,
> -					    acpi_size size)
> -{
> -	return ioremap_cache(phys, size);
> -}
> -#endif
> -
>  extern bool acpi_permanent_mmap;
>  
>  void __iomem __ref
> -- 
> 2.17.1
> 

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v2] Revert "ACPI: Add memory semantics to acpi_os_map_memory()"
  2021-09-10 12:28 [PATCH] Revert "ACPI: Add memory semantics to acpi_os_map_memory()" Jia He
  2021-09-10 13:54 ` Lorenzo Pieralisi
@ 2021-09-10 14:32 ` Jia He
  2021-09-10 17:28   ` Ard Biesheuvel
  2021-09-22 16:33   ` Lorenzo Pieralisi
  1 sibling, 2 replies; 23+ messages in thread
From: Jia He @ 2021-09-10 14:32 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: Will Deacon, Len Brown, Robert Moore, Erik Kaneda,
	linux-arm-kernel, linux-kernel, linux-acpi, devel, Jia He,
	Ard Biesheuvel, Hanjun Guo, Catalin Marinas, Rafael J . Wysocki,
	Harb Abdulhamid

This reverts commit 437b38c51162f8b87beb28a833c4d5dc85fa864e.

After this commit, a boot panic is alway hit on an Ampere EMAG server
with call trace as follows:
 Internal error: synchronous external abort: 96000410 [#1] SMP
 Modules linked in:
 CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.14.0+ #462
 Hardware name: MiTAC RAPTOR EV-883832-X3-0001/RAPTOR, BIOS 0.14 02/22/2019
 pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[...snip...]
 Call trace:
  acpi_ex_system_memory_space_handler+0x26c/0x2c8
  acpi_ev_address_space_dispatch+0x228/0x2c4
  acpi_ex_access_region+0x114/0x268
  acpi_ex_field_datum_io+0x128/0x1b8
  acpi_ex_extract_from_field+0x14c/0x2ac
  acpi_ex_read_data_from_field+0x190/0x1b8
  acpi_ex_resolve_node_to_value+0x1ec/0x288
  acpi_ex_resolve_to_value+0x250/0x274
  acpi_ds_evaluate_name_path+0xac/0x124
  acpi_ds_exec_end_op+0x90/0x410
  acpi_ps_parse_loop+0x4ac/0x5d8
  acpi_ps_parse_aml+0xe0/0x2c8
  acpi_ps_execute_method+0x19c/0x1ac
  acpi_ns_evaluate+0x1f8/0x26c
  acpi_ns_init_one_device+0x104/0x140
  acpi_ns_walk_namespace+0x158/0x1d0
  acpi_ns_initialize_devices+0x194/0x218
  acpi_initialize_objects+0x48/0x50
  acpi_init+0xe0/0x498

As mentioned by Lorenzo:
  "We are forcing memory semantics mappings to PROT_NORMAL_NC, which
  eMAG does not like at all and I'd need to understand why. It looks
  like the issue happen in SystemMemory Opregion handler."

Hence just revert it before everything is clear.

Fixes: 437b38c51162 ("ACPI: Add memory semantics to acpi_os_map_memory()")
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Ard Biesheuvel <ardb@kernel.org>
Cc: Hanjun Guo <guohanjun@huawei.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: Harb Abdulhamid <harb@amperecomputing.com>

Signed-off-by: Jia He <justin.he@arm.com>
---
v2: Improve the commit message

 arch/arm64/include/asm/acpi.h |  3 ---
 arch/arm64/kernel/acpi.c      | 19 +++----------------
 drivers/acpi/osl.c            | 23 +++++++----------------
 include/acpi/acpi_io.h        |  8 --------
 4 files changed, 10 insertions(+), 43 deletions(-)

diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
index 7535dc7cc5aa..bd68e1b7f29f 100644
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -50,9 +50,6 @@ pgprot_t __acpi_get_mem_attribute(phys_addr_t addr);
 void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size);
 #define acpi_os_ioremap acpi_os_ioremap
 
-void __iomem *acpi_os_memmap(acpi_physical_address phys, acpi_size size);
-#define acpi_os_memmap acpi_os_memmap
-
 typedef u64 phys_cpuid_t;
 #define PHYS_CPUID_INVALID INVALID_HWID
 
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
index 1c9c2f7a1c04..f3851724fe35 100644
--- a/arch/arm64/kernel/acpi.c
+++ b/arch/arm64/kernel/acpi.c
@@ -273,8 +273,7 @@ pgprot_t __acpi_get_mem_attribute(phys_addr_t addr)
 	return __pgprot(PROT_DEVICE_nGnRnE);
 }
 
-static void __iomem *__acpi_os_ioremap(acpi_physical_address phys,
-				       acpi_size size, bool memory)
+void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size)
 {
 	efi_memory_desc_t *md, *region = NULL;
 	pgprot_t prot;
@@ -300,11 +299,9 @@ static void __iomem *__acpi_os_ioremap(acpi_physical_address phys,
 	 * It is fine for AML to remap regions that are not represented in the
 	 * EFI memory map at all, as it only describes normal memory, and MMIO
 	 * regions that require a virtual mapping to make them accessible to
-	 * the EFI runtime services. Determine the region default
-	 * attributes by checking the requested memory semantics.
+	 * the EFI runtime services.
 	 */
-	prot = memory ? __pgprot(PROT_NORMAL_NC) :
-			__pgprot(PROT_DEVICE_nGnRnE);
+	prot = __pgprot(PROT_DEVICE_nGnRnE);
 	if (region) {
 		switch (region->type) {
 		case EFI_LOADER_CODE:
@@ -364,16 +361,6 @@ static void __iomem *__acpi_os_ioremap(acpi_physical_address phys,
 	return __ioremap(phys, size, prot);
 }
 
-void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size)
-{
-	return __acpi_os_ioremap(phys, size, false);
-}
-
-void __iomem *acpi_os_memmap(acpi_physical_address phys, acpi_size size)
-{
-	return __acpi_os_ioremap(phys, size, true);
-}
-
 /*
  * Claim Synchronous External Aborts as a firmware first notification.
  *
diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index a43f1521efe6..45c5c0e45e33 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -284,8 +284,7 @@ acpi_map_lookup_virt(void __iomem *virt, acpi_size size)
 #define should_use_kmap(pfn)   page_is_ram(pfn)
 #endif
 
-static void __iomem *acpi_map(acpi_physical_address pg_off, unsigned long pg_sz,
-			      bool memory)
+static void __iomem *acpi_map(acpi_physical_address pg_off, unsigned long pg_sz)
 {
 	unsigned long pfn;
 
@@ -295,8 +294,7 @@ static void __iomem *acpi_map(acpi_physical_address pg_off, unsigned long pg_sz,
 			return NULL;
 		return (void __iomem __force *)kmap(pfn_to_page(pfn));
 	} else
-		return memory ? acpi_os_memmap(pg_off, pg_sz) :
-				acpi_os_ioremap(pg_off, pg_sz);
+		return acpi_os_ioremap(pg_off, pg_sz);
 }
 
 static void acpi_unmap(acpi_physical_address pg_off, void __iomem *vaddr)
@@ -311,10 +309,9 @@ static void acpi_unmap(acpi_physical_address pg_off, void __iomem *vaddr)
 }
 
 /**
- * __acpi_os_map_iomem - Get a virtual address for a given physical address range.
+ * acpi_os_map_iomem - Get a virtual address for a given physical address range.
  * @phys: Start of the physical address range to map.
  * @size: Size of the physical address range to map.
- * @memory: true if remapping memory, false if IO
  *
  * Look up the given physical address range in the list of existing ACPI memory
  * mappings.  If found, get a reference to it and return a pointer to it (its
@@ -324,8 +321,8 @@ static void acpi_unmap(acpi_physical_address pg_off, void __iomem *vaddr)
  * During early init (when acpi_permanent_mmap has not been set yet) this
  * routine simply calls __acpi_map_table() to get the job done.
  */
-static void __iomem __ref
-*__acpi_os_map_iomem(acpi_physical_address phys, acpi_size size, bool memory)
+void __iomem __ref
+*acpi_os_map_iomem(acpi_physical_address phys, acpi_size size)
 {
 	struct acpi_ioremap *map;
 	void __iomem *virt;
@@ -356,7 +353,7 @@ static void __iomem __ref
 
 	pg_off = round_down(phys, PAGE_SIZE);
 	pg_sz = round_up(phys + size, PAGE_SIZE) - pg_off;
-	virt = acpi_map(phys, size, memory);
+	virt = acpi_map(phys, size);
 	if (!virt) {
 		mutex_unlock(&acpi_ioremap_lock);
 		kfree(map);
@@ -375,17 +372,11 @@ static void __iomem __ref
 	mutex_unlock(&acpi_ioremap_lock);
 	return map->virt + (phys - map->phys);
 }
-
-void __iomem *__ref
-acpi_os_map_iomem(acpi_physical_address phys, acpi_size size)
-{
-	return __acpi_os_map_iomem(phys, size, false);
-}
 EXPORT_SYMBOL_GPL(acpi_os_map_iomem);
 
 void *__ref acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
 {
-	return (void *)__acpi_os_map_iomem(phys, size, true);
+	return (void *)acpi_os_map_iomem(phys, size);
 }
 EXPORT_SYMBOL_GPL(acpi_os_map_memory);
 
diff --git a/include/acpi/acpi_io.h b/include/acpi/acpi_io.h
index a0212e67d6f4..027faa8883aa 100644
--- a/include/acpi/acpi_io.h
+++ b/include/acpi/acpi_io.h
@@ -14,14 +14,6 @@ static inline void __iomem *acpi_os_ioremap(acpi_physical_address phys,
 }
 #endif
 
-#ifndef acpi_os_memmap
-static inline void __iomem *acpi_os_memmap(acpi_physical_address phys,
-					    acpi_size size)
-{
-	return ioremap_cache(phys, size);
-}
-#endif
-
 extern bool acpi_permanent_mmap;
 
 void __iomem __ref
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [PATCH v2] Revert "ACPI: Add memory semantics to acpi_os_map_memory()"
  2021-09-10 14:32 ` [PATCH v2] " Jia He
@ 2021-09-10 17:28   ` Ard Biesheuvel
  2021-09-11 10:14     ` Lorenzo Pieralisi
  2021-09-16 16:08     ` Lorenzo Pieralisi
  2021-09-22 16:33   ` Lorenzo Pieralisi
  1 sibling, 2 replies; 23+ messages in thread
From: Ard Biesheuvel @ 2021-09-10 17:28 UTC (permalink / raw)
  To: Jia He
  Cc: Lorenzo Pieralisi, Will Deacon, Len Brown, Robert Moore,
	Erik Kaneda, Linux ARM, Linux Kernel Mailing List,
	ACPI Devel Maling List,
	open list:ACPI COMPONENT ARCHITECTURE (ACPICA),
	Hanjun Guo, Catalin Marinas, Rafael J . Wysocki, Harb Abdulhamid

On Fri, 10 Sept 2021 at 16:32, Jia He <justin.he@arm.com> wrote:
>
> This reverts commit 437b38c51162f8b87beb28a833c4d5dc85fa864e.
>
> After this commit, a boot panic is alway hit on an Ampere EMAG server
> with call trace as follows:
>  Internal error: synchronous external abort: 96000410 [#1] SMP
>  Modules linked in:
>  CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.14.0+ #462
>  Hardware name: MiTAC RAPTOR EV-883832-X3-0001/RAPTOR, BIOS 0.14 02/22/2019
>  pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [...snip...]
>  Call trace:
>   acpi_ex_system_memory_space_handler+0x26c/0x2c8
>   acpi_ev_address_space_dispatch+0x228/0x2c4
>   acpi_ex_access_region+0x114/0x268
>   acpi_ex_field_datum_io+0x128/0x1b8
>   acpi_ex_extract_from_field+0x14c/0x2ac
>   acpi_ex_read_data_from_field+0x190/0x1b8
>   acpi_ex_resolve_node_to_value+0x1ec/0x288
>   acpi_ex_resolve_to_value+0x250/0x274
>   acpi_ds_evaluate_name_path+0xac/0x124
>   acpi_ds_exec_end_op+0x90/0x410
>   acpi_ps_parse_loop+0x4ac/0x5d8
>   acpi_ps_parse_aml+0xe0/0x2c8
>   acpi_ps_execute_method+0x19c/0x1ac
>   acpi_ns_evaluate+0x1f8/0x26c
>   acpi_ns_init_one_device+0x104/0x140
>   acpi_ns_walk_namespace+0x158/0x1d0
>   acpi_ns_initialize_devices+0x194/0x218
>   acpi_initialize_objects+0x48/0x50
>   acpi_init+0xe0/0x498
>
> As mentioned by Lorenzo:
>   "We are forcing memory semantics mappings to PROT_NORMAL_NC, which
>   eMAG does not like at all and I'd need to understand why. It looks
>   like the issue happen in SystemMemory Opregion handler."
>
> Hence just revert it before everything is clear.
>

Can we try to find the root cause first? -rc1 is not even out yet, and
reverting it now means we can not resubmit it until the next merge
window.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2] Revert "ACPI: Add memory semantics to acpi_os_map_memory()"
  2021-09-10 17:28   ` Ard Biesheuvel
@ 2021-09-11 10:14     ` Lorenzo Pieralisi
  2021-09-16 16:08     ` Lorenzo Pieralisi
  1 sibling, 0 replies; 23+ messages in thread
From: Lorenzo Pieralisi @ 2021-09-11 10:14 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Jia He, Will Deacon, Len Brown, Robert Moore, Erik Kaneda,
	Linux ARM, Linux Kernel Mailing List, ACPI Devel Maling List,
	open list:ACPI COMPONENT ARCHITECTURE (ACPICA),
	Hanjun Guo, Catalin Marinas, Rafael J . Wysocki, Harb Abdulhamid

On Fri, Sep 10, 2021 at 07:28:49PM +0200, Ard Biesheuvel wrote:
> On Fri, 10 Sept 2021 at 16:32, Jia He <justin.he@arm.com> wrote:
> >
> > This reverts commit 437b38c51162f8b87beb28a833c4d5dc85fa864e.
> >
> > After this commit, a boot panic is alway hit on an Ampere EMAG server
> > with call trace as follows:
> >  Internal error: synchronous external abort: 96000410 [#1] SMP
> >  Modules linked in:
> >  CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.14.0+ #462
> >  Hardware name: MiTAC RAPTOR EV-883832-X3-0001/RAPTOR, BIOS 0.14 02/22/2019
> >  pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> > [...snip...]
> >  Call trace:
> >   acpi_ex_system_memory_space_handler+0x26c/0x2c8
> >   acpi_ev_address_space_dispatch+0x228/0x2c4
> >   acpi_ex_access_region+0x114/0x268
> >   acpi_ex_field_datum_io+0x128/0x1b8
> >   acpi_ex_extract_from_field+0x14c/0x2ac
> >   acpi_ex_read_data_from_field+0x190/0x1b8
> >   acpi_ex_resolve_node_to_value+0x1ec/0x288
> >   acpi_ex_resolve_to_value+0x250/0x274
> >   acpi_ds_evaluate_name_path+0xac/0x124
> >   acpi_ds_exec_end_op+0x90/0x410
> >   acpi_ps_parse_loop+0x4ac/0x5d8
> >   acpi_ps_parse_aml+0xe0/0x2c8
> >   acpi_ps_execute_method+0x19c/0x1ac
> >   acpi_ns_evaluate+0x1f8/0x26c
> >   acpi_ns_init_one_device+0x104/0x140
> >   acpi_ns_walk_namespace+0x158/0x1d0
> >   acpi_ns_initialize_devices+0x194/0x218
> >   acpi_initialize_objects+0x48/0x50
> >   acpi_init+0xe0/0x498
> >
> > As mentioned by Lorenzo:
> >   "We are forcing memory semantics mappings to PROT_NORMAL_NC, which
> >   eMAG does not like at all and I'd need to understand why. It looks
> >   like the issue happen in SystemMemory Opregion handler."
> >
> > Hence just revert it before everything is clear.
> >
> 
> Can we try to find the root cause first? -rc1 is not even out yet, and
> reverting it now means we can not resubmit it until the next merge
> window.

Yes, absolutely. We need to understand where the problem is, because it
looks like we can't map SystemMemory Opregion with NORMAL_NC if the PA
is not in the EFI map, that's a problem (ie how can we determine the
right memory attributes for SystemMemory Operation regions then) but
let's not speculate and find what the issue is first.

Lorenzo

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2] Revert "ACPI: Add memory semantics to acpi_os_map_memory()"
  2021-09-10 17:28   ` Ard Biesheuvel
  2021-09-11 10:14     ` Lorenzo Pieralisi
@ 2021-09-16 16:08     ` Lorenzo Pieralisi
  2021-09-20 17:00       ` Lorenzo Pieralisi
  1 sibling, 1 reply; 23+ messages in thread
From: Lorenzo Pieralisi @ 2021-09-16 16:08 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Jia He, Will Deacon, Len Brown, Robert Moore, Erik Kaneda,
	Linux ARM, Linux Kernel Mailing List, ACPI Devel Maling List,
	open list:ACPI COMPONENT ARCHITECTURE (ACPICA),
	Hanjun Guo, Catalin Marinas, Rafael J . Wysocki, Harb Abdulhamid

On Fri, Sep 10, 2021 at 07:28:49PM +0200, Ard Biesheuvel wrote:
> On Fri, 10 Sept 2021 at 16:32, Jia He <justin.he@arm.com> wrote:
> >
> > This reverts commit 437b38c51162f8b87beb28a833c4d5dc85fa864e.
> >
> > After this commit, a boot panic is alway hit on an Ampere EMAG server
> > with call trace as follows:
> >  Internal error: synchronous external abort: 96000410 [#1] SMP
> >  Modules linked in:
> >  CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.14.0+ #462
> >  Hardware name: MiTAC RAPTOR EV-883832-X3-0001/RAPTOR, BIOS 0.14 02/22/2019
> >  pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> > [...snip...]
> >  Call trace:
> >   acpi_ex_system_memory_space_handler+0x26c/0x2c8
> >   acpi_ev_address_space_dispatch+0x228/0x2c4
> >   acpi_ex_access_region+0x114/0x268
> >   acpi_ex_field_datum_io+0x128/0x1b8
> >   acpi_ex_extract_from_field+0x14c/0x2ac
> >   acpi_ex_read_data_from_field+0x190/0x1b8
> >   acpi_ex_resolve_node_to_value+0x1ec/0x288
> >   acpi_ex_resolve_to_value+0x250/0x274
> >   acpi_ds_evaluate_name_path+0xac/0x124
> >   acpi_ds_exec_end_op+0x90/0x410
> >   acpi_ps_parse_loop+0x4ac/0x5d8
> >   acpi_ps_parse_aml+0xe0/0x2c8
> >   acpi_ps_execute_method+0x19c/0x1ac
> >   acpi_ns_evaluate+0x1f8/0x26c
> >   acpi_ns_init_one_device+0x104/0x140
> >   acpi_ns_walk_namespace+0x158/0x1d0
> >   acpi_ns_initialize_devices+0x194/0x218
> >   acpi_initialize_objects+0x48/0x50
> >   acpi_init+0xe0/0x498
> >
> > As mentioned by Lorenzo:
> >   "We are forcing memory semantics mappings to PROT_NORMAL_NC, which
> >   eMAG does not like at all and I'd need to understand why. It looks
> >   like the issue happen in SystemMemory Opregion handler."
> >
> > Hence just revert it before everything is clear.
> >
> 
> Can we try to find the root cause first? -rc1 is not even out yet, and
> reverting it now means we can not resubmit it until the next merge
> window.

I am waiting to debug this on an eMAG but I noticed something that
I wanted to bring up.

SystemMemory Operation region handler - ie

acpi_ex_system_memory_space_handler()

maps the Operation Region (that AFAICS is MMIO, it is _not_ memory)
with acpi_os_map_memory() and I believe that's what is causing this
bug.

On the other hand, acpi_os_map_generic_address(), to handle spaceid
ACPI_ADR_SPACE_SYSTEM_MEMORY, uses acpi_os_map_iomem() that is more
in line with my expectations.

Question is: is the mapping in acpi_ex_system_memory_space_handler()
wrong (and should be patched with acpi_os_map_iomem() ?)

On x86 this should not change a thing, on ARM it would.

I don't think it is right to map SystemMemory Operation regions with
memory semantics but on the other hand, other than the EFI memory map,
there is nothing we can do to determine what a SystemMemory Operation
region address space actually represents.

Thoughts ? Before embarking on patching

acpi_ex_system_memory_space_handler()

I want to make sure my understanding of the SystemMemory space is
correct, comments welcome.

I will pinpoint the trigger for this bug shortly and before doing
anything else.

Thanks,
Lorenzo

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2] Revert "ACPI: Add memory semantics to acpi_os_map_memory()"
  2021-09-16 16:08     ` Lorenzo Pieralisi
@ 2021-09-20 17:00       ` Lorenzo Pieralisi
  2021-09-20 17:32         ` Rafael J. Wysocki
  0 siblings, 1 reply; 23+ messages in thread
From: Lorenzo Pieralisi @ 2021-09-20 17:00 UTC (permalink / raw)
  To: Ard Biesheuvel, rafael.j.wysocki, rjw
  Cc: Jia He, Will Deacon, Len Brown, Robert Moore, Erik Kaneda,
	Linux ARM, Linux Kernel Mailing List, ACPI Devel Maling List,
	open list:ACPI COMPONENT ARCHITECTURE (ACPICA),
	Hanjun Guo, Catalin Marinas, Harb Abdulhamid

On Thu, Sep 16, 2021 at 05:08:27PM +0100, Lorenzo Pieralisi wrote:
> On Fri, Sep 10, 2021 at 07:28:49PM +0200, Ard Biesheuvel wrote:
> > On Fri, 10 Sept 2021 at 16:32, Jia He <justin.he@arm.com> wrote:
> > >
> > > This reverts commit 437b38c51162f8b87beb28a833c4d5dc85fa864e.
> > >
> > > After this commit, a boot panic is alway hit on an Ampere EMAG server
> > > with call trace as follows:
> > >  Internal error: synchronous external abort: 96000410 [#1] SMP
> > >  Modules linked in:
> > >  CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.14.0+ #462
> > >  Hardware name: MiTAC RAPTOR EV-883832-X3-0001/RAPTOR, BIOS 0.14 02/22/2019
> > >  pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> > > [...snip...]
> > >  Call trace:
> > >   acpi_ex_system_memory_space_handler+0x26c/0x2c8
> > >   acpi_ev_address_space_dispatch+0x228/0x2c4
> > >   acpi_ex_access_region+0x114/0x268
> > >   acpi_ex_field_datum_io+0x128/0x1b8
> > >   acpi_ex_extract_from_field+0x14c/0x2ac
> > >   acpi_ex_read_data_from_field+0x190/0x1b8
> > >   acpi_ex_resolve_node_to_value+0x1ec/0x288
> > >   acpi_ex_resolve_to_value+0x250/0x274
> > >   acpi_ds_evaluate_name_path+0xac/0x124
> > >   acpi_ds_exec_end_op+0x90/0x410
> > >   acpi_ps_parse_loop+0x4ac/0x5d8
> > >   acpi_ps_parse_aml+0xe0/0x2c8
> > >   acpi_ps_execute_method+0x19c/0x1ac
> > >   acpi_ns_evaluate+0x1f8/0x26c
> > >   acpi_ns_init_one_device+0x104/0x140
> > >   acpi_ns_walk_namespace+0x158/0x1d0
> > >   acpi_ns_initialize_devices+0x194/0x218
> > >   acpi_initialize_objects+0x48/0x50
> > >   acpi_init+0xe0/0x498
> > >
> > > As mentioned by Lorenzo:
> > >   "We are forcing memory semantics mappings to PROT_NORMAL_NC, which
> > >   eMAG does not like at all and I'd need to understand why. It looks
> > >   like the issue happen in SystemMemory Opregion handler."
> > >
> > > Hence just revert it before everything is clear.
> > >
> > 
> > Can we try to find the root cause first? -rc1 is not even out yet, and
> > reverting it now means we can not resubmit it until the next merge
> > window.
> 
> I am waiting to debug this on an eMAG but I noticed something that
> I wanted to bring up.
> 
> SystemMemory Operation region handler - ie
> 
> acpi_ex_system_memory_space_handler()
> 
> maps the Operation Region (that AFAICS is MMIO, it is _not_ memory)
> with acpi_os_map_memory() and I believe that's what is causing this
> bug.
> 
> On the other hand, acpi_os_map_generic_address(), to handle spaceid
> ACPI_ADR_SPACE_SYSTEM_MEMORY, uses acpi_os_map_iomem() that is more
> in line with my expectations.

Hi Rafael,

I wanted to ask please if you have any insights on why

(1) acpi_ex_system_memory_space_handler()
(2) acpi_os_map_generic_address()

Use two different calls to map memory for the _same_ address space ID
(SystemMemory).

(3) acpi_os_map_memory()
vs
(4) acpi_os_map_iomem()

I am struggling to understand why (1) uses (3) ("memory semantics") when
(2) uses (4) - it is actually unclear how the distinction between
the two mapping APIs is to be drawn and on what basis one should
choose which one to use.

I am still waiting to grab some HW to debug this report but the issue
here is that we are mapping an OpRegion SystemMemory with (3) in the
memory space handler and given the patch we are reverting we end up
mapping the operation region with normal non-cacheable memory attributes
that probably the physical address range behind the OpRegion does not
support.

Thanks a lot,
Lorenzo

> 
> Question is: is the mapping in acpi_ex_system_memory_space_handler()
> wrong (and should be patched with acpi_os_map_iomem() ?)
> 
> On x86 this should not change a thing, on ARM it would.
> 
> I don't think it is right to map SystemMemory Operation regions with
> memory semantics but on the other hand, other than the EFI memory map,
> there is nothing we can do to determine what a SystemMemory Operation
> region address space actually represents.
> 
> Thoughts ? Before embarking on patching
> 
> acpi_ex_system_memory_space_handler()
> 
> I want to make sure my understanding of the SystemMemory space is
> correct, comments welcome.
> 
> I will pinpoint the trigger for this bug shortly and before doing
> anything else.
> 
> Thanks,
> Lorenzo

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2] Revert "ACPI: Add memory semantics to acpi_os_map_memory()"
  2021-09-20 17:00       ` Lorenzo Pieralisi
@ 2021-09-20 17:32         ` Rafael J. Wysocki
  2021-09-21 10:05           ` Lorenzo Pieralisi
  0 siblings, 1 reply; 23+ messages in thread
From: Rafael J. Wysocki @ 2021-09-20 17:32 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: Ard Biesheuvel, Rafael Wysocki, Rafael J. Wysocki, Jia He,
	Will Deacon, Len Brown, Robert Moore, Erik Kaneda, Linux ARM,
	Linux Kernel Mailing List, ACPI Devel Maling List,
	open list:ACPI COMPONENT ARCHITECTURE (ACPICA),
	Hanjun Guo, Catalin Marinas, Harb Abdulhamid

On Mon, Sep 20, 2021 at 7:03 PM Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
>
> On Thu, Sep 16, 2021 at 05:08:27PM +0100, Lorenzo Pieralisi wrote:
> > On Fri, Sep 10, 2021 at 07:28:49PM +0200, Ard Biesheuvel wrote:
> > > On Fri, 10 Sept 2021 at 16:32, Jia He <justin.he@arm.com> wrote:
> > > >
> > > > This reverts commit 437b38c51162f8b87beb28a833c4d5dc85fa864e.
> > > >
> > > > After this commit, a boot panic is alway hit on an Ampere EMAG server
> > > > with call trace as follows:
> > > >  Internal error: synchronous external abort: 96000410 [#1] SMP
> > > >  Modules linked in:
> > > >  CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.14.0+ #462
> > > >  Hardware name: MiTAC RAPTOR EV-883832-X3-0001/RAPTOR, BIOS 0.14 02/22/2019
> > > >  pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> > > > [...snip...]
> > > >  Call trace:
> > > >   acpi_ex_system_memory_space_handler+0x26c/0x2c8
> > > >   acpi_ev_address_space_dispatch+0x228/0x2c4
> > > >   acpi_ex_access_region+0x114/0x268
> > > >   acpi_ex_field_datum_io+0x128/0x1b8
> > > >   acpi_ex_extract_from_field+0x14c/0x2ac
> > > >   acpi_ex_read_data_from_field+0x190/0x1b8
> > > >   acpi_ex_resolve_node_to_value+0x1ec/0x288
> > > >   acpi_ex_resolve_to_value+0x250/0x274
> > > >   acpi_ds_evaluate_name_path+0xac/0x124
> > > >   acpi_ds_exec_end_op+0x90/0x410
> > > >   acpi_ps_parse_loop+0x4ac/0x5d8
> > > >   acpi_ps_parse_aml+0xe0/0x2c8
> > > >   acpi_ps_execute_method+0x19c/0x1ac
> > > >   acpi_ns_evaluate+0x1f8/0x26c
> > > >   acpi_ns_init_one_device+0x104/0x140
> > > >   acpi_ns_walk_namespace+0x158/0x1d0
> > > >   acpi_ns_initialize_devices+0x194/0x218
> > > >   acpi_initialize_objects+0x48/0x50
> > > >   acpi_init+0xe0/0x498
> > > >
> > > > As mentioned by Lorenzo:
> > > >   "We are forcing memory semantics mappings to PROT_NORMAL_NC, which
> > > >   eMAG does not like at all and I'd need to understand why. It looks
> > > >   like the issue happen in SystemMemory Opregion handler."
> > > >
> > > > Hence just revert it before everything is clear.
> > > >
> > >
> > > Can we try to find the root cause first? -rc1 is not even out yet, and
> > > reverting it now means we can not resubmit it until the next merge
> > > window.
> >
> > I am waiting to debug this on an eMAG but I noticed something that
> > I wanted to bring up.
> >
> > SystemMemory Operation region handler - ie
> >
> > acpi_ex_system_memory_space_handler()
> >
> > maps the Operation Region (that AFAICS is MMIO, it is _not_ memory)
> > with acpi_os_map_memory() and I believe that's what is causing this
> > bug.
> >
> > On the other hand, acpi_os_map_generic_address(), to handle spaceid
> > ACPI_ADR_SPACE_SYSTEM_MEMORY, uses acpi_os_map_iomem() that is more
> > in line with my expectations.
>
> Hi Rafael,
>
> I wanted to ask please if you have any insights on why
>
> (1) acpi_ex_system_memory_space_handler()
> (2) acpi_os_map_generic_address()
>
> Use two different calls to map memory for the _same_ address space ID
> (SystemMemory).
>
> (3) acpi_os_map_memory()
> vs
> (4) acpi_os_map_iomem()

I don't really have a good answer here.

On x86 this doesn't really matter and that's where
acpi_ex_system_memory_space_handler() was first introduced.  It is not
only used for IOMEM (there are SystemMemory operation regions in RAM),
but since it may be in IOMEM, it should assume so.

> I am struggling to understand why (1) uses (3) ("memory semantics") when
> (2) uses (4) - it is actually unclear how the distinction between
> the two mapping APIs is to be drawn and on what basis one should
> choose which one to use.
>
> I am still waiting to grab some HW to debug this report but the issue
> here is that we are mapping an OpRegion SystemMemory with (3) in the
> memory space handler and given the patch we are reverting we end up
> mapping the operation region with normal non-cacheable memory attributes
> that probably the physical address range behind the OpRegion does not
> support.

If that is the case, there needs to be a mechanism to decide what kind
of mapping to use for SystemMemory operation regions based on the type
of physical memory the address range in question is located in.


> > Question is: is the mapping in acpi_ex_system_memory_space_handler()
> > wrong (and should be patched with acpi_os_map_iomem() ?)
> >
> > On x86 this should not change a thing, on ARM it would.
> >
> > I don't think it is right to map SystemMemory Operation regions with
> > memory semantics but on the other hand, other than the EFI memory map,
> > there is nothing we can do to determine what a SystemMemory Operation
> > region address space actually represents.
> >
> > Thoughts ? Before embarking on patching
> >
> > acpi_ex_system_memory_space_handler()
> >
> > I want to make sure my understanding of the SystemMemory space is
> > correct, comments welcome.
> >
> > I will pinpoint the trigger for this bug shortly and before doing
> > anything else.
> >
> > Thanks,
> > Lorenzo

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2] Revert "ACPI: Add memory semantics to acpi_os_map_memory()"
  2021-09-20 17:32         ` Rafael J. Wysocki
@ 2021-09-21 10:05           ` Lorenzo Pieralisi
  2021-09-22 11:11             ` Ard Biesheuvel
  0 siblings, 1 reply; 23+ messages in thread
From: Lorenzo Pieralisi @ 2021-09-21 10:05 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Ard Biesheuvel, Rafael Wysocki, Rafael J. Wysocki, Jia He,
	Will Deacon, Len Brown, Robert Moore, Erik Kaneda, Linux ARM,
	Linux Kernel Mailing List, ACPI Devel Maling List,
	open list:ACPI COMPONENT ARCHITECTURE (ACPICA),
	Hanjun Guo, Catalin Marinas, Harb Abdulhamid

On Mon, Sep 20, 2021 at 07:32:56PM +0200, Rafael J. Wysocki wrote:
> On Mon, Sep 20, 2021 at 7:03 PM Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
> >
> > On Thu, Sep 16, 2021 at 05:08:27PM +0100, Lorenzo Pieralisi wrote:
> > > On Fri, Sep 10, 2021 at 07:28:49PM +0200, Ard Biesheuvel wrote:
> > > > On Fri, 10 Sept 2021 at 16:32, Jia He <justin.he@arm.com> wrote:
> > > > >
> > > > > This reverts commit 437b38c51162f8b87beb28a833c4d5dc85fa864e.
> > > > >
> > > > > After this commit, a boot panic is alway hit on an Ampere EMAG server
> > > > > with call trace as follows:
> > > > >  Internal error: synchronous external abort: 96000410 [#1] SMP
> > > > >  Modules linked in:
> > > > >  CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.14.0+ #462
> > > > >  Hardware name: MiTAC RAPTOR EV-883832-X3-0001/RAPTOR, BIOS 0.14 02/22/2019
> > > > >  pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> > > > > [...snip...]
> > > > >  Call trace:
> > > > >   acpi_ex_system_memory_space_handler+0x26c/0x2c8
> > > > >   acpi_ev_address_space_dispatch+0x228/0x2c4
> > > > >   acpi_ex_access_region+0x114/0x268
> > > > >   acpi_ex_field_datum_io+0x128/0x1b8
> > > > >   acpi_ex_extract_from_field+0x14c/0x2ac
> > > > >   acpi_ex_read_data_from_field+0x190/0x1b8
> > > > >   acpi_ex_resolve_node_to_value+0x1ec/0x288
> > > > >   acpi_ex_resolve_to_value+0x250/0x274
> > > > >   acpi_ds_evaluate_name_path+0xac/0x124
> > > > >   acpi_ds_exec_end_op+0x90/0x410
> > > > >   acpi_ps_parse_loop+0x4ac/0x5d8
> > > > >   acpi_ps_parse_aml+0xe0/0x2c8
> > > > >   acpi_ps_execute_method+0x19c/0x1ac
> > > > >   acpi_ns_evaluate+0x1f8/0x26c
> > > > >   acpi_ns_init_one_device+0x104/0x140
> > > > >   acpi_ns_walk_namespace+0x158/0x1d0
> > > > >   acpi_ns_initialize_devices+0x194/0x218
> > > > >   acpi_initialize_objects+0x48/0x50
> > > > >   acpi_init+0xe0/0x498
> > > > >
> > > > > As mentioned by Lorenzo:
> > > > >   "We are forcing memory semantics mappings to PROT_NORMAL_NC, which
> > > > >   eMAG does not like at all and I'd need to understand why. It looks
> > > > >   like the issue happen in SystemMemory Opregion handler."
> > > > >
> > > > > Hence just revert it before everything is clear.
> > > > >
> > > >
> > > > Can we try to find the root cause first? -rc1 is not even out yet, and
> > > > reverting it now means we can not resubmit it until the next merge
> > > > window.
> > >
> > > I am waiting to debug this on an eMAG but I noticed something that
> > > I wanted to bring up.
> > >
> > > SystemMemory Operation region handler - ie
> > >
> > > acpi_ex_system_memory_space_handler()
> > >
> > > maps the Operation Region (that AFAICS is MMIO, it is _not_ memory)
> > > with acpi_os_map_memory() and I believe that's what is causing this
> > > bug.
> > >
> > > On the other hand, acpi_os_map_generic_address(), to handle spaceid
> > > ACPI_ADR_SPACE_SYSTEM_MEMORY, uses acpi_os_map_iomem() that is more
> > > in line with my expectations.
> >
> > Hi Rafael,
> >
> > I wanted to ask please if you have any insights on why
> >
> > (1) acpi_ex_system_memory_space_handler()
> > (2) acpi_os_map_generic_address()
> >
> > Use two different calls to map memory for the _same_ address space ID
> > (SystemMemory).
> >
> > (3) acpi_os_map_memory()
> > vs
> > (4) acpi_os_map_iomem()
> 
> I don't really have a good answer here.
> 
> On x86 this doesn't really matter and that's where
> acpi_ex_system_memory_space_handler() was first introduced.  It is not
> only used for IOMEM (there are SystemMemory operation regions in RAM),
> but since it may be in IOMEM, it should assume so.
> 
> > I am struggling to understand why (1) uses (3) ("memory semantics") when
> > (2) uses (4) - it is actually unclear how the distinction between
> > the two mapping APIs is to be drawn and on what basis one should
> > choose which one to use.
> >
> > I am still waiting to grab some HW to debug this report but the issue
> > here is that we are mapping an OpRegion SystemMemory with (3) in the
> > memory space handler and given the patch we are reverting we end up
> > mapping the operation region with normal non-cacheable memory attributes
> > that probably the physical address range behind the OpRegion does not
> > support.
> 
> If that is the case, there needs to be a mechanism to decide what kind
> of mapping to use for SystemMemory operation regions based on the type
> of physical memory the address range in question is located in.

Thank you Rafael. The mechanism we are currently relying on is the EFI
memory map but if the Opregion address is not described there then we
are left with a default choice to make (theoretically I may also parse
all _CRS in the namespace to find whether a resource include the
Opregion and I may infer attributes from the _CRS resource entry).

Maybe we should update the ACPI specs to enforce it; with current
firmware the idea of using the OS expected *usage* of memory (ie
memory vs IO) described by the mapping function prototype can't work
as this revert shows (even though it would be better if I manage
to find what the precise issue is).

We can't map something with specific attributes if we don't know
whether the physical address space backing the region supports it.

I am left with little choice: I assume the best thing I could do
to fix the original bug is to use ioremap_* in acpi_data_show()
instead of acpi_os_map/unmap_memory() to map that memory with
specific attributes (for BERT error regions, they must be RAM
so, _hopefully_, we know it can be mapped with eg normal memory
mappings).

Thoughts ?

Thanks a lot,
Lorenzo

> > > Question is: is the mapping in acpi_ex_system_memory_space_handler()
> > > wrong (and should be patched with acpi_os_map_iomem() ?)
> > >
> > > On x86 this should not change a thing, on ARM it would.
> > >
> > > I don't think it is right to map SystemMemory Operation regions with
> > > memory semantics but on the other hand, other than the EFI memory map,
> > > there is nothing we can do to determine what a SystemMemory Operation
> > > region address space actually represents.
> > >
> > > Thoughts ? Before embarking on patching
> > >
> > > acpi_ex_system_memory_space_handler()
> > >
> > > I want to make sure my understanding of the SystemMemory space is
> > > correct, comments welcome.
> > >
> > > I will pinpoint the trigger for this bug shortly and before doing
> > > anything else.
> > >
> > > Thanks,
> > > Lorenzo

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2] Revert "ACPI: Add memory semantics to acpi_os_map_memory()"
  2021-09-21 10:05           ` Lorenzo Pieralisi
@ 2021-09-22 11:11             ` Ard Biesheuvel
  2021-09-22 13:07               ` Lorenzo Pieralisi
  2021-09-22 23:45               ` Jeremy Linton
  0 siblings, 2 replies; 23+ messages in thread
From: Ard Biesheuvel @ 2021-09-22 11:11 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: Rafael J. Wysocki, Rafael Wysocki, Rafael J. Wysocki, Jia He,
	Will Deacon, Len Brown, Robert Moore, Erik Kaneda, Linux ARM,
	Linux Kernel Mailing List, ACPI Devel Maling List,
	open list:ACPI COMPONENT ARCHITECTURE (ACPICA),
	Hanjun Guo, Catalin Marinas, Harb Abdulhamid

On Tue, 21 Sept 2021 at 12:05, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
>
> On Mon, Sep 20, 2021 at 07:32:56PM +0200, Rafael J. Wysocki wrote:
> > On Mon, Sep 20, 2021 at 7:03 PM Lorenzo Pieralisi
> > <lorenzo.pieralisi@arm.com> wrote:
> > >
> > > On Thu, Sep 16, 2021 at 05:08:27PM +0100, Lorenzo Pieralisi wrote:
> > > > On Fri, Sep 10, 2021 at 07:28:49PM +0200, Ard Biesheuvel wrote:
> > > > > On Fri, 10 Sept 2021 at 16:32, Jia He <justin.he@arm.com> wrote:
> > > > > >
> > > > > > This reverts commit 437b38c51162f8b87beb28a833c4d5dc85fa864e.
> > > > > >
> > > > > > After this commit, a boot panic is alway hit on an Ampere EMAG server
> > > > > > with call trace as follows:
> > > > > >  Internal error: synchronous external abort: 96000410 [#1] SMP
> > > > > >  Modules linked in:
> > > > > >  CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.14.0+ #462
> > > > > >  Hardware name: MiTAC RAPTOR EV-883832-X3-0001/RAPTOR, BIOS 0.14 02/22/2019
> > > > > >  pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> > > > > > [...snip...]
> > > > > >  Call trace:
> > > > > >   acpi_ex_system_memory_space_handler+0x26c/0x2c8
> > > > > >   acpi_ev_address_space_dispatch+0x228/0x2c4
> > > > > >   acpi_ex_access_region+0x114/0x268
> > > > > >   acpi_ex_field_datum_io+0x128/0x1b8
> > > > > >   acpi_ex_extract_from_field+0x14c/0x2ac
> > > > > >   acpi_ex_read_data_from_field+0x190/0x1b8
> > > > > >   acpi_ex_resolve_node_to_value+0x1ec/0x288
> > > > > >   acpi_ex_resolve_to_value+0x250/0x274
> > > > > >   acpi_ds_evaluate_name_path+0xac/0x124
> > > > > >   acpi_ds_exec_end_op+0x90/0x410
> > > > > >   acpi_ps_parse_loop+0x4ac/0x5d8
> > > > > >   acpi_ps_parse_aml+0xe0/0x2c8
> > > > > >   acpi_ps_execute_method+0x19c/0x1ac
> > > > > >   acpi_ns_evaluate+0x1f8/0x26c
> > > > > >   acpi_ns_init_one_device+0x104/0x140
> > > > > >   acpi_ns_walk_namespace+0x158/0x1d0
> > > > > >   acpi_ns_initialize_devices+0x194/0x218
> > > > > >   acpi_initialize_objects+0x48/0x50
> > > > > >   acpi_init+0xe0/0x498
> > > > > >
> > > > > > As mentioned by Lorenzo:
> > > > > >   "We are forcing memory semantics mappings to PROT_NORMAL_NC, which
> > > > > >   eMAG does not like at all and I'd need to understand why. It looks
> > > > > >   like the issue happen in SystemMemory Opregion handler."
> > > > > >
> > > > > > Hence just revert it before everything is clear.
> > > > > >
> > > > >
> > > > > Can we try to find the root cause first? -rc1 is not even out yet, and
> > > > > reverting it now means we can not resubmit it until the next merge
> > > > > window.
> > > >
> > > > I am waiting to debug this on an eMAG but I noticed something that
> > > > I wanted to bring up.
> > > >
> > > > SystemMemory Operation region handler - ie
> > > >
> > > > acpi_ex_system_memory_space_handler()
> > > >
> > > > maps the Operation Region (that AFAICS is MMIO, it is _not_ memory)
> > > > with acpi_os_map_memory() and I believe that's what is causing this
> > > > bug.
> > > >
> > > > On the other hand, acpi_os_map_generic_address(), to handle spaceid
> > > > ACPI_ADR_SPACE_SYSTEM_MEMORY, uses acpi_os_map_iomem() that is more
> > > > in line with my expectations.
> > >
> > > Hi Rafael,
> > >
> > > I wanted to ask please if you have any insights on why
> > >
> > > (1) acpi_ex_system_memory_space_handler()
> > > (2) acpi_os_map_generic_address()
> > >
> > > Use two different calls to map memory for the _same_ address space ID
> > > (SystemMemory).
> > >
> > > (3) acpi_os_map_memory()
> > > vs
> > > (4) acpi_os_map_iomem()
> >
> > I don't really have a good answer here.
> >
> > On x86 this doesn't really matter and that's where
> > acpi_ex_system_memory_space_handler() was first introduced.  It is not
> > only used for IOMEM (there are SystemMemory operation regions in RAM),
> > but since it may be in IOMEM, it should assume so.
> >
> > > I am struggling to understand why (1) uses (3) ("memory semantics") when
> > > (2) uses (4) - it is actually unclear how the distinction between
> > > the two mapping APIs is to be drawn and on what basis one should
> > > choose which one to use.
> > >
> > > I am still waiting to grab some HW to debug this report but the issue
> > > here is that we are mapping an OpRegion SystemMemory with (3) in the
> > > memory space handler and given the patch we are reverting we end up
> > > mapping the operation region with normal non-cacheable memory attributes
> > > that probably the physical address range behind the OpRegion does not
> > > support.
> >
> > If that is the case, there needs to be a mechanism to decide what kind
> > of mapping to use for SystemMemory operation regions based on the type
> > of physical memory the address range in question is located in.
>
> Thank you Rafael. The mechanism we are currently relying on is the EFI
> memory map but if the Opregion address is not described there then we
> are left with a default choice to make (theoretically I may also parse
> all _CRS in the namespace to find whether a resource include the
> Opregion and I may infer attributes from the _CRS resource entry).
>

I'm not sure that would help, as I would expected the memory described
by _CRS to be mostly mutually exclusive from memory used by OpRegions.

> Maybe we should update the ACPI specs to enforce it; with current
> firmware the idea of using the OS expected *usage* of memory (ie
> memory vs IO) described by the mapping function prototype can't work
> as this revert shows (even though it would be better if I manage
> to find what the precise issue is).
>
> We can't map something with specific attributes if we don't know
> whether the physical address space backing the region supports it.
>

We don't have a a safe default in either direction, so I agree this is
a hole in the specs.

> I am left with little choice: I assume the best thing I could do
> to fix the original bug is to use ioremap_* in acpi_data_show()
> instead of acpi_os_map/unmap_memory() to map that memory with
> specific attributes (for BERT error regions, they must be RAM
> so, _hopefully_, we know it can be mapped with eg normal memory
> mappings).
>
> Thoughts ?
>

One thing I just realized is that the EFI memory map is not a complete
solution to begin with, as it may not cover hot/coldplugged memory
regions that are only described via ACPI.

Did you make any progress with the eMAG?

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2] Revert "ACPI: Add memory semantics to acpi_os_map_memory()"
  2021-09-22 11:11             ` Ard Biesheuvel
@ 2021-09-22 13:07               ` Lorenzo Pieralisi
  2021-09-22 23:45               ` Jeremy Linton
  1 sibling, 0 replies; 23+ messages in thread
From: Lorenzo Pieralisi @ 2021-09-22 13:07 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Rafael J. Wysocki, Rafael Wysocki, Rafael J. Wysocki, Jia He,
	Will Deacon, Len Brown, Robert Moore, Erik Kaneda, Linux ARM,
	Linux Kernel Mailing List, ACPI Devel Maling List,
	open list:ACPI COMPONENT ARCHITECTURE (ACPICA),
	Hanjun Guo, Catalin Marinas, Harb Abdulhamid

On Wed, Sep 22, 2021 at 01:11:26PM +0200, Ard Biesheuvel wrote:
> On Tue, 21 Sept 2021 at 12:05, Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
> >
> > On Mon, Sep 20, 2021 at 07:32:56PM +0200, Rafael J. Wysocki wrote:
> > > On Mon, Sep 20, 2021 at 7:03 PM Lorenzo Pieralisi
> > > <lorenzo.pieralisi@arm.com> wrote:
> > > >
> > > > On Thu, Sep 16, 2021 at 05:08:27PM +0100, Lorenzo Pieralisi wrote:
> > > > > On Fri, Sep 10, 2021 at 07:28:49PM +0200, Ard Biesheuvel wrote:
> > > > > > On Fri, 10 Sept 2021 at 16:32, Jia He <justin.he@arm.com> wrote:
> > > > > > >
> > > > > > > This reverts commit 437b38c51162f8b87beb28a833c4d5dc85fa864e.
> > > > > > >
> > > > > > > After this commit, a boot panic is alway hit on an Ampere EMAG server
> > > > > > > with call trace as follows:
> > > > > > >  Internal error: synchronous external abort: 96000410 [#1] SMP
> > > > > > >  Modules linked in:
> > > > > > >  CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.14.0+ #462
> > > > > > >  Hardware name: MiTAC RAPTOR EV-883832-X3-0001/RAPTOR, BIOS 0.14 02/22/2019
> > > > > > >  pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> > > > > > > [...snip...]
> > > > > > >  Call trace:
> > > > > > >   acpi_ex_system_memory_space_handler+0x26c/0x2c8
> > > > > > >   acpi_ev_address_space_dispatch+0x228/0x2c4
> > > > > > >   acpi_ex_access_region+0x114/0x268
> > > > > > >   acpi_ex_field_datum_io+0x128/0x1b8
> > > > > > >   acpi_ex_extract_from_field+0x14c/0x2ac
> > > > > > >   acpi_ex_read_data_from_field+0x190/0x1b8
> > > > > > >   acpi_ex_resolve_node_to_value+0x1ec/0x288
> > > > > > >   acpi_ex_resolve_to_value+0x250/0x274
> > > > > > >   acpi_ds_evaluate_name_path+0xac/0x124
> > > > > > >   acpi_ds_exec_end_op+0x90/0x410
> > > > > > >   acpi_ps_parse_loop+0x4ac/0x5d8
> > > > > > >   acpi_ps_parse_aml+0xe0/0x2c8
> > > > > > >   acpi_ps_execute_method+0x19c/0x1ac
> > > > > > >   acpi_ns_evaluate+0x1f8/0x26c
> > > > > > >   acpi_ns_init_one_device+0x104/0x140
> > > > > > >   acpi_ns_walk_namespace+0x158/0x1d0
> > > > > > >   acpi_ns_initialize_devices+0x194/0x218
> > > > > > >   acpi_initialize_objects+0x48/0x50
> > > > > > >   acpi_init+0xe0/0x498
> > > > > > >
> > > > > > > As mentioned by Lorenzo:
> > > > > > >   "We are forcing memory semantics mappings to PROT_NORMAL_NC, which
> > > > > > >   eMAG does not like at all and I'd need to understand why. It looks
> > > > > > >   like the issue happen in SystemMemory Opregion handler."
> > > > > > >
> > > > > > > Hence just revert it before everything is clear.
> > > > > > >
> > > > > >
> > > > > > Can we try to find the root cause first? -rc1 is not even out yet, and
> > > > > > reverting it now means we can not resubmit it until the next merge
> > > > > > window.
> > > > >
> > > > > I am waiting to debug this on an eMAG but I noticed something that
> > > > > I wanted to bring up.
> > > > >
> > > > > SystemMemory Operation region handler - ie
> > > > >
> > > > > acpi_ex_system_memory_space_handler()
> > > > >
> > > > > maps the Operation Region (that AFAICS is MMIO, it is _not_ memory)
> > > > > with acpi_os_map_memory() and I believe that's what is causing this
> > > > > bug.
> > > > >
> > > > > On the other hand, acpi_os_map_generic_address(), to handle spaceid
> > > > > ACPI_ADR_SPACE_SYSTEM_MEMORY, uses acpi_os_map_iomem() that is more
> > > > > in line with my expectations.
> > > >
> > > > Hi Rafael,
> > > >
> > > > I wanted to ask please if you have any insights on why
> > > >
> > > > (1) acpi_ex_system_memory_space_handler()
> > > > (2) acpi_os_map_generic_address()
> > > >
> > > > Use two different calls to map memory for the _same_ address space ID
> > > > (SystemMemory).
> > > >
> > > > (3) acpi_os_map_memory()
> > > > vs
> > > > (4) acpi_os_map_iomem()
> > >
> > > I don't really have a good answer here.
> > >
> > > On x86 this doesn't really matter and that's where
> > > acpi_ex_system_memory_space_handler() was first introduced.  It is not
> > > only used for IOMEM (there are SystemMemory operation regions in RAM),
> > > but since it may be in IOMEM, it should assume so.
> > >
> > > > I am struggling to understand why (1) uses (3) ("memory semantics") when
> > > > (2) uses (4) - it is actually unclear how the distinction between
> > > > the two mapping APIs is to be drawn and on what basis one should
> > > > choose which one to use.
> > > >
> > > > I am still waiting to grab some HW to debug this report but the issue
> > > > here is that we are mapping an OpRegion SystemMemory with (3) in the
> > > > memory space handler and given the patch we are reverting we end up
> > > > mapping the operation region with normal non-cacheable memory attributes
> > > > that probably the physical address range behind the OpRegion does not
> > > > support.
> > >
> > > If that is the case, there needs to be a mechanism to decide what kind
> > > of mapping to use for SystemMemory operation regions based on the type
> > > of physical memory the address range in question is located in.
> >
> > Thank you Rafael. The mechanism we are currently relying on is the EFI
> > memory map but if the Opregion address is not described there then we
> > are left with a default choice to make (theoretically I may also parse
> > all _CRS in the namespace to find whether a resource include the
> > Opregion and I may infer attributes from the _CRS resource entry).
> >
> 
> I'm not sure that would help, as I would expected the memory described
> by _CRS to be mostly mutually exclusive from memory used by OpRegions.
> 
> > Maybe we should update the ACPI specs to enforce it; with current
> > firmware the idea of using the OS expected *usage* of memory (ie
> > memory vs IO) described by the mapping function prototype can't work
> > as this revert shows (even though it would be better if I manage
> > to find what the precise issue is).
> >
> > We can't map something with specific attributes if we don't know
> > whether the physical address space backing the region supports it.
> >
> 
> We don't have a a safe default in either direction, so I agree this is
> a hole in the specs.
> 
> > I am left with little choice: I assume the best thing I could do
> > to fix the original bug is to use ioremap_* in acpi_data_show()
> > instead of acpi_os_map/unmap_memory() to map that memory with
> > specific attributes (for BERT error regions, they must be RAM
> > so, _hopefully_, we know it can be mapped with eg normal memory
> > mappings).
> >
> > Thoughts ?
> >
> 
> One thing I just realized is that the EFI memory map is not a complete
> solution to begin with, as it may not cover hot/coldplugged memory
> regions that are only described via ACPI.
> 
> Did you make any progress with the eMAG?

I manage to get the ACPI tables dump. The fault is triggered on
a SystemMemory OPregion access (FYI - should be a reset register),
probably (but on this only Ampere can help us) because the MMIO
range in question does not support the AXI attributes assigned
by the NormalNC mapping.

I believe mapping SystemMemory Opregions as NormalNC does not make
much sense anyway.

The UEFI specs seem to hint that the ACPI Op-region cacheability
attributes must be determined through the UEFI memory map, not
sure whether that means that the OpRegion itself _must_ be in
the EFI memory map.

I believe we need to go on with the revert and find a way to fix the
BERT error region mappings, to make them NormalNC so that we can do
unaligned accesses on them.

What to do specs side - to be debated, we have to do something because
it is impossible to handle it sensibly otherwise.

Lorenzo

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2] Revert "ACPI: Add memory semantics to acpi_os_map_memory()"
  2021-09-10 14:32 ` [PATCH v2] " Jia He
  2021-09-10 17:28   ` Ard Biesheuvel
@ 2021-09-22 16:33   ` Lorenzo Pieralisi
  2021-09-22 23:09     ` Mark Kettenis
  1 sibling, 1 reply; 23+ messages in thread
From: Lorenzo Pieralisi @ 2021-09-22 16:33 UTC (permalink / raw)
  To: Jia He, Harb Abdulhamid
  Cc: Will Deacon, Len Brown, Robert Moore, Erik Kaneda,
	linux-arm-kernel, linux-kernel, linux-acpi, devel,
	Ard Biesheuvel, Hanjun Guo, Catalin Marinas, Rafael J . Wysocki

On Fri, Sep 10, 2021 at 10:32:23PM +0800, Jia He wrote:
> This reverts commit 437b38c51162f8b87beb28a833c4d5dc85fa864e.
> 
> After this commit, a boot panic is alway hit on an Ampere EMAG server
> with call trace as follows:
>  Internal error: synchronous external abort: 96000410 [#1] SMP
>  Modules linked in:
>  CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.14.0+ #462
>  Hardware name: MiTAC RAPTOR EV-883832-X3-0001/RAPTOR, BIOS 0.14 02/22/2019
>  pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [...snip...]
>  Call trace:
>   acpi_ex_system_memory_space_handler+0x26c/0x2c8
>   acpi_ev_address_space_dispatch+0x228/0x2c4
>   acpi_ex_access_region+0x114/0x268
>   acpi_ex_field_datum_io+0x128/0x1b8
>   acpi_ex_extract_from_field+0x14c/0x2ac
>   acpi_ex_read_data_from_field+0x190/0x1b8
>   acpi_ex_resolve_node_to_value+0x1ec/0x288
>   acpi_ex_resolve_to_value+0x250/0x274
>   acpi_ds_evaluate_name_path+0xac/0x124
>   acpi_ds_exec_end_op+0x90/0x410
>   acpi_ps_parse_loop+0x4ac/0x5d8
>   acpi_ps_parse_aml+0xe0/0x2c8
>   acpi_ps_execute_method+0x19c/0x1ac
>   acpi_ns_evaluate+0x1f8/0x26c
>   acpi_ns_init_one_device+0x104/0x140
>   acpi_ns_walk_namespace+0x158/0x1d0
>   acpi_ns_initialize_devices+0x194/0x218
>   acpi_initialize_objects+0x48/0x50
>   acpi_init+0xe0/0x498
> 
> As mentioned by Lorenzo:
>   "We are forcing memory semantics mappings to PROT_NORMAL_NC, which
>   eMAG does not like at all and I'd need to understand why. It looks
>   like the issue happen in SystemMemory Opregion handler."
> 
> Hence just revert it before everything is clear.
> 
> Fixes: 437b38c51162 ("ACPI: Add memory semantics to acpi_os_map_memory()")
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Ard Biesheuvel <ardb@kernel.org>
> Cc: Hanjun Guo <guohanjun@huawei.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: Harb Abdulhamid <harb@amperecomputing.com>
> 
> Signed-off-by: Jia He <justin.he@arm.com>

Rewrote the commit log, please take the patch below and repost
it as a v3.

It would still be great if Ampere can help us understand why
the NormalNC attributes trigger a sync abort on the opregion
before merging it.

-- >8 --
Subject: [PATCH] Revert "ACPI: Add memory semantics to acpi_os_map_memory()"

This reverts commit 437b38c51162f8b87beb28a833c4d5dc85fa864e.

The memory semantics added in commit 437b38c51162 causes SystemMemory
Operation region, whose address range is not described in the EFI memory
map to be mapped as NormalNC memory on arm64 platforms (through
acpi_os_map_memory() in acpi_ex_system_memory_space_handler()).

This triggers the following abort on an ARM64 Ampere eMAG machine,
because presumably the physical address range area backing the Opregion
does not support NormalNC memory attributes driven on the bus.

 Internal error: synchronous external abort: 96000410 [#1] SMP
 Modules linked in:
 CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.14.0+ #462
 Hardware name: MiTAC RAPTOR EV-883832-X3-0001/RAPTOR, BIOS 0.14 02/22/2019
 pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[...snip...]
 Call trace:
  acpi_ex_system_memory_space_handler+0x26c/0x2c8
  acpi_ev_address_space_dispatch+0x228/0x2c4
  acpi_ex_access_region+0x114/0x268
  acpi_ex_field_datum_io+0x128/0x1b8
  acpi_ex_extract_from_field+0x14c/0x2ac
  acpi_ex_read_data_from_field+0x190/0x1b8
  acpi_ex_resolve_node_to_value+0x1ec/0x288
  acpi_ex_resolve_to_value+0x250/0x274
  acpi_ds_evaluate_name_path+0xac/0x124
  acpi_ds_exec_end_op+0x90/0x410
  acpi_ps_parse_loop+0x4ac/0x5d8
  acpi_ps_parse_aml+0xe0/0x2c8
  acpi_ps_execute_method+0x19c/0x1ac
  acpi_ns_evaluate+0x1f8/0x26c
  acpi_ns_init_one_device+0x104/0x140
  acpi_ns_walk_namespace+0x158/0x1d0
  acpi_ns_initialize_devices+0x194/0x218
  acpi_initialize_objects+0x48/0x50
  acpi_init+0xe0/0x498

If the Opregion address range is not present in the EFI memory map there
is no way for us to determine the memory attributes to use to map it -
defaulting to NormalNC does not work (and it is not correct on a memory
region that may have read side-effects) and therefore commit
437b38c51162 should be reverted, which means reverting back to the
original behavior whereby address ranges that are mapped using
acpi_os_map_memory() default to the safe devicenGnRnE attributes on
ARM64 if the mapped address range is not defined in the EFI memory map.

Fixes: 437b38c51162 ("ACPI: Add memory semantics to acpi_os_map_memory()")
Signed-off-by: Jia He <justin.he@arm.com>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Ard Biesheuvel <ardb@kernel.org>
Cc: Hanjun Guo <guohanjun@huawei.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: Harb Abdulhamid <harb@amperecomputing.com>
---
 arch/arm64/include/asm/acpi.h |  3 ---
 arch/arm64/kernel/acpi.c      | 19 +++----------------
 drivers/acpi/osl.c            | 23 +++++++----------------
 include/acpi/acpi_io.h        |  8 --------
 4 files changed, 10 insertions(+), 43 deletions(-)

diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
index 7535dc7cc5aa..bd68e1b7f29f 100644
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -50,9 +50,6 @@ pgprot_t __acpi_get_mem_attribute(phys_addr_t addr);
 void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size);
 #define acpi_os_ioremap acpi_os_ioremap
 
-void __iomem *acpi_os_memmap(acpi_physical_address phys, acpi_size size);
-#define acpi_os_memmap acpi_os_memmap
-
 typedef u64 phys_cpuid_t;
 #define PHYS_CPUID_INVALID INVALID_HWID
 
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
index 1c9c2f7a1c04..f3851724fe35 100644
--- a/arch/arm64/kernel/acpi.c
+++ b/arch/arm64/kernel/acpi.c
@@ -273,8 +273,7 @@ pgprot_t __acpi_get_mem_attribute(phys_addr_t addr)
 	return __pgprot(PROT_DEVICE_nGnRnE);
 }
 
-static void __iomem *__acpi_os_ioremap(acpi_physical_address phys,
-				       acpi_size size, bool memory)
+void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size)
 {
 	efi_memory_desc_t *md, *region = NULL;
 	pgprot_t prot;
@@ -300,11 +299,9 @@ static void __iomem *__acpi_os_ioremap(acpi_physical_address phys,
 	 * It is fine for AML to remap regions that are not represented in the
 	 * EFI memory map at all, as it only describes normal memory, and MMIO
 	 * regions that require a virtual mapping to make them accessible to
-	 * the EFI runtime services. Determine the region default
-	 * attributes by checking the requested memory semantics.
+	 * the EFI runtime services.
 	 */
-	prot = memory ? __pgprot(PROT_NORMAL_NC) :
-			__pgprot(PROT_DEVICE_nGnRnE);
+	prot = __pgprot(PROT_DEVICE_nGnRnE);
 	if (region) {
 		switch (region->type) {
 		case EFI_LOADER_CODE:
@@ -364,16 +361,6 @@ static void __iomem *__acpi_os_ioremap(acpi_physical_address phys,
 	return __ioremap(phys, size, prot);
 }
 
-void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size)
-{
-	return __acpi_os_ioremap(phys, size, false);
-}
-
-void __iomem *acpi_os_memmap(acpi_physical_address phys, acpi_size size)
-{
-	return __acpi_os_ioremap(phys, size, true);
-}
-
 /*
  * Claim Synchronous External Aborts as a firmware first notification.
  *
diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index a43f1521efe6..45c5c0e45e33 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -284,8 +284,7 @@ acpi_map_lookup_virt(void __iomem *virt, acpi_size size)
 #define should_use_kmap(pfn)   page_is_ram(pfn)
 #endif
 
-static void __iomem *acpi_map(acpi_physical_address pg_off, unsigned long pg_sz,
-			      bool memory)
+static void __iomem *acpi_map(acpi_physical_address pg_off, unsigned long pg_sz)
 {
 	unsigned long pfn;
 
@@ -295,8 +294,7 @@ static void __iomem *acpi_map(acpi_physical_address pg_off, unsigned long pg_sz,
 			return NULL;
 		return (void __iomem __force *)kmap(pfn_to_page(pfn));
 	} else
-		return memory ? acpi_os_memmap(pg_off, pg_sz) :
-				acpi_os_ioremap(pg_off, pg_sz);
+		return acpi_os_ioremap(pg_off, pg_sz);
 }
 
 static void acpi_unmap(acpi_physical_address pg_off, void __iomem *vaddr)
@@ -311,10 +309,9 @@ static void acpi_unmap(acpi_physical_address pg_off, void __iomem *vaddr)
 }
 
 /**
- * __acpi_os_map_iomem - Get a virtual address for a given physical address range.
+ * acpi_os_map_iomem - Get a virtual address for a given physical address range.
  * @phys: Start of the physical address range to map.
  * @size: Size of the physical address range to map.
- * @memory: true if remapping memory, false if IO
  *
  * Look up the given physical address range in the list of existing ACPI memory
  * mappings.  If found, get a reference to it and return a pointer to it (its
@@ -324,8 +321,8 @@ static void acpi_unmap(acpi_physical_address pg_off, void __iomem *vaddr)
  * During early init (when acpi_permanent_mmap has not been set yet) this
  * routine simply calls __acpi_map_table() to get the job done.
  */
-static void __iomem __ref
-*__acpi_os_map_iomem(acpi_physical_address phys, acpi_size size, bool memory)
+void __iomem __ref
+*acpi_os_map_iomem(acpi_physical_address phys, acpi_size size)
 {
 	struct acpi_ioremap *map;
 	void __iomem *virt;
@@ -356,7 +353,7 @@ static void __iomem __ref
 
 	pg_off = round_down(phys, PAGE_SIZE);
 	pg_sz = round_up(phys + size, PAGE_SIZE) - pg_off;
-	virt = acpi_map(phys, size, memory);
+	virt = acpi_map(phys, size);
 	if (!virt) {
 		mutex_unlock(&acpi_ioremap_lock);
 		kfree(map);
@@ -375,17 +372,11 @@ static void __iomem __ref
 	mutex_unlock(&acpi_ioremap_lock);
 	return map->virt + (phys - map->phys);
 }
-
-void __iomem *__ref
-acpi_os_map_iomem(acpi_physical_address phys, acpi_size size)
-{
-	return __acpi_os_map_iomem(phys, size, false);
-}
 EXPORT_SYMBOL_GPL(acpi_os_map_iomem);
 
 void *__ref acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
 {
-	return (void *)__acpi_os_map_iomem(phys, size, true);
+	return (void *)acpi_os_map_iomem(phys, size);
 }
 EXPORT_SYMBOL_GPL(acpi_os_map_memory);
 
diff --git a/include/acpi/acpi_io.h b/include/acpi/acpi_io.h
index a0212e67d6f4..027faa8883aa 100644
--- a/include/acpi/acpi_io.h
+++ b/include/acpi/acpi_io.h
@@ -14,14 +14,6 @@ static inline void __iomem *acpi_os_ioremap(acpi_physical_address phys,
 }
 #endif
 
-#ifndef acpi_os_memmap
-static inline void __iomem *acpi_os_memmap(acpi_physical_address phys,
-					    acpi_size size)
-{
-	return ioremap_cache(phys, size);
-}
-#endif
-
 extern bool acpi_permanent_mmap;
 
 void __iomem __ref
-- 
2.31.0

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [PATCH v2] Revert "ACPI: Add memory semantics to acpi_os_map_memory()"
  2021-09-22 16:33   ` Lorenzo Pieralisi
@ 2021-09-22 23:09     ` Mark Kettenis
  2021-09-23  9:40       ` Lorenzo Pieralisi
  0 siblings, 1 reply; 23+ messages in thread
From: Mark Kettenis @ 2021-09-22 23:09 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: justin.he, harb, will, lenb, robert.moore, erik.kaneda,
	linux-arm-kernel, linux-kernel, linux-acpi, devel, ardb,
	guohanjun, catalin.marinas, rafael.j.wysocki

> Date: Wed, 22 Sep 2021 17:33:36 +0100
> From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> 
> On Fri, Sep 10, 2021 at 10:32:23PM +0800, Jia He wrote:
> > This reverts commit 437b38c51162f8b87beb28a833c4d5dc85fa864e.
> > 
> > After this commit, a boot panic is alway hit on an Ampere EMAG server
> > with call trace as follows:
> >  Internal error: synchronous external abort: 96000410 [#1] SMP
> >  Modules linked in:
> >  CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.14.0+ #462
> >  Hardware name: MiTAC RAPTOR EV-883832-X3-0001/RAPTOR, BIOS 0.14 02/22/2019
> >  pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> > [...snip...]
> >  Call trace:
> >   acpi_ex_system_memory_space_handler+0x26c/0x2c8
> >   acpi_ev_address_space_dispatch+0x228/0x2c4
> >   acpi_ex_access_region+0x114/0x268
> >   acpi_ex_field_datum_io+0x128/0x1b8
> >   acpi_ex_extract_from_field+0x14c/0x2ac
> >   acpi_ex_read_data_from_field+0x190/0x1b8
> >   acpi_ex_resolve_node_to_value+0x1ec/0x288
> >   acpi_ex_resolve_to_value+0x250/0x274
> >   acpi_ds_evaluate_name_path+0xac/0x124
> >   acpi_ds_exec_end_op+0x90/0x410
> >   acpi_ps_parse_loop+0x4ac/0x5d8
> >   acpi_ps_parse_aml+0xe0/0x2c8
> >   acpi_ps_execute_method+0x19c/0x1ac
> >   acpi_ns_evaluate+0x1f8/0x26c
> >   acpi_ns_init_one_device+0x104/0x140
> >   acpi_ns_walk_namespace+0x158/0x1d0
> >   acpi_ns_initialize_devices+0x194/0x218
> >   acpi_initialize_objects+0x48/0x50
> >   acpi_init+0xe0/0x498
> > 
> > As mentioned by Lorenzo:
> >   "We are forcing memory semantics mappings to PROT_NORMAL_NC, which
> >   eMAG does not like at all and I'd need to understand why. It looks
> >   like the issue happen in SystemMemory Opregion handler."
> > 
> > Hence just revert it before everything is clear.
> > 
> > Fixes: 437b38c51162 ("ACPI: Add memory semantics to acpi_os_map_memory()")
> > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > Cc: Ard Biesheuvel <ardb@kernel.org>
> > Cc: Hanjun Guo <guohanjun@huawei.com>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > Cc: Harb Abdulhamid <harb@amperecomputing.com>
> > 
> > Signed-off-by: Jia He <justin.he@arm.com>
> 
> Rewrote the commit log, please take the patch below and repost
> it as a v3.
> 
> It would still be great if Ampere can help us understand why
> the NormalNC attributes trigger a sync abort on the opregion
> before merging it.

To be honest, I don't think you really need an explanation from Ampere
here.  Mapping a part of the address space that doesn't provide memory
semantics with NormalNC attributes is wrong and triggering a sync
abort in that case is way better than silently ignoring the access.

Putting my OpenBSD hat on (where we have our own ACPI OSPM
implementation) I must say that we always interpreted SystemMemory as
memory mapped IO and I think that is a logical choice as SystemIO is
used for (non-memory mapped) IO.  And I'd say that the ACPI OSPM code
should make sure that it uses properly aligned access to any Field
object that doesn't use AnyAcc as its access type.  Even on x86!  And
I'd say that AML that uses AnyAcc fields for SystemMemory OpRegions on
arm64 is buggy.

But maybe relaxing this when the EFI memory map indicates that the
address space in question does provide memory semantics does make
sense.  That should defenitely be documented in the ACPI standard
though.

> -- >8 --
> Subject: [PATCH] Revert "ACPI: Add memory semantics to acpi_os_map_memory()"
> 
> This reverts commit 437b38c51162f8b87beb28a833c4d5dc85fa864e.
> 
> The memory semantics added in commit 437b38c51162 causes SystemMemory
> Operation region, whose address range is not described in the EFI memory
> map to be mapped as NormalNC memory on arm64 platforms (through
> acpi_os_map_memory() in acpi_ex_system_memory_space_handler()).
> 
> This triggers the following abort on an ARM64 Ampere eMAG machine,
> because presumably the physical address range area backing the Opregion
> does not support NormalNC memory attributes driven on the bus.
> 
>  Internal error: synchronous external abort: 96000410 [#1] SMP
>  Modules linked in:
>  CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.14.0+ #462
>  Hardware name: MiTAC RAPTOR EV-883832-X3-0001/RAPTOR, BIOS 0.14 02/22/2019
>  pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [...snip...]
>  Call trace:
>   acpi_ex_system_memory_space_handler+0x26c/0x2c8
>   acpi_ev_address_space_dispatch+0x228/0x2c4
>   acpi_ex_access_region+0x114/0x268
>   acpi_ex_field_datum_io+0x128/0x1b8
>   acpi_ex_extract_from_field+0x14c/0x2ac
>   acpi_ex_read_data_from_field+0x190/0x1b8
>   acpi_ex_resolve_node_to_value+0x1ec/0x288
>   acpi_ex_resolve_to_value+0x250/0x274
>   acpi_ds_evaluate_name_path+0xac/0x124
>   acpi_ds_exec_end_op+0x90/0x410
>   acpi_ps_parse_loop+0x4ac/0x5d8
>   acpi_ps_parse_aml+0xe0/0x2c8
>   acpi_ps_execute_method+0x19c/0x1ac
>   acpi_ns_evaluate+0x1f8/0x26c
>   acpi_ns_init_one_device+0x104/0x140
>   acpi_ns_walk_namespace+0x158/0x1d0
>   acpi_ns_initialize_devices+0x194/0x218
>   acpi_initialize_objects+0x48/0x50
>   acpi_init+0xe0/0x498
> 
> If the Opregion address range is not present in the EFI memory map there
> is no way for us to determine the memory attributes to use to map it -
> defaulting to NormalNC does not work (and it is not correct on a memory
> region that may have read side-effects) and therefore commit
> 437b38c51162 should be reverted, which means reverting back to the
> original behavior whereby address ranges that are mapped using
> acpi_os_map_memory() default to the safe devicenGnRnE attributes on
> ARM64 if the mapped address range is not defined in the EFI memory map.
> 
> Fixes: 437b38c51162 ("ACPI: Add memory semantics to acpi_os_map_memory()")
> Signed-off-by: Jia He <justin.he@arm.com>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Ard Biesheuvel <ardb@kernel.org>
> Cc: Hanjun Guo <guohanjun@huawei.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: Harb Abdulhamid <harb@amperecomputing.com>
> ---
>  arch/arm64/include/asm/acpi.h |  3 ---
>  arch/arm64/kernel/acpi.c      | 19 +++----------------
>  drivers/acpi/osl.c            | 23 +++++++----------------
>  include/acpi/acpi_io.h        |  8 --------
>  4 files changed, 10 insertions(+), 43 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
> index 7535dc7cc5aa..bd68e1b7f29f 100644
> --- a/arch/arm64/include/asm/acpi.h
> +++ b/arch/arm64/include/asm/acpi.h
> @@ -50,9 +50,6 @@ pgprot_t __acpi_get_mem_attribute(phys_addr_t addr);
>  void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size);
>  #define acpi_os_ioremap acpi_os_ioremap
>  
> -void __iomem *acpi_os_memmap(acpi_physical_address phys, acpi_size size);
> -#define acpi_os_memmap acpi_os_memmap
> -
>  typedef u64 phys_cpuid_t;
>  #define PHYS_CPUID_INVALID INVALID_HWID
>  
> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
> index 1c9c2f7a1c04..f3851724fe35 100644
> --- a/arch/arm64/kernel/acpi.c
> +++ b/arch/arm64/kernel/acpi.c
> @@ -273,8 +273,7 @@ pgprot_t __acpi_get_mem_attribute(phys_addr_t addr)
>  	return __pgprot(PROT_DEVICE_nGnRnE);
>  }
>  
> -static void __iomem *__acpi_os_ioremap(acpi_physical_address phys,
> -				       acpi_size size, bool memory)
> +void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size)
>  {
>  	efi_memory_desc_t *md, *region = NULL;
>  	pgprot_t prot;
> @@ -300,11 +299,9 @@ static void __iomem *__acpi_os_ioremap(acpi_physical_address phys,
>  	 * It is fine for AML to remap regions that are not represented in the
>  	 * EFI memory map at all, as it only describes normal memory, and MMIO
>  	 * regions that require a virtual mapping to make them accessible to
> -	 * the EFI runtime services. Determine the region default
> -	 * attributes by checking the requested memory semantics.
> +	 * the EFI runtime services.
>  	 */
> -	prot = memory ? __pgprot(PROT_NORMAL_NC) :
> -			__pgprot(PROT_DEVICE_nGnRnE);
> +	prot = __pgprot(PROT_DEVICE_nGnRnE);
>  	if (region) {
>  		switch (region->type) {
>  		case EFI_LOADER_CODE:
> @@ -364,16 +361,6 @@ static void __iomem *__acpi_os_ioremap(acpi_physical_address phys,
>  	return __ioremap(phys, size, prot);
>  }
>  
> -void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size)
> -{
> -	return __acpi_os_ioremap(phys, size, false);
> -}
> -
> -void __iomem *acpi_os_memmap(acpi_physical_address phys, acpi_size size)
> -{
> -	return __acpi_os_ioremap(phys, size, true);
> -}
> -
>  /*
>   * Claim Synchronous External Aborts as a firmware first notification.
>   *
> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> index a43f1521efe6..45c5c0e45e33 100644
> --- a/drivers/acpi/osl.c
> +++ b/drivers/acpi/osl.c
> @@ -284,8 +284,7 @@ acpi_map_lookup_virt(void __iomem *virt, acpi_size size)
>  #define should_use_kmap(pfn)   page_is_ram(pfn)
>  #endif
>  
> -static void __iomem *acpi_map(acpi_physical_address pg_off, unsigned long pg_sz,
> -			      bool memory)
> +static void __iomem *acpi_map(acpi_physical_address pg_off, unsigned long pg_sz)
>  {
>  	unsigned long pfn;
>  
> @@ -295,8 +294,7 @@ static void __iomem *acpi_map(acpi_physical_address pg_off, unsigned long pg_sz,
>  			return NULL;
>  		return (void __iomem __force *)kmap(pfn_to_page(pfn));
>  	} else
> -		return memory ? acpi_os_memmap(pg_off, pg_sz) :
> -				acpi_os_ioremap(pg_off, pg_sz);
> +		return acpi_os_ioremap(pg_off, pg_sz);
>  }
>  
>  static void acpi_unmap(acpi_physical_address pg_off, void __iomem *vaddr)
> @@ -311,10 +309,9 @@ static void acpi_unmap(acpi_physical_address pg_off, void __iomem *vaddr)
>  }
>  
>  /**
> - * __acpi_os_map_iomem - Get a virtual address for a given physical address range.
> + * acpi_os_map_iomem - Get a virtual address for a given physical address range.
>   * @phys: Start of the physical address range to map.
>   * @size: Size of the physical address range to map.
> - * @memory: true if remapping memory, false if IO
>   *
>   * Look up the given physical address range in the list of existing ACPI memory
>   * mappings.  If found, get a reference to it and return a pointer to it (its
> @@ -324,8 +321,8 @@ static void acpi_unmap(acpi_physical_address pg_off, void __iomem *vaddr)
>   * During early init (when acpi_permanent_mmap has not been set yet) this
>   * routine simply calls __acpi_map_table() to get the job done.
>   */
> -static void __iomem __ref
> -*__acpi_os_map_iomem(acpi_physical_address phys, acpi_size size, bool memory)
> +void __iomem __ref
> +*acpi_os_map_iomem(acpi_physical_address phys, acpi_size size)
>  {
>  	struct acpi_ioremap *map;
>  	void __iomem *virt;
> @@ -356,7 +353,7 @@ static void __iomem __ref
>  
>  	pg_off = round_down(phys, PAGE_SIZE);
>  	pg_sz = round_up(phys + size, PAGE_SIZE) - pg_off;
> -	virt = acpi_map(phys, size, memory);
> +	virt = acpi_map(phys, size);
>  	if (!virt) {
>  		mutex_unlock(&acpi_ioremap_lock);
>  		kfree(map);
> @@ -375,17 +372,11 @@ static void __iomem __ref
>  	mutex_unlock(&acpi_ioremap_lock);
>  	return map->virt + (phys - map->phys);
>  }
> -
> -void __iomem *__ref
> -acpi_os_map_iomem(acpi_physical_address phys, acpi_size size)
> -{
> -	return __acpi_os_map_iomem(phys, size, false);
> -}
>  EXPORT_SYMBOL_GPL(acpi_os_map_iomem);
>  
>  void *__ref acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
>  {
> -	return (void *)__acpi_os_map_iomem(phys, size, true);
> +	return (void *)acpi_os_map_iomem(phys, size);
>  }
>  EXPORT_SYMBOL_GPL(acpi_os_map_memory);
>  
> diff --git a/include/acpi/acpi_io.h b/include/acpi/acpi_io.h
> index a0212e67d6f4..027faa8883aa 100644
> --- a/include/acpi/acpi_io.h
> +++ b/include/acpi/acpi_io.h
> @@ -14,14 +14,6 @@ static inline void __iomem *acpi_os_ioremap(acpi_physical_address phys,
>  }
>  #endif
>  
> -#ifndef acpi_os_memmap
> -static inline void __iomem *acpi_os_memmap(acpi_physical_address phys,
> -					    acpi_size size)
> -{
> -	return ioremap_cache(phys, size);
> -}
> -#endif
> -
>  extern bool acpi_permanent_mmap;
>  
>  void __iomem __ref
> -- 
> 2.31.0
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2] Revert "ACPI: Add memory semantics to acpi_os_map_memory()"
  2021-09-22 11:11             ` Ard Biesheuvel
  2021-09-22 13:07               ` Lorenzo Pieralisi
@ 2021-09-22 23:45               ` Jeremy Linton
  1 sibling, 0 replies; 23+ messages in thread
From: Jeremy Linton @ 2021-09-22 23:45 UTC (permalink / raw)
  To: Ard Biesheuvel, Lorenzo Pieralisi
  Cc: Rafael J. Wysocki, Rafael Wysocki, Rafael J. Wysocki, Jia He,
	Will Deacon, Len Brown, Robert Moore, Erik Kaneda, Linux ARM,
	Linux Kernel Mailing List, ACPI Devel Maling List,
	open list:ACPI COMPONENT ARCHITECTURE (ACPICA),
	Hanjun Guo, Catalin Marinas, Harb Abdulhamid

Hi,

On 9/22/21 6:11 AM, Ard Biesheuvel wrote:
> On Tue, 21 Sept 2021 at 12:05, Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
>>
>> On Mon, Sep 20, 2021 at 07:32:56PM +0200, Rafael J. Wysocki wrote:
>>> On Mon, Sep 20, 2021 at 7:03 PM Lorenzo Pieralisi
>>> <lorenzo.pieralisi@arm.com> wrote:
>>>>
>>>> On Thu, Sep 16, 2021 at 05:08:27PM +0100, Lorenzo Pieralisi wrote:
>>>>> On Fri, Sep 10, 2021 at 07:28:49PM +0200, Ard Biesheuvel wrote:
>>>>>> On Fri, 10 Sept 2021 at 16:32, Jia He <justin.he@arm.com> wrote:
>>>>>>>
>>>>>>> This reverts commit 437b38c51162f8b87beb28a833c4d5dc85fa864e.
>>>>>>>
>>>>>>> After this commit, a boot panic is alway hit on an Ampere EMAG server
>>>>>>> with call trace as follows:
>>>>>>>   Internal error: synchronous external abort: 96000410 [#1] SMP
>>>>>>>   Modules linked in:
>>>>>>>   CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.14.0+ #462
>>>>>>>   Hardware name: MiTAC RAPTOR EV-883832-X3-0001/RAPTOR, BIOS 0.14 02/22/2019
>>>>>>>   pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>>>>>>> [...snip...]
>>>>>>>   Call trace:
>>>>>>>    acpi_ex_system_memory_space_handler+0x26c/0x2c8
>>>>>>>    acpi_ev_address_space_dispatch+0x228/0x2c4
>>>>>>>    acpi_ex_access_region+0x114/0x268
>>>>>>>    acpi_ex_field_datum_io+0x128/0x1b8
>>>>>>>    acpi_ex_extract_from_field+0x14c/0x2ac
>>>>>>>    acpi_ex_read_data_from_field+0x190/0x1b8
>>>>>>>    acpi_ex_resolve_node_to_value+0x1ec/0x288
>>>>>>>    acpi_ex_resolve_to_value+0x250/0x274
>>>>>>>    acpi_ds_evaluate_name_path+0xac/0x124
>>>>>>>    acpi_ds_exec_end_op+0x90/0x410
>>>>>>>    acpi_ps_parse_loop+0x4ac/0x5d8
>>>>>>>    acpi_ps_parse_aml+0xe0/0x2c8
>>>>>>>    acpi_ps_execute_method+0x19c/0x1ac
>>>>>>>    acpi_ns_evaluate+0x1f8/0x26c
>>>>>>>    acpi_ns_init_one_device+0x104/0x140
>>>>>>>    acpi_ns_walk_namespace+0x158/0x1d0
>>>>>>>    acpi_ns_initialize_devices+0x194/0x218
>>>>>>>    acpi_initialize_objects+0x48/0x50
>>>>>>>    acpi_init+0xe0/0x498
>>>>>>>
>>>>>>> As mentioned by Lorenzo:
>>>>>>>    "We are forcing memory semantics mappings to PROT_NORMAL_NC, which
>>>>>>>    eMAG does not like at all and I'd need to understand why. It looks
>>>>>>>    like the issue happen in SystemMemory Opregion handler."
>>>>>>>
>>>>>>> Hence just revert it before everything is clear.
>>>>>>>
>>>>>>
>>>>>> Can we try to find the root cause first? -rc1 is not even out yet, and
>>>>>> reverting it now means we can not resubmit it until the next merge
>>>>>> window.
>>>>>
>>>>> I am waiting to debug this on an eMAG but I noticed something that
>>>>> I wanted to bring up.
>>>>>
>>>>> SystemMemory Operation region handler - ie
>>>>>
>>>>> acpi_ex_system_memory_space_handler()
>>>>>
>>>>> maps the Operation Region (that AFAICS is MMIO, it is _not_ memory)
>>>>> with acpi_os_map_memory() and I believe that's what is causing this
>>>>> bug.
>>>>>
>>>>> On the other hand, acpi_os_map_generic_address(), to handle spaceid
>>>>> ACPI_ADR_SPACE_SYSTEM_MEMORY, uses acpi_os_map_iomem() that is more
>>>>> in line with my expectations.
>>>>
>>>> Hi Rafael,
>>>>
>>>> I wanted to ask please if you have any insights on why
>>>>
>>>> (1) acpi_ex_system_memory_space_handler()
>>>> (2) acpi_os_map_generic_address()
>>>>
>>>> Use two different calls to map memory for the _same_ address space ID
>>>> (SystemMemory).
>>>>
>>>> (3) acpi_os_map_memory()
>>>> vs
>>>> (4) acpi_os_map_iomem()
>>>
>>> I don't really have a good answer here.
>>>
>>> On x86 this doesn't really matter and that's where
>>> acpi_ex_system_memory_space_handler() was first introduced.  It is not
>>> only used for IOMEM (there are SystemMemory operation regions in RAM),
>>> but since it may be in IOMEM, it should assume so.
>>>
>>>> I am struggling to understand why (1) uses (3) ("memory semantics") when
>>>> (2) uses (4) - it is actually unclear how the distinction between
>>>> the two mapping APIs is to be drawn and on what basis one should
>>>> choose which one to use.
>>>>
>>>> I am still waiting to grab some HW to debug this report but the issue
>>>> here is that we are mapping an OpRegion SystemMemory with (3) in the
>>>> memory space handler and given the patch we are reverting we end up
>>>> mapping the operation region with normal non-cacheable memory attributes
>>>> that probably the physical address range behind the OpRegion does not
>>>> support.
>>>
>>> If that is the case, there needs to be a mechanism to decide what kind
>>> of mapping to use for SystemMemory operation regions based on the type
>>> of physical memory the address range in question is located in.
>>
>> Thank you Rafael. The mechanism we are currently relying on is the EFI
>> memory map but if the Opregion address is not described there then we
>> are left with a default choice to make (theoretically I may also parse
>> all _CRS in the namespace to find whether a resource include the
>> Opregion and I may infer attributes from the _CRS resource entry).
>>
> 
> I'm not sure that would help, as I would expected the memory described
> by _CRS to be mostly mutually exclusive from memory used by OpRegions.
> 
>> Maybe we should update the ACPI specs to enforce it; with current
>> firmware the idea of using the OS expected *usage* of memory (ie
>> memory vs IO) described by the mapping function prototype can't work
>> as this revert shows (even though it would be better if I manage
>> to find what the precise issue is).
>>
>> We can't map something with specific attributes if we don't know
>> whether the physical address space backing the region supports it.
>>
> 
> We don't have a a safe default in either direction, so I agree this is
> a hole in the specs.

I just debugged down to this patch because of boot failures with the 
rpi4. Then I found this thread.


I had always assumed that SystemIO was x86/PIO, and SystemMemory was 
defined as MMIO but on arm we would have to determine if the memory 
region was described in the uefi memory map as actual system ram, and if 
not assume device memory. I was looking at tweaking acpi_map() to check 
that similar to what is happening on !arm64.

Gotta run, but throwing that out there since it seems a possible fix 
without creating a new OpregionType to differentiate actual memory vs MMIO.




> 
>> I am left with little choice: I assume the best thing I could do
>> to fix the original bug is to use ioremap_* in acpi_data_show()
>> instead of acpi_os_map/unmap_memory() to map that memory with
>> specific attributes (for BERT error regions, they must be RAM
>> so, _hopefully_, we know it can be mapped with eg normal memory
>> mappings).
>>
>> Thoughts ?
>>
> 
> One thing I just realized is that the EFI memory map is not a complete
> solution to begin with, as it may not cover hot/coldplugged memory
> regions that are only described via ACPI.
> 
> Did you make any progress with the eMAG?
> 


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2] Revert "ACPI: Add memory semantics to acpi_os_map_memory()"
  2021-09-22 23:09     ` Mark Kettenis
@ 2021-09-23  9:40       ` Lorenzo Pieralisi
  2021-09-23 11:05         ` Rafael J. Wysocki
  0 siblings, 1 reply; 23+ messages in thread
From: Lorenzo Pieralisi @ 2021-09-23  9:40 UTC (permalink / raw)
  To: Mark Kettenis
  Cc: justin.he, harb, will, lenb, robert.moore, erik.kaneda,
	linux-arm-kernel, linux-kernel, linux-acpi, devel, ardb,
	guohanjun, catalin.marinas, rafael.j.wysocki

On Thu, Sep 23, 2021 at 01:09:58AM +0200, Mark Kettenis wrote:
> > Date: Wed, 22 Sep 2021 17:33:36 +0100
> > From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > 
> > On Fri, Sep 10, 2021 at 10:32:23PM +0800, Jia He wrote:
> > > This reverts commit 437b38c51162f8b87beb28a833c4d5dc85fa864e.
> > > 
> > > After this commit, a boot panic is alway hit on an Ampere EMAG server
> > > with call trace as follows:
> > >  Internal error: synchronous external abort: 96000410 [#1] SMP
> > >  Modules linked in:
> > >  CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.14.0+ #462
> > >  Hardware name: MiTAC RAPTOR EV-883832-X3-0001/RAPTOR, BIOS 0.14 02/22/2019
> > >  pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> > > [...snip...]
> > >  Call trace:
> > >   acpi_ex_system_memory_space_handler+0x26c/0x2c8
> > >   acpi_ev_address_space_dispatch+0x228/0x2c4
> > >   acpi_ex_access_region+0x114/0x268
> > >   acpi_ex_field_datum_io+0x128/0x1b8
> > >   acpi_ex_extract_from_field+0x14c/0x2ac
> > >   acpi_ex_read_data_from_field+0x190/0x1b8
> > >   acpi_ex_resolve_node_to_value+0x1ec/0x288
> > >   acpi_ex_resolve_to_value+0x250/0x274
> > >   acpi_ds_evaluate_name_path+0xac/0x124
> > >   acpi_ds_exec_end_op+0x90/0x410
> > >   acpi_ps_parse_loop+0x4ac/0x5d8
> > >   acpi_ps_parse_aml+0xe0/0x2c8
> > >   acpi_ps_execute_method+0x19c/0x1ac
> > >   acpi_ns_evaluate+0x1f8/0x26c
> > >   acpi_ns_init_one_device+0x104/0x140
> > >   acpi_ns_walk_namespace+0x158/0x1d0
> > >   acpi_ns_initialize_devices+0x194/0x218
> > >   acpi_initialize_objects+0x48/0x50
> > >   acpi_init+0xe0/0x498
> > > 
> > > As mentioned by Lorenzo:
> > >   "We are forcing memory semantics mappings to PROT_NORMAL_NC, which
> > >   eMAG does not like at all and I'd need to understand why. It looks
> > >   like the issue happen in SystemMemory Opregion handler."
> > > 
> > > Hence just revert it before everything is clear.
> > > 
> > > Fixes: 437b38c51162 ("ACPI: Add memory semantics to acpi_os_map_memory()")
> > > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > > Cc: Ard Biesheuvel <ardb@kernel.org>
> > > Cc: Hanjun Guo <guohanjun@huawei.com>
> > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > Cc: Harb Abdulhamid <harb@amperecomputing.com>
> > > 
> > > Signed-off-by: Jia He <justin.he@arm.com>
> > 
> > Rewrote the commit log, please take the patch below and repost
> > it as a v3.
> > 
> > It would still be great if Ampere can help us understand why
> > the NormalNC attributes trigger a sync abort on the opregion
> > before merging it.
> 
> To be honest, I don't think you really need an explanation from Ampere
> here.  Mapping a part of the address space that doesn't provide memory
> semantics with NormalNC attributes is wrong and triggering a sync
> abort in that case is way better than silently ignoring the access.

That's understood and that's what I explained in the revert commit
log, no question about it.

I was just asking to confirm if that's what's actually happening.

> Putting my OpenBSD hat on (where we have our own ACPI OSPM
> implementation) I must say that we always interpreted SystemMemory as
> memory mapped IO and I think that is a logical choice as SystemIO is
> used for (non-memory mapped) IO.  And I'd say that the ACPI OSPM code
> should make sure that it uses properly aligned access to any Field
> object that doesn't use AnyAcc as its access type.  Even on x86!  And
> I'd say that AML that uses AnyAcc fields for SystemMemory OpRegions on
> arm64 is buggy.
> 
> But maybe relaxing this when the EFI memory map indicates that the
> address space in question does provide memory semantics does make
> sense.  That should defenitely be documented in the ACPI standard
> though.

Mapping SystemMemory Opregions as "memory" does not make sense
at all to me. Still, that's what Linux ACPICA code does (*if*
that's what acpi_os_map_memory() is supposed to mean).

https://lore.kernel.org/linux-acpi/20210916160827.GA4525@lpieralisi

Where do we go from here, to be defined, we still have a bug
to fix after the revert is applied.

drivers/acpi/sysfs.c

maps BERT error regions with acpi_os_map_memory(). If the BERT error
region is not in the EFI memory map, we map that memory as device-nGnRnE
and we execute memory semantics operation on it.

https://lore.kernel.org/linux-acpi/e548e72c-83a4-2366-dd57-3e746040fea9@arm.com

I could change that code to map those regions as ioremap_wc() because
supposedly we *know* that's memory but this is becoming a slippery
slope to follow IMO.

> > -- >8 --
> > Subject: [PATCH] Revert "ACPI: Add memory semantics to acpi_os_map_memory()"
> > 
> > This reverts commit 437b38c51162f8b87beb28a833c4d5dc85fa864e.
> > 
> > The memory semantics added in commit 437b38c51162 causes SystemMemory
> > Operation region, whose address range is not described in the EFI memory
> > map to be mapped as NormalNC memory on arm64 platforms (through
> > acpi_os_map_memory() in acpi_ex_system_memory_space_handler()).
> > 
> > This triggers the following abort on an ARM64 Ampere eMAG machine,
> > because presumably the physical address range area backing the Opregion
> > does not support NormalNC memory attributes driven on the bus.
> > 
> >  Internal error: synchronous external abort: 96000410 [#1] SMP
> >  Modules linked in:
> >  CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.14.0+ #462
> >  Hardware name: MiTAC RAPTOR EV-883832-X3-0001/RAPTOR, BIOS 0.14 02/22/2019
> >  pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> > [...snip...]
> >  Call trace:
> >   acpi_ex_system_memory_space_handler+0x26c/0x2c8
> >   acpi_ev_address_space_dispatch+0x228/0x2c4
> >   acpi_ex_access_region+0x114/0x268
> >   acpi_ex_field_datum_io+0x128/0x1b8
> >   acpi_ex_extract_from_field+0x14c/0x2ac
> >   acpi_ex_read_data_from_field+0x190/0x1b8
> >   acpi_ex_resolve_node_to_value+0x1ec/0x288
> >   acpi_ex_resolve_to_value+0x250/0x274
> >   acpi_ds_evaluate_name_path+0xac/0x124
> >   acpi_ds_exec_end_op+0x90/0x410
> >   acpi_ps_parse_loop+0x4ac/0x5d8
> >   acpi_ps_parse_aml+0xe0/0x2c8
> >   acpi_ps_execute_method+0x19c/0x1ac
> >   acpi_ns_evaluate+0x1f8/0x26c
> >   acpi_ns_init_one_device+0x104/0x140
> >   acpi_ns_walk_namespace+0x158/0x1d0
> >   acpi_ns_initialize_devices+0x194/0x218
> >   acpi_initialize_objects+0x48/0x50
> >   acpi_init+0xe0/0x498
> > 
> > If the Opregion address range is not present in the EFI memory map there
> > is no way for us to determine the memory attributes to use to map it -
> > defaulting to NormalNC does not work (and it is not correct on a memory
> > region that may have read side-effects) and therefore commit
> > 437b38c51162 should be reverted, which means reverting back to the
> > original behavior whereby address ranges that are mapped using
> > acpi_os_map_memory() default to the safe devicenGnRnE attributes on
> > ARM64 if the mapped address range is not defined in the EFI memory map.
> > 
> > Fixes: 437b38c51162 ("ACPI: Add memory semantics to acpi_os_map_memory()")
> > Signed-off-by: Jia He <justin.he@arm.com>
> > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > Cc: Ard Biesheuvel <ardb@kernel.org>
> > Cc: Hanjun Guo <guohanjun@huawei.com>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > Cc: Harb Abdulhamid <harb@amperecomputing.com>
> > ---
> >  arch/arm64/include/asm/acpi.h |  3 ---
> >  arch/arm64/kernel/acpi.c      | 19 +++----------------
> >  drivers/acpi/osl.c            | 23 +++++++----------------
> >  include/acpi/acpi_io.h        |  8 --------
> >  4 files changed, 10 insertions(+), 43 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
> > index 7535dc7cc5aa..bd68e1b7f29f 100644
> > --- a/arch/arm64/include/asm/acpi.h
> > +++ b/arch/arm64/include/asm/acpi.h
> > @@ -50,9 +50,6 @@ pgprot_t __acpi_get_mem_attribute(phys_addr_t addr);
> >  void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size);
> >  #define acpi_os_ioremap acpi_os_ioremap
> >  
> > -void __iomem *acpi_os_memmap(acpi_physical_address phys, acpi_size size);
> > -#define acpi_os_memmap acpi_os_memmap
> > -
> >  typedef u64 phys_cpuid_t;
> >  #define PHYS_CPUID_INVALID INVALID_HWID
> >  
> > diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
> > index 1c9c2f7a1c04..f3851724fe35 100644
> > --- a/arch/arm64/kernel/acpi.c
> > +++ b/arch/arm64/kernel/acpi.c
> > @@ -273,8 +273,7 @@ pgprot_t __acpi_get_mem_attribute(phys_addr_t addr)
> >  	return __pgprot(PROT_DEVICE_nGnRnE);
> >  }
> >  
> > -static void __iomem *__acpi_os_ioremap(acpi_physical_address phys,
> > -				       acpi_size size, bool memory)
> > +void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size)
> >  {
> >  	efi_memory_desc_t *md, *region = NULL;
> >  	pgprot_t prot;
> > @@ -300,11 +299,9 @@ static void __iomem *__acpi_os_ioremap(acpi_physical_address phys,
> >  	 * It is fine for AML to remap regions that are not represented in the
> >  	 * EFI memory map at all, as it only describes normal memory, and MMIO
> >  	 * regions that require a virtual mapping to make them accessible to
> > -	 * the EFI runtime services. Determine the region default
> > -	 * attributes by checking the requested memory semantics.
> > +	 * the EFI runtime services.
> >  	 */
> > -	prot = memory ? __pgprot(PROT_NORMAL_NC) :
> > -			__pgprot(PROT_DEVICE_nGnRnE);
> > +	prot = __pgprot(PROT_DEVICE_nGnRnE);
> >  	if (region) {
> >  		switch (region->type) {
> >  		case EFI_LOADER_CODE:
> > @@ -364,16 +361,6 @@ static void __iomem *__acpi_os_ioremap(acpi_physical_address phys,
> >  	return __ioremap(phys, size, prot);
> >  }
> >  
> > -void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size)
> > -{
> > -	return __acpi_os_ioremap(phys, size, false);
> > -}
> > -
> > -void __iomem *acpi_os_memmap(acpi_physical_address phys, acpi_size size)
> > -{
> > -	return __acpi_os_ioremap(phys, size, true);
> > -}
> > -
> >  /*
> >   * Claim Synchronous External Aborts as a firmware first notification.
> >   *
> > diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> > index a43f1521efe6..45c5c0e45e33 100644
> > --- a/drivers/acpi/osl.c
> > +++ b/drivers/acpi/osl.c
> > @@ -284,8 +284,7 @@ acpi_map_lookup_virt(void __iomem *virt, acpi_size size)
> >  #define should_use_kmap(pfn)   page_is_ram(pfn)
> >  #endif
> >  
> > -static void __iomem *acpi_map(acpi_physical_address pg_off, unsigned long pg_sz,
> > -			      bool memory)
> > +static void __iomem *acpi_map(acpi_physical_address pg_off, unsigned long pg_sz)
> >  {
> >  	unsigned long pfn;
> >  
> > @@ -295,8 +294,7 @@ static void __iomem *acpi_map(acpi_physical_address pg_off, unsigned long pg_sz,
> >  			return NULL;
> >  		return (void __iomem __force *)kmap(pfn_to_page(pfn));
> >  	} else
> > -		return memory ? acpi_os_memmap(pg_off, pg_sz) :
> > -				acpi_os_ioremap(pg_off, pg_sz);
> > +		return acpi_os_ioremap(pg_off, pg_sz);
> >  }
> >  
> >  static void acpi_unmap(acpi_physical_address pg_off, void __iomem *vaddr)
> > @@ -311,10 +309,9 @@ static void acpi_unmap(acpi_physical_address pg_off, void __iomem *vaddr)
> >  }
> >  
> >  /**
> > - * __acpi_os_map_iomem - Get a virtual address for a given physical address range.
> > + * acpi_os_map_iomem - Get a virtual address for a given physical address range.
> >   * @phys: Start of the physical address range to map.
> >   * @size: Size of the physical address range to map.
> > - * @memory: true if remapping memory, false if IO
> >   *
> >   * Look up the given physical address range in the list of existing ACPI memory
> >   * mappings.  If found, get a reference to it and return a pointer to it (its
> > @@ -324,8 +321,8 @@ static void acpi_unmap(acpi_physical_address pg_off, void __iomem *vaddr)
> >   * During early init (when acpi_permanent_mmap has not been set yet) this
> >   * routine simply calls __acpi_map_table() to get the job done.
> >   */
> > -static void __iomem __ref
> > -*__acpi_os_map_iomem(acpi_physical_address phys, acpi_size size, bool memory)
> > +void __iomem __ref
> > +*acpi_os_map_iomem(acpi_physical_address phys, acpi_size size)
> >  {
> >  	struct acpi_ioremap *map;
> >  	void __iomem *virt;
> > @@ -356,7 +353,7 @@ static void __iomem __ref
> >  
> >  	pg_off = round_down(phys, PAGE_SIZE);
> >  	pg_sz = round_up(phys + size, PAGE_SIZE) - pg_off;
> > -	virt = acpi_map(phys, size, memory);
> > +	virt = acpi_map(phys, size);
> >  	if (!virt) {
> >  		mutex_unlock(&acpi_ioremap_lock);
> >  		kfree(map);
> > @@ -375,17 +372,11 @@ static void __iomem __ref
> >  	mutex_unlock(&acpi_ioremap_lock);
> >  	return map->virt + (phys - map->phys);
> >  }
> > -
> > -void __iomem *__ref
> > -acpi_os_map_iomem(acpi_physical_address phys, acpi_size size)
> > -{
> > -	return __acpi_os_map_iomem(phys, size, false);
> > -}
> >  EXPORT_SYMBOL_GPL(acpi_os_map_iomem);
> >  
> >  void *__ref acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
> >  {
> > -	return (void *)__acpi_os_map_iomem(phys, size, true);
> > +	return (void *)acpi_os_map_iomem(phys, size);
> >  }
> >  EXPORT_SYMBOL_GPL(acpi_os_map_memory);
> >  
> > diff --git a/include/acpi/acpi_io.h b/include/acpi/acpi_io.h
> > index a0212e67d6f4..027faa8883aa 100644
> > --- a/include/acpi/acpi_io.h
> > +++ b/include/acpi/acpi_io.h
> > @@ -14,14 +14,6 @@ static inline void __iomem *acpi_os_ioremap(acpi_physical_address phys,
> >  }
> >  #endif
> >  
> > -#ifndef acpi_os_memmap
> > -static inline void __iomem *acpi_os_memmap(acpi_physical_address phys,
> > -					    acpi_size size)
> > -{
> > -	return ioremap_cache(phys, size);
> > -}
> > -#endif
> > -
> >  extern bool acpi_permanent_mmap;
> >  
> >  void __iomem __ref
> > -- 
> > 2.31.0
> > 
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > 

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2] Revert "ACPI: Add memory semantics to acpi_os_map_memory()"
  2021-09-23  9:40       ` Lorenzo Pieralisi
@ 2021-09-23 11:05         ` Rafael J. Wysocki
  2021-09-23 12:26           ` Mark Kettenis
  0 siblings, 1 reply; 23+ messages in thread
From: Rafael J. Wysocki @ 2021-09-23 11:05 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: Mark Kettenis, Jia He, Harb Abdulhamid, Will Deacon, Len Brown,
	Robert Moore, Erik Kaneda, Linux ARM, Linux Kernel Mailing List,
	ACPI Devel Maling List,
	open list:ACPI COMPONENT ARCHITECTURE (ACPICA),
	Ard Biesheuvel, Hanjun Guo, Catalin Marinas, Rafael Wysocki

On Thu, Sep 23, 2021 at 11:40 AM Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
>
> On Thu, Sep 23, 2021 at 01:09:58AM +0200, Mark Kettenis wrote:
> > > Date: Wed, 22 Sep 2021 17:33:36 +0100
> > > From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > >
> > > On Fri, Sep 10, 2021 at 10:32:23PM +0800, Jia He wrote:
> > > > This reverts commit 437b38c51162f8b87beb28a833c4d5dc85fa864e.
> > > >
> > > > After this commit, a boot panic is alway hit on an Ampere EMAG server
> > > > with call trace as follows:
> > > >  Internal error: synchronous external abort: 96000410 [#1] SMP
> > > >  Modules linked in:
> > > >  CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.14.0+ #462
> > > >  Hardware name: MiTAC RAPTOR EV-883832-X3-0001/RAPTOR, BIOS 0.14 02/22/2019
> > > >  pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> > > > [...snip...]
> > > >  Call trace:
> > > >   acpi_ex_system_memory_space_handler+0x26c/0x2c8
> > > >   acpi_ev_address_space_dispatch+0x228/0x2c4
> > > >   acpi_ex_access_region+0x114/0x268
> > > >   acpi_ex_field_datum_io+0x128/0x1b8
> > > >   acpi_ex_extract_from_field+0x14c/0x2ac
> > > >   acpi_ex_read_data_from_field+0x190/0x1b8
> > > >   acpi_ex_resolve_node_to_value+0x1ec/0x288
> > > >   acpi_ex_resolve_to_value+0x250/0x274
> > > >   acpi_ds_evaluate_name_path+0xac/0x124
> > > >   acpi_ds_exec_end_op+0x90/0x410
> > > >   acpi_ps_parse_loop+0x4ac/0x5d8
> > > >   acpi_ps_parse_aml+0xe0/0x2c8
> > > >   acpi_ps_execute_method+0x19c/0x1ac
> > > >   acpi_ns_evaluate+0x1f8/0x26c
> > > >   acpi_ns_init_one_device+0x104/0x140
> > > >   acpi_ns_walk_namespace+0x158/0x1d0
> > > >   acpi_ns_initialize_devices+0x194/0x218
> > > >   acpi_initialize_objects+0x48/0x50
> > > >   acpi_init+0xe0/0x498
> > > >
> > > > As mentioned by Lorenzo:
> > > >   "We are forcing memory semantics mappings to PROT_NORMAL_NC, which
> > > >   eMAG does not like at all and I'd need to understand why. It looks
> > > >   like the issue happen in SystemMemory Opregion handler."
> > > >
> > > > Hence just revert it before everything is clear.
> > > >
> > > > Fixes: 437b38c51162 ("ACPI: Add memory semantics to acpi_os_map_memory()")
> > > > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > > > Cc: Ard Biesheuvel <ardb@kernel.org>
> > > > Cc: Hanjun Guo <guohanjun@huawei.com>
> > > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > Cc: Harb Abdulhamid <harb@amperecomputing.com>
> > > >
> > > > Signed-off-by: Jia He <justin.he@arm.com>
> > >
> > > Rewrote the commit log, please take the patch below and repost
> > > it as a v3.
> > >
> > > It would still be great if Ampere can help us understand why
> > > the NormalNC attributes trigger a sync abort on the opregion
> > > before merging it.
> >
> > To be honest, I don't think you really need an explanation from Ampere
> > here.  Mapping a part of the address space that doesn't provide memory
> > semantics with NormalNC attributes is wrong and triggering a sync
> > abort in that case is way better than silently ignoring the access.
>
> That's understood and that's what I explained in the revert commit
> log, no question about it.
>
> I was just asking to confirm if that's what's actually happening.
>
> > Putting my OpenBSD hat on (where we have our own ACPI OSPM
> > implementation) I must say that we always interpreted SystemMemory as
> > memory mapped IO and I think that is a logical choice as SystemIO is
> > used for (non-memory mapped) IO.  And I'd say that the ACPI OSPM code
> > should make sure that it uses properly aligned access to any Field
> > object that doesn't use AnyAcc as its access type.  Even on x86!  And
> > I'd say that AML that uses AnyAcc fields for SystemMemory OpRegions on
> > arm64 is buggy.
> >
> > But maybe relaxing this when the EFI memory map indicates that the
> > address space in question does provide memory semantics does make
> > sense.  That should defenitely be documented in the ACPI standard
> > though.
>
> Mapping SystemMemory Opregions as "memory" does not make sense
> at all to me. Still, that's what Linux ACPICA code does (*if*
> that's what acpi_os_map_memory() is supposed to mean).
>
> https://lore.kernel.org/linux-acpi/20210916160827.GA4525@lpieralisi

It doesn't need to do that, though, if there are good enough arguments
to change the current behavior (and the argument here is that it may
be an MMIO region, so mapping it as memory doesn't really work, but it
also may be a region in memory - there is no rule in the spec by which
SystemMemory Opregions cannot be "memory" AFAICS) and if that change
doesn't introduce regressions in the installed base.

> Where do we go from here, to be defined, we still have a bug
> to fix after the revert is applied.
>
> drivers/acpi/sysfs.c
>
> maps BERT error regions with acpi_os_map_memory().

That mechanism is basically used for exporting ACPI tables to user
space and they are known to reside in memory.  Whether or not BERT
regions should be mapped in the same way is a good question.

> If the BERT error
> region is not in the EFI memory map, we map that memory as device-nGnRnE
> and we execute memory semantics operation on it.
>
> https://lore.kernel.org/linux-acpi/e548e72c-83a4-2366-dd57-3e746040fea9@arm.com
>
> I could change that code to map those regions as ioremap_wc() because
> supposedly we *know* that's memory but this is becoming a slippery
> slope to follow IMO.
>
> > > -- >8 --
> > > Subject: [PATCH] Revert "ACPI: Add memory semantics to acpi_os_map_memory()"
> > >
> > > This reverts commit 437b38c51162f8b87beb28a833c4d5dc85fa864e.
> > >
> > > The memory semantics added in commit 437b38c51162 causes SystemMemory
> > > Operation region, whose address range is not described in the EFI memory
> > > map to be mapped as NormalNC memory on arm64 platforms (through
> > > acpi_os_map_memory() in acpi_ex_system_memory_space_handler()).
> > >
> > > This triggers the following abort on an ARM64 Ampere eMAG machine,
> > > because presumably the physical address range area backing the Opregion
> > > does not support NormalNC memory attributes driven on the bus.
> > >
> > >  Internal error: synchronous external abort: 96000410 [#1] SMP
> > >  Modules linked in:
> > >  CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.14.0+ #462
> > >  Hardware name: MiTAC RAPTOR EV-883832-X3-0001/RAPTOR, BIOS 0.14 02/22/2019
> > >  pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> > > [...snip...]
> > >  Call trace:
> > >   acpi_ex_system_memory_space_handler+0x26c/0x2c8
> > >   acpi_ev_address_space_dispatch+0x228/0x2c4
> > >   acpi_ex_access_region+0x114/0x268
> > >   acpi_ex_field_datum_io+0x128/0x1b8
> > >   acpi_ex_extract_from_field+0x14c/0x2ac
> > >   acpi_ex_read_data_from_field+0x190/0x1b8
> > >   acpi_ex_resolve_node_to_value+0x1ec/0x288
> > >   acpi_ex_resolve_to_value+0x250/0x274
> > >   acpi_ds_evaluate_name_path+0xac/0x124
> > >   acpi_ds_exec_end_op+0x90/0x410
> > >   acpi_ps_parse_loop+0x4ac/0x5d8
> > >   acpi_ps_parse_aml+0xe0/0x2c8
> > >   acpi_ps_execute_method+0x19c/0x1ac
> > >   acpi_ns_evaluate+0x1f8/0x26c
> > >   acpi_ns_init_one_device+0x104/0x140
> > >   acpi_ns_walk_namespace+0x158/0x1d0
> > >   acpi_ns_initialize_devices+0x194/0x218
> > >   acpi_initialize_objects+0x48/0x50
> > >   acpi_init+0xe0/0x498
> > >
> > > If the Opregion address range is not present in the EFI memory map there
> > > is no way for us to determine the memory attributes to use to map it -
> > > defaulting to NormalNC does not work (and it is not correct on a memory
> > > region that may have read side-effects) and therefore commit
> > > 437b38c51162 should be reverted, which means reverting back to the
> > > original behavior whereby address ranges that are mapped using
> > > acpi_os_map_memory() default to the safe devicenGnRnE attributes on
> > > ARM64 if the mapped address range is not defined in the EFI memory map.
> > >
> > > Fixes: 437b38c51162 ("ACPI: Add memory semantics to acpi_os_map_memory()")
> > > Signed-off-by: Jia He <justin.he@arm.com>
> > > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > > Cc: Ard Biesheuvel <ardb@kernel.org>
> > > Cc: Hanjun Guo <guohanjun@huawei.com>
> > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > Cc: Harb Abdulhamid <harb@amperecomputing.com>
> > > ---
> > >  arch/arm64/include/asm/acpi.h |  3 ---
> > >  arch/arm64/kernel/acpi.c      | 19 +++----------------
> > >  drivers/acpi/osl.c            | 23 +++++++----------------
> > >  include/acpi/acpi_io.h        |  8 --------
> > >  4 files changed, 10 insertions(+), 43 deletions(-)
> > >
> > > diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
> > > index 7535dc7cc5aa..bd68e1b7f29f 100644
> > > --- a/arch/arm64/include/asm/acpi.h
> > > +++ b/arch/arm64/include/asm/acpi.h
> > > @@ -50,9 +50,6 @@ pgprot_t __acpi_get_mem_attribute(phys_addr_t addr);
> > >  void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size);
> > >  #define acpi_os_ioremap acpi_os_ioremap
> > >
> > > -void __iomem *acpi_os_memmap(acpi_physical_address phys, acpi_size size);
> > > -#define acpi_os_memmap acpi_os_memmap
> > > -
> > >  typedef u64 phys_cpuid_t;
> > >  #define PHYS_CPUID_INVALID INVALID_HWID
> > >
> > > diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
> > > index 1c9c2f7a1c04..f3851724fe35 100644
> > > --- a/arch/arm64/kernel/acpi.c
> > > +++ b/arch/arm64/kernel/acpi.c
> > > @@ -273,8 +273,7 @@ pgprot_t __acpi_get_mem_attribute(phys_addr_t addr)
> > >     return __pgprot(PROT_DEVICE_nGnRnE);
> > >  }
> > >
> > > -static void __iomem *__acpi_os_ioremap(acpi_physical_address phys,
> > > -                                  acpi_size size, bool memory)
> > > +void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size)
> > >  {
> > >     efi_memory_desc_t *md, *region = NULL;
> > >     pgprot_t prot;
> > > @@ -300,11 +299,9 @@ static void __iomem *__acpi_os_ioremap(acpi_physical_address phys,
> > >      * It is fine for AML to remap regions that are not represented in the
> > >      * EFI memory map at all, as it only describes normal memory, and MMIO
> > >      * regions that require a virtual mapping to make them accessible to
> > > -    * the EFI runtime services. Determine the region default
> > > -    * attributes by checking the requested memory semantics.
> > > +    * the EFI runtime services.
> > >      */
> > > -   prot = memory ? __pgprot(PROT_NORMAL_NC) :
> > > -                   __pgprot(PROT_DEVICE_nGnRnE);
> > > +   prot = __pgprot(PROT_DEVICE_nGnRnE);
> > >     if (region) {
> > >             switch (region->type) {
> > >             case EFI_LOADER_CODE:
> > > @@ -364,16 +361,6 @@ static void __iomem *__acpi_os_ioremap(acpi_physical_address phys,
> > >     return __ioremap(phys, size, prot);
> > >  }
> > >
> > > -void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size)
> > > -{
> > > -   return __acpi_os_ioremap(phys, size, false);
> > > -}
> > > -
> > > -void __iomem *acpi_os_memmap(acpi_physical_address phys, acpi_size size)
> > > -{
> > > -   return __acpi_os_ioremap(phys, size, true);
> > > -}
> > > -
> > >  /*
> > >   * Claim Synchronous External Aborts as a firmware first notification.
> > >   *
> > > diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> > > index a43f1521efe6..45c5c0e45e33 100644
> > > --- a/drivers/acpi/osl.c
> > > +++ b/drivers/acpi/osl.c
> > > @@ -284,8 +284,7 @@ acpi_map_lookup_virt(void __iomem *virt, acpi_size size)
> > >  #define should_use_kmap(pfn)   page_is_ram(pfn)
> > >  #endif
> > >
> > > -static void __iomem *acpi_map(acpi_physical_address pg_off, unsigned long pg_sz,
> > > -                         bool memory)
> > > +static void __iomem *acpi_map(acpi_physical_address pg_off, unsigned long pg_sz)
> > >  {
> > >     unsigned long pfn;
> > >
> > > @@ -295,8 +294,7 @@ static void __iomem *acpi_map(acpi_physical_address pg_off, unsigned long pg_sz,
> > >                     return NULL;
> > >             return (void __iomem __force *)kmap(pfn_to_page(pfn));
> > >     } else
> > > -           return memory ? acpi_os_memmap(pg_off, pg_sz) :
> > > -                           acpi_os_ioremap(pg_off, pg_sz);
> > > +           return acpi_os_ioremap(pg_off, pg_sz);
> > >  }
> > >
> > >  static void acpi_unmap(acpi_physical_address pg_off, void __iomem *vaddr)
> > > @@ -311,10 +309,9 @@ static void acpi_unmap(acpi_physical_address pg_off, void __iomem *vaddr)
> > >  }
> > >
> > >  /**
> > > - * __acpi_os_map_iomem - Get a virtual address for a given physical address range.
> > > + * acpi_os_map_iomem - Get a virtual address for a given physical address range.
> > >   * @phys: Start of the physical address range to map.
> > >   * @size: Size of the physical address range to map.
> > > - * @memory: true if remapping memory, false if IO
> > >   *
> > >   * Look up the given physical address range in the list of existing ACPI memory
> > >   * mappings.  If found, get a reference to it and return a pointer to it (its
> > > @@ -324,8 +321,8 @@ static void acpi_unmap(acpi_physical_address pg_off, void __iomem *vaddr)
> > >   * During early init (when acpi_permanent_mmap has not been set yet) this
> > >   * routine simply calls __acpi_map_table() to get the job done.
> > >   */
> > > -static void __iomem __ref
> > > -*__acpi_os_map_iomem(acpi_physical_address phys, acpi_size size, bool memory)
> > > +void __iomem __ref
> > > +*acpi_os_map_iomem(acpi_physical_address phys, acpi_size size)
> > >  {
> > >     struct acpi_ioremap *map;
> > >     void __iomem *virt;
> > > @@ -356,7 +353,7 @@ static void __iomem __ref
> > >
> > >     pg_off = round_down(phys, PAGE_SIZE);
> > >     pg_sz = round_up(phys + size, PAGE_SIZE) - pg_off;
> > > -   virt = acpi_map(phys, size, memory);
> > > +   virt = acpi_map(phys, size);
> > >     if (!virt) {
> > >             mutex_unlock(&acpi_ioremap_lock);
> > >             kfree(map);
> > > @@ -375,17 +372,11 @@ static void __iomem __ref
> > >     mutex_unlock(&acpi_ioremap_lock);
> > >     return map->virt + (phys - map->phys);
> > >  }
> > > -
> > > -void __iomem *__ref
> > > -acpi_os_map_iomem(acpi_physical_address phys, acpi_size size)
> > > -{
> > > -   return __acpi_os_map_iomem(phys, size, false);
> > > -}
> > >  EXPORT_SYMBOL_GPL(acpi_os_map_iomem);
> > >
> > >  void *__ref acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
> > >  {
> > > -   return (void *)__acpi_os_map_iomem(phys, size, true);
> > > +   return (void *)acpi_os_map_iomem(phys, size);
> > >  }
> > >  EXPORT_SYMBOL_GPL(acpi_os_map_memory);
> > >
> > > diff --git a/include/acpi/acpi_io.h b/include/acpi/acpi_io.h
> > > index a0212e67d6f4..027faa8883aa 100644
> > > --- a/include/acpi/acpi_io.h
> > > +++ b/include/acpi/acpi_io.h
> > > @@ -14,14 +14,6 @@ static inline void __iomem *acpi_os_ioremap(acpi_physical_address phys,
> > >  }
> > >  #endif
> > >
> > > -#ifndef acpi_os_memmap
> > > -static inline void __iomem *acpi_os_memmap(acpi_physical_address phys,
> > > -                                       acpi_size size)
> > > -{
> > > -   return ioremap_cache(phys, size);
> > > -}
> > > -#endif
> > > -
> > >  extern bool acpi_permanent_mmap;
> > >
> > >  void __iomem __ref
> > > --
> > > 2.31.0
> > >
> > > _______________________________________________
> > > linux-arm-kernel mailing list
> > > linux-arm-kernel@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > >

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2] Revert "ACPI: Add memory semantics to acpi_os_map_memory()"
  2021-09-23 11:05         ` Rafael J. Wysocki
@ 2021-09-23 12:26           ` Mark Kettenis
  2021-09-23 12:54             ` Rafael J. Wysocki
  0 siblings, 1 reply; 23+ messages in thread
From: Mark Kettenis @ 2021-09-23 12:26 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: lorenzo.pieralisi, justin.he, harb, will, lenb, robert.moore,
	erik.kaneda, linux-arm-kernel, linux-kernel, linux-acpi, devel,
	ardb, guohanjun, catalin.marinas, rafael.j.wysocki

> From: "Rafael J. Wysocki" <rafael@kernel.org>
> Date: Thu, 23 Sep 2021 13:05:05 +0200
> 
> On Thu, Sep 23, 2021 at 11:40 AM Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
> >
> > On Thu, Sep 23, 2021 at 01:09:58AM +0200, Mark Kettenis wrote:
> > > > Date: Wed, 22 Sep 2021 17:33:36 +0100
> > > > From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > > >
> > > > On Fri, Sep 10, 2021 at 10:32:23PM +0800, Jia He wrote:
> > > > > This reverts commit 437b38c51162f8b87beb28a833c4d5dc85fa864e.
> > > > >
> > > > > After this commit, a boot panic is alway hit on an Ampere EMAG server
> > > > > with call trace as follows:
> > > > >  Internal error: synchronous external abort: 96000410 [#1] SMP
> > > > >  Modules linked in:
> > > > >  CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.14.0+ #462
> > > > >  Hardware name: MiTAC RAPTOR EV-883832-X3-0001/RAPTOR, BIOS 0.14 02/22/2019
> > > > >  pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> > > > > [...snip...]
> > > > >  Call trace:
> > > > >   acpi_ex_system_memory_space_handler+0x26c/0x2c8
> > > > >   acpi_ev_address_space_dispatch+0x228/0x2c4
> > > > >   acpi_ex_access_region+0x114/0x268
> > > > >   acpi_ex_field_datum_io+0x128/0x1b8
> > > > >   acpi_ex_extract_from_field+0x14c/0x2ac
> > > > >   acpi_ex_read_data_from_field+0x190/0x1b8
> > > > >   acpi_ex_resolve_node_to_value+0x1ec/0x288
> > > > >   acpi_ex_resolve_to_value+0x250/0x274
> > > > >   acpi_ds_evaluate_name_path+0xac/0x124
> > > > >   acpi_ds_exec_end_op+0x90/0x410
> > > > >   acpi_ps_parse_loop+0x4ac/0x5d8
> > > > >   acpi_ps_parse_aml+0xe0/0x2c8
> > > > >   acpi_ps_execute_method+0x19c/0x1ac
> > > > >   acpi_ns_evaluate+0x1f8/0x26c
> > > > >   acpi_ns_init_one_device+0x104/0x140
> > > > >   acpi_ns_walk_namespace+0x158/0x1d0
> > > > >   acpi_ns_initialize_devices+0x194/0x218
> > > > >   acpi_initialize_objects+0x48/0x50
> > > > >   acpi_init+0xe0/0x498
> > > > >
> > > > > As mentioned by Lorenzo:
> > > > >   "We are forcing memory semantics mappings to PROT_NORMAL_NC, which
> > > > >   eMAG does not like at all and I'd need to understand why. It looks
> > > > >   like the issue happen in SystemMemory Opregion handler."
> > > > >
> > > > > Hence just revert it before everything is clear.
> > > > >
> > > > > Fixes: 437b38c51162 ("ACPI: Add memory semantics to acpi_os_map_memory()")
> > > > > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > > > > Cc: Ard Biesheuvel <ardb@kernel.org>
> > > > > Cc: Hanjun Guo <guohanjun@huawei.com>
> > > > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > > > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > Cc: Harb Abdulhamid <harb@amperecomputing.com>
> > > > >
> > > > > Signed-off-by: Jia He <justin.he@arm.com>
> > > >
> > > > Rewrote the commit log, please take the patch below and repost
> > > > it as a v3.
> > > >
> > > > It would still be great if Ampere can help us understand why
> > > > the NormalNC attributes trigger a sync abort on the opregion
> > > > before merging it.
> > >
> > > To be honest, I don't think you really need an explanation from Ampere
> > > here.  Mapping a part of the address space that doesn't provide memory
> > > semantics with NormalNC attributes is wrong and triggering a sync
> > > abort in that case is way better than silently ignoring the access.
> >
> > That's understood and that's what I explained in the revert commit
> > log, no question about it.
> >
> > I was just asking to confirm if that's what's actually happening.
> >
> > > Putting my OpenBSD hat on (where we have our own ACPI OSPM
> > > implementation) I must say that we always interpreted SystemMemory as
> > > memory mapped IO and I think that is a logical choice as SystemIO is
> > > used for (non-memory mapped) IO.  And I'd say that the ACPI OSPM code
> > > should make sure that it uses properly aligned access to any Field
> > > object that doesn't use AnyAcc as its access type.  Even on x86!  And
> > > I'd say that AML that uses AnyAcc fields for SystemMemory OpRegions on
> > > arm64 is buggy.
> > >
> > > But maybe relaxing this when the EFI memory map indicates that the
> > > address space in question does provide memory semantics does make
> > > sense.  That should defenitely be documented in the ACPI standard
> > > though.
> >
> > Mapping SystemMemory Opregions as "memory" does not make sense
> > at all to me. Still, that's what Linux ACPICA code does (*if*
> > that's what acpi_os_map_memory() is supposed to mean).
> >
> > https://lore.kernel.org/linux-acpi/20210916160827.GA4525@lpieralisi
> 
> It doesn't need to do that, though, if there are good enough arguments
> to change the current behavior (and the argument here is that it may
> be an MMIO region, so mapping it as memory doesn't really work, but it
> also may be a region in memory - there is no rule in the spec by which
> SystemMemory Opregions cannot be "memory" AFAICS) and if that change
> doesn't introduce regressions in the installed base.
> 
> > Where do we go from here, to be defined, we still have a bug
> > to fix after the revert is applied.
> >
> > drivers/acpi/sysfs.c
> >
> > maps BERT error regions with acpi_os_map_memory().
> 
> That mechanism is basically used for exporting ACPI tables to user
> space and they are known to reside in memory.  Whether or not BERT
> regions should be mapped in the same way is a good question.

It is not inconceivable that BERT regions actually live in memory of
the BMC that is exposed over a bus that doesn't implement memory
semantics is it?

> > If the BERT error
> > region is not in the EFI memory map, we map that memory as device-nGnRnE
> > and we execute memory semantics operation on it.
> >
> > https://lore.kernel.org/linux-acpi/e548e72c-83a4-2366-dd57-3e746040fea9@arm.com
> >
> > I could change that code to map those regions as ioremap_wc() because
> > supposedly we *know* that's memory but this is becoming a slippery
> > slope to follow IMO.
> >
> > > > -- >8 --
> > > > Subject: [PATCH] Revert "ACPI: Add memory semantics to acpi_os_map_memory()"
> > > >
> > > > This reverts commit 437b38c51162f8b87beb28a833c4d5dc85fa864e.
> > > >
> > > > The memory semantics added in commit 437b38c51162 causes SystemMemory
> > > > Operation region, whose address range is not described in the EFI memory
> > > > map to be mapped as NormalNC memory on arm64 platforms (through
> > > > acpi_os_map_memory() in acpi_ex_system_memory_space_handler()).
> > > >
> > > > This triggers the following abort on an ARM64 Ampere eMAG machine,
> > > > because presumably the physical address range area backing the Opregion
> > > > does not support NormalNC memory attributes driven on the bus.
> > > >
> > > >  Internal error: synchronous external abort: 96000410 [#1] SMP
> > > >  Modules linked in:
> > > >  CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.14.0+ #462
> > > >  Hardware name: MiTAC RAPTOR EV-883832-X3-0001/RAPTOR, BIOS 0.14 02/22/2019
> > > >  pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> > > > [...snip...]
> > > >  Call trace:
> > > >   acpi_ex_system_memory_space_handler+0x26c/0x2c8
> > > >   acpi_ev_address_space_dispatch+0x228/0x2c4
> > > >   acpi_ex_access_region+0x114/0x268
> > > >   acpi_ex_field_datum_io+0x128/0x1b8
> > > >   acpi_ex_extract_from_field+0x14c/0x2ac
> > > >   acpi_ex_read_data_from_field+0x190/0x1b8
> > > >   acpi_ex_resolve_node_to_value+0x1ec/0x288
> > > >   acpi_ex_resolve_to_value+0x250/0x274
> > > >   acpi_ds_evaluate_name_path+0xac/0x124
> > > >   acpi_ds_exec_end_op+0x90/0x410
> > > >   acpi_ps_parse_loop+0x4ac/0x5d8
> > > >   acpi_ps_parse_aml+0xe0/0x2c8
> > > >   acpi_ps_execute_method+0x19c/0x1ac
> > > >   acpi_ns_evaluate+0x1f8/0x26c
> > > >   acpi_ns_init_one_device+0x104/0x140
> > > >   acpi_ns_walk_namespace+0x158/0x1d0
> > > >   acpi_ns_initialize_devices+0x194/0x218
> > > >   acpi_initialize_objects+0x48/0x50
> > > >   acpi_init+0xe0/0x498
> > > >
> > > > If the Opregion address range is not present in the EFI memory map there
> > > > is no way for us to determine the memory attributes to use to map it -
> > > > defaulting to NormalNC does not work (and it is not correct on a memory
> > > > region that may have read side-effects) and therefore commit
> > > > 437b38c51162 should be reverted, which means reverting back to the
> > > > original behavior whereby address ranges that are mapped using
> > > > acpi_os_map_memory() default to the safe devicenGnRnE attributes on
> > > > ARM64 if the mapped address range is not defined in the EFI memory map.
> > > >
> > > > Fixes: 437b38c51162 ("ACPI: Add memory semantics to acpi_os_map_memory()")
> > > > Signed-off-by: Jia He <justin.he@arm.com>
> > > > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > > > Cc: Ard Biesheuvel <ardb@kernel.org>
> > > > Cc: Hanjun Guo <guohanjun@huawei.com>
> > > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > Cc: Harb Abdulhamid <harb@amperecomputing.com>
> > > > ---
> > > >  arch/arm64/include/asm/acpi.h |  3 ---
> > > >  arch/arm64/kernel/acpi.c      | 19 +++----------------
> > > >  drivers/acpi/osl.c            | 23 +++++++----------------
> > > >  include/acpi/acpi_io.h        |  8 --------
> > > >  4 files changed, 10 insertions(+), 43 deletions(-)
> > > >
> > > > diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
> > > > index 7535dc7cc5aa..bd68e1b7f29f 100644
> > > > --- a/arch/arm64/include/asm/acpi.h
> > > > +++ b/arch/arm64/include/asm/acpi.h
> > > > @@ -50,9 +50,6 @@ pgprot_t __acpi_get_mem_attribute(phys_addr_t addr);
> > > >  void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size);
> > > >  #define acpi_os_ioremap acpi_os_ioremap
> > > >
> > > > -void __iomem *acpi_os_memmap(acpi_physical_address phys, acpi_size size);
> > > > -#define acpi_os_memmap acpi_os_memmap
> > > > -
> > > >  typedef u64 phys_cpuid_t;
> > > >  #define PHYS_CPUID_INVALID INVALID_HWID
> > > >
> > > > diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
> > > > index 1c9c2f7a1c04..f3851724fe35 100644
> > > > --- a/arch/arm64/kernel/acpi.c
> > > > +++ b/arch/arm64/kernel/acpi.c
> > > > @@ -273,8 +273,7 @@ pgprot_t __acpi_get_mem_attribute(phys_addr_t addr)
> > > >     return __pgprot(PROT_DEVICE_nGnRnE);
> > > >  }
> > > >
> > > > -static void __iomem *__acpi_os_ioremap(acpi_physical_address phys,
> > > > -                                  acpi_size size, bool memory)
> > > > +void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size)
> > > >  {
> > > >     efi_memory_desc_t *md, *region = NULL;
> > > >     pgprot_t prot;
> > > > @@ -300,11 +299,9 @@ static void __iomem *__acpi_os_ioremap(acpi_physical_address phys,
> > > >      * It is fine for AML to remap regions that are not represented in the
> > > >      * EFI memory map at all, as it only describes normal memory, and MMIO
> > > >      * regions that require a virtual mapping to make them accessible to
> > > > -    * the EFI runtime services. Determine the region default
> > > > -    * attributes by checking the requested memory semantics.
> > > > +    * the EFI runtime services.
> > > >      */
> > > > -   prot = memory ? __pgprot(PROT_NORMAL_NC) :
> > > > -                   __pgprot(PROT_DEVICE_nGnRnE);
> > > > +   prot = __pgprot(PROT_DEVICE_nGnRnE);
> > > >     if (region) {
> > > >             switch (region->type) {
> > > >             case EFI_LOADER_CODE:
> > > > @@ -364,16 +361,6 @@ static void __iomem *__acpi_os_ioremap(acpi_physical_address phys,
> > > >     return __ioremap(phys, size, prot);
> > > >  }
> > > >
> > > > -void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size)
> > > > -{
> > > > -   return __acpi_os_ioremap(phys, size, false);
> > > > -}
> > > > -
> > > > -void __iomem *acpi_os_memmap(acpi_physical_address phys, acpi_size size)
> > > > -{
> > > > -   return __acpi_os_ioremap(phys, size, true);
> > > > -}
> > > > -
> > > >  /*
> > > >   * Claim Synchronous External Aborts as a firmware first notification.
> > > >   *
> > > > diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> > > > index a43f1521efe6..45c5c0e45e33 100644
> > > > --- a/drivers/acpi/osl.c
> > > > +++ b/drivers/acpi/osl.c
> > > > @@ -284,8 +284,7 @@ acpi_map_lookup_virt(void __iomem *virt, acpi_size size)
> > > >  #define should_use_kmap(pfn)   page_is_ram(pfn)
> > > >  #endif
> > > >
> > > > -static void __iomem *acpi_map(acpi_physical_address pg_off, unsigned long pg_sz,
> > > > -                         bool memory)
> > > > +static void __iomem *acpi_map(acpi_physical_address pg_off, unsigned long pg_sz)
> > > >  {
> > > >     unsigned long pfn;
> > > >
> > > > @@ -295,8 +294,7 @@ static void __iomem *acpi_map(acpi_physical_address pg_off, unsigned long pg_sz,
> > > >                     return NULL;
> > > >             return (void __iomem __force *)kmap(pfn_to_page(pfn));
> > > >     } else
> > > > -           return memory ? acpi_os_memmap(pg_off, pg_sz) :
> > > > -                           acpi_os_ioremap(pg_off, pg_sz);
> > > > +           return acpi_os_ioremap(pg_off, pg_sz);
> > > >  }
> > > >
> > > >  static void acpi_unmap(acpi_physical_address pg_off, void __iomem *vaddr)
> > > > @@ -311,10 +309,9 @@ static void acpi_unmap(acpi_physical_address pg_off, void __iomem *vaddr)
> > > >  }
> > > >
> > > >  /**
> > > > - * __acpi_os_map_iomem - Get a virtual address for a given physical address range.
> > > > + * acpi_os_map_iomem - Get a virtual address for a given physical address range.
> > > >   * @phys: Start of the physical address range to map.
> > > >   * @size: Size of the physical address range to map.
> > > > - * @memory: true if remapping memory, false if IO
> > > >   *
> > > >   * Look up the given physical address range in the list of existing ACPI memory
> > > >   * mappings.  If found, get a reference to it and return a pointer to it (its
> > > > @@ -324,8 +321,8 @@ static void acpi_unmap(acpi_physical_address pg_off, void __iomem *vaddr)
> > > >   * During early init (when acpi_permanent_mmap has not been set yet) this
> > > >   * routine simply calls __acpi_map_table() to get the job done.
> > > >   */
> > > > -static void __iomem __ref
> > > > -*__acpi_os_map_iomem(acpi_physical_address phys, acpi_size size, bool memory)
> > > > +void __iomem __ref
> > > > +*acpi_os_map_iomem(acpi_physical_address phys, acpi_size size)
> > > >  {
> > > >     struct acpi_ioremap *map;
> > > >     void __iomem *virt;
> > > > @@ -356,7 +353,7 @@ static void __iomem __ref
> > > >
> > > >     pg_off = round_down(phys, PAGE_SIZE);
> > > >     pg_sz = round_up(phys + size, PAGE_SIZE) - pg_off;
> > > > -   virt = acpi_map(phys, size, memory);
> > > > +   virt = acpi_map(phys, size);
> > > >     if (!virt) {
> > > >             mutex_unlock(&acpi_ioremap_lock);
> > > >             kfree(map);
> > > > @@ -375,17 +372,11 @@ static void __iomem __ref
> > > >     mutex_unlock(&acpi_ioremap_lock);
> > > >     return map->virt + (phys - map->phys);
> > > >  }
> > > > -
> > > > -void __iomem *__ref
> > > > -acpi_os_map_iomem(acpi_physical_address phys, acpi_size size)
> > > > -{
> > > > -   return __acpi_os_map_iomem(phys, size, false);
> > > > -}
> > > >  EXPORT_SYMBOL_GPL(acpi_os_map_iomem);
> > > >
> > > >  void *__ref acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
> > > >  {
> > > > -   return (void *)__acpi_os_map_iomem(phys, size, true);
> > > > +   return (void *)acpi_os_map_iomem(phys, size);
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(acpi_os_map_memory);
> > > >
> > > > diff --git a/include/acpi/acpi_io.h b/include/acpi/acpi_io.h
> > > > index a0212e67d6f4..027faa8883aa 100644
> > > > --- a/include/acpi/acpi_io.h
> > > > +++ b/include/acpi/acpi_io.h
> > > > @@ -14,14 +14,6 @@ static inline void __iomem *acpi_os_ioremap(acpi_physical_address phys,
> > > >  }
> > > >  #endif
> > > >
> > > > -#ifndef acpi_os_memmap
> > > > -static inline void __iomem *acpi_os_memmap(acpi_physical_address phys,
> > > > -                                       acpi_size size)
> > > > -{
> > > > -   return ioremap_cache(phys, size);
> > > > -}
> > > > -#endif
> > > > -
> > > >  extern bool acpi_permanent_mmap;
> > > >
> > > >  void __iomem __ref
> > > > --
> > > > 2.31.0
> > > >
> > > > _______________________________________________
> > > > linux-arm-kernel mailing list
> > > > linux-arm-kernel@lists.infradead.org
> > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > > >
> 

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2] Revert "ACPI: Add memory semantics to acpi_os_map_memory()"
  2021-09-23 12:26           ` Mark Kettenis
@ 2021-09-23 12:54             ` Rafael J. Wysocki
  2021-09-24  9:04               ` Lorenzo Pieralisi
  0 siblings, 1 reply; 23+ messages in thread
From: Rafael J. Wysocki @ 2021-09-23 12:54 UTC (permalink / raw)
  To: Mark Kettenis
  Cc: Rafael J. Wysocki, Lorenzo Pieralisi, Jia He, Harb Abdulhamid,
	Will Deacon, Len Brown, Robert Moore, Erik Kaneda, Linux ARM,
	Linux Kernel Mailing List, ACPI Devel Maling List,
	open list:ACPI COMPONENT ARCHITECTURE (ACPICA),
	Ard Biesheuvel, Hanjun Guo, Catalin Marinas, Rafael Wysocki

On Thu, Sep 23, 2021 at 2:26 PM Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
>
> > From: "Rafael J. Wysocki" <rafael@kernel.org>
> > Date: Thu, 23 Sep 2021 13:05:05 +0200
> >
> > On Thu, Sep 23, 2021 at 11:40 AM Lorenzo Pieralisi
> > <lorenzo.pieralisi@arm.com> wrote:
> > >
> > > On Thu, Sep 23, 2021 at 01:09:58AM +0200, Mark Kettenis wrote:
> > > > > Date: Wed, 22 Sep 2021 17:33:36 +0100
> > > > > From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > > > >
> > > > > On Fri, Sep 10, 2021 at 10:32:23PM +0800, Jia He wrote:
> > > > > > This reverts commit 437b38c51162f8b87beb28a833c4d5dc85fa864e.
> > > > > >
> > > > > > After this commit, a boot panic is alway hit on an Ampere EMAG server
> > > > > > with call trace as follows:
> > > > > >  Internal error: synchronous external abort: 96000410 [#1] SMP
> > > > > >  Modules linked in:
> > > > > >  CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.14.0+ #462
> > > > > >  Hardware name: MiTAC RAPTOR EV-883832-X3-0001/RAPTOR, BIOS 0.14 02/22/2019
> > > > > >  pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> > > > > > [...snip...]
> > > > > >  Call trace:
> > > > > >   acpi_ex_system_memory_space_handler+0x26c/0x2c8
> > > > > >   acpi_ev_address_space_dispatch+0x228/0x2c4
> > > > > >   acpi_ex_access_region+0x114/0x268
> > > > > >   acpi_ex_field_datum_io+0x128/0x1b8
> > > > > >   acpi_ex_extract_from_field+0x14c/0x2ac
> > > > > >   acpi_ex_read_data_from_field+0x190/0x1b8
> > > > > >   acpi_ex_resolve_node_to_value+0x1ec/0x288
> > > > > >   acpi_ex_resolve_to_value+0x250/0x274
> > > > > >   acpi_ds_evaluate_name_path+0xac/0x124
> > > > > >   acpi_ds_exec_end_op+0x90/0x410
> > > > > >   acpi_ps_parse_loop+0x4ac/0x5d8
> > > > > >   acpi_ps_parse_aml+0xe0/0x2c8
> > > > > >   acpi_ps_execute_method+0x19c/0x1ac
> > > > > >   acpi_ns_evaluate+0x1f8/0x26c
> > > > > >   acpi_ns_init_one_device+0x104/0x140
> > > > > >   acpi_ns_walk_namespace+0x158/0x1d0
> > > > > >   acpi_ns_initialize_devices+0x194/0x218
> > > > > >   acpi_initialize_objects+0x48/0x50
> > > > > >   acpi_init+0xe0/0x498
> > > > > >
> > > > > > As mentioned by Lorenzo:
> > > > > >   "We are forcing memory semantics mappings to PROT_NORMAL_NC, which
> > > > > >   eMAG does not like at all and I'd need to understand why. It looks
> > > > > >   like the issue happen in SystemMemory Opregion handler."
> > > > > >
> > > > > > Hence just revert it before everything is clear.
> > > > > >
> > > > > > Fixes: 437b38c51162 ("ACPI: Add memory semantics to acpi_os_map_memory()")
> > > > > > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > > > > > Cc: Ard Biesheuvel <ardb@kernel.org>
> > > > > > Cc: Hanjun Guo <guohanjun@huawei.com>
> > > > > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > > > > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > > Cc: Harb Abdulhamid <harb@amperecomputing.com>
> > > > > >
> > > > > > Signed-off-by: Jia He <justin.he@arm.com>
> > > > >
> > > > > Rewrote the commit log, please take the patch below and repost
> > > > > it as a v3.
> > > > >
> > > > > It would still be great if Ampere can help us understand why
> > > > > the NormalNC attributes trigger a sync abort on the opregion
> > > > > before merging it.
> > > >
> > > > To be honest, I don't think you really need an explanation from Ampere
> > > > here.  Mapping a part of the address space that doesn't provide memory
> > > > semantics with NormalNC attributes is wrong and triggering a sync
> > > > abort in that case is way better than silently ignoring the access.
> > >
> > > That's understood and that's what I explained in the revert commit
> > > log, no question about it.
> > >
> > > I was just asking to confirm if that's what's actually happening.
> > >
> > > > Putting my OpenBSD hat on (where we have our own ACPI OSPM
> > > > implementation) I must say that we always interpreted SystemMemory as
> > > > memory mapped IO and I think that is a logical choice as SystemIO is
> > > > used for (non-memory mapped) IO.  And I'd say that the ACPI OSPM code
> > > > should make sure that it uses properly aligned access to any Field
> > > > object that doesn't use AnyAcc as its access type.  Even on x86!  And
> > > > I'd say that AML that uses AnyAcc fields for SystemMemory OpRegions on
> > > > arm64 is buggy.
> > > >
> > > > But maybe relaxing this when the EFI memory map indicates that the
> > > > address space in question does provide memory semantics does make
> > > > sense.  That should defenitely be documented in the ACPI standard
> > > > though.
> > >
> > > Mapping SystemMemory Opregions as "memory" does not make sense
> > > at all to me. Still, that's what Linux ACPICA code does (*if*
> > > that's what acpi_os_map_memory() is supposed to mean).
> > >
> > > https://lore.kernel.org/linux-acpi/20210916160827.GA4525@lpieralisi
> >
> > It doesn't need to do that, though, if there are good enough arguments
> > to change the current behavior (and the argument here is that it may
> > be an MMIO region, so mapping it as memory doesn't really work, but it
> > also may be a region in memory - there is no rule in the spec by which
> > SystemMemory Opregions cannot be "memory" AFAICS) and if that change
> > doesn't introduce regressions in the installed base.
> >
> > > Where do we go from here, to be defined, we still have a bug
> > > to fix after the revert is applied.
> > >
> > > drivers/acpi/sysfs.c
> > >
> > > maps BERT error regions with acpi_os_map_memory().
> >
> > That mechanism is basically used for exporting ACPI tables to user
> > space and they are known to reside in memory.  Whether or not BERT
> > regions should be mapped in the same way is a good question.
>
> It is not inconceivable that BERT regions actually live in memory of
> the BMC that is exposed over a bus that doesn't implement memory
> semantics is it?

No, it isn't, which is why I think that mapping them as RAM may not be
a good idea in general.

At the same time, mapping the ACPI tables like the DSDT etc. as RAM is
always valid.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2] Revert "ACPI: Add memory semantics to acpi_os_map_memory()"
  2021-09-23 12:54             ` Rafael J. Wysocki
@ 2021-09-24  9:04               ` Lorenzo Pieralisi
  2021-09-28 17:26                 ` Rafael J. Wysocki
  0 siblings, 1 reply; 23+ messages in thread
From: Lorenzo Pieralisi @ 2021-09-24  9:04 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Mark Kettenis, Jia He, Harb Abdulhamid, Will Deacon, Len Brown,
	Robert Moore, Erik Kaneda, Linux ARM, Linux Kernel Mailing List,
	ACPI Devel Maling List,
	open list:ACPI COMPONENT ARCHITECTURE (ACPICA),
	Ard Biesheuvel, Hanjun Guo, Catalin Marinas, Rafael Wysocki

On Thu, Sep 23, 2021 at 02:54:52PM +0200, Rafael J. Wysocki wrote:
> On Thu, Sep 23, 2021 at 2:26 PM Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> >
> > > From: "Rafael J. Wysocki" <rafael@kernel.org>
> > > Date: Thu, 23 Sep 2021 13:05:05 +0200
> > >
> > > On Thu, Sep 23, 2021 at 11:40 AM Lorenzo Pieralisi
> > > <lorenzo.pieralisi@arm.com> wrote:
> > > >
> > > > On Thu, Sep 23, 2021 at 01:09:58AM +0200, Mark Kettenis wrote:
> > > > > > Date: Wed, 22 Sep 2021 17:33:36 +0100
> > > > > > From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > > > > >
> > > > > > On Fri, Sep 10, 2021 at 10:32:23PM +0800, Jia He wrote:
> > > > > > > This reverts commit 437b38c51162f8b87beb28a833c4d5dc85fa864e.
> > > > > > >
> > > > > > > After this commit, a boot panic is alway hit on an Ampere EMAG server
> > > > > > > with call trace as follows:
> > > > > > >  Internal error: synchronous external abort: 96000410 [#1] SMP
> > > > > > >  Modules linked in:
> > > > > > >  CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.14.0+ #462
> > > > > > >  Hardware name: MiTAC RAPTOR EV-883832-X3-0001/RAPTOR, BIOS 0.14 02/22/2019
> > > > > > >  pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> > > > > > > [...snip...]
> > > > > > >  Call trace:
> > > > > > >   acpi_ex_system_memory_space_handler+0x26c/0x2c8
> > > > > > >   acpi_ev_address_space_dispatch+0x228/0x2c4
> > > > > > >   acpi_ex_access_region+0x114/0x268
> > > > > > >   acpi_ex_field_datum_io+0x128/0x1b8
> > > > > > >   acpi_ex_extract_from_field+0x14c/0x2ac
> > > > > > >   acpi_ex_read_data_from_field+0x190/0x1b8
> > > > > > >   acpi_ex_resolve_node_to_value+0x1ec/0x288
> > > > > > >   acpi_ex_resolve_to_value+0x250/0x274
> > > > > > >   acpi_ds_evaluate_name_path+0xac/0x124
> > > > > > >   acpi_ds_exec_end_op+0x90/0x410
> > > > > > >   acpi_ps_parse_loop+0x4ac/0x5d8
> > > > > > >   acpi_ps_parse_aml+0xe0/0x2c8
> > > > > > >   acpi_ps_execute_method+0x19c/0x1ac
> > > > > > >   acpi_ns_evaluate+0x1f8/0x26c
> > > > > > >   acpi_ns_init_one_device+0x104/0x140
> > > > > > >   acpi_ns_walk_namespace+0x158/0x1d0
> > > > > > >   acpi_ns_initialize_devices+0x194/0x218
> > > > > > >   acpi_initialize_objects+0x48/0x50
> > > > > > >   acpi_init+0xe0/0x498
> > > > > > >
> > > > > > > As mentioned by Lorenzo:
> > > > > > >   "We are forcing memory semantics mappings to PROT_NORMAL_NC, which
> > > > > > >   eMAG does not like at all and I'd need to understand why. It looks
> > > > > > >   like the issue happen in SystemMemory Opregion handler."
> > > > > > >
> > > > > > > Hence just revert it before everything is clear.
> > > > > > >
> > > > > > > Fixes: 437b38c51162 ("ACPI: Add memory semantics to acpi_os_map_memory()")
> > > > > > > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > > > > > > Cc: Ard Biesheuvel <ardb@kernel.org>
> > > > > > > Cc: Hanjun Guo <guohanjun@huawei.com>
> > > > > > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > > > > > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > > > Cc: Harb Abdulhamid <harb@amperecomputing.com>
> > > > > > >
> > > > > > > Signed-off-by: Jia He <justin.he@arm.com>
> > > > > >
> > > > > > Rewrote the commit log, please take the patch below and repost
> > > > > > it as a v3.
> > > > > >
> > > > > > It would still be great if Ampere can help us understand why
> > > > > > the NormalNC attributes trigger a sync abort on the opregion
> > > > > > before merging it.
> > > > >
> > > > > To be honest, I don't think you really need an explanation from Ampere
> > > > > here.  Mapping a part of the address space that doesn't provide memory
> > > > > semantics with NormalNC attributes is wrong and triggering a sync
> > > > > abort in that case is way better than silently ignoring the access.
> > > >
> > > > That's understood and that's what I explained in the revert commit
> > > > log, no question about it.
> > > >
> > > > I was just asking to confirm if that's what's actually happening.
> > > >
> > > > > Putting my OpenBSD hat on (where we have our own ACPI OSPM
> > > > > implementation) I must say that we always interpreted SystemMemory as
> > > > > memory mapped IO and I think that is a logical choice as SystemIO is
> > > > > used for (non-memory mapped) IO.  And I'd say that the ACPI OSPM code
> > > > > should make sure that it uses properly aligned access to any Field
> > > > > object that doesn't use AnyAcc as its access type.  Even on x86!  And
> > > > > I'd say that AML that uses AnyAcc fields for SystemMemory OpRegions on
> > > > > arm64 is buggy.
> > > > >
> > > > > But maybe relaxing this when the EFI memory map indicates that the
> > > > > address space in question does provide memory semantics does make
> > > > > sense.  That should defenitely be documented in the ACPI standard
> > > > > though.
> > > >
> > > > Mapping SystemMemory Opregions as "memory" does not make sense
> > > > at all to me. Still, that's what Linux ACPICA code does (*if*
> > > > that's what acpi_os_map_memory() is supposed to mean).
> > > >
> > > > https://lore.kernel.org/linux-acpi/20210916160827.GA4525@lpieralisi
> > >
> > > It doesn't need to do that, though, if there are good enough arguments
> > > to change the current behavior (and the argument here is that it may
> > > be an MMIO region, so mapping it as memory doesn't really work, but it
> > > also may be a region in memory - there is no rule in the spec by which
> > > SystemMemory Opregions cannot be "memory" AFAICS) and if that change
> > > doesn't introduce regressions in the installed base.
> > >
> > > > Where do we go from here, to be defined, we still have a bug
> > > > to fix after the revert is applied.
> > > >
> > > > drivers/acpi/sysfs.c
> > > >
> > > > maps BERT error regions with acpi_os_map_memory().
> > >
> > > That mechanism is basically used for exporting ACPI tables to user
> > > space and they are known to reside in memory.  Whether or not BERT
> > > regions should be mapped in the same way is a good question.
> >
> > It is not inconceivable that BERT regions actually live in memory of
> > the BMC that is exposed over a bus that doesn't implement memory
> > semantics is it?
> 
> No, it isn't, which is why I think that mapping them as RAM may not be
> a good idea in general.

Should I patch acpi_data_show() to map BERT error regions (well, that's
what acpi_data_show() is used on at the moment) as MMIO and use the
related memcpy routine to read them then :) ?

Lorenzo

> At the same time, mapping the ACPI tables like the DSDT etc. as RAM is
> always valid.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2] Revert "ACPI: Add memory semantics to acpi_os_map_memory()"
  2021-09-24  9:04               ` Lorenzo Pieralisi
@ 2021-09-28 17:26                 ` Rafael J. Wysocki
  2021-09-29 13:31                   ` Lorenzo Pieralisi
  2021-11-02 15:11                   ` Lorenzo Pieralisi
  0 siblings, 2 replies; 23+ messages in thread
From: Rafael J. Wysocki @ 2021-09-28 17:26 UTC (permalink / raw)
  To: Lorenzo Pieralisi, Rafael J. Wysocki
  Cc: Mark Kettenis, Jia He, Harb Abdulhamid, Will Deacon, Len Brown,
	Robert Moore, Erik Kaneda, Linux ARM, Linux Kernel Mailing List,
	ACPI Devel Maling List,
	open list:ACPI COMPONENT ARCHITECTURE (ACPICA),
	Ard Biesheuvel, Hanjun Guo, Catalin Marinas

On 9/24/2021 11:04 AM, Lorenzo Pieralisi wrote:
> On Thu, Sep 23, 2021 at 02:54:52PM +0200, Rafael J. Wysocki wrote:
>> On Thu, Sep 23, 2021 at 2:26 PM Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
>>>> From: "Rafael J. Wysocki" <rafael@kernel.org>
>>>> Date: Thu, 23 Sep 2021 13:05:05 +0200
>>>>
>>>> On Thu, Sep 23, 2021 at 11:40 AM Lorenzo Pieralisi
>>>> <lorenzo.pieralisi@arm.com> wrote:
>>>>> On Thu, Sep 23, 2021 at 01:09:58AM +0200, Mark Kettenis wrote:
>>>>>>> Date: Wed, 22 Sep 2021 17:33:36 +0100
>>>>>>> From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>>>>>>>
>>>>>>> On Fri, Sep 10, 2021 at 10:32:23PM +0800, Jia He wrote:
>>>>>>>> This reverts commit 437b38c51162f8b87beb28a833c4d5dc85fa864e.
>>>>>>>>
>>>>>>>> After this commit, a boot panic is alway hit on an Ampere EMAG server
>>>>>>>> with call trace as follows:
>>>>>>>>   Internal error: synchronous external abort: 96000410 [#1] SMP
>>>>>>>>   Modules linked in:
>>>>>>>>   CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.14.0+ #462
>>>>>>>>   Hardware name: MiTAC RAPTOR EV-883832-X3-0001/RAPTOR, BIOS 0.14 02/22/2019
>>>>>>>>   pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>>>>>>>> [...snip...]
>>>>>>>>   Call trace:
>>>>>>>>    acpi_ex_system_memory_space_handler+0x26c/0x2c8
>>>>>>>>    acpi_ev_address_space_dispatch+0x228/0x2c4
>>>>>>>>    acpi_ex_access_region+0x114/0x268
>>>>>>>>    acpi_ex_field_datum_io+0x128/0x1b8
>>>>>>>>    acpi_ex_extract_from_field+0x14c/0x2ac
>>>>>>>>    acpi_ex_read_data_from_field+0x190/0x1b8
>>>>>>>>    acpi_ex_resolve_node_to_value+0x1ec/0x288
>>>>>>>>    acpi_ex_resolve_to_value+0x250/0x274
>>>>>>>>    acpi_ds_evaluate_name_path+0xac/0x124
>>>>>>>>    acpi_ds_exec_end_op+0x90/0x410
>>>>>>>>    acpi_ps_parse_loop+0x4ac/0x5d8
>>>>>>>>    acpi_ps_parse_aml+0xe0/0x2c8
>>>>>>>>    acpi_ps_execute_method+0x19c/0x1ac
>>>>>>>>    acpi_ns_evaluate+0x1f8/0x26c
>>>>>>>>    acpi_ns_init_one_device+0x104/0x140
>>>>>>>>    acpi_ns_walk_namespace+0x158/0x1d0
>>>>>>>>    acpi_ns_initialize_devices+0x194/0x218
>>>>>>>>    acpi_initialize_objects+0x48/0x50
>>>>>>>>    acpi_init+0xe0/0x498
>>>>>>>>
>>>>>>>> As mentioned by Lorenzo:
>>>>>>>>    "We are forcing memory semantics mappings to PROT_NORMAL_NC, which
>>>>>>>>    eMAG does not like at all and I'd need to understand why. It looks
>>>>>>>>    like the issue happen in SystemMemory Opregion handler."
>>>>>>>>
>>>>>>>> Hence just revert it before everything is clear.
>>>>>>>>
>>>>>>>> Fixes: 437b38c51162 ("ACPI: Add memory semantics to acpi_os_map_memory()")
>>>>>>>> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>>>>>>>> Cc: Ard Biesheuvel <ardb@kernel.org>
>>>>>>>> Cc: Hanjun Guo <guohanjun@huawei.com>
>>>>>>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>>>>>>>> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>>>>>> Cc: Harb Abdulhamid <harb@amperecomputing.com>
>>>>>>>>
>>>>>>>> Signed-off-by: Jia He <justin.he@arm.com>
>>>>>>> Rewrote the commit log, please take the patch below and repost
>>>>>>> it as a v3.
>>>>>>>
>>>>>>> It would still be great if Ampere can help us understand why
>>>>>>> the NormalNC attributes trigger a sync abort on the opregion
>>>>>>> before merging it.
>>>>>> To be honest, I don't think you really need an explanation from Ampere
>>>>>> here.  Mapping a part of the address space that doesn't provide memory
>>>>>> semantics with NormalNC attributes is wrong and triggering a sync
>>>>>> abort in that case is way better than silently ignoring the access.
>>>>> That's understood and that's what I explained in the revert commit
>>>>> log, no question about it.
>>>>>
>>>>> I was just asking to confirm if that's what's actually happening.
>>>>>
>>>>>> Putting my OpenBSD hat on (where we have our own ACPI OSPM
>>>>>> implementation) I must say that we always interpreted SystemMemory as
>>>>>> memory mapped IO and I think that is a logical choice as SystemIO is
>>>>>> used for (non-memory mapped) IO.  And I'd say that the ACPI OSPM code
>>>>>> should make sure that it uses properly aligned access to any Field
>>>>>> object that doesn't use AnyAcc as its access type.  Even on x86!  And
>>>>>> I'd say that AML that uses AnyAcc fields for SystemMemory OpRegions on
>>>>>> arm64 is buggy.
>>>>>>
>>>>>> But maybe relaxing this when the EFI memory map indicates that the
>>>>>> address space in question does provide memory semantics does make
>>>>>> sense.  That should defenitely be documented in the ACPI standard
>>>>>> though.
>>>>> Mapping SystemMemory Opregions as "memory" does not make sense
>>>>> at all to me. Still, that's what Linux ACPICA code does (*if*
>>>>> that's what acpi_os_map_memory() is supposed to mean).
>>>>>
>>>>> https://lore.kernel.org/linux-acpi/20210916160827.GA4525@lpieralisi
>>>> It doesn't need to do that, though, if there are good enough arguments
>>>> to change the current behavior (and the argument here is that it may
>>>> be an MMIO region, so mapping it as memory doesn't really work, but it
>>>> also may be a region in memory - there is no rule in the spec by which
>>>> SystemMemory Opregions cannot be "memory" AFAICS) and if that change
>>>> doesn't introduce regressions in the installed base.
>>>>
>>>>> Where do we go from here, to be defined, we still have a bug
>>>>> to fix after the revert is applied.
>>>>>
>>>>> drivers/acpi/sysfs.c
>>>>>
>>>>> maps BERT error regions with acpi_os_map_memory().
>>>> That mechanism is basically used for exporting ACPI tables to user
>>>> space and they are known to reside in memory.  Whether or not BERT
>>>> regions should be mapped in the same way is a good question.
>>> It is not inconceivable that BERT regions actually live in memory of
>>> the BMC that is exposed over a bus that doesn't implement memory
>>> semantics is it?
>> No, it isn't, which is why I think that mapping them as RAM may not be
>> a good idea in general.
> Should I patch acpi_data_show() to map BERT error regions (well, that's
> what acpi_data_show() is used on at the moment) as MMIO and use the
> related memcpy routine to read them then :) ?

It actually would be good to clean it up so it is clear that this is 
only used for BERT.

And then there is this question: if this is not RAM (so effectively it 
is device memory), should it be exposed directly to user space?



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2] Revert "ACPI: Add memory semantics to acpi_os_map_memory()"
  2021-09-28 17:26                 ` Rafael J. Wysocki
@ 2021-09-29 13:31                   ` Lorenzo Pieralisi
  2021-09-29 16:30                     ` Luck, Tony
  2021-11-02 15:11                   ` Lorenzo Pieralisi
  1 sibling, 1 reply; 23+ messages in thread
From: Lorenzo Pieralisi @ 2021-09-29 13:31 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Rafael J. Wysocki, Mark Kettenis, Jia He, Harb Abdulhamid,
	Will Deacon, Len Brown, Robert Moore, Erik Kaneda, Linux ARM,
	Linux Kernel Mailing List, ACPI Devel Maling List,
	open list:ACPI COMPONENT ARCHITECTURE (ACPICA),
	Ard Biesheuvel, Hanjun Guo, Catalin Marinas, tony.luck

[+Tony]

On Tue, Sep 28, 2021 at 07:26:52PM +0200, Rafael J. Wysocki wrote:
> On 9/24/2021 11:04 AM, Lorenzo Pieralisi wrote:
> > On Thu, Sep 23, 2021 at 02:54:52PM +0200, Rafael J. Wysocki wrote:
> > > On Thu, Sep 23, 2021 at 2:26 PM Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> > > > > From: "Rafael J. Wysocki" <rafael@kernel.org>
> > > > > Date: Thu, 23 Sep 2021 13:05:05 +0200
> > > > > 
> > > > > On Thu, Sep 23, 2021 at 11:40 AM Lorenzo Pieralisi
> > > > > <lorenzo.pieralisi@arm.com> wrote:
> > > > > > On Thu, Sep 23, 2021 at 01:09:58AM +0200, Mark Kettenis wrote:
> > > > > > > > Date: Wed, 22 Sep 2021 17:33:36 +0100
> > > > > > > > From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > > > > > > > 
> > > > > > > > On Fri, Sep 10, 2021 at 10:32:23PM +0800, Jia He wrote:
> > > > > > > > > This reverts commit 437b38c51162f8b87beb28a833c4d5dc85fa864e.
> > > > > > > > > 
> > > > > > > > > After this commit, a boot panic is alway hit on an Ampere EMAG server
> > > > > > > > > with call trace as follows:
> > > > > > > > >   Internal error: synchronous external abort: 96000410 [#1] SMP
> > > > > > > > >   Modules linked in:
> > > > > > > > >   CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.14.0+ #462
> > > > > > > > >   Hardware name: MiTAC RAPTOR EV-883832-X3-0001/RAPTOR, BIOS 0.14 02/22/2019
> > > > > > > > >   pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> > > > > > > > > [...snip...]
> > > > > > > > >   Call trace:
> > > > > > > > >    acpi_ex_system_memory_space_handler+0x26c/0x2c8
> > > > > > > > >    acpi_ev_address_space_dispatch+0x228/0x2c4
> > > > > > > > >    acpi_ex_access_region+0x114/0x268
> > > > > > > > >    acpi_ex_field_datum_io+0x128/0x1b8
> > > > > > > > >    acpi_ex_extract_from_field+0x14c/0x2ac
> > > > > > > > >    acpi_ex_read_data_from_field+0x190/0x1b8
> > > > > > > > >    acpi_ex_resolve_node_to_value+0x1ec/0x288
> > > > > > > > >    acpi_ex_resolve_to_value+0x250/0x274
> > > > > > > > >    acpi_ds_evaluate_name_path+0xac/0x124
> > > > > > > > >    acpi_ds_exec_end_op+0x90/0x410
> > > > > > > > >    acpi_ps_parse_loop+0x4ac/0x5d8
> > > > > > > > >    acpi_ps_parse_aml+0xe0/0x2c8
> > > > > > > > >    acpi_ps_execute_method+0x19c/0x1ac
> > > > > > > > >    acpi_ns_evaluate+0x1f8/0x26c
> > > > > > > > >    acpi_ns_init_one_device+0x104/0x140
> > > > > > > > >    acpi_ns_walk_namespace+0x158/0x1d0
> > > > > > > > >    acpi_ns_initialize_devices+0x194/0x218
> > > > > > > > >    acpi_initialize_objects+0x48/0x50
> > > > > > > > >    acpi_init+0xe0/0x498
> > > > > > > > > 
> > > > > > > > > As mentioned by Lorenzo:
> > > > > > > > >    "We are forcing memory semantics mappings to PROT_NORMAL_NC, which
> > > > > > > > >    eMAG does not like at all and I'd need to understand why. It looks
> > > > > > > > >    like the issue happen in SystemMemory Opregion handler."
> > > > > > > > > 
> > > > > > > > > Hence just revert it before everything is clear.
> > > > > > > > > 
> > > > > > > > > Fixes: 437b38c51162 ("ACPI: Add memory semantics to acpi_os_map_memory()")
> > > > > > > > > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > > > > > > > > Cc: Ard Biesheuvel <ardb@kernel.org>
> > > > > > > > > Cc: Hanjun Guo <guohanjun@huawei.com>
> > > > > > > > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > > > > > > > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > > > > > Cc: Harb Abdulhamid <harb@amperecomputing.com>
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Jia He <justin.he@arm.com>
> > > > > > > > Rewrote the commit log, please take the patch below and repost
> > > > > > > > it as a v3.
> > > > > > > > 
> > > > > > > > It would still be great if Ampere can help us understand why
> > > > > > > > the NormalNC attributes trigger a sync abort on the opregion
> > > > > > > > before merging it.
> > > > > > > To be honest, I don't think you really need an explanation from Ampere
> > > > > > > here.  Mapping a part of the address space that doesn't provide memory
> > > > > > > semantics with NormalNC attributes is wrong and triggering a sync
> > > > > > > abort in that case is way better than silently ignoring the access.
> > > > > > That's understood and that's what I explained in the revert commit
> > > > > > log, no question about it.
> > > > > > 
> > > > > > I was just asking to confirm if that's what's actually happening.
> > > > > > 
> > > > > > > Putting my OpenBSD hat on (where we have our own ACPI OSPM
> > > > > > > implementation) I must say that we always interpreted SystemMemory as
> > > > > > > memory mapped IO and I think that is a logical choice as SystemIO is
> > > > > > > used for (non-memory mapped) IO.  And I'd say that the ACPI OSPM code
> > > > > > > should make sure that it uses properly aligned access to any Field
> > > > > > > object that doesn't use AnyAcc as its access type.  Even on x86!  And
> > > > > > > I'd say that AML that uses AnyAcc fields for SystemMemory OpRegions on
> > > > > > > arm64 is buggy.
> > > > > > > 
> > > > > > > But maybe relaxing this when the EFI memory map indicates that the
> > > > > > > address space in question does provide memory semantics does make
> > > > > > > sense.  That should defenitely be documented in the ACPI standard
> > > > > > > though.
> > > > > > Mapping SystemMemory Opregions as "memory" does not make sense
> > > > > > at all to me. Still, that's what Linux ACPICA code does (*if*
> > > > > > that's what acpi_os_map_memory() is supposed to mean).
> > > > > > 
> > > > > > https://lore.kernel.org/linux-acpi/20210916160827.GA4525@lpieralisi
> > > > > It doesn't need to do that, though, if there are good enough arguments
> > > > > to change the current behavior (and the argument here is that it may
> > > > > be an MMIO region, so mapping it as memory doesn't really work, but it
> > > > > also may be a region in memory - there is no rule in the spec by which
> > > > > SystemMemory Opregions cannot be "memory" AFAICS) and if that change
> > > > > doesn't introduce regressions in the installed base.
> > > > > 
> > > > > > Where do we go from here, to be defined, we still have a bug
> > > > > > to fix after the revert is applied.
> > > > > > 
> > > > > > drivers/acpi/sysfs.c
> > > > > > 
> > > > > > maps BERT error regions with acpi_os_map_memory().
> > > > > That mechanism is basically used for exporting ACPI tables to user
> > > > > space and they are known to reside in memory.  Whether or not BERT
> > > > > regions should be mapped in the same way is a good question.
> > > > It is not inconceivable that BERT regions actually live in memory of
> > > > the BMC that is exposed over a bus that doesn't implement memory
> > > > semantics is it?
> > > No, it isn't, which is why I think that mapping them as RAM may not be
> > > a good idea in general.
> > Should I patch acpi_data_show() to map BERT error regions (well, that's
> > what acpi_data_show() is used on at the moment) as MMIO and use the
> > related memcpy routine to read them then :) ?
> 
> It actually would be good to clean it up so it is clear that this is only
> used for BERT.
> 
> And then there is this question: if this is not RAM (so effectively it is
> device memory), should it be exposed directly to user space?

Do you mean from a security standpoint ? I believe there might be users
out there so if we want to remove that sysfs entry it may be
problematic.

Maybe Tony has more insights into this than I do:

commit 7dae6326ed76 ("ACPI / sysfs: Extend ACPI sysfs to provide access to boot error region")

Thanks,
Lorenzo

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2] Revert "ACPI: Add memory semantics to acpi_os_map_memory()"
  2021-09-29 13:31                   ` Lorenzo Pieralisi
@ 2021-09-29 16:30                     ` Luck, Tony
  0 siblings, 0 replies; 23+ messages in thread
From: Luck, Tony @ 2021-09-29 16:30 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: Rafael J. Wysocki, Rafael J. Wysocki, Mark Kettenis, Jia He,
	Harb Abdulhamid, Will Deacon, Len Brown, Robert Moore,
	Erik Kaneda, Linux ARM, Linux Kernel Mailing List,
	ACPI Devel Maling List,
	open list:ACPI COMPONENT ARCHITECTURE (ACPICA),
	Ard Biesheuvel, Hanjun Guo, Catalin Marinas

On Wed, Sep 29, 2021 at 02:31:31PM +0100, Lorenzo Pieralisi wrote:
> [+Tony]
> 
> On Tue, Sep 28, 2021 at 07:26:52PM +0200, Rafael J. Wysocki wrote:
> > On 9/24/2021 11:04 AM, Lorenzo Pieralisi wrote:
> > > On Thu, Sep 23, 2021 at 02:54:52PM +0200, Rafael J. Wysocki wrote:
> > > > On Thu, Sep 23, 2021 at 2:26 PM Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> > > > > > From: "Rafael J. Wysocki" <rafael@kernel.org>
> > > > > > Date: Thu, 23 Sep 2021 13:05:05 +0200
> > > > > > 
> > > > > > On Thu, Sep 23, 2021 at 11:40 AM Lorenzo Pieralisi
> > > > > > <lorenzo.pieralisi@arm.com> wrote:
> > > > > > > On Thu, Sep 23, 2021 at 01:09:58AM +0200, Mark Kettenis wrote:
> > > > > > > > > Date: Wed, 22 Sep 2021 17:33:36 +0100
> > > > > > > > > From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > > > > > > > > 
> > > > > > > > > On Fri, Sep 10, 2021 at 10:32:23PM +0800, Jia He wrote:
> > > > > > > > > > This reverts commit 437b38c51162f8b87beb28a833c4d5dc85fa864e.
> > > > > > > > > > 
> > > > > > > > > > After this commit, a boot panic is alway hit on an Ampere EMAG server
> > > > > > > > > > with call trace as follows:
> > > > > > > > > >   Internal error: synchronous external abort: 96000410 [#1] SMP
> > > > > > > > > >   Modules linked in:
> > > > > > > > > >   CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.14.0+ #462
> > > > > > > > > >   Hardware name: MiTAC RAPTOR EV-883832-X3-0001/RAPTOR, BIOS 0.14 02/22/2019
> > > > > > > > > >   pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> > > > > > > > > > [...snip...]
> > > > > > > > > >   Call trace:
> > > > > > > > > >    acpi_ex_system_memory_space_handler+0x26c/0x2c8
> > > > > > > > > >    acpi_ev_address_space_dispatch+0x228/0x2c4
> > > > > > > > > >    acpi_ex_access_region+0x114/0x268
> > > > > > > > > >    acpi_ex_field_datum_io+0x128/0x1b8
> > > > > > > > > >    acpi_ex_extract_from_field+0x14c/0x2ac
> > > > > > > > > >    acpi_ex_read_data_from_field+0x190/0x1b8
> > > > > > > > > >    acpi_ex_resolve_node_to_value+0x1ec/0x288
> > > > > > > > > >    acpi_ex_resolve_to_value+0x250/0x274
> > > > > > > > > >    acpi_ds_evaluate_name_path+0xac/0x124
> > > > > > > > > >    acpi_ds_exec_end_op+0x90/0x410
> > > > > > > > > >    acpi_ps_parse_loop+0x4ac/0x5d8
> > > > > > > > > >    acpi_ps_parse_aml+0xe0/0x2c8
> > > > > > > > > >    acpi_ps_execute_method+0x19c/0x1ac
> > > > > > > > > >    acpi_ns_evaluate+0x1f8/0x26c
> > > > > > > > > >    acpi_ns_init_one_device+0x104/0x140
> > > > > > > > > >    acpi_ns_walk_namespace+0x158/0x1d0
> > > > > > > > > >    acpi_ns_initialize_devices+0x194/0x218
> > > > > > > > > >    acpi_initialize_objects+0x48/0x50
> > > > > > > > > >    acpi_init+0xe0/0x498
> > > > > > > > > > 
> > > > > > > > > > As mentioned by Lorenzo:
> > > > > > > > > >    "We are forcing memory semantics mappings to PROT_NORMAL_NC, which
> > > > > > > > > >    eMAG does not like at all and I'd need to understand why. It looks
> > > > > > > > > >    like the issue happen in SystemMemory Opregion handler."
> > > > > > > > > > 
> > > > > > > > > > Hence just revert it before everything is clear.
> > > > > > > > > > 
> > > > > > > > > > Fixes: 437b38c51162 ("ACPI: Add memory semantics to acpi_os_map_memory()")
> > > > > > > > > > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > > > > > > > > > Cc: Ard Biesheuvel <ardb@kernel.org>
> > > > > > > > > > Cc: Hanjun Guo <guohanjun@huawei.com>
> > > > > > > > > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > > > > > > > > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > > > > > > Cc: Harb Abdulhamid <harb@amperecomputing.com>
> > > > > > > > > > 
> > > > > > > > > > Signed-off-by: Jia He <justin.he@arm.com>
> > > > > > > > > Rewrote the commit log, please take the patch below and repost
> > > > > > > > > it as a v3.
> > > > > > > > > 
> > > > > > > > > It would still be great if Ampere can help us understand why
> > > > > > > > > the NormalNC attributes trigger a sync abort on the opregion
> > > > > > > > > before merging it.
> > > > > > > > To be honest, I don't think you really need an explanation from Ampere
> > > > > > > > here.  Mapping a part of the address space that doesn't provide memory
> > > > > > > > semantics with NormalNC attributes is wrong and triggering a sync
> > > > > > > > abort in that case is way better than silently ignoring the access.
> > > > > > > That's understood and that's what I explained in the revert commit
> > > > > > > log, no question about it.
> > > > > > > 
> > > > > > > I was just asking to confirm if that's what's actually happening.
> > > > > > > 
> > > > > > > > Putting my OpenBSD hat on (where we have our own ACPI OSPM
> > > > > > > > implementation) I must say that we always interpreted SystemMemory as
> > > > > > > > memory mapped IO and I think that is a logical choice as SystemIO is
> > > > > > > > used for (non-memory mapped) IO.  And I'd say that the ACPI OSPM code
> > > > > > > > should make sure that it uses properly aligned access to any Field
> > > > > > > > object that doesn't use AnyAcc as its access type.  Even on x86!  And
> > > > > > > > I'd say that AML that uses AnyAcc fields for SystemMemory OpRegions on
> > > > > > > > arm64 is buggy.
> > > > > > > > 
> > > > > > > > But maybe relaxing this when the EFI memory map indicates that the
> > > > > > > > address space in question does provide memory semantics does make
> > > > > > > > sense.  That should defenitely be documented in the ACPI standard
> > > > > > > > though.
> > > > > > > Mapping SystemMemory Opregions as "memory" does not make sense
> > > > > > > at all to me. Still, that's what Linux ACPICA code does (*if*
> > > > > > > that's what acpi_os_map_memory() is supposed to mean).
> > > > > > > 
> > > > > > > https://lore.kernel.org/linux-acpi/20210916160827.GA4525@lpieralisi
> > > > > > It doesn't need to do that, though, if there are good enough arguments
> > > > > > to change the current behavior (and the argument here is that it may
> > > > > > be an MMIO region, so mapping it as memory doesn't really work, but it
> > > > > > also may be a region in memory - there is no rule in the spec by which
> > > > > > SystemMemory Opregions cannot be "memory" AFAICS) and if that change
> > > > > > doesn't introduce regressions in the installed base.
> > > > > > 
> > > > > > > Where do we go from here, to be defined, we still have a bug
> > > > > > > to fix after the revert is applied.
> > > > > > > 
> > > > > > > drivers/acpi/sysfs.c
> > > > > > > 
> > > > > > > maps BERT error regions with acpi_os_map_memory().
> > > > > > That mechanism is basically used for exporting ACPI tables to user
> > > > > > space and they are known to reside in memory.  Whether or not BERT
> > > > > > regions should be mapped in the same way is a good question.
> > > > > It is not inconceivable that BERT regions actually live in memory of
> > > > > the BMC that is exposed over a bus that doesn't implement memory
> > > > > semantics is it?
> > > > No, it isn't, which is why I think that mapping them as RAM may not be
> > > > a good idea in general.
> > > Should I patch acpi_data_show() to map BERT error regions (well, that's
> > > what acpi_data_show() is used on at the moment) as MMIO and use the
> > > related memcpy routine to read them then :) ?
> > 
> > It actually would be good to clean it up so it is clear that this is only
> > used for BERT.
> > 
> > And then there is this question: if this is not RAM (so effectively it is
> > device memory), should it be exposed directly to user space?
> 
> Do you mean from a security standpoint ? I believe there might be users
> out there so if we want to remove that sysfs entry it may be
> problematic.
> 
> Maybe Tony has more insights into this than I do:
> 
> commit 7dae6326ed76 ("ACPI / sysfs: Extend ACPI sysfs to provide access to boot error region")
> 
> Thanks,
> Lorenzo

There are definelty users of /sys/firmware/acpi/tables/data/BERT.

If there is a concern about mapping the original BIOS memory to
provide this entry, then we need to allocate kernel memory and make
a copy that appears in the blob exported to /sys.

-Tony

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2] Revert "ACPI: Add memory semantics to acpi_os_map_memory()"
  2021-09-28 17:26                 ` Rafael J. Wysocki
  2021-09-29 13:31                   ` Lorenzo Pieralisi
@ 2021-11-02 15:11                   ` Lorenzo Pieralisi
  1 sibling, 0 replies; 23+ messages in thread
From: Lorenzo Pieralisi @ 2021-11-02 15:11 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Rafael J. Wysocki, Mark Kettenis, Jia He, Harb Abdulhamid,
	Will Deacon, Len Brown, Robert Moore, Erik Kaneda, Linux ARM,
	Linux Kernel Mailing List, ACPI Devel Maling List,
	open list:ACPI COMPONENT ARCHITECTURE (ACPICA),
	Ard Biesheuvel, Hanjun Guo, Catalin Marinas

On Tue, Sep 28, 2021 at 07:26:52PM +0200, Rafael J. Wysocki wrote:
> On 9/24/2021 11:04 AM, Lorenzo Pieralisi wrote:
> > On Thu, Sep 23, 2021 at 02:54:52PM +0200, Rafael J. Wysocki wrote:
> > > On Thu, Sep 23, 2021 at 2:26 PM Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> > > > > From: "Rafael J. Wysocki" <rafael@kernel.org>
> > > > > Date: Thu, 23 Sep 2021 13:05:05 +0200
> > > > > 
> > > > > On Thu, Sep 23, 2021 at 11:40 AM Lorenzo Pieralisi
> > > > > <lorenzo.pieralisi@arm.com> wrote:
> > > > > > On Thu, Sep 23, 2021 at 01:09:58AM +0200, Mark Kettenis wrote:
> > > > > > > > Date: Wed, 22 Sep 2021 17:33:36 +0100
> > > > > > > > From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > > > > > > > 
> > > > > > > > On Fri, Sep 10, 2021 at 10:32:23PM +0800, Jia He wrote:
> > > > > > > > > This reverts commit 437b38c51162f8b87beb28a833c4d5dc85fa864e.
> > > > > > > > > 
> > > > > > > > > After this commit, a boot panic is alway hit on an Ampere EMAG server
> > > > > > > > > with call trace as follows:
> > > > > > > > >   Internal error: synchronous external abort: 96000410 [#1] SMP
> > > > > > > > >   Modules linked in:
> > > > > > > > >   CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.14.0+ #462
> > > > > > > > >   Hardware name: MiTAC RAPTOR EV-883832-X3-0001/RAPTOR, BIOS 0.14 02/22/2019
> > > > > > > > >   pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> > > > > > > > > [...snip...]
> > > > > > > > >   Call trace:
> > > > > > > > >    acpi_ex_system_memory_space_handler+0x26c/0x2c8
> > > > > > > > >    acpi_ev_address_space_dispatch+0x228/0x2c4
> > > > > > > > >    acpi_ex_access_region+0x114/0x268
> > > > > > > > >    acpi_ex_field_datum_io+0x128/0x1b8
> > > > > > > > >    acpi_ex_extract_from_field+0x14c/0x2ac
> > > > > > > > >    acpi_ex_read_data_from_field+0x190/0x1b8
> > > > > > > > >    acpi_ex_resolve_node_to_value+0x1ec/0x288
> > > > > > > > >    acpi_ex_resolve_to_value+0x250/0x274
> > > > > > > > >    acpi_ds_evaluate_name_path+0xac/0x124
> > > > > > > > >    acpi_ds_exec_end_op+0x90/0x410
> > > > > > > > >    acpi_ps_parse_loop+0x4ac/0x5d8
> > > > > > > > >    acpi_ps_parse_aml+0xe0/0x2c8
> > > > > > > > >    acpi_ps_execute_method+0x19c/0x1ac
> > > > > > > > >    acpi_ns_evaluate+0x1f8/0x26c
> > > > > > > > >    acpi_ns_init_one_device+0x104/0x140
> > > > > > > > >    acpi_ns_walk_namespace+0x158/0x1d0
> > > > > > > > >    acpi_ns_initialize_devices+0x194/0x218
> > > > > > > > >    acpi_initialize_objects+0x48/0x50
> > > > > > > > >    acpi_init+0xe0/0x498
> > > > > > > > > 
> > > > > > > > > As mentioned by Lorenzo:
> > > > > > > > >    "We are forcing memory semantics mappings to PROT_NORMAL_NC, which
> > > > > > > > >    eMAG does not like at all and I'd need to understand why. It looks
> > > > > > > > >    like the issue happen in SystemMemory Opregion handler."
> > > > > > > > > 
> > > > > > > > > Hence just revert it before everything is clear.
> > > > > > > > > 
> > > > > > > > > Fixes: 437b38c51162 ("ACPI: Add memory semantics to acpi_os_map_memory()")
> > > > > > > > > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > > > > > > > > Cc: Ard Biesheuvel <ardb@kernel.org>
> > > > > > > > > Cc: Hanjun Guo <guohanjun@huawei.com>
> > > > > > > > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > > > > > > > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > > > > > Cc: Harb Abdulhamid <harb@amperecomputing.com>
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Jia He <justin.he@arm.com>
> > > > > > > > Rewrote the commit log, please take the patch below and repost
> > > > > > > > it as a v3.
> > > > > > > > 
> > > > > > > > It would still be great if Ampere can help us understand why
> > > > > > > > the NormalNC attributes trigger a sync abort on the opregion
> > > > > > > > before merging it.
> > > > > > > To be honest, I don't think you really need an explanation from Ampere
> > > > > > > here.  Mapping a part of the address space that doesn't provide memory
> > > > > > > semantics with NormalNC attributes is wrong and triggering a sync
> > > > > > > abort in that case is way better than silently ignoring the access.
> > > > > > That's understood and that's what I explained in the revert commit
> > > > > > log, no question about it.
> > > > > > 
> > > > > > I was just asking to confirm if that's what's actually happening.
> > > > > > 
> > > > > > > Putting my OpenBSD hat on (where we have our own ACPI OSPM
> > > > > > > implementation) I must say that we always interpreted SystemMemory as
> > > > > > > memory mapped IO and I think that is a logical choice as SystemIO is
> > > > > > > used for (non-memory mapped) IO.  And I'd say that the ACPI OSPM code
> > > > > > > should make sure that it uses properly aligned access to any Field
> > > > > > > object that doesn't use AnyAcc as its access type.  Even on x86!  And
> > > > > > > I'd say that AML that uses AnyAcc fields for SystemMemory OpRegions on
> > > > > > > arm64 is buggy.
> > > > > > > 
> > > > > > > But maybe relaxing this when the EFI memory map indicates that the
> > > > > > > address space in question does provide memory semantics does make
> > > > > > > sense.  That should defenitely be documented in the ACPI standard
> > > > > > > though.
> > > > > > Mapping SystemMemory Opregions as "memory" does not make sense
> > > > > > at all to me. Still, that's what Linux ACPICA code does (*if*
> > > > > > that's what acpi_os_map_memory() is supposed to mean).
> > > > > > 
> > > > > > https://lore.kernel.org/linux-acpi/20210916160827.GA4525@lpieralisi
> > > > > It doesn't need to do that, though, if there are good enough arguments
> > > > > to change the current behavior (and the argument here is that it may
> > > > > be an MMIO region, so mapping it as memory doesn't really work, but it
> > > > > also may be a region in memory - there is no rule in the spec by which
> > > > > SystemMemory Opregions cannot be "memory" AFAICS) and if that change
> > > > > doesn't introduce regressions in the installed base.
> > > > > 
> > > > > > Where do we go from here, to be defined, we still have a bug
> > > > > > to fix after the revert is applied.
> > > > > > 
> > > > > > drivers/acpi/sysfs.c
> > > > > > 
> > > > > > maps BERT error regions with acpi_os_map_memory().
> > > > > That mechanism is basically used for exporting ACPI tables to user
> > > > > space and they are known to reside in memory.  Whether or not BERT
> > > > > regions should be mapped in the same way is a good question.
> > > > It is not inconceivable that BERT regions actually live in memory of
> > > > the BMC that is exposed over a bus that doesn't implement memory
> > > > semantics is it?
> > > No, it isn't, which is why I think that mapping them as RAM may not be
> > > a good idea in general.
> > Should I patch acpi_data_show() to map BERT error regions (well, that's
> > what acpi_data_show() is used on at the moment) as MMIO and use the
> > related memcpy routine to read them then :) ?
> 
> It actually would be good to clean it up so it is clear that this is
> only used for BERT.

I could, I wonder what's best to do that though.

Maybe making acpi_table_data_init() acpi_table_bert_data_init() and
remove the infrastructure built on top of acpi_data_obj ?

I wonder whether adding a bin_attribute.read() pointer in the
acpi_data_obj struct (that would make it table specific) would be the
most elegant solution (even though the whole infrastructure has been
used only for BERT for quite a while).

Lorenzo

^ permalink raw reply	[flat|nested] 23+ messages in thread

end of thread, other threads:[~2021-11-02 15:12 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-10 12:28 [PATCH] Revert "ACPI: Add memory semantics to acpi_os_map_memory()" Jia He
2021-09-10 13:54 ` Lorenzo Pieralisi
2021-09-10 14:32 ` [PATCH v2] " Jia He
2021-09-10 17:28   ` Ard Biesheuvel
2021-09-11 10:14     ` Lorenzo Pieralisi
2021-09-16 16:08     ` Lorenzo Pieralisi
2021-09-20 17:00       ` Lorenzo Pieralisi
2021-09-20 17:32         ` Rafael J. Wysocki
2021-09-21 10:05           ` Lorenzo Pieralisi
2021-09-22 11:11             ` Ard Biesheuvel
2021-09-22 13:07               ` Lorenzo Pieralisi
2021-09-22 23:45               ` Jeremy Linton
2021-09-22 16:33   ` Lorenzo Pieralisi
2021-09-22 23:09     ` Mark Kettenis
2021-09-23  9:40       ` Lorenzo Pieralisi
2021-09-23 11:05         ` Rafael J. Wysocki
2021-09-23 12:26           ` Mark Kettenis
2021-09-23 12:54             ` Rafael J. Wysocki
2021-09-24  9:04               ` Lorenzo Pieralisi
2021-09-28 17:26                 ` Rafael J. Wysocki
2021-09-29 13:31                   ` Lorenzo Pieralisi
2021-09-29 16:30                     ` Luck, Tony
2021-11-02 15:11                   ` Lorenzo Pieralisi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).