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

[David Gibson]

On Fri, Aug 07, 2020 at 02:36:01PM +0200, Cédric Le Goater wrote:
> 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>

Appled to ppc-for-5.2.

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

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

signature.asc
Description: PGP signature