Re: [PATCH v3 1/7] spapr_drc.c: do not call spapr_drc_detach() in drc_isolate_logical()

[David Gibson]

On Mon, Feb 15, 2021 at 11:40:06AM +0100, Greg Kurz wrote:
> On Thu, 11 Feb 2021 19:52:40 -0300
> Daniel Henrique Barboza <danielhb413@gmail.com> wrote:
> 
> > drc_isolate_logical() is used to move the DRC from the "Configured" to the
> > "Available" state, erroring out if the DRC is in the unexpected "Unisolate"
> > state and doing nothing (with RTAS_OUT_SUCCESS) if the DRC is already in
> > "Available" or in "Unusable" state.
> > 
> > When moving from "Configured" to "Available", the DRC is moved to the
> > LOGICAL_AVAILABLE state, a drc->unplug_requested check is done and, if true,
> > spapr_drc_detach() is called.
> > 
> > What spapr_drc_detach() does then is:
> > 
> > - set drc->unplug_requested to true. In fact, this is the only place where
> > unplug_request is set to true;
> > - does nothing else if drc->state != drck->empty_state. If the DRC state
> > is equal to drck->empty_state, spapr_drc_release() is called. For logical
> > DRCs, drck->empty_state = LOGICAL_UNUSABLE.
> > 
> > In short, calling spapr_drc_detach() in drc_isolate_logical() does nothing. 
> > It'll
> > set unplug_request to true again ('again' since it was already true - 
> > otherwise the
> > function wouldn't be called), and will return without calling 
> > spapr_drc_release()
> > because the DRC is not in LOGICAL_UNUSABLE, since drc_isolate_logical() just
> > moved it to LOGICAL_AVAILABLE. The only place where the logical DRC is 
> > released
> > is when called from drc_set_unusable(), when it is moved to the "Unusable" 
> > state.
> > As it should, according to PAPR.
> > 
> > Even though calling spapr_drc_detach() in drc_isolate_logical() is benign, 
> > removing
> > it will avoid further thought about the matter. So let's go ahead and do 
> > that.
> > 
> 
> Good catch. This path looks useless for logical DRCs indeed.
> 
> > As a note, this logic was introduced in commit bbf5c878ab76. Since then, 
> > the DRC
> > handling code was refactored and enhanced, and PAPR itself went through some
> > changes in the DRC area as well. It is expected that some assumptions we 
> > had back
> > then are now deprecated.
> > 
> 
> As specified in [1]:
> 
> Please do not use lines that are longer than 76 characters in your
> commit message (so that the text still shows up nicely with "git show"
> in a 80-columns terminal window).
> 
> [1] 
> https://wiki.qemu.org/Contribute/SubmitAPatch#Write_a_meaningful_commit_message

I've applied this patch, but re-wrapped the commit message.

> > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> > ---
> >  hw/ppc/spapr_drc.c | 13 -------------
> >  1 file changed, 13 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> > index 8571d5bafe..84bd3c881f 100644
> > --- a/hw/ppc/spapr_drc.c
> > +++ b/hw/ppc/spapr_drc.c
> > @@ -132,19 +132,6 @@ static uint32_t drc_isolate_logical(SpaprDrc *drc)
> >  
> >      drc->state = SPAPR_DRC_STATE_LOGICAL_AVAILABLE;
> >  
> > -    /* 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->unplug_requested) {
> > -        uint32_t drc_index = spapr_drc_index(drc);
> > -        trace_spapr_drc_set_isolation_state_finalizing(drc_index);
> 
> I was expecting a change in hw/ppc/trace-events to ditch this trace,
> but it is still called by drc_isolate_physical(), so we're good.
> 
> Reviewed-by: Greg Kurz <groug@kaod.org>
> 
> > -        spapr_drc_detach(drc);
> > -    }
> >      return RTAS_OUT_SUCCESS;
> >  }
> >  
> 

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

signature.asc
Description: PGP signature