[PATCH 0/8] xen/arm: Fix and clean-up for ACPI and EFI

[PATCH 0/8] xen/arm: Fix and clean-up for ACPI and EFI

[Julien Grall]

Hi all,

This patch series contains a bunch of fix and clean-up for ACPI and EFI on
ARM64.

Note that the patch "xen/arm64: Don't zero BSS when booting using EFI" [1] is
required in order to test this series.

Cheers,

[1] https://lists.xenproject.org/archives/html/xen-devel/2017-02/msg00228.html

Julien Grall (8):
  xen/arm: acpi: Handle correctly detection of GICv2 on GICv3
  xen/arm: acpi: Rework acpi_boot_table_init error paths
  xen/arm: acpi: Don't fallback on DT when user request ACPI
  xen/arm: Print whether Xen is booting using ACPI or DT
  xen/arm: efi: Avoid duplicating the addition of a new bank
  xen/arm: efi: Avoid duplicating the addition of a new efi memory
    descriptor
  xen/arm: efi: Rework acpi_create_efi_mmap_table to avoid
    memory_map[offset]
  xen/arm: acpi: Move the ACPI banks in bootinfo

 xen/arch/arm/acpi/boot.c    | 30 ++++++++++++++--------
 xen/arch/arm/efi/efi-boot.h | 35 +++++++++++++-------------
 xen/arch/arm/efi/efi-dom0.c | 61 +++++++++++++++++++++------------------------
 xen/arch/arm/efi/efi-dom0.h |  8 ------
 xen/arch/arm/gic-v3.c       | 20 ++++++++++++++-
 xen/arch/arm/setup.c        |  5 ++++
 xen/include/asm-arm/setup.h |  3 +++
 7 files changed, 93 insertions(+), 69 deletions(-)
 delete mode 100644 xen/arch/arm/efi/efi-dom0.h

-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 1/8] xen/arm: acpi: Handle correctly detection of GICv2 on GICv3

[Julien Grall]

When the GICv3 is not GICv2 compatible, the associated field in the MADT
will be zeroed. However, the rest of the code expects the variable to
be set to INVALID_PADDR.

This will result to false detection of GICv2 and give I/O access to page
0 for the hardware domain.

Thankfully, it will fail because the size of GICV has not been set.

Fix the detection by converting 0 to INVALID_PADDR for the GICC and
GICV base. At the same time only set the size of each region when the
base address is not 0.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/gic-v3.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 955591b..bb1861e 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1356,7 +1356,6 @@ gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
     if ( !cpu_base_assigned )
     {
         cbase = processor->base_address;
-        csize = SZ_8K;
         vbase = processor->gicv_base_address;
         gicv3_info.maintenance_irq = processor->vgic_interrupt;
 
@@ -1505,6 +1504,25 @@ static void __init gicv3_acpi_init(void)
         panic("GICv3: No valid GICC entries exists");
 
     gicv3.rdist_stride = 0;
+
+    /*
+     * In ACPI, 0 is considered as the invalid address. However the rest
+     * of the initialization rely on the invalid address to be
+     * INVALID_ADDR.
+     *
+     * Also set the size of the GICC and GICV when there base address
+     * is not invalid as those values are not present in ACPI.
+     */
+    if ( !cbase )
+        cbase = INVALID_PADDR;
+    else
+        csize = SZ_8K;
+
+    if ( !vbase )
+        vbase = INVALID_PADDR;
+    else
+        vsize = GUEST_GICC_SIZE;
+
 }
 #else
 static void __init gicv3_acpi_init(void) { }
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/8] xen/arm: acpi: Handle correctly detection of GICv2 on GICv3

[Stefano Stabellini]

On Fri, 3 Feb 2017, Julien Grall wrote:
> When the GICv3 is not GICv2 compatible, the associated field in the MADT
> will be zeroed. However, the rest of the code expects the variable to
> be set to INVALID_PADDR.
> 
> This will result to false detection of GICv2 and give I/O access to page
> 0 for the hardware domain.
> 
> Thankfully, it will fail because the size of GICV has not been set.
> 
> Fix the detection by converting 0 to INVALID_PADDR for the GICC and
> GICV base. At the same time only set the size of each region when the
> base address is not 0.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

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

> ---
>  xen/arch/arm/gic-v3.c | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 955591b..bb1861e 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1356,7 +1356,6 @@ gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
>      if ( !cpu_base_assigned )
>      {
>          cbase = processor->base_address;
> -        csize = SZ_8K;
>          vbase = processor->gicv_base_address;
>          gicv3_info.maintenance_irq = processor->vgic_interrupt;
>  
> @@ -1505,6 +1504,25 @@ static void __init gicv3_acpi_init(void)
>          panic("GICv3: No valid GICC entries exists");
>  
>      gicv3.rdist_stride = 0;
> +
> +    /*
> +     * In ACPI, 0 is considered as the invalid address. However the rest
> +     * of the initialization rely on the invalid address to be
> +     * INVALID_ADDR.
> +     *
> +     * Also set the size of the GICC and GICV when there base address
> +     * is not invalid as those values are not present in ACPI.
> +     */
> +    if ( !cbase )
> +        cbase = INVALID_PADDR;
> +    else
> +        csize = SZ_8K;
> +
> +    if ( !vbase )
> +        vbase = INVALID_PADDR;
> +    else
> +        vsize = GUEST_GICC_SIZE;
> +
>  }
>  #else
>  static void __init gicv3_acpi_init(void) { }
> -- 
> 1.9.1
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 1/7] xen/arm: acpi: Rework acpi_boot_table_init error paths

[Julien Grall]

There are multiple path disable ACPI on error. Consolidate in a single
place, this will help in a follow-up patch to add more code on the error
path.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/acpi/boot.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c
index c3242a0..889208a 100644
--- a/xen/arch/arm/acpi/boot.c
+++ b/xen/arch/arm/acpi/boot.c
@@ -234,7 +234,7 @@ static int __init dt_scan_depth1_nodes(const void *fdt, int node,
  */
 int __init acpi_boot_table_init(void)
 {
-    int error;
+    int error = 0;
 
     /*
      * Enable ACPI instead of device tree unless
@@ -245,10 +245,7 @@ int __init acpi_boot_table_init(void)
     if ( param_acpi_off || ( !param_acpi_force
                              && device_tree_for_each_node(device_tree_flattened,
                                                    dt_scan_depth1_nodes, NULL)))
-    {
-        disable_acpi();
-        return 0;
-    }
+        goto disable;
 
     /*
      * ACPI is disabled at this point. Enable it in order to parse
@@ -260,16 +257,22 @@ int __init acpi_boot_table_init(void)
     error = acpi_table_init();
     if ( error )
     {
-        disable_acpi();
-        return error;
+        printk("%s: Unable to initialize table parser (%d)\n",
+               __FUNCTION__, error);
+        goto disable;
     }
 
-    if ( acpi_table_parse(ACPI_SIG_FADT, acpi_parse_fadt) )
+    error = acpi_table_parse(ACPI_SIG_FADT, acpi_parse_fadt);
+    if ( error )
     {
-        /* disable ACPI if no FADT is found */
-        disable_acpi();
-        printk("Can't find FADT\n");
+        printk("%s: FADT not found (%d)\n", __FUNCTION__, error);
+        goto disable;
     }
 
     return 0;
+
+disable:
+    disable_acpi();
+
+    return error;
 }
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/7] xen/arm: acpi: Rework acpi_boot_table_init error paths

[Wei Liu]

On Fri, Feb 03, 2017 at 07:18:46PM +0000, Julien Grall wrote:
> There are multiple path disable ACPI on error. Consolidate in a single
> place, this will help in a follow-up patch to add more code on the error
> path.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/arm/acpi/boot.c | 25 ++++++++++++++-----------
>  1 file changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c
> index c3242a0..889208a 100644
> --- a/xen/arch/arm/acpi/boot.c
> +++ b/xen/arch/arm/acpi/boot.c
> @@ -234,7 +234,7 @@ static int __init dt_scan_depth1_nodes(const void *fdt, int node,
>   */
>  int __init acpi_boot_table_init(void)
>  {
> -    int error;
> +    int error = 0;
>  
>      /*
>       * Enable ACPI instead of device tree unless
> @@ -245,10 +245,7 @@ int __init acpi_boot_table_init(void)
>      if ( param_acpi_off || ( !param_acpi_force
>                               && device_tree_for_each_node(device_tree_flattened,
>                                                     dt_scan_depth1_nodes, NULL)))
> -    {
> -        disable_acpi();
> -        return 0;
> -    }
> +        goto disable;
>  
>      /*
>       * ACPI is disabled at this point. Enable it in order to parse
> @@ -260,16 +257,22 @@ int __init acpi_boot_table_init(void)
>      error = acpi_table_init();
>      if ( error )
>      {
> -        disable_acpi();
> -        return error;
> +        printk("%s: Unable to initialize table parser (%d)\n",
> +               __FUNCTION__, error);

Please use __func__ instead.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 2/7] xen/arm: acpi: Don't fallback on DT when user request ACPI

[Julien Grall]

On ARM, when the user put 'acpi=force' Xen will use ACPI over DT.
Currently, if Xen fails to initialize ACPI it will fallback on DT.

This behavior makes difficult for a user to notice Xen didn't used ACPI
has requested on platform where the firmware is providing both ACPI and DT.

Rather than fallback on DT during a failure, panic when 'acpi=force'.

Signed-off-by: Julien Grall <julien.grall@arm.com>

---
    I am wondering if we should do the same with acpi=on. So a user
    would notice directly if something went wrong with ACPI.
    Otherwise you would boot up to the prompt and barely notice that DT
    was used.
---
 xen/arch/arm/acpi/boot.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c
index 889208a..65ef632 100644
--- a/xen/arch/arm/acpi/boot.c
+++ b/xen/arch/arm/acpi/boot.c
@@ -272,6 +272,11 @@ int __init acpi_boot_table_init(void)
     return 0;
 
 disable:
+
+    /* Panic if the user has requested ACPI but Xen is able to initialize. */
+    if ( param_acpi_force )
+        panic("Unable to use ACPI");
+
     disable_acpi();
 
     return error;
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 2/8] xen/arm: acpi: Rework acpi_boot_table_init error paths

[Julien Grall]

There are multiple path disable ACPI on error. Consolidate in a single
place, this will help in a follow-up patch to add more code on the error
path.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/acpi/boot.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c
index c3242a0..889208a 100644
--- a/xen/arch/arm/acpi/boot.c
+++ b/xen/arch/arm/acpi/boot.c
@@ -234,7 +234,7 @@ static int __init dt_scan_depth1_nodes(const void *fdt, int node,
  */
 int __init acpi_boot_table_init(void)
 {
-    int error;
+    int error = 0;
 
     /*
      * Enable ACPI instead of device tree unless
@@ -245,10 +245,7 @@ int __init acpi_boot_table_init(void)
     if ( param_acpi_off || ( !param_acpi_force
                              && device_tree_for_each_node(device_tree_flattened,
                                                    dt_scan_depth1_nodes, NULL)))
-    {
-        disable_acpi();
-        return 0;
-    }
+        goto disable;
 
     /*
      * ACPI is disabled at this point. Enable it in order to parse
@@ -260,16 +257,22 @@ int __init acpi_boot_table_init(void)
     error = acpi_table_init();
     if ( error )
     {
-        disable_acpi();
-        return error;
+        printk("%s: Unable to initialize table parser (%d)\n",
+               __FUNCTION__, error);
+        goto disable;
     }
 
-    if ( acpi_table_parse(ACPI_SIG_FADT, acpi_parse_fadt) )
+    error = acpi_table_parse(ACPI_SIG_FADT, acpi_parse_fadt);
+    if ( error )
     {
-        /* disable ACPI if no FADT is found */
-        disable_acpi();
-        printk("Can't find FADT\n");
+        printk("%s: FADT not found (%d)\n", __FUNCTION__, error);
+        goto disable;
     }
 
     return 0;
+
+disable:
+    disable_acpi();
+
+    return error;
 }
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/8] xen/arm: acpi: Rework acpi_boot_table_init error paths

[Andrew Cooper]

On 03/02/17 19:18, Julien Grall wrote:
> @@ -260,16 +257,22 @@ int __init acpi_boot_table_init(void)
>      error = acpi_table_init();
>      if ( error )
>      {
> -        disable_acpi();
> -        return error;
> +        printk("%s: Unable to initialize table parser (%d)\n",
> +               __FUNCTION__, error);

As a nit, please use __func__.

It is standard C, whereas __FUNCTION__ is a GCCism.

(On that note, there are very few remaining uses in the hypervisor - let
me submit a patch killing the remaining uses.)

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/8] xen/arm: acpi: Rework acpi_boot_table_init error paths

[Julien Grall]

Hi Andrew,

On 03/02/17 20:44, Andrew Cooper wrote:
> On 03/02/17 19:18, Julien Grall wrote:
>> @@ -260,16 +257,22 @@ int __init acpi_boot_table_init(void)
>>      error = acpi_table_init();
>>      if ( error )
>>      {
>> -        disable_acpi();
>> -        return error;
>> +        printk("%s: Unable to initialize table parser (%d)\n",
>> +               __FUNCTION__, error);
>
> As a nit, please use __func__.
>
> It is standard C, whereas __FUNCTION__ is a GCCism.
>
> (On that note, there are very few remaining uses in the hypervisor - let
> me submit a patch killing the remaining uses.)

I will fix it and try to remember for next time.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/8] xen/arm: acpi: Rework acpi_boot_table_init error paths

[Stefano Stabellini]

On Fri, 3 Feb 2017, Julien Grall wrote:
> There are multiple path disable ACPI on error. Consolidate in a single
> place, this will help in a follow-up patch to add more code on the error
> path.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

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


> ---
>  xen/arch/arm/acpi/boot.c | 25 ++++++++++++++-----------
>  1 file changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c
> index c3242a0..889208a 100644
> --- a/xen/arch/arm/acpi/boot.c
> +++ b/xen/arch/arm/acpi/boot.c
> @@ -234,7 +234,7 @@ static int __init dt_scan_depth1_nodes(const void *fdt, int node,
>   */
>  int __init acpi_boot_table_init(void)
>  {
> -    int error;
> +    int error = 0;
>  
>      /*
>       * Enable ACPI instead of device tree unless
> @@ -245,10 +245,7 @@ int __init acpi_boot_table_init(void)
>      if ( param_acpi_off || ( !param_acpi_force
>                               && device_tree_for_each_node(device_tree_flattened,
>                                                     dt_scan_depth1_nodes, NULL)))
> -    {
> -        disable_acpi();
> -        return 0;
> -    }
> +        goto disable;
>  
>      /*
>       * ACPI is disabled at this point. Enable it in order to parse
> @@ -260,16 +257,22 @@ int __init acpi_boot_table_init(void)
>      error = acpi_table_init();
>      if ( error )
>      {
> -        disable_acpi();
> -        return error;
> +        printk("%s: Unable to initialize table parser (%d)\n",
> +               __FUNCTION__, error);
> +        goto disable;
>      }
>  
> -    if ( acpi_table_parse(ACPI_SIG_FADT, acpi_parse_fadt) )
> +    error = acpi_table_parse(ACPI_SIG_FADT, acpi_parse_fadt);
> +    if ( error )
>      {
> -        /* disable ACPI if no FADT is found */
> -        disable_acpi();
> -        printk("Can't find FADT\n");
> +        printk("%s: FADT not found (%d)\n", __FUNCTION__, error);
> +        goto disable;
>      }
>  
>      return 0;
> +
> +disable:
> +    disable_acpi();
> +
> +    return error;
>  }
> -- 
> 1.9.1
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 3/8] xen/arm: acpi: Don't fallback on DT when user request ACPI

[Julien Grall]

On ARM, when the user put 'acpi=force' Xen will use ACPI over DT.
Currently, if Xen fails to initialize ACPI it will fallback on DT.

This behavior makes difficult for a user to notice Xen didn't used ACPI
has requested on platform where the firmware is providing both ACPI and DT.

Rather than fallback on DT during a failure, panic when 'acpi=force'.

Signed-off-by: Julien Grall <julien.grall@arm.com>

---
    I am wondering if we should do the same with acpi=on. So a user
    would notice directly if something went wrong with ACPI.
    Otherwise you would boot up to the prompt and barely notice that DT
    was used.
---
 xen/arch/arm/acpi/boot.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c
index 889208a..65ef632 100644
--- a/xen/arch/arm/acpi/boot.c
+++ b/xen/arch/arm/acpi/boot.c
@@ -272,6 +272,11 @@ int __init acpi_boot_table_init(void)
     return 0;
 
 disable:
+
+    /* Panic if the user has requested ACPI but Xen is able to initialize. */
+    if ( param_acpi_force )
+        panic("Unable to use ACPI");
+
     disable_acpi();
 
     return error;
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 3/8] xen/arm: acpi: Don't fallback on DT when user request ACPI

[Stefano Stabellini]

On Fri, 3 Feb 2017, Julien Grall wrote:
> On ARM, when the user put 'acpi=force' Xen will use ACPI over DT.
> Currently, if Xen fails to initialize ACPI it will fallback on DT.
> 
> This behavior makes difficult for a user to notice Xen didn't used ACPI
> has requested on platform where the firmware is providing both ACPI and DT.
> 
> Rather than fallback on DT during a failure, panic when 'acpi=force'.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> ---
>     I am wondering if we should do the same with acpi=on. So a user
>     would notice directly if something went wrong with ACPI.
>     Otherwise you would boot up to the prompt and barely notice that DT
>     was used.

I would keep the current behavior as is and add new parameter that means
"acpi and only acpi". Today acpi=force means "acpi is preferred to
device tree". It doesn't mean that if acpi fail, Xen should panic.

Maybe acpi=strict or acpi=mandatory?


> ---
>  xen/arch/arm/acpi/boot.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c
> index 889208a..65ef632 100644
> --- a/xen/arch/arm/acpi/boot.c
> +++ b/xen/arch/arm/acpi/boot.c
> @@ -272,6 +272,11 @@ int __init acpi_boot_table_init(void)
>      return 0;
>  
>  disable:
> +
> +    /* Panic if the user has requested ACPI but Xen is able to initialize. */
> +    if ( param_acpi_force )
> +        panic("Unable to use ACPI");
> +
>      disable_acpi();
>  
>      return error;
> -- 
> 1.9.1
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 3/8] xen/arm: acpi: Don't fallback on DT when user request ACPI

[Julien Grall]

Hi Stefano,

On 02/16/2017 01:41 AM, Stefano Stabellini wrote:
> On Fri, 3 Feb 2017, Julien Grall wrote:
>> On ARM, when the user put 'acpi=force' Xen will use ACPI over DT.
>> Currently, if Xen fails to initialize ACPI it will fallback on DT.
>>
>> This behavior makes difficult for a user to notice Xen didn't used ACPI
>> has requested on platform where the firmware is providing both ACPI and DT.
>>
>> Rather than fallback on DT during a failure, panic when 'acpi=force'.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>
>> ---
>>     I am wondering if we should do the same with acpi=on. So a user
>>     would notice directly if something went wrong with ACPI.
>>     Otherwise you would boot up to the prompt and barely notice that DT
>>     was used.
>
> I would keep the current behavior as is and add new parameter that means
> "acpi and only acpi". Today acpi=force means "acpi is preferred to
> device tree". It doesn't mean that if acpi fail, Xen should panic.
>
> Maybe acpi=strict or acpi=mandatory?

We have a bunch of option that makes little sense today:
    - off: Turned off ACPI
    - on: Turned on ACPI only if DT is empty
    - force: Turned on ACPI

If ACPI fails you fallback on DT. This is a pain to detect whether DT or 
ACPI is been used. So I think adding another option will only confuse 
the user.

Looking at Linux the option are:
     - off: Turned off ACPI
     - on: Prefer ACPI over DT. Fallback on DT is failed
     - force: Use ACPI and panic if not available

I would much prefer to use the behavior of the acpi param on Linux 
because it will avoid the user to be confused on the usage.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 3/8] xen/arm: acpi: Don't fallback on DT when user request ACPI

[Stefano Stabellini]

On Mon, 20 Feb 2017, Julien Grall wrote:
> Hi Stefano,
> 
> On 02/16/2017 01:41 AM, Stefano Stabellini wrote:
> > On Fri, 3 Feb 2017, Julien Grall wrote:
> > > On ARM, when the user put 'acpi=force' Xen will use ACPI over DT.
> > > Currently, if Xen fails to initialize ACPI it will fallback on DT.
> > > 
> > > This behavior makes difficult for a user to notice Xen didn't used ACPI
> > > has requested on platform where the firmware is providing both ACPI and
> > > DT.
> > > 
> > > Rather than fallback on DT during a failure, panic when 'acpi=force'.
> > > 
> > > Signed-off-by: Julien Grall <julien.grall@arm.com>
> > > 
> > > ---
> > >     I am wondering if we should do the same with acpi=on. So a user
> > >     would notice directly if something went wrong with ACPI.
> > >     Otherwise you would boot up to the prompt and barely notice that DT
> > >     was used.
> > 
> > I would keep the current behavior as is and add new parameter that means
> > "acpi and only acpi". Today acpi=force means "acpi is preferred to
> > device tree". It doesn't mean that if acpi fail, Xen should panic.
> > 
> > Maybe acpi=strict or acpi=mandatory?
> 
> We have a bunch of option that makes little sense today:
>    - off: Turned off ACPI
>    - on: Turned on ACPI only if DT is empty
>    - force: Turned on ACPI
> 
> If ACPI fails you fallback on DT. This is a pain to detect whether DT or ACPI
> is been used. So I think adding another option will only confuse the user.
> 
> Looking at Linux the option are:
>     - off: Turned off ACPI
>     - on: Prefer ACPI over DT. Fallback on DT is failed
>     - force: Use ACPI and panic if not available
> 
> I would much prefer to use the behavior of the acpi param on Linux because it
> will avoid the user to be confused on the usage.

All right, but in this case you need to update
docs/misc/xen-command-line.markdown.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 3/8] xen/arm: acpi: Don't fallback on DT when user request ACPI

[Julien Grall]

Hi Stefano,

On 20/02/17 18:12, Stefano Stabellini wrote:
> On Mon, 20 Feb 2017, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 02/16/2017 01:41 AM, Stefano Stabellini wrote:
>>> On Fri, 3 Feb 2017, Julien Grall wrote:
>>>> On ARM, when the user put 'acpi=force' Xen will use ACPI over DT.
>>>> Currently, if Xen fails to initialize ACPI it will fallback on DT.
>>>>
>>>> This behavior makes difficult for a user to notice Xen didn't used ACPI
>>>> has requested on platform where the firmware is providing both ACPI and
>>>> DT.
>>>>
>>>> Rather than fallback on DT during a failure, panic when 'acpi=force'.
>>>>
>>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>>>
>>>> ---
>>>>     I am wondering if we should do the same with acpi=on. So a user
>>>>     would notice directly if something went wrong with ACPI.
>>>>     Otherwise you would boot up to the prompt and barely notice that DT
>>>>     was used.
>>>
>>> I would keep the current behavior as is and add new parameter that means
>>> "acpi and only acpi". Today acpi=force means "acpi is preferred to
>>> device tree". It doesn't mean that if acpi fail, Xen should panic.
>>>
>>> Maybe acpi=strict or acpi=mandatory?
>>
>> We have a bunch of option that makes little sense today:
>>    - off: Turned off ACPI
>>    - on: Turned on ACPI only if DT is empty
>>    - force: Turned on ACPI
>>
>> If ACPI fails you fallback on DT. This is a pain to detect whether DT or ACPI
>> is been used. So I think adding another option will only confuse the user.
>>
>> Looking at Linux the option are:
>>     - off: Turned off ACPI
>>     - on: Prefer ACPI over DT. Fallback on DT is failed
>>     - force: Use ACPI and panic if not available
>>
>> I would much prefer to use the behavior of the acpi param on Linux because it
>> will avoid the user to be confused on the usage.
>
> All right, but in this case you need to update
> docs/misc/xen-command-line.markdown.

Sure, I will do it in the next version.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 3/8] xen/arm: acpi: Don't fallback on DT when user request ACPI

[Julien Grall]

Hi Stefano,

On 20/02/17 18:13, Julien Grall wrote:
> On 20/02/17 18:12, Stefano Stabellini wrote:
>> On Mon, 20 Feb 2017, Julien Grall wrote:
>>> Hi Stefano,
>>>
>>> On 02/16/2017 01:41 AM, Stefano Stabellini wrote:
>>>> On Fri, 3 Feb 2017, Julien Grall wrote:
>>>>> On ARM, when the user put 'acpi=force' Xen will use ACPI over DT.
>>>>> Currently, if Xen fails to initialize ACPI it will fallback on DT.
>>>>>
>>>>> This behavior makes difficult for a user to notice Xen didn't used
>>>>> ACPI
>>>>> has requested on platform where the firmware is providing both ACPI
>>>>> and
>>>>> DT.
>>>>>
>>>>> Rather than fallback on DT during a failure, panic when 'acpi=force'.
>>>>>
>>>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>>>>
>>>>> ---
>>>>>     I am wondering if we should do the same with acpi=on. So a user
>>>>>     would notice directly if something went wrong with ACPI.
>>>>>     Otherwise you would boot up to the prompt and barely notice
>>>>> that DT
>>>>>     was used.
>>>>
>>>> I would keep the current behavior as is and add new parameter that
>>>> means
>>>> "acpi and only acpi". Today acpi=force means "acpi is preferred to
>>>> device tree". It doesn't mean that if acpi fail, Xen should panic.
>>>>
>>>> Maybe acpi=strict or acpi=mandatory?
>>>
>>> We have a bunch of option that makes little sense today:
>>>    - off: Turned off ACPI
>>>    - on: Turned on ACPI only if DT is empty
>>>    - force: Turned on ACPI
>>>
>>> If ACPI fails you fallback on DT. This is a pain to detect whether DT
>>> or ACPI
>>> is been used. So I think adding another option will only confuse the
>>> user.
>>>
>>> Looking at Linux the option are:
>>>     - off: Turned off ACPI
>>>     - on: Prefer ACPI over DT. Fallback on DT is failed
>>>     - force: Use ACPI and panic if not available
>>>
>>> I would much prefer to use the behavior of the acpi param on Linux
>>> because it
>>> will avoid the user to be confused on the usage.
>>
>> All right, but in this case you need to update
>> docs/misc/xen-command-line.markdown.
>
> Sure, I will do it in the next version.

Actually, I will defer this patch a bit and resend the rest of the series.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 3/7] xen/arm: Print whether Xen is booting using ACPI or DT

[Julien Grall]

Make it easier to figure out whether Xen is booting using ACPI or DT by
printing a message on the console.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/setup.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 049e449..41aa1dd 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -753,6 +753,11 @@ void __init start_xen(unsigned long boot_phys_offset,
     /* Parse the ACPI tables for possible boot-time configuration */
     acpi_boot_table_init();
 
+    if ( acpi_disabled )
+        printk("Booting using Device Tree\n");
+    else
+        printk("Booting using ACPI\n");
+
     end_boot_allocator();
 
     vm_init();
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 4/7] xen/arm: efi: Avoid duplicating the addition of a new bank

[Julien Grall]

The code to add a new bank is duplicated twice. Add a new helper that
checks if the maximum of bank has not reached and adds the bank.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/efi/efi-boot.h | 34 ++++++++++++++++++----------------
 1 file changed, 18 insertions(+), 16 deletions(-)

diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
index 045d6ce..757d9c6 100644
--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -124,15 +124,27 @@ static void __init *lookup_fdt_config_table(EFI_SYSTEM_TABLE *sys_table)
     return fdt;
 }
 
+static bool meminfo_add_bank(struct meminfo *mem, EFI_MEMORY_DESCRIPTOR *desc)
+{
+    struct membank *bank;
+
+    if ( mem->nr_banks > NR_MEM_BANKS )
+        return false;
+
+    bank = &mem->bank[mem->nr_banks];
+    bank->start = desc->PhysicalStart;
+    bank->size = desc->NumberOfPages * EFI_PAGE_SIZE;
+
+    mem->nr_banks++;
+
+    return true;
+}
+
 static EFI_STATUS __init efi_process_memory_map_bootinfo(EFI_MEMORY_DESCRIPTOR *map,
                                                 UINTN mmap_size,
                                                 UINTN desc_size)
 {
     int Index;
-    int i = 0;
-#ifdef CONFIG_ACPI
-    int j = 0;
-#endif
     EFI_MEMORY_DESCRIPTOR *desc_ptr = map;
 
     for ( Index = 0; Index < (mmap_size / desc_size); Index++ )
@@ -142,37 +154,27 @@ static EFI_STATUS __init efi_process_memory_map_bootinfo(EFI_MEMORY_DESCRIPTOR *
               (desc_ptr->Type == EfiBootServicesCode ||
                desc_ptr->Type == EfiBootServicesData)) )
         {
-            if ( i >= NR_MEM_BANKS )
+            if ( !meminfo_add_bank(&bootinfo.mem, desc_ptr) )
             {
                 PrintStr(L"Warning: All " __stringify(NR_MEM_BANKS)
                           " bootinfo mem banks exhausted.\r\n");
                 break;
             }
-            bootinfo.mem.bank[i].start = desc_ptr->PhysicalStart;
-            bootinfo.mem.bank[i].size = desc_ptr->NumberOfPages * EFI_PAGE_SIZE;
-            ++i;
         }
 #ifdef CONFIG_ACPI
         else if ( desc_ptr->Type == EfiACPIReclaimMemory )
         {
-            if ( j >= NR_MEM_BANKS )
+            if ( !meminfo_add_bank(&acpi_mem, desc_ptr) )
             {
                 PrintStr(L"Error: All " __stringify(NR_MEM_BANKS)
                           " acpi meminfo mem banks exhausted.\r\n");
                 return EFI_LOAD_ERROR;
             }
-            acpi_mem.bank[j].start = desc_ptr->PhysicalStart;
-            acpi_mem.bank[j].size  = desc_ptr->NumberOfPages * EFI_PAGE_SIZE;
-            ++j;
         }
 #endif
         desc_ptr = NextMemoryDescriptor(desc_ptr, desc_size);
     }
 
-    bootinfo.mem.nr_banks = i;
-#ifdef CONFIG_ACPI
-    acpi_mem.nr_banks = j;
-#endif
     return EFI_SUCCESS;
 }
 
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 4/8] xen/arm: Print whether Xen is booting using ACPI or DT

[Julien Grall]

Make it easier to figure out whether Xen is booting using ACPI or DT by
printing a message on the console.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/setup.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 049e449..41aa1dd 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -753,6 +753,11 @@ void __init start_xen(unsigned long boot_phys_offset,
     /* Parse the ACPI tables for possible boot-time configuration */
     acpi_boot_table_init();
 
+    if ( acpi_disabled )
+        printk("Booting using Device Tree\n");
+    else
+        printk("Booting using ACPI\n");
+
     end_boot_allocator();
 
     vm_init();
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 4/8] xen/arm: Print whether Xen is booting using ACPI or DT

[Stefano Stabellini]

On Fri, 3 Feb 2017, Julien Grall wrote:
> Make it easier to figure out whether Xen is booting using ACPI or DT by
> printing a message on the console.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

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


> ---
>  xen/arch/arm/setup.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 049e449..41aa1dd 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -753,6 +753,11 @@ void __init start_xen(unsigned long boot_phys_offset,
>      /* Parse the ACPI tables for possible boot-time configuration */
>      acpi_boot_table_init();
>  
> +    if ( acpi_disabled )
> +        printk("Booting using Device Tree\n");
> +    else
> +        printk("Booting using ACPI\n");
> +
>      end_boot_allocator();
>  
>      vm_init();
> -- 
> 1.9.1
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 5/8] xen/arm: efi: Avoid duplicating the addition of a new bank

[Julien Grall]

The code to add a new bank is duplicated twice. Add a new helper that
checks if the maximum of bank has not reached and adds the bank.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/efi/efi-boot.h | 34 ++++++++++++++++++----------------
 1 file changed, 18 insertions(+), 16 deletions(-)

diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
index 045d6ce..757d9c6 100644
--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -124,15 +124,27 @@ static void __init *lookup_fdt_config_table(EFI_SYSTEM_TABLE *sys_table)
     return fdt;
 }
 
+static bool meminfo_add_bank(struct meminfo *mem, EFI_MEMORY_DESCRIPTOR *desc)
+{
+    struct membank *bank;
+
+    if ( mem->nr_banks > NR_MEM_BANKS )
+        return false;
+
+    bank = &mem->bank[mem->nr_banks];
+    bank->start = desc->PhysicalStart;
+    bank->size = desc->NumberOfPages * EFI_PAGE_SIZE;
+
+    mem->nr_banks++;
+
+    return true;
+}
+
 static EFI_STATUS __init efi_process_memory_map_bootinfo(EFI_MEMORY_DESCRIPTOR *map,
                                                 UINTN mmap_size,
                                                 UINTN desc_size)
 {
     int Index;
-    int i = 0;
-#ifdef CONFIG_ACPI
-    int j = 0;
-#endif
     EFI_MEMORY_DESCRIPTOR *desc_ptr = map;
 
     for ( Index = 0; Index < (mmap_size / desc_size); Index++ )
@@ -142,37 +154,27 @@ static EFI_STATUS __init efi_process_memory_map_bootinfo(EFI_MEMORY_DESCRIPTOR *
               (desc_ptr->Type == EfiBootServicesCode ||
                desc_ptr->Type == EfiBootServicesData)) )
         {
-            if ( i >= NR_MEM_BANKS )
+            if ( !meminfo_add_bank(&bootinfo.mem, desc_ptr) )
             {
                 PrintStr(L"Warning: All " __stringify(NR_MEM_BANKS)
                           " bootinfo mem banks exhausted.\r\n");
                 break;
             }
-            bootinfo.mem.bank[i].start = desc_ptr->PhysicalStart;
-            bootinfo.mem.bank[i].size = desc_ptr->NumberOfPages * EFI_PAGE_SIZE;
-            ++i;
         }
 #ifdef CONFIG_ACPI
         else if ( desc_ptr->Type == EfiACPIReclaimMemory )
         {
-            if ( j >= NR_MEM_BANKS )
+            if ( !meminfo_add_bank(&acpi_mem, desc_ptr) )
             {
                 PrintStr(L"Error: All " __stringify(NR_MEM_BANKS)
                           " acpi meminfo mem banks exhausted.\r\n");
                 return EFI_LOAD_ERROR;
             }
-            acpi_mem.bank[j].start = desc_ptr->PhysicalStart;
-            acpi_mem.bank[j].size  = desc_ptr->NumberOfPages * EFI_PAGE_SIZE;
-            ++j;
         }
 #endif
         desc_ptr = NextMemoryDescriptor(desc_ptr, desc_size);
     }
 
-    bootinfo.mem.nr_banks = i;
-#ifdef CONFIG_ACPI
-    acpi_mem.nr_banks = j;
-#endif
     return EFI_SUCCESS;
 }
 
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 5/8] xen/arm: efi: Avoid duplicating the addition of a new bank

[Stefano Stabellini]

On Fri, 3 Feb 2017, Julien Grall wrote:
> The code to add a new bank is duplicated twice. Add a new helper that
> checks if the maximum of bank has not reached and adds the bank.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

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


> ---
>  xen/arch/arm/efi/efi-boot.h | 34 ++++++++++++++++++----------------
>  1 file changed, 18 insertions(+), 16 deletions(-)
> 
> diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
> index 045d6ce..757d9c6 100644
> --- a/xen/arch/arm/efi/efi-boot.h
> +++ b/xen/arch/arm/efi/efi-boot.h
> @@ -124,15 +124,27 @@ static void __init *lookup_fdt_config_table(EFI_SYSTEM_TABLE *sys_table)
>      return fdt;
>  }
>  
> +static bool meminfo_add_bank(struct meminfo *mem, EFI_MEMORY_DESCRIPTOR *desc)
> +{
> +    struct membank *bank;
> +
> +    if ( mem->nr_banks > NR_MEM_BANKS )
> +        return false;
> +
> +    bank = &mem->bank[mem->nr_banks];
> +    bank->start = desc->PhysicalStart;
> +    bank->size = desc->NumberOfPages * EFI_PAGE_SIZE;
> +
> +    mem->nr_banks++;
> +
> +    return true;
> +}
> +
>  static EFI_STATUS __init efi_process_memory_map_bootinfo(EFI_MEMORY_DESCRIPTOR *map,
>                                                  UINTN mmap_size,
>                                                  UINTN desc_size)
>  {
>      int Index;
> -    int i = 0;
> -#ifdef CONFIG_ACPI
> -    int j = 0;
> -#endif
>      EFI_MEMORY_DESCRIPTOR *desc_ptr = map;
>  
>      for ( Index = 0; Index < (mmap_size / desc_size); Index++ )
> @@ -142,37 +154,27 @@ static EFI_STATUS __init efi_process_memory_map_bootinfo(EFI_MEMORY_DESCRIPTOR *
>                (desc_ptr->Type == EfiBootServicesCode ||
>                 desc_ptr->Type == EfiBootServicesData)) )
>          {
> -            if ( i >= NR_MEM_BANKS )
> +            if ( !meminfo_add_bank(&bootinfo.mem, desc_ptr) )
>              {
>                  PrintStr(L"Warning: All " __stringify(NR_MEM_BANKS)
>                            " bootinfo mem banks exhausted.\r\n");
>                  break;
>              }
> -            bootinfo.mem.bank[i].start = desc_ptr->PhysicalStart;
> -            bootinfo.mem.bank[i].size = desc_ptr->NumberOfPages * EFI_PAGE_SIZE;
> -            ++i;
>          }
>  #ifdef CONFIG_ACPI
>          else if ( desc_ptr->Type == EfiACPIReclaimMemory )
>          {
> -            if ( j >= NR_MEM_BANKS )
> +            if ( !meminfo_add_bank(&acpi_mem, desc_ptr) )
>              {
>                  PrintStr(L"Error: All " __stringify(NR_MEM_BANKS)
>                            " acpi meminfo mem banks exhausted.\r\n");
>                  return EFI_LOAD_ERROR;
>              }
> -            acpi_mem.bank[j].start = desc_ptr->PhysicalStart;
> -            acpi_mem.bank[j].size  = desc_ptr->NumberOfPages * EFI_PAGE_SIZE;
> -            ++j;
>          }
>  #endif
>          desc_ptr = NextMemoryDescriptor(desc_ptr, desc_size);
>      }
>  
> -    bootinfo.mem.nr_banks = i;
> -#ifdef CONFIG_ACPI
> -    acpi_mem.nr_banks = j;
> -#endif
>      return EFI_SUCCESS;
>  }
>  
> -- 
> 1.9.1
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 5/8] xen/arm: efi: Avoid duplicating the addition of a new bank

[Stefano Stabellini]

On Wed, 15 Feb 2017, Stefano Stabellini wrote:
> On Fri, 3 Feb 2017, Julien Grall wrote:
> > The code to add a new bank is duplicated twice. Add a new helper that
> > checks if the maximum of bank has not reached and adds the bank.
> > 
> > Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> 
> > ---
> >  xen/arch/arm/efi/efi-boot.h | 34 ++++++++++++++++++----------------
> >  1 file changed, 18 insertions(+), 16 deletions(-)
> > 
> > diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
> > index 045d6ce..757d9c6 100644
> > --- a/xen/arch/arm/efi/efi-boot.h
> > +++ b/xen/arch/arm/efi/efi-boot.h
> > @@ -124,15 +124,27 @@ static void __init *lookup_fdt_config_table(EFI_SYSTEM_TABLE *sys_table)
> >      return fdt;
> >  }
> >  
> > +static bool meminfo_add_bank(struct meminfo *mem, EFI_MEMORY_DESCRIPTOR *desc)

actually this function should be __init, right?


> > +{
> > +    struct membank *bank;
> > +
> > +    if ( mem->nr_banks > NR_MEM_BANKS )
> > +        return false;
> > +
> > +    bank = &mem->bank[mem->nr_banks];
> > +    bank->start = desc->PhysicalStart;
> > +    bank->size = desc->NumberOfPages * EFI_PAGE_SIZE;
> > +
> > +    mem->nr_banks++;
> > +
> > +    return true;
> > +}
> > +
> >  static EFI_STATUS __init efi_process_memory_map_bootinfo(EFI_MEMORY_DESCRIPTOR *map,
> >                                                  UINTN mmap_size,
> >                                                  UINTN desc_size)
> >  {
> >      int Index;
> > -    int i = 0;
> > -#ifdef CONFIG_ACPI
> > -    int j = 0;
> > -#endif
> >      EFI_MEMORY_DESCRIPTOR *desc_ptr = map;
> >  
> >      for ( Index = 0; Index < (mmap_size / desc_size); Index++ )
> > @@ -142,37 +154,27 @@ static EFI_STATUS __init efi_process_memory_map_bootinfo(EFI_MEMORY_DESCRIPTOR *
> >                (desc_ptr->Type == EfiBootServicesCode ||
> >                 desc_ptr->Type == EfiBootServicesData)) )
> >          {
> > -            if ( i >= NR_MEM_BANKS )
> > +            if ( !meminfo_add_bank(&bootinfo.mem, desc_ptr) )
> >              {
> >                  PrintStr(L"Warning: All " __stringify(NR_MEM_BANKS)
> >                            " bootinfo mem banks exhausted.\r\n");
> >                  break;
> >              }
> > -            bootinfo.mem.bank[i].start = desc_ptr->PhysicalStart;
> > -            bootinfo.mem.bank[i].size = desc_ptr->NumberOfPages * EFI_PAGE_SIZE;
> > -            ++i;
> >          }
> >  #ifdef CONFIG_ACPI
> >          else if ( desc_ptr->Type == EfiACPIReclaimMemory )
> >          {
> > -            if ( j >= NR_MEM_BANKS )
> > +            if ( !meminfo_add_bank(&acpi_mem, desc_ptr) )
> >              {
> >                  PrintStr(L"Error: All " __stringify(NR_MEM_BANKS)
> >                            " acpi meminfo mem banks exhausted.\r\n");
> >                  return EFI_LOAD_ERROR;
> >              }
> > -            acpi_mem.bank[j].start = desc_ptr->PhysicalStart;
> > -            acpi_mem.bank[j].size  = desc_ptr->NumberOfPages * EFI_PAGE_SIZE;
> > -            ++j;
> >          }
> >  #endif
> >          desc_ptr = NextMemoryDescriptor(desc_ptr, desc_size);
> >      }
> >  
> > -    bootinfo.mem.nr_banks = i;
> > -#ifdef CONFIG_ACPI
> > -    acpi_mem.nr_banks = j;
> > -#endif
> >      return EFI_SUCCESS;
> >  }
> >  
> > -- 
> > 1.9.1
> > 
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 5/8] xen/arm: efi: Avoid duplicating the addition of a new bank

[Julien Grall]

Hi Stefano,

On 16/02/17 01:49, Stefano Stabellini wrote:
> On Wed, 15 Feb 2017, Stefano Stabellini wrote:
>> On Fri, 3 Feb 2017, Julien Grall wrote:
>>> The code to add a new bank is duplicated twice. Add a new helper that
>>> checks if the maximum of bank has not reached and adds the bank.
>>>
>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>
>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>>
>>
>>> ---
>>>  xen/arch/arm/efi/efi-boot.h | 34 ++++++++++++++++++----------------
>>>  1 file changed, 18 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
>>> index 045d6ce..757d9c6 100644
>>> --- a/xen/arch/arm/efi/efi-boot.h
>>> +++ b/xen/arch/arm/efi/efi-boot.h
>>> @@ -124,15 +124,27 @@ static void __init *lookup_fdt_config_table(EFI_SYSTEM_TABLE *sys_table)
>>>      return fdt;
>>>  }
>>>
>>> +static bool meminfo_add_bank(struct meminfo *mem, EFI_MEMORY_DESCRIPTOR *desc)
>
> actually this function should be __init, right?

Yes it should, I will fix it in the next version.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 6/8] xen/arm: efi: Avoid duplicating the addition of a new efi memory descriptor

[Julien Grall]

The code to add a new memory descriptor is duplicated three times. Add a
new helper that adds the descriptor.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/efi/efi-dom0.c | 40 ++++++++++++++++++++--------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/xen/arch/arm/efi/efi-dom0.c b/xen/arch/arm/efi/efi-dom0.c
index c40a7c5..f307f26 100644
--- a/xen/arch/arm/efi/efi-dom0.c
+++ b/xen/arch/arm/efi/efi-dom0.c
@@ -96,6 +96,18 @@ void __init acpi_create_efi_system_table(struct domain *d,
     tbl_add[TBL_EFIT].size = table_size;
 }
 
+static void __init fill_efi_memory_descriptor(EFI_MEMORY_DESCRIPTOR *desc,
+                                              UINT32 type,
+                                              EFI_PHYSICAL_ADDRESS start,
+                                              UINT64 size)
+{
+    desc->Type = type;
+    desc->PhysicalStart = start;
+    BUG_ON(size & EFI_PAGE_MASK);
+    desc->NumberOfPages = EFI_SIZE_TO_PAGES(size);
+    desc->Attribute = EFI_MEMORY_WB;
+}
+
 void __init acpi_create_efi_mmap_table(struct domain *d,
                                        const struct meminfo *mem,
                                        struct membank tbl_add[])
@@ -110,28 +122,16 @@ void __init acpi_create_efi_mmap_table(struct domain *d,
 
     offset = 0;
     for( i = 0; i < mem->nr_banks; i++, offset++ )
-    {
-        memory_map[offset].Type = EfiConventionalMemory;
-        memory_map[offset].PhysicalStart = mem->bank[i].start;
-        BUG_ON(mem->bank[i].size & EFI_PAGE_MASK);
-        memory_map[offset].NumberOfPages = EFI_SIZE_TO_PAGES(mem->bank[i].size);
-        memory_map[offset].Attribute = EFI_MEMORY_WB;
-    }
+        fill_efi_memory_descriptor(&memory_map[offset], EfiConventionalMemory,
+                                   mem->bank[i].start, mem->bank[i].size);
 
     for( i = 0; i < acpi_mem.nr_banks; i++, offset++ )
-    {
-        memory_map[offset].Type = EfiACPIReclaimMemory;
-        memory_map[offset].PhysicalStart = acpi_mem.bank[i].start;
-        BUG_ON(acpi_mem.bank[i].size & EFI_PAGE_MASK);
-        memory_map[offset].NumberOfPages = EFI_SIZE_TO_PAGES(acpi_mem.bank[i].size);
-        memory_map[offset].Attribute = EFI_MEMORY_WB;
-    }
-
-    memory_map[offset].Type = EfiACPIReclaimMemory;
-    memory_map[offset].PhysicalStart = d->arch.efi_acpi_gpa;
-    BUG_ON(d->arch.efi_acpi_len & EFI_PAGE_MASK);
-    memory_map[offset].NumberOfPages = EFI_SIZE_TO_PAGES(d->arch.efi_acpi_len);
-    memory_map[offset].Attribute = EFI_MEMORY_WB;
+        fill_efi_memory_descriptor(&memory_map[offset], EfiACPIReclaimMemory,
+                                   acpi_mem.bank[i].start,
+                                   acpi_mem.bank[i].size);
+
+    fill_efi_memory_descriptor(&memory_map[offset], EfiACPIReclaimMemory,
+                               d->arch.efi_acpi_gpa, d->arch.efi_acpi_len);
 
     tbl_add[TBL_MMAP].start = d->arch.efi_acpi_gpa
                               + acpi_get_table_offset(tbl_add, TBL_MMAP);
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 6/8] xen/arm: efi: Avoid duplicating the addition of a new efi memory descriptor

[Stefano Stabellini]

On Fri, 3 Feb 2017, Julien Grall wrote:
> The code to add a new memory descriptor is duplicated three times. Add a
> new helper that adds the descriptor.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

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

> ---
>  xen/arch/arm/efi/efi-dom0.c | 40 ++++++++++++++++++++--------------------
>  1 file changed, 20 insertions(+), 20 deletions(-)
> 
> diff --git a/xen/arch/arm/efi/efi-dom0.c b/xen/arch/arm/efi/efi-dom0.c
> index c40a7c5..f307f26 100644
> --- a/xen/arch/arm/efi/efi-dom0.c
> +++ b/xen/arch/arm/efi/efi-dom0.c
> @@ -96,6 +96,18 @@ void __init acpi_create_efi_system_table(struct domain *d,
>      tbl_add[TBL_EFIT].size = table_size;
>  }
>  
> +static void __init fill_efi_memory_descriptor(EFI_MEMORY_DESCRIPTOR *desc,
> +                                              UINT32 type,
> +                                              EFI_PHYSICAL_ADDRESS start,
> +                                              UINT64 size)
> +{
> +    desc->Type = type;
> +    desc->PhysicalStart = start;
> +    BUG_ON(size & EFI_PAGE_MASK);
> +    desc->NumberOfPages = EFI_SIZE_TO_PAGES(size);
> +    desc->Attribute = EFI_MEMORY_WB;
> +}
> +
>  void __init acpi_create_efi_mmap_table(struct domain *d,
>                                         const struct meminfo *mem,
>                                         struct membank tbl_add[])
> @@ -110,28 +122,16 @@ void __init acpi_create_efi_mmap_table(struct domain *d,
>  
>      offset = 0;
>      for( i = 0; i < mem->nr_banks; i++, offset++ )
> -    {
> -        memory_map[offset].Type = EfiConventionalMemory;
> -        memory_map[offset].PhysicalStart = mem->bank[i].start;
> -        BUG_ON(mem->bank[i].size & EFI_PAGE_MASK);
> -        memory_map[offset].NumberOfPages = EFI_SIZE_TO_PAGES(mem->bank[i].size);
> -        memory_map[offset].Attribute = EFI_MEMORY_WB;
> -    }
> +        fill_efi_memory_descriptor(&memory_map[offset], EfiConventionalMemory,
> +                                   mem->bank[i].start, mem->bank[i].size);
>  
>      for( i = 0; i < acpi_mem.nr_banks; i++, offset++ )
> -    {
> -        memory_map[offset].Type = EfiACPIReclaimMemory;
> -        memory_map[offset].PhysicalStart = acpi_mem.bank[i].start;
> -        BUG_ON(acpi_mem.bank[i].size & EFI_PAGE_MASK);
> -        memory_map[offset].NumberOfPages = EFI_SIZE_TO_PAGES(acpi_mem.bank[i].size);
> -        memory_map[offset].Attribute = EFI_MEMORY_WB;
> -    }
> -
> -    memory_map[offset].Type = EfiACPIReclaimMemory;
> -    memory_map[offset].PhysicalStart = d->arch.efi_acpi_gpa;
> -    BUG_ON(d->arch.efi_acpi_len & EFI_PAGE_MASK);
> -    memory_map[offset].NumberOfPages = EFI_SIZE_TO_PAGES(d->arch.efi_acpi_len);
> -    memory_map[offset].Attribute = EFI_MEMORY_WB;
> +        fill_efi_memory_descriptor(&memory_map[offset], EfiACPIReclaimMemory,
> +                                   acpi_mem.bank[i].start,
> +                                   acpi_mem.bank[i].size);
> +
> +    fill_efi_memory_descriptor(&memory_map[offset], EfiACPIReclaimMemory,
> +                               d->arch.efi_acpi_gpa, d->arch.efi_acpi_len);
>  
>      tbl_add[TBL_MMAP].start = d->arch.efi_acpi_gpa
>                                + acpi_get_table_offset(tbl_add, TBL_MMAP);
> -- 
> 1.9.1
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 6/7] xen/arm: efi: Rework acpi_create_efi_mmap_table to avoid memory_map[offset]

[Julien Grall]

The code contains a lot of memory_map[offset]. This could be simplified
by incrementing the descriptor pointer every time.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/efi/efi-dom0.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/xen/arch/arm/efi/efi-dom0.c b/xen/arch/arm/efi/efi-dom0.c
index f307f26..f0ceaa6 100644
--- a/xen/arch/arm/efi/efi-dom0.c
+++ b/xen/arch/arm/efi/efi-dom0.c
@@ -112,25 +112,24 @@ void __init acpi_create_efi_mmap_table(struct domain *d,
                                        const struct meminfo *mem,
                                        struct membank tbl_add[])
 {
-    EFI_MEMORY_DESCRIPTOR *memory_map;
-    unsigned int i, offset;
+    EFI_MEMORY_DESCRIPTOR *desc;
+    unsigned int i;
     u8 *base_ptr;
 
     base_ptr = d->arch.efi_acpi_table
                + acpi_get_table_offset(tbl_add, TBL_MMAP);
-    memory_map = (EFI_MEMORY_DESCRIPTOR *)base_ptr;
+    desc = (EFI_MEMORY_DESCRIPTOR *)base_ptr;
 
-    offset = 0;
-    for( i = 0; i < mem->nr_banks; i++, offset++ )
-        fill_efi_memory_descriptor(&memory_map[offset], EfiConventionalMemory,
+    for ( i = 0; i < mem->nr_banks; i++, desc++ )
+        fill_efi_memory_descriptor(desc, EfiConventionalMemory,
                                    mem->bank[i].start, mem->bank[i].size);
 
-    for( i = 0; i < acpi_mem.nr_banks; i++, offset++ )
-        fill_efi_memory_descriptor(&memory_map[offset], EfiACPIReclaimMemory,
+    for ( i = 0; i < acpi_mem.nr_banks; i++, desc++ )
+        fill_efi_memory_descriptor(desc, EfiACPIReclaimMemory,
                                    acpi_mem.bank[i].start,
                                    acpi_mem.bank[i].size);
 
-    fill_efi_memory_descriptor(&memory_map[offset], EfiACPIReclaimMemory,
+    fill_efi_memory_descriptor(desc, EfiACPIReclaimMemory,
                                d->arch.efi_acpi_gpa, d->arch.efi_acpi_len);
 
     tbl_add[TBL_MMAP].start = d->arch.efi_acpi_gpa
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 7/7] xen/arm: acpi: Move the ACPI banks in bootinfo

[Julien Grall]

Currently the acpi banks are stored in a separate variable and have an
header just for them.

This variable can be moved in the structure bootinfo removing an header
and a global variable.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/efi/efi-boot.h |  3 +--
 xen/arch/arm/efi/efi-dom0.c | 12 +++++-------
 xen/arch/arm/efi/efi-dom0.h |  8 --------
 xen/include/asm-arm/setup.h |  3 +++
 4 files changed, 9 insertions(+), 17 deletions(-)
 delete mode 100644 xen/arch/arm/efi/efi-dom0.h

diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
index 757d9c6..2e3e169 100644
--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -7,7 +7,6 @@
 #include <xen/libfdt/libfdt.h>
 #include <asm/setup.h>
 #include <asm/smp.h>
-#include "efi-dom0.h"
 
 void noreturn efi_xen_start(void *fdt_ptr, uint32_t fdt_size);
 void __flush_dcache_area(const void *vaddr, unsigned long size);
@@ -164,7 +163,7 @@ static EFI_STATUS __init efi_process_memory_map_bootinfo(EFI_MEMORY_DESCRIPTOR *
 #ifdef CONFIG_ACPI
         else if ( desc_ptr->Type == EfiACPIReclaimMemory )
         {
-            if ( !meminfo_add_bank(&acpi_mem, desc_ptr) )
+            if ( !meminfo_add_bank(&bootinfo.acpi, desc_ptr) )
             {
                 PrintStr(L"Error: All " __stringify(NR_MEM_BANKS)
                           " acpi meminfo mem banks exhausted.\r\n");
diff --git a/xen/arch/arm/efi/efi-dom0.c b/xen/arch/arm/efi/efi-dom0.c
index f0ceaa6..1c35654 100644
--- a/xen/arch/arm/efi/efi-dom0.c
+++ b/xen/arch/arm/efi/efi-dom0.c
@@ -22,7 +22,6 @@
  */
 
 #include "efi.h"
-#include "efi-dom0.h"
 #include <xen/sched.h>
 #include <xen/pfn.h>
 #include <xen/libfdt/libfdt.h>
@@ -32,7 +31,6 @@
 #define XZ_EXTERN STATIC
 #include "../../../common/xz/crc32.c"
 
-struct meminfo __initdata acpi_mem;
 /* Constant to indicate "Xen" in unicode u16 format */
 static const CHAR16 xen_efi_fw_vendor[] = {0x0058, 0x0065, 0x006E, 0x0000};
 
@@ -46,7 +44,7 @@ size_t __init estimate_efi_size(int mem_nr_banks)
     int acpi_mem_nr_banks = 0;
 
     if ( !acpi_disabled )
-        acpi_mem_nr_banks = acpi_mem.nr_banks;
+        acpi_mem_nr_banks = bootinfo.acpi.nr_banks;
 
     size = ROUNDUP(est_size + ect_size + fw_vendor_size, 8);
     /* plus 1 for new created tables */
@@ -124,10 +122,10 @@ void __init acpi_create_efi_mmap_table(struct domain *d,
         fill_efi_memory_descriptor(desc, EfiConventionalMemory,
                                    mem->bank[i].start, mem->bank[i].size);
 
-    for ( i = 0; i < acpi_mem.nr_banks; i++, desc++ )
+    for ( i = 0; i < bootinfo.acpi.nr_banks; i++, desc++ )
         fill_efi_memory_descriptor(desc, EfiACPIReclaimMemory,
-                                   acpi_mem.bank[i].start,
-                                   acpi_mem.bank[i].size);
+                                   bootinfo.acpi.bank[i].start,
+                                   bootinfo.acpi.bank[i].size);
 
     fill_efi_memory_descriptor(desc, EfiACPIReclaimMemory,
                                d->arch.efi_acpi_gpa, d->arch.efi_acpi_len);
@@ -135,7 +133,7 @@ void __init acpi_create_efi_mmap_table(struct domain *d,
     tbl_add[TBL_MMAP].start = d->arch.efi_acpi_gpa
                               + acpi_get_table_offset(tbl_add, TBL_MMAP);
     tbl_add[TBL_MMAP].size = sizeof(EFI_MEMORY_DESCRIPTOR)
-                             * (mem->nr_banks + acpi_mem.nr_banks + 1);
+                             * (mem->nr_banks + bootinfo.acpi.nr_banks + 1);
 }
 
 /* Create /hypervisor/uefi node for efi properties. */
diff --git a/xen/arch/arm/efi/efi-dom0.h b/xen/arch/arm/efi/efi-dom0.h
deleted file mode 100644
index 3cd4caa..0000000
--- a/xen/arch/arm/efi/efi-dom0.h
+++ /dev/null
@@ -1,8 +0,0 @@
-#ifndef __ARM_EFI_DOM0_H__
-#define __ARM_EFI_DOM0_H__
-
-#include <asm/setup.h>
-
-extern struct meminfo acpi_mem;
-
-#endif
diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h
index 3da7a49..7c76185 100644
--- a/xen/include/asm-arm/setup.h
+++ b/xen/include/asm-arm/setup.h
@@ -43,6 +43,9 @@ struct bootmodules {
 struct bootinfo {
     struct meminfo mem;
     struct bootmodules modules;
+#ifdef CONFIG_ACPI
+    struct meminfo acpi;
+#endif
 };
 
 extern struct bootinfo bootinfo;
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 7/8] xen/arm: efi: Rework acpi_create_efi_mmap_table to avoid memory_map[offset]

[Julien Grall]

The code contains a lot of memory_map[offset]. This could be simplified
by incrementing the descriptor pointer every time.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/efi/efi-dom0.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/xen/arch/arm/efi/efi-dom0.c b/xen/arch/arm/efi/efi-dom0.c
index f307f26..f0ceaa6 100644
--- a/xen/arch/arm/efi/efi-dom0.c
+++ b/xen/arch/arm/efi/efi-dom0.c
@@ -112,25 +112,24 @@ void __init acpi_create_efi_mmap_table(struct domain *d,
                                        const struct meminfo *mem,
                                        struct membank tbl_add[])
 {
-    EFI_MEMORY_DESCRIPTOR *memory_map;
-    unsigned int i, offset;
+    EFI_MEMORY_DESCRIPTOR *desc;
+    unsigned int i;
     u8 *base_ptr;
 
     base_ptr = d->arch.efi_acpi_table
                + acpi_get_table_offset(tbl_add, TBL_MMAP);
-    memory_map = (EFI_MEMORY_DESCRIPTOR *)base_ptr;
+    desc = (EFI_MEMORY_DESCRIPTOR *)base_ptr;
 
-    offset = 0;
-    for( i = 0; i < mem->nr_banks; i++, offset++ )
-        fill_efi_memory_descriptor(&memory_map[offset], EfiConventionalMemory,
+    for ( i = 0; i < mem->nr_banks; i++, desc++ )
+        fill_efi_memory_descriptor(desc, EfiConventionalMemory,
                                    mem->bank[i].start, mem->bank[i].size);
 
-    for( i = 0; i < acpi_mem.nr_banks; i++, offset++ )
-        fill_efi_memory_descriptor(&memory_map[offset], EfiACPIReclaimMemory,
+    for ( i = 0; i < acpi_mem.nr_banks; i++, desc++ )
+        fill_efi_memory_descriptor(desc, EfiACPIReclaimMemory,
                                    acpi_mem.bank[i].start,
                                    acpi_mem.bank[i].size);
 
-    fill_efi_memory_descriptor(&memory_map[offset], EfiACPIReclaimMemory,
+    fill_efi_memory_descriptor(desc, EfiACPIReclaimMemory,
                                d->arch.efi_acpi_gpa, d->arch.efi_acpi_len);
 
     tbl_add[TBL_MMAP].start = d->arch.efi_acpi_gpa
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 7/8] xen/arm: efi: Rework acpi_create_efi_mmap_table to avoid memory_map[offset]

[Stefano Stabellini]

On Fri, 3 Feb 2017, Julien Grall wrote:
> The code contains a lot of memory_map[offset]. This could be simplified
> by incrementing the descriptor pointer every time.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

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

> ---
>  xen/arch/arm/efi/efi-dom0.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/arch/arm/efi/efi-dom0.c b/xen/arch/arm/efi/efi-dom0.c
> index f307f26..f0ceaa6 100644
> --- a/xen/arch/arm/efi/efi-dom0.c
> +++ b/xen/arch/arm/efi/efi-dom0.c
> @@ -112,25 +112,24 @@ void __init acpi_create_efi_mmap_table(struct domain *d,
>                                         const struct meminfo *mem,
>                                         struct membank tbl_add[])
>  {
> -    EFI_MEMORY_DESCRIPTOR *memory_map;
> -    unsigned int i, offset;
> +    EFI_MEMORY_DESCRIPTOR *desc;
> +    unsigned int i;
>      u8 *base_ptr;
>  
>      base_ptr = d->arch.efi_acpi_table
>                 + acpi_get_table_offset(tbl_add, TBL_MMAP);
> -    memory_map = (EFI_MEMORY_DESCRIPTOR *)base_ptr;
> +    desc = (EFI_MEMORY_DESCRIPTOR *)base_ptr;
>  
> -    offset = 0;
> -    for( i = 0; i < mem->nr_banks; i++, offset++ )
> -        fill_efi_memory_descriptor(&memory_map[offset], EfiConventionalMemory,
> +    for ( i = 0; i < mem->nr_banks; i++, desc++ )
> +        fill_efi_memory_descriptor(desc, EfiConventionalMemory,
>                                     mem->bank[i].start, mem->bank[i].size);
>  
> -    for( i = 0; i < acpi_mem.nr_banks; i++, offset++ )
> -        fill_efi_memory_descriptor(&memory_map[offset], EfiACPIReclaimMemory,
> +    for ( i = 0; i < acpi_mem.nr_banks; i++, desc++ )
> +        fill_efi_memory_descriptor(desc, EfiACPIReclaimMemory,
>                                     acpi_mem.bank[i].start,
>                                     acpi_mem.bank[i].size);
>  
> -    fill_efi_memory_descriptor(&memory_map[offset], EfiACPIReclaimMemory,
> +    fill_efi_memory_descriptor(desc, EfiACPIReclaimMemory,
>                                 d->arch.efi_acpi_gpa, d->arch.efi_acpi_len);
>  
>      tbl_add[TBL_MMAP].start = d->arch.efi_acpi_gpa
> -- 
> 1.9.1
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 8/8] xen/arm: acpi: Move the ACPI banks in bootinfo

[Julien Grall]

Currently the acpi banks are stored in a separate variable and have an
header just for them.

This variable can be moved in the structure bootinfo removing an header
and a global variable.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/efi/efi-boot.h |  3 +--
 xen/arch/arm/efi/efi-dom0.c | 12 +++++-------
 xen/arch/arm/efi/efi-dom0.h |  8 --------
 xen/include/asm-arm/setup.h |  3 +++
 4 files changed, 9 insertions(+), 17 deletions(-)
 delete mode 100644 xen/arch/arm/efi/efi-dom0.h

diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
index 757d9c6..2e3e169 100644
--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -7,7 +7,6 @@
 #include <xen/libfdt/libfdt.h>
 #include <asm/setup.h>
 #include <asm/smp.h>
-#include "efi-dom0.h"
 
 void noreturn efi_xen_start(void *fdt_ptr, uint32_t fdt_size);
 void __flush_dcache_area(const void *vaddr, unsigned long size);
@@ -164,7 +163,7 @@ static EFI_STATUS __init efi_process_memory_map_bootinfo(EFI_MEMORY_DESCRIPTOR *
 #ifdef CONFIG_ACPI
         else if ( desc_ptr->Type == EfiACPIReclaimMemory )
         {
-            if ( !meminfo_add_bank(&acpi_mem, desc_ptr) )
+            if ( !meminfo_add_bank(&bootinfo.acpi, desc_ptr) )
             {
                 PrintStr(L"Error: All " __stringify(NR_MEM_BANKS)
                           " acpi meminfo mem banks exhausted.\r\n");
diff --git a/xen/arch/arm/efi/efi-dom0.c b/xen/arch/arm/efi/efi-dom0.c
index f0ceaa6..1c35654 100644
--- a/xen/arch/arm/efi/efi-dom0.c
+++ b/xen/arch/arm/efi/efi-dom0.c
@@ -22,7 +22,6 @@
  */
 
 #include "efi.h"
-#include "efi-dom0.h"
 #include <xen/sched.h>
 #include <xen/pfn.h>
 #include <xen/libfdt/libfdt.h>
@@ -32,7 +31,6 @@
 #define XZ_EXTERN STATIC
 #include "../../../common/xz/crc32.c"
 
-struct meminfo __initdata acpi_mem;
 /* Constant to indicate "Xen" in unicode u16 format */
 static const CHAR16 xen_efi_fw_vendor[] = {0x0058, 0x0065, 0x006E, 0x0000};
 
@@ -46,7 +44,7 @@ size_t __init estimate_efi_size(int mem_nr_banks)
     int acpi_mem_nr_banks = 0;
 
     if ( !acpi_disabled )
-        acpi_mem_nr_banks = acpi_mem.nr_banks;
+        acpi_mem_nr_banks = bootinfo.acpi.nr_banks;
 
     size = ROUNDUP(est_size + ect_size + fw_vendor_size, 8);
     /* plus 1 for new created tables */
@@ -124,10 +122,10 @@ void __init acpi_create_efi_mmap_table(struct domain *d,
         fill_efi_memory_descriptor(desc, EfiConventionalMemory,
                                    mem->bank[i].start, mem->bank[i].size);
 
-    for ( i = 0; i < acpi_mem.nr_banks; i++, desc++ )
+    for ( i = 0; i < bootinfo.acpi.nr_banks; i++, desc++ )
         fill_efi_memory_descriptor(desc, EfiACPIReclaimMemory,
-                                   acpi_mem.bank[i].start,
-                                   acpi_mem.bank[i].size);
+                                   bootinfo.acpi.bank[i].start,
+                                   bootinfo.acpi.bank[i].size);
 
     fill_efi_memory_descriptor(desc, EfiACPIReclaimMemory,
                                d->arch.efi_acpi_gpa, d->arch.efi_acpi_len);
@@ -135,7 +133,7 @@ void __init acpi_create_efi_mmap_table(struct domain *d,
     tbl_add[TBL_MMAP].start = d->arch.efi_acpi_gpa
                               + acpi_get_table_offset(tbl_add, TBL_MMAP);
     tbl_add[TBL_MMAP].size = sizeof(EFI_MEMORY_DESCRIPTOR)
-                             * (mem->nr_banks + acpi_mem.nr_banks + 1);
+                             * (mem->nr_banks + bootinfo.acpi.nr_banks + 1);
 }
 
 /* Create /hypervisor/uefi node for efi properties. */
diff --git a/xen/arch/arm/efi/efi-dom0.h b/xen/arch/arm/efi/efi-dom0.h
deleted file mode 100644
index 3cd4caa..0000000
--- a/xen/arch/arm/efi/efi-dom0.h
+++ /dev/null
@@ -1,8 +0,0 @@
-#ifndef __ARM_EFI_DOM0_H__
-#define __ARM_EFI_DOM0_H__
-
-#include <asm/setup.h>
-
-extern struct meminfo acpi_mem;
-
-#endif
diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h
index 3da7a49..7c76185 100644
--- a/xen/include/asm-arm/setup.h
+++ b/xen/include/asm-arm/setup.h
@@ -43,6 +43,9 @@ struct bootmodules {
 struct bootinfo {
     struct meminfo mem;
     struct bootmodules modules;
+#ifdef CONFIG_ACPI
+    struct meminfo acpi;
+#endif
 };
 
 extern struct bootinfo bootinfo;
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 8/8] xen/arm: acpi: Move the ACPI banks in bootinfo

[Stefano Stabellini]

On Fri, 3 Feb 2017, Julien Grall wrote:
> Currently the acpi banks are stored in a separate variable and have an
> header just for them.
> 
> This variable can be moved in the structure bootinfo removing an header
> and a global variable.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

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


> ---
>  xen/arch/arm/efi/efi-boot.h |  3 +--
>  xen/arch/arm/efi/efi-dom0.c | 12 +++++-------
>  xen/arch/arm/efi/efi-dom0.h |  8 --------
>  xen/include/asm-arm/setup.h |  3 +++
>  4 files changed, 9 insertions(+), 17 deletions(-)
>  delete mode 100644 xen/arch/arm/efi/efi-dom0.h
> 
> diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
> index 757d9c6..2e3e169 100644
> --- a/xen/arch/arm/efi/efi-boot.h
> +++ b/xen/arch/arm/efi/efi-boot.h
> @@ -7,7 +7,6 @@
>  #include <xen/libfdt/libfdt.h>
>  #include <asm/setup.h>
>  #include <asm/smp.h>
> -#include "efi-dom0.h"
>  
>  void noreturn efi_xen_start(void *fdt_ptr, uint32_t fdt_size);
>  void __flush_dcache_area(const void *vaddr, unsigned long size);
> @@ -164,7 +163,7 @@ static EFI_STATUS __init efi_process_memory_map_bootinfo(EFI_MEMORY_DESCRIPTOR *
>  #ifdef CONFIG_ACPI
>          else if ( desc_ptr->Type == EfiACPIReclaimMemory )
>          {
> -            if ( !meminfo_add_bank(&acpi_mem, desc_ptr) )
> +            if ( !meminfo_add_bank(&bootinfo.acpi, desc_ptr) )
>              {
>                  PrintStr(L"Error: All " __stringify(NR_MEM_BANKS)
>                            " acpi meminfo mem banks exhausted.\r\n");
> diff --git a/xen/arch/arm/efi/efi-dom0.c b/xen/arch/arm/efi/efi-dom0.c
> index f0ceaa6..1c35654 100644
> --- a/xen/arch/arm/efi/efi-dom0.c
> +++ b/xen/arch/arm/efi/efi-dom0.c
> @@ -22,7 +22,6 @@
>   */
>  
>  #include "efi.h"
> -#include "efi-dom0.h"
>  #include <xen/sched.h>
>  #include <xen/pfn.h>
>  #include <xen/libfdt/libfdt.h>
> @@ -32,7 +31,6 @@
>  #define XZ_EXTERN STATIC
>  #include "../../../common/xz/crc32.c"
>  
> -struct meminfo __initdata acpi_mem;
>  /* Constant to indicate "Xen" in unicode u16 format */
>  static const CHAR16 xen_efi_fw_vendor[] = {0x0058, 0x0065, 0x006E, 0x0000};
>  
> @@ -46,7 +44,7 @@ size_t __init estimate_efi_size(int mem_nr_banks)
>      int acpi_mem_nr_banks = 0;
>  
>      if ( !acpi_disabled )
> -        acpi_mem_nr_banks = acpi_mem.nr_banks;
> +        acpi_mem_nr_banks = bootinfo.acpi.nr_banks;
>  
>      size = ROUNDUP(est_size + ect_size + fw_vendor_size, 8);
>      /* plus 1 for new created tables */
> @@ -124,10 +122,10 @@ void __init acpi_create_efi_mmap_table(struct domain *d,
>          fill_efi_memory_descriptor(desc, EfiConventionalMemory,
>                                     mem->bank[i].start, mem->bank[i].size);
>  
> -    for ( i = 0; i < acpi_mem.nr_banks; i++, desc++ )
> +    for ( i = 0; i < bootinfo.acpi.nr_banks; i++, desc++ )
>          fill_efi_memory_descriptor(desc, EfiACPIReclaimMemory,
> -                                   acpi_mem.bank[i].start,
> -                                   acpi_mem.bank[i].size);
> +                                   bootinfo.acpi.bank[i].start,
> +                                   bootinfo.acpi.bank[i].size);
>  
>      fill_efi_memory_descriptor(desc, EfiACPIReclaimMemory,
>                                 d->arch.efi_acpi_gpa, d->arch.efi_acpi_len);
> @@ -135,7 +133,7 @@ void __init acpi_create_efi_mmap_table(struct domain *d,
>      tbl_add[TBL_MMAP].start = d->arch.efi_acpi_gpa
>                                + acpi_get_table_offset(tbl_add, TBL_MMAP);
>      tbl_add[TBL_MMAP].size = sizeof(EFI_MEMORY_DESCRIPTOR)
> -                             * (mem->nr_banks + acpi_mem.nr_banks + 1);
> +                             * (mem->nr_banks + bootinfo.acpi.nr_banks + 1);
>  }
>  
>  /* Create /hypervisor/uefi node for efi properties. */
> diff --git a/xen/arch/arm/efi/efi-dom0.h b/xen/arch/arm/efi/efi-dom0.h
> deleted file mode 100644
> index 3cd4caa..0000000
> --- a/xen/arch/arm/efi/efi-dom0.h
> +++ /dev/null
> @@ -1,8 +0,0 @@
> -#ifndef __ARM_EFI_DOM0_H__
> -#define __ARM_EFI_DOM0_H__
> -
> -#include <asm/setup.h>
> -
> -extern struct meminfo acpi_mem;
> -
> -#endif
> diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h
> index 3da7a49..7c76185 100644
> --- a/xen/include/asm-arm/setup.h
> +++ b/xen/include/asm-arm/setup.h
> @@ -43,6 +43,9 @@ struct bootmodules {
>  struct bootinfo {
>      struct meminfo mem;
>      struct bootmodules modules;
> +#ifdef CONFIG_ACPI
> +    struct meminfo acpi;
> +#endif
>  };
>  
>  extern struct bootinfo bootinfo;
> -- 
> 1.9.1
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 0/8] xen/arm: Fix and clean-up for ACPI and EFI

[Stefano Stabellini]

On Fri, 3 Feb 2017, Julien Grall wrote:
> Hi all,
> 
> This patch series contains a bunch of fix and clean-up for ACPI and EFI on
> ARM64.
> 
> Note that the patch "xen/arm64: Don't zero BSS when booting using EFI" [1] is
> required in order to test this series.

Thanks Julien! I committed patch #1, #2 and #4.


> Cheers,
> 
> [1] https://lists.xenproject.org/archives/html/xen-devel/2017-02/msg00228.html
> 
> Julien Grall (8):
>   xen/arm: acpi: Handle correctly detection of GICv2 on GICv3
>   xen/arm: acpi: Rework acpi_boot_table_init error paths
>   xen/arm: acpi: Don't fallback on DT when user request ACPI
>   xen/arm: Print whether Xen is booting using ACPI or DT
>   xen/arm: efi: Avoid duplicating the addition of a new bank
>   xen/arm: efi: Avoid duplicating the addition of a new efi memory
>     descriptor
>   xen/arm: efi: Rework acpi_create_efi_mmap_table to avoid
>     memory_map[offset]
>   xen/arm: acpi: Move the ACPI banks in bootinfo
> 
>  xen/arch/arm/acpi/boot.c    | 30 ++++++++++++++--------
>  xen/arch/arm/efi/efi-boot.h | 35 +++++++++++++-------------
>  xen/arch/arm/efi/efi-dom0.c | 61 +++++++++++++++++++++------------------------
>  xen/arch/arm/efi/efi-dom0.h |  8 ------
>  xen/arch/arm/gic-v3.c       | 20 ++++++++++++++-
>  xen/arch/arm/setup.c        |  5 ++++
>  xen/include/asm-arm/setup.h |  3 +++
>  7 files changed, 93 insertions(+), 69 deletions(-)
>  delete mode 100644 xen/arch/arm/efi/efi-dom0.h
> 
> -- 
> 1.9.1
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel