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

[Michal Orzel <michal.orzel@xxxxxxx>]

On 09/03/2023 11:39, Bertrand Marquis wrote:
> 
> 
> Hi Michal,
> 
>> On 9 Mar 2023, at 11:05, Michal Orzel <michal.orzel@xxxxxxx> wrote:
>>
>>
>>
>> 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)
> 
> This might be needed by EL3 but not by Xen.
Xen requires system with EL3 and if there is EL3, both Arm spec and dt bindings 
requires sec-phys timer to be there.
So it would be very strange to see a fake interrupt with IRQ being 0. But if we 
relly want to only care about
what Xen needs, then we could live with that (although it is difficult for me 
to find justification for 0 there).
Device trees are created per system and if system has EL3, then why forcing 0 
to be listed for sec-phys timer?

~Michal