| Message ID | 20190102055743.5052-9-clg@kaod.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | spapr: introduce the 'dual' interrupt mode XICS/XIVE | expand |
On Wed, Jan 02, 2019 at 06:57:41AM +0100, Cédric Le Goater wrote: > commit 15ed653fa49a ("ppc/xics: An ICS with offset 0 is assumed to be > uninitialized") introduced an extra check on the ICS offset which is > not strictly necessary. The commit message for that suggests it was added to make pnv easier. I take it you no longer need this for the current or expected pnv code? > Revert the change to be able to map the XICS IRQ number space on the > XIVE IRQ number space. > > Signed-off-by: Cédric Le Goater <clg@kaod.org> > --- > include/hw/ppc/xics.h | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h > index 7668c381a887..07508cbd217e 100644 > --- a/include/hw/ppc/xics.h > +++ b/include/hw/ppc/xics.h > @@ -139,8 +139,7 @@ struct ICSState { > > static inline bool ics_valid_irq(ICSState *ics, uint32_t nr) > { > - return (ics->offset != 0) && (nr >= ics->offset) > - && (nr < (ics->offset + ics->nr_irqs)); > + return (nr >= ics->offset) && (nr < (ics->offset + ics->nr_irqs)); > } > > struct ICSIRQState {
On 1/3/19 5:33 AM, David Gibson wrote: > On Wed, Jan 02, 2019 at 06:57:41AM +0100, Cédric Le Goater wrote: >> commit 15ed653fa49a ("ppc/xics: An ICS with offset 0 is assumed to be >> uninitialized") introduced an extra check on the ICS offset which is >> not strictly necessary. > > The commit message for that suggests it was added to make pnv easier. > I take it you no longer need this for the current or expected pnv > code? This is really just a runtime check which makes sure that the ICSState offset is initialized by the FW before being used. The problem is more global. It comes from the fact the ICSState offset in QEMU and the XICS offset in KVM is hardcoded to 0x1000. Thinking aloud : May be we should default the offset to 0 and introduce a service to set the value in QEMU and KVM. The 'dual' interrupt mode (using the qirq array at the machine level) needs the IRQ number space of the XIVE and XICS interrupt mode to be in sync, that is to start a 0x0, even if the lower 4K are ignored in XICS. So the 'dual' mode tunes the offset to 0 once the ICSState is created and ignores the lower 4K when initializing the KVM XICS device. C. >> Revert the change to be able to map the XICS IRQ number space on the >> XIVE IRQ number space. > > > >> >> Signed-off-by: Cédric Le Goater <clg@kaod.org> >> --- >> include/hw/ppc/xics.h | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h >> index 7668c381a887..07508cbd217e 100644 >> --- a/include/hw/ppc/xics.h >> +++ b/include/hw/ppc/xics.h >> @@ -139,8 +139,7 @@ struct ICSState { >> >> static inline bool ics_valid_irq(ICSState *ics, uint32_t nr) >> { >> - return (ics->offset != 0) && (nr >= ics->offset) >> - && (nr < (ics->offset + ics->nr_irqs)); >> + return (nr >= ics->offset) && (nr < (ics->offset + ics->nr_irqs)); >> } >> >> struct ICSIRQState { >
On Thu, Jan 03, 2019 at 06:45:25PM +0100, Cédric Le Goater wrote: > On 1/3/19 5:33 AM, David Gibson wrote: > > On Wed, Jan 02, 2019 at 06:57:41AM +0100, Cédric Le Goater wrote: > >> commit 15ed653fa49a ("ppc/xics: An ICS with offset 0 is assumed to be > >> uninitialized") introduced an extra check on the ICS offset which is > >> not strictly necessary. > > > > The commit message for that suggests it was added to make pnv easier. > > I take it you no longer need this for the current or expected pnv > > code? > > This is really just a runtime check which makes sure that the > ICSState offset is initialized by the FW before being used. > > The problem is more global. It comes from the fact the ICSState > offset in QEMU and the XICS offset in KVM is hardcoded to 0x1000. > > Thinking aloud : > > May be we should default the offset to 0 and introduce a service > to set the value in QEMU and KVM. > > The 'dual' interrupt mode (using the qirq array at the machine level) > needs the IRQ number space of the XIVE and XICS interrupt mode to > be in sync, that is to start a 0x0, even if the lower 4K are ignored > in XICS. > > So the 'dual' mode tunes the offset to 0 once the ICSState is created > and ignores the lower 4K when initializing the KVM XICS device. Hm, ok. I've applied patches 5..8. > > C. > > >> Revert the change to be able to map the XICS IRQ number space on the > >> XIVE IRQ number space. > > > > > > > >> > >> Signed-off-by: Cédric Le Goater <clg@kaod.org> > >> --- > >> include/hw/ppc/xics.h | 3 +-- > >> 1 file changed, 1 insertion(+), 2 deletions(-) > >> > >> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h > >> index 7668c381a887..07508cbd217e 100644 > >> --- a/include/hw/ppc/xics.h > >> +++ b/include/hw/ppc/xics.h > >> @@ -139,8 +139,7 @@ struct ICSState { > >> > >> static inline bool ics_valid_irq(ICSState *ics, uint32_t nr) > >> { > >> - return (ics->offset != 0) && (nr >= ics->offset) > >> - && (nr < (ics->offset + ics->nr_irqs)); > >> + return (nr >= ics->offset) && (nr < (ics->offset + ics->nr_irqs)); > >> } > >> > >> struct ICSIRQState { > > >
diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h index 7668c381a887..07508cbd217e 100644 --- a/include/hw/ppc/xics.h +++ b/include/hw/ppc/xics.h @@ -139,8 +139,7 @@ struct ICSState { static inline bool ics_valid_irq(ICSState *ics, uint32_t nr) { - return (ics->offset != 0) && (nr >= ics->offset) - && (nr < (ics->offset + ics->nr_irqs)); + return (nr >= ics->offset) && (nr < (ics->offset + ics->nr_irqs)); } struct ICSIRQState {
commit 15ed653fa49a ("ppc/xics: An ICS with offset 0 is assumed to be uninitialized") introduced an extra check on the ICS offset which is not strictly necessary. Revert the change to be able to map the XICS IRQ number space on the XIVE IRQ number space. Signed-off-by: Cédric Le Goater <clg@kaod.org> --- include/hw/ppc/xics.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)