| Message ID | 20201120174646.619395-4-groug@kaod.org |
|---|---|
| State | New |
| Headers | show |
| Series | spapr: Address the confusion between IPI numbers and vCPU ids | expand |
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);
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); >
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); >
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); > > >
>> 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. We need to clarify the naming. this is confusing. What we care about is the number of vCPUs to compute the width of the IPI range. C.
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);
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(-)