| Message ID | 1502883336-14608-1-git-send-email-bharata@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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;
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) { + /* + * 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. + */ 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) { + /* + * 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;
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(-)