[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH] xen/arm: vtimer: Don't read/use the secure physical timer interrupt for ACPI
On Thu, 5 Oct 2023, Julien Grall wrote: > From: Julien Grall <jgrall@xxxxxxxxxx> > > 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@xxxxxxxxxx> > > ---- > > 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@xxxxxxxxxx> > 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 >
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |