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

[Michal Orzel <michal.orzel@xxxxxxx>]

On 09/03/2023 09:02, Bertrand Marquis wrote:
> 
> 
> Hi Stefano,
> 
>> On 7 Mar 2023, at 22:02, Stefano Stabellini <sstabellini@xxxxxxxxxx> wrote:
>>
>> On Tue, 7 Mar 2023, Bertrand Marquis wrote:
>>>> 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.
>>>
>>>> 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 ?
>>
>> platform_get_irq returns 0 if the irq is 0. The irq cannot be 0 because
>> 0 is reserved for SGIs, not PPIs. So I think it is OK to consider 0 an
>> error.
> 
> Problem here is that a DTB might not specify all interrupts and just put
> 0 for the one not used (or not available for example if you have no secure
> world).
Xen requires presence of EL3,EL2 and on such system, at least the following 
timers needs to be there
according to Arm ARM:
- EL3 phys (if EL3 is there)
- EL1 phys (always)
- EL1 virt (always)
- NS EL2 phys (if EL2 is there)

therefore, if we get 0 for one of those, it means that something went wrong and 
we shall consider
it as an error.

~Michal