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

[Henry Wang <Henry.Wang@xxxxxxx>]

Hi,

> On Oct 6, 2023, at 06:53, Stefano Stabellini <sstabellini@xxxxxxxxxx> wrote:
> 
> 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>

Release-acked-by: Henry Wang <Henry.Wang@xxxxxxx>

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
>>