Re: [PATCH v3 1/2] ppc/spapr: Introduce SPAPR_IRQ_NR_IPIS to refer IRQ range for CPU IPIs.

[Cédric Le Goater]

On 11/23/23 10:31, Harsh Prateek Bora wrote:
> On 11/23/23 14:20, Cédric Le Goater wrote:
> > On 11/23/23 06:57, Harsh Prateek Bora wrote:
> > > spapr_irq_init currently uses existing macro SPAPR_XIRQ_BASE to refer to
> > > the range of CPU IPIs during initialization of nr-irqs property.
> > > It is more appropriate to have its own define which can be further
> > > reused as appropriate for correct interpretation.
> > >
> > > Signed-off-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
> > > Suggested-by: Cedric Le Goater <clg@kaod.org>
> >
> > One comment below
> >
> > Reviewed-by: Cédric Le Goater <clg@kaod.org>
>
> Thanks, responding below ..
>
> > > ---
> > >   include/hw/ppc/spapr_irq.h | 14 +++++++++++++-
> > >   hw/ppc/spapr_irq.c         |  6 ++++--
> > >   2 files changed, 17 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
> > > index c22a72c9e2..4fd2d5853d 100644
> > > --- a/include/hw/ppc/spapr_irq.h
> > > +++ b/include/hw/ppc/spapr_irq.h
> > > @@ -14,9 +14,21 @@
> > >   #include "qom/object.h"
> > >   /*
> > > - * IRQ range offsets per device type
> > > + * The XIVE IRQ backend uses the same layout as the XICS backend but
> > > + * covers the full range of the IRQ number space. The IRQ numbers for
> > > + * the CPU IPIs are allocated at the bottom of this space, below 4K,
> > > + * to preserve compatibility with XICS which does not use that range.
> > > + */
> > > +
> > > +/*
> > > + * CPU IPI range (XIVE only)
> > >    */
> > >   #define SPAPR_IRQ_IPI        0x0
> > > +#define SPAPR_IRQ_NR_IPIS    0x1000
> > > +
> > > +/*
> > > + * IRQ range offsets per device type
> > > + */
> > >   #define SPAPR_XIRQ_BASE      XICS_IRQ_BASE /* 0x1000 */
> > >   #define SPAPR_IRQ_EPOW       (SPAPR_XIRQ_BASE + 0x0000)
> > > diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> > > index a0d1e1298e..97b2fc42ab 100644
> > > --- a/hw/ppc/spapr_irq.c
> > > +++ b/hw/ppc/spapr_irq.c
> > > @@ -23,6 +23,8 @@
> > >   #include "trace.h"
> > > +QEMU_BUILD_BUG_ON(SPAPR_IRQ_NR_IPIS > SPAPR_XIRQ_BASE);
> > > +
> >
> > I would have put the check in include/hw/ppc/spapr_irq.h but since
> > SPAPR_XIRQ_BASE is only used in hw/ppc/spapr_irq.c which is always
> > compiled, this is fine. You might want to change that in case a
> > respin is asked for.
>
> I had initially tried keeping it in spapr_irq.h , but that would give a build 
> break for XICS_IRQ_BASE not defined since that gets defined in spapr_xics.h and 
> is included later in some files, however, the QEMU_BUILD_BUG_ON expects it to 
> be defined before it reaches here.

ah. good catch. this went unnoticed and is a bit ugly. We should fix
in some ways. May with a define SPAPR_XIRQ_BASE to 0x1000 simply ?

Also, we could probably define the ICS offset to SPAPR_XIRQ_BASE
directly under spapr_irq_init() and get rid of ics_instance_init().
The HW IRQ Number offset in the PNV ICS instances is assigned
dynamically by the OS (see pnv_phb3). So it should befine to do
the same for spapr. In which case we can get rid of XICS_IRQ_BASE.

Thanks,

C.



> regards,
> Harsh
>
> > Thanks,
> >
> > C.
> >
> >
> > >   static const TypeInfo spapr_intc_info = {
> > >       .name = TYPE_SPAPR_INTC,
> > >       .parent = TYPE_INTERFACE,
> > > @@ -329,7 +331,7 @@ void spapr_irq_init(SpaprMachineState *spapr, Error **errp)
> > >           int i;
> > >           dev = qdev_new(TYPE_SPAPR_XIVE);
> > > -        qdev_prop_set_uint32(dev, "nr-irqs", smc->nr_xirqs + SPAPR_XIRQ_BASE);
> > > +        qdev_prop_set_uint32(dev, "nr-irqs", smc->nr_xirqs + 
> > > SPAPR_IRQ_NR_IPIS);
> > >           /*
> > >            * 8 XIVE END structures per CPU. One for each available
> > >            * priority
> > > @@ -356,7 +358,7 @@ void spapr_irq_init(SpaprMachineState *spapr, Error **errp)
> > >       }
> > >       spapr->qirqs = qemu_allocate_irqs(spapr_set_irq, spapr,
> > > -                                      smc->nr_xirqs + SPAPR_XIRQ_BASE);
> > > +                                      smc->nr_xirqs + SPAPR_IRQ_NR_IPIS);
> > >       /*
> > >        * Mostly we don't actually need this until reset, except that not