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

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Andrei,

On 09/03/2023 17:19, 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 changed to treating 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,
> during vgic_reserve_virq().
vgic_reserve_virq() itself does not call BUG(), so to be more precise,
instead of "during vgic_reserve_virq()", how about:
"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@xxxxxxx>
> ---
>  xen/arch/arm/include/asm/time.h |  3 ++-
>  xen/arch/arm/time.c             | 27 +++++++++++++++++++++++----
>  2 files changed, 25 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..868e03ecf6 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] = {
This wants to be marked as __initconst as it is const and only used in init 
code.
So it would be:
static const char *const timer_irq_names[...

Furthermore, as the only user of this array is init_dt_xen_time(), you could 
move the
definition to the function itself.

~Michal