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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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