On Mon, 23 Nov 2020 15:18:09 +1100 David Gibson <da...@gibson.dropbear.id.au> wrote:
> On Fri, Nov 20, 2020 at 06:46:44PM +0100, Greg Kurz wrote:
> > The sPAPR ICS device exposes the range of vCPU ids it can handle in
> > the "ibm,interrupt-server-ranges" FDT property. The highest vCPU
> > id, ie. spapr_max_server_number(), is obtained from the machine
> > through the "nr_servers" argument of the generic spapr_irq_dt() call.
> >
> > We want to drop the "nr_servers" argument from the API because it
> > doesn't make sense for the sPAPR XIVE device actually.
> >
> > On POWER9, we also pass the highest vCPU id to the KVM XICS-on-XIVE
> > device, 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.
> >
> > Signed-off-by: Greg Kurz <gr...@kaod.org>
> > ---
> > include/hw/ppc/spapr.h | 4 ++--
> > include/hw/ppc/xics_spapr.h | 21 +++++++++++++++++---
> > hw/intc/xics_kvm.c | 2 +-
> > hw/intc/xics_spapr.c | 38 ++++++++++++++++++++++++-------------
> > hw/ppc/spapr.c | 5 +++--
> > hw/ppc/spapr_irq.c | 7 +++++--
> > 6 files changed, 54 insertions(+), 23 deletions(-)
> >
> > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > index 2e89e36cfbdc..b76c84a2f884 100644
> > --- a/include/hw/ppc/spapr.h
> > +++ b/include/hw/ppc/spapr.h
> > @@ -10,7 +10,7 @@
> > #include "hw/ppc/spapr_irq.h"
> > #include "qom/object.h"
> > #include "hw/ppc/spapr_xive.h" /* For SpaprXive */
> > -#include "hw/ppc/xics.h" /* For ICSState */
> > +#include "hw/ppc/xics_spapr.h" /* For IcsSpaprState */
> > #include "hw/ppc/spapr_tpm_proxy.h"
> >
> > struct SpaprVioBus;
> > @@ -230,7 +230,7 @@ struct SpaprMachineState {
> > SpaprIrq *irq;
> > qemu_irq *qirqs;
> > SpaprInterruptController *active_intc;
> > - ICSState *ics;
> > + IcsSpaprState *ics;
> > SpaprXive *xive;
> >
> > bool cmd_line_caps[SPAPR_CAP_NUM];
> > diff --git a/include/hw/ppc/xics_spapr.h b/include/hw/ppc/xics_spapr.h
> > index de752c0d2c7e..1a483a873b62 100644
> > --- a/include/hw/ppc/xics_spapr.h
> > +++ b/include/hw/ppc/xics_spapr.h
> > @@ -28,12 +28,27 @@
> > #define XICS_SPAPR_H
> >
> > #include "hw/ppc/spapr.h"
> > +#include "hw/ppc/xics.h"
> > #include "qom/object.h"
> >
> > +typedef struct IcsSpaprState {
> > + /*< private >*/
> > + ICPState parent_obj;
>
> It's called "*Ics*SpaprState" and it's replacing an ICSState, but it's
> parent object is an *ICP*State - that doesn't seem right.
>
Oops, typo :-\
> > +
> > + /*
> > + * The ICS needs to know the upper limit to vCPU ids it
> > + * might be exposed to in order to size the vCPU id range
> > + * in "ibm,interrupt-server-ranges" and to optimize HW
> > + * resource allocation when using the XICS-on-XIVE KVM
> > + * device. It is the purpose of the "nr-servers" property
> > + * which *must* be set to a non-null value before realizing
> > + * the ICS.
> > + */
> > + uint32_t nr_servers;
>
> That said, I'm a but dubious about attaching the number of servers to
> the ICS side, rather than the ICP side, since server numbers is
> basically an ICP concept. In fact... things about the overall
> topology are usually done via the XicsFabric, so I'm wondering if we
> should add a callback there for nr_servers.
>
I agree this would be the way to go. I sent a patch to do so a year ago
but it didn't reach consensus at the time:
http://patchwork.ozlabs.org/project/qemu-devel/patch/157010405465.246126.7760334967989385566.st...@bahia.lan/
I'll revisit the approach.
> > +} IcsSpaprState;
> > +
> > #define TYPE_ICS_SPAPR "ics-spapr"
> > -/* This is reusing the ICSState typedef from TYPE_ICS */
> > -DECLARE_INSTANCE_CHECKER(ICSState, ICS_SPAPR,
> > - TYPE_ICS_SPAPR)
> > +DECLARE_INSTANCE_CHECKER(IcsSpaprState, ICS_SPAPR, TYPE_ICS_SPAPR)
> >
> > int xics_kvm_connect(SpaprInterruptController *intc, uint32_t nr_servers,
> > Error **errp);
> > diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> > index 570d635bcc08..ecbbda0e249b 100644
> > --- a/hw/intc/xics_kvm.c
> > +++ b/hw/intc/xics_kvm.c
> > @@ -350,7 +350,7 @@ void ics_kvm_set_irq(ICSState *ics, int srcno, int val)
> > int xics_kvm_connect(SpaprInterruptController *intc, uint32_t nr_servers,
> > Error **errp)
> > {
> > - ICSState *ics = ICS_SPAPR(intc);
> > + ICSState *ics = ICS(intc);
> > int rc;
> > CPUState *cs;
> > Error *local_err = NULL;
> > diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
> > index 8ae4f41459c3..ce147e8980ed 100644
> > --- a/hw/intc/xics_spapr.c
> > +++ b/hw/intc/xics_spapr.c
> > @@ -34,6 +34,7 @@
> > #include "hw/ppc/xics.h"
> > #include "hw/ppc/xics_spapr.h"
> > #include "hw/ppc/fdt.h"
> > +#include "hw/qdev-properties.h"
> > #include "qapi/visitor.h"
> >
> > /*
> > @@ -154,7 +155,7 @@ static void rtas_set_xive(PowerPCCPU *cpu,
> > SpaprMachineState *spapr,
> > uint32_t nargs, target_ulong args,
> > uint32_t nret, target_ulong rets)
> > {
> > - ICSState *ics = spapr->ics;
> > + ICSState *ics = ICS(spapr->ics);
> > uint32_t nr, srcno, server, priority;
> >
> > CHECK_EMULATED_XICS_RTAS(spapr, rets);
> > @@ -189,7 +190,7 @@ static void rtas_get_xive(PowerPCCPU *cpu,
> > SpaprMachineState *spapr,
> > uint32_t nargs, target_ulong args,
> > uint32_t nret, target_ulong rets)
> > {
> > - ICSState *ics = spapr->ics;
> > + ICSState *ics = ICS(spapr->ics);
> > uint32_t nr, srcno;
> >
> > CHECK_EMULATED_XICS_RTAS(spapr, rets);
> > @@ -221,7 +222,7 @@ static void rtas_int_off(PowerPCCPU *cpu,
> > SpaprMachineState *spapr,
> > uint32_t nargs, target_ulong args,
> > uint32_t nret, target_ulong rets)
> > {
> > - ICSState *ics = spapr->ics;
> > + ICSState *ics = ICS(spapr->ics);
> > uint32_t nr, srcno;
> >
> > CHECK_EMULATED_XICS_RTAS(spapr, rets);
> > @@ -254,7 +255,7 @@ static void rtas_int_on(PowerPCCPU *cpu,
> > SpaprMachineState *spapr,
> > uint32_t nargs, target_ulong args,
> > uint32_t nret, target_ulong rets)
> > {
> > - ICSState *ics = spapr->ics;
> > + ICSState *ics = ICS(spapr->ics);
> > uint32_t nr, srcno;
> >
> > CHECK_EMULATED_XICS_RTAS(spapr, rets);
> > @@ -285,10 +286,13 @@ static void rtas_int_on(PowerPCCPU *cpu,
> > SpaprMachineState *spapr,
> >
> > static void ics_spapr_realize(DeviceState *dev, Error **errp)
> > {
> > - ICSState *ics = ICS_SPAPR(dev);
> > - ICSStateClass *icsc = ICS_GET_CLASS(ics);
> > + IcsSpaprState *sics = ICS_SPAPR(dev);
> > + ICSStateClass *icsc = ICS_GET_CLASS(dev);
> > Error *local_err = NULL;
> >
> > + /* Set by spapr_irq_init() */
> > + g_assert(sics->nr_servers);
> > +
> > icsc->parent_realize(dev, &local_err);
> > if (local_err) {
> > error_propagate(errp, local_err);
> > @@ -312,7 +316,7 @@ static void xics_spapr_dt(SpaprInterruptController
> > *intc, uint32_t nr_servers,
> > void *fdt, uint32_t phandle)
> > {
> > uint32_t interrupt_server_ranges_prop[] = {
> > - 0, cpu_to_be32(nr_servers),
> > + 0, cpu_to_be32(ICS_SPAPR(intc)->nr_servers),
> > };
> > int node;
> >
> > @@ -333,7 +337,7 @@ static void xics_spapr_dt(SpaprInterruptController
> > *intc, uint32_t nr_servers,
> > static int xics_spapr_cpu_intc_create(SpaprInterruptController *intc,
> > PowerPCCPU *cpu, Error **errp)
> > {
> > - ICSState *ics = ICS_SPAPR(intc);
> > + ICSState *ics = ICS(intc);
> > Object *obj;
> > SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
> >
> > @@ -364,7 +368,7 @@ static void
> > xics_spapr_cpu_intc_destroy(SpaprInterruptController *intc,
> > static int xics_spapr_claim_irq(SpaprInterruptController *intc, int irq,
> > bool lsi, Error **errp)
> > {
> > - ICSState *ics = ICS_SPAPR(intc);
> > + ICSState *ics = ICS(intc);
> >
> > assert(ics);
> > assert(ics_valid_irq(ics, irq));
> > @@ -380,7 +384,7 @@ static int
> > xics_spapr_claim_irq(SpaprInterruptController *intc, int irq,
> >
> > static void xics_spapr_free_irq(SpaprInterruptController *intc, int irq)
> > {
> > - ICSState *ics = ICS_SPAPR(intc);
> > + ICSState *ics = ICS(intc);
> > uint32_t srcno = irq - ics->offset;
> >
> > assert(ics_valid_irq(ics, irq));
> > @@ -390,7 +394,7 @@ static void
> > xics_spapr_free_irq(SpaprInterruptController *intc, int irq)
> >
> > static void xics_spapr_set_irq(SpaprInterruptController *intc, int irq,
> > int val)
> > {
> > - ICSState *ics = ICS_SPAPR(intc);
> > + ICSState *ics = ICS(intc);
> > uint32_t srcno = irq - ics->offset;
> >
> > ics_set_irq(ics, srcno, val);
> > @@ -398,7 +402,7 @@ static void xics_spapr_set_irq(SpaprInterruptController
> > *intc, int irq, int val)
> >
> > static void xics_spapr_print_info(SpaprInterruptController *intc, Monitor
> > *mon)
> > {
> > - ICSState *ics = ICS_SPAPR(intc);
> > + ICSState *ics = ICS(intc);
> > CPUState *cs;
> >
> > CPU_FOREACH(cs) {
> > @@ -426,7 +430,8 @@ static int xics_spapr_activate(SpaprInterruptController
> > *intc,
> > uint32_t nr_servers, Error **errp)
> > {
> > if (kvm_enabled()) {
> > - return spapr_irq_init_kvm(xics_kvm_connect, intc, nr_servers,
> > errp);
> > + return spapr_irq_init_kvm(xics_kvm_connect, intc,
> > + ICS_SPAPR(intc)->nr_servers, errp);
> > }
> > return 0;
> > }
> > @@ -438,6 +443,11 @@ static void
> > xics_spapr_deactivate(SpaprInterruptController *intc)
> > }
> > }
> >
> > +static Property ics_spapr_properties[] = {
> > + DEFINE_PROP_UINT32("nr-servers", IcsSpaprState, nr_servers, 0),
> > + DEFINE_PROP_END_OF_LIST(),
> > +};
> > +
> > static void ics_spapr_class_init(ObjectClass *klass, void *data)
> > {
> > DeviceClass *dc = DEVICE_CLASS(klass);
> > @@ -446,6 +456,7 @@ static void ics_spapr_class_init(ObjectClass *klass,
> > void *data)
> >
> > device_class_set_parent_realize(dc, ics_spapr_realize,
> > &isc->parent_realize);
> > + device_class_set_props(dc, ics_spapr_properties);
> > sicc->activate = xics_spapr_activate;
> > sicc->deactivate = xics_spapr_deactivate;
> > sicc->cpu_intc_create = xics_spapr_cpu_intc_create;
> > @@ -462,6 +473,7 @@ static void ics_spapr_class_init(ObjectClass *klass,
> > void *data)
> > static const TypeInfo ics_spapr_info = {
> > .name = TYPE_ICS_SPAPR,
> > .parent = TYPE_ICS,
> > + .instance_size = sizeof(IcsSpaprState),
> > .class_init = ics_spapr_class_init,
> > .interfaces = (InterfaceInfo[]) {
> > { TYPE_SPAPR_INTC },
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 12a012d9dd09..21de0456446b 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -4218,15 +4218,16 @@ static void spapr_phb_placement(SpaprMachineState
> > *spapr, uint32_t index,
> > static ICSState *spapr_ics_get(XICSFabric *dev, int irq)
> > {
> > SpaprMachineState *spapr = SPAPR_MACHINE(dev);
> > + ICSState *ics = ICS(spapr->ics);
> >
> > - return ics_valid_irq(spapr->ics, irq) ? spapr->ics : NULL;
> > + return ics_valid_irq(ics, irq) ? ics : NULL;
> > }
> >
> > static void spapr_ics_resend(XICSFabric *dev)
> > {
> > SpaprMachineState *spapr = SPAPR_MACHINE(dev);
> >
> > - ics_resend(spapr->ics);
> > + ics_resend(ICS(spapr->ics));
> > }
> >
> > static ICPState *spapr_icp_get(XICSFabric *xi, int vcpu_id)
> > diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> > index 2dacbecfd5fd..be6f4041e433 100644
> > --- a/hw/ppc/spapr_irq.c
> > +++ b/hw/ppc/spapr_irq.c
> > @@ -316,6 +316,9 @@ void spapr_irq_init(SpaprMachineState *spapr, Error
> > **errp)
> > object_property_set_link(obj, ICS_PROP_XICS, OBJECT(spapr),
> > &error_abort);
> > object_property_set_int(obj, "nr-irqs", smc->nr_xirqs,
> > &error_abort);
> > + object_property_set_uint(obj, "nr-servers",
> > + spapr_max_server_number(spapr),
> > + &error_abort);
> > if (!qdev_realize(DEVICE(obj), NULL, errp)) {
> > return;
> > }
> > @@ -426,7 +429,7 @@ qemu_irq spapr_qirq(SpaprMachineState *spapr, int irq)
> > assert(irq < (smc->nr_xirqs + SPAPR_XIRQ_BASE));
> >
> > if (spapr->ics) {
> > - assert(ics_valid_irq(spapr->ics, irq));
> > + assert(ics_valid_irq(ICS(spapr->ics), irq));
> > }
> > if (spapr->xive) {
> > assert(irq < spapr->xive->nr_irqs);
> > @@ -556,7 +559,7 @@ static int ics_find_free_block(ICSState *ics, int num,
> > int alignnum)
> >
> > int spapr_irq_find(SpaprMachineState *spapr, int num, bool align, Error
> > **errp)
> > {
> > - ICSState *ics = spapr->ics;
> > + ICSState *ics = ICS(spapr->ics);
> > int first = -1;
> >
> > assert(ics);
>
pgpinzfbs1GER.pgp
Description: OpenPGP digital signature
