A recent change in spapr_machine_reset() showed that resetting the compat
mode in spapr_machine_reset() for the boot vCPU and in spapr_cpu_reset()
for all other vCPUs was fragile. The fix was thus to reset the compat mode
for all vCPUs in spapr_machine_reset(), but we still have to propagate
it to hot-plugged CPUs. This is still performed from spapr_cpu_reset(),
hence resulting in ppc_set_compat() being called twice for every vCPU at
machine reset. Apart from wasting cycles, which isn't really an issue
during machine reset, this seems to indicate that spapr_cpu_reset() isn't
the best place to set the compat mode.

A natural candidate for CPU-hotplug specific code is spapr_core_plug().
Also, it sits in the same file as spapr_machine_reset() : this makes
it easier for someone who wants to know when the compat PVR is set.

Call ppc_set_compat() from there. This doesn't need to be done for
initial vCPUs since the compat PVR is 0 and spapr_machine_reset() sets
the appropriate value later. No need to do this on manually added vCPUS
on the destination QEMU during migration since the compat PVR is
part of the migrated vCPU state. Both conditions can be checked with
spapr_drc_hotplugged().

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr.c          |   24 ++++++++++++++++--------
 hw/ppc/spapr_cpu_core.c |    7 -------
 2 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 447fb5c4eaaf..ea56499b4b3e 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1758,12 +1758,6 @@ static void spapr_machine_reset(MachineState *machine)
         spapr_ovec_cleanup(spapr->ov5_cas);
         spapr->ov5_cas = spapr_ovec_new();
 
-        /*
-         * reset compat_pvr for all CPUs
-         * as qemu_devices_reset() is called before this,
-         * it can't be propagated by spapr_cpu_reset()
-         * from the first CPU to all the others
-         */
         ppc_set_compat_all(spapr->max_compat_pvr, &error_fatal);
     }
 
@@ -3841,6 +3835,7 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
     CPUArchId *core_slot;
     int index;
     bool hotplugged = spapr_drc_hotplugged(dev);
+    int i;
 
     core_slot = spapr_find_cpu_slot(MACHINE(hotplug_dev), cc->core_id, &index);
     if (!core_slot) {
@@ -3874,13 +3869,26 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
     core_slot->cpu = OBJECT(dev);
 
     if (smc->pre_2_10_has_unused_icps) {
-        int i;
-
         for (i = 0; i < cc->nr_threads; i++) {
             cs = CPU(core->threads[i]);
             pre_2_10_vmstate_unregister_dummy_icp(cs->cpu_index);
         }
     }
+
+    /*
+     * Set compatibility mode to match the boot CPU, which was either set
+     * by the machine reset code or by CAS.
+     */
+    if (hotplugged) {
+        for (i = 0; i < cc->nr_threads; i++) {
+            ppc_set_compat(core->threads[i], POWERPC_CPU(first_cpu)->compat_pvr,
+                           &local_err);
+            if (local_err) {
+                error_propagate(errp, local_err);
+                return;
+            }
+        }
+    }
 }
 
 static void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 45e2f2747ffc..1d93de8161f3 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -41,13 +41,6 @@ static void spapr_cpu_reset(void *opaque)
      * using an RTAS call */
     cs->halted = 1;
 
-    /* Set compatibility mode to match the boot CPU, which was either set
-     * by the machine reset code or by CAS. This should never fail.
-     * At startup the value is already set for all the CPUs
-     * but we need this when we hotplug a new CPU
-     */
-    ppc_set_compat(cpu, POWERPC_CPU(first_cpu)->compat_pvr, &error_abort);
-
     env->spr[SPR_HIOR] = 0;
 
     lpcr = env->spr[SPR_LPCR];

On Wed, Aug 28, 2019 at 07:20:53PM +0200, Greg Kurz wrote:
> A recent change in spapr_machine_reset() showed that resetting the compat
> mode in spapr_machine_reset() for the boot vCPU and in spapr_cpu_reset()
> for all other vCPUs was fragile. The fix was thus to reset the compat mode
> for all vCPUs in spapr_machine_reset(), but we still have to propagate
> it to hot-plugged CPUs. This is still performed from spapr_cpu_reset(),
> hence resulting in ppc_set_compat() being called twice for every vCPU at
> machine reset. Apart from wasting cycles, which isn't really an issue
> during machine reset, this seems to indicate that spapr_cpu_reset() isn't
> the best place to set the compat mode.
> 
> A natural candidate for CPU-hotplug specific code is spapr_core_plug().
> Also, it sits in the same file as spapr_machine_reset() : this makes
> it easier for someone who wants to know when the compat PVR is set.
> 
> Call ppc_set_compat() from there. This doesn't need to be done for
> initial vCPUs since the compat PVR is 0 and spapr_machine_reset() sets
> the appropriate value later. No need to do this on manually added vCPUS
> on the destination QEMU during migration since the compat PVR is
> part of the migrated vCPU state. Both conditions can be checked with
> spapr_drc_hotplugged().
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Applied, thanks.

> ---
>  hw/ppc/spapr.c          |   24 ++++++++++++++++--------
>  hw/ppc/spapr_cpu_core.c |    7 -------
>  2 files changed, 16 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 447fb5c4eaaf..ea56499b4b3e 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1758,12 +1758,6 @@ static void spapr_machine_reset(MachineState *machine)
>          spapr_ovec_cleanup(spapr->ov5_cas);
>          spapr->ov5_cas = spapr_ovec_new();
>  
> -        /*
> -         * reset compat_pvr for all CPUs
> -         * as qemu_devices_reset() is called before this,
> -         * it can't be propagated by spapr_cpu_reset()
> -         * from the first CPU to all the others
> -         */
>          ppc_set_compat_all(spapr->max_compat_pvr, &error_fatal);
>      }
>  
> @@ -3841,6 +3835,7 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>      CPUArchId *core_slot;
>      int index;
>      bool hotplugged = spapr_drc_hotplugged(dev);
> +    int i;
>  
>      core_slot = spapr_find_cpu_slot(MACHINE(hotplug_dev), cc->core_id, &index);
>      if (!core_slot) {
> @@ -3874,13 +3869,26 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>      core_slot->cpu = OBJECT(dev);
>  
>      if (smc->pre_2_10_has_unused_icps) {
> -        int i;
> -
>          for (i = 0; i < cc->nr_threads; i++) {
>              cs = CPU(core->threads[i]);
>              pre_2_10_vmstate_unregister_dummy_icp(cs->cpu_index);
>          }
>      }
> +
> +    /*
> +     * Set compatibility mode to match the boot CPU, which was either set
> +     * by the machine reset code or by CAS.
> +     */
> +    if (hotplugged) {
> +        for (i = 0; i < cc->nr_threads; i++) {
> +            ppc_set_compat(core->threads[i], POWERPC_CPU(first_cpu)->compat_pvr,
> +                           &local_err);
> +            if (local_err) {
> +                error_propagate(errp, local_err);
> +                return;
> +            }
> +        }
> +    }
>  }
>  
>  static void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 45e2f2747ffc..1d93de8161f3 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -41,13 +41,6 @@ static void spapr_cpu_reset(void *opaque)
>       * using an RTAS call */
>      cs->halted = 1;
>  
> -    /* Set compatibility mode to match the boot CPU, which was either set
> -     * by the machine reset code or by CAS. This should never fail.
> -     * At startup the value is already set for all the CPUs
> -     * but we need this when we hotplug a new CPU
> -     */
> -    ppc_set_compat(cpu, POWERPC_CPU(first_cpu)->compat_pvr, &error_abort);
> -
>      env->spr[SPR_HIOR] = 0;
>  
>      lpcr = env->spr[SPR_LPCR];
> 

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