Re: [Qemu-ppc] [PATCH 5/6] spapr: Clean up DRC set_allocation_state path

[Greg Kurz]

On Thu,  8 Jun 2017 15:09:29 +1000
David Gibson <address@hidden> wrote:

> The allocation-state indicator should only actually be implemented for
> "logical" DRCs, not physical ones.  Factor a check for this, and also for
> valid indicator state values into rtas_set_allocation_state().  Because
> they don't exist for physical DRCs, there's no reason that we'd ever want
> more than one method implementation, so it can just be a plain function.
> 
> In addition, the setting to USABLE and setting to UNUSABLE paths in
> set_allocation_state() don't actually have much in common.  So, split the
> method separate functions for each parameter value (drc_set_usable()
> and drc_set_unusable()).
> 
> Signed-off-by: David Gibson <address@hidden>
> ---

Reviewed-by: Greg Kurz <address@hidden>

>  hw/ppc/spapr_drc.c         | 85 
> +++++++++++++++++++++++++---------------------
>  include/hw/ppc/spapr_drc.h |  2 --
>  2 files changed, 46 insertions(+), 41 deletions(-)
> 
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 7e17f34..9e01d7b 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -114,44 +114,43 @@ static uint32_t set_isolation_state(sPAPRDRConnector 
> *drc,
>      return RTAS_OUT_SUCCESS;
>  }
>  
> -static uint32_t set_allocation_state(sPAPRDRConnector *drc,
> -                                     sPAPRDRAllocationState state)
> +static uint32_t drc_set_usable(sPAPRDRConnector *drc)
>  {
> -    trace_spapr_drc_set_allocation_state(spapr_drc_index(drc), state);
> -
> -    if (state == SPAPR_DR_ALLOCATION_STATE_USABLE) {
> -        /* if there's no resource/device associated with the DRC, there's
> -         * no way for us to put it in an allocation state consistent with
> -         * being 'USABLE'. PAPR 2.7, 13.5.3.4 documents that this should
> -         * result in an RTAS return code of -3 / "no such indicator"
> +    /* if there's no resource/device associated with the DRC, there's
> +     * no way for us to put it in an allocation state consistent with
> +     * being 'USABLE'. PAPR 2.7, 13.5.3.4 documents that this should
> +     * result in an RTAS return code of -3 / "no such indicator"
> +     */
> +    if (!drc->dev) {
> +        return RTAS_OUT_NO_SUCH_INDICATOR;
> +    }
> +    if (drc->awaiting_release && drc->awaiting_allocation) {
> +        /* kernel is acknowledging a previous hotplug event
> +         * while we are already removing it.
> +         * it's safe to ignore awaiting_allocation here since we know the
> +         * situation is predicated on the guest either already having done
> +         * so (boot-time hotplug), or never being able to acquire in the
> +         * first place (hotplug followed by immediate unplug).
>           */
> -        if (!drc->dev) {
> -            return RTAS_OUT_NO_SUCH_INDICATOR;
> -        }
> -        if (drc->awaiting_release && drc->awaiting_allocation) {
> -            /* kernel is acknowledging a previous hotplug event
> -             * while we are already removing it.
> -             * it's safe to ignore awaiting_allocation here since we know the
> -             * situation is predicated on the guest either already having 
> done
> -             * so (boot-time hotplug), or never being able to acquire in the
> -             * first place (hotplug followed by immediate unplug).
> -             */
> -            drc->awaiting_allocation_skippable = true;
> -            return RTAS_OUT_NO_SUCH_INDICATOR;
> -        }
> +        drc->awaiting_allocation_skippable = true;
> +        return RTAS_OUT_NO_SUCH_INDICATOR;
>      }
>  
> -    if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
> -        drc->allocation_state = state;
> -        if (drc->awaiting_release &&
> -            drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
> -            uint32_t drc_index = spapr_drc_index(drc);
> -            trace_spapr_drc_set_allocation_state_finalizing(drc_index);
> -            spapr_drc_detach(drc, DEVICE(drc->dev), NULL);
> -        } else if (drc->allocation_state == 
> SPAPR_DR_ALLOCATION_STATE_USABLE) {
> -            drc->awaiting_allocation = false;
> -        }
> +    drc->allocation_state = SPAPR_DR_ALLOCATION_STATE_USABLE;
> +    drc->awaiting_allocation = false;
> +
> +    return RTAS_OUT_SUCCESS;
> +}
> +
> +static uint32_t drc_set_unusable(sPAPRDRConnector *drc)
> +{
> +    drc->allocation_state = SPAPR_DR_ALLOCATION_STATE_UNUSABLE;
> +    if (drc->awaiting_release) {
> +        uint32_t drc_index = spapr_drc_index(drc);
> +        trace_spapr_drc_set_allocation_state_finalizing(drc_index);
> +        spapr_drc_detach(drc, DEVICE(drc->dev), NULL);
>      }
> +
>      return RTAS_OUT_SUCCESS;
>  }
>  
> @@ -553,7 +552,6 @@ static void spapr_dr_connector_class_init(ObjectClass *k, 
> void *data)
>      dk->realize = realize;
>      dk->unrealize = unrealize;
>      drck->set_isolation_state = set_isolation_state;
> -    drck->set_allocation_state = set_allocation_state;
>      drck->release_pending = release_pending;
>      /*
>       * Reason: it crashes FIXME find and document the real reason
> @@ -823,14 +821,23 @@ static uint32_t rtas_set_isolation_state(uint32_t idx, 
> uint32_t state)
>  static uint32_t rtas_set_allocation_state(uint32_t idx, uint32_t state)
>  {
>      sPAPRDRConnector *drc = spapr_drc_by_index(idx);
> -    sPAPRDRConnectorClass *drck;
>  
> -    if (!drc) {
> -        return RTAS_OUT_PARAM_ERROR;
> +    if (!drc || !object_dynamic_cast(OBJECT(drc), TYPE_SPAPR_DRC_LOGICAL)) {
> +        return RTAS_OUT_NO_SUCH_INDICATOR;
>      }
>  
> -    drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> -    return drck->set_allocation_state(drc, state);
> +    trace_spapr_drc_set_allocation_state(spapr_drc_index(drc), state);
> +
> +    switch (state) {
> +    case SPAPR_DR_ALLOCATION_STATE_USABLE:
> +        return drc_set_usable(drc);
> +
> +    case SPAPR_DR_ALLOCATION_STATE_UNUSABLE:
> +        return drc_set_unusable(drc);
> +
> +    default:
> +        return RTAS_OUT_PARAM_ERROR;
> +    }
>  }
>  
>  static uint32_t rtas_set_dr_indicator(uint32_t idx, uint32_t state)
> diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> index f24188d..0e09afb 100644
> --- a/include/hw/ppc/spapr_drc.h
> +++ b/include/hw/ppc/spapr_drc.h
> @@ -220,8 +220,6 @@ typedef struct sPAPRDRConnectorClass {
>      /* accessors for guest-visible (generally via RTAS) DR state */
>      uint32_t (*set_isolation_state)(sPAPRDRConnector *drc,
>                                      sPAPRDRIsolationState state);
> -    uint32_t (*set_allocation_state)(sPAPRDRConnector *drc,
> -                                     sPAPRDRAllocationState state);
>  
>      /* QEMU interfaces for managing hotplug operations */
>      bool (*release_pending)(sPAPRDRConnector *drc);

pgpIhB_5JMvtE.pgp
Description: OpenPGP digital signature