Hi Stefano,
> On 7 Mar 2023, at 22:02, Stefano Stabellini <sstabell...@kernel.org> wrote:
>
> On Tue, 7 Mar 2023, Bertrand Marquis wrote:
>>> On 7 Mar 2023, at 11:09, Andrei Cherechesu (OSS)
>>> <andrei.cherech...@oss.nxp.com> wrote:
>>>
>>> From: Andrei Cherechesu <andrei.cherech...@nxp.com>
>>>
>>> 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.cherech...@nxp.com>
>>> ---
>>> 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).
So I think we need to keep the current behaviour, might be ok to put a
debug print.
What I would think is feasible would be to panic for interrupt numbers we
need only.
Cheers
Bertrand
