From: Greg Kurz <groug@kaod.org>
To: David Gibson <david@gibson.dropbear.id.au>
Cc: qemu-ppc@nongnu.org, qemu-devel@nongnu.org,
"Cédric Le Goater" <clg@kaod.org>
Subject: Re: [PATCH for-6.0 v2 1/3] spapr: Improve naming of some vCPU id related items
Date: Thu, 3 Dec 2020 10:06:53 +0100 [thread overview]
Message-ID: <20201203100653.1c722b27@bahia.lan> (raw)
In-Reply-To: <20201202045616.GC7801@yekko.fritz.box>
[-- Attachment #1: Type: text/plain, Size: 16880 bytes --]
On Wed, 2 Dec 2020 15:56:16 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:
> On Mon, Nov 30, 2020 at 05:52:56PM +0100, Greg Kurz wrote:
> > The machine tells the IC backend the number of vCPU ids it will be
> > exposed to, in order to:
> > - fill the "ibm,interrupt-server-ranges" property in the DT (XICS)
> > - size the VP block used by the in-kernel chip (XICS-on-XIVE, XIVE)
> >
> > The current "nr_servers" and "spapr_max_server_number" naming can
> > mislead people info thinking it is about a quantity of CPUs. Make
> > it clear this is all about vCPU ids.
> >
> > Signed-off-by: Greg Kurz <groug@kaod.org>
>
> I know it seems very finicky, but can you please
> s/max_vcpu_ids/max_vcpu_id/g
>
> At least to be "max_vcpu_ids" has some of the same confusion as the
> existing code - it reads like the maximum *number* of IDs, rather than
> the maximum *value* of IDs.
>
Well... this is precisely the point. Finding a suitable name for
something that is essentially the "size of a range of consecutive
IDs starting from 0" and not the "maximum value of IDs". Not sure
of the more appropriate wording to describe this is a _least upper
bound_ actually.
> > ---
> > include/hw/ppc/spapr.h | 2 +-
> > include/hw/ppc/spapr_irq.h | 8 ++++----
> > include/hw/ppc/spapr_xive.h | 2 +-
> > include/hw/ppc/xics_spapr.h | 2 +-
> > hw/intc/spapr_xive.c | 8 ++++----
> > hw/intc/spapr_xive_kvm.c | 4 ++--
> > hw/intc/xics_kvm.c | 4 ++--
> > hw/intc/xics_spapr.c | 8 ++++----
> > hw/ppc/spapr.c | 8 ++++----
> > hw/ppc/spapr_irq.c | 18 +++++++++---------
> > 10 files changed, 32 insertions(+), 32 deletions(-)
> >
> > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > index b7ced9faebf5..dc99d45e2852 100644
> > --- a/include/hw/ppc/spapr.h
> > +++ b/include/hw/ppc/spapr.h
> > @@ -849,7 +849,7 @@ int spapr_hpt_shift_for_ramsize(uint64_t ramsize);
> > int spapr_reallocate_hpt(SpaprMachineState *spapr, int shift, Error **errp);
> > void spapr_clear_pending_events(SpaprMachineState *spapr);
> > void spapr_clear_pending_hotplug_events(SpaprMachineState *spapr);
> > -int spapr_max_server_number(SpaprMachineState *spapr);
> > +int spapr_max_vcpu_ids(SpaprMachineState *spapr);
> > void spapr_store_hpte(PowerPCCPU *cpu, hwaddr ptex,
> > uint64_t pte0, uint64_t pte1);
> > void spapr_mce_req_event(PowerPCCPU *cpu, bool recovered);
> > diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
> > index c22a72c9e270..2e53fc9e6cbb 100644
> > --- a/include/hw/ppc/spapr_irq.h
> > +++ b/include/hw/ppc/spapr_irq.h
> > @@ -43,7 +43,7 @@ DECLARE_CLASS_CHECKERS(SpaprInterruptControllerClass, SPAPR_INTC,
> > struct SpaprInterruptControllerClass {
> > InterfaceClass parent;
> >
> > - int (*activate)(SpaprInterruptController *intc, uint32_t nr_servers,
> > + int (*activate)(SpaprInterruptController *intc, uint32_t max_vcpu_ids,
> > Error **errp);
> > void (*deactivate)(SpaprInterruptController *intc);
> >
> > @@ -62,7 +62,7 @@ struct SpaprInterruptControllerClass {
> > /* These methods should only be called on the active intc */
> > void (*set_irq)(SpaprInterruptController *intc, int irq, int val);
> > void (*print_info)(SpaprInterruptController *intc, Monitor *mon);
> > - void (*dt)(SpaprInterruptController *intc, uint32_t nr_servers,
> > + void (*dt)(SpaprInterruptController *intc, uint32_t max_vcpu_ids,
> > void *fdt, uint32_t phandle);
> > int (*post_load)(SpaprInterruptController *intc, int version_id);
> > };
> > @@ -74,7 +74,7 @@ int spapr_irq_cpu_intc_create(struct SpaprMachineState *spapr,
> > void spapr_irq_cpu_intc_reset(struct SpaprMachineState *spapr, PowerPCCPU *cpu);
> > void spapr_irq_cpu_intc_destroy(struct SpaprMachineState *spapr, PowerPCCPU *cpu);
> > void spapr_irq_print_info(struct SpaprMachineState *spapr, Monitor *mon);
> > -void spapr_irq_dt(struct SpaprMachineState *spapr, uint32_t nr_servers,
> > +void spapr_irq_dt(struct SpaprMachineState *spapr, uint32_t max_vcpu_ids,
> > void *fdt, uint32_t phandle);
> >
> > uint32_t spapr_irq_nr_msis(struct SpaprMachineState *spapr);
> > @@ -105,7 +105,7 @@ typedef int (*SpaprInterruptControllerInitKvm)(SpaprInterruptController *,
> >
> > int spapr_irq_init_kvm(SpaprInterruptControllerInitKvm fn,
> > SpaprInterruptController *intc,
> > - uint32_t nr_servers,
> > + uint32_t max_vcpu_ids,
> > Error **errp);
> >
> > /*
> > diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
> > index 26c8d90d7196..643129b13536 100644
> > --- a/include/hw/ppc/spapr_xive.h
> > +++ b/include/hw/ppc/spapr_xive.h
> > @@ -79,7 +79,7 @@ int spapr_xive_end_to_target(uint8_t end_blk, uint32_t end_idx,
> > /*
> > * KVM XIVE device helpers
> > */
> > -int kvmppc_xive_connect(SpaprInterruptController *intc, uint32_t nr_servers,
> > +int kvmppc_xive_connect(SpaprInterruptController *intc, uint32_t max_vcpu_ids,
> > Error **errp);
> > void kvmppc_xive_disconnect(SpaprInterruptController *intc);
> > void kvmppc_xive_reset(SpaprXive *xive, Error **errp);
> > diff --git a/include/hw/ppc/xics_spapr.h b/include/hw/ppc/xics_spapr.h
> > index de752c0d2c7e..5c0e9430a964 100644
> > --- a/include/hw/ppc/xics_spapr.h
> > +++ b/include/hw/ppc/xics_spapr.h
> > @@ -35,7 +35,7 @@
> > DECLARE_INSTANCE_CHECKER(ICSState, ICS_SPAPR,
> > TYPE_ICS_SPAPR)
> >
> > -int xics_kvm_connect(SpaprInterruptController *intc, uint32_t nr_servers,
> > +int xics_kvm_connect(SpaprInterruptController *intc, uint32_t max_vcpu_ids,
> > Error **errp);
> > void xics_kvm_disconnect(SpaprInterruptController *intc);
> > bool xics_kvm_has_broken_disconnect(void);
> > diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> > index 12dd6d3ce357..d0a0ca822367 100644
> > --- a/hw/intc/spapr_xive.c
> > +++ b/hw/intc/spapr_xive.c
> > @@ -669,7 +669,7 @@ static void spapr_xive_print_info(SpaprInterruptController *intc, Monitor *mon)
> > spapr_xive_pic_print_info(xive, mon);
> > }
> >
> > -static void spapr_xive_dt(SpaprInterruptController *intc, uint32_t nr_servers,
> > +static void spapr_xive_dt(SpaprInterruptController *intc, uint32_t max_vcpu_ids,
> > void *fdt, uint32_t phandle)
> > {
> > SpaprXive *xive = SPAPR_XIVE(intc);
> > @@ -678,7 +678,7 @@ static void spapr_xive_dt(SpaprInterruptController *intc, uint32_t nr_servers,
> > /* Interrupt number ranges for the IPIs */
> > uint32_t lisn_ranges[] = {
> > cpu_to_be32(SPAPR_IRQ_IPI),
> > - cpu_to_be32(SPAPR_IRQ_IPI + nr_servers),
> > + cpu_to_be32(SPAPR_IRQ_IPI + max_vcpu_ids),
> > };
> > /*
> > * EQ size - the sizes of pages supported by the system 4K, 64K,
> > @@ -733,12 +733,12 @@ static void spapr_xive_dt(SpaprInterruptController *intc, uint32_t nr_servers,
> > }
> >
> > static int spapr_xive_activate(SpaprInterruptController *intc,
> > - uint32_t nr_servers, Error **errp)
> > + uint32_t max_vcpu_ids, Error **errp)
> > {
> > 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, max_vcpu_ids,
> > errp);
> > if (rc < 0) {
> > return rc;
> > diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
> > index e8667ce5f621..2a938b4429a8 100644
> > --- a/hw/intc/spapr_xive_kvm.c
> > +++ b/hw/intc/spapr_xive_kvm.c
> > @@ -716,7 +716,7 @@ static void *kvmppc_xive_mmap(SpaprXive *xive, int pgoff, size_t len,
> > * All the XIVE memory regions are now backed by mappings from the KVM
> > * XIVE device.
> > */
> > -int kvmppc_xive_connect(SpaprInterruptController *intc, uint32_t nr_servers,
> > +int kvmppc_xive_connect(SpaprInterruptController *intc, uint32_t max_vcpu_ids,
> > Error **errp)
> > {
> > SpaprXive *xive = SPAPR_XIVE(intc);
> > @@ -753,7 +753,7 @@ int kvmppc_xive_connect(SpaprInterruptController *intc, uint32_t nr_servers,
> > if (kvm_device_check_attr(xive->fd, KVM_DEV_XIVE_GRP_CTRL,
> > KVM_DEV_XIVE_NR_SERVERS)) {
> > ret = kvm_device_access(xive->fd, KVM_DEV_XIVE_GRP_CTRL,
> > - KVM_DEV_XIVE_NR_SERVERS, &nr_servers, true,
> > + KVM_DEV_XIVE_NR_SERVERS, &max_vcpu_ids, true,
> > errp);
> > if (ret < 0) {
> > goto fail;
> > diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> > index 570d635bcc08..74e47752185c 100644
> > --- a/hw/intc/xics_kvm.c
> > +++ b/hw/intc/xics_kvm.c
> > @@ -347,7 +347,7 @@ void ics_kvm_set_irq(ICSState *ics, int srcno, int val)
> > }
> > }
> >
> > -int xics_kvm_connect(SpaprInterruptController *intc, uint32_t nr_servers,
> > +int xics_kvm_connect(SpaprInterruptController *intc, uint32_t max_vcpu_ids,
> > Error **errp)
> > {
> > ICSState *ics = ICS_SPAPR(intc);
> > @@ -408,7 +408,7 @@ int xics_kvm_connect(SpaprInterruptController *intc, uint32_t nr_servers,
> > if (kvm_device_check_attr(rc, KVM_DEV_XICS_GRP_CTRL,
> > KVM_DEV_XICS_NR_SERVERS)) {
> > if (kvm_device_access(rc, KVM_DEV_XICS_GRP_CTRL,
> > - KVM_DEV_XICS_NR_SERVERS, &nr_servers, true,
> > + KVM_DEV_XICS_NR_SERVERS, &max_vcpu_ids, true,
> > &local_err)) {
> > goto fail;
> > }
> > diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
> > index 8ae4f41459c3..8f753a858cc2 100644
> > --- a/hw/intc/xics_spapr.c
> > +++ b/hw/intc/xics_spapr.c
> > @@ -308,11 +308,11 @@ static void ics_spapr_realize(DeviceState *dev, Error **errp)
> > spapr_register_hypercall(H_IPOLL, h_ipoll);
> > }
> >
> > -static void xics_spapr_dt(SpaprInterruptController *intc, uint32_t nr_servers,
> > +static void xics_spapr_dt(SpaprInterruptController *intc, uint32_t max_vcpu_ids,
> > void *fdt, uint32_t phandle)
> > {
> > uint32_t interrupt_server_ranges_prop[] = {
> > - 0, cpu_to_be32(nr_servers),
> > + 0, cpu_to_be32(max_vcpu_ids),
> > };
> > int node;
> >
> > @@ -423,10 +423,10 @@ static int xics_spapr_post_load(SpaprInterruptController *intc, int version_id)
> > }
> >
> > static int xics_spapr_activate(SpaprInterruptController *intc,
> > - uint32_t nr_servers, Error **errp)
> > + uint32_t max_vcpu_ids, 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, max_vcpu_ids, errp);
> > }
> > return 0;
> > }
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 7e954bc84bed..ab59bfe941d0 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -161,7 +161,7 @@ static void pre_2_10_vmstate_unregister_dummy_icp(int i)
> > (void *)(uintptr_t) i);
> > }
> >
> > -int spapr_max_server_number(SpaprMachineState *spapr)
> > +int spapr_max_vcpu_ids(SpaprMachineState *spapr)
> > {
> > MachineState *ms = MACHINE(spapr);
> >
> > @@ -1164,7 +1164,7 @@ void *spapr_build_fdt(SpaprMachineState *spapr, bool reset, size_t space)
> > _FDT(fdt_setprop_cell(fdt, 0, "#size-cells", 2));
> >
> > /* /interrupt controller */
> > - spapr_irq_dt(spapr, spapr_max_server_number(spapr), fdt, PHANDLE_INTC);
> > + spapr_irq_dt(spapr, spapr_max_vcpu_ids(spapr), fdt, PHANDLE_INTC);
> >
> > ret = spapr_dt_memory(spapr, fdt);
> > if (ret < 0) {
> > @@ -2558,7 +2558,7 @@ static void spapr_init_cpus(SpaprMachineState *spapr)
> > if (smc->pre_2_10_has_unused_icps) {
> > int i;
> >
> > - for (i = 0; i < spapr_max_server_number(spapr); i++) {
> > + for (i = 0; i < spapr_max_vcpu_ids(spapr); i++) {
> > /* Dummy entries get deregistered when real ICPState objects
> > * are registered during CPU core hotplug.
> > */
> > @@ -2709,7 +2709,7 @@ static void spapr_machine_init(MachineState *machine)
> >
> > /*
> > * VSMT must be set in order to be able to compute VCPU ids, ie to
> > - * call spapr_max_server_number() or spapr_vcpu_id().
> > + * call spapr_max_vcpu_ids() or spapr_vcpu_id().
> > */
> > spapr_set_vsmt_mode(spapr, &error_fatal);
> >
> > diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> > index a0d1e1298e1e..552e30e93036 100644
> > --- a/hw/ppc/spapr_irq.c
> > +++ b/hw/ppc/spapr_irq.c
> > @@ -72,13 +72,13 @@ void spapr_irq_msi_free(SpaprMachineState *spapr, int irq, uint32_t num)
> >
> > int spapr_irq_init_kvm(SpaprInterruptControllerInitKvm fn,
> > SpaprInterruptController *intc,
> > - uint32_t nr_servers,
> > + uint32_t max_vcpu_ids,
> > Error **errp)
> > {
> > Error *local_err = NULL;
> >
> > if (kvm_enabled() && kvm_kernel_irqchip_allowed()) {
> > - if (fn(intc, nr_servers, &local_err) < 0) {
> > + if (fn(intc, max_vcpu_ids, &local_err) < 0) {
> > if (kvm_kernel_irqchip_required()) {
> > error_prepend(&local_err,
> > "kernel_irqchip requested but unavailable: ");
> > @@ -271,13 +271,13 @@ void spapr_irq_print_info(SpaprMachineState *spapr, Monitor *mon)
> > sicc->print_info(spapr->active_intc, mon);
> > }
> >
> > -void spapr_irq_dt(SpaprMachineState *spapr, uint32_t nr_servers,
> > +void spapr_irq_dt(SpaprMachineState *spapr, uint32_t max_vcpu_ids,
> > void *fdt, uint32_t phandle)
> > {
> > SpaprInterruptControllerClass *sicc
> > = SPAPR_INTC_GET_CLASS(spapr->active_intc);
> >
> > - sicc->dt(spapr->active_intc, nr_servers, fdt, phandle);
> > + sicc->dt(spapr->active_intc, max_vcpu_ids, fdt, phandle);
> > }
> >
> > uint32_t spapr_irq_nr_msis(SpaprMachineState *spapr)
> > @@ -324,7 +324,7 @@ void spapr_irq_init(SpaprMachineState *spapr, Error **errp)
> > }
> >
> > if (spapr->irq->xive) {
> > - uint32_t nr_servers = spapr_max_server_number(spapr);
> > + uint32_t max_vcpu_ids = spapr_max_vcpu_ids(spapr);
> > DeviceState *dev;
> > int i;
> >
> > @@ -334,7 +334,7 @@ void spapr_irq_init(SpaprMachineState *spapr, Error **errp)
> > * 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-ends", max_vcpu_ids << 3);
> > object_property_set_link(OBJECT(dev), "xive-fabric", OBJECT(spapr),
> > &error_abort);
> > sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> > @@ -342,7 +342,7 @@ void spapr_irq_init(SpaprMachineState *spapr, Error **errp)
> > spapr->xive = SPAPR_XIVE(dev);
> >
> > /* Enable the CPU IPIs */
> > - for (i = 0; i < nr_servers; ++i) {
> > + for (i = 0; i < max_vcpu_ids; ++i) {
> > SpaprInterruptControllerClass *sicc
> > = SPAPR_INTC_GET_CLASS(spapr->xive);
> >
> > @@ -479,7 +479,7 @@ static void set_active_intc(SpaprMachineState *spapr,
> > SpaprInterruptController *new_intc)
> > {
> > SpaprInterruptControllerClass *sicc;
> > - uint32_t nr_servers = spapr_max_server_number(spapr);
> > + uint32_t max_vcpu_ids = spapr_max_vcpu_ids(spapr);
> >
> > assert(new_intc);
> >
> > @@ -497,7 +497,7 @@ static void set_active_intc(SpaprMachineState *spapr,
> >
> > sicc = SPAPR_INTC_GET_CLASS(new_intc);
> > if (sicc->activate) {
> > - sicc->activate(new_intc, nr_servers, &error_fatal);
> > + sicc->activate(new_intc, max_vcpu_ids, &error_fatal);
> > }
> >
> > spapr->active_intc = new_intc;
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next prev parent reply other threads:[~2020-12-03 9:08 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-30 16:52 [PATCH for-6.0 v2 0/3] spapr: Address the confusion between IPI numbers and vCPU ids Greg Kurz
2020-11-30 16:52 ` [PATCH for-6.0 v2 1/3] spapr: Improve naming of some vCPU id related items Greg Kurz
2020-11-30 17:32 ` Cédric Le Goater
2020-11-30 18:08 ` Cédric Le Goater
2020-11-30 18:30 ` Greg Kurz
2020-12-02 4:56 ` David Gibson
2020-12-03 9:06 ` Greg Kurz [this message]
2020-11-30 16:52 ` [PATCH for-6.0 v2 2/3] spapr/xive: Fix size of END table and number of claimed IPIs Greg Kurz
2020-11-30 18:07 ` Cédric Le Goater
2020-12-03 9:51 ` Cédric Le Goater
2020-12-03 10:50 ` Greg Kurz
2020-12-04 8:46 ` Cédric Le Goater
2020-12-04 9:11 ` Greg Kurz
2020-11-30 16:52 ` [PATCH for-6.0 v2 3/3] spapr/xive: Fix the "ibm, xive-lisn-ranges" property Greg Kurz
2020-11-30 17:28 ` [PATCH for-6.0 v2 0/3] spapr: Address the confusion between IPI numbers and vCPU ids Cédric Le Goater
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=20201203100653.1c722b27@bahia.lan \
--to=groug@kaod.org \
--cc=clg@kaod.org \
--cc=david@gibson.dropbear.id.au \
--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).