[PATCH v2 0/7] xen/arm: Unbreak ACPI

[PATCH v2 0/7] xen/arm: Unbreak ACPI

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

Hi all,

Xen on ARM has been broken for quite a while on ACPI systems. This
series aims to fix it.

This series also introduced support for ACPI 5.1. This allows Xen to
boot on QEMU.

I have only build tested the x86 side so far.

Cheers,

Julien Grall (7):
  xen/acpi: Rework acpi_os_map_memory() and acpi_os_unmap_memory()
  xen/arm: acpi: The fixmap area should always be cleared during
    failure/unmap
  xen/arm: Check if the platform is not using ACPI before initializing
    Dom0less
  xen/arm: Introduce fw_unreserved_regions() and use it
  xen/arm: acpi: add BAD_MADT_GICC_ENTRY() macro
  xen/arm: gic-v2: acpi: Use the correct length for the GICC structure
  xen/arm: acpi: Allow Xen to boot with ACPI 5.1

 xen/arch/arm/acpi/boot.c    |  6 +--
 xen/arch/arm/acpi/lib.c     | 79 ++++++++++++++++++++++++++++++-------
 xen/arch/arm/gic-v2.c       |  5 ++-
 xen/arch/arm/gic-v3.c       |  2 +-
 xen/arch/arm/kernel.c       |  2 +-
 xen/arch/arm/setup.c        | 25 +++++++++---
 xen/arch/x86/acpi/lib.c     | 18 +++++++++
 xen/drivers/acpi/osl.c      | 34 ++++++++--------
 xen/include/asm-arm/acpi.h  |  8 ++++
 xen/include/asm-arm/setup.h |  3 +-
 xen/include/xen/acpi.h      |  1 +
 11 files changed, 139 insertions(+), 44 deletions(-)

-- 
2.17.1

[PATCH v2 1/7] xen/acpi: Rework acpi_os_map_memory() and acpi_os_unmap_memory()

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

The functions acpi_os_{un,}map_memory() are meant to be arch-agnostic
while the __acpi_os_{un,}map_memory() are meant to be arch-specific.

Currently, the former are still containing x86 specific code.

To avoid this rather strange split, the generic helpers are reworked so
they are arch-agnostic. This requires the introduction of a new helper
__acpi_os_unmap_memory() that will undo any mapping done by
__acpi_os_map_memory().

Currently, the arch-helper for unmap is basically a no-op so it only
returns whether the mapping was arch specific. But this will change
in the future.

Note that the x86 version of acpi_os_map_memory() was already able to
able the 1MB region. Hence why there is no addition of new code.

Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Rahul Singh <rahul.singh@arm.com>
Tested-by: Rahul Singh <rahul.singh@arm.com>

---
    Changes in v2:
        - Constify ptr in __acpi_unmap_table()
        - Coding style fixes
        - Fix build on arm64
        - Use PAGE_OFFSET() rather than open-coding it
        - Add Rahul's tested-by and reviewed-by
---
 xen/arch/arm/acpi/lib.c | 12 ++++++++++++
 xen/arch/x86/acpi/lib.c | 18 ++++++++++++++++++
 xen/drivers/acpi/osl.c  | 34 ++++++++++++++++++----------------
 xen/include/xen/acpi.h  |  1 +
 4 files changed, 49 insertions(+), 16 deletions(-)

diff --git a/xen/arch/arm/acpi/lib.c b/xen/arch/arm/acpi/lib.c
index 4fc6e17322c1..fcc186b03399 100644
--- a/xen/arch/arm/acpi/lib.c
+++ b/xen/arch/arm/acpi/lib.c
@@ -30,6 +30,10 @@ char *__acpi_map_table(paddr_t phys, unsigned long size)
     unsigned long base, offset, mapped_size;
     int idx;
 
+    /* No arch specific implementation after early boot */
+    if ( system_state >= SYS_STATE_boot )
+        return NULL;
+
     offset = phys & (PAGE_SIZE - 1);
     mapped_size = PAGE_SIZE - offset;
     set_fixmap(FIXMAP_ACPI_BEGIN, maddr_to_mfn(phys), PAGE_HYPERVISOR);
@@ -49,6 +53,14 @@ char *__acpi_map_table(paddr_t phys, unsigned long size)
     return ((char *) base + offset);
 }
 
+bool __acpi_unmap_table(const void *ptr, unsigned long size)
+{
+    vaddr_t vaddr = (vaddr_t)ptr;
+
+    return ((vaddr >= FIXMAP_ADDR(FIXMAP_ACPI_BEGIN)) &&
+            (vaddr < (FIXMAP_ADDR(FIXMAP_ACPI_END) + PAGE_SIZE)));
+}
+
 /* True to indicate PSCI 0.2+ is implemented */
 bool __init acpi_psci_present(void)
 {
diff --git a/xen/arch/x86/acpi/lib.c b/xen/arch/x86/acpi/lib.c
index 265b9ad81905..a22414a05c13 100644
--- a/xen/arch/x86/acpi/lib.c
+++ b/xen/arch/x86/acpi/lib.c
@@ -46,6 +46,10 @@ char *__acpi_map_table(paddr_t phys, unsigned long size)
 	if ((phys + size) <= (1 * 1024 * 1024))
 		return __va(phys);
 
+	/* No further arch specific implementation after early boot */
+	if (system_state >= SYS_STATE_boot)
+		return NULL;
+
 	offset = phys & (PAGE_SIZE - 1);
 	mapped_size = PAGE_SIZE - offset;
 	set_fixmap(FIX_ACPI_END, phys);
@@ -66,6 +70,20 @@ char *__acpi_map_table(paddr_t phys, unsigned long size)
 	return ((char *) base + offset);
 }
 
+bool __acpi_unmap_table(const void *ptr, unsigned long size)
+{
+	unsigned long vaddr = (unsigned long)ptr;
+
+	if ((vaddr >= DIRECTMAP_VIRT_START) &&
+	    (vaddr < DIRECTMAP_VIRT_END)) {
+		ASSERT(!((__pa(ptr) + size - 1) >> 20));
+		return true;
+	}
+
+	return ((vaddr >= __fix_to_virt(FIX_ACPI_END)) &&
+		(vaddr < (__fix_to_virt(FIX_ACPI_BEGIN) + PAGE_SIZE)));
+}
+
 unsigned int acpi_get_processor_id(unsigned int cpu)
 {
 	unsigned int acpiid, apicid;
diff --git a/xen/drivers/acpi/osl.c b/xen/drivers/acpi/osl.c
index 4c8bb7839eda..389505f78666 100644
--- a/xen/drivers/acpi/osl.c
+++ b/xen/drivers/acpi/osl.c
@@ -92,27 +92,29 @@ acpi_physical_address __init acpi_os_get_root_pointer(void)
 void __iomem *
 acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
 {
-	if (system_state >= SYS_STATE_boot) {
-		mfn_t mfn = _mfn(PFN_DOWN(phys));
-		unsigned int offs = phys & (PAGE_SIZE - 1);
-
-		/* The low first Mb is always mapped on x86. */
-		if (IS_ENABLED(CONFIG_X86) && !((phys + size - 1) >> 20))
-			return __va(phys);
-		return __vmap(&mfn, PFN_UP(offs + size), 1, 1,
-			      ACPI_MAP_MEM_ATTR, VMAP_DEFAULT) + offs;
-	}
-	return __acpi_map_table(phys, size);
+	void *ptr;
+	mfn_t mfn = _mfn(PFN_DOWN(phys));
+	unsigned int offs = PAGE_OFFSET(phys);
+
+	/* Try the arch specific implementation first */
+	ptr = __acpi_map_table(phys, size);
+	if (ptr)
+		return ptr;
+
+	/* No common implementation for early boot map */
+	if (unlikely(system_state < SYS_STATE_boot))
+		return NULL;
+
+	ptr = __vmap(&mfn, PFN_UP(offs + size), 1, 1,
+		     ACPI_MAP_MEM_ATTR, VMAP_DEFAULT);
+
+	return !ptr ? NULL : (ptr + offs);
 }
 
 void acpi_os_unmap_memory(void __iomem * virt, acpi_size size)
 {
-	if (IS_ENABLED(CONFIG_X86) &&
-	    (unsigned long)virt >= DIRECTMAP_VIRT_START &&
-	    (unsigned long)virt < DIRECTMAP_VIRT_END) {
-		ASSERT(!((__pa(virt) + size - 1) >> 20));
+	if (__acpi_unmap_table(virt, size))
 		return;
-	}
 
 	if (system_state >= SYS_STATE_boot)
 		vunmap((void *)((unsigned long)virt & PAGE_MASK));
diff --git a/xen/include/xen/acpi.h b/xen/include/xen/acpi.h
index c945ab05c864..21d5e9feb5ae 100644
--- a/xen/include/xen/acpi.h
+++ b/xen/include/xen/acpi.h
@@ -68,6 +68,7 @@ typedef int (*acpi_table_entry_handler) (struct acpi_subtable_header *header, co
 
 unsigned int acpi_get_processor_id (unsigned int cpu);
 char * __acpi_map_table (paddr_t phys_addr, unsigned long size);
+bool __acpi_unmap_table(const void *ptr, unsigned long size);
 int acpi_boot_init (void);
 int acpi_boot_table_init (void);
 int acpi_numa_init (void);
-- 
2.17.1

Re: [PATCH v2 1/7] xen/acpi: Rework acpi_os_map_memory() and acpi_os_unmap_memory()

[Jan Beulich]

On 23.10.2020 17:41, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> The functions acpi_os_{un,}map_memory() are meant to be arch-agnostic
> while the __acpi_os_{un,}map_memory() are meant to be arch-specific.
> 
> Currently, the former are still containing x86 specific code.
> 
> To avoid this rather strange split, the generic helpers are reworked so
> they are arch-agnostic. This requires the introduction of a new helper
> __acpi_os_unmap_memory() that will undo any mapping done by
> __acpi_os_map_memory().
> 
> Currently, the arch-helper for unmap is basically a no-op so it only
> returns whether the mapping was arch specific. But this will change
> in the future.
> 
> Note that the x86 version of acpi_os_map_memory() was already able to
> able the 1MB region. Hence why there is no addition of new code.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> Reviewed-by: Rahul Singh <rahul.singh@arm.com>
> Tested-by: Rahul Singh <rahul.singh@arm.com>

Non-Arm parts
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [PATCH v2 1/7] xen/acpi: Rework acpi_os_map_memory() and acpi_os_unmap_memory()

[Stefano Stabellini]

On Fri, 23 Oct 2020, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> The functions acpi_os_{un,}map_memory() are meant to be arch-agnostic
> while the __acpi_os_{un,}map_memory() are meant to be arch-specific.
> 
> Currently, the former are still containing x86 specific code.
> 
> To avoid this rather strange split, the generic helpers are reworked so
> they are arch-agnostic. This requires the introduction of a new helper
> __acpi_os_unmap_memory() that will undo any mapping done by
> __acpi_os_map_memory().
> 
> Currently, the arch-helper for unmap is basically a no-op so it only
> returns whether the mapping was arch specific. But this will change
> in the future.
> 
> Note that the x86 version of acpi_os_map_memory() was already able to
> able the 1MB region. Hence why there is no addition of new code.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> Reviewed-by: Rahul Singh <rahul.singh@arm.com>
> Tested-by: Rahul Singh <rahul.singh@arm.com>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>     Changes in v2:
>         - Constify ptr in __acpi_unmap_table()
>         - Coding style fixes
>         - Fix build on arm64
>         - Use PAGE_OFFSET() rather than open-coding it
>         - Add Rahul's tested-by and reviewed-by
> ---
>  xen/arch/arm/acpi/lib.c | 12 ++++++++++++
>  xen/arch/x86/acpi/lib.c | 18 ++++++++++++++++++
>  xen/drivers/acpi/osl.c  | 34 ++++++++++++++++++----------------
>  xen/include/xen/acpi.h  |  1 +
>  4 files changed, 49 insertions(+), 16 deletions(-)
> 
> diff --git a/xen/arch/arm/acpi/lib.c b/xen/arch/arm/acpi/lib.c
> index 4fc6e17322c1..fcc186b03399 100644
> --- a/xen/arch/arm/acpi/lib.c
> +++ b/xen/arch/arm/acpi/lib.c
> @@ -30,6 +30,10 @@ char *__acpi_map_table(paddr_t phys, unsigned long size)
>      unsigned long base, offset, mapped_size;
>      int idx;
>  
> +    /* No arch specific implementation after early boot */
> +    if ( system_state >= SYS_STATE_boot )
> +        return NULL;
> +
>      offset = phys & (PAGE_SIZE - 1);
>      mapped_size = PAGE_SIZE - offset;
>      set_fixmap(FIXMAP_ACPI_BEGIN, maddr_to_mfn(phys), PAGE_HYPERVISOR);
> @@ -49,6 +53,14 @@ char *__acpi_map_table(paddr_t phys, unsigned long size)
>      return ((char *) base + offset);
>  }
>  
> +bool __acpi_unmap_table(const void *ptr, unsigned long size)
> +{
> +    vaddr_t vaddr = (vaddr_t)ptr;
> +
> +    return ((vaddr >= FIXMAP_ADDR(FIXMAP_ACPI_BEGIN)) &&
> +            (vaddr < (FIXMAP_ADDR(FIXMAP_ACPI_END) + PAGE_SIZE)));
> +}
> +
>  /* True to indicate PSCI 0.2+ is implemented */
>  bool __init acpi_psci_present(void)
>  {
> diff --git a/xen/arch/x86/acpi/lib.c b/xen/arch/x86/acpi/lib.c
> index 265b9ad81905..a22414a05c13 100644
> --- a/xen/arch/x86/acpi/lib.c
> +++ b/xen/arch/x86/acpi/lib.c
> @@ -46,6 +46,10 @@ char *__acpi_map_table(paddr_t phys, unsigned long size)
>  	if ((phys + size) <= (1 * 1024 * 1024))
>  		return __va(phys);
>  
> +	/* No further arch specific implementation after early boot */
> +	if (system_state >= SYS_STATE_boot)
> +		return NULL;
> +
>  	offset = phys & (PAGE_SIZE - 1);
>  	mapped_size = PAGE_SIZE - offset;
>  	set_fixmap(FIX_ACPI_END, phys);
> @@ -66,6 +70,20 @@ char *__acpi_map_table(paddr_t phys, unsigned long size)
>  	return ((char *) base + offset);
>  }
>  
> +bool __acpi_unmap_table(const void *ptr, unsigned long size)
> +{
> +	unsigned long vaddr = (unsigned long)ptr;
> +
> +	if ((vaddr >= DIRECTMAP_VIRT_START) &&
> +	    (vaddr < DIRECTMAP_VIRT_END)) {
> +		ASSERT(!((__pa(ptr) + size - 1) >> 20));
> +		return true;
> +	}
> +
> +	return ((vaddr >= __fix_to_virt(FIX_ACPI_END)) &&
> +		(vaddr < (__fix_to_virt(FIX_ACPI_BEGIN) + PAGE_SIZE)));
> +}
> +
>  unsigned int acpi_get_processor_id(unsigned int cpu)
>  {
>  	unsigned int acpiid, apicid;
> diff --git a/xen/drivers/acpi/osl.c b/xen/drivers/acpi/osl.c
> index 4c8bb7839eda..389505f78666 100644
> --- a/xen/drivers/acpi/osl.c
> +++ b/xen/drivers/acpi/osl.c
> @@ -92,27 +92,29 @@ acpi_physical_address __init acpi_os_get_root_pointer(void)
>  void __iomem *
>  acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
>  {
> -	if (system_state >= SYS_STATE_boot) {
> -		mfn_t mfn = _mfn(PFN_DOWN(phys));
> -		unsigned int offs = phys & (PAGE_SIZE - 1);
> -
> -		/* The low first Mb is always mapped on x86. */
> -		if (IS_ENABLED(CONFIG_X86) && !((phys + size - 1) >> 20))
> -			return __va(phys);
> -		return __vmap(&mfn, PFN_UP(offs + size), 1, 1,
> -			      ACPI_MAP_MEM_ATTR, VMAP_DEFAULT) + offs;
> -	}
> -	return __acpi_map_table(phys, size);
> +	void *ptr;
> +	mfn_t mfn = _mfn(PFN_DOWN(phys));
> +	unsigned int offs = PAGE_OFFSET(phys);
> +
> +	/* Try the arch specific implementation first */
> +	ptr = __acpi_map_table(phys, size);
> +	if (ptr)
> +		return ptr;
> +
> +	/* No common implementation for early boot map */
> +	if (unlikely(system_state < SYS_STATE_boot))
> +		return NULL;
> +
> +	ptr = __vmap(&mfn, PFN_UP(offs + size), 1, 1,
> +		     ACPI_MAP_MEM_ATTR, VMAP_DEFAULT);
> +
> +	return !ptr ? NULL : (ptr + offs);
>  }
>  
>  void acpi_os_unmap_memory(void __iomem * virt, acpi_size size)
>  {
> -	if (IS_ENABLED(CONFIG_X86) &&
> -	    (unsigned long)virt >= DIRECTMAP_VIRT_START &&
> -	    (unsigned long)virt < DIRECTMAP_VIRT_END) {
> -		ASSERT(!((__pa(virt) + size - 1) >> 20));
> +	if (__acpi_unmap_table(virt, size))
>  		return;
> -	}
>  
>  	if (system_state >= SYS_STATE_boot)
>  		vunmap((void *)((unsigned long)virt & PAGE_MASK));
> diff --git a/xen/include/xen/acpi.h b/xen/include/xen/acpi.h
> index c945ab05c864..21d5e9feb5ae 100644
> --- a/xen/include/xen/acpi.h
> +++ b/xen/include/xen/acpi.h
> @@ -68,6 +68,7 @@ typedef int (*acpi_table_entry_handler) (struct acpi_subtable_header *header, co
>  
>  unsigned int acpi_get_processor_id (unsigned int cpu);
>  char * __acpi_map_table (paddr_t phys_addr, unsigned long size);
> +bool __acpi_unmap_table(const void *ptr, unsigned long size);
>  int acpi_boot_init (void);
>  int acpi_boot_table_init (void);
>  int acpi_numa_init (void);
> -- 
> 2.17.1
> 

Re: [PATCH v2 1/7] xen/acpi: Rework acpi_os_map_memory() and acpi_os_unmap_memory()

[Jan Beulich]

On 23.10.2020 17:41, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> The functions acpi_os_{un,}map_memory() are meant to be arch-agnostic
> while the __acpi_os_{un,}map_memory() are meant to be arch-specific.
> 
> Currently, the former are still containing x86 specific code.
> 
> To avoid this rather strange split, the generic helpers are reworked so
> they are arch-agnostic. This requires the introduction of a new helper
> __acpi_os_unmap_memory() that will undo any mapping done by
> __acpi_os_map_memory().
> 
> Currently, the arch-helper for unmap is basically a no-op so it only
> returns whether the mapping was arch specific. But this will change
> in the future.
> 
> Note that the x86 version of acpi_os_map_memory() was already able to
> able the 1MB region. Hence why there is no addition of new code.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> Reviewed-by: Rahul Singh <rahul.singh@arm.com>
> Tested-by: Rahul Singh <rahul.singh@arm.com>

This change breaks shutdown on x86. Either Dom0 no longer requests S5
(in which case I'd expect some data collection there to fail), or Xen
refuses the request. As a result, things go the machine_halt() path
instead. I've looked over the change again, but couldn't spot anything
yet which might explain the behavior. Yet reverting (just the non-Arm
parts, so I wouldn't have to revert multiple commits) made things
work again.

Jan

Re: [PATCH v2 1/7] xen/acpi: Rework acpi_os_map_memory() and acpi_os_unmap_memory()

[Julien Grall]

Hi Jan,

On 20/11/2020 16:03, Jan Beulich wrote:
> On 23.10.2020 17:41, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> The functions acpi_os_{un,}map_memory() are meant to be arch-agnostic
>> while the __acpi_os_{un,}map_memory() are meant to be arch-specific.
>>
>> Currently, the former are still containing x86 specific code.
>>
>> To avoid this rather strange split, the generic helpers are reworked so
>> they are arch-agnostic. This requires the introduction of a new helper
>> __acpi_os_unmap_memory() that will undo any mapping done by
>> __acpi_os_map_memory().
>>
>> Currently, the arch-helper for unmap is basically a no-op so it only
>> returns whether the mapping was arch specific. But this will change
>> in the future.
>>
>> Note that the x86 version of acpi_os_map_memory() was already able to
>> able the 1MB region. Hence why there is no addition of new code.
>>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>> Reviewed-by: Rahul Singh <rahul.singh@arm.com>
>> Tested-by: Rahul Singh <rahul.singh@arm.com>
> 
> This change breaks shutdown on x86. Either Dom0 no longer requests S5
> (in which case I'd expect some data collection there to fail), or Xen
> refuses the request. As a result, things go the machine_halt() path
> instead. I've looked over the change again, but couldn't spot anything
> yet which might explain the behavior. Yet reverting (just the non-Arm
> parts, so I wouldn't have to revert multiple commits) made things
> work again.

Thank you for the report and sorry for the breakage.

When changing the behavior of __acpi_map_table(), I failed to realize 
that some x86 code will call it directly rather than 
acpi_os_map_memory(). This is the case of acpi_fadt_parse_sleep_info() 
which detects whether ACPI can be used to put the system in sleep state.

I am tempted to require all the callers requiring to map memory to use 
the generic implementation acpi_os_{, un}map_memory().

However, AFAICT, some of the callers (such as acpi_sleep_prepare()) are 
using __acpi_map_table() because the function never failed before. By 
using the generic function, all mappings after boot will be using vmap() 
which may fail.

Would this new behavior be acceptable to you?

Cheers,

-- 
Julien Grall

Re: [PATCH v2 1/7] xen/acpi: Rework acpi_os_map_memory() and acpi_os_unmap_memory()

[Jan Beulich]

On 20.11.2020 18:44, Julien Grall wrote:
> Hi Jan,
> 
> On 20/11/2020 16:03, Jan Beulich wrote:
>> On 23.10.2020 17:41, Julien Grall wrote:
>>> From: Julien Grall <jgrall@amazon.com>
>>>
>>> The functions acpi_os_{un,}map_memory() are meant to be arch-agnostic
>>> while the __acpi_os_{un,}map_memory() are meant to be arch-specific.
>>>
>>> Currently, the former are still containing x86 specific code.
>>>
>>> To avoid this rather strange split, the generic helpers are reworked so
>>> they are arch-agnostic. This requires the introduction of a new helper
>>> __acpi_os_unmap_memory() that will undo any mapping done by
>>> __acpi_os_map_memory().
>>>
>>> Currently, the arch-helper for unmap is basically a no-op so it only
>>> returns whether the mapping was arch specific. But this will change
>>> in the future.
>>>
>>> Note that the x86 version of acpi_os_map_memory() was already able to
>>> able the 1MB region. Hence why there is no addition of new code.
>>>
>>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>>> Reviewed-by: Rahul Singh <rahul.singh@arm.com>
>>> Tested-by: Rahul Singh <rahul.singh@arm.com>
>>
>> This change breaks shutdown on x86. Either Dom0 no longer requests S5
>> (in which case I'd expect some data collection there to fail), or Xen
>> refuses the request. As a result, things go the machine_halt() path
>> instead. I've looked over the change again, but couldn't spot anything
>> yet which might explain the behavior. Yet reverting (just the non-Arm
>> parts, so I wouldn't have to revert multiple commits) made things
>> work again.
> 
> Thank you for the report and sorry for the breakage.
> 
> When changing the behavior of __acpi_map_table(), I failed to realize 
> that some x86 code will call it directly rather than 
> acpi_os_map_memory(). This is the case of acpi_fadt_parse_sleep_info() 
> which detects whether ACPI can be used to put the system in sleep state.
> 
> I am tempted to require all the callers requiring to map memory to use 
> the generic implementation acpi_os_{, un}map_memory().
> 
> However, AFAICT, some of the callers (such as acpi_sleep_prepare()) are 
> using __acpi_map_table() because the function never failed before. By 
> using the generic function, all mappings after boot will be using vmap() 
> which may fail.

The FACS mapping can (and perhaps should long have been) be switched
to acpi_os_map_memory(). The other two direct uses of the function,
however, will require more care. I'm in the process or making a
series, also noticing some further shortcomings of the FACS handling.

Jan

[PATCH v2 2/7] xen/arm: acpi: The fixmap area should always be cleared during failure/unmap

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

Commit 022387ee1ad3 "xen/arm: mm: Don't open-code Xen PT update in
{set, clear}_fixmap()" enforced that each set_fixmap() should be
paired with a clear_fixmap(). Any failure to follow the model would
result to a platform crash.

Unfortunately, the use of fixmap in the ACPI code was overlooked as it
is calling set_fixmap() but not clear_fixmap().

The function __acpi_os_map_table() is reworked so:
    - We know before the mapping whether the fixmap region is big
    enough for the mapping.
    - It will fail if the fixmap is already in use. This is not a
    change of behavior but clarifying the current expectation to avoid
    hitting a BUG().

The function __acpi_os_unmap_table() will now call clear_fixmap().

Reported-by: Wei Xu <xuwei5@hisilicon.com>
Signed-off-by: Julien Grall <jgrall@amazon.com>

---

The discussion on the original thread [1] suggested to also zap it on
x86. This is technically not necessary today, so it is left alone for
now.

I looked at making the fixmap code common but the index are inverted
between Arm and x86.

    Changes in v2:
        - Clarify the commit message
        - Fix the size computation in __acpi_unmap_table()

[1] https://lore.kernel.org/xen-devel/5E26C935.9080107@hisilicon.com/
---
 xen/arch/arm/acpi/lib.c | 73 +++++++++++++++++++++++++++++++----------
 1 file changed, 56 insertions(+), 17 deletions(-)

diff --git a/xen/arch/arm/acpi/lib.c b/xen/arch/arm/acpi/lib.c
index fcc186b03399..b755620e67b5 100644
--- a/xen/arch/arm/acpi/lib.c
+++ b/xen/arch/arm/acpi/lib.c
@@ -25,40 +25,79 @@
 #include <xen/init.h>
 #include <xen/mm.h>
 
+static bool fixmap_inuse;
+
 char *__acpi_map_table(paddr_t phys, unsigned long size)
 {
-    unsigned long base, offset, mapped_size;
-    int idx;
+    unsigned long base, offset;
+    mfn_t mfn;
+    unsigned int idx;
 
     /* No arch specific implementation after early boot */
     if ( system_state >= SYS_STATE_boot )
         return NULL;
 
     offset = phys & (PAGE_SIZE - 1);
-    mapped_size = PAGE_SIZE - offset;
-    set_fixmap(FIXMAP_ACPI_BEGIN, maddr_to_mfn(phys), PAGE_HYPERVISOR);
-    base = FIXMAP_ADDR(FIXMAP_ACPI_BEGIN);
+    base = FIXMAP_ADDR(FIXMAP_ACPI_BEGIN) + offset;
+
+    /* Check the fixmap is big enough to map the region */
+    if ( (FIXMAP_ADDR(FIXMAP_ACPI_END) + PAGE_SIZE - base) < size )
+        return NULL;
+
+    /* With the fixmap, we can only map one region at the time */
+    if ( fixmap_inuse )
+        return NULL;
 
-    /* Most cases can be covered by the below. */
+    fixmap_inuse = true;
+
+    size += offset;
+    mfn = maddr_to_mfn(phys);
     idx = FIXMAP_ACPI_BEGIN;
-    while ( mapped_size < size )
-    {
-        if ( ++idx > FIXMAP_ACPI_END )
-            return NULL;    /* cannot handle this */
-        phys += PAGE_SIZE;
-        set_fixmap(idx, maddr_to_mfn(phys), PAGE_HYPERVISOR);
-        mapped_size += PAGE_SIZE;
-    }
 
-    return ((char *) base + offset);
+    do {
+        set_fixmap(idx, mfn, PAGE_HYPERVISOR);
+        size -= min(size, (unsigned long)PAGE_SIZE);
+        mfn = mfn_add(mfn, 1);
+        idx++;
+    } while ( size > 0 );
+
+    return (char *)base;
 }
 
 bool __acpi_unmap_table(const void *ptr, unsigned long size)
 {
     vaddr_t vaddr = (vaddr_t)ptr;
+    unsigned int idx;
+
+    /* We are only handling fixmap address in the arch code */
+    if ( (vaddr < FIXMAP_ADDR(FIXMAP_ACPI_BEGIN)) ||
+         (vaddr >= FIXMAP_ADDR(FIXMAP_ACPI_END)) )
+        return false;
+
+    /*
+     * __acpi_map_table() will always return a pointer in the first page
+     * for the ACPI fixmap region. The caller is expected to free with
+     * the same address.
+     */
+    ASSERT((vaddr & PAGE_MASK) == FIXMAP_ADDR(FIXMAP_ACPI_BEGIN));
+
+    /* The region allocated fit in the ACPI fixmap region. */
+    ASSERT(size < (FIXMAP_ADDR(FIXMAP_ACPI_END) + PAGE_SIZE - vaddr));
+    ASSERT(fixmap_inuse);
+
+    fixmap_inuse = false;
+
+    size += vaddr - FIXMAP_ADDR(FIXMAP_ACPI_BEGIN);
+    idx = FIXMAP_ACPI_BEGIN;
+
+    do
+    {
+        clear_fixmap(idx);
+        size -= min(size, (unsigned long)PAGE_SIZE);
+        idx++;
+    } while ( size > 0 );
 
-    return ((vaddr >= FIXMAP_ADDR(FIXMAP_ACPI_BEGIN)) &&
-            (vaddr < (FIXMAP_ADDR(FIXMAP_ACPI_END) + PAGE_SIZE)));
+    return true;
 }
 
 /* True to indicate PSCI 0.2+ is implemented */
-- 
2.17.1

Re: [PATCH v2 2/7] xen/arm: acpi: The fixmap area should always be cleared during failure/unmap

[Stefano Stabellini]

On Fri, 23 Oct 2020, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> Commit 022387ee1ad3 "xen/arm: mm: Don't open-code Xen PT update in
> {set, clear}_fixmap()" enforced that each set_fixmap() should be
> paired with a clear_fixmap(). Any failure to follow the model would
> result to a platform crash.
> 
> Unfortunately, the use of fixmap in the ACPI code was overlooked as it
> is calling set_fixmap() but not clear_fixmap().
> 
> The function __acpi_os_map_table() is reworked so:
>     - We know before the mapping whether the fixmap region is big
>     enough for the mapping.
>     - It will fail if the fixmap is already in use. This is not a
>     change of behavior but clarifying the current expectation to avoid
>     hitting a BUG().
> 
> The function __acpi_os_unmap_table() will now call clear_fixmap().
> 
> Reported-by: Wei Xu <xuwei5@hisilicon.com>
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> ---
> 
> The discussion on the original thread [1] suggested to also zap it on
> x86. This is technically not necessary today, so it is left alone for
> now.
> 
> I looked at making the fixmap code common but the index are inverted
> between Arm and x86.
> 
>     Changes in v2:
>         - Clarify the commit message
>         - Fix the size computation in __acpi_unmap_table()
> 
> [1] https://lore.kernel.org/xen-devel/5E26C935.9080107@hisilicon.com/
> ---
>  xen/arch/arm/acpi/lib.c | 73 +++++++++++++++++++++++++++++++----------
>  1 file changed, 56 insertions(+), 17 deletions(-)
> 
> diff --git a/xen/arch/arm/acpi/lib.c b/xen/arch/arm/acpi/lib.c
> index fcc186b03399..b755620e67b5 100644
> --- a/xen/arch/arm/acpi/lib.c
> +++ b/xen/arch/arm/acpi/lib.c
> @@ -25,40 +25,79 @@
>  #include <xen/init.h>
>  #include <xen/mm.h>
>  
> +static bool fixmap_inuse;
> +
>  char *__acpi_map_table(paddr_t phys, unsigned long size)
>  {
> -    unsigned long base, offset, mapped_size;
> -    int idx;
> +    unsigned long base, offset;
> +    mfn_t mfn;
> +    unsigned int idx;
>  
>      /* No arch specific implementation after early boot */
>      if ( system_state >= SYS_STATE_boot )
>          return NULL;
>  
>      offset = phys & (PAGE_SIZE - 1);
> -    mapped_size = PAGE_SIZE - offset;
> -    set_fixmap(FIXMAP_ACPI_BEGIN, maddr_to_mfn(phys), PAGE_HYPERVISOR);
> -    base = FIXMAP_ADDR(FIXMAP_ACPI_BEGIN);
> +    base = FIXMAP_ADDR(FIXMAP_ACPI_BEGIN) + offset;
> +
> +    /* Check the fixmap is big enough to map the region */
> +    if ( (FIXMAP_ADDR(FIXMAP_ACPI_END) + PAGE_SIZE - base) < size )
> +        return NULL;
> +
> +    /* With the fixmap, we can only map one region at the time */
> +    if ( fixmap_inuse )
> +        return NULL;
>  
> -    /* Most cases can be covered by the below. */
> +    fixmap_inuse = true;
> +
> +    size += offset;
> +    mfn = maddr_to_mfn(phys);
>      idx = FIXMAP_ACPI_BEGIN;
> -    while ( mapped_size < size )
> -    {
> -        if ( ++idx > FIXMAP_ACPI_END )
> -            return NULL;    /* cannot handle this */
> -        phys += PAGE_SIZE;
> -        set_fixmap(idx, maddr_to_mfn(phys), PAGE_HYPERVISOR);
> -        mapped_size += PAGE_SIZE;
> -    }
>  
> -    return ((char *) base + offset);
> +    do {
> +        set_fixmap(idx, mfn, PAGE_HYPERVISOR);
> +        size -= min(size, (unsigned long)PAGE_SIZE);
> +        mfn = mfn_add(mfn, 1);
> +        idx++;
> +    } while ( size > 0 );
> +
> +    return (char *)base;
>  }
>  
>  bool __acpi_unmap_table(const void *ptr, unsigned long size)
>  {
>      vaddr_t vaddr = (vaddr_t)ptr;
> +    unsigned int idx;
> +
> +    /* We are only handling fixmap address in the arch code */
> +    if ( (vaddr < FIXMAP_ADDR(FIXMAP_ACPI_BEGIN)) ||
> +         (vaddr >= FIXMAP_ADDR(FIXMAP_ACPI_END)) )

Is it missing "+ PAGE_SIZE"?

   if ( (vaddr < FIXMAP_ADDR(FIXMAP_ACPI_BEGIN)) ||
        (vaddr >= FIXMAP_ADDR(FIXMAP_ACPI_END) + PAGE_SIZE) )


> +        return false;
> +
> +    /*
> +     * __acpi_map_table() will always return a pointer in the first page
> +     * for the ACPI fixmap region. The caller is expected to free with
> +     * the same address.
> +     */
> +    ASSERT((vaddr & PAGE_MASK) == FIXMAP_ADDR(FIXMAP_ACPI_BEGIN));
> +
> +    /* The region allocated fit in the ACPI fixmap region. */
> +    ASSERT(size < (FIXMAP_ADDR(FIXMAP_ACPI_END) + PAGE_SIZE - vaddr));
> +    ASSERT(fixmap_inuse);
> +
> +    fixmap_inuse = false;
> +
> +    size += vaddr - FIXMAP_ADDR(FIXMAP_ACPI_BEGIN);
> +    idx = FIXMAP_ACPI_BEGIN;
> +
> +    do
> +    {
> +        clear_fixmap(idx);
> +        size -= min(size, (unsigned long)PAGE_SIZE);
> +        idx++;
> +    } while ( size > 0 );
>  
> -    return ((vaddr >= FIXMAP_ADDR(FIXMAP_ACPI_BEGIN)) &&
> -            (vaddr < (FIXMAP_ADDR(FIXMAP_ACPI_END) + PAGE_SIZE)));
> +    return true;
>  }
>  
>  /* True to indicate PSCI 0.2+ is implemented */

Re: [PATCH v2 2/7] xen/arm: acpi: The fixmap area should always be cleared during failure/unmap

[Julien Grall]

Hi Stefano,

On 24/10/2020 01:16, Stefano Stabellini wrote:
> On Fri, 23 Oct 2020, Julien Grall wrote:
>>   bool __acpi_unmap_table(const void *ptr, unsigned long size)
>>   {
>>       vaddr_t vaddr = (vaddr_t)ptr;
>> +    unsigned int idx;
>> +
>> +    /* We are only handling fixmap address in the arch code */
>> +    if ( (vaddr < FIXMAP_ADDR(FIXMAP_ACPI_BEGIN)) ||
>> +         (vaddr >= FIXMAP_ADDR(FIXMAP_ACPI_END)) )
> 
> Is it missing "+ PAGE_SIZE"?
> 
>     if ( (vaddr < FIXMAP_ADDR(FIXMAP_ACPI_BEGIN)) ||
>          (vaddr >= FIXMAP_ADDR(FIXMAP_ACPI_END) + PAGE_SIZE) )

Yes it should be + PAGE_SIZE. Do you mind if I fix it on commit?

Cheers,

-- 
Julien Grall

Re: [PATCH v2 2/7] xen/arm: acpi: The fixmap area should always be cleared during failure/unmap

[Stefano Stabellini]

On Fri, 30 Oct 2020, Julien Grall wrote:
> Hi Stefano,
> 
> On 24/10/2020 01:16, Stefano Stabellini wrote:
> > On Fri, 23 Oct 2020, Julien Grall wrote:
> > >   bool __acpi_unmap_table(const void *ptr, unsigned long size)
> > >   {
> > >       vaddr_t vaddr = (vaddr_t)ptr;
> > > +    unsigned int idx;
> > > +
> > > +    /* We are only handling fixmap address in the arch code */
> > > +    if ( (vaddr < FIXMAP_ADDR(FIXMAP_ACPI_BEGIN)) ||
> > > +         (vaddr >= FIXMAP_ADDR(FIXMAP_ACPI_END)) )
> > 
> > Is it missing "+ PAGE_SIZE"?
> > 
> >     if ( (vaddr < FIXMAP_ADDR(FIXMAP_ACPI_BEGIN)) ||
> >          (vaddr >= FIXMAP_ADDR(FIXMAP_ACPI_END) + PAGE_SIZE) )
> 
> Yes it should be + PAGE_SIZE. Do you mind if I fix it on commit?

No, I don't mind. Go ahead.

Re: [PATCH v2 2/7] xen/arm: acpi: The fixmap area should always be cleared during failure/unmap

[Julien Grall]

Hi,

On 30/10/2020 18:28, Stefano Stabellini wrote:
> On Fri, 30 Oct 2020, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 24/10/2020 01:16, Stefano Stabellini wrote:
>>> On Fri, 23 Oct 2020, Julien Grall wrote:
>>>>    bool __acpi_unmap_table(const void *ptr, unsigned long size)
>>>>    {
>>>>        vaddr_t vaddr = (vaddr_t)ptr;
>>>> +    unsigned int idx;
>>>> +
>>>> +    /* We are only handling fixmap address in the arch code */
>>>> +    if ( (vaddr < FIXMAP_ADDR(FIXMAP_ACPI_BEGIN)) ||
>>>> +         (vaddr >= FIXMAP_ADDR(FIXMAP_ACPI_END)) )
>>>
>>> Is it missing "+ PAGE_SIZE"?
>>>
>>>      if ( (vaddr < FIXMAP_ADDR(FIXMAP_ACPI_BEGIN)) ||
>>>           (vaddr >= FIXMAP_ADDR(FIXMAP_ACPI_END) + PAGE_SIZE) )
>>
>> Yes it should be + PAGE_SIZE. Do you mind if I fix it on commit?
> 
> No, I don't mind. Go ahead.

I technically don't have a ack for this patch. Can I consider this as a 
Acked-by? :)

Cheers,

-- 
Julien Grall

Re: [PATCH v2 2/7] xen/arm: acpi: The fixmap area should always be cleared during failure/unmap

[Stefano Stabellini]

On Fri, 30 Oct 2020, Julien Grall wrote:
> Hi,
> 
> On 30/10/2020 18:28, Stefano Stabellini wrote:
> > On Fri, 30 Oct 2020, Julien Grall wrote:
> > > Hi Stefano,
> > > 
> > > On 24/10/2020 01:16, Stefano Stabellini wrote:
> > > > On Fri, 23 Oct 2020, Julien Grall wrote:
> > > > >    bool __acpi_unmap_table(const void *ptr, unsigned long size)
> > > > >    {
> > > > >        vaddr_t vaddr = (vaddr_t)ptr;
> > > > > +    unsigned int idx;
> > > > > +
> > > > > +    /* We are only handling fixmap address in the arch code */
> > > > > +    if ( (vaddr < FIXMAP_ADDR(FIXMAP_ACPI_BEGIN)) ||
> > > > > +         (vaddr >= FIXMAP_ADDR(FIXMAP_ACPI_END)) )
> > > > 
> > > > Is it missing "+ PAGE_SIZE"?
> > > > 
> > > >      if ( (vaddr < FIXMAP_ADDR(FIXMAP_ACPI_BEGIN)) ||
> > > >           (vaddr >= FIXMAP_ADDR(FIXMAP_ACPI_END) + PAGE_SIZE) )
> > > 
> > > Yes it should be + PAGE_SIZE. Do you mind if I fix it on commit?
> > 
> > No, I don't mind. Go ahead.
> 
> I technically don't have a ack for this patch. Can I consider this as a
> Acked-by? :)

Acked-by: Stefano Stabellini <sstabellini@kernel.org>

[PATCH v2 3/7] xen/arm: Check if the platform is not using ACPI before initializing Dom0less

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

Dom0less requires a device-tree. However, since commit 6e3e77120378
"xen/arm: setup: Relocate the Device-Tree later on in the boot", the
device-tree will not get unflatten when using ACPI.

This will lead to a crash during boot.

Given the complexity to setup dom0less with ACPI (for instance how to
assign device?), we should skip any code related to Dom0less when using
ACPI.

Signed-off-by: Julien Grall <jgrall@amazon.com>
Tested-by: Rahul Singh <rahul.singh@arm.com>
Reviewed-by: Rahul Singh <rahul.singh@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

---
    Changes in v2:
        - Add Rahul's tested-by and reviewed-by
        - Add Stefano's reviewed-by
---
 xen/arch/arm/setup.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index f16b33fa87a2..35e5bee04efa 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -987,7 +987,8 @@ void __init start_xen(unsigned long boot_phys_offset,
 
     system_state = SYS_STATE_active;
 
-    create_domUs();
+    if ( acpi_disabled )
+        create_domUs();
 
     domain_unpause_by_systemcontroller(dom0);
 
-- 
2.17.1

[PATCH v2 4/7] xen/arm: Introduce fw_unreserved_regions() and use it

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

Since commit 6e3e77120378 "xen/arm: setup: Relocate the Device-Tree
later on in the boot", the device-tree will not be kept mapped when
using ACPI.

However, a few places are calling dt_unreserved_regions() which expects
a valid DT. This will lead to a crash.

As the DT should not be used for ACPI (other than for detecting the
modules), a new function fw_unreserved_regions() is introduced.

It will behave the same way on DT system. On ACPI system, it will
unreserve the whole region.

Take the opportunity to clarify that bootinfo.reserved_mem is only used
when booting using Device-Tree.

Signed-off-by: Julien Grall <jgrall@amazon.com>

---

Is there any region we should exclude on ACPI?

    Changes in v2:
        - Add a comment on top of bootinfo.reserved_mem.
---
 xen/arch/arm/kernel.c       |  2 +-
 xen/arch/arm/setup.c        | 22 +++++++++++++++++-----
 xen/include/asm-arm/setup.h |  3 ++-
 3 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index 032923853f2c..ab78689ed2a6 100644
--- a/xen/arch/arm/kernel.c
+++ b/xen/arch/arm/kernel.c
@@ -307,7 +307,7 @@ static __init int kernel_decompress(struct bootmodule *mod)
      * Free the original kernel, update the pointers to the
      * decompressed kernel
      */
-    dt_unreserved_regions(addr, addr + size, init_domheap_pages, 0);
+    fw_unreserved_regions(addr, addr + size, init_domheap_pages, 0);
 
     return 0;
 }
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 35e5bee04efa..7fcff9af2a7e 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -196,8 +196,9 @@ static void __init processor_id(void)
     processor_setup();
 }
 
-void __init dt_unreserved_regions(paddr_t s, paddr_t e,
-                                  void (*cb)(paddr_t, paddr_t), int first)
+static void __init dt_unreserved_regions(paddr_t s, paddr_t e,
+                                         void (*cb)(paddr_t, paddr_t),
+                                         int first)
 {
     int i, nr = fdt_num_mem_rsv(device_tree_flattened);
 
@@ -244,6 +245,17 @@ void __init dt_unreserved_regions(paddr_t s, paddr_t e,
     cb(s, e);
 }
 
+void __init fw_unreserved_regions(paddr_t s, paddr_t e,
+                                  void (*cb)(paddr_t, paddr_t), int first)
+{
+    if ( acpi_disabled )
+        dt_unreserved_regions(s, e, cb, first);
+    else
+        cb(s, e);
+}
+
+
+
 struct bootmodule __init *add_boot_module(bootmodule_kind kind,
                                           paddr_t start, paddr_t size,
                                           bool domU)
@@ -405,7 +417,7 @@ void __init discard_initial_modules(void)
              !mfn_valid(maddr_to_mfn(e)) )
             continue;
 
-        dt_unreserved_regions(s, e, init_domheap_pages, 0);
+        fw_unreserved_regions(s, e, init_domheap_pages, 0);
     }
 
     mi->nr_mods = 0;
@@ -712,7 +724,7 @@ static void __init setup_mm(void)
                 n = mfn_to_maddr(mfn_add(xenheap_mfn_start, xenheap_pages));
             }
 
-            dt_unreserved_regions(s, e, init_boot_pages, 0);
+            fw_unreserved_regions(s, e, init_boot_pages, 0);
 
             s = n;
         }
@@ -765,7 +777,7 @@ static void __init setup_mm(void)
             if ( e > bank_end )
                 e = bank_end;
 
-            dt_unreserved_regions(s, e, init_boot_pages, 0);
+            fw_unreserved_regions(s, e, init_boot_pages, 0);
             s = n;
         }
     }
diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h
index 2f8f24e286ed..28bf622aa196 100644
--- a/xen/include/asm-arm/setup.h
+++ b/xen/include/asm-arm/setup.h
@@ -67,6 +67,7 @@ struct bootcmdlines {
 
 struct bootinfo {
     struct meminfo mem;
+    /* The reserved regions are only used when booting using Device-Tree */
     struct meminfo reserved_mem;
     struct bootmodules modules;
     struct bootcmdlines cmdlines;
@@ -96,7 +97,7 @@ int construct_dom0(struct domain *d);
 void create_domUs(void);
 
 void discard_initial_modules(void);
-void dt_unreserved_regions(paddr_t s, paddr_t e,
+void fw_unreserved_regions(paddr_t s, paddr_t e,
                            void (*cb)(paddr_t, paddr_t), int first);
 
 size_t boot_fdt_info(const void *fdt, paddr_t paddr);
-- 
2.17.1

Re: [PATCH v2 4/7] xen/arm: Introduce fw_unreserved_regions() and use it

[Stefano Stabellini]

On Fri, 23 Oct 2020, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> Since commit 6e3e77120378 "xen/arm: setup: Relocate the Device-Tree
> later on in the boot", the device-tree will not be kept mapped when
> using ACPI.
> 
> However, a few places are calling dt_unreserved_regions() which expects
> a valid DT. This will lead to a crash.
> 
> As the DT should not be used for ACPI (other than for detecting the
> modules), a new function fw_unreserved_regions() is introduced.
> 
> It will behave the same way on DT system. On ACPI system, it will
> unreserve the whole region.
> 
> Take the opportunity to clarify that bootinfo.reserved_mem is only used
> when booting using Device-Tree.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
> 
> Is there any region we should exclude on ACPI?
> 
>     Changes in v2:
>         - Add a comment on top of bootinfo.reserved_mem.
> ---
>  xen/arch/arm/kernel.c       |  2 +-
>  xen/arch/arm/setup.c        | 22 +++++++++++++++++-----
>  xen/include/asm-arm/setup.h |  3 ++-
>  3 files changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
> index 032923853f2c..ab78689ed2a6 100644
> --- a/xen/arch/arm/kernel.c
> +++ b/xen/arch/arm/kernel.c
> @@ -307,7 +307,7 @@ static __init int kernel_decompress(struct bootmodule *mod)
>       * Free the original kernel, update the pointers to the
>       * decompressed kernel
>       */
> -    dt_unreserved_regions(addr, addr + size, init_domheap_pages, 0);
> +    fw_unreserved_regions(addr, addr + size, init_domheap_pages, 0);
>  
>      return 0;
>  }
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 35e5bee04efa..7fcff9af2a7e 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -196,8 +196,9 @@ static void __init processor_id(void)
>      processor_setup();
>  }
>  
> -void __init dt_unreserved_regions(paddr_t s, paddr_t e,
> -                                  void (*cb)(paddr_t, paddr_t), int first)
> +static void __init dt_unreserved_regions(paddr_t s, paddr_t e,
> +                                         void (*cb)(paddr_t, paddr_t),
> +                                         int first)
>  {
>      int i, nr = fdt_num_mem_rsv(device_tree_flattened);
>  
> @@ -244,6 +245,17 @@ void __init dt_unreserved_regions(paddr_t s, paddr_t e,
>      cb(s, e);
>  }
>  
> +void __init fw_unreserved_regions(paddr_t s, paddr_t e,
> +                                  void (*cb)(paddr_t, paddr_t), int first)
> +{
> +    if ( acpi_disabled )
> +        dt_unreserved_regions(s, e, cb, first);
> +    else
> +        cb(s, e);
> +}
> +
> +
> +
>  struct bootmodule __init *add_boot_module(bootmodule_kind kind,
>                                            paddr_t start, paddr_t size,
>                                            bool domU)
> @@ -405,7 +417,7 @@ void __init discard_initial_modules(void)
>               !mfn_valid(maddr_to_mfn(e)) )
>              continue;
>  
> -        dt_unreserved_regions(s, e, init_domheap_pages, 0);
> +        fw_unreserved_regions(s, e, init_domheap_pages, 0);
>      }
>  
>      mi->nr_mods = 0;
> @@ -712,7 +724,7 @@ static void __init setup_mm(void)
>                  n = mfn_to_maddr(mfn_add(xenheap_mfn_start, xenheap_pages));
>              }
>  
> -            dt_unreserved_regions(s, e, init_boot_pages, 0);
> +            fw_unreserved_regions(s, e, init_boot_pages, 0);
>  
>              s = n;
>          }
> @@ -765,7 +777,7 @@ static void __init setup_mm(void)
>              if ( e > bank_end )
>                  e = bank_end;
>  
> -            dt_unreserved_regions(s, e, init_boot_pages, 0);
> +            fw_unreserved_regions(s, e, init_boot_pages, 0);
>              s = n;
>          }
>      }
> diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h
> index 2f8f24e286ed..28bf622aa196 100644
> --- a/xen/include/asm-arm/setup.h
> +++ b/xen/include/asm-arm/setup.h
> @@ -67,6 +67,7 @@ struct bootcmdlines {
>  
>  struct bootinfo {
>      struct meminfo mem;
> +    /* The reserved regions are only used when booting using Device-Tree */
>      struct meminfo reserved_mem;
>      struct bootmodules modules;
>      struct bootcmdlines cmdlines;
> @@ -96,7 +97,7 @@ int construct_dom0(struct domain *d);
>  void create_domUs(void);
>  
>  void discard_initial_modules(void);
> -void dt_unreserved_regions(paddr_t s, paddr_t e,
> +void fw_unreserved_regions(paddr_t s, paddr_t e,
>                             void (*cb)(paddr_t, paddr_t), int first);
>  
>  size_t boot_fdt_info(const void *fdt, paddr_t paddr);
> -- 
> 2.17.1
> 

[PATCH v2 5/7] xen/arm: acpi: add BAD_MADT_GICC_ENTRY() macro

[Julien Grall]

From: Julien Grall <julien.grall@arm.com>

Imported from Linux commit b6cfb277378ef831c0fa84bcff5049307294adc6:

    The BAD_MADT_ENTRY() macro is designed to work for all of the subtables
    of the MADT.  In the ACPI 5.1 version of the spec, the struct for the
    GICC subtable (struct acpi_madt_generic_interrupt) is 76 bytes long; in
    ACPI 6.0, the struct is 80 bytes long.  But, there is only one definition
    in ACPICA for this struct -- and that is the 6.0 version.  Hence, when
    BAD_MADT_ENTRY() compares the struct size to the length in the GICC
    subtable, it fails if 5.1 structs are in use, and there are systems in
    the wild that have them.

    This patch adds the BAD_MADT_GICC_ENTRY() that checks the GICC subtable
    only, accounting for the difference in specification versions that are
    possible.  The BAD_MADT_ENTRY() will continue to work as is for all other
    MADT subtables.

    This code is being added to an arm64 header file since that is currently
    the only architecture using the GICC subtable of the MADT.  As a GIC is
    specific to ARM, it is also unlikely the subtable will be used elsewhere.

    Fixes: aeb823bbacc2 ("ACPICA: ACPI 6.0: Add changes for FADT table.")
    Signed-off-by: Al Stone <al.stone@linaro.org>
    Acked-by: Will Deacon <will.deacon@arm.com>
    Acked-by: "Rafael J. Wysocki" <rjw@rjwysocki.net>
    [catalin.marinas@arm.com: extra brackets around macro arguments]
    Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>

Signed-off-by: Julien Grall <julien.grall@arm.com>
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
Signed-off-by: Julien Grall <jgrall@amazon.com>

---

Changes in v2:
    - Patch added

We may want to consider to also import:

commit 9eb1c92b47c73249465d388eaa394fe436a3b489
Author: Jeremy Linton <jeremy.linton@arm.com>
Date:   Tue Nov 27 17:59:12 2018 +0000

    arm64: acpi: Prepare for longer MADTs

    The BAD_MADT_GICC_ENTRY check is a little too strict because
    it rejects MADT entries that don't match the currently known
    lengths. We should remove this restriction to avoid problems
    if the table length changes. Future code which might depend on
    additional fields should be written to validate those fields
    before using them, rather than trying to globally check
    known MADT version lengths.

    Link: https://lkml.kernel.org/r/20181012192937.3819951-1-jeremy.linton@arm.com
    Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
    [lorenzo.pieralisi@arm.com: added MADT macro comments]
    Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
    Acked-by: Sudeep Holla <sudeep.holla@arm.com>
    Cc: Will Deacon <will.deacon@arm.com>
    Cc: Catalin Marinas <catalin.marinas@arm.com>
    Cc: Al Stone <ahs3@redhat.com>
    Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
    Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 xen/include/asm-arm/acpi.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/xen/include/asm-arm/acpi.h b/xen/include/asm-arm/acpi.h
index 50340281a917..b52ae2d6ef72 100644
--- a/xen/include/asm-arm/acpi.h
+++ b/xen/include/asm-arm/acpi.h
@@ -54,6 +54,14 @@ void acpi_smp_init_cpus(void);
  */
 paddr_t acpi_get_table_offset(struct membank tbl_add[], EFI_MEM_RES index);
 
+/* Macros for consistency checks of the GICC subtable of MADT */
+#define ACPI_MADT_GICC_LENGTH	\
+    (acpi_gbl_FADT.header.revision < 6 ? 76 : 80)
+
+#define BAD_MADT_GICC_ENTRY(entry, end)						\
+    (!(entry) || (unsigned long)(entry) + sizeof(*(entry)) > (end) ||	\
+     (entry)->header.length != ACPI_MADT_GICC_LENGTH)
+
 #ifdef CONFIG_ACPI
 extern bool acpi_disabled;
 /* Basic configuration for ACPI */
-- 
2.17.1

Re: [PATCH v2 5/7] xen/arm: acpi: add BAD_MADT_GICC_ENTRY() macro

[Stefano Stabellini]

On Fri, 23 Oct 2020, Julien Grall wrote:
> From: Julien Grall <julien.grall@arm.com>
> 
> Imported from Linux commit b6cfb277378ef831c0fa84bcff5049307294adc6:
> 
>     The BAD_MADT_ENTRY() macro is designed to work for all of the subtables
>     of the MADT.  In the ACPI 5.1 version of the spec, the struct for the
>     GICC subtable (struct acpi_madt_generic_interrupt) is 76 bytes long; in
>     ACPI 6.0, the struct is 80 bytes long.  But, there is only one definition
>     in ACPICA for this struct -- and that is the 6.0 version.  Hence, when
>     BAD_MADT_ENTRY() compares the struct size to the length in the GICC
>     subtable, it fails if 5.1 structs are in use, and there are systems in
>     the wild that have them.
> 
>     This patch adds the BAD_MADT_GICC_ENTRY() that checks the GICC subtable
>     only, accounting for the difference in specification versions that are
>     possible.  The BAD_MADT_ENTRY() will continue to work as is for all other
>     MADT subtables.
> 
>     This code is being added to an arm64 header file since that is currently
>     the only architecture using the GICC subtable of the MADT.  As a GIC is
>     specific to ARM, it is also unlikely the subtable will be used elsewhere.
> 
>     Fixes: aeb823bbacc2 ("ACPICA: ACPI 6.0: Add changes for FADT table.")
>     Signed-off-by: Al Stone <al.stone@linaro.org>
>     Acked-by: Will Deacon <will.deacon@arm.com>
>     Acked-by: "Rafael J. Wysocki" <rjw@rjwysocki.net>
>     [catalin.marinas@arm.com: extra brackets around macro arguments]
>     Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> Signed-off-by: Julien Grall <jgrall@amazon.com>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
> 
> Changes in v2:
>     - Patch added
> 
> We may want to consider to also import:
> 
> commit 9eb1c92b47c73249465d388eaa394fe436a3b489
> Author: Jeremy Linton <jeremy.linton@arm.com>
> Date:   Tue Nov 27 17:59:12 2018 +0000

Sure


>     arm64: acpi: Prepare for longer MADTs
> 
>     The BAD_MADT_GICC_ENTRY check is a little too strict because
>     it rejects MADT entries that don't match the currently known
>     lengths. We should remove this restriction to avoid problems
>     if the table length changes. Future code which might depend on
>     additional fields should be written to validate those fields
>     before using them, rather than trying to globally check
>     known MADT version lengths.
> 
>     Link: https://lkml.kernel.org/r/20181012192937.3819951-1-jeremy.linton@arm.com
>     Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
>     [lorenzo.pieralisi@arm.com: added MADT macro comments]
>     Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>     Acked-by: Sudeep Holla <sudeep.holla@arm.com>
>     Cc: Will Deacon <will.deacon@arm.com>
>     Cc: Catalin Marinas <catalin.marinas@arm.com>
>     Cc: Al Stone <ahs3@redhat.com>
>     Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
>     Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
>  xen/include/asm-arm/acpi.h | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/xen/include/asm-arm/acpi.h b/xen/include/asm-arm/acpi.h
> index 50340281a917..b52ae2d6ef72 100644
> --- a/xen/include/asm-arm/acpi.h
> +++ b/xen/include/asm-arm/acpi.h
> @@ -54,6 +54,14 @@ void acpi_smp_init_cpus(void);
>   */
>  paddr_t acpi_get_table_offset(struct membank tbl_add[], EFI_MEM_RES index);
>  
> +/* Macros for consistency checks of the GICC subtable of MADT */
> +#define ACPI_MADT_GICC_LENGTH	\
> +    (acpi_gbl_FADT.header.revision < 6 ? 76 : 80)
> +
> +#define BAD_MADT_GICC_ENTRY(entry, end)						\
> +    (!(entry) || (unsigned long)(entry) + sizeof(*(entry)) > (end) ||	\
> +     (entry)->header.length != ACPI_MADT_GICC_LENGTH)
> +
>  #ifdef CONFIG_ACPI
>  extern bool acpi_disabled;
>  /* Basic configuration for ACPI */
> -- 
> 2.17.1
> 

Re: [PATCH v2 5/7] xen/arm: acpi: add BAD_MADT_GICC_ENTRY() macro

[Julien Grall]

Hi Stefano,

On 24/10/2020 01:32, Stefano Stabellini wrote:
> On Fri, 23 Oct 2020, Julien Grall wrote:
>> From: Julien Grall <julien.grall@arm.com>
>>
>> Imported from Linux commit b6cfb277378ef831c0fa84bcff5049307294adc6:
>>
>>      The BAD_MADT_ENTRY() macro is designed to work for all of the subtables
>>      of the MADT.  In the ACPI 5.1 version of the spec, the struct for the
>>      GICC subtable (struct acpi_madt_generic_interrupt) is 76 bytes long; in
>>      ACPI 6.0, the struct is 80 bytes long.  But, there is only one definition
>>      in ACPICA for this struct -- and that is the 6.0 version.  Hence, when
>>      BAD_MADT_ENTRY() compares the struct size to the length in the GICC
>>      subtable, it fails if 5.1 structs are in use, and there are systems in
>>      the wild that have them.
>>
>>      This patch adds the BAD_MADT_GICC_ENTRY() that checks the GICC subtable
>>      only, accounting for the difference in specification versions that are
>>      possible.  The BAD_MADT_ENTRY() will continue to work as is for all other
>>      MADT subtables.
>>
>>      This code is being added to an arm64 header file since that is currently
>>      the only architecture using the GICC subtable of the MADT.  As a GIC is
>>      specific to ARM, it is also unlikely the subtable will be used elsewhere.
>>
>>      Fixes: aeb823bbacc2 ("ACPICA: ACPI 6.0: Add changes for FADT table.")
>>      Signed-off-by: Al Stone <al.stone@linaro.org>
>>      Acked-by: Will Deacon <will.deacon@arm.com>
>>      Acked-by: "Rafael J. Wysocki" <rjw@rjwysocki.net>
>>      [catalin.marinas@arm.com: extra brackets around macro arguments]
>>      Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> Acked-by: Stefano Stabellini <sstabellini@kernel.org>

Thanks!

>> ---
>>
>> Changes in v2:
>>      - Patch added
>>
>> We may want to consider to also import:
>>
>> commit 9eb1c92b47c73249465d388eaa394fe436a3b489
>> Author: Jeremy Linton <jeremy.linton@arm.com>
>> Date:   Tue Nov 27 17:59:12 2018 +0000
> 
> Sure

I will add it in my todo list of further improvement.

Cheers,

-- 
Julien Grall

[PATCH v2 6/7] xen/arm: gic-v2: acpi: Use the correct length for the GICC structure

[Julien Grall]

From: Julien Grall <julien.grall@arm.com>

The length of the GICC structure in the MADT ACPI table differs between
version 5.1 and 6.0, although there are no other relevant differences.

Use the BAD_MADT_GICC_ENTRY macro, which was specifically designed to
overcome this issue.

Signed-off-by: Julien Grall <julien.grall@arm.com>
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
Signed-off-by: Julien Grall <jgrall@amazon.com>

---
    Changes in v2:
        - Patch added
---
 xen/arch/arm/acpi/boot.c | 2 +-
 xen/arch/arm/gic-v2.c    | 5 +++--
 xen/arch/arm/gic-v3.c    | 2 +-
 3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c
index 30e4bd1bc5a7..55c3e5cbc834 100644
--- a/xen/arch/arm/acpi/boot.c
+++ b/xen/arch/arm/acpi/boot.c
@@ -131,7 +131,7 @@ acpi_parse_gic_cpu_interface(struct acpi_subtable_header *header,
     struct acpi_madt_generic_interrupt *processor =
                container_of(header, struct acpi_madt_generic_interrupt, header);
 
-    if ( BAD_MADT_ENTRY(processor, end) )
+    if ( BAD_MADT_GICC_ENTRY(processor, end) )
         return -EINVAL;
 
     acpi_table_print_madt_entry(header);
diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 0f747538dbcd..0e5f23201974 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -1136,7 +1136,8 @@ static int gicv2_make_hwdom_madt(const struct domain *d, u32 offset)
 
     host_gicc = container_of(header, struct acpi_madt_generic_interrupt,
                              header);
-    size = sizeof(struct acpi_madt_generic_interrupt);
+
+    size = ACPI_MADT_GICC_LENGTH;
     /* Add Generic Interrupt */
     for ( i = 0; i < d->max_vcpus; i++ )
     {
@@ -1165,7 +1166,7 @@ gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
     struct acpi_madt_generic_interrupt *processor =
                container_of(header, struct acpi_madt_generic_interrupt, header);
 
-    if ( BAD_MADT_ENTRY(processor, end) )
+    if ( BAD_MADT_GICC_ENTRY(processor, end) )
         return -EINVAL;
 
     /* Read from APIC table and fill up the GIC variables */
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 0f6cbf6224e9..ce202402c0ed 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1558,7 +1558,7 @@ gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
     struct acpi_madt_generic_interrupt *processor =
                container_of(header, struct acpi_madt_generic_interrupt, header);
 
-    if ( BAD_MADT_ENTRY(processor, end) )
+    if ( BAD_MADT_GICC_ENTRY(processor, end) )
         return -EINVAL;
 
     /* Read from APIC table and fill up the GIC variables */
-- 
2.17.1

Re: [PATCH v2 6/7] xen/arm: gic-v2: acpi: Use the correct length for the GICC structure

[Stefano Stabellini]

On Fri, 23 Oct 2020, Julien Grall wrote:
> From: Julien Grall <julien.grall@arm.com>
> 
> The length of the GICC structure in the MADT ACPI table differs between
> version 5.1 and 6.0, although there are no other relevant differences.
> 
> Use the BAD_MADT_GICC_ENTRY macro, which was specifically designed to
> overcome this issue.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> Signed-off-by: Julien Grall <jgrall@amazon.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>     Changes in v2:
>         - Patch added
> ---
>  xen/arch/arm/acpi/boot.c | 2 +-
>  xen/arch/arm/gic-v2.c    | 5 +++--
>  xen/arch/arm/gic-v3.c    | 2 +-
>  3 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c
> index 30e4bd1bc5a7..55c3e5cbc834 100644
> --- a/xen/arch/arm/acpi/boot.c
> +++ b/xen/arch/arm/acpi/boot.c
> @@ -131,7 +131,7 @@ acpi_parse_gic_cpu_interface(struct acpi_subtable_header *header,
>      struct acpi_madt_generic_interrupt *processor =
>                 container_of(header, struct acpi_madt_generic_interrupt, header);
>  
> -    if ( BAD_MADT_ENTRY(processor, end) )
> +    if ( BAD_MADT_GICC_ENTRY(processor, end) )
>          return -EINVAL;
>  
>      acpi_table_print_madt_entry(header);
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index 0f747538dbcd..0e5f23201974 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -1136,7 +1136,8 @@ static int gicv2_make_hwdom_madt(const struct domain *d, u32 offset)
>  
>      host_gicc = container_of(header, struct acpi_madt_generic_interrupt,
>                               header);
> -    size = sizeof(struct acpi_madt_generic_interrupt);
> +
> +    size = ACPI_MADT_GICC_LENGTH;
>      /* Add Generic Interrupt */
>      for ( i = 0; i < d->max_vcpus; i++ )
>      {
> @@ -1165,7 +1166,7 @@ gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
>      struct acpi_madt_generic_interrupt *processor =
>                 container_of(header, struct acpi_madt_generic_interrupt, header);
>  
> -    if ( BAD_MADT_ENTRY(processor, end) )
> +    if ( BAD_MADT_GICC_ENTRY(processor, end) )
>          return -EINVAL;
>  
>      /* Read from APIC table and fill up the GIC variables */
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 0f6cbf6224e9..ce202402c0ed 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1558,7 +1558,7 @@ gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
>      struct acpi_madt_generic_interrupt *processor =
>                 container_of(header, struct acpi_madt_generic_interrupt, header);
>  
> -    if ( BAD_MADT_ENTRY(processor, end) )
> +    if ( BAD_MADT_GICC_ENTRY(processor, end) )
>          return -EINVAL;
>  
>      /* Read from APIC table and fill up the GIC variables */
> -- 
> 2.17.1
> 

Re: [PATCH v2 6/7] xen/arm: gic-v2: acpi: Use the correct length for the GICC structure

[Stefano Stabellini]

On Fri, 23 Oct 2020, Stefano Stabellini wrote:
> On Fri, 23 Oct 2020, Julien Grall wrote:
> > From: Julien Grall <julien.grall@arm.com>
> > 
> > The length of the GICC structure in the MADT ACPI table differs between
> > version 5.1 and 6.0, although there are no other relevant differences.
> > 
> > Use the BAD_MADT_GICC_ENTRY macro, which was specifically designed to
> > overcome this issue.
> > 
> > Signed-off-by: Julien Grall <julien.grall@arm.com>
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

Actually it looks we need to do substitutions in a couple of other places:

- xen/arch/arm/gic-v3.c:gicv3_make_hwdom_madt
- xen/arch/arm/gic-v3.c:gic_acpi_get_madt_cpu_num
- xen/arch/arm/gic.c:gic_get_hwdom_madt_size





> >     Changes in v2:
> >         - Patch added
> > ---
> >  xen/arch/arm/acpi/boot.c | 2 +-
> >  xen/arch/arm/gic-v2.c    | 5 +++--
> >  xen/arch/arm/gic-v3.c    | 2 +-
> >  3 files changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c
> > index 30e4bd1bc5a7..55c3e5cbc834 100644
> > --- a/xen/arch/arm/acpi/boot.c
> > +++ b/xen/arch/arm/acpi/boot.c
> > @@ -131,7 +131,7 @@ acpi_parse_gic_cpu_interface(struct acpi_subtable_header *header,
> >      struct acpi_madt_generic_interrupt *processor =
> >                 container_of(header, struct acpi_madt_generic_interrupt, header);
> >  
> > -    if ( BAD_MADT_ENTRY(processor, end) )
> > +    if ( BAD_MADT_GICC_ENTRY(processor, end) )
> >          return -EINVAL;
> >  
> >      acpi_table_print_madt_entry(header);
> > diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> > index 0f747538dbcd..0e5f23201974 100644
> > --- a/xen/arch/arm/gic-v2.c
> > +++ b/xen/arch/arm/gic-v2.c
> > @@ -1136,7 +1136,8 @@ static int gicv2_make_hwdom_madt(const struct domain *d, u32 offset)
> >  
> >      host_gicc = container_of(header, struct acpi_madt_generic_interrupt,
> >                               header);
> > -    size = sizeof(struct acpi_madt_generic_interrupt);
> > +
> > +    size = ACPI_MADT_GICC_LENGTH;
> >      /* Add Generic Interrupt */
> >      for ( i = 0; i < d->max_vcpus; i++ )
> >      {
> > @@ -1165,7 +1166,7 @@ gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
> >      struct acpi_madt_generic_interrupt *processor =
> >                 container_of(header, struct acpi_madt_generic_interrupt, header);
> >  
> > -    if ( BAD_MADT_ENTRY(processor, end) )
> > +    if ( BAD_MADT_GICC_ENTRY(processor, end) )
> >          return -EINVAL;
> >  
> >      /* Read from APIC table and fill up the GIC variables */
> > diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> > index 0f6cbf6224e9..ce202402c0ed 100644
> > --- a/xen/arch/arm/gic-v3.c
> > +++ b/xen/arch/arm/gic-v3.c
> > @@ -1558,7 +1558,7 @@ gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
> >      struct acpi_madt_generic_interrupt *processor =
> >                 container_of(header, struct acpi_madt_generic_interrupt, header);
> >  
> > -    if ( BAD_MADT_ENTRY(processor, end) )
> > +    if ( BAD_MADT_GICC_ENTRY(processor, end) )
> >          return -EINVAL;
> >  
> >      /* Read from APIC table and fill up the GIC variables */
> > -- 
> > 2.17.1
> > 
> 

Re: [PATCH v2 6/7] xen/arm: gic-v2: acpi: Use the correct length for the GICC structure

[Julien Grall]

Hi Stefano,

I just realized the title says "gic-v2" when I also modified "gic-v3". I 
will update the title on the next version.

On 24/10/2020 01:45, Stefano Stabellini wrote:
> On Fri, 23 Oct 2020, Stefano Stabellini wrote:
>> On Fri, 23 Oct 2020, Julien Grall wrote:
>>> From: Julien Grall <julien.grall@arm.com>
>>>
>>> The length of the GICC structure in the MADT ACPI table differs between
>>> version 5.1 and 6.0, although there are no other relevant differences.
>>>
>>> Use the BAD_MADT_GICC_ENTRY macro, which was specifically designed to
>>> overcome this issue.
>>>
>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>>
>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> Actually it looks we need to do substitutions in a couple of other places:
> 
> - xen/arch/arm/gic-v3.c:gicv3_make_hwdom_madt
> - xen/arch/arm/gic-v3.c:gic_acpi_get_madt_cpu_num
> - xen/arch/arm/gic.c:gic_get_hwdom_madt_size
I will update the 3 and resend the series.

Cheers,

-- 
Julien Grall

[PATCH v2 7/7] xen/arm: acpi: Allow Xen to boot with ACPI 5.1

[Julien Grall]

From: Julien Grall <julien.grall@arm.com>

At the moment Xen requires the FADT ACPI table to be at least version
6.0, apparently because of some reliance on other ACPI v6.0 features.

But actually this is overzealous, and Xen works now fine with ACPI v5.1.

Let's relax the version check for the FADT table to allow QEMU to
run the hypervisor with ACPI.

Signed-off-by: Julien Grall <julien.grall@arm.com>
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
Signed-off-by: Julien Grall <jgrall@amazon.com>

---
    Changes in v2:
        - Patch added
---
 xen/arch/arm/acpi/boot.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c
index 55c3e5cbc834..7ea2990cb82c 100644
--- a/xen/arch/arm/acpi/boot.c
+++ b/xen/arch/arm/acpi/boot.c
@@ -181,8 +181,8 @@ static int __init acpi_parse_fadt(struct acpi_table_header *table)
      * we only deal with ACPI 6.0 or newer revision to get GIC and SMP
      * boot protocol configuration data, or we will disable ACPI.
      */
-    if ( table->revision > 6
-         || (table->revision == 6 && fadt->minor_revision >= 0) )
+    if ( table->revision > 5
+         || (table->revision == 5 && fadt->minor_revision >= 1) )
         return 0;
 
     printk("Unsupported FADT revision %d.%d, should be 6.0+, will disable ACPI\n",
-- 
2.17.1

Re: [PATCH v2 7/7] xen/arm: acpi: Allow Xen to boot with ACPI 5.1

[Stefano Stabellini]

On Fri, 23 Oct 2020, Julien Grall wrote:
> From: Julien Grall <julien.grall@arm.com>
> 
> At the moment Xen requires the FADT ACPI table to be at least version
> 6.0, apparently because of some reliance on other ACPI v6.0 features.
> 
> But actually this is overzealous, and Xen works now fine with ACPI v5.1.
> 
> Let's relax the version check for the FADT table to allow QEMU to
> run the hypervisor with ACPI.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> Signed-off-by: Julien Grall <jgrall@amazon.com>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>     Changes in v2:
>         - Patch added
> ---
>  xen/arch/arm/acpi/boot.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c
> index 55c3e5cbc834..7ea2990cb82c 100644
> --- a/xen/arch/arm/acpi/boot.c
> +++ b/xen/arch/arm/acpi/boot.c
> @@ -181,8 +181,8 @@ static int __init acpi_parse_fadt(struct acpi_table_header *table)
>       * we only deal with ACPI 6.0 or newer revision to get GIC and SMP
>       * boot protocol configuration data, or we will disable ACPI.
>       */
> -    if ( table->revision > 6
> -         || (table->revision == 6 && fadt->minor_revision >= 0) )
> +    if ( table->revision > 5
> +         || (table->revision == 5 && fadt->minor_revision >= 1) )
>          return 0;
>  
>      printk("Unsupported FADT revision %d.%d, should be 6.0+, will disable ACPI\n",
> -- 
> 2.17.1
> 

Re: [PATCH v2 0/7] xen/arm: Unbreak ACPI

[Elliott Mitchell]

On Fri, Oct 23, 2020 at 04:41:49PM +0100, Julien Grall wrote:
> Xen on ARM has been broken for quite a while on ACPI systems. This
> series aims to fix it.
> 
> This series also introduced support for ACPI 5.1. This allows Xen to
> boot on QEMU.
> 
> I have only build tested the x86 side so far.

On a Tianocore-utilizing Raspberry PI 4B, this series allows successful
boot (some other distinct issues remain).  As such, for the series on an
ARM device:

Tested-by: Elliott Mitchell <ehem+xen@m5p.com>


-- 
(\___(\___(\______          --=> 8-) EHM <=--          ______/)___/)___/)
 \BS (    |         ehem+sigmsg@m5p.com  PGP 87145445         |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445