diff mbox series

[11/20] spapr: Fix indexing of XICS irqs

Message ID 20190925064534.19155-12-david@gibson.dropbear.id.au
State New, archived
Headers show
Series spapr: IRQ subsystem cleanups | expand

Commit Message

David Gibson Sept. 25, 2019, 6:45 a.m. UTC
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(-)

Comments

Cédric Le Goater Sept. 25, 2019, 7:11 a.m. UTC | #1
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)
>
Greg Kurz Sept. 25, 2019, 8:17 p.m. UTC | #2
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)
David Gibson Sept. 26, 2019, 1:31 a.m. UTC | #3
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.
Greg Kurz Sept. 26, 2019, 7:21 a.m. UTC | #4
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 ?
David Gibson Sept. 26, 2019, 11:32 a.m. UTC | #5
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.
Greg Kurz Sept. 26, 2019, 2:44 p.m. UTC | #6
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 mbox series

Patch

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)