[Qemu-devel] [PATCH 0/2] spapr/xive: change default interrupt mode to 'dual'

[Qemu-devel] [PATCH 0/2] spapr/xive: change default interrupt mode to 'dual'

[Cédric Le Goater]

Hello,

Here is a little series fixing multiple resets when in 'dual'
interrupt mode and changing the default mode to 'dual'.

Thanks,

C.

Cédric Le Goater (2):
  spapr/xive: fix multiple resets when using the 'dual' interrupt mode
  spapr: change default interrupt mode to 'dual'

 hw/intc/spapr_xive.c     | 11 +++++------
 hw/intc/spapr_xive_kvm.c |  4 ----
 hw/ppc/spapr.c           |  3 ++-
 3 files changed, 7 insertions(+), 11 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH 1/2] spapr/xive: fix multiple resets when using the 'dual' interrupt mode

[Cédric Le Goater]

Today, when a reset occurs on a pseries machine using the 'dual'
interrupt mode, the KVM devices are released and recreated depending
on the interrupt mode selected by CAS. If XIVE is selected, the SysBus
memory regions of the SpaprXive model are initialized by the KVM
backend initialization routine each time a reset occurs. This leads to
a crash after a couple of resets because the machine reaches the
QDEV_MAX_MMIO limit of SysBusDevice :

qemu-system-ppc64: hw/core/sysbus.c:193: sysbus_init_mmio: Assertion `dev->num_mmio < QDEV_MAX_MMIO' failed.

To fix, initialize the SysBus memory regions in spapr_xive_realize()
called only once and remove the same inits from the QEMU and KVM
backend initialization routines which are called at each reset.

Reported-by: Satheesh Rajendran <sathnaga@linux.vnet.ibm.com>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/intc/spapr_xive.c     | 11 +++++------
 hw/intc/spapr_xive_kvm.c |  4 ----
 2 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
index f6f6c29d6ae4..62e0ef8fa5b4 100644
--- a/hw/intc/spapr_xive.c
+++ b/hw/intc/spapr_xive.c
@@ -331,12 +331,16 @@ static void spapr_xive_realize(DeviceState *dev, Error **errp)
                            xive->tm_base + XIVE_TM_USER_PAGE * (1 << TM_SHIFT));
 
     qemu_register_reset(spapr_xive_reset, dev);
+
+    /* Define all XIVE MMIO regions on SysBus */
+    sysbus_init_mmio(SYS_BUS_DEVICE(xive), &xsrc->esb_mmio);
+    sysbus_init_mmio(SYS_BUS_DEVICE(xive), &end_xsrc->esb_mmio);
+    sysbus_init_mmio(SYS_BUS_DEVICE(xive), &xive->tm_mmio);
 }
 
 void spapr_xive_init(SpaprXive *xive, Error **errp)
 {
     XiveSource *xsrc = &xive->source;
-    XiveENDSource *end_xsrc = &xive->end_source;
 
     /*
      * The emulated XIVE device can only be initialized once. If the
@@ -351,11 +355,6 @@ void spapr_xive_init(SpaprXive *xive, Error **errp)
     memory_region_init_io(&xive->tm_mmio, OBJECT(xive), &xive_tm_ops, xive,
                           "xive.tima", 4ull << TM_SHIFT);
 
-    /* Define all XIVE MMIO regions on SysBus */
-    sysbus_init_mmio(SYS_BUS_DEVICE(xive), &xsrc->esb_mmio);
-    sysbus_init_mmio(SYS_BUS_DEVICE(xive), &end_xsrc->esb_mmio);
-    sysbus_init_mmio(SYS_BUS_DEVICE(xive), &xive->tm_mmio);
-
     /* Map all regions */
     spapr_xive_map_mmio(xive);
 }
diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
index ec170b304555..b48f135838f2 100644
--- a/hw/intc/spapr_xive_kvm.c
+++ b/hw/intc/spapr_xive_kvm.c
@@ -693,7 +693,6 @@ static void *kvmppc_xive_mmap(SpaprXive *xive, int pgoff, size_t len,
 void kvmppc_xive_connect(SpaprXive *xive, Error **errp)
 {
     XiveSource *xsrc = &xive->source;
-    XiveENDSource *end_xsrc = &xive->end_source;
     Error *local_err = NULL;
     size_t esb_len = (1ull << xsrc->esb_shift) * xsrc->nr_irqs;
     size_t tima_len = 4ull << TM_SHIFT;
@@ -731,12 +730,10 @@ void kvmppc_xive_connect(SpaprXive *xive, Error **errp)
 
     memory_region_init_ram_device_ptr(&xsrc->esb_mmio, OBJECT(xsrc),
                                       "xive.esb", esb_len, xsrc->esb_mmap);
-    sysbus_init_mmio(SYS_BUS_DEVICE(xive), &xsrc->esb_mmio);
 
     /*
      * 2. END ESB pages (No KVM support yet)
      */
-    sysbus_init_mmio(SYS_BUS_DEVICE(xive), &end_xsrc->esb_mmio);
 
     /*
      * 3. TIMA pages - KVM mapping
@@ -749,7 +746,6 @@ void kvmppc_xive_connect(SpaprXive *xive, Error **errp)
     }
     memory_region_init_ram_device_ptr(&xive->tm_mmio, OBJECT(xive),
                                       "xive.tima", tima_len, xive->tm_mmap);
-    sysbus_init_mmio(SYS_BUS_DEVICE(xive), &xive->tm_mmio);
 
     xive->change = qemu_add_vm_change_state_handler(
         kvmppc_xive_change_state_handler, xive);
-- 
2.20.1

Re: [Qemu-devel] [Qemu-ppc] [PATCH 1/2] spapr/xive: fix multiple resets when using the 'dual' interrupt mode

[Greg Kurz]

On Wed, 22 May 2019 09:40:15 +0200
Cédric Le Goater <clg@kaod.org> wrote:

> Today, when a reset occurs on a pseries machine using the 'dual'
> interrupt mode, the KVM devices are released and recreated depending
> on the interrupt mode selected by CAS. If XIVE is selected, the SysBus
> memory regions of the SpaprXive model are initialized by the KVM
> backend initialization routine each time a reset occurs. This leads to
> a crash after a couple of resets because the machine reaches the
> QDEV_MAX_MMIO limit of SysBusDevice :
> 
> qemu-system-ppc64: hw/core/sysbus.c:193: sysbus_init_mmio: Assertion `dev->num_mmio < QDEV_MAX_MMIO' failed.
> 
> To fix, initialize the SysBus memory regions in spapr_xive_realize()
> called only once and remove the same inits from the QEMU and KVM
> backend initialization routines which are called at each reset.
> 

Yeah, sysbus_init_mmio() simply records the memory region pointer into the
mmio array of the sysbus device. It doesn't require the memory region to be
initialized beforehand and it really must be called only once during the
device life time. Certainly not a reset thing. Doing this at realize looks
a lot better.

> Reported-by: Satheesh Rajendran <sathnaga@linux.vnet.ibm.com>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  hw/intc/spapr_xive.c     | 11 +++++------
>  hw/intc/spapr_xive_kvm.c |  4 ----
>  2 files changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> index f6f6c29d6ae4..62e0ef8fa5b4 100644
> --- a/hw/intc/spapr_xive.c
> +++ b/hw/intc/spapr_xive.c
> @@ -331,12 +331,16 @@ static void spapr_xive_realize(DeviceState *dev, Error **errp)
>                             xive->tm_base + XIVE_TM_USER_PAGE * (1 << TM_SHIFT));
>  
>      qemu_register_reset(spapr_xive_reset, dev);
> +
> +    /* Define all XIVE MMIO regions on SysBus */
> +    sysbus_init_mmio(SYS_BUS_DEVICE(xive), &xsrc->esb_mmio);
> +    sysbus_init_mmio(SYS_BUS_DEVICE(xive), &end_xsrc->esb_mmio);
> +    sysbus_init_mmio(SYS_BUS_DEVICE(xive), &xive->tm_mmio);
>  }
>  
>  void spapr_xive_init(SpaprXive *xive, Error **errp)
>  {
>      XiveSource *xsrc = &xive->source;
> -    XiveENDSource *end_xsrc = &xive->end_source;
>  
>      /*
>       * The emulated XIVE device can only be initialized once. If the
> @@ -351,11 +355,6 @@ void spapr_xive_init(SpaprXive *xive, Error **errp)
>      memory_region_init_io(&xive->tm_mmio, OBJECT(xive), &xive_tm_ops, xive,
>                            "xive.tima", 4ull << TM_SHIFT);
>  
> -    /* Define all XIVE MMIO regions on SysBus */
> -    sysbus_init_mmio(SYS_BUS_DEVICE(xive), &xsrc->esb_mmio);
> -    sysbus_init_mmio(SYS_BUS_DEVICE(xive), &end_xsrc->esb_mmio);
> -    sysbus_init_mmio(SYS_BUS_DEVICE(xive), &xive->tm_mmio);
> -
>      /* Map all regions */
>      spapr_xive_map_mmio(xive);
>  }
> diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
> index ec170b304555..b48f135838f2 100644
> --- a/hw/intc/spapr_xive_kvm.c
> +++ b/hw/intc/spapr_xive_kvm.c
> @@ -693,7 +693,6 @@ static void *kvmppc_xive_mmap(SpaprXive *xive, int pgoff, size_t len,
>  void kvmppc_xive_connect(SpaprXive *xive, Error **errp)
>  {
>      XiveSource *xsrc = &xive->source;
> -    XiveENDSource *end_xsrc = &xive->end_source;
>      Error *local_err = NULL;
>      size_t esb_len = (1ull << xsrc->esb_shift) * xsrc->nr_irqs;
>      size_t tima_len = 4ull << TM_SHIFT;
> @@ -731,12 +730,10 @@ void kvmppc_xive_connect(SpaprXive *xive, Error **errp)
>  
>      memory_region_init_ram_device_ptr(&xsrc->esb_mmio, OBJECT(xsrc),
>                                        "xive.esb", esb_len, xsrc->esb_mmap);
> -    sysbus_init_mmio(SYS_BUS_DEVICE(xive), &xsrc->esb_mmio);
>  
>      /*
>       * 2. END ESB pages (No KVM support yet)
>       */
> -    sysbus_init_mmio(SYS_BUS_DEVICE(xive), &end_xsrc->esb_mmio);
>  

It looks a bit weird to end up with a comment but no related code...
maybe simply drop it and s/3/2 in the other comment below ?

Apart from that, LGTM:

Reviewed-by: Greg Kurz <groug@kaod.org>

>      /*
>       * 3. TIMA pages - KVM mapping
> @@ -749,7 +746,6 @@ void kvmppc_xive_connect(SpaprXive *xive, Error **errp)
>      }
>      memory_region_init_ram_device_ptr(&xive->tm_mmio, OBJECT(xive),
>                                        "xive.tima", tima_len, xive->tm_mmap);
> -    sysbus_init_mmio(SYS_BUS_DEVICE(xive), &xive->tm_mmio);
>  
>      xive->change = qemu_add_vm_change_state_handler(
>          kvmppc_xive_change_state_handler, xive);

Re: [Qemu-devel] [Qemu-ppc] [PATCH 1/2] spapr/xive: fix multiple resets when using the 'dual' interrupt mode

[Cédric Le Goater]

On 5/22/19 10:52 AM, Greg Kurz wrote:
> On Wed, 22 May 2019 09:40:15 +0200
> Cédric Le Goater <clg@kaod.org> wrote:
> 
>> Today, when a reset occurs on a pseries machine using the 'dual'
>> interrupt mode, the KVM devices are released and recreated depending
>> on the interrupt mode selected by CAS. If XIVE is selected, the SysBus
>> memory regions of the SpaprXive model are initialized by the KVM
>> backend initialization routine each time a reset occurs. This leads to
>> a crash after a couple of resets because the machine reaches the
>> QDEV_MAX_MMIO limit of SysBusDevice :
>>
>> qemu-system-ppc64: hw/core/sysbus.c:193: sysbus_init_mmio: Assertion `dev->num_mmio < QDEV_MAX_MMIO' failed.
>>
>> To fix, initialize the SysBus memory regions in spapr_xive_realize()
>> called only once and remove the same inits from the QEMU and KVM
>> backend initialization routines which are called at each reset.
>>
> 
> Yeah, sysbus_init_mmio() simply records the memory region pointer into the
> mmio array of the sysbus device. It doesn't require the memory region to be
> initialized beforehand and it really must be called only once during the
> device life time. Certainly not a reset thing. Doing this at realize looks
> a lot better.
> 
>> Reported-by: Satheesh Rajendran <sathnaga@linux.vnet.ibm.com>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  hw/intc/spapr_xive.c     | 11 +++++------
>>  hw/intc/spapr_xive_kvm.c |  4 ----
>>  2 files changed, 5 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
>> index f6f6c29d6ae4..62e0ef8fa5b4 100644
>> --- a/hw/intc/spapr_xive.c
>> +++ b/hw/intc/spapr_xive.c
>> @@ -331,12 +331,16 @@ static void spapr_xive_realize(DeviceState *dev, Error **errp)
>>                             xive->tm_base + XIVE_TM_USER_PAGE * (1 << TM_SHIFT));
>>  
>>      qemu_register_reset(spapr_xive_reset, dev);
>> +
>> +    /* Define all XIVE MMIO regions on SysBus */
>> +    sysbus_init_mmio(SYS_BUS_DEVICE(xive), &xsrc->esb_mmio);
>> +    sysbus_init_mmio(SYS_BUS_DEVICE(xive), &end_xsrc->esb_mmio);
>> +    sysbus_init_mmio(SYS_BUS_DEVICE(xive), &xive->tm_mmio);
>>  }
>>  
>>  void spapr_xive_init(SpaprXive *xive, Error **errp)
>>  {
>>      XiveSource *xsrc = &xive->source;
>> -    XiveENDSource *end_xsrc = &xive->end_source;
>>  
>>      /*
>>       * The emulated XIVE device can only be initialized once. If the
>> @@ -351,11 +355,6 @@ void spapr_xive_init(SpaprXive *xive, Error **errp)
>>      memory_region_init_io(&xive->tm_mmio, OBJECT(xive), &xive_tm_ops, xive,
>>                            "xive.tima", 4ull << TM_SHIFT);
>>  
>> -    /* Define all XIVE MMIO regions on SysBus */
>> -    sysbus_init_mmio(SYS_BUS_DEVICE(xive), &xsrc->esb_mmio);
>> -    sysbus_init_mmio(SYS_BUS_DEVICE(xive), &end_xsrc->esb_mmio);
>> -    sysbus_init_mmio(SYS_BUS_DEVICE(xive), &xive->tm_mmio);
>> -
>>      /* Map all regions */
>>      spapr_xive_map_mmio(xive);
>>  }
>> diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
>> index ec170b304555..b48f135838f2 100644
>> --- a/hw/intc/spapr_xive_kvm.c
>> +++ b/hw/intc/spapr_xive_kvm.c
>> @@ -693,7 +693,6 @@ static void *kvmppc_xive_mmap(SpaprXive *xive, int pgoff, size_t len,
>>  void kvmppc_xive_connect(SpaprXive *xive, Error **errp)
>>  {
>>      XiveSource *xsrc = &xive->source;
>> -    XiveENDSource *end_xsrc = &xive->end_source;
>>      Error *local_err = NULL;
>>      size_t esb_len = (1ull << xsrc->esb_shift) * xsrc->nr_irqs;
>>      size_t tima_len = 4ull << TM_SHIFT;
>> @@ -731,12 +730,10 @@ void kvmppc_xive_connect(SpaprXive *xive, Error **errp)
>>  
>>      memory_region_init_ram_device_ptr(&xsrc->esb_mmio, OBJECT(xsrc),
>>                                        "xive.esb", esb_len, xsrc->esb_mmap);
>> -    sysbus_init_mmio(SYS_BUS_DEVICE(xive), &xsrc->esb_mmio);
>>  
>>      /*
>>       * 2. END ESB pages (No KVM support yet)
>>       */
>> -    sysbus_init_mmio(SYS_BUS_DEVICE(xive), &end_xsrc->esb_mmio);
>>  
> 
> It looks a bit weird to end up with a comment but no related code...
> maybe simply drop it and s/3/2 in the other comment below ?

I rather keep it that way. It reminds people that there are three
distinct memory regions and one is currently not supported (by KVM 
and the guest OS)

> Apart from that, LGTM:
> 
> Reviewed-by: Greg Kurz <groug@kaod.org>

Thanks,

C. 


> 
>>      /*
>>       * 3. TIMA pages - KVM mapping
>> @@ -749,7 +746,6 @@ void kvmppc_xive_connect(SpaprXive *xive, Error **errp)
>>      }
>>      memory_region_init_ram_device_ptr(&xive->tm_mmio, OBJECT(xive),
>>                                        "xive.tima", tima_len, xive->tm_mmap);
>> -    sysbus_init_mmio(SYS_BUS_DEVICE(xive), &xive->tm_mmio);
>>  
>>      xive->change = qemu_add_vm_change_state_handler(
>>          kvmppc_xive_change_state_handler, xive);
> 

[Qemu-devel] [PATCH 2/2] spapr: change default interrupt mode to 'dual'

[Cédric Le Goater]

Now that XIVE support is complete (QEMU emulated and KVM devices),
change the pseries machine to advertise both interrupt modes: XICS
(P7/P8) and XIVE (P9).

The machine default interrupt modes depends on the version. Current
settings are:

    pseries   default interrupt mode

    4.1       dual
    4.0       xics
    3.1       xics
    3.0       legacy xics (different IRQ number space layout)

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/ppc/spapr.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 39e698e9b013..4fd16b43f014 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -4352,7 +4352,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
     smc->default_caps.caps[SPAPR_CAP_LARGE_DECREMENTER] = SPAPR_CAP_ON;
     smc->default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_OFF;
     spapr_caps_add_properties(smc, &error_abort);
-    smc->irq = &spapr_irq_xics;
+    smc->irq = &spapr_irq_dual;
     smc->dr_phb_enabled = true;
 }
 
@@ -4430,6 +4430,7 @@ static void spapr_machine_4_0_class_options(MachineClass *mc)
     spapr_machine_4_1_class_options(mc);
     compat_props_add(mc->compat_props, hw_compat_4_0, hw_compat_4_0_len);
     smc->phb_placement = phb_placement_4_0;
+    smc->irq = &spapr_irq_xics;
 }
 
 DEFINE_SPAPR_MACHINE(4_0, "4.0", false);
-- 
2.20.1

Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/2] spapr: change default interrupt mode to 'dual'

[Greg Kurz]

On Wed, 22 May 2019 09:40:16 +0200
Cédric Le Goater <clg@kaod.org> wrote:

> Now that XIVE support is complete (QEMU emulated and KVM devices),
> change the pseries machine to advertise both interrupt modes: XICS
> (P7/P8) and XIVE (P9).
> 
> The machine default interrupt modes depends on the version. Current
> settings are:
> 
>     pseries   default interrupt mode
> 
>     4.1       dual
>     4.0       xics
>     3.1       xics
>     3.0       legacy xics (different IRQ number space layout)
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---

At last ! :)

Reviewed-by: Greg Kurz <groug@kaod.org>

>  hw/ppc/spapr.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 39e698e9b013..4fd16b43f014 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -4352,7 +4352,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
>      smc->default_caps.caps[SPAPR_CAP_LARGE_DECREMENTER] = SPAPR_CAP_ON;
>      smc->default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_OFF;
>      spapr_caps_add_properties(smc, &error_abort);
> -    smc->irq = &spapr_irq_xics;
> +    smc->irq = &spapr_irq_dual;
>      smc->dr_phb_enabled = true;
>  }
>  
> @@ -4430,6 +4430,7 @@ static void spapr_machine_4_0_class_options(MachineClass *mc)
>      spapr_machine_4_1_class_options(mc);
>      compat_props_add(mc->compat_props, hw_compat_4_0, hw_compat_4_0_len);
>      smc->phb_placement = phb_placement_4_0;
> +    smc->irq = &spapr_irq_xics;
>  }
>  
>  DEFINE_SPAPR_MACHINE(4_0, "4.0", false);

Re: [Qemu-devel] [PATCH 0/2] spapr/xive: change default interrupt mode to 'dual'

[David Gibson]

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

On Wed, May 22, 2019 at 09:40:14AM +0200, Cédric Le Goater wrote:
> Hello,
> 
> Here is a little series fixing multiple resets when in 'dual'
> interrupt mode and changing the default mode to 'dual'.

Applied, thanks.

> 
> Thanks,
> 
> C.
> 
> Cédric Le Goater (2):
>   spapr/xive: fix multiple resets when using the 'dual' interrupt mode
>   spapr: change default interrupt mode to 'dual'
> 
>  hw/intc/spapr_xive.c     | 11 +++++------
>  hw/intc/spapr_xive_kvm.c |  4 ----
>  hw/ppc/spapr.c           |  3 ++-
>  3 files changed, 7 insertions(+), 11 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 --]