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

[Julien Grall <julien@xxxxxxx>]

Hi,

On 13/03/2023 13:08, Andrei Cherechesu (OSS) 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 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@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..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