Re: [Qemu-devel] [PATCH 2/6] spapr: Eliminate DRC 'signalled' state variable

[Greg Kurz]

On Thu,  8 Jun 2017 15:09:26 +1000
David Gibson <address@hidden> 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 <address@hidden>
> ---

Reviewed-by: Greg Kurz <address@hidden>

>  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);

pgptOP8gM9sVW.pgp
Description: OpenPGP digital signature