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 <[email protected]> > --- > 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
signature.asc
Description: PGP signature
