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

[Andrei Cherechesu <andrei.cherechesu@xxxxxxxxxxx>]

On 09/03/2023 15:46, Michal Orzel wrote:
> 
> 
> On 09/03/2023 13:45, Bertrand Marquis wrote:
>>
>>
>> Hi Michal,
>>
>>> On 9 Mar 2023, at 13:42, Michal Orzel <michal.orzel@xxxxxxx> wrote:
>>>
>>> Hi Bertrand,
>>>
>>> On 09/03/2023 13:19, Bertrand Marquis wrote:
>>>>
>>>>
>>>> Hi Michal,
>>>>
>>>>> On 9 Mar 2023, at 12:35, Michal Orzel <michal.orzel@xxxxxxx> wrote:
>>>>>
>>>>>
>>>>>
>>>>> 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?
>>>>>
>>>>
>>>> Let's see that on the other angle: why should Xen check stuff that it does 
>>>> not need ?
>>> Because apart from what it needs or not, there is a matter of a failure in 
>>> Xen.
>>> Xen exposes timer IRQs to dom0 as they were taken from dtb and allowing 0 
>>> for any of the timer IRQ would result
>>> in a Xen failure when reserving such IRQ. Xen presets SGI IRQs, meaning 
>>> bits 0:15 in allocated_irqs bitmap are set.
>>> This is why when calling vgic_reserve_virq and passing SGI always results 
>>> in calling a BUG().
>>>
>>> So we have two options:
>>> - panic earlier with a meaningful message when IRQ is 0
>>> - let Xen continue and reach BUG which would be not that obvious for people 
>>> using but not knowing Xen
>>
>> So you are saying that in the current state 0 would be ignored during scan 
>> and create a bug later.
> Yes, provided platform_get_irq() returns 0. This is however only theory 
> because IMO it is impossible at the moment.
> Both GICs, do not allow specifying SGIs in dt bindings and require at least 3 
> cells where 1st one specifies type.
> So if we have a fake IRQ 0 as PPI, platform_get_irq will return 16 and for 
> SPI - 32.
> Therefore I cannot see how it would return 0.
> 
> ~Michal
> 


This was my original thought process as well when I initially
implemented this fix. Thus, I figured 0 should also be an error case.

Extending this, all SGI possible return values (0 to 15) should be
errors here, right? But I'm not sure if we should also treat those extra
cases here (1 to 15).

So, to make sure we all aligned, the only change to be made in a v2 for
this patch is the coding style for the added comment? Or do you also
think a more specific check for valid PPI IDs returned (16 <= id <= 31)
would be beneficial?

Regards,
Andrei