Re: [PATCH v3 for-5.2 1/3] ppc/xive: Rework setup of XiveSource::esb_mmio

[Cédric Le Goater]

On 8/7/20 1:32 PM, Greg Kurz wrote:
> Depending on whether XIVE is emultated or backed with a KVM XIVE device,
> the ESB MMIOs of a XIVE source point to an I/O memory region or a mapped
> memory region.
> 
> This is currently handled by checking kvm_irqchip_in_kernel() returns
> false in xive_source_realize(). This is a bit awkward as we usually
> need to do extra things when we're using the in-kernel backend, not
> less. But most important, we can do better: turn the existing "xive.esb"
> memory region into a plain container, introduce an "xive.esb-emulated"
> I/O subregion and rename the existing "xive.esb" subregion in the KVM
> code to "xive.esb-kvm". Since "xive.esb-kvm" is added with overlap
> and a higher priority, it prevails over "xive.esb-emulated" (ie.
> a guest using KVM XIVE will interact with "xive.esb-kvm" instead of
> the default "xive.esb-emulated" region.
> 
> While here, consolidate the computation of the MMIO region size in
> a common helper.
> 
> Suggested-by: Cédric Le Goater <clg@kaod.org>
> Signed-off-by: Greg Kurz <groug@kaod.org>

This is much better

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.

> ---
>  hw/intc/spapr_xive_kvm.c |    4 ++--
>  hw/intc/xive.c           |   11 ++++++-----
>  include/hw/ppc/xive.h    |    6 ++++++
>  3 files changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
> index 893a1ee77e70..6130882be678 100644
> --- a/hw/intc/spapr_xive_kvm.c
> +++ b/hw/intc/spapr_xive_kvm.c
> @@ -742,7 +742,7 @@ int kvmppc_xive_connect(SpaprInterruptController *intc, 
> uint32_t nr_servers,
>      SpaprXive *xive = SPAPR_XIVE(intc);
>      XiveSource *xsrc = &xive->source;
>      Error *local_err = NULL;
> -    size_t esb_len = (1ull << xsrc->esb_shift) * xsrc->nr_irqs;
> +    size_t esb_len = xive_source_esb_len(xsrc);
>      size_t tima_len = 4ull << TM_SHIFT;
>      CPUState *cs;
>      int fd;
> @@ -788,7 +788,7 @@ int kvmppc_xive_connect(SpaprInterruptController *intc, 
> uint32_t nr_servers,
>      }
>  
>      memory_region_init_ram_device_ptr(&xsrc->esb_mmio_kvm, OBJECT(xsrc),
> -                                      "xive.esb", esb_len, xsrc->esb_mmap);
> +                                      "xive.esb-kvm", esb_len, 
> xsrc->esb_mmap);
>      memory_region_add_subregion_overlap(&xsrc->esb_mmio, 0,
>                                          &xsrc->esb_mmio_kvm, 1);
>  
> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
> index 9b55e0356c62..561d746cd1da 100644
> --- a/hw/intc/xive.c
> +++ b/hw/intc/xive.c
> @@ -1128,6 +1128,7 @@ static void xive_source_reset(void *dev)
>  static void xive_source_realize(DeviceState *dev, Error **errp)
>  {
>      XiveSource *xsrc = XIVE_SOURCE(dev);
> +    size_t esb_len = xive_source_esb_len(xsrc);
>  
>      assert(xsrc->xive);
>  
> @@ -1147,11 +1148,11 @@ static void xive_source_realize(DeviceState *dev, 
> Error **errp)
>      xsrc->status = g_malloc0(xsrc->nr_irqs);
>      xsrc->lsi_map = bitmap_new(xsrc->nr_irqs);
>  
> -    if (!kvm_irqchip_in_kernel()) {
> -        memory_region_init_io(&xsrc->esb_mmio, OBJECT(xsrc),
> -                              &xive_source_esb_ops, xsrc, "xive.esb",
> -                              (1ull << xsrc->esb_shift) * xsrc->nr_irqs);
> -    }
> +    memory_region_init(&xsrc->esb_mmio, OBJECT(xsrc), "xive.esb", esb_len);
> +    memory_region_init_io(&xsrc->esb_mmio_emulated, OBJECT(xsrc),
> +                          &xive_source_esb_ops, xsrc, "xive.esb-emulated",
> +                          esb_len);
> +    memory_region_add_subregion(&xsrc->esb_mmio, 0, 
> &xsrc->esb_mmio_emulated);
>  
>      qemu_register_reset(xive_source_reset, dev);
>  }
> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
> index 705cf48176fc..82a61eaca74f 100644
> --- a/include/hw/ppc/xive.h
> +++ b/include/hw/ppc/xive.h
> @@ -191,6 +191,7 @@ typedef struct XiveSource {
>      uint64_t        esb_flags;
>      uint32_t        esb_shift;
>      MemoryRegion    esb_mmio;
> +    MemoryRegion    esb_mmio_emulated;
>  
>      /* KVM support */
>      void            *esb_mmap;
> @@ -215,6 +216,11 @@ static inline bool xive_source_esb_has_2page(XiveSource 
> *xsrc)
>          xsrc->esb_shift == XIVE_ESB_4K_2PAGE;
>  }
>  
> +static inline size_t xive_source_esb_len(XiveSource *xsrc)
> +{
> +    return (1ull << xsrc->esb_shift) * xsrc->nr_irqs;
> +}
> +
>  /* The trigger page is always the first/even page */
>  static inline hwaddr xive_source_esb_page(XiveSource *xsrc, uint32_t srcno)
>  {
> 
>