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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi,

> On 9 Mar 2023, at 15:36, Andrei Cherechesu <andrei.cherechesu@xxxxxxxxxxx> 
> wrote:
> 
> 
> 
> 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?
> 

No i think we should not check more.
Just add something in the commit message to mention this change and why it is 
ok.


Cheers
Bertrand

> Regards,
> Andrei