On Thu, Jun 08, 2017 at 03:09:28PM +1000, David Gibson wrote:
> The reset handler for DRCs attempts several state transitions which are
> subject to various checks and restrictions. But at reset time we know
> there is no guest, so we can ignore most of the usual sequencing rules and
> just set the DRC back to a known state. In fact, it's safer to do so.
>
> The existing code also has several redundant checks for
> drc->awaiting_release inside a block which has already tested that. This
> patch removes those and sets the DRC to a fixed initial state based only
> on whether a device is currently plugged or not.
>
> With DRCs correctly reset to a state based on device presence, we don't
> need to force state transitions as cold plugged devices are processed.
> This allows us to remove all the callers of the set_*_state() methods from
> outside spapr_drc.c.
>
> Signed-off-by: David Gibson <da...@gibson.dropbear.id.au>
> ---
> hw/ppc/spapr.c | 15 ---------------
> hw/ppc/spapr_drc.c | 28 ++++++++--------------------
> 2 files changed, 8 insertions(+), 35 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 01dda9e..c988e38 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2545,12 +2545,6 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t
> addr_start, uint64_t size,
>
> spapr_drc_attach(drc, dev, fdt, fdt_offset, !dev->hotplugged, errp);
> addr += SPAPR_MEMORY_BLOCK_SIZE;
> - if (!dev->hotplugged) {
> - sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> - /* guests expect coldplugged LMBs to be pre-allocated */
> - drck->set_allocation_state(drc,
> SPAPR_DR_ALLOCATION_STATE_USABLE);
> - drck->set_isolation_state(drc,
> SPAPR_DR_ISOLATION_STATE_UNISOLATED);
> - }
> }
> /* send hotplug notification to the
> * guest only in case of hotplugged memory
> @@ -2901,15 +2895,6 @@ static void spapr_core_plug(HotplugHandler
> *hotplug_dev, DeviceState *dev,
> * of hotplugged CPUs.
> */
> spapr_hotplug_req_add_by_index(drc);
> - } else {
> - /*
> - * Set the right DRC states for cold plugged CPU.
> - */
> - if (drc) {
> - sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> - drck->set_allocation_state(drc,
> SPAPR_DR_ALLOCATION_STATE_USABLE);
> - drck->set_isolation_state(drc,
> SPAPR_DR_ISOLATION_STATE_UNISOLATED);
So here you are removing the initial state setting for cold plugged CPUs and ...
> - }
> }
> core_slot->cpu = OBJECT(dev);
> }
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index dc4ac77..7e17f34 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -393,7 +393,6 @@ static bool release_pending(sPAPRDRConnector *drc)
> static void reset(DeviceState *d)
> {
> sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(d);
> - sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
>
> trace_spapr_drc_reset(spapr_drc_index(drc));
>
> @@ -401,28 +400,17 @@ static void reset(DeviceState *d)
> drc->ccs = NULL;
>
> /* immediately upon reset we can safely assume DRCs whose devices
> - * are pending removal can be safely removed, and that they will
> - * subsequently be left in an ISOLATED state. move the DRC to this
> - * state in these cases (which will in turn complete any pending
> - * device removals)
> + * are pending removal can be safely removed.
> */
> if (drc->awaiting_release) {
> - drck->set_isolation_state(drc, SPAPR_DR_ISOLATION_STATE_ISOLATED);
> - /* generally this should also finalize the removal, but if the device
> - * hasn't yet been configured we normally defer removal under the
> - * assumption that this transition is taking place as part of device
> - * configuration. so check if we're still waiting after this, and
> - * force removal if we are
> - */
> - if (drc->awaiting_release) {
> - spapr_drc_detach(drc, DEVICE(drc->dev), NULL);
> - }
> + spapr_drc_release(drc);
> + }
>
> - /* non-PCI devices may be awaiting a transition to UNUSABLE */
> - if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI &&
> - drc->awaiting_release) {
> - drck->set_allocation_state(drc,
> SPAPR_DR_ALLOCATION_STATE_UNUSABLE);
> - }
> + drc->isolation_state = drc->dev ? SPAPR_DR_ISOLATION_STATE_UNISOLATED
> + : SPAPR_DR_ISOLATION_STATE_ISOLATED;
> + if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
> + drc->allocation_state = drc->dev ? SPAPR_DR_ALLOCATION_STATE_USABLE
> + : SPAPR_DR_ALLOCATION_STATE_UNUSABLE;
> }
... adding it via reset. However you are setting drc->isolation_state and
drc->allocation_state directly rather than going via
drck->set_isolation_state() and drck->set_allocation_state() routines.
This results in awaiting_allocation not geting cleared for cold plugged
CPUs. So the effect after this commit is that we can't remove the
CPUs that are specified on cmdline using -device.
The following fixes the issue for me:
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index fd9e07d..da6979a 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -450,13 +450,13 @@ static void reset(DeviceState *d)
/* 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_set_usable(drc);
}
} 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_set_unusable(drc);
}
}
}
However I thought this will restore the previous behaviour completely:
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index fd9e07d..b7726ef 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -433,6 +433,7 @@ static bool release_pending(sPAPRDRConnector *drc)
static void reset(DeviceState *d)
{
sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(d);
+ sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
trace_spapr_drc_reset(spapr_drc_index(drc));
@@ -448,15 +449,15 @@ static void reset(DeviceState *d)
if (drc->dev) {
/* A device present at reset is coldplugged */
- drc->isolation_state = SPAPR_DR_ISOLATION_STATE_UNISOLATED;
+ drck->unisolate(drc);
if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
- drc->allocation_state = SPAPR_DR_ALLOCATION_STATE_USABLE;
+ drc_set_usable(drc);
}
} else {
/* Otherwise device is absent, but might be hotplugged */
- drc->isolation_state = SPAPR_DR_ISOLATION_STATE_ISOLATED;
+ drck->isolate(drc);
if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
- drc->allocation_state = SPAPR_DR_ALLOCATION_STATE_UNUSABLE;
+ drc_set_unusable(drc);
}
}
}
Regards,
Bharata.
