On Fri, 14 Jun 2019 18:59:19 +0200
Cédric Le Goater <c...@kaod.org> wrote:
> Today, the interrupt device is fully initialized at reset when the CAS
> negotiation process has completed. Depending on the KVM capabilities,
> the SpaprXive memory regions (ESB, TIMA) are initialized with a host
> MMIO backend or a QEMU emulated backend. This results in a complex
> initialization sequence partially done at realize and later at reset,
> and some memory region leaks.
>
> To simplify this sequence and to remove of the late initialization of
> the emulated device which is required to be done only once, we
> introduce new memory regions specific for KVM. These regions are
> mapped as overlaps on top of the emulated device to make use of the
> host MMIOs. Also provide proper cleanups of these regions when the
> XIVE KVM device is destroyed to fix the leaks.
>
> Signed-off-by: Cédric Le Goater <c...@kaod.org>
> ---
Nice !
Reviewed-by: Greg Kurz <gr...@kaod.org>
> include/hw/ppc/spapr_xive.h | 2 +-
> include/hw/ppc/xive.h | 1 +
> hw/intc/spapr_xive.c | 38 ++++++++++---------------------------
> hw/intc/spapr_xive_kvm.c | 21 +++++++++++---------
> hw/ppc/spapr_irq.c | 1 -
> 5 files changed, 24 insertions(+), 39 deletions(-)
>
> diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
> index b26befcf6b56..719714426524 100644
> --- a/include/hw/ppc/spapr_xive.h
> +++ b/include/hw/ppc/spapr_xive.h
> @@ -42,6 +42,7 @@ typedef struct SpaprXive {
> /* KVM support */
> int fd;
> void *tm_mmap;
> + MemoryRegion tm_mmio_kvm;
> VMChangeStateEntry *change;
> } SpaprXive;
>
> @@ -66,7 +67,6 @@ void spapr_xive_map_mmio(SpaprXive *xive);
>
> int spapr_xive_end_to_target(uint8_t end_blk, uint32_t end_idx,
> uint32_t *out_server, uint8_t *out_prio);
> -void spapr_xive_init(SpaprXive *xive, Error **errp);
>
> /*
> * KVM XIVE device helpers
> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
> index a6ee7e831d8b..55c53c741776 100644
> --- a/include/hw/ppc/xive.h
> +++ b/include/hw/ppc/xive.h
> @@ -197,6 +197,7 @@ typedef struct XiveSource {
>
> /* KVM support */
> void *esb_mmap;
> + MemoryRegion esb_mmio_kvm;
>
> XiveNotifier *xive;
> } XiveSource;
> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> index 58c2e5d890bd..3ae311d9ff7f 100644
> --- a/hw/intc/spapr_xive.c
> +++ b/hw/intc/spapr_xive.c
> @@ -194,13 +194,6 @@ void spapr_xive_pic_print_info(SpaprXive *xive, Monitor
> *mon)
> }
> }
>
> -void spapr_xive_map_mmio(SpaprXive *xive)
> -{
> - sysbus_mmio_map(SYS_BUS_DEVICE(xive), 0, xive->vc_base);
> - sysbus_mmio_map(SYS_BUS_DEVICE(xive), 1, xive->end_base);
> - sysbus_mmio_map(SYS_BUS_DEVICE(xive), 2, xive->tm_base);
> -}
> -
> void spapr_xive_mmio_set_enabled(SpaprXive *xive, bool enable)
> {
> memory_region_set_enabled(&xive->source.esb_mmio, enable);
> @@ -305,6 +298,7 @@ static void spapr_xive_realize(DeviceState *dev, Error
> **errp)
> error_propagate(errp, local_err);
> return;
> }
> + sysbus_init_mmio(SYS_BUS_DEVICE(xive), &xsrc->esb_mmio);
>
> /*
> * Initialize the END ESB source
> @@ -318,6 +312,7 @@ static void spapr_xive_realize(DeviceState *dev, Error
> **errp)
> error_propagate(errp, local_err);
> return;
> }
> + sysbus_init_mmio(SYS_BUS_DEVICE(xive), &end_xsrc->esb_mmio);
>
> /* Set the mapping address of the END ESB pages after the source ESBs */
> xive->end_base = xive->vc_base + (1ull << xsrc->esb_shift) *
> xsrc->nr_irqs;
> @@ -333,31 +328,18 @@ static void spapr_xive_realize(DeviceState *dev, Error
> **errp)
>
> 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;
> -
> - /*
> - * The emulated XIVE device can only be initialized once. If the
> - * ESB memory region has been already mapped, it means we have been
> - * through there.
> - */
> - if (memory_region_is_mapped(&xsrc->esb_mmio)) {
> - return;
> - }
> -
> /* TIMA initialization */
> memory_region_init_io(&xive->tm_mmio, OBJECT(xive), &xive_tm_ops, xive,
> "xive.tima", 4ull << TM_SHIFT);
> + sysbus_init_mmio(SYS_BUS_DEVICE(xive), &xive->tm_mmio);
>
> - /* Map all regions */
> - spapr_xive_map_mmio(xive);
> + /*
> + * Map all regions. These will be enabled or disabled at reset and
> + * can also be overridden by KVM memory regions if active
> + */
> + sysbus_mmio_map(SYS_BUS_DEVICE(xive), 0, xive->vc_base);
> + sysbus_mmio_map(SYS_BUS_DEVICE(xive), 1, xive->end_base);
> + sysbus_mmio_map(SYS_BUS_DEVICE(xive), 2, xive->tm_base);
> }
>
> static int spapr_xive_get_eas(XiveRouter *xrtr, uint8_t eas_blk,
> diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
> index b48f135838f2..5559f8bce5ef 100644
> --- a/hw/intc/spapr_xive_kvm.c
> +++ b/hw/intc/spapr_xive_kvm.c
> @@ -728,8 +728,10 @@ void kvmppc_xive_connect(SpaprXive *xive, Error **errp)
> return;
> }
>
> - memory_region_init_ram_device_ptr(&xsrc->esb_mmio, OBJECT(xsrc),
> + memory_region_init_ram_device_ptr(&xsrc->esb_mmio_kvm, OBJECT(xsrc),
> "xive.esb", esb_len, xsrc->esb_mmap);
> + memory_region_add_subregion_overlap(&xsrc->esb_mmio, 0,
> + &xsrc->esb_mmio_kvm, 1);
>
> /*
> * 2. END ESB pages (No KVM support yet)
> @@ -744,8 +746,10 @@ void kvmppc_xive_connect(SpaprXive *xive, Error **errp)
> error_propagate(errp, local_err);
> return;
> }
> - memory_region_init_ram_device_ptr(&xive->tm_mmio, OBJECT(xive),
> + memory_region_init_ram_device_ptr(&xive->tm_mmio_kvm, OBJECT(xive),
> "xive.tima", tima_len, xive->tm_mmap);
> + memory_region_add_subregion_overlap(&xive->tm_mmio, 0,
> + &xive->tm_mmio_kvm, 1);
>
> xive->change = qemu_add_vm_change_state_handler(
> kvmppc_xive_change_state_handler, xive);
> @@ -771,9 +775,6 @@ void kvmppc_xive_connect(SpaprXive *xive, Error **errp)
> kvm_kernel_irqchip = true;
> kvm_msi_via_irqfd_allowed = true;
> kvm_gsi_direct_mapping = true;
> -
> - /* Map all regions */
> - spapr_xive_map_mmio(xive);
> }
>
> void kvmppc_xive_disconnect(SpaprXive *xive, Error **errp)
> @@ -795,13 +796,15 @@ void kvmppc_xive_disconnect(SpaprXive *xive, Error
> **errp)
> xsrc = &xive->source;
> esb_len = (1ull << xsrc->esb_shift) * xsrc->nr_irqs;
>
> - sysbus_mmio_unmap(SYS_BUS_DEVICE(xive), 0);
> + memory_region_del_subregion(&xsrc->esb_mmio, &xsrc->esb_mmio_kvm);
> + object_unparent(OBJECT(&xsrc->esb_mmio_kvm));
> munmap(xsrc->esb_mmap, esb_len);
> + xsrc->esb_mmap = NULL;
>
> - sysbus_mmio_unmap(SYS_BUS_DEVICE(xive), 1);
> -
> - sysbus_mmio_unmap(SYS_BUS_DEVICE(xive), 2);
> + memory_region_del_subregion(&xive->tm_mmio, &xive->tm_mmio_kvm);
> + object_unparent(OBJECT(&xive->tm_mmio_kvm));
> munmap(xive->tm_mmap, 4ull << TM_SHIFT);
> + xive->tm_mmap = NULL;
>
> /*
> * When the KVM device fd is closed, the KVM device is destroyed
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index 75654fc67aba..73e6f10da165 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -413,7 +413,6 @@ static const char
> *spapr_irq_get_nodename_xive(SpaprMachineState *spapr)
>
> static void spapr_irq_init_emu_xive(SpaprMachineState *spapr, Error **errp)
> {
> - spapr_xive_init(spapr->xive, errp);
> }
>
> static void spapr_irq_init_kvm_xive(SpaprMachineState *spapr, Error **errp)
