Re: [PATCH for-6.0 3/8] spapr/xive: Add "nr-servers" property

[Greg Kurz]

On Mon, 23 Nov 2020 14:52:14 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Fri, Nov 20, 2020 at 06:46:41PM +0100, Greg Kurz wrote:
> > The sPAPR XIVE object has an "nr-ends" property that is used
> > to size the END table. This property is set by the machine
> > code to a value derived from spapr_max_server_number().
> > 
> > spapr_max_server_number() is also used to inform the KVM XIVE
> > device about the range of vCPU ids it might be exposed to,
> > in order to optimize resource allocation in the HW.
> > 
> > This is enough motivation to introduce an "nr-servers" property
> > and to use it for both purposes. The existing "nr-ends" property
> > is now longer used. It is kept around though because it is exposed
> > to -global. It will continue to be ignored as before without
> > causing QEMU to exit.
> 
> I'm a little dubious about keeping the property around.  It's
> technically a breaking change to remove it, but since IIUC, it's
> *never* had any effect, it seems implausible anyone out there's using
> it.
> 

Likely. BTW, I find a bit odd that these properties are silently
ignored.

> Can we at least put it straight into the deprecation document?
> 

Yes we can initiate a formal deprecation. This would allow us
to drop the property in QEMU 6.2. We usually also print out
a warning so that users are aware of the upcoming removal.

I'll look into it.

> > The associated nr_ends field cannot be dropped from SpaprXive
> > because it is explicitly used by vmstate_spapr_xive(). It is
> > thus renamed to nr_ends_vmstate.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  include/hw/ppc/spapr_xive.h | 16 +++++++++++++++-
> >  hw/intc/spapr_xive.c        | 28 ++++++++++++++++++++++------
> >  hw/ppc/spapr_irq.c          |  6 +-----
> >  3 files changed, 38 insertions(+), 12 deletions(-)
> > 
> > diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
> > index 4b967f13c030..7123ea07ed78 100644
> > --- a/include/hw/ppc/spapr_xive.h
> > +++ b/include/hw/ppc/spapr_xive.h
> > @@ -23,6 +23,16 @@
> >  typedef struct SpaprXive {
> >      XiveRouter    parent;
> >  
> > +    /*
> > +     * The XIVE device needs to know the highest vCPU id it might
> > +     * be exposed to in order to size the END table. It may also
> > +     * propagate the value to the KVM XIVE device in order to
> > +     * optimize resource allocation in the HW.
> > +     * This must be set to a non-null value using the "nr-servers"
> > +     * property, before realizing the device.
> > +     */
> > +    uint32_t      nr_servers;
> > +
> >      /* Internal interrupt source for IPIs and virtual devices */
> >      XiveSource    source;
> >      hwaddr        vc_base;
> > @@ -38,7 +48,11 @@ typedef struct SpaprXive {
> >      XiveEAS       *eat;
> >      uint32_t      nr_irqs;
> >      XiveEND       *endt;
> > -    uint32_t      nr_ends;
> > +    /*
> > +     * This is derived from nr_servers but it must be kept around because
> > +     * vmstate_spapr_xive uses it.
> > +     */
> > +    uint32_t      nr_ends_vmstate;
> >  
> >      /* TIMA mapping address */
> >      hwaddr        tm_base;
> > diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> > index f473ad9cba47..e4dbf9c2c409 100644
> > --- a/hw/intc/spapr_xive.c
> > +++ b/hw/intc/spapr_xive.c
> > @@ -99,6 +99,12 @@ int spapr_xive_end_to_target(uint8_t end_blk, uint32_t 
> > end_idx,
> >      return 0;
> >  }
> >  
> > +/*
> > + * 8 XIVE END structures per CPU. One for each available
> > + * priority
> > + */
> > +#define spapr_xive_cpu_end_idx(vcpu, prio) (((vcpu) << 3) + prio)
> > +
> >  static void spapr_xive_cpu_to_end(PowerPCCPU *cpu, uint8_t prio,
> >                                    uint8_t *out_end_blk, uint32_t 
> > *out_end_idx)
> >  {
> > @@ -109,7 +115,7 @@ static void spapr_xive_cpu_to_end(PowerPCCPU *cpu, 
> > uint8_t prio,
> >      }
> >  
> >      if (out_end_idx) {
> > -        *out_end_idx = (cpu->vcpu_id << 3) + prio;
> > +        *out_end_idx = spapr_xive_cpu_end_idx(cpu->vcpu_id, prio);
> >      }
> >  }
> >  
> > @@ -290,7 +296,8 @@ static void spapr_xive_instance_init(Object *obj)
> >  
> >  uint32_t spapr_xive_nr_ends(const SpaprXive *xive)
> >  {
> > -    return xive->nr_ends;
> > +    g_assert(xive->nr_servers);
> > +    return spapr_xive_cpu_end_idx(xive->nr_servers, 0);
> >  }
> >  
> >  static void spapr_xive_realize(DeviceState *dev, Error **errp)
> > @@ -303,7 +310,7 @@ static void spapr_xive_realize(DeviceState *dev, Error 
> > **errp)
> >  
> >      /* Set by spapr_irq_init() */
> >      g_assert(xive->nr_irqs);
> > -    g_assert(xive->nr_ends);
> > +    g_assert(xive->nr_servers);
> >  
> >      sxc->parent_realize(dev, &local_err);
> >      if (local_err) {
> > @@ -360,6 +367,8 @@ static void spapr_xive_realize(DeviceState *dev, Error 
> > **errp)
> >      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);
> > +
> > +    xive->nr_ends_vmstate = spapr_xive_nr_ends(xive);
> >  }
> >  
> >  static int spapr_xive_get_eas(XiveRouter *xrtr, uint8_t eas_blk,
> > @@ -547,7 +556,7 @@ static const VMStateDescription vmstate_spapr_xive = {
> >          VMSTATE_UINT32_EQUAL(nr_irqs, SpaprXive, NULL),
> >          VMSTATE_STRUCT_VARRAY_POINTER_UINT32(eat, SpaprXive, nr_irqs,
> >                                       vmstate_spapr_xive_eas, XiveEAS),
> > -        VMSTATE_STRUCT_VARRAY_POINTER_UINT32(endt, SpaprXive, nr_ends,
> > +        VMSTATE_STRUCT_VARRAY_POINTER_UINT32(endt, SpaprXive, 
> > nr_ends_vmstate,
> >                                               vmstate_spapr_xive_end, 
> > XiveEND),
> >          VMSTATE_END_OF_LIST()
> >      },
> > @@ -591,7 +600,14 @@ static void 
> > spapr_xive_free_irq(SpaprInterruptController *intc, int lisn)
> >  
> >  static Property spapr_xive_properties[] = {
> >      DEFINE_PROP_UINT32("nr-irqs", SpaprXive, nr_irqs, 0),
> > -    DEFINE_PROP_UINT32("nr-ends", SpaprXive, nr_ends, 0),
> > +    /*
> > +     * "nr-ends" is deprecated by "nr-servers" since QEMU 6.0.
> > +     * It is just kept around because it is exposed to the user
> > +     * through -global and we don't want it to fail, even if
> > +     * the value is actually overridden internally.
> > +     */
> > +    DEFINE_PROP_UINT32("nr-ends", SpaprXive, nr_ends_vmstate, 0),
> > +    DEFINE_PROP_UINT32("nr-servers", SpaprXive, nr_servers, 0),
> >      DEFINE_PROP_UINT64("vc-base", SpaprXive, vc_base, SPAPR_XIVE_VC_BASE),
> >      DEFINE_PROP_UINT64("tm-base", SpaprXive, tm_base, SPAPR_XIVE_TM_BASE),
> >      DEFINE_PROP_UINT8("hv-prio", SpaprXive, hv_prio, 7),
> > @@ -742,7 +758,7 @@ static int spapr_xive_activate(SpaprInterruptController 
> > *intc,
> >      SpaprXive *xive = SPAPR_XIVE(intc);
> >  
> >      if (kvm_enabled()) {
> > -        int rc = spapr_irq_init_kvm(kvmppc_xive_connect, intc, nr_servers,
> > +        int rc = spapr_irq_init_kvm(kvmppc_xive_connect, intc, 
> > xive->nr_servers,
> 
> Hmm.  So we're now ignoring the 'nr_servers' parameter to this
> function, which doesn't seem right.  Should we be assert()ing that
> it's equal to xive->nr_servers?
> 
> >                                      errp);
> >          if (rc < 0) {
> >              return rc;
> > diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> > index f59960339ec3..8c5627225636 100644
> > --- a/hw/ppc/spapr_irq.c
> > +++ b/hw/ppc/spapr_irq.c
> > @@ -330,11 +330,7 @@ void spapr_irq_init(SpaprMachineState *spapr, Error 
> > **errp)
> >  
> >          dev = qdev_new(TYPE_SPAPR_XIVE);
> >          qdev_prop_set_uint32(dev, "nr-irqs", smc->nr_xirqs + 
> > SPAPR_XIRQ_BASE);
> > -        /*
> > -         * 8 XIVE END structures per CPU. One for each available
> > -         * priority
> > -         */
> > -        qdev_prop_set_uint32(dev, "nr-ends", nr_servers << 3);
> > +        qdev_prop_set_uint32(dev, "nr-servers", nr_servers);
> >          object_property_set_link(OBJECT(dev), "xive-fabric", OBJECT(spapr),
> >                                   &error_abort);
> >          sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>

pgpI5bllUdmhk.pgp
Description: OpenPGP digital signature