* [PATCH for-6.0 1/9] spapr: Do PCI device hotplug sanity checks at pre-plug only
2020-11-20 23:41 [PATCH for-6.0 0/9] spapr: Perform hotplug sanity checks at pre-plug Greg Kurz
@ 2020-11-20 23:42 ` Greg Kurz
2020-11-23 4:50 ` David Gibson
2020-11-20 23:42 ` [PATCH for-6.0 2/9] spapr: Do NVDIMM/PC-DIMM " Greg Kurz
` (7 subsequent siblings)
8 siblings, 1 reply; 30+ messages in thread
From: Greg Kurz @ 2020-11-20 23:42 UTC (permalink / raw)
To: qemu-devel; +Cc: Igor Mammedov, qemu-ppc, Greg Kurz, David Gibson
The PHB acts as the hotplug handler for PCI devices. It does some
sanity checks on DR enablement, PCI bridge chassis numbers and
multifunction. These checks are currently performed at plug time,
but they would best sit in a pre-plug handler in order to error
out as early as possible.
Create a spapr_pci_pre_plug() handler and move all the checking
there. Add a check that the associated DRC doesn't already have
an attached device. This is equivalent to the slot availability
check performed by do_pci_register_device() upon realization of
the PCI device.
This allows to pass &error_abort to spapr_drc_attach() and to end
up with a plug handler that doesn't need to report errors anymore.
Signed-off-by: Greg Kurz <groug@kaod.org>
---
hw/ppc/spapr_pci.c | 43 +++++++++++++++++++++++++++++++++----------
1 file changed, 33 insertions(+), 10 deletions(-)
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 88ce87f130a5..2829f298d9c1 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1532,8 +1532,8 @@ static bool bridge_has_valid_chassis_nr(Object *bridge, Error **errp)
return true;
}
-static void spapr_pci_plug(HotplugHandler *plug_handler,
- DeviceState *plugged_dev, Error **errp)
+static void spapr_pci_pre_plug(HotplugHandler *plug_handler,
+ DeviceState *plugged_dev, Error **errp)
{
SpaprPhbState *phb = SPAPR_PCI_HOST_BRIDGE(DEVICE(plug_handler));
PCIDevice *pdev = PCI_DEVICE(plugged_dev);
@@ -1542,9 +1542,6 @@ static void spapr_pci_plug(HotplugHandler *plug_handler,
PCIBus *bus = PCI_BUS(qdev_get_parent_bus(DEVICE(pdev)));
uint32_t slotnr = PCI_SLOT(pdev->devfn);
- /* if DR is disabled we don't need to do anything in the case of
- * hotplug or coldplug callbacks
- */
if (!phb->dr_enabled) {
/* if this is a hotplug operation initiated by the user
* we need to let them know it's not enabled
@@ -1552,17 +1549,14 @@ static void spapr_pci_plug(HotplugHandler *plug_handler,
if (plugged_dev->hotplugged) {
error_setg(errp, QERR_BUS_NO_HOTPLUG,
object_get_typename(OBJECT(phb)));
+ return;
}
- return;
}
- g_assert(drc);
-
if (pc->is_bridge) {
if (!bridge_has_valid_chassis_nr(OBJECT(plugged_dev), errp)) {
return;
}
- spapr_pci_bridge_plug(phb, PCI_BRIDGE(plugged_dev));
}
/* Following the QEMU convention used for PCIe multifunction
@@ -1574,13 +1568,41 @@ static void spapr_pci_plug(HotplugHandler *plug_handler,
error_setg(errp, "PCI: slot %d function 0 already occupied by %s,"
" additional functions can no longer be exposed to guest.",
slotnr, bus->devices[PCI_DEVFN(slotnr, 0)]->name);
+ }
+
+ if (drc && drc->dev) {
+ error_setg(errp, "PCI: slot %d already occupied by %s", slotnr,
+ pci_get_function_0(PCI_DEVICE(drc->dev))->name);
return;
}
+}
+
+static void spapr_pci_plug(HotplugHandler *plug_handler,
+ DeviceState *plugged_dev, Error **errp)
+{
+ SpaprPhbState *phb = SPAPR_PCI_HOST_BRIDGE(DEVICE(plug_handler));
+ PCIDevice *pdev = PCI_DEVICE(plugged_dev);
+ PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(plugged_dev);
+ SpaprDrc *drc = drc_from_dev(phb, pdev);
+ uint32_t slotnr = PCI_SLOT(pdev->devfn);
- if (!spapr_drc_attach(drc, DEVICE(pdev), errp)) {
+ /*
+ * If DR is disabled we don't need to do anything in the case of
+ * hotplug or coldplug callbacks.
+ */
+ if (!phb->dr_enabled) {
return;
}
+ g_assert(drc);
+
+ if (pc->is_bridge) {
+ spapr_pci_bridge_plug(phb, PCI_BRIDGE(plugged_dev));
+ }
+
+ /* spapr_pci_pre_plug() already checked the DRC is attachable */
+ spapr_drc_attach(drc, DEVICE(pdev), &error_abort);
+
/* If this is function 0, signal hotplug for all the device functions.
* Otherwise defer sending the hotplug event.
*/
@@ -2223,6 +2245,7 @@ static void spapr_phb_class_init(ObjectClass *klass, void *data)
/* Supported by TYPE_SPAPR_MACHINE */
dc->user_creatable = true;
set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
+ hp->pre_plug = spapr_pci_pre_plug;
hp->plug = spapr_pci_plug;
hp->unplug = spapr_pci_unplug;
hp->unplug_request = spapr_pci_unplug_request;
--
2.26.2
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH for-6.0 1/9] spapr: Do PCI device hotplug sanity checks at pre-plug only
2020-11-20 23:42 ` [PATCH for-6.0 1/9] spapr: Do PCI device hotplug sanity checks at pre-plug only Greg Kurz
@ 2020-11-23 4:50 ` David Gibson
0 siblings, 0 replies; 30+ messages in thread
From: David Gibson @ 2020-11-23 4:50 UTC (permalink / raw)
To: Greg Kurz; +Cc: Igor Mammedov, qemu-ppc, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 5085 bytes --]
On Sat, Nov 21, 2020 at 12:42:00AM +0100, Greg Kurz wrote:
> The PHB acts as the hotplug handler for PCI devices. It does some
> sanity checks on DR enablement, PCI bridge chassis numbers and
> multifunction. These checks are currently performed at plug time,
> but they would best sit in a pre-plug handler in order to error
> out as early as possible.
>
> Create a spapr_pci_pre_plug() handler and move all the checking
> there. Add a check that the associated DRC doesn't already have
> an attached device. This is equivalent to the slot availability
> check performed by do_pci_register_device() upon realization of
> the PCI device.
>
> This allows to pass &error_abort to spapr_drc_attach() and to end
> up with a plug handler that doesn't need to report errors anymore.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
Applied to ppc-for-6.0, thanks.
> ---
> hw/ppc/spapr_pci.c | 43 +++++++++++++++++++++++++++++++++----------
> 1 file changed, 33 insertions(+), 10 deletions(-)
>
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 88ce87f130a5..2829f298d9c1 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1532,8 +1532,8 @@ static bool bridge_has_valid_chassis_nr(Object *bridge, Error **errp)
> return true;
> }
>
> -static void spapr_pci_plug(HotplugHandler *plug_handler,
> - DeviceState *plugged_dev, Error **errp)
> +static void spapr_pci_pre_plug(HotplugHandler *plug_handler,
> + DeviceState *plugged_dev, Error **errp)
> {
> SpaprPhbState *phb = SPAPR_PCI_HOST_BRIDGE(DEVICE(plug_handler));
> PCIDevice *pdev = PCI_DEVICE(plugged_dev);
> @@ -1542,9 +1542,6 @@ static void spapr_pci_plug(HotplugHandler *plug_handler,
> PCIBus *bus = PCI_BUS(qdev_get_parent_bus(DEVICE(pdev)));
> uint32_t slotnr = PCI_SLOT(pdev->devfn);
>
> - /* if DR is disabled we don't need to do anything in the case of
> - * hotplug or coldplug callbacks
> - */
> if (!phb->dr_enabled) {
> /* if this is a hotplug operation initiated by the user
> * we need to let them know it's not enabled
> @@ -1552,17 +1549,14 @@ static void spapr_pci_plug(HotplugHandler *plug_handler,
> if (plugged_dev->hotplugged) {
> error_setg(errp, QERR_BUS_NO_HOTPLUG,
> object_get_typename(OBJECT(phb)));
> + return;
> }
> - return;
> }
>
> - g_assert(drc);
> -
> if (pc->is_bridge) {
> if (!bridge_has_valid_chassis_nr(OBJECT(plugged_dev), errp)) {
> return;
> }
> - spapr_pci_bridge_plug(phb, PCI_BRIDGE(plugged_dev));
> }
>
> /* Following the QEMU convention used for PCIe multifunction
> @@ -1574,13 +1568,41 @@ static void spapr_pci_plug(HotplugHandler *plug_handler,
> error_setg(errp, "PCI: slot %d function 0 already occupied by %s,"
> " additional functions can no longer be exposed to guest.",
> slotnr, bus->devices[PCI_DEVFN(slotnr, 0)]->name);
> + }
> +
> + if (drc && drc->dev) {
> + error_setg(errp, "PCI: slot %d already occupied by %s", slotnr,
> + pci_get_function_0(PCI_DEVICE(drc->dev))->name);
> return;
> }
> +}
> +
> +static void spapr_pci_plug(HotplugHandler *plug_handler,
> + DeviceState *plugged_dev, Error **errp)
> +{
> + SpaprPhbState *phb = SPAPR_PCI_HOST_BRIDGE(DEVICE(plug_handler));
> + PCIDevice *pdev = PCI_DEVICE(plugged_dev);
> + PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(plugged_dev);
> + SpaprDrc *drc = drc_from_dev(phb, pdev);
> + uint32_t slotnr = PCI_SLOT(pdev->devfn);
>
> - if (!spapr_drc_attach(drc, DEVICE(pdev), errp)) {
> + /*
> + * If DR is disabled we don't need to do anything in the case of
> + * hotplug or coldplug callbacks.
> + */
> + if (!phb->dr_enabled) {
> return;
> }
>
> + g_assert(drc);
> +
> + if (pc->is_bridge) {
> + spapr_pci_bridge_plug(phb, PCI_BRIDGE(plugged_dev));
> + }
> +
> + /* spapr_pci_pre_plug() already checked the DRC is attachable */
> + spapr_drc_attach(drc, DEVICE(pdev), &error_abort);
> +
> /* If this is function 0, signal hotplug for all the device functions.
> * Otherwise defer sending the hotplug event.
> */
> @@ -2223,6 +2245,7 @@ static void spapr_phb_class_init(ObjectClass *klass, void *data)
> /* Supported by TYPE_SPAPR_MACHINE */
> dc->user_creatable = true;
> set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
> + hp->pre_plug = spapr_pci_pre_plug;
> hp->plug = spapr_pci_plug;
> hp->unplug = spapr_pci_unplug;
> hp->unplug_request = spapr_pci_unplug_request;
--
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] 30+ messages in thread
* [PATCH for-6.0 2/9] spapr: Do NVDIMM/PC-DIMM device hotplug sanity checks at pre-plug only
2020-11-20 23:41 [PATCH for-6.0 0/9] spapr: Perform hotplug sanity checks at pre-plug Greg Kurz
2020-11-20 23:42 ` [PATCH for-6.0 1/9] spapr: Do PCI device hotplug sanity checks at pre-plug only Greg Kurz
@ 2020-11-20 23:42 ` Greg Kurz
2020-11-23 4:55 ` David Gibson
2020-11-20 23:42 ` [PATCH for-6.0 3/9] spapr: Fix pre-2.10 dummy ICP hack Greg Kurz
` (6 subsequent siblings)
8 siblings, 1 reply; 30+ messages in thread
From: Greg Kurz @ 2020-11-20 23:42 UTC (permalink / raw)
To: qemu-devel; +Cc: Igor Mammedov, qemu-ppc, Greg Kurz, David Gibson
Pre-plug of a memory device, be it an NVDIMM or a PC-DIMM, ensures
that the memory slot is available and that addresses don't overlap
with existing memory regions. The corresponding DRCs in the LMB
and PMEM namespaces are thus necessarily attachable at plug time.
Pass &error_abort to spapr_drc_attach() in spapr_add_lmbs() and
spapr_add_nvdimm(). This allows to greatly simplify error handling
on the plug path.
Signed-off-by: Greg Kurz <groug@kaod.org>
---
include/hw/ppc/spapr_nvdimm.h | 2 +-
hw/ppc/spapr.c | 40 ++++++++++++-----------------------
hw/ppc/spapr_nvdimm.c | 11 +++++-----
3 files changed, 20 insertions(+), 33 deletions(-)
diff --git a/include/hw/ppc/spapr_nvdimm.h b/include/hw/ppc/spapr_nvdimm.h
index 344582d2f5f7..73be250e2ac9 100644
--- a/include/hw/ppc/spapr_nvdimm.h
+++ b/include/hw/ppc/spapr_nvdimm.h
@@ -30,6 +30,6 @@ int spapr_pmem_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr,
void spapr_dt_persistent_memory(SpaprMachineState *spapr, void *fdt);
bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm,
uint64_t size, Error **errp);
-bool spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp);
+void spapr_add_nvdimm(DeviceState *dev, uint64_t slot);
#endif
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 12a012d9dd09..394d28d9e081 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3382,8 +3382,8 @@ int spapr_lmb_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr,
return 0;
}
-static bool spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
- bool dedicated_hp_event_source, Error **errp)
+static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
+ bool dedicated_hp_event_source)
{
SpaprDrc *drc;
uint32_t nr_lmbs = size/SPAPR_MEMORY_BLOCK_SIZE;
@@ -3396,15 +3396,12 @@ static bool spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
addr / SPAPR_MEMORY_BLOCK_SIZE);
g_assert(drc);
- if (!spapr_drc_attach(drc, dev, errp)) {
- while (addr > addr_start) {
- addr -= SPAPR_MEMORY_BLOCK_SIZE;
- drc = spapr_drc_by_id(TYPE_SPAPR_DRC_LMB,
- addr / SPAPR_MEMORY_BLOCK_SIZE);
- spapr_drc_detach(drc);
- }
- return false;
- }
+ /*
+ * memory_device_get_free_addr() provided a range of free addresses
+ * that doesn't overlap with any existing mapping at pre-plug. The
+ * corresponding LMB DRCs are thus assumed to be all attachable.
+ */
+ spapr_drc_attach(drc, dev, &error_abort);
if (!hotplugged) {
spapr_drc_reset(drc);
}
@@ -3425,11 +3422,9 @@ static bool spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
nr_lmbs);
}
}
- return true;
}
-static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
- Error **errp)
+static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev)
{
SpaprMachineState *ms = SPAPR_MACHINE(hotplug_dev);
PCDIMMDevice *dimm = PC_DIMM(dev);
@@ -3444,24 +3439,15 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
if (!is_nvdimm) {
addr = object_property_get_uint(OBJECT(dimm),
PC_DIMM_ADDR_PROP, &error_abort);
- if (!spapr_add_lmbs(dev, addr, size,
- spapr_ovec_test(ms->ov5_cas, OV5_HP_EVT), errp)) {
- goto out_unplug;
- }
+ spapr_add_lmbs(dev, addr, size,
+ spapr_ovec_test(ms->ov5_cas, OV5_HP_EVT));
} else {
slot = object_property_get_int(OBJECT(dimm),
PC_DIMM_SLOT_PROP, &error_abort);
/* We should have valid slot number at this point */
g_assert(slot >= 0);
- if (!spapr_add_nvdimm(dev, slot, errp)) {
- goto out_unplug;
- }
+ spapr_add_nvdimm(dev, slot);
}
-
- return;
-
-out_unplug:
- pc_dimm_unplug(dimm, MACHINE(ms));
}
static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
@@ -4009,7 +3995,7 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
DeviceState *dev, Error **errp)
{
if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
- spapr_memory_plug(hotplug_dev, dev, errp);
+ spapr_memory_plug(hotplug_dev, dev);
} else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
spapr_core_plug(hotplug_dev, dev, errp);
} else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_PCI_HOST_BRIDGE)) {
diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
index a833a63b5ed3..2f1c196e1b76 100644
--- a/hw/ppc/spapr_nvdimm.c
+++ b/hw/ppc/spapr_nvdimm.c
@@ -89,7 +89,7 @@ bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm,
}
-bool spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp)
+void spapr_add_nvdimm(DeviceState *dev, uint64_t slot)
{
SpaprDrc *drc;
bool hotplugged = spapr_drc_hotplugged(dev);
@@ -97,14 +97,15 @@ bool spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp)
drc = spapr_drc_by_id(TYPE_SPAPR_DRC_PMEM, slot);
g_assert(drc);
- if (!spapr_drc_attach(drc, dev, errp)) {
- return false;
- }
+ /*
+ * pc_dimm_get_free_slot() provided a free slot at pre-plug. The
+ * corresponding DRC is thus assumed to be attachable.
+ */
+ spapr_drc_attach(drc, dev, &error_abort);
if (hotplugged) {
spapr_hotplug_req_add_by_index(drc);
}
- return true;
}
static int spapr_dt_nvdimm(SpaprMachineState *spapr, void *fdt,
--
2.26.2
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH for-6.0 2/9] spapr: Do NVDIMM/PC-DIMM device hotplug sanity checks at pre-plug only
2020-11-20 23:42 ` [PATCH for-6.0 2/9] spapr: Do NVDIMM/PC-DIMM " Greg Kurz
@ 2020-11-23 4:55 ` David Gibson
0 siblings, 0 replies; 30+ messages in thread
From: David Gibson @ 2020-11-23 4:55 UTC (permalink / raw)
To: Greg Kurz; +Cc: Igor Mammedov, qemu-ppc, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 6715 bytes --]
On Sat, Nov 21, 2020 at 12:42:01AM +0100, Greg Kurz wrote:
> Pre-plug of a memory device, be it an NVDIMM or a PC-DIMM, ensures
> that the memory slot is available and that addresses don't overlap
> with existing memory regions. The corresponding DRCs in the LMB
> and PMEM namespaces are thus necessarily attachable at plug time.
>
> Pass &error_abort to spapr_drc_attach() in spapr_add_lmbs() and
> spapr_add_nvdimm(). This allows to greatly simplify error handling
> on the plug path.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
Applied to ppc-for-6.0, thanks.
> ---
> include/hw/ppc/spapr_nvdimm.h | 2 +-
> hw/ppc/spapr.c | 40 ++++++++++++-----------------------
> hw/ppc/spapr_nvdimm.c | 11 +++++-----
> 3 files changed, 20 insertions(+), 33 deletions(-)
>
> diff --git a/include/hw/ppc/spapr_nvdimm.h b/include/hw/ppc/spapr_nvdimm.h
> index 344582d2f5f7..73be250e2ac9 100644
> --- a/include/hw/ppc/spapr_nvdimm.h
> +++ b/include/hw/ppc/spapr_nvdimm.h
> @@ -30,6 +30,6 @@ int spapr_pmem_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr,
> void spapr_dt_persistent_memory(SpaprMachineState *spapr, void *fdt);
> bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm,
> uint64_t size, Error **errp);
> -bool spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp);
> +void spapr_add_nvdimm(DeviceState *dev, uint64_t slot);
>
> #endif
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 12a012d9dd09..394d28d9e081 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3382,8 +3382,8 @@ int spapr_lmb_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr,
> return 0;
> }
>
> -static bool spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
> - bool dedicated_hp_event_source, Error **errp)
> +static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
> + bool dedicated_hp_event_source)
> {
> SpaprDrc *drc;
> uint32_t nr_lmbs = size/SPAPR_MEMORY_BLOCK_SIZE;
> @@ -3396,15 +3396,12 @@ static bool spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
> addr / SPAPR_MEMORY_BLOCK_SIZE);
> g_assert(drc);
>
> - if (!spapr_drc_attach(drc, dev, errp)) {
> - while (addr > addr_start) {
> - addr -= SPAPR_MEMORY_BLOCK_SIZE;
> - drc = spapr_drc_by_id(TYPE_SPAPR_DRC_LMB,
> - addr / SPAPR_MEMORY_BLOCK_SIZE);
> - spapr_drc_detach(drc);
> - }
> - return false;
> - }
> + /*
> + * memory_device_get_free_addr() provided a range of free addresses
> + * that doesn't overlap with any existing mapping at pre-plug. The
> + * corresponding LMB DRCs are thus assumed to be all attachable.
> + */
> + spapr_drc_attach(drc, dev, &error_abort);
> if (!hotplugged) {
> spapr_drc_reset(drc);
> }
> @@ -3425,11 +3422,9 @@ static bool spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
> nr_lmbs);
> }
> }
> - return true;
> }
>
> -static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> - Error **errp)
> +static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev)
> {
> SpaprMachineState *ms = SPAPR_MACHINE(hotplug_dev);
> PCDIMMDevice *dimm = PC_DIMM(dev);
> @@ -3444,24 +3439,15 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> if (!is_nvdimm) {
> addr = object_property_get_uint(OBJECT(dimm),
> PC_DIMM_ADDR_PROP, &error_abort);
> - if (!spapr_add_lmbs(dev, addr, size,
> - spapr_ovec_test(ms->ov5_cas, OV5_HP_EVT), errp)) {
> - goto out_unplug;
> - }
> + spapr_add_lmbs(dev, addr, size,
> + spapr_ovec_test(ms->ov5_cas, OV5_HP_EVT));
> } else {
> slot = object_property_get_int(OBJECT(dimm),
> PC_DIMM_SLOT_PROP, &error_abort);
> /* We should have valid slot number at this point */
> g_assert(slot >= 0);
> - if (!spapr_add_nvdimm(dev, slot, errp)) {
> - goto out_unplug;
> - }
> + spapr_add_nvdimm(dev, slot);
> }
> -
> - return;
> -
> -out_unplug:
> - pc_dimm_unplug(dimm, MACHINE(ms));
> }
>
> static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> @@ -4009,7 +3995,7 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
> DeviceState *dev, Error **errp)
> {
> if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> - spapr_memory_plug(hotplug_dev, dev, errp);
> + spapr_memory_plug(hotplug_dev, dev);
> } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> spapr_core_plug(hotplug_dev, dev, errp);
> } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_PCI_HOST_BRIDGE)) {
> diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
> index a833a63b5ed3..2f1c196e1b76 100644
> --- a/hw/ppc/spapr_nvdimm.c
> +++ b/hw/ppc/spapr_nvdimm.c
> @@ -89,7 +89,7 @@ bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm,
> }
>
>
> -bool spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp)
> +void spapr_add_nvdimm(DeviceState *dev, uint64_t slot)
> {
> SpaprDrc *drc;
> bool hotplugged = spapr_drc_hotplugged(dev);
> @@ -97,14 +97,15 @@ bool spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp)
> drc = spapr_drc_by_id(TYPE_SPAPR_DRC_PMEM, slot);
> g_assert(drc);
>
> - if (!spapr_drc_attach(drc, dev, errp)) {
> - return false;
> - }
> + /*
> + * pc_dimm_get_free_slot() provided a free slot at pre-plug. The
> + * corresponding DRC is thus assumed to be attachable.
> + */
> + spapr_drc_attach(drc, dev, &error_abort);
>
> if (hotplugged) {
> spapr_hotplug_req_add_by_index(drc);
> }
> - return true;
> }
>
> static int spapr_dt_nvdimm(SpaprMachineState *spapr, void *fdt,
--
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] 30+ messages in thread
* [PATCH for-6.0 3/9] spapr: Fix pre-2.10 dummy ICP hack
2020-11-20 23:41 [PATCH for-6.0 0/9] spapr: Perform hotplug sanity checks at pre-plug Greg Kurz
2020-11-20 23:42 ` [PATCH for-6.0 1/9] spapr: Do PCI device hotplug sanity checks at pre-plug only Greg Kurz
2020-11-20 23:42 ` [PATCH for-6.0 2/9] spapr: Do NVDIMM/PC-DIMM " Greg Kurz
@ 2020-11-20 23:42 ` Greg Kurz
2020-11-23 5:03 ` David Gibson
2020-11-20 23:42 ` [PATCH for-6.0 4/9] spapr: Set compat mode in spapr_reset_vcpu() Greg Kurz
` (5 subsequent siblings)
8 siblings, 1 reply; 30+ messages in thread
From: Greg Kurz @ 2020-11-20 23:42 UTC (permalink / raw)
To: qemu-devel; +Cc: Igor Mammedov, qemu-ppc, Greg Kurz, David Gibson
This hack registers dummy VMState entries of ICPs in order to
support migration of old pseries machine types that used to
create all smp.max_cpus possible ICPs at machine init.
Part of the work is to unregister the dummy entries when plugging
an actual vCPU core, and to register them back when unplugging the
core. The code that unregisters the dummy ICPs in spapr_core_plug()
is misplaced: if ppc_set_compat() fails afterwards, the hotplug
operation will be cancelled and the dummy ICPs won't be registered
back since the unplug handler isn't called.
Unregister the dummy ICPs at the end of spapr_core_plug().
Signed-off-by: Greg Kurz <groug@kaod.org>
---
The next patch makes this patch a bit useless. I post it
anyway for the records because it is a programming error.
---
hw/ppc/spapr.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 394d28d9e081..f58f77389e8e 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3785,13 +3785,6 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
core_slot->cpu = OBJECT(dev);
- if (smc->pre_2_10_has_unused_icps) {
- for (i = 0; i < cc->nr_threads; i++) {
- cs = CPU(core->threads[i]);
- pre_2_10_vmstate_unregister_dummy_icp(cs->cpu_index);
- }
- }
-
/*
* Set compatibility mode to match the boot CPU, which was either set
* by the machine reset code or by CAS.
@@ -3805,6 +3798,13 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
}
}
}
+
+ if (smc->pre_2_10_has_unused_icps) {
+ for (i = 0; i < cc->nr_threads; i++) {
+ cs = CPU(core->threads[i]);
+ pre_2_10_vmstate_unregister_dummy_icp(cs->cpu_index);
+ }
+ }
}
static void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
--
2.26.2
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH for-6.0 3/9] spapr: Fix pre-2.10 dummy ICP hack
2020-11-20 23:42 ` [PATCH for-6.0 3/9] spapr: Fix pre-2.10 dummy ICP hack Greg Kurz
@ 2020-11-23 5:03 ` David Gibson
0 siblings, 0 replies; 30+ messages in thread
From: David Gibson @ 2020-11-23 5:03 UTC (permalink / raw)
To: Greg Kurz; +Cc: Igor Mammedov, qemu-ppc, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 2366 bytes --]
On Sat, Nov 21, 2020 at 12:42:02AM +0100, Greg Kurz wrote:
> This hack registers dummy VMState entries of ICPs in order to
> support migration of old pseries machine types that used to
> create all smp.max_cpus possible ICPs at machine init.
>
> Part of the work is to unregister the dummy entries when plugging
> an actual vCPU core, and to register them back when unplugging the
> core. The code that unregisters the dummy ICPs in spapr_core_plug()
> is misplaced: if ppc_set_compat() fails afterwards, the hotplug
> operation will be cancelled and the dummy ICPs won't be registered
> back since the unplug handler isn't called.
>
> Unregister the dummy ICPs at the end of spapr_core_plug().
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
Applied to ppc-for-6.0, thanks.
> ---
>
> The next patch makes this patch a bit useless. I post it
> anyway for the records because it is a programming error.
> ---
> hw/ppc/spapr.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 394d28d9e081..f58f77389e8e 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3785,13 +3785,6 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>
> core_slot->cpu = OBJECT(dev);
>
> - if (smc->pre_2_10_has_unused_icps) {
> - for (i = 0; i < cc->nr_threads; i++) {
> - cs = CPU(core->threads[i]);
> - pre_2_10_vmstate_unregister_dummy_icp(cs->cpu_index);
> - }
> - }
> -
> /*
> * Set compatibility mode to match the boot CPU, which was either set
> * by the machine reset code or by CAS.
> @@ -3805,6 +3798,13 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> }
> }
> }
> +
> + if (smc->pre_2_10_has_unused_icps) {
> + for (i = 0; i < cc->nr_threads; i++) {
> + cs = CPU(core->threads[i]);
> + pre_2_10_vmstate_unregister_dummy_icp(cs->cpu_index);
> + }
> + }
> }
>
> static void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
--
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] 30+ messages in thread
* [PATCH for-6.0 4/9] spapr: Set compat mode in spapr_reset_vcpu()
2020-11-20 23:41 [PATCH for-6.0 0/9] spapr: Perform hotplug sanity checks at pre-plug Greg Kurz
` (2 preceding siblings ...)
2020-11-20 23:42 ` [PATCH for-6.0 3/9] spapr: Fix pre-2.10 dummy ICP hack Greg Kurz
@ 2020-11-20 23:42 ` Greg Kurz
2020-11-23 5:11 ` David Gibson
2020-11-20 23:42 ` [PATCH for-6.0 5/9] spapr: Simplify error path of spapr_core_plug() Greg Kurz
` (4 subsequent siblings)
8 siblings, 1 reply; 30+ messages in thread
From: Greg Kurz @ 2020-11-20 23:42 UTC (permalink / raw)
To: qemu-devel; +Cc: Igor Mammedov, qemu-ppc, Greg Kurz, David Gibson
When it comes to resetting the compat mode of the vCPUS, there are
two situations to consider:
(1) machine reset should set the compat mode back to the machine default,
ie. spapr->max_compat_pvr
(2) hot plugged vCPUs should set their compat mode to mach the boot vCPU,
ie. POWERPC_CPU(first_cpu)->compat_pvr
This is currently handled in two separate places: globally for all vCPUs
from the machine reset code for (1) and for each thread of a core from
the hotplug path for (2).
Since the machine reset code already resets all vCPUs, starting with boot
vCPU, consolidate the logic in spapr_reset_vcpu(). Special case the boot
vCPU so that it resets its compat mode back to the machine default. Any
other vCPU just need to match the compat mode of the boot vCPU.
Failing to set the compat mode during machine reset is a fatal error,
but not for hot plugged vCPUs. This is arguable because if we've been
able to set the boot vCPU compat mode at CAS or during machine reset,
it should definitely not fail for other vCPUs. Since spapr_reset_vcpu()
already has a fatal error path for kvm_check_mmu() failures, do the
same for ppc_set_compat().
This gets rid of an error path in spapr_core_plug(). It will allow
further simplifications.
Signed-off-by: Greg Kurz <groug@kaod.org>
---
hw/ppc/spapr.c | 16 ----------------
hw/ppc/spapr_cpu_core.c | 13 +++++++++++++
2 files changed, 13 insertions(+), 16 deletions(-)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index f58f77389e8e..da7586f548df 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1606,8 +1606,6 @@ static void spapr_machine_reset(MachineState *machine)
spapr_ovec_cleanup(spapr->ov5_cas);
spapr->ov5_cas = spapr_ovec_new();
- ppc_set_compat_all(spapr->max_compat_pvr, &error_fatal);
-
/*
* This is fixing some of the default configuration of the XIVE
* devices. To be called after the reset of the machine devices.
@@ -3785,20 +3783,6 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
core_slot->cpu = OBJECT(dev);
- /*
- * Set compatibility mode to match the boot CPU, which was either set
- * by the machine reset code or by CAS.
- */
- if (hotplugged) {
- for (i = 0; i < cc->nr_threads; i++) {
- if (ppc_set_compat(core->threads[i],
- POWERPC_CPU(first_cpu)->compat_pvr,
- errp) < 0) {
- return;
- }
- }
- }
-
if (smc->pre_2_10_has_unused_icps) {
for (i = 0; i < cc->nr_threads; i++) {
cs = CPU(core->threads[i]);
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 2f7dc3c23ded..17741a3fb77f 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -27,6 +27,7 @@
static void spapr_reset_vcpu(PowerPCCPU *cpu)
{
+ PowerPCCPU *first_ppc_cpu = POWERPC_CPU(first_cpu);
CPUState *cs = CPU(cpu);
CPUPPCState *env = &cpu->env;
PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
@@ -69,6 +70,18 @@ static void spapr_reset_vcpu(PowerPCCPU *cpu)
kvm_check_mmu(cpu, &error_fatal);
spapr_irq_cpu_intc_reset(spapr, cpu);
+
+ /*
+ * The boot CPU is only reset during machine reset : reset its
+ * compatibility mode to the machine default. For other CPUs,
+ * either cold plugged or hot plugged, set the compatibility mode
+ * to match the boot CPU, which was either set by the machine reset
+ * code or by CAS.
+ */
+ ppc_set_compat(cpu,
+ cpu == first_ppc_cpu ?
+ spapr->max_compat_pvr : first_ppc_cpu->compat_pvr,
+ &error_fatal);
}
void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip,
--
2.26.2
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH for-6.0 4/9] spapr: Set compat mode in spapr_reset_vcpu()
2020-11-20 23:42 ` [PATCH for-6.0 4/9] spapr: Set compat mode in spapr_reset_vcpu() Greg Kurz
@ 2020-11-23 5:11 ` David Gibson
2020-11-23 11:51 ` Greg Kurz
0 siblings, 1 reply; 30+ messages in thread
From: David Gibson @ 2020-11-23 5:11 UTC (permalink / raw)
To: Greg Kurz; +Cc: Igor Mammedov, qemu-ppc, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 4508 bytes --]
On Sat, Nov 21, 2020 at 12:42:03AM +0100, Greg Kurz wrote:
> When it comes to resetting the compat mode of the vCPUS, there are
> two situations to consider:
> (1) machine reset should set the compat mode back to the machine default,
> ie. spapr->max_compat_pvr
> (2) hot plugged vCPUs should set their compat mode to mach the boot vCPU,
> ie. POWERPC_CPU(first_cpu)->compat_pvr
>
> This is currently handled in two separate places: globally for all vCPUs
> from the machine reset code for (1) and for each thread of a core from
> the hotplug path for (2).
>
> Since the machine reset code already resets all vCPUs, starting with boot
> vCPU, consolidate the logic in spapr_reset_vcpu(). Special case the boot
> vCPU so that it resets its compat mode back to the machine default. Any
> other vCPU just need to match the compat mode of the boot vCPU.
>
> Failing to set the compat mode during machine reset is a fatal error,
> but not for hot plugged vCPUs. This is arguable because if we've been
> able to set the boot vCPU compat mode at CAS or during machine reset,
> it should definitely not fail for other vCPUs. Since spapr_reset_vcpu()
> already has a fatal error path for kvm_check_mmu() failures, do the
> same for ppc_set_compat().
>
> This gets rid of an error path in spapr_core_plug(). It will allow
> further simplifications.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
> hw/ppc/spapr.c | 16 ----------------
> hw/ppc/spapr_cpu_core.c | 13 +++++++++++++
> 2 files changed, 13 insertions(+), 16 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index f58f77389e8e..da7586f548df 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1606,8 +1606,6 @@ static void spapr_machine_reset(MachineState *machine)
> spapr_ovec_cleanup(spapr->ov5_cas);
> spapr->ov5_cas = spapr_ovec_new();
>
> - ppc_set_compat_all(spapr->max_compat_pvr, &error_fatal);
> -
> /*
> * This is fixing some of the default configuration of the XIVE
> * devices. To be called after the reset of the machine devices.
> @@ -3785,20 +3783,6 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>
> core_slot->cpu = OBJECT(dev);
>
> - /*
> - * Set compatibility mode to match the boot CPU, which was either set
> - * by the machine reset code or by CAS.
> - */
> - if (hotplugged) {
> - for (i = 0; i < cc->nr_threads; i++) {
> - if (ppc_set_compat(core->threads[i],
> - POWERPC_CPU(first_cpu)->compat_pvr,
> - errp) < 0) {
> - return;
> - }
> - }
> - }
> -
> if (smc->pre_2_10_has_unused_icps) {
> for (i = 0; i < cc->nr_threads; i++) {
> cs = CPU(core->threads[i]);
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 2f7dc3c23ded..17741a3fb77f 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -27,6 +27,7 @@
>
> static void spapr_reset_vcpu(PowerPCCPU *cpu)
> {
> + PowerPCCPU *first_ppc_cpu = POWERPC_CPU(first_cpu);
> CPUState *cs = CPU(cpu);
> CPUPPCState *env = &cpu->env;
> PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> @@ -69,6 +70,18 @@ static void spapr_reset_vcpu(PowerPCCPU *cpu)
> kvm_check_mmu(cpu, &error_fatal);
>
> spapr_irq_cpu_intc_reset(spapr, cpu);
> +
> + /*
> + * The boot CPU is only reset during machine reset : reset its
> + * compatibility mode to the machine default. For other CPUs,
> + * either cold plugged or hot plugged, set the compatibility mode
> + * to match the boot CPU, which was either set by the machine reset
> + * code or by CAS.
> + */
> + ppc_set_compat(cpu,
> + cpu == first_ppc_cpu ?
> + spapr->max_compat_pvr : first_ppc_cpu->compat_pvr,
> + &error_fatal);
This assumes that when it is called for a non-boot CPU, it has already
been called for the boot CPU.. Are we certain that's guaranteed by
the sequence of reset calls during a full machine reset?
> }
>
> void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip,
--
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] 30+ messages in thread* Re: [PATCH for-6.0 4/9] spapr: Set compat mode in spapr_reset_vcpu()
2020-11-23 5:11 ` David Gibson
@ 2020-11-23 11:51 ` Greg Kurz
2020-11-25 2:39 ` David Gibson
0 siblings, 1 reply; 30+ messages in thread
From: Greg Kurz @ 2020-11-23 11:51 UTC (permalink / raw)
To: David Gibson; +Cc: Igor Mammedov, qemu-ppc, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 4825 bytes --]
On Mon, 23 Nov 2020 16:11:30 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:
> On Sat, Nov 21, 2020 at 12:42:03AM +0100, Greg Kurz wrote:
> > When it comes to resetting the compat mode of the vCPUS, there are
> > two situations to consider:
> > (1) machine reset should set the compat mode back to the machine default,
> > ie. spapr->max_compat_pvr
> > (2) hot plugged vCPUs should set their compat mode to mach the boot vCPU,
> > ie. POWERPC_CPU(first_cpu)->compat_pvr
> >
> > This is currently handled in two separate places: globally for all vCPUs
> > from the machine reset code for (1) and for each thread of a core from
> > the hotplug path for (2).
> >
> > Since the machine reset code already resets all vCPUs, starting with boot
> > vCPU, consolidate the logic in spapr_reset_vcpu(). Special case the boot
> > vCPU so that it resets its compat mode back to the machine default. Any
> > other vCPU just need to match the compat mode of the boot vCPU.
> >
> > Failing to set the compat mode during machine reset is a fatal error,
> > but not for hot plugged vCPUs. This is arguable because if we've been
> > able to set the boot vCPU compat mode at CAS or during machine reset,
> > it should definitely not fail for other vCPUs. Since spapr_reset_vcpu()
> > already has a fatal error path for kvm_check_mmu() failures, do the
> > same for ppc_set_compat().
> >
> > This gets rid of an error path in spapr_core_plug(). It will allow
> > further simplifications.
> >
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> > hw/ppc/spapr.c | 16 ----------------
> > hw/ppc/spapr_cpu_core.c | 13 +++++++++++++
> > 2 files changed, 13 insertions(+), 16 deletions(-)
> >
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index f58f77389e8e..da7586f548df 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1606,8 +1606,6 @@ static void spapr_machine_reset(MachineState *machine)
> > spapr_ovec_cleanup(spapr->ov5_cas);
> > spapr->ov5_cas = spapr_ovec_new();
> >
> > - ppc_set_compat_all(spapr->max_compat_pvr, &error_fatal);
> > -
> > /*
> > * This is fixing some of the default configuration of the XIVE
> > * devices. To be called after the reset of the machine devices.
> > @@ -3785,20 +3783,6 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >
> > core_slot->cpu = OBJECT(dev);
> >
> > - /*
> > - * Set compatibility mode to match the boot CPU, which was either set
> > - * by the machine reset code or by CAS.
> > - */
> > - if (hotplugged) {
> > - for (i = 0; i < cc->nr_threads; i++) {
> > - if (ppc_set_compat(core->threads[i],
> > - POWERPC_CPU(first_cpu)->compat_pvr,
> > - errp) < 0) {
> > - return;
> > - }
> > - }
> > - }
> > -
> > if (smc->pre_2_10_has_unused_icps) {
> > for (i = 0; i < cc->nr_threads; i++) {
> > cs = CPU(core->threads[i]);
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > index 2f7dc3c23ded..17741a3fb77f 100644
> > --- a/hw/ppc/spapr_cpu_core.c
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -27,6 +27,7 @@
> >
> > static void spapr_reset_vcpu(PowerPCCPU *cpu)
> > {
> > + PowerPCCPU *first_ppc_cpu = POWERPC_CPU(first_cpu);
> > CPUState *cs = CPU(cpu);
> > CPUPPCState *env = &cpu->env;
> > PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> > @@ -69,6 +70,18 @@ static void spapr_reset_vcpu(PowerPCCPU *cpu)
> > kvm_check_mmu(cpu, &error_fatal);
> >
> > spapr_irq_cpu_intc_reset(spapr, cpu);
> > +
> > + /*
> > + * The boot CPU is only reset during machine reset : reset its
> > + * compatibility mode to the machine default. For other CPUs,
> > + * either cold plugged or hot plugged, set the compatibility mode
> > + * to match the boot CPU, which was either set by the machine reset
> > + * code or by CAS.
> > + */
> > + ppc_set_compat(cpu,
> > + cpu == first_ppc_cpu ?
> > + spapr->max_compat_pvr : first_ppc_cpu->compat_pvr,
> > + &error_fatal);
>
> This assumes that when it is called for a non-boot CPU, it has already
> been called for the boot CPU.. Are we certain that's guaranteed by
> the sequence of reset calls during a full machine reset?
>
This happens to be the case. Basically because the boot CPU core
is created (including registering its reset handler) first and
qemu_devices_reset() calls handlers in the same order they were
registered.
> > }
> >
> > void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip,
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH for-6.0 4/9] spapr: Set compat mode in spapr_reset_vcpu()
2020-11-23 11:51 ` Greg Kurz
@ 2020-11-25 2:39 ` David Gibson
2020-11-25 9:51 ` Greg Kurz
0 siblings, 1 reply; 30+ messages in thread
From: David Gibson @ 2020-11-25 2:39 UTC (permalink / raw)
To: Greg Kurz; +Cc: Igor Mammedov, qemu-ppc, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 5540 bytes --]
On Mon, Nov 23, 2020 at 12:51:08PM +0100, Greg Kurz wrote:
> On Mon, 23 Nov 2020 16:11:30 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
>
> > On Sat, Nov 21, 2020 at 12:42:03AM +0100, Greg Kurz wrote:
> > > When it comes to resetting the compat mode of the vCPUS, there are
> > > two situations to consider:
> > > (1) machine reset should set the compat mode back to the machine default,
> > > ie. spapr->max_compat_pvr
> > > (2) hot plugged vCPUs should set their compat mode to mach the boot vCPU,
> > > ie. POWERPC_CPU(first_cpu)->compat_pvr
> > >
> > > This is currently handled in two separate places: globally for all vCPUs
> > > from the machine reset code for (1) and for each thread of a core from
> > > the hotplug path for (2).
> > >
> > > Since the machine reset code already resets all vCPUs, starting with boot
> > > vCPU, consolidate the logic in spapr_reset_vcpu(). Special case the boot
> > > vCPU so that it resets its compat mode back to the machine default. Any
> > > other vCPU just need to match the compat mode of the boot vCPU.
> > >
> > > Failing to set the compat mode during machine reset is a fatal error,
> > > but not for hot plugged vCPUs. This is arguable because if we've been
> > > able to set the boot vCPU compat mode at CAS or during machine reset,
> > > it should definitely not fail for other vCPUs. Since spapr_reset_vcpu()
> > > already has a fatal error path for kvm_check_mmu() failures, do the
> > > same for ppc_set_compat().
> > >
> > > This gets rid of an error path in spapr_core_plug(). It will allow
> > > further simplifications.
> > >
> > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > ---
> > > hw/ppc/spapr.c | 16 ----------------
> > > hw/ppc/spapr_cpu_core.c | 13 +++++++++++++
> > > 2 files changed, 13 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index f58f77389e8e..da7586f548df 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -1606,8 +1606,6 @@ static void spapr_machine_reset(MachineState *machine)
> > > spapr_ovec_cleanup(spapr->ov5_cas);
> > > spapr->ov5_cas = spapr_ovec_new();
> > >
> > > - ppc_set_compat_all(spapr->max_compat_pvr, &error_fatal);
> > > -
> > > /*
> > > * This is fixing some of the default configuration of the XIVE
> > > * devices. To be called after the reset of the machine devices.
> > > @@ -3785,20 +3783,6 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > >
> > > core_slot->cpu = OBJECT(dev);
> > >
> > > - /*
> > > - * Set compatibility mode to match the boot CPU, which was either set
> > > - * by the machine reset code or by CAS.
> > > - */
> > > - if (hotplugged) {
> > > - for (i = 0; i < cc->nr_threads; i++) {
> > > - if (ppc_set_compat(core->threads[i],
> > > - POWERPC_CPU(first_cpu)->compat_pvr,
> > > - errp) < 0) {
> > > - return;
> > > - }
> > > - }
> > > - }
> > > -
> > > if (smc->pre_2_10_has_unused_icps) {
> > > for (i = 0; i < cc->nr_threads; i++) {
> > > cs = CPU(core->threads[i]);
> > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > index 2f7dc3c23ded..17741a3fb77f 100644
> > > --- a/hw/ppc/spapr_cpu_core.c
> > > +++ b/hw/ppc/spapr_cpu_core.c
> > > @@ -27,6 +27,7 @@
> > >
> > > static void spapr_reset_vcpu(PowerPCCPU *cpu)
> > > {
> > > + PowerPCCPU *first_ppc_cpu = POWERPC_CPU(first_cpu);
> > > CPUState *cs = CPU(cpu);
> > > CPUPPCState *env = &cpu->env;
> > > PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> > > @@ -69,6 +70,18 @@ static void spapr_reset_vcpu(PowerPCCPU *cpu)
> > > kvm_check_mmu(cpu, &error_fatal);
> > >
> > > spapr_irq_cpu_intc_reset(spapr, cpu);
> > > +
> > > + /*
> > > + * The boot CPU is only reset during machine reset : reset its
> > > + * compatibility mode to the machine default. For other CPUs,
> > > + * either cold plugged or hot plugged, set the compatibility mode
> > > + * to match the boot CPU, which was either set by the machine reset
> > > + * code or by CAS.
> > > + */
> > > + ppc_set_compat(cpu,
> > > + cpu == first_ppc_cpu ?
> > > + spapr->max_compat_pvr : first_ppc_cpu->compat_pvr,
> > > + &error_fatal);
> >
> > This assumes that when it is called for a non-boot CPU, it has already
> > been called for the boot CPU.. Are we certain that's guaranteed by
> > the sequence of reset calls during a full machine reset?
> >
>
> This happens to be the case. Basically because the boot CPU core
> is created (including registering its reset handler) first and
> qemu_devices_reset() calls handlers in the same order they were
> registered.
Right, I assumed it works for now, but it seems rather fragile, since
I'm not sure we're relying on guaranteed properties of the interface.
Is there any way we could at least assert() if things are called out
of order?
>
> > > }
> > >
> > > void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip,
> >
>
--
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] 30+ messages in thread* Re: [PATCH for-6.0 4/9] spapr: Set compat mode in spapr_reset_vcpu()
2020-11-25 2:39 ` David Gibson
@ 2020-11-25 9:51 ` Greg Kurz
2020-11-26 4:57 ` David Gibson
0 siblings, 1 reply; 30+ messages in thread
From: Greg Kurz @ 2020-11-25 9:51 UTC (permalink / raw)
To: David Gibson; +Cc: Igor Mammedov, qemu-ppc, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 6128 bytes --]
On Wed, 25 Nov 2020 13:39:47 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:
> On Mon, Nov 23, 2020 at 12:51:08PM +0100, Greg Kurz wrote:
> > On Mon, 23 Nov 2020 16:11:30 +1100
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >
> > > On Sat, Nov 21, 2020 at 12:42:03AM +0100, Greg Kurz wrote:
> > > > When it comes to resetting the compat mode of the vCPUS, there are
> > > > two situations to consider:
> > > > (1) machine reset should set the compat mode back to the machine default,
> > > > ie. spapr->max_compat_pvr
> > > > (2) hot plugged vCPUs should set their compat mode to mach the boot vCPU,
> > > > ie. POWERPC_CPU(first_cpu)->compat_pvr
> > > >
> > > > This is currently handled in two separate places: globally for all vCPUs
> > > > from the machine reset code for (1) and for each thread of a core from
> > > > the hotplug path for (2).
> > > >
> > > > Since the machine reset code already resets all vCPUs, starting with boot
> > > > vCPU, consolidate the logic in spapr_reset_vcpu(). Special case the boot
> > > > vCPU so that it resets its compat mode back to the machine default. Any
> > > > other vCPU just need to match the compat mode of the boot vCPU.
> > > >
> > > > Failing to set the compat mode during machine reset is a fatal error,
> > > > but not for hot plugged vCPUs. This is arguable because if we've been
> > > > able to set the boot vCPU compat mode at CAS or during machine reset,
> > > > it should definitely not fail for other vCPUs. Since spapr_reset_vcpu()
> > > > already has a fatal error path for kvm_check_mmu() failures, do the
> > > > same for ppc_set_compat().
> > > >
> > > > This gets rid of an error path in spapr_core_plug(). It will allow
> > > > further simplifications.
> > > >
> > > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > > ---
> > > > hw/ppc/spapr.c | 16 ----------------
> > > > hw/ppc/spapr_cpu_core.c | 13 +++++++++++++
> > > > 2 files changed, 13 insertions(+), 16 deletions(-)
> > > >
> > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > index f58f77389e8e..da7586f548df 100644
> > > > --- a/hw/ppc/spapr.c
> > > > +++ b/hw/ppc/spapr.c
> > > > @@ -1606,8 +1606,6 @@ static void spapr_machine_reset(MachineState *machine)
> > > > spapr_ovec_cleanup(spapr->ov5_cas);
> > > > spapr->ov5_cas = spapr_ovec_new();
> > > >
> > > > - ppc_set_compat_all(spapr->max_compat_pvr, &error_fatal);
> > > > -
> > > > /*
> > > > * This is fixing some of the default configuration of the XIVE
> > > > * devices. To be called after the reset of the machine devices.
> > > > @@ -3785,20 +3783,6 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > > >
> > > > core_slot->cpu = OBJECT(dev);
> > > >
> > > > - /*
> > > > - * Set compatibility mode to match the boot CPU, which was either set
> > > > - * by the machine reset code or by CAS.
> > > > - */
> > > > - if (hotplugged) {
> > > > - for (i = 0; i < cc->nr_threads; i++) {
> > > > - if (ppc_set_compat(core->threads[i],
> > > > - POWERPC_CPU(first_cpu)->compat_pvr,
> > > > - errp) < 0) {
> > > > - return;
> > > > - }
> > > > - }
> > > > - }
> > > > -
> > > > if (smc->pre_2_10_has_unused_icps) {
> > > > for (i = 0; i < cc->nr_threads; i++) {
> > > > cs = CPU(core->threads[i]);
> > > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > > index 2f7dc3c23ded..17741a3fb77f 100644
> > > > --- a/hw/ppc/spapr_cpu_core.c
> > > > +++ b/hw/ppc/spapr_cpu_core.c
> > > > @@ -27,6 +27,7 @@
> > > >
> > > > static void spapr_reset_vcpu(PowerPCCPU *cpu)
> > > > {
> > > > + PowerPCCPU *first_ppc_cpu = POWERPC_CPU(first_cpu);
> > > > CPUState *cs = CPU(cpu);
> > > > CPUPPCState *env = &cpu->env;
> > > > PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> > > > @@ -69,6 +70,18 @@ static void spapr_reset_vcpu(PowerPCCPU *cpu)
> > > > kvm_check_mmu(cpu, &error_fatal);
> > > >
> > > > spapr_irq_cpu_intc_reset(spapr, cpu);
> > > > +
> > > > + /*
> > > > + * The boot CPU is only reset during machine reset : reset its
> > > > + * compatibility mode to the machine default. For other CPUs,
> > > > + * either cold plugged or hot plugged, set the compatibility mode
> > > > + * to match the boot CPU, which was either set by the machine reset
> > > > + * code or by CAS.
> > > > + */
> > > > + ppc_set_compat(cpu,
> > > > + cpu == first_ppc_cpu ?
> > > > + spapr->max_compat_pvr : first_ppc_cpu->compat_pvr,
> > > > + &error_fatal);
> > >
> > > This assumes that when it is called for a non-boot CPU, it has already
> > > been called for the boot CPU.. Are we certain that's guaranteed by
> > > the sequence of reset calls during a full machine reset?
> > >
> >
> > This happens to be the case. Basically because the boot CPU core
> > is created (including registering its reset handler) first and
> > qemu_devices_reset() calls handlers in the same order they were
> > registered.
>
> Right, I assumed it works for now, but it seems rather fragile, since
> I'm not sure we're relying on guaranteed properties of the interface.
The reset handler interface is absolutely undocumented, so I guess we
have no formal guarantees at the present time. But since the current
implementation has the property, would it be acceptable to carve it
in stone with added documentation ? In the event of unlikely changes
to the reset handler logic, people would _just_ need to make sure
handlers are called in the same order they were registered.
> Is there any way we could at least assert() if things are called out
> of order?
>
Maybe. I'll look into it.
> >
> > > > }
> > > >
> > > > void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip,
> > >
> >
>
>
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH for-6.0 4/9] spapr: Set compat mode in spapr_reset_vcpu()
2020-11-25 9:51 ` Greg Kurz
@ 2020-11-26 4:57 ` David Gibson
2020-11-26 9:10 ` Greg Kurz
0 siblings, 1 reply; 30+ messages in thread
From: David Gibson @ 2020-11-26 4:57 UTC (permalink / raw)
To: Greg Kurz; +Cc: Igor Mammedov, qemu-ppc, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 6833 bytes --]
On Wed, Nov 25, 2020 at 10:51:05AM +0100, Greg Kurz wrote:
> On Wed, 25 Nov 2020 13:39:47 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
>
> > On Mon, Nov 23, 2020 at 12:51:08PM +0100, Greg Kurz wrote:
> > > On Mon, 23 Nov 2020 16:11:30 +1100
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >
> > > > On Sat, Nov 21, 2020 at 12:42:03AM +0100, Greg Kurz wrote:
> > > > > When it comes to resetting the compat mode of the vCPUS, there are
> > > > > two situations to consider:
> > > > > (1) machine reset should set the compat mode back to the machine default,
> > > > > ie. spapr->max_compat_pvr
> > > > > (2) hot plugged vCPUs should set their compat mode to mach the boot vCPU,
> > > > > ie. POWERPC_CPU(first_cpu)->compat_pvr
> > > > >
> > > > > This is currently handled in two separate places: globally for all vCPUs
> > > > > from the machine reset code for (1) and for each thread of a core from
> > > > > the hotplug path for (2).
> > > > >
> > > > > Since the machine reset code already resets all vCPUs, starting with boot
> > > > > vCPU, consolidate the logic in spapr_reset_vcpu(). Special case the boot
> > > > > vCPU so that it resets its compat mode back to the machine default. Any
> > > > > other vCPU just need to match the compat mode of the boot vCPU.
> > > > >
> > > > > Failing to set the compat mode during machine reset is a fatal error,
> > > > > but not for hot plugged vCPUs. This is arguable because if we've been
> > > > > able to set the boot vCPU compat mode at CAS or during machine reset,
> > > > > it should definitely not fail for other vCPUs. Since spapr_reset_vcpu()
> > > > > already has a fatal error path for kvm_check_mmu() failures, do the
> > > > > same for ppc_set_compat().
> > > > >
> > > > > This gets rid of an error path in spapr_core_plug(). It will allow
> > > > > further simplifications.
> > > > >
> > > > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > > > ---
> > > > > hw/ppc/spapr.c | 16 ----------------
> > > > > hw/ppc/spapr_cpu_core.c | 13 +++++++++++++
> > > > > 2 files changed, 13 insertions(+), 16 deletions(-)
> > > > >
> > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > > index f58f77389e8e..da7586f548df 100644
> > > > > --- a/hw/ppc/spapr.c
> > > > > +++ b/hw/ppc/spapr.c
> > > > > @@ -1606,8 +1606,6 @@ static void spapr_machine_reset(MachineState *machine)
> > > > > spapr_ovec_cleanup(spapr->ov5_cas);
> > > > > spapr->ov5_cas = spapr_ovec_new();
> > > > >
> > > > > - ppc_set_compat_all(spapr->max_compat_pvr, &error_fatal);
> > > > > -
> > > > > /*
> > > > > * This is fixing some of the default configuration of the XIVE
> > > > > * devices. To be called after the reset of the machine devices.
> > > > > @@ -3785,20 +3783,6 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > > > >
> > > > > core_slot->cpu = OBJECT(dev);
> > > > >
> > > > > - /*
> > > > > - * Set compatibility mode to match the boot CPU, which was either set
> > > > > - * by the machine reset code or by CAS.
> > > > > - */
> > > > > - if (hotplugged) {
> > > > > - for (i = 0; i < cc->nr_threads; i++) {
> > > > > - if (ppc_set_compat(core->threads[i],
> > > > > - POWERPC_CPU(first_cpu)->compat_pvr,
> > > > > - errp) < 0) {
> > > > > - return;
> > > > > - }
> > > > > - }
> > > > > - }
> > > > > -
> > > > > if (smc->pre_2_10_has_unused_icps) {
> > > > > for (i = 0; i < cc->nr_threads; i++) {
> > > > > cs = CPU(core->threads[i]);
> > > > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > > > index 2f7dc3c23ded..17741a3fb77f 100644
> > > > > --- a/hw/ppc/spapr_cpu_core.c
> > > > > +++ b/hw/ppc/spapr_cpu_core.c
> > > > > @@ -27,6 +27,7 @@
> > > > >
> > > > > static void spapr_reset_vcpu(PowerPCCPU *cpu)
> > > > > {
> > > > > + PowerPCCPU *first_ppc_cpu = POWERPC_CPU(first_cpu);
> > > > > CPUState *cs = CPU(cpu);
> > > > > CPUPPCState *env = &cpu->env;
> > > > > PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> > > > > @@ -69,6 +70,18 @@ static void spapr_reset_vcpu(PowerPCCPU *cpu)
> > > > > kvm_check_mmu(cpu, &error_fatal);
> > > > >
> > > > > spapr_irq_cpu_intc_reset(spapr, cpu);
> > > > > +
> > > > > + /*
> > > > > + * The boot CPU is only reset during machine reset : reset its
> > > > > + * compatibility mode to the machine default. For other CPUs,
> > > > > + * either cold plugged or hot plugged, set the compatibility mode
> > > > > + * to match the boot CPU, which was either set by the machine reset
> > > > > + * code or by CAS.
> > > > > + */
> > > > > + ppc_set_compat(cpu,
> > > > > + cpu == first_ppc_cpu ?
> > > > > + spapr->max_compat_pvr : first_ppc_cpu->compat_pvr,
> > > > > + &error_fatal);
> > > >
> > > > This assumes that when it is called for a non-boot CPU, it has already
> > > > been called for the boot CPU.. Are we certain that's guaranteed by
> > > > the sequence of reset calls during a full machine reset?
> > > >
> > >
> > > This happens to be the case. Basically because the boot CPU core
> > > is created (including registering its reset handler) first and
> > > qemu_devices_reset() calls handlers in the same order they were
> > > registered.
> >
> > Right, I assumed it works for now, but it seems rather fragile, since
> > I'm not sure we're relying on guaranteed properties of the interface.
>
> The reset handler interface is absolutely undocumented, so I guess we
> have no formal guarantees at the present time. But since the current
> implementation has the property, would it be acceptable to carve it
> in stone with added documentation ? In the event of unlikely changes
> to the reset handler logic, people would _just_ need to make sure
> handlers are called in the same order they were registered.
Yeah, maybe.
One other thing occurs to me: will we still do things in the right
order if the (initial) boot cpu is hot unplugged, then replugged
before a reset?
> > Is there any way we could at least assert() if things are called out
> > of order?
> >
>
> Maybe. I'll look into it.
>
> > >
> > > > > }
> > > > >
> > > > > void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip,
> > > >
> > >
> >
> >
> >
>
--
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] 30+ messages in thread* Re: [PATCH for-6.0 4/9] spapr: Set compat mode in spapr_reset_vcpu()
2020-11-26 4:57 ` David Gibson
@ 2020-11-26 9:10 ` Greg Kurz
2020-11-26 16:23 ` Igor Mammedov
2020-11-27 4:59 ` David Gibson
0 siblings, 2 replies; 30+ messages in thread
From: Greg Kurz @ 2020-11-26 9:10 UTC (permalink / raw)
To: David Gibson; +Cc: Igor Mammedov, qemu-ppc, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 7913 bytes --]
On Thu, 26 Nov 2020 15:57:37 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:
> On Wed, Nov 25, 2020 at 10:51:05AM +0100, Greg Kurz wrote:
> > On Wed, 25 Nov 2020 13:39:47 +1100
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >
> > > On Mon, Nov 23, 2020 at 12:51:08PM +0100, Greg Kurz wrote:
> > > > On Mon, 23 Nov 2020 16:11:30 +1100
> > > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > > >
> > > > > On Sat, Nov 21, 2020 at 12:42:03AM +0100, Greg Kurz wrote:
> > > > > > When it comes to resetting the compat mode of the vCPUS, there are
> > > > > > two situations to consider:
> > > > > > (1) machine reset should set the compat mode back to the machine default,
> > > > > > ie. spapr->max_compat_pvr
> > > > > > (2) hot plugged vCPUs should set their compat mode to mach the boot vCPU,
> > > > > > ie. POWERPC_CPU(first_cpu)->compat_pvr
> > > > > >
> > > > > > This is currently handled in two separate places: globally for all vCPUs
> > > > > > from the machine reset code for (1) and for each thread of a core from
> > > > > > the hotplug path for (2).
> > > > > >
> > > > > > Since the machine reset code already resets all vCPUs, starting with boot
> > > > > > vCPU, consolidate the logic in spapr_reset_vcpu(). Special case the boot
> > > > > > vCPU so that it resets its compat mode back to the machine default. Any
> > > > > > other vCPU just need to match the compat mode of the boot vCPU.
> > > > > >
> > > > > > Failing to set the compat mode during machine reset is a fatal error,
> > > > > > but not for hot plugged vCPUs. This is arguable because if we've been
> > > > > > able to set the boot vCPU compat mode at CAS or during machine reset,
> > > > > > it should definitely not fail for other vCPUs. Since spapr_reset_vcpu()
> > > > > > already has a fatal error path for kvm_check_mmu() failures, do the
> > > > > > same for ppc_set_compat().
> > > > > >
> > > > > > This gets rid of an error path in spapr_core_plug(). It will allow
> > > > > > further simplifications.
> > > > > >
> > > > > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > > > > ---
> > > > > > hw/ppc/spapr.c | 16 ----------------
> > > > > > hw/ppc/spapr_cpu_core.c | 13 +++++++++++++
> > > > > > 2 files changed, 13 insertions(+), 16 deletions(-)
> > > > > >
> > > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > > > index f58f77389e8e..da7586f548df 100644
> > > > > > --- a/hw/ppc/spapr.c
> > > > > > +++ b/hw/ppc/spapr.c
> > > > > > @@ -1606,8 +1606,6 @@ static void spapr_machine_reset(MachineState *machine)
> > > > > > spapr_ovec_cleanup(spapr->ov5_cas);
> > > > > > spapr->ov5_cas = spapr_ovec_new();
> > > > > >
> > > > > > - ppc_set_compat_all(spapr->max_compat_pvr, &error_fatal);
> > > > > > -
> > > > > > /*
> > > > > > * This is fixing some of the default configuration of the XIVE
> > > > > > * devices. To be called after the reset of the machine devices.
> > > > > > @@ -3785,20 +3783,6 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > > > > >
> > > > > > core_slot->cpu = OBJECT(dev);
> > > > > >
> > > > > > - /*
> > > > > > - * Set compatibility mode to match the boot CPU, which was either set
> > > > > > - * by the machine reset code or by CAS.
> > > > > > - */
> > > > > > - if (hotplugged) {
> > > > > > - for (i = 0; i < cc->nr_threads; i++) {
> > > > > > - if (ppc_set_compat(core->threads[i],
> > > > > > - POWERPC_CPU(first_cpu)->compat_pvr,
> > > > > > - errp) < 0) {
> > > > > > - return;
> > > > > > - }
> > > > > > - }
> > > > > > - }
> > > > > > -
> > > > > > if (smc->pre_2_10_has_unused_icps) {
> > > > > > for (i = 0; i < cc->nr_threads; i++) {
> > > > > > cs = CPU(core->threads[i]);
> > > > > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > > > > index 2f7dc3c23ded..17741a3fb77f 100644
> > > > > > --- a/hw/ppc/spapr_cpu_core.c
> > > > > > +++ b/hw/ppc/spapr_cpu_core.c
> > > > > > @@ -27,6 +27,7 @@
> > > > > >
> > > > > > static void spapr_reset_vcpu(PowerPCCPU *cpu)
> > > > > > {
> > > > > > + PowerPCCPU *first_ppc_cpu = POWERPC_CPU(first_cpu);
> > > > > > CPUState *cs = CPU(cpu);
> > > > > > CPUPPCState *env = &cpu->env;
> > > > > > PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> > > > > > @@ -69,6 +70,18 @@ static void spapr_reset_vcpu(PowerPCCPU *cpu)
> > > > > > kvm_check_mmu(cpu, &error_fatal);
> > > > > >
> > > > > > spapr_irq_cpu_intc_reset(spapr, cpu);
> > > > > > +
> > > > > > + /*
> > > > > > + * The boot CPU is only reset during machine reset : reset its
> > > > > > + * compatibility mode to the machine default. For other CPUs,
> > > > > > + * either cold plugged or hot plugged, set the compatibility mode
> > > > > > + * to match the boot CPU, which was either set by the machine reset
> > > > > > + * code or by CAS.
> > > > > > + */
> > > > > > + ppc_set_compat(cpu,
> > > > > > + cpu == first_ppc_cpu ?
> > > > > > + spapr->max_compat_pvr : first_ppc_cpu->compat_pvr,
> > > > > > + &error_fatal);
> > > > >
> > > > > This assumes that when it is called for a non-boot CPU, it has already
> > > > > been called for the boot CPU.. Are we certain that's guaranteed by
> > > > > the sequence of reset calls during a full machine reset?
> > > > >
> > > >
> > > > This happens to be the case. Basically because the boot CPU core
> > > > is created (including registering its reset handler) first and
> > > > qemu_devices_reset() calls handlers in the same order they were
> > > > registered.
> > >
> > > Right, I assumed it works for now, but it seems rather fragile, since
> > > I'm not sure we're relying on guaranteed properties of the interface.
> >
> > The reset handler interface is absolutely undocumented, so I guess we
> > have no formal guarantees at the present time. But since the current
> > implementation has the property, would it be acceptable to carve it
> > in stone with added documentation ? In the event of unlikely changes
> > to the reset handler logic, people would _just_ need to make sure
> > handlers are called in the same order they were registered.
>
> Yeah, maybe.
>
> One other thing occurs to me: will we still do things in the right
> order if the (initial) boot cpu is hot unplugged, then replugged
> before a reset?
>
This can't happen AFAICT.
(qemu) qom-get /machine/unattached/device[1] core-id
0
(qemu) device_del /machine/unattached/device[1]
Error: Boot CPU core may not be unplugged
commit 62be8b044adf47327ebefdefb25f28a40316ebd0
Author: Bharata B Rao <bharata@linux.vnet.ibm.com>
Date: Wed Jul 27 10:44:42 2016 +0530
spapr: Prevent boot CPU core removal
So yes, this adds yet another road block on the way to support hot
unplug of the boot CPU. Is this a concern ?
If we go forward with this patch, maybe I should mention in the
changelog/documentation the various assumptions which this patch
is made under:
- reset handlers are called in the same order they were registered
- boot CPU registers its reset handler before other CPUs
- boot CPU cannot be hot unplugged
These guarantee that the boot core is always reset before other
cores during reset.
> > > Is there any way we could at least assert() if things are called out
> > > of order?
> > >
> >
> > Maybe. I'll look into it.
> >
> > > >
> > > > > > }
> > > > > >
> > > > > > void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip,
> > > > >
> > > >
> > >
> > >
> > >
> >
>
>
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH for-6.0 4/9] spapr: Set compat mode in spapr_reset_vcpu()
2020-11-26 9:10 ` Greg Kurz
@ 2020-11-26 16:23 ` Igor Mammedov
2020-11-26 20:53 ` Igor Mammedov
2020-11-27 4:59 ` David Gibson
1 sibling, 1 reply; 30+ messages in thread
From: Igor Mammedov @ 2020-11-26 16:23 UTC (permalink / raw)
To: Greg Kurz; +Cc: qemu-ppc, qemu-devel, David Gibson
On Thu, 26 Nov 2020 10:10:27 +0100
Greg Kurz <groug@kaod.org> wrote:
> On Thu, 26 Nov 2020 15:57:37 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
>
> > On Wed, Nov 25, 2020 at 10:51:05AM +0100, Greg Kurz wrote:
> > > On Wed, 25 Nov 2020 13:39:47 +1100
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >
> > > > On Mon, Nov 23, 2020 at 12:51:08PM +0100, Greg Kurz wrote:
> > > > > On Mon, 23 Nov 2020 16:11:30 +1100
> > > > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > > > >
> > > > > > On Sat, Nov 21, 2020 at 12:42:03AM +0100, Greg Kurz wrote:
> > > > > > > When it comes to resetting the compat mode of the vCPUS, there are
> > > > > > > two situations to consider:
> > > > > > > (1) machine reset should set the compat mode back to the machine default,
> > > > > > > ie. spapr->max_compat_pvr
> > > > > > > (2) hot plugged vCPUs should set their compat mode to mach the boot vCPU,
> > > > > > > ie. POWERPC_CPU(first_cpu)->compat_pvr
> > > > > > >
> > > > > > > This is currently handled in two separate places: globally for all vCPUs
> > > > > > > from the machine reset code for (1) and for each thread of a core from
> > > > > > > the hotplug path for (2).
> > > > > > >
> > > > > > > Since the machine reset code already resets all vCPUs, starting with boot
> > > > > > > vCPU, consolidate the logic in spapr_reset_vcpu(). Special case the boot
> > > > > > > vCPU so that it resets its compat mode back to the machine default. Any
> > > > > > > other vCPU just need to match the compat mode of the boot vCPU.
> > > > > > >
> > > > > > > Failing to set the compat mode during machine reset is a fatal error,
> > > > > > > but not for hot plugged vCPUs. This is arguable because if we've been
> > > > > > > able to set the boot vCPU compat mode at CAS or during machine reset,
> > > > > > > it should definitely not fail for other vCPUs. Since spapr_reset_vcpu()
> > > > > > > already has a fatal error path for kvm_check_mmu() failures, do the
> > > > > > > same for ppc_set_compat().
> > > > > > >
> > > > > > > This gets rid of an error path in spapr_core_plug(). It will allow
> > > > > > > further simplifications.
> > > > > > >
> > > > > > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > > > > > ---
> > > > > > > hw/ppc/spapr.c | 16 ----------------
> > > > > > > hw/ppc/spapr_cpu_core.c | 13 +++++++++++++
> > > > > > > 2 files changed, 13 insertions(+), 16 deletions(-)
> > > > > > >
> > > > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > > > > index f58f77389e8e..da7586f548df 100644
> > > > > > > --- a/hw/ppc/spapr.c
> > > > > > > +++ b/hw/ppc/spapr.c
> > > > > > > @@ -1606,8 +1606,6 @@ static void spapr_machine_reset(MachineState *machine)
> > > > > > > spapr_ovec_cleanup(spapr->ov5_cas);
> > > > > > > spapr->ov5_cas = spapr_ovec_new();
> > > > > > >
> > > > > > > - ppc_set_compat_all(spapr->max_compat_pvr, &error_fatal);
> > > > > > > -
> > > > > > > /*
> > > > > > > * This is fixing some of the default configuration of the XIVE
> > > > > > > * devices. To be called after the reset of the machine devices.
> > > > > > > @@ -3785,20 +3783,6 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > > > > > >
> > > > > > > core_slot->cpu = OBJECT(dev);
> > > > > > >
> > > > > > > - /*
> > > > > > > - * Set compatibility mode to match the boot CPU, which was either set
> > > > > > > - * by the machine reset code or by CAS.
> > > > > > > - */
> > > > > > > - if (hotplugged) {
> > > > > > > - for (i = 0; i < cc->nr_threads; i++) {
> > > > > > > - if (ppc_set_compat(core->threads[i],
> > > > > > > - POWERPC_CPU(first_cpu)->compat_pvr,
> > > > > > > - errp) < 0) {
> > > > > > > - return;
> > > > > > > - }
> > > > > > > - }
> > > > > > > - }
> > > > > > > -
> > > > > > > if (smc->pre_2_10_has_unused_icps) {
> > > > > > > for (i = 0; i < cc->nr_threads; i++) {
> > > > > > > cs = CPU(core->threads[i]);
> > > > > > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > > > > > index 2f7dc3c23ded..17741a3fb77f 100644
> > > > > > > --- a/hw/ppc/spapr_cpu_core.c
> > > > > > > +++ b/hw/ppc/spapr_cpu_core.c
> > > > > > > @@ -27,6 +27,7 @@
> > > > > > >
> > > > > > > static void spapr_reset_vcpu(PowerPCCPU *cpu)
> > > > > > > {
> > > > > > > + PowerPCCPU *first_ppc_cpu = POWERPC_CPU(first_cpu);
> > > > > > > CPUState *cs = CPU(cpu);
> > > > > > > CPUPPCState *env = &cpu->env;
> > > > > > > PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> > > > > > > @@ -69,6 +70,18 @@ static void spapr_reset_vcpu(PowerPCCPU *cpu)
> > > > > > > kvm_check_mmu(cpu, &error_fatal);
> > > > > > >
> > > > > > > spapr_irq_cpu_intc_reset(spapr, cpu);
> > > > > > > +
> > > > > > > + /*
> > > > > > > + * The boot CPU is only reset during machine reset : reset its
> > > > > > > + * compatibility mode to the machine default. For other CPUs,
> > > > > > > + * either cold plugged or hot plugged, set the compatibility mode
> > > > > > > + * to match the boot CPU, which was either set by the machine reset
> > > > > > > + * code or by CAS.
> > > > > > > + */
> > > > > > > + ppc_set_compat(cpu,
> > > > > > > + cpu == first_ppc_cpu ?
> > > > > > > + spapr->max_compat_pvr : first_ppc_cpu->compat_pvr,
> > > > > > > + &error_fatal);
> > > > > >
> > > > > > This assumes that when it is called for a non-boot CPU, it has already
> > > > > > been called for the boot CPU.. Are we certain that's guaranteed by
> > > > > > the sequence of reset calls during a full machine reset?
> > > > > >
> > > > >
> > > > > This happens to be the case. Basically because the boot CPU core
> > > > > is created (including registering its reset handler) first and
> > > > > qemu_devices_reset() calls handlers in the same order they were
> > > > > registered.
> > > >
> > > > Right, I assumed it works for now, but it seems rather fragile, since
> > > > I'm not sure we're relying on guaranteed properties of the interface.
> > >
> > > The reset handler interface is absolutely undocumented, so I guess we
> > > have no formal guarantees at the present time. But since the current
> > > implementation has the property, would it be acceptable to carve it
> > > in stone with added documentation ? In the event of unlikely changes
> > > to the reset handler logic, people would _just_ need to make sure
> > > handlers are called in the same order they were registered.
> >
> > Yeah, maybe.
> >
> > One other thing occurs to me: will we still do things in the right
> > order if the (initial) boot cpu is hot unplugged, then replugged
> > before a reset?
> >
>
> This can't happen AFAICT.
>
> (qemu) qom-get /machine/unattached/device[1] core-id
> 0
> (qemu) device_del /machine/unattached/device[1]
> Error: Boot CPU core may not be unplugged
>
> commit 62be8b044adf47327ebefdefb25f28a40316ebd0
> Author: Bharata B Rao <bharata@linux.vnet.ibm.com>
> Date: Wed Jul 27 10:44:42 2016 +0530
>
> spapr: Prevent boot CPU core removal
>
>
> So yes, this adds yet another road block on the way to support hot
> unplug of the boot CPU. Is this a concern ?
>
> If we go forward with this patch, maybe I should mention in the
> changelog/documentation the various assumptions which this patch
> is made under:
> - reset handlers are called in the same order they were registered
> - boot CPU registers its reset handler before other CPUs
> - boot CPU cannot be hot unplugged
>
> These guarantee that the boot core is always reset before other
> cores during reset.
it might work for now but it seems fragile to me.
What if we make compat mode a property and move setting it to machine code,
more precisely treat it like any other cpu feature property.
if(need_compat_more)
register_global_property(compat_mode)
that way when any cpu is created it will have this property set
and it won't depend on the order CPUs are created/reset
>
> > > > Is there any way we could at least assert() if things are called out
> > > > of order?
> > > >
> > >
> > > Maybe. I'll look into it.
> > >
> > > > >
> > > > > > > }
> > > > > > >
> > > > > > > void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip,
> > > > > >
> > > > >
> > > >
> > > >
> > > >
> > >
> >
> >
> >
>
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH for-6.0 4/9] spapr: Set compat mode in spapr_reset_vcpu()
2020-11-26 16:23 ` Igor Mammedov
@ 2020-11-26 20:53 ` Igor Mammedov
0 siblings, 0 replies; 30+ messages in thread
From: Igor Mammedov @ 2020-11-26 20:53 UTC (permalink / raw)
To: Greg Kurz; +Cc: qemu-ppc, qemu-devel, David Gibson
On Thu, 26 Nov 2020 17:23:20 +0100
Igor Mammedov <imammedo@redhat.com> wrote:
> On Thu, 26 Nov 2020 10:10:27 +0100
> Greg Kurz <groug@kaod.org> wrote:
>
> > On Thu, 26 Nov 2020 15:57:37 +1100
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >
> > > On Wed, Nov 25, 2020 at 10:51:05AM +0100, Greg Kurz wrote:
> > > > On Wed, 25 Nov 2020 13:39:47 +1100
> > > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > > >
> > > > > On Mon, Nov 23, 2020 at 12:51:08PM +0100, Greg Kurz wrote:
> > > > > > On Mon, 23 Nov 2020 16:11:30 +1100
> > > > > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > > > > >
> > > > > > > On Sat, Nov 21, 2020 at 12:42:03AM +0100, Greg Kurz wrote:
> > > > > > > > When it comes to resetting the compat mode of the vCPUS, there are
> > > > > > > > two situations to consider:
> > > > > > > > (1) machine reset should set the compat mode back to the machine default,
> > > > > > > > ie. spapr->max_compat_pvr
> > > > > > > > (2) hot plugged vCPUs should set their compat mode to mach the boot vCPU,
> > > > > > > > ie. POWERPC_CPU(first_cpu)->compat_pvr
> > > > > > > >
> > > > > > > > This is currently handled in two separate places: globally for all vCPUs
> > > > > > > > from the machine reset code for (1) and for each thread of a core from
> > > > > > > > the hotplug path for (2).
> > > > > > > >
> > > > > > > > Since the machine reset code already resets all vCPUs, starting with boot
> > > > > > > > vCPU, consolidate the logic in spapr_reset_vcpu(). Special case the boot
> > > > > > > > vCPU so that it resets its compat mode back to the machine default. Any
> > > > > > > > other vCPU just need to match the compat mode of the boot vCPU.
> > > > > > > >
> > > > > > > > Failing to set the compat mode during machine reset is a fatal error,
> > > > > > > > but not for hot plugged vCPUs. This is arguable because if we've been
> > > > > > > > able to set the boot vCPU compat mode at CAS or during machine reset,
> > > > > > > > it should definitely not fail for other vCPUs. Since spapr_reset_vcpu()
> > > > > > > > already has a fatal error path for kvm_check_mmu() failures, do the
> > > > > > > > same for ppc_set_compat().
> > > > > > > >
> > > > > > > > This gets rid of an error path in spapr_core_plug(). It will allow
> > > > > > > > further simplifications.
> > > > > > > >
> > > > > > > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > > > > > > ---
> > > > > > > > hw/ppc/spapr.c | 16 ----------------
> > > > > > > > hw/ppc/spapr_cpu_core.c | 13 +++++++++++++
> > > > > > > > 2 files changed, 13 insertions(+), 16 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > > > > > index f58f77389e8e..da7586f548df 100644
> > > > > > > > --- a/hw/ppc/spapr.c
> > > > > > > > +++ b/hw/ppc/spapr.c
> > > > > > > > @@ -1606,8 +1606,6 @@ static void spapr_machine_reset(MachineState *machine)
> > > > > > > > spapr_ovec_cleanup(spapr->ov5_cas);
> > > > > > > > spapr->ov5_cas = spapr_ovec_new();
> > > > > > > >
> > > > > > > > - ppc_set_compat_all(spapr->max_compat_pvr, &error_fatal);
> > > > > > > > -
> > > > > > > > /*
> > > > > > > > * This is fixing some of the default configuration of the XIVE
> > > > > > > > * devices. To be called after the reset of the machine devices.
> > > > > > > > @@ -3785,20 +3783,6 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > > > > > > >
> > > > > > > > core_slot->cpu = OBJECT(dev);
> > > > > > > >
> > > > > > > > - /*
> > > > > > > > - * Set compatibility mode to match the boot CPU, which was either set
> > > > > > > > - * by the machine reset code or by CAS.
> > > > > > > > - */
> > > > > > > > - if (hotplugged) {
> > > > > > > > - for (i = 0; i < cc->nr_threads; i++) {
> > > > > > > > - if (ppc_set_compat(core->threads[i],
> > > > > > > > - POWERPC_CPU(first_cpu)->compat_pvr,
> > > > > > > > - errp) < 0) {
> > > > > > > > - return;
> > > > > > > > - }
> > > > > > > > - }
> > > > > > > > - }
> > > > > > > > -
> > > > > > > > if (smc->pre_2_10_has_unused_icps) {
> > > > > > > > for (i = 0; i < cc->nr_threads; i++) {
> > > > > > > > cs = CPU(core->threads[i]);
> > > > > > > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > > > > > > index 2f7dc3c23ded..17741a3fb77f 100644
> > > > > > > > --- a/hw/ppc/spapr_cpu_core.c
> > > > > > > > +++ b/hw/ppc/spapr_cpu_core.c
> > > > > > > > @@ -27,6 +27,7 @@
> > > > > > > >
> > > > > > > > static void spapr_reset_vcpu(PowerPCCPU *cpu)
> > > > > > > > {
> > > > > > > > + PowerPCCPU *first_ppc_cpu = POWERPC_CPU(first_cpu);
> > > > > > > > CPUState *cs = CPU(cpu);
> > > > > > > > CPUPPCState *env = &cpu->env;
> > > > > > > > PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> > > > > > > > @@ -69,6 +70,18 @@ static void spapr_reset_vcpu(PowerPCCPU *cpu)
> > > > > > > > kvm_check_mmu(cpu, &error_fatal);
> > > > > > > >
> > > > > > > > spapr_irq_cpu_intc_reset(spapr, cpu);
> > > > > > > > +
> > > > > > > > + /*
> > > > > > > > + * The boot CPU is only reset during machine reset : reset its
> > > > > > > > + * compatibility mode to the machine default. For other CPUs,
> > > > > > > > + * either cold plugged or hot plugged, set the compatibility mode
> > > > > > > > + * to match the boot CPU, which was either set by the machine reset
> > > > > > > > + * code or by CAS.
> > > > > > > > + */
> > > > > > > > + ppc_set_compat(cpu,
> > > > > > > > + cpu == first_ppc_cpu ?
> > > > > > > > + spapr->max_compat_pvr : first_ppc_cpu->compat_pvr,
> > > > > > > > + &error_fatal);
> > > > > > >
> > > > > > > This assumes that when it is called for a non-boot CPU, it has already
> > > > > > > been called for the boot CPU.. Are we certain that's guaranteed by
> > > > > > > the sequence of reset calls during a full machine reset?
> > > > > > >
> > > > > >
> > > > > > This happens to be the case. Basically because the boot CPU core
> > > > > > is created (including registering its reset handler) first and
> > > > > > qemu_devices_reset() calls handlers in the same order they were
> > > > > > registered.
> > > > >
> > > > > Right, I assumed it works for now, but it seems rather fragile, since
> > > > > I'm not sure we're relying on guaranteed properties of the interface.
> > > >
> > > > The reset handler interface is absolutely undocumented, so I guess we
> > > > have no formal guarantees at the present time. But since the current
> > > > implementation has the property, would it be acceptable to carve it
> > > > in stone with added documentation ? In the event of unlikely changes
> > > > to the reset handler logic, people would _just_ need to make sure
> > > > handlers are called in the same order they were registered.
> > >
> > > Yeah, maybe.
> > >
> > > One other thing occurs to me: will we still do things in the right
> > > order if the (initial) boot cpu is hot unplugged, then replugged
> > > before a reset?
> > >
> >
> > This can't happen AFAICT.
> >
> > (qemu) qom-get /machine/unattached/device[1] core-id
> > 0
> > (qemu) device_del /machine/unattached/device[1]
> > Error: Boot CPU core may not be unplugged
> >
> > commit 62be8b044adf47327ebefdefb25f28a40316ebd0
> > Author: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > Date: Wed Jul 27 10:44:42 2016 +0530
> >
> > spapr: Prevent boot CPU core removal
> >
> >
> > So yes, this adds yet another road block on the way to support hot
> > unplug of the boot CPU. Is this a concern ?
> >
> > If we go forward with this patch, maybe I should mention in the
> > changelog/documentation the various assumptions which this patch
> > is made under:
> > - reset handlers are called in the same order they were registered
> > - boot CPU registers its reset handler before other CPUs
> > - boot CPU cannot be hot unplugged
> >
> > These guarantee that the boot core is always reset before other
> > cores during reset.
> it might work for now but it seems fragile to me.
>
> What if we make compat mode a property and move setting it to machine code,
> more precisely treat it like any other cpu feature property.
>
> if(need_compat_more)
> register_global_property(compat_mode)
>
> that way when any cpu is created it will have this property set
> and it won't depend on the order CPUs are created/reset
Ah it's more complicated, ignore this nonsense pls.
>
> >
> > > > > Is there any way we could at least assert() if things are called out
> > > > > of order?
> > > > >
> > > >
> > > > Maybe. I'll look into it.
> > > >
> > > > > >
> > > > > > > > }
> > > > > > > >
> > > > > > > > void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip,
> > > > > > >
> > > > > >
> > > > >
> > > > >
> > > > >
> > > >
> > >
> > >
> > >
> >
>
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH for-6.0 4/9] spapr: Set compat mode in spapr_reset_vcpu()
2020-11-26 9:10 ` Greg Kurz
2020-11-26 16:23 ` Igor Mammedov
@ 2020-11-27 4:59 ` David Gibson
2020-11-27 13:25 ` Greg Kurz
1 sibling, 1 reply; 30+ messages in thread
From: David Gibson @ 2020-11-27 4:59 UTC (permalink / raw)
To: Greg Kurz; +Cc: Igor Mammedov, qemu-ppc, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 8572 bytes --]
On Thu, Nov 26, 2020 at 10:10:27AM +0100, Greg Kurz wrote:
> On Thu, 26 Nov 2020 15:57:37 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
>
> > On Wed, Nov 25, 2020 at 10:51:05AM +0100, Greg Kurz wrote:
> > > On Wed, 25 Nov 2020 13:39:47 +1100
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >
> > > > On Mon, Nov 23, 2020 at 12:51:08PM +0100, Greg Kurz wrote:
> > > > > On Mon, 23 Nov 2020 16:11:30 +1100
> > > > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > > > >
> > > > > > On Sat, Nov 21, 2020 at 12:42:03AM +0100, Greg Kurz wrote:
> > > > > > > When it comes to resetting the compat mode of the vCPUS, there are
> > > > > > > two situations to consider:
> > > > > > > (1) machine reset should set the compat mode back to the machine default,
> > > > > > > ie. spapr->max_compat_pvr
> > > > > > > (2) hot plugged vCPUs should set their compat mode to mach the boot vCPU,
> > > > > > > ie. POWERPC_CPU(first_cpu)->compat_pvr
> > > > > > >
> > > > > > > This is currently handled in two separate places: globally for all vCPUs
> > > > > > > from the machine reset code for (1) and for each thread of a core from
> > > > > > > the hotplug path for (2).
> > > > > > >
> > > > > > > Since the machine reset code already resets all vCPUs, starting with boot
> > > > > > > vCPU, consolidate the logic in spapr_reset_vcpu(). Special case the boot
> > > > > > > vCPU so that it resets its compat mode back to the machine default. Any
> > > > > > > other vCPU just need to match the compat mode of the boot vCPU.
> > > > > > >
> > > > > > > Failing to set the compat mode during machine reset is a fatal error,
> > > > > > > but not for hot plugged vCPUs. This is arguable because if we've been
> > > > > > > able to set the boot vCPU compat mode at CAS or during machine reset,
> > > > > > > it should definitely not fail for other vCPUs. Since spapr_reset_vcpu()
> > > > > > > already has a fatal error path for kvm_check_mmu() failures, do the
> > > > > > > same for ppc_set_compat().
> > > > > > >
> > > > > > > This gets rid of an error path in spapr_core_plug(). It will allow
> > > > > > > further simplifications.
> > > > > > >
> > > > > > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > > > > > ---
> > > > > > > hw/ppc/spapr.c | 16 ----------------
> > > > > > > hw/ppc/spapr_cpu_core.c | 13 +++++++++++++
> > > > > > > 2 files changed, 13 insertions(+), 16 deletions(-)
> > > > > > >
> > > > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > > > > index f58f77389e8e..da7586f548df 100644
> > > > > > > --- a/hw/ppc/spapr.c
> > > > > > > +++ b/hw/ppc/spapr.c
> > > > > > > @@ -1606,8 +1606,6 @@ static void spapr_machine_reset(MachineState *machine)
> > > > > > > spapr_ovec_cleanup(spapr->ov5_cas);
> > > > > > > spapr->ov5_cas = spapr_ovec_new();
> > > > > > >
> > > > > > > - ppc_set_compat_all(spapr->max_compat_pvr, &error_fatal);
> > > > > > > -
> > > > > > > /*
> > > > > > > * This is fixing some of the default configuration of the XIVE
> > > > > > > * devices. To be called after the reset of the machine devices.
> > > > > > > @@ -3785,20 +3783,6 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > > > > > >
> > > > > > > core_slot->cpu = OBJECT(dev);
> > > > > > >
> > > > > > > - /*
> > > > > > > - * Set compatibility mode to match the boot CPU, which was either set
> > > > > > > - * by the machine reset code or by CAS.
> > > > > > > - */
> > > > > > > - if (hotplugged) {
> > > > > > > - for (i = 0; i < cc->nr_threads; i++) {
> > > > > > > - if (ppc_set_compat(core->threads[i],
> > > > > > > - POWERPC_CPU(first_cpu)->compat_pvr,
> > > > > > > - errp) < 0) {
> > > > > > > - return;
> > > > > > > - }
> > > > > > > - }
> > > > > > > - }
> > > > > > > -
> > > > > > > if (smc->pre_2_10_has_unused_icps) {
> > > > > > > for (i = 0; i < cc->nr_threads; i++) {
> > > > > > > cs = CPU(core->threads[i]);
> > > > > > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > > > > > index 2f7dc3c23ded..17741a3fb77f 100644
> > > > > > > --- a/hw/ppc/spapr_cpu_core.c
> > > > > > > +++ b/hw/ppc/spapr_cpu_core.c
> > > > > > > @@ -27,6 +27,7 @@
> > > > > > >
> > > > > > > static void spapr_reset_vcpu(PowerPCCPU *cpu)
> > > > > > > {
> > > > > > > + PowerPCCPU *first_ppc_cpu = POWERPC_CPU(first_cpu);
> > > > > > > CPUState *cs = CPU(cpu);
> > > > > > > CPUPPCState *env = &cpu->env;
> > > > > > > PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> > > > > > > @@ -69,6 +70,18 @@ static void spapr_reset_vcpu(PowerPCCPU *cpu)
> > > > > > > kvm_check_mmu(cpu, &error_fatal);
> > > > > > >
> > > > > > > spapr_irq_cpu_intc_reset(spapr, cpu);
> > > > > > > +
> > > > > > > + /*
> > > > > > > + * The boot CPU is only reset during machine reset : reset its
> > > > > > > + * compatibility mode to the machine default. For other CPUs,
> > > > > > > + * either cold plugged or hot plugged, set the compatibility mode
> > > > > > > + * to match the boot CPU, which was either set by the machine reset
> > > > > > > + * code or by CAS.
> > > > > > > + */
> > > > > > > + ppc_set_compat(cpu,
> > > > > > > + cpu == first_ppc_cpu ?
> > > > > > > + spapr->max_compat_pvr : first_ppc_cpu->compat_pvr,
> > > > > > > + &error_fatal);
> > > > > >
> > > > > > This assumes that when it is called for a non-boot CPU, it has already
> > > > > > been called for the boot CPU.. Are we certain that's guaranteed by
> > > > > > the sequence of reset calls during a full machine reset?
> > > > > >
> > > > >
> > > > > This happens to be the case. Basically because the boot CPU core
> > > > > is created (including registering its reset handler) first and
> > > > > qemu_devices_reset() calls handlers in the same order they were
> > > > > registered.
> > > >
> > > > Right, I assumed it works for now, but it seems rather fragile, since
> > > > I'm not sure we're relying on guaranteed properties of the interface.
> > >
> > > The reset handler interface is absolutely undocumented, so I guess we
> > > have no formal guarantees at the present time. But since the current
> > > implementation has the property, would it be acceptable to carve it
> > > in stone with added documentation ? In the event of unlikely changes
> > > to the reset handler logic, people would _just_ need to make sure
> > > handlers are called in the same order they were registered.
> >
> > Yeah, maybe.
> >
> > One other thing occurs to me: will we still do things in the right
> > order if the (initial) boot cpu is hot unplugged, then replugged
> > before a reset?
> >
>
> This can't happen AFAICT.
>
> (qemu) qom-get /machine/unattached/device[1] core-id
> 0
> (qemu) device_del /machine/unattached/device[1]
> Error: Boot CPU core may not be unplugged
>
> commit 62be8b044adf47327ebefdefb25f28a40316ebd0
> Author: Bharata B Rao <bharata@linux.vnet.ibm.com>
> Date: Wed Jul 27 10:44:42 2016 +0530
>
> spapr: Prevent boot CPU core removal
Oh yeah, I'd forgotten we did this.
>
> So yes, this adds yet another road block on the way to support hot
> unplug of the boot CPU. Is this a concern ?
>
> If we go forward with this patch, maybe I should mention in the
> changelog/documentation the various assumptions which this patch
> is made under:
> - reset handlers are called in the same order they were registered
> - boot CPU registers its reset handler before other CPUs
> - boot CPU cannot be hot unplugged
>
> These guarantee that the boot core is always reset before other
> cores during reset.
>
> > > > Is there any way we could at least assert() if things are called out
> > > > of order?
> > > >
> > >
> > > Maybe. I'll look into it.
> > >
> > > > >
> > > > > > > }
> > > > > > >
> > > > > > > void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip,
> > > > > >
> > > > >
> > > >
> > > >
> > > >
> > >
> >
> >
> >
>
--
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] 30+ messages in thread* Re: [PATCH for-6.0 4/9] spapr: Set compat mode in spapr_reset_vcpu()
2020-11-27 4:59 ` David Gibson
@ 2020-11-27 13:25 ` Greg Kurz
0 siblings, 0 replies; 30+ messages in thread
From: Greg Kurz @ 2020-11-27 13:25 UTC (permalink / raw)
To: David Gibson; +Cc: Igor Mammedov, qemu-ppc, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1159 bytes --]
On Fri, 27 Nov 2020 15:59:16 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:
[...]
> >
> > This can't happen AFAICT.
> >
> > (qemu) qom-get /machine/unattached/device[1] core-id
> > 0
> > (qemu) device_del /machine/unattached/device[1]
> > Error: Boot CPU core may not be unplugged
> >
> > commit 62be8b044adf47327ebefdefb25f28a40316ebd0
> > Author: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > Date: Wed Jul 27 10:44:42 2016 +0530
> >
> > spapr: Prevent boot CPU core removal
>
> Oh yeah, I'd forgotten we did this.
>
Anyway, both you and Igor noted that this change is fragile, which
is true. So maybe we should just go on with the current behavior.
As mentioned in the changelog, one of the motivation to do this was
to get rid of the error path in spapr_core_plug() like other patches
in this series do for the other plug handlers. And I guess I should
do it like these other patches do : come up with a check in
spapr_core_pre_plug() that guarantees that ppc_set_compat() shouldn't
fail in spapr_core_plug() and can be passed &error_abort.
So I'll try to do just that. You can forget this patch.
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH for-6.0 5/9] spapr: Simplify error path of spapr_core_plug()
2020-11-20 23:41 [PATCH for-6.0 0/9] spapr: Perform hotplug sanity checks at pre-plug Greg Kurz
` (3 preceding siblings ...)
2020-11-20 23:42 ` [PATCH for-6.0 4/9] spapr: Set compat mode in spapr_reset_vcpu() Greg Kurz
@ 2020-11-20 23:42 ` Greg Kurz
2020-11-23 5:13 ` David Gibson
2020-11-20 23:42 ` [PATCH for-6.0 6/9] spapr: Make PHB placement functions and spapr_pre_plug_phb() return status Greg Kurz
` (3 subsequent siblings)
8 siblings, 1 reply; 30+ messages in thread
From: Greg Kurz @ 2020-11-20 23:42 UTC (permalink / raw)
To: qemu-devel; +Cc: Igor Mammedov, qemu-ppc, Greg Kurz, David Gibson
spapr_core_pre_plug() already guarantees that the slot for the given core
ID is available. It is thus safe to assume that spapr_find_cpu_slot()
returns a slot during plug. Turn the error path into an assertion.
It is also safe to assume that no device is attached to the corresponding
DRC and that spapr_drc_attach() shouldn't fail.
Pass &error_abort to spapr_drc_attach() and simplify error handling.
Signed-off-by: Greg Kurz <groug@kaod.org>
---
hw/ppc/spapr.c | 21 ++++++++++-----------
1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index da7586f548df..cfca033c7b14 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3739,8 +3739,7 @@ int spapr_core_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr,
return 0;
}
-static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
- Error **errp)
+static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev)
{
SpaprMachineState *spapr = SPAPR_MACHINE(OBJECT(hotplug_dev));
MachineClass *mc = MACHINE_GET_CLASS(spapr);
@@ -3755,20 +3754,20 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
int i;
core_slot = spapr_find_cpu_slot(MACHINE(hotplug_dev), cc->core_id, &index);
- if (!core_slot) {
- error_setg(errp, "Unable to find CPU core with core-id: %d",
- cc->core_id);
- return;
- }
+ g_assert(core_slot); /* Already checked in spapr_core_pre_plug() */
+
drc = spapr_drc_by_id(TYPE_SPAPR_DRC_CPU,
spapr_vcpu_id(spapr, cc->core_id));
g_assert(drc || !mc->has_hotpluggable_cpus);
if (drc) {
- if (!spapr_drc_attach(drc, dev, errp)) {
- return;
- }
+ /*
+ * spapr_core_pre_plug() already buys us this is a brand new
+ * core being plugged into a free slot. Nothing should already
+ * be attached to the corresponding DRC.
+ */
+ spapr_drc_attach(drc, dev, &error_abort);
if (hotplugged) {
/*
@@ -3981,7 +3980,7 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
spapr_memory_plug(hotplug_dev, dev);
} else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
- spapr_core_plug(hotplug_dev, dev, errp);
+ spapr_core_plug(hotplug_dev, dev);
} else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_PCI_HOST_BRIDGE)) {
spapr_phb_plug(hotplug_dev, dev, errp);
} else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_TPM_PROXY)) {
--
2.26.2
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH for-6.0 5/9] spapr: Simplify error path of spapr_core_plug()
2020-11-20 23:42 ` [PATCH for-6.0 5/9] spapr: Simplify error path of spapr_core_plug() Greg Kurz
@ 2020-11-23 5:13 ` David Gibson
2020-11-24 13:07 ` Greg Kurz
0 siblings, 1 reply; 30+ messages in thread
From: David Gibson @ 2020-11-23 5:13 UTC (permalink / raw)
To: Greg Kurz; +Cc: Igor Mammedov, qemu-ppc, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 3145 bytes --]
On Sat, Nov 21, 2020 at 12:42:04AM +0100, Greg Kurz wrote:
> spapr_core_pre_plug() already guarantees that the slot for the given core
> ID is available. It is thus safe to assume that spapr_find_cpu_slot()
> returns a slot during plug. Turn the error path into an assertion.
> It is also safe to assume that no device is attached to the corresponding
> DRC and that spapr_drc_attach() shouldn't fail.
>
> Pass &error_abort to spapr_drc_attach() and simplify error handling.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
Applied to ppc-for-6.0, thanks.
> ---
> hw/ppc/spapr.c | 21 ++++++++++-----------
> 1 file changed, 10 insertions(+), 11 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index da7586f548df..cfca033c7b14 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3739,8 +3739,7 @@ int spapr_core_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr,
> return 0;
> }
>
> -static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> - Error **errp)
> +static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev)
> {
> SpaprMachineState *spapr = SPAPR_MACHINE(OBJECT(hotplug_dev));
> MachineClass *mc = MACHINE_GET_CLASS(spapr);
> @@ -3755,20 +3754,20 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> int i;
>
> core_slot = spapr_find_cpu_slot(MACHINE(hotplug_dev), cc->core_id, &index);
> - if (!core_slot) {
> - error_setg(errp, "Unable to find CPU core with core-id: %d",
> - cc->core_id);
> - return;
> - }
> + g_assert(core_slot); /* Already checked in spapr_core_pre_plug() */
> +
> drc = spapr_drc_by_id(TYPE_SPAPR_DRC_CPU,
> spapr_vcpu_id(spapr, cc->core_id));
>
> g_assert(drc || !mc->has_hotpluggable_cpus);
>
> if (drc) {
> - if (!spapr_drc_attach(drc, dev, errp)) {
> - return;
> - }
> + /*
> + * spapr_core_pre_plug() already buys us this is a brand new
> + * core being plugged into a free slot. Nothing should already
> + * be attached to the corresponding DRC.
> + */
> + spapr_drc_attach(drc, dev, &error_abort);
>
> if (hotplugged) {
> /*
> @@ -3981,7 +3980,7 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
> if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> spapr_memory_plug(hotplug_dev, dev);
> } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> - spapr_core_plug(hotplug_dev, dev, errp);
> + spapr_core_plug(hotplug_dev, dev);
> } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_PCI_HOST_BRIDGE)) {
> spapr_phb_plug(hotplug_dev, dev, errp);
> } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_TPM_PROXY)) {
--
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] 30+ messages in thread* Re: [PATCH for-6.0 5/9] spapr: Simplify error path of spapr_core_plug()
2020-11-23 5:13 ` David Gibson
@ 2020-11-24 13:07 ` Greg Kurz
2020-11-25 2:40 ` David Gibson
0 siblings, 1 reply; 30+ messages in thread
From: Greg Kurz @ 2020-11-24 13:07 UTC (permalink / raw)
To: David Gibson; +Cc: Igor Mammedov, qemu-ppc, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 3949 bytes --]
On Mon, 23 Nov 2020 16:13:18 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:
> On Sat, Nov 21, 2020 at 12:42:04AM +0100, Greg Kurz wrote:
> > spapr_core_pre_plug() already guarantees that the slot for the given core
> > ID is available. It is thus safe to assume that spapr_find_cpu_slot()
> > returns a slot during plug. Turn the error path into an assertion.
> > It is also safe to assume that no device is attached to the corresponding
> > DRC and that spapr_drc_attach() shouldn't fail.
> >
> > Pass &error_abort to spapr_drc_attach() and simplify error handling.
> >
> > Signed-off-by: Greg Kurz <groug@kaod.org>
>
> Applied to ppc-for-6.0, thanks.
>
This patch depends on the previous one.
> > ---
> > hw/ppc/spapr.c | 21 ++++++++++-----------
> > 1 file changed, 10 insertions(+), 11 deletions(-)
> >
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index da7586f548df..cfca033c7b14 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -3739,8 +3739,7 @@ int spapr_core_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr,
> > return 0;
> > }
> >
> > -static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > - Error **errp)
../../hw/ppc/spapr.c: In function ‘spapr_core_plug’:
../../hw/ppc/spapr.c:3802:32: error: ‘errp’ undeclared (first use in this function); did you mean ‘errno’?
errp) < 0) {
^~~~
errno
../../hw/ppc/spapr.c:3802:32: note: each undeclared identifier is reported only once for each function it appears in
Please either drop it from ppc-for-6.0 or possibly adapt spapr_core_plug()
to handle errors from ppc_set_compat().
<my 2 cents>
Since I can't see how this could fail for a hotplugged CPU if it
succeeded for the boot CPU, I'd pass &error_abort despite this
being a hotplug path.
</my 2 cents>
> > +static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev)
> > {
> > SpaprMachineState *spapr = SPAPR_MACHINE(OBJECT(hotplug_dev));
> > MachineClass *mc = MACHINE_GET_CLASS(spapr);
> > @@ -3755,20 +3754,20 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > int i;
> >
> > core_slot = spapr_find_cpu_slot(MACHINE(hotplug_dev), cc->core_id, &index);
> > - if (!core_slot) {
> > - error_setg(errp, "Unable to find CPU core with core-id: %d",
> > - cc->core_id);
> > - return;
> > - }
> > + g_assert(core_slot); /* Already checked in spapr_core_pre_plug() */
> > +
> > drc = spapr_drc_by_id(TYPE_SPAPR_DRC_CPU,
> > spapr_vcpu_id(spapr, cc->core_id));
> >
> > g_assert(drc || !mc->has_hotpluggable_cpus);
> >
> > if (drc) {
> > - if (!spapr_drc_attach(drc, dev, errp)) {
> > - return;
> > - }
> > + /*
> > + * spapr_core_pre_plug() already buys us this is a brand new
> > + * core being plugged into a free slot. Nothing should already
> > + * be attached to the corresponding DRC.
> > + */
> > + spapr_drc_attach(drc, dev, &error_abort);
> >
> > if (hotplugged) {
> > /*
> > @@ -3981,7 +3980,7 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
> > if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> > spapr_memory_plug(hotplug_dev, dev);
> > } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> > - spapr_core_plug(hotplug_dev, dev, errp);
> > + spapr_core_plug(hotplug_dev, dev);
> > } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_PCI_HOST_BRIDGE)) {
> > spapr_phb_plug(hotplug_dev, dev, errp);
> > } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_TPM_PROXY)) {
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH for-6.0 5/9] spapr: Simplify error path of spapr_core_plug()
2020-11-24 13:07 ` Greg Kurz
@ 2020-11-25 2:40 ` David Gibson
0 siblings, 0 replies; 30+ messages in thread
From: David Gibson @ 2020-11-25 2:40 UTC (permalink / raw)
To: Greg Kurz; +Cc: Igor Mammedov, qemu-ppc, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 4472 bytes --]
On Tue, Nov 24, 2020 at 02:07:27PM +0100, Greg Kurz wrote:
> On Mon, 23 Nov 2020 16:13:18 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
>
> > On Sat, Nov 21, 2020 at 12:42:04AM +0100, Greg Kurz wrote:
> > > spapr_core_pre_plug() already guarantees that the slot for the given core
> > > ID is available. It is thus safe to assume that spapr_find_cpu_slot()
> > > returns a slot during plug. Turn the error path into an assertion.
> > > It is also safe to assume that no device is attached to the corresponding
> > > DRC and that spapr_drc_attach() shouldn't fail.
> > >
> > > Pass &error_abort to spapr_drc_attach() and simplify error handling.
> > >
> > > Signed-off-by: Greg Kurz <groug@kaod.org>
> >
> > Applied to ppc-for-6.0, thanks.
> >
>
> This patch depends on the previous one.
>
> > > ---
> > > hw/ppc/spapr.c | 21 ++++++++++-----------
> > > 1 file changed, 10 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index da7586f548df..cfca033c7b14 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -3739,8 +3739,7 @@ int spapr_core_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr,
> > > return 0;
> > > }
> > >
> > > -static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > > - Error **errp)
>
> ../../hw/ppc/spapr.c: In function ‘spapr_core_plug’:
> ../../hw/ppc/spapr.c:3802:32: error: ‘errp’ undeclared (first use in this function); did you mean ‘errno’?
> errp) < 0) {
> ^~~~
> errno
> ../../hw/ppc/spapr.c:3802:32: note: each undeclared identifier is reported only once for each function it appears in
>
> Please either drop it from ppc-for-6.0 or possibly adapt spapr_core_plug()
> to handle errors from ppc_set_compat().
Dropped for now (along with a later patch that depended on this one).
>
> <my 2 cents>
> Since I can't see how this could fail for a hotplugged CPU if it
> succeeded for the boot CPU, I'd pass &error_abort despite this
> being a hotplug path.
> </my 2 cents>
>
> > > +static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev)
> > > {
> > > SpaprMachineState *spapr = SPAPR_MACHINE(OBJECT(hotplug_dev));
> > > MachineClass *mc = MACHINE_GET_CLASS(spapr);
> > > @@ -3755,20 +3754,20 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > > int i;
> > >
> > > core_slot = spapr_find_cpu_slot(MACHINE(hotplug_dev), cc->core_id, &index);
> > > - if (!core_slot) {
> > > - error_setg(errp, "Unable to find CPU core with core-id: %d",
> > > - cc->core_id);
> > > - return;
> > > - }
> > > + g_assert(core_slot); /* Already checked in spapr_core_pre_plug() */
> > > +
> > > drc = spapr_drc_by_id(TYPE_SPAPR_DRC_CPU,
> > > spapr_vcpu_id(spapr, cc->core_id));
> > >
> > > g_assert(drc || !mc->has_hotpluggable_cpus);
> > >
> > > if (drc) {
> > > - if (!spapr_drc_attach(drc, dev, errp)) {
> > > - return;
> > > - }
> > > + /*
> > > + * spapr_core_pre_plug() already buys us this is a brand new
> > > + * core being plugged into a free slot. Nothing should already
> > > + * be attached to the corresponding DRC.
> > > + */
> > > + spapr_drc_attach(drc, dev, &error_abort);
> > >
> > > if (hotplugged) {
> > > /*
> > > @@ -3981,7 +3980,7 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
> > > if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> > > spapr_memory_plug(hotplug_dev, dev);
> > > } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> > > - spapr_core_plug(hotplug_dev, dev, errp);
> > > + spapr_core_plug(hotplug_dev, dev);
> > > } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_PCI_HOST_BRIDGE)) {
> > > spapr_phb_plug(hotplug_dev, dev, errp);
> > > } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_TPM_PROXY)) {
> >
>
--
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] 30+ messages in thread
* [PATCH for-6.0 6/9] spapr: Make PHB placement functions and spapr_pre_plug_phb() return status
2020-11-20 23:41 [PATCH for-6.0 0/9] spapr: Perform hotplug sanity checks at pre-plug Greg Kurz
` (4 preceding siblings ...)
2020-11-20 23:42 ` [PATCH for-6.0 5/9] spapr: Simplify error path of spapr_core_plug() Greg Kurz
@ 2020-11-20 23:42 ` Greg Kurz
2020-11-23 5:14 ` David Gibson
2020-11-20 23:42 ` [PATCH for-6.0 7/9] spapr: Do PHB hoplug sanity check at pre-plug Greg Kurz
` (2 subsequent siblings)
8 siblings, 1 reply; 30+ messages in thread
From: Greg Kurz @ 2020-11-20 23:42 UTC (permalink / raw)
To: qemu-devel; +Cc: Igor Mammedov, qemu-ppc, Greg Kurz, David Gibson
Read documentation in "qapi/error.h" and changelog of commit
e3fe3988d785 ("error: Document Error API usage rules") for
rationale.
Signed-off-by: Greg Kurz <groug@kaod.org>
---
include/hw/ppc/spapr.h | 2 +-
hw/ppc/spapr.c | 40 +++++++++++++++++++++++-----------------
2 files changed, 24 insertions(+), 18 deletions(-)
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 2e89e36cfbdc..b7ced9faebf5 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -140,7 +140,7 @@ struct SpaprMachineClass {
bool pre_5_1_assoc_refpoints;
bool pre_5_2_numa_associativity;
- void (*phb_placement)(SpaprMachineState *spapr, uint32_t index,
+ bool (*phb_placement)(SpaprMachineState *spapr, uint32_t index,
uint64_t *buid, hwaddr *pio,
hwaddr *mmio32, hwaddr *mmio64,
unsigned n_dma, uint32_t *liobns, hwaddr *nv2gpa,
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index cfca033c7b14..81bac59887ab 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3865,7 +3865,7 @@ int spapr_phb_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr,
return 0;
}
-static void spapr_phb_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
+static bool spapr_phb_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
Error **errp)
{
SpaprMachineState *spapr = SPAPR_MACHINE(OBJECT(hotplug_dev));
@@ -3875,24 +3875,25 @@ static void spapr_phb_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
if (dev->hotplugged && !smc->dr_phb_enabled) {
error_setg(errp, "PHB hotplug not supported for this machine");
- return;
+ return false;
}
if (sphb->index == (uint32_t)-1) {
error_setg(errp, "\"index\" for PAPR PHB is mandatory");
- return;
+ return false;
}
/*
* This will check that sphb->index doesn't exceed the maximum number of
* PHBs for the current machine type.
*/
- smc->phb_placement(spapr, sphb->index,
- &sphb->buid, &sphb->io_win_addr,
- &sphb->mem_win_addr, &sphb->mem64_win_addr,
- windows_supported, sphb->dma_liobn,
- &sphb->nv2_gpa_win_addr, &sphb->nv2_atsd_win_addr,
- errp);
+ return
+ smc->phb_placement(spapr, sphb->index,
+ &sphb->buid, &sphb->io_win_addr,
+ &sphb->mem_win_addr, &sphb->mem64_win_addr,
+ windows_supported, sphb->dma_liobn,
+ &sphb->nv2_gpa_win_addr, &sphb->nv2_atsd_win_addr,
+ errp);
}
static void spapr_phb_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
@@ -4130,7 +4131,7 @@ static const CPUArchIdList *spapr_possible_cpu_arch_ids(MachineState *machine)
return machine->possible_cpus;
}
-static void spapr_phb_placement(SpaprMachineState *spapr, uint32_t index,
+static bool spapr_phb_placement(SpaprMachineState *spapr, uint32_t index,
uint64_t *buid, hwaddr *pio,
hwaddr *mmio32, hwaddr *mmio64,
unsigned n_dma, uint32_t *liobns,
@@ -4168,7 +4169,7 @@ static void spapr_phb_placement(SpaprMachineState *spapr, uint32_t index,
if (index >= SPAPR_MAX_PHBS) {
error_setg(errp, "\"index\" for PAPR PHB is too large (max %llu)",
SPAPR_MAX_PHBS - 1);
- return;
+ return false;
}
*buid = base_buid + index;
@@ -4182,6 +4183,7 @@ static void spapr_phb_placement(SpaprMachineState *spapr, uint32_t index,
*nv2gpa = SPAPR_PCI_NV2RAM64_WIN_BASE + index * SPAPR_PCI_NV2RAM64_WIN_SIZE;
*nv2atsd = SPAPR_PCI_NV2ATSD_WIN_BASE + index * SPAPR_PCI_NV2ATSD_WIN_SIZE;
+ return true;
}
static ICSState *spapr_ics_get(XICSFabric *dev, int irq)
@@ -4561,18 +4563,21 @@ DEFINE_SPAPR_MACHINE(4_1, "4.1", false);
/*
* pseries-4.0
*/
-static void phb_placement_4_0(SpaprMachineState *spapr, uint32_t index,
+static bool phb_placement_4_0(SpaprMachineState *spapr, uint32_t index,
uint64_t *buid, hwaddr *pio,
hwaddr *mmio32, hwaddr *mmio64,
unsigned n_dma, uint32_t *liobns,
hwaddr *nv2gpa, hwaddr *nv2atsd, Error **errp)
{
- spapr_phb_placement(spapr, index, buid, pio, mmio32, mmio64, n_dma, liobns,
- nv2gpa, nv2atsd, errp);
+ if (!spapr_phb_placement(spapr, index, buid, pio, mmio32, mmio64, n_dma,
+ liobns, nv2gpa, nv2atsd, errp)) {
+ return false;
+ }
+
*nv2gpa = 0;
*nv2atsd = 0;
+ return true;
}
-
static void spapr_machine_4_0_class_options(MachineClass *mc)
{
SpaprMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
@@ -4732,7 +4737,7 @@ DEFINE_SPAPR_MACHINE(2_8, "2.8", false);
* pseries-2.7
*/
-static void phb_placement_2_7(SpaprMachineState *spapr, uint32_t index,
+static bool phb_placement_2_7(SpaprMachineState *spapr, uint32_t index,
uint64_t *buid, hwaddr *pio,
hwaddr *mmio32, hwaddr *mmio64,
unsigned n_dma, uint32_t *liobns,
@@ -4764,7 +4769,7 @@ static void phb_placement_2_7(SpaprMachineState *spapr, uint32_t index,
if (index > max_index) {
error_setg(errp, "\"index\" for PAPR PHB is too large (max %u)",
max_index);
- return;
+ return false;
}
*buid = base_buid + index;
@@ -4783,6 +4788,7 @@ static void phb_placement_2_7(SpaprMachineState *spapr, uint32_t index,
*nv2gpa = 0;
*nv2atsd = 0;
+ return true;
}
static void spapr_machine_2_7_class_options(MachineClass *mc)
--
2.26.2
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH for-6.0 6/9] spapr: Make PHB placement functions and spapr_pre_plug_phb() return status
2020-11-20 23:42 ` [PATCH for-6.0 6/9] spapr: Make PHB placement functions and spapr_pre_plug_phb() return status Greg Kurz
@ 2020-11-23 5:14 ` David Gibson
0 siblings, 0 replies; 30+ messages in thread
From: David Gibson @ 2020-11-23 5:14 UTC (permalink / raw)
To: Greg Kurz; +Cc: Igor Mammedov, qemu-ppc, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 6648 bytes --]
On Sat, Nov 21, 2020 at 12:42:05AM +0100, Greg Kurz wrote:
> Read documentation in "qapi/error.h" and changelog of commit
> e3fe3988d785 ("error: Document Error API usage rules") for
> rationale.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
Applied to ppc-for-6.0, thanks.
> ---
> include/hw/ppc/spapr.h | 2 +-
> hw/ppc/spapr.c | 40 +++++++++++++++++++++++-----------------
> 2 files changed, 24 insertions(+), 18 deletions(-)
>
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 2e89e36cfbdc..b7ced9faebf5 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -140,7 +140,7 @@ struct SpaprMachineClass {
> bool pre_5_1_assoc_refpoints;
> bool pre_5_2_numa_associativity;
>
> - void (*phb_placement)(SpaprMachineState *spapr, uint32_t index,
> + bool (*phb_placement)(SpaprMachineState *spapr, uint32_t index,
> uint64_t *buid, hwaddr *pio,
> hwaddr *mmio32, hwaddr *mmio64,
> unsigned n_dma, uint32_t *liobns, hwaddr *nv2gpa,
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index cfca033c7b14..81bac59887ab 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3865,7 +3865,7 @@ int spapr_phb_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr,
> return 0;
> }
>
> -static void spapr_phb_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> +static bool spapr_phb_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> Error **errp)
> {
> SpaprMachineState *spapr = SPAPR_MACHINE(OBJECT(hotplug_dev));
> @@ -3875,24 +3875,25 @@ static void spapr_phb_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>
> if (dev->hotplugged && !smc->dr_phb_enabled) {
> error_setg(errp, "PHB hotplug not supported for this machine");
> - return;
> + return false;
> }
>
> if (sphb->index == (uint32_t)-1) {
> error_setg(errp, "\"index\" for PAPR PHB is mandatory");
> - return;
> + return false;
> }
>
> /*
> * This will check that sphb->index doesn't exceed the maximum number of
> * PHBs for the current machine type.
> */
> - smc->phb_placement(spapr, sphb->index,
> - &sphb->buid, &sphb->io_win_addr,
> - &sphb->mem_win_addr, &sphb->mem64_win_addr,
> - windows_supported, sphb->dma_liobn,
> - &sphb->nv2_gpa_win_addr, &sphb->nv2_atsd_win_addr,
> - errp);
> + return
> + smc->phb_placement(spapr, sphb->index,
> + &sphb->buid, &sphb->io_win_addr,
> + &sphb->mem_win_addr, &sphb->mem64_win_addr,
> + windows_supported, sphb->dma_liobn,
> + &sphb->nv2_gpa_win_addr, &sphb->nv2_atsd_win_addr,
> + errp);
> }
>
> static void spapr_phb_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> @@ -4130,7 +4131,7 @@ static const CPUArchIdList *spapr_possible_cpu_arch_ids(MachineState *machine)
> return machine->possible_cpus;
> }
>
> -static void spapr_phb_placement(SpaprMachineState *spapr, uint32_t index,
> +static bool spapr_phb_placement(SpaprMachineState *spapr, uint32_t index,
> uint64_t *buid, hwaddr *pio,
> hwaddr *mmio32, hwaddr *mmio64,
> unsigned n_dma, uint32_t *liobns,
> @@ -4168,7 +4169,7 @@ static void spapr_phb_placement(SpaprMachineState *spapr, uint32_t index,
> if (index >= SPAPR_MAX_PHBS) {
> error_setg(errp, "\"index\" for PAPR PHB is too large (max %llu)",
> SPAPR_MAX_PHBS - 1);
> - return;
> + return false;
> }
>
> *buid = base_buid + index;
> @@ -4182,6 +4183,7 @@ static void spapr_phb_placement(SpaprMachineState *spapr, uint32_t index,
>
> *nv2gpa = SPAPR_PCI_NV2RAM64_WIN_BASE + index * SPAPR_PCI_NV2RAM64_WIN_SIZE;
> *nv2atsd = SPAPR_PCI_NV2ATSD_WIN_BASE + index * SPAPR_PCI_NV2ATSD_WIN_SIZE;
> + return true;
> }
>
> static ICSState *spapr_ics_get(XICSFabric *dev, int irq)
> @@ -4561,18 +4563,21 @@ DEFINE_SPAPR_MACHINE(4_1, "4.1", false);
> /*
> * pseries-4.0
> */
> -static void phb_placement_4_0(SpaprMachineState *spapr, uint32_t index,
> +static bool phb_placement_4_0(SpaprMachineState *spapr, uint32_t index,
> uint64_t *buid, hwaddr *pio,
> hwaddr *mmio32, hwaddr *mmio64,
> unsigned n_dma, uint32_t *liobns,
> hwaddr *nv2gpa, hwaddr *nv2atsd, Error **errp)
> {
> - spapr_phb_placement(spapr, index, buid, pio, mmio32, mmio64, n_dma, liobns,
> - nv2gpa, nv2atsd, errp);
> + if (!spapr_phb_placement(spapr, index, buid, pio, mmio32, mmio64, n_dma,
> + liobns, nv2gpa, nv2atsd, errp)) {
> + return false;
> + }
> +
> *nv2gpa = 0;
> *nv2atsd = 0;
> + return true;
> }
> -
> static void spapr_machine_4_0_class_options(MachineClass *mc)
> {
> SpaprMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
> @@ -4732,7 +4737,7 @@ DEFINE_SPAPR_MACHINE(2_8, "2.8", false);
> * pseries-2.7
> */
>
> -static void phb_placement_2_7(SpaprMachineState *spapr, uint32_t index,
> +static bool phb_placement_2_7(SpaprMachineState *spapr, uint32_t index,
> uint64_t *buid, hwaddr *pio,
> hwaddr *mmio32, hwaddr *mmio64,
> unsigned n_dma, uint32_t *liobns,
> @@ -4764,7 +4769,7 @@ static void phb_placement_2_7(SpaprMachineState *spapr, uint32_t index,
> if (index > max_index) {
> error_setg(errp, "\"index\" for PAPR PHB is too large (max %u)",
> max_index);
> - return;
> + return false;
> }
>
> *buid = base_buid + index;
> @@ -4783,6 +4788,7 @@ static void phb_placement_2_7(SpaprMachineState *spapr, uint32_t index,
>
> *nv2gpa = 0;
> *nv2atsd = 0;
> + return true;
> }
>
> static void spapr_machine_2_7_class_options(MachineClass *mc)
--
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] 30+ messages in thread
* [PATCH for-6.0 7/9] spapr: Do PHB hoplug sanity check at pre-plug
2020-11-20 23:41 [PATCH for-6.0 0/9] spapr: Perform hotplug sanity checks at pre-plug Greg Kurz
` (5 preceding siblings ...)
2020-11-20 23:42 ` [PATCH for-6.0 6/9] spapr: Make PHB placement functions and spapr_pre_plug_phb() return status Greg Kurz
@ 2020-11-20 23:42 ` Greg Kurz
2020-11-23 5:26 ` David Gibson
2020-11-20 23:42 ` [PATCH for-6.0 8/9] spapr: Do TPM proxy hotplug sanity checks " Greg Kurz
2020-11-20 23:42 ` [PATCH for-6.0 9/9] spapr: spapr_drc_attach() cannot fail Greg Kurz
8 siblings, 1 reply; 30+ messages in thread
From: Greg Kurz @ 2020-11-20 23:42 UTC (permalink / raw)
To: qemu-devel; +Cc: Igor Mammedov, qemu-ppc, Greg Kurz, David Gibson
We currently detect that a PHB index is already in use at plug time.
But this can be decteted at pre-plug in order to error out earlier.
This allows to pass &error_abort to spapr_drc_attach() and to end
up with a plug handler that doesn't need to report errors anymore.
Signed-off-by: Greg Kurz <groug@kaod.org>
---
hw/ppc/spapr.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 81bac59887ab..bded059d59c8 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3872,6 +3872,7 @@ static bool spapr_phb_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
SpaprPhbState *sphb = SPAPR_PCI_HOST_BRIDGE(dev);
SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
const unsigned windows_supported = spapr_phb_windows_supported(sphb);
+ SpaprDrc *drc;
if (dev->hotplugged && !smc->dr_phb_enabled) {
error_setg(errp, "PHB hotplug not supported for this machine");
@@ -3883,6 +3884,12 @@ static bool spapr_phb_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
return false;
}
+ drc = spapr_drc_by_id(TYPE_SPAPR_DRC_PHB, sphb->index);
+ if (drc && drc->dev) {
+ error_setg(errp, "PHB %d already attached", sphb->index);
+ return false;
+ }
+
/*
* This will check that sphb->index doesn't exceed the maximum number of
* PHBs for the current machine type.
@@ -3896,8 +3903,7 @@ static bool spapr_phb_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
errp);
}
-static void spapr_phb_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
- Error **errp)
+static void spapr_phb_plug(HotplugHandler *hotplug_dev, DeviceState *dev)
{
SpaprMachineState *spapr = SPAPR_MACHINE(OBJECT(hotplug_dev));
SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
@@ -3913,9 +3919,8 @@ static void spapr_phb_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
/* hotplug hooks should check it's enabled before getting this far */
assert(drc);
- if (!spapr_drc_attach(drc, dev, errp)) {
- return;
- }
+ /* spapr_phb_pre_plug() already checked the DRC is attachable */
+ spapr_drc_attach(drc, dev, &error_abort);
if (hotplugged) {
spapr_hotplug_req_add_by_index(drc);
@@ -3983,7 +3988,7 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
} else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
spapr_core_plug(hotplug_dev, dev);
} else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_PCI_HOST_BRIDGE)) {
- spapr_phb_plug(hotplug_dev, dev, errp);
+ spapr_phb_plug(hotplug_dev, dev);
} else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_TPM_PROXY)) {
spapr_tpm_proxy_plug(hotplug_dev, dev, errp);
}
--
2.26.2
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH for-6.0 7/9] spapr: Do PHB hoplug sanity check at pre-plug
2020-11-20 23:42 ` [PATCH for-6.0 7/9] spapr: Do PHB hoplug sanity check at pre-plug Greg Kurz
@ 2020-11-23 5:26 ` David Gibson
0 siblings, 0 replies; 30+ messages in thread
From: David Gibson @ 2020-11-23 5:26 UTC (permalink / raw)
To: Greg Kurz; +Cc: Igor Mammedov, qemu-ppc, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 3346 bytes --]
On Sat, Nov 21, 2020 at 12:42:06AM +0100, Greg Kurz wrote:
> We currently detect that a PHB index is already in use at plug time.
> But this can be decteted at pre-plug in order to error out earlier.
>
> This allows to pass &error_abort to spapr_drc_attach() and to end
> up with a plug handler that doesn't need to report errors anymore.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
Applied to ppc-for-6.0, thanks.
> ---
> hw/ppc/spapr.c | 17 +++++++++++------
> 1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 81bac59887ab..bded059d59c8 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3872,6 +3872,7 @@ static bool spapr_phb_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> SpaprPhbState *sphb = SPAPR_PCI_HOST_BRIDGE(dev);
> SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> const unsigned windows_supported = spapr_phb_windows_supported(sphb);
> + SpaprDrc *drc;
>
> if (dev->hotplugged && !smc->dr_phb_enabled) {
> error_setg(errp, "PHB hotplug not supported for this machine");
> @@ -3883,6 +3884,12 @@ static bool spapr_phb_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> return false;
> }
>
> + drc = spapr_drc_by_id(TYPE_SPAPR_DRC_PHB, sphb->index);
> + if (drc && drc->dev) {
> + error_setg(errp, "PHB %d already attached", sphb->index);
> + return false;
> + }
> +
> /*
> * This will check that sphb->index doesn't exceed the maximum number of
> * PHBs for the current machine type.
> @@ -3896,8 +3903,7 @@ static bool spapr_phb_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> errp);
> }
>
> -static void spapr_phb_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> - Error **errp)
> +static void spapr_phb_plug(HotplugHandler *hotplug_dev, DeviceState *dev)
> {
> SpaprMachineState *spapr = SPAPR_MACHINE(OBJECT(hotplug_dev));
> SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> @@ -3913,9 +3919,8 @@ static void spapr_phb_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> /* hotplug hooks should check it's enabled before getting this far */
> assert(drc);
>
> - if (!spapr_drc_attach(drc, dev, errp)) {
> - return;
> - }
> + /* spapr_phb_pre_plug() already checked the DRC is attachable */
> + spapr_drc_attach(drc, dev, &error_abort);
>
> if (hotplugged) {
> spapr_hotplug_req_add_by_index(drc);
> @@ -3983,7 +3988,7 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
> } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> spapr_core_plug(hotplug_dev, dev);
> } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_PCI_HOST_BRIDGE)) {
> - spapr_phb_plug(hotplug_dev, dev, errp);
> + spapr_phb_plug(hotplug_dev, dev);
> } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_TPM_PROXY)) {
> spapr_tpm_proxy_plug(hotplug_dev, dev, errp);
> }
--
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] 30+ messages in thread
* [PATCH for-6.0 8/9] spapr: Do TPM proxy hotplug sanity checks at pre-plug
2020-11-20 23:41 [PATCH for-6.0 0/9] spapr: Perform hotplug sanity checks at pre-plug Greg Kurz
` (6 preceding siblings ...)
2020-11-20 23:42 ` [PATCH for-6.0 7/9] spapr: Do PHB hoplug sanity check at pre-plug Greg Kurz
@ 2020-11-20 23:42 ` Greg Kurz
2020-11-23 5:32 ` David Gibson
2020-11-20 23:42 ` [PATCH for-6.0 9/9] spapr: spapr_drc_attach() cannot fail Greg Kurz
8 siblings, 1 reply; 30+ messages in thread
From: Greg Kurz @ 2020-11-20 23:42 UTC (permalink / raw)
To: qemu-devel; +Cc: Igor Mammedov, qemu-ppc, Greg Kurz, David Gibson
There can be only one TPM proxy at a time. This is currently
checked at plug time. But this can be detected at pre-plug in
order to error out earlier.
This allows to get rid of error handling in the plug handler.
Signed-off-by: Greg Kurz <groug@kaod.org>
---
hw/ppc/spapr.c | 23 ++++++++++++++++++-----
1 file changed, 18 insertions(+), 5 deletions(-)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index bded059d59c8..5e32d1d396b4 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3957,17 +3957,28 @@ static void spapr_phb_unplug_request(HotplugHandler *hotplug_dev,
}
}
-static void spapr_tpm_proxy_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
- Error **errp)
+static
+bool spapr_tpm_proxy_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
+ Error **errp)
{
SpaprMachineState *spapr = SPAPR_MACHINE(OBJECT(hotplug_dev));
- SpaprTpmProxy *tpm_proxy = SPAPR_TPM_PROXY(dev);
if (spapr->tpm_proxy != NULL) {
error_setg(errp, "Only one TPM proxy can be specified for this machine");
- return;
+ return false;
}
+ return true;
+}
+
+static void spapr_tpm_proxy_plug(HotplugHandler *hotplug_dev, DeviceState *dev)
+{
+ SpaprMachineState *spapr = SPAPR_MACHINE(OBJECT(hotplug_dev));
+ SpaprTpmProxy *tpm_proxy = SPAPR_TPM_PROXY(dev);
+
+ /* Already checked in spapr_tpm_proxy_pre_plug() */
+ g_assert(spapr->tpm_proxy == NULL);
+
spapr->tpm_proxy = tpm_proxy;
}
@@ -3990,7 +4001,7 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
} else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_PCI_HOST_BRIDGE)) {
spapr_phb_plug(hotplug_dev, dev);
} else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_TPM_PROXY)) {
- spapr_tpm_proxy_plug(hotplug_dev, dev, errp);
+ spapr_tpm_proxy_plug(hotplug_dev, dev);
}
}
@@ -4053,6 +4064,8 @@ static void spapr_machine_device_pre_plug(HotplugHandler *hotplug_dev,
spapr_core_pre_plug(hotplug_dev, dev, errp);
} else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_PCI_HOST_BRIDGE)) {
spapr_phb_pre_plug(hotplug_dev, dev, errp);
+ } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_TPM_PROXY)) {
+ spapr_tpm_proxy_pre_plug(hotplug_dev, dev, errp);
}
}
--
2.26.2
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH for-6.0 8/9] spapr: Do TPM proxy hotplug sanity checks at pre-plug
2020-11-20 23:42 ` [PATCH for-6.0 8/9] spapr: Do TPM proxy hotplug sanity checks " Greg Kurz
@ 2020-11-23 5:32 ` David Gibson
0 siblings, 0 replies; 30+ messages in thread
From: David Gibson @ 2020-11-23 5:32 UTC (permalink / raw)
To: Greg Kurz; +Cc: Igor Mammedov, qemu-ppc, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 2831 bytes --]
On Sat, Nov 21, 2020 at 12:42:07AM +0100, Greg Kurz wrote:
> There can be only one TPM proxy at a time. This is currently
> checked at plug time. But this can be detected at pre-plug in
> order to error out earlier.
>
> This allows to get rid of error handling in the plug handler.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
Applied to ppc-for-6.0, thanks.
> ---
> hw/ppc/spapr.c | 23 ++++++++++++++++++-----
> 1 file changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index bded059d59c8..5e32d1d396b4 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3957,17 +3957,28 @@ static void spapr_phb_unplug_request(HotplugHandler *hotplug_dev,
> }
> }
>
> -static void spapr_tpm_proxy_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> - Error **errp)
> +static
> +bool spapr_tpm_proxy_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> + Error **errp)
> {
> SpaprMachineState *spapr = SPAPR_MACHINE(OBJECT(hotplug_dev));
> - SpaprTpmProxy *tpm_proxy = SPAPR_TPM_PROXY(dev);
>
> if (spapr->tpm_proxy != NULL) {
> error_setg(errp, "Only one TPM proxy can be specified for this machine");
> - return;
> + return false;
> }
>
> + return true;
> +}
> +
> +static void spapr_tpm_proxy_plug(HotplugHandler *hotplug_dev, DeviceState *dev)
> +{
> + SpaprMachineState *spapr = SPAPR_MACHINE(OBJECT(hotplug_dev));
> + SpaprTpmProxy *tpm_proxy = SPAPR_TPM_PROXY(dev);
> +
> + /* Already checked in spapr_tpm_proxy_pre_plug() */
> + g_assert(spapr->tpm_proxy == NULL);
> +
> spapr->tpm_proxy = tpm_proxy;
> }
>
> @@ -3990,7 +4001,7 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
> } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_PCI_HOST_BRIDGE)) {
> spapr_phb_plug(hotplug_dev, dev);
> } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_TPM_PROXY)) {
> - spapr_tpm_proxy_plug(hotplug_dev, dev, errp);
> + spapr_tpm_proxy_plug(hotplug_dev, dev);
> }
> }
>
> @@ -4053,6 +4064,8 @@ static void spapr_machine_device_pre_plug(HotplugHandler *hotplug_dev,
> spapr_core_pre_plug(hotplug_dev, dev, errp);
> } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_PCI_HOST_BRIDGE)) {
> spapr_phb_pre_plug(hotplug_dev, dev, errp);
> + } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_TPM_PROXY)) {
> + spapr_tpm_proxy_pre_plug(hotplug_dev, dev, errp);
> }
> }
>
--
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] 30+ messages in thread
* [PATCH for-6.0 9/9] spapr: spapr_drc_attach() cannot fail
2020-11-20 23:41 [PATCH for-6.0 0/9] spapr: Perform hotplug sanity checks at pre-plug Greg Kurz
` (7 preceding siblings ...)
2020-11-20 23:42 ` [PATCH for-6.0 8/9] spapr: Do TPM proxy hotplug sanity checks " Greg Kurz
@ 2020-11-20 23:42 ` Greg Kurz
2020-11-23 5:34 ` David Gibson
8 siblings, 1 reply; 30+ messages in thread
From: Greg Kurz @ 2020-11-20 23:42 UTC (permalink / raw)
To: qemu-devel; +Cc: Igor Mammedov, qemu-ppc, Greg Kurz, David Gibson
All users are passing &error_abort already. Document the fact
that spapr_drc_attach() should only be passed a free DRC, which
is supposedly the case if appropriate checking is done earlier.
Signed-off-by: Greg Kurz <groug@kaod.org>
---
include/hw/ppc/spapr_drc.h | 8 +++++++-
hw/ppc/spapr.c | 6 +++---
hw/ppc/spapr_drc.c | 8 ++------
hw/ppc/spapr_nvdimm.c | 2 +-
hw/ppc/spapr_pci.c | 2 +-
5 files changed, 14 insertions(+), 12 deletions(-)
diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
index 165b281496bb..def3593adc8b 100644
--- a/include/hw/ppc/spapr_drc.h
+++ b/include/hw/ppc/spapr_drc.h
@@ -235,7 +235,13 @@ SpaprDrc *spapr_drc_by_index(uint32_t index);
SpaprDrc *spapr_drc_by_id(const char *type, uint32_t id);
int spapr_dt_drc(void *fdt, int offset, Object *owner, uint32_t drc_type_mask);
-bool spapr_drc_attach(SpaprDrc *drc, DeviceState *d, Error **errp);
+/*
+ * These functions respectively abort if called with a device already
+ * attached or no device attached. In the case of spapr_drc_attach(),
+ * this means that the attachability of the DRC *must* be checked
+ * beforehand (eg. check drc->dev at pre-plug).
+ */
+void spapr_drc_attach(SpaprDrc *drc, DeviceState *d);
void spapr_drc_detach(SpaprDrc *drc);
/* Returns true if a hot plug/unplug request is pending */
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 5e32d1d396b4..e0047f41073e 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3399,7 +3399,7 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
* that doesn't overlap with any existing mapping at pre-plug. The
* corresponding LMB DRCs are thus assumed to be all attachable.
*/
- spapr_drc_attach(drc, dev, &error_abort);
+ spapr_drc_attach(drc, dev);
if (!hotplugged) {
spapr_drc_reset(drc);
}
@@ -3767,7 +3767,7 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev)
* core being plugged into a free slot. Nothing should already
* be attached to the corresponding DRC.
*/
- spapr_drc_attach(drc, dev, &error_abort);
+ spapr_drc_attach(drc, dev);
if (hotplugged) {
/*
@@ -3920,7 +3920,7 @@ static void spapr_phb_plug(HotplugHandler *hotplug_dev, DeviceState *dev)
assert(drc);
/* spapr_phb_pre_plug() already checked the DRC is attachable */
- spapr_drc_attach(drc, dev, &error_abort);
+ spapr_drc_attach(drc, dev);
if (hotplugged) {
spapr_hotplug_req_add_by_index(drc);
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 77718cde1ff2..f991cf89a08a 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -369,14 +369,11 @@ static void prop_get_fdt(Object *obj, Visitor *v, const char *name,
} while (fdt_depth != 0);
}
-bool spapr_drc_attach(SpaprDrc *drc, DeviceState *d, Error **errp)
+void spapr_drc_attach(SpaprDrc *drc, DeviceState *d)
{
trace_spapr_drc_attach(spapr_drc_index(drc));
- if (drc->dev) {
- error_setg(errp, "an attached device is still awaiting release");
- return false;
- }
+ g_assert(!drc->dev);
g_assert((drc->state == SPAPR_DRC_STATE_LOGICAL_UNUSABLE)
|| (drc->state == SPAPR_DRC_STATE_PHYSICAL_POWERON));
@@ -386,7 +383,6 @@ bool spapr_drc_attach(SpaprDrc *drc, DeviceState *d, Error **errp)
object_get_typename(OBJECT(drc->dev)),
(Object **)(&drc->dev),
NULL, 0);
- return true;
}
static void spapr_drc_release(SpaprDrc *drc)
diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
index 2f1c196e1b76..73ee006541a6 100644
--- a/hw/ppc/spapr_nvdimm.c
+++ b/hw/ppc/spapr_nvdimm.c
@@ -101,7 +101,7 @@ void spapr_add_nvdimm(DeviceState *dev, uint64_t slot)
* pc_dimm_get_free_slot() provided a free slot at pre-plug. The
* corresponding DRC is thus assumed to be attachable.
*/
- spapr_drc_attach(drc, dev, &error_abort);
+ spapr_drc_attach(drc, dev);
if (hotplugged) {
spapr_hotplug_req_add_by_index(drc);
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 2829f298d9c1..e946bd5055cc 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1601,7 +1601,7 @@ static void spapr_pci_plug(HotplugHandler *plug_handler,
}
/* spapr_pci_pre_plug() already checked the DRC is attachable */
- spapr_drc_attach(drc, DEVICE(pdev), &error_abort);
+ spapr_drc_attach(drc, DEVICE(pdev));
/* If this is function 0, signal hotplug for all the device functions.
* Otherwise defer sending the hotplug event.
--
2.26.2
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH for-6.0 9/9] spapr: spapr_drc_attach() cannot fail
2020-11-20 23:42 ` [PATCH for-6.0 9/9] spapr: spapr_drc_attach() cannot fail Greg Kurz
@ 2020-11-23 5:34 ` David Gibson
0 siblings, 0 replies; 30+ messages in thread
From: David Gibson @ 2020-11-23 5:34 UTC (permalink / raw)
To: Greg Kurz; +Cc: Igor Mammedov, qemu-ppc, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 5360 bytes --]
On Sat, Nov 21, 2020 at 12:42:08AM +0100, Greg Kurz wrote:
> All users are passing &error_abort already. Document the fact
> that spapr_drc_attach() should only be passed a free DRC, which
> is supposedly the case if appropriate checking is done earlier.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
Applied to ppc-for-6.0, thanks.
> ---
> include/hw/ppc/spapr_drc.h | 8 +++++++-
> hw/ppc/spapr.c | 6 +++---
> hw/ppc/spapr_drc.c | 8 ++------
> hw/ppc/spapr_nvdimm.c | 2 +-
> hw/ppc/spapr_pci.c | 2 +-
> 5 files changed, 14 insertions(+), 12 deletions(-)
>
> diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> index 165b281496bb..def3593adc8b 100644
> --- a/include/hw/ppc/spapr_drc.h
> +++ b/include/hw/ppc/spapr_drc.h
> @@ -235,7 +235,13 @@ SpaprDrc *spapr_drc_by_index(uint32_t index);
> SpaprDrc *spapr_drc_by_id(const char *type, uint32_t id);
> int spapr_dt_drc(void *fdt, int offset, Object *owner, uint32_t drc_type_mask);
>
> -bool spapr_drc_attach(SpaprDrc *drc, DeviceState *d, Error **errp);
> +/*
> + * These functions respectively abort if called with a device already
> + * attached or no device attached. In the case of spapr_drc_attach(),
> + * this means that the attachability of the DRC *must* be checked
> + * beforehand (eg. check drc->dev at pre-plug).
> + */
> +void spapr_drc_attach(SpaprDrc *drc, DeviceState *d);
> void spapr_drc_detach(SpaprDrc *drc);
>
> /* Returns true if a hot plug/unplug request is pending */
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 5e32d1d396b4..e0047f41073e 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3399,7 +3399,7 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
> * that doesn't overlap with any existing mapping at pre-plug. The
> * corresponding LMB DRCs are thus assumed to be all attachable.
> */
> - spapr_drc_attach(drc, dev, &error_abort);
> + spapr_drc_attach(drc, dev);
> if (!hotplugged) {
> spapr_drc_reset(drc);
> }
> @@ -3767,7 +3767,7 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev)
> * core being plugged into a free slot. Nothing should already
> * be attached to the corresponding DRC.
> */
> - spapr_drc_attach(drc, dev, &error_abort);
> + spapr_drc_attach(drc, dev);
>
> if (hotplugged) {
> /*
> @@ -3920,7 +3920,7 @@ static void spapr_phb_plug(HotplugHandler *hotplug_dev, DeviceState *dev)
> assert(drc);
>
> /* spapr_phb_pre_plug() already checked the DRC is attachable */
> - spapr_drc_attach(drc, dev, &error_abort);
> + spapr_drc_attach(drc, dev);
>
> if (hotplugged) {
> spapr_hotplug_req_add_by_index(drc);
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 77718cde1ff2..f991cf89a08a 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -369,14 +369,11 @@ static void prop_get_fdt(Object *obj, Visitor *v, const char *name,
> } while (fdt_depth != 0);
> }
>
> -bool spapr_drc_attach(SpaprDrc *drc, DeviceState *d, Error **errp)
> +void spapr_drc_attach(SpaprDrc *drc, DeviceState *d)
> {
> trace_spapr_drc_attach(spapr_drc_index(drc));
>
> - if (drc->dev) {
> - error_setg(errp, "an attached device is still awaiting release");
> - return false;
> - }
> + g_assert(!drc->dev);
> g_assert((drc->state == SPAPR_DRC_STATE_LOGICAL_UNUSABLE)
> || (drc->state == SPAPR_DRC_STATE_PHYSICAL_POWERON));
>
> @@ -386,7 +383,6 @@ bool spapr_drc_attach(SpaprDrc *drc, DeviceState *d, Error **errp)
> object_get_typename(OBJECT(drc->dev)),
> (Object **)(&drc->dev),
> NULL, 0);
> - return true;
> }
>
> static void spapr_drc_release(SpaprDrc *drc)
> diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
> index 2f1c196e1b76..73ee006541a6 100644
> --- a/hw/ppc/spapr_nvdimm.c
> +++ b/hw/ppc/spapr_nvdimm.c
> @@ -101,7 +101,7 @@ void spapr_add_nvdimm(DeviceState *dev, uint64_t slot)
> * pc_dimm_get_free_slot() provided a free slot at pre-plug. The
> * corresponding DRC is thus assumed to be attachable.
> */
> - spapr_drc_attach(drc, dev, &error_abort);
> + spapr_drc_attach(drc, dev);
>
> if (hotplugged) {
> spapr_hotplug_req_add_by_index(drc);
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 2829f298d9c1..e946bd5055cc 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1601,7 +1601,7 @@ static void spapr_pci_plug(HotplugHandler *plug_handler,
> }
>
> /* spapr_pci_pre_plug() already checked the DRC is attachable */
> - spapr_drc_attach(drc, DEVICE(pdev), &error_abort);
> + spapr_drc_attach(drc, DEVICE(pdev));
>
> /* If this is function 0, signal hotplug for all the device functions.
> * Otherwise defer sending the hotplug event.
--
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] 30+ messages in thread