| Message ID | 20231005165454.18143-1-julien@xen.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/arm: vtimer: Don't read/use the secure physical timer interrupt for ACPI | expand |
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 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 >>
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,
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
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,
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)) )