[REPOST] spapr: Allow memory unplug to always succeed

Commit Message

Greg Kurz Dec. 14, 2020, 10:05 a.m. UTC

It is currently impossible to hot-unplug a memory device between
machine reset and CAS.

(qemu) device_del dimm1
Error: Memory hot unplug not supported for this guest

This limitation was introduced in order to provide an explicit
error path for older guests that didn't support hot-plug event
sources (and thus memory hot-unplug).

The linux kernel has been supporting these since 4.11. All recent
enough guests are thus capable of handling the removal of a memory
device at all time, including during early boot.

Lift the limitation for the latest machine type. This means that
trying to unplug memory from a guest that doesn't support it will
likely just do nothing and the memory will only get removed at
next reboot. Such older guests can still get the existing behavior
by using an older machine type.

Signed-off-by: Greg Kurz <[e-mail]>
---
This patch was initially posted before the compat machine types
for 6.0 got merged upstream. During the same period, David was
also transitioning from github to gitlab, and the patch didn't
applied cleanly. Now it does. Just reposting it unchanged for
convenience.
---
 hw/ppc/spapr.c         |    6 +++++-
 hw/ppc/spapr_events.c  |    3 ++-
 include/hw/ppc/spapr.h |    1 +
 3 files changed, 8 insertions(+), 2 deletions(-)

Comments

David Gibson Dec. 16, 2020, 5:45 a.m. UTC | #1

On Mon, Dec 14, 2020 at 11:05:50AM +0100, Greg Kurz wrote:
65;6201;1c> It is currently impossible to hot-unplug a memory device between
> machine reset and CAS.
> 
> (qemu) device_del dimm1
> Error: Memory hot unplug not supported for this guest
> 
> This limitation was introduced in order to provide an explicit
> error path for older guests that didn't support hot-plug event
> sources (and thus memory hot-unplug).
> 
> The linux kernel has been supporting these since 4.11. All recent
> enough guests are thus capable of handling the removal of a memory
> device at all time, including during early boot.
> 
> Lift the limitation for the latest machine type. This means that
> trying to unplug memory from a guest that doesn't support it will
> likely just do nothing and the memory will only get removed at
> next reboot. Such older guests can still get the existing behavior
> by using an older machine type.
> 
> Signed-off-by: Greg Kurz <[e-mail]>
> ---
> This patch was initially posted before the compat machine types
> for 6.0 got merged upstream. During the same period, David was
> also transitioning from github to gitlab, and the patch didn't
> applied cleanly. Now it does. Just reposting it unchanged for
> convenience.

Applied to ppc-for-6.0, thanks.

> ---
>  hw/ppc/spapr.c         |    6 +++++-
>  hw/ppc/spapr_events.c  |    3 ++-
>  include/hw/ppc/spapr.h |    1 +
>  3 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index dee48a0043bb..481c800a5a33 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -4051,7 +4051,8 @@ static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,
>      SpaprMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
>  
>      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> -        if (spapr_ovec_test(sms->ov5_cas, OV5_HP_EVT)) {
> +        if (!smc->pre_6_0_memory_unplug ||
> +            spapr_ovec_test(sms->ov5_cas, OV5_HP_EVT)) {
>              spapr_memory_unplug_request(hotplug_dev, dev, errp);
>          } else {
>              /* NOTE: this means there is a window after guest reset, prior to
> @@ -4537,8 +4538,11 @@ DEFINE_SPAPR_MACHINE(6_0, "6.0", true);
>   */
>  static void spapr_machine_5_2_class_options(MachineClass *mc)
>  {
> +    SpaprMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
> +
>      spapr_machine_6_0_class_options(mc);
>      compat_props_add(mc->compat_props, hw_compat_5_2, hw_compat_5_2_len);
> +    smc->pre_6_0_memory_unplug = true;
>  }
>  
>  DEFINE_SPAPR_MACHINE(5_2, "5.2", false);
> diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
> index 3f37b49fd8ad..6aedd988b3d0 100644
> --- a/hw/ppc/spapr_events.c
> +++ b/hw/ppc/spapr_events.c
> @@ -658,7 +658,8 @@ static void spapr_hotplug_req_event(uint8_t hp_id, uint8_t hp_action,
>          /* we should not be using count_indexed value unless the guest
>           * supports dedicated hotplug event source
>           */
> -        g_assert(spapr_ovec_test(spapr->ov5_cas, OV5_HP_EVT));
> +        g_assert(!SPAPR_MACHINE_GET_CLASS(spapr)->pre_6_0_memory_unplug ||
> +                 spapr_ovec_test(spapr->ov5_cas, OV5_HP_EVT));
>          hp->drc_id.count_indexed.count =
>              cpu_to_be32(drc_id->count_indexed.count);
>          hp->drc_id.count_indexed.index =
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index e0f10f252c08..06a5b4259f20 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -139,6 +139,7 @@ struct SpaprMachineClass {
>      hwaddr rma_limit;          /* clamp the RMA to this size */
>      bool pre_5_1_assoc_refpoints;
>      bool pre_5_2_numa_associativity;
> +    bool pre_6_0_memory_unplug;
>  
>      bool (*phb_placement)(SpaprMachineState *spapr, uint32_t index,
>                            uint64_t *buid, hwaddr *pio, 
> 
>

Patch

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index dee48a0043bb..481c800a5a33 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -4051,7 +4051,8 @@  static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,
     SpaprMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
 
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
-        if (spapr_ovec_test(sms->ov5_cas, OV5_HP_EVT)) {
+        if (!smc->pre_6_0_memory_unplug ||
+            spapr_ovec_test(sms->ov5_cas, OV5_HP_EVT)) {
             spapr_memory_unplug_request(hotplug_dev, dev, errp);
         } else {
             /* NOTE: this means there is a window after guest reset, prior to
@@ -4537,8 +4538,11 @@  DEFINE_SPAPR_MACHINE(6_0, "6.0", true);
  */
 static void spapr_machine_5_2_class_options(MachineClass *mc)
 {
+    SpaprMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
+
     spapr_machine_6_0_class_options(mc);
     compat_props_add(mc->compat_props, hw_compat_5_2, hw_compat_5_2_len);
+    smc->pre_6_0_memory_unplug = true;
 }
 
 DEFINE_SPAPR_MACHINE(5_2, "5.2", false);
diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
index 3f37b49fd8ad..6aedd988b3d0 100644
--- a/hw/ppc/spapr_events.c
+++ b/hw/ppc/spapr_events.c
@@ -658,7 +658,8 @@  static void spapr_hotplug_req_event(uint8_t hp_id, uint8_t hp_action,
         /* we should not be using count_indexed value unless the guest
          * supports dedicated hotplug event source
          */
-        g_assert(spapr_ovec_test(spapr->ov5_cas, OV5_HP_EVT));
+        g_assert(!SPAPR_MACHINE_GET_CLASS(spapr)->pre_6_0_memory_unplug ||
+                 spapr_ovec_test(spapr->ov5_cas, OV5_HP_EVT));
         hp->drc_id.count_indexed.count =
             cpu_to_be32(drc_id->count_indexed.count);
         hp->drc_id.count_indexed.index =
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index e0f10f252c08..06a5b4259f20 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -139,6 +139,7 @@  struct SpaprMachineClass {
     hwaddr rma_limit;          /* clamp the RMA to this size */
     bool pre_5_1_assoc_refpoints;
     bool pre_5_2_numa_associativity;
+    bool pre_6_0_memory_unplug;
 
     bool (*phb_placement)(SpaprMachineState *spapr, uint32_t index,
                           uint64_t *buid, hwaddr *pio,