On Thu, 8 Jun 2017 15:09:26 +1000 David Gibson <da...@gibson.dropbear.id.au> wrote:
> The 'signalled' field in the DRC appears to be entirely a torturous
> workaround for the fact that PCI devices were started in UNISOLATED state
> for unclear reasons.
>
> 1) 'signalled' is already meaningless for logical (so far, all non PCI)
> DRCs. It's always set to true (at least at any point it might be tested),
> and can't be assigned any real meaning due to the way signalling works for
> logical DRCs.
>
> 2) For PCI DRCs, the only time signalled would be false is when non-zero
> functions of a multifunction device are hotplugged, followed by function
> zero (the other way around is explicitly not permitted). In that case the
> secondary function DRCs are attached, but the notification isn't sent to
> the guest until function 0 is plugged.
>
> 3) signalled being false is used to allow a DRC detach to switch mode
> back to ISOLATED state, which allows a secondary function to be hotplugged
> then unplugged with function 0 never inserted. Without this a secondary
> function starting in UNISOLATED state couldn't be detached again without
> function 0 being inserted, all the functions configured by the guest, then
> sent back to ISOLATED state.
>
> 4) But now that PCI DRCs start in ISOLATED state, there's nothing to be
> done. If the guest doesn't get the notification, it won't switch the
> device to UNISOLATED state, so nothing prevents it from being unplugged.
> If the guest does move it to UNISOLATED state without the signal (due to
> a manual drmgr call, for instance) then it really isn't safe to unplug it.
>
>
> So, this patch removes the signalled variable and all code related to it.
>
> Signed-off-by: David Gibson <da...@gibson.dropbear.id.au>
> ---
Reviewed-by: Greg Kurz <gr...@kaod.org>
> hw/ppc/spapr_drc.c | 45 +--------------------------------------------
> hw/ppc/spapr_events.c | 10 ----------
> include/hw/ppc/spapr_drc.h | 2 --
> 3 files changed, 1 insertion(+), 56 deletions(-)
>
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 6186f79..afd68f4 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -183,12 +183,6 @@ static const char *spapr_drc_name(sPAPRDRConnector *drc)
> return g_strdup_printf("%s%d", drck->drc_name_prefix, drc->id);
> }
>
> -/* has the guest been notified of device attachment? */
> -static void set_signalled(sPAPRDRConnector *drc)
> -{
> - drc->signalled = true;
> -}
> -
> /*
> * dr-entity-sense sensor value
> * returned via get-sensor-state RTAS calls
> @@ -321,17 +315,6 @@ void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState
> *d, void *fdt,
> drc->fdt = fdt;
> drc->fdt_start_offset = fdt_start_offset;
> drc->configured = coldplug;
> - /* 'logical' DR resources such as memory/cpus are in some cases treated
> - * as a pool of resources from which the guest is free to choose from
> - * based on only a count. for resources that can be assigned in this
> - * fashion, we must assume the resource is signalled immediately
> - * since a single hotplug request might make an arbitrary number of
> - * such attached resources available to the guest, as opposed to
> - * 'physical' DR resources such as PCI where each device/resource is
> - * signalled individually.
> - */
> - drc->signalled = (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI)
> - ? true : coldplug;
>
> if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
> drc->awaiting_allocation = true;
> @@ -347,26 +330,6 @@ void spapr_drc_detach(sPAPRDRConnector *drc, DeviceState
> *d, Error **errp)
> {
> trace_spapr_drc_detach(spapr_drc_index(drc));
>
> - /* if we've signalled device presence to the guest, or if the guest
> - * has gone ahead and configured the device (via manually-executed
> - * device add via drmgr in guest, namely), we need to wait
> - * for the guest to quiesce the device before completing detach.
> - * Otherwise, we can assume the guest hasn't seen it and complete the
> - * detach immediately. Note that there is a small race window
> - * just before, or during, configuration, which is this context
> - * refers mainly to fetching the device tree via RTAS.
> - * During this window the device access will be arbitrated by
> - * associated DRC, which will simply fail the RTAS calls as invalid.
> - * This is recoverable within guest and current implementations of
> - * drmgr should be able to cope.
> - */
> - if (!drc->signalled && !drc->configured) {
> - /* if the guest hasn't seen the device we can't rely on it to
> - * set it back to an isolated state via RTAS, so do it here manually
> - */
> - drc->isolation_state = SPAPR_DR_ISOLATION_STATE_ISOLATED;
> - }
> -
> if (drc->isolation_state != SPAPR_DR_ISOLATION_STATE_ISOLATED) {
> trace_spapr_drc_awaiting_isolated(spapr_drc_index(drc));
> drc->awaiting_release = true;
> @@ -455,10 +418,6 @@ static void reset(DeviceState *d)
> drck->set_allocation_state(drc,
> SPAPR_DR_ALLOCATION_STATE_UNUSABLE);
> }
> }
> -
> - if (drck->dr_entity_sense(drc) == SPAPR_DR_ENTITY_SENSE_PRESENT) {
> - drck->set_signalled(drc);
> - }
> }
>
> static bool spapr_drc_needed(void *opaque)
> @@ -483,7 +442,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->signalled && !drc->awaiting_release);
> + drc->configured && !drc->awaiting_release);
> break;
> case SPAPR_DR_CONNECTOR_TYPE_PHB:
> case SPAPR_DR_CONNECTOR_TYPE_VIO:
> @@ -505,7 +464,6 @@ static const VMStateDescription vmstate_spapr_drc = {
> VMSTATE_BOOL(configured, sPAPRDRConnector),
> VMSTATE_BOOL(awaiting_release, sPAPRDRConnector),
> VMSTATE_BOOL(awaiting_allocation, sPAPRDRConnector),
> - VMSTATE_BOOL(signalled, sPAPRDRConnector),
> VMSTATE_END_OF_LIST()
> }
> };
> @@ -603,7 +561,6 @@ static void spapr_dr_connector_class_init(ObjectClass *k,
> void *data)
> drck->set_isolation_state = set_isolation_state;
> drck->set_allocation_state = set_allocation_state;
> drck->release_pending = release_pending;
> - drck->set_signalled = set_signalled;
> /*
> * Reason: it crashes FIXME find and document the real reason
> */
> diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
> index 171aedc..587a3da 100644
> --- a/hw/ppc/spapr_events.c
> +++ b/hw/ppc/spapr_events.c
> @@ -475,13 +475,6 @@ static void spapr_powerdown_req(Notifier *n, void
> *opaque)
> RTAS_LOG_TYPE_EPOW)));
> }
>
> -static void spapr_hotplug_set_signalled(uint32_t drc_index)
> -{
> - sPAPRDRConnector *drc = spapr_drc_by_index(drc_index);
> - sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> - drck->set_signalled(drc);
> -}
> -
> static void spapr_hotplug_req_event(uint8_t hp_id, uint8_t hp_action,
> sPAPRDRConnectorType drc_type,
> union drc_identifier *drc_id)
> @@ -528,9 +521,6 @@ static void spapr_hotplug_req_event(uint8_t hp_id,
> uint8_t hp_action,
> switch (drc_type) {
> case SPAPR_DR_CONNECTOR_TYPE_PCI:
> hp->hotplug_type = RTAS_LOG_V6_HP_TYPE_PCI;
> - if (hp->hotplug_action == RTAS_LOG_V6_HP_ACTION_ADD) {
> - spapr_hotplug_set_signalled(drc_id->index);
> - }
> break;
> case SPAPR_DR_CONNECTOR_TYPE_LMB:
> hp->hotplug_type = RTAS_LOG_V6_HP_TYPE_MEMORY;
> diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> index c487123..f24188d 100644
> --- a/include/hw/ppc/spapr_drc.h
> +++ b/include/hw/ppc/spapr_drc.h
> @@ -199,7 +199,6 @@ typedef struct sPAPRDRConnector {
> sPAPRConfigureConnectorState *ccs;
>
> bool awaiting_release;
> - bool signalled;
> bool awaiting_allocation;
> bool awaiting_allocation_skippable;
>
> @@ -226,7 +225,6 @@ typedef struct sPAPRDRConnectorClass {
>
> /* QEMU interfaces for managing hotplug operations */
> bool (*release_pending)(sPAPRDRConnector *drc);
> - void (*set_signalled)(sPAPRDRConnector *drc);
> } sPAPRDRConnectorClass;
>
> uint32_t spapr_drc_index(sPAPRDRConnector *drc);
pgp5aKGRjEwhp.pgp
Description: OpenPGP digital signature
