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

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

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

On Wed, Aug 16, 2017 at 05:05:36PM +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
> LMBs.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>  hw/ppc/spapr_drc.c | 37 +++++++++++++++++++++++++++++++------
>  1 file changed, 31 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 5260b5d..2dd9635 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -446,8 +446,17 @@ void spapr_drc_reset(sPAPRDRConnector *drc)
>          drc->state = drck->empty_state;
>      }
>  
> -    drc->ccs_offset = -1;
> -    drc->ccs_depth = -1;
> +    if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_LMB) {

We shouldn't have a type dependency here.  If we want the configure
connector process to be repeatable, it should be repeatable for
everything, even if we only actually need it for LMBs.

> +        /*
> +         * Ensure that we are able to send the FDT fragment of the
> +         * LMB again via configure-connector call if guest requests.
> +         */
> +        drc->ccs_offset = drc->fdt_start_offset;
> +        drc->ccs_depth = 0;
> +    } else {
> +        drc->ccs_offset = -1;
> +        drc->ccs_depth = -1;
> +    }
>  }
>  
>  static void drc_reset(void *opaque)
> @@ -1071,8 +1080,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) &&
> +        (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_LMB)) {
> +        /*
> +         * Need to unisolate the device before configuring, however
> +         * LMB DRCs are exempted from this check as guest can issue
> +         * configure-connector calls for an already configured
> +         * LMB DRC.
> +         */

Same here - but we do need to explicitly check that the state is
either UNISOLATE *or* CONFIGURED, and not allow it from some other
random state.

>          rc = SPAPR_DR_CC_RESPONSE_NOT_CONFIGURABLE;
>          goto out;
>      }
> @@ -1108,8 +1123,18 @@ 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;
> +                if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_LMB) {

And again.

> +                    /*
> +                     * Ensure that we are able to send the FDT fragment of the
> +                     * LMB again via configure-connector call if guest requests.
> +                     */
> +                    drc->ccs_offset = drc->fdt_start_offset;
> +                    drc->ccs_depth = 0;
> +                    fdt_offset_next = drc->fdt_start_offset;
> +                } else {
> +                    drc->ccs_offset = -1;
> +                    drc->ccs_depth = -1;
> +                }
>                  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 --]