[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-ppc] [PATCH v9 3/6] hw/ppc: migrating the DRC state of hotplug
From: |
David Gibson |
Subject: |
Re: [Qemu-ppc] [PATCH v9 3/6] hw/ppc: migrating the DRC state of hotplugged devices |
Date: |
Fri, 12 May 2017 16:11:28 +1000 |
User-agent: |
Mutt/1.8.0 (2017-02-23) |
On Fri, May 05, 2017 at 05:47:43PM -0300, Daniel Henrique Barboza wrote:
> In pseries, a firmware abstraction called Dynamic Reconfiguration
> Connector (DRC) is used to assign a particular dynamic resource
> to the guest and provide an interface to manage configuration/removal
> of the resource associated with it. In other words, DRC is the
> 'plugged state' of a device.
>
> Before this patch, DRC wasn't being migrated. This causes
> post-migration problems due to DRC state mismatch between source and
> target. The DRC state of a device X in the source might
> change, while in the target the DRC state of X is still fresh. When
> migrating the guest, X will not have the same hotplugged state as it
> did in the source. This means that we can't hot unplug X in the
> target after migration is completed because its DRC state is not consistent.
> https://bugs.launchpad.net/ubuntu/+source/qemu/+bug/1677552 is one
> bug that is caused by this DRC state mismatch between source and
> target.
>
> To migrate the DRC state, we defined the VMStateDescription struct for
> spapr_drc to enable the transmission of spapr_drc state in migration.
> Not all the elements in the DRC state are migrated - only those
> that can be modified by guest actions or device add/remove
> operations:
>
> - 'isolation_state', 'allocation_state' and 'indicator_state'
> are involved in the DR state transition diagram from
> PAPR+ 2.7, 13.4;
>
> - 'configured', 'signalled', 'awaiting_release' and 'awaiting_allocation'
> are needed in attaching and detaching devices;
>
> - 'indicator_state' provides users with hardware state information.
>
> These are the DRC elements that are migrated.
>
> In this patch the DRC state is migrated for PCI, LMB and CPU
> connector types. At this moment there is no support to migrate
> DRC for the PHB (PCI Host Bridge) type.
>
> In the 'realize' function the DRC is registered using vmstate_register,
> similar to what hw/ppc/spapr_iommu.c does in 'spapr_tce_table_realize'.
> This approach works because DRCs are bus-less and do not sit
> on a BusClass that implements bc->get_dev_path, so as a fallback the
> VMSD gets identified via "spapr_drc"/get_index(drc).
>
> Signed-off-by: Daniel Henrique Barboza <address@hidden>
> ---
> hw/ppc/spapr_drc.c | 61
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 61 insertions(+)
>
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 1c72160..926b945 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -519,6 +519,65 @@ static void reset(DeviceState *d)
> }
> }
>
> +static bool spapr_drc_needed(void *opaque)
> +{
> + sPAPRDRConnector *drc = (sPAPRDRConnector *)opaque;
> + sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> + bool rc = false;
> + sPAPRDREntitySense value;
Blank line after the declarations, please.
> + drck->entity_sense(drc, &value);
> + /* If no dev is plugged in there is no need to migrate the DRC state */
> + if (value != SPAPR_DR_ENTITY_SENSE_PRESENT) {
> + return false;
> + }
> +
> + /*
> + * If there is dev plugged in, we need to migrate the DRC state when
> + * it is different from cold-plugged state
> + */
> + switch (drc->type) {
> +
No blank line here please.
> + case SPAPR_DR_CONNECTOR_TYPE_PCI:
> + rc = !((drc->isolation_state == SPAPR_DR_ISOLATION_STATE_UNISOLATED)
> &&
> + (drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_USABLE) &&
> + drc->configured && drc->signalled && !drc->awaiting_release);
You don't do any more manipulation of the rc value, so you might as
well just 'return' directly here.
> + break;
> +
> + case SPAPR_DR_CONNECTOR_TYPE_LMB:
> + rc = !((drc->isolation_state == SPAPR_DR_ISOLATION_STATE_ISOLATED) &&
> + (drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE)
> &&
> + drc->configured && drc->signalled && !drc->awaiting_release);
> + break;
> +
> + case SPAPR_DR_CONNECTOR_TYPE_CPU:
> + rc = !((drc->isolation_state == SPAPR_DR_ISOLATION_STATE_ISOLATED) &&
> + (drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE)
> &&
> + drc->configured && drc->signalled && !drc->awaiting_release);
> + break;
> +
> + default:
> + ;
This should probably assert().
> + }
> + return rc;
> +}
> +
> +static const VMStateDescription vmstate_spapr_drc = {
> + .name = "spapr_drc",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .needed = spapr_drc_needed,
> + .fields = (VMStateField []) {
> + VMSTATE_UINT32(isolation_state, sPAPRDRConnector),
> + VMSTATE_UINT32(allocation_state, sPAPRDRConnector),
> + VMSTATE_UINT32(indicator_state, sPAPRDRConnector),
> + VMSTATE_BOOL(configured, sPAPRDRConnector),
> + VMSTATE_BOOL(awaiting_release, sPAPRDRConnector),
> + VMSTATE_BOOL(awaiting_allocation, sPAPRDRConnector),
> + VMSTATE_BOOL(signalled, sPAPRDRConnector),
Hrm. I'm happy translation the 3 state integers, since their meaning
is in the PAPR spec. The others are qemu local information, so it's
not as clear that they'll have a stable meaning. Are you absolutely
sure you need all of them?
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> static void realize(DeviceState *d, Error **errp)
> {
> sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(d);
> @@ -547,6 +606,8 @@ static void realize(DeviceState *d, Error **errp)
> object_unref(OBJECT(drc));
> }
> g_free(child_name);
> + vmstate_register(DEVICE(drc), drck->get_index(drc), &vmstate_spapr_drc,
> + drc);
> trace_spapr_drc_realize_complete(drck->get_index(drc));
> }
>
--
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
signature.asc
Description: PGP signature