| Message ID | 20231123055733.1002890-2-harshpb@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce SPAPR_IRQ_NR_IPIS and fix max-cpus | expand |
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> > --- > 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. 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
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. 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 >
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 >>
On 11/23/23 19:42, Cédric Le Goater wrote: > 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 ? > Hmm, I can do that if a re-spin is reqd, or can be done as a separate patch later also along with other improvements. > 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. > Hmm, I am not so familiar with XICS yet, so not sure if we really need to do that, but it can be done along with other improvements if needed. regards, Harsh > 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 >>> >
On 11/24/23 09:01, Harsh Prateek Bora wrote: > > > On 11/23/23 19:42, Cédric Le Goater wrote: >> 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 ? >> > > Hmm, I can do that if a re-spin is reqd, or can be done as a separate > patch later also along with other improvements. yes. This is food for thoughts for further improvements. Thanks, C. > >> 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. >> > > Hmm, I am not so familiar with XICS yet, so not sure if we really need > to do that, but it can be done along with other improvements if needed. > > regards, > Harsh > >> 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 >>>> >>
On 24/11/23 13:39, Cédric Le Goater wrote: > On 11/24/23 09:01, Harsh Prateek Bora wrote: >> >> >> On 11/23/23 19:42, Cédric Le Goater wrote: >>> On 11/23/23 10:31, Harsh Prateek Bora wrote: >>>> >>>> >>>> On 11/23/23 14:20, CI've applied these patches and verified on the >>>> latest upstream qemu. The code is working as expected. >>>> >>>> Tested-by: Kowshik Jois<kowsjois@linux.ibm.com>é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 ? >>> >> >> Hmm, I can do that if a re-spin is reqd, or can be done as a separate >> patch later also along with other improvements. > > yes. This is food for thoughts for further improvements. > > Thanks, > > C. > > > >> >>> 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. >>> >> >> Hmm, I am not so familiar with XICS yet, so not sure if we really need >> to do that, but it can be done along with other improvements if needed. >> >> regards, >> Harsh >> >>> 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 >>>>> >>> > > I've applied these patches and verified on the latest upstream qemu. > The code is working as expected. > > Tested-by: Kowshik Jois<kowsjois@linux.ibm.com>
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); + 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
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> --- include/hw/ppc/spapr_irq.h | 14 +++++++++++++- hw/ppc/spapr_irq.c | 6 ++++-- 2 files changed, 17 insertions(+), 3 deletions(-)