[for-4.7 0/5] xen/arm: acpi: Bunch of fixes to use ACPI with SMP and PL011

[for-4.7 0/5] xen/arm: acpi: Bunch of fixes to use ACPI with SMP and PL011

[Julien Grall]

Hello,

This patch series fixes secondary bring up and the use of the PL011 UART driver
when Xen boots using ACPI.

Regards,

Cc: wei.liu2@citrix.com

Julien Grall (5):
  drivers/pl011: ACPI: The interrupt should always be high level
    triggered
  xen/arm: acpi: The boot CPU does not always match the first entry in
    the MADT
  xen/arm: acpi: Fix SMP support when booting with ACPI
  xen/arm: acpi: Remove uncessary check in acpi_map_gic_cpu_interface
  xen/arm: acpi: Print more error messages in acpi_map_gic_cpu_interface

 xen/arch/arm/acpi/boot.c | 38 ++++++++++++++++++++++++++------------
 xen/drivers/char/pl011.c |  2 +-
 2 files changed, 27 insertions(+), 13 deletions(-)

-- 
1.9.1


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

[for-4.7 1/5] drivers/pl011: ACPI: The interrupt should always be high level triggered

[Julien Grall]

The SPCR does not specify if the interrupt is edge or level triggered.
So the configuration needs to be hardcoded in the code.

Based on the PL011 TRM (see 2.2.8 in ARM DDI 0183G), the interrupt generated
will be active high. This wording implies the interrupt should be high level
triggered. Note that a rising edge triggered interrupt would be described as
"high going edge".

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/drivers/char/pl011.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/drivers/char/pl011.c b/xen/drivers/char/pl011.c
index fa22edf..88d8488 100644
--- a/xen/drivers/char/pl011.c
+++ b/xen/drivers/char/pl011.c
@@ -327,7 +327,7 @@ static int __init pl011_acpi_uart_init(const void *data)
     }
 
     /* trigger/polarity information is not available in spcr */
-    irq_set_type(spcr->interrupt, IRQ_TYPE_EDGE_BOTH);
+    irq_set_type(spcr->interrupt, IRQ_TYPE_LEVEL_MASK);
 
     res = pl011_uart_init(spcr->interrupt, spcr->serial_port.address,
                           PAGE_SIZE);
-- 
1.9.1


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

[for-4.7 2/5] xen/arm: acpi: The boot CPU does not always match the first entry in the MADT

[Julien Grall]

Since the ACPI 6.0 errata document [1], the first entry in the MADT
does not have to correspond to the boot CPU.

Introduce a new variable to know if a MADT entry matching the boot CPU
is found. Furthermore, it's not necessary to check if the MPIDR is
duplicated for the boot CPU. So the rest of the function can be skipped.

[1] 1380 Unnecessary restrictions to FW vendors in ordering of GIC structures
in MADT

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

diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c
index 859aa86..2a71660 100644
--- a/xen/arch/arm/acpi/boot.c
+++ b/xen/arch/arm/acpi/boot.c
@@ -37,7 +37,8 @@
 #include <asm/setup.h>
 
 /* Processors with enabled flag and sane MPIDR */
-static unsigned int enabled_cpus;
+static unsigned int enabled_cpus = 1;
+static bool __initdata bootcpu_valid;
 
 /* total number of cpus in this system */
 static unsigned int __initdata total_cpus;
@@ -71,10 +72,15 @@ acpi_map_gic_cpu_interface(struct acpi_madt_generic_interrupt *processor)
     }
 
     /* Check if GICC structure of boot CPU is available in the MADT */
-    if ( (enabled_cpus == 0) && (cpu_logical_map(0) != mpidr) )
+    if ( cpu_logical_map(0) == mpidr )
     {
-        printk("Firmware bug, invalid CPU MPIDR for cpu0: 0x%"PRIx64" in MADT\n",
-               mpidr);
+        if ( bootcpu_valid )
+        {
+            printk("Firmware bug, duplicate boot CPU MPIDR: 0x%"PRIx64" in MADT\n",
+                   mpidr);
+            return;
+        }
+        bootcpu_valid = true;
         return;
     }
 
-- 
1.9.1


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

[for-4.7 3/5] xen/arm: acpi: Fix SMP support when booting with ACPI

[Julien Grall]

The variable enabled_cpus is used to know the number of CPU enabled in
the MADT.

Currently this variable is used to check the validity of the boot CPU.
It will be considered invalid when "enabled_cpus > 1".

However, this condition also means that multiple CPUs are present on the
system. So secondary will never be brought up.

The correct way to check the validity of the boot CPU is to use the
variable bootcpu_valid.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/acpi/boot.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c
index 2a71660..fd29bdc 100644
--- a/xen/arch/arm/acpi/boot.c
+++ b/xen/arch/arm/acpi/boot.c
@@ -149,7 +149,7 @@ void __init acpi_smp_init_cpus(void)
         return;
     }
 
-    if ( enabled_cpus > 1 )
+    if ( !bootcpu_valid )
     {
         printk("MADT missing boot CPU MPIDR, not enabling secondaries\n");
         return;
-- 
1.9.1


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

[for-4.7 4/5] xen/arm: acpi: Remove uncessary check in acpi_map_gic_cpu_interface

[Julien Grall]

This part of the code will never be executed when the entry
corresponds to the boot CPU.

Also print an error message rather when arch_cpu_init has failed.

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

diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c
index fd29bdc..602ab39 100644
--- a/xen/arch/arm/acpi/boot.c
+++ b/xen/arch/arm/acpi/boot.c
@@ -51,6 +51,7 @@ static void __init
 acpi_map_gic_cpu_interface(struct acpi_madt_generic_interrupt *processor)
 {
     int i;
+    int rc;
     u64 mpidr = processor->arm_mpidr & MPIDR_HWID_MASK;
     bool_t enabled = !!(processor->flags & ACPI_MADT_ENABLED);
 
@@ -102,16 +103,16 @@ acpi_map_gic_cpu_interface(struct acpi_madt_generic_interrupt *processor)
     if ( !acpi_psci_present() )
         return;
 
-    /* CPU 0 was already initialized */
-    if ( enabled_cpus )
+    if ( (rc = arch_cpu_init(enabled_cpus, NULL)) < 0 )
     {
-        if ( arch_cpu_init(enabled_cpus, NULL) < 0 )
-            return;
-
-        /* map the logical cpu id to cpu MPIDR */
-        cpu_logical_map(enabled_cpus) = mpidr;
+        printk("cpu%d: init failed (0x%"PRIx64" MPIDR): %d\n",
+               enabled_cpus, mpidr, rc);
+        return;
     }
 
+    /* map the logical cpu id to cpu MPIDR */
+    cpu_logical_map(enabled_cpus) = mpidr;
+
     enabled_cpus++;
 }
 
-- 
1.9.1


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

[for-4.7 5/5] xen/arm: acpi: Print more error messages in acpi_map_gic_cpu_interface

[Julien Grall]

It's helpful to spot any error without having to modify the hypervisor
code.

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

diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c
index 602ab39..23285f7 100644
--- a/xen/arch/arm/acpi/boot.c
+++ b/xen/arch/arm/acpi/boot.c
@@ -63,7 +63,10 @@ acpi_map_gic_cpu_interface(struct acpi_madt_generic_interrupt *processor)
 
     total_cpus++;
     if ( !enabled )
+    {
+        printk("Skipping disabled CPU entry with 0x%"PRIx64" MPIDR\n", mpidr);
         return;
+    }
 
     if ( enabled_cpus >=  NR_CPUS )
     {
@@ -101,7 +104,11 @@ acpi_map_gic_cpu_interface(struct acpi_madt_generic_interrupt *processor)
     }
 
     if ( !acpi_psci_present() )
+    {
+        printk("PSCI not present, skipping CPU MPIDR 0x%"PRIx64"\n",
+               mpidr);
         return;
+    }
 
     if ( (rc = arch_cpu_init(enabled_cpus, NULL)) < 0 )
     {
-- 
1.9.1


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

Re: [for-4.7 3/5] xen/arm: acpi: Fix SMP support when booting with ACPI

[Shannon Zhao]

On 2016/4/7 18:59, Julien Grall wrote:
> The variable enabled_cpus is used to know the number of CPU enabled in
> the MADT.
> 
> Currently this variable is used to check the validity of the boot CPU.
> It will be considered invalid when "enabled_cpus > 1".
> 
> However, this condition also means that multiple CPUs are present on the
> system. So secondary will never be brought up.
> 
> The correct way to check the validity of the boot CPU is to use the
> variable bootcpu_valid.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Reviewed-by: Shannon Zhao <shannon.zhao@linaro.org>

> ---
>  xen/arch/arm/acpi/boot.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c
> index 2a71660..fd29bdc 100644
> --- a/xen/arch/arm/acpi/boot.c
> +++ b/xen/arch/arm/acpi/boot.c
> @@ -149,7 +149,7 @@ void __init acpi_smp_init_cpus(void)
>          return;
>      }
>  
> -    if ( enabled_cpus > 1 )
> +    if ( !bootcpu_valid )
>      {
>          printk("MADT missing boot CPU MPIDR, not enabling secondaries\n");
>          return;
> 

-- 
Shannon


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

Re: [for-4.7 5/5] xen/arm: acpi: Print more error messages in acpi_map_gic_cpu_interface

[Shannon Zhao]

On 2016/4/7 18:59, Julien Grall wrote:
> It's helpful to spot any error without having to modify the hypervisor
> code.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Reviewed-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
>  xen/arch/arm/acpi/boot.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c
> index 602ab39..23285f7 100644
> --- a/xen/arch/arm/acpi/boot.c
> +++ b/xen/arch/arm/acpi/boot.c
> @@ -63,7 +63,10 @@ acpi_map_gic_cpu_interface(struct acpi_madt_generic_interrupt *processor)
>  
>      total_cpus++;
>      if ( !enabled )
> +    {
> +        printk("Skipping disabled CPU entry with 0x%"PRIx64" MPIDR\n", mpidr);
>          return;
> +    }
>  
>      if ( enabled_cpus >=  NR_CPUS )
>      {
> @@ -101,7 +104,11 @@ acpi_map_gic_cpu_interface(struct acpi_madt_generic_interrupt *processor)
>      }
>  
>      if ( !acpi_psci_present() )
> +    {
> +        printk("PSCI not present, skipping CPU MPIDR 0x%"PRIx64"\n",
> +               mpidr);
>          return;
> +    }
>  
>      if ( (rc = arch_cpu_init(enabled_cpus, NULL)) < 0 )
>      {
> 

-- 
Shannon


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

Re: [for-4.7 4/5] xen/arm: acpi: Remove uncessary check in acpi_map_gic_cpu_interface

[Shannon Zhao]

On 2016/4/7 18:59, Julien Grall wrote:
> This part of the code will never be executed when the entry
> corresponds to the boot CPU.
> 
> Also print an error message rather when arch_cpu_init has failed.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Reviewed-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
>  xen/arch/arm/acpi/boot.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c
> index fd29bdc..602ab39 100644
> --- a/xen/arch/arm/acpi/boot.c
> +++ b/xen/arch/arm/acpi/boot.c
> @@ -51,6 +51,7 @@ static void __init
>  acpi_map_gic_cpu_interface(struct acpi_madt_generic_interrupt *processor)
>  {
>      int i;
> +    int rc;
>      u64 mpidr = processor->arm_mpidr & MPIDR_HWID_MASK;
>      bool_t enabled = !!(processor->flags & ACPI_MADT_ENABLED);
>  
> @@ -102,16 +103,16 @@ acpi_map_gic_cpu_interface(struct acpi_madt_generic_interrupt *processor)
>      if ( !acpi_psci_present() )
>          return;
>  
> -    /* CPU 0 was already initialized */
> -    if ( enabled_cpus )
> +    if ( (rc = arch_cpu_init(enabled_cpus, NULL)) < 0 )
>      {
> -        if ( arch_cpu_init(enabled_cpus, NULL) < 0 )
> -            return;
> -
> -        /* map the logical cpu id to cpu MPIDR */
> -        cpu_logical_map(enabled_cpus) = mpidr;
> +        printk("cpu%d: init failed (0x%"PRIx64" MPIDR): %d\n",
> +               enabled_cpus, mpidr, rc);
> +        return;
>      }
>  
> +    /* map the logical cpu id to cpu MPIDR */
> +    cpu_logical_map(enabled_cpus) = mpidr;
> +
>      enabled_cpus++;
>  }
>  
> 

-- 
Shannon


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

Re: [for-4.7 1/5] drivers/pl011: ACPI: The interrupt should always be high level triggered

[Shannon Zhao]

On 2016/4/7 18:59, Julien Grall wrote:
> The SPCR does not specify if the interrupt is edge or level triggered.
> So the configuration needs to be hardcoded in the code.
> 
> Based on the PL011 TRM (see 2.2.8 in ARM DDI 0183G), the interrupt generated
> will be active high. This wording implies the interrupt should be high level
> triggered.
I think active high can stand rising edge triggered for edge triggered
interrupt.

E.g. see "Table 5-118 Flag Definitions: Virtual Timer, EL2 timers, and
Secure & Non-Secure EL1 timers" in ACPI SPEC 6.0.

> Note that a rising edge triggered interrupt would be described as
> "high going edge".
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>  xen/drivers/char/pl011.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/drivers/char/pl011.c b/xen/drivers/char/pl011.c
> index fa22edf..88d8488 100644
> --- a/xen/drivers/char/pl011.c
> +++ b/xen/drivers/char/pl011.c
> @@ -327,7 +327,7 @@ static int __init pl011_acpi_uart_init(const void *data)
>      }
>  
>      /* trigger/polarity information is not available in spcr */
> -    irq_set_type(spcr->interrupt, IRQ_TYPE_EDGE_BOTH);
> +    irq_set_type(spcr->interrupt, IRQ_TYPE_LEVEL_MASK);
>  
>      res = pl011_uart_init(spcr->interrupt, spcr->serial_port.address,
>                            PAGE_SIZE);
> 

-- 
Shannon


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

Re: [for-4.7 2/5] xen/arm: acpi: The boot CPU does not always match the first entry in the MADT

[Shannon Zhao]

On 2016/4/7 18:59, Julien Grall wrote:
> Since the ACPI 6.0 errata document [1], the first entry in the MADT
> does not have to correspond to the boot CPU.
> 
> Introduce a new variable to know if a MADT entry matching the boot CPU
> is found. Furthermore, it's not necessary to check if the MPIDR is
> duplicated for the boot CPU. So the rest of the function can be skipped.
> 
> [1] 1380 Unnecessary restrictions to FW vendors in ordering of GIC structures
> in MADT
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Reviewed-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
>  xen/arch/arm/acpi/boot.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c
> index 859aa86..2a71660 100644
> --- a/xen/arch/arm/acpi/boot.c
> +++ b/xen/arch/arm/acpi/boot.c
> @@ -37,7 +37,8 @@
>  #include <asm/setup.h>
>  
>  /* Processors with enabled flag and sane MPIDR */
> -static unsigned int enabled_cpus;
> +static unsigned int enabled_cpus = 1;
> +static bool __initdata bootcpu_valid;
>  
>  /* total number of cpus in this system */
>  static unsigned int __initdata total_cpus;
> @@ -71,10 +72,15 @@ acpi_map_gic_cpu_interface(struct acpi_madt_generic_interrupt *processor)
>      }
>  
>      /* Check if GICC structure of boot CPU is available in the MADT */
> -    if ( (enabled_cpus == 0) && (cpu_logical_map(0) != mpidr) )
> +    if ( cpu_logical_map(0) == mpidr )
>      {
> -        printk("Firmware bug, invalid CPU MPIDR for cpu0: 0x%"PRIx64" in MADT\n",
> -               mpidr);
> +        if ( bootcpu_valid )
> +        {
> +            printk("Firmware bug, duplicate boot CPU MPIDR: 0x%"PRIx64" in MADT\n",
> +                   mpidr);
> +            return;
> +        }
> +        bootcpu_valid = true;
>          return;
>      }
>  
> 

-- 
Shannon


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

Re: [for-4.7 1/5] drivers/pl011: ACPI: The interrupt should always be high level triggered

[Julien Grall]

Hi Shannon,

Thank you for the review.

On 07/04/16 13:30, Shannon Zhao wrote:
>
>
> On 2016/4/7 18:59, Julien Grall wrote:
>> The SPCR does not specify if the interrupt is edge or level triggered.
>> So the configuration needs to be hardcoded in the code.
>>
>> Based on the PL011 TRM (see 2.2.8 in ARM DDI 0183G), the interrupt generated
>> will be active high. This wording implies the interrupt should be high level
>> triggered.
> I think active high can stand rising edge triggered for edge triggered
> interrupt.
>
> E.g. see "Table 5-118 Flag Definitions: Virtual Timer, EL2 timers, and
> Secure & Non-Secure EL1 timers" in ACPI SPEC 6.0.

I've spoken with multiple person about the wording and the consensus is 
"active high" would imply high level triggered. So it's very ambiguous.

However, the PL011 is always using a high level triggered. You can look 
at the device tree bindings such as the one for the foundation model.

Also, the SBSA (section 4.3.2 in ARM-DEN-0029 v2.3) states the PL011 
implemented with a level triggered interrupt.

Note, I wasn't able to get the serial console working on my platform 
with edge triggered interrupt.

Regards,

-- 
Julien Grall

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

Re: [for-4.7 1/5] drivers/pl011: ACPI: The interrupt should always be high level triggered

[Shannon Zhao]

On 2016年04月07日 21:41, Julien Grall wrote:
> 
> On 07/04/16 13:30, Shannon Zhao wrote:
>>
>>
>> On 2016/4/7 18:59, Julien Grall wrote:
>>> The SPCR does not specify if the interrupt is edge or level triggered.
>>> So the configuration needs to be hardcoded in the code.
>>>
>>> Based on the PL011 TRM (see 2.2.8 in ARM DDI 0183G), the interrupt
>>> generated
>>> will be active high. This wording implies the interrupt should be
>>> high level
>>> triggered.
>> I think active high can stand rising edge triggered for edge triggered
>> interrupt.
>>
>> E.g. see "Table 5-118 Flag Definitions: Virtual Timer, EL2 timers, and
>> Secure & Non-Secure EL1 timers" in ACPI SPEC 6.0.
> 
> I've spoken with multiple person about the wording and the consensus is
> "active high" would imply high level triggered. So it's very ambiguous.
> 
> However, the PL011 is always using a high level triggered. You can look
> at the device tree bindings such as the one for the foundation model.
> 
> Also, the SBSA (section 4.3.2 in ARM-DEN-0029 v2.3) states the PL011
> implemented with a level triggered interrupt.
> 
> Note, I wasn't able to get the serial console working on my platform
> with edge triggered interrupt.

So how about IRQ_TYPE_LEVEL_HIGH instead of IRQ_TYPE_LEVEL_MASK?

Thanks,
-- 
Shannon

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

Re: [for-4.7 1/5] drivers/pl011: ACPI: The interrupt should always be high level triggered

[Julien Grall]

On 07/04/16 14:57, Shannon Zhao wrote:
> On 2016年04月07日 21:41, Julien Grall wrote:
>>
>> On 07/04/16 13:30, Shannon Zhao wrote:
>>>
>>>
>>> On 2016/4/7 18:59, Julien Grall wrote:
>>>> The SPCR does not specify if the interrupt is edge or level triggered.
>>>> So the configuration needs to be hardcoded in the code.
>>>>
>>>> Based on the PL011 TRM (see 2.2.8 in ARM DDI 0183G), the interrupt
>>>> generated
>>>> will be active high. This wording implies the interrupt should be
>>>> high level
>>>> triggered.
>>> I think active high can stand rising edge triggered for edge triggered
>>> interrupt.
>>>
>>> E.g. see "Table 5-118 Flag Definitions: Virtual Timer, EL2 timers, and
>>> Secure & Non-Secure EL1 timers" in ACPI SPEC 6.0.
>>
>> I've spoken with multiple person about the wording and the consensus is
>> "active high" would imply high level triggered. So it's very ambiguous.
>>
>> However, the PL011 is always using a high level triggered. You can look
>> at the device tree bindings such as the one for the foundation model.
>>
>> Also, the SBSA (section 4.3.2 in ARM-DEN-0029 v2.3) states the PL011
>> implemented with a level triggered interrupt.
>>
>> Note, I wasn't able to get the serial console working on my platform
>> with edge triggered interrupt.
>
> So how about IRQ_TYPE_LEVEL_HIGH instead of IRQ_TYPE_LEVEL_MASK?

Good point. I will likely resend only this patch and update the commit 
message too.

Regards,

-- 
Julien Grall

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

Re: [for-4.7 2/5] xen/arm: acpi: The boot CPU does not always match the first entry in the MADT

[Stefano Stabellini]

On Thu, 7 Apr 2016, Julien Grall wrote:
> Since the ACPI 6.0 errata document [1], the first entry in the MADT
> does not have to correspond to the boot CPU.
> 
> Introduce a new variable to know if a MADT entry matching the boot CPU
> is found. Furthermore, it's not necessary to check if the MPIDR is
> duplicated for the boot CPU. So the rest of the function can be skipped.
> 
> [1] 1380 Unnecessary restrictions to FW vendors in ordering of GIC structures
> in MADT
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/arm/acpi/boot.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c
> index 859aa86..2a71660 100644
> --- a/xen/arch/arm/acpi/boot.c
> +++ b/xen/arch/arm/acpi/boot.c
> @@ -37,7 +37,8 @@
>  #include <asm/setup.h>
>  
>  /* Processors with enabled flag and sane MPIDR */
> -static unsigned int enabled_cpus;
> +static unsigned int enabled_cpus = 1;
> +static bool __initdata bootcpu_valid;
>  
>  /* total number of cpus in this system */
>  static unsigned int __initdata total_cpus;
> @@ -71,10 +72,15 @@ acpi_map_gic_cpu_interface(struct acpi_madt_generic_interrupt *processor)
>      }
>  
>      /* Check if GICC structure of boot CPU is available in the MADT */
> -    if ( (enabled_cpus == 0) && (cpu_logical_map(0) != mpidr) )
> +    if ( cpu_logical_map(0) == mpidr )
>      {
> -        printk("Firmware bug, invalid CPU MPIDR for cpu0: 0x%"PRIx64" in MADT\n",
> -               mpidr);
> +        if ( bootcpu_valid )
> +        {
> +            printk("Firmware bug, duplicate boot CPU MPIDR: 0x%"PRIx64" in MADT\n",
> +                   mpidr);
> +            return;
> +        }
> +        bootcpu_valid = true;
>          return;
>      }

In that case you can start the other loop below from i = 1, instead of
i = 0.

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

Re: [for-4.7 3/5] xen/arm: acpi: Fix SMP support when booting with ACPI

[Stefano Stabellini]

On Thu, 7 Apr 2016, Julien Grall wrote:
> The variable enabled_cpus is used to know the number of CPU enabled in
> the MADT.
> 
> Currently this variable is used to check the validity of the boot CPU.
> It will be considered invalid when "enabled_cpus > 1".
> 
> However, this condition also means that multiple CPUs are present on the
> system. So secondary will never be brought up.
> 
> The correct way to check the validity of the boot CPU is to use the
> variable bootcpu_valid.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>


>  xen/arch/arm/acpi/boot.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c
> index 2a71660..fd29bdc 100644
> --- a/xen/arch/arm/acpi/boot.c
> +++ b/xen/arch/arm/acpi/boot.c
> @@ -149,7 +149,7 @@ void __init acpi_smp_init_cpus(void)
>          return;
>      }
>  
> -    if ( enabled_cpus > 1 )
> +    if ( !bootcpu_valid )
>      {
>          printk("MADT missing boot CPU MPIDR, not enabling secondaries\n");
>          return;
> -- 
> 1.9.1
> 

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

Re: [for-4.7 4/5] xen/arm: acpi: Remove uncessary check in acpi_map_gic_cpu_interface

[Stefano Stabellini]

On Thu, 7 Apr 2016, Julien Grall wrote:
> This part of the code will never be executed when the entry
> corresponds to the boot CPU.
> 
> Also print an error message rather when arch_cpu_init has failed.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>


>  xen/arch/arm/acpi/boot.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c
> index fd29bdc..602ab39 100644
> --- a/xen/arch/arm/acpi/boot.c
> +++ b/xen/arch/arm/acpi/boot.c
> @@ -51,6 +51,7 @@ static void __init
>  acpi_map_gic_cpu_interface(struct acpi_madt_generic_interrupt *processor)
>  {
>      int i;
> +    int rc;
>      u64 mpidr = processor->arm_mpidr & MPIDR_HWID_MASK;
>      bool_t enabled = !!(processor->flags & ACPI_MADT_ENABLED);
>  
> @@ -102,16 +103,16 @@ acpi_map_gic_cpu_interface(struct acpi_madt_generic_interrupt *processor)
>      if ( !acpi_psci_present() )
>          return;
>  
> -    /* CPU 0 was already initialized */
> -    if ( enabled_cpus )
> +    if ( (rc = arch_cpu_init(enabled_cpus, NULL)) < 0 )
>      {
> -        if ( arch_cpu_init(enabled_cpus, NULL) < 0 )
> -            return;
> -
> -        /* map the logical cpu id to cpu MPIDR */
> -        cpu_logical_map(enabled_cpus) = mpidr;
> +        printk("cpu%d: init failed (0x%"PRIx64" MPIDR): %d\n",
> +               enabled_cpus, mpidr, rc);
> +        return;
>      }
>  
> +    /* map the logical cpu id to cpu MPIDR */
> +    cpu_logical_map(enabled_cpus) = mpidr;
> +
>      enabled_cpus++;
>  }
>  
> -- 
> 1.9.1
> 

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

Re: [for-4.7 5/5] xen/arm: acpi: Print more error messages in acpi_map_gic_cpu_interface

[Stefano Stabellini]

On Thu, 7 Apr 2016, Julien Grall wrote:
> It's helpful to spot any error without having to modify the hypervisor
> code.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>


>  xen/arch/arm/acpi/boot.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c
> index 602ab39..23285f7 100644
> --- a/xen/arch/arm/acpi/boot.c
> +++ b/xen/arch/arm/acpi/boot.c
> @@ -63,7 +63,10 @@ acpi_map_gic_cpu_interface(struct acpi_madt_generic_interrupt *processor)
>  
>      total_cpus++;
>      if ( !enabled )
> +    {
> +        printk("Skipping disabled CPU entry with 0x%"PRIx64" MPIDR\n", mpidr);
>          return;
> +    }
>  
>      if ( enabled_cpus >=  NR_CPUS )
>      {
> @@ -101,7 +104,11 @@ acpi_map_gic_cpu_interface(struct acpi_madt_generic_interrupt *processor)
>      }
>  
>      if ( !acpi_psci_present() )
> +    {
> +        printk("PSCI not present, skipping CPU MPIDR 0x%"PRIx64"\n",
> +               mpidr);
>          return;
> +    }
>  
>      if ( (rc = arch_cpu_init(enabled_cpus, NULL)) < 0 )
>      {
> -- 
> 1.9.1
> 

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