Re: [PATCH v1 2/2] arch/arm: time: Add support for parsing interrupts by names

[Andrei Cherechesu <andrei.cherechesu@xxxxxxxxxxx>]

On 07/03/2023 17:38, Bertrand Marquis wrote:
> Hi Andrei,
> 
>> On 7 Mar 2023, at 11:09, Andrei Cherechesu (OSS) 
>> <andrei.cherechesu@xxxxxxxxxxx> wrote:
>>
>> From: Andrei Cherechesu <andrei.cherechesu@xxxxxxx>
>>
>> Added support for parsing the ARM generic timer interrupts DT
>> node by the "interrupt-names" property, if it is available.
>>
>> If not available, the usual parsing based on the expected
>> IRQ order is performed.
>>
>> Also added the "hyp-virt" PPI to the timer PPI list, even
>> though it's currently not in use. If the "hyp-virt" PPI is
>> not found, the hypervisor won't panic.
>>
>> Signed-off-by: Andrei Cherechesu <andrei.cherechesu@xxxxxxx>
>> ---
>> xen/arch/arm/include/asm/time.h |  3 ++-
>> xen/arch/arm/time.c             | 26 ++++++++++++++++++++++----
>> 2 files changed, 24 insertions(+), 5 deletions(-)
>>
>> diff --git a/xen/arch/arm/include/asm/time.h 
>> b/xen/arch/arm/include/asm/time.h
>> index 4b401c1110..49ad8c1a6d 100644
>> --- a/xen/arch/arm/include/asm/time.h
>> +++ b/xen/arch/arm/include/asm/time.h
>> @@ -82,7 +82,8 @@ enum timer_ppi
>>     TIMER_PHYS_NONSECURE_PPI = 1,
>>     TIMER_VIRT_PPI = 2,
>>     TIMER_HYP_PPI = 3,
>> -    MAX_TIMER_PPI = 4,
>> +    TIMER_HYP_VIRT_PPI = 4,
>> +    MAX_TIMER_PPI = 5,
>> };
>>
>> /*
>> diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
>> index 433d7be909..794da646d6 100644
>> --- a/xen/arch/arm/time.c
>> +++ b/xen/arch/arm/time.c
>> @@ -38,6 +38,14 @@ uint32_t __read_mostly timer_dt_clock_frequency;
>>
>> static unsigned int timer_irq[MAX_TIMER_PPI];
>>
>> +static const char *timer_irq_names[MAX_TIMER_PPI] = {
>> +    [TIMER_PHYS_SECURE_PPI] = "sec-phys",
>> +    [TIMER_PHYS_NONSECURE_PPI] = "phys",
>> +    [TIMER_VIRT_PPI] = "virt",
>> +    [TIMER_HYP_PPI] = "hyp-phys",
>> +    [TIMER_HYP_VIRT_PPI] = "hyp-virt",
>> +};
>> +
> 
> I would need some reference or a pointer to some doc to check those.

Hi Bertrand,

This implementation follows the one in Linux [0]. The parsing order for
the IRQs remains the same whether or not the "interrupt-names" property
is available, since the driver in both Linux and Xen expects them in a
specific order (defined by enum arch_timer_ppi_nr in Linux, for example)
which, most of the time, does not correspond to how they are mapped onto
the SoC. But now it can discover them correctly regardless of their
order in the "interrupts" property in the DT node.

Only the "hyp-virt" IRQ is not required to be present, which is also the
last one parsed.

[0]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/clocksource/arm_arch_timer.c?id=86332e9e3477af8f31c9d5f3e81e57e0fd2118e7

> 
>> unsigned int timer_get_irq(enum timer_ppi ppi)
>> {
>>     ASSERT(ppi >= TIMER_PHYS_SECURE_PPI && ppi < MAX_TIMER_PPI);
>> @@ -149,15 +157,25 @@ static void __init init_dt_xen_time(void)
>> {
>>     int res;
>>     unsigned int i;
>> +    bool has_names;
>> +
>> +    has_names = dt_property_read_bool(timer, "interrupt-names");
>>
>>     /* Retrieve all IRQs for the timer */
>>     for ( i = TIMER_PHYS_SECURE_PPI; i < MAX_TIMER_PPI; i++ )
>>     {
>> -        res = platform_get_irq(timer, i);
>> -
>> -        if ( res < 0 )
>> +        if ( has_names )
>> +            res = platform_get_irq_byname(timer, timer_irq_names[i]);
>> +        else
>> +            res = platform_get_irq(timer, i);
>> +
>> +        if ( res > 0 )
> 
> The behaviour of the code is changed here compared to the current
> version as res = 0 will now generate a panic.
> 
> Some device tree might not specify an interrupt number and just put
> 0 and Xen will now panic on those systems.
> As I have no idea if such systems exists and the behaviour is modified
> you should justify this and mention it in the commit message or keep
> the old behaviour and let 0 go through without a panic.
> 
> @stefano, julien any idea here ? should just keep the old behaviour ?
> 

You're right, I didn't take the dummy fake interrupts case into
consideration. I also think we should keep the old behaviour then, and
let 0 go through too, as you mentioned.


>> +            timer_irq[i] = res;
>> +        /* Do not panic if "hyp-virt" PPI is not found, since it's not
>> +         * currently used.
>> +         */
> 
> Please respect the standard for comments and keep the first line empty:
> /*
>  * comment
>  */
> 

Will update in v2.

>> +        else if ( i != TIMER_HYP_VIRT_PPI )
>>             panic("Timer: Unable to retrieve IRQ %u from the device tree\n", 
>> i);
>> -        timer_irq[i] = res;
>>     }
>> }
> 
> Cheers
> Bertrand
> 

Thanks for the review.

Regards,
Andrei

>>
>> -- 
>> 2.35.1
>>
>>
>