1. Patchew
  2. Xen
  3. [PATCH v3 0/2] Fix ARM Generic Timer interrupt parsing
  4. View patch

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

Andrei Cherechesu (OSS) posted 2 patches 2 years, 11 months ago
[PATCH v3 2/2] arch/arm: time: Add support for parsing interrupts by names
Posted by Andrei Cherechesu (OSS) 2 years, 11 months ago 
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 treated returning 0 as an error case for the
platform_get_irq() calls, since it is not a valid PPI ID and
treating it as a valid case would only cause Xen to BUG() later,
when trying to reserve vIRQ being SGI.

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..0b482d7db3 100644
--- a/xen/arch/arm/time.c
+++ b/xen/arch/arm/time.c
@@ -149,15 +149,33 @@ static void __init init_dt_xen_time(void)
 {
     int res;
     unsigned int i;
+    bool has_names;
+    static const char * const timer_irq_names[MAX_TIMER_PPI] __initconst = {
+        [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",
+    };
+
+    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
Re: [PATCH v3 2/2] arch/arm: time: Add support for parsing interrupts by names
Posted by Julien Grall 2 years, 10 months ago 
Hi,

On 13/03/2023 13:08, Andrei Cherechesu (OSS) 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 treated returning 0 as an error case for the
> platform_get_irq() calls, since it is not a valid PPI ID and
> treating it as a valid case would only cause Xen to BUG() later,
> when trying to reserve vIRQ being SGI.
> 
> 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.

I know this was already merged. But it would have been nice to explain 
why this is added. As this stands, it looks unnecessary dead code which 
would have been okay if ...

> 
> 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..0b482d7db3 100644
> --- a/xen/arch/arm/time.c
> +++ b/xen/arch/arm/time.c
> @@ -149,15 +149,33 @@ static void __init init_dt_xen_time(void)
>   {
>       int res;
>       unsigned int i;
> +    bool has_names;
> +    static const char * const timer_irq_names[MAX_TIMER_PPI] __initconst = {
> +        [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",
> +    };
> +
> +    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);

... this wasn't necessary.

> -        timer_irq[i] = res;
>       }
>   }
>   

Cheers,

-- 
Julien Grall
Re: [PATCH v3 2/2] arch/arm: time: Add support for parsing interrupts by names
Posted by Michal Orzel 2 years, 11 months ago 
On 13/03/2023 14:08, Andrei Cherechesu (OSS) 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 treated returning 0 as an error case for the
> platform_get_irq() calls, since it is not a valid PPI ID and
> treating it as a valid case would only cause Xen to BUG() later,
> when trying to reserve vIRQ being SGI.
> 
> 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>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

~Michal
Re: [PATCH v3 2/2] arch/arm: time: Add support for parsing interrupts by names
Posted by Bertrand Marquis 2 years, 11 months ago 
HI Andrei,

> On 13 Mar 2023, at 14:08, 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 treated returning 0 as an error case for the
> platform_get_irq() calls, since it is not a valid PPI ID and
> treating it as a valid case would only cause Xen to BUG() later,
> when trying to reserve vIRQ being SGI.
> 
> 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>

Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Cheers
Bertrand

> ---
> 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..0b482d7db3 100644
> --- a/xen/arch/arm/time.c
> +++ b/xen/arch/arm/time.c
> @@ -149,15 +149,33 @@ static void __init init_dt_xen_time(void)
> {
>     int res;
>     unsigned int i;
> +    bool has_names;
> +    static const char * const timer_irq_names[MAX_TIMER_PPI] __initconst = {
> +        [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",
> +    };
> +
> +    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
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.