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.
Can we at least put it straight into the deprecation document?
> 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);
--
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
