From: David Gibson <david@gibson.dropbear.id.au>
To: Greg Kurz <groug@kaod.org>
Cc: qemu-ppc@nongnu.org, qemu-devel@nongnu.org,
"Cédric Le Goater" <clg@kaod.org>
Subject: Re: [PATCH for-6.0 6/8] spapr/xics: Add "nr-servers" property
Date: Mon, 23 Nov 2020 15:18:09 +1100 [thread overview]
Message-ID: <20201123041809.GF521467@yekko.fritz.box> (raw)
In-Reply-To: <20201120174646.619395-7-groug@kaod.org>
[-- Attachment #1: Type: text/plain, Size: 12848 bytes --]
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next prev parent reply other threads:[~2020-11-23 4:43 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-20 17:46 [PATCH for-6.0 0/8] spapr: Address the confusion between IPI numbers and vCPU ids Greg Kurz
2020-11-20 17:46 ` [PATCH for-6.0 1/8] spapr/xive: Turn some sanity checks into assertions Greg Kurz
2020-11-23 3:33 ` David Gibson
2020-11-23 8:09 ` Cédric Le Goater
2020-11-20 17:46 ` [PATCH for-6.0 2/8] spapr/xive: Introduce spapr_xive_nr_ends() Greg Kurz
2020-11-23 3:33 ` David Gibson
2020-11-25 22:43 ` Greg Kurz
2020-11-26 0:06 ` David Gibson
2020-11-23 9:46 ` Cédric Le Goater
2020-11-23 11:16 ` Greg Kurz
2020-11-24 13:54 ` Cédric Le Goater
2020-11-24 17:01 ` Greg Kurz
2020-11-24 17:56 ` Cédric Le Goater
2020-11-25 9:33 ` Greg Kurz
2020-11-25 11:34 ` Cédric Le Goater
2020-11-25 12:26 ` Greg Kurz
2020-11-26 7:06 ` Cédric Le Goater
2020-11-20 17:46 ` [PATCH for-6.0 3/8] spapr/xive: Add "nr-servers" property Greg Kurz
2020-11-23 3:52 ` David Gibson
2020-11-23 9:20 ` Greg Kurz
2020-11-23 9:56 ` Cédric Le Goater
2020-11-23 11:25 ` Greg Kurz
2020-11-24 14:18 ` Cédric Le Goater
2020-11-20 17:46 ` [PATCH for-6.0 4/8] spapr/xive: Add "nr-ipis" property Greg Kurz
2020-11-23 4:10 ` David Gibson
2020-11-23 10:13 ` Cédric Le Goater
2020-11-24 17:18 ` Greg Kurz
2020-11-20 17:46 ` [PATCH for-6.0 5/8] spapr/xics: Drop unused argument to xics_kvm_has_broken_disconnect() Greg Kurz
2020-11-23 4:10 ` David Gibson
2020-11-23 10:15 ` Cédric Le Goater
2020-11-20 17:46 ` [PATCH for-6.0 6/8] spapr/xics: Add "nr-servers" property Greg Kurz
2020-11-23 4:18 ` David Gibson [this message]
2020-11-23 9:39 ` Greg Kurz
2020-11-23 10:24 ` Cédric Le Goater
2020-11-20 17:46 ` [PATCH for-6.0 7/8] spapr: Drop "nr_servers" argument of the sPAPR IC activate() operation Greg Kurz
2020-11-23 4:38 ` David Gibson
2020-11-23 9:47 ` Greg Kurz
2020-11-20 17:46 ` [PATCH for-6.0 8/8] spapr: Drop "nr_servers" argument of the sPAPR IC dt() operation Greg Kurz
2020-11-23 8:04 ` [PATCH for-6.0 0/8] spapr: Address the confusion between IPI numbers and vCPU ids Cédric Le Goater
2020-11-23 10:07 ` Greg Kurz
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20201123041809.GF521467@yekko.fritz.box \
--to=david@gibson.dropbear.id.au \
--cc=clg@kaod.org \
--cc=groug@kaod.org \
--cc=qemu-devel@nongnu.org \
--cc=qemu-ppc@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).