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 <address@hidden> 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 <address@hidden>
>> Signed-off-by: Cédric Le Goater <address@hidden>
>> ---
>>  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 <address@hidden>

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);
>