[Qemu-devel] [PATCH 0/3] spapr: fix CPU device tree nodes

[Qemu-devel] [PATCH 0/3] spapr: fix CPU device tree nodes

[Greg Kurz]

When a compatibility mode is requested on the command line,
only the boot CPU core is correctly advertised in the device
tree. Any other CPU core, either hot-plugged or cold-plugged,
appears to be in raw mode with the host PVR.

Guest started on a POWER8 host with:
     -smp cores=2 -machine pseries,max-cpu-compat=compat7

# dtc -f -I fs -O dts /proc/device-tree | egrep 'cpu-version|pa-features'
                        ibm,pa-features = [18 00 f6 3f c7 c0 80 f0 80 00
 00 00 00 00 00 00 00 00 80 00 80 00 80 00 00 00];
                        cpu-version = <0x4d0200>;

                               ^^^
                        second CPU core

                        ibm,pa-features = <0x600f63f 0xc70080c0>;
                        cpu-version = <0xf000003>;

                               ^^^
                          boot CPU core

This happens because the compatibility mode for any CPU but
the boot one is set after the device tree was populated, and
exposed to the guest.

This series fixes the issue by setting the compatibility mode
during CPU reset. This is done in the last patch.

The first two patches are preliminary cleanup of the CPU reset
and CPU hotplug paths. They are not strictly needed, but I
think they are valuable anyway.

--
Greg

---

Greg Kurz (3):
      spapr: drop duplicate variable in spapr_core_plug()
      spapr_cpu_core: don't reset CPUs during realization
      spapr: fix device tree properties when using compatibility mode


 hw/ppc/spapr.c                  |   30 ++++++++++++++++++------------
 hw/ppc/spapr_cpu_core.c         |   15 +++++++++++++--
 hw/ppc/spapr_rtas.c             |    9 ---------
 include/hw/ppc/spapr_cpu_core.h |    2 ++
 4 files changed, 33 insertions(+), 23 deletions(-)

[Qemu-devel] [PATCH 1/3] spapr: drop duplicate variable in spapr_core_plug()

[Greg Kurz]

A variable is already defined at the begining of the function to
hold a pointer to the CPU core object:

    sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));

No need to define it again in the pre-2.10 compatibility code snipplet.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 499ab647d882..bca838fce638 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3362,9 +3362,7 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
         int i;
 
         for (i = 0; i < cc->nr_threads; i++) {
-            sPAPRCPUCore *sc = SPAPR_CPU_CORE(dev);
-
-            cs = CPU(sc->threads[i]);
+            cs = CPU(core->threads[i]);
             pre_2_10_vmstate_unregister_dummy_icp(cs->cpu_index);
         }
     }

Re: [Qemu-devel] [PATCH 1/3] spapr: drop duplicate variable in spapr_core_plug()

[David Gibson]

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

On Wed, Jan 17, 2018 at 10:20:27AM +0100, Greg Kurz wrote:
> A variable is already defined at the begining of the function to
> hold a pointer to the CPU core object:
> 
>     sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));
> 
> No need to define it again in the pre-2.10 compatibility code snipplet.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Applied to ppc-for-2.12, thanks.

> ---
>  hw/ppc/spapr.c |    4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 499ab647d882..bca838fce638 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3362,9 +3362,7 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>          int i;
>  
>          for (i = 0; i < cc->nr_threads; i++) {
> -            sPAPRCPUCore *sc = SPAPR_CPU_CORE(dev);
> -
> -            cs = CPU(sc->threads[i]);
> +            cs = CPU(core->threads[i]);
>              pre_2_10_vmstate_unregister_dummy_icp(cs->cpu_index);
>          }
>      }
> 

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

[Qemu-devel] [PATCH 2/3] spapr_cpu_core: don't reset CPUs during realization

[Greg Kurz]

When QEMU is started, all cold-plugged CPUs are reset twice: first
during initialization and then during machine reset. This is sub-
optimal.

The first reset is only needed for hot-plugged CPUs because the CPU
hotplug code doesn't reset them. This patch adds the necessary code
to reset hot-plugged CPUs on the CPU core hotplug path, and removes
the now useless initial CPU reset.

We just need to mark the newly created CPU as halted to prevent it
to run until it is put online later.

Full CPU reset is now explicitely triggered from the machine code
only, either during system reset or during CPU hotplug.

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

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index bca838fce638..a2ff401f738a 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3336,6 +3336,14 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
         void *fdt;
         int fdt_offset;
 
+        if (hotplugged) {
+            int i;
+
+            for (i = 0; i < cc->nr_threads; i++) {
+                spapr_cpu_reset(core->threads[i]);
+            }
+        }
+
         fdt = spapr_populate_hotplug_cpu_dt(cs, &fdt_offset, spapr);
 
         spapr_drc_attach(drc, dev, fdt, fdt_offset, &local_err);
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index ac19b2e0b72c..268be7784efb 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -22,7 +22,7 @@
 #include "sysemu/hw_accel.h"
 #include "qemu/error-report.h"
 
-static void spapr_cpu_reset(void *opaque)
+void spapr_cpu_reset(void *opaque)
 {
     PowerPCCPU *cpu = opaque;
     CPUState *cs = CPU(cpu);
@@ -63,7 +63,11 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
     cpu_ppc_set_papr(cpu, PPC_VIRTUAL_HYPERVISOR(spapr));
 
     qemu_register_reset(spapr_cpu_reset, cpu);
-    spapr_cpu_reset(cpu);
+
+    /* CPU must not execute anything until explicitely started otherwise the
+     * guest will crash.
+     */
+    CPU(cpu)->halted = 1;
 }
 
 /*
diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
index 1129f344aa0c..763a2168461e 100644
--- a/include/hw/ppc/spapr_cpu_core.h
+++ b/include/hw/ppc/spapr_cpu_core.h
@@ -38,4 +38,6 @@ typedef struct sPAPRCPUCoreClass {
 } sPAPRCPUCoreClass;
 
 const char *spapr_get_cpu_core_type(const char *cpu_type);
+void spapr_cpu_reset(void *opaque);
+
 #endif

Re: [Qemu-devel] [PATCH 2/3] spapr_cpu_core: don't reset CPUs during realization

[David Gibson]

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

On Wed, Jan 17, 2018 at 10:20:35AM +0100, Greg Kurz wrote:
> When QEMU is started, all cold-plugged CPUs are reset twice: first
> during initialization and then during machine reset. This is sub-
> optimal.
> 
> The first reset is only needed for hot-plugged CPUs because the CPU
> hotplug code doesn't reset them. This patch adds the necessary code
> to reset hot-plugged CPUs on the CPU core hotplug path, and removes
> the now useless initial CPU reset.
> 
> We just need to mark the newly created CPU as halted to prevent it
> to run until it is put online later.
> 
> Full CPU reset is now explicitely triggered from the machine code
> only, either during system reset or during CPU hotplug.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Hrm, this looks ok in outline, but makes me nervous in a couple of
ways.


> ---
>  hw/ppc/spapr.c                  |    8 ++++++++
>  hw/ppc/spapr_cpu_core.c         |    8 ++++++--
>  include/hw/ppc/spapr_cpu_core.h |    2 ++
>  3 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index bca838fce638..a2ff401f738a 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3336,6 +3336,14 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>          void *fdt;
>          int fdt_offset;
>  
> +        if (hotplugged) {

First, I'm always wary of using the hotplugged parameter, because what
qemu means by it often doesn't line up with what PAPR means by it.

> +            int i;
> +
> +            for (i = 0; i < cc->nr_threads; i++) {
> +                spapr_cpu_reset(core->threads[i]);
> +            }
> +        }
> +
>          fdt = spapr_populate_hotplug_cpu_dt(cs, &fdt_offset, spapr);
>  
>          spapr_drc_attach(drc, dev, fdt, fdt_offset, &local_err);
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index ac19b2e0b72c..268be7784efb 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -22,7 +22,7 @@
>  #include "sysemu/hw_accel.h"
>  #include "qemu/error-report.h"
>  
> -static void spapr_cpu_reset(void *opaque)
> +void spapr_cpu_reset(void *opaque)
>  {
>      PowerPCCPU *cpu = opaque;
>      CPUState *cs = CPU(cpu);
> @@ -63,7 +63,11 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
>      cpu_ppc_set_papr(cpu, PPC_VIRTUAL_HYPERVISOR(spapr));
>  
>      qemu_register_reset(spapr_cpu_reset, cpu);
> -    spapr_cpu_reset(cpu);
> +
> +    /* CPU must not execute anything until explicitely started otherwise the
> +     * guest will crash.
> +     */
> +    CPU(cpu)->halted = 1;

And poking specifics in a CPU that hasn't already been set to a known
state by a reset also worries me.

>  }
>  
>  /*
> diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
> index 1129f344aa0c..763a2168461e 100644
> --- a/include/hw/ppc/spapr_cpu_core.h
> +++ b/include/hw/ppc/spapr_cpu_core.h
> @@ -38,4 +38,6 @@ typedef struct sPAPRCPUCoreClass {
>  } sPAPRCPUCoreClass;
>  
>  const char *spapr_get_cpu_core_type(const char *cpu_type);
> +void spapr_cpu_reset(void *opaque);
> +
>  #endif
> 

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

Re: [Qemu-devel] [PATCH 2/3] spapr_cpu_core: don't reset CPUs during realization

[Greg Kurz]

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

On Thu, 18 Jan 2018 14:43:40 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Wed, Jan 17, 2018 at 10:20:35AM +0100, Greg Kurz wrote:
> > When QEMU is started, all cold-plugged CPUs are reset twice: first
> > during initialization and then during machine reset. This is sub-
> > optimal.
> > 
> > The first reset is only needed for hot-plugged CPUs because the CPU
> > hotplug code doesn't reset them. This patch adds the necessary code
> > to reset hot-plugged CPUs on the CPU core hotplug path, and removes
> > the now useless initial CPU reset.
> > 
> > We just need to mark the newly created CPU as halted to prevent it
> > to run until it is put online later.
> > 
> > Full CPU reset is now explicitely triggered from the machine code
> > only, either during system reset or during CPU hotplug.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>  
> 
> Hrm, this looks ok in outline, but makes me nervous in a couple of
> ways.
> 
> 
> > ---
> >  hw/ppc/spapr.c                  |    8 ++++++++
> >  hw/ppc/spapr_cpu_core.c         |    8 ++++++--
> >  include/hw/ppc/spapr_cpu_core.h |    2 ++
> >  3 files changed, 16 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index bca838fce638..a2ff401f738a 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -3336,6 +3336,14 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >          void *fdt;
> >          int fdt_offset;
> >  
> > +        if (hotplugged) {  
> 
> First, I'm always wary of using the hotplugged parameter, because what
> qemu means by it often doesn't line up with what PAPR means by it.
> 

Hmmm... you're right, hotplugged in QDEV simply means that the device
was not created during initial machine startup.

ie, any device added with QMP/HMP is always hotplugged.

To cope with the DRC state management, commit 94fd9cbaa3190 added the
fact that QEMU mustn't be waiting for an incoming migration as well.

ie, if QEMU is started with -incoming and CPUs are added before migration
starts, like libvirt does, this code wouldn't reset the CPUs...

I guess we should check qdev->hotplugged instead. Makes sense ?

> > +            int i;
> > +
> > +            for (i = 0; i < cc->nr_threads; i++) {
> > +                spapr_cpu_reset(core->threads[i]);
> > +            }
> > +        }
> > +
> >          fdt = spapr_populate_hotplug_cpu_dt(cs, &fdt_offset, spapr);
> >  
> >          spapr_drc_attach(drc, dev, fdt, fdt_offset, &local_err);
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > index ac19b2e0b72c..268be7784efb 100644
> > --- a/hw/ppc/spapr_cpu_core.c
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -22,7 +22,7 @@
> >  #include "sysemu/hw_accel.h"
> >  #include "qemu/error-report.h"
> >  
> > -static void spapr_cpu_reset(void *opaque)
> > +void spapr_cpu_reset(void *opaque)
> >  {
> >      PowerPCCPU *cpu = opaque;
> >      CPUState *cs = CPU(cpu);
> > @@ -63,7 +63,11 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
> >      cpu_ppc_set_papr(cpu, PPC_VIRTUAL_HYPERVISOR(spapr));
> >  
> >      qemu_register_reset(spapr_cpu_reset, cpu);
> > -    spapr_cpu_reset(cpu);
> > +
> > +    /* CPU must not execute anything until explicitely started otherwise the
> > +     * guest will crash.
> > +     */
> > +    CPU(cpu)->halted = 1;  
> 
> And poking specifics in a CPU that hasn't already been set to a known
> state by a reset also worries me.
> 

IIUC the halted flag doesn't really depend any CPU state. It is
only a way to prevent the CPU from executing, which is needed
if the CPU wasn't set to a known state.

FWIW I've seen other places where it is set before resetting the
CPU (eg, s390_cpu_initfn() or cpu_devinit() for Sun4m).

I was thinking of another solution: create a DeviceClass reset function
that would call spapr_cpu_reset() for all CPUs of a core, and register
it in spapr_cpu_core_class_init(). It would be called by QDEV during
realization of hot-plugged cores only. Unfortunately, this also happens
after the call to spapr_core_plug() (see device_set_realized()).

> >  }
> >  
> >  /*
> > diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
> > index 1129f344aa0c..763a2168461e 100644
> > --- a/include/hw/ppc/spapr_cpu_core.h
> > +++ b/include/hw/ppc/spapr_cpu_core.h
> > @@ -38,4 +38,6 @@ typedef struct sPAPRCPUCoreClass {
> >  } sPAPRCPUCoreClass;
> >  
> >  const char *spapr_get_cpu_core_type(const char *cpu_type);
> > +void spapr_cpu_reset(void *opaque);
> > +
> >  #endif
> >   
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/3] spapr_cpu_core: don't reset CPUs during realization

[Greg Kurz]

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

On Thu, 18 Jan 2018 16:39:30 +0100
Greg Kurz <groug@kaod.org> wrote:

> On Thu, 18 Jan 2018 14:43:40 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Wed, Jan 17, 2018 at 10:20:35AM +0100, Greg Kurz wrote:  
> > > When QEMU is started, all cold-plugged CPUs are reset twice: first
> > > during initialization and then during machine reset. This is sub-
> > > optimal.
> > > 
> > > The first reset is only needed for hot-plugged CPUs because the CPU
> > > hotplug code doesn't reset them. This patch adds the necessary code
> > > to reset hot-plugged CPUs on the CPU core hotplug path, and removes
> > > the now useless initial CPU reset.
> > > 
> > > We just need to mark the newly created CPU as halted to prevent it
> > > to run until it is put online later.
> > > 
> > > Full CPU reset is now explicitely triggered from the machine code
> > > only, either during system reset or during CPU hotplug.
> > > 
> > > Signed-off-by: Greg Kurz <groug@kaod.org>    
> > 
> > Hrm, this looks ok in outline, but makes me nervous in a couple of
> > ways.
> > 
> >   
> > > ---
> > >  hw/ppc/spapr.c                  |    8 ++++++++
> > >  hw/ppc/spapr_cpu_core.c         |    8 ++++++--
> > >  include/hw/ppc/spapr_cpu_core.h |    2 ++
> > >  3 files changed, 16 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index bca838fce638..a2ff401f738a 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -3336,6 +3336,14 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > >          void *fdt;
> > >          int fdt_offset;
> > >  
> > > +        if (hotplugged) {    
> > 
> > First, I'm always wary of using the hotplugged parameter, because what
> > qemu means by it often doesn't line up with what PAPR means by it.
> >   
> 
> Hmmm... you're right, hotplugged in QDEV simply means that the device
> was not created during initial machine startup.
> 
> ie, any device added with QMP/HMP is always hotplugged.
> 
> To cope with the DRC state management, commit 94fd9cbaa3190 added the
> fact that QEMU mustn't be waiting for an incoming migration as well.
> 
> ie, if QEMU is started with -incoming and CPUs are added before migration
> starts, like libvirt does, this code wouldn't reset the CPUs...
> 
> I guess we should check qdev->hotplugged instead. Makes sense ?
> 

Actually, we could even just do this in spapr_cpu_init():

-    spapr_cpu_reset(cpu);
+    if (DEVICE(cpu)->hotplugged) {
+        spapr_cpu_reset(cpu);
+    }


> > > +            int i;
> > > +
> > > +            for (i = 0; i < cc->nr_threads; i++) {
> > > +                spapr_cpu_reset(core->threads[i]);
> > > +            }
> > > +        }
> > > +
> > >          fdt = spapr_populate_hotplug_cpu_dt(cs, &fdt_offset, spapr);
> > >  
> > >          spapr_drc_attach(drc, dev, fdt, fdt_offset, &local_err);
> > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > index ac19b2e0b72c..268be7784efb 100644
> > > --- a/hw/ppc/spapr_cpu_core.c
> > > +++ b/hw/ppc/spapr_cpu_core.c
> > > @@ -22,7 +22,7 @@
> > >  #include "sysemu/hw_accel.h"
> > >  #include "qemu/error-report.h"
> > >  
> > > -static void spapr_cpu_reset(void *opaque)
> > > +void spapr_cpu_reset(void *opaque)
> > >  {
> > >      PowerPCCPU *cpu = opaque;
> > >      CPUState *cs = CPU(cpu);
> > > @@ -63,7 +63,11 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
> > >      cpu_ppc_set_papr(cpu, PPC_VIRTUAL_HYPERVISOR(spapr));
> > >  
> > >      qemu_register_reset(spapr_cpu_reset, cpu);
> > > -    spapr_cpu_reset(cpu);
> > > +
> > > +    /* CPU must not execute anything until explicitely started otherwise the
> > > +     * guest will crash.
> > > +     */
> > > +    CPU(cpu)->halted = 1;    
> > 
> > And poking specifics in a CPU that hasn't already been set to a known
> > state by a reset also worries me.
> >   
> 
> IIUC the halted flag doesn't really depend any CPU state. It is
> only a way to prevent the CPU from executing, which is needed
> if the CPU wasn't set to a known state.
> 
> FWIW I've seen other places where it is set before resetting the
> CPU (eg, s390_cpu_initfn() or cpu_devinit() for Sun4m).
> 
> I was thinking of another solution: create a DeviceClass reset function
> that would call spapr_cpu_reset() for all CPUs of a core, and register
> it in spapr_cpu_core_class_init(). It would be called by QDEV during
> realization of hot-plugged cores only. Unfortunately, this also happens
> after the call to spapr_core_plug() (see device_set_realized()).
> 
> > >  }
> > >  
> > >  /*
> > > diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
> > > index 1129f344aa0c..763a2168461e 100644
> > > --- a/include/hw/ppc/spapr_cpu_core.h
> > > +++ b/include/hw/ppc/spapr_cpu_core.h
> > > @@ -38,4 +38,6 @@ typedef struct sPAPRCPUCoreClass {
> > >  } sPAPRCPUCoreClass;
> > >  
> > >  const char *spapr_get_cpu_core_type(const char *cpu_type);
> > > +void spapr_cpu_reset(void *opaque);
> > > +
> > >  #endif
> > >     
> >   
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH 2/3] spapr_cpu_core: don't reset CPUs during realization

[David Gibson]

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

On Thu, Jan 18, 2018 at 04:39:30PM +0100, Greg Kurz wrote:
> On Thu, 18 Jan 2018 14:43:40 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Wed, Jan 17, 2018 at 10:20:35AM +0100, Greg Kurz wrote:
> > > When QEMU is started, all cold-plugged CPUs are reset twice: first
> > > during initialization and then during machine reset. This is sub-
> > > optimal.
> > > 
> > > The first reset is only needed for hot-plugged CPUs because the CPU
> > > hotplug code doesn't reset them. This patch adds the necessary code
> > > to reset hot-plugged CPUs on the CPU core hotplug path, and removes
> > > the now useless initial CPU reset.
> > > 
> > > We just need to mark the newly created CPU as halted to prevent it
> > > to run until it is put online later.
> > > 
> > > Full CPU reset is now explicitely triggered from the machine code
> > > only, either during system reset or during CPU hotplug.
> > > 
> > > Signed-off-by: Greg Kurz <groug@kaod.org>  
> > 
> > Hrm, this looks ok in outline, but makes me nervous in a couple of
> > ways.
> > 
> > 
> > > ---
> > >  hw/ppc/spapr.c                  |    8 ++++++++
> > >  hw/ppc/spapr_cpu_core.c         |    8 ++++++--
> > >  include/hw/ppc/spapr_cpu_core.h |    2 ++
> > >  3 files changed, 16 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index bca838fce638..a2ff401f738a 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -3336,6 +3336,14 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > >          void *fdt;
> > >          int fdt_offset;
> > >  
> > > +        if (hotplugged) {  
> > 
> > First, I'm always wary of using the hotplugged parameter, because what
> > qemu means by it often doesn't line up with what PAPR means by it.
> > 
> 
> Hmmm... you're right, hotplugged in QDEV simply means that the device
> was not created during initial machine startup.
> 
> ie, any device added with QMP/HMP is always hotplugged.
> 
> To cope with the DRC state management, commit 94fd9cbaa3190 added the
> fact that QEMU mustn't be waiting for an incoming migration as well.
> 
> ie, if QEMU is started with -incoming and CPUs are added before migration
> starts, like libvirt does, this code wouldn't reset the CPUs...
> 
> I guess we should check qdev->hotplugged instead. Makes sense ?

Actually I don't think that dev->hotplugged is any better in terms of
meaning what you want it to.

> > > +            int i;
> > > +
> > > +            for (i = 0; i < cc->nr_threads; i++) {
> > > +                spapr_cpu_reset(core->threads[i]);
> > > +            }
> > > +        }
> > > +
> > >          fdt = spapr_populate_hotplug_cpu_dt(cs, &fdt_offset, spapr);
> > >  
> > >          spapr_drc_attach(drc, dev, fdt, fdt_offset, &local_err);
> > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > index ac19b2e0b72c..268be7784efb 100644
> > > --- a/hw/ppc/spapr_cpu_core.c
> > > +++ b/hw/ppc/spapr_cpu_core.c
> > > @@ -22,7 +22,7 @@
> > >  #include "sysemu/hw_accel.h"
> > >  #include "qemu/error-report.h"
> > >  
> > > -static void spapr_cpu_reset(void *opaque)
> > > +void spapr_cpu_reset(void *opaque)
> > >  {
> > >      PowerPCCPU *cpu = opaque;
> > >      CPUState *cs = CPU(cpu);
> > > @@ -63,7 +63,11 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
> > >      cpu_ppc_set_papr(cpu, PPC_VIRTUAL_HYPERVISOR(spapr));
> > >  
> > >      qemu_register_reset(spapr_cpu_reset, cpu);
> > > -    spapr_cpu_reset(cpu);
> > > +
> > > +    /* CPU must not execute anything until explicitely started otherwise the
> > > +     * guest will crash.
> > > +     */
> > > +    CPU(cpu)->halted = 1;  
> > 
> > And poking specifics in a CPU that hasn't already been set to a known
> > state by a reset also worries me.
> > 
> 
> IIUC the halted flag doesn't really depend any CPU state. It is
> only a way to prevent the CPU from executing, which is needed
> if the CPU wasn't set to a known state.
> 
> FWIW I've seen other places where it is set before resetting the
> CPU (eg, s390_cpu_initfn() or cpu_devinit() for Sun4m).

Its effects do, though - e.g. the recent patches to explicitly disable
the decrementer on stopped vcpus, because halted alone wasn't enough
to keep them quiescent.

> I was thinking of another solution: create a DeviceClass reset function
> that would call spapr_cpu_reset() for all CPUs of a core, and register
> it in spapr_cpu_core_class_init(). It would be called by QDEV during
> realization of hot-plugged cores only. Unfortunately, this also happens
> after the call to spapr_core_plug() (see device_set_realized()).

Well, feel free to come up with something.  AIUI there's no urgency
about this, though - I'm not too worried by extraneous resets, as long
as we have enough to be correct.


> 
> > >  }
> > >  
> > >  /*
> > > diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
> > > index 1129f344aa0c..763a2168461e 100644
> > > --- a/include/hw/ppc/spapr_cpu_core.h
> > > +++ b/include/hw/ppc/spapr_cpu_core.h
> > > @@ -38,4 +38,6 @@ typedef struct sPAPRCPUCoreClass {
> > >  } sPAPRCPUCoreClass;
> > >  
> > >  const char *spapr_get_cpu_core_type(const char *cpu_type);
> > > +void spapr_cpu_reset(void *opaque);
> > > +
> > >  #endif
> > >   
> > 
> 



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

[Qemu-devel] [PATCH 3/3] spapr: fix device tree properties when using compatibility mode

[Greg Kurz]

Commit 51f84465dd98 changed the compatility mode setting logic:
- machine reset only sets compatibility mode for the boot CPU
- compatibility mode is set for other CPUs when they are put online
  by the guest with the "start-cpu" RTAS call

This causes a regression for machines started with max-compat-cpu:
the device tree nodes related to secondary CPU cores contain wrong
"cpu-version" and "ibm,pa-features" values, as shown below.

Guest started on a POWER8 host with:
     -smp cores=2 -machine pseries,max-cpu-compat=compat7

# dtc -f -I fs -O dts /proc/device-tree | egrep 'cpu-version|pa-features'
                        ibm,pa-features = [18 00 f6 3f c7 c0 80 f0 80 00
 00 00 00 00 00 00 00 00 80 00 80 00 80 00 00 00];
                        cpu-version = <0x4d0200>;

                               ^^^
                        second CPU core

                        ibm,pa-features = <0x600f63f 0xc70080c0>;
                        cpu-version = <0xf000003>;

                               ^^^
                          boot CPU core

The second core is advertised in raw POWER8 mode. This happens because
CAS assumes all CPUs to have the same compatibility mode. Since the
boot CPU already has the requested compatibility mode, the CAS code
does not set it for the secondary one, and exposes the bogus device
tree properties in in the CAS response to the guest.

A similar situation is observed when hot-plugging a CPU core. The
related device tree properties are generated and exposed to guest
with the "ibm,configure-connector" RTAS before "start-cpu" is called.
The CPU core is advertised to the guest in raw mode as well.

It both cases, it boils down to the fact that "start-cpu" happens too
late. This can be fixed globally by propagating the compatibility mode
of the boot CPU to the other CPUs during reset.  For this to work, the
compatibility mode of the boot CPU must be set before the machine code
actually resets all CPUs.

It is not needed to set the compatibility mode in "start-cpu" anymore,
so the code is dropped.

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

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index a2ff401f738a..a00bff823f95 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1484,6 +1484,15 @@ static void spapr_machine_reset(void)
         spapr_setup_hpt_and_vrma(spapr);
     }
 
+    /* if this reset wasn't generated by CAS, we should reset our
+     * negotiated options and start from scratch */
+    if (!spapr->cas_reboot) {
+        spapr_ovec_cleanup(spapr->ov5_cas);
+        spapr->ov5_cas = spapr_ovec_new();
+
+        ppc_set_compat(first_ppc_cpu, spapr->max_compat_pvr, &error_fatal);
+    }
+
     qemu_devices_reset();
 
     /* DRC reset may cause a device to be unplugged. This will cause troubles
@@ -1504,15 +1513,6 @@ static void spapr_machine_reset(void)
     rtas_addr = rtas_limit - RTAS_MAX_SIZE;
     fdt_addr = rtas_addr - FDT_MAX_SIZE;
 
-    /* if this reset wasn't generated by CAS, we should reset our
-     * negotiated options and start from scratch */
-    if (!spapr->cas_reboot) {
-        spapr_ovec_cleanup(spapr->ov5_cas);
-        spapr->ov5_cas = spapr_ovec_new();
-
-        ppc_set_compat(first_ppc_cpu, spapr->max_compat_pvr, &error_fatal);
-    }
-
     fdt = spapr_build_fdt(spapr, rtas_addr, spapr->rtas_size);
 
     spapr_load_rtas(spapr, fdt, rtas_addr);
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 268be7784efb..f5e5ead5f4b0 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -44,6 +44,13 @@ void spapr_cpu_reset(void *opaque)
     if (cs != first_cpu) {
         env->spr[SPR_LPCR] &= ~pcc->lpcr_pm;
     }
+
+    /* Set compatibility mode to match the boot CPU, which was either set
+     * by the machine reset code or by CAS. This should never fail.
+     */
+    if (cs != first_cpu) {
+        ppc_set_compat(cpu, POWERPC_CPU(first_cpu)->compat_pvr, &error_abort);
+    }
 }
 
 static void spapr_cpu_destroy(PowerPCCPU *cpu)
diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
index 2b89e1d448e4..4bb939d3d111 100644
--- a/hw/ppc/spapr_rtas.c
+++ b/hw/ppc/spapr_rtas.c
@@ -163,7 +163,6 @@ static void rtas_start_cpu(PowerPCCPU *cpu_, sPAPRMachineState *spapr,
         CPUState *cs = CPU(cpu);
         CPUPPCState *env = &cpu->env;
         PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
-        Error *local_err = NULL;
 
         if (!cs->halted) {
             rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
@@ -175,14 +174,6 @@ static void rtas_start_cpu(PowerPCCPU *cpu_, sPAPRMachineState *spapr,
          * new cpu enters */
         kvm_cpu_synchronize_state(cs);
 
-        /* Set compatibility mode to match existing cpus */
-        ppc_set_compat(cpu, POWERPC_CPU(first_cpu)->compat_pvr, &local_err);
-        if (local_err) {
-            error_report_err(local_err);
-            rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
-            return;
-        }
-
         env->msr = (1ULL << MSR_SF) | (1ULL << MSR_ME);
 
         /* Enable Power-saving mode Exit Cause exceptions for the new CPU */

Re: [Qemu-devel] [PATCH 3/3] spapr: fix device tree properties when using compatibility mode

[David Gibson]

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

On Wed, Jan 17, 2018 at 10:20:42AM +0100, Greg Kurz wrote:
> Commit 51f84465dd98 changed the compatility mode setting logic:
> - machine reset only sets compatibility mode for the boot CPU
> - compatibility mode is set for other CPUs when they are put online
>   by the guest with the "start-cpu" RTAS call
> 
> This causes a regression for machines started with max-compat-cpu:
> the device tree nodes related to secondary CPU cores contain wrong
> "cpu-version" and "ibm,pa-features" values, as shown below.
> 
> Guest started on a POWER8 host with:
>      -smp cores=2 -machine pseries,max-cpu-compat=compat7
> 
> # dtc -f -I fs -O dts /proc/device-tree | egrep 'cpu-version|pa-features'
>                         ibm,pa-features = [18 00 f6 3f c7 c0 80 f0 80 00
>  00 00 00 00 00 00 00 00 80 00 80 00 80 00 00 00];
>                         cpu-version = <0x4d0200>;
> 
>                                ^^^
>                         second CPU core
> 
>                         ibm,pa-features = <0x600f63f 0xc70080c0>;
>                         cpu-version = <0xf000003>;
> 
>                                ^^^
>                           boot CPU core
> 
> The second core is advertised in raw POWER8 mode. This happens because
> CAS assumes all CPUs to have the same compatibility mode. Since the
> boot CPU already has the requested compatibility mode, the CAS code
> does not set it for the secondary one, and exposes the bogus device
> tree properties in in the CAS response to the guest.
> 
> A similar situation is observed when hot-plugging a CPU core. The
> related device tree properties are generated and exposed to guest
> with the "ibm,configure-connector" RTAS before "start-cpu" is called.
> The CPU core is advertised to the guest in raw mode as well.
> 
> It both cases, it boils down to the fact that "start-cpu" happens too
> late. This can be fixed globally by propagating the compatibility mode
> of the boot CPU to the other CPUs during reset.  For this to work, the
> compatibility mode of the boot CPU must be set before the machine code
> actually resets all CPUs.
> 
> It is not needed to set the compatibility mode in "start-cpu" anymore,
> so the code is dropped.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Applied to ppc-for-2.12.

I forgot that the RTAS call came after constructing the device tree
fragment, and is therefore too late to set the compatibility mode.

> ---
>  hw/ppc/spapr.c          |   18 +++++++++---------
>  hw/ppc/spapr_cpu_core.c |    7 +++++++
>  hw/ppc/spapr_rtas.c     |    9 ---------
>  3 files changed, 16 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index a2ff401f738a..a00bff823f95 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1484,6 +1484,15 @@ static void spapr_machine_reset(void)
>          spapr_setup_hpt_and_vrma(spapr);
>      }
>  
> +    /* if this reset wasn't generated by CAS, we should reset our
> +     * negotiated options and start from scratch */
> +    if (!spapr->cas_reboot) {
> +        spapr_ovec_cleanup(spapr->ov5_cas);
> +        spapr->ov5_cas = spapr_ovec_new();
> +
> +        ppc_set_compat(first_ppc_cpu, spapr->max_compat_pvr, &error_fatal);
> +    }
> +
>      qemu_devices_reset();
>  
>      /* DRC reset may cause a device to be unplugged. This will cause troubles
> @@ -1504,15 +1513,6 @@ static void spapr_machine_reset(void)
>      rtas_addr = rtas_limit - RTAS_MAX_SIZE;
>      fdt_addr = rtas_addr - FDT_MAX_SIZE;
>  
> -    /* if this reset wasn't generated by CAS, we should reset our
> -     * negotiated options and start from scratch */
> -    if (!spapr->cas_reboot) {
> -        spapr_ovec_cleanup(spapr->ov5_cas);
> -        spapr->ov5_cas = spapr_ovec_new();
> -
> -        ppc_set_compat(first_ppc_cpu, spapr->max_compat_pvr, &error_fatal);
> -    }
> -
>      fdt = spapr_build_fdt(spapr, rtas_addr, spapr->rtas_size);
>  
>      spapr_load_rtas(spapr, fdt, rtas_addr);
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 268be7784efb..f5e5ead5f4b0 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -44,6 +44,13 @@ void spapr_cpu_reset(void *opaque)
>      if (cs != first_cpu) {
>          env->spr[SPR_LPCR] &= ~pcc->lpcr_pm;
>      }
> +
> +    /* Set compatibility mode to match the boot CPU, which was either set
> +     * by the machine reset code or by CAS. This should never fail.
> +     */
> +    if (cs != first_cpu) {
> +        ppc_set_compat(cpu, POWERPC_CPU(first_cpu)->compat_pvr, &error_abort);
> +    }
>  }
>  
>  static void spapr_cpu_destroy(PowerPCCPU *cpu)
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index 2b89e1d448e4..4bb939d3d111 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -163,7 +163,6 @@ static void rtas_start_cpu(PowerPCCPU *cpu_, sPAPRMachineState *spapr,
>          CPUState *cs = CPU(cpu);
>          CPUPPCState *env = &cpu->env;
>          PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> -        Error *local_err = NULL;
>  
>          if (!cs->halted) {
>              rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
> @@ -175,14 +174,6 @@ static void rtas_start_cpu(PowerPCCPU *cpu_, sPAPRMachineState *spapr,
>           * new cpu enters */
>          kvm_cpu_synchronize_state(cs);
>  
> -        /* Set compatibility mode to match existing cpus */
> -        ppc_set_compat(cpu, POWERPC_CPU(first_cpu)->compat_pvr, &local_err);
> -        if (local_err) {
> -            error_report_err(local_err);
> -            rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
> -            return;
> -        }
> -
>          env->msr = (1ULL << MSR_SF) | (1ULL << MSR_ME);
>  
>          /* Enable Power-saving mode Exit Cause exceptions for the new CPU */
> 

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