| Message ID | 20190925064534.19155-12-david@gibson.dropbear.id.au (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | spapr: IRQ subsystem cleanups | expand |
On 25/09/2019 08:45, David Gibson wrote: > spapr global irq numbers are different from the source numbers on the ICS > when using XICS - they're offset by XICS_IRQ_BASE (0x1000). But > spapr_irq_set_irq_xics() was passing through the global irq number to > the ICS code unmodified. > > We only got away with this because of a counteracting bug - we were > incorrectly adjusting the qemu_irq we returned for a requested global irq > number. > > That approach mostly worked but is very confusing, incorrectly relies on > the way the qemu_irq array is allocated, and undermines the intention of > having the global array of qemu_irqs for spapr have a consistent meaning > regardless of irq backend. > > So, fix both set_irq and qemu_irq indexing. We rename some parameters at > the same time to make it clear that they are referring to spapr global > irq numbers. > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> The commit log clearly reflects what you think of the current solution in the code :) It is hideous, but it worked fine and this is even better :) Reviewed-by: Cédric Le Goater <clg@kaod.org> > --- > hw/ppc/spapr_irq.c | 16 ++++++++-------- > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c > index 300c65be3a..9a9e486eb5 100644 > --- a/hw/ppc/spapr_irq.c > +++ b/hw/ppc/spapr_irq.c > @@ -153,10 +153,9 @@ static void spapr_irq_free_xics(SpaprMachineState *spapr, int irq, int num) > static qemu_irq spapr_qirq_xics(SpaprMachineState *spapr, int irq) > { > ICSState *ics = spapr->ics; > - uint32_t srcno = irq - ics->offset; > > if (ics_valid_irq(ics, irq)) { > - return spapr->qirqs[srcno]; > + return spapr->qirqs[irq]; > } > > return NULL; > @@ -204,9 +203,10 @@ static int spapr_irq_post_load_xics(SpaprMachineState *spapr, int version_id) > return 0; > } > > -static void spapr_irq_set_irq_xics(void *opaque, int srcno, int val) > +static void spapr_irq_set_irq_xics(void *opaque, int irq, int val) > { > SpaprMachineState *spapr = opaque; > + uint32_t srcno = irq - spapr->ics->offset; > > ics_set_irq(spapr->ics, srcno, val); > } > @@ -377,14 +377,14 @@ static void spapr_irq_reset_xive(SpaprMachineState *spapr, Error **errp) > spapr_xive_mmio_set_enabled(spapr->xive, true); > } > > -static void spapr_irq_set_irq_xive(void *opaque, int srcno, int val) > +static void spapr_irq_set_irq_xive(void *opaque, int irq, int val) > { > SpaprMachineState *spapr = opaque; > > if (kvm_irqchip_in_kernel()) { > - kvmppc_xive_source_set_irq(&spapr->xive->source, srcno, val); > + kvmppc_xive_source_set_irq(&spapr->xive->source, irq, val); > } else { > - xive_source_set_irq(&spapr->xive->source, srcno, val); > + xive_source_set_irq(&spapr->xive->source, irq, val); > } > } > > @@ -563,11 +563,11 @@ static void spapr_irq_reset_dual(SpaprMachineState *spapr, Error **errp) > spapr_irq_current(spapr)->reset(spapr, errp); > } > > -static void spapr_irq_set_irq_dual(void *opaque, int srcno, int val) > +static void spapr_irq_set_irq_dual(void *opaque, int irq, int val) > { > SpaprMachineState *spapr = opaque; > > - spapr_irq_current(spapr)->set_irq(spapr, srcno, val); > + spapr_irq_current(spapr)->set_irq(spapr, irq, val); > } > > static const char *spapr_irq_get_nodename_dual(SpaprMachineState *spapr) >
On Wed, 25 Sep 2019 16:45:25 +1000 David Gibson <david@gibson.dropbear.id.au> wrote: > spapr global irq numbers are different from the source numbers on the ICS > when using XICS - they're offset by XICS_IRQ_BASE (0x1000). But > spapr_irq_set_irq_xics() was passing through the global irq number to > the ICS code unmodified. > > We only got away with this because of a counteracting bug - we were > incorrectly adjusting the qemu_irq we returned for a requested global irq > number. > > That approach mostly worked but is very confusing, incorrectly relies on > the way the qemu_irq array is allocated, and undermines the intention of > having the global array of qemu_irqs for spapr have a consistent meaning > regardless of irq backend. > > So, fix both set_irq and qemu_irq indexing. We rename some parameters at > the same time to make it clear that they are referring to spapr global > irq numbers. > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > --- Reviewed-by: Greg Kurz <groug@kaod.org> Further cleanup could be to have the XICS backend to only take global irq numbers and to convert them to ICS source numbers internally. This would put an end to the confusion between srcno/irq in the frontend code. > hw/ppc/spapr_irq.c | 16 ++++++++-------- > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c > index 300c65be3a..9a9e486eb5 100644 > --- a/hw/ppc/spapr_irq.c > +++ b/hw/ppc/spapr_irq.c > @@ -153,10 +153,9 @@ static void spapr_irq_free_xics(SpaprMachineState *spapr, int irq, int num) > static qemu_irq spapr_qirq_xics(SpaprMachineState *spapr, int irq) > { > ICSState *ics = spapr->ics; > - uint32_t srcno = irq - ics->offset; > > if (ics_valid_irq(ics, irq)) { > - return spapr->qirqs[srcno]; > + return spapr->qirqs[irq]; > } > > return NULL; > @@ -204,9 +203,10 @@ static int spapr_irq_post_load_xics(SpaprMachineState *spapr, int version_id) > return 0; > } > > -static void spapr_irq_set_irq_xics(void *opaque, int srcno, int val) > +static void spapr_irq_set_irq_xics(void *opaque, int irq, int val) > { > SpaprMachineState *spapr = opaque; > + uint32_t srcno = irq - spapr->ics->offset; > > ics_set_irq(spapr->ics, srcno, val); > } > @@ -377,14 +377,14 @@ static void spapr_irq_reset_xive(SpaprMachineState *spapr, Error **errp) > spapr_xive_mmio_set_enabled(spapr->xive, true); > } > > -static void spapr_irq_set_irq_xive(void *opaque, int srcno, int val) > +static void spapr_irq_set_irq_xive(void *opaque, int irq, int val) > { > SpaprMachineState *spapr = opaque; > > if (kvm_irqchip_in_kernel()) { > - kvmppc_xive_source_set_irq(&spapr->xive->source, srcno, val); > + kvmppc_xive_source_set_irq(&spapr->xive->source, irq, val); > } else { > - xive_source_set_irq(&spapr->xive->source, srcno, val); > + xive_source_set_irq(&spapr->xive->source, irq, val); > } > } > > @@ -563,11 +563,11 @@ static void spapr_irq_reset_dual(SpaprMachineState *spapr, Error **errp) > spapr_irq_current(spapr)->reset(spapr, errp); > } > > -static void spapr_irq_set_irq_dual(void *opaque, int srcno, int val) > +static void spapr_irq_set_irq_dual(void *opaque, int irq, int val) > { > SpaprMachineState *spapr = opaque; > > - spapr_irq_current(spapr)->set_irq(spapr, srcno, val); > + spapr_irq_current(spapr)->set_irq(spapr, irq, val); > } > > static const char *spapr_irq_get_nodename_dual(SpaprMachineState *spapr)
On Wed, Sep 25, 2019 at 10:17:46PM +0200, Greg Kurz wrote: > On Wed, 25 Sep 2019 16:45:25 +1000 > David Gibson <david@gibson.dropbear.id.au> wrote: > > > spapr global irq numbers are different from the source numbers on the ICS > > when using XICS - they're offset by XICS_IRQ_BASE (0x1000). But > > spapr_irq_set_irq_xics() was passing through the global irq number to > > the ICS code unmodified. > > > > We only got away with this because of a counteracting bug - we were > > incorrectly adjusting the qemu_irq we returned for a requested global irq > > number. > > > > That approach mostly worked but is very confusing, incorrectly relies on > > the way the qemu_irq array is allocated, and undermines the intention of > > having the global array of qemu_irqs for spapr have a consistent meaning > > regardless of irq backend. > > > > So, fix both set_irq and qemu_irq indexing. We rename some parameters at > > the same time to make it clear that they are referring to spapr global > > irq numbers. > > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > --- > > Reviewed-by: Greg Kurz <groug@kaod.org> > > Further cleanup could be to have the XICS backend to only take global > irq numbers and to convert them to ICS source numbers internally. This > would put an end to the confusion between srcno/irq in the frontend > code. Yeah, maybe. But the local srcnos do actually make sense from within the perspective of ICS, so I'm not all that keen to do that.
On Thu, 26 Sep 2019 11:31:48 +1000 David Gibson <david@gibson.dropbear.id.au> wrote: > On Wed, Sep 25, 2019 at 10:17:46PM +0200, Greg Kurz wrote: > > On Wed, 25 Sep 2019 16:45:25 +1000 > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > spapr global irq numbers are different from the source numbers on the ICS > > > when using XICS - they're offset by XICS_IRQ_BASE (0x1000). But > > > spapr_irq_set_irq_xics() was passing through the global irq number to > > > the ICS code unmodified. > > > > > > We only got away with this because of a counteracting bug - we were > > > incorrectly adjusting the qemu_irq we returned for a requested global irq > > > number. > > > > > > That approach mostly worked but is very confusing, incorrectly relies on > > > the way the qemu_irq array is allocated, and undermines the intention of > > > having the global array of qemu_irqs for spapr have a consistent meaning > > > regardless of irq backend. > > > > > > So, fix both set_irq and qemu_irq indexing. We rename some parameters at > > > the same time to make it clear that they are referring to spapr global > > > irq numbers. > > > > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > > --- > > > > Reviewed-by: Greg Kurz <groug@kaod.org> > > > > Further cleanup could be to have the XICS backend to only take global > > irq numbers and to convert them to ICS source numbers internally. This > > would put an end to the confusion between srcno/irq in the frontend > > code. > > Yeah, maybe. But the local srcnos do actually make sense from within > the perspective of ICS, so I'm not all that keen to do that. > Not sure to understand what you mean by "within the perspective of ICS". My concern is actually to get rid of ics->offset users in spapr_irq.c. eg, static void spapr_irq_set_irq_xics(void *opaque, int irq, int val) { SpaprMachineState *spapr = opaque; uint32_t srcno = irq - spapr->ics->offset; ics_set_irq(spapr->ics, srcno, val); } It looks like we should do something like: static void spapr_irq_set_irq_xics(void *opaque, int irq, int val) { SpaprMachineState *spapr = opaque; ics_set_irq(spapr->ics, irq, val); } and have ics_set_irq() do: uint32_t srcno = irq - spapr->ics->offset; Are you inferring that it is better to keep the irq to srcno conversions in spapr_irq.c ?
On Thu, Sep 26, 2019 at 09:21:41AM +0200, Greg Kurz wrote: > On Thu, 26 Sep 2019 11:31:48 +1000 > David Gibson <david@gibson.dropbear.id.au> wrote: > > > On Wed, Sep 25, 2019 at 10:17:46PM +0200, Greg Kurz wrote: > > > On Wed, 25 Sep 2019 16:45:25 +1000 > > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > > > spapr global irq numbers are different from the source numbers on the ICS > > > > when using XICS - they're offset by XICS_IRQ_BASE (0x1000). But > > > > spapr_irq_set_irq_xics() was passing through the global irq number to > > > > the ICS code unmodified. > > > > > > > > We only got away with this because of a counteracting bug - we were > > > > incorrectly adjusting the qemu_irq we returned for a requested global irq > > > > number. > > > > > > > > That approach mostly worked but is very confusing, incorrectly relies on > > > > the way the qemu_irq array is allocated, and undermines the intention of > > > > having the global array of qemu_irqs for spapr have a consistent meaning > > > > regardless of irq backend. > > > > > > > > So, fix both set_irq and qemu_irq indexing. We rename some parameters at > > > > the same time to make it clear that they are referring to spapr global > > > > irq numbers. > > > > > > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > > > --- > > > > > > Reviewed-by: Greg Kurz <groug@kaod.org> > > > > > > Further cleanup could be to have the XICS backend to only take global > > > irq numbers and to convert them to ICS source numbers internally. This > > > would put an end to the confusion between srcno/irq in the frontend > > > code. > > > > Yeah, maybe. But the local srcnos do actually make sense from within > > the perspective of ICS, so I'm not all that keen to do that. > > > > Not sure to understand what you mean by "within the perspective of ICS". > > My concern is actually to get rid of ics->offset users in spapr_irq.c. > > eg, > > static void spapr_irq_set_irq_xics(void *opaque, int irq, int val) > { > SpaprMachineState *spapr = opaque; > uint32_t srcno = irq - spapr->ics->offset; > > ics_set_irq(spapr->ics, srcno, val); > } > > It looks like we should do something like: > > static void spapr_irq_set_irq_xics(void *opaque, int irq, int val) > { > SpaprMachineState *spapr = opaque; > > ics_set_irq(spapr->ics, irq, val); > } > > and have ics_set_irq() do: > > uint32_t srcno = irq - spapr->ics->offset; > > Are you inferring that it is better to keep the irq to srcno conversions > in spapr_irq.c ? Ah, I see what you mean. So, the reason srcno appears here is that ics_set_irq() is arguably an internal xics function that we shouldn't be calling directly from here, but do because of.. reasons. As it happens, I have another patch in the works which will move this function back into xics_spapr.c and bind it more tightly to the xics code, so I think that will address your concern.
On Thu, 26 Sep 2019 21:32:18 +1000 David Gibson <david@gibson.dropbear.id.au> wrote: > On Thu, Sep 26, 2019 at 09:21:41AM +0200, Greg Kurz wrote: > > On Thu, 26 Sep 2019 11:31:48 +1000 > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > On Wed, Sep 25, 2019 at 10:17:46PM +0200, Greg Kurz wrote: > > > > On Wed, 25 Sep 2019 16:45:25 +1000 > > > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > > > > > spapr global irq numbers are different from the source numbers on the ICS > > > > > when using XICS - they're offset by XICS_IRQ_BASE (0x1000). But > > > > > spapr_irq_set_irq_xics() was passing through the global irq number to > > > > > the ICS code unmodified. > > > > > > > > > > We only got away with this because of a counteracting bug - we were > > > > > incorrectly adjusting the qemu_irq we returned for a requested global irq > > > > > number. > > > > > > > > > > That approach mostly worked but is very confusing, incorrectly relies on > > > > > the way the qemu_irq array is allocated, and undermines the intention of > > > > > having the global array of qemu_irqs for spapr have a consistent meaning > > > > > regardless of irq backend. > > > > > > > > > > So, fix both set_irq and qemu_irq indexing. We rename some parameters at > > > > > the same time to make it clear that they are referring to spapr global > > > > > irq numbers. > > > > > > > > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > > > > --- > > > > > > > > Reviewed-by: Greg Kurz <groug@kaod.org> > > > > > > > > Further cleanup could be to have the XICS backend to only take global > > > > irq numbers and to convert them to ICS source numbers internally. This > > > > would put an end to the confusion between srcno/irq in the frontend > > > > code. > > > > > > Yeah, maybe. But the local srcnos do actually make sense from within > > > the perspective of ICS, so I'm not all that keen to do that. > > > > > > > Not sure to understand what you mean by "within the perspective of ICS". > > > > My concern is actually to get rid of ics->offset users in spapr_irq.c. > > > > eg, > > > > static void spapr_irq_set_irq_xics(void *opaque, int irq, int val) > > { > > SpaprMachineState *spapr = opaque; > > uint32_t srcno = irq - spapr->ics->offset; > > > > ics_set_irq(spapr->ics, srcno, val); > > } > > > > It looks like we should do something like: > > > > static void spapr_irq_set_irq_xics(void *opaque, int irq, int val) > > { > > SpaprMachineState *spapr = opaque; > > > > ics_set_irq(spapr->ics, irq, val); > > } > > > > and have ics_set_irq() do: > > > > uint32_t srcno = irq - spapr->ics->offset; > > > > Are you inferring that it is better to keep the irq to srcno conversions > > in spapr_irq.c ? > > Ah, I see what you mean. So, the reason srcno appears here is that > ics_set_irq() is arguably an internal xics function that we shouldn't > be calling directly from here, but do because of.. reasons. > > As it happens, I have another patch in the works which will move this > function back into xics_spapr.c and bind it more tightly to the xics > code, so I think that will address your concern. > Great ! :)
diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c index 300c65be3a..9a9e486eb5 100644 --- a/hw/ppc/spapr_irq.c +++ b/hw/ppc/spapr_irq.c @@ -153,10 +153,9 @@ static void spapr_irq_free_xics(SpaprMachineState *spapr, int irq, int num) static qemu_irq spapr_qirq_xics(SpaprMachineState *spapr, int irq) { ICSState *ics = spapr->ics; - uint32_t srcno = irq - ics->offset; if (ics_valid_irq(ics, irq)) { - return spapr->qirqs[srcno]; + return spapr->qirqs[irq]; } return NULL; @@ -204,9 +203,10 @@ static int spapr_irq_post_load_xics(SpaprMachineState *spapr, int version_id) return 0; } -static void spapr_irq_set_irq_xics(void *opaque, int srcno, int val) +static void spapr_irq_set_irq_xics(void *opaque, int irq, int val) { SpaprMachineState *spapr = opaque; + uint32_t srcno = irq - spapr->ics->offset; ics_set_irq(spapr->ics, srcno, val); } @@ -377,14 +377,14 @@ static void spapr_irq_reset_xive(SpaprMachineState *spapr, Error **errp) spapr_xive_mmio_set_enabled(spapr->xive, true); } -static void spapr_irq_set_irq_xive(void *opaque, int srcno, int val) +static void spapr_irq_set_irq_xive(void *opaque, int irq, int val) { SpaprMachineState *spapr = opaque; if (kvm_irqchip_in_kernel()) { - kvmppc_xive_source_set_irq(&spapr->xive->source, srcno, val); + kvmppc_xive_source_set_irq(&spapr->xive->source, irq, val); } else { - xive_source_set_irq(&spapr->xive->source, srcno, val); + xive_source_set_irq(&spapr->xive->source, irq, val); } } @@ -563,11 +563,11 @@ static void spapr_irq_reset_dual(SpaprMachineState *spapr, Error **errp) spapr_irq_current(spapr)->reset(spapr, errp); } -static void spapr_irq_set_irq_dual(void *opaque, int srcno, int val) +static void spapr_irq_set_irq_dual(void *opaque, int irq, int val) { SpaprMachineState *spapr = opaque; - spapr_irq_current(spapr)->set_irq(spapr, srcno, val); + spapr_irq_current(spapr)->set_irq(spapr, irq, val); } static const char *spapr_irq_get_nodename_dual(SpaprMachineState *spapr)
spapr global irq numbers are different from the source numbers on the ICS when using XICS - they're offset by XICS_IRQ_BASE (0x1000). But spapr_irq_set_irq_xics() was passing through the global irq number to the ICS code unmodified. We only got away with this because of a counteracting bug - we were incorrectly adjusting the qemu_irq we returned for a requested global irq number. That approach mostly worked but is very confusing, incorrectly relies on the way the qemu_irq array is allocated, and undermines the intention of having the global array of qemu_irqs for spapr have a consistent meaning regardless of irq backend. So, fix both set_irq and qemu_irq indexing. We rename some parameters at the same time to make it clear that they are referring to spapr global irq numbers. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> --- hw/ppc/spapr_irq.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)