[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [FIX PATCH v1] spapr: Allow configure-connector to be c
From: |
David Gibson |
Subject: |
Re: [Qemu-devel] [FIX PATCH v1] spapr: Allow configure-connector to be called multiple times |
Date: |
Thu, 17 Aug 2017 16:44:02 +1000 |
User-agent: |
Mutt/1.8.3 (2017-05-23) |
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 <address@hidden>
> ---
> 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
signature.asc
Description: PGP signature