On Fri, Feb 26, 2021 at 01:32:58PM -0300, Daniel Henrique Barboza wrote:
Now that we're asserting the first DRC LMB earlier, use it to query if
the DRC is already pending unplug and, in this case, issue the same
error we already do.
The previous check was introduced in commit 2a129767ebb1 and it works,
but it's easier to check the unplug_requested flag instead of looking
for the existence of the sPAPRDIMMState. It's also compliant with what
is already done in other unplug_request functions for other devices.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
I'm having some trouble completely convincing myself this is right.
What about this situation:
1. We initiate a DIMM unplug
- unplug_request is set on all the LMBs
- all the LMBs go on the pending_unplug list
2. The guest encounters no problems, and starts issuing set
indicator calls to mark the LMBs unusable, starting from the
lowest address
3. On drc_set_unusable() for the first LMB, we see that unplug is
requested and call spapr_drc_release()
4. spapr_drc_release() on the first LMB clears unplug_requested
5. At this point, but before this is done on *all* of the DIMM's
LMBs, the user attempts another unplug triggering the code
below
AFAICT this will now skip the error, since the first LMB is no longer
in unplug_requested state, but there *are* still pending unplugs for
some of the remaining LMBs, so the old code would have tripped the
error.
---
hw/ppc/spapr.c | 8 +-------
1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 74e046b522..149dc2113f 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3681,13 +3681,7 @@ static void spapr_memory_unplug_request(HotplugHandler
*hotplug_dev,
addr_start / SPAPR_MEMORY_BLOCK_SIZE);
g_assert(drc_start);
- /*
- * An existing pending dimm state for this DIMM means that there is an
- * unplug operation in progress, waiting for the spapr_lmb_release
- * callback to complete the job (BQL can't cover that far). In this case,
- * bail out to avoid detaching DRCs that were already released.
- */
- if (spapr_pending_dimm_unplugs_find(spapr, dimm)) {
+ if (spapr_drc_unplug_requested(drc_start)) {
error_setg(errp, "Memory unplug already in progress for device %s",
dev->id);
return;