Re: [Qemu-devel] [PATCH 6/6] spapr: Clean up DRC set_isolation_state() path

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 7915 bytes --]

On Mon, Jun 19, 2017 at 05:52:27PM -0500, Michael Roth wrote:
> Quoting David Gibson (2017-06-08 00:09:30)
> > There are substantial differences in the various paths through
> > set_isolation_state(), both for setting to ISOLATED versus UNISOLATED
> > state and for logical versus physical DRCs.
> > 
> > So, split the set_isolation_state() method into isolate() and unisolate()
> > methods, and give it different implementations for the two DRC types.
> > 
> > Factor some minimal common checks, including for valid indicator values
> > (which we weren't previously checking) into rtas_set_isolation_state().
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/ppc/spapr_drc.c         | 145 ++++++++++++++++++++++++++++++++-------------
> >  include/hw/ppc/spapr_drc.h |   6 +-
> >  2 files changed, 105 insertions(+), 46 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> > index 9e01d7b..90c0fde 100644
> > --- a/hw/ppc/spapr_drc.c
> > +++ b/hw/ppc/spapr_drc.c
> > @@ -46,30 +46,64 @@ uint32_t spapr_drc_index(sPAPRDRConnector *drc)
> >          | (drc->id & DRC_INDEX_ID_MASK);
> >  }
> > 
> > -static uint32_t set_isolation_state(sPAPRDRConnector *drc,
> > -                                    sPAPRDRIsolationState state)
> > +static uint32_t drc_isolate_physical(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;
> > -    }
> > +    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)
> > -         */
> > -        if (!drc->dev ||
> > -            drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
> > -            return RTAS_OUT_NO_SUCH_INDICATOR;
> > +    drc->isolation_state = SPAPR_DR_ISOLATION_STATE_ISOLATED;
> > +
> > +    /* if we're awaiting release, but still in an unconfigured state,
> > +     * it's likely the guest is still in the process of configuring
> > +     * the device and is transitioning the devices to an ISOLATED
> > +     * state as a part of that process. so we only complete the
> > +     * removal when this transition happens for a device in a
> > +     * configured state, as suggested by the state diagram from PAPR+
> > +     * 2.7, 13.4
> > +     */
> > +    if (drc->awaiting_release) {
> > +        uint32_t drc_index = spapr_drc_index(drc);
> > +        if (drc->configured) {
> > +            trace_spapr_drc_set_isolation_state_finalizing(drc_index);
> > +            spapr_drc_detach(drc, DEVICE(drc->dev), NULL);
> > +        } else {
> > +            trace_spapr_drc_set_isolation_state_deferring(drc_index);
> >          }
> >      }
> > +    drc->configured = false;
> > +
> > +    return RTAS_OUT_SUCCESS;
> > +}
> > +
> > +static uint32_t drc_unisolate_physical(sPAPRDRConnector *drc)
> > +{
> > +    /* cannot unisolate a non-existent resource, and, or resources
> > +     * which are in an 'UNUSABLE' allocation state. (PAPR 2.7,
> > +     * 13.5.3.5)
> > +     */
> > +    if (!drc->dev) {
> > +        return RTAS_OUT_NO_SUCH_INDICATOR;
> > +    }
> > +
> > +    drc->isolation_state = SPAPR_DR_ISOLATION_STATE_UNISOLATED;
> > +
> > +    return RTAS_OUT_SUCCESS;
> > +}
> > +
> > +static uint32_t drc_isolate_logical(sPAPRDRConnector *drc)
> > +{
> > +    /* 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)
> > +     */
> > +    g_free(drc->ccs);
> > +    drc->ccs = NULL;
> > 
> >      /*
> >       * Fail any requests to ISOLATE the LMB DRC if this LMB doesn't
> > @@ -81,35 +115,47 @@ static uint32_t set_isolation_state(sPAPRDRConnector *drc,
> >       * If the LMB being removed doesn't belong to a DIMM device that is
> >       * actually being unplugged, fail the isolation request here.
> >       */
> > -    if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_LMB) {
> > -        if ((state == SPAPR_DR_ISOLATION_STATE_ISOLATED) &&
> > -             !drc->awaiting_release) {
> > -            return RTAS_OUT_HW_ERROR;
> > -        }
> > +    if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_LMB
> > +        && !drc->awaiting_release) {
> > +        return RTAS_OUT_HW_ERROR;
> >      }
> > 
> > -    drc->isolation_state = state;
> > +    drc->isolation_state = SPAPR_DR_ISOLATION_STATE_ISOLATED;
> > 
> > -    if (drc->isolation_state == SPAPR_DR_ISOLATION_STATE_ISOLATED) {
> > -        /* if we're awaiting release, but still in an unconfigured state,
> > -         * it's likely the guest is still in the process of configuring
> > -         * the device and is transitioning the devices to an ISOLATED
> > -         * state as a part of that process. so we only complete the
> > -         * removal when this transition happens for a device in a
> > -         * configured state, as suggested by the state diagram from
> > -         * PAPR+ 2.7, 13.4
> > -         */
> > -        if (drc->awaiting_release) {
> > -            uint32_t drc_index = spapr_drc_index(drc);
> > -            if (drc->configured) {
> > -                trace_spapr_drc_set_isolation_state_finalizing(drc_index);
> > -                spapr_drc_detach(drc, DEVICE(drc->dev), NULL);
> > -            } else {
> > -                trace_spapr_drc_set_isolation_state_deferring(drc_index);
> > -            }
> > +    /* if we're awaiting release, but still in an unconfigured state,
> > +     * it's likely the guest is still in the process of configuring
> > +     * the device and is transitioning the devices to an ISOLATED
> > +     * state as a part of that process. so we only complete the
> > +     * removal when this transition happens for a device in a
> > +     * configured state, as suggested by the state diagram from PAPR+
> > +     * 2.7, 13.4
> > +     */
> > +    if (drc->awaiting_release) {
> > +        uint32_t drc_index = spapr_drc_index(drc);
> > +        if (drc->configured) {
> > +            trace_spapr_drc_set_isolation_state_finalizing(drc_index);
> > +            spapr_drc_detach(drc, DEVICE(drc->dev), NULL);
> > +        } else {
> > +            trace_spapr_drc_set_isolation_state_deferring(drc_index);
> 
> Not sure this is the right patch to do it, but this refactoring does make
> it more apparent that the if (drc->configured) checks should get pushed
> down into spapr_drc_detach() like the other deferral checks at some point.
> 
> (There are 2 locations that do the detach without checking configured,
> but you switched out the one in reset() to use spapr_drc_release()
> already, and I don't think drc_set_unusable() without first going
> through drc_isolate_*() is a transition that PAPR would allow anyway)

Right, but no, I don't think this patch is the place to do it.

I'll see what this looks like once I've made other cleanups on my
queue.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]