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

Bharata B Rao posted 1 patch 6 years, 7 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/1502947002-19016-1-git-send-email-bharata@linux.vnet.ibm.com
Test FreeBSD passed
Test checkpatch passed
Test docker passed
Test s390x passed
hw/ppc/spapr_drc.c | 27 +++++++++++++++++++++------
1 file changed, 21 insertions(+), 6 deletions(-)
[Qemu-devel] [FIX PATCH v1] spapr: Allow configure-connector to be called multiple times
Posted by Bharata B Rao 6 years, 7 months ago
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(-)

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;
 }
 
 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;
-- 
2.7.4


Re: [Qemu-devel] [FIX PATCH v1] spapr: Allow configure-connector to be called multiple times
Posted by David Gibson 6 years, 7 months ago
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