On Mon, 23 Nov 2020 14:52:14 +1100 David Gibson <da...@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 <gr...@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);
>
pgp_BhHXUWf7W.pgp
Description: OpenPGP digital signature
