diff mbox series

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

Message ID 20201120174646.619395-4-groug@kaod.org
State New
Headers show
Series spapr: Address the confusion between IPI numbers and vCPU ids | expand

Commit Message

Greg Kurz Nov. 20, 2020, 5:46 p.m. UTC
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.

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

Comments

David Gibson Nov. 23, 2020, 3:52 a.m. UTC | #1
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);
Greg Kurz Nov. 23, 2020, 9:20 a.m. UTC | #2
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);
>
Cédric Le Goater Nov. 23, 2020, 9:56 a.m. UTC | #3
On 11/20/20 6:46 PM, 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().

Yes. This is done at the machine level.

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

Yes. It's deeply buried in the spapr/irq/xive/kvm layers but it is
called by set_active_intc() which is, for me, a machine level 
operation.

The only other place where the number of vCPUs is used is in 
spapr_xive_dt() to define the vCPU IPI range, which done at
the machine level again.

So I don't agree with this patch.

C.


> 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.
> 
> 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,
>                                      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);
>
Greg Kurz Nov. 23, 2020, 11:25 a.m. UTC | #4
On Mon, 23 Nov 2020 10:56:13 +0100
Cédric Le Goater <clg@kaod.org> wrote:

> On 11/20/20 6:46 PM, 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().
> 
> Yes. This is done at the machine level.
> 
> > 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.
> 
> Yes. It's deeply buried in the spapr/irq/xive/kvm layers but it is
> called by set_active_intc() which is, for me, a machine level 
> operation.
> 
> The only other place where the number of vCPUs is used is in 
> spapr_xive_dt() to define the vCPU IPI range, which done at
> the machine level again.
> 

spapr_max_server_number() isn't the number of vCPUs. It is the
number of consecutive vCPU ids the IC might be exposed to,
starting from 0.

> So I don't agree with this patch.
> 

As said in another mail, I'll try a different approach.

> C.
> 
> 
> > 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.
> > 
> > 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,
> >                                      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);
> > 
>
diff mbox series

Patch

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