[PATCH] spapr: Handle pending hot plug/unplug requests at CAS

[PATCH] spapr: Handle pending hot plug/unplug requests at CAS

[Greg Kurz]

If a hot plug or unplug request is pending at CAS, we currently trigger
a CAS reboot, which severely increases the guest boot time. This is
because SLOF doesn't handle hot plug events and we had no way to fix
the FDT that gets presented to the guest.

We can do better thanks to recent changes in QEMU and SLOF:

- we now return a full FDT to SLOF during CAS

- SLOF was fixed to correctly detect any device that was either added or
  removed since boot time and to update its internal DT accordingly.

The right solution is to process all pending hot plug/unplug requests
during CAS: convert hot plugged devices to cold plugged devices and
remove the hot unplugged ones, which is exactly what spapr_drc_reset()
does. Also clear all hot plug events that are currently queued since
they're no longer relevant.

Note that SLOF cannot currently populate hot plugged PCI bridges or PHBs
at CAS. Until this limitation is lifted, SLOF will reset the machine when
this scenario occurs : this will allow the FDT to be fully processed when
SLOF is started again (ie. the same effect as the CAS reboot that would
occur anyway without this patch).

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr_events.c  |   13 +++++++++++++
 hw/ppc/spapr_hcall.c   |   11 +++++------
 include/hw/ppc/spapr.h |    1 +
 3 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
index 8b32b7eea526..2afd1844e4d4 100644
--- a/hw/ppc/spapr_events.c
+++ b/hw/ppc/spapr_events.c
@@ -983,6 +983,19 @@ void spapr_clear_pending_events(SpaprMachineState *spapr)
     }
 }
 
+void spapr_clear_pending_hotplug_events(SpaprMachineState *spapr)
+{
+    SpaprEventLogEntry *entry = NULL, *next_entry;
+
+    QTAILQ_FOREACH_SAFE(entry, &spapr->pending_events, next, next_entry) {
+        if (spapr_event_log_entry_type(entry) == RTAS_LOG_TYPE_HOTPLUG) {
+            QTAILQ_REMOVE(&spapr->pending_events, entry, next);
+            g_free(entry->extended_log);
+            g_free(entry);
+        }
+    }
+}
+
 void spapr_events_init(SpaprMachineState *spapr)
 {
     int epow_irq = SPAPR_IRQ_EPOW;
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 6db3dbde9c92..5992849c1664 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1640,7 +1640,7 @@ static uint32_t cas_check_pvr(SpaprMachineState *spapr, PowerPCCPU *cpu,
     return best_compat;
 }
 
-static bool spapr_transient_dev_before_cas(void)
+static void spapr_handle_transient_dev_before_cas(SpaprMachineState *spapr)
 {
     Object *drc_container;
     ObjectProperty *prop;
@@ -1658,10 +1658,11 @@ static bool spapr_transient_dev_before_cas(void)
                                                           prop->name, NULL));
 
         if (spapr_drc_transient(drc)) {
-            return true;
+            spapr_drc_reset(drc);
         }
     }
-    return false;
+
+    spapr_clear_pending_hotplug_events(spapr);
 }
 
 static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
@@ -1834,9 +1835,7 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
 
     spapr_irq_update_active_intc(spapr);
 
-    if (spapr_transient_dev_before_cas()) {
-        spapr->cas_reboot = true;
-    }
+    spapr_handle_transient_dev_before_cas(spapr);
 
     if (!spapr->cas_reboot) {
         void *fdt;
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 09110961a589..a4216935a148 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -824,6 +824,7 @@ int spapr_hpt_shift_for_ramsize(uint64_t ramsize);
 void spapr_reallocate_hpt(SpaprMachineState *spapr, int shift,
                           Error **errp);
 void spapr_clear_pending_events(SpaprMachineState *spapr);
+void spapr_clear_pending_hotplug_events(SpaprMachineState *spapr);
 int spapr_max_server_number(SpaprMachineState *spapr);
 void spapr_store_hpte(PowerPCCPU *cpu, hwaddr ptex,
                       uint64_t pte0, uint64_t pte1);

Re: [PATCH] spapr: Handle pending hot plug/unplug requests at CAS

[David Gibson]

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

On Mon, Feb 24, 2020 at 08:23:43PM +0100, Greg Kurz wrote:
> If a hot plug or unplug request is pending at CAS, we currently trigger
> a CAS reboot, which severely increases the guest boot time. This is
> because SLOF doesn't handle hot plug events and we had no way to fix
> the FDT that gets presented to the guest.
> 
> We can do better thanks to recent changes in QEMU and SLOF:
> 
> - we now return a full FDT to SLOF during CAS
> 
> - SLOF was fixed to correctly detect any device that was either added or
>   removed since boot time and to update its internal DT accordingly.
> 
> The right solution is to process all pending hot plug/unplug requests
> during CAS: convert hot plugged devices to cold plugged devices and
> remove the hot unplugged ones, which is exactly what spapr_drc_reset()
> does. Also clear all hot plug events that are currently queued since
> they're no longer relevant.
> 
> Note that SLOF cannot currently populate hot plugged PCI bridges or PHBs
> at CAS. Until this limitation is lifted, SLOF will reset the machine when
> this scenario occurs : this will allow the FDT to be fully processed when
> SLOF is started again (ie. the same effect as the CAS reboot that would
> occur anyway without this patch).
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

LGTM, applied to ppc-for-5.0.

> ---
>  hw/ppc/spapr_events.c  |   13 +++++++++++++
>  hw/ppc/spapr_hcall.c   |   11 +++++------
>  include/hw/ppc/spapr.h |    1 +
>  3 files changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
> index 8b32b7eea526..2afd1844e4d4 100644
> --- a/hw/ppc/spapr_events.c
> +++ b/hw/ppc/spapr_events.c
> @@ -983,6 +983,19 @@ void spapr_clear_pending_events(SpaprMachineState *spapr)
>      }
>  }
>  
> +void spapr_clear_pending_hotplug_events(SpaprMachineState *spapr)
> +{
> +    SpaprEventLogEntry *entry = NULL, *next_entry;
> +
> +    QTAILQ_FOREACH_SAFE(entry, &spapr->pending_events, next, next_entry) {
> +        if (spapr_event_log_entry_type(entry) == RTAS_LOG_TYPE_HOTPLUG) {
> +            QTAILQ_REMOVE(&spapr->pending_events, entry, next);
> +            g_free(entry->extended_log);
> +            g_free(entry);
> +        }
> +    }
> +}
> +
>  void spapr_events_init(SpaprMachineState *spapr)
>  {
>      int epow_irq = SPAPR_IRQ_EPOW;
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 6db3dbde9c92..5992849c1664 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1640,7 +1640,7 @@ static uint32_t cas_check_pvr(SpaprMachineState *spapr, PowerPCCPU *cpu,
>      return best_compat;
>  }
>  
> -static bool spapr_transient_dev_before_cas(void)
> +static void spapr_handle_transient_dev_before_cas(SpaprMachineState *spapr)
>  {
>      Object *drc_container;
>      ObjectProperty *prop;
> @@ -1658,10 +1658,11 @@ static bool spapr_transient_dev_before_cas(void)
>                                                            prop->name, NULL));
>  
>          if (spapr_drc_transient(drc)) {
> -            return true;
> +            spapr_drc_reset(drc);
>          }
>      }
> -    return false;
> +
> +    spapr_clear_pending_hotplug_events(spapr);
>  }
>  
>  static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
> @@ -1834,9 +1835,7 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
>  
>      spapr_irq_update_active_intc(spapr);
>  
> -    if (spapr_transient_dev_before_cas()) {
> -        spapr->cas_reboot = true;
> -    }
> +    spapr_handle_transient_dev_before_cas(spapr);
>  
>      if (!spapr->cas_reboot) {
>          void *fdt;
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 09110961a589..a4216935a148 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -824,6 +824,7 @@ int spapr_hpt_shift_for_ramsize(uint64_t ramsize);
>  void spapr_reallocate_hpt(SpaprMachineState *spapr, int shift,
>                            Error **errp);
>  void spapr_clear_pending_events(SpaprMachineState *spapr);
> +void spapr_clear_pending_hotplug_events(SpaprMachineState *spapr);
>  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 --]