[PATCH 0/5] send QAPI_EVENT_MEM_UNPLUG_ERROR for ppc64 unplugs

[PATCH 0/5] send QAPI_EVENT_MEM_UNPLUG_ERROR for ppc64 unplugs

[Daniel Henrique Barboza]

Hi,

This series is a follow-up of the existing change in ppc-for-6.0
branch in David's tree, where the pSeries machine is now detecting
one case in which the memory hotunplug is rejected by the guest kernel.

Initially I would like to also add a QAPI event to report a CPU unplug
error, but we don't have this event available yet. Next time.



Daniel Henrique Barboza (5):
  spapr.c: assert first DRC LMB earlier in spapr_memory_unplug_request()
  spapr.c: check unplug_request flag in spapr_memory_unplug_request()
  spapr.c: add 'unplug already in progress' message for PHB unplug
  spapr_pci.c: add 'unplug already in progress' message for PCI unplug
  spapr.c: send QAPI event when memory hotunplug fails

 hw/ppc/spapr.c         | 37 +++++++++++++++++++++++--------------
 hw/ppc/spapr_drc.c     |  5 ++---
 hw/ppc/spapr_pci.c     |  4 ++++
 include/hw/ppc/spapr.h |  3 +--
 4 files changed, 30 insertions(+), 19 deletions(-)

-- 
2.29.2

[PATCH 1/5] spapr.c: assert first DRC LMB earlier in spapr_memory_unplug_request()

[Daniel Henrique Barboza]

We are asserting the existence of the first DRC LMB after sending unplug
requests to all LMBs of the DIMM, where every DRC is being asserted
inside the loop. This means that the first DRC is being asserted twice.

We will use the first DRC to simplify the code a bit in the next patch,
so instead of removing the duplicated assert, let's do it earlier.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/ppc/spapr.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 6eaddb12cb..74e046b522 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3664,7 +3664,7 @@ static void spapr_memory_unplug_request(HotplugHandler *hotplug_dev,
     uint32_t nr_lmbs;
     uint64_t size, addr_start, addr;
     int i;
-    SpaprDrc *drc;
+    SpaprDrc *drc, *drc_start;
 
     if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
         error_setg(errp, "nvdimm device hot unplug is not supported yet.");
@@ -3677,6 +3677,10 @@ static void spapr_memory_unplug_request(HotplugHandler *hotplug_dev,
     addr_start = object_property_get_uint(OBJECT(dimm), PC_DIMM_ADDR_PROP,
                                           &error_abort);
 
+    drc_start = spapr_drc_by_id(TYPE_SPAPR_DRC_LMB,
+                                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
@@ -3701,11 +3705,9 @@ static void spapr_memory_unplug_request(HotplugHandler *hotplug_dev,
         addr += SPAPR_MEMORY_BLOCK_SIZE;
     }
 
-    drc = spapr_drc_by_id(TYPE_SPAPR_DRC_LMB,
-                          addr_start / SPAPR_MEMORY_BLOCK_SIZE);
-    g_assert(drc);
     spapr_hotplug_req_remove_by_count_indexed(SPAPR_DR_CONNECTOR_TYPE_LMB,
-                                              nr_lmbs, spapr_drc_index(drc));
+                                              nr_lmbs,
+                                              spapr_drc_index(drc_start));
 }
 
 /* Callback to be called during DRC release. */
-- 
2.29.2

[PATCH 2/5] spapr.c: check unplug_request flag in spapr_memory_unplug_request()

[Daniel Henrique Barboza]

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>
---
 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;
-- 
2.29.2

[PATCH 3/5] spapr.c: add 'unplug already in progress' message for PHB unplug

[Daniel Henrique Barboza]

Both CPU hotunplug and PC_DIMM unplug reports an user warning,
mentioning that the hotunplug is in progress, if consecutive
'device_del' are issued in quick succession.

Do the same for PHBs in spapr_phb_unplug_request().

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/ppc/spapr.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 149dc2113f..6ef72ee7bd 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -4030,6 +4030,10 @@ static void spapr_phb_unplug_request(HotplugHandler *hotplug_dev,
     if (!spapr_drc_unplug_requested(drc)) {
         spapr_drc_unplug_request(drc);
         spapr_hotplug_req_remove_by_index(drc);
+    } else {
+        error_setg(errp,
+                   "PCI Host Bridge unplug already in progress for device %s",
+                   dev->id);
     }
 }
 
-- 
2.29.2

[PATCH 4/5] spapr_pci.c: add 'unplug already in progress' message for PCI unplug

[Daniel Henrique Barboza]

Hotunplug for all other devices are warning the user when the hotunplug
is already in progress. Do the same for PCI devices in
spapr_pci_unplug_request().

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/ppc/spapr_pci.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index b00e9609ae..feba18cb12 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1743,6 +1743,10 @@ static void spapr_pci_unplug_request(HotplugHandler *plug_handler,
                 }
             }
         }
+    } else {
+        error_setg(errp,
+                   "PCI device unplug already in progress for device %s",
+                   drc->dev->id);
     }
 }
 
-- 
2.29.2

[PATCH 5/5] spapr.c: send QAPI event when memory hotunplug fails

[Daniel Henrique Barboza]

Recent changes allowed the pSeries machine to rollback the hotunplug
process for the DIMM when the guest kernel signals, via a
reconfiguration of the DR connector, that it's not going to release the
LMBs.

Let's also warn QAPI listerners about it. One place to do it would be
right after the unplug state is cleaned up,
spapr_clear_pending_dimm_unplug_state(). This would mean that the
function is now doing more than cleaning up the pending dimm state
though.

This patch does the following changes in spapr.c:

- send a QAPI event to inform that we experienced a failure in the
  hotunplug of the DIMM;

- rename spapr_clear_pending_dimm_unplug_state() to
  spapr_memory_unplug_rollback(). This is a better fit for what the
  function is now doing, and it makes callers care more about what the
  function goal is and less about spapr.c internals such as clearing
  the pending dimm unplug state.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/ppc/spapr.c         | 13 +++++++++++--
 hw/ppc/spapr_drc.c     |  5 ++---
 include/hw/ppc/spapr.h |  3 +--
 3 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 6ef72ee7bd..cbe5cafb14 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -28,6 +28,7 @@
 #include "qemu-common.h"
 #include "qemu/datadir.h"
 #include "qapi/error.h"
+#include "qapi/qapi-events-machine.h"
 #include "qapi/visitor.h"
 #include "sysemu/sysemu.h"
 #include "sysemu/hostmem.h"
@@ -3575,14 +3576,14 @@ static SpaprDimmState *spapr_recover_pending_dimm_state(SpaprMachineState *ms,
     return spapr_pending_dimm_unplugs_add(ms, avail_lmbs, dimm);
 }
 
-void spapr_clear_pending_dimm_unplug_state(SpaprMachineState *spapr,
-                                           DeviceState *dev)
+void spapr_memory_unplug_rollback(SpaprMachineState *spapr, DeviceState *dev)
 {
     SpaprDimmState *ds;
     PCDIMMDevice *dimm;
     SpaprDrc *drc;
     uint32_t nr_lmbs;
     uint64_t size, addr_start, addr;
+    g_autofree char *qapi_error = NULL;
     int i;
 
     if (!dev) {
@@ -3616,6 +3617,14 @@ void spapr_clear_pending_dimm_unplug_state(SpaprMachineState *spapr,
         drc->unplug_requested = false;
         addr += SPAPR_MEMORY_BLOCK_SIZE;
     }
+
+    /*
+     * Tell QAPI that something happened and the memory
+     * hotunplug wasn't successful.
+     */
+    qapi_error = g_strdup_printf("Memory hotunplug failed for device %s",
+                                 dev->id);
+    qapi_event_send_mem_unplug_error(dev->id, qapi_error);
 }
 
 /* Callback to be called during DRC release. */
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 8c4997d795..8faaf9f1dd 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -1232,12 +1232,11 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
 
     /*
      * This indicates that the kernel is reconfiguring a LMB due to
-     * a failed hotunplug. Clear the pending unplug state for the whole
-     * DIMM.
+     * a failed hotunplug. Rollback the DIMM unplug process.
      */
     if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_LMB &&
         drc->unplug_requested) {
-        spapr_clear_pending_dimm_unplug_state(spapr, drc->dev);
+        spapr_memory_unplug_rollback(spapr, drc->dev);
     }
 
     if (!drc->fdt) {
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index d6edeaaaff..47cebaf3ac 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -847,8 +847,7 @@ int spapr_hpt_shift_for_ramsize(uint64_t ramsize);
 int spapr_reallocate_hpt(SpaprMachineState *spapr, int shift, Error **errp);
 void spapr_clear_pending_events(SpaprMachineState *spapr);
 void spapr_clear_pending_hotplug_events(SpaprMachineState *spapr);
-void spapr_clear_pending_dimm_unplug_state(SpaprMachineState *spapr,
-                                           DeviceState *dev);
+void spapr_memory_unplug_rollback(SpaprMachineState *spapr, DeviceState *dev);
 int spapr_max_server_number(SpaprMachineState *spapr);
 void spapr_store_hpte(PowerPCCPU *cpu, hwaddr ptex,
                       uint64_t pte0, uint64_t pte1);
-- 
2.29.2

Re: [PATCH 1/5] spapr.c: assert first DRC LMB earlier in spapr_memory_unplug_request()

[Greg Kurz]

On Fri, 26 Feb 2021 13:32:57 -0300
Daniel Henrique Barboza <danielhb413@gmail.com> wrote:

> We are asserting the existence of the first DRC LMB after sending unplug
> requests to all LMBs of the DIMM, where every DRC is being asserted
> inside the loop. This means that the first DRC is being asserted twice.
> 
> We will use the first DRC to simplify the code a bit in the next patch,
> so instead of removing the duplicated assert, let's do it earlier.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  hw/ppc/spapr.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 6eaddb12cb..74e046b522 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3664,7 +3664,7 @@ static void spapr_memory_unplug_request(HotplugHandler *hotplug_dev,
>      uint32_t nr_lmbs;
>      uint64_t size, addr_start, addr;
>      int i;
> -    SpaprDrc *drc;
> +    SpaprDrc *drc, *drc_start;
>  
>      if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
>          error_setg(errp, "nvdimm device hot unplug is not supported yet.");
> @@ -3677,6 +3677,10 @@ static void spapr_memory_unplug_request(HotplugHandler *hotplug_dev,
>      addr_start = object_property_get_uint(OBJECT(dimm), PC_DIMM_ADDR_PROP,
>                                            &error_abort);
>  
> +    drc_start = spapr_drc_by_id(TYPE_SPAPR_DRC_LMB,
> +                                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
> @@ -3701,11 +3705,9 @@ static void spapr_memory_unplug_request(HotplugHandler *hotplug_dev,
>          addr += SPAPR_MEMORY_BLOCK_SIZE;
>      }
>  
> -    drc = spapr_drc_by_id(TYPE_SPAPR_DRC_LMB,
> -                          addr_start / SPAPR_MEMORY_BLOCK_SIZE);
> -    g_assert(drc);
>      spapr_hotplug_req_remove_by_count_indexed(SPAPR_DR_CONNECTOR_TYPE_LMB,
> -                                              nr_lmbs, spapr_drc_index(drc));
> +                                              nr_lmbs,
> +                                              spapr_drc_index(drc_start));
>  }
>  
>  /* Callback to be called during DRC release. */

Re: [PATCH 2/5] spapr.c: check unplug_request flag in spapr_memory_unplug_request()

[Greg Kurz]

On Fri, 26 Feb 2021 13:32:58 -0300
Daniel Henrique Barboza <danielhb413@gmail.com> 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>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  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;

Re: [PATCH 3/5] spapr.c: add 'unplug already in progress' message for PHB unplug

[Greg Kurz]

On Fri, 26 Feb 2021 13:32:59 -0300
Daniel Henrique Barboza <danielhb413@gmail.com> wrote:

> Both CPU hotunplug and PC_DIMM unplug reports an user warning,
> mentioning that the hotunplug is in progress, if consecutive
> 'device_del' are issued in quick succession.
> 
> Do the same for PHBs in spapr_phb_unplug_request().
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  hw/ppc/spapr.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 149dc2113f..6ef72ee7bd 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -4030,6 +4030,10 @@ static void spapr_phb_unplug_request(HotplugHandler *hotplug_dev,
>      if (!spapr_drc_unplug_requested(drc)) {
>          spapr_drc_unplug_request(drc);
>          spapr_hotplug_req_remove_by_index(drc);
> +    } else {
> +        error_setg(errp,
> +                   "PCI Host Bridge unplug already in progress for device %s",
> +                   dev->id);
>      }
>  }
>  

Re: [PATCH 4/5] spapr_pci.c: add 'unplug already in progress' message for PCI unplug

[Greg Kurz]

On Fri, 26 Feb 2021 13:33:00 -0300
Daniel Henrique Barboza <danielhb413@gmail.com> wrote:

> Hotunplug for all other devices are warning the user when the hotunplug
> is already in progress. Do the same for PCI devices in
> spapr_pci_unplug_request().
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  hw/ppc/spapr_pci.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index b00e9609ae..feba18cb12 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1743,6 +1743,10 @@ static void spapr_pci_unplug_request(HotplugHandler *plug_handler,
>                  }
>              }
>          }
> +    } else {
> +        error_setg(errp,
> +                   "PCI device unplug already in progress for device %s",
> +                   drc->dev->id);
>      }
>  }
>  

Re: [PATCH 5/5] spapr.c: send QAPI event when memory hotunplug fails

[Greg Kurz]

On Fri, 26 Feb 2021 13:33:01 -0300
Daniel Henrique Barboza <danielhb413@gmail.com> wrote:

> Recent changes allowed the pSeries machine to rollback the hotunplug
> process for the DIMM when the guest kernel signals, via a
> reconfiguration of the DR connector, that it's not going to release the
> LMBs.
> 
> Let's also warn QAPI listerners about it. One place to do it would be
> right after the unplug state is cleaned up,
> spapr_clear_pending_dimm_unplug_state(). This would mean that the
> function is now doing more than cleaning up the pending dimm state
> though.
> 
> This patch does the following changes in spapr.c:
> 
> - send a QAPI event to inform that we experienced a failure in the
>   hotunplug of the DIMM;
> 
> - rename spapr_clear_pending_dimm_unplug_state() to
>   spapr_memory_unplug_rollback(). This is a better fit for what the
>   function is now doing, and it makes callers care more about what the
>   function goal is and less about spapr.c internals such as clearing
>   the pending dimm unplug state.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---

LGTM

Reviewed-by: Greg Kurz <groug@kaod.org>

>  hw/ppc/spapr.c         | 13 +++++++++++--
>  hw/ppc/spapr_drc.c     |  5 ++---
>  include/hw/ppc/spapr.h |  3 +--
>  3 files changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 6ef72ee7bd..cbe5cafb14 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -28,6 +28,7 @@
>  #include "qemu-common.h"
>  #include "qemu/datadir.h"
>  #include "qapi/error.h"
> +#include "qapi/qapi-events-machine.h"
>  #include "qapi/visitor.h"
>  #include "sysemu/sysemu.h"
>  #include "sysemu/hostmem.h"
> @@ -3575,14 +3576,14 @@ static SpaprDimmState *spapr_recover_pending_dimm_state(SpaprMachineState *ms,
>      return spapr_pending_dimm_unplugs_add(ms, avail_lmbs, dimm);
>  }
>  
> -void spapr_clear_pending_dimm_unplug_state(SpaprMachineState *spapr,
> -                                           DeviceState *dev)
> +void spapr_memory_unplug_rollback(SpaprMachineState *spapr, DeviceState *dev)
>  {
>      SpaprDimmState *ds;
>      PCDIMMDevice *dimm;
>      SpaprDrc *drc;
>      uint32_t nr_lmbs;
>      uint64_t size, addr_start, addr;
> +    g_autofree char *qapi_error = NULL;
>      int i;
>  
>      if (!dev) {
> @@ -3616,6 +3617,14 @@ void spapr_clear_pending_dimm_unplug_state(SpaprMachineState *spapr,
>          drc->unplug_requested = false;
>          addr += SPAPR_MEMORY_BLOCK_SIZE;
>      }
> +
> +    /*
> +     * Tell QAPI that something happened and the memory
> +     * hotunplug wasn't successful.
> +     */
> +    qapi_error = g_strdup_printf("Memory hotunplug failed for device %s",
> +                                 dev->id);
> +    qapi_event_send_mem_unplug_error(dev->id, qapi_error);
>  }
>  
>  /* Callback to be called during DRC release. */
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 8c4997d795..8faaf9f1dd 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -1232,12 +1232,11 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
>  
>      /*
>       * This indicates that the kernel is reconfiguring a LMB due to
> -     * a failed hotunplug. Clear the pending unplug state for the whole
> -     * DIMM.
> +     * a failed hotunplug. Rollback the DIMM unplug process.
>       */
>      if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_LMB &&
>          drc->unplug_requested) {
> -        spapr_clear_pending_dimm_unplug_state(spapr, drc->dev);
> +        spapr_memory_unplug_rollback(spapr, drc->dev);
>      }
>  
>      if (!drc->fdt) {
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index d6edeaaaff..47cebaf3ac 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -847,8 +847,7 @@ int spapr_hpt_shift_for_ramsize(uint64_t ramsize);
>  int spapr_reallocate_hpt(SpaprMachineState *spapr, int shift, Error **errp);
>  void spapr_clear_pending_events(SpaprMachineState *spapr);
>  void spapr_clear_pending_hotplug_events(SpaprMachineState *spapr);
> -void spapr_clear_pending_dimm_unplug_state(SpaprMachineState *spapr,
> -                                           DeviceState *dev);
> +void spapr_memory_unplug_rollback(SpaprMachineState *spapr, DeviceState *dev);
>  int spapr_max_server_number(SpaprMachineState *spapr);
>  void spapr_store_hpte(PowerPCCPU *cpu, hwaddr ptex,
>                        uint64_t pte0, uint64_t pte1);

Re: [PATCH 2/5] spapr.c: check unplug_request flag in spapr_memory_unplug_request()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2758 bytes --]

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;

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/5] spapr.c: add 'unplug already in progress' message for PHB unplug

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1303 bytes --]

On Fri, Feb 26, 2021 at 01:32:59PM -0300, Daniel Henrique Barboza wrote:
> Both CPU hotunplug and PC_DIMM unplug reports an user warning,
> mentioning that the hotunplug is in progress, if consecutive
> 'device_del' are issued in quick succession.
> 
> Do the same for PHBs in spapr_phb_unplug_request().
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

LGTM, and doesn't have strong dependencies on the other patches, so
applied to ppc-for-6.0.

> ---
>  hw/ppc/spapr.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 149dc2113f..6ef72ee7bd 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -4030,6 +4030,10 @@ static void spapr_phb_unplug_request(HotplugHandler *hotplug_dev,
>      if (!spapr_drc_unplug_requested(drc)) {
>          spapr_drc_unplug_request(drc);
>          spapr_hotplug_req_remove_by_index(drc);
> +    } else {
> +        error_setg(errp,
> +                   "PCI Host Bridge unplug already in progress for device %s",
> +                   dev->id);
>      }
>  }
>  

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 4/5] spapr_pci.c: add 'unplug already in progress' message for PCI unplug

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1093 bytes --]

On Fri, Feb 26, 2021 at 01:33:00PM -0300, Daniel Henrique Barboza wrote:
> Hotunplug for all other devices are warning the user when the hotunplug
> is already in progress. Do the same for PCI devices in
> spapr_pci_unplug_request().
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

Applied to ppc-for-6.0.

> ---
>  hw/ppc/spapr_pci.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index b00e9609ae..feba18cb12 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1743,6 +1743,10 @@ static void spapr_pci_unplug_request(HotplugHandler *plug_handler,
>                  }
>              }
>          }
> +    } else {
> +        error_setg(errp,
> +                   "PCI device unplug already in progress for device %s",
> +                   drc->dev->id);
>      }
>  }
>  

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 5/5] spapr.c: send QAPI event when memory hotunplug fails

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4848 bytes --]

On Fri, Feb 26, 2021 at 01:33:01PM -0300, Daniel Henrique Barboza wrote:
> Recent changes allowed the pSeries machine to rollback the hotunplug
> process for the DIMM when the guest kernel signals, via a
> reconfiguration of the DR connector, that it's not going to release the
> LMBs.
> 
> Let's also warn QAPI listerners about it. One place to do it would be
> right after the unplug state is cleaned up,
> spapr_clear_pending_dimm_unplug_state(). This would mean that the
> function is now doing more than cleaning up the pending dimm state
> though.
> 
> This patch does the following changes in spapr.c:
> 
> - send a QAPI event to inform that we experienced a failure in the
>   hotunplug of the DIMM;
> 
> - rename spapr_clear_pending_dimm_unplug_state() to
>   spapr_memory_unplug_rollback(). This is a better fit for what the
>   function is now doing, and it makes callers care more about what the
>   function goal is and less about spapr.c internals such as clearing
>   the pending dimm unplug state.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>  hw/ppc/spapr.c         | 13 +++++++++++--
>  hw/ppc/spapr_drc.c     |  5 ++---
>  include/hw/ppc/spapr.h |  3 +--
>  3 files changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 6ef72ee7bd..cbe5cafb14 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -28,6 +28,7 @@
>  #include "qemu-common.h"
>  #include "qemu/datadir.h"
>  #include "qapi/error.h"
> +#include "qapi/qapi-events-machine.h"
>  #include "qapi/visitor.h"
>  #include "sysemu/sysemu.h"
>  #include "sysemu/hostmem.h"
> @@ -3575,14 +3576,14 @@ static SpaprDimmState *spapr_recover_pending_dimm_state(SpaprMachineState *ms,
>      return spapr_pending_dimm_unplugs_add(ms, avail_lmbs, dimm);
>  }
>  
> -void spapr_clear_pending_dimm_unplug_state(SpaprMachineState *spapr,
> -                                           DeviceState *dev)
> +void spapr_memory_unplug_rollback(SpaprMachineState *spapr, DeviceState *dev)
>  {
>      SpaprDimmState *ds;
>      PCDIMMDevice *dimm;
>      SpaprDrc *drc;
>      uint32_t nr_lmbs;
>      uint64_t size, addr_start, addr;
> +    g_autofree char *qapi_error = NULL;
>      int i;
>  
>      if (!dev) {
> @@ -3616,6 +3617,14 @@ void spapr_clear_pending_dimm_unplug_state(SpaprMachineState *spapr,
>          drc->unplug_requested = false;
>          addr += SPAPR_MEMORY_BLOCK_SIZE;
>      }
> +
> +    /*
> +     * Tell QAPI that something happened and the memory
> +     * hotunplug wasn't successful.
> +     */
> +    qapi_error = g_strdup_printf("Memory hotunplug failed for device %s",
> +                                 dev->id);

Might be worth adjusting the error message to make it clearer that it
was the guest which specifically rejected the unplug.  Other than
that, LGTM.

> +    qapi_event_send_mem_unplug_error(dev->id, qapi_error);
>  }
>  
>  /* Callback to be called during DRC release. */
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 8c4997d795..8faaf9f1dd 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -1232,12 +1232,11 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
>  
>      /*
>       * This indicates that the kernel is reconfiguring a LMB due to
> -     * a failed hotunplug. Clear the pending unplug state for the whole
> -     * DIMM.
> +     * a failed hotunplug. Rollback the DIMM unplug process.
>       */
>      if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_LMB &&
>          drc->unplug_requested) {
> -        spapr_clear_pending_dimm_unplug_state(spapr, drc->dev);
> +        spapr_memory_unplug_rollback(spapr, drc->dev);
>      }
>  
>      if (!drc->fdt) {
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index d6edeaaaff..47cebaf3ac 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -847,8 +847,7 @@ int spapr_hpt_shift_for_ramsize(uint64_t ramsize);
>  int spapr_reallocate_hpt(SpaprMachineState *spapr, int shift, Error **errp);
>  void spapr_clear_pending_events(SpaprMachineState *spapr);
>  void spapr_clear_pending_hotplug_events(SpaprMachineState *spapr);
> -void spapr_clear_pending_dimm_unplug_state(SpaprMachineState *spapr,
> -                                           DeviceState *dev);
> +void spapr_memory_unplug_rollback(SpaprMachineState *spapr, DeviceState *dev);
>  int spapr_max_server_number(SpaprMachineState *spapr);
>  void spapr_store_hpte(PowerPCCPU *cpu, hwaddr ptex,
>                        uint64_t pte0, uint64_t pte1);

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/5] spapr.c: check unplug_request flag in spapr_memory_unplug_request()

[Daniel Henrique Barboza]

On 3/1/21 11:03 PM, David Gibson wrote:
> 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.

Good point. Checking the existence of the sPAPRDIMMState struct at this
point is the same as checking for drc->unplug_requested for all the DRCs
of the DIMM.

I could check for drc->unplug_requested inside the loop where we instantiate
each DRC, but there is no gain in doing that instead of what we already have.

I'll drop this patch and change patch 1 to just remove the duplicated assert.


Thanks,


DHB

> 
>> ---
>>   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;
>