* [Qemu-devel] [PATCHv2 1/8] spapr: Treat devices added before inbound migration as coldplugged
2017-07-12 5:53 [Qemu-devel] [PATCHv2 0/8] spapr: DRC cleanups (part VI) David Gibson
@ 2017-07-12 5:53 ` David Gibson
2017-07-12 8:41 ` Greg Kurz
2017-07-12 5:53 ` [Qemu-devel] [PATCHv2 2/8] spapr: Remove 'awaiting_allocation' DRC flag David Gibson
` (8 subsequent siblings)
9 siblings, 1 reply; 30+ messages in thread
From: David Gibson @ 2017-07-12 5:53 UTC (permalink / raw)
To: mdroth, groug, danielhb
Cc: qemu-ppc, qemu-devel, lvivier, sjitindarsingh, bharata,
David Gibson
From: Laurent Vivier <lvivier@redhat.com>
When migrating a guest which has already had devices hotplugged,
libvirt typically starts the destination qemu with -incoming defer,
adds those hotplugged devices with qmp, then initiates the incoming
migration.
This causes problems for the management of spapr DRC state. Because
the device is treated as hotplugged, it goes into a DRC state for a
device immediately after it's plugged, but before the guest has
acknowledged its presence. However, chances are the guest on the
source machine *has* acknowledged the device's presence and configured
it.
If the source has fully configured the device, then DRC state won't be
sent in the migration stream: for maximum migration compatibility with
earlier versions we don't migrate DRCs in coldplug-equivalent state.
That means that the DRC effectively changes state over the migrate,
causing problems later on.
In addition, logging hotplug events for these devices isn't what we
want because a) those events should already have been issued on the
source host and b) the event queue should get wiped out by the
incoming state anyway.
In short, what we really want is to treat devices added before an
incoming migration as if they were coldplugged.
To do this, we first add a spapr_drc_hotplugged() helper which
determines if the device is hotplugged in the sense relevant for DRC
state management. We only send hotplug events when this is true.
Second, when we add a device which isn't hotplugged in this sense, we
force a reset of the DRC state - this ensures the DRC is in a
coldplug-equivalent state (there isn't usually a system reset between
these device adds and the incoming migration).
This is based on an earlier patch by Laurent Vivier, cleaned up and
extended.
Signed-off-by: Laurent Vivier <lvivier@redhat.com>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
hw/ppc/spapr.c | 24 ++++++++++++++++--------
hw/ppc/spapr_drc.c | 9 ++++++---
hw/ppc/spapr_pci.c | 4 +++-
include/hw/ppc/spapr_drc.h | 8 ++++++++
4 files changed, 33 insertions(+), 12 deletions(-)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 12b3f09..2a059d5 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2636,6 +2636,7 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
int i, fdt_offset, fdt_size;
void *fdt;
uint64_t addr = addr_start;
+ bool hotplugged = spapr_drc_hotplugged(dev);
Error *local_err = NULL;
for (i = 0; i < nr_lmbs; i++) {
@@ -2659,12 +2660,15 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
error_propagate(errp, local_err);
return;
}
+ if (!hotplugged) {
+ spapr_drc_reset(drc);
+ }
addr += SPAPR_MEMORY_BLOCK_SIZE;
}
/* send hotplug notification to the
* guest only in case of hotplugged memory
*/
- if (dev->hotplugged) {
+ if (hotplugged) {
if (dedicated_hp_event_source) {
drc = spapr_drc_by_id(TYPE_SPAPR_DRC_LMB,
addr_start / SPAPR_MEMORY_BLOCK_SIZE);
@@ -2998,6 +3002,7 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
int smt = kvmppc_smt_threads();
CPUArchId *core_slot;
int index;
+ bool hotplugged = spapr_drc_hotplugged(dev);
core_slot = spapr_find_cpu_slot(MACHINE(hotplug_dev), cc->core_id, &index);
if (!core_slot) {
@@ -3018,15 +3023,18 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
error_propagate(errp, local_err);
return;
}
- }
- if (dev->hotplugged) {
- /*
- * Send hotplug notification interrupt to the guest only in case
- * of hotplugged CPUs.
- */
- spapr_hotplug_req_add_by_index(drc);
+ if (hotplugged) {
+ /*
+ * Send hotplug notification interrupt to the guest only
+ * in case of hotplugged CPUs.
+ */
+ spapr_hotplug_req_add_by_index(drc);
+ } else {
+ spapr_drc_reset(drc);
+ }
}
+
core_slot->cpu = OBJECT(dev);
if (smc->pre_2_10_has_unused_icps) {
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index f34355d..9b07f80 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -412,10 +412,8 @@ static bool release_pending(sPAPRDRConnector *drc)
return drc->awaiting_release;
}
-static void drc_reset(void *opaque)
+void spapr_drc_reset(sPAPRDRConnector *drc)
{
- sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(opaque);
-
trace_spapr_drc_reset(spapr_drc_index(drc));
g_free(drc->ccs);
@@ -447,6 +445,11 @@ static void drc_reset(void *opaque)
}
}
+static void drc_reset(void *opaque)
+{
+ spapr_drc_reset(SPAPR_DR_CONNECTOR(opaque));
+}
+
static bool spapr_drc_needed(void *opaque)
{
sPAPRDRConnector *drc = (sPAPRDRConnector *)opaque;
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index a52dcf8..1e84c55 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1443,7 +1443,9 @@ static void spapr_pci_plug(HotplugHandler *plug_handler,
/* If this is function 0, signal hotplug for all the device functions.
* Otherwise defer sending the hotplug event.
*/
- if (plugged_dev->hotplugged && PCI_FUNC(pdev->devfn) == 0) {
+ if (!spapr_drc_hotplugged(plugged_dev)) {
+ spapr_drc_reset(drc);
+ } else if (PCI_FUNC(pdev->devfn) == 0) {
int i;
for (i = 0; i < 8; i++) {
diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
index d15e9eb..715016b 100644
--- a/include/hw/ppc/spapr_drc.h
+++ b/include/hw/ppc/spapr_drc.h
@@ -15,6 +15,7 @@
#include <libfdt.h>
#include "qom/object.h"
+#include "sysemu/sysemu.h"
#include "hw/qdev.h"
#define TYPE_SPAPR_DR_CONNECTOR "spapr-dr-connector"
@@ -223,6 +224,13 @@ typedef struct sPAPRDRConnectorClass {
bool (*release_pending)(sPAPRDRConnector *drc);
} sPAPRDRConnectorClass;
+static inline bool spapr_drc_hotplugged(DeviceState *dev)
+{
+ return dev->hotplugged && !runstate_check(RUN_STATE_INMIGRATE);
+}
+
+void spapr_drc_reset(sPAPRDRConnector *drc);
+
uint32_t spapr_drc_index(sPAPRDRConnector *drc);
sPAPRDRConnectorType spapr_drc_type(sPAPRDRConnector *drc);
--
2.9.4
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [Qemu-devel] [PATCHv2 1/8] spapr: Treat devices added before inbound migration as coldplugged
2017-07-12 5:53 ` [Qemu-devel] [PATCHv2 1/8] spapr: Treat devices added before inbound migration as coldplugged David Gibson
@ 2017-07-12 8:41 ` Greg Kurz
0 siblings, 0 replies; 30+ messages in thread
From: Greg Kurz @ 2017-07-12 8:41 UTC (permalink / raw)
To: David Gibson
Cc: mdroth, danielhb, qemu-ppc, qemu-devel, lvivier, sjitindarsingh,
bharata
[-- Attachment #1: Type: text/plain, Size: 7042 bytes --]
On Wed, 12 Jul 2017 15:53:10 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:
> From: Laurent Vivier <lvivier@redhat.com>
>
> When migrating a guest which has already had devices hotplugged,
> libvirt typically starts the destination qemu with -incoming defer,
> adds those hotplugged devices with qmp, then initiates the incoming
> migration.
>
> This causes problems for the management of spapr DRC state. Because
> the device is treated as hotplugged, it goes into a DRC state for a
> device immediately after it's plugged, but before the guest has
> acknowledged its presence. However, chances are the guest on the
> source machine *has* acknowledged the device's presence and configured
> it.
>
> If the source has fully configured the device, then DRC state won't be
> sent in the migration stream: for maximum migration compatibility with
> earlier versions we don't migrate DRCs in coldplug-equivalent state.
> That means that the DRC effectively changes state over the migrate,
> causing problems later on.
>
> In addition, logging hotplug events for these devices isn't what we
> want because a) those events should already have been issued on the
> source host and b) the event queue should get wiped out by the
> incoming state anyway.
>
> In short, what we really want is to treat devices added before an
> incoming migration as if they were coldplugged.
>
> To do this, we first add a spapr_drc_hotplugged() helper which
> determines if the device is hotplugged in the sense relevant for DRC
> state management. We only send hotplug events when this is true.
> Second, when we add a device which isn't hotplugged in this sense, we
> force a reset of the DRC state - this ensures the DRC is in a
> coldplug-equivalent state (there isn't usually a system reset between
> these device adds and the incoming migration).
>
> This is based on an earlier patch by Laurent Vivier, cleaned up and
> extended.
>
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
Reviewed-by: Greg Kurz <groug@kaod.org>
> hw/ppc/spapr.c | 24 ++++++++++++++++--------
> hw/ppc/spapr_drc.c | 9 ++++++---
> hw/ppc/spapr_pci.c | 4 +++-
> include/hw/ppc/spapr_drc.h | 8 ++++++++
> 4 files changed, 33 insertions(+), 12 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 12b3f09..2a059d5 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2636,6 +2636,7 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
> int i, fdt_offset, fdt_size;
> void *fdt;
> uint64_t addr = addr_start;
> + bool hotplugged = spapr_drc_hotplugged(dev);
> Error *local_err = NULL;
>
> for (i = 0; i < nr_lmbs; i++) {
> @@ -2659,12 +2660,15 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
> error_propagate(errp, local_err);
> return;
> }
> + if (!hotplugged) {
> + spapr_drc_reset(drc);
> + }
> addr += SPAPR_MEMORY_BLOCK_SIZE;
> }
> /* send hotplug notification to the
> * guest only in case of hotplugged memory
> */
> - if (dev->hotplugged) {
> + if (hotplugged) {
> if (dedicated_hp_event_source) {
> drc = spapr_drc_by_id(TYPE_SPAPR_DRC_LMB,
> addr_start / SPAPR_MEMORY_BLOCK_SIZE);
> @@ -2998,6 +3002,7 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> int smt = kvmppc_smt_threads();
> CPUArchId *core_slot;
> int index;
> + bool hotplugged = spapr_drc_hotplugged(dev);
>
> core_slot = spapr_find_cpu_slot(MACHINE(hotplug_dev), cc->core_id, &index);
> if (!core_slot) {
> @@ -3018,15 +3023,18 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> error_propagate(errp, local_err);
> return;
> }
> - }
>
> - if (dev->hotplugged) {
> - /*
> - * Send hotplug notification interrupt to the guest only in case
> - * of hotplugged CPUs.
> - */
> - spapr_hotplug_req_add_by_index(drc);
> + if (hotplugged) {
> + /*
> + * Send hotplug notification interrupt to the guest only
> + * in case of hotplugged CPUs.
> + */
> + spapr_hotplug_req_add_by_index(drc);
> + } else {
> + spapr_drc_reset(drc);
> + }
> }
> +
> core_slot->cpu = OBJECT(dev);
>
> if (smc->pre_2_10_has_unused_icps) {
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index f34355d..9b07f80 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -412,10 +412,8 @@ static bool release_pending(sPAPRDRConnector *drc)
> return drc->awaiting_release;
> }
>
> -static void drc_reset(void *opaque)
> +void spapr_drc_reset(sPAPRDRConnector *drc)
> {
> - sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(opaque);
> -
> trace_spapr_drc_reset(spapr_drc_index(drc));
>
> g_free(drc->ccs);
> @@ -447,6 +445,11 @@ static void drc_reset(void *opaque)
> }
> }
>
> +static void drc_reset(void *opaque)
> +{
> + spapr_drc_reset(SPAPR_DR_CONNECTOR(opaque));
> +}
> +
> static bool spapr_drc_needed(void *opaque)
> {
> sPAPRDRConnector *drc = (sPAPRDRConnector *)opaque;
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index a52dcf8..1e84c55 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1443,7 +1443,9 @@ static void spapr_pci_plug(HotplugHandler *plug_handler,
> /* If this is function 0, signal hotplug for all the device functions.
> * Otherwise defer sending the hotplug event.
> */
> - if (plugged_dev->hotplugged && PCI_FUNC(pdev->devfn) == 0) {
> + if (!spapr_drc_hotplugged(plugged_dev)) {
> + spapr_drc_reset(drc);
> + } else if (PCI_FUNC(pdev->devfn) == 0) {
> int i;
>
> for (i = 0; i < 8; i++) {
> diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> index d15e9eb..715016b 100644
> --- a/include/hw/ppc/spapr_drc.h
> +++ b/include/hw/ppc/spapr_drc.h
> @@ -15,6 +15,7 @@
>
> #include <libfdt.h>
> #include "qom/object.h"
> +#include "sysemu/sysemu.h"
> #include "hw/qdev.h"
>
> #define TYPE_SPAPR_DR_CONNECTOR "spapr-dr-connector"
> @@ -223,6 +224,13 @@ typedef struct sPAPRDRConnectorClass {
> bool (*release_pending)(sPAPRDRConnector *drc);
> } sPAPRDRConnectorClass;
>
> +static inline bool spapr_drc_hotplugged(DeviceState *dev)
> +{
> + return dev->hotplugged && !runstate_check(RUN_STATE_INMIGRATE);
> +}
> +
> +void spapr_drc_reset(sPAPRDRConnector *drc);
> +
> uint32_t spapr_drc_index(sPAPRDRConnector *drc);
> sPAPRDRConnectorType spapr_drc_type(sPAPRDRConnector *drc);
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCHv2 2/8] spapr: Remove 'awaiting_allocation' DRC flag
2017-07-12 5:53 [Qemu-devel] [PATCHv2 0/8] spapr: DRC cleanups (part VI) David Gibson
2017-07-12 5:53 ` [Qemu-devel] [PATCHv2 1/8] spapr: Treat devices added before inbound migration as coldplugged David Gibson
@ 2017-07-12 5:53 ` David Gibson
2017-07-12 9:38 ` Laurent Vivier
2017-07-12 10:00 ` Greg Kurz
2017-07-12 5:53 ` [Qemu-devel] [PATCHv2 3/8] spapr: Simplify unplug path David Gibson
` (7 subsequent siblings)
9 siblings, 2 replies; 30+ messages in thread
From: David Gibson @ 2017-07-12 5:53 UTC (permalink / raw)
To: mdroth, groug, danielhb
Cc: qemu-ppc, qemu-devel, lvivier, sjitindarsingh, bharata,
David Gibson
The awaiting_allocation flag in the DRC was introduced by aab9913
"spapr_drc: Prevent detach racing against attach for CPU DR", allegedly to
prevent a guest crash on racing attach and detach. Except.. information
from the BZ actually suggests a qemu crash, not a guest crash. And there
shouldn't be a problem here anyway: if the guest has already moved the DRC
away from UNUSABLE state, the detach would already be deferred, and if it
hadn't it should be safe to detach it (the guest should fail gracefully
when it attempts to change the allocation state).
I think this was probably just a bandaid for some other problem in the
state management. So, remove awaiting_allocation and associated code.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
hw/ppc/spapr_drc.c | 25 +++----------------------
include/hw/ppc/spapr_drc.h | 1 -
2 files changed, 3 insertions(+), 23 deletions(-)
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 9b07f80..89ba3d6 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -170,19 +170,13 @@ static uint32_t drc_set_usable(sPAPRDRConnector *drc)
if (!drc->dev) {
return RTAS_OUT_NO_SUCH_INDICATOR;
}
- if (drc->awaiting_release && drc->awaiting_allocation) {
- /* kernel is acknowledging a previous hotplug event
- * while we are already removing it.
- * it's safe to ignore awaiting_allocation here since we know the
- * situation is predicated on the guest either already having done
- * so (boot-time hotplug), or never being able to acquire in the
- * first place (hotplug followed by immediate unplug).
- */
+ if (drc->awaiting_release) {
+ /* Don't allow the guest to move a device away from UNUSABLE
+ * state when we want to unplug it */
return RTAS_OUT_NO_SUCH_INDICATOR;
}
drc->allocation_state = SPAPR_DR_ALLOCATION_STATE_USABLE;
- drc->awaiting_allocation = false;
return RTAS_OUT_SUCCESS;
}
@@ -357,10 +351,6 @@ void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
drc->fdt = fdt;
drc->fdt_start_offset = fdt_start_offset;
- if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
- drc->awaiting_allocation = true;
- }
-
object_property_add_link(OBJECT(drc), "device",
object_get_typename(OBJECT(drc->dev)),
(Object **)(&drc->dev),
@@ -398,12 +388,6 @@ void spapr_drc_detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp)
return;
}
- if (drc->awaiting_allocation) {
- drc->awaiting_release = true;
- trace_spapr_drc_awaiting_allocation(spapr_drc_index(drc));
- return;
- }
-
spapr_drc_release(drc);
}
@@ -426,8 +410,6 @@ void spapr_drc_reset(sPAPRDRConnector *drc)
spapr_drc_release(drc);
}
- drc->awaiting_allocation = false;
-
if (drc->dev) {
/* A device present at reset is coldplugged */
drc->isolation_state = SPAPR_DR_ISOLATION_STATE_UNISOLATED;
@@ -493,7 +475,6 @@ static const VMStateDescription vmstate_spapr_drc = {
VMSTATE_UINT32(dr_indicator, sPAPRDRConnector),
VMSTATE_BOOL(configured, sPAPRDRConnector),
VMSTATE_BOOL(awaiting_release, sPAPRDRConnector),
- VMSTATE_BOOL(awaiting_allocation, sPAPRDRConnector),
VMSTATE_END_OF_LIST()
}
};
diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
index 715016b..18a196e 100644
--- a/include/hw/ppc/spapr_drc.h
+++ b/include/hw/ppc/spapr_drc.h
@@ -200,7 +200,6 @@ typedef struct sPAPRDRConnector {
sPAPRConfigureConnectorState *ccs;
bool awaiting_release;
- bool awaiting_allocation;
/* device pointer, via link property */
DeviceState *dev;
--
2.9.4
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [Qemu-devel] [PATCHv2 2/8] spapr: Remove 'awaiting_allocation' DRC flag
2017-07-12 5:53 ` [Qemu-devel] [PATCHv2 2/8] spapr: Remove 'awaiting_allocation' DRC flag David Gibson
@ 2017-07-12 9:38 ` Laurent Vivier
2017-07-12 10:00 ` Greg Kurz
1 sibling, 0 replies; 30+ messages in thread
From: Laurent Vivier @ 2017-07-12 9:38 UTC (permalink / raw)
To: David Gibson, mdroth, groug, danielhb
Cc: qemu-ppc, qemu-devel, sjitindarsingh, bharata
On 12/07/2017 07:53, David Gibson wrote:
> The awaiting_allocation flag in the DRC was introduced by aab9913
> "spapr_drc: Prevent detach racing against attach for CPU DR", allegedly to
> prevent a guest crash on racing attach and detach. Except.. information
> from the BZ actually suggests a qemu crash, not a guest crash. And there
> shouldn't be a problem here anyway: if the guest has already moved the DRC
> away from UNUSABLE state, the detach would already be deferred, and if it
> hadn't it should be safe to detach it (the guest should fail gracefully
> when it attempts to change the allocation state).
>
> I think this was probably just a bandaid for some other problem in the
> state management. So, remove awaiting_allocation and associated code.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Laurent Vivier <lvivier@redhat.com>
> ---
> hw/ppc/spapr_drc.c | 25 +++----------------------
> include/hw/ppc/spapr_drc.h | 1 -
> 2 files changed, 3 insertions(+), 23 deletions(-)
>
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 9b07f80..89ba3d6 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -170,19 +170,13 @@ static uint32_t drc_set_usable(sPAPRDRConnector *drc)
> if (!drc->dev) {
> return RTAS_OUT_NO_SUCH_INDICATOR;
> }
> - if (drc->awaiting_release && drc->awaiting_allocation) {
> - /* kernel is acknowledging a previous hotplug event
> - * while we are already removing it.
> - * it's safe to ignore awaiting_allocation here since we know the
> - * situation is predicated on the guest either already having done
> - * so (boot-time hotplug), or never being able to acquire in the
> - * first place (hotplug followed by immediate unplug).
> - */
> + if (drc->awaiting_release) {
> + /* Don't allow the guest to move a device away from UNUSABLE
> + * state when we want to unplug it */
> return RTAS_OUT_NO_SUCH_INDICATOR;
> }
>
> drc->allocation_state = SPAPR_DR_ALLOCATION_STATE_USABLE;
> - drc->awaiting_allocation = false;
>
> return RTAS_OUT_SUCCESS;
> }
> @@ -357,10 +351,6 @@ void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
> drc->fdt = fdt;
> drc->fdt_start_offset = fdt_start_offset;
>
> - if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
> - drc->awaiting_allocation = true;
> - }
> -
> object_property_add_link(OBJECT(drc), "device",
> object_get_typename(OBJECT(drc->dev)),
> (Object **)(&drc->dev),
> @@ -398,12 +388,6 @@ void spapr_drc_detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp)
> return;
> }
>
> - if (drc->awaiting_allocation) {
> - drc->awaiting_release = true;
> - trace_spapr_drc_awaiting_allocation(spapr_drc_index(drc));
> - return;
> - }
> -
> spapr_drc_release(drc);
> }
>
> @@ -426,8 +410,6 @@ void spapr_drc_reset(sPAPRDRConnector *drc)
> spapr_drc_release(drc);
> }
>
> - drc->awaiting_allocation = false;
> -
> if (drc->dev) {
> /* A device present at reset is coldplugged */
> drc->isolation_state = SPAPR_DR_ISOLATION_STATE_UNISOLATED;
> @@ -493,7 +475,6 @@ static const VMStateDescription vmstate_spapr_drc = {
> VMSTATE_UINT32(dr_indicator, sPAPRDRConnector),
> VMSTATE_BOOL(configured, sPAPRDRConnector),
> VMSTATE_BOOL(awaiting_release, sPAPRDRConnector),
> - VMSTATE_BOOL(awaiting_allocation, sPAPRDRConnector),
> VMSTATE_END_OF_LIST()
> }
> };
> diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> index 715016b..18a196e 100644
> --- a/include/hw/ppc/spapr_drc.h
> +++ b/include/hw/ppc/spapr_drc.h
> @@ -200,7 +200,6 @@ typedef struct sPAPRDRConnector {
> sPAPRConfigureConnectorState *ccs;
>
> bool awaiting_release;
> - bool awaiting_allocation;
>
> /* device pointer, via link property */
> DeviceState *dev;
>
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [Qemu-devel] [PATCHv2 2/8] spapr: Remove 'awaiting_allocation' DRC flag
2017-07-12 5:53 ` [Qemu-devel] [PATCHv2 2/8] spapr: Remove 'awaiting_allocation' DRC flag David Gibson
2017-07-12 9:38 ` Laurent Vivier
@ 2017-07-12 10:00 ` Greg Kurz
2017-07-12 11:05 ` David Gibson
1 sibling, 1 reply; 30+ messages in thread
From: Greg Kurz @ 2017-07-12 10:00 UTC (permalink / raw)
To: David Gibson
Cc: mdroth, danielhb, qemu-ppc, qemu-devel, lvivier, sjitindarsingh,
bharata
[-- Attachment #1: Type: text/plain, Size: 4237 bytes --]
On Wed, 12 Jul 2017 15:53:11 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:
> The awaiting_allocation flag in the DRC was introduced by aab9913
> "spapr_drc: Prevent detach racing against attach for CPU DR", allegedly to
> prevent a guest crash on racing attach and detach. Except.. information
> from the BZ actually suggests a qemu crash, not a guest crash. And there
Can you share an url to the BZ ?
> shouldn't be a problem here anyway: if the guest has already moved the DRC
> away from UNUSABLE state, the detach would already be deferred, and if it
> hadn't it should be safe to detach it (the guest should fail gracefully
> when it attempts to change the allocation state).
>
> I think this was probably just a bandaid for some other problem in the
> state management. So, remove awaiting_allocation and associated code.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> hw/ppc/spapr_drc.c | 25 +++----------------------
> include/hw/ppc/spapr_drc.h | 1 -
> 2 files changed, 3 insertions(+), 23 deletions(-)
>
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 9b07f80..89ba3d6 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -170,19 +170,13 @@ static uint32_t drc_set_usable(sPAPRDRConnector *drc)
> if (!drc->dev) {
> return RTAS_OUT_NO_SUCH_INDICATOR;
> }
> - if (drc->awaiting_release && drc->awaiting_allocation) {
> - /* kernel is acknowledging a previous hotplug event
> - * while we are already removing it.
> - * it's safe to ignore awaiting_allocation here since we know the
> - * situation is predicated on the guest either already having done
> - * so (boot-time hotplug), or never being able to acquire in the
> - * first place (hotplug followed by immediate unplug).
> - */
> + if (drc->awaiting_release) {
> + /* Don't allow the guest to move a device away from UNUSABLE
> + * state when we want to unplug it */
> return RTAS_OUT_NO_SUCH_INDICATOR;
> }
>
> drc->allocation_state = SPAPR_DR_ALLOCATION_STATE_USABLE;
> - drc->awaiting_allocation = false;
>
> return RTAS_OUT_SUCCESS;
> }
> @@ -357,10 +351,6 @@ void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
> drc->fdt = fdt;
> drc->fdt_start_offset = fdt_start_offset;
>
> - if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
> - drc->awaiting_allocation = true;
> - }
> -
> object_property_add_link(OBJECT(drc), "device",
> object_get_typename(OBJECT(drc->dev)),
> (Object **)(&drc->dev),
> @@ -398,12 +388,6 @@ void spapr_drc_detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp)
> return;
> }
>
> - if (drc->awaiting_allocation) {
> - drc->awaiting_release = true;
> - trace_spapr_drc_awaiting_allocation(spapr_drc_index(drc));
> - return;
> - }
> -
> spapr_drc_release(drc);
> }
>
> @@ -426,8 +410,6 @@ void spapr_drc_reset(sPAPRDRConnector *drc)
> spapr_drc_release(drc);
> }
>
> - drc->awaiting_allocation = false;
> -
> if (drc->dev) {
> /* A device present at reset is coldplugged */
> drc->isolation_state = SPAPR_DR_ISOLATION_STATE_UNISOLATED;
> @@ -493,7 +475,6 @@ static const VMStateDescription vmstate_spapr_drc = {
> VMSTATE_UINT32(dr_indicator, sPAPRDRConnector),
> VMSTATE_BOOL(configured, sPAPRDRConnector),
> VMSTATE_BOOL(awaiting_release, sPAPRDRConnector),
> - VMSTATE_BOOL(awaiting_allocation, sPAPRDRConnector),
> VMSTATE_END_OF_LIST()
> }
> };
> diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> index 715016b..18a196e 100644
> --- a/include/hw/ppc/spapr_drc.h
> +++ b/include/hw/ppc/spapr_drc.h
> @@ -200,7 +200,6 @@ typedef struct sPAPRDRConnector {
> sPAPRConfigureConnectorState *ccs;
>
> bool awaiting_release;
> - bool awaiting_allocation;
>
> /* device pointer, via link property */
> DeviceState *dev;
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [Qemu-devel] [PATCHv2 2/8] spapr: Remove 'awaiting_allocation' DRC flag
2017-07-12 10:00 ` Greg Kurz
@ 2017-07-12 11:05 ` David Gibson
2017-07-12 11:27 ` Greg Kurz
0 siblings, 1 reply; 30+ messages in thread
From: David Gibson @ 2017-07-12 11:05 UTC (permalink / raw)
To: Greg Kurz
Cc: mdroth, danielhb, qemu-ppc, qemu-devel, lvivier, sjitindarsingh,
bharata
[-- Attachment #1: Type: text/plain, Size: 4829 bytes --]
On Wed, Jul 12, 2017 at 12:00:01PM +0200, Greg Kurz wrote:
> On Wed, 12 Jul 2017 15:53:11 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
>
> > The awaiting_allocation flag in the DRC was introduced by aab9913
> > "spapr_drc: Prevent detach racing against attach for CPU DR", allegedly to
> > prevent a guest crash on racing attach and detach. Except.. information
> > from the BZ actually suggests a qemu crash, not a guest crash. And there
>
> Can you share an url to the BZ ?
Alas, no. I have that information second hand from.. Bharata,
think.. and I believe the BZ is an IBM internal one.
>
> > shouldn't be a problem here anyway: if the guest has already moved the DRC
> > away from UNUSABLE state, the detach would already be deferred, and if it
> > hadn't it should be safe to detach it (the guest should fail gracefully
> > when it attempts to change the allocation state).
> >
> > I think this was probably just a bandaid for some other problem in the
> > state management. So, remove awaiting_allocation and associated code.
> >
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> > hw/ppc/spapr_drc.c | 25 +++----------------------
> > include/hw/ppc/spapr_drc.h | 1 -
> > 2 files changed, 3 insertions(+), 23 deletions(-)
> >
> > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> > index 9b07f80..89ba3d6 100644
> > --- a/hw/ppc/spapr_drc.c
> > +++ b/hw/ppc/spapr_drc.c
> > @@ -170,19 +170,13 @@ static uint32_t drc_set_usable(sPAPRDRConnector *drc)
> > if (!drc->dev) {
> > return RTAS_OUT_NO_SUCH_INDICATOR;
> > }
> > - if (drc->awaiting_release && drc->awaiting_allocation) {
> > - /* kernel is acknowledging a previous hotplug event
> > - * while we are already removing it.
> > - * it's safe to ignore awaiting_allocation here since we know the
> > - * situation is predicated on the guest either already having done
> > - * so (boot-time hotplug), or never being able to acquire in the
> > - * first place (hotplug followed by immediate unplug).
> > - */
> > + if (drc->awaiting_release) {
> > + /* Don't allow the guest to move a device away from UNUSABLE
> > + * state when we want to unplug it */
> > return RTAS_OUT_NO_SUCH_INDICATOR;
> > }
> >
> > drc->allocation_state = SPAPR_DR_ALLOCATION_STATE_USABLE;
> > - drc->awaiting_allocation = false;
> >
> > return RTAS_OUT_SUCCESS;
> > }
> > @@ -357,10 +351,6 @@ void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
> > drc->fdt = fdt;
> > drc->fdt_start_offset = fdt_start_offset;
> >
> > - if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
> > - drc->awaiting_allocation = true;
> > - }
> > -
> > object_property_add_link(OBJECT(drc), "device",
> > object_get_typename(OBJECT(drc->dev)),
> > (Object **)(&drc->dev),
> > @@ -398,12 +388,6 @@ void spapr_drc_detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp)
> > return;
> > }
> >
> > - if (drc->awaiting_allocation) {
> > - drc->awaiting_release = true;
> > - trace_spapr_drc_awaiting_allocation(spapr_drc_index(drc));
> > - return;
> > - }
> > -
> > spapr_drc_release(drc);
> > }
> >
> > @@ -426,8 +410,6 @@ void spapr_drc_reset(sPAPRDRConnector *drc)
> > spapr_drc_release(drc);
> > }
> >
> > - drc->awaiting_allocation = false;
> > -
> > if (drc->dev) {
> > /* A device present at reset is coldplugged */
> > drc->isolation_state = SPAPR_DR_ISOLATION_STATE_UNISOLATED;
> > @@ -493,7 +475,6 @@ static const VMStateDescription vmstate_spapr_drc = {
> > VMSTATE_UINT32(dr_indicator, sPAPRDRConnector),
> > VMSTATE_BOOL(configured, sPAPRDRConnector),
> > VMSTATE_BOOL(awaiting_release, sPAPRDRConnector),
> > - VMSTATE_BOOL(awaiting_allocation, sPAPRDRConnector),
> > VMSTATE_END_OF_LIST()
> > }
> > };
> > diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> > index 715016b..18a196e 100644
> > --- a/include/hw/ppc/spapr_drc.h
> > +++ b/include/hw/ppc/spapr_drc.h
> > @@ -200,7 +200,6 @@ typedef struct sPAPRDRConnector {
> > sPAPRConfigureConnectorState *ccs;
> >
> > bool awaiting_release;
> > - bool awaiting_allocation;
> >
> > /* device pointer, via link property */
> > 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: 819 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [Qemu-devel] [PATCHv2 2/8] spapr: Remove 'awaiting_allocation' DRC flag
2017-07-12 11:05 ` David Gibson
@ 2017-07-12 11:27 ` Greg Kurz
2017-07-12 17:04 ` Greg Kurz
0 siblings, 1 reply; 30+ messages in thread
From: Greg Kurz @ 2017-07-12 11:27 UTC (permalink / raw)
To: David Gibson
Cc: mdroth, danielhb, qemu-ppc, qemu-devel, lvivier, sjitindarsingh,
bharata
[-- Attachment #1: Type: text/plain, Size: 5284 bytes --]
On Wed, 12 Jul 2017 21:05:45 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:
> On Wed, Jul 12, 2017 at 12:00:01PM +0200, Greg Kurz wrote:
> > On Wed, 12 Jul 2017 15:53:11 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >
> > > The awaiting_allocation flag in the DRC was introduced by aab9913
> > > "spapr_drc: Prevent detach racing against attach for CPU DR", allegedly to
> > > prevent a guest crash on racing attach and detach. Except.. information
> > > from the BZ actually suggests a qemu crash, not a guest crash. And there
> >
> > Can you share an url to the BZ ?
>
> Alas, no. I have that information second hand from.. Bharata,
> think.. and I believe the BZ is an IBM internal one.
>
I did some digging and I could find it in our internal bugzilla. And indeed,
it mentions QEMU crashing because of a SEGV...
> >
> > > shouldn't be a problem here anyway: if the guest has already moved the DRC
> > > away from UNUSABLE state, the detach would already be deferred, and if it
> > > hadn't it should be safe to detach it (the guest should fail gracefully
> > > when it attempts to change the allocation state).
> > >
> > > I think this was probably just a bandaid for some other problem in the
> > > state management. So, remove awaiting_allocation and associated code.
> > >
... so I tend to agree this was a bandaid.
Reviewed-by: Greg Kurz <groug@kaod.org>
I'll re-try the scenario from the BZ with this patch applied to see if we
hit the crash again.
> > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > ---
> > > hw/ppc/spapr_drc.c | 25 +++----------------------
> > > include/hw/ppc/spapr_drc.h | 1 -
> > > 2 files changed, 3 insertions(+), 23 deletions(-)
> > >
> > > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> > > index 9b07f80..89ba3d6 100644
> > > --- a/hw/ppc/spapr_drc.c
> > > +++ b/hw/ppc/spapr_drc.c
> > > @@ -170,19 +170,13 @@ static uint32_t drc_set_usable(sPAPRDRConnector *drc)
> > > if (!drc->dev) {
> > > return RTAS_OUT_NO_SUCH_INDICATOR;
> > > }
> > > - if (drc->awaiting_release && drc->awaiting_allocation) {
> > > - /* kernel is acknowledging a previous hotplug event
> > > - * while we are already removing it.
> > > - * it's safe to ignore awaiting_allocation here since we know the
> > > - * situation is predicated on the guest either already having done
> > > - * so (boot-time hotplug), or never being able to acquire in the
> > > - * first place (hotplug followed by immediate unplug).
> > > - */
> > > + if (drc->awaiting_release) {
> > > + /* Don't allow the guest to move a device away from UNUSABLE
> > > + * state when we want to unplug it */
> > > return RTAS_OUT_NO_SUCH_INDICATOR;
> > > }
> > >
> > > drc->allocation_state = SPAPR_DR_ALLOCATION_STATE_USABLE;
> > > - drc->awaiting_allocation = false;
> > >
> > > return RTAS_OUT_SUCCESS;
> > > }
> > > @@ -357,10 +351,6 @@ void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
> > > drc->fdt = fdt;
> > > drc->fdt_start_offset = fdt_start_offset;
> > >
> > > - if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
> > > - drc->awaiting_allocation = true;
> > > - }
> > > -
> > > object_property_add_link(OBJECT(drc), "device",
> > > object_get_typename(OBJECT(drc->dev)),
> > > (Object **)(&drc->dev),
> > > @@ -398,12 +388,6 @@ void spapr_drc_detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp)
> > > return;
> > > }
> > >
> > > - if (drc->awaiting_allocation) {
> > > - drc->awaiting_release = true;
> > > - trace_spapr_drc_awaiting_allocation(spapr_drc_index(drc));
> > > - return;
> > > - }
> > > -
> > > spapr_drc_release(drc);
> > > }
> > >
> > > @@ -426,8 +410,6 @@ void spapr_drc_reset(sPAPRDRConnector *drc)
> > > spapr_drc_release(drc);
> > > }
> > >
> > > - drc->awaiting_allocation = false;
> > > -
> > > if (drc->dev) {
> > > /* A device present at reset is coldplugged */
> > > drc->isolation_state = SPAPR_DR_ISOLATION_STATE_UNISOLATED;
> > > @@ -493,7 +475,6 @@ static const VMStateDescription vmstate_spapr_drc = {
> > > VMSTATE_UINT32(dr_indicator, sPAPRDRConnector),
> > > VMSTATE_BOOL(configured, sPAPRDRConnector),
> > > VMSTATE_BOOL(awaiting_release, sPAPRDRConnector),
> > > - VMSTATE_BOOL(awaiting_allocation, sPAPRDRConnector),
> > > VMSTATE_END_OF_LIST()
> > > }
> > > };
> > > diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> > > index 715016b..18a196e 100644
> > > --- a/include/hw/ppc/spapr_drc.h
> > > +++ b/include/hw/ppc/spapr_drc.h
> > > @@ -200,7 +200,6 @@ typedef struct sPAPRDRConnector {
> > > sPAPRConfigureConnectorState *ccs;
> > >
> > > bool awaiting_release;
> > > - bool awaiting_allocation;
> > >
> > > /* device pointer, via link property */
> > > DeviceState *dev;
> >
>
>
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [Qemu-devel] [PATCHv2 2/8] spapr: Remove 'awaiting_allocation' DRC flag
2017-07-12 11:27 ` Greg Kurz
@ 2017-07-12 17:04 ` Greg Kurz
0 siblings, 0 replies; 30+ messages in thread
From: Greg Kurz @ 2017-07-12 17:04 UTC (permalink / raw)
To: David Gibson
Cc: mdroth, danielhb, qemu-ppc, qemu-devel, lvivier, sjitindarsingh,
bharata, Shivaprasad G Bhat
[-- Attachment #1: Type: text/plain, Size: 6992 bytes --]
On Wed, 12 Jul 2017 13:27:42 +0200
Greg Kurz <groug@kaod.org> wrote:
> On Wed, 12 Jul 2017 21:05:45 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
>
> > On Wed, Jul 12, 2017 at 12:00:01PM +0200, Greg Kurz wrote:
> > > On Wed, 12 Jul 2017 15:53:11 +1000
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >
> > > > The awaiting_allocation flag in the DRC was introduced by aab9913
> > > > "spapr_drc: Prevent detach racing against attach for CPU DR", allegedly to
> > > > prevent a guest crash on racing attach and detach. Except.. information
> > > > from the BZ actually suggests a qemu crash, not a guest crash. And there
> > >
> > > Can you share an url to the BZ ?
> >
> > Alas, no. I have that information second hand from.. Bharata,
> > think.. and I believe the BZ is an IBM internal one.
> >
>
> I did some digging and I could find it in our internal bugzilla. And indeed,
> it mentions QEMU crashing because of a SEGV...
>
> > >
> > > > shouldn't be a problem here anyway: if the guest has already moved the DRC
> > > > away from UNUSABLE state, the detach would already be deferred, and if it
> > > > hadn't it should be safe to detach it (the guest should fail gracefully
> > > > when it attempts to change the allocation state).
> > > >
> > > > I think this was probably just a bandaid for some other problem in the
> > > > state management. So, remove awaiting_allocation and associated code.
> > > >
>
> ... so I tend to agree this was a bandaid.
>
> Reviewed-by: Greg Kurz <groug@kaod.org>
>
> I'll re-try the scenario from the BZ with this patch applied to see if we
> hit the crash again.
>
The crashing scenario was basically start a guest using libvirt, to add
a bunch of vCPUs and then remove them.
I've been running the addition/removal in a loop (using 'virsh setvcpus') and
no QEMU crash is observed, with or without this patch.
But, without this patch I would quickly (ie, 8 invocations of setvcpus) reach
a situation where:
- 'virsh setvcpus' would time out and fail forever from now on
- 'lscpu' in the guest and 'virsh vcpuinfo' show different number of vCPUs
With this patch applied, I could run the loop much much longer, and only
observe a 'virsh setvcpus' timeout from time to time. But at some point,
'virsh setvcpus' would fail consecutively 3 or 4 times with:
error: internal error: qemu didn't report thread id for vcpu '8'
and then fail with the following error forever:
error: unsupported configuration: failed to find appropriate hotpluggable
vcpus to reach the desired target vcpu count
'lscpu' and 'virsh vcpuinfo' also produce inconsistent output.
I could continue to hotplug/unplug vCPUs with the QEMU monitor though,
so I guess there's something wrong in libvirt. I've asked Shiva (on Cc)
to help on the investigation.
Anyway, this patch seems to improve things and doesn't cause QEMU
to crash. I thus maintain my R-b and you can add:
Tested-by: Greg Kurz <groug@kaod.org>
> > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > > ---
> > > > hw/ppc/spapr_drc.c | 25 +++----------------------
> > > > include/hw/ppc/spapr_drc.h | 1 -
> > > > 2 files changed, 3 insertions(+), 23 deletions(-)
> > > >
> > > > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> > > > index 9b07f80..89ba3d6 100644
> > > > --- a/hw/ppc/spapr_drc.c
> > > > +++ b/hw/ppc/spapr_drc.c
> > > > @@ -170,19 +170,13 @@ static uint32_t drc_set_usable(sPAPRDRConnector *drc)
> > > > if (!drc->dev) {
> > > > return RTAS_OUT_NO_SUCH_INDICATOR;
> > > > }
> > > > - if (drc->awaiting_release && drc->awaiting_allocation) {
> > > > - /* kernel is acknowledging a previous hotplug event
> > > > - * while we are already removing it.
> > > > - * it's safe to ignore awaiting_allocation here since we know the
> > > > - * situation is predicated on the guest either already having done
> > > > - * so (boot-time hotplug), or never being able to acquire in the
> > > > - * first place (hotplug followed by immediate unplug).
> > > > - */
> > > > + if (drc->awaiting_release) {
> > > > + /* Don't allow the guest to move a device away from UNUSABLE
> > > > + * state when we want to unplug it */
> > > > return RTAS_OUT_NO_SUCH_INDICATOR;
> > > > }
> > > >
> > > > drc->allocation_state = SPAPR_DR_ALLOCATION_STATE_USABLE;
> > > > - drc->awaiting_allocation = false;
> > > >
> > > > return RTAS_OUT_SUCCESS;
> > > > }
> > > > @@ -357,10 +351,6 @@ void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
> > > > drc->fdt = fdt;
> > > > drc->fdt_start_offset = fdt_start_offset;
> > > >
> > > > - if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
> > > > - drc->awaiting_allocation = true;
> > > > - }
> > > > -
> > > > object_property_add_link(OBJECT(drc), "device",
> > > > object_get_typename(OBJECT(drc->dev)),
> > > > (Object **)(&drc->dev),
> > > > @@ -398,12 +388,6 @@ void spapr_drc_detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp)
> > > > return;
> > > > }
> > > >
> > > > - if (drc->awaiting_allocation) {
> > > > - drc->awaiting_release = true;
> > > > - trace_spapr_drc_awaiting_allocation(spapr_drc_index(drc));
> > > > - return;
> > > > - }
> > > > -
> > > > spapr_drc_release(drc);
> > > > }
> > > >
> > > > @@ -426,8 +410,6 @@ void spapr_drc_reset(sPAPRDRConnector *drc)
> > > > spapr_drc_release(drc);
> > > > }
> > > >
> > > > - drc->awaiting_allocation = false;
> > > > -
> > > > if (drc->dev) {
> > > > /* A device present at reset is coldplugged */
> > > > drc->isolation_state = SPAPR_DR_ISOLATION_STATE_UNISOLATED;
> > > > @@ -493,7 +475,6 @@ static const VMStateDescription vmstate_spapr_drc = {
> > > > VMSTATE_UINT32(dr_indicator, sPAPRDRConnector),
> > > > VMSTATE_BOOL(configured, sPAPRDRConnector),
> > > > VMSTATE_BOOL(awaiting_release, sPAPRDRConnector),
> > > > - VMSTATE_BOOL(awaiting_allocation, sPAPRDRConnector),
> > > > VMSTATE_END_OF_LIST()
> > > > }
> > > > };
> > > > diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> > > > index 715016b..18a196e 100644
> > > > --- a/include/hw/ppc/spapr_drc.h
> > > > +++ b/include/hw/ppc/spapr_drc.h
> > > > @@ -200,7 +200,6 @@ typedef struct sPAPRDRConnector {
> > > > sPAPRConfigureConnectorState *ccs;
> > > >
> > > > bool awaiting_release;
> > > > - bool awaiting_allocation;
> > > >
> > > > /* device pointer, via link property */
> > > > DeviceState *dev;
> > >
> >
> >
> >
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCHv2 3/8] spapr: Simplify unplug path
2017-07-12 5:53 [Qemu-devel] [PATCHv2 0/8] spapr: DRC cleanups (part VI) David Gibson
2017-07-12 5:53 ` [Qemu-devel] [PATCHv2 1/8] spapr: Treat devices added before inbound migration as coldplugged David Gibson
2017-07-12 5:53 ` [Qemu-devel] [PATCHv2 2/8] spapr: Remove 'awaiting_allocation' DRC flag David Gibson
@ 2017-07-12 5:53 ` David Gibson
2017-07-12 10:04 ` Greg Kurz
2017-07-12 5:53 ` [Qemu-devel] [PATCHv2 4/8] spapr: Refactor spapr_drc_detach() David Gibson
` (6 subsequent siblings)
9 siblings, 1 reply; 30+ messages in thread
From: David Gibson @ 2017-07-12 5:53 UTC (permalink / raw)
To: mdroth, groug, danielhb
Cc: qemu-ppc, qemu-devel, lvivier, sjitindarsingh, bharata,
David Gibson
spapr_lmb_release() and spapr_core_release() call hotplug_handler_unplug()
which after a bunch of indirection calls spapr_memory_unplug() or
spapr_core_unplug(). But we already know which is the appropriate thing
to call here, so we can just fold it directly into the release function.
Once that's done, there's no need for an hc->unplug method in the spapr
machine at all: since we also have an hc->unplug_request method, the
hotplug core will never use ->unplug.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
hw/ppc/spapr.c | 54 ++++++++----------------------------------------------
1 file changed, 8 insertions(+), 46 deletions(-)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 2a059d5..ff2aa6b 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2818,6 +2818,9 @@ void spapr_lmb_release(DeviceState *dev)
{
HotplugHandler *hotplug_ctrl = qdev_get_hotplug_handler(dev);
sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_ctrl);
+ PCDIMMDevice *dimm = PC_DIMM(dev);
+ PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
+ MemoryRegion *mr = ddc->get_memory_region(dimm);
sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, PC_DIMM(dev));
/* This information will get lost if a migration occurs
@@ -2838,18 +2841,7 @@ void spapr_lmb_release(DeviceState *dev)
* Now that all the LMBs have been removed by the guest, call the
* pc-dimm unplug handler to cleanup up the pc-dimm device.
*/
- hotplug_handler_unplug(hotplug_ctrl, dev, &error_abort);
-}
-
-static void spapr_memory_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
- Error **errp)
-{
- sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev);
- PCDIMMDevice *dimm = PC_DIMM(dev);
- PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
- MemoryRegion *mr = ddc->get_memory_region(dimm);
-
- pc_dimm_memory_unplug(dev, &ms->hotplug_memory, mr);
+ pc_dimm_memory_unplug(dev, &spapr->hotplug_memory, mr);
object_unparent(OBJECT(dev));
}
@@ -2918,10 +2910,11 @@ static void *spapr_populate_hotplug_cpu_dt(CPUState *cs, int *fdt_offset,
return fdt;
}
-static void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
- Error **errp)
+/* Callback to be called during DRC release. */
+void spapr_core_release(DeviceState *dev)
{
- MachineState *ms = MACHINE(qdev_get_machine());
+ HotplugHandler *hotplug_ctrl = qdev_get_hotplug_handler(dev);
+ MachineState *ms = MACHINE(hotplug_ctrl);
sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
CPUCore *cc = CPU_CORE(dev);
CPUArchId *core_slot = spapr_find_cpu_slot(ms, cc->core_id, NULL);
@@ -2945,15 +2938,6 @@ static void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
object_unparent(OBJECT(dev));
}
-/* Callback to be called during DRC release. */
-void spapr_core_release(DeviceState *dev)
-{
- HotplugHandler *hotplug_ctrl;
-
- hotplug_ctrl = qdev_get_hotplug_handler(dev);
- hotplug_handler_unplug(hotplug_ctrl, dev, &error_abort);
-}
-
static
void spapr_core_unplug_request(HotplugHandler *hotplug_dev, DeviceState *dev,
Error **errp)
@@ -3159,27 +3143,6 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
}
}
-static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
- DeviceState *dev, Error **errp)
-{
- sPAPRMachineState *sms = SPAPR_MACHINE(qdev_get_machine());
- MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
-
- if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
- if (spapr_ovec_test(sms->ov5_cas, OV5_HP_EVT)) {
- spapr_memory_unplug(hotplug_dev, dev, errp);
- } else {
- error_setg(errp, "Memory hot unplug not supported for this guest");
- }
- } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
- if (!mc->has_hotpluggable_cpus) {
- error_setg(errp, "CPU hot unplug not supported on this machine");
- return;
- }
- spapr_core_unplug(hotplug_dev, dev, errp);
- }
-}
-
static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,
DeviceState *dev, Error **errp)
{
@@ -3397,7 +3360,6 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
mc->get_hotplug_handler = spapr_get_hotplug_handler;
hc->pre_plug = spapr_machine_device_pre_plug;
hc->plug = spapr_machine_device_plug;
- hc->unplug = spapr_machine_device_unplug;
mc->cpu_index_to_instance_props = spapr_cpu_index_to_props;
mc->possible_cpu_arch_ids = spapr_possible_cpu_arch_ids;
hc->unplug_request = spapr_machine_device_unplug_request;
--
2.9.4
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [Qemu-devel] [PATCHv2 3/8] spapr: Simplify unplug path
2017-07-12 5:53 ` [Qemu-devel] [PATCHv2 3/8] spapr: Simplify unplug path David Gibson
@ 2017-07-12 10:04 ` Greg Kurz
2017-07-12 10:31 ` Greg Kurz
0 siblings, 1 reply; 30+ messages in thread
From: Greg Kurz @ 2017-07-12 10:04 UTC (permalink / raw)
To: David Gibson
Cc: mdroth, danielhb, qemu-ppc, qemu-devel, lvivier, sjitindarsingh,
bharata
[-- Attachment #1: Type: text/plain, Size: 5317 bytes --]
On Wed, 12 Jul 2017 15:53:12 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:
> spapr_lmb_release() and spapr_core_release() call hotplug_handler_unplug()
> which after a bunch of indirection calls spapr_memory_unplug() or
> spapr_core_unplug(). But we already know which is the appropriate thing
> to call here, so we can just fold it directly into the release function.
>
> Once that's done, there's no need for an hc->unplug method in the spapr
> machine at all: since we also have an hc->unplug_request method, the
> hotplug core will never use ->unplug.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
Nice cleanup :)
Reviewed-by: Greg Kurz <groug@kaod.org>
> hw/ppc/spapr.c | 54 ++++++++----------------------------------------------
> 1 file changed, 8 insertions(+), 46 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 2a059d5..ff2aa6b 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2818,6 +2818,9 @@ void spapr_lmb_release(DeviceState *dev)
> {
> HotplugHandler *hotplug_ctrl = qdev_get_hotplug_handler(dev);
> sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_ctrl);
> + PCDIMMDevice *dimm = PC_DIMM(dev);
> + PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> + MemoryRegion *mr = ddc->get_memory_region(dimm);
> sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, PC_DIMM(dev));
>
> /* This information will get lost if a migration occurs
> @@ -2838,18 +2841,7 @@ void spapr_lmb_release(DeviceState *dev)
> * Now that all the LMBs have been removed by the guest, call the
> * pc-dimm unplug handler to cleanup up the pc-dimm device.
> */
> - hotplug_handler_unplug(hotplug_ctrl, dev, &error_abort);
> -}
> -
> -static void spapr_memory_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
> - Error **errp)
> -{
> - sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev);
> - PCDIMMDevice *dimm = PC_DIMM(dev);
> - PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> - MemoryRegion *mr = ddc->get_memory_region(dimm);
> -
> - pc_dimm_memory_unplug(dev, &ms->hotplug_memory, mr);
> + pc_dimm_memory_unplug(dev, &spapr->hotplug_memory, mr);
> object_unparent(OBJECT(dev));
> }
>
> @@ -2918,10 +2910,11 @@ static void *spapr_populate_hotplug_cpu_dt(CPUState *cs, int *fdt_offset,
> return fdt;
> }
>
> -static void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
> - Error **errp)
> +/* Callback to be called during DRC release. */
> +void spapr_core_release(DeviceState *dev)
> {
> - MachineState *ms = MACHINE(qdev_get_machine());
> + HotplugHandler *hotplug_ctrl = qdev_get_hotplug_handler(dev);
> + MachineState *ms = MACHINE(hotplug_ctrl);
> sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
> CPUCore *cc = CPU_CORE(dev);
> CPUArchId *core_slot = spapr_find_cpu_slot(ms, cc->core_id, NULL);
> @@ -2945,15 +2938,6 @@ static void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
> object_unparent(OBJECT(dev));
> }
>
> -/* Callback to be called during DRC release. */
> -void spapr_core_release(DeviceState *dev)
> -{
> - HotplugHandler *hotplug_ctrl;
> -
> - hotplug_ctrl = qdev_get_hotplug_handler(dev);
> - hotplug_handler_unplug(hotplug_ctrl, dev, &error_abort);
> -}
> -
> static
> void spapr_core_unplug_request(HotplugHandler *hotplug_dev, DeviceState *dev,
> Error **errp)
> @@ -3159,27 +3143,6 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
> }
> }
>
> -static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
> - DeviceState *dev, Error **errp)
> -{
> - sPAPRMachineState *sms = SPAPR_MACHINE(qdev_get_machine());
> - MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
> -
> - if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> - if (spapr_ovec_test(sms->ov5_cas, OV5_HP_EVT)) {
> - spapr_memory_unplug(hotplug_dev, dev, errp);
> - } else {
> - error_setg(errp, "Memory hot unplug not supported for this guest");
> - }
> - } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> - if (!mc->has_hotpluggable_cpus) {
> - error_setg(errp, "CPU hot unplug not supported on this machine");
> - return;
> - }
> - spapr_core_unplug(hotplug_dev, dev, errp);
> - }
> -}
> -
> static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,
> DeviceState *dev, Error **errp)
> {
> @@ -3397,7 +3360,6 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
> mc->get_hotplug_handler = spapr_get_hotplug_handler;
> hc->pre_plug = spapr_machine_device_pre_plug;
> hc->plug = spapr_machine_device_plug;
> - hc->unplug = spapr_machine_device_unplug;
> mc->cpu_index_to_instance_props = spapr_cpu_index_to_props;
> mc->possible_cpu_arch_ids = spapr_possible_cpu_arch_ids;
> hc->unplug_request = spapr_machine_device_unplug_request;
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [Qemu-devel] [PATCHv2 3/8] spapr: Simplify unplug path
2017-07-12 10:04 ` Greg Kurz
@ 2017-07-12 10:31 ` Greg Kurz
2017-07-13 0:30 ` David Gibson
0 siblings, 1 reply; 30+ messages in thread
From: Greg Kurz @ 2017-07-12 10:31 UTC (permalink / raw)
To: David Gibson
Cc: mdroth, danielhb, qemu-ppc, qemu-devel, lvivier, sjitindarsingh,
bharata
[-- Attachment #1: Type: text/plain, Size: 5757 bytes --]
On Wed, 12 Jul 2017 12:04:51 +0200
Greg Kurz <groug@kaod.org> wrote:
> On Wed, 12 Jul 2017 15:53:12 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
>
> > spapr_lmb_release() and spapr_core_release() call hotplug_handler_unplug()
> > which after a bunch of indirection calls spapr_memory_unplug() or
> > spapr_core_unplug(). But we already know which is the appropriate thing
> > to call here, so we can just fold it directly into the release function.
> >
> > Once that's done, there's no need for an hc->unplug method in the spapr
> > machine at all: since we also have an hc->unplug_request method, the
> > hotplug core will never use ->unplug.
> >
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
>
> Nice cleanup :)
>
> Reviewed-by: Greg Kurz <groug@kaod.org>
>
> > hw/ppc/spapr.c | 54 ++++++++----------------------------------------------
> > 1 file changed, 8 insertions(+), 46 deletions(-)
> >
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 2a059d5..ff2aa6b 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -2818,6 +2818,9 @@ void spapr_lmb_release(DeviceState *dev)
> > {
> > HotplugHandler *hotplug_ctrl = qdev_get_hotplug_handler(dev);
> > sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_ctrl);
BTW, maybe you can also drop the hotplug_ctrl variable since it only has
one trivial user.
> > + PCDIMMDevice *dimm = PC_DIMM(dev);
> > + PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> > + MemoryRegion *mr = ddc->get_memory_region(dimm);
> > sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, PC_DIMM(dev));
> >
> > /* This information will get lost if a migration occurs
> > @@ -2838,18 +2841,7 @@ void spapr_lmb_release(DeviceState *dev)
> > * Now that all the LMBs have been removed by the guest, call the
> > * pc-dimm unplug handler to cleanup up the pc-dimm device.
> > */
> > - hotplug_handler_unplug(hotplug_ctrl, dev, &error_abort);
> > -}
> > -
> > -static void spapr_memory_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > - Error **errp)
> > -{
> > - sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev);
> > - PCDIMMDevice *dimm = PC_DIMM(dev);
> > - PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> > - MemoryRegion *mr = ddc->get_memory_region(dimm);
> > -
> > - pc_dimm_memory_unplug(dev, &ms->hotplug_memory, mr);
> > + pc_dimm_memory_unplug(dev, &spapr->hotplug_memory, mr);
> > object_unparent(OBJECT(dev));
> > }
> >
> > @@ -2918,10 +2910,11 @@ static void *spapr_populate_hotplug_cpu_dt(CPUState *cs, int *fdt_offset,
> > return fdt;
> > }
> >
> > -static void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > - Error **errp)
> > +/* Callback to be called during DRC release. */
> > +void spapr_core_release(DeviceState *dev)
> > {
> > - MachineState *ms = MACHINE(qdev_get_machine());
> > + HotplugHandler *hotplug_ctrl = qdev_get_hotplug_handler(dev);
> > + MachineState *ms = MACHINE(hotplug_ctrl);
Same here.
> > sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
> > CPUCore *cc = CPU_CORE(dev);
> > CPUArchId *core_slot = spapr_find_cpu_slot(ms, cc->core_id, NULL);
> > @@ -2945,15 +2938,6 @@ static void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > object_unparent(OBJECT(dev));
> > }
> >
> > -/* Callback to be called during DRC release. */
> > -void spapr_core_release(DeviceState *dev)
> > -{
> > - HotplugHandler *hotplug_ctrl;
> > -
> > - hotplug_ctrl = qdev_get_hotplug_handler(dev);
> > - hotplug_handler_unplug(hotplug_ctrl, dev, &error_abort);
> > -}
> > -
> > static
> > void spapr_core_unplug_request(HotplugHandler *hotplug_dev, DeviceState *dev,
> > Error **errp)
> > @@ -3159,27 +3143,6 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
> > }
> > }
> >
> > -static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
> > - DeviceState *dev, Error **errp)
> > -{
> > - sPAPRMachineState *sms = SPAPR_MACHINE(qdev_get_machine());
> > - MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
> > -
> > - if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> > - if (spapr_ovec_test(sms->ov5_cas, OV5_HP_EVT)) {
> > - spapr_memory_unplug(hotplug_dev, dev, errp);
> > - } else {
> > - error_setg(errp, "Memory hot unplug not supported for this guest");
> > - }
> > - } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> > - if (!mc->has_hotpluggable_cpus) {
> > - error_setg(errp, "CPU hot unplug not supported on this machine");
> > - return;
> > - }
> > - spapr_core_unplug(hotplug_dev, dev, errp);
> > - }
> > -}
> > -
> > static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,
> > DeviceState *dev, Error **errp)
> > {
> > @@ -3397,7 +3360,6 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
> > mc->get_hotplug_handler = spapr_get_hotplug_handler;
> > hc->pre_plug = spapr_machine_device_pre_plug;
> > hc->plug = spapr_machine_device_plug;
> > - hc->unplug = spapr_machine_device_unplug;
> > mc->cpu_index_to_instance_props = spapr_cpu_index_to_props;
> > mc->possible_cpu_arch_ids = spapr_possible_cpu_arch_ids;
> > hc->unplug_request = spapr_machine_device_unplug_request;
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [Qemu-devel] [PATCHv2 3/8] spapr: Simplify unplug path
2017-07-12 10:31 ` Greg Kurz
@ 2017-07-13 0:30 ` David Gibson
0 siblings, 0 replies; 30+ messages in thread
From: David Gibson @ 2017-07-13 0:30 UTC (permalink / raw)
To: Greg Kurz
Cc: mdroth, danielhb, qemu-ppc, qemu-devel, lvivier, sjitindarsingh,
bharata
[-- Attachment #1: Type: text/plain, Size: 6309 bytes --]
On Wed, Jul 12, 2017 at 12:31:48PM +0200, Greg Kurz wrote:
> On Wed, 12 Jul 2017 12:04:51 +0200
> Greg Kurz <groug@kaod.org> wrote:
>
> > On Wed, 12 Jul 2017 15:53:12 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >
> > > spapr_lmb_release() and spapr_core_release() call hotplug_handler_unplug()
> > > which after a bunch of indirection calls spapr_memory_unplug() or
> > > spapr_core_unplug(). But we already know which is the appropriate thing
> > > to call here, so we can just fold it directly into the release function.
> > >
> > > Once that's done, there's no need for an hc->unplug method in the spapr
> > > machine at all: since we also have an hc->unplug_request method, the
> > > hotplug core will never use ->unplug.
> > >
> > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > ---
> >
> > Nice cleanup :)
> >
> > Reviewed-by: Greg Kurz <groug@kaod.org>
> >
> > > hw/ppc/spapr.c | 54 ++++++++----------------------------------------------
> > > 1 file changed, 8 insertions(+), 46 deletions(-)
> > >
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index 2a059d5..ff2aa6b 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -2818,6 +2818,9 @@ void spapr_lmb_release(DeviceState *dev)
> > > {
> > > HotplugHandler *hotplug_ctrl = qdev_get_hotplug_handler(dev);
> > > sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_ctrl);
>
> BTW, maybe you can also drop the hotplug_ctrl variable since it only has
> one trivial user.
Good idea, done.
>
> > > + PCDIMMDevice *dimm = PC_DIMM(dev);
> > > + PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> > > + MemoryRegion *mr = ddc->get_memory_region(dimm);
> > > sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, PC_DIMM(dev));
> > >
> > > /* This information will get lost if a migration occurs
> > > @@ -2838,18 +2841,7 @@ void spapr_lmb_release(DeviceState *dev)
> > > * Now that all the LMBs have been removed by the guest, call the
> > > * pc-dimm unplug handler to cleanup up the pc-dimm device.
> > > */
> > > - hotplug_handler_unplug(hotplug_ctrl, dev, &error_abort);
> > > -}
> > > -
> > > -static void spapr_memory_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > > - Error **errp)
> > > -{
> > > - sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev);
> > > - PCDIMMDevice *dimm = PC_DIMM(dev);
> > > - PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> > > - MemoryRegion *mr = ddc->get_memory_region(dimm);
> > > -
> > > - pc_dimm_memory_unplug(dev, &ms->hotplug_memory, mr);
> > > + pc_dimm_memory_unplug(dev, &spapr->hotplug_memory, mr);
> > > object_unparent(OBJECT(dev));
> > > }
> > >
> > > @@ -2918,10 +2910,11 @@ static void *spapr_populate_hotplug_cpu_dt(CPUState *cs, int *fdt_offset,
> > > return fdt;
> > > }
> > >
> > > -static void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > > - Error **errp)
> > > +/* Callback to be called during DRC release. */
> > > +void spapr_core_release(DeviceState *dev)
> > > {
> > > - MachineState *ms = MACHINE(qdev_get_machine());
> > > + HotplugHandler *hotplug_ctrl = qdev_get_hotplug_handler(dev);
> > > + MachineState *ms = MACHINE(hotplug_ctrl);
>
> Same here.
>
> > > sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
> > > CPUCore *cc = CPU_CORE(dev);
> > > CPUArchId *core_slot = spapr_find_cpu_slot(ms, cc->core_id, NULL);
> > > @@ -2945,15 +2938,6 @@ static void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > > object_unparent(OBJECT(dev));
> > > }
> > >
> > > -/* Callback to be called during DRC release. */
> > > -void spapr_core_release(DeviceState *dev)
> > > -{
> > > - HotplugHandler *hotplug_ctrl;
> > > -
> > > - hotplug_ctrl = qdev_get_hotplug_handler(dev);
> > > - hotplug_handler_unplug(hotplug_ctrl, dev, &error_abort);
> > > -}
> > > -
> > > static
> > > void spapr_core_unplug_request(HotplugHandler *hotplug_dev, DeviceState *dev,
> > > Error **errp)
> > > @@ -3159,27 +3143,6 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
> > > }
> > > }
> > >
> > > -static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
> > > - DeviceState *dev, Error **errp)
> > > -{
> > > - sPAPRMachineState *sms = SPAPR_MACHINE(qdev_get_machine());
> > > - MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
> > > -
> > > - if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> > > - if (spapr_ovec_test(sms->ov5_cas, OV5_HP_EVT)) {
> > > - spapr_memory_unplug(hotplug_dev, dev, errp);
> > > - } else {
> > > - error_setg(errp, "Memory hot unplug not supported for this guest");
> > > - }
> > > - } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> > > - if (!mc->has_hotpluggable_cpus) {
> > > - error_setg(errp, "CPU hot unplug not supported on this machine");
> > > - return;
> > > - }
> > > - spapr_core_unplug(hotplug_dev, dev, errp);
> > > - }
> > > -}
> > > -
> > > static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,
> > > DeviceState *dev, Error **errp)
> > > {
> > > @@ -3397,7 +3360,6 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
> > > mc->get_hotplug_handler = spapr_get_hotplug_handler;
> > > hc->pre_plug = spapr_machine_device_pre_plug;
> > > hc->plug = spapr_machine_device_plug;
> > > - hc->unplug = spapr_machine_device_unplug;
> > > mc->cpu_index_to_instance_props = spapr_cpu_index_to_props;
> > > mc->possible_cpu_arch_ids = spapr_possible_cpu_arch_ids;
> > > hc->unplug_request = spapr_machine_device_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: 819 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCHv2 4/8] spapr: Refactor spapr_drc_detach()
2017-07-12 5:53 [Qemu-devel] [PATCHv2 0/8] spapr: DRC cleanups (part VI) David Gibson
` (2 preceding siblings ...)
2017-07-12 5:53 ` [Qemu-devel] [PATCHv2 3/8] spapr: Simplify unplug path David Gibson
@ 2017-07-12 5:53 ` David Gibson
2017-07-12 11:47 ` Greg Kurz
2017-07-12 5:53 ` [Qemu-devel] [PATCHv2 5/8] spapr: Cleanups relating to DRC awaiting_release field David Gibson
` (5 subsequent siblings)
9 siblings, 1 reply; 30+ messages in thread
From: David Gibson @ 2017-07-12 5:53 UTC (permalink / raw)
To: mdroth, groug, danielhb
Cc: qemu-ppc, qemu-devel, lvivier, sjitindarsingh, bharata,
David Gibson
This function has two unused parameters - remove them.
It also sets awaiting_release on all paths, except one. On that path
setting it is harmless, since it will be immediately cleared by
spapr_drc_release(). So factor it out of the if statements.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
hw/ppc/spapr.c | 11 +++--------
hw/ppc/spapr_drc.c | 12 ++++++------
hw/ppc/spapr_pci.c | 7 +------
include/hw/ppc/spapr_drc.h | 2 +-
4 files changed, 11 insertions(+), 21 deletions(-)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index ff2aa6b..5c528e2 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2654,7 +2654,7 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
addr -= SPAPR_MEMORY_BLOCK_SIZE;
drc = spapr_drc_by_id(TYPE_SPAPR_DRC_LMB,
addr / SPAPR_MEMORY_BLOCK_SIZE);
- spapr_drc_detach(drc, dev, NULL);
+ spapr_drc_detach(drc);
}
g_free(fdt);
error_propagate(errp, local_err);
@@ -2877,7 +2877,7 @@ static void spapr_memory_unplug_request(HotplugHandler *hotplug_dev,
addr / SPAPR_MEMORY_BLOCK_SIZE);
g_assert(drc);
- spapr_drc_detach(drc, dev, errp);
+ spapr_drc_detach(drc);
addr += SPAPR_MEMORY_BLOCK_SIZE;
}
@@ -2944,7 +2944,6 @@ void spapr_core_unplug_request(HotplugHandler *hotplug_dev, DeviceState *dev,
{
int index;
sPAPRDRConnector *drc;
- Error *local_err = NULL;
CPUCore *cc = CPU_CORE(dev);
int smt = kvmppc_smt_threads();
@@ -2961,11 +2960,7 @@ void spapr_core_unplug_request(HotplugHandler *hotplug_dev, DeviceState *dev,
drc = spapr_drc_by_id(TYPE_SPAPR_DRC_CPU, index * smt);
g_assert(drc);
- spapr_drc_detach(drc, dev, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
- return;
- }
+ spapr_drc_detach(drc);
spapr_hotplug_req_remove_by_index(drc);
}
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 89ba3d6..08fc715 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -70,7 +70,7 @@ static uint32_t drc_isolate_physical(sPAPRDRConnector *drc)
uint32_t drc_index = spapr_drc_index(drc);
if (drc->configured) {
trace_spapr_drc_set_isolation_state_finalizing(drc_index);
- spapr_drc_detach(drc, DEVICE(drc->dev), NULL);
+ spapr_drc_detach(drc);
} else {
trace_spapr_drc_set_isolation_state_deferring(drc_index);
}
@@ -134,7 +134,7 @@ static uint32_t drc_isolate_logical(sPAPRDRConnector *drc)
uint32_t drc_index = spapr_drc_index(drc);
if (drc->configured) {
trace_spapr_drc_set_isolation_state_finalizing(drc_index);
- spapr_drc_detach(drc, DEVICE(drc->dev), NULL);
+ spapr_drc_detach(drc);
} else {
trace_spapr_drc_set_isolation_state_deferring(drc_index);
}
@@ -187,7 +187,7 @@ static uint32_t drc_set_unusable(sPAPRDRConnector *drc)
if (drc->awaiting_release) {
uint32_t drc_index = spapr_drc_index(drc);
trace_spapr_drc_set_allocation_state_finalizing(drc_index);
- spapr_drc_detach(drc, DEVICE(drc->dev), NULL);
+ spapr_drc_detach(drc);
}
return RTAS_OUT_SUCCESS;
@@ -371,20 +371,20 @@ static void spapr_drc_release(sPAPRDRConnector *drc)
drc->dev = NULL;
}
-void spapr_drc_detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp)
+void spapr_drc_detach(sPAPRDRConnector *drc)
{
trace_spapr_drc_detach(spapr_drc_index(drc));
+ drc->awaiting_release = true;
+
if (drc->isolation_state != SPAPR_DR_ISOLATION_STATE_ISOLATED) {
trace_spapr_drc_awaiting_isolated(spapr_drc_index(drc));
- drc->awaiting_release = true;
return;
}
if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI &&
drc->allocation_state != SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
trace_spapr_drc_awaiting_unusable(spapr_drc_index(drc));
- drc->awaiting_release = true;
return;
}
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 1e84c55..092a2f5 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1478,7 +1478,6 @@ static void spapr_pci_unplug_request(HotplugHandler *plug_handler,
PCIDevice *pdev = PCI_DEVICE(plugged_dev);
sPAPRDRConnectorClass *drck;
sPAPRDRConnector *drc = spapr_phb_get_pci_drc(phb, pdev);
- Error *local_err = NULL;
if (!phb->dr_enabled) {
error_setg(errp, QERR_BUS_NO_HOTPLUG,
@@ -1516,11 +1515,7 @@ static void spapr_pci_unplug_request(HotplugHandler *plug_handler,
}
}
- spapr_drc_detach(drc, DEVICE(pdev), &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
- return;
- }
+ spapr_drc_detach(drc);
/* if this isn't func 0, defer unplug event. otherwise signal removal
* for all present functions
diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
index 18a196e..fc8b721 100644
--- a/include/hw/ppc/spapr_drc.h
+++ b/include/hw/ppc/spapr_drc.h
@@ -242,6 +242,6 @@ int spapr_drc_populate_dt(void *fdt, int fdt_offset, Object *owner,
void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
int fdt_start_offset, Error **errp);
-void spapr_drc_detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp);
+void spapr_drc_detach(sPAPRDRConnector *drc);
#endif /* HW_SPAPR_DRC_H */
--
2.9.4
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [Qemu-devel] [PATCHv2 4/8] spapr: Refactor spapr_drc_detach()
2017-07-12 5:53 ` [Qemu-devel] [PATCHv2 4/8] spapr: Refactor spapr_drc_detach() David Gibson
@ 2017-07-12 11:47 ` Greg Kurz
2017-07-13 0:53 ` David Gibson
0 siblings, 1 reply; 30+ messages in thread
From: Greg Kurz @ 2017-07-12 11:47 UTC (permalink / raw)
To: David Gibson
Cc: mdroth, danielhb, qemu-ppc, qemu-devel, lvivier, sjitindarsingh,
bharata
[-- Attachment #1: Type: text/plain, Size: 6895 bytes --]
On Wed, 12 Jul 2017 15:53:13 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:
> This function has two unused parameters - remove them.
>
> It also sets awaiting_release on all paths, except one. On that path
> setting it is harmless, since it will be immediately cleared by
> spapr_drc_release(). So factor it out of the if statements.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
Patch 3 drops the hotplug_handler_unplug(hotplug_ctrl, dev, &error_abort)
lines that could be called below spapr_drc_detach(), but we still have:
spapr_drc_detach()
spapr_drc_release()
object_property_del(OBJECT(drc), "device", NULL);
^^
Should this be &error_abort or should we pass an Error ** down to here ?
(which, would require to add an errp argument to both spapr_drc_release()
and spapr_drc_reset())
I would favor &error_abort since a failure in object_property_del() would
mean we're calling spapr_drc_detach() on a DRC that wasn't attached to a
device... ie, a bug. In this case:
Reviewed-by: Greg Kurz <groug@kaod.org>
> hw/ppc/spapr.c | 11 +++--------
> hw/ppc/spapr_drc.c | 12 ++++++------
> hw/ppc/spapr_pci.c | 7 +------
> include/hw/ppc/spapr_drc.h | 2 +-
> 4 files changed, 11 insertions(+), 21 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index ff2aa6b..5c528e2 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2654,7 +2654,7 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
> addr -= SPAPR_MEMORY_BLOCK_SIZE;
> drc = spapr_drc_by_id(TYPE_SPAPR_DRC_LMB,
> addr / SPAPR_MEMORY_BLOCK_SIZE);
> - spapr_drc_detach(drc, dev, NULL);
> + spapr_drc_detach(drc);
> }
> g_free(fdt);
> error_propagate(errp, local_err);
> @@ -2877,7 +2877,7 @@ static void spapr_memory_unplug_request(HotplugHandler *hotplug_dev,
> addr / SPAPR_MEMORY_BLOCK_SIZE);
> g_assert(drc);
>
> - spapr_drc_detach(drc, dev, errp);
> + spapr_drc_detach(drc);
> addr += SPAPR_MEMORY_BLOCK_SIZE;
> }
>
> @@ -2944,7 +2944,6 @@ void spapr_core_unplug_request(HotplugHandler *hotplug_dev, DeviceState *dev,
> {
> int index;
> sPAPRDRConnector *drc;
> - Error *local_err = NULL;
> CPUCore *cc = CPU_CORE(dev);
> int smt = kvmppc_smt_threads();
>
> @@ -2961,11 +2960,7 @@ void spapr_core_unplug_request(HotplugHandler *hotplug_dev, DeviceState *dev,
> drc = spapr_drc_by_id(TYPE_SPAPR_DRC_CPU, index * smt);
> g_assert(drc);
>
> - spapr_drc_detach(drc, dev, &local_err);
> - if (local_err) {
> - error_propagate(errp, local_err);
> - return;
> - }
> + spapr_drc_detach(drc);
>
> spapr_hotplug_req_remove_by_index(drc);
> }
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 89ba3d6..08fc715 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -70,7 +70,7 @@ static uint32_t drc_isolate_physical(sPAPRDRConnector *drc)
> uint32_t drc_index = spapr_drc_index(drc);
> if (drc->configured) {
> trace_spapr_drc_set_isolation_state_finalizing(drc_index);
> - spapr_drc_detach(drc, DEVICE(drc->dev), NULL);
> + spapr_drc_detach(drc);
> } else {
> trace_spapr_drc_set_isolation_state_deferring(drc_index);
> }
> @@ -134,7 +134,7 @@ static uint32_t drc_isolate_logical(sPAPRDRConnector *drc)
> uint32_t drc_index = spapr_drc_index(drc);
> if (drc->configured) {
> trace_spapr_drc_set_isolation_state_finalizing(drc_index);
> - spapr_drc_detach(drc, DEVICE(drc->dev), NULL);
> + spapr_drc_detach(drc);
> } else {
> trace_spapr_drc_set_isolation_state_deferring(drc_index);
> }
> @@ -187,7 +187,7 @@ static uint32_t drc_set_unusable(sPAPRDRConnector *drc)
> if (drc->awaiting_release) {
> uint32_t drc_index = spapr_drc_index(drc);
> trace_spapr_drc_set_allocation_state_finalizing(drc_index);
> - spapr_drc_detach(drc, DEVICE(drc->dev), NULL);
> + spapr_drc_detach(drc);
> }
>
> return RTAS_OUT_SUCCESS;
> @@ -371,20 +371,20 @@ static void spapr_drc_release(sPAPRDRConnector *drc)
> drc->dev = NULL;
> }
>
> -void spapr_drc_detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp)
> +void spapr_drc_detach(sPAPRDRConnector *drc)
> {
> trace_spapr_drc_detach(spapr_drc_index(drc));
>
> + drc->awaiting_release = true;
> +
> if (drc->isolation_state != SPAPR_DR_ISOLATION_STATE_ISOLATED) {
> trace_spapr_drc_awaiting_isolated(spapr_drc_index(drc));
> - drc->awaiting_release = true;
> return;
> }
>
> if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI &&
> drc->allocation_state != SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
> trace_spapr_drc_awaiting_unusable(spapr_drc_index(drc));
> - drc->awaiting_release = true;
> return;
> }
>
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 1e84c55..092a2f5 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1478,7 +1478,6 @@ static void spapr_pci_unplug_request(HotplugHandler *plug_handler,
> PCIDevice *pdev = PCI_DEVICE(plugged_dev);
> sPAPRDRConnectorClass *drck;
> sPAPRDRConnector *drc = spapr_phb_get_pci_drc(phb, pdev);
> - Error *local_err = NULL;
>
> if (!phb->dr_enabled) {
> error_setg(errp, QERR_BUS_NO_HOTPLUG,
> @@ -1516,11 +1515,7 @@ static void spapr_pci_unplug_request(HotplugHandler *plug_handler,
> }
> }
>
> - spapr_drc_detach(drc, DEVICE(pdev), &local_err);
> - if (local_err) {
> - error_propagate(errp, local_err);
> - return;
> - }
> + spapr_drc_detach(drc);
>
> /* if this isn't func 0, defer unplug event. otherwise signal removal
> * for all present functions
> diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> index 18a196e..fc8b721 100644
> --- a/include/hw/ppc/spapr_drc.h
> +++ b/include/hw/ppc/spapr_drc.h
> @@ -242,6 +242,6 @@ int spapr_drc_populate_dt(void *fdt, int fdt_offset, Object *owner,
>
> void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
> int fdt_start_offset, Error **errp);
> -void spapr_drc_detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp);
> +void spapr_drc_detach(sPAPRDRConnector *drc);
>
> #endif /* HW_SPAPR_DRC_H */
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [Qemu-devel] [PATCHv2 4/8] spapr: Refactor spapr_drc_detach()
2017-07-12 11:47 ` Greg Kurz
@ 2017-07-13 0:53 ` David Gibson
0 siblings, 0 replies; 30+ messages in thread
From: David Gibson @ 2017-07-13 0:53 UTC (permalink / raw)
To: Greg Kurz
Cc: mdroth, danielhb, qemu-ppc, qemu-devel, lvivier, sjitindarsingh,
bharata
[-- Attachment #1: Type: text/plain, Size: 7587 bytes --]
On Wed, Jul 12, 2017 at 01:47:15PM +0200, Greg Kurz wrote:
> On Wed, 12 Jul 2017 15:53:13 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
>
> > This function has two unused parameters - remove them.
> >
> > It also sets awaiting_release on all paths, except one. On that path
> > setting it is harmless, since it will be immediately cleared by
> > spapr_drc_release(). So factor it out of the if statements.
> >
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
>
> Patch 3 drops the hotplug_handler_unplug(hotplug_ctrl, dev, &error_abort)
> lines that could be called below spapr_drc_detach(), but we still have:
>
> spapr_drc_detach()
> spapr_drc_release()
> object_property_del(OBJECT(drc), "device", NULL);
> ^^
> Should this be &error_abort or should we pass an Error ** down to here ?
> (which, would require to add an errp argument to both spapr_drc_release()
> and spapr_drc_reset())
>
> I would favor &error_abort since a failure in object_property_del() would
> mean we're calling spapr_drc_detach() on a DRC that wasn't attached to a
> device... ie, a bug. In this case:
Done, although as a separate patch, since it's not really tied to the
changes here.
>
> Reviewed-by: Greg Kurz <groug@kaod.org>
>
> > hw/ppc/spapr.c | 11 +++--------
> > hw/ppc/spapr_drc.c | 12 ++++++------
> > hw/ppc/spapr_pci.c | 7 +------
> > include/hw/ppc/spapr_drc.h | 2 +-
> > 4 files changed, 11 insertions(+), 21 deletions(-)
> >
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index ff2aa6b..5c528e2 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -2654,7 +2654,7 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
> > addr -= SPAPR_MEMORY_BLOCK_SIZE;
> > drc = spapr_drc_by_id(TYPE_SPAPR_DRC_LMB,
> > addr / SPAPR_MEMORY_BLOCK_SIZE);
> > - spapr_drc_detach(drc, dev, NULL);
> > + spapr_drc_detach(drc);
> > }
> > g_free(fdt);
> > error_propagate(errp, local_err);
> > @@ -2877,7 +2877,7 @@ static void spapr_memory_unplug_request(HotplugHandler *hotplug_dev,
> > addr / SPAPR_MEMORY_BLOCK_SIZE);
> > g_assert(drc);
> >
> > - spapr_drc_detach(drc, dev, errp);
> > + spapr_drc_detach(drc);
> > addr += SPAPR_MEMORY_BLOCK_SIZE;
> > }
> >
> > @@ -2944,7 +2944,6 @@ void spapr_core_unplug_request(HotplugHandler *hotplug_dev, DeviceState *dev,
> > {
> > int index;
> > sPAPRDRConnector *drc;
> > - Error *local_err = NULL;
> > CPUCore *cc = CPU_CORE(dev);
> > int smt = kvmppc_smt_threads();
> >
> > @@ -2961,11 +2960,7 @@ void spapr_core_unplug_request(HotplugHandler *hotplug_dev, DeviceState *dev,
> > drc = spapr_drc_by_id(TYPE_SPAPR_DRC_CPU, index * smt);
> > g_assert(drc);
> >
> > - spapr_drc_detach(drc, dev, &local_err);
> > - if (local_err) {
> > - error_propagate(errp, local_err);
> > - return;
> > - }
> > + spapr_drc_detach(drc);
> >
> > spapr_hotplug_req_remove_by_index(drc);
> > }
> > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> > index 89ba3d6..08fc715 100644
> > --- a/hw/ppc/spapr_drc.c
> > +++ b/hw/ppc/spapr_drc.c
> > @@ -70,7 +70,7 @@ static uint32_t drc_isolate_physical(sPAPRDRConnector *drc)
> > uint32_t drc_index = spapr_drc_index(drc);
> > if (drc->configured) {
> > trace_spapr_drc_set_isolation_state_finalizing(drc_index);
> > - spapr_drc_detach(drc, DEVICE(drc->dev), NULL);
> > + spapr_drc_detach(drc);
> > } else {
> > trace_spapr_drc_set_isolation_state_deferring(drc_index);
> > }
> > @@ -134,7 +134,7 @@ static uint32_t drc_isolate_logical(sPAPRDRConnector *drc)
> > uint32_t drc_index = spapr_drc_index(drc);
> > if (drc->configured) {
> > trace_spapr_drc_set_isolation_state_finalizing(drc_index);
> > - spapr_drc_detach(drc, DEVICE(drc->dev), NULL);
> > + spapr_drc_detach(drc);
> > } else {
> > trace_spapr_drc_set_isolation_state_deferring(drc_index);
> > }
> > @@ -187,7 +187,7 @@ static uint32_t drc_set_unusable(sPAPRDRConnector *drc)
> > if (drc->awaiting_release) {
> > uint32_t drc_index = spapr_drc_index(drc);
> > trace_spapr_drc_set_allocation_state_finalizing(drc_index);
> > - spapr_drc_detach(drc, DEVICE(drc->dev), NULL);
> > + spapr_drc_detach(drc);
> > }
> >
> > return RTAS_OUT_SUCCESS;
> > @@ -371,20 +371,20 @@ static void spapr_drc_release(sPAPRDRConnector *drc)
> > drc->dev = NULL;
> > }
> >
> > -void spapr_drc_detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp)
> > +void spapr_drc_detach(sPAPRDRConnector *drc)
> > {
> > trace_spapr_drc_detach(spapr_drc_index(drc));
> >
> > + drc->awaiting_release = true;
> > +
> > if (drc->isolation_state != SPAPR_DR_ISOLATION_STATE_ISOLATED) {
> > trace_spapr_drc_awaiting_isolated(spapr_drc_index(drc));
> > - drc->awaiting_release = true;
> > return;
> > }
> >
> > if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI &&
> > drc->allocation_state != SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
> > trace_spapr_drc_awaiting_unusable(spapr_drc_index(drc));
> > - drc->awaiting_release = true;
> > return;
> > }
> >
> > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > index 1e84c55..092a2f5 100644
> > --- a/hw/ppc/spapr_pci.c
> > +++ b/hw/ppc/spapr_pci.c
> > @@ -1478,7 +1478,6 @@ static void spapr_pci_unplug_request(HotplugHandler *plug_handler,
> > PCIDevice *pdev = PCI_DEVICE(plugged_dev);
> > sPAPRDRConnectorClass *drck;
> > sPAPRDRConnector *drc = spapr_phb_get_pci_drc(phb, pdev);
> > - Error *local_err = NULL;
> >
> > if (!phb->dr_enabled) {
> > error_setg(errp, QERR_BUS_NO_HOTPLUG,
> > @@ -1516,11 +1515,7 @@ static void spapr_pci_unplug_request(HotplugHandler *plug_handler,
> > }
> > }
> >
> > - spapr_drc_detach(drc, DEVICE(pdev), &local_err);
> > - if (local_err) {
> > - error_propagate(errp, local_err);
> > - return;
> > - }
> > + spapr_drc_detach(drc);
> >
> > /* if this isn't func 0, defer unplug event. otherwise signal removal
> > * for all present functions
> > diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> > index 18a196e..fc8b721 100644
> > --- a/include/hw/ppc/spapr_drc.h
> > +++ b/include/hw/ppc/spapr_drc.h
> > @@ -242,6 +242,6 @@ int spapr_drc_populate_dt(void *fdt, int fdt_offset, Object *owner,
> >
> > void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
> > int fdt_start_offset, Error **errp);
> > -void spapr_drc_detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp);
> > +void spapr_drc_detach(sPAPRDRConnector *drc);
> >
> > #endif /* HW_SPAPR_DRC_H */
>
--
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: 819 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCHv2 5/8] spapr: Cleanups relating to DRC awaiting_release field
2017-07-12 5:53 [Qemu-devel] [PATCHv2 0/8] spapr: DRC cleanups (part VI) David Gibson
` (3 preceding siblings ...)
2017-07-12 5:53 ` [Qemu-devel] [PATCHv2 4/8] spapr: Refactor spapr_drc_detach() David Gibson
@ 2017-07-12 5:53 ` David Gibson
2017-07-12 5:53 ` [Qemu-devel] [PATCHv2 6/8] spapr: Consolidate DRC state variables David Gibson
` (4 subsequent siblings)
9 siblings, 0 replies; 30+ messages in thread
From: David Gibson @ 2017-07-12 5:53 UTC (permalink / raw)
To: mdroth, groug, danielhb
Cc: qemu-ppc, qemu-devel, lvivier, sjitindarsingh, bharata,
David Gibson
'awaiting_release' indicates that the host has requested an unplug of the
device attached to the DRC, but the guest has not (yet) put the device
into a state where it is safe to complete removal.
1. Rename it to 'unplug_requested' which to me at least is clearer
2. Remove the ->release_pending() method used to check this from outside
spapr_drc.c. The method only plausibly has one implementation, so use
a plain function (spapr_drc_unplug_requested()) instead.
3. Remove it from the migration stream. Attempting to migrate mid-unplug
is broken not just for spapr - in general management has no good way to
determine if the device should be present on the destination or not. So,
until that's fixed, there's no point adding extra things to the stream.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Greg Kurz <groug@kaod.org>
---
hw/ppc/spapr_drc.c | 26 +++++++++-----------------
hw/ppc/spapr_pci.c | 6 ++----
include/hw/ppc/spapr_drc.h | 11 ++++++-----
3 files changed, 17 insertions(+), 26 deletions(-)
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 08fc715..8326f3a 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -66,7 +66,7 @@ static uint32_t drc_isolate_physical(sPAPRDRConnector *drc)
* configured state, as suggested by the state diagram from PAPR+
* 2.7, 13.4
*/
- if (drc->awaiting_release) {
+ if (drc->unplug_requested) {
uint32_t drc_index = spapr_drc_index(drc);
if (drc->configured) {
trace_spapr_drc_set_isolation_state_finalizing(drc_index);
@@ -116,7 +116,7 @@ static uint32_t drc_isolate_logical(sPAPRDRConnector *drc)
* actually being unplugged, fail the isolation request here.
*/
if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_LMB
- && !drc->awaiting_release) {
+ && !drc->unplug_requested) {
return RTAS_OUT_HW_ERROR;
}
@@ -130,7 +130,7 @@ static uint32_t drc_isolate_logical(sPAPRDRConnector *drc)
* configured state, as suggested by the state diagram from PAPR+
* 2.7, 13.4
*/
- if (drc->awaiting_release) {
+ if (drc->unplug_requested) {
uint32_t drc_index = spapr_drc_index(drc);
if (drc->configured) {
trace_spapr_drc_set_isolation_state_finalizing(drc_index);
@@ -170,7 +170,7 @@ static uint32_t drc_set_usable(sPAPRDRConnector *drc)
if (!drc->dev) {
return RTAS_OUT_NO_SUCH_INDICATOR;
}
- if (drc->awaiting_release) {
+ if (drc->unplug_requested) {
/* Don't allow the guest to move a device away from UNUSABLE
* state when we want to unplug it */
return RTAS_OUT_NO_SUCH_INDICATOR;
@@ -184,7 +184,7 @@ static uint32_t drc_set_usable(sPAPRDRConnector *drc)
static uint32_t drc_set_unusable(sPAPRDRConnector *drc)
{
drc->allocation_state = SPAPR_DR_ALLOCATION_STATE_UNUSABLE;
- if (drc->awaiting_release) {
+ if (drc->unplug_requested) {
uint32_t drc_index = spapr_drc_index(drc);
trace_spapr_drc_set_allocation_state_finalizing(drc_index);
spapr_drc_detach(drc);
@@ -363,7 +363,7 @@ static void spapr_drc_release(sPAPRDRConnector *drc)
drck->release(drc->dev);
- drc->awaiting_release = false;
+ drc->unplug_requested = false;
g_free(drc->fdt);
drc->fdt = NULL;
drc->fdt_start_offset = 0;
@@ -375,7 +375,7 @@ void spapr_drc_detach(sPAPRDRConnector *drc)
{
trace_spapr_drc_detach(spapr_drc_index(drc));
- drc->awaiting_release = true;
+ drc->unplug_requested = true;
if (drc->isolation_state != SPAPR_DR_ISOLATION_STATE_ISOLATED) {
trace_spapr_drc_awaiting_isolated(spapr_drc_index(drc));
@@ -391,11 +391,6 @@ void spapr_drc_detach(sPAPRDRConnector *drc)
spapr_drc_release(drc);
}
-static bool release_pending(sPAPRDRConnector *drc)
-{
- return drc->awaiting_release;
-}
-
void spapr_drc_reset(sPAPRDRConnector *drc)
{
trace_spapr_drc_reset(spapr_drc_index(drc));
@@ -406,7 +401,7 @@ void spapr_drc_reset(sPAPRDRConnector *drc)
/* immediately upon reset we can safely assume DRCs whose devices
* are pending removal can be safely removed.
*/
- if (drc->awaiting_release) {
+ if (drc->unplug_requested) {
spapr_drc_release(drc);
}
@@ -454,7 +449,7 @@ static bool spapr_drc_needed(void *opaque)
case SPAPR_DR_CONNECTOR_TYPE_LMB:
rc = !((drc->isolation_state == SPAPR_DR_ISOLATION_STATE_UNISOLATED) &&
(drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_USABLE) &&
- drc->configured && !drc->awaiting_release);
+ drc->configured);
break;
case SPAPR_DR_CONNECTOR_TYPE_PHB:
case SPAPR_DR_CONNECTOR_TYPE_VIO:
@@ -474,7 +469,6 @@ static const VMStateDescription vmstate_spapr_drc = {
VMSTATE_UINT32(allocation_state, sPAPRDRConnector),
VMSTATE_UINT32(dr_indicator, sPAPRDRConnector),
VMSTATE_BOOL(configured, sPAPRDRConnector),
- VMSTATE_BOOL(awaiting_release, sPAPRDRConnector),
VMSTATE_END_OF_LIST()
}
};
@@ -565,11 +559,9 @@ static void spapr_dr_connector_instance_init(Object *obj)
static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
{
DeviceClass *dk = DEVICE_CLASS(k);
- sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_CLASS(k);
dk->realize = realize;
dk->unrealize = unrealize;
- drck->release_pending = release_pending;
/*
* Reason: it crashes FIXME find and document the real reason
*/
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 092a2f5..6ecdf29 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1476,7 +1476,6 @@ static void spapr_pci_unplug_request(HotplugHandler *plug_handler,
{
sPAPRPHBState *phb = SPAPR_PCI_HOST_BRIDGE(DEVICE(plug_handler));
PCIDevice *pdev = PCI_DEVICE(plugged_dev);
- sPAPRDRConnectorClass *drck;
sPAPRDRConnector *drc = spapr_phb_get_pci_drc(phb, pdev);
if (!phb->dr_enabled) {
@@ -1488,8 +1487,7 @@ static void spapr_pci_unplug_request(HotplugHandler *plug_handler,
g_assert(drc);
g_assert(drc->dev == plugged_dev);
- drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
- if (!drck->release_pending(drc)) {
+ if (!spapr_drc_unplug_requested(drc)) {
PCIBus *bus = PCI_BUS(qdev_get_parent_bus(DEVICE(pdev)));
uint32_t slotnr = PCI_SLOT(pdev->devfn);
sPAPRDRConnector *func_drc;
@@ -1505,7 +1503,7 @@ static void spapr_pci_unplug_request(HotplugHandler *plug_handler,
func_drck = SPAPR_DR_CONNECTOR_GET_CLASS(func_drc);
state = func_drck->dr_entity_sense(func_drc);
if (state == SPAPR_DR_ENTITY_SENSE_PRESENT
- && !func_drck->release_pending(func_drc)) {
+ && !spapr_drc_unplug_requested(func_drc)) {
error_setg(errp,
"PCI: slot %d, function %d still present. "
"Must unplug all non-0 functions first.",
diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
index fc8b721..5fa502e 100644
--- a/include/hw/ppc/spapr_drc.h
+++ b/include/hw/ppc/spapr_drc.h
@@ -199,10 +199,9 @@ typedef struct sPAPRDRConnector {
bool configured;
sPAPRConfigureConnectorState *ccs;
- bool awaiting_release;
-
/* device pointer, via link property */
DeviceState *dev;
+ bool unplug_requested;
} sPAPRDRConnector;
typedef struct sPAPRDRConnectorClass {
@@ -218,9 +217,6 @@ typedef struct sPAPRDRConnectorClass {
uint32_t (*isolate)(sPAPRDRConnector *drc);
uint32_t (*unisolate)(sPAPRDRConnector *drc);
void (*release)(DeviceState *dev);
-
- /* QEMU interfaces for managing hotplug operations */
- bool (*release_pending)(sPAPRDRConnector *drc);
} sPAPRDRConnectorClass;
static inline bool spapr_drc_hotplugged(DeviceState *dev)
@@ -244,4 +240,9 @@ void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
int fdt_start_offset, Error **errp);
void spapr_drc_detach(sPAPRDRConnector *drc);
+static inline bool spapr_drc_unplug_requested(sPAPRDRConnector *drc)
+{
+ return drc->unplug_requested;
+}
+
#endif /* HW_SPAPR_DRC_H */
--
2.9.4
^ permalink raw reply related [flat|nested] 30+ messages in thread* [Qemu-devel] [PATCHv2 6/8] spapr: Consolidate DRC state variables
2017-07-12 5:53 [Qemu-devel] [PATCHv2 0/8] spapr: DRC cleanups (part VI) David Gibson
` (4 preceding siblings ...)
2017-07-12 5:53 ` [Qemu-devel] [PATCHv2 5/8] spapr: Cleanups relating to DRC awaiting_release field David Gibson
@ 2017-07-12 5:53 ` David Gibson
2017-07-12 17:36 ` [Qemu-devel] [Qemu-ppc] " Daniel Henrique Barboza
2017-07-12 5:53 ` [Qemu-devel] [PATCHv2 7/8] spapr: Remove sPAPRConfigureConnectorState sub-structure David Gibson
` (3 subsequent siblings)
9 siblings, 1 reply; 30+ messages in thread
From: David Gibson @ 2017-07-12 5:53 UTC (permalink / raw)
To: mdroth, groug, danielhb
Cc: qemu-ppc, qemu-devel, lvivier, sjitindarsingh, bharata,
David Gibson
Each DRC has three fields describing its state: isolation_state,
allocation_state and configured. At first this seems like a reasonable
representation, since its based directly on the PAPR defined
isolation-state and allocation-state indicators. However:
* Only a few combinations of the two fields' values are permitted
* allocation_state isn't used at all for physical DRCs
* The indicators are write only so they don't really have a well
defined current value independent of each other
This replaces these variables with a single state variable, whose names
and numbers are based on the diagram in LoPAPR section 13.4. Along with
this we add code to check the current state on various operations and make
sure the requested transition is permitted.
Strictly speaking, this makes guest visible changes to behaviour (since we
probably allowed some transitions we shouldn't have before). However, a
hypothetical guest broken by that wasn't PAPR compliant, and probably
wouldn't have worked under PowerVM.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
hw/ppc/spapr_drc.c | 225 +++++++++++++++++++++++++--------------------
hw/ppc/trace-events | 3 +-
include/hw/ppc/spapr_drc.h | 25 ++++-
3 files changed, 145 insertions(+), 108 deletions(-)
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 8326f3a..dbc9c0f 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -48,6 +48,17 @@ uint32_t spapr_drc_index(sPAPRDRConnector *drc)
static uint32_t drc_isolate_physical(sPAPRDRConnector *drc)
{
+ switch (drc->state) {
+ case SPAPR_DRC_STATE_PHYSICAL_POWERON:
+ return RTAS_OUT_SUCCESS; /* Nothing to do */
+ case SPAPR_DRC_STATE_PHYSICAL_CONFIGURED:
+ break; /* see below */
+ case SPAPR_DRC_STATE_PHYSICAL_UNISOLATE:
+ return RTAS_OUT_PARAM_ERROR; /* not allowed */
+ default:
+ g_assert_not_reached();
+ }
+
/* if the guest is configuring a device attached to this DRC, we
* should reset the configuration state at this point since it may
* no longer be reliable (guest released device and needs to start
@@ -56,32 +67,29 @@ static uint32_t drc_isolate_physical(sPAPRDRConnector *drc)
g_free(drc->ccs);
drc->ccs = NULL;
- drc->isolation_state = SPAPR_DR_ISOLATION_STATE_ISOLATED;
+ drc->state = SPAPR_DRC_STATE_PHYSICAL_POWERON;
- /* if we're awaiting release, but still in an unconfigured state,
- * it's likely the guest is still in the process of configuring
- * the device and is transitioning the devices to an ISOLATED
- * state as a part of that process. so we only complete the
- * removal when this transition happens for a device in a
- * configured state, as suggested by the state diagram from PAPR+
- * 2.7, 13.4
- */
if (drc->unplug_requested) {
uint32_t drc_index = spapr_drc_index(drc);
- if (drc->configured) {
- trace_spapr_drc_set_isolation_state_finalizing(drc_index);
- spapr_drc_detach(drc);
- } else {
- trace_spapr_drc_set_isolation_state_deferring(drc_index);
- }
+ trace_spapr_drc_set_isolation_state_finalizing(drc_index);
+ spapr_drc_detach(drc);
}
- drc->configured = false;
return RTAS_OUT_SUCCESS;
}
static uint32_t drc_unisolate_physical(sPAPRDRConnector *drc)
{
+ switch (drc->state) {
+ case SPAPR_DRC_STATE_PHYSICAL_UNISOLATE:
+ case SPAPR_DRC_STATE_PHYSICAL_CONFIGURED:
+ return RTAS_OUT_SUCCESS; /* Nothing to do */
+ case SPAPR_DRC_STATE_PHYSICAL_POWERON:
+ break; /* see below */
+ default:
+ g_assert_not_reached();
+ }
+
/* cannot unisolate a non-existent resource, and, or resources
* which are in an 'UNUSABLE' allocation state. (PAPR 2.7,
* 13.5.3.5)
@@ -90,13 +98,25 @@ static uint32_t drc_unisolate_physical(sPAPRDRConnector *drc)
return RTAS_OUT_NO_SUCH_INDICATOR;
}
- drc->isolation_state = SPAPR_DR_ISOLATION_STATE_UNISOLATED;
+ drc->state = SPAPR_DRC_STATE_PHYSICAL_UNISOLATE;
return RTAS_OUT_SUCCESS;
}
static uint32_t drc_isolate_logical(sPAPRDRConnector *drc)
{
+ switch (drc->state) {
+ case SPAPR_DRC_STATE_LOGICAL_AVAILABLE:
+ case SPAPR_DRC_STATE_LOGICAL_UNUSABLE:
+ return RTAS_OUT_SUCCESS; /* Nothing to do */
+ case SPAPR_DRC_STATE_LOGICAL_CONFIGURED:
+ break; /* see below */
+ case SPAPR_DRC_STATE_LOGICAL_UNISOLATE:
+ return RTAS_OUT_PARAM_ERROR; /* not allowed */
+ default:
+ g_assert_not_reached();
+ }
+
/* if the guest is configuring a device attached to this DRC, we
* should reset the configuration state at this point since it may
* no longer be reliable (guest released device and needs to start
@@ -120,7 +140,7 @@ static uint32_t drc_isolate_logical(sPAPRDRConnector *drc)
return RTAS_OUT_HW_ERROR;
}
- drc->isolation_state = SPAPR_DR_ISOLATION_STATE_ISOLATED;
+ drc->state = SPAPR_DRC_STATE_LOGICAL_AVAILABLE;
/* if we're awaiting release, but still in an unconfigured state,
* it's likely the guest is still in the process of configuring
@@ -132,36 +152,46 @@ static uint32_t drc_isolate_logical(sPAPRDRConnector *drc)
*/
if (drc->unplug_requested) {
uint32_t drc_index = spapr_drc_index(drc);
- if (drc->configured) {
- trace_spapr_drc_set_isolation_state_finalizing(drc_index);
- spapr_drc_detach(drc);
- } else {
- trace_spapr_drc_set_isolation_state_deferring(drc_index);
- }
+ trace_spapr_drc_set_isolation_state_finalizing(drc_index);
+ spapr_drc_detach(drc);
}
- drc->configured = false;
-
return RTAS_OUT_SUCCESS;
}
static uint32_t drc_unisolate_logical(sPAPRDRConnector *drc)
{
- /* cannot unisolate a non-existent resource, and, or resources
- * which are in an 'UNUSABLE' allocation state. (PAPR 2.7,
- * 13.5.3.5)
- */
- if (!drc->dev ||
- drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
- return RTAS_OUT_NO_SUCH_INDICATOR;
+ switch (drc->state) {
+ case SPAPR_DRC_STATE_LOGICAL_UNISOLATE:
+ case SPAPR_DRC_STATE_LOGICAL_CONFIGURED:
+ return RTAS_OUT_SUCCESS; /* Nothing to do */
+ case SPAPR_DRC_STATE_LOGICAL_AVAILABLE:
+ break; /* see below */
+ case SPAPR_DRC_STATE_LOGICAL_UNUSABLE:
+ return RTAS_OUT_NO_SUCH_INDICATOR; /* not allowed */
+ default:
+ g_assert_not_reached();
}
- drc->isolation_state = SPAPR_DR_ISOLATION_STATE_UNISOLATED;
+ /* Move to AVAILABLE state should have ensured device was present */
+ g_assert(drc->dev);
+ drc->state = SPAPR_DRC_STATE_LOGICAL_UNISOLATE;
return RTAS_OUT_SUCCESS;
}
static uint32_t drc_set_usable(sPAPRDRConnector *drc)
{
+ switch (drc->state) {
+ case SPAPR_DRC_STATE_LOGICAL_AVAILABLE:
+ case SPAPR_DRC_STATE_LOGICAL_UNISOLATE:
+ case SPAPR_DRC_STATE_LOGICAL_CONFIGURED:
+ return RTAS_OUT_SUCCESS; /* Nothing to do */
+ case SPAPR_DRC_STATE_LOGICAL_UNUSABLE:
+ break; /* see below */
+ default:
+ g_assert_not_reached();
+ }
+
/* if there's no resource/device associated with the DRC, there's
* no way for us to put it in an allocation state consistent with
* being 'USABLE'. PAPR 2.7, 13.5.3.4 documents that this should
@@ -176,14 +206,26 @@ static uint32_t drc_set_usable(sPAPRDRConnector *drc)
return RTAS_OUT_NO_SUCH_INDICATOR;
}
- drc->allocation_state = SPAPR_DR_ALLOCATION_STATE_USABLE;
+ drc->state = SPAPR_DRC_STATE_LOGICAL_AVAILABLE;
return RTAS_OUT_SUCCESS;
}
static uint32_t drc_set_unusable(sPAPRDRConnector *drc)
{
- drc->allocation_state = SPAPR_DR_ALLOCATION_STATE_UNUSABLE;
+ switch (drc->state) {
+ case SPAPR_DRC_STATE_LOGICAL_UNUSABLE:
+ return RTAS_OUT_SUCCESS; /* Nothing to do */
+ case SPAPR_DRC_STATE_LOGICAL_AVAILABLE:
+ break; /* see below */
+ case SPAPR_DRC_STATE_LOGICAL_UNISOLATE:
+ case SPAPR_DRC_STATE_LOGICAL_CONFIGURED:
+ return RTAS_OUT_NO_SUCH_INDICATOR; /* not allowed */
+ default:
+ g_assert_not_reached();
+ }
+
+ drc->state = SPAPR_DRC_STATE_LOGICAL_UNUSABLE;
if (drc->unplug_requested) {
uint32_t drc_index = spapr_drc_index(drc);
trace_spapr_drc_set_allocation_state_finalizing(drc_index);
@@ -241,11 +283,16 @@ static sPAPRDREntitySense physical_entity_sense(sPAPRDRConnector *drc)
static sPAPRDREntitySense logical_entity_sense(sPAPRDRConnector *drc)
{
- if (drc->dev
- && (drc->allocation_state != SPAPR_DR_ALLOCATION_STATE_UNUSABLE)) {
- return SPAPR_DR_ENTITY_SENSE_PRESENT;
- } else {
+ switch (drc->state) {
+ case SPAPR_DRC_STATE_LOGICAL_UNUSABLE:
return SPAPR_DR_ENTITY_SENSE_UNUSABLE;
+ case SPAPR_DRC_STATE_LOGICAL_AVAILABLE:
+ case SPAPR_DRC_STATE_LOGICAL_UNISOLATE:
+ case SPAPR_DRC_STATE_LOGICAL_CONFIGURED:
+ g_assert(drc->dev);
+ return SPAPR_DR_ENTITY_SENSE_PRESENT;
+ default:
+ g_assert_not_reached();
}
}
@@ -338,13 +385,12 @@ void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
{
trace_spapr_drc_attach(spapr_drc_index(drc));
- if (drc->isolation_state != SPAPR_DR_ISOLATION_STATE_ISOLATED) {
+ if (drc->dev) {
error_setg(errp, "an attached device is still awaiting release");
return;
}
- if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_PCI) {
- g_assert(drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_USABLE);
- }
+ g_assert((drc->state == SPAPR_DRC_STATE_LOGICAL_UNUSABLE)
+ || (drc->state == SPAPR_DRC_STATE_PHYSICAL_POWERON));
g_assert(fdt);
drc->dev = d;
@@ -373,18 +419,16 @@ static void spapr_drc_release(sPAPRDRConnector *drc)
void spapr_drc_detach(sPAPRDRConnector *drc)
{
+ sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
+
trace_spapr_drc_detach(spapr_drc_index(drc));
- drc->unplug_requested = true;
+ g_assert(drc->dev);
- if (drc->isolation_state != SPAPR_DR_ISOLATION_STATE_ISOLATED) {
- trace_spapr_drc_awaiting_isolated(spapr_drc_index(drc));
- return;
- }
+ drc->unplug_requested = true;
- if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI &&
- drc->allocation_state != SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
- trace_spapr_drc_awaiting_unusable(spapr_drc_index(drc));
+ if (drc->state != drck->empty_state) {
+ trace_spapr_drc_awaiting_quiesce(spapr_drc_index(drc));
return;
}
@@ -393,6 +437,8 @@ void spapr_drc_detach(sPAPRDRConnector *drc)
void spapr_drc_reset(sPAPRDRConnector *drc)
{
+ sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
+
trace_spapr_drc_reset(spapr_drc_index(drc));
g_free(drc->ccs);
@@ -406,19 +452,10 @@ void spapr_drc_reset(sPAPRDRConnector *drc)
}
if (drc->dev) {
- /* A device present at reset is coldplugged */
- drc->isolation_state = SPAPR_DR_ISOLATION_STATE_UNISOLATED;
- if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
- drc->allocation_state = SPAPR_DR_ALLOCATION_STATE_USABLE;
- }
- drc->dr_indicator = SPAPR_DR_INDICATOR_ACTIVE;
+ /* A device present at reset is ready to go, same as coldplugged */
+ drc->state = drck->ready_state;
} else {
- /* Otherwise device is absent, but might be hotplugged */
- drc->isolation_state = SPAPR_DR_ISOLATION_STATE_ISOLATED;
- if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
- drc->allocation_state = SPAPR_DR_ALLOCATION_STATE_UNUSABLE;
- }
- drc->dr_indicator = SPAPR_DR_INDICATOR_INACTIVE;
+ drc->state = drck->empty_state;
}
}
@@ -431,7 +468,6 @@ static bool spapr_drc_needed(void *opaque)
{
sPAPRDRConnector *drc = (sPAPRDRConnector *)opaque;
sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
- bool rc = false;
sPAPRDREntitySense value = drck->dr_entity_sense(drc);
/* If no dev is plugged in there is no need to migrate the DRC state */
@@ -440,23 +476,10 @@ static bool spapr_drc_needed(void *opaque)
}
/*
- * If there is dev plugged in, we need to migrate the DRC state when
- * it is different from cold-plugged state
- */
- switch (spapr_drc_type(drc)) {
- case SPAPR_DR_CONNECTOR_TYPE_PCI:
- case SPAPR_DR_CONNECTOR_TYPE_CPU:
- case SPAPR_DR_CONNECTOR_TYPE_LMB:
- rc = !((drc->isolation_state == SPAPR_DR_ISOLATION_STATE_UNISOLATED) &&
- (drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_USABLE) &&
- drc->configured);
- break;
- case SPAPR_DR_CONNECTOR_TYPE_PHB:
- case SPAPR_DR_CONNECTOR_TYPE_VIO:
- default:
- g_assert_not_reached();
- }
- return rc;
+ * We need to migrate the state if it's not equal to the expected
+ * long-term state, which is the same as the coldplugged initial
+ * state */
+ return (drc->state != drck->ready_state);
}
static const VMStateDescription vmstate_spapr_drc = {
@@ -465,10 +488,8 @@ static const VMStateDescription vmstate_spapr_drc = {
.minimum_version_id = 1,
.needed = spapr_drc_needed,
.fields = (VMStateField []) {
- VMSTATE_UINT32(isolation_state, sPAPRDRConnector),
- VMSTATE_UINT32(allocation_state, sPAPRDRConnector),
+ VMSTATE_UINT32(state, sPAPRDRConnector),
VMSTATE_UINT32(dr_indicator, sPAPRDRConnector),
- VMSTATE_BOOL(configured, sPAPRDRConnector),
VMSTATE_END_OF_LIST()
}
};
@@ -537,23 +558,20 @@ sPAPRDRConnector *spapr_dr_connector_new(Object *owner, const char *type,
object_property_set_bool(OBJECT(drc), true, "realized", NULL);
g_free(prop_name);
- /* PCI slot always start in a USABLE state, and stay there */
- if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_PCI) {
- drc->allocation_state = SPAPR_DR_ALLOCATION_STATE_USABLE;
- }
-
return drc;
}
static void spapr_dr_connector_instance_init(Object *obj)
{
sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(obj);
+ sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
object_property_add_uint32_ptr(obj, "id", &drc->id, NULL);
object_property_add(obj, "index", "uint32", prop_get_index,
NULL, NULL, NULL, NULL);
object_property_add(obj, "fdt", "struct", prop_get_fdt,
NULL, NULL, NULL, NULL);
+ drc->state = drck->empty_state;
}
static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
@@ -575,6 +593,8 @@ static void spapr_drc_physical_class_init(ObjectClass *k, void *data)
drck->dr_entity_sense = physical_entity_sense;
drck->isolate = drc_isolate_physical;
drck->unisolate = drc_unisolate_physical;
+ drck->ready_state = SPAPR_DRC_STATE_PHYSICAL_CONFIGURED;
+ drck->empty_state = SPAPR_DRC_STATE_PHYSICAL_POWERON;
}
static void spapr_drc_logical_class_init(ObjectClass *k, void *data)
@@ -584,6 +604,8 @@ static void spapr_drc_logical_class_init(ObjectClass *k, void *data)
drck->dr_entity_sense = logical_entity_sense;
drck->isolate = drc_isolate_logical;
drck->unisolate = drc_unisolate_logical;
+ drck->ready_state = SPAPR_DRC_STATE_LOGICAL_CONFIGURED;
+ drck->empty_state = SPAPR_DRC_STATE_LOGICAL_UNUSABLE;
}
static void spapr_drc_cpu_class_init(ObjectClass *k, void *data)
@@ -987,6 +1009,7 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
uint64_t wa_offset;
uint32_t drc_index;
sPAPRDRConnector *drc;
+ sPAPRDRConnectorClass *drck;
sPAPRConfigureConnectorState *ccs;
sPAPRDRCCResponse resp = SPAPR_DR_CC_RESPONSE_CONTINUE;
int rc;
@@ -1006,12 +1029,17 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
goto out;
}
- if (!drc->fdt) {
- trace_spapr_rtas_ibm_configure_connector_missing_fdt(drc_index);
+ if ((drc->state != SPAPR_DRC_STATE_LOGICAL_UNISOLATE)
+ && (drc->state != SPAPR_DRC_STATE_PHYSICAL_UNISOLATE)) {
+ /* Need to unisolate the device before configuring */
rc = SPAPR_DR_CC_RESPONSE_NOT_CONFIGURABLE;
goto out;
}
+ g_assert(drc->fdt);
+
+ drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
+
ccs = drc->ccs;
if (!ccs) {
ccs = g_new0(sPAPRConfigureConnectorState, 1);
@@ -1041,18 +1069,11 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
case FDT_END_NODE:
ccs->fdt_depth--;
if (ccs->fdt_depth == 0) {
- sPAPRDRIsolationState state = drc->isolation_state;
uint32_t drc_index = spapr_drc_index(drc);
- /* done sending the device tree, don't need to track
- * the state anymore
- */
+
+ /* done sending the device tree, move to configured state */
trace_spapr_drc_set_configured(drc_index);
- if (state == SPAPR_DR_ISOLATION_STATE_UNISOLATED) {
- drc->configured = true;
- } else {
- /* guest should be not configuring an isolated device */
- trace_spapr_drc_set_configured_skipping(drc_index);
- }
+ drc->state = drck->ready_state;
g_free(ccs);
drc->ccs = NULL;
ccs = NULL;
diff --git a/hw/ppc/trace-events b/hw/ppc/trace-events
index 3e8e3cf..8e79f7e 100644
--- a/hw/ppc/trace-events
+++ b/hw/ppc/trace-events
@@ -46,8 +46,7 @@ spapr_drc_set_configured(uint32_t index) "drc: 0x%"PRIx32
spapr_drc_set_configured_skipping(uint32_t index) "drc: 0x%"PRIx32", isolated device"
spapr_drc_attach(uint32_t index) "drc: 0x%"PRIx32
spapr_drc_detach(uint32_t index) "drc: 0x%"PRIx32
-spapr_drc_awaiting_isolated(uint32_t index) "drc: 0x%"PRIx32
-spapr_drc_awaiting_unusable(uint32_t index) "drc: 0x%"PRIx32
+spapr_drc_awaiting_quiesce(uint32_t index) "drc: 0x%"PRIx32
spapr_drc_awaiting_allocation(uint32_t index) "drc: 0x%"PRIx32
spapr_drc_reset(uint32_t index) "drc: 0x%"PRIx32
spapr_drc_realize(uint32_t index) "drc: 0x%"PRIx32
diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
index 5fa502e..4ceaaf0 100644
--- a/include/hw/ppc/spapr_drc.h
+++ b/include/hw/ppc/spapr_drc.h
@@ -173,6 +173,24 @@ typedef enum {
SPAPR_DR_CC_RESPONSE_NOT_CONFIGURABLE = -9003,
} sPAPRDRCCResponse;
+typedef enum {
+ /*
+ * Values come from Fig. 12 in LoPAPR section 13.4
+ *
+ * These are exposed in the migration stream, so don't change
+ * them.
+ */
+ SPAPR_DRC_STATE_INVALID = 0,
+ SPAPR_DRC_STATE_LOGICAL_UNUSABLE = 1,
+ SPAPR_DRC_STATE_LOGICAL_AVAILABLE = 2,
+ SPAPR_DRC_STATE_LOGICAL_UNISOLATE = 3,
+ SPAPR_DRC_STATE_LOGICAL_CONFIGURED = 4,
+ SPAPR_DRC_STATE_PHYSICAL_AVAILABLE = 5,
+ SPAPR_DRC_STATE_PHYSICAL_POWERON = 6,
+ SPAPR_DRC_STATE_PHYSICAL_UNISOLATE = 7,
+ SPAPR_DRC_STATE_PHYSICAL_CONFIGURED = 8,
+} sPAPRDRCState;
+
/* rtas-configure-connector state */
typedef struct sPAPRConfigureConnectorState {
int fdt_offset;
@@ -189,14 +207,11 @@ typedef struct sPAPRDRConnector {
/* DR-indicator */
uint32_t dr_indicator;
- /* sensor/indicator states */
- uint32_t isolation_state;
- uint32_t allocation_state;
+ uint32_t state;
/* configure-connector state */
void *fdt;
int fdt_start_offset;
- bool configured;
sPAPRConfigureConnectorState *ccs;
/* device pointer, via link property */
@@ -207,6 +222,8 @@ typedef struct sPAPRDRConnector {
typedef struct sPAPRDRConnectorClass {
/*< private >*/
DeviceClass parent;
+ sPAPRDRCState empty_state;
+ sPAPRDRCState ready_state;
/*< public >*/
sPAPRDRConnectorTypeShift typeshift;
--
2.9.4
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [Qemu-devel] [Qemu-ppc] [PATCHv2 6/8] spapr: Consolidate DRC state variables
2017-07-12 5:53 ` [Qemu-devel] [PATCHv2 6/8] spapr: Consolidate DRC state variables David Gibson
@ 2017-07-12 17:36 ` Daniel Henrique Barboza
0 siblings, 0 replies; 30+ messages in thread
From: Daniel Henrique Barboza @ 2017-07-12 17:36 UTC (permalink / raw)
To: David Gibson, mdroth, groug
Cc: lvivier, qemu-devel, qemu-ppc, sjitindarsingh, bharata
On 07/12/2017 02:53 AM, David Gibson wrote:
> Each DRC has three fields describing its state: isolation_state,
> allocation_state and configured. At first this seems like a reasonable
> representation, since its based directly on the PAPR defined
> isolation-state and allocation-state indicators. However:
> * Only a few combinations of the two fields' values are permitted
> * allocation_state isn't used at all for physical DRCs
> * The indicators are write only so they don't really have a well
> defined current value independent of each other
>
> This replaces these variables with a single state variable, whose names
> and numbers are based on the diagram in LoPAPR section 13.4. Along with
> this we add code to check the current state on various operations and make
> sure the requested transition is permitted.
>
> Strictly speaking, this makes guest visible changes to behaviour (since we
> probably allowed some transitions we shouldn't have before). However, a
> hypothetical guest broken by that wasn't PAPR compliant, and probably
> wouldn't have worked under PowerVM.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Daniel Barboza <danielhb@linux.vnet.ibm.com>
> ---
> hw/ppc/spapr_drc.c | 225 +++++++++++++++++++++++++--------------------
> hw/ppc/trace-events | 3 +-
> include/hw/ppc/spapr_drc.h | 25 ++++-
> 3 files changed, 145 insertions(+), 108 deletions(-)
>
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 8326f3a..dbc9c0f 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -48,6 +48,17 @@ uint32_t spapr_drc_index(sPAPRDRConnector *drc)
>
> static uint32_t drc_isolate_physical(sPAPRDRConnector *drc)
> {
> + switch (drc->state) {
> + case SPAPR_DRC_STATE_PHYSICAL_POWERON:
> + return RTAS_OUT_SUCCESS; /* Nothing to do */
> + case SPAPR_DRC_STATE_PHYSICAL_CONFIGURED:
> + break; /* see below */
> + case SPAPR_DRC_STATE_PHYSICAL_UNISOLATE:
> + return RTAS_OUT_PARAM_ERROR; /* not allowed */
> + default:
> + g_assert_not_reached();
> + }
> +
> /* if the guest is configuring a device attached to this DRC, we
> * should reset the configuration state at this point since it may
> * no longer be reliable (guest released device and needs to start
> @@ -56,32 +67,29 @@ static uint32_t drc_isolate_physical(sPAPRDRConnector *drc)
> g_free(drc->ccs);
> drc->ccs = NULL;
>
> - drc->isolation_state = SPAPR_DR_ISOLATION_STATE_ISOLATED;
> + drc->state = SPAPR_DRC_STATE_PHYSICAL_POWERON;
>
> - /* if we're awaiting release, but still in an unconfigured state,
> - * it's likely the guest is still in the process of configuring
> - * the device and is transitioning the devices to an ISOLATED
> - * state as a part of that process. so we only complete the
> - * removal when this transition happens for a device in a
> - * configured state, as suggested by the state diagram from PAPR+
> - * 2.7, 13.4
> - */
> if (drc->unplug_requested) {
> uint32_t drc_index = spapr_drc_index(drc);
> - if (drc->configured) {
> - trace_spapr_drc_set_isolation_state_finalizing(drc_index);
> - spapr_drc_detach(drc);
> - } else {
> - trace_spapr_drc_set_isolation_state_deferring(drc_index);
> - }
> + trace_spapr_drc_set_isolation_state_finalizing(drc_index);
> + spapr_drc_detach(drc);
> }
> - drc->configured = false;
>
> return RTAS_OUT_SUCCESS;
> }
>
> static uint32_t drc_unisolate_physical(sPAPRDRConnector *drc)
> {
> + switch (drc->state) {
> + case SPAPR_DRC_STATE_PHYSICAL_UNISOLATE:
> + case SPAPR_DRC_STATE_PHYSICAL_CONFIGURED:
> + return RTAS_OUT_SUCCESS; /* Nothing to do */
> + case SPAPR_DRC_STATE_PHYSICAL_POWERON:
> + break; /* see below */
> + default:
> + g_assert_not_reached();
> + }
> +
> /* cannot unisolate a non-existent resource, and, or resources
> * which are in an 'UNUSABLE' allocation state. (PAPR 2.7,
> * 13.5.3.5)
> @@ -90,13 +98,25 @@ static uint32_t drc_unisolate_physical(sPAPRDRConnector *drc)
> return RTAS_OUT_NO_SUCH_INDICATOR;
> }
>
> - drc->isolation_state = SPAPR_DR_ISOLATION_STATE_UNISOLATED;
> + drc->state = SPAPR_DRC_STATE_PHYSICAL_UNISOLATE;
>
> return RTAS_OUT_SUCCESS;
> }
>
> static uint32_t drc_isolate_logical(sPAPRDRConnector *drc)
> {
> + switch (drc->state) {
> + case SPAPR_DRC_STATE_LOGICAL_AVAILABLE:
> + case SPAPR_DRC_STATE_LOGICAL_UNUSABLE:
> + return RTAS_OUT_SUCCESS; /* Nothing to do */
> + case SPAPR_DRC_STATE_LOGICAL_CONFIGURED:
> + break; /* see below */
> + case SPAPR_DRC_STATE_LOGICAL_UNISOLATE:
> + return RTAS_OUT_PARAM_ERROR; /* not allowed */
> + default:
> + g_assert_not_reached();
> + }
> +
> /* if the guest is configuring a device attached to this DRC, we
> * should reset the configuration state at this point since it may
> * no longer be reliable (guest released device and needs to start
> @@ -120,7 +140,7 @@ static uint32_t drc_isolate_logical(sPAPRDRConnector *drc)
> return RTAS_OUT_HW_ERROR;
> }
>
> - drc->isolation_state = SPAPR_DR_ISOLATION_STATE_ISOLATED;
> + drc->state = SPAPR_DRC_STATE_LOGICAL_AVAILABLE;
>
> /* if we're awaiting release, but still in an unconfigured state,
> * it's likely the guest is still in the process of configuring
> @@ -132,36 +152,46 @@ static uint32_t drc_isolate_logical(sPAPRDRConnector *drc)
> */
> if (drc->unplug_requested) {
> uint32_t drc_index = spapr_drc_index(drc);
> - if (drc->configured) {
> - trace_spapr_drc_set_isolation_state_finalizing(drc_index);
> - spapr_drc_detach(drc);
> - } else {
> - trace_spapr_drc_set_isolation_state_deferring(drc_index);
> - }
> + trace_spapr_drc_set_isolation_state_finalizing(drc_index);
> + spapr_drc_detach(drc);
> }
> - drc->configured = false;
> -
> return RTAS_OUT_SUCCESS;
> }
>
> static uint32_t drc_unisolate_logical(sPAPRDRConnector *drc)
> {
> - /* cannot unisolate a non-existent resource, and, or resources
> - * which are in an 'UNUSABLE' allocation state. (PAPR 2.7,
> - * 13.5.3.5)
> - */
> - if (!drc->dev ||
> - drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
> - return RTAS_OUT_NO_SUCH_INDICATOR;
> + switch (drc->state) {
> + case SPAPR_DRC_STATE_LOGICAL_UNISOLATE:
> + case SPAPR_DRC_STATE_LOGICAL_CONFIGURED:
> + return RTAS_OUT_SUCCESS; /* Nothing to do */
> + case SPAPR_DRC_STATE_LOGICAL_AVAILABLE:
> + break; /* see below */
> + case SPAPR_DRC_STATE_LOGICAL_UNUSABLE:
> + return RTAS_OUT_NO_SUCH_INDICATOR; /* not allowed */
> + default:
> + g_assert_not_reached();
> }
>
> - drc->isolation_state = SPAPR_DR_ISOLATION_STATE_UNISOLATED;
> + /* Move to AVAILABLE state should have ensured device was present */
> + g_assert(drc->dev);
>
> + drc->state = SPAPR_DRC_STATE_LOGICAL_UNISOLATE;
> return RTAS_OUT_SUCCESS;
> }
>
> static uint32_t drc_set_usable(sPAPRDRConnector *drc)
> {
> + switch (drc->state) {
> + case SPAPR_DRC_STATE_LOGICAL_AVAILABLE:
> + case SPAPR_DRC_STATE_LOGICAL_UNISOLATE:
> + case SPAPR_DRC_STATE_LOGICAL_CONFIGURED:
> + return RTAS_OUT_SUCCESS; /* Nothing to do */
> + case SPAPR_DRC_STATE_LOGICAL_UNUSABLE:
> + break; /* see below */
> + default:
> + g_assert_not_reached();
> + }
> +
> /* if there's no resource/device associated with the DRC, there's
> * no way for us to put it in an allocation state consistent with
> * being 'USABLE'. PAPR 2.7, 13.5.3.4 documents that this should
> @@ -176,14 +206,26 @@ static uint32_t drc_set_usable(sPAPRDRConnector *drc)
> return RTAS_OUT_NO_SUCH_INDICATOR;
> }
>
> - drc->allocation_state = SPAPR_DR_ALLOCATION_STATE_USABLE;
> + drc->state = SPAPR_DRC_STATE_LOGICAL_AVAILABLE;
>
> return RTAS_OUT_SUCCESS;
> }
>
> static uint32_t drc_set_unusable(sPAPRDRConnector *drc)
> {
> - drc->allocation_state = SPAPR_DR_ALLOCATION_STATE_UNUSABLE;
> + switch (drc->state) {
> + case SPAPR_DRC_STATE_LOGICAL_UNUSABLE:
> + return RTAS_OUT_SUCCESS; /* Nothing to do */
> + case SPAPR_DRC_STATE_LOGICAL_AVAILABLE:
> + break; /* see below */
> + case SPAPR_DRC_STATE_LOGICAL_UNISOLATE:
> + case SPAPR_DRC_STATE_LOGICAL_CONFIGURED:
> + return RTAS_OUT_NO_SUCH_INDICATOR; /* not allowed */
> + default:
> + g_assert_not_reached();
> + }
> +
> + drc->state = SPAPR_DRC_STATE_LOGICAL_UNUSABLE;
> if (drc->unplug_requested) {
> uint32_t drc_index = spapr_drc_index(drc);
> trace_spapr_drc_set_allocation_state_finalizing(drc_index);
> @@ -241,11 +283,16 @@ static sPAPRDREntitySense physical_entity_sense(sPAPRDRConnector *drc)
>
> static sPAPRDREntitySense logical_entity_sense(sPAPRDRConnector *drc)
> {
> - if (drc->dev
> - && (drc->allocation_state != SPAPR_DR_ALLOCATION_STATE_UNUSABLE)) {
> - return SPAPR_DR_ENTITY_SENSE_PRESENT;
> - } else {
> + switch (drc->state) {
> + case SPAPR_DRC_STATE_LOGICAL_UNUSABLE:
> return SPAPR_DR_ENTITY_SENSE_UNUSABLE;
> + case SPAPR_DRC_STATE_LOGICAL_AVAILABLE:
> + case SPAPR_DRC_STATE_LOGICAL_UNISOLATE:
> + case SPAPR_DRC_STATE_LOGICAL_CONFIGURED:
> + g_assert(drc->dev);
> + return SPAPR_DR_ENTITY_SENSE_PRESENT;
> + default:
> + g_assert_not_reached();
> }
> }
>
> @@ -338,13 +385,12 @@ void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
> {
> trace_spapr_drc_attach(spapr_drc_index(drc));
>
> - if (drc->isolation_state != SPAPR_DR_ISOLATION_STATE_ISOLATED) {
> + if (drc->dev) {
> error_setg(errp, "an attached device is still awaiting release");
> return;
> }
> - if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_PCI) {
> - g_assert(drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_USABLE);
> - }
> + g_assert((drc->state == SPAPR_DRC_STATE_LOGICAL_UNUSABLE)
> + || (drc->state == SPAPR_DRC_STATE_PHYSICAL_POWERON));
> g_assert(fdt);
>
> drc->dev = d;
> @@ -373,18 +419,16 @@ static void spapr_drc_release(sPAPRDRConnector *drc)
>
> void spapr_drc_detach(sPAPRDRConnector *drc)
> {
> + sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> +
> trace_spapr_drc_detach(spapr_drc_index(drc));
>
> - drc->unplug_requested = true;
> + g_assert(drc->dev);
>
> - if (drc->isolation_state != SPAPR_DR_ISOLATION_STATE_ISOLATED) {
> - trace_spapr_drc_awaiting_isolated(spapr_drc_index(drc));
> - return;
> - }
> + drc->unplug_requested = true;
>
> - if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI &&
> - drc->allocation_state != SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
> - trace_spapr_drc_awaiting_unusable(spapr_drc_index(drc));
> + if (drc->state != drck->empty_state) {
> + trace_spapr_drc_awaiting_quiesce(spapr_drc_index(drc));
> return;
> }
>
> @@ -393,6 +437,8 @@ void spapr_drc_detach(sPAPRDRConnector *drc)
>
> void spapr_drc_reset(sPAPRDRConnector *drc)
> {
> + sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> +
> trace_spapr_drc_reset(spapr_drc_index(drc));
>
> g_free(drc->ccs);
> @@ -406,19 +452,10 @@ void spapr_drc_reset(sPAPRDRConnector *drc)
> }
>
> if (drc->dev) {
> - /* A device present at reset is coldplugged */
> - drc->isolation_state = SPAPR_DR_ISOLATION_STATE_UNISOLATED;
> - if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
> - drc->allocation_state = SPAPR_DR_ALLOCATION_STATE_USABLE;
> - }
> - drc->dr_indicator = SPAPR_DR_INDICATOR_ACTIVE;
> + /* A device present at reset is ready to go, same as coldplugged */
> + drc->state = drck->ready_state;
> } else {
> - /* Otherwise device is absent, but might be hotplugged */
> - drc->isolation_state = SPAPR_DR_ISOLATION_STATE_ISOLATED;
> - if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
> - drc->allocation_state = SPAPR_DR_ALLOCATION_STATE_UNUSABLE;
> - }
> - drc->dr_indicator = SPAPR_DR_INDICATOR_INACTIVE;
> + drc->state = drck->empty_state;
> }
> }
>
> @@ -431,7 +468,6 @@ static bool spapr_drc_needed(void *opaque)
> {
> sPAPRDRConnector *drc = (sPAPRDRConnector *)opaque;
> sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> - bool rc = false;
> sPAPRDREntitySense value = drck->dr_entity_sense(drc);
>
> /* If no dev is plugged in there is no need to migrate the DRC state */
> @@ -440,23 +476,10 @@ static bool spapr_drc_needed(void *opaque)
> }
>
> /*
> - * If there is dev plugged in, we need to migrate the DRC state when
> - * it is different from cold-plugged state
> - */
> - switch (spapr_drc_type(drc)) {
> - case SPAPR_DR_CONNECTOR_TYPE_PCI:
> - case SPAPR_DR_CONNECTOR_TYPE_CPU:
> - case SPAPR_DR_CONNECTOR_TYPE_LMB:
> - rc = !((drc->isolation_state == SPAPR_DR_ISOLATION_STATE_UNISOLATED) &&
> - (drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_USABLE) &&
> - drc->configured);
> - break;
> - case SPAPR_DR_CONNECTOR_TYPE_PHB:
> - case SPAPR_DR_CONNECTOR_TYPE_VIO:
> - default:
> - g_assert_not_reached();
> - }
> - return rc;
> + * We need to migrate the state if it's not equal to the expected
> + * long-term state, which is the same as the coldplugged initial
> + * state */
> + return (drc->state != drck->ready_state);
> }
>
> static const VMStateDescription vmstate_spapr_drc = {
> @@ -465,10 +488,8 @@ static const VMStateDescription vmstate_spapr_drc = {
> .minimum_version_id = 1,
> .needed = spapr_drc_needed,
> .fields = (VMStateField []) {
> - VMSTATE_UINT32(isolation_state, sPAPRDRConnector),
> - VMSTATE_UINT32(allocation_state, sPAPRDRConnector),
> + VMSTATE_UINT32(state, sPAPRDRConnector),
> VMSTATE_UINT32(dr_indicator, sPAPRDRConnector),
> - VMSTATE_BOOL(configured, sPAPRDRConnector),
> VMSTATE_END_OF_LIST()
> }
> };
> @@ -537,23 +558,20 @@ sPAPRDRConnector *spapr_dr_connector_new(Object *owner, const char *type,
> object_property_set_bool(OBJECT(drc), true, "realized", NULL);
> g_free(prop_name);
>
> - /* PCI slot always start in a USABLE state, and stay there */
> - if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_PCI) {
> - drc->allocation_state = SPAPR_DR_ALLOCATION_STATE_USABLE;
> - }
> -
> return drc;
> }
>
> static void spapr_dr_connector_instance_init(Object *obj)
> {
> sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(obj);
> + sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
>
> object_property_add_uint32_ptr(obj, "id", &drc->id, NULL);
> object_property_add(obj, "index", "uint32", prop_get_index,
> NULL, NULL, NULL, NULL);
> object_property_add(obj, "fdt", "struct", prop_get_fdt,
> NULL, NULL, NULL, NULL);
> + drc->state = drck->empty_state;
> }
>
> static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
> @@ -575,6 +593,8 @@ static void spapr_drc_physical_class_init(ObjectClass *k, void *data)
> drck->dr_entity_sense = physical_entity_sense;
> drck->isolate = drc_isolate_physical;
> drck->unisolate = drc_unisolate_physical;
> + drck->ready_state = SPAPR_DRC_STATE_PHYSICAL_CONFIGURED;
> + drck->empty_state = SPAPR_DRC_STATE_PHYSICAL_POWERON;
> }
>
> static void spapr_drc_logical_class_init(ObjectClass *k, void *data)
> @@ -584,6 +604,8 @@ static void spapr_drc_logical_class_init(ObjectClass *k, void *data)
> drck->dr_entity_sense = logical_entity_sense;
> drck->isolate = drc_isolate_logical;
> drck->unisolate = drc_unisolate_logical;
> + drck->ready_state = SPAPR_DRC_STATE_LOGICAL_CONFIGURED;
> + drck->empty_state = SPAPR_DRC_STATE_LOGICAL_UNUSABLE;
> }
>
> static void spapr_drc_cpu_class_init(ObjectClass *k, void *data)
> @@ -987,6 +1009,7 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
> uint64_t wa_offset;
> uint32_t drc_index;
> sPAPRDRConnector *drc;
> + sPAPRDRConnectorClass *drck;
> sPAPRConfigureConnectorState *ccs;
> sPAPRDRCCResponse resp = SPAPR_DR_CC_RESPONSE_CONTINUE;
> int rc;
> @@ -1006,12 +1029,17 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
> goto out;
> }
>
> - if (!drc->fdt) {
> - trace_spapr_rtas_ibm_configure_connector_missing_fdt(drc_index);
> + if ((drc->state != SPAPR_DRC_STATE_LOGICAL_UNISOLATE)
> + && (drc->state != SPAPR_DRC_STATE_PHYSICAL_UNISOLATE)) {
> + /* Need to unisolate the device before configuring */
> rc = SPAPR_DR_CC_RESPONSE_NOT_CONFIGURABLE;
> goto out;
> }
>
> + g_assert(drc->fdt);
> +
> + drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> +
> ccs = drc->ccs;
> if (!ccs) {
> ccs = g_new0(sPAPRConfigureConnectorState, 1);
> @@ -1041,18 +1069,11 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
> case FDT_END_NODE:
> ccs->fdt_depth--;
> if (ccs->fdt_depth == 0) {
> - sPAPRDRIsolationState state = drc->isolation_state;
> uint32_t drc_index = spapr_drc_index(drc);
> - /* done sending the device tree, don't need to track
> - * the state anymore
> - */
> +
> + /* done sending the device tree, move to configured state */
> trace_spapr_drc_set_configured(drc_index);
> - if (state == SPAPR_DR_ISOLATION_STATE_UNISOLATED) {
> - drc->configured = true;
> - } else {
> - /* guest should be not configuring an isolated device */
> - trace_spapr_drc_set_configured_skipping(drc_index);
> - }
> + drc->state = drck->ready_state;
> g_free(ccs);
> drc->ccs = NULL;
> ccs = NULL;
> diff --git a/hw/ppc/trace-events b/hw/ppc/trace-events
> index 3e8e3cf..8e79f7e 100644
> --- a/hw/ppc/trace-events
> +++ b/hw/ppc/trace-events
> @@ -46,8 +46,7 @@ spapr_drc_set_configured(uint32_t index) "drc: 0x%"PRIx32
> spapr_drc_set_configured_skipping(uint32_t index) "drc: 0x%"PRIx32", isolated device"
> spapr_drc_attach(uint32_t index) "drc: 0x%"PRIx32
> spapr_drc_detach(uint32_t index) "drc: 0x%"PRIx32
> -spapr_drc_awaiting_isolated(uint32_t index) "drc: 0x%"PRIx32
> -spapr_drc_awaiting_unusable(uint32_t index) "drc: 0x%"PRIx32
> +spapr_drc_awaiting_quiesce(uint32_t index) "drc: 0x%"PRIx32
> spapr_drc_awaiting_allocation(uint32_t index) "drc: 0x%"PRIx32
> spapr_drc_reset(uint32_t index) "drc: 0x%"PRIx32
> spapr_drc_realize(uint32_t index) "drc: 0x%"PRIx32
> diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> index 5fa502e..4ceaaf0 100644
> --- a/include/hw/ppc/spapr_drc.h
> +++ b/include/hw/ppc/spapr_drc.h
> @@ -173,6 +173,24 @@ typedef enum {
> SPAPR_DR_CC_RESPONSE_NOT_CONFIGURABLE = -9003,
> } sPAPRDRCCResponse;
>
> +typedef enum {
> + /*
> + * Values come from Fig. 12 in LoPAPR section 13.4
> + *
> + * These are exposed in the migration stream, so don't change
> + * them.
> + */
> + SPAPR_DRC_STATE_INVALID = 0,
> + SPAPR_DRC_STATE_LOGICAL_UNUSABLE = 1,
> + SPAPR_DRC_STATE_LOGICAL_AVAILABLE = 2,
> + SPAPR_DRC_STATE_LOGICAL_UNISOLATE = 3,
> + SPAPR_DRC_STATE_LOGICAL_CONFIGURED = 4,
> + SPAPR_DRC_STATE_PHYSICAL_AVAILABLE = 5,
> + SPAPR_DRC_STATE_PHYSICAL_POWERON = 6,
> + SPAPR_DRC_STATE_PHYSICAL_UNISOLATE = 7,
> + SPAPR_DRC_STATE_PHYSICAL_CONFIGURED = 8,
> +} sPAPRDRCState;
> +
> /* rtas-configure-connector state */
> typedef struct sPAPRConfigureConnectorState {
> int fdt_offset;
> @@ -189,14 +207,11 @@ typedef struct sPAPRDRConnector {
> /* DR-indicator */
> uint32_t dr_indicator;
>
> - /* sensor/indicator states */
> - uint32_t isolation_state;
> - uint32_t allocation_state;
> + uint32_t state;
>
> /* configure-connector state */
> void *fdt;
> int fdt_start_offset;
> - bool configured;
> sPAPRConfigureConnectorState *ccs;
>
> /* device pointer, via link property */
> @@ -207,6 +222,8 @@ typedef struct sPAPRDRConnector {
> typedef struct sPAPRDRConnectorClass {
> /*< private >*/
> DeviceClass parent;
> + sPAPRDRCState empty_state;
> + sPAPRDRCState ready_state;
>
> /*< public >*/
> sPAPRDRConnectorTypeShift typeshift;
^ permalink raw reply [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCHv2 7/8] spapr: Remove sPAPRConfigureConnectorState sub-structure
2017-07-12 5:53 [Qemu-devel] [PATCHv2 0/8] spapr: DRC cleanups (part VI) David Gibson
` (5 preceding siblings ...)
2017-07-12 5:53 ` [Qemu-devel] [PATCHv2 6/8] spapr: Consolidate DRC state variables David Gibson
@ 2017-07-12 5:53 ` David Gibson
2017-07-12 17:40 ` [Qemu-devel] [Qemu-ppc] " Daniel Henrique Barboza
2017-07-12 5:53 ` [Qemu-devel] [PATCHv2 8/8] spapr: Implement DR-indicator for physical DRCs only David Gibson
` (2 subsequent siblings)
9 siblings, 1 reply; 30+ messages in thread
From: David Gibson @ 2017-07-12 5:53 UTC (permalink / raw)
To: mdroth, groug, danielhb
Cc: qemu-ppc, qemu-devel, lvivier, sjitindarsingh, bharata,
David Gibson
Most of the time, the state of a DRC object is contained in the single
'state' variable. However, during the transition from UNISOLATE to
CONFIGURED state requires multiple calls to the ibm,configure-connector
RTAS call to retrieve the device tree for the attached device. We need
some extra state to keep track of where we're up to in delivering the
device tree information to the guest.
Currently that extra state is in a sPAPRConfigureConnectorState
substructure which is only allocated when we're in the middle of the
configure connector process. That sounds like a good idea, but the extra
state is only two integers - on many platforms that will take up the same
room as the (maybe NULL) ccs pointer even before malloc() overhead. Plus
it's another object whose lifetime we need to manage. In short, it's not
worth it.
So, fold the sPAPRConfigureConnectorState substructure directly into the
DRC object.
Previously the structure was allocated lazily when the configure-connector
call discovers it's not there. Now, we need to initialize the subfields
pre-emptively, as soon as we enter UNISOLATE state.
Although it's not strictly necessary (the field values should only ever
be consulted when in UNISOLATE state), we try to keep them at -1 when in
other states, as a debugging aid.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
hw/ppc/spapr_drc.c | 56 +++++++++++++++-------------------------------
include/hw/ppc/spapr_drc.h | 16 +++++--------
2 files changed, 24 insertions(+), 48 deletions(-)
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index dbc9c0f..423db80 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -59,14 +59,6 @@ static uint32_t drc_isolate_physical(sPAPRDRConnector *drc)
g_assert_not_reached();
}
- /* if the guest is configuring a device attached to this DRC, we
- * should reset the configuration state at this point since it may
- * no longer be reliable (guest released device and needs to start
- * over, or unplug occurred so the FDT is no longer valid)
- */
- g_free(drc->ccs);
- drc->ccs = NULL;
-
drc->state = SPAPR_DRC_STATE_PHYSICAL_POWERON;
if (drc->unplug_requested) {
@@ -99,6 +91,8 @@ static uint32_t drc_unisolate_physical(sPAPRDRConnector *drc)
}
drc->state = SPAPR_DRC_STATE_PHYSICAL_UNISOLATE;
+ drc->ccs_offset = drc->fdt_start_offset;
+ drc->ccs_depth = 0;
return RTAS_OUT_SUCCESS;
}
@@ -117,14 +111,6 @@ static uint32_t drc_isolate_logical(sPAPRDRConnector *drc)
g_assert_not_reached();
}
- /* if the guest is configuring a device attached to this DRC, we
- * should reset the configuration state at this point since it may
- * no longer be reliable (guest released device and needs to start
- * over, or unplug occurred so the FDT is no longer valid)
- */
- g_free(drc->ccs);
- drc->ccs = NULL;
-
/*
* Fail any requests to ISOLATE the LMB DRC if this LMB doesn't
* belong to a DIMM device that is marked for removal.
@@ -176,6 +162,9 @@ static uint32_t drc_unisolate_logical(sPAPRDRConnector *drc)
g_assert(drc->dev);
drc->state = SPAPR_DRC_STATE_LOGICAL_UNISOLATE;
+ drc->ccs_offset = drc->fdt_start_offset;
+ drc->ccs_depth = 0;
+
return RTAS_OUT_SUCCESS;
}
@@ -441,9 +430,6 @@ void spapr_drc_reset(sPAPRDRConnector *drc)
trace_spapr_drc_reset(spapr_drc_index(drc));
- g_free(drc->ccs);
- drc->ccs = NULL;
-
/* immediately upon reset we can safely assume DRCs whose devices
* are pending removal can be safely removed.
*/
@@ -457,6 +443,9 @@ void spapr_drc_reset(sPAPRDRConnector *drc)
} else {
drc->state = drck->empty_state;
}
+
+ drc->ccs_offset = -1;
+ drc->ccs_depth = -1;
}
static void drc_reset(void *opaque)
@@ -1010,7 +999,6 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
uint32_t drc_index;
sPAPRDRConnector *drc;
sPAPRDRConnectorClass *drck;
- sPAPRConfigureConnectorState *ccs;
sPAPRDRCCResponse resp = SPAPR_DR_CC_RESPONSE_CONTINUE;
int rc;
@@ -1040,25 +1028,18 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
- ccs = drc->ccs;
- if (!ccs) {
- ccs = g_new0(sPAPRConfigureConnectorState, 1);
- ccs->fdt_offset = drc->fdt_start_offset;
- drc->ccs = ccs;
- }
-
do {
uint32_t tag;
const char *name;
const struct fdt_property *prop;
int fdt_offset_next, prop_len;
- tag = fdt_next_tag(drc->fdt, ccs->fdt_offset, &fdt_offset_next);
+ tag = fdt_next_tag(drc->fdt, drc->ccs_offset, &fdt_offset_next);
switch (tag) {
case FDT_BEGIN_NODE:
- ccs->fdt_depth++;
- name = fdt_get_name(drc->fdt, ccs->fdt_offset, NULL);
+ drc->ccs_depth++;
+ name = fdt_get_name(drc->fdt, drc->ccs_offset, NULL);
/* provide the name of the next OF node */
wa_offset = CC_VAL_DATA_OFFSET;
@@ -1067,23 +1048,22 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
resp = SPAPR_DR_CC_RESPONSE_NEXT_CHILD;
break;
case FDT_END_NODE:
- ccs->fdt_depth--;
- if (ccs->fdt_depth == 0) {
+ drc->ccs_depth--;
+ if (drc->ccs_depth == 0) {
uint32_t drc_index = spapr_drc_index(drc);
/* done sending the device tree, move to configured state */
trace_spapr_drc_set_configured(drc_index);
drc->state = drck->ready_state;
- g_free(ccs);
- drc->ccs = NULL;
- ccs = NULL;
+ drc->ccs_offset = -1;
+ drc->ccs_depth = -1;
resp = SPAPR_DR_CC_RESPONSE_SUCCESS;
} else {
resp = SPAPR_DR_CC_RESPONSE_PREV_PARENT;
}
break;
case FDT_PROP:
- prop = fdt_get_property_by_offset(drc->fdt, ccs->fdt_offset,
+ prop = fdt_get_property_by_offset(drc->fdt, drc->ccs_offset,
&prop_len);
name = fdt_string(drc->fdt, fdt32_to_cpu(prop->nameoff));
@@ -1108,8 +1088,8 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
/* keep seeking for an actionable tag */
break;
}
- if (ccs) {
- ccs->fdt_offset = fdt_offset_next;
+ if (drc->ccs_offset >= 0) {
+ drc->ccs_offset = fdt_offset_next;
}
} while (resp == SPAPR_DR_CC_RESPONSE_CONTINUE);
diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
index 4ceaaf0..9d4fd41 100644
--- a/include/hw/ppc/spapr_drc.h
+++ b/include/hw/ppc/spapr_drc.h
@@ -191,12 +191,6 @@ typedef enum {
SPAPR_DRC_STATE_PHYSICAL_CONFIGURED = 8,
} sPAPRDRCState;
-/* rtas-configure-connector state */
-typedef struct sPAPRConfigureConnectorState {
- int fdt_offset;
- int fdt_depth;
-} sPAPRConfigureConnectorState;
-
typedef struct sPAPRDRConnector {
/*< private >*/
DeviceState parent;
@@ -209,14 +203,16 @@ typedef struct sPAPRDRConnector {
uint32_t state;
- /* configure-connector state */
- void *fdt;
- int fdt_start_offset;
- sPAPRConfigureConnectorState *ccs;
+ /* RTAS ibm,configure-connector state */
+ /* (only valid in UNISOLATE state) */
+ int ccs_offset;
+ int ccs_depth;
/* device pointer, via link property */
DeviceState *dev;
bool unplug_requested;
+ void *fdt;
+ int fdt_start_offset;
} sPAPRDRConnector;
typedef struct sPAPRDRConnectorClass {
--
2.9.4
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [Qemu-devel] [Qemu-ppc] [PATCHv2 7/8] spapr: Remove sPAPRConfigureConnectorState sub-structure
2017-07-12 5:53 ` [Qemu-devel] [PATCHv2 7/8] spapr: Remove sPAPRConfigureConnectorState sub-structure David Gibson
@ 2017-07-12 17:40 ` Daniel Henrique Barboza
0 siblings, 0 replies; 30+ messages in thread
From: Daniel Henrique Barboza @ 2017-07-12 17:40 UTC (permalink / raw)
To: David Gibson, mdroth, groug
Cc: lvivier, qemu-devel, qemu-ppc, sjitindarsingh, bharata
On 07/12/2017 02:53 AM, David Gibson wrote:
> Most of the time, the state of a DRC object is contained in the single
> 'state' variable. However, during the transition from UNISOLATE to
> CONFIGURED state requires multiple calls to the ibm,configure-connector
> RTAS call to retrieve the device tree for the attached device. We need
> some extra state to keep track of where we're up to in delivering the
> device tree information to the guest.
>
> Currently that extra state is in a sPAPRConfigureConnectorState
> substructure which is only allocated when we're in the middle of the
> configure connector process. That sounds like a good idea, but the extra
> state is only two integers - on many platforms that will take up the same
> room as the (maybe NULL) ccs pointer even before malloc() overhead. Plus
> it's another object whose lifetime we need to manage. In short, it's not
> worth it.
>
> So, fold the sPAPRConfigureConnectorState substructure directly into the
> DRC object.
>
> Previously the structure was allocated lazily when the configure-connector
> call discovers it's not there. Now, we need to initialize the subfields
> pre-emptively, as soon as we enter UNISOLATE state.
>
> Although it's not strictly necessary (the field values should only ever
> be consulted when in UNISOLATE state), we try to keep them at -1 when in
> other states, as a debugging aid.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Daniel Barboza <danielhb@linux.vnet.ibm.com>
> ---
> hw/ppc/spapr_drc.c | 56 +++++++++++++++-------------------------------
> include/hw/ppc/spapr_drc.h | 16 +++++--------
> 2 files changed, 24 insertions(+), 48 deletions(-)
>
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index dbc9c0f..423db80 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -59,14 +59,6 @@ static uint32_t drc_isolate_physical(sPAPRDRConnector *drc)
> g_assert_not_reached();
> }
>
> - /* if the guest is configuring a device attached to this DRC, we
> - * should reset the configuration state at this point since it may
> - * no longer be reliable (guest released device and needs to start
> - * over, or unplug occurred so the FDT is no longer valid)
> - */
> - g_free(drc->ccs);
> - drc->ccs = NULL;
> -
> drc->state = SPAPR_DRC_STATE_PHYSICAL_POWERON;
>
> if (drc->unplug_requested) {
> @@ -99,6 +91,8 @@ static uint32_t drc_unisolate_physical(sPAPRDRConnector *drc)
> }
>
> drc->state = SPAPR_DRC_STATE_PHYSICAL_UNISOLATE;
> + drc->ccs_offset = drc->fdt_start_offset;
> + drc->ccs_depth = 0;
>
> return RTAS_OUT_SUCCESS;
> }
> @@ -117,14 +111,6 @@ static uint32_t drc_isolate_logical(sPAPRDRConnector *drc)
> g_assert_not_reached();
> }
>
> - /* if the guest is configuring a device attached to this DRC, we
> - * should reset the configuration state at this point since it may
> - * no longer be reliable (guest released device and needs to start
> - * over, or unplug occurred so the FDT is no longer valid)
> - */
> - g_free(drc->ccs);
> - drc->ccs = NULL;
> -
> /*
> * Fail any requests to ISOLATE the LMB DRC if this LMB doesn't
> * belong to a DIMM device that is marked for removal.
> @@ -176,6 +162,9 @@ static uint32_t drc_unisolate_logical(sPAPRDRConnector *drc)
> g_assert(drc->dev);
>
> drc->state = SPAPR_DRC_STATE_LOGICAL_UNISOLATE;
> + drc->ccs_offset = drc->fdt_start_offset;
> + drc->ccs_depth = 0;
> +
> return RTAS_OUT_SUCCESS;
> }
>
> @@ -441,9 +430,6 @@ void spapr_drc_reset(sPAPRDRConnector *drc)
>
> trace_spapr_drc_reset(spapr_drc_index(drc));
>
> - g_free(drc->ccs);
> - drc->ccs = NULL;
> -
> /* immediately upon reset we can safely assume DRCs whose devices
> * are pending removal can be safely removed.
> */
> @@ -457,6 +443,9 @@ void spapr_drc_reset(sPAPRDRConnector *drc)
> } else {
> drc->state = drck->empty_state;
> }
> +
> + drc->ccs_offset = -1;
> + drc->ccs_depth = -1;
> }
>
> static void drc_reset(void *opaque)
> @@ -1010,7 +999,6 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
> uint32_t drc_index;
> sPAPRDRConnector *drc;
> sPAPRDRConnectorClass *drck;
> - sPAPRConfigureConnectorState *ccs;
> sPAPRDRCCResponse resp = SPAPR_DR_CC_RESPONSE_CONTINUE;
> int rc;
>
> @@ -1040,25 +1028,18 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
>
> drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
>
> - ccs = drc->ccs;
> - if (!ccs) {
> - ccs = g_new0(sPAPRConfigureConnectorState, 1);
> - ccs->fdt_offset = drc->fdt_start_offset;
> - drc->ccs = ccs;
> - }
> -
> do {
> uint32_t tag;
> const char *name;
> const struct fdt_property *prop;
> int fdt_offset_next, prop_len;
>
> - tag = fdt_next_tag(drc->fdt, ccs->fdt_offset, &fdt_offset_next);
> + tag = fdt_next_tag(drc->fdt, drc->ccs_offset, &fdt_offset_next);
>
> switch (tag) {
> case FDT_BEGIN_NODE:
> - ccs->fdt_depth++;
> - name = fdt_get_name(drc->fdt, ccs->fdt_offset, NULL);
> + drc->ccs_depth++;
> + name = fdt_get_name(drc->fdt, drc->ccs_offset, NULL);
>
> /* provide the name of the next OF node */
> wa_offset = CC_VAL_DATA_OFFSET;
> @@ -1067,23 +1048,22 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
> resp = SPAPR_DR_CC_RESPONSE_NEXT_CHILD;
> break;
> case FDT_END_NODE:
> - ccs->fdt_depth--;
> - if (ccs->fdt_depth == 0) {
> + drc->ccs_depth--;
> + if (drc->ccs_depth == 0) {
> uint32_t drc_index = spapr_drc_index(drc);
>
> /* done sending the device tree, move to configured state */
> trace_spapr_drc_set_configured(drc_index);
> drc->state = drck->ready_state;
> - g_free(ccs);
> - drc->ccs = NULL;
> - ccs = NULL;
> + drc->ccs_offset = -1;
> + drc->ccs_depth = -1;
> resp = SPAPR_DR_CC_RESPONSE_SUCCESS;
> } else {
> resp = SPAPR_DR_CC_RESPONSE_PREV_PARENT;
> }
> break;
> case FDT_PROP:
> - prop = fdt_get_property_by_offset(drc->fdt, ccs->fdt_offset,
> + prop = fdt_get_property_by_offset(drc->fdt, drc->ccs_offset,
> &prop_len);
> name = fdt_string(drc->fdt, fdt32_to_cpu(prop->nameoff));
>
> @@ -1108,8 +1088,8 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
> /* keep seeking for an actionable tag */
> break;
> }
> - if (ccs) {
> - ccs->fdt_offset = fdt_offset_next;
> + if (drc->ccs_offset >= 0) {
> + drc->ccs_offset = fdt_offset_next;
> }
> } while (resp == SPAPR_DR_CC_RESPONSE_CONTINUE);
>
> diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> index 4ceaaf0..9d4fd41 100644
> --- a/include/hw/ppc/spapr_drc.h
> +++ b/include/hw/ppc/spapr_drc.h
> @@ -191,12 +191,6 @@ typedef enum {
> SPAPR_DRC_STATE_PHYSICAL_CONFIGURED = 8,
> } sPAPRDRCState;
>
> -/* rtas-configure-connector state */
> -typedef struct sPAPRConfigureConnectorState {
> - int fdt_offset;
> - int fdt_depth;
> -} sPAPRConfigureConnectorState;
> -
> typedef struct sPAPRDRConnector {
> /*< private >*/
> DeviceState parent;
> @@ -209,14 +203,16 @@ typedef struct sPAPRDRConnector {
>
> uint32_t state;
>
> - /* configure-connector state */
> - void *fdt;
> - int fdt_start_offset;
> - sPAPRConfigureConnectorState *ccs;
> + /* RTAS ibm,configure-connector state */
> + /* (only valid in UNISOLATE state) */
> + int ccs_offset;
> + int ccs_depth;
>
> /* device pointer, via link property */
> DeviceState *dev;
> bool unplug_requested;
> + void *fdt;
> + int fdt_start_offset;
> } sPAPRDRConnector;
>
> typedef struct sPAPRDRConnectorClass {
^ permalink raw reply [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCHv2 8/8] spapr: Implement DR-indicator for physical DRCs only
2017-07-12 5:53 [Qemu-devel] [PATCHv2 0/8] spapr: DRC cleanups (part VI) David Gibson
` (6 preceding siblings ...)
2017-07-12 5:53 ` [Qemu-devel] [PATCHv2 7/8] spapr: Remove sPAPRConfigureConnectorState sub-structure David Gibson
@ 2017-07-12 5:53 ` David Gibson
2017-07-12 17:44 ` [Qemu-devel] [Qemu-ppc] " Daniel Henrique Barboza
2017-07-12 13:48 ` [Qemu-devel] [Qemu-ppc] [PATCHv2 0/8] spapr: DRC cleanups (part VI) Daniel Henrique Barboza
2017-07-13 1:09 ` [Qemu-devel] " David Gibson
9 siblings, 1 reply; 30+ messages in thread
From: David Gibson @ 2017-07-12 5:53 UTC (permalink / raw)
To: mdroth, groug, danielhb
Cc: qemu-ppc, qemu-devel, lvivier, sjitindarsingh, bharata,
David Gibson
According to PAPR, the DR-indicator should only be valid for physical DRCs,
not logical DRCs. At the moment we implement it for all DRCs, so restrict
it to physical ones only.
We move the state to the physical DRC subclass, which means adding some
QOM boilerplate to handle the newly distinct type.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
hw/ppc/spapr_drc.c | 68 ++++++++++++++++++++++++++++++++++++++++++----
include/hw/ppc/spapr_drc.h | 13 ++++++---
2 files changed, 72 insertions(+), 9 deletions(-)
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 423db80..39b7cef 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -478,7 +478,6 @@ static const VMStateDescription vmstate_spapr_drc = {
.needed = spapr_drc_needed,
.fields = (VMStateField []) {
VMSTATE_UINT32(state, sPAPRDRConnector),
- VMSTATE_UINT32(dr_indicator, sPAPRDRConnector),
VMSTATE_END_OF_LIST()
}
};
@@ -575,10 +574,63 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
dk->user_creatable = false;
}
+static bool drc_physical_needed(void *opaque)
+{
+ sPAPRDRCPhysical *drcp = (sPAPRDRCPhysical *)opaque;
+ sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(drcp);
+
+ if ((drc->dev && (drcp->dr_indicator == SPAPR_DR_INDICATOR_ACTIVE))
+ || (!drc->dev && (drcp->dr_indicator == SPAPR_DR_INDICATOR_INACTIVE))) {
+ return false;
+ }
+ return true;
+}
+
+static const VMStateDescription vmstate_spapr_drc_physical = {
+ .name = "spapr_drc",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .needed = drc_physical_needed,
+ .fields = (VMStateField []) {
+ VMSTATE_UINT32(dr_indicator, sPAPRDRCPhysical),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
+static void drc_physical_reset(void *opaque)
+{
+ sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(opaque);
+ sPAPRDRCPhysical *drcp = SPAPR_DRC_PHYSICAL(drc);
+
+ if (drc->dev) {
+ drcp->dr_indicator = SPAPR_DR_INDICATOR_ACTIVE;
+ } else {
+ drcp->dr_indicator = SPAPR_DR_INDICATOR_INACTIVE;
+ }
+}
+
+static void realize_physical(DeviceState *d, Error **errp)
+{
+ sPAPRDRCPhysical *drcp = SPAPR_DRC_PHYSICAL(d);
+ Error *local_err = NULL;
+
+ realize(d, &local_err);
+ if (local_err) {
+ error_propagate(errp, local_err);
+ return;
+ }
+
+ vmstate_register(DEVICE(drcp), spapr_drc_index(SPAPR_DR_CONNECTOR(drcp)),
+ &vmstate_spapr_drc_physical, drcp);
+ qemu_register_reset(drc_physical_reset, drcp);
+}
+
static void spapr_drc_physical_class_init(ObjectClass *k, void *data)
{
+ DeviceClass *dk = DEVICE_CLASS(k);
sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_CLASS(k);
+ dk->realize = realize_physical;
drck->dr_entity_sense = physical_entity_sense;
drck->isolate = drc_isolate_physical;
drck->unisolate = drc_unisolate_physical;
@@ -640,7 +692,7 @@ static const TypeInfo spapr_dr_connector_info = {
static const TypeInfo spapr_drc_physical_info = {
.name = TYPE_SPAPR_DRC_PHYSICAL,
.parent = TYPE_SPAPR_DR_CONNECTOR,
- .instance_size = sizeof(sPAPRDRConnector),
+ .instance_size = sizeof(sPAPRDRCPhysical),
.class_init = spapr_drc_physical_class_init,
.abstract = true,
};
@@ -883,12 +935,18 @@ static uint32_t rtas_set_dr_indicator(uint32_t idx, uint32_t state)
{
sPAPRDRConnector *drc = spapr_drc_by_index(idx);
- if (!drc) {
- return RTAS_OUT_PARAM_ERROR;
+ if (!drc || !object_dynamic_cast(OBJECT(drc), TYPE_SPAPR_DRC_PHYSICAL)) {
+ return RTAS_OUT_NO_SUCH_INDICATOR;
+ }
+ if ((state != SPAPR_DR_INDICATOR_INACTIVE)
+ && (state != SPAPR_DR_INDICATOR_ACTIVE)
+ && (state != SPAPR_DR_INDICATOR_IDENTIFY)
+ && (state != SPAPR_DR_INDICATOR_ACTION)) {
+ return RTAS_OUT_PARAM_ERROR; /* bad state parameter */
}
trace_spapr_drc_set_dr_indicator(idx, state);
- drc->dr_indicator = state;
+ SPAPR_DRC_PHYSICAL(drc)->dr_indicator = state;
return RTAS_OUT_SUCCESS;
}
diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
index 9d4fd41..a7958d0 100644
--- a/include/hw/ppc/spapr_drc.h
+++ b/include/hw/ppc/spapr_drc.h
@@ -33,7 +33,7 @@
#define SPAPR_DRC_PHYSICAL_CLASS(klass) \
OBJECT_CLASS_CHECK(sPAPRDRConnectorClass, klass, \
TYPE_SPAPR_DRC_PHYSICAL)
-#define SPAPR_DRC_PHYSICAL(obj) OBJECT_CHECK(sPAPRDRConnector, (obj), \
+#define SPAPR_DRC_PHYSICAL(obj) OBJECT_CHECK(sPAPRDRCPhysical, (obj), \
TYPE_SPAPR_DRC_PHYSICAL)
#define TYPE_SPAPR_DRC_LOGICAL "spapr-drc-logical"
@@ -198,9 +198,6 @@ typedef struct sPAPRDRConnector {
uint32_t id;
Object *owner;
- /* DR-indicator */
- uint32_t dr_indicator;
-
uint32_t state;
/* RTAS ibm,configure-connector state */
@@ -232,6 +229,14 @@ typedef struct sPAPRDRConnectorClass {
void (*release)(DeviceState *dev);
} sPAPRDRConnectorClass;
+typedef struct sPAPRDRCPhysical {
+ /*< private >*/
+ sPAPRDRConnector parent;
+
+ /* DR-indicator */
+ uint32_t dr_indicator;
+} sPAPRDRCPhysical;
+
static inline bool spapr_drc_hotplugged(DeviceState *dev)
{
return dev->hotplugged && !runstate_check(RUN_STATE_INMIGRATE);
--
2.9.4
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [Qemu-devel] [Qemu-ppc] [PATCHv2 8/8] spapr: Implement DR-indicator for physical DRCs only
2017-07-12 5:53 ` [Qemu-devel] [PATCHv2 8/8] spapr: Implement DR-indicator for physical DRCs only David Gibson
@ 2017-07-12 17:44 ` Daniel Henrique Barboza
0 siblings, 0 replies; 30+ messages in thread
From: Daniel Henrique Barboza @ 2017-07-12 17:44 UTC (permalink / raw)
To: David Gibson, mdroth, groug
Cc: lvivier, qemu-devel, qemu-ppc, sjitindarsingh, bharata
On 07/12/2017 02:53 AM, David Gibson wrote:
> According to PAPR, the DR-indicator should only be valid for physical DRCs,
> not logical DRCs. At the moment we implement it for all DRCs, so restrict
> it to physical ones only.
>
> We move the state to the physical DRC subclass, which means adding some
> QOM boilerplate to handle the newly distinct type.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Daniel Barboza <danielhb@linux.vnet.ibm.com>
> ---
> hw/ppc/spapr_drc.c | 68 ++++++++++++++++++++++++++++++++++++++++++----
> include/hw/ppc/spapr_drc.h | 13 ++++++---
> 2 files changed, 72 insertions(+), 9 deletions(-)
>
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 423db80..39b7cef 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -478,7 +478,6 @@ static const VMStateDescription vmstate_spapr_drc = {
> .needed = spapr_drc_needed,
> .fields = (VMStateField []) {
> VMSTATE_UINT32(state, sPAPRDRConnector),
> - VMSTATE_UINT32(dr_indicator, sPAPRDRConnector),
> VMSTATE_END_OF_LIST()
> }
> };
> @@ -575,10 +574,63 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
> dk->user_creatable = false;
> }
>
> +static bool drc_physical_needed(void *opaque)
> +{
> + sPAPRDRCPhysical *drcp = (sPAPRDRCPhysical *)opaque;
> + sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(drcp);
> +
> + if ((drc->dev && (drcp->dr_indicator == SPAPR_DR_INDICATOR_ACTIVE))
> + || (!drc->dev && (drcp->dr_indicator == SPAPR_DR_INDICATOR_INACTIVE))) {
> + return false;
> + }
> + return true;
> +}
> +
> +static const VMStateDescription vmstate_spapr_drc_physical = {
> + .name = "spapr_drc",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .needed = drc_physical_needed,
> + .fields = (VMStateField []) {
> + VMSTATE_UINT32(dr_indicator, sPAPRDRCPhysical),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> +static void drc_physical_reset(void *opaque)
> +{
> + sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(opaque);
> + sPAPRDRCPhysical *drcp = SPAPR_DRC_PHYSICAL(drc);
> +
> + if (drc->dev) {
> + drcp->dr_indicator = SPAPR_DR_INDICATOR_ACTIVE;
> + } else {
> + drcp->dr_indicator = SPAPR_DR_INDICATOR_INACTIVE;
> + }
> +}
> +
> +static void realize_physical(DeviceState *d, Error **errp)
> +{
> + sPAPRDRCPhysical *drcp = SPAPR_DRC_PHYSICAL(d);
> + Error *local_err = NULL;
> +
> + realize(d, &local_err);
> + if (local_err) {
> + error_propagate(errp, local_err);
> + return;
> + }
> +
> + vmstate_register(DEVICE(drcp), spapr_drc_index(SPAPR_DR_CONNECTOR(drcp)),
> + &vmstate_spapr_drc_physical, drcp);
> + qemu_register_reset(drc_physical_reset, drcp);
> +}
> +
> static void spapr_drc_physical_class_init(ObjectClass *k, void *data)
> {
> + DeviceClass *dk = DEVICE_CLASS(k);
> sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_CLASS(k);
>
> + dk->realize = realize_physical;
> drck->dr_entity_sense = physical_entity_sense;
> drck->isolate = drc_isolate_physical;
> drck->unisolate = drc_unisolate_physical;
> @@ -640,7 +692,7 @@ static const TypeInfo spapr_dr_connector_info = {
> static const TypeInfo spapr_drc_physical_info = {
> .name = TYPE_SPAPR_DRC_PHYSICAL,
> .parent = TYPE_SPAPR_DR_CONNECTOR,
> - .instance_size = sizeof(sPAPRDRConnector),
> + .instance_size = sizeof(sPAPRDRCPhysical),
> .class_init = spapr_drc_physical_class_init,
> .abstract = true,
> };
> @@ -883,12 +935,18 @@ static uint32_t rtas_set_dr_indicator(uint32_t idx, uint32_t state)
> {
> sPAPRDRConnector *drc = spapr_drc_by_index(idx);
>
> - if (!drc) {
> - return RTAS_OUT_PARAM_ERROR;
> + if (!drc || !object_dynamic_cast(OBJECT(drc), TYPE_SPAPR_DRC_PHYSICAL)) {
> + return RTAS_OUT_NO_SUCH_INDICATOR;
> + }
> + if ((state != SPAPR_DR_INDICATOR_INACTIVE)
> + && (state != SPAPR_DR_INDICATOR_ACTIVE)
> + && (state != SPAPR_DR_INDICATOR_IDENTIFY)
> + && (state != SPAPR_DR_INDICATOR_ACTION)) {
> + return RTAS_OUT_PARAM_ERROR; /* bad state parameter */
> }
>
> trace_spapr_drc_set_dr_indicator(idx, state);
> - drc->dr_indicator = state;
> + SPAPR_DRC_PHYSICAL(drc)->dr_indicator = state;
> return RTAS_OUT_SUCCESS;
> }
>
> diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> index 9d4fd41..a7958d0 100644
> --- a/include/hw/ppc/spapr_drc.h
> +++ b/include/hw/ppc/spapr_drc.h
> @@ -33,7 +33,7 @@
> #define SPAPR_DRC_PHYSICAL_CLASS(klass) \
> OBJECT_CLASS_CHECK(sPAPRDRConnectorClass, klass, \
> TYPE_SPAPR_DRC_PHYSICAL)
> -#define SPAPR_DRC_PHYSICAL(obj) OBJECT_CHECK(sPAPRDRConnector, (obj), \
> +#define SPAPR_DRC_PHYSICAL(obj) OBJECT_CHECK(sPAPRDRCPhysical, (obj), \
> TYPE_SPAPR_DRC_PHYSICAL)
>
> #define TYPE_SPAPR_DRC_LOGICAL "spapr-drc-logical"
> @@ -198,9 +198,6 @@ typedef struct sPAPRDRConnector {
> uint32_t id;
> Object *owner;
>
> - /* DR-indicator */
> - uint32_t dr_indicator;
> -
> uint32_t state;
>
> /* RTAS ibm,configure-connector state */
> @@ -232,6 +229,14 @@ typedef struct sPAPRDRConnectorClass {
> void (*release)(DeviceState *dev);
> } sPAPRDRConnectorClass;
>
> +typedef struct sPAPRDRCPhysical {
> + /*< private >*/
> + sPAPRDRConnector parent;
> +
> + /* DR-indicator */
> + uint32_t dr_indicator;
> +} sPAPRDRCPhysical;
> +
> static inline bool spapr_drc_hotplugged(DeviceState *dev)
> {
> return dev->hotplugged && !runstate_check(RUN_STATE_INMIGRATE);
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [PATCHv2 0/8] spapr: DRC cleanups (part VI)
2017-07-12 5:53 [Qemu-devel] [PATCHv2 0/8] spapr: DRC cleanups (part VI) David Gibson
` (7 preceding siblings ...)
2017-07-12 5:53 ` [Qemu-devel] [PATCHv2 8/8] spapr: Implement DR-indicator for physical DRCs only David Gibson
@ 2017-07-12 13:48 ` Daniel Henrique Barboza
2017-07-13 0:57 ` David Gibson
2017-07-13 1:09 ` [Qemu-devel] " David Gibson
9 siblings, 1 reply; 30+ messages in thread
From: Daniel Henrique Barboza @ 2017-07-12 13:48 UTC (permalink / raw)
To: David Gibson, mdroth, groug
Cc: lvivier, qemu-devel, qemu-ppc, sjitindarsingh, bharata
The dreaded Libvirt hotplug-migrate-hotunplug scenario is working nicely.
device_add when the machine is in RUN_STATE_PRELAUNCH (-S) still doesn't
work but it is expected - as discussed in "[RFC drcVI PATCH] spapr:
reset DRCs
on migration pre_load", this scenario can't be fixed solely by this DRC
cleanup.
Given that we'll review the DT code sometime in the future I guess we can
postpone the fix for device_adding in pre-launch for that time.
Tested-by: Daniel Barboza <danielhb@linux.vnet.ibm.com>
On 07/12/2017 02:53 AM, David Gibson wrote:
> This sixth set of DRC cleanup patches is a complete rework of DRC
> state management. We stop tracking some unnecessary things, and
> change the basic state representation to a simpler and more robust
> model.
>
> Many of the patches in this set "break" migration from earlier git
> snapshots, but not from any released qemu version. The previous
> migration stream format had multiple problems, so better to fix them
> now, before 2.10 is out.
>
> Although there are certainly more things that can be improved in the
> DRC system, with this series we should have a solid foundation for
> migrating DRCs - the state trasferred is about as minimal and well
> defined as it's possible to be.
>
> Changes since v1:
> * Rebased onto current tree
> * Added cleanup to unplug path
> * Added restriction of DR-indicator to physical DRCs
> * Included revised version of Laurent's patch to correctly handle
> things "hot" plugged before incoming migration
>
> David Gibson (7):
> spapr: Remove 'awaiting_allocation' DRC flag
> spapr: Simplify unplug path
> spapr: Refactor spapr_drc_detach()
> spapr: Cleanups relating to DRC awaiting_release field
> spapr: Consolidate DRC state variables
> spapr: Remove sPAPRConfigureConnectorState sub-structure
> spapr: Implement DR-indicator for physical DRCs only
>
> Laurent Vivier (1):
> spapr: Treat devices added before inbound migration as coldplugged
>
> hw/ppc/spapr.c | 89 +++-------
> hw/ppc/spapr_drc.c | 399 ++++++++++++++++++++++++---------------------
> hw/ppc/spapr_pci.c | 17 +-
> hw/ppc/trace-events | 3 +-
> include/hw/ppc/spapr_drc.h | 74 ++++++---
> 5 files changed, 301 insertions(+), 281 deletions(-)
>
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [Qemu-devel] [Qemu-ppc] [PATCHv2 0/8] spapr: DRC cleanups (part VI)
2017-07-12 13:48 ` [Qemu-devel] [Qemu-ppc] [PATCHv2 0/8] spapr: DRC cleanups (part VI) Daniel Henrique Barboza
@ 2017-07-13 0:57 ` David Gibson
2017-07-13 10:13 ` Daniel Henrique Barboza
0 siblings, 1 reply; 30+ messages in thread
From: David Gibson @ 2017-07-13 0:57 UTC (permalink / raw)
To: Daniel Henrique Barboza
Cc: mdroth, groug, lvivier, qemu-devel, qemu-ppc, sjitindarsingh,
bharata
[-- Attachment #1: Type: text/plain, Size: 2966 bytes --]
On Wed, Jul 12, 2017 at 10:48:38AM -0300, Daniel Henrique Barboza wrote:
> The dreaded Libvirt hotplug-migrate-hotunplug scenario is working nicely.
Good to hear.
> device_add when the machine is in RUN_STATE_PRELAUNCH (-S) still doesn't
> work but it is expected - as discussed in "[RFC drcVI PATCH] spapr: reset
> DRCs
> on migration pre_load", this scenario can't be fixed solely by this DRC
> cleanup.
Hmm.. what's the exact test case you're using here? The prelaunch
case I tried _did_ work (queueing the event during prelaunch, then
completing the hotplug sequence once the guest had booted).
> Given that we'll review the DT code sometime in the future I guess we can
> postpone the fix for device_adding in pre-launch for that time.
>
>
> Tested-by: Daniel Barboza <danielhb@linux.vnet.ibm.com>
>
> On 07/12/2017 02:53 AM, David Gibson wrote:
> > This sixth set of DRC cleanup patches is a complete rework of DRC
> > state management. We stop tracking some unnecessary things, and
> > change the basic state representation to a simpler and more robust
> > model.
> >
> > Many of the patches in this set "break" migration from earlier git
> > snapshots, but not from any released qemu version. The previous
> > migration stream format had multiple problems, so better to fix them
> > now, before 2.10 is out.
> >
> > Although there are certainly more things that can be improved in the
> > DRC system, with this series we should have a solid foundation for
> > migrating DRCs - the state trasferred is about as minimal and well
> > defined as it's possible to be.
> >
> > Changes since v1:
> > * Rebased onto current tree
> > * Added cleanup to unplug path
> > * Added restriction of DR-indicator to physical DRCs
> > * Included revised version of Laurent's patch to correctly handle
> > things "hot" plugged before incoming migration
> >
> > David Gibson (7):
> > spapr: Remove 'awaiting_allocation' DRC flag
> > spapr: Simplify unplug path
> > spapr: Refactor spapr_drc_detach()
> > spapr: Cleanups relating to DRC awaiting_release field
> > spapr: Consolidate DRC state variables
> > spapr: Remove sPAPRConfigureConnectorState sub-structure
> > spapr: Implement DR-indicator for physical DRCs only
> >
> > Laurent Vivier (1):
> > spapr: Treat devices added before inbound migration as coldplugged
> >
> > hw/ppc/spapr.c | 89 +++-------
> > hw/ppc/spapr_drc.c | 399 ++++++++++++++++++++++++---------------------
> > hw/ppc/spapr_pci.c | 17 +-
> > hw/ppc/trace-events | 3 +-
> > include/hw/ppc/spapr_drc.h | 74 ++++++---
> > 5 files changed, 301 insertions(+), 281 deletions(-)
> >
>
--
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: 819 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [PATCHv2 0/8] spapr: DRC cleanups (part VI)
2017-07-13 0:57 ` David Gibson
@ 2017-07-13 10:13 ` Daniel Henrique Barboza
2017-07-14 6:53 ` David Gibson
0 siblings, 1 reply; 30+ messages in thread
From: Daniel Henrique Barboza @ 2017-07-13 10:13 UTC (permalink / raw)
To: David Gibson
Cc: mdroth, groug, lvivier, qemu-devel, qemu-ppc, sjitindarsingh,
bharata
On 07/12/2017 09:57 PM, David Gibson wrote:
> On Wed, Jul 12, 2017 at 10:48:38AM -0300, Daniel Henrique Barboza wrote:
>> The dreaded Libvirt hotplug-migrate-hotunplug scenario is working nicely.
> Good to hear.
>
>> device_add when the machine is in RUN_STATE_PRELAUNCH (-S) still doesn't
>> work but it is expected - as discussed in "[RFC drcVI PATCH] spapr: reset
>> DRCs
>> on migration pre_load", this scenario can't be fixed solely by this DRC
>> cleanup.
> Hmm.. what's the exact test case you're using here? The prelaunch
> case I tried _did_ work (queueing the event during prelaunch, then
> completing the hotplug sequence once the guest had booted).
This is the test case:
sudo ./qemu-system-ppc64 -name migrate_qemu -boot strict=on --enable-kvm
-device nec-usb-xhci,id=usb,bus=pci.0,addr=0xf -device
spapr-vscsi,id=scsi0,reg=0x2000 -smp
1,maxcpus=4,sockets=4,cores=1,threads=1 --machine
pseries,accel=kvm,usb=off,dump-guest-core=off -m 4G,slots=32,maxmem=32G
-drive
file=/home/danielhb/vm_imgs/ubuntu1704.qcow2,format=qcow2,if=none,id=drive-virtio-disk0,cache=none
-device
virtio-blk-pci,scsi=off,bus=pci.0,addr=0x2,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1
-nographic -S
QEMU 2.9.50 monitor - type 'help' for more information
(qemu)
(qemu) device_add host-spapr-cpu-core,id=core1,core-id=1
(qemu) cont
(...)
After OS boots:
danielhb@ubuntu1704:~$ lscpu
Architecture: ppc64le
Byte Order: Little Endian
CPU(s): 1
On-line CPU(s) list: 0
Thread(s) per core: 1
Core(s) per socket: 1
Socket(s): 1
NUMA node(s): 1
Model: 2.1 (pvr 004b 0201)
Model name: POWER8E (raw), altivec supported
Hypervisor vendor: horizontal
Virtualization type: full
L1d cache: 64K
L1i cache: 32K
NUMA node0 CPU(s): 0
danielhb@ubuntu1704:~$ (qemu)
(qemu) info cpus
* CPU #0: nip=0xc0000000000a3e0c thread_id=6134
CPU #1: nip=0x0000000000000000 (halted) thread_id=6163
(qemu) info hotpluggable-cpus
Hotpluggable CPUs:
type: "host-spapr-cpu-core"
vcpus_count: "1"
CPUInstance Properties:
core-id: "3"
type: "host-spapr-cpu-core"
vcpus_count: "1"
CPUInstance Properties:
core-id: "2"
type: "host-spapr-cpu-core"
vcpus_count: "1"
qom_path: "/machine/peripheral/core1"
CPUInstance Properties:
core-id: "1"
type: "host-spapr-cpu-core"
vcpus_count: "1"
qom_path: "/machine/unattached/device[0]"
CPUInstance Properties:
core-id: "0"
(qemu)
I am not aware of anyone (e.g. Libvirt) using device_add at that point
so it's not
urgent to make this work AFAIC. I was just curious of why it doesn't.
Daniel
>
>> Given that we'll review the DT code sometime in the future I guess we can
>> postpone the fix for device_adding in pre-launch for that time.
>>
>>
>> Tested-by: Daniel Barboza <danielhb@linux.vnet.ibm.com>
>>
>> On 07/12/2017 02:53 AM, David Gibson wrote:
>>> This sixth set of DRC cleanup patches is a complete rework of DRC
>>> state management. We stop tracking some unnecessary things, and
>>> change the basic state representation to a simpler and more robust
>>> model.
>>>
>>> Many of the patches in this set "break" migration from earlier git
>>> snapshots, but not from any released qemu version. The previous
>>> migration stream format had multiple problems, so better to fix them
>>> now, before 2.10 is out.
>>>
>>> Although there are certainly more things that can be improved in the
>>> DRC system, with this series we should have a solid foundation for
>>> migrating DRCs - the state trasferred is about as minimal and well
>>> defined as it's possible to be.
>>>
>>> Changes since v1:
>>> * Rebased onto current tree
>>> * Added cleanup to unplug path
>>> * Added restriction of DR-indicator to physical DRCs
>>> * Included revised version of Laurent's patch to correctly handle
>>> things "hot" plugged before incoming migration
>>>
>>> David Gibson (7):
>>> spapr: Remove 'awaiting_allocation' DRC flag
>>> spapr: Simplify unplug path
>>> spapr: Refactor spapr_drc_detach()
>>> spapr: Cleanups relating to DRC awaiting_release field
>>> spapr: Consolidate DRC state variables
>>> spapr: Remove sPAPRConfigureConnectorState sub-structure
>>> spapr: Implement DR-indicator for physical DRCs only
>>>
>>> Laurent Vivier (1):
>>> spapr: Treat devices added before inbound migration as coldplugged
>>>
>>> hw/ppc/spapr.c | 89 +++-------
>>> hw/ppc/spapr_drc.c | 399 ++++++++++++++++++++++++---------------------
>>> hw/ppc/spapr_pci.c | 17 +-
>>> hw/ppc/trace-events | 3 +-
>>> include/hw/ppc/spapr_drc.h | 74 ++++++---
>>> 5 files changed, 301 insertions(+), 281 deletions(-)
>>>
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [Qemu-devel] [Qemu-ppc] [PATCHv2 0/8] spapr: DRC cleanups (part VI)
2017-07-13 10:13 ` Daniel Henrique Barboza
@ 2017-07-14 6:53 ` David Gibson
2017-07-14 13:50 ` Daniel Henrique Barboza
0 siblings, 1 reply; 30+ messages in thread
From: David Gibson @ 2017-07-14 6:53 UTC (permalink / raw)
To: Daniel Henrique Barboza
Cc: mdroth, groug, lvivier, qemu-devel, qemu-ppc, sjitindarsingh,
bharata
[-- Attachment #1: Type: text/plain, Size: 3657 bytes --]
On Thu, Jul 13, 2017 at 07:13:23AM -0300, Daniel Henrique Barboza wrote:
>
>
> On 07/12/2017 09:57 PM, David Gibson wrote:
> > On Wed, Jul 12, 2017 at 10:48:38AM -0300, Daniel Henrique Barboza wrote:
> > > The dreaded Libvirt hotplug-migrate-hotunplug scenario is working nicely.
> > Good to hear.
> >
> > > device_add when the machine is in RUN_STATE_PRELAUNCH (-S) still doesn't
> > > work but it is expected - as discussed in "[RFC drcVI PATCH] spapr: reset
> > > DRCs
> > > on migration pre_load", this scenario can't be fixed solely by this DRC
> > > cleanup.
> > Hmm.. what's the exact test case you're using here? The prelaunch
> > case I tried _did_ work (queueing the event during prelaunch, then
> > completing the hotplug sequence once the guest had booted).
>
> This is the test case:
>
> sudo ./qemu-system-ppc64 -name migrate_qemu -boot strict=on --enable-kvm
> -device nec-usb-xhci,id=usb,bus=pci.0,addr=0xf -device
> spapr-vscsi,id=scsi0,reg=0x2000 -smp 1,maxcpus=4,sockets=4,cores=1,threads=1
> --machine pseries,accel=kvm,usb=off,dump-guest-core=off -m
> 4G,slots=32,maxmem=32G -drive file=/home/danielhb/vm_imgs/ubuntu1704.qcow2,format=qcow2,if=none,id=drive-virtio-disk0,cache=none
> -device virtio-blk-pci,scsi=off,bus=pci.0,addr=0x2,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1
> -nographic -S
> QEMU 2.9.50 monitor - type 'help' for more information
> (qemu)
> (qemu) device_add host-spapr-cpu-core,id=core1,core-id=1
> (qemu) cont
>
> (...)
>
> After OS boots:
>
> danielhb@ubuntu1704:~$ lscpu
> Architecture: ppc64le
> Byte Order: Little Endian
> CPU(s): 1
> On-line CPU(s) list: 0
> Thread(s) per core: 1
> Core(s) per socket: 1
> Socket(s): 1
> NUMA node(s): 1
> Model: 2.1 (pvr 004b 0201)
> Model name: POWER8E (raw), altivec supported
> Hypervisor vendor: horizontal
> Virtualization type: full
> L1d cache: 64K
> L1i cache: 32K
> NUMA node0 CPU(s): 0
> danielhb@ubuntu1704:~$ (qemu)
> (qemu) info cpus
> * CPU #0: nip=0xc0000000000a3e0c thread_id=6134
> CPU #1: nip=0x0000000000000000 (halted) thread_id=6163
> (qemu) info hotpluggable-cpus
> Hotpluggable CPUs:
> type: "host-spapr-cpu-core"
> vcpus_count: "1"
> CPUInstance Properties:
> core-id: "3"
> type: "host-spapr-cpu-core"
> vcpus_count: "1"
> CPUInstance Properties:
> core-id: "2"
> type: "host-spapr-cpu-core"
> vcpus_count: "1"
> qom_path: "/machine/peripheral/core1"
> CPUInstance Properties:
> core-id: "1"
> type: "host-spapr-cpu-core"
> vcpus_count: "1"
> qom_path: "/machine/unattached/device[0]"
> CPUInstance Properties:
> core-id: "0"
> (qemu)
Huh. I tried basically the same thing, and I get the second cpu once
the OS is booted. My first guess would be that the difference is in
the guest (mine is RHEL 7.3). Have you double checked that rtas_errd
and drmgr are present in your guest?
> I am not aware of anyone (e.g. Libvirt) using device_add at that point so
> it's not
> urgent to make this work AFAIC. I was just curious of why it
> doesn't.
Hm, I thought libvirt did use device_add here in at least some
circumstances.
I've gone ahead and sent a pull req with the changes, since AFAICT his
is not a regression, so the DRC cleanups still improve matters
overall.
--
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: [Qemu-devel] [Qemu-ppc] [PATCHv2 0/8] spapr: DRC cleanups (part VI)
2017-07-14 6:53 ` David Gibson
@ 2017-07-14 13:50 ` Daniel Henrique Barboza
2017-07-15 2:42 ` David Gibson
0 siblings, 1 reply; 30+ messages in thread
From: Daniel Henrique Barboza @ 2017-07-14 13:50 UTC (permalink / raw)
To: David Gibson
Cc: lvivier, qemu-devel, mdroth, groug, qemu-ppc, sjitindarsingh,
bharata
On 07/14/2017 03:53 AM, David Gibson wrote:
> On Thu, Jul 13, 2017 at 07:13:23AM -0300, Daniel Henrique Barboza wrote:
>>
>> On 07/12/2017 09:57 PM, David Gibson wrote:
>>> On Wed, Jul 12, 2017 at 10:48:38AM -0300, Daniel Henrique Barboza wrote:
>>>> The dreaded Libvirt hotplug-migrate-hotunplug scenario is working nicely.
>>> Good to hear.
>>>
>>>> device_add when the machine is in RUN_STATE_PRELAUNCH (-S) still doesn't
>>>> work but it is expected - as discussed in "[RFC drcVI PATCH] spapr: reset
>>>> DRCs
>>>> on migration pre_load", this scenario can't be fixed solely by this DRC
>>>> cleanup.
>>> Hmm.. what's the exact test case you're using here? The prelaunch
>>> case I tried _did_ work (queueing the event during prelaunch, then
>>> completing the hotplug sequence once the guest had booted).
>> This is the test case:
>>
>> sudo ./qemu-system-ppc64 -name migrate_qemu -boot strict=on --enable-kvm
>> -device nec-usb-xhci,id=usb,bus=pci.0,addr=0xf -device
>> spapr-vscsi,id=scsi0,reg=0x2000 -smp 1,maxcpus=4,sockets=4,cores=1,threads=1
>> --machine pseries,accel=kvm,usb=off,dump-guest-core=off -m
>> 4G,slots=32,maxmem=32G -drive file=/home/danielhb/vm_imgs/ubuntu1704.qcow2,format=qcow2,if=none,id=drive-virtio-disk0,cache=none
>> -device virtio-blk-pci,scsi=off,bus=pci.0,addr=0x2,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1
>> -nographic -S
>> QEMU 2.9.50 monitor - type 'help' for more information
>> (qemu)
>> (qemu) device_add host-spapr-cpu-core,id=core1,core-id=1
>> (qemu) cont
>>
>> (...)
>>
>> After OS boots:
>>
>> danielhb@ubuntu1704:~$ lscpu
>> Architecture: ppc64le
>> Byte Order: Little Endian
>> CPU(s): 1
>> On-line CPU(s) list: 0
>> Thread(s) per core: 1
>> Core(s) per socket: 1
>> Socket(s): 1
>> NUMA node(s): 1
>> Model: 2.1 (pvr 004b 0201)
>> Model name: POWER8E (raw), altivec supported
>> Hypervisor vendor: horizontal
>> Virtualization type: full
>> L1d cache: 64K
>> L1i cache: 32K
>> NUMA node0 CPU(s): 0
>> danielhb@ubuntu1704:~$ (qemu)
>> (qemu) info cpus
>> * CPU #0: nip=0xc0000000000a3e0c thread_id=6134
>> CPU #1: nip=0x0000000000000000 (halted) thread_id=6163
>> (qemu) info hotpluggable-cpus
>> Hotpluggable CPUs:
>> type: "host-spapr-cpu-core"
>> vcpus_count: "1"
>> CPUInstance Properties:
>> core-id: "3"
>> type: "host-spapr-cpu-core"
>> vcpus_count: "1"
>> CPUInstance Properties:
>> core-id: "2"
>> type: "host-spapr-cpu-core"
>> vcpus_count: "1"
>> qom_path: "/machine/peripheral/core1"
>> CPUInstance Properties:
>> core-id: "1"
>> type: "host-spapr-cpu-core"
>> vcpus_count: "1"
>> qom_path: "/machine/unattached/device[0]"
>> CPUInstance Properties:
>> core-id: "0"
>> (qemu)
> Huh. I tried basically the same thing, and I get the second cpu once
> the OS is booted. My first guess would be that the difference is in
> the guest (mine is RHEL 7.3). Have you double checked that rtas_errd
> and drmgr are present in your guest?
Yeah the guest has drmgr and rtas_errd running. As you said, there might be
something different in the guests that explains why yours work and mine
doesn't.
Coupling that with the fact that this is not a common usage, I believe we
can leave it as a FYI/reminder if we need to revisit this issue in the
future.
Daniel
>
>> I am not aware of anyone (e.g. Libvirt) using device_add at that point so
>> it's not
>> urgent to make this work AFAIC. I was just curious of why it
>> doesn't.
> Hm, I thought libvirt did use device_add here in at least some
> circumstances.
>
> I've gone ahead and sent a pull req with the changes, since AFAICT his
> is not a regression, so the DRC cleanups still improve matters
> overall.
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [PATCHv2 0/8] spapr: DRC cleanups (part VI)
2017-07-14 13:50 ` Daniel Henrique Barboza
@ 2017-07-15 2:42 ` David Gibson
0 siblings, 0 replies; 30+ messages in thread
From: David Gibson @ 2017-07-15 2:42 UTC (permalink / raw)
To: Daniel Henrique Barboza
Cc: lvivier, qemu-devel, mdroth, groug, qemu-ppc, sjitindarsingh,
bharata
[-- Attachment #1: Type: text/plain, Size: 4055 bytes --]
On Fri, Jul 14, 2017 at 10:50:06AM -0300, Daniel Henrique Barboza wrote:
>
>
> On 07/14/2017 03:53 AM, David Gibson wrote:
> > On Thu, Jul 13, 2017 at 07:13:23AM -0300, Daniel Henrique Barboza wrote:
> > >
> > > On 07/12/2017 09:57 PM, David Gibson wrote:
> > > > On Wed, Jul 12, 2017 at 10:48:38AM -0300, Daniel Henrique Barboza wrote:
> > > > > The dreaded Libvirt hotplug-migrate-hotunplug scenario is working nicely.
> > > > Good to hear.
> > > >
> > > > > device_add when the machine is in RUN_STATE_PRELAUNCH (-S) still doesn't
> > > > > work but it is expected - as discussed in "[RFC drcVI PATCH] spapr: reset
> > > > > DRCs
> > > > > on migration pre_load", this scenario can't be fixed solely by this DRC
> > > > > cleanup.
> > > > Hmm.. what's the exact test case you're using here? The prelaunch
> > > > case I tried _did_ work (queueing the event during prelaunch, then
> > > > completing the hotplug sequence once the guest had booted).
> > > This is the test case:
> > >
> > > sudo ./qemu-system-ppc64 -name migrate_qemu -boot strict=on --enable-kvm
> > > -device nec-usb-xhci,id=usb,bus=pci.0,addr=0xf -device
> > > spapr-vscsi,id=scsi0,reg=0x2000 -smp 1,maxcpus=4,sockets=4,cores=1,threads=1
> > > --machine pseries,accel=kvm,usb=off,dump-guest-core=off -m
> > > 4G,slots=32,maxmem=32G -drive file=/home/danielhb/vm_imgs/ubuntu1704.qcow2,format=qcow2,if=none,id=drive-virtio-disk0,cache=none
> > > -device virtio-blk-pci,scsi=off,bus=pci.0,addr=0x2,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1
> > > -nographic -S
> > > QEMU 2.9.50 monitor - type 'help' for more information
> > > (qemu)
> > > (qemu) device_add host-spapr-cpu-core,id=core1,core-id=1
> > > (qemu) cont
> > >
> > > (...)
> > >
> > > After OS boots:
> > >
> > > danielhb@ubuntu1704:~$ lscpu
> > > Architecture: ppc64le
> > > Byte Order: Little Endian
> > > CPU(s): 1
> > > On-line CPU(s) list: 0
> > > Thread(s) per core: 1
> > > Core(s) per socket: 1
> > > Socket(s): 1
> > > NUMA node(s): 1
> > > Model: 2.1 (pvr 004b 0201)
> > > Model name: POWER8E (raw), altivec supported
> > > Hypervisor vendor: horizontal
> > > Virtualization type: full
> > > L1d cache: 64K
> > > L1i cache: 32K
> > > NUMA node0 CPU(s): 0
> > > danielhb@ubuntu1704:~$ (qemu)
> > > (qemu) info cpus
> > > * CPU #0: nip=0xc0000000000a3e0c thread_id=6134
> > > CPU #1: nip=0x0000000000000000 (halted) thread_id=6163
> > > (qemu) info hotpluggable-cpus
> > > Hotpluggable CPUs:
> > > type: "host-spapr-cpu-core"
> > > vcpus_count: "1"
> > > CPUInstance Properties:
> > > core-id: "3"
> > > type: "host-spapr-cpu-core"
> > > vcpus_count: "1"
> > > CPUInstance Properties:
> > > core-id: "2"
> > > type: "host-spapr-cpu-core"
> > > vcpus_count: "1"
> > > qom_path: "/machine/peripheral/core1"
> > > CPUInstance Properties:
> > > core-id: "1"
> > > type: "host-spapr-cpu-core"
> > > vcpus_count: "1"
> > > qom_path: "/machine/unattached/device[0]"
> > > CPUInstance Properties:
> > > core-id: "0"
> > > (qemu)
> > Huh. I tried basically the same thing, and I get the second cpu once
> > the OS is booted. My first guess would be that the difference is in
> > the guest (mine is RHEL 7.3). Have you double checked that rtas_errd
> > and drmgr are present in your guest?
>
> Yeah the guest has drmgr and rtas_errd running. As you said, there might be
> something different in the guests that explains why yours work and mine
> doesn't.
> Coupling that with the fact that this is not a common usage, I believe we
> can leave it as a FYI/reminder if we need to revisit this issue in the
> future.
I concur.
--
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: [Qemu-devel] [PATCHv2 0/8] spapr: DRC cleanups (part VI)
2017-07-12 5:53 [Qemu-devel] [PATCHv2 0/8] spapr: DRC cleanups (part VI) David Gibson
` (8 preceding siblings ...)
2017-07-12 13:48 ` [Qemu-devel] [Qemu-ppc] [PATCHv2 0/8] spapr: DRC cleanups (part VI) Daniel Henrique Barboza
@ 2017-07-13 1:09 ` David Gibson
9 siblings, 0 replies; 30+ messages in thread
From: David Gibson @ 2017-07-13 1:09 UTC (permalink / raw)
To: mdroth, groug, danielhb
Cc: qemu-ppc, qemu-devel, lvivier, sjitindarsingh, bharata
[-- Attachment #1: Type: text/plain, Size: 2195 bytes --]
On Wed, Jul 12, 2017 at 03:53:09PM +1000, David Gibson wrote:
> This sixth set of DRC cleanup patches is a complete rework of DRC
> state management. We stop tracking some unnecessary things, and
> change the basic state representation to a simpler and more robust
> model.
>
> Many of the patches in this set "break" migration from earlier git
> snapshots, but not from any released qemu version. The previous
> migration stream format had multiple problems, so better to fix them
> now, before 2.10 is out.
>
> Although there are certainly more things that can be improved in the
> DRC system, with this series we should have a solid foundation for
> migrating DRCs - the state trasferred is about as minimal and well
> defined as it's possible to be.
Thanks for the reviews. I've made a few tweaks as suggested. I'm now
sufficiently confident of the series, that I've merged it into
ppc-for-2.10.
>
> Changes since v1:
> * Rebased onto current tree
> * Added cleanup to unplug path
> * Added restriction of DR-indicator to physical DRCs
> * Included revised version of Laurent's patch to correctly handle
> things "hot" plugged before incoming migration
>
> David Gibson (7):
> spapr: Remove 'awaiting_allocation' DRC flag
> spapr: Simplify unplug path
> spapr: Refactor spapr_drc_detach()
> spapr: Cleanups relating to DRC awaiting_release field
> spapr: Consolidate DRC state variables
> spapr: Remove sPAPRConfigureConnectorState sub-structure
> spapr: Implement DR-indicator for physical DRCs only
>
> Laurent Vivier (1):
> spapr: Treat devices added before inbound migration as coldplugged
>
> hw/ppc/spapr.c | 89 +++-------
> hw/ppc/spapr_drc.c | 399 ++++++++++++++++++++++++---------------------
> hw/ppc/spapr_pci.c | 17 +-
> hw/ppc/trace-events | 3 +-
> include/hw/ppc/spapr_drc.h | 74 ++++++---
> 5 files changed, 301 insertions(+), 281 deletions(-)
>
--
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: 819 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread