On 5/22/19 10:52 AM, Greg Kurz wrote:
> On Wed, 22 May 2019 09:40:15 +0200
> Cédric Le Goater <c...@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 <sathn...@linux.vnet.ibm.com>
>> Signed-off-by: Cédric Le Goater <c...@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 <gr...@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);
>
