Re: [Qemu-devel] [FIX PATCH v1] spapr: Allow configure-connector to be called multiple times

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

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

On Thu, Aug 17, 2017 at 10:46:42AM +0530, Bharata B Rao wrote:
> In case of in-kernel memory hot unplug, when the guest is not able
> to remove all the LMBs that are requested for removal, it will add back
> any LMBs that have been successfully removed. The DR Connectors of
> these LMBs wouldn't have been unconfigured and hence the addition of
> these LMBs will result in configure-connector call being issued on
> LMB DR connectors that are already in configured state. Such
> configure-connector calls will fail resulting in a DIMM which is
> partially unplugged.
> 
> This however worked till recently before we overhauled the DRC
> implementation in QEMU. Commit 9d4c0f4f0a71e: "spapr: Consolidate
> DRC state variables" is the first commit where this problem shows up
> as per git bisect.
> 
> Ideally guest shouldn't be issuing configure-connector call on an
> already configured DR connector. However for now, work around this in
> QEMU by allowing configure-connector to be called multiple times for
> all types of DR connectors.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
> v0: https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg02942.html
> Changes in v1:
>  - Allow configure-connector to be called multiple times for all types
>    of DR connectors and not just LMB DRCs. (David Gibson)
>  - Explicitly allow configure-connector to proceed only if the DRC is
>    either in unisolated or in configured state. (David Gibson)
> 
>  hw/ppc/spapr_drc.c | 27 +++++++++++++++++++++------
>  1 file changed, 21 insertions(+), 6 deletions(-)

I've applied with a small correction:

> 
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 5260b5d..40d1e99 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -446,8 +446,12 @@ void spapr_drc_reset(sPAPRDRConnector *drc)
>          drc->state = drck->empty_state;
>      }
>  
> -    drc->ccs_offset = -1;
> -    drc->ccs_depth = -1;
> +    /*
> +     * Ensure that we are able to send the FDT fragment again
> +     * via configure-connector call if the guest requests.
> +     */
> +    drc->ccs_offset = drc->fdt_start_offset;
> +    drc->ccs_depth = 0;

This isn't quite right - we should only set these values ready when we
go into ready state (==CONFIGURED == device present) rather than empty
state (==UNALLOCATED == no device present).

>  }
>  
>  static void drc_reset(void *opaque)
> @@ -1071,8 +1075,14 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
>      }
>  
>      if ((drc->state != SPAPR_DRC_STATE_LOGICAL_UNISOLATE)
> -        && (drc->state != SPAPR_DRC_STATE_PHYSICAL_UNISOLATE)) {
> -        /* Need to unisolate the device before configuring */
> +        && (drc->state != SPAPR_DRC_STATE_PHYSICAL_UNISOLATE)
> +        && (drc->state != SPAPR_DRC_STATE_LOGICAL_CONFIGURED)
> +        && (drc->state != SPAPR_DRC_STATE_PHYSICAL_CONFIGURED)) {
> +        /*
> +         * Need to unisolate the device before configuring
> +         * or it should already be in configured state to
> +         * allow configure-connector be called repeatedly.
> +         */
>          rc = SPAPR_DR_CC_RESPONSE_NOT_CONFIGURABLE;
>          goto out;
>      }
> @@ -1108,8 +1118,13 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
>                  /* done sending the device tree, move to configured state */
>                  trace_spapr_drc_set_configured(drc_index);
>                  drc->state = drck->ready_state;
> -                drc->ccs_offset = -1;
> -                drc->ccs_depth = -1;
> +                /*
> +                 * Ensure that we are able to send the FDT fragment
> +                 * again via configure-connector call if the guest requests.
> +                 */
> +                drc->ccs_offset = drc->fdt_start_offset;
> +                drc->ccs_depth = 0;
> +                fdt_offset_next = drc->fdt_start_offset;
>                  resp = SPAPR_DR_CC_RESPONSE_SUCCESS;
>              } else {
>                  resp = SPAPR_DR_CC_RESPONSE_PREV_PARENT;

-- 
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: 833 bytes --]