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

[Michal Orzel <michal.orzel@xxxxxxx>]

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