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
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
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
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 >
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 >>
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
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
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
© 2016 - 2024 Red Hat, Inc.