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

["Driscoll, Dan" <dan.driscoll@xxxxxxxxxxx>]

Julien,

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

Thanks,
Dan

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: Thursday, October 5, 2023 11:55 AM
> To: xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Henry.Wang@xxxxxxx; Driscoll, Dan (DI SW CAS ES TO)
> <dan.driscoll@xxxxxxxxxxx>; Raghuraman, Arvind (DI SW CAS ES)
> <arvind.raghuraman@xxxxxxxxxxx>; michal.orzel@xxxxxxx; Julien Grall
> <jgrall@xxxxxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>; Julien Grall
> <julien@xxxxxxx>; Bertrand Marquis <bertrand.marquis@xxxxxxx>; Volodymyr
> Babchuk <Volodymyr_Babchuk@xxxxxxxx>
> Subject: [PATCH] xen/arm: vtimer: Don't read/use the secure physical timer
> interrupt for ACPI
> 
> 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.
> +    */
>      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