[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH for-6.0 6/8] spapr/xics: Add "nr-servers" property
|
From: |
David Gibson |
|
Subject: |
Re: [PATCH for-6.0 6/8] spapr/xics: Add "nr-servers" property |
|
Date: |
Mon, 23 Nov 2020 15:18:09 +1100 |
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 <groug@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.
> +
> + /*
> + * 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.
> +} 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);
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
- Re: [PATCH for-6.0 4/8] spapr/xive: Add "nr-ipis" property, (continued)
- [PATCH for-6.0 1/8] spapr/xive: Turn some sanity checks into assertions, Greg Kurz, 2020/11/20
- [PATCH for-6.0 7/8] spapr: Drop "nr_servers" argument of the sPAPR IC activate() operation, Greg Kurz, 2020/11/20
- [PATCH for-6.0 8/8] spapr: Drop "nr_servers" argument of the sPAPR IC dt() operation, Greg Kurz, 2020/11/20
- [PATCH for-6.0 6/8] spapr/xics: Add "nr-servers" property, Greg Kurz, 2020/11/20
- [PATCH for-6.0 5/8] spapr/xics: Drop unused argument to xics_kvm_has_broken_disconnect(), Greg Kurz, 2020/11/20
- [PATCH for-6.0 3/8] spapr/xive: Add "nr-servers" property, Greg Kurz, 2020/11/20