On Thu, 11 Feb 2021 19:52:40 -0300
Daniel Henrique Barboza <danielhb...@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
> Signed-off-by: Daniel Henrique Barboza <danielhb...@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 <gr...@kaod.org>
> - spapr_drc_detach(drc);
> - }
> return RTAS_OUT_SUCCESS;
> }
>
