1. Patchew
  2. Xen
  3. View series

[PATCH] xen/arm: vtimer: Don't read/use the secure physical timer interrupt for ACPI

Julien Grall posted 1 patch 2 years, 4 months ago
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20231005165454.18143-1-julien@xen.org
xen/arch/arm/time.c   |  4 ----
xen/arch/arm/vtimer.c | 17 +++++++++++++++--
2 files changed, 15 insertions(+), 6 deletions(-)
[PATCH] xen/arm: vtimer: Don't read/use the secure physical timer interrupt for ACPI
Posted by Julien Grall 2 years, 4 months ago 
From: Julien Grall <jgrall@amazon.com>

Per ACPI 6.5 section 5.2.25 ("Generic Timer Description Table (GTDT)"),
the fields "Secure EL1 Timer GSIV/Flags" are optional and an OS running
in non-secure world is meant to ignore the values.

However, Xen is trying to reserve the value. When booting on Graviton
2 metal instances, this would result to crash a boot because the
value is 0 which is already reserved (I haven't checked for which device).
While nothing prevent a PPI to be shared, the field should have been
ignored by Xen.

For the Device-Tree case, I couldn't find a statement suggesting
that the secure physical timer interrupt  is ignored. In fact, I have
found some code in Linux using it as a fallback. That said, it should
never be used.

As I am not aware of any issue when booting using Device-Tree, the
physical timer interrupt is only ignored for ACPI.

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

----

This has not been tested on Graviton 2 because I can't seem to get
the serial console working properly. @Dan would you be able to try it?

It would also be good to understand why 0 why already reserved. This
may be a sign for other issues in the ACPI code.
---
 xen/arch/arm/time.c   |  4 ----
 xen/arch/arm/vtimer.c | 17 +++++++++++++++--
 2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
index 3535bd8ac7c7..8fc14cd3ff62 100644
--- a/xen/arch/arm/time.c
+++ b/xen/arch/arm/time.c
@@ -78,10 +78,6 @@ static int __init arch_timer_acpi_init(struct acpi_table_header *header)
     irq_set_type(gtdt->non_secure_el1_interrupt, irq_type);
     timer_irq[TIMER_PHYS_NONSECURE_PPI] = gtdt->non_secure_el1_interrupt;
 
-    irq_type = acpi_get_timer_irq_type(gtdt->secure_el1_flags);
-    irq_set_type(gtdt->secure_el1_interrupt, irq_type);
-    timer_irq[TIMER_PHYS_SECURE_PPI] = gtdt->secure_el1_interrupt;
-
     irq_type = acpi_get_timer_irq_type(gtdt->virtual_timer_flags);
     irq_set_type(gtdt->virtual_timer_interrupt, irq_type);
     timer_irq[TIMER_VIRT_PPI] = gtdt->virtual_timer_interrupt;
diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
index c54360e20266..e73ae33c1b58 100644
--- a/xen/arch/arm/vtimer.c
+++ b/xen/arch/arm/vtimer.c
@@ -8,6 +8,7 @@
  * Copyright (c) 2011 Citrix Systems.
  */
 
+#include <xen/acpi.h>
 #include <xen/lib.h>
 #include <xen/perfc.h>
 #include <xen/sched.h>
@@ -61,10 +62,22 @@ int domain_vtimer_init(struct domain *d, struct xen_arch_domainconfig *config)
 
     config->clock_frequency = timer_dt_clock_frequency;
 
-    /* At this stage vgic_reserve_virq can't fail */
+    /*
+     * Per the ACPI specification, providing a secure EL1 timer
+     * interrupt is optional and will be ignored by non-secure OS.
+     * Therefore don't reserve the interrupt number for the HW domain
+     * and ACPI.
+     *
+     * Note that we should still reserve it when using the Device-Tree
+     * because the interrupt is not optional. That said, we are not
+     * expecting any OS to use it when running on top of Xen.
+     *
+     * At this stage vgic_reserve_virq() is not meant to fail.
+    */
     if ( is_hardware_domain(d) )
     {
-        if ( !vgic_reserve_virq(d, timer_get_irq(TIMER_PHYS_SECURE_PPI)) )
+        if ( acpi_disabled &&
+             !vgic_reserve_virq(d, timer_get_irq(TIMER_PHYS_SECURE_PPI)) )
             BUG();
 
         if ( !vgic_reserve_virq(d, timer_get_irq(TIMER_PHYS_NONSECURE_PPI)) )
-- 
2.40.1
RE: [PATCH] xen/arm: vtimer: Don't read/use the secure physical timer interrupt for ACPI
Posted by Driscoll, Dan 2 years, 4 months ago 
Julien,

	Verified this patch works on Graviton 2... so looks good from this perspective.

Thanks,
Dan

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Thursday, October 5, 2023 11:55 AM
> To: xen-devel@lists.xenproject.org
> Cc: Henry.Wang@arm.com; Driscoll, Dan (DI SW CAS ES TO)
> <dan.driscoll@siemens.com>; Raghuraman, Arvind (DI SW CAS ES)
> <arvind.raghuraman@siemens.com>; michal.orzel@amd.com; Julien Grall
> <jgrall@amazon.com>; Stefano Stabellini <sstabellini@kernel.org>; Julien Grall
> <julien@xen.org>; Bertrand Marquis <bertrand.marquis@arm.com>; Volodymyr
> Babchuk <Volodymyr_Babchuk@epam.com>
> Subject: [PATCH] xen/arm: vtimer: Don't read/use the secure physical timer
> interrupt for ACPI
> 
> From: Julien Grall <jgrall@amazon.com>
> 
> Per ACPI 6.5 section 5.2.25 ("Generic Timer Description Table (GTDT)"), the fields
> "Secure EL1 Timer GSIV/Flags" are optional and an OS running in non-secure
> world is meant to ignore the values.
> 
> However, Xen is trying to reserve the value. When booting on Graviton
> 2 metal instances, this would result to crash a boot because the value is 0 which is
> already reserved (I haven't checked for which device).
> While nothing prevent a PPI to be shared, the field should have been ignored by
> Xen.
> 
> For the Device-Tree case, I couldn't find a statement suggesting that the secure
> physical timer interrupt  is ignored. In fact, I have found some code in Linux using it
> as a fallback. That said, it should never be used.
> 
> As I am not aware of any issue when booting using Device-Tree, the physical timer
> interrupt is only ignored for ACPI.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> ----
> 
> This has not been tested on Graviton 2 because I can't seem to get the serial
> console working properly. @Dan would you be able to try it?
> 
> It would also be good to understand why 0 why already reserved. This may be a
> sign for other issues in the ACPI code.
> ---
>  xen/arch/arm/time.c   |  4 ----
>  xen/arch/arm/vtimer.c | 17 +++++++++++++++--
>  2 files changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c index
> 3535bd8ac7c7..8fc14cd3ff62 100644
> --- a/xen/arch/arm/time.c
> +++ b/xen/arch/arm/time.c
> @@ -78,10 +78,6 @@ static int __init arch_timer_acpi_init(struct
> acpi_table_header *header)
>      irq_set_type(gtdt->non_secure_el1_interrupt, irq_type);
>      timer_irq[TIMER_PHYS_NONSECURE_PPI] = gtdt->non_secure_el1_interrupt;
> 
> -    irq_type = acpi_get_timer_irq_type(gtdt->secure_el1_flags);
> -    irq_set_type(gtdt->secure_el1_interrupt, irq_type);
> -    timer_irq[TIMER_PHYS_SECURE_PPI] = gtdt->secure_el1_interrupt;
> -
>      irq_type = acpi_get_timer_irq_type(gtdt->virtual_timer_flags);
>      irq_set_type(gtdt->virtual_timer_interrupt, irq_type);
>      timer_irq[TIMER_VIRT_PPI] = gtdt->virtual_timer_interrupt; diff --git
> a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c index
> c54360e20266..e73ae33c1b58 100644
> --- a/xen/arch/arm/vtimer.c
> +++ b/xen/arch/arm/vtimer.c
> @@ -8,6 +8,7 @@
>   * Copyright (c) 2011 Citrix Systems.
>   */
> 
> +#include <xen/acpi.h>
>  #include <xen/lib.h>
>  #include <xen/perfc.h>
>  #include <xen/sched.h>
> @@ -61,10 +62,22 @@ int domain_vtimer_init(struct domain *d, struct
> xen_arch_domainconfig *config)
> 
>      config->clock_frequency = timer_dt_clock_frequency;
> 
> -    /* At this stage vgic_reserve_virq can't fail */
> +    /*
> +     * Per the ACPI specification, providing a secure EL1 timer
> +     * interrupt is optional and will be ignored by non-secure OS.
> +     * Therefore don't reserve the interrupt number for the HW domain
> +     * and ACPI.
> +     *
> +     * Note that we should still reserve it when using the Device-Tree
> +     * because the interrupt is not optional. That said, we are not
> +     * expecting any OS to use it when running on top of Xen.
> +     *
> +     * At this stage vgic_reserve_virq() is not meant to fail.
> +    */
>      if ( is_hardware_domain(d) )
>      {
> -        if ( !vgic_reserve_virq(d, timer_get_irq(TIMER_PHYS_SECURE_PPI)) )
> +        if ( acpi_disabled &&
> +             !vgic_reserve_virq(d,
> + timer_get_irq(TIMER_PHYS_SECURE_PPI)) )
>              BUG();
> 
>          if ( !vgic_reserve_virq(d, timer_get_irq(TIMER_PHYS_NONSECURE_PPI)) )
> --
> 2.40.1
Re: [PATCH] xen/arm: vtimer: Don't read/use the secure physical timer interrupt for ACPI
Posted by Julien Grall 2 years, 3 months ago 
Hi Dan,

On 10/10/2023 18:11, Driscoll, Dan wrote:
> Julien,
> 
> 	Verified this patch works on Graviton 2... so looks good from this perspective.

Thanks for testing. I will commit the patch then to staging so it will 
be included in the next release (4.18.0).

Cheers,

-- 
Julien Grall
Re: [PATCH] xen/arm: vtimer: Don't read/use the secure physical timer interrupt for ACPI
Posted by Stefano Stabellini 2 years, 4 months ago 
On Thu, 5 Oct 2023, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> Per ACPI 6.5 section 5.2.25 ("Generic Timer Description Table (GTDT)"),
> the fields "Secure EL1 Timer GSIV/Flags" are optional and an OS running
> in non-secure world is meant to ignore the values.
> 
> However, Xen is trying to reserve the value. When booting on Graviton
> 2 metal instances, this would result to crash a boot because the
> value is 0 which is already reserved (I haven't checked for which device).
> While nothing prevent a PPI to be shared, the field should have been
> ignored by Xen.
> 
> For the Device-Tree case, I couldn't find a statement suggesting
> that the secure physical timer interrupt  is ignored. In fact, I have
> found some code in Linux using it as a fallback. That said, it should
> never be used.
> 
> As I am not aware of any issue when booting using Device-Tree, the
> physical timer interrupt is only ignored for ACPI.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> ----
> 
> This has not been tested on Graviton 2 because I can't seem to get
> the serial console working properly. @Dan would you be able to try it?
> 
> It would also be good to understand why 0 why already reserved. This
> may be a sign for other issues in the ACPI code.
> ---
>  xen/arch/arm/time.c   |  4 ----
>  xen/arch/arm/vtimer.c | 17 +++++++++++++++--
>  2 files changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
> index 3535bd8ac7c7..8fc14cd3ff62 100644
> --- a/xen/arch/arm/time.c
> +++ b/xen/arch/arm/time.c
> @@ -78,10 +78,6 @@ static int __init arch_timer_acpi_init(struct acpi_table_header *header)
>      irq_set_type(gtdt->non_secure_el1_interrupt, irq_type);
>      timer_irq[TIMER_PHYS_NONSECURE_PPI] = gtdt->non_secure_el1_interrupt;
>  
> -    irq_type = acpi_get_timer_irq_type(gtdt->secure_el1_flags);
> -    irq_set_type(gtdt->secure_el1_interrupt, irq_type);
> -    timer_irq[TIMER_PHYS_SECURE_PPI] = gtdt->secure_el1_interrupt;
> -
>      irq_type = acpi_get_timer_irq_type(gtdt->virtual_timer_flags);
>      irq_set_type(gtdt->virtual_timer_interrupt, irq_type);
>      timer_irq[TIMER_VIRT_PPI] = gtdt->virtual_timer_interrupt;
> diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
> index c54360e20266..e73ae33c1b58 100644
> --- a/xen/arch/arm/vtimer.c
> +++ b/xen/arch/arm/vtimer.c
> @@ -8,6 +8,7 @@
>   * Copyright (c) 2011 Citrix Systems.
>   */
>  
> +#include <xen/acpi.h>
>  #include <xen/lib.h>
>  #include <xen/perfc.h>
>  #include <xen/sched.h>
> @@ -61,10 +62,22 @@ int domain_vtimer_init(struct domain *d, struct xen_arch_domainconfig *config)
>  
>      config->clock_frequency = timer_dt_clock_frequency;
>  
> -    /* At this stage vgic_reserve_virq can't fail */
> +    /*
> +     * Per the ACPI specification, providing a secure EL1 timer
> +     * interrupt is optional and will be ignored by non-secure OS.
> +     * Therefore don't reserve the interrupt number for the HW domain
> +     * and ACPI.
> +     *
> +     * Note that we should still reserve it when using the Device-Tree
> +     * because the interrupt is not optional. That said, we are not
> +     * expecting any OS to use it when running on top of Xen.
> +     *
> +     * At this stage vgic_reserve_virq() is not meant to fail.
> +    */

NIT: minor code style issue that can be solved on commit

Assuming it passes Dan's test:

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


>      if ( is_hardware_domain(d) )
>      {
> -        if ( !vgic_reserve_virq(d, timer_get_irq(TIMER_PHYS_SECURE_PPI)) )
> +        if ( acpi_disabled &&
> +             !vgic_reserve_virq(d, timer_get_irq(TIMER_PHYS_SECURE_PPI)) )
>              BUG();
>  
>          if ( !vgic_reserve_virq(d, timer_get_irq(TIMER_PHYS_NONSECURE_PPI)) )
> -- 
> 2.40.1
>
Re: [PATCH] xen/arm: vtimer: Don't read/use the secure physical timer interrupt for ACPI
Posted by Henry Wang 2 years, 4 months ago 
Hi,

> On Oct 6, 2023, at 06:53, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> On Thu, 5 Oct 2023, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>> 
>> Per ACPI 6.5 section 5.2.25 ("Generic Timer Description Table (GTDT)"),
>> the fields "Secure EL1 Timer GSIV/Flags" are optional and an OS running
>> in non-secure world is meant to ignore the values.
>> 
>> However, Xen is trying to reserve the value. When booting on Graviton
>> 2 metal instances, this would result to crash a boot because the
>> value is 0 which is already reserved (I haven't checked for which device).
>> While nothing prevent a PPI to be shared, the field should have been
>> ignored by Xen.
>> 
>> For the Device-Tree case, I couldn't find a statement suggesting
>> that the secure physical timer interrupt  is ignored. In fact, I have
>> found some code in Linux using it as a fallback. That said, it should
>> never be used.
>> 
>> As I am not aware of any issue when booting using Device-Tree, the
>> physical timer interrupt is only ignored for ACPI.
>> 
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>> 
>> ----
>> 
>> This has not been tested on Graviton 2 because I can't seem to get
>> the serial console working properly. @Dan would you be able to try it?
>> 
>> It would also be good to understand why 0 why already reserved. This
>> may be a sign for other issues in the ACPI code.
>> ---
>> xen/arch/arm/time.c   |  4 ----
>> xen/arch/arm/vtimer.c | 17 +++++++++++++++--
>> 2 files changed, 15 insertions(+), 6 deletions(-)
>> 
>> diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
>> index 3535bd8ac7c7..8fc14cd3ff62 100644
>> --- a/xen/arch/arm/time.c
>> +++ b/xen/arch/arm/time.c
>> @@ -78,10 +78,6 @@ static int __init arch_timer_acpi_init(struct acpi_table_header *header)
>>     irq_set_type(gtdt->non_secure_el1_interrupt, irq_type);
>>     timer_irq[TIMER_PHYS_NONSECURE_PPI] = gtdt->non_secure_el1_interrupt;
>> 
>> -    irq_type = acpi_get_timer_irq_type(gtdt->secure_el1_flags);
>> -    irq_set_type(gtdt->secure_el1_interrupt, irq_type);
>> -    timer_irq[TIMER_PHYS_SECURE_PPI] = gtdt->secure_el1_interrupt;
>> -
>>     irq_type = acpi_get_timer_irq_type(gtdt->virtual_timer_flags);
>>     irq_set_type(gtdt->virtual_timer_interrupt, irq_type);
>>     timer_irq[TIMER_VIRT_PPI] = gtdt->virtual_timer_interrupt;
>> diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
>> index c54360e20266..e73ae33c1b58 100644
>> --- a/xen/arch/arm/vtimer.c
>> +++ b/xen/arch/arm/vtimer.c
>> @@ -8,6 +8,7 @@
>>  * Copyright (c) 2011 Citrix Systems.
>>  */
>> 
>> +#include <xen/acpi.h>
>> #include <xen/lib.h>
>> #include <xen/perfc.h>
>> #include <xen/sched.h>
>> @@ -61,10 +62,22 @@ int domain_vtimer_init(struct domain *d, struct xen_arch_domainconfig *config)
>> 
>>     config->clock_frequency = timer_dt_clock_frequency;
>> 
>> -    /* At this stage vgic_reserve_virq can't fail */
>> +    /*
>> +     * Per the ACPI specification, providing a secure EL1 timer
>> +     * interrupt is optional and will be ignored by non-secure OS.
>> +     * Therefore don't reserve the interrupt number for the HW domain
>> +     * and ACPI.
>> +     *
>> +     * Note that we should still reserve it when using the Device-Tree
>> +     * because the interrupt is not optional. That said, we are not
>> +     * expecting any OS to use it when running on top of Xen.
>> +     *
>> +     * At this stage vgic_reserve_virq() is not meant to fail.
>> +    */
> 
> NIT: minor code style issue that can be solved on commit
> 
> Assuming it passes Dan's test:
> 
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

Release-acked-by: Henry Wang <Henry.Wang@arm.com>

Kind regards,
Henry

> 
> 
>>     if ( is_hardware_domain(d) )
>>     {
>> -        if ( !vgic_reserve_virq(d, timer_get_irq(TIMER_PHYS_SECURE_PPI)) )
>> +        if ( acpi_disabled &&
>> +             !vgic_reserve_virq(d, timer_get_irq(TIMER_PHYS_SECURE_PPI)) )
>>             BUG();
>> 
>>         if ( !vgic_reserve_virq(d, timer_get_irq(TIMER_PHYS_NONSECURE_PPI)) )
>> -- 
>> 2.40.1
>>
Re: [PATCH] xen/arm: vtimer: Don't read/use the secure physical timer interrupt for ACPI
Posted by Michal Orzel 2 years, 4 months ago 
Hi Julien,

On 05/10/2023 18:54, Julien Grall wrote:
> 
> 
> From: Julien Grall <jgrall@amazon.com>
> 
> Per ACPI 6.5 section 5.2.25 ("Generic Timer Description Table (GTDT)"),
> the fields "Secure EL1 Timer GSIV/Flags" are optional and an OS running
> in non-secure world is meant to ignore the values.
> 
> However, Xen is trying to reserve the value. When booting on Graviton
> 2 metal instances, this would result to crash a boot because the
> value is 0 which is already reserved (I haven't checked for which device).
Per my understanding it is not reserved by any device.
0 means SGI and for SGIs we pre-reserve the bits in allocated_irqs at the very start.

~Michal
Re: [PATCH] xen/arm: vtimer: Don't read/use the secure physical timer interrupt for ACPI
Posted by Julien Grall 2 years, 4 months ago 

On 05/10/2023 21:15, Michal Orzel wrote:
> Hi Julien,

Hi Michal,

> On 05/10/2023 18:54, Julien Grall wrote:
>>
>>
>> From: Julien Grall <jgrall@amazon.com>
>>
>> Per ACPI 6.5 section 5.2.25 ("Generic Timer Description Table (GTDT)"),
>> the fields "Secure EL1 Timer GSIV/Flags" are optional and an OS running
>> in non-secure world is meant to ignore the values.
>>
>> However, Xen is trying to reserve the value. When booting on Graviton
>> 2 metal instances, this would result to crash a boot because the
>> value is 0 which is already reserved (I haven't checked for which device).
> Per my understanding it is not reserved by any device.
> 0 means SGI and for SGIs we pre-reserve the bits in allocated_irqs at the very start.

Ah yes good point. Somehow, I had in mind that PPI was starting at 0 
'^^. How about replacing the paragraph with:

"However, Xen is trying to reserve the value. The ACPI tables for 
Graviton 2 metal instances will provide the value 0 which is not a 
correct PPI (PPIs start at 16) and would have in fact been already 
reserved by Xen as this is an SGI. Xen will hit the BUG() and panic()".

Cheers,

-- 
Julien Grall
Re: [PATCH] xen/arm: vtimer: Don't read/use the secure physical timer interrupt for ACPI
Posted by Michal Orzel 2 years, 4 months ago 
Hi Julien,

On 06/10/2023 11:43, Julien Grall wrote:
> 
> 
> On 05/10/2023 21:15, Michal Orzel wrote:
>> Hi Julien,
> 
> Hi Michal,
> 
>> On 05/10/2023 18:54, Julien Grall wrote:
>>>
>>>
>>> From: Julien Grall <jgrall@amazon.com>
>>>
>>> Per ACPI 6.5 section 5.2.25 ("Generic Timer Description Table (GTDT)"),
>>> the fields "Secure EL1 Timer GSIV/Flags" are optional and an OS running
>>> in non-secure world is meant to ignore the values.
>>>
>>> However, Xen is trying to reserve the value. When booting on Graviton
>>> 2 metal instances, this would result to crash a boot because the
>>> value is 0 which is already reserved (I haven't checked for which device).
>> Per my understanding it is not reserved by any device.
>> 0 means SGI and for SGIs we pre-reserve the bits in allocated_irqs at the very start.
> 
> Ah yes good point. Somehow, I had in mind that PPI was starting at 0
> '^^. How about replacing the paragraph with:
> 
> "However, Xen is trying to reserve the value. The ACPI tables for
> Graviton 2 metal instances will provide the value 0 which is not a
> correct PPI (PPIs start at 16) and would have in fact been already
> reserved by Xen as this is an SGI. Xen will hit the BUG() and panic()".
Yes, this sounds better. With that:
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

~Michal

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.