diff mbox series

[for-6.0,4/9] spapr: Set compat mode in spapr_reset_vcpu()

Message ID 20201120234208.683521-5-groug@kaod.org
State New
Headers show
Series spapr: Perform hotplug sanity checks at pre-plug | expand

Commit Message

Greg Kurz Nov. 20, 2020, 11:42 p.m. UTC
When it comes to resetting the compat mode of the vCPUS, there are
two situations to consider:
(1) machine reset should set the compat mode back to the machine default,
    ie. spapr->max_compat_pvr
(2) hot plugged vCPUs should set their compat mode to mach the boot vCPU,
    ie. POWERPC_CPU(first_cpu)->compat_pvr

This is currently handled in two separate places: globally for all vCPUs
from the machine reset code for (1) and for each thread of a core from
the hotplug path for (2).

Since the machine reset code already resets all vCPUs, starting with boot
vCPU, consolidate the logic in spapr_reset_vcpu(). Special case the boot
vCPU so that it resets its compat mode back to the machine default. Any
other vCPU just need to match the compat mode of the boot vCPU.

Failing to set the compat mode during machine reset is a fatal error,
but not for hot plugged vCPUs. This is arguable because if we've been
able to set the boot vCPU compat mode at CAS or during machine reset,
it should definitely not fail for other vCPUs. Since spapr_reset_vcpu()
already has a fatal error path for kvm_check_mmu() failures, do the
same for ppc_set_compat().

This gets rid of an error path in spapr_core_plug(). It will allow
further simplifications.

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

Comments

David Gibson Nov. 23, 2020, 5:11 a.m. UTC | #1
On Sat, Nov 21, 2020 at 12:42:03AM +0100, Greg Kurz wrote:
> When it comes to resetting the compat mode of the vCPUS, there are
> two situations to consider:
> (1) machine reset should set the compat mode back to the machine default,
>     ie. spapr->max_compat_pvr
> (2) hot plugged vCPUs should set their compat mode to mach the boot vCPU,
>     ie. POWERPC_CPU(first_cpu)->compat_pvr
> 
> This is currently handled in two separate places: globally for all vCPUs
> from the machine reset code for (1) and for each thread of a core from
> the hotplug path for (2).
> 
> Since the machine reset code already resets all vCPUs, starting with boot
> vCPU, consolidate the logic in spapr_reset_vcpu(). Special case the boot
> vCPU so that it resets its compat mode back to the machine default. Any
> other vCPU just need to match the compat mode of the boot vCPU.
> 
> Failing to set the compat mode during machine reset is a fatal error,
> but not for hot plugged vCPUs. This is arguable because if we've been
> able to set the boot vCPU compat mode at CAS or during machine reset,
> it should definitely not fail for other vCPUs. Since spapr_reset_vcpu()
> already has a fatal error path for kvm_check_mmu() failures, do the
> same for ppc_set_compat().
> 
> This gets rid of an error path in spapr_core_plug(). It will allow
> further simplifications.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/ppc/spapr.c          | 16 ----------------
>  hw/ppc/spapr_cpu_core.c | 13 +++++++++++++
>  2 files changed, 13 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index f58f77389e8e..da7586f548df 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1606,8 +1606,6 @@ static void spapr_machine_reset(MachineState *machine)
>      spapr_ovec_cleanup(spapr->ov5_cas);
>      spapr->ov5_cas = spapr_ovec_new();
>  
> -    ppc_set_compat_all(spapr->max_compat_pvr, &error_fatal);
> -
>      /*
>       * This is fixing some of the default configuration of the XIVE
>       * devices. To be called after the reset of the machine devices.
> @@ -3785,20 +3783,6 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>  
>      core_slot->cpu = OBJECT(dev);
>  
> -    /*
> -     * 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++) {
> -            if (ppc_set_compat(core->threads[i],
> -                               POWERPC_CPU(first_cpu)->compat_pvr,
> -                               errp) < 0) {
> -                return;
> -            }
> -        }
> -    }
> -
>      if (smc->pre_2_10_has_unused_icps) {
>          for (i = 0; i < cc->nr_threads; i++) {
>              cs = CPU(core->threads[i]);
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 2f7dc3c23ded..17741a3fb77f 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -27,6 +27,7 @@
>  
>  static void spapr_reset_vcpu(PowerPCCPU *cpu)
>  {
> +    PowerPCCPU *first_ppc_cpu = POWERPC_CPU(first_cpu);
>      CPUState *cs = CPU(cpu);
>      CPUPPCState *env = &cpu->env;
>      PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> @@ -69,6 +70,18 @@ static void spapr_reset_vcpu(PowerPCCPU *cpu)
>      kvm_check_mmu(cpu, &error_fatal);
>  
>      spapr_irq_cpu_intc_reset(spapr, cpu);
> +
> +    /*
> +     * The boot CPU is only reset during machine reset : reset its
> +     * compatibility mode to the machine default. For other CPUs,
> +     * either cold plugged or hot plugged, set the compatibility mode
> +     * to match the boot CPU, which was either set by the machine reset
> +     * code or by CAS.
> +     */
> +    ppc_set_compat(cpu,
> +                   cpu == first_ppc_cpu ?
> +                   spapr->max_compat_pvr : first_ppc_cpu->compat_pvr,
> +                   &error_fatal);

This assumes that when it is called for a non-boot CPU, it has already
been called for the boot CPU..  Are we certain that's guaranteed by
the sequence of reset calls during a full machine reset?

>  }
>  
>  void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip,
Greg Kurz Nov. 23, 2020, 11:51 a.m. UTC | #2
On Mon, 23 Nov 2020 16:11:30 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Sat, Nov 21, 2020 at 12:42:03AM +0100, Greg Kurz wrote:
> > When it comes to resetting the compat mode of the vCPUS, there are
> > two situations to consider:
> > (1) machine reset should set the compat mode back to the machine default,
> >     ie. spapr->max_compat_pvr
> > (2) hot plugged vCPUs should set their compat mode to mach the boot vCPU,
> >     ie. POWERPC_CPU(first_cpu)->compat_pvr
> > 
> > This is currently handled in two separate places: globally for all vCPUs
> > from the machine reset code for (1) and for each thread of a core from
> > the hotplug path for (2).
> > 
> > Since the machine reset code already resets all vCPUs, starting with boot
> > vCPU, consolidate the logic in spapr_reset_vcpu(). Special case the boot
> > vCPU so that it resets its compat mode back to the machine default. Any
> > other vCPU just need to match the compat mode of the boot vCPU.
> > 
> > Failing to set the compat mode during machine reset is a fatal error,
> > but not for hot plugged vCPUs. This is arguable because if we've been
> > able to set the boot vCPU compat mode at CAS or during machine reset,
> > it should definitely not fail for other vCPUs. Since spapr_reset_vcpu()
> > already has a fatal error path for kvm_check_mmu() failures, do the
> > same for ppc_set_compat().
> > 
> > This gets rid of an error path in spapr_core_plug(). It will allow
> > further simplifications.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  hw/ppc/spapr.c          | 16 ----------------
> >  hw/ppc/spapr_cpu_core.c | 13 +++++++++++++
> >  2 files changed, 13 insertions(+), 16 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index f58f77389e8e..da7586f548df 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1606,8 +1606,6 @@ static void spapr_machine_reset(MachineState *machine)
> >      spapr_ovec_cleanup(spapr->ov5_cas);
> >      spapr->ov5_cas = spapr_ovec_new();
> >  
> > -    ppc_set_compat_all(spapr->max_compat_pvr, &error_fatal);
> > -
> >      /*
> >       * This is fixing some of the default configuration of the XIVE
> >       * devices. To be called after the reset of the machine devices.
> > @@ -3785,20 +3783,6 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >  
> >      core_slot->cpu = OBJECT(dev);
> >  
> > -    /*
> > -     * 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++) {
> > -            if (ppc_set_compat(core->threads[i],
> > -                               POWERPC_CPU(first_cpu)->compat_pvr,
> > -                               errp) < 0) {
> > -                return;
> > -            }
> > -        }
> > -    }
> > -
> >      if (smc->pre_2_10_has_unused_icps) {
> >          for (i = 0; i < cc->nr_threads; i++) {
> >              cs = CPU(core->threads[i]);
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > index 2f7dc3c23ded..17741a3fb77f 100644
> > --- a/hw/ppc/spapr_cpu_core.c
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -27,6 +27,7 @@
> >  
> >  static void spapr_reset_vcpu(PowerPCCPU *cpu)
> >  {
> > +    PowerPCCPU *first_ppc_cpu = POWERPC_CPU(first_cpu);
> >      CPUState *cs = CPU(cpu);
> >      CPUPPCState *env = &cpu->env;
> >      PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> > @@ -69,6 +70,18 @@ static void spapr_reset_vcpu(PowerPCCPU *cpu)
> >      kvm_check_mmu(cpu, &error_fatal);
> >  
> >      spapr_irq_cpu_intc_reset(spapr, cpu);
> > +
> > +    /*
> > +     * The boot CPU is only reset during machine reset : reset its
> > +     * compatibility mode to the machine default. For other CPUs,
> > +     * either cold plugged or hot plugged, set the compatibility mode
> > +     * to match the boot CPU, which was either set by the machine reset
> > +     * code or by CAS.
> > +     */
> > +    ppc_set_compat(cpu,
> > +                   cpu == first_ppc_cpu ?
> > +                   spapr->max_compat_pvr : first_ppc_cpu->compat_pvr,
> > +                   &error_fatal);
> 
> This assumes that when it is called for a non-boot CPU, it has already
> been called for the boot CPU..  Are we certain that's guaranteed by
> the sequence of reset calls during a full machine reset?
> 

This happens to be the case. Basically because the boot CPU core
is created (including registering its reset handler) first and
qemu_devices_reset() calls handlers in the same order they were
registered.

> >  }
> >  
> >  void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip,
>
diff mbox series

Patch

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index f58f77389e8e..da7586f548df 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1606,8 +1606,6 @@  static void spapr_machine_reset(MachineState *machine)
     spapr_ovec_cleanup(spapr->ov5_cas);
     spapr->ov5_cas = spapr_ovec_new();
 
-    ppc_set_compat_all(spapr->max_compat_pvr, &error_fatal);
-
     /*
      * This is fixing some of the default configuration of the XIVE
      * devices. To be called after the reset of the machine devices.
@@ -3785,20 +3783,6 @@  static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
 
     core_slot->cpu = OBJECT(dev);
 
-    /*
-     * 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++) {
-            if (ppc_set_compat(core->threads[i],
-                               POWERPC_CPU(first_cpu)->compat_pvr,
-                               errp) < 0) {
-                return;
-            }
-        }
-    }
-
     if (smc->pre_2_10_has_unused_icps) {
         for (i = 0; i < cc->nr_threads; i++) {
             cs = CPU(core->threads[i]);
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 2f7dc3c23ded..17741a3fb77f 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -27,6 +27,7 @@ 
 
 static void spapr_reset_vcpu(PowerPCCPU *cpu)
 {
+    PowerPCCPU *first_ppc_cpu = POWERPC_CPU(first_cpu);
     CPUState *cs = CPU(cpu);
     CPUPPCState *env = &cpu->env;
     PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
@@ -69,6 +70,18 @@  static void spapr_reset_vcpu(PowerPCCPU *cpu)
     kvm_check_mmu(cpu, &error_fatal);
 
     spapr_irq_cpu_intc_reset(spapr, cpu);
+
+    /*
+     * The boot CPU is only reset during machine reset : reset its
+     * compatibility mode to the machine default. For other CPUs,
+     * either cold plugged or hot plugged, set the compatibility mode
+     * to match the boot CPU, which was either set by the machine reset
+     * code or by CAS.
+     */
+    ppc_set_compat(cpu,
+                   cpu == first_ppc_cpu ?
+                   spapr->max_compat_pvr : first_ppc_cpu->compat_pvr,
+                   &error_fatal);
 }
 
 void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip,