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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 7779 bytes --]

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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]