From: Andrei Cherechesu <andrei.cherechesu@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.cherechesu@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",
+};
+
 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 )
+            timer_irq[i] = res;
+        /* Do not panic if "hyp-virt" PPI is not found, since it's not
+         * currently used.
+         */
+        else if ( i != TIMER_HYP_VIRT_PPI )
             panic("Timer: Unable to retrieve IRQ %u from the device tree\n", i);
-        timer_irq[i] = res;
     }
 }
 
-- 
2.35.1

Hi Andrei,

> On 7 Mar 2023, at 11:09, Andrei Cherechesu (OSS) <andrei.cherechesu@oss.nxp.com> wrote:
> 
> From: Andrei Cherechesu <andrei.cherechesu@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.cherechesu@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 ?

> +            timer_irq[i] = res;
> +        /* Do not panic if "hyp-virt" PPI is not found, since it's not
> +         * currently used.
> +         */

Please respect the standard for comments and keep the first line empty:
/*
 * comment
 */

> +        else if ( i != TIMER_HYP_VIRT_PPI )
>             panic("Timer: Unable to retrieve IRQ %u from the device tree\n", i);
> -        timer_irq[i] = res;
>     }
> }

Cheers
Bertrand

> 
> -- 
> 2.35.1
> 
> 

On Tue, 7 Mar 2023, Bertrand Marquis wrote:
> > On 7 Mar 2023, at 11:09, Andrei Cherechesu (OSS) <andrei.cherechesu@oss.nxp.com> wrote:
> > 
> > From: Andrei Cherechesu <andrei.cherechesu@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.cherechesu@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.

Hi Stefano,

> On 7 Mar 2023, at 22:02, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> On Tue, 7 Mar 2023, Bertrand Marquis wrote:
>>> On 7 Mar 2023, at 11:09, Andrei Cherechesu (OSS) <andrei.cherechesu@oss.nxp.com> wrote:
>>> 
>>> From: Andrei Cherechesu <andrei.cherechesu@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.cherechesu@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

On 09/03/2023 09:02, Bertrand Marquis wrote:
> 
> 
> Hi Stefano,
> 
>> On 7 Mar 2023, at 22:02, Stefano Stabellini <sstabellini@kernel.org> wrote:
>>
>> On Tue, 7 Mar 2023, Bertrand Marquis wrote:
>>>> On 7 Mar 2023, at 11:09, Andrei Cherechesu (OSS) <andrei.cherechesu@oss.nxp.com> wrote:
>>>>
>>>> From: Andrei Cherechesu <andrei.cherechesu@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.cherechesu@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)
- EL1 phys (always)
- EL1 virt (always)
- NS EL2 phys (if EL2 is there)

therefore, if we get 0 for one of those, it means that something went wrong and we shall consider
it as an error.

~Michal

Hi Michal,

> On 9 Mar 2023, at 11:05, Michal Orzel <michal.orzel@amd.com> wrote:
> 
> 
> 
> On 09/03/2023 09:02, Bertrand Marquis wrote:
>> 
>> 
>> Hi Stefano,
>> 
>>> On 7 Mar 2023, at 22:02, Stefano Stabellini <sstabellini@kernel.org> wrote:
>>> 
>>> On Tue, 7 Mar 2023, Bertrand Marquis wrote:
>>>>> On 7 Mar 2023, at 11:09, Andrei Cherechesu (OSS) <andrei.cherechesu@oss.nxp.com> wrote:
>>>>> 
>>>>> From: Andrei Cherechesu <andrei.cherechesu@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.cherechesu@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.

> - EL1 phys (always)
> - EL1 virt (always)
> - NS EL2 phys (if EL2 is there)

Agree

> 
> therefore, if we get 0 for one of those, it means that something went wrong and we shall consider
> it as an error.

Agree except for the EL3 one :-)

Cheers
Bertrand

> 
> ~Michal

On 09/03/2023 11:39, Bertrand Marquis wrote:
> 
> 
> Hi Michal,
> 
>> On 9 Mar 2023, at 11:05, Michal Orzel <michal.orzel@amd.com> wrote:
>>
>>
>>
>> On 09/03/2023 09:02, Bertrand Marquis wrote:
>>>
>>>
>>> Hi Stefano,
>>>
>>>> On 7 Mar 2023, at 22:02, Stefano Stabellini <sstabellini@kernel.org> wrote:
>>>>
>>>> On Tue, 7 Mar 2023, Bertrand Marquis wrote:
>>>>>> On 7 Mar 2023, at 11:09, Andrei Cherechesu (OSS) <andrei.cherechesu@oss.nxp.com> wrote:
>>>>>>
>>>>>> From: Andrei Cherechesu <andrei.cherechesu@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.cherechesu@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?

~Michal

Hi Michal,

> On 9 Mar 2023, at 12:35, Michal Orzel <michal.orzel@amd.com> wrote:
> 
> 
> 
> On 09/03/2023 11:39, Bertrand Marquis wrote:
>> 
>> 
>> Hi Michal,
>> 
>>> On 9 Mar 2023, at 11:05, Michal Orzel <michal.orzel@amd.com> wrote:
>>> 
>>> 
>>> 
>>> On 09/03/2023 09:02, Bertrand Marquis wrote:
>>>> 
>>>> 
>>>> Hi Stefano,
>>>> 
>>>>> On 7 Mar 2023, at 22:02, Stefano Stabellini <sstabellini@kernel.org> wrote:
>>>>> 
>>>>> On Tue, 7 Mar 2023, Bertrand Marquis wrote:
>>>>>>> On 7 Mar 2023, at 11:09, Andrei Cherechesu (OSS) <andrei.cherechesu@oss.nxp.com> wrote:
>>>>>>> 
>>>>>>> From: Andrei Cherechesu <andrei.cherechesu@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.cherechesu@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

Hi Bertrand,

On 09/03/2023 13:19, Bertrand Marquis wrote:
> 
> 
> Hi Michal,
> 
>> On 9 Mar 2023, at 12:35, Michal Orzel <michal.orzel@amd.com> wrote:
>>
>>
>>
>> On 09/03/2023 11:39, Bertrand Marquis wrote:
>>>
>>>
>>> Hi Michal,
>>>
>>>> On 9 Mar 2023, at 11:05, Michal Orzel <michal.orzel@amd.com> wrote:
>>>>
>>>>
>>>>
>>>> On 09/03/2023 09:02, Bertrand Marquis wrote:
>>>>>
>>>>>
>>>>> Hi Stefano,
>>>>>
>>>>>> On 7 Mar 2023, at 22:02, Stefano Stabellini <sstabellini@kernel.org> wrote:
>>>>>>
>>>>>> On Tue, 7 Mar 2023, Bertrand Marquis wrote:
>>>>>>>> On 7 Mar 2023, at 11:09, Andrei Cherechesu (OSS) <andrei.cherechesu@oss.nxp.com> wrote:
>>>>>>>>
>>>>>>>> From: Andrei Cherechesu <andrei.cherechesu@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.cherechesu@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 ?
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

I think first option is always better.

~Michal

Hi Michal,

> On 9 Mar 2023, at 13:42, Michal Orzel <michal.orzel@amd.com> 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@amd.com> wrote:
>>> 
>>> 
>>> 
>>> On 09/03/2023 11:39, Bertrand Marquis wrote:
>>>> 
>>>> 
>>>> Hi Michal,
>>>> 
>>>>> On 9 Mar 2023, at 11:05, Michal Orzel <michal.orzel@amd.com> wrote:
>>>>> 
>>>>> 
>>>>> 
>>>>> On 09/03/2023 09:02, Bertrand Marquis wrote:
>>>>>> 
>>>>>> 
>>>>>> Hi Stefano,
>>>>>> 
>>>>>>> On 7 Mar 2023, at 22:02, Stefano Stabellini <sstabellini@kernel.org> wrote:
>>>>>>> 
>>>>>>> On Tue, 7 Mar 2023, Bertrand Marquis wrote:
>>>>>>>>> On 7 Mar 2023, at 11:09, Andrei Cherechesu (OSS) <andrei.cherechesu@oss.nxp.com> wrote:
>>>>>>>>> 
>>>>>>>>> From: Andrei Cherechesu <andrei.cherechesu@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.cherechesu@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 ?
> 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.

If this is the case than definitely we should panic earlier with a proper message I agree.

Regards
Bertrand

> 
> I think first option is always better.
> 
> ~Michal

On 09/03/2023 13:45, Bertrand Marquis wrote:
> 
> 
> Hi Michal,
> 
>> On 9 Mar 2023, at 13:42, Michal Orzel <michal.orzel@amd.com> 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@amd.com> wrote:
>>>>
>>>>
>>>>
>>>> On 09/03/2023 11:39, Bertrand Marquis wrote:
>>>>>
>>>>>
>>>>> Hi Michal,
>>>>>
>>>>>> On 9 Mar 2023, at 11:05, Michal Orzel <michal.orzel@amd.com> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 09/03/2023 09:02, Bertrand Marquis wrote:
>>>>>>>
>>>>>>>
>>>>>>> Hi Stefano,
>>>>>>>
>>>>>>>> On 7 Mar 2023, at 22:02, Stefano Stabellini <sstabellini@kernel.org> wrote:
>>>>>>>>
>>>>>>>> On Tue, 7 Mar 2023, Bertrand Marquis wrote:
>>>>>>>>>> On 7 Mar 2023, at 11:09, Andrei Cherechesu (OSS) <andrei.cherechesu@oss.nxp.com> wrote:
>>>>>>>>>>
>>>>>>>>>> From: Andrei Cherechesu <andrei.cherechesu@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.cherechesu@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 ?
>> 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

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@amd.com> 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@amd.com> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 09/03/2023 11:39, Bertrand Marquis wrote:
>>>>>>
>>>>>>
>>>>>> Hi Michal,
>>>>>>
>>>>>>> On 9 Mar 2023, at 11:05, Michal Orzel <michal.orzel@amd.com> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 09/03/2023 09:02, Bertrand Marquis wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> Hi Stefano,
>>>>>>>>
>>>>>>>>> On 7 Mar 2023, at 22:02, Stefano Stabellini <sstabellini@kernel.org> wrote:
>>>>>>>>>
>>>>>>>>> On Tue, 7 Mar 2023, Bertrand Marquis wrote:
>>>>>>>>>>> On 7 Mar 2023, at 11:09, Andrei Cherechesu (OSS) <andrei.cherechesu@oss.nxp.com> wrote:
>>>>>>>>>>>
>>>>>>>>>>> From: Andrei Cherechesu <andrei.cherechesu@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.cherechesu@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 ?
>>> 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

Hi,

> On 9 Mar 2023, at 15:36, Andrei Cherechesu <andrei.cherechesu@oss.nxp.com> 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@amd.com> 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@amd.com> wrote:
>>>>>> 
>>>>>> 
>>>>>> 
>>>>>> On 09/03/2023 11:39, Bertrand Marquis wrote:
>>>>>>> 
>>>>>>> 
>>>>>>> Hi Michal,
>>>>>>> 
>>>>>>>> On 9 Mar 2023, at 11:05, Michal Orzel <michal.orzel@amd.com> wrote:
>>>>>>>> 
>>>>>>>> 
>>>>>>>> 
>>>>>>>> On 09/03/2023 09:02, Bertrand Marquis wrote:
>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> Hi Stefano,
>>>>>>>>> 
>>>>>>>>>> On 7 Mar 2023, at 22:02, Stefano Stabellini <sstabellini@kernel.org> wrote:
>>>>>>>>>> 
>>>>>>>>>> On Tue, 7 Mar 2023, Bertrand Marquis wrote:
>>>>>>>>>>>> On 7 Mar 2023, at 11:09, Andrei Cherechesu (OSS) <andrei.cherechesu@oss.nxp.com> wrote:
>>>>>>>>>>>> 
>>>>>>>>>>>> From: Andrei Cherechesu <andrei.cherechesu@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.cherechesu@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 ?
>>>> 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

On 07/03/2023 17:38, Bertrand Marquis wrote:
> Hi Andrei,
> 
>> On 7 Mar 2023, at 11:09, Andrei Cherechesu (OSS) <andrei.cherechesu@oss.nxp.com> wrote:
>>
>> From: Andrei Cherechesu <andrei.cherechesu@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.cherechesu@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.

Hi Bertrand,

This implementation follows the one in Linux [0]. The parsing order for
the IRQs remains the same whether or not the "interrupt-names" property
is available, since the driver in both Linux and Xen expects them in a
specific order (defined by enum arch_timer_ppi_nr in Linux, for example)
which, most of the time, does not correspond to how they are mapped onto
the SoC. But now it can discover them correctly regardless of their
order in the "interrupts" property in the DT node.

Only the "hyp-virt" IRQ is not required to be present, which is also the
last one parsed.

[0]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/clocksource/arm_arch_timer.c?id=86332e9e3477af8f31c9d5f3e81e57e0fd2118e7

> 
>> 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 ?
> 

You're right, I didn't take the dummy fake interrupts case into
consideration. I also think we should keep the old behaviour then, and
let 0 go through too, as you mentioned.


>> +            timer_irq[i] = res;
>> +        /* Do not panic if "hyp-virt" PPI is not found, since it's not
>> +         * currently used.
>> +         */
> 
> Please respect the standard for comments and keep the first line empty:
> /*
>  * comment
>  */
> 

Will update in v2.

>> +        else if ( i != TIMER_HYP_VIRT_PPI )
>>             panic("Timer: Unable to retrieve IRQ %u from the device tree\n", i);
>> -        timer_irq[i] = res;
>>     }
>> }
> 
> Cheers
> Bertrand
> 

Thanks for the review.

Regards,
Andrei

>>
>> -- 
>> 2.35.1
>>
>>
>