Re: [Qemu-devel] [PATCH 3/5] spapr: Move configure-connector state into DRC

[Michael Roth]

Quoting David Gibson (2017-06-02 02:29:50)
> Currently the sPAPRMachineState contains a of sPAPRConfigureConnector

"contains a list"?

> structures which store intermediate state for the ibm,configure-connector
> RTAS call.
> 
> This was an attempt to separate this state from the core of the DRC state.
> However the configure connector process is intimately tied to the DRC
> model, so there's really no point trying to have two levels of interface
> here.
> 
> Moving the configure-connector state into its corresponding DRC allows
> removal of a number of helpers for maintaining the anciliary list.
> 
> Signed-off-by: David Gibson <address@hidden>

Reviewed-by: Michael Roth <address@hidden>

> ---
>  hw/ppc/spapr.c             |  4 ---
>  hw/ppc/spapr_drc.c         | 73 
> ++++++++++++----------------------------------
>  include/hw/ppc/spapr.h     | 14 ---------
>  include/hw/ppc/spapr_drc.h |  7 +++++
>  4 files changed, 25 insertions(+), 73 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 7aac3b9..6234dbd 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2339,10 +2339,6 @@ static void ppc_spapr_init(MachineState *machine)
>      register_savevm_live(NULL, "spapr/htab", -1, 1,
>                           &savevm_htab_handlers, spapr);
> 
> -    /* used by RTAS */
> -    QTAILQ_INIT(&spapr->ccs_list);
> -    qemu_register_reset(spapr_ccs_reset_hook, spapr);
> -
>      qemu_register_boot_set(spapr_boot_set, spapr);
> 
>      if (kvm_enabled()) {
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 2514f87..27d4bd3 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -27,34 +27,6 @@
>  #define DRC_INDEX_TYPE_SHIFT 28
>  #define DRC_INDEX_ID_MASK ((1ULL << DRC_INDEX_TYPE_SHIFT) - 1)
> 
> -static sPAPRConfigureConnectorState *spapr_ccs_find(sPAPRMachineState *spapr,
> -                                                    uint32_t drc_index)
> -{
> -    sPAPRConfigureConnectorState *ccs = NULL;
> -
> -    QTAILQ_FOREACH(ccs, &spapr->ccs_list, next) {
> -        if (ccs->drc_index == drc_index) {
> -            break;
> -        }
> -    }
> -
> -    return ccs;
> -}
> -
> -static void spapr_ccs_add(sPAPRMachineState *spapr,
> -                          sPAPRConfigureConnectorState *ccs)
> -{
> -    g_assert(!spapr_ccs_find(spapr, ccs->drc_index));
> -    QTAILQ_INSERT_HEAD(&spapr->ccs_list, ccs, next);
> -}
> -
> -static void spapr_ccs_remove(sPAPRMachineState *spapr,
> -                             sPAPRConfigureConnectorState *ccs)
> -{
> -    QTAILQ_REMOVE(&spapr->ccs_list, ccs, next);
> -    g_free(ccs);
> -}
> -
>  sPAPRDRConnectorType spapr_drc_type(sPAPRDRConnector *drc)
>  {
>      sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> @@ -81,6 +53,16 @@ static uint32_t set_isolation_state(sPAPRDRConnector *drc,
> 
>      trace_spapr_drc_set_isolation_state(spapr_drc_index(drc), state);
> 
> +    /* 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)
> +     */
> +    if (state == SPAPR_DR_ISOLATION_STATE_ISOLATED) {
> +        g_free(drc->ccs);
> +        drc->ccs = NULL;
> +    }
> +
>      if (state == SPAPR_DR_ISOLATION_STATE_UNISOLATED) {
>          /* cannot unisolate a non-existent resource, and, or resources
>           * which are in an 'UNUSABLE' allocation state. (PAPR 2.7, 13.5.3.5)
> @@ -485,6 +467,10 @@ static void reset(DeviceState *d)
>      sPAPRDREntitySense state;
> 
>      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, and that they will
>       * subsequently be left in an ISOLATED state. move the DRC to this
> @@ -1020,19 +1006,6 @@ static void rtas_set_indicator(PowerPCCPU *cpu, 
> sPAPRMachineState *spapr,
> 
>      switch (sensor_type) {
>      case RTAS_SENSOR_TYPE_ISOLATION_STATE:
> -        /* 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)
> -         */
> -        if (sensor_state == SPAPR_DR_ISOLATION_STATE_ISOLATED) {
> -            sPAPRConfigureConnectorState *ccs = spapr_ccs_find(spapr,
> -                                                               sensor_index);
> -            if (ccs) {
> -                spapr_ccs_remove(spapr, ccs);
> -            }
> -        }
>          ret = drck->set_isolation_state(drc, sensor_state);
>          break;
>      case RTAS_SENSOR_TYPE_DR:
> @@ -1116,16 +1089,6 @@ static void configure_connector_st(target_ulong addr, 
> target_ulong offset,
>                                buf, MIN(len, CC_WA_LEN - offset));
>  }
> 
> -void spapr_ccs_reset_hook(void *opaque)
> -{
> -    sPAPRMachineState *spapr = opaque;
> -    sPAPRConfigureConnectorState *ccs, *ccs_tmp;
> -
> -    QTAILQ_FOREACH_SAFE(ccs, &spapr->ccs_list, next, ccs_tmp) {
> -        spapr_ccs_remove(spapr, ccs);
> -    }
> -}
> -
>  static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
>                                           sPAPRMachineState *spapr,
>                                           uint32_t token, uint32_t nargs,
> @@ -1161,12 +1124,11 @@ static void rtas_ibm_configure_connector(PowerPCCPU 
> *cpu,
>          goto out;
>      }
> 
> -    ccs = spapr_ccs_find(spapr, drc_index);
> +    ccs = drc->ccs;
>      if (!ccs) {
>          ccs = g_new0(sPAPRConfigureConnectorState, 1);
>          ccs->fdt_offset = drc->fdt_start_offset;
> -        ccs->drc_index = drc_index;
> -        spapr_ccs_add(spapr, ccs);
> +        drc->ccs = ccs;
>      }
> 
>      do {
> @@ -1203,7 +1165,8 @@ static void rtas_ibm_configure_connector(PowerPCCPU 
> *cpu,
>                      /* guest should be not configuring an isolated device */
>                      trace_spapr_drc_set_configured_skipping(drc_index);
>                  }
> -                spapr_ccs_remove(spapr, ccs);
> +                g_free(ccs);
> +                drc->ccs = NULL;
>                  ccs = NULL;
>                  resp = SPAPR_DR_CC_RESPONSE_SUCCESS;
>              } else {
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 98fb78b..f973b02 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -11,7 +11,6 @@
>  struct VIOsPAPRBus;
>  struct sPAPRPHBState;
>  struct sPAPRNVRAM;
> -typedef struct sPAPRConfigureConnectorState sPAPRConfigureConnectorState;
>  typedef struct sPAPREventLogEntry sPAPREventLogEntry;
>  typedef struct sPAPREventSource sPAPREventSource;
> 
> @@ -102,9 +101,6 @@ struct sPAPRMachineState {
>      bool htab_first_pass;
>      int htab_fd;
> 
> -    /* RTAS state */
> -    QTAILQ_HEAD(, sPAPRConfigureConnectorState) ccs_list;
> -
>      /* Pending DIMM unplug cache. It is populated when a LMB
>       * unplug starts. It can be regenerated if a migration
>       * occurs during the unplug process. */
> @@ -646,16 +642,6 @@ void *spapr_populate_hotplug_cpu_dt(CPUState *cs, int 
> *fdt_offset,
>  void spapr_core_release(DeviceState *dev);
>  void spapr_lmb_release(DeviceState *dev);
> 
> -/* rtas-configure-connector state */
> -struct sPAPRConfigureConnectorState {
> -    uint32_t drc_index;
> -    int fdt_offset;
> -    int fdt_depth;
> -    QTAILQ_ENTRY(sPAPRConfigureConnectorState) next;
> -};
> -
> -void spapr_ccs_reset_hook(void *opaque);
> -
>  void spapr_rtc_read(sPAPRRTCState *rtc, struct tm *tm, uint32_t *ns);
>  int spapr_rtc_import_offset(sPAPRRTCState *rtc, int64_t legacy_offset);
> 
> diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> index d8cb84b..53b0f8b 100644
> --- a/include/hw/ppc/spapr_drc.h
> +++ b/include/hw/ppc/spapr_drc.h
> @@ -172,6 +172,12 @@ typedef enum {
>      SPAPR_DR_CC_RESPONSE_NOT_CONFIGURABLE = -9003,
>  } sPAPRDRCCResponse;
> 
> +/* rtas-configure-connector state */
> +typedef struct sPAPRConfigureConnectorState {
> +    int fdt_offset;
> +    int fdt_depth;
> +} sPAPRConfigureConnectorState;
> +
>  typedef struct sPAPRDRConnector {
>      /*< private >*/
>      DeviceState parent;
> @@ -189,6 +195,7 @@ typedef struct sPAPRDRConnector {
>      void *fdt;
>      int fdt_start_offset;
>      bool configured;
> +    sPAPRConfigureConnectorState *ccs;
> 
>      bool awaiting_release;
>      bool signalled;
> -- 
> 2.9.4
>