[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-ppc] [FIX PATCH v0] spapr: Allow configure-connector to be cal
From: |
David Gibson |
Subject: |
Re: [Qemu-ppc] [FIX PATCH v0] spapr: Allow configure-connector to be called multiple times for LMBs |
Date: |
Thu, 17 Aug 2017 11:29:19 +1000 |
User-agent: |
Mutt/1.8.3 (2017-05-23) |
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 <address@hidden>
> ---
> 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