[Qemu-devel] [PATCH 0/2] spapr/irq: remove the XICS offset adjustment for the dual machine

[Qemu-devel] [PATCH 0/2] spapr/irq: remove the XICS offset adjustment for the dual machine

[Cédric Le Goater]

Hello,

When using the 'dual' interrupt mode, the source numbers of both sPAPR
IRQ backends are aligned to share a common IRQ number space and to use
a similar mapping of the machine qemu_irq array which is indexed by
the source number.

The XICS IRQ number range initially being [ 0x1000 - 0x2000 ], this
requires to change the XICS ICSState offset to 0 and to provision for
an extra 4K of source numbers and qemu_irqs which will never be used
by the machine when running under the XICS interrupt mode. This is not
an optimal solution.

Moreover, the KVM support for the 'dual' interrupt mode will require
some adjustments. The XICS KVM device considers that the IRQ numbers
start at XICS_IRQ_BASE (0x1000) and we have no way to inform KVM of
the offset change. We will need to ignore the lower 4K numbers when
capturing or restoring the source states from the XICS KVM device.

The first patch allocates an IRQ number space of the expected size for
the XICS sPAPR IRQ backend. The overall IRQ number space size and the
qemu_irq array size is still the one required by the 'dual' sPAPR IRQ
backend, that is, in sync with XIVE. The second patch removes the
ICSState offset adjustment. This last change adds some benefits as it
simplifies greatly the qirq() method of the 'dual' sPAPR IRQ backend.

C.

Cédric Le Goater (2):
  spapr/irq: add an 'nr_irq' parameter to initialize the backend.
  spapr/irq: remove the XICS offset adjustment

 include/hw/ppc/spapr_irq.h |  2 +-
 hw/ppc/spapr_irq.c         | 45 ++++++++++----------------------------
 2 files changed, 12 insertions(+), 35 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH 1/2] spapr/irq: add an 'nr_irq' parameter to initialize the backend.

[Cédric Le Goater]

When using the 'dual' interrupt mode, the source numbers of both sPAPR
IRQ backends are aligned to share a common IRQ number space and to use
a similar mapping of the machine qemu_irq array which is indexed by
the source number.

The XICS IRQ number range initially being [ 0x1000 - 0x2000 ], this
requires to change the XICS ICSState offset to 0 and to provision for
an extra 4K of source numbers and qemu_irqs which will never be used
by the machine when running under the XICS interrupt mode. This is not
an optimal solution.

Change the init() method to allocate an IRQ number space of the
expected size for the XICS sPAPR IRQ backend. It breaks the interrupt
signaling when under the 'dual' mode because source numbers have
unexpected values but next patch will fix that.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/ppc/spapr_irq.h |  2 +-
 hw/ppc/spapr_irq.c         | 18 ++++++++++--------
 2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
index 14b02c3aca33..488511c3d890 100644
--- a/include/hw/ppc/spapr_irq.h
+++ b/include/hw/ppc/spapr_irq.h
@@ -35,7 +35,7 @@ typedef struct sPAPRIrq {
     uint32_t    nr_msis;
     uint8_t     ov5;
 
-    void (*init)(sPAPRMachineState *spapr, Error **errp);
+    void (*init)(sPAPRMachineState *spapr, int nr_irqs, Error **errp);
     int (*claim)(sPAPRMachineState *spapr, int irq, bool lsi, Error **errp);
     void (*free)(sPAPRMachineState *spapr, int irq, int num);
     qemu_irq (*qirq)(sPAPRMachineState *spapr, int irq);
diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
index 2d7a7c163876..99c3f6ba251a 100644
--- a/hw/ppc/spapr_irq.c
+++ b/hw/ppc/spapr_irq.c
@@ -93,10 +93,10 @@ error:
     return NULL;
 }
 
-static void spapr_irq_init_xics(sPAPRMachineState *spapr, Error **errp)
+static void spapr_irq_init_xics(sPAPRMachineState *spapr, int nr_irqs,
+                                Error **errp)
 {
     MachineState *machine = MACHINE(spapr);
-    int nr_irqs = spapr->irq->nr_irqs;
     Error *local_err = NULL;
 
     if (kvm_enabled()) {
@@ -262,7 +262,8 @@ sPAPRIrq spapr_irq_xics = {
 /*
  * XIVE IRQ backend.
  */
-static void spapr_irq_init_xive(sPAPRMachineState *spapr, Error **errp)
+static void spapr_irq_init_xive(sPAPRMachineState *spapr, int nr_irqs,
+                                Error **errp)
 {
     MachineState *machine = MACHINE(spapr);
     uint32_t nr_servers = spapr_max_server_number(spapr);
@@ -278,7 +279,7 @@ static void spapr_irq_init_xive(sPAPRMachineState *spapr, Error **errp)
     }
 
     dev = qdev_create(NULL, TYPE_SPAPR_XIVE);
-    qdev_prop_set_uint32(dev, "nr-irqs", spapr->irq->nr_irqs);
+    qdev_prop_set_uint32(dev, "nr-irqs", nr_irqs);
     /*
      * 8 XIVE END structures per CPU. One for each available priority
      */
@@ -435,7 +436,8 @@ static sPAPRIrq *spapr_irq_current(sPAPRMachineState *spapr)
         &spapr_irq_xive : &spapr_irq_xics;
 }
 
-static void spapr_irq_init_dual(sPAPRMachineState *spapr, Error **errp)
+static void spapr_irq_init_dual(sPAPRMachineState *spapr, int nr_irqs,
+                                Error **errp)
 {
     MachineState *machine = MACHINE(spapr);
     Error *local_err = NULL;
@@ -445,7 +447,7 @@ static void spapr_irq_init_dual(sPAPRMachineState *spapr, Error **errp)
         return;
     }
 
-    spapr_irq_xics.init(spapr, &local_err);
+    spapr_irq_xics.init(spapr, spapr_irq_xics.nr_irqs, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         return;
@@ -462,7 +464,7 @@ static void spapr_irq_init_dual(sPAPRMachineState *spapr, Error **errp)
      */
     spapr->ics->offset = 0;
 
-    spapr_irq_xive.init(spapr, &local_err);
+    spapr_irq_xive.init(spapr, spapr_irq_xive.nr_irqs, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         return;
@@ -605,7 +607,7 @@ void spapr_irq_init(sPAPRMachineState *spapr, Error **errp)
         spapr_irq_msi_init(spapr, spapr->irq->nr_msis);
     }
 
-    spapr->irq->init(spapr, errp);
+    spapr->irq->init(spapr, spapr->irq->nr_irqs, errp);
 
     spapr->qirqs = qemu_allocate_irqs(spapr->irq->set_irq, spapr,
                                       spapr->irq->nr_irqs);
-- 
2.20.1

[Qemu-devel] [PATCH 2/2] spapr/irq: remove the XICS offset adjustment

[Cédric Le Goater]

Now that we have changed the XICS and the XIVE interrupt backend to
have different size for their IRQ number space, we do not need to
align their source numbers anymore. Remove the offset adjustment and
wire the dual 'qirq' handler to the 'qirq' handler of the current
interrupt mode in use.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/ppc/spapr_irq.c | 27 +--------------------------
 1 file changed, 1 insertion(+), 26 deletions(-)

diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
index 99c3f6ba251a..6a6571e043b3 100644
--- a/hw/ppc/spapr_irq.c
+++ b/hw/ppc/spapr_irq.c
@@ -453,17 +453,6 @@ static void spapr_irq_init_dual(sPAPRMachineState *spapr, int nr_irqs,
         return;
     }
 
-    /*
-     * Align the XICS and the XIVE IRQ number space under QEMU.
-     *
-     * However, the XICS KVM device still considers that the IRQ
-     * numbers should start at XICS_IRQ_BASE (0x1000). Either we
-     * should introduce a KVM device ioctl to set the offset or ignore
-     * the lower 4K numbers when using the get/set ioctl of the XICS
-     * KVM device. The second option seems the least intrusive.
-     */
-    spapr->ics->offset = 0;
-
     spapr_irq_xive.init(spapr, spapr_irq_xive.nr_irqs, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
@@ -500,21 +489,7 @@ static void spapr_irq_free_dual(sPAPRMachineState *spapr, int irq, int num)
 
 static qemu_irq spapr_qirq_dual(sPAPRMachineState *spapr, int irq)
 {
-    sPAPRXive *xive = spapr->xive;
-    ICSState *ics = spapr->ics;
-
-    if (irq >= spapr->irq->nr_irqs) {
-        return NULL;
-    }
-
-    /*
-     * The IRQ number should have been claimed under both interrupt
-     * controllers.
-     */
-    assert(!ICS_IRQ_FREE(ics, irq - ics->offset));
-    assert(xive_eas_is_valid(&xive->eat[irq]));
-
-    return spapr->qirqs[irq];
+    return spapr_irq_current(spapr)->qirq(spapr, irq);
 }
 
 static void spapr_irq_print_info_dual(sPAPRMachineState *spapr, Monitor *mon)
-- 
2.20.1

Re: [Qemu-devel] [PATCH 0/2] spapr/irq: remove the XICS offset adjustment for the dual machine

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2029 bytes --]

On Wed, Feb 13, 2019 at 10:07:54PM +0100, Cédric Le Goater wrote:
> Hello,
> 
> When using the 'dual' interrupt mode, the source numbers of both sPAPR
> IRQ backends are aligned to share a common IRQ number space and to use
> a similar mapping of the machine qemu_irq array which is indexed by
> the source number.
> 
> The XICS IRQ number range initially being [ 0x1000 - 0x2000 ], this
> requires to change the XICS ICSState offset to 0 and to provision for
> an extra 4K of source numbers and qemu_irqs which will never be used
> by the machine when running under the XICS interrupt mode. This is not
> an optimal solution.
> 
> Moreover, the KVM support for the 'dual' interrupt mode will require
> some adjustments. The XICS KVM device considers that the IRQ numbers
> start at XICS_IRQ_BASE (0x1000) and we have no way to inform KVM of
> the offset change. We will need to ignore the lower 4K numbers when
> capturing or restoring the source states from the XICS KVM device.
> 
> The first patch allocates an IRQ number space of the expected size for
> the XICS sPAPR IRQ backend. The overall IRQ number space size and the
> qemu_irq array size is still the one required by the 'dual' sPAPR IRQ
> backend, that is, in sync with XIVE. The second patch removes the
> ICSState offset adjustment. This last change adds some benefits as it
> simplifies greatly the qirq() method of the 'dual' sPAPR IRQ
> backend.

Ah, much nicer.  Applied to ppc-for-4.0, thanks.


> 
> C.
> 
> Cédric Le Goater (2):
>   spapr/irq: add an 'nr_irq' parameter to initialize the backend.
>   spapr/irq: remove the XICS offset adjustment
> 
>  include/hw/ppc/spapr_irq.h |  2 +-
>  hw/ppc/spapr_irq.c         | 45 ++++++++++----------------------------
>  2 files changed, 12 insertions(+), 35 deletions(-)
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH 0/2] spapr/irq: remove the XICS offset adjustment for the dual machine

[Cédric Le Goater]

On 2/14/19 1:49 AM, David Gibson wrote:
> On Wed, Feb 13, 2019 at 10:07:54PM +0100, Cédric Le Goater wrote:
>> Hello,
>>
>> When using the 'dual' interrupt mode, the source numbers of both sPAPR
>> IRQ backends are aligned to share a common IRQ number space and to use
>> a similar mapping of the machine qemu_irq array which is indexed by
>> the source number.
>>
>> The XICS IRQ number range initially being [ 0x1000 - 0x2000 ], this
>> requires to change the XICS ICSState offset to 0 and to provision for
>> an extra 4K of source numbers and qemu_irqs which will never be used
>> by the machine when running under the XICS interrupt mode. This is not
>> an optimal solution.
>>
>> Moreover, the KVM support for the 'dual' interrupt mode will require
>> some adjustments. The XICS KVM device considers that the IRQ numbers
>> start at XICS_IRQ_BASE (0x1000) and we have no way to inform KVM of
>> the offset change. We will need to ignore the lower 4K numbers when
>> capturing or restoring the source states from the XICS KVM device.
>>
>> The first patch allocates an IRQ number space of the expected size for

s/IRQ number space/source numbers/

There is still some confusion in the way I use the terms "IRQ number" 
and "source number". These are two different number spaces. I should
pay more attention.

>> the XICS sPAPR IRQ backend. The overall IRQ number space size and the
>> qemu_irq array size is still the one required by the 'dual' sPAPR IRQ
>> backend, that is, in sync with XIVE. The second patch removes the
>> ICSState offset adjustment. This last change adds some benefits as it
>> simplifies greatly the qirq() method of the 'dual' sPAPR IRQ
>> backend.
> 
> Ah, much nicer.  Applied to ppc-for-4.0, thanks.

I will just drop patch "spapr/xics: ignore the lower 4K in the IRQ 
number space"

Thanks,

C.