Hi Michal,
> On 9 Mar 2023, at 12:35, Michal Orzel <michal.or...@amd.com> wrote:
>
>
>
> On 09/03/2023 11:39, Bertrand Marquis wrote:
>>
>>
>> Hi Michal,
>>
>>> On 9 Mar 2023, at 11:05, Michal Orzel <michal.or...@amd.com> wrote:
>>>
>>>
>>>
>>> On 09/03/2023 09:02, Bertrand Marquis wrote:
>>>>
>>>>
>>>> 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).
>>> 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 ?
Bertrand
> ~Michal
