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

[David Gibson]

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 <address@hidden>

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

signature.asc
Description: PGP signature