[Qemu-devel] [PATCH for-2.13 00/10] spapr: Cleanups to PAPR mode setup

[Qemu-devel] [PATCH for-2.13 00/10] spapr: Cleanups to PAPR mode setup

[David Gibson]

This series contains a number of cleanups to the way we set up and
start a guest in PAPR mode.

Applies on top of my ppc-for-2.13 branch.

David Gibson (10):
  spapr: Avoid redundant calls to spapr_cpu_reset()
  spapr: Remove support for PowerPC 970 with pseries machine type
  target/ppc: Remove unnecessary initialization of LPCR_UPRT
  spapr: Set compatibility mode before the rest of spapr_cpu_reset()
  spapr: Move PAPR mode register initialization to spapr code
  target/ppc: Add ppc_store_lpcr() helper
  spapr: Make a helper to set up cpu entry point state
  spapr: Clean up handling of LPCR power-saving exit bits
  target/ppc: Don't bother with MSR_EP in cpu_ppc_set_papr()
  spapr: Move PAPR specific cpu logic to pseries machine type

 hw/ppc/spapr.c                  | 71 ++++++++++++++---------------------------
 hw/ppc/spapr_cpu_core.c         | 71 +++++++++++++++++++++++++++++++----------
 hw/ppc/spapr_rtas.c             | 10 ++----
 include/hw/ppc/spapr_cpu_core.h |  5 +++
 target/ppc/cpu.h                |  2 +-
 target/ppc/kvm.c                | 40 +++--------------------
 target/ppc/kvm_ppc.h            |  6 ----
 target/ppc/mmu-hash64.c         | 15 ++++++---
 target/ppc/mmu-hash64.h         |  3 +-
 target/ppc/translate_init.c     | 71 +++--------------------------------------
 10 files changed, 109 insertions(+), 185 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCH for-2.13 01/10] spapr: Avoid redundant calls to spapr_cpu_reset()

[David Gibson]

af81cf323c1 "spapr: CPU hotplug support" added a direct call to
spapr_cpu_reset() in spapr_cpu_init(), as well as registering it as a
reset callback.  That was in order to make sure that the reset function
got called for a newly hotplugged cpu, which would miss the global machine
reset.

However, this change means that spapr_cpu_reset() gets called twice for
normal cold-plugged cpus: once from spapr_cpu_init(), then again during
the system reset.  As well as being ugly in its redundancy, the first call
happens before the machine reset calls have happened, which will cause
problems for some things we're going to want to add.

So, we remove the reset call from spapr_cpu_init().  We instead put an
explicit reset call in the hotplug specific path.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr.c                  |  6 ++++--
 hw/ppc/spapr_cpu_core.c         | 13 ++++++++++++-
 include/hw/ppc/spapr_cpu_core.h |  2 ++
 3 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 7b2bc4e25d..81b50af3b5 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3370,9 +3370,11 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
 
         if (hotplugged) {
             /*
-             * Send hotplug notification interrupt to the guest only
-             * in case of hotplugged CPUs.
+             * For hotplugged CPUs, we need to reset them (they missed
+             * out on the system reset), and send the guest a
+             * notification
              */
+            spapr_cpu_core_reset(core);
             spapr_hotplug_req_add_by_index(drc);
         } else {
             spapr_drc_reset(drc);
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 94afeb399e..aa17626cda 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -70,7 +70,6 @@ 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);
 }
 
 /*
@@ -208,6 +207,18 @@ err:
     error_propagate(errp, local_err);
 }
 
+void spapr_cpu_core_reset(sPAPRCPUCore *sc)
+{
+    CPUCore *cc = CPU_CORE(sc);
+    int i;
+
+    for (i = 0; i < cc->nr_threads; i++) {
+        PowerPCCPU *cpu = sc->threads[i];
+
+        spapr_cpu_reset(cpu);
+    }
+}
+
 static Property spapr_cpu_core_properties[] = {
     DEFINE_PROP_INT32("node-id", sPAPRCPUCore, node_id, CPU_UNSET_NUMA_NODE_ID),
     DEFINE_PROP_END_OF_LIST()
diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
index 1129f344aa..1a38544706 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_core_reset(sPAPRCPUCore *sc);
+
 #endif
-- 
2.14.3

Re: [Qemu-devel] [PATCH for-2.13 01/10] spapr: Avoid redundant calls to spapr_cpu_reset()

[Greg Kurz]

On Tue, 17 Apr 2018 17:17:13 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> af81cf323c1 "spapr: CPU hotplug support" added a direct call to
> spapr_cpu_reset() in spapr_cpu_init(), as well as registering it as a
> reset callback.  That was in order to make sure that the reset function
> got called for a newly hotplugged cpu, which would miss the global machine
> reset.
> 
> However, this change means that spapr_cpu_reset() gets called twice for
> normal cold-plugged cpus: once from spapr_cpu_init(), then again during
> the system reset.  As well as being ugly in its redundancy, the first call
> happens before the machine reset calls have happened, which will cause
> problems for some things we're going to want to add.
> 
> So, we remove the reset call from spapr_cpu_init().  We instead put an
> explicit reset call in the hotplug specific path.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---

I had sent a tentative patch to do something similar earlier this year:

https://patchwork.ozlabs.org/patch/862116/

but it got nacked for several reasons, one of them being you were
"always wary of using the hotplugged parameter, because what qemu
means by it often doesn't line up with what PAPR means by it."

>  hw/ppc/spapr.c                  |  6 ++++--
>  hw/ppc/spapr_cpu_core.c         | 13 ++++++++++++-
>  include/hw/ppc/spapr_cpu_core.h |  2 ++
>  3 files changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 7b2bc4e25d..81b50af3b5 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3370,9 +3370,11 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>  
>          if (hotplugged) {

... but you rely on it here. Can you explain why it is
okay now ?

Also, if QEMU is started with -incoming and the CPU core
is hotplugged before migration begins, the following will
return false:

static inline bool spapr_drc_hotplugged(DeviceState *dev)
{
    return dev->hotplugged && !runstate_check(RUN_STATE_INMIGRATE);
}

and the CPU core won't be reset.

>              /*
> -             * Send hotplug notification interrupt to the guest only
> -             * in case of hotplugged CPUs.
> +             * For hotplugged CPUs, we need to reset them (they missed
> +             * out on the system reset), and send the guest a
> +             * notification
>               */
> +            spapr_cpu_core_reset(core);

spapr_cpu_reset() also sets the compat mode, which is used
to set some properties in the DT, ie, this should be called
before spapr_populate_hotplug_cpu_dt().

>              spapr_hotplug_req_add_by_index(drc);
>          } else {
>              spapr_drc_reset(drc);
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 94afeb399e..aa17626cda 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -70,7 +70,6 @@ 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);

spapr_cpu_reset() also sets cs->halted to 1, to let the guest start
non-boot CPUs with an RTAS call. With this change, a hotplugged CPU
is started right away and may run in the guest before the hotplug
path could even reset it.

Not sure how to handle that other than setting halted to 1 in
spapr_cpu_init() as well, but you already nacked that approach
because it would mean "poking specifics in a CPU that hasn't
already been set to a known state by a reset"... 

>  }
>  
>  /*
> @@ -208,6 +207,18 @@ err:
>      error_propagate(errp, local_err);
>  }
>  
> +void spapr_cpu_core_reset(sPAPRCPUCore *sc)
> +{
> +    CPUCore *cc = CPU_CORE(sc);
> +    int i;
> +
> +    for (i = 0; i < cc->nr_threads; i++) {
> +        PowerPCCPU *cpu = sc->threads[i];
> +
> +        spapr_cpu_reset(cpu);
> +    }
> +}
> +
>  static Property spapr_cpu_core_properties[] = {
>      DEFINE_PROP_INT32("node-id", sPAPRCPUCore, node_id, CPU_UNSET_NUMA_NODE_ID),
>      DEFINE_PROP_END_OF_LIST()
> diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
> index 1129f344aa..1a38544706 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_core_reset(sPAPRCPUCore *sc);
> +
>  #endif

Re: [Qemu-devel] [PATCH for-2.13 01/10] spapr: Avoid redundant calls to spapr_cpu_reset()

[David Gibson]

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

On Thu, Apr 19, 2018 at 03:48:23PM +0200, Greg Kurz wrote:
> On Tue, 17 Apr 2018 17:17:13 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > af81cf323c1 "spapr: CPU hotplug support" added a direct call to
> > spapr_cpu_reset() in spapr_cpu_init(), as well as registering it as a
> > reset callback.  That was in order to make sure that the reset function
> > got called for a newly hotplugged cpu, which would miss the global machine
> > reset.
> > 
> > However, this change means that spapr_cpu_reset() gets called twice for
> > normal cold-plugged cpus: once from spapr_cpu_init(), then again during
> > the system reset.  As well as being ugly in its redundancy, the first call
> > happens before the machine reset calls have happened, which will cause
> > problems for some things we're going to want to add.
> > 
> > So, we remove the reset call from spapr_cpu_init().  We instead put an
> > explicit reset call in the hotplug specific path.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> 
> I had sent a tentative patch to do something similar earlier this year:
> 
> https://patchwork.ozlabs.org/patch/862116/
> 
> but it got nacked for several reasons, one of them being you were
> "always wary of using the hotplugged parameter, because what qemu
> means by it often doesn't line up with what PAPR means by it."

Yeah, I was and am wary of that, but convinced myself it was correct
in this case (which doesn't really interact with the PAPR meaning of
hotplug).

> >  hw/ppc/spapr.c                  |  6 ++++--
> >  hw/ppc/spapr_cpu_core.c         | 13 ++++++++++++-
> >  include/hw/ppc/spapr_cpu_core.h |  2 ++
> >  3 files changed, 18 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 7b2bc4e25d..81b50af3b5 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -3370,9 +3370,11 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >  
> >          if (hotplugged) {
> 
> ... but you rely on it here. Can you explain why it is
> okay now ?

So the value I actually need here is "wasn't present at the last
system reset" (with false positives being mostly harmless, but not
false negatives).

> Also, if QEMU is started with -incoming and the CPU core
> is hotplugged before migration begins, the following will
> return false:
> 
> static inline bool spapr_drc_hotplugged(DeviceState *dev)
> {
>     return dev->hotplugged && !runstate_check(RUN_STATE_INMIGRATE);
> }
> 
> and the CPU core won't be reset.

Uh... spapr_dtc_hotplugged() would definitely be wrong here, which is
why I'm not using it.

> 
> >              /*
> > -             * Send hotplug notification interrupt to the guest only
> > -             * in case of hotplugged CPUs.
> > +             * For hotplugged CPUs, we need to reset them (they missed
> > +             * out on the system reset), and send the guest a
> > +             * notification
> >               */
> > +            spapr_cpu_core_reset(core);
> 
> spapr_cpu_reset() also sets the compat mode, which is used
> to set some properties in the DT, ie, this should be called
> before spapr_populate_hotplug_cpu_dt().

Good point.  I've moved the reset to fix that.

> 
> >              spapr_hotplug_req_add_by_index(drc);
> >          } else {
> >              spapr_drc_reset(drc);
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > index 94afeb399e..aa17626cda 100644
> > --- a/hw/ppc/spapr_cpu_core.c
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -70,7 +70,6 @@ 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);
> 
> spapr_cpu_reset() also sets cs->halted to 1, to let the guest start
> non-boot CPUs with an RTAS call. With this change, a hotplugged CPU
> is started right away and may run in the guest before the hotplug
> path could even reset it.

Uh.. I was assuming the plug path (the qemu side, obviously, not the
guest side) would be completed before the cpu could execute.  If
that's not the case I'll definitely have to rethink this.

> Not sure how to handle that other than setting halted to 1 in
> spapr_cpu_init() as well, but you already nacked that approach
> because it would mean "poking specifics in a CPU that hasn't
> already been set to a known state by a reset"...

Yeah.. I may have been off base with that comment.  Bear in mind that
at the time I didn't see a specific reason to avoid the double reset -
now I do.

-- 
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 for-2.13 01/10] spapr: Avoid redundant calls to spapr_cpu_reset()

[Greg Kurz]

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

On Fri, 20 Apr 2018 16:34:37 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Thu, Apr 19, 2018 at 03:48:23PM +0200, Greg Kurz wrote:
> > On Tue, 17 Apr 2018 17:17:13 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > af81cf323c1 "spapr: CPU hotplug support" added a direct call to
> > > spapr_cpu_reset() in spapr_cpu_init(), as well as registering it as a
> > > reset callback.  That was in order to make sure that the reset function
> > > got called for a newly hotplugged cpu, which would miss the global machine
> > > reset.
> > > 
> > > However, this change means that spapr_cpu_reset() gets called twice for
> > > normal cold-plugged cpus: once from spapr_cpu_init(), then again during
> > > the system reset.  As well as being ugly in its redundancy, the first call
> > > happens before the machine reset calls have happened, which will cause
> > > problems for some things we're going to want to add.
> > > 
> > > So, we remove the reset call from spapr_cpu_init().  We instead put an
> > > explicit reset call in the hotplug specific path.
> > > 
> > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > ---  
> > 
> > I had sent a tentative patch to do something similar earlier this year:
> > 
> > https://patchwork.ozlabs.org/patch/862116/
> > 
> > but it got nacked for several reasons, one of them being you were
> > "always wary of using the hotplugged parameter, because what qemu
> > means by it often doesn't line up with what PAPR means by it."  
> 
> Yeah, I was and am wary of that, but convinced myself it was correct
> in this case (which doesn't really interact with the PAPR meaning of
> hotplug).
> 
> > >  hw/ppc/spapr.c                  |  6 ++++--
> > >  hw/ppc/spapr_cpu_core.c         | 13 ++++++++++++-
> > >  include/hw/ppc/spapr_cpu_core.h |  2 ++
> > >  3 files changed, 18 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index 7b2bc4e25d..81b50af3b5 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -3370,9 +3370,11 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > >  
> > >          if (hotplugged) {  
> > 
> > ... but you rely on it here. Can you explain why it is
> > okay now ?  
> 
> So the value I actually need here is "wasn't present at the last
> system reset" (with false positives being mostly harmless, but not
> false negatives).
> 

Hmm... It is rather the other way around, sth like "will be caught
by the initial machine reset".

> > Also, if QEMU is started with -incoming and the CPU core
> > is hotplugged before migration begins, the following will
> > return false:
> > 
> > static inline bool spapr_drc_hotplugged(DeviceState *dev)
> > {
> >     return dev->hotplugged && !runstate_check(RUN_STATE_INMIGRATE);
> > }
> > 
> > and the CPU core won't be reset.  
> 
> Uh... spapr_dtc_hotplugged() would definitely be wrong here, which is
> why I'm not using it.
> 

This is how hotplugged is set in spapr_core_plug():

    bool hotplugged = spapr_drc_hotplugged(dev);

but to detect the "will be caught by the initial machine reset" condition,
we only need to check dev->hotplugged actually.

> >   
> > >              /*
> > > -             * Send hotplug notification interrupt to the guest only
> > > -             * in case of hotplugged CPUs.
> > > +             * For hotplugged CPUs, we need to reset them (they missed
> > > +             * out on the system reset), and send the guest a
> > > +             * notification
> > >               */
> > > +            spapr_cpu_core_reset(core);  
> > 
> > spapr_cpu_reset() also sets the compat mode, which is used
> > to set some properties in the DT, ie, this should be called
> > before spapr_populate_hotplug_cpu_dt().  
> 
> Good point.  I've moved the reset to fix that.
> 
> >   
> > >              spapr_hotplug_req_add_by_index(drc);
> > >          } else {
> > >              spapr_drc_reset(drc);
> > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > index 94afeb399e..aa17626cda 100644
> > > --- a/hw/ppc/spapr_cpu_core.c
> > > +++ b/hw/ppc/spapr_cpu_core.c
> > > @@ -70,7 +70,6 @@ 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);  
> > 
> > spapr_cpu_reset() also sets cs->halted to 1, to let the guest start
> > non-boot CPUs with an RTAS call. With this change, a hotplugged CPU
> > is started right away and may run in the guest before the hotplug
> > path could even reset it.  
> 
> Uh.. I was assuming the plug path (the qemu side, obviously, not the
> guest side) would be completed before the cpu could execute.  If
> that's not the case I'll definitely have to rethink this.
> 

It isn't the case unless @halted is set.

> > Not sure how to handle that other than setting halted to 1 in
> > spapr_cpu_init() as well, but you already nacked that approach
> > because it would mean "poking specifics in a CPU that hasn't
> > already been set to a known state by a reset"...  
> 
> Yeah.. I may have been off base with that comment.  Bear in mind that
> at the time I didn't see a specific reason to avoid the double reset -
> now I do.
> 

Fair enough :)

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

Re: [Qemu-devel] [Qemu-ppc] [PATCH for-2.13 01/10] spapr: Avoid redundant calls to spapr_cpu_reset()

[Greg Kurz]

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

On Fri, 20 Apr 2018 11:15:01 +0200
Greg Kurz <groug@kaod.org> wrote:

> On Fri, 20 Apr 2018 16:34:37 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Thu, Apr 19, 2018 at 03:48:23PM +0200, Greg Kurz wrote:  
> > > On Tue, 17 Apr 2018 17:17:13 +1000
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >     
> > > > af81cf323c1 "spapr: CPU hotplug support" added a direct call to
> > > > spapr_cpu_reset() in spapr_cpu_init(), as well as registering it as a
> > > > reset callback.  That was in order to make sure that the reset function
> > > > got called for a newly hotplugged cpu, which would miss the global machine
> > > > reset.
> > > > 
> > > > However, this change means that spapr_cpu_reset() gets called twice for
> > > > normal cold-plugged cpus: once from spapr_cpu_init(), then again during
> > > > the system reset.  As well as being ugly in its redundancy, the first call
> > > > happens before the machine reset calls have happened, which will cause
> > > > problems for some things we're going to want to add.
> > > > 
> > > > So, we remove the reset call from spapr_cpu_init().  We instead put an
> > > > explicit reset call in the hotplug specific path.
> > > > 
> > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > > ---    
> > > 
> > > I had sent a tentative patch to do something similar earlier this year:
> > > 
> > > https://patchwork.ozlabs.org/patch/862116/
> > > 
> > > but it got nacked for several reasons, one of them being you were
> > > "always wary of using the hotplugged parameter, because what qemu
> > > means by it often doesn't line up with what PAPR means by it."    
> > 
> > Yeah, I was and am wary of that, but convinced myself it was correct
> > in this case (which doesn't really interact with the PAPR meaning of
> > hotplug).
> >   
> > > >  hw/ppc/spapr.c                  |  6 ++++--
> > > >  hw/ppc/spapr_cpu_core.c         | 13 ++++++++++++-
> > > >  include/hw/ppc/spapr_cpu_core.h |  2 ++
> > > >  3 files changed, 18 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > index 7b2bc4e25d..81b50af3b5 100644
> > > > --- a/hw/ppc/spapr.c
> > > > +++ b/hw/ppc/spapr.c
> > > > @@ -3370,9 +3370,11 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > > >  
> > > >          if (hotplugged) {    
> > > 
> > > ... but you rely on it here. Can you explain why it is
> > > okay now ?    
> > 
> > So the value I actually need here is "wasn't present at the last
> > system reset" (with false positives being mostly harmless, but not
> > false negatives).
> >   
> 
> Hmm... It is rather the other way around, sth like "will be caught
> by the initial machine reset".
> 
> > > Also, if QEMU is started with -incoming and the CPU core
> > > is hotplugged before migration begins, the following will
> > > return false:
> > > 
> > > static inline bool spapr_drc_hotplugged(DeviceState *dev)
> > > {
> > >     return dev->hotplugged && !runstate_check(RUN_STATE_INMIGRATE);
> > > }
> > > 
> > > and the CPU core won't be reset.    
> > 
> > Uh... spapr_dtc_hotplugged() would definitely be wrong here, which is
> > why I'm not using it.
> >   
> 
> This is how hotplugged is set in spapr_core_plug():
> 
>     bool hotplugged = spapr_drc_hotplugged(dev);
> 
> but to detect the "will be caught by the initial machine reset" condition,
> we only need to check dev->hotplugged actually.
> 
> > >     
> > > >              /*
> > > > -             * Send hotplug notification interrupt to the guest only
> > > > -             * in case of hotplugged CPUs.
> > > > +             * For hotplugged CPUs, we need to reset them (they missed
> > > > +             * out on the system reset), and send the guest a
> > > > +             * notification
> > > >               */
> > > > +            spapr_cpu_core_reset(core);    
> > > 
> > > spapr_cpu_reset() also sets the compat mode, which is used
> > > to set some properties in the DT, ie, this should be called
> > > before spapr_populate_hotplug_cpu_dt().    
> > 
> > Good point.  I've moved the reset to fix that.
> >   

Thinking of it again: since cold-plugged devices reach this before machine
reset, we would then attach to the DRC a DT blob based on a non-reset CPU :-\

Instead of registering a reset handler for each individual CPUs, maybe
we should rather register it a the CPU core level. The handler would
first reset all CPUs in the core and then setup the DRC for new cores only,
like it is done currently in spapr_core_plug().

spapr_core_plug() would then just need to register the reset handler,
and call it only for hotplugged cores.

I've tweaked your patch to implement this, and could do some basic
plug/unplug tests without seeing anything wrong. But I'll be on
vacation next week (currently packing), so I inline it below as
food for thought.

-----------------------------------------------------------------------------------
 hw/ppc/spapr.c                  |   62 ++++++++++++++++++++++++++++-----------
 hw/ppc/spapr_cpu_core.c         |   27 +++++++++++------
 include/hw/ppc/spapr_cpu_core.h |    2 +
 3 files changed, 64 insertions(+), 27 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 9bf3eba9c5ea..51a0349c2bcc 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3388,6 +3388,36 @@ static void *spapr_populate_hotplug_cpu_dt(CPUState *cs, int *fdt_offset,
     return fdt;
 }
 
+static void spapr_core_reset(void *opaque)
+{
+    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
+    DeviceState *dev = DEVICE(opaque);
+    CPUCore *cc = CPU_CORE(dev);
+    sPAPRCPUCore *sc = SPAPR_CPU_CORE(dev);
+    CPUState *cs = CPU(sc->threads[0]);
+    sPAPRDRConnector *drc;
+
+    spapr_cpu_core_reset(sc);
+
+    drc = spapr_drc_by_id(TYPE_SPAPR_DRC_CPU,
+                          spapr_vcpu_id(spapr, cc->core_id));
+
+    assert(drc);
+
+    if (!drc->dev) {
+        void *fdt;
+        int fdt_offset;
+
+        fdt = spapr_populate_hotplug_cpu_dt(cs, &fdt_offset, spapr);
+
+        spapr_drc_attach(drc, dev, fdt, fdt_offset, &error_abort);
+
+        if (!spapr_drc_hotplugged(dev)) {
+            spapr_drc_reset(drc);
+        }
+    }
+}
+
 /* Callback to be called during DRC release. */
 void spapr_core_release(DeviceState *dev)
 {
@@ -3395,9 +3425,9 @@ void spapr_core_release(DeviceState *dev)
     sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
     CPUCore *cc = CPU_CORE(dev);
     CPUArchId *core_slot = spapr_find_cpu_slot(ms, cc->core_id, NULL);
+    sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
 
     if (smc->pre_2_10_has_unused_icps) {
-        sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
         int i;
 
         for (i = 0; i < cc->nr_threads; i++) {
@@ -3407,6 +3437,8 @@ void spapr_core_release(DeviceState *dev)
         }
     }
 
+    qemu_unregister_reset(spapr_core_reset, sc);
+
     assert(core_slot);
     core_slot->cpu = NULL;
     object_unparent(OBJECT(dev));
@@ -3448,12 +3480,9 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
     sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
     sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));
     CPUCore *cc = CPU_CORE(dev);
-    CPUState *cs = CPU(core->threads[0]);
     sPAPRDRConnector *drc;
-    Error *local_err = NULL;
     CPUArchId *core_slot;
     int index;
-    bool hotplugged = spapr_drc_hotplugged(dev);
 
     core_slot = spapr_find_cpu_slot(MACHINE(hotplug_dev), cc->core_id, &index);
     if (!core_slot) {
@@ -3467,32 +3496,31 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
     g_assert(drc || !mc->has_hotpluggable_cpus);
 
     if (drc) {
-        void *fdt;
-        int fdt_offset;
-
-        fdt = spapr_populate_hotplug_cpu_dt(cs, &fdt_offset, spapr);
-
-        spapr_drc_attach(drc, dev, fdt, fdt_offset, &local_err);
-        if (local_err) {
-            g_free(fdt);
-            error_propagate(errp, local_err);
-            return;
+        /* The boot core and any core specified on the command line will
+         * be reset during the initial machine reset. Filter them out based
+         * on @hotplugged which is set to false in this case. Another case
+         * is when a core is added after the machine was shutdown: no need
+         * to reset here since a system reset is needed to restart the machine.
+         */
+        if (dev->hotplugged && !runstate_check(RUN_STATE_SHUTDOWN)) {
+            spapr_core_reset(core);
         }
 
-        if (hotplugged) {
+        if (spapr_drc_hotplugged(dev)) {
             /*
              * Send hotplug notification interrupt to the guest only
              * in case of hotplugged CPUs.
              */
             spapr_hotplug_req_add_by_index(drc);
-        } else {
-            spapr_drc_reset(drc);
         }
     }
 
+    qemu_register_reset(spapr_core_reset, core);
+
     core_slot->cpu = OBJECT(dev);
 
     if (smc->pre_2_10_has_unused_icps) {
+        CPUState *cs = CPU(core->threads[0]);
         int i;
 
         for (i = 0; i < cc->nr_threads; i++) {
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 90e4b72c3e96..aaae90bbcb6d 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -22,9 +22,8 @@
 #include "sysemu/hw_accel.h"
 #include "qemu/error-report.h"
 
-static void spapr_cpu_reset(void *opaque)
+static void spapr_cpu_reset(PowerPCCPU *cpu)
 {
-    PowerPCCPU *cpu = opaque;
     CPUState *cs = CPU(cpu);
     CPUPPCState *env = &cpu->env;
     PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
@@ -50,11 +49,6 @@ static void spapr_cpu_reset(void *opaque)
 
 }
 
-static void spapr_cpu_destroy(PowerPCCPU *cpu)
-{
-    qemu_unregister_reset(spapr_cpu_reset, cpu);
-}
-
 static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
                            Error **errp)
 {
@@ -66,8 +60,10 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
     /* Enable PAPR mode in TCG or KVM */
     cpu_ppc_set_papr(cpu, PPC_VIRTUAL_HYPERVISOR(spapr));
 
-    qemu_register_reset(spapr_cpu_reset, cpu);
-    spapr_cpu_reset(cpu);
+    /* All CPUs start halted.  CPU0 is unhalted from the machine level
+     * reset code and the rest are explicitly started up by the guest
+     * using an RTAS call */
+    CPU(cpu)->halted = 1;
 }
 
 /*
@@ -101,7 +97,6 @@ static void spapr_cpu_core_unrealizefn(DeviceState *dev, Error **errp)
         CPUState *cs = CPU(dev);
         PowerPCCPU *cpu = POWERPC_CPU(cs);
 
-        spapr_cpu_destroy(cpu);
         object_unparent(cpu->intc);
         cpu_remove_sync(cs);
         object_unparent(obj);
@@ -205,6 +200,18 @@ err:
     error_propagate(errp, local_err);
 }
 
+void spapr_cpu_core_reset(sPAPRCPUCore *sc)
+{
+    CPUCore *cc = CPU_CORE(sc);
+    int i;
+
+    for (i = 0; i < cc->nr_threads; i++) {
+        PowerPCCPU *cpu = sc->threads[i];
+
+        spapr_cpu_reset(cpu);
+    }
+}
+
 static Property spapr_cpu_core_properties[] = {
     DEFINE_PROP_INT32("node-id", sPAPRCPUCore, node_id, CPU_UNSET_NUMA_NODE_ID),
     DEFINE_PROP_END_OF_LIST()
diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
index 1129f344aa0c..1a38544706e7 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_core_reset(sPAPRCPUCore *sc);
+
 #endif

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

Re: [Qemu-devel] [Qemu-ppc] [PATCH for-2.13 01/10] spapr: Avoid redundant calls to spapr_cpu_reset()

[David Gibson]

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

On Fri, Apr 20, 2018 at 05:39:42PM +0200, Greg Kurz wrote:
> On Fri, 20 Apr 2018 11:15:01 +0200
> Greg Kurz <groug@kaod.org> wrote:
> 
> > On Fri, 20 Apr 2018 16:34:37 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> > 
> > > On Thu, Apr 19, 2018 at 03:48:23PM +0200, Greg Kurz wrote:  
> > > > On Tue, 17 Apr 2018 17:17:13 +1000
> > > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > > >     
> > > > > af81cf323c1 "spapr: CPU hotplug support" added a direct call to
> > > > > spapr_cpu_reset() in spapr_cpu_init(), as well as registering it as a
> > > > > reset callback.  That was in order to make sure that the reset function
> > > > > got called for a newly hotplugged cpu, which would miss the global machine
> > > > > reset.
> > > > > 
> > > > > However, this change means that spapr_cpu_reset() gets called twice for
> > > > > normal cold-plugged cpus: once from spapr_cpu_init(), then again during
> > > > > the system reset.  As well as being ugly in its redundancy, the first call
> > > > > happens before the machine reset calls have happened, which will cause
> > > > > problems for some things we're going to want to add.
> > > > > 
> > > > > So, we remove the reset call from spapr_cpu_init().  We instead put an
> > > > > explicit reset call in the hotplug specific path.
> > > > > 
> > > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > > > ---    
> > > > 
> > > > I had sent a tentative patch to do something similar earlier this year:
> > > > 
> > > > https://patchwork.ozlabs.org/patch/862116/
> > > > 
> > > > but it got nacked for several reasons, one of them being you were
> > > > "always wary of using the hotplugged parameter, because what qemu
> > > > means by it often doesn't line up with what PAPR means by it."    
> > > 
> > > Yeah, I was and am wary of that, but convinced myself it was correct
> > > in this case (which doesn't really interact with the PAPR meaning of
> > > hotplug).
> > >   
> > > > >  hw/ppc/spapr.c                  |  6 ++++--
> > > > >  hw/ppc/spapr_cpu_core.c         | 13 ++++++++++++-
> > > > >  include/hw/ppc/spapr_cpu_core.h |  2 ++
> > > > >  3 files changed, 18 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > > index 7b2bc4e25d..81b50af3b5 100644
> > > > > --- a/hw/ppc/spapr.c
> > > > > +++ b/hw/ppc/spapr.c
> > > > > @@ -3370,9 +3370,11 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > > > >  
> > > > >          if (hotplugged) {    
> > > > 
> > > > ... but you rely on it here. Can you explain why it is
> > > > okay now ?    
> > > 
> > > So the value I actually need here is "wasn't present at the last
> > > system reset" (with false positives being mostly harmless, but not
> > > false negatives).
> > >   
> > 
> > Hmm... It is rather the other way around, sth like "will be caught
> > by the initial machine reset".
> > 
> > > > Also, if QEMU is started with -incoming and the CPU core
> > > > is hotplugged before migration begins, the following will
> > > > return false:
> > > > 
> > > > static inline bool spapr_drc_hotplugged(DeviceState *dev)
> > > > {
> > > >     return dev->hotplugged && !runstate_check(RUN_STATE_INMIGRATE);
> > > > }
> > > > 
> > > > and the CPU core won't be reset.    
> > > 
> > > Uh... spapr_dtc_hotplugged() would definitely be wrong here, which is
> > > why I'm not using it.
> > >   
> > 
> > This is how hotplugged is set in spapr_core_plug():
> > 
> >     bool hotplugged = spapr_drc_hotplugged(dev);
> > 
> > but to detect the "will be caught by the initial machine reset" condition,
> > we only need to check dev->hotplugged actually.
> > 
> > > >     
> > > > >              /*
> > > > > -             * Send hotplug notification interrupt to the guest only
> > > > > -             * in case of hotplugged CPUs.
> > > > > +             * For hotplugged CPUs, we need to reset them (they missed
> > > > > +             * out on the system reset), and send the guest a
> > > > > +             * notification
> > > > >               */
> > > > > +            spapr_cpu_core_reset(core);    
> > > > 
> > > > spapr_cpu_reset() also sets the compat mode, which is used
> > > > to set some properties in the DT, ie, this should be called
> > > > before spapr_populate_hotplug_cpu_dt().    
> > > 
> > > Good point.  I've moved the reset to fix that.
> > >   
> 
> Thinking of it again: since cold-plugged devices reach this before machine
> reset, we would then attach to the DRC a DT blob based on a non-reset CPU :-\
> 
> Instead of registering a reset handler for each individual CPUs, maybe
> we should rather register it a the CPU core level. The handler would
> first reset all CPUs in the core and then setup the DRC for new cores only,
> like it is done currently in spapr_core_plug().
> 
> spapr_core_plug() would then just need to register the reset handler,
> and call it only for hotplugged cores.

Handling the resets via the core level might be a good idea anyway,
but I don't think it can address the problem we're hitting here.

I've investigated further and I'm pretty sure we can't fix this
without generic code changes.  cpu_common_realizefn() (which is called
by our ppc cpu realize hook via the parent_realize chain) contains
this:

    if (dev->hotplugged) {
        cpu_synchronize_post_init(cpu);
        cpu_resume(cpu);
    }

So, as soon as the hotplugged cpu is realized, it's running, which
means by the time we call the plug() hotplug handler we're too late to
do any reset initialization.

I think there are two ways to look at this:

1) The reset handlers are specifically about *system* reset, not
device reset, and so we shoudln't really expect them to be called for
hotplugged devices.  If we want to share reset initialization with
"initial" initialization, we should explicitly call the reset handler
from the (realize time) init code.. which is what we do now.

2) Common core realize should _not_ activate the cpu.  Instead that
should be the plug() handler's job.  This would require changing the
x86 cpu plug handler (and whatever else) to kick off the cpu after
realization.

For now I'm inclined to just let it stay at (1).

The problem I had which I thought required this, doesn't after all.  I
came up with a different solution that involves moving the spapr caps
initialization earlier, instead of the cpu reset later.  That turned
out to be substantially easier than I first thought, and regardless of
what we do above long term, I think it's a better way to handle the
caps.

-- 
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] [Qemu-ppc] [PATCH for-2.13 01/10] spapr: Avoid redundant calls to spapr_cpu_reset()

[Greg Kurz]

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

On Mon, 18 Jun 2018 13:42:37 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Fri, Apr 20, 2018 at 05:39:42PM +0200, Greg Kurz wrote:
> > On Fri, 20 Apr 2018 11:15:01 +0200
> > Greg Kurz <groug@kaod.org> wrote:
> >   
> > > On Fri, 20 Apr 2018 16:34:37 +1000
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >   
> > > > On Thu, Apr 19, 2018 at 03:48:23PM +0200, Greg Kurz wrote:    
> > > > > On Tue, 17 Apr 2018 17:17:13 +1000
> > > > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > > > >       
> > > > > > af81cf323c1 "spapr: CPU hotplug support" added a direct call to
> > > > > > spapr_cpu_reset() in spapr_cpu_init(), as well as registering it as a
> > > > > > reset callback.  That was in order to make sure that the reset function
> > > > > > got called for a newly hotplugged cpu, which would miss the global machine
> > > > > > reset.
> > > > > > 
> > > > > > However, this change means that spapr_cpu_reset() gets called twice for
> > > > > > normal cold-plugged cpus: once from spapr_cpu_init(), then again during
> > > > > > the system reset.  As well as being ugly in its redundancy, the first call
> > > > > > happens before the machine reset calls have happened, which will cause
> > > > > > problems for some things we're going to want to add.
> > > > > > 
> > > > > > So, we remove the reset call from spapr_cpu_init().  We instead put an
> > > > > > explicit reset call in the hotplug specific path.
> > > > > > 
> > > > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > > > > ---      
> > > > > 
> > > > > I had sent a tentative patch to do something similar earlier this year:
> > > > > 
> > > > > https://patchwork.ozlabs.org/patch/862116/
> > > > > 
> > > > > but it got nacked for several reasons, one of them being you were
> > > > > "always wary of using the hotplugged parameter, because what qemu
> > > > > means by it often doesn't line up with what PAPR means by it."      
> > > > 
> > > > Yeah, I was and am wary of that, but convinced myself it was correct
> > > > in this case (which doesn't really interact with the PAPR meaning of
> > > > hotplug).
> > > >     
> > > > > >  hw/ppc/spapr.c                  |  6 ++++--
> > > > > >  hw/ppc/spapr_cpu_core.c         | 13 ++++++++++++-
> > > > > >  include/hw/ppc/spapr_cpu_core.h |  2 ++
> > > > > >  3 files changed, 18 insertions(+), 3 deletions(-)
> > > > > > 
> > > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > > > index 7b2bc4e25d..81b50af3b5 100644
> > > > > > --- a/hw/ppc/spapr.c
> > > > > > +++ b/hw/ppc/spapr.c
> > > > > > @@ -3370,9 +3370,11 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > > > > >  
> > > > > >          if (hotplugged) {      
> > > > > 
> > > > > ... but you rely on it here. Can you explain why it is
> > > > > okay now ?      
> > > > 
> > > > So the value I actually need here is "wasn't present at the last
> > > > system reset" (with false positives being mostly harmless, but not
> > > > false negatives).
> > > >     
> > > 
> > > Hmm... It is rather the other way around, sth like "will be caught
> > > by the initial machine reset".
> > >   
> > > > > Also, if QEMU is started with -incoming and the CPU core
> > > > > is hotplugged before migration begins, the following will
> > > > > return false:
> > > > > 
> > > > > static inline bool spapr_drc_hotplugged(DeviceState *dev)
> > > > > {
> > > > >     return dev->hotplugged && !runstate_check(RUN_STATE_INMIGRATE);
> > > > > }
> > > > > 
> > > > > and the CPU core won't be reset.      
> > > > 
> > > > Uh... spapr_dtc_hotplugged() would definitely be wrong here, which is
> > > > why I'm not using it.
> > > >     
> > > 
> > > This is how hotplugged is set in spapr_core_plug():
> > > 
> > >     bool hotplugged = spapr_drc_hotplugged(dev);
> > > 
> > > but to detect the "will be caught by the initial machine reset" condition,
> > > we only need to check dev->hotplugged actually.
> > >   
> > > > >       
> > > > > >              /*
> > > > > > -             * Send hotplug notification interrupt to the guest only
> > > > > > -             * in case of hotplugged CPUs.
> > > > > > +             * For hotplugged CPUs, we need to reset them (they missed
> > > > > > +             * out on the system reset), and send the guest a
> > > > > > +             * notification
> > > > > >               */
> > > > > > +            spapr_cpu_core_reset(core);      
> > > > > 
> > > > > spapr_cpu_reset() also sets the compat mode, which is used
> > > > > to set some properties in the DT, ie, this should be called
> > > > > before spapr_populate_hotplug_cpu_dt().      
> > > > 
> > > > Good point.  I've moved the reset to fix that.
> > > >     
> > 
> > Thinking of it again: since cold-plugged devices reach this before machine
> > reset, we would then attach to the DRC a DT blob based on a non-reset CPU :-\
> > 
> > Instead of registering a reset handler for each individual CPUs, maybe
> > we should rather register it a the CPU core level. The handler would
> > first reset all CPUs in the core and then setup the DRC for new cores only,
> > like it is done currently in spapr_core_plug().
> > 
> > spapr_core_plug() would then just need to register the reset handler,
> > and call it only for hotplugged cores.  
> 
> Handling the resets via the core level might be a good idea anyway,
> but I don't think it can address the problem we're hitting here.
> 
> I've investigated further and I'm pretty sure we can't fix this
> without generic code changes.  cpu_common_realizefn() (which is called
> by our ppc cpu realize hook via the parent_realize chain) contains
> this:
> 
>     if (dev->hotplugged) {
>         cpu_synchronize_post_init(cpu);
>         cpu_resume(cpu);
>     }
> 

Yes, these come from:

commit 13eed94ed5617b98e657163490584dc2a0cc4b32
Author: Igor Mammedov <imammedo@redhat.com>
Date:   Tue Apr 23 10:29:36 2013 +0200

    cpu: Call cpu_synchronize_post_init() from DeviceClass::realize()
    
    If hotplugged, synchronize CPU state to KVM.
    
    Signed-off-by: Igor Mammedov <imammedo@redhat.com>
    Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
    Signed-off-by: Andreas Färber <afaerber@suse.de>

and

commit 6afb4721f3e45da727110470a61aafcd6682395e
Author: Igor Mammedov <imammedo@redhat.com>
Date:   Tue Apr 23 10:29:38 2013 +0200

    cpu: Resume CPU from DeviceClass::realize() if hot-plugged
    
    Signed-off-by: Igor Mammedov <imammedo@redhat.com>
    Signed-off-by: Andreas Färber <afaerber@suse.de>

> So, as soon as the hotplugged cpu is realized, it's running, which
> means by the time we call the plug() hotplug handler we're too late to
> do any reset initialization.
> 
> I think there are two ways to look at this:
> 
> 1) The reset handlers are specifically about *system* reset, not
> device reset, and so we shoudln't really expect them to be called for
> hotplugged devices.  If we want to share reset initialization with
> "initial" initialization, we should explicitly call the reset handler
> from the (realize time) init code.. which is what we do now.
> 
> 2) Common core realize should _not_ activate the cpu.  Instead that
> should be the plug() handler's job.  This would require changing the
> x86 cpu plug handler (and whatever else) to kick off the cpu after
> realization.
> 
> For now I'm inclined to just let it stay at (1).
> 

Maybe Igor and Eduardo (now Cc'd) can provide a hint about 2) ?

> The problem I had which I thought required this, doesn't after all.  I
> came up with a different solution that involves moving the spapr caps
> initialization earlier, instead of the cpu reset later.  That turned
> out to be substantially easier than I first thought, and regardless of
> what we do above long term, I think it's a better way to handle the
> caps.
> 


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

[Qemu-devel] [PATCH for-2.13 02/10] spapr: Remove support for PowerPC 970 with pseries machine type

[David Gibson]

Current POWER cpus allow for a VRMA, a special mapping which describes a
guest's view of memory when in real mode (MMU off, from the guest's point
of view).  Older cpus didn't have that which meant that to support a guest
a special host-contiguous region of memory was needed to give the guest its
Real Mode Area (RMA).

This was useful in the early days of KVM on Power to allow it to be tested
on PowerPC 970 chips as used in Macintosh G5 machines.  Now, however, those
machines are so old as to be irrelevant, and the host kernel has long since
dropped support for this mode.  It hasn't been tested in ages either.

So, to simplify the code, drop the support from qemu as well.

As well as the code for handling contiguous RMAs, we can remove some
code to set the HIOR register, which existed on 970 but not on the
current and supported CPUs.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr.c          | 61 +++++++++++++++----------------------------------
 hw/ppc/spapr_cpu_core.c |  2 --
 target/ppc/kvm.c        | 36 -----------------------------
 target/ppc/kvm_ppc.h    |  6 -----
 4 files changed, 19 insertions(+), 86 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 81b50af3b5..fbb2c6752c 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2376,9 +2376,6 @@ static void spapr_machine_init(MachineState *machine)
     int i;
     MemoryRegion *sysmem = get_system_memory();
     MemoryRegion *ram = g_new(MemoryRegion, 1);
-    MemoryRegion *rma_region;
-    void *rma = NULL;
-    hwaddr rma_alloc_size;
     hwaddr node0_size = spapr_node0_size(machine);
     long load_limit, fw_size;
     char *filename;
@@ -2417,40 +2414,28 @@ static void spapr_machine_init(MachineState *machine)
         exit(1);
     }
 
-    /* Allocate RMA if necessary */
-    rma_alloc_size = kvmppc_alloc_rma(&rma);
+    spapr->rma_size = node0_size;
 
-    if (rma_alloc_size == -1) {
-        error_report("Unable to create RMA");
-        exit(1);
+    /* With KVM, we don't actually know whether KVM supports an
+     * unbounded RMA (PR KVM) or is limited by the hash table size (HV
+     * KVM using VRMA), so we always assume the latter
+     *
+     * In that case, we also limit the initial allocations for RTAS
+     * etc... to 256M since we have no way to know what the VRMA size
+     * is going to be as it depends on the size of the hash table
+     * isn't determined yet.
+     */
+    if (kvm_enabled()) {
+        spapr->vrma_adjust = 1;
+        spapr->rma_size = MIN(spapr->rma_size, 0x10000000);
     }
 
-    if (rma_alloc_size && (rma_alloc_size < node0_size)) {
-        spapr->rma_size = rma_alloc_size;
-    } else {
-        spapr->rma_size = node0_size;
-
-        /* With KVM, we don't actually know whether KVM supports an
-         * unbounded RMA (PR KVM) or is limited by the hash table size
-         * (HV KVM using VRMA), so we always assume the latter
-         *
-         * In that case, we also limit the initial allocations for RTAS
-         * etc... to 256M since we have no way to know what the VRMA size
-         * is going to be as it depends on the size of the hash table
-         * isn't determined yet.
-         */
-        if (kvm_enabled()) {
-            spapr->vrma_adjust = 1;
-            spapr->rma_size = MIN(spapr->rma_size, 0x10000000);
-        }
-
-        /* Actually we don't support unbounded RMA anymore since we
-         * added proper emulation of HV mode. The max we can get is
-         * 16G which also happens to be what we configure for PAPR
-         * mode so make sure we don't do anything bigger than that
-         */
-        spapr->rma_size = MIN(spapr->rma_size, 0x400000000ull);
-    }
+    /* Actually we don't support unbounded RMA anymore since we
+     * added proper emulation of HV mode. The max we can get is
+     * 16G which also happens to be what we configure for PAPR
+     * mode so make sure we don't do anything bigger than that
+     */
+    spapr->rma_size = MIN(spapr->rma_size, 0x400000000ull);
 
     if (spapr->rma_size > node0_size) {
         error_report("Numa node 0 has to span the RMA (%#08"HWADDR_PRIx")",
@@ -2508,14 +2493,6 @@ static void spapr_machine_init(MachineState *machine)
                                          machine->ram_size);
     memory_region_add_subregion(sysmem, 0, ram);
 
-    if (rma_alloc_size && rma) {
-        rma_region = g_new(MemoryRegion, 1);
-        memory_region_init_ram_ptr(rma_region, NULL, "ppc_spapr.rma",
-                                   rma_alloc_size, rma);
-        vmstate_register_ram_global(rma_region);
-        memory_region_add_subregion(sysmem, 0, rma_region);
-    }
-
     /* initialize hotplug memory address space */
     if (machine->ram_size < machine->maxram_size) {
         ram_addr_t hotplug_mem_size = machine->maxram_size - machine->ram_size;
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index aa17626cda..f39d99a8da 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -36,8 +36,6 @@ static void spapr_cpu_reset(void *opaque)
      * using an RTAS call */
     cs->halted = 1;
 
-    env->spr[SPR_HIOR] = 0;
-
     /* Disable Power-saving mode Exit Cause exceptions for the CPU.
      * This can cause issues when rebooting the guest if a secondary
      * is awaken */
diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index 6de59c5b21..51177a8e00 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -2159,42 +2159,6 @@ void kvmppc_hint_smt_possible(Error **errp)
 
 
 #ifdef TARGET_PPC64
-off_t kvmppc_alloc_rma(void **rma)
-{
-    off_t size;
-    int fd;
-    struct kvm_allocate_rma ret;
-
-    /* If cap_ppc_rma == 0, contiguous RMA allocation is not supported
-     * if cap_ppc_rma == 1, contiguous RMA allocation is supported, but
-     *                      not necessary on this hardware
-     * if cap_ppc_rma == 2, contiguous RMA allocation is needed on this hardware
-     *
-     * FIXME: We should allow the user to force contiguous RMA
-     * allocation in the cap_ppc_rma==1 case.
-     */
-    if (cap_ppc_rma < 2) {
-        return 0;
-    }
-
-    fd = kvm_vm_ioctl(kvm_state, KVM_ALLOCATE_RMA, &ret);
-    if (fd < 0) {
-        fprintf(stderr, "KVM: Error on KVM_ALLOCATE_RMA: %s\n",
-                strerror(errno));
-        return -1;
-    }
-
-    size = MIN(ret.rma_size, 256ul << 20);
-
-    *rma = mmap(NULL, size, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0);
-    if (*rma == MAP_FAILED) {
-        fprintf(stderr, "KVM: Error mapping RMA: %s\n", strerror(errno));
-        return -1;
-    };
-
-    return size;
-}
-
 uint64_t kvmppc_rma_size(uint64_t current_size, unsigned int hash_shift)
 {
     struct kvm_ppc_smmu_info info;
diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
index 4d2789eef6..e2840e1d33 100644
--- a/target/ppc/kvm_ppc.h
+++ b/target/ppc/kvm_ppc.h
@@ -37,7 +37,6 @@ target_ulong kvmppc_configure_v3_mmu(PowerPCCPU *cpu,
                                      bool radix, bool gtse,
                                      uint64_t proc_tbl);
 #ifndef CONFIG_USER_ONLY
-off_t kvmppc_alloc_rma(void **rma);
 bool kvmppc_spapr_use_multitce(void);
 int kvmppc_spapr_enable_inkernel_multitce(void);
 void *kvmppc_create_spapr_tce(uint32_t liobn, uint32_t page_shift,
@@ -188,11 +187,6 @@ static inline target_ulong kvmppc_configure_v3_mmu(PowerPCCPU *cpu,
 }
 
 #ifndef CONFIG_USER_ONLY
-static inline off_t kvmppc_alloc_rma(void **rma)
-{
-    return 0;
-}
-
 static inline bool kvmppc_spapr_use_multitce(void)
 {
     return false;
-- 
2.14.3

Re: [Qemu-devel] [PATCH for-2.13 02/10] spapr: Remove support for PowerPC 970 with pseries machine type

[Greg Kurz]

On Tue, 17 Apr 2018 17:17:14 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> Current POWER cpus allow for a VRMA, a special mapping which describes a
> guest's view of memory when in real mode (MMU off, from the guest's point
> of view).  Older cpus didn't have that which meant that to support a guest
> a special host-contiguous region of memory was needed to give the guest its
> Real Mode Area (RMA).
> 
> This was useful in the early days of KVM on Power to allow it to be tested
> on PowerPC 970 chips as used in Macintosh G5 machines.  Now, however, those
> machines are so old as to be irrelevant, and the host kernel has long since
> dropped support for this mode.  It hasn't been tested in ages either.
> 
> So, to simplify the code, drop the support from qemu as well.
> 

So this could possibly break TCG guests with 970, which happens to be
bootable with the current code ?

$ cat /etc/redhat-release
Red Hat Enterprise Linux Server release 6.9 (Santiago)
$ cat /proc/cpuinfo 
processor       : 0
cpu             : PPC970, altivec supported
clock           : 1000.000000MHz
revision        : 2.2 (pvr 0039 0202)

timebase        : 512000000
platform        : pSeries
model           : IBM pSeries (emulated by qemu)
machine         : CHRP IBM pSeries (emulated by qemu)

I guess nobody uses this setup, but my understanding is that some
rules must be followed when it comes to removing something that
works.

https://wiki.qemu.org/Features/LegacyRemoval#Rules_for_removing_an_interface

Maybe add a warning if 970 is used, and turn it into an error in two releases
along with this patch ?

> As well as the code for handling contiguous RMAs, we can remove some
> code to set the HIOR register, which existed on 970 but not on the
> current and supported CPUs.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr.c          | 61 +++++++++++++++----------------------------------
>  hw/ppc/spapr_cpu_core.c |  2 --
>  target/ppc/kvm.c        | 36 -----------------------------
>  target/ppc/kvm_ppc.h    |  6 -----
>  4 files changed, 19 insertions(+), 86 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 81b50af3b5..fbb2c6752c 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2376,9 +2376,6 @@ static void spapr_machine_init(MachineState *machine)
>      int i;
>      MemoryRegion *sysmem = get_system_memory();
>      MemoryRegion *ram = g_new(MemoryRegion, 1);
> -    MemoryRegion *rma_region;
> -    void *rma = NULL;
> -    hwaddr rma_alloc_size;
>      hwaddr node0_size = spapr_node0_size(machine);
>      long load_limit, fw_size;
>      char *filename;
> @@ -2417,40 +2414,28 @@ static void spapr_machine_init(MachineState *machine)
>          exit(1);
>      }
>  
> -    /* Allocate RMA if necessary */
> -    rma_alloc_size = kvmppc_alloc_rma(&rma);
> +    spapr->rma_size = node0_size;
>  
> -    if (rma_alloc_size == -1) {
> -        error_report("Unable to create RMA");
> -        exit(1);
> +    /* With KVM, we don't actually know whether KVM supports an
> +     * unbounded RMA (PR KVM) or is limited by the hash table size (HV
> +     * KVM using VRMA), so we always assume the latter
> +     *
> +     * In that case, we also limit the initial allocations for RTAS
> +     * etc... to 256M since we have no way to know what the VRMA size
> +     * is going to be as it depends on the size of the hash table
> +     * isn't determined yet.
> +     */
> +    if (kvm_enabled()) {
> +        spapr->vrma_adjust = 1;
> +        spapr->rma_size = MIN(spapr->rma_size, 0x10000000);
>      }
>  
> -    if (rma_alloc_size && (rma_alloc_size < node0_size)) {
> -        spapr->rma_size = rma_alloc_size;
> -    } else {
> -        spapr->rma_size = node0_size;
> -
> -        /* With KVM, we don't actually know whether KVM supports an
> -         * unbounded RMA (PR KVM) or is limited by the hash table size
> -         * (HV KVM using VRMA), so we always assume the latter
> -         *
> -         * In that case, we also limit the initial allocations for RTAS
> -         * etc... to 256M since we have no way to know what the VRMA size
> -         * is going to be as it depends on the size of the hash table
> -         * isn't determined yet.
> -         */
> -        if (kvm_enabled()) {
> -            spapr->vrma_adjust = 1;
> -            spapr->rma_size = MIN(spapr->rma_size, 0x10000000);
> -        }
> -
> -        /* Actually we don't support unbounded RMA anymore since we
> -         * added proper emulation of HV mode. The max we can get is
> -         * 16G which also happens to be what we configure for PAPR
> -         * mode so make sure we don't do anything bigger than that
> -         */
> -        spapr->rma_size = MIN(spapr->rma_size, 0x400000000ull);
> -    }
> +    /* Actually we don't support unbounded RMA anymore since we
> +     * added proper emulation of HV mode. The max we can get is
> +     * 16G which also happens to be what we configure for PAPR
> +     * mode so make sure we don't do anything bigger than that
> +     */
> +    spapr->rma_size = MIN(spapr->rma_size, 0x400000000ull);
>  
>      if (spapr->rma_size > node0_size) {
>          error_report("Numa node 0 has to span the RMA (%#08"HWADDR_PRIx")",
> @@ -2508,14 +2493,6 @@ static void spapr_machine_init(MachineState *machine)
>                                           machine->ram_size);
>      memory_region_add_subregion(sysmem, 0, ram);
>  
> -    if (rma_alloc_size && rma) {
> -        rma_region = g_new(MemoryRegion, 1);
> -        memory_region_init_ram_ptr(rma_region, NULL, "ppc_spapr.rma",
> -                                   rma_alloc_size, rma);
> -        vmstate_register_ram_global(rma_region);
> -        memory_region_add_subregion(sysmem, 0, rma_region);
> -    }
> -
>      /* initialize hotplug memory address space */
>      if (machine->ram_size < machine->maxram_size) {
>          ram_addr_t hotplug_mem_size = machine->maxram_size - machine->ram_size;
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index aa17626cda..f39d99a8da 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -36,8 +36,6 @@ static void spapr_cpu_reset(void *opaque)
>       * using an RTAS call */
>      cs->halted = 1;
>  
> -    env->spr[SPR_HIOR] = 0;
> -
>      /* Disable Power-saving mode Exit Cause exceptions for the CPU.
>       * This can cause issues when rebooting the guest if a secondary
>       * is awaken */
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 6de59c5b21..51177a8e00 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -2159,42 +2159,6 @@ void kvmppc_hint_smt_possible(Error **errp)
>  
>  
>  #ifdef TARGET_PPC64
> -off_t kvmppc_alloc_rma(void **rma)
> -{
> -    off_t size;
> -    int fd;
> -    struct kvm_allocate_rma ret;
> -
> -    /* If cap_ppc_rma == 0, contiguous RMA allocation is not supported
> -     * if cap_ppc_rma == 1, contiguous RMA allocation is supported, but
> -     *                      not necessary on this hardware
> -     * if cap_ppc_rma == 2, contiguous RMA allocation is needed on this hardware
> -     *
> -     * FIXME: We should allow the user to force contiguous RMA
> -     * allocation in the cap_ppc_rma==1 case.
> -     */
> -    if (cap_ppc_rma < 2) {
> -        return 0;
> -    }
> -
> -    fd = kvm_vm_ioctl(kvm_state, KVM_ALLOCATE_RMA, &ret);
> -    if (fd < 0) {
> -        fprintf(stderr, "KVM: Error on KVM_ALLOCATE_RMA: %s\n",
> -                strerror(errno));
> -        return -1;
> -    }
> -
> -    size = MIN(ret.rma_size, 256ul << 20);
> -
> -    *rma = mmap(NULL, size, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0);
> -    if (*rma == MAP_FAILED) {
> -        fprintf(stderr, "KVM: Error mapping RMA: %s\n", strerror(errno));
> -        return -1;
> -    };
> -
> -    return size;
> -}
> -
>  uint64_t kvmppc_rma_size(uint64_t current_size, unsigned int hash_shift)
>  {
>      struct kvm_ppc_smmu_info info;
> diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
> index 4d2789eef6..e2840e1d33 100644
> --- a/target/ppc/kvm_ppc.h
> +++ b/target/ppc/kvm_ppc.h
> @@ -37,7 +37,6 @@ target_ulong kvmppc_configure_v3_mmu(PowerPCCPU *cpu,
>                                       bool radix, bool gtse,
>                                       uint64_t proc_tbl);
>  #ifndef CONFIG_USER_ONLY
> -off_t kvmppc_alloc_rma(void **rma);
>  bool kvmppc_spapr_use_multitce(void);
>  int kvmppc_spapr_enable_inkernel_multitce(void);
>  void *kvmppc_create_spapr_tce(uint32_t liobn, uint32_t page_shift,
> @@ -188,11 +187,6 @@ static inline target_ulong kvmppc_configure_v3_mmu(PowerPCCPU *cpu,
>  }
>  
>  #ifndef CONFIG_USER_ONLY
> -static inline off_t kvmppc_alloc_rma(void **rma)
> -{
> -    return 0;
> -}
> -
>  static inline bool kvmppc_spapr_use_multitce(void)
>  {
>      return false;

Re: [Qemu-devel] [Qemu-ppc] [PATCH for-2.13 02/10] spapr: Remove support for PowerPC 970 with pseries machine type

[Thomas Huth]

On 19.04.2018 19:21, Greg Kurz wrote:
> On Tue, 17 Apr 2018 17:17:14 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
>> Current POWER cpus allow for a VRMA, a special mapping which describes a
>> guest's view of memory when in real mode (MMU off, from the guest's point
>> of view).  Older cpus didn't have that which meant that to support a guest
>> a special host-contiguous region of memory was needed to give the guest its
>> Real Mode Area (RMA).
>>
>> This was useful in the early days of KVM on Power to allow it to be tested
>> on PowerPC 970 chips as used in Macintosh G5 machines.  Now, however, those
>> machines are so old as to be irrelevant, and the host kernel has long since
>> dropped support for this mode.  It hasn't been tested in ages either.
>>
>> So, to simplify the code, drop the support from qemu as well.
>>
> 
> So this could possibly break TCG guests with 970, which happens to be
> bootable with the current code ?
> 
> $ cat /etc/redhat-release
> Red Hat Enterprise Linux Server release 6.9 (Santiago)
> $ cat /proc/cpuinfo 
> processor       : 0
> cpu             : PPC970, altivec supported
> clock           : 1000.000000MHz
> revision        : 2.2 (pvr 0039 0202)
> 
> timebase        : 512000000
> platform        : pSeries
> model           : IBM pSeries (emulated by qemu)
> machine         : CHRP IBM pSeries (emulated by qemu)
> 
> I guess nobody uses this setup, but my understanding is that some
> rules must be followed when it comes to removing something that
> works.
> 
> https://wiki.qemu.org/Features/LegacyRemoval#Rules_for_removing_an_interface
> 
> Maybe add a warning if 970 is used, and turn it into an error in two releases
> along with this patch ?

Right, we've got a process for deprecating old features, so please
follow that process.

 Thomas

Re: [Qemu-devel] [PATCH for-2.13 02/10] spapr: Remove support for PowerPC 970 with pseries machine type

[David Gibson]

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

On Thu, Apr 19, 2018 at 07:21:05PM +0200, Greg Kurz wrote:
> On Tue, 17 Apr 2018 17:17:14 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > Current POWER cpus allow for a VRMA, a special mapping which describes a
> > guest's view of memory when in real mode (MMU off, from the guest's point
> > of view).  Older cpus didn't have that which meant that to support a guest
> > a special host-contiguous region of memory was needed to give the guest its
> > Real Mode Area (RMA).
> > 
> > This was useful in the early days of KVM on Power to allow it to be tested
> > on PowerPC 970 chips as used in Macintosh G5 machines.  Now, however, those
> > machines are so old as to be irrelevant, and the host kernel has long since
> > dropped support for this mode.  It hasn't been tested in ages either.
> > 
> > So, to simplify the code, drop the support from qemu as well.
> > 
> 
> So this could possibly break TCG guests with 970, which happens to be
> bootable with the current code ?

Hm.. so that's actually a good question.  In theory the HIOR change
could break TCG.. except that AFAICT we don't do anything with the
HIOR register except pass it to KVM.

The allocated RMA changes are only relevant with 970 *and* KVM HV.
And allocate RMA mode was always kind of broken because there was no
real way to ensure a consistent guest environment between HV and TCG.

The KVM side support for this has been gone for some time.

> $ cat /etc/redhat-release
> Red Hat Enterprise Linux Server release 6.9 (Santiago)
> $ cat /proc/cpuinfo 
> processor       : 0
> cpu             : PPC970, altivec supported
> clock           : 1000.000000MHz
> revision        : 2.2 (pvr 0039 0202)
> 
> timebase        : 512000000
> platform        : pSeries
> model           : IBM pSeries (emulated by qemu)
> machine         : CHRP IBM pSeries (emulated by qemu)
> 
> I guess nobody uses this setup, but my understanding is that some
> rules must be followed when it comes to removing something that
> works.
> 
> https://wiki.qemu.org/Features/LegacyRemoval#Rules_for_removing_an_interface

Well, that's for "interfaces" and I'm not really sure if this qualifies.

> Maybe add a warning if 970 is used, and turn it into an error in two releases
> along with this patch ?

I really don't want to wait on that for the rma allocation stuff.
It's a hideous wart that makes it really, really hard to make other
changes to the init path (because testing if we broke it means finding
a real 970 host).

> 
> > As well as the code for handling contiguous RMAs, we can remove some
> > code to set the HIOR register, which existed on 970 but not on the
> > current and supported CPUs.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/ppc/spapr.c          | 61 +++++++++++++++----------------------------------
> >  hw/ppc/spapr_cpu_core.c |  2 --
> >  target/ppc/kvm.c        | 36 -----------------------------
> >  target/ppc/kvm_ppc.h    |  6 -----
> >  4 files changed, 19 insertions(+), 86 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 81b50af3b5..fbb2c6752c 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -2376,9 +2376,6 @@ static void spapr_machine_init(MachineState *machine)
> >      int i;
> >      MemoryRegion *sysmem = get_system_memory();
> >      MemoryRegion *ram = g_new(MemoryRegion, 1);
> > -    MemoryRegion *rma_region;
> > -    void *rma = NULL;
> > -    hwaddr rma_alloc_size;
> >      hwaddr node0_size = spapr_node0_size(machine);
> >      long load_limit, fw_size;
> >      char *filename;
> > @@ -2417,40 +2414,28 @@ static void spapr_machine_init(MachineState *machine)
> >          exit(1);
> >      }
> >  
> > -    /* Allocate RMA if necessary */
> > -    rma_alloc_size = kvmppc_alloc_rma(&rma);
> > +    spapr->rma_size = node0_size;
> >  
> > -    if (rma_alloc_size == -1) {
> > -        error_report("Unable to create RMA");
> > -        exit(1);
> > +    /* With KVM, we don't actually know whether KVM supports an
> > +     * unbounded RMA (PR KVM) or is limited by the hash table size (HV
> > +     * KVM using VRMA), so we always assume the latter
> > +     *
> > +     * In that case, we also limit the initial allocations for RTAS
> > +     * etc... to 256M since we have no way to know what the VRMA size
> > +     * is going to be as it depends on the size of the hash table
> > +     * isn't determined yet.
> > +     */
> > +    if (kvm_enabled()) {
> > +        spapr->vrma_adjust = 1;
> > +        spapr->rma_size = MIN(spapr->rma_size, 0x10000000);
> >      }
> >  
> > -    if (rma_alloc_size && (rma_alloc_size < node0_size)) {
> > -        spapr->rma_size = rma_alloc_size;
> > -    } else {
> > -        spapr->rma_size = node0_size;
> > -
> > -        /* With KVM, we don't actually know whether KVM supports an
> > -         * unbounded RMA (PR KVM) or is limited by the hash table size
> > -         * (HV KVM using VRMA), so we always assume the latter
> > -         *
> > -         * In that case, we also limit the initial allocations for RTAS
> > -         * etc... to 256M since we have no way to know what the VRMA size
> > -         * is going to be as it depends on the size of the hash table
> > -         * isn't determined yet.
> > -         */
> > -        if (kvm_enabled()) {
> > -            spapr->vrma_adjust = 1;
> > -            spapr->rma_size = MIN(spapr->rma_size, 0x10000000);
> > -        }
> > -
> > -        /* Actually we don't support unbounded RMA anymore since we
> > -         * added proper emulation of HV mode. The max we can get is
> > -         * 16G which also happens to be what we configure for PAPR
> > -         * mode so make sure we don't do anything bigger than that
> > -         */
> > -        spapr->rma_size = MIN(spapr->rma_size, 0x400000000ull);
> > -    }
> > +    /* Actually we don't support unbounded RMA anymore since we
> > +     * added proper emulation of HV mode. The max we can get is
> > +     * 16G which also happens to be what we configure for PAPR
> > +     * mode so make sure we don't do anything bigger than that
> > +     */
> > +    spapr->rma_size = MIN(spapr->rma_size, 0x400000000ull);
> >  
> >      if (spapr->rma_size > node0_size) {
> >          error_report("Numa node 0 has to span the RMA (%#08"HWADDR_PRIx")",
> > @@ -2508,14 +2493,6 @@ static void spapr_machine_init(MachineState *machine)
> >                                           machine->ram_size);
> >      memory_region_add_subregion(sysmem, 0, ram);
> >  
> > -    if (rma_alloc_size && rma) {
> > -        rma_region = g_new(MemoryRegion, 1);
> > -        memory_region_init_ram_ptr(rma_region, NULL, "ppc_spapr.rma",
> > -                                   rma_alloc_size, rma);
> > -        vmstate_register_ram_global(rma_region);
> > -        memory_region_add_subregion(sysmem, 0, rma_region);
> > -    }
> > -
> >      /* initialize hotplug memory address space */
> >      if (machine->ram_size < machine->maxram_size) {
> >          ram_addr_t hotplug_mem_size = machine->maxram_size - machine->ram_size;
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > index aa17626cda..f39d99a8da 100644
> > --- a/hw/ppc/spapr_cpu_core.c
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -36,8 +36,6 @@ static void spapr_cpu_reset(void *opaque)
> >       * using an RTAS call */
> >      cs->halted = 1;
> >  
> > -    env->spr[SPR_HIOR] = 0;
> > -
> >      /* Disable Power-saving mode Exit Cause exceptions for the CPU.
> >       * This can cause issues when rebooting the guest if a secondary
> >       * is awaken */
> > diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> > index 6de59c5b21..51177a8e00 100644
> > --- a/target/ppc/kvm.c
> > +++ b/target/ppc/kvm.c
> > @@ -2159,42 +2159,6 @@ void kvmppc_hint_smt_possible(Error **errp)
> >  
> >  
> >  #ifdef TARGET_PPC64
> > -off_t kvmppc_alloc_rma(void **rma)
> > -{
> > -    off_t size;
> > -    int fd;
> > -    struct kvm_allocate_rma ret;
> > -
> > -    /* If cap_ppc_rma == 0, contiguous RMA allocation is not supported
> > -     * if cap_ppc_rma == 1, contiguous RMA allocation is supported, but
> > -     *                      not necessary on this hardware
> > -     * if cap_ppc_rma == 2, contiguous RMA allocation is needed on this hardware
> > -     *
> > -     * FIXME: We should allow the user to force contiguous RMA
> > -     * allocation in the cap_ppc_rma==1 case.
> > -     */
> > -    if (cap_ppc_rma < 2) {
> > -        return 0;
> > -    }
> > -
> > -    fd = kvm_vm_ioctl(kvm_state, KVM_ALLOCATE_RMA, &ret);
> > -    if (fd < 0) {
> > -        fprintf(stderr, "KVM: Error on KVM_ALLOCATE_RMA: %s\n",
> > -                strerror(errno));
> > -        return -1;
> > -    }
> > -
> > -    size = MIN(ret.rma_size, 256ul << 20);
> > -
> > -    *rma = mmap(NULL, size, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0);
> > -    if (*rma == MAP_FAILED) {
> > -        fprintf(stderr, "KVM: Error mapping RMA: %s\n", strerror(errno));
> > -        return -1;
> > -    };
> > -
> > -    return size;
> > -}
> > -
> >  uint64_t kvmppc_rma_size(uint64_t current_size, unsigned int hash_shift)
> >  {
> >      struct kvm_ppc_smmu_info info;
> > diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
> > index 4d2789eef6..e2840e1d33 100644
> > --- a/target/ppc/kvm_ppc.h
> > +++ b/target/ppc/kvm_ppc.h
> > @@ -37,7 +37,6 @@ target_ulong kvmppc_configure_v3_mmu(PowerPCCPU *cpu,
> >                                       bool radix, bool gtse,
> >                                       uint64_t proc_tbl);
> >  #ifndef CONFIG_USER_ONLY
> > -off_t kvmppc_alloc_rma(void **rma);
> >  bool kvmppc_spapr_use_multitce(void);
> >  int kvmppc_spapr_enable_inkernel_multitce(void);
> >  void *kvmppc_create_spapr_tce(uint32_t liobn, uint32_t page_shift,
> > @@ -188,11 +187,6 @@ static inline target_ulong kvmppc_configure_v3_mmu(PowerPCCPU *cpu,
> >  }
> >  
> >  #ifndef CONFIG_USER_ONLY
> > -static inline off_t kvmppc_alloc_rma(void **rma)
> > -{
> > -    return 0;
> > -}
> > -
> >  static inline bool kvmppc_spapr_use_multitce(void)
> >  {
> >      return false;
> 

-- 
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] [Qemu-ppc] [PATCH for-2.13 02/10] spapr: Remove support for PowerPC 970 with pseries machine type

[luigi burdo]

(because testing if we broke it means finding
a real 970 host).

Hi, i have a real 970 host if you need an hand to test just ask

Luigi

Re: [Qemu-devel] [Qemu-ppc] [PATCH for-2.13 02/10] spapr: Remove support for PowerPC 970 with pseries machine type

[David Gibson]

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

On Fri, Apr 20, 2018 at 06:48:11AM +0000, luigi burdo wrote:
> (because testing if we broke it means finding
> a real 970 host).
> 
> Hi, i have a real 970 host if you need an hand to test just ask

Thanks, and I'll keep it in mind, but borrowing a system from some
random person isn't really a manageable approach for the long term.

-- 
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 for-2.13 02/10] spapr: Remove support for PowerPC 970 with pseries machine type

[Greg Kurz]

On Tue, 17 Apr 2018 17:17:14 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> Current POWER cpus allow for a VRMA, a special mapping which describes a
> guest's view of memory when in real mode (MMU off, from the guest's point
> of view).  Older cpus didn't have that which meant that to support a guest
> a special host-contiguous region of memory was needed to give the guest its
> Real Mode Area (RMA).
> 
> This was useful in the early days of KVM on Power to allow it to be tested
> on PowerPC 970 chips as used in Macintosh G5 machines.  Now, however, those
> machines are so old as to be irrelevant, and the host kernel has long since
> dropped support for this mode.  It hasn't been tested in ages either.
> 
> So, to simplify the code, drop the support from qemu as well.
> 
> As well as the code for handling contiguous RMAs, we can remove some
> code to set the HIOR register, which existed on 970 but not on the
> current and supported CPUs.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---

Regardless of the discussion on the deprecation process, just
a cosmetic remark...

>  hw/ppc/spapr.c          | 61 +++++++++++++++----------------------------------
>  hw/ppc/spapr_cpu_core.c |  2 --
>  target/ppc/kvm.c        | 36 -----------------------------
>  target/ppc/kvm_ppc.h    |  6 -----
>  4 files changed, 19 insertions(+), 86 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 81b50af3b5..fbb2c6752c 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2376,9 +2376,6 @@ static void spapr_machine_init(MachineState *machine)
>      int i;
>      MemoryRegion *sysmem = get_system_memory();
>      MemoryRegion *ram = g_new(MemoryRegion, 1);
> -    MemoryRegion *rma_region;
> -    void *rma = NULL;
> -    hwaddr rma_alloc_size;
>      hwaddr node0_size = spapr_node0_size(machine);
>      long load_limit, fw_size;
>      char *filename;
> @@ -2417,40 +2414,28 @@ static void spapr_machine_init(MachineState *machine)
>          exit(1);
>      }
>  
> -    /* Allocate RMA if necessary */
> -    rma_alloc_size = kvmppc_alloc_rma(&rma);
> +    spapr->rma_size = node0_size;
>  
> -    if (rma_alloc_size == -1) {
> -        error_report("Unable to create RMA");
> -        exit(1);
> +    /* With KVM, we don't actually know whether KVM supports an
> +     * unbounded RMA (PR KVM) or is limited by the hash table size (HV
> +     * KVM using VRMA), so we always assume the latter
> +     *
> +     * In that case, we also limit the initial allocations for RTAS
> +     * etc... to 256M since we have no way to know what the VRMA size
> +     * is going to be as it depends on the size of the hash table
> +     * isn't determined yet.

... maybe s/isn't/which isn't/ while at it ?

> +     */
> +    if (kvm_enabled()) {
> +        spapr->vrma_adjust = 1;
> +        spapr->rma_size = MIN(spapr->rma_size, 0x10000000);
>      }
>  
> -    if (rma_alloc_size && (rma_alloc_size < node0_size)) {
> -        spapr->rma_size = rma_alloc_size;
> -    } else {
> -        spapr->rma_size = node0_size;
> -
> -        /* With KVM, we don't actually know whether KVM supports an
> -         * unbounded RMA (PR KVM) or is limited by the hash table size
> -         * (HV KVM using VRMA), so we always assume the latter
> -         *
> -         * In that case, we also limit the initial allocations for RTAS
> -         * etc... to 256M since we have no way to know what the VRMA size
> -         * is going to be as it depends on the size of the hash table
> -         * isn't determined yet.
> -         */
> -        if (kvm_enabled()) {
> -            spapr->vrma_adjust = 1;
> -            spapr->rma_size = MIN(spapr->rma_size, 0x10000000);
> -        }
> -
> -        /* Actually we don't support unbounded RMA anymore since we
> -         * added proper emulation of HV mode. The max we can get is
> -         * 16G which also happens to be what we configure for PAPR
> -         * mode so make sure we don't do anything bigger than that
> -         */
> -        spapr->rma_size = MIN(spapr->rma_size, 0x400000000ull);
> -    }
> +    /* Actually we don't support unbounded RMA anymore since we
> +     * added proper emulation of HV mode. The max we can get is
> +     * 16G which also happens to be what we configure for PAPR
> +     * mode so make sure we don't do anything bigger than that
> +     */
> +    spapr->rma_size = MIN(spapr->rma_size, 0x400000000ull);
>  
>      if (spapr->rma_size > node0_size) {
>          error_report("Numa node 0 has to span the RMA (%#08"HWADDR_PRIx")",
> @@ -2508,14 +2493,6 @@ static void spapr_machine_init(MachineState *machine)
>                                           machine->ram_size);
>      memory_region_add_subregion(sysmem, 0, ram);
>  
> -    if (rma_alloc_size && rma) {
> -        rma_region = g_new(MemoryRegion, 1);
> -        memory_region_init_ram_ptr(rma_region, NULL, "ppc_spapr.rma",
> -                                   rma_alloc_size, rma);
> -        vmstate_register_ram_global(rma_region);
> -        memory_region_add_subregion(sysmem, 0, rma_region);
> -    }
> -
>      /* initialize hotplug memory address space */
>      if (machine->ram_size < machine->maxram_size) {
>          ram_addr_t hotplug_mem_size = machine->maxram_size - machine->ram_size;
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index aa17626cda..f39d99a8da 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -36,8 +36,6 @@ static void spapr_cpu_reset(void *opaque)
>       * using an RTAS call */
>      cs->halted = 1;
>  
> -    env->spr[SPR_HIOR] = 0;
> -
>      /* Disable Power-saving mode Exit Cause exceptions for the CPU.
>       * This can cause issues when rebooting the guest if a secondary
>       * is awaken */
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 6de59c5b21..51177a8e00 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -2159,42 +2159,6 @@ void kvmppc_hint_smt_possible(Error **errp)
>  
>  
>  #ifdef TARGET_PPC64
> -off_t kvmppc_alloc_rma(void **rma)
> -{
> -    off_t size;
> -    int fd;
> -    struct kvm_allocate_rma ret;
> -
> -    /* If cap_ppc_rma == 0, contiguous RMA allocation is not supported
> -     * if cap_ppc_rma == 1, contiguous RMA allocation is supported, but
> -     *                      not necessary on this hardware
> -     * if cap_ppc_rma == 2, contiguous RMA allocation is needed on this hardware
> -     *
> -     * FIXME: We should allow the user to force contiguous RMA
> -     * allocation in the cap_ppc_rma==1 case.
> -     */
> -    if (cap_ppc_rma < 2) {
> -        return 0;
> -    }
> -
> -    fd = kvm_vm_ioctl(kvm_state, KVM_ALLOCATE_RMA, &ret);
> -    if (fd < 0) {
> -        fprintf(stderr, "KVM: Error on KVM_ALLOCATE_RMA: %s\n",
> -                strerror(errno));
> -        return -1;
> -    }
> -
> -    size = MIN(ret.rma_size, 256ul << 20);
> -
> -    *rma = mmap(NULL, size, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0);
> -    if (*rma == MAP_FAILED) {
> -        fprintf(stderr, "KVM: Error mapping RMA: %s\n", strerror(errno));
> -        return -1;
> -    };
> -
> -    return size;
> -}
> -
>  uint64_t kvmppc_rma_size(uint64_t current_size, unsigned int hash_shift)
>  {
>      struct kvm_ppc_smmu_info info;
> diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
> index 4d2789eef6..e2840e1d33 100644
> --- a/target/ppc/kvm_ppc.h
> +++ b/target/ppc/kvm_ppc.h
> @@ -37,7 +37,6 @@ target_ulong kvmppc_configure_v3_mmu(PowerPCCPU *cpu,
>                                       bool radix, bool gtse,
>                                       uint64_t proc_tbl);
>  #ifndef CONFIG_USER_ONLY
> -off_t kvmppc_alloc_rma(void **rma);
>  bool kvmppc_spapr_use_multitce(void);
>  int kvmppc_spapr_enable_inkernel_multitce(void);
>  void *kvmppc_create_spapr_tce(uint32_t liobn, uint32_t page_shift,
> @@ -188,11 +187,6 @@ static inline target_ulong kvmppc_configure_v3_mmu(PowerPCCPU *cpu,
>  }
>  
>  #ifndef CONFIG_USER_ONLY
> -static inline off_t kvmppc_alloc_rma(void **rma)
> -{
> -    return 0;
> -}
> -
>  static inline bool kvmppc_spapr_use_multitce(void)
>  {
>      return false;

Re: [Qemu-devel] [PATCH for-2.13 02/10] spapr: Remove support for PowerPC 970 with pseries machine type

[David Gibson]

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

On Fri, Apr 20, 2018 at 02:25:23PM +0200, Greg Kurz wrote:
> On Tue, 17 Apr 2018 17:17:14 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > Current POWER cpus allow for a VRMA, a special mapping which describes a
> > guest's view of memory when in real mode (MMU off, from the guest's point
> > of view).  Older cpus didn't have that which meant that to support a guest
> > a special host-contiguous region of memory was needed to give the guest its
> > Real Mode Area (RMA).
> > 
> > This was useful in the early days of KVM on Power to allow it to be tested
> > on PowerPC 970 chips as used in Macintosh G5 machines.  Now, however, those
> > machines are so old as to be irrelevant, and the host kernel has long since
> > dropped support for this mode.  It hasn't been tested in ages either.
> > 
> > So, to simplify the code, drop the support from qemu as well.
> > 
> > As well as the code for handling contiguous RMAs, we can remove some
> > code to set the HIOR register, which existed on 970 but not on the
> > current and supported CPUs.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> 
> Regardless of the discussion on the deprecation process, just
> a cosmetic remark...
> 
> >  hw/ppc/spapr.c          | 61 +++++++++++++++----------------------------------
> >  hw/ppc/spapr_cpu_core.c |  2 --
> >  target/ppc/kvm.c        | 36 -----------------------------
> >  target/ppc/kvm_ppc.h    |  6 -----
> >  4 files changed, 19 insertions(+), 86 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 81b50af3b5..fbb2c6752c 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -2376,9 +2376,6 @@ static void spapr_machine_init(MachineState *machine)
> >      int i;
> >      MemoryRegion *sysmem = get_system_memory();
> >      MemoryRegion *ram = g_new(MemoryRegion, 1);
> > -    MemoryRegion *rma_region;
> > -    void *rma = NULL;
> > -    hwaddr rma_alloc_size;
> >      hwaddr node0_size = spapr_node0_size(machine);
> >      long load_limit, fw_size;
> >      char *filename;
> > @@ -2417,40 +2414,28 @@ static void spapr_machine_init(MachineState *machine)
> >          exit(1);
> >      }
> >  
> > -    /* Allocate RMA if necessary */
> > -    rma_alloc_size = kvmppc_alloc_rma(&rma);
> > +    spapr->rma_size = node0_size;
> >  
> > -    if (rma_alloc_size == -1) {
> > -        error_report("Unable to create RMA");
> > -        exit(1);
> > +    /* With KVM, we don't actually know whether KVM supports an
> > +     * unbounded RMA (PR KVM) or is limited by the hash table size (HV
> > +     * KVM using VRMA), so we always assume the latter
> > +     *
> > +     * In that case, we also limit the initial allocations for RTAS
> > +     * etc... to 256M since we have no way to know what the VRMA size
> > +     * is going to be as it depends on the size of the hash table
> > +     * isn't determined yet.
> 
> ... maybe s/isn't/which isn't/ while at it ?

Thanks, I've made that change.

-- 
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 for-2.13 03/10] target/ppc: Remove unnecessary initialization of LPCR_UPRT

[David Gibson]

In cpu_ppc_set_papr() the UPRT and GTSE bits of the LPCR are initialized
based on on ppc64_radix_guest().  Which seems reasonable, except that
ppc64_radix_guest() is based on spapr->patb_entry which is only set up
in spapr_machine_reset, called much later than cpu_ppc_set_papr().

So the initialization here is pointless.  The base cpu initialization
already sets a value that's good enough until the guest uses an hcall to
configure it's preferred MMU mode.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 target/ppc/translate_init.c | 16 ----------------
 1 file changed, 16 deletions(-)

diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
index bb79d23b50..14f346f441 100644
--- a/target/ppc/translate_init.c
+++ b/target/ppc/translate_init.c
@@ -8897,22 +8897,6 @@ void cpu_ppc_set_papr(PowerPCCPU *cpu, PPCVirtualHypervisor *vhyp)
     lpcr->default_value &= ~LPCR_RMLS;
     lpcr->default_value |= 1ull << LPCR_RMLS_SHIFT;
 
-    if (env->mmu_model == POWERPC_MMU_3_00) {
-        /* By default we choose legacy mode and switch to new hash or radix
-         * when a register process table hcall is made. So disable process
-         * tables and guest translation shootdown by default
-         *
-         * Hot-plugged CPUs inherit from the guest radix setting under
-         * KVM but not under TCG. Update the default LPCR to keep new
-         * CPUs in sync when radix is enabled.
-         */
-        if (ppc64_radix_guest(cpu)) {
-            lpcr->default_value |= LPCR_UPRT | LPCR_GTSE;
-        } else {
-            lpcr->default_value &= ~(LPCR_UPRT | LPCR_GTSE);
-        }
-    }
-
     /* Only enable Power-saving mode Exit Cause exceptions on the boot
      * CPU. The RTAS command start-cpu will enable them on secondaries.
      */
-- 
2.14.3

Re: [Qemu-devel] [PATCH for-2.13 03/10] target/ppc: Remove unnecessary initialization of LPCR_UPRT

[Greg Kurz]

On Tue, 17 Apr 2018 17:17:15 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> In cpu_ppc_set_papr() the UPRT and GTSE bits of the LPCR are initialized
> based on on ppc64_radix_guest().  Which seems reasonable, except that
> ppc64_radix_guest() is based on spapr->patb_entry which is only set up
> in spapr_machine_reset, called much later than cpu_ppc_set_papr().
> 
> So the initialization here is pointless.  The base cpu initialization
> already sets a value that's good enough until the guest uses an hcall to
> configure it's preferred MMU mode.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  target/ppc/translate_init.c | 16 ----------------
>  1 file changed, 16 deletions(-)
> 
> diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
> index bb79d23b50..14f346f441 100644
> --- a/target/ppc/translate_init.c
> +++ b/target/ppc/translate_init.c
> @@ -8897,22 +8897,6 @@ void cpu_ppc_set_papr(PowerPCCPU *cpu, PPCVirtualHypervisor *vhyp)
>      lpcr->default_value &= ~LPCR_RMLS;
>      lpcr->default_value |= 1ull << LPCR_RMLS_SHIFT;
>  
> -    if (env->mmu_model == POWERPC_MMU_3_00) {
> -        /* By default we choose legacy mode and switch to new hash or radix
> -         * when a register process table hcall is made. So disable process
> -         * tables and guest translation shootdown by default
> -         *
> -         * Hot-plugged CPUs inherit from the guest radix setting under
> -         * KVM but not under TCG. Update the default LPCR to keep new
> -         * CPUs in sync when radix is enabled.
> -         */
> -        if (ppc64_radix_guest(cpu)) {
> -            lpcr->default_value |= LPCR_UPRT | LPCR_GTSE;
> -        } else {
> -            lpcr->default_value &= ~(LPCR_UPRT | LPCR_GTSE);
> -        }
> -    }
> -
>      /* Only enable Power-saving mode Exit Cause exceptions on the boot
>       * CPU. The RTAS command start-cpu will enable them on secondaries.
>       */

Re: [Qemu-devel] [PATCH for-2.13 03/10] target/ppc: Remove unnecessary initialization of LPCR_UPRT

[David Gibson]

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

On Fri, Apr 20, 2018 at 01:34:14PM +0200, Greg Kurz wrote:
> On Tue, 17 Apr 2018 17:17:15 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > In cpu_ppc_set_papr() the UPRT and GTSE bits of the LPCR are initialized
> > based on on ppc64_radix_guest().  Which seems reasonable, except that
> > ppc64_radix_guest() is based on spapr->patb_entry which is only set up
> > in spapr_machine_reset, called much later than cpu_ppc_set_papr().
> > 
> > So the initialization here is pointless.  The base cpu initialization
> > already sets a value that's good enough until the guest uses an hcall to
> > configure it's preferred MMU mode.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> 
> Reviewed-by: Greg Kurz <groug@kaod.org>

Merged to ppc-for-2.13.

-- 
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] [Qemu-ppc] [PATCH for-2.13 03/10] target/ppc: Remove unnecessary initialization of LPCR_UPRT

[Cédric Le Goater]

On 04/17/2018 09:17 AM, David Gibson wrote:
> In cpu_ppc_set_papr() the UPRT and GTSE bits of the LPCR are initialized
> based on on ppc64_radix_guest().  Which seems reasonable, except that
> ppc64_radix_guest() is based on spapr->patb_entry which is only set up
> in spapr_machine_reset, called much later than cpu_ppc_set_papr().
> 
> So the initialization here is pointless.  The base cpu initialization
> already sets a value that's good enough until the guest uses an hcall to
> configure it's preferred MMU mode.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  target/ppc/translate_init.c | 16 ----------------
>  1 file changed, 16 deletions(-)
> 
> diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
> index bb79d23b50..14f346f441 100644
> --- a/target/ppc/translate_init.c
> +++ b/target/ppc/translate_init.c
> @@ -8897,22 +8897,6 @@ void cpu_ppc_set_papr(PowerPCCPU *cpu, PPCVirtualHypervisor *vhyp)
>      lpcr->default_value &= ~LPCR_RMLS;
>      lpcr->default_value |= 1ull << LPCR_RMLS_SHIFT;
>  
> -    if (env->mmu_model == POWERPC_MMU_3_00) {
> -        /* By default we choose legacy mode and switch to new hash or radix
> -         * when a register process table hcall is made. So disable process
> -         * tables and guest translation shootdown by default
> -         *
> -         * Hot-plugged CPUs inherit from the guest radix setting under
> -         * KVM but not under TCG. Update the default LPCR to keep new
> -         * CPUs in sync when radix is enabled.
> -         */


This is breaking CPU hotplug under TCG. Should we reintroduce the same 
settings in spapr_cpu_reset() now ?

Thanks,

C. 


> -        if (ppc64_radix_guest(cpu)) {
> -            lpcr->default_value |= LPCR_UPRT | LPCR_GTSE;
> -        } else {
> -            lpcr->default_value &= ~(LPCR_UPRT | LPCR_GTSE);
> -        }
> -    }
> -
>      /* Only enable Power-saving mode Exit Cause exceptions on the boot
>       * CPU. The RTAS command start-cpu will enable them on secondaries.
>       */
> 

Re: [Qemu-devel] [Qemu-ppc] [PATCH for-2.13 03/10] target/ppc: Remove unnecessary initialization of LPCR_UPRT

[David Gibson]

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

On Wed, Apr 25, 2018 at 11:52:18AM +0200, Cédric Le Goater wrote:
> On 04/17/2018 09:17 AM, David Gibson wrote:
> > In cpu_ppc_set_papr() the UPRT and GTSE bits of the LPCR are initialized
> > based on on ppc64_radix_guest().  Which seems reasonable, except that
> > ppc64_radix_guest() is based on spapr->patb_entry which is only set up
> > in spapr_machine_reset, called much later than cpu_ppc_set_papr().
> > 
> > So the initialization here is pointless.  The base cpu initialization
> > already sets a value that's good enough until the guest uses an hcall to
> > configure it's preferred MMU mode.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  target/ppc/translate_init.c | 16 ----------------
> >  1 file changed, 16 deletions(-)
> > 
> > diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
> > index bb79d23b50..14f346f441 100644
> > --- a/target/ppc/translate_init.c
> > +++ b/target/ppc/translate_init.c
> > @@ -8897,22 +8897,6 @@ void cpu_ppc_set_papr(PowerPCCPU *cpu, PPCVirtualHypervisor *vhyp)
> >      lpcr->default_value &= ~LPCR_RMLS;
> >      lpcr->default_value |= 1ull << LPCR_RMLS_SHIFT;
> >  
> > -    if (env->mmu_model == POWERPC_MMU_3_00) {
> > -        /* By default we choose legacy mode and switch to new hash or radix
> > -         * when a register process table hcall is made. So disable process
> > -         * tables and guest translation shootdown by default
> > -         *
> > -         * Hot-plugged CPUs inherit from the guest radix setting under
> > -         * KVM but not under TCG. Update the default LPCR to keep new
> > -         * CPUs in sync when radix is enabled.
> > -         */
> 
> 
> This is breaking CPU hotplug under TCG. Should we reintroduce the same 
> settings in spapr_cpu_reset() now ?

Sod.  Yeah, this code is important for the hotplug case.

But, no, I don't think it should go into reset, I think it should go
into the rtas start-cpu path; it only makes sense for secondary CPUs.

-- 
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] [Qemu-ppc] [PATCH for-2.13 03/10] target/ppc: Remove unnecessary initialization of LPCR_UPRT

[Cédric Le Goater]

On 04/26/2018 08:46 AM, David Gibson wrote:
> On Wed, Apr 25, 2018 at 11:52:18AM +0200, Cédric Le Goater wrote:
>> On 04/17/2018 09:17 AM, David Gibson wrote:
>>> In cpu_ppc_set_papr() the UPRT and GTSE bits of the LPCR are initialized
>>> based on on ppc64_radix_guest().  Which seems reasonable, except that
>>> ppc64_radix_guest() is based on spapr->patb_entry which is only set up
>>> in spapr_machine_reset, called much later than cpu_ppc_set_papr().
>>>
>>> So the initialization here is pointless.  The base cpu initialization
>>> already sets a value that's good enough until the guest uses an hcall to
>>> configure it's preferred MMU mode.
>>>
>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>>> ---
>>>  target/ppc/translate_init.c | 16 ----------------
>>>  1 file changed, 16 deletions(-)
>>>
>>> diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
>>> index bb79d23b50..14f346f441 100644
>>> --- a/target/ppc/translate_init.c
>>> +++ b/target/ppc/translate_init.c
>>> @@ -8897,22 +8897,6 @@ void cpu_ppc_set_papr(PowerPCCPU *cpu, PPCVirtualHypervisor *vhyp)
>>>      lpcr->default_value &= ~LPCR_RMLS;
>>>      lpcr->default_value |= 1ull << LPCR_RMLS_SHIFT;
>>>  
>>> -    if (env->mmu_model == POWERPC_MMU_3_00) {
>>> -        /* By default we choose legacy mode and switch to new hash or radix
>>> -         * when a register process table hcall is made. So disable process
>>> -         * tables and guest translation shootdown by default
>>> -         *
>>> -         * Hot-plugged CPUs inherit from the guest radix setting under
>>> -         * KVM but not under TCG. Update the default LPCR to keep new
>>> -         * CPUs in sync when radix is enabled.
>>> -         */
>>
>>
>> This is breaking CPU hotplug under TCG. Should we reintroduce the same 
>> settings in spapr_cpu_reset() now ?
> 
> Sod.  Yeah, this code is important for the hotplug case.
> 
> But, no, I don't think it should go into reset, I think it should go
> into the rtas start-cpu path; it only makes sense for secondary CPUs.

yes. I will send a fix.

Thanks,

C. 

Re: [Qemu-devel] [Qemu-ppc] [PATCH for-2.13 03/10] target/ppc: Remove unnecessary initialization of LPCR_UPRT

[David Gibson]

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

On Thu, Apr 26, 2018 at 09:20:11AM +0200, Cédric Le Goater wrote:
> On 04/26/2018 08:46 AM, David Gibson wrote:
> > On Wed, Apr 25, 2018 at 11:52:18AM +0200, Cédric Le Goater wrote:
> >> On 04/17/2018 09:17 AM, David Gibson wrote:
> >>> In cpu_ppc_set_papr() the UPRT and GTSE bits of the LPCR are initialized
> >>> based on on ppc64_radix_guest().  Which seems reasonable, except that
> >>> ppc64_radix_guest() is based on spapr->patb_entry which is only set up
> >>> in spapr_machine_reset, called much later than cpu_ppc_set_papr().
> >>>
> >>> So the initialization here is pointless.  The base cpu initialization
> >>> already sets a value that's good enough until the guest uses an hcall to
> >>> configure it's preferred MMU mode.
> >>>
> >>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> >>> ---
> >>>  target/ppc/translate_init.c | 16 ----------------
> >>>  1 file changed, 16 deletions(-)
> >>>
> >>> diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
> >>> index bb79d23b50..14f346f441 100644
> >>> --- a/target/ppc/translate_init.c
> >>> +++ b/target/ppc/translate_init.c
> >>> @@ -8897,22 +8897,6 @@ void cpu_ppc_set_papr(PowerPCCPU *cpu, PPCVirtualHypervisor *vhyp)
> >>>      lpcr->default_value &= ~LPCR_RMLS;
> >>>      lpcr->default_value |= 1ull << LPCR_RMLS_SHIFT;
> >>>  
> >>> -    if (env->mmu_model == POWERPC_MMU_3_00) {
> >>> -        /* By default we choose legacy mode and switch to new hash or radix
> >>> -         * when a register process table hcall is made. So disable process
> >>> -         * tables and guest translation shootdown by default
> >>> -         *
> >>> -         * Hot-plugged CPUs inherit from the guest radix setting under
> >>> -         * KVM but not under TCG. Update the default LPCR to keep new
> >>> -         * CPUs in sync when radix is enabled.
> >>> -         */
> >>
> >>
> >> This is breaking CPU hotplug under TCG. Should we reintroduce the same 
> >> settings in spapr_cpu_reset() now ?
> > 
> > Sod.  Yeah, this code is important for the hotplug case.
> > 
> > But, no, I don't think it should go into reset, I think it should go
> > into the rtas start-cpu path; it only makes sense for secondary CPUs.
> 
> yes. I will send a fix.

Not sure if this is still on your radar, but if it is, don't bother.
I've already made a fix (along with various others) and will be
posting soonish.

-- 
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] [Qemu-ppc] [PATCH for-2.13 03/10] target/ppc: Remove unnecessary initialization of LPCR_UPRT

[Cédric Le Goater]

On 05/01/2018 08:39 AM, David Gibson wrote:
> On Thu, Apr 26, 2018 at 09:20:11AM +0200, Cédric Le Goater wrote:
>> On 04/26/2018 08:46 AM, David Gibson wrote:
>>> On Wed, Apr 25, 2018 at 11:52:18AM +0200, Cédric Le Goater wrote:
>>>> On 04/17/2018 09:17 AM, David Gibson wrote:
>>>>> In cpu_ppc_set_papr() the UPRT and GTSE bits of the LPCR are initialized
>>>>> based on on ppc64_radix_guest().  Which seems reasonable, except that
>>>>> ppc64_radix_guest() is based on spapr->patb_entry which is only set up
>>>>> in spapr_machine_reset, called much later than cpu_ppc_set_papr().
>>>>>
>>>>> So the initialization here is pointless.  The base cpu initialization
>>>>> already sets a value that's good enough until the guest uses an hcall to
>>>>> configure it's preferred MMU mode.
>>>>>
>>>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>>>>> ---
>>>>>  target/ppc/translate_init.c | 16 ----------------
>>>>>  1 file changed, 16 deletions(-)
>>>>>
>>>>> diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
>>>>> index bb79d23b50..14f346f441 100644
>>>>> --- a/target/ppc/translate_init.c
>>>>> +++ b/target/ppc/translate_init.c
>>>>> @@ -8897,22 +8897,6 @@ void cpu_ppc_set_papr(PowerPCCPU *cpu, PPCVirtualHypervisor *vhyp)
>>>>>      lpcr->default_value &= ~LPCR_RMLS;
>>>>>      lpcr->default_value |= 1ull << LPCR_RMLS_SHIFT;
>>>>>  
>>>>> -    if (env->mmu_model == POWERPC_MMU_3_00) {
>>>>> -        /* By default we choose legacy mode and switch to new hash or radix
>>>>> -         * when a register process table hcall is made. So disable process
>>>>> -         * tables and guest translation shootdown by default
>>>>> -         *
>>>>> -         * Hot-plugged CPUs inherit from the guest radix setting under
>>>>> -         * KVM but not under TCG. Update the default LPCR to keep new
>>>>> -         * CPUs in sync when radix is enabled.
>>>>> -         */
>>>>
>>>>
>>>> This is breaking CPU hotplug under TCG. Should we reintroduce the same 
>>>> settings in spapr_cpu_reset() now ?
>>>
>>> Sod.  Yeah, this code is important for the hotplug case.
>>>
>>> But, no, I don't think it should go into reset, I think it should go
>>> into the rtas start-cpu path; it only makes sense for secondary CPUs.
>>
>> yes. I will send a fix.
> 
> Not sure if this is still on your radar, but if it is, don't bother.
> I've already made a fix (along with various others) and will be
> posting soonish.

As you reverted the changes in the ppc-2.13 branch, I postponed the
changes. Go ahead. 

Thanks,

C. 

[Qemu-devel] [PATCH for-2.13 04/10] spapr: Set compatibility mode before the rest of spapr_cpu_reset()

[David Gibson]

Although the order doesn't really matter at the moment, it's possible
other initializastions could depend on the compatiblity mode, so make sure
we set it first in spapr_cpu_reset().

While we're at it drop the test against first_cpu.  Setting the compat mode
to the value it already has is redundant, but harmless, so we might as well
make a small simplification to the code.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_cpu_core.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index f39d99a8da..2aab6ccd15 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -31,6 +31,11 @@ static void spapr_cpu_reset(void *opaque)
 
     cpu_reset(cs);
 
+    /* Set compatibility mode to match the boot CPU, which was either set
+     * by the machine reset code or by CAS. This should never fail.
+     */
+    ppc_set_compat(cpu, POWERPC_CPU(first_cpu)->compat_pvr, &error_abort);
+
     /* All CPUs start halted.  CPU0 is unhalted from the machine level
      * reset code and the rest are explicitly started up by the guest
      * using an RTAS call */
@@ -43,12 +48,6 @@ static void spapr_cpu_reset(void *opaque)
         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)
-- 
2.14.3

Re: [Qemu-devel] [PATCH for-2.13 04/10] spapr: Set compatibility mode before the rest of spapr_cpu_reset()

[Greg Kurz]

On Tue, 17 Apr 2018 17:17:16 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> Although the order doesn't really matter at the moment, it's possible
> other initializastions could depend on the compatiblity mode, so make sure
> we set it first in spapr_cpu_reset().
> 
> While we're at it drop the test against first_cpu.  Setting the compat mode
> to the value it already has is redundant, but harmless, so we might as well
> make a small simplification to the code.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  hw/ppc/spapr_cpu_core.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index f39d99a8da..2aab6ccd15 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -31,6 +31,11 @@ static void spapr_cpu_reset(void *opaque)
>  
>      cpu_reset(cs);
>  
> +    /* Set compatibility mode to match the boot CPU, which was either set
> +     * by the machine reset code or by CAS. This should never fail.
> +     */
> +    ppc_set_compat(cpu, POWERPC_CPU(first_cpu)->compat_pvr, &error_abort);
> +
>      /* All CPUs start halted.  CPU0 is unhalted from the machine level
>       * reset code and the rest are explicitly started up by the guest
>       * using an RTAS call */
> @@ -43,12 +48,6 @@ static void spapr_cpu_reset(void *opaque)
>          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)

Re: [Qemu-devel] [PATCH for-2.13 04/10] spapr: Set compatibility mode before the rest of spapr_cpu_reset()

[David Gibson]

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

On Fri, Apr 20, 2018 at 11:16:27AM +0200, Greg Kurz wrote:
> On Tue, 17 Apr 2018 17:17:16 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > Although the order doesn't really matter at the moment, it's possible
> > other initializastions could depend on the compatiblity mode, so make sure
> > we set it first in spapr_cpu_reset().
> > 
> > While we're at it drop the test against first_cpu.  Setting the compat mode
> > to the value it already has is redundant, but harmless, so we might as well
> > make a small simplification to the code.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> 
> Reviewed-by: Greg Kurz <groug@kaod.org>

Ta.  This one also doesn't really depend on the others, so I've merged
it into ppc-for-2.13.

> 
> >  hw/ppc/spapr_cpu_core.c | 11 +++++------
> >  1 file changed, 5 insertions(+), 6 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > index f39d99a8da..2aab6ccd15 100644
> > --- a/hw/ppc/spapr_cpu_core.c
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -31,6 +31,11 @@ static void spapr_cpu_reset(void *opaque)
> >  
> >      cpu_reset(cs);
> >  
> > +    /* Set compatibility mode to match the boot CPU, which was either set
> > +     * by the machine reset code or by CAS. This should never fail.
> > +     */
> > +    ppc_set_compat(cpu, POWERPC_CPU(first_cpu)->compat_pvr, &error_abort);
> > +
> >      /* All CPUs start halted.  CPU0 is unhalted from the machine level
> >       * reset code and the rest are explicitly started up by the guest
> >       * using an RTAS call */
> > @@ -43,12 +48,6 @@ static void spapr_cpu_reset(void *opaque)
> >          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)
> 

-- 
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 for-2.13 05/10] spapr: Move PAPR mode register initialization to spapr code

[David Gibson]

cpu_ppc_set_papr() has code to make sure the LPCR and AMOR (hypervisor
privileged registers) have values which will make TCG behave correctly for
paravirtualized guests, where we don't emulate the cpu when in hypervisor
mode.

It does this by mangling the default values of the SPRs, so that they will
be set correctly at reset time.  Manipulating usually-static parameters of
the cpu model like this is kind of ugly, especially since the values used
really have more to do with the platform than the cpu.

The spapr code already has places for PAPR specific initializations of
register state, so move the handling of LPCR and AMOR to there.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_cpu_core.c     | 36 +++++++++++++++++++++++++++++++++++-
 target/ppc/translate_init.c | 39 ---------------------------------------
 2 files changed, 35 insertions(+), 40 deletions(-)

diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 2aab6ccd15..9080664ec1 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -28,6 +28,7 @@ static void spapr_cpu_reset(void *opaque)
     CPUState *cs = CPU(cpu);
     CPUPPCState *env = &cpu->env;
     PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
+    target_ulong lpcr;
 
     cpu_reset(cs);
 
@@ -41,13 +42,46 @@ static void spapr_cpu_reset(void *opaque)
      * using an RTAS call */
     cs->halted = 1;
 
+    lpcr = env->spr[SPR_LPCR];
+
+    /* Set emulated LPCR to not send interrupts to hypervisor. Note that
+     * under KVM, the actual HW LPCR will be set differently by KVM itself,
+     * the settings below ensure proper operations with TCG in absence of
+     * a real hypervisor.
+     *
+     * Clearing VPM0 will also cause us to use RMOR in mmu-hash64.c for
+     * real mode accesses, which thankfully defaults to 0 and isn't
+     * accessible in guest mode.
+     */
+    lpcr &= ~(LPCR_VPM0 | LPCR_VPM1 | LPCR_ISL | LPCR_KBV);
+    lpcr |= LPCR_LPES0 | LPCR_LPES1;
+
+    /* Set RMLS to the max (ie, 16G) */
+    lpcr &= ~LPCR_RMLS;
+    lpcr |= 1ull << LPCR_RMLS_SHIFT;
+
+    /* Only enable Power-saving mode Exit Cause exceptions on the boot
+     * CPU. The RTAS command start-cpu will enable them on secondaries.
+     */
+    if (cs == first_cpu) {
+        lpcr |= pcc->lpcr_pm;
+    }
+
     /* Disable Power-saving mode Exit Cause exceptions for the CPU.
      * This can cause issues when rebooting the guest if a secondary
      * is awaken */
     if (cs != first_cpu) {
-        env->spr[SPR_LPCR] &= ~pcc->lpcr_pm;
+        lpcr &= ~pcc->lpcr_pm;
     }
 
+    env->spr[SPR_LPCR] = lpcr;
+
+    /* Set a full AMOR so guest can use the AMR as it sees fit */
+    env->spr[SPR_AMOR] = 0xffffffffffffffffull;
+
+    /* Update some env bits based on new LPCR value */
+    ppc_hash64_update_rmls(cpu);
+    ppc_hash64_update_vrma(cpu);
 }
 
 static void spapr_cpu_destroy(PowerPCCPU *cpu)
diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
index 14f346f441..5e89901149 100644
--- a/target/ppc/translate_init.c
+++ b/target/ppc/translate_init.c
@@ -8866,11 +8866,7 @@ POWERPC_FAMILY(POWER9)(ObjectClass *oc, void *data)
 #if !defined(CONFIG_USER_ONLY)
 void cpu_ppc_set_papr(PowerPCCPU *cpu, PPCVirtualHypervisor *vhyp)
 {
-    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
     CPUPPCState *env = &cpu->env;
-    ppc_spr_t *lpcr = &env->spr_cb[SPR_LPCR];
-    ppc_spr_t *amor = &env->spr_cb[SPR_AMOR];
-    CPUState *cs = CPU(cpu);
 
     cpu->vhyp = vhyp;
 
@@ -8881,41 +8877,6 @@ void cpu_ppc_set_papr(PowerPCCPU *cpu, PPCVirtualHypervisor *vhyp)
      */
     env->msr_mask &= ~((1ull << MSR_EP) | MSR_HVB);
 
-    /* Set emulated LPCR to not send interrupts to hypervisor. Note that
-     * under KVM, the actual HW LPCR will be set differently by KVM itself,
-     * the settings below ensure proper operations with TCG in absence of
-     * a real hypervisor.
-     *
-     * Clearing VPM0 will also cause us to use RMOR in mmu-hash64.c for
-     * real mode accesses, which thankfully defaults to 0 and isn't
-     * accessible in guest mode.
-     */
-    lpcr->default_value &= ~(LPCR_VPM0 | LPCR_VPM1 | LPCR_ISL | LPCR_KBV);
-    lpcr->default_value |= LPCR_LPES0 | LPCR_LPES1;
-
-    /* Set RMLS to the max (ie, 16G) */
-    lpcr->default_value &= ~LPCR_RMLS;
-    lpcr->default_value |= 1ull << LPCR_RMLS_SHIFT;
-
-    /* Only enable Power-saving mode Exit Cause exceptions on the boot
-     * CPU. The RTAS command start-cpu will enable them on secondaries.
-     */
-    if (cs == first_cpu) {
-        lpcr->default_value |= pcc->lpcr_pm;
-    }
-
-    /* We should be followed by a CPU reset but update the active value
-     * just in case...
-     */
-    env->spr[SPR_LPCR] = lpcr->default_value;
-
-    /* Set a full AMOR so guest can use the AMR as it sees fit */
-    env->spr[SPR_AMOR] = amor->default_value = 0xffffffffffffffffull;
-
-    /* Update some env bits based on new LPCR value */
-    ppc_hash64_update_rmls(cpu);
-    ppc_hash64_update_vrma(cpu);
-
     /* Tell KVM that we're in PAPR mode */
     if (kvm_enabled()) {
         kvmppc_set_papr(cpu);
-- 
2.14.3

Re: [Qemu-devel] [PATCH for-2.13 05/10] spapr: Move PAPR mode register initialization to spapr code

[Greg Kurz]

On Tue, 17 Apr 2018 17:17:17 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> cpu_ppc_set_papr() has code to make sure the LPCR and AMOR (hypervisor
> privileged registers) have values which will make TCG behave correctly for
> paravirtualized guests, where we don't emulate the cpu when in hypervisor
> mode.
> 
> It does this by mangling the default values of the SPRs, so that they will
> be set correctly at reset time.  Manipulating usually-static parameters of
> the cpu model like this is kind of ugly, especially since the values used
> really have more to do with the platform than the cpu.
> 
> The spapr code already has places for PAPR specific initializations of
> register state, so move the handling of LPCR and AMOR to there.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr_cpu_core.c     | 36 +++++++++++++++++++++++++++++++++++-
>  target/ppc/translate_init.c | 39 ---------------------------------------
>  2 files changed, 35 insertions(+), 40 deletions(-)
> 
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 2aab6ccd15..9080664ec1 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -28,6 +28,7 @@ static void spapr_cpu_reset(void *opaque)
>      CPUState *cs = CPU(cpu);
>      CPUPPCState *env = &cpu->env;
>      PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> +    target_ulong lpcr;
>  
>      cpu_reset(cs);
>  
> @@ -41,13 +42,46 @@ static void spapr_cpu_reset(void *opaque)
>       * using an RTAS call */
>      cs->halted = 1;
>  
> +    lpcr = env->spr[SPR_LPCR];
> +
> +    /* Set emulated LPCR to not send interrupts to hypervisor. Note that
> +     * under KVM, the actual HW LPCR will be set differently by KVM itself,
> +     * the settings below ensure proper operations with TCG in absence of
> +     * a real hypervisor.
> +     *
> +     * Clearing VPM0 will also cause us to use RMOR in mmu-hash64.c for
> +     * real mode accesses, which thankfully defaults to 0 and isn't
> +     * accessible in guest mode.
> +     */
> +    lpcr &= ~(LPCR_VPM0 | LPCR_VPM1 | LPCR_ISL | LPCR_KBV);
> +    lpcr |= LPCR_LPES0 | LPCR_LPES1;
> +
> +    /* Set RMLS to the max (ie, 16G) */
> +    lpcr &= ~LPCR_RMLS;
> +    lpcr |= 1ull << LPCR_RMLS_SHIFT;
> +
> +    /* Only enable Power-saving mode Exit Cause exceptions on the boot
> +     * CPU. The RTAS command start-cpu will enable them on secondaries.
> +     */
> +    if (cs == first_cpu) {
> +        lpcr |= pcc->lpcr_pm;
> +    }
> +
>      /* Disable Power-saving mode Exit Cause exceptions for the CPU.
>       * This can cause issues when rebooting the guest if a secondary
>       * is awaken */
>      if (cs != first_cpu) {
> -        env->spr[SPR_LPCR] &= ~pcc->lpcr_pm;
> +        lpcr &= ~pcc->lpcr_pm;
>      }
>  

Maybe fold these into a single if-else with a consolidated comment ?

    /* Only enable Power-saving mode Exit Cause exceptions on the boot
     * CPU and disable it for secondaries because it is known to cause
     * issues if a secondary is awaken too early when rebooting the guest.
     * The RTAS command start-cpu will enable exceptions on secondaries.
     */
    if (cs == first_cpu) {
        lpcr |= pcc->lpcr_pm;
    } else {
        lpcr &= ~pcc->lpcr_pm;
    }

Anyway, the patch looks good and makes cpu_ppc_set_papr() a lot clearer.

Reviewed-by: Greg Kurz <groug@kaod.org>

> +    env->spr[SPR_LPCR] = lpcr;
> +
> +    /* Set a full AMOR so guest can use the AMR as it sees fit */
> +    env->spr[SPR_AMOR] = 0xffffffffffffffffull;
> +
> +    /* Update some env bits based on new LPCR value */
> +    ppc_hash64_update_rmls(cpu);
> +    ppc_hash64_update_vrma(cpu);
>  }
>  
>  static void spapr_cpu_destroy(PowerPCCPU *cpu)
> diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
> index 14f346f441..5e89901149 100644
> --- a/target/ppc/translate_init.c
> +++ b/target/ppc/translate_init.c
> @@ -8866,11 +8866,7 @@ POWERPC_FAMILY(POWER9)(ObjectClass *oc, void *data)
>  #if !defined(CONFIG_USER_ONLY)
>  void cpu_ppc_set_papr(PowerPCCPU *cpu, PPCVirtualHypervisor *vhyp)
>  {
> -    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
>      CPUPPCState *env = &cpu->env;
> -    ppc_spr_t *lpcr = &env->spr_cb[SPR_LPCR];
> -    ppc_spr_t *amor = &env->spr_cb[SPR_AMOR];
> -    CPUState *cs = CPU(cpu);
>  
>      cpu->vhyp = vhyp;
>  
> @@ -8881,41 +8877,6 @@ void cpu_ppc_set_papr(PowerPCCPU *cpu, PPCVirtualHypervisor *vhyp)
>       */
>      env->msr_mask &= ~((1ull << MSR_EP) | MSR_HVB);
>  
> -    /* Set emulated LPCR to not send interrupts to hypervisor. Note that
> -     * under KVM, the actual HW LPCR will be set differently by KVM itself,
> -     * the settings below ensure proper operations with TCG in absence of
> -     * a real hypervisor.
> -     *
> -     * Clearing VPM0 will also cause us to use RMOR in mmu-hash64.c for
> -     * real mode accesses, which thankfully defaults to 0 and isn't
> -     * accessible in guest mode.
> -     */
> -    lpcr->default_value &= ~(LPCR_VPM0 | LPCR_VPM1 | LPCR_ISL | LPCR_KBV);
> -    lpcr->default_value |= LPCR_LPES0 | LPCR_LPES1;
> -
> -    /* Set RMLS to the max (ie, 16G) */
> -    lpcr->default_value &= ~LPCR_RMLS;
> -    lpcr->default_value |= 1ull << LPCR_RMLS_SHIFT;
> -
> -    /* Only enable Power-saving mode Exit Cause exceptions on the boot
> -     * CPU. The RTAS command start-cpu will enable them on secondaries.
> -     */
> -    if (cs == first_cpu) {
> -        lpcr->default_value |= pcc->lpcr_pm;
> -    }
> -
> -    /* We should be followed by a CPU reset but update the active value
> -     * just in case...
> -     */
> -    env->spr[SPR_LPCR] = lpcr->default_value;
> -
> -    /* Set a full AMOR so guest can use the AMR as it sees fit */
> -    env->spr[SPR_AMOR] = amor->default_value = 0xffffffffffffffffull;
> -
> -    /* Update some env bits based on new LPCR value */
> -    ppc_hash64_update_rmls(cpu);
> -    ppc_hash64_update_vrma(cpu);
> -
>      /* Tell KVM that we're in PAPR mode */
>      if (kvm_enabled()) {
>          kvmppc_set_papr(cpu);

[Qemu-devel] [PATCH for-2.13 06/10] target/ppc: Add ppc_store_lpcr() helper

[David Gibson]

There are some fields in the cpu state which need to be updated when the
LPCR register is changed, which is done by ppc_hash64_update_rmls() and
ppc_hash64_update_vrma().  Code which alters env->spr[SPR_LPCR] needs to
call them afterwards to make sure the state is up to date.

That's easy to get wrong.  The normal way of dealing with sitautions like
that is to use a helper which both updates the basic register value and the
derived state.

So, do that.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_cpu_core.c |  6 +-----
 target/ppc/mmu-hash64.c | 15 +++++++++++----
 target/ppc/mmu-hash64.h |  3 +--
 3 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 9080664ec1..b1c3cf11f0 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -74,14 +74,10 @@ static void spapr_cpu_reset(void *opaque)
         lpcr &= ~pcc->lpcr_pm;
     }
 
-    env->spr[SPR_LPCR] = lpcr;
+    ppc_store_lpcr(cpu, lpcr);
 
     /* Set a full AMOR so guest can use the AMR as it sees fit */
     env->spr[SPR_AMOR] = 0xffffffffffffffffull;
-
-    /* Update some env bits based on new LPCR value */
-    ppc_hash64_update_rmls(cpu);
-    ppc_hash64_update_vrma(cpu);
 }
 
 static void spapr_cpu_destroy(PowerPCCPU *cpu)
diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
index 7e0adecfd9..a1db20e3a8 100644
--- a/target/ppc/mmu-hash64.c
+++ b/target/ppc/mmu-hash64.c
@@ -942,7 +942,7 @@ void ppc_hash64_tlb_flush_hpte(PowerPCCPU *cpu, target_ulong ptex,
     cpu->env.tlb_need_flush = TLB_NEED_GLOBAL_FLUSH | TLB_NEED_LOCAL_FLUSH;
 }
 
-void ppc_hash64_update_rmls(PowerPCCPU *cpu)
+static void ppc_hash64_update_rmls(PowerPCCPU *cpu)
 {
     CPUPPCState *env = &cpu->env;
     uint64_t lpcr = env->spr[SPR_LPCR];
@@ -977,7 +977,7 @@ void ppc_hash64_update_rmls(PowerPCCPU *cpu)
     }
 }
 
-void ppc_hash64_update_vrma(PowerPCCPU *cpu)
+static void ppc_hash64_update_vrma(PowerPCCPU *cpu)
 {
     CPUPPCState *env = &cpu->env;
     const PPCHash64SegmentPageSizes *sps = NULL;
@@ -1028,9 +1028,9 @@ void ppc_hash64_update_vrma(PowerPCCPU *cpu)
     slb->sps = sps;
 }
 
-void helper_store_lpcr(CPUPPCState *env, target_ulong val)
+void ppc_store_lpcr(PowerPCCPU *cpu, target_ulong val)
 {
-    PowerPCCPU *cpu = ppc_env_get_cpu(env);
+    CPUPPCState *env = &cpu->env;
     uint64_t lpcr = 0;
 
     /* Filter out bits */
@@ -1096,6 +1096,13 @@ void helper_store_lpcr(CPUPPCState *env, target_ulong val)
     ppc_hash64_update_vrma(cpu);
 }
 
+void helper_store_lpcr(CPUPPCState *env, target_ulong val)
+{
+    PowerPCCPU *cpu = ppc_env_get_cpu(env);
+
+    ppc_store_lpcr(cpu, val);
+}
+
 void ppc_hash64_init(PowerPCCPU *cpu)
 {
     CPUPPCState *env = &cpu->env;
diff --git a/target/ppc/mmu-hash64.h b/target/ppc/mmu-hash64.h
index d5fc03441d..f23b78d787 100644
--- a/target/ppc/mmu-hash64.h
+++ b/target/ppc/mmu-hash64.h
@@ -17,8 +17,7 @@ void ppc_hash64_tlb_flush_hpte(PowerPCCPU *cpu,
                                target_ulong pte0, target_ulong pte1);
 unsigned ppc_hash64_hpte_page_shift_noslb(PowerPCCPU *cpu,
                                           uint64_t pte0, uint64_t pte1);
-void ppc_hash64_update_vrma(PowerPCCPU *cpu);
-void ppc_hash64_update_rmls(PowerPCCPU *cpu);
+void ppc_store_lpcr(PowerPCCPU *cpu, target_ulong val);
 void ppc_hash64_init(PowerPCCPU *cpu);
 void ppc_hash64_finalize(PowerPCCPU *cpu);
 #endif
-- 
2.14.3

Re: [Qemu-devel] [PATCH for-2.13 06/10] target/ppc: Add ppc_store_lpcr() helper

[Greg Kurz]

On Tue, 17 Apr 2018 17:17:18 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> There are some fields in the cpu state which need to be updated when the
> LPCR register is changed, which is done by ppc_hash64_update_rmls() and
> ppc_hash64_update_vrma().  Code which alters env->spr[SPR_LPCR] needs to
> call them afterwards to make sure the state is up to date.
> 
> That's easy to get wrong.  The normal way of dealing with sitautions like

                                                    s/sitautions/situations/

> that is to use a helper which both updates the basic register value and the
> derived state.
> 
> So, do that.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  hw/ppc/spapr_cpu_core.c |  6 +-----
>  target/ppc/mmu-hash64.c | 15 +++++++++++----
>  target/ppc/mmu-hash64.h |  3 +--
>  3 files changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 9080664ec1..b1c3cf11f0 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -74,14 +74,10 @@ static void spapr_cpu_reset(void *opaque)
>          lpcr &= ~pcc->lpcr_pm;
>      }
>  
> -    env->spr[SPR_LPCR] = lpcr;
> +    ppc_store_lpcr(cpu, lpcr);
>  
>      /* Set a full AMOR so guest can use the AMR as it sees fit */
>      env->spr[SPR_AMOR] = 0xffffffffffffffffull;
> -
> -    /* Update some env bits based on new LPCR value */
> -    ppc_hash64_update_rmls(cpu);
> -    ppc_hash64_update_vrma(cpu);
>  }
>  
>  static void spapr_cpu_destroy(PowerPCCPU *cpu)
> diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
> index 7e0adecfd9..a1db20e3a8 100644
> --- a/target/ppc/mmu-hash64.c
> +++ b/target/ppc/mmu-hash64.c
> @@ -942,7 +942,7 @@ void ppc_hash64_tlb_flush_hpte(PowerPCCPU *cpu, target_ulong ptex,
>      cpu->env.tlb_need_flush = TLB_NEED_GLOBAL_FLUSH | TLB_NEED_LOCAL_FLUSH;
>  }
>  
> -void ppc_hash64_update_rmls(PowerPCCPU *cpu)
> +static void ppc_hash64_update_rmls(PowerPCCPU *cpu)
>  {
>      CPUPPCState *env = &cpu->env;
>      uint64_t lpcr = env->spr[SPR_LPCR];
> @@ -977,7 +977,7 @@ void ppc_hash64_update_rmls(PowerPCCPU *cpu)
>      }
>  }
>  
> -void ppc_hash64_update_vrma(PowerPCCPU *cpu)
> +static void ppc_hash64_update_vrma(PowerPCCPU *cpu)
>  {
>      CPUPPCState *env = &cpu->env;
>      const PPCHash64SegmentPageSizes *sps = NULL;
> @@ -1028,9 +1028,9 @@ void ppc_hash64_update_vrma(PowerPCCPU *cpu)
>      slb->sps = sps;
>  }
>  
> -void helper_store_lpcr(CPUPPCState *env, target_ulong val)
> +void ppc_store_lpcr(PowerPCCPU *cpu, target_ulong val)
>  {
> -    PowerPCCPU *cpu = ppc_env_get_cpu(env);
> +    CPUPPCState *env = &cpu->env;
>      uint64_t lpcr = 0;
>  
>      /* Filter out bits */
> @@ -1096,6 +1096,13 @@ void helper_store_lpcr(CPUPPCState *env, target_ulong val)
>      ppc_hash64_update_vrma(cpu);
>  }
>  
> +void helper_store_lpcr(CPUPPCState *env, target_ulong val)
> +{
> +    PowerPCCPU *cpu = ppc_env_get_cpu(env);
> +
> +    ppc_store_lpcr(cpu, val);
> +}
> +
>  void ppc_hash64_init(PowerPCCPU *cpu)
>  {
>      CPUPPCState *env = &cpu->env;
> diff --git a/target/ppc/mmu-hash64.h b/target/ppc/mmu-hash64.h
> index d5fc03441d..f23b78d787 100644
> --- a/target/ppc/mmu-hash64.h
> +++ b/target/ppc/mmu-hash64.h
> @@ -17,8 +17,7 @@ void ppc_hash64_tlb_flush_hpte(PowerPCCPU *cpu,
>                                 target_ulong pte0, target_ulong pte1);
>  unsigned ppc_hash64_hpte_page_shift_noslb(PowerPCCPU *cpu,
>                                            uint64_t pte0, uint64_t pte1);
> -void ppc_hash64_update_vrma(PowerPCCPU *cpu);
> -void ppc_hash64_update_rmls(PowerPCCPU *cpu);
> +void ppc_store_lpcr(PowerPCCPU *cpu, target_ulong val);
>  void ppc_hash64_init(PowerPCCPU *cpu);
>  void ppc_hash64_finalize(PowerPCCPU *cpu);
>  #endif

[Qemu-devel] [PATCH for-2.13 07/10] spapr: Make a helper to set up cpu entry point state

[David Gibson]

Under PAPR, only the boot CPU is active when the system starts.  Other cpus
must be explicitly activated using an RTAS call.  The entry state for the
boot and secondary cpus isn't identical, but it has some things in common.
We're going to add a bit more common setup later, too, so to simplify
make a helper which sets up the common entry state for both boot and
secondary cpu threads.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr.c                  | 4 +---
 hw/ppc/spapr_cpu_core.c         | 9 +++++++++
 hw/ppc/spapr_rtas.c             | 6 +++---
 include/hw/ppc/spapr_cpu_core.h | 3 +++
 4 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index fbb2c6752c..e0cabfa6ee 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1536,10 +1536,8 @@ static void spapr_machine_reset(void)
     g_free(fdt);
 
     /* Set up the entry state */
-    first_ppc_cpu->env.gpr[3] = fdt_addr;
+    spapr_cpu_set_entry_state(first_ppc_cpu, SPAPR_ENTRY_POINT, fdt_addr);
     first_ppc_cpu->env.gpr[5] = 0;
-    first_cpu->halted = 0;
-    first_ppc_cpu->env.nip = SPAPR_ENTRY_POINT;
 
     spapr->cas_reboot = false;
 }
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index b1c3cf11f0..ecd40dbf03 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -80,6 +80,15 @@ static void spapr_cpu_reset(void *opaque)
     env->spr[SPR_AMOR] = 0xffffffffffffffffull;
 }
 
+void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip, target_ulong r3)
+{
+    CPUPPCState *env = &cpu->env;
+
+    env->nip = SPAPR_ENTRY_POINT;
+    env->gpr[3] = r3;
+    CPU(cpu)->halted = 0;
+}
+
 static void spapr_cpu_destroy(PowerPCCPU *cpu)
 {
     qemu_unregister_reset(spapr_cpu_reset, cpu);
diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
index 0ec5fa4cfe..d79aa44467 100644
--- a/hw/ppc/spapr_rtas.c
+++ b/hw/ppc/spapr_rtas.c
@@ -37,6 +37,7 @@
 #include "hw/ppc/spapr.h"
 #include "hw/ppc/spapr_vio.h"
 #include "hw/ppc/spapr_rtas.h"
+#include "hw/ppc/spapr_cpu_core.h"
 #include "hw/ppc/ppc.h"
 #include "hw/boards.h"
 
@@ -173,14 +174,13 @@ static void rtas_start_cpu(PowerPCCPU *cpu_, sPAPRMachineState *spapr,
          * new cpu enters */
         kvm_cpu_synchronize_state(cs);
 
+        spapr_cpu_set_entry_state(cpu, start, r3);
+
         env->msr = (1ULL << MSR_SF) | (1ULL << MSR_ME);
 
         /* Enable Power-saving mode Exit Cause exceptions for the new CPU */
         env->spr[SPR_LPCR] |= pcc->lpcr_pm;
 
-        env->nip = start;
-        env->gpr[3] = r3;
-        cs->halted = 0;
         spapr_cpu_set_endianness(cpu);
         spapr_cpu_update_tb_offset(cpu);
 
diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
index 1a38544706..11cab30838 100644
--- a/include/hw/ppc/spapr_cpu_core.h
+++ b/include/hw/ppc/spapr_cpu_core.h
@@ -12,6 +12,7 @@
 #include "hw/qdev.h"
 #include "hw/cpu/core.h"
 #include "target/ppc/cpu-qom.h"
+#include "target/ppc/cpu.h"
 
 #define TYPE_SPAPR_CPU_CORE "spapr-cpu-core"
 #define SPAPR_CPU_CORE(obj) \
@@ -40,4 +41,6 @@ typedef struct sPAPRCPUCoreClass {
 const char *spapr_get_cpu_core_type(const char *cpu_type);
 void spapr_cpu_core_reset(sPAPRCPUCore *sc);
 
+void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip, target_ulong r3);
+
 #endif
-- 
2.14.3

Re: [Qemu-devel] [PATCH for-2.13 07/10] spapr: Make a helper to set up cpu entry point state

[Greg Kurz]

On Tue, 17 Apr 2018 17:17:19 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> Under PAPR, only the boot CPU is active when the system starts.  Other cpus
> must be explicitly activated using an RTAS call.  The entry state for the
> boot and secondary cpus isn't identical, but it has some things in common.
> We're going to add a bit more common setup later, too, so to simplify
> make a helper which sets up the common entry state for both boot and
> secondary cpu threads.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  hw/ppc/spapr.c                  | 4 +---
>  hw/ppc/spapr_cpu_core.c         | 9 +++++++++
>  hw/ppc/spapr_rtas.c             | 6 +++---
>  include/hw/ppc/spapr_cpu_core.h | 3 +++
>  4 files changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index fbb2c6752c..e0cabfa6ee 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1536,10 +1536,8 @@ static void spapr_machine_reset(void)
>      g_free(fdt);
>  
>      /* Set up the entry state */
> -    first_ppc_cpu->env.gpr[3] = fdt_addr;
> +    spapr_cpu_set_entry_state(first_ppc_cpu, SPAPR_ENTRY_POINT, fdt_addr);
>      first_ppc_cpu->env.gpr[5] = 0;
> -    first_cpu->halted = 0;
> -    first_ppc_cpu->env.nip = SPAPR_ENTRY_POINT;
>  
>      spapr->cas_reboot = false;
>  }
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index b1c3cf11f0..ecd40dbf03 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -80,6 +80,15 @@ static void spapr_cpu_reset(void *opaque)
>      env->spr[SPR_AMOR] = 0xffffffffffffffffull;
>  }
>  
> +void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip, target_ulong r3)
> +{
> +    CPUPPCState *env = &cpu->env;
> +
> +    env->nip = SPAPR_ENTRY_POINT;
> +    env->gpr[3] = r3;
> +    CPU(cpu)->halted = 0;
> +}
> +
>  static void spapr_cpu_destroy(PowerPCCPU *cpu)
>  {
>      qemu_unregister_reset(spapr_cpu_reset, cpu);
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index 0ec5fa4cfe..d79aa44467 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -37,6 +37,7 @@
>  #include "hw/ppc/spapr.h"
>  #include "hw/ppc/spapr_vio.h"
>  #include "hw/ppc/spapr_rtas.h"
> +#include "hw/ppc/spapr_cpu_core.h"
>  #include "hw/ppc/ppc.h"
>  #include "hw/boards.h"
>  
> @@ -173,14 +174,13 @@ static void rtas_start_cpu(PowerPCCPU *cpu_, sPAPRMachineState *spapr,
>           * new cpu enters */
>          kvm_cpu_synchronize_state(cs);
>  
> +        spapr_cpu_set_entry_state(cpu, start, r3);
> +
>          env->msr = (1ULL << MSR_SF) | (1ULL << MSR_ME);
>  
>          /* Enable Power-saving mode Exit Cause exceptions for the new CPU */
>          env->spr[SPR_LPCR] |= pcc->lpcr_pm;
>  
> -        env->nip = start;
> -        env->gpr[3] = r3;
> -        cs->halted = 0;
>          spapr_cpu_set_endianness(cpu);
>          spapr_cpu_update_tb_offset(cpu);
>  
> diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
> index 1a38544706..11cab30838 100644
> --- a/include/hw/ppc/spapr_cpu_core.h
> +++ b/include/hw/ppc/spapr_cpu_core.h
> @@ -12,6 +12,7 @@
>  #include "hw/qdev.h"
>  #include "hw/cpu/core.h"
>  #include "target/ppc/cpu-qom.h"
> +#include "target/ppc/cpu.h"
>  
>  #define TYPE_SPAPR_CPU_CORE "spapr-cpu-core"
>  #define SPAPR_CPU_CORE(obj) \
> @@ -40,4 +41,6 @@ typedef struct sPAPRCPUCoreClass {
>  const char *spapr_get_cpu_core_type(const char *cpu_type);
>  void spapr_cpu_core_reset(sPAPRCPUCore *sc);
>  
> +void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip, target_ulong r3);
> +
>  #endif

[Qemu-devel] [PATCH for-2.13 08/10] spapr: Clean up handling of LPCR power-saving exit bits

[David Gibson]

To prevent spurious wakeups on cpus that are supposed to be disabled, we
need to clear the LPCR bits which control certain wakeup events.
spapr_cpu_reset() has separate cases here for boot and non-boot (initially
inactive) cpus.  rtas_start_cpu() then turns the LPCR bits on when the
non-boot cpus are activated.

But explicit checks against first_cpu are not how we usually do things:
instead spapr_cpu_reset() generally sets things up for non-boot (inactive)
cpus, then spapr_machine_reset() and/or rtas_start_cpu() override as
necessary.

So, do that instead.  Because the LPCR activation is identical for boot
cpus and non-boot cpus just activated with rtas_start_cpu() we can put the
code common in spapr_cpu_set_entry_state().

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_cpu_core.c | 22 +++++++---------------
 hw/ppc/spapr_rtas.c     |  4 ----
 2 files changed, 7 insertions(+), 19 deletions(-)

diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index ecd40dbf03..8be0265d04 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -52,28 +52,17 @@ static void spapr_cpu_reset(void *opaque)
      * Clearing VPM0 will also cause us to use RMOR in mmu-hash64.c for
      * real mode accesses, which thankfully defaults to 0 and isn't
      * accessible in guest mode.
+     *
+     * Disable Power-saving mode Exit Cause exceptions for the CPU, so
+     * we don't get spurious wakups before an RTAS start-cpu call.
      */
-    lpcr &= ~(LPCR_VPM0 | LPCR_VPM1 | LPCR_ISL | LPCR_KBV);
+    lpcr &= ~(LPCR_VPM0 | LPCR_VPM1 | LPCR_ISL | LPCR_KBV | pcc->lpcr_pm);
     lpcr |= LPCR_LPES0 | LPCR_LPES1;
 
     /* Set RMLS to the max (ie, 16G) */
     lpcr &= ~LPCR_RMLS;
     lpcr |= 1ull << LPCR_RMLS_SHIFT;
 
-    /* Only enable Power-saving mode Exit Cause exceptions on the boot
-     * CPU. The RTAS command start-cpu will enable them on secondaries.
-     */
-    if (cs == first_cpu) {
-        lpcr |= pcc->lpcr_pm;
-    }
-
-    /* Disable Power-saving mode Exit Cause exceptions for the CPU.
-     * This can cause issues when rebooting the guest if a secondary
-     * is awaken */
-    if (cs != first_cpu) {
-        lpcr &= ~pcc->lpcr_pm;
-    }
-
     ppc_store_lpcr(cpu, lpcr);
 
     /* Set a full AMOR so guest can use the AMR as it sees fit */
@@ -82,11 +71,14 @@ static void spapr_cpu_reset(void *opaque)
 
 void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip, target_ulong r3)
 {
+    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
     CPUPPCState *env = &cpu->env;
 
     env->nip = SPAPR_ENTRY_POINT;
     env->gpr[3] = r3;
     CPU(cpu)->halted = 0;
+    /* Enable Power-saving mode Exit Cause exceptions */
+    ppc_store_lpcr(cpu, env->spr[SPR_LPCR] | pcc->lpcr_pm);
 }
 
 static void spapr_cpu_destroy(PowerPCCPU *cpu)
diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
index d79aa44467..e720d54eb5 100644
--- a/hw/ppc/spapr_rtas.c
+++ b/hw/ppc/spapr_rtas.c
@@ -162,7 +162,6 @@ static void rtas_start_cpu(PowerPCCPU *cpu_, sPAPRMachineState *spapr,
     if (cpu != NULL) {
         CPUState *cs = CPU(cpu);
         CPUPPCState *env = &cpu->env;
-        PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
 
         if (!cs->halted) {
             rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
@@ -178,9 +177,6 @@ static void rtas_start_cpu(PowerPCCPU *cpu_, sPAPRMachineState *spapr,
 
         env->msr = (1ULL << MSR_SF) | (1ULL << MSR_ME);
 
-        /* Enable Power-saving mode Exit Cause exceptions for the new CPU */
-        env->spr[SPR_LPCR] |= pcc->lpcr_pm;
-
         spapr_cpu_set_endianness(cpu);
         spapr_cpu_update_tb_offset(cpu);
 
-- 
2.14.3

Re: [Qemu-devel] [PATCH for-2.13 08/10] spapr: Clean up handling of LPCR power-saving exit bits

[Greg Kurz]

On Tue, 17 Apr 2018 17:17:20 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> To prevent spurious wakeups on cpus that are supposed to be disabled, we
> need to clear the LPCR bits which control certain wakeup events.
> spapr_cpu_reset() has separate cases here for boot and non-boot (initially
> inactive) cpus.  rtas_start_cpu() then turns the LPCR bits on when the
> non-boot cpus are activated.
> 
> But explicit checks against first_cpu are not how we usually do things:
> instead spapr_cpu_reset() generally sets things up for non-boot (inactive)
> cpus, then spapr_machine_reset() and/or rtas_start_cpu() override as
> necessary.
> 
> So, do that instead.  Because the LPCR activation is identical for boot
> cpus and non-boot cpus just activated with rtas_start_cpu() we can put the
> code common in spapr_cpu_set_entry_state().
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---

Ok, ok, I hadn't seen this patch yet when I commented on patch 5 :)

Reviewed-by: Greg Kurz <groug@kaod.org>

>  hw/ppc/spapr_cpu_core.c | 22 +++++++---------------
>  hw/ppc/spapr_rtas.c     |  4 ----
>  2 files changed, 7 insertions(+), 19 deletions(-)
> 
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index ecd40dbf03..8be0265d04 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -52,28 +52,17 @@ static void spapr_cpu_reset(void *opaque)
>       * Clearing VPM0 will also cause us to use RMOR in mmu-hash64.c for
>       * real mode accesses, which thankfully defaults to 0 and isn't
>       * accessible in guest mode.
> +     *
> +     * Disable Power-saving mode Exit Cause exceptions for the CPU, so
> +     * we don't get spurious wakups before an RTAS start-cpu call.
>       */
> -    lpcr &= ~(LPCR_VPM0 | LPCR_VPM1 | LPCR_ISL | LPCR_KBV);
> +    lpcr &= ~(LPCR_VPM0 | LPCR_VPM1 | LPCR_ISL | LPCR_KBV | pcc->lpcr_pm);
>      lpcr |= LPCR_LPES0 | LPCR_LPES1;
>  
>      /* Set RMLS to the max (ie, 16G) */
>      lpcr &= ~LPCR_RMLS;
>      lpcr |= 1ull << LPCR_RMLS_SHIFT;
>  
> -    /* Only enable Power-saving mode Exit Cause exceptions on the boot
> -     * CPU. The RTAS command start-cpu will enable them on secondaries.
> -     */
> -    if (cs == first_cpu) {
> -        lpcr |= pcc->lpcr_pm;
> -    }
> -
> -    /* Disable Power-saving mode Exit Cause exceptions for the CPU.
> -     * This can cause issues when rebooting the guest if a secondary
> -     * is awaken */
> -    if (cs != first_cpu) {
> -        lpcr &= ~pcc->lpcr_pm;
> -    }
> -
>      ppc_store_lpcr(cpu, lpcr);
>  
>      /* Set a full AMOR so guest can use the AMR as it sees fit */
> @@ -82,11 +71,14 @@ static void spapr_cpu_reset(void *opaque)
>  
>  void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip, target_ulong r3)
>  {
> +    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
>      CPUPPCState *env = &cpu->env;
>  
>      env->nip = SPAPR_ENTRY_POINT;
>      env->gpr[3] = r3;
>      CPU(cpu)->halted = 0;
> +    /* Enable Power-saving mode Exit Cause exceptions */
> +    ppc_store_lpcr(cpu, env->spr[SPR_LPCR] | pcc->lpcr_pm);
>  }
>  
>  static void spapr_cpu_destroy(PowerPCCPU *cpu)
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index d79aa44467..e720d54eb5 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -162,7 +162,6 @@ static void rtas_start_cpu(PowerPCCPU *cpu_, sPAPRMachineState *spapr,
>      if (cpu != NULL) {
>          CPUState *cs = CPU(cpu);
>          CPUPPCState *env = &cpu->env;
> -        PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
>  
>          if (!cs->halted) {
>              rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
> @@ -178,9 +177,6 @@ static void rtas_start_cpu(PowerPCCPU *cpu_, sPAPRMachineState *spapr,
>  
>          env->msr = (1ULL << MSR_SF) | (1ULL << MSR_ME);
>  
> -        /* Enable Power-saving mode Exit Cause exceptions for the new CPU */
> -        env->spr[SPR_LPCR] |= pcc->lpcr_pm;
> -
>          spapr_cpu_set_endianness(cpu);
>          spapr_cpu_update_tb_offset(cpu);
>  

[Qemu-devel] [PATCH for-2.13 09/10] target/ppc: Don't bother with MSR_EP in cpu_ppc_set_papr()

[David Gibson]

cpu_ppc_set_papr() removes the EP and HV bits from the MSR mask.  While
removing the HV bit makes sense (a cpu in PAPR mode should never be
emulated in hypervisor mode), the EP bit is just bizarre.  Although it's
true that a papr mode guest shouldn't be able to change the exception
prefix, the MSR[EP] bit doesn't even exist on the cpus supported for PAPR
mode, so it's pointless to do anything with it here.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 target/ppc/translate_init.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
index 5e89901149..bb5559d799 100644
--- a/target/ppc/translate_init.c
+++ b/target/ppc/translate_init.c
@@ -8870,12 +8870,11 @@ void cpu_ppc_set_papr(PowerPCCPU *cpu, PPCVirtualHypervisor *vhyp)
 
     cpu->vhyp = vhyp;
 
-    /* PAPR always has exception vectors in RAM not ROM. To ensure this,
-     * MSR[IP] should never be set.
-     *
-     * We also disallow setting of MSR_HV
+    /*
+     * With a virtual hypervisor mode we never allow the CPU to go
+     * hypervisor mode itself
      */
-    env->msr_mask &= ~((1ull << MSR_EP) | MSR_HVB);
+    env->msr_mask &= ~MSR_HVB;
 
     /* Tell KVM that we're in PAPR mode */
     if (kvm_enabled()) {
-- 
2.14.3

Re: [Qemu-devel] [Qemu-ppc] [PATCH for-2.13 09/10] target/ppc: Don't bother with MSR_EP in cpu_ppc_set_papr()

[Thomas Huth]

On 17.04.2018 09:17, David Gibson wrote:
> cpu_ppc_set_papr() removes the EP and HV bits from the MSR mask.  While
> removing the HV bit makes sense (a cpu in PAPR mode should never be
> emulated in hypervisor mode), the EP bit is just bizarre.  Although it's
> true that a papr mode guest shouldn't be able to change the exception
> prefix, the MSR[EP] bit doesn't even exist on the cpus supported for PAPR
> mode, so it's pointless to do anything with it here.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  target/ppc/translate_init.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
> index 5e89901149..bb5559d799 100644
> --- a/target/ppc/translate_init.c
> +++ b/target/ppc/translate_init.c
> @@ -8870,12 +8870,11 @@ void cpu_ppc_set_papr(PowerPCCPU *cpu, PPCVirtualHypervisor *vhyp)
>  
>      cpu->vhyp = vhyp;
>  
> -    /* PAPR always has exception vectors in RAM not ROM. To ensure this,
> -     * MSR[IP] should never be set.
> -     *
> -     * We also disallow setting of MSR_HV
> +    /*
> +     * With a virtual hypervisor mode we never allow the CPU to go
> +     * hypervisor mode itself
>       */
> -    env->msr_mask &= ~((1ull << MSR_EP) | MSR_HVB);
> +    env->msr_mask &= ~MSR_HVB;
>  
>      /* Tell KVM that we're in PAPR mode */
>      if (kvm_enabled()) {

Looks right.

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [Qemu-ppc] [PATCH for-2.13 09/10] target/ppc: Don't bother with MSR_EP in cpu_ppc_set_papr()

[David Gibson]

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

On Fri, Apr 20, 2018 at 08:08:59AM +0200, Thomas Huth wrote:
> On 17.04.2018 09:17, David Gibson wrote:
> > cpu_ppc_set_papr() removes the EP and HV bits from the MSR mask.  While
> > removing the HV bit makes sense (a cpu in PAPR mode should never be
> > emulated in hypervisor mode), the EP bit is just bizarre.  Although it's
> > true that a papr mode guest shouldn't be able to change the exception
> > prefix, the MSR[EP] bit doesn't even exist on the cpus supported for PAPR
> > mode, so it's pointless to do anything with it here.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  target/ppc/translate_init.c | 9 ++++-----
> >  1 file changed, 4 insertions(+), 5 deletions(-)
> > 
> > diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
> > index 5e89901149..bb5559d799 100644
> > --- a/target/ppc/translate_init.c
> > +++ b/target/ppc/translate_init.c
> > @@ -8870,12 +8870,11 @@ void cpu_ppc_set_papr(PowerPCCPU *cpu, PPCVirtualHypervisor *vhyp)
> >  
> >      cpu->vhyp = vhyp;
> >  
> > -    /* PAPR always has exception vectors in RAM not ROM. To ensure this,
> > -     * MSR[IP] should never be set.
> > -     *
> > -     * We also disallow setting of MSR_HV
> > +    /*
> > +     * With a virtual hypervisor mode we never allow the CPU to go
> > +     * hypervisor mode itself
> >       */
> > -    env->msr_mask &= ~((1ull << MSR_EP) | MSR_HVB);
> > +    env->msr_mask &= ~MSR_HVB;
> >  
> >      /* Tell KVM that we're in PAPR mode */
> >      if (kvm_enabled()) {
> 
> Looks right.
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>

Turns out this one is pretty much independent of the rest of the
series, so I've merged it to ppc-for-2.13 already.


-- 
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 for-2.13 10/10] spapr: Move PAPR specific cpu logic to pseries machine type

[David Gibson]

cpu_ppc_set_papr() does three things: 1) it sets up the virtual hypervisor
interface, 2) it prevents the cpu from ever entering hypervisor mode and
3) it tells KVM that we're emulating a cpu in PAPR mode.

(1) & (2) make sense for any virtual hypervisor (if one ever exists).  (3)
belongs more properly in the machine type specific to a PAPR guest.

While we're at it, remove an unnecessary test on kvm_enabled() by making
kvmppc_set_papr() a safe no-op on non-KVM.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_cpu_core.c     | 4 ++--
 target/ppc/cpu.h            | 2 +-
 target/ppc/kvm.c            | 4 ++++
 target/ppc/translate_init.c | 7 +------
 4 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 8be0265d04..0a668b8954 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -94,8 +94,8 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
     /* Set time-base frequency to 512 MHz */
     cpu_ppc_tb_init(env, SPAPR_TIMEBASE_FREQ);
 
-    /* Enable PAPR mode in TCG or KVM */
-    cpu_ppc_set_papr(cpu, PPC_VIRTUAL_HYPERVISOR(spapr));
+    cpu_ppc_set_vhyp(cpu, PPC_VIRTUAL_HYPERVISOR(spapr));
+    kvmppc_set_papr(cpu);
 
     qemu_register_reset(spapr_cpu_reset, cpu);
 }
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 8c9e03f54d..740939a085 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1331,7 +1331,7 @@ void store_booke_tcr (CPUPPCState *env, target_ulong val);
 void store_booke_tsr (CPUPPCState *env, target_ulong val);
 void ppc_tlb_invalidate_all (CPUPPCState *env);
 void ppc_tlb_invalidate_one (CPUPPCState *env, target_ulong addr);
-void cpu_ppc_set_papr(PowerPCCPU *cpu, PPCVirtualHypervisor *vhyp);
+void cpu_ppc_set_vhyp(PowerPCCPU *cpu, PPCVirtualHypervisor *vhyp);
 #endif
 #endif
 
diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index 51177a8e00..e4341d6ff7 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -2090,6 +2090,10 @@ void kvmppc_set_papr(PowerPCCPU *cpu)
     CPUState *cs = CPU(cpu);
     int ret;
 
+    if (!kvm_enabled()) {
+        return;
+    }
+
     ret = kvm_vcpu_enable_cap(cs, KVM_CAP_PPC_PAPR, 0);
     if (ret) {
         error_report("This vCPU type or KVM version does not support PAPR");
diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
index bb5559d799..95e8dba97a 100644
--- a/target/ppc/translate_init.c
+++ b/target/ppc/translate_init.c
@@ -8864,7 +8864,7 @@ POWERPC_FAMILY(POWER9)(ObjectClass *oc, void *data)
 }
 
 #if !defined(CONFIG_USER_ONLY)
-void cpu_ppc_set_papr(PowerPCCPU *cpu, PPCVirtualHypervisor *vhyp)
+void cpu_ppc_set_vhyp(PowerPCCPU *cpu, PPCVirtualHypervisor *vhyp)
 {
     CPUPPCState *env = &cpu->env;
 
@@ -8875,11 +8875,6 @@ void cpu_ppc_set_papr(PowerPCCPU *cpu, PPCVirtualHypervisor *vhyp)
      * hypervisor mode itself
      */
     env->msr_mask &= ~MSR_HVB;
-
-    /* Tell KVM that we're in PAPR mode */
-    if (kvm_enabled()) {
-        kvmppc_set_papr(cpu);
-    }
 }
 
 #endif /* !defined(CONFIG_USER_ONLY) */
-- 
2.14.3

Re: [Qemu-devel] [PATCH for-2.13 10/10] spapr: Move PAPR specific cpu logic to pseries machine type

[Greg Kurz]

On Tue, 17 Apr 2018 17:17:22 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> cpu_ppc_set_papr() does three things: 1) it sets up the virtual hypervisor
> interface, 2) it prevents the cpu from ever entering hypervisor mode and
> 3) it tells KVM that we're emulating a cpu in PAPR mode.
> 
> (1) & (2) make sense for any virtual hypervisor (if one ever exists).  (3)
> belongs more properly in the machine type specific to a PAPR guest.
> 
> While we're at it, remove an unnecessary test on kvm_enabled() by making
> kvmppc_set_papr() a safe no-op on non-KVM.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  hw/ppc/spapr_cpu_core.c     | 4 ++--
>  target/ppc/cpu.h            | 2 +-
>  target/ppc/kvm.c            | 4 ++++
>  target/ppc/translate_init.c | 7 +------
>  4 files changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 8be0265d04..0a668b8954 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -94,8 +94,8 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
>      /* Set time-base frequency to 512 MHz */
>      cpu_ppc_tb_init(env, SPAPR_TIMEBASE_FREQ);
>  
> -    /* Enable PAPR mode in TCG or KVM */
> -    cpu_ppc_set_papr(cpu, PPC_VIRTUAL_HYPERVISOR(spapr));
> +    cpu_ppc_set_vhyp(cpu, PPC_VIRTUAL_HYPERVISOR(spapr));
> +    kvmppc_set_papr(cpu);
>  
>      qemu_register_reset(spapr_cpu_reset, cpu);
>  }
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 8c9e03f54d..740939a085 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1331,7 +1331,7 @@ void store_booke_tcr (CPUPPCState *env, target_ulong val);
>  void store_booke_tsr (CPUPPCState *env, target_ulong val);
>  void ppc_tlb_invalidate_all (CPUPPCState *env);
>  void ppc_tlb_invalidate_one (CPUPPCState *env, target_ulong addr);
> -void cpu_ppc_set_papr(PowerPCCPU *cpu, PPCVirtualHypervisor *vhyp);
> +void cpu_ppc_set_vhyp(PowerPCCPU *cpu, PPCVirtualHypervisor *vhyp);
>  #endif
>  #endif
>  
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 51177a8e00..e4341d6ff7 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -2090,6 +2090,10 @@ void kvmppc_set_papr(PowerPCCPU *cpu)
>      CPUState *cs = CPU(cpu);
>      int ret;
>  
> +    if (!kvm_enabled()) {
> +        return;
> +    }
> +
>      ret = kvm_vcpu_enable_cap(cs, KVM_CAP_PPC_PAPR, 0);
>      if (ret) {
>          error_report("This vCPU type or KVM version does not support PAPR");
> diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
> index bb5559d799..95e8dba97a 100644
> --- a/target/ppc/translate_init.c
> +++ b/target/ppc/translate_init.c
> @@ -8864,7 +8864,7 @@ POWERPC_FAMILY(POWER9)(ObjectClass *oc, void *data)
>  }
>  
>  #if !defined(CONFIG_USER_ONLY)
> -void cpu_ppc_set_papr(PowerPCCPU *cpu, PPCVirtualHypervisor *vhyp)
> +void cpu_ppc_set_vhyp(PowerPCCPU *cpu, PPCVirtualHypervisor *vhyp)
>  {
>      CPUPPCState *env = &cpu->env;
>  
> @@ -8875,11 +8875,6 @@ void cpu_ppc_set_papr(PowerPCCPU *cpu, PPCVirtualHypervisor *vhyp)
>       * hypervisor mode itself
>       */
>      env->msr_mask &= ~MSR_HVB;
> -
> -    /* Tell KVM that we're in PAPR mode */
> -    if (kvm_enabled()) {
> -        kvmppc_set_papr(cpu);
> -    }
>  }
>  
>  #endif /* !defined(CONFIG_USER_ONLY) */