* [PATCH for-6.0 v2 0/3] spapr: Address the confusion between IPI numbers and vCPU ids
@ 2020-11-30 16:52 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
` (3 more replies)
0 siblings, 4 replies; 15+ messages in thread
From: Greg Kurz @ 2020-11-30 16:52 UTC (permalink / raw)
To: David Gibson; +Cc: qemu-ppc, qemu-devel, Cédric Le Goater
A regression was recently fixed in the sPAPR XIVE code for QEMU 5.2
RC3 [1]. It boiled down to a confusion between IPI numbers and vCPU
ids, which happen to be numerically equal in general, but are really
different entities that can diverge in some setups. This was causing
QEMU to misconfigure XIVE and to crash the guest.
The confusion comes from XICS actually. Interrupt presenters in XICS
are identified by a "server number" which is a 1:1 mapping to vCPU
ids. The range of these "server numbers" is exposed to the guest in
the "ibm,interrupt-server-ranges" property. A xics_max_server_number()
helper was introduced at some point to compute the upper limit of the
range. When XIVE was added, commit 1a518e7693c9 renamed the helper to
spapr_max_server_number(). It ended up being used to size a bunch of
things in XIVE that are per-vCPU, such as internal END tables or
IPI ranges presented to the guest. The problem is that the maximum
"server number" can be much higher (up to 8 times) than the actual
number of vCPUs when the VSMT mode doesn't match the number of threads
per core in the guest:
DIV_ROUND_UP(ms->smp.max_cpus * spapr->vsmt, ms->smp.threads);
Since QEMU 4.2, the default behavior is to set spapr->vsmt to
ms->smp.threads. Setups with custom VSMT settings will configure XIVE
to use more HW resources than needed. This is a bit unfortunate but
not extremely harmful, unless maybe if a lot of guests are running
on the host. The sizing of the IPI range is more problematic though
as it eventually led to [1].
This series first does some renaming to make it clear when we're
dealing with vCPU ids. It then fixes the machine code to pass
smp.max_cpus to XIVE where appropriate. Since these changes are
guest/migration visible, a machine property is added to keep the
existing behavior for older machine types. The series is thus based
on Connie's recent patch that introduces compat machines for
QEMU 6.0.
Based-on: 20201109173928.1001764-1-cohuck@redhat.com
Note that we still use spapr_max_vcpu_ids() when activating the
in-kernel irqchip because this is what both XICS-on-XIVE and XIVE
KVM devices expect.
[1] https://bugs.launchpad.net/qemu/+bug/1900241
v2: - comments on v1 highlighted that problems mostly come from
spapr_max_server_number() which got misused over the years.
Updated the cover letter accordingly.
- completely new approach. Instead of messing with device properties,
pass the appropriate values to the IC backend handlers.
- rename a few things using the "max_vcpu_ids" wording instead of
"nr_servers" and "max_server_number"
Greg Kurz (3):
spapr: Improve naming of some vCPU id related items
spapr/xive: Fix size of END table and number of claimed IPIs
spapr/xive: Fix the "ibm,xive-lisn-ranges" property
include/hw/ppc/spapr.h | 3 ++-
include/hw/ppc/spapr_irq.h | 12 ++++++------
include/hw/ppc/spapr_xive.h | 2 +-
include/hw/ppc/xics_spapr.h | 2 +-
hw/intc/spapr_xive.c | 9 +++++----
hw/intc/spapr_xive_kvm.c | 4 ++--
hw/intc/xics_kvm.c | 4 ++--
hw/intc/xics_spapr.c | 11 ++++++-----
hw/ppc/spapr.c | 12 ++++++++----
hw/ppc/spapr_irq.c | 34 ++++++++++++++++++++++++----------
10 files changed, 57 insertions(+), 36 deletions(-)
--
2.26.2
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH for-6.0 v2 1/3] spapr: Improve naming of some vCPU id related items
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 ` Greg Kurz
2020-11-30 17:32 ` Cédric Le Goater
2020-12-02 4:56 ` David Gibson
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
` (2 subsequent siblings)
3 siblings, 2 replies; 15+ messages in thread
From: Greg Kurz @ 2020-11-30 16:52 UTC (permalink / raw)
To: David Gibson; +Cc: qemu-ppc, qemu-devel, Cédric Le Goater
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>
---
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;
--
2.26.2
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH for-6.0 v2 2/3] spapr/xive: Fix size of END table and number of claimed IPIs
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 16:52 ` Greg Kurz
2020-11-30 18:07 ` Cédric Le Goater
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
3 siblings, 1 reply; 15+ messages in thread
From: Greg Kurz @ 2020-11-30 16:52 UTC (permalink / raw)
To: David Gibson; +Cc: qemu-ppc, qemu-devel, Cédric Le Goater
The sPAPR XIVE device has an internal ENDT table the size of
which is configurable by the machine. This table is supposed
to contain END structures for all possible vCPUs that may
enter the guest. The machine must also claim IPIs for all
possible vCPUs since this is expected by the guest.
spapr_irq_init() takes care of that under the assumption that
spapr_max_vcpu_ids() returns the number of possible vCPUs.
This happens to be the case when the VSMT mode is set to match
the number of threads per core in the guest (default behavior).
With non-default VSMT settings, this limit is > to the number
of vCPUs. In the worst case, we can end up allocating an 8 times
bigger ENDT and claiming 8 times more IPIs than needed. But more
importantly, this creates a confusion between number of vCPUs and
vCPU ids, which can lead to subtle bugs like [1].
Use smp.max_cpus instead of spapr_max_vcpu_ids() in
spapr_irq_init() for the latest machine type. Older machine
types continue to use spapr_max_vcpu_ids() since the size of
the ENDT is migration visible.
[1] https://bugs.launchpad.net/qemu/+bug/1900241
Signed-off-by: Greg Kurz <groug@kaod.org>
---
include/hw/ppc/spapr.h | 1 +
hw/ppc/spapr.c | 3 +++
hw/ppc/spapr_irq.c | 16 +++++++++++++---
3 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index dc99d45e2852..95bf210d0bf6 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -139,6 +139,7 @@ struct SpaprMachineClass {
hwaddr rma_limit; /* clamp the RMA to this size */
bool pre_5_1_assoc_refpoints;
bool pre_5_2_numa_associativity;
+ bool pre_6_0_xive_max_cpus;
bool (*phb_placement)(SpaprMachineState *spapr, uint32_t index,
uint64_t *buid, hwaddr *pio,
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index ab59bfe941d0..227a926dfd48 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -4530,8 +4530,11 @@ DEFINE_SPAPR_MACHINE(6_0, "6.0", true);
*/
static void spapr_machine_5_2_class_options(MachineClass *mc)
{
+ SpaprMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
+
spapr_machine_6_0_class_options(mc);
compat_props_add(mc->compat_props, hw_compat_5_2, hw_compat_5_2_len);
+ smc->pre_6_0_xive_max_cpus = true;
}
DEFINE_SPAPR_MACHINE(5_2, "5.2", false);
diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
index 552e30e93036..4d9ecd5d0af8 100644
--- a/hw/ppc/spapr_irq.c
+++ b/hw/ppc/spapr_irq.c
@@ -324,17 +324,27 @@ void spapr_irq_init(SpaprMachineState *spapr, Error **errp)
}
if (spapr->irq->xive) {
- uint32_t max_vcpu_ids = spapr_max_vcpu_ids(spapr);
+ uint32_t max_cpus = MACHINE(spapr)->smp.max_cpus;
DeviceState *dev;
int i;
+ /*
+ * Older machine types used to size the ENDT and IPI range
+ * according to the upper limit of vCPU ids, which can be
+ * higher than smp.max_cpus with custom VSMT settings. Keep
+ * the previous behavior for compatibility with such setups.
+ */
+ if (smc->pre_6_0_xive_max_cpus) {
+ max_cpus = spapr_max_vcpu_ids(spapr);
+ }
+
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", max_vcpu_ids << 3);
+ qdev_prop_set_uint32(dev, "nr-ends", max_cpus << 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 +352,7 @@ void spapr_irq_init(SpaprMachineState *spapr, Error **errp)
spapr->xive = SPAPR_XIVE(dev);
/* Enable the CPU IPIs */
- for (i = 0; i < max_vcpu_ids; ++i) {
+ for (i = 0; i < max_cpus; ++i) {
SpaprInterruptControllerClass *sicc
= SPAPR_INTC_GET_CLASS(spapr->xive);
--
2.26.2
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH for-6.0 v2 3/3] spapr/xive: Fix the "ibm, xive-lisn-ranges" property
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 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 16:52 ` 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
3 siblings, 0 replies; 15+ messages in thread
From: Greg Kurz @ 2020-11-30 16:52 UTC (permalink / raw)
To: David Gibson; +Cc: qemu-ppc, qemu-devel, Cédric Le Goater
The dt() callback of the sPAPR IC class has a "nr_servers"
argument which is used by both XIVE and XICS to setup the
"interrupt-controller" node in the DT.
The machine currently passes spapr_max_server_number() to
spapr_irq_dt(). This is perfectly fine to populate the range
of vCPU ids in the "ibm,interrupt-server-ranges" property
for XICS. However, this doesn't makes sense for XIVE's
"ibm,xive-lisn-ranges" property which really expects the
maximum number of vCPUs instead.
Add a new "max_cpus" argument to spapr_irq_dt() and the
dt() handler to convey the maximum number of vCPUs. Have
the latest machine type to pass smp.max_cpus and sPAPR XIVE
to use that for "ibm,xive-lisn-ranges". Older machine types
go on with the previous behavior since this is guest visible.
Signed-off-by: Greg Kurz <groug@kaod.org>
---
include/hw/ppc/spapr_irq.h | 4 ++--
hw/intc/spapr_xive.c | 3 ++-
hw/intc/xics_spapr.c | 3 ++-
hw/ppc/spapr.c | 3 ++-
hw/ppc/spapr_irq.c | 8 ++++++--
5 files changed, 14 insertions(+), 7 deletions(-)
diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
index 2e53fc9e6cbb..1edf4851afa4 100644
--- a/include/hw/ppc/spapr_irq.h
+++ b/include/hw/ppc/spapr_irq.h
@@ -63,7 +63,7 @@ struct SpaprInterruptControllerClass {
void (*set_irq)(SpaprInterruptController *intc, int irq, int val);
void (*print_info)(SpaprInterruptController *intc, Monitor *mon);
void (*dt)(SpaprInterruptController *intc, uint32_t max_vcpu_ids,
- void *fdt, uint32_t phandle);
+ uint32_t max_cpus, void *fdt, uint32_t phandle);
int (*post_load)(SpaprInterruptController *intc, int version_id);
};
@@ -75,7 +75,7 @@ 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 max_vcpu_ids,
- void *fdt, uint32_t phandle);
+ uint32_t max_cpus, void *fdt, uint32_t phandle);
uint32_t spapr_irq_nr_msis(struct SpaprMachineState *spapr);
int spapr_irq_msi_alloc(struct SpaprMachineState *spapr, uint32_t num, bool align,
diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
index d0a0ca822367..f9a563cd0a9b 100644
--- a/hw/intc/spapr_xive.c
+++ b/hw/intc/spapr_xive.c
@@ -670,6 +670,7 @@ static void spapr_xive_print_info(SpaprInterruptController *intc, Monitor *mon)
}
static void spapr_xive_dt(SpaprInterruptController *intc, uint32_t max_vcpu_ids,
+ uint32_t max_cpus,
void *fdt, uint32_t phandle)
{
SpaprXive *xive = SPAPR_XIVE(intc);
@@ -678,7 +679,7 @@ static void spapr_xive_dt(SpaprInterruptController *intc, uint32_t max_vcpu_ids,
/* Interrupt number ranges for the IPIs */
uint32_t lisn_ranges[] = {
cpu_to_be32(SPAPR_IRQ_IPI),
- cpu_to_be32(SPAPR_IRQ_IPI + max_vcpu_ids),
+ cpu_to_be32(SPAPR_IRQ_IPI + max_cpus),
};
/*
* EQ size - the sizes of pages supported by the system 4K, 64K,
diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
index 8f753a858cc2..d9f887dfd303 100644
--- a/hw/intc/xics_spapr.c
+++ b/hw/intc/xics_spapr.c
@@ -309,7 +309,8 @@ static void ics_spapr_realize(DeviceState *dev, Error **errp)
}
static void xics_spapr_dt(SpaprInterruptController *intc, uint32_t max_vcpu_ids,
- void *fdt, uint32_t phandle)
+ uint32_t max_cpus, void *fdt,
+ uint32_t phandle)
{
uint32_t interrupt_server_ranges_prop[] = {
0, cpu_to_be32(max_vcpu_ids),
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 227a926dfd48..be3b4b9119b7 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1164,7 +1164,8 @@ 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_vcpu_ids(spapr), fdt, PHANDLE_INTC);
+ spapr_irq_dt(spapr, spapr_max_vcpu_ids(spapr), machine->smp.max_cpus,
+ fdt, PHANDLE_INTC);
ret = spapr_dt_memory(spapr, fdt);
if (ret < 0) {
diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
index 4d9ecd5d0af8..e1fd777aff62 100644
--- a/hw/ppc/spapr_irq.c
+++ b/hw/ppc/spapr_irq.c
@@ -272,12 +272,16 @@ void spapr_irq_print_info(SpaprMachineState *spapr, Monitor *mon)
}
void spapr_irq_dt(SpaprMachineState *spapr, uint32_t max_vcpu_ids,
- void *fdt, uint32_t phandle)
+ uint32_t max_cpus, void *fdt, uint32_t phandle)
{
SpaprInterruptControllerClass *sicc
= SPAPR_INTC_GET_CLASS(spapr->active_intc);
- sicc->dt(spapr->active_intc, max_vcpu_ids, fdt, phandle);
+ /* For older machine types in case they have an unusual VSMT setting */
+ if (SPAPR_MACHINE_GET_CLASS(spapr)->pre_6_0_xive_max_cpus) {
+ max_cpus = spapr_max_vcpu_ids(spapr);
+ }
+ sicc->dt(spapr->active_intc, max_vcpu_ids, max_cpus, fdt, phandle);
}
uint32_t spapr_irq_nr_msis(SpaprMachineState *spapr)
--
2.26.2
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH for-6.0 v2 0/3] spapr: Address the confusion between IPI numbers and vCPU ids
2020-11-30 16:52 [PATCH for-6.0 v2 0/3] spapr: Address the confusion between IPI numbers and vCPU ids Greg Kurz
` (2 preceding siblings ...)
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 ` Cédric Le Goater
3 siblings, 0 replies; 15+ messages in thread
From: Cédric Le Goater @ 2020-11-30 17:28 UTC (permalink / raw)
To: Greg Kurz, David Gibson; +Cc: qemu-ppc, qemu-devel
On 11/30/20 5:52 PM, Greg Kurz wrote:
> A regression was recently fixed in the sPAPR XIVE code for QEMU 5.2
> RC3 [1]. It boiled down to a confusion between IPI numbers and vCPU
> ids, which happen to be numerically equal in general, but are really
> different entities that can diverge in some setups. This was causing
> QEMU to misconfigure XIVE and to crash the guest.
>
> The confusion comes from XICS actually. Interrupt presenters in XICS
> are identified by a "server number" which is a 1:1 mapping to vCPU
> ids. The range of these "server numbers" is exposed to the guest in
> the "ibm,interrupt-server-ranges" property. A xics_max_server_number()
> helper was introduced at some point to compute the upper limit of the
> range. When XIVE was added, commit 1a518e7693c9 renamed the helper to
> spapr_max_server_number(). It ended up being used to size a bunch of
> things in XIVE that are per-vCPU, such as internal END tables or
> IPI ranges presented to the guest. The problem is that the maximum
> "server number" can be much higher (up to 8 times) than the actual
> number of vCPUs when the VSMT mode doesn't match the number of threads
> per core in the guest:
>
> DIV_ROUND_UP(ms->smp.max_cpus * spapr->vsmt, ms->smp.threads);
>
> Since QEMU 4.2, the default behavior is to set spapr->vsmt to
> ms->smp.threads. Setups with custom VSMT settings will configure XIVE
> to use more HW resources than needed. This is a bit unfortunate but
> not extremely harmful,
Indeed. The default usage case (without vsmt) has no impact since
it does not fragment the XIVE VP space more than needed.
> unless maybe if a lot of guests are running on the host.
We can run 4K (-2) KVM guests today on a P9 system. To reach the
internal limits, each should have 32 vCPUs. It's possible with a
lot of RAM but it's not a common scenario.
C.
> The sizing of the IPI range is more problematic though
> as it eventually led to [1].
>
> This series first does some renaming to make it clear when we're
> dealing with vCPU ids. It then fixes the machine code to pass
> smp.max_cpus to XIVE where appropriate. Since these changes are
> guest/migration visible, a machine property is added to keep the
> existing behavior for older machine types. The series is thus based
> on Connie's recent patch that introduces compat machines for
> QEMU 6.0.
>
> Based-on: 20201109173928.1001764-1-cohuck@redhat.com
>
> Note that we still use spapr_max_vcpu_ids() when activating the
> in-kernel irqchip because this is what both XICS-on-XIVE and XIVE
> KVM devices expect.
>
> [1] https://bugs.launchpad.net/qemu/+bug/1900241
>
> v2: - comments on v1 highlighted that problems mostly come from
> spapr_max_server_number() which got misused over the years.
> Updated the cover letter accordingly.
> - completely new approach. Instead of messing with device properties,
> pass the appropriate values to the IC backend handlers.
> - rename a few things using the "max_vcpu_ids" wording instead of
> "nr_servers" and "max_server_number"
>
> Greg Kurz (3):
> spapr: Improve naming of some vCPU id related items
> spapr/xive: Fix size of END table and number of claimed IPIs
> spapr/xive: Fix the "ibm,xive-lisn-ranges" property
>
> include/hw/ppc/spapr.h | 3 ++-
> include/hw/ppc/spapr_irq.h | 12 ++++++------
> include/hw/ppc/spapr_xive.h | 2 +-
> include/hw/ppc/xics_spapr.h | 2 +-
> hw/intc/spapr_xive.c | 9 +++++----
> hw/intc/spapr_xive_kvm.c | 4 ++--
> hw/intc/xics_kvm.c | 4 ++--
> hw/intc/xics_spapr.c | 11 ++++++-----
> hw/ppc/spapr.c | 12 ++++++++----
> hw/ppc/spapr_irq.c | 34 ++++++++++++++++++++++++----------
> 10 files changed, 57 insertions(+), 36 deletions(-)
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH for-6.0 v2 1/3] spapr: Improve naming of some vCPU id related items
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
1 sibling, 2 replies; 15+ messages in thread
From: Cédric Le Goater @ 2020-11-30 17:32 UTC (permalink / raw)
To: Greg Kurz, David Gibson; +Cc: qemu-ppc, qemu-devel
On 11/30/20 5:52 PM, 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.
OK. This looks fine.
But, XIVE does not care about vCPU ids. Only the count of vCPUs
is relevant. So, it would be nice to add a comment in the code
that we got it wrong at some point or that XICS imposed the use
of max vCPU ids.
C.
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
> 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;
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH for-6.0 v2 2/3] spapr/xive: Fix size of END table and number of claimed IPIs
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
0 siblings, 1 reply; 15+ messages in thread
From: Cédric Le Goater @ 2020-11-30 18:07 UTC (permalink / raw)
To: Greg Kurz, David Gibson; +Cc: qemu-ppc, qemu-devel
On 11/30/20 5:52 PM, Greg Kurz wrote:
> The sPAPR XIVE device has an internal ENDT table the size of
> which is configurable by the machine. This table is supposed
> to contain END structures for all possible vCPUs that may
> enter the guest. The machine must also claim IPIs for all
> possible vCPUs since this is expected by the guest.
>
> spapr_irq_init() takes care of that under the assumption that
> spapr_max_vcpu_ids() returns the number of possible vCPUs.
> This happens to be the case when the VSMT mode is set to match
> the number of threads per core in the guest (default behavior).
> With non-default VSMT settings, this limit is > to the number
> of vCPUs. In the worst case, we can end up allocating an 8 times
> bigger ENDT and claiming 8 times more IPIs than needed. But more
> importantly, this creates a confusion between number of vCPUs and
> vCPU ids, which can lead to subtle bugs like [1].
>
> Use smp.max_cpus instead of spapr_max_vcpu_ids() in
> spapr_irq_init() for the latest machine type. Older machine
> types continue to use spapr_max_vcpu_ids() since the size of
> the ENDT is migration visible.
>
> [1] https://bugs.launchpad.net/qemu/+bug/1900241
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
Thanks,
C.
> ---
> include/hw/ppc/spapr.h | 1 +
> hw/ppc/spapr.c | 3 +++
> hw/ppc/spapr_irq.c | 16 +++++++++++++---
> 3 files changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index dc99d45e2852..95bf210d0bf6 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -139,6 +139,7 @@ struct SpaprMachineClass {
> hwaddr rma_limit; /* clamp the RMA to this size */
> bool pre_5_1_assoc_refpoints;
> bool pre_5_2_numa_associativity;
> + bool pre_6_0_xive_max_cpus;
>
> bool (*phb_placement)(SpaprMachineState *spapr, uint32_t index,
> uint64_t *buid, hwaddr *pio,
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index ab59bfe941d0..227a926dfd48 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -4530,8 +4530,11 @@ DEFINE_SPAPR_MACHINE(6_0, "6.0", true);
> */
> static void spapr_machine_5_2_class_options(MachineClass *mc)
> {
> + SpaprMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
> +
> spapr_machine_6_0_class_options(mc);
> compat_props_add(mc->compat_props, hw_compat_5_2, hw_compat_5_2_len);
> + smc->pre_6_0_xive_max_cpus = true;
> }
>
> DEFINE_SPAPR_MACHINE(5_2, "5.2", false);
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index 552e30e93036..4d9ecd5d0af8 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -324,17 +324,27 @@ void spapr_irq_init(SpaprMachineState *spapr, Error **errp)
> }
>
> if (spapr->irq->xive) {
> - uint32_t max_vcpu_ids = spapr_max_vcpu_ids(spapr);
> + uint32_t max_cpus = MACHINE(spapr)->smp.max_cpus;
> DeviceState *dev;
> int i;
>
> + /*
> + * Older machine types used to size the ENDT and IPI range
> + * according to the upper limit of vCPU ids, which can be
> + * higher than smp.max_cpus with custom VSMT settings. Keep
> + * the previous behavior for compatibility with such setups.
> + */
> + if (smc->pre_6_0_xive_max_cpus) {
> + max_cpus = spapr_max_vcpu_ids(spapr);
> + }
> +
> 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", max_vcpu_ids << 3);
> + qdev_prop_set_uint32(dev, "nr-ends", max_cpus << 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 +352,7 @@ void spapr_irq_init(SpaprMachineState *spapr, Error **errp)
> spapr->xive = SPAPR_XIVE(dev);
>
> /* Enable the CPU IPIs */
> - for (i = 0; i < max_vcpu_ids; ++i) {
> + for (i = 0; i < max_cpus; ++i) {
> SpaprInterruptControllerClass *sicc
> = SPAPR_INTC_GET_CLASS(spapr->xive);
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH for-6.0 v2 1/3] spapr: Improve naming of some vCPU id related items
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
1 sibling, 0 replies; 15+ messages in thread
From: Cédric Le Goater @ 2020-11-30 18:08 UTC (permalink / raw)
To: Greg Kurz, David Gibson; +Cc: qemu-ppc, qemu-devel
On 11/30/20 6:32 PM, Cédric Le Goater wrote:
> On 11/30/20 5:52 PM, 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.
>
> OK. This looks fine.
>
> But, XIVE does not care about vCPU ids. Only the count of vCPUs
> is relevant. So, it would be nice to add a comment in the code
> that we got it wrong at some point or that XICS imposed the use
> of max vCPU ids.
Which you do in the next patch,
Reviewed-by: Cédric Le Goater <clg@kaod.org>
Thanks,
C.
>
>> Signed-off-by: Greg Kurz <groug@kaod.org>
>> ---
>> 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;
>>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH for-6.0 v2 1/3] spapr: Improve naming of some vCPU id related items
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
1 sibling, 0 replies; 15+ messages in thread
From: Greg Kurz @ 2020-11-30 18:30 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: qemu-ppc, qemu-devel, David Gibson
On Mon, 30 Nov 2020 18:32:27 +0100
Cédric Le Goater <clg@kaod.org> wrote:
> On 11/30/20 5:52 PM, 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.
>
> OK. This looks fine.
>
> But, XIVE does not care about vCPU ids. Only the count of vCPUs
XIVE does not care about vCPU ids indeed but KVM does. The KVM XIVE code
has been indexing VPs with vCPU ids from the start as visible in linux
commit 5af50993850a ("KVM: PPC: Book3S HV: Native usage of the XIVE
interrupt controller") :
+int kvmppc_xive_connect_vcpu(struct kvm_device *dev,
+ struct kvm_vcpu *vcpu, u32 cpu)
+{
[...]
+ xc->vp_id = xive->vp_base + cpu;
This allows to have a quick conversion between vCPU id and VP id
without relying on an intermediate mapping structure.
> is relevant. So, it would be nice to add a comment in the code
> that we got it wrong at some point or that XICS imposed the use
> of max vCPU ids.
>
There's more to it. The vCPU id spacing is a hack that was
introduced to workaround the limitation of pre-POWER9 cpu
types that cannot have HW threads from the same core running
in different MMU contexts. This was probably not such a good
idea but we have to live with it now since vCPU ids are guest
visible. I don't think it brings much to complain once more
over vCPU spacing since this has been addressed with VSMT.
The machine now always have consecutive vCPU ids by default,
ie. number of vCPUs == number of vCPU ids. Only custom machine
setups that tweak VSMT to some other value can be affected
by the spacing.
> C.
>
>
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> > 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;
> >
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH for-6.0 v2 1/3] spapr: Improve naming of some vCPU id related items
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-12-02 4:56 ` David Gibson
2020-12-03 9:06 ` Greg Kurz
1 sibling, 1 reply; 15+ messages in thread
From: David Gibson @ 2020-12-02 4:56 UTC (permalink / raw)
To: Greg Kurz; +Cc: qemu-ppc, qemu-devel, Cédric Le Goater
[-- Attachment #1: Type: text/plain, Size: 16004 bytes --]
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.
> ---
> 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;
--
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 --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH for-6.0 v2 1/3] spapr: Improve naming of some vCPU id related items
2020-12-02 4:56 ` David Gibson
@ 2020-12-03 9:06 ` Greg Kurz
0 siblings, 0 replies; 15+ messages in thread
From: Greg Kurz @ 2020-12-03 9:06 UTC (permalink / raw)
To: David Gibson; +Cc: qemu-ppc, qemu-devel, Cédric Le Goater
[-- 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 --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH for-6.0 v2 2/3] spapr/xive: Fix size of END table and number of claimed IPIs
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
0 siblings, 1 reply; 15+ messages in thread
From: Cédric Le Goater @ 2020-12-03 9:51 UTC (permalink / raw)
To: Greg Kurz, David Gibson; +Cc: qemu-ppc, qemu-devel
On 11/30/20 7:07 PM, Cédric Le Goater wrote:
> On 11/30/20 5:52 PM, Greg Kurz wrote:
>> The sPAPR XIVE device has an internal ENDT table the size of
>> which is configurable by the machine. This table is supposed
>> to contain END structures for all possible vCPUs that may
>> enter the guest. The machine must also claim IPIs for all
>> possible vCPUs since this is expected by the guest.
>>
>> spapr_irq_init() takes care of that under the assumption that
>> spapr_max_vcpu_ids() returns the number of possible vCPUs.
>> This happens to be the case when the VSMT mode is set to match
>> the number of threads per core in the guest (default behavior).
>> With non-default VSMT settings, this limit is > to the number
>> of vCPUs. In the worst case, we can end up allocating an 8 times
>> bigger ENDT and claiming 8 times more IPIs than needed. But more
>> importantly, this creates a confusion between number of vCPUs and
>> vCPU ids, which can lead to subtle bugs like [1].
>>
>> Use smp.max_cpus instead of spapr_max_vcpu_ids() in
>> spapr_irq_init() for the latest machine type. Older machine
>> types continue to use spapr_max_vcpu_ids() since the size of
>> the ENDT is migration visible.
>>
>> [1] https://bugs.launchpad.net/qemu/+bug/1900241
>>
>> Signed-off-by: Greg Kurz <groug@kaod.org>
>
>
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
I gave patch 2 and 3 a little more thinking.
I don't think we need much more than patch 1 which clarifies the
nature of the values being manipulated, quantities vs. numbering.
The last 2 patches are adding complexity to try to optimize the
XIVE VP space in a case scenario which is not very common (vSMT).
May be it's not worth it.
Today, we can start 4K (-2) KVM guests with 16 vCPUs each on
a witherspoon (2 socket P9) and we are far from reaching the
limits of the VP space. Available RAM is more a problem.
VP space is even bigger on P10. The width was increased to 24bit
per chip.
C.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH for-6.0 v2 2/3] spapr/xive: Fix size of END table and number of claimed IPIs
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
0 siblings, 1 reply; 15+ messages in thread
From: Greg Kurz @ 2020-12-03 10:50 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: qemu-ppc, qemu-devel, David Gibson
On Thu, 3 Dec 2020 10:51:10 +0100
Cédric Le Goater <clg@kaod.org> wrote:
> On 11/30/20 7:07 PM, Cédric Le Goater wrote:
> > On 11/30/20 5:52 PM, Greg Kurz wrote:
> >> The sPAPR XIVE device has an internal ENDT table the size of
> >> which is configurable by the machine. This table is supposed
> >> to contain END structures for all possible vCPUs that may
> >> enter the guest. The machine must also claim IPIs for all
> >> possible vCPUs since this is expected by the guest.
> >>
> >> spapr_irq_init() takes care of that under the assumption that
> >> spapr_max_vcpu_ids() returns the number of possible vCPUs.
> >> This happens to be the case when the VSMT mode is set to match
> >> the number of threads per core in the guest (default behavior).
> >> With non-default VSMT settings, this limit is > to the number
> >> of vCPUs. In the worst case, we can end up allocating an 8 times
> >> bigger ENDT and claiming 8 times more IPIs than needed. But more
> >> importantly, this creates a confusion between number of vCPUs and
> >> vCPU ids, which can lead to subtle bugs like [1].
> >>
> >> Use smp.max_cpus instead of spapr_max_vcpu_ids() in
> >> spapr_irq_init() for the latest machine type. Older machine
> >> types continue to use spapr_max_vcpu_ids() since the size of
> >> the ENDT is migration visible.
> >>
> >> [1] https://bugs.launchpad.net/qemu/+bug/1900241
> >>
> >> Signed-off-by: Greg Kurz <groug@kaod.org>
> >
> >
> > Reviewed-by: Cédric Le Goater <clg@kaod.org>
>
>
> I gave patch 2 and 3 a little more thinking.
>
> I don't think we need much more than patch 1 which clarifies the
> nature of the values being manipulated, quantities vs. numbering.
>
> The last 2 patches are adding complexity to try to optimize the
> XIVE VP space in a case scenario which is not very common (vSMT).
> May be it's not worth it.
>
Well, the motivation isn't about optimization really since
a non-default vSMT setting already wastes VP space because
of the vCPU spacing. This is more about not using values
with wrong semantics in the code to avoid confusion in
future changes.
I agree though that the extra complexity, especially the
compat crust, might be excessive. So maybe I should just
add comments in the code to clarify when we're using the
wrong semantics ?
> Today, we can start 4K (-2) KVM guests with 16 vCPUs each on
> a witherspoon (2 socket P9) and we are far from reaching the
> limits of the VP space. Available RAM is more a problem.
>
> VP space is even bigger on P10. The width was increased to 24bit
> per chip.
>
> C.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH for-6.0 v2 2/3] spapr/xive: Fix size of END table and number of claimed IPIs
2020-12-03 10:50 ` Greg Kurz
@ 2020-12-04 8:46 ` Cédric Le Goater
2020-12-04 9:11 ` Greg Kurz
0 siblings, 1 reply; 15+ messages in thread
From: Cédric Le Goater @ 2020-12-04 8:46 UTC (permalink / raw)
To: Greg Kurz; +Cc: qemu-ppc, qemu-devel, David Gibson
>> I don't think we need much more than patch 1 which clarifies the
>> nature of the values being manipulated, quantities vs. numbering.
>>
>> The last 2 patches are adding complexity to try to optimize the
>> XIVE VP space in a case scenario which is not very common (vSMT).
>> May be it's not worth it.
>>
>
> Well, the motivation isn't about optimization really since
> a non-default vSMT setting already wastes VP space because
> of the vCPU spacing.
I don't see any VPs being wasted when not using vSMT. What's
your command line ?
> This is more about not using values
> with wrong semantics in the code to avoid confusion in
> future changes.
yes.
> I agree though that the extra complexity, especially the
> compat crust, might be excessive.
It's nice and correct but it seems a bit like extra noise
if the default case is not wasting VPs. Let's check that
first.
> So maybe I should just
> add comments in the code to clarify when we're using the
> wrong semantics ?
yes. I think this is enough.
Thanks,
C.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH for-6.0 v2 2/3] spapr/xive: Fix size of END table and number of claimed IPIs
2020-12-04 8:46 ` Cédric Le Goater
@ 2020-12-04 9:11 ` Greg Kurz
0 siblings, 0 replies; 15+ messages in thread
From: Greg Kurz @ 2020-12-04 9:11 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: qemu-ppc, qemu-devel, David Gibson
On Fri, 4 Dec 2020 09:46:31 +0100
Cédric Le Goater <clg@kaod.org> wrote:
> >> I don't think we need much more than patch 1 which clarifies the
> >> nature of the values being manipulated, quantities vs. numbering.
> >>
> >> The last 2 patches are adding complexity to try to optimize the
> >> XIVE VP space in a case scenario which is not very common (vSMT).
> >> May be it's not worth it.
> >>
> >
> > Well, the motivation isn't about optimization really since
> > a non-default vSMT setting already wastes VP space because
> > of the vCPU spacing.
>
> I don't see any VPs being wasted when not using vSMT. What's
> your command line ?
>
I think there's confusion here. VSMT is always being used.
When you don't specify it on the command line, the machine
code sets it internally for you to be equal to the number
of threads/core of the guest. Thanks to that, you get
consecutive vCPU ids and no VP waste. Of course, you
get the same result if you do:
-M pseries,vsmt=N -smp threads=N
If you pass different values to vsmt and threads, though,
you get the spacing and the VP waste.
> > This is more about not using values
> > with wrong semantics in the code to avoid confusion in
> > future changes.
>
> yes.
>
> > I agree though that the extra complexity, especially the
> > compat crust, might be excessive.
>
> It's nice and correct but it seems a bit like extra noise
> if the default case is not wasting VPs. Let's check that
> first.
>
> > So maybe I should just
> > add comments in the code to clarify when we're using the
> > wrong semantics ?
>
> yes. I think this is enough.
>
I'll do this in v3 then.
> Thanks,
>
> C.
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2020-12-04 9:13 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
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
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.