[Qemu-devel] [PATCH for-2.10 0/2] Add some missing POWER9 SPRs for migration

[Qemu-devel] [PATCH for-2.10 0/2] Add some missing POWER9 SPRs for migration

[David Gibson]

This series adds a couple of new POWER9 SPRs: TIDR and PSSCR.

These aren't particularly complete implementations - the registers
aren't all that interesting from a TCG / qemu point of view.  However
having them at least minimally represented means that state is synced
with KVM and migrated, which fixes some bugs.

David Gibson (2):
  target/ppc: Implement TIDR
  target/ppc: Add stub implementation of the PSSCR

 target/ppc/cpu.h            |  2 ++
 target/ppc/translate_init.c | 10 ++++++++++
 2 files changed, 12 insertions(+)

-- 
2.13.4

[Qemu-devel] [PATCH for-2.10 1/2] target/ppc: Implement TIDR

[David Gibson]

This adds a trivial implementation of the TIDR register added in
POWER9.  This isn't particularly important to qemu directly - it's
used by accelerator modules that we don't emulate.

However, since qemu isn't aware of it, its state is not synchronized
with KVM and therefore not migrated, which can be a problem.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 target/ppc/cpu.h            | 1 +
 target/ppc/translate_init.c | 5 +++++
 2 files changed, 6 insertions(+)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 6ee2a26a96..f6e5413fad 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1451,6 +1451,7 @@ void ppc_compat_add_property(Object *obj, const char *name,
 #define SPR_TEXASR            (0x082)
 #define SPR_TEXASRU           (0x083)
 #define SPR_UCTRL             (0x088)
+#define SPR_TIDR              (0x090)
 #define SPR_MPC_CMPA          (0x090)
 #define SPR_MPC_CMPB          (0x091)
 #define SPR_MPC_CMPC          (0x092)
diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
index 01723bdfec..94800cd29d 100644
--- a/target/ppc/translate_init.c
+++ b/target/ppc/translate_init.c
@@ -8841,6 +8841,11 @@ static void init_proc_POWER9(CPUPPCState *env)
     gen_spr_power8_book4(env);
     gen_spr_power8_rpr(env);
 
+    /* POWER9 Specific registers */
+    spr_register_kvm(env, SPR_TIDR, "TIDR", NULL, NULL,
+                     spr_read_generic, spr_write_generic,
+                     KVM_REG_PPC_TIDR, 0);
+
     /* env variables */
 #if !defined(CONFIG_USER_ONLY)
     env->slb_nr = 32;
-- 
2.13.4

Re: [Qemu-devel] [PATCH for-2.10 1/2] target/ppc: Implement TIDR

[Cédric Le Goater]

On 08/08/2017 08:08 AM, David Gibson wrote:
> This adds a trivial implementation of the TIDR register added in
> POWER9.  This isn't particularly important to qemu directly - it's
> used by accelerator modules that we don't emulate.
> 
> However, since qemu isn't aware of it, its state is not synchronized
> with KVM and therefore not migrated, which can be a problem.

The Thread ID Register is defined in Power ISA 3.0B.

Reviewed-by: Cédric Le Goater <clg@kaod.org>

C. 


> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  target/ppc/cpu.h            | 1 +
>  target/ppc/translate_init.c | 5 +++++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 6ee2a26a96..f6e5413fad 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1451,6 +1451,7 @@ void ppc_compat_add_property(Object *obj, const char *name,
>  #define SPR_TEXASR            (0x082)
>  #define SPR_TEXASRU           (0x083)
>  #define SPR_UCTRL             (0x088)
> +#define SPR_TIDR              (0x090)
>  #define SPR_MPC_CMPA          (0x090)
>  #define SPR_MPC_CMPB          (0x091)
>  #define SPR_MPC_CMPC          (0x092)
> diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
> index 01723bdfec..94800cd29d 100644
> --- a/target/ppc/translate_init.c
> +++ b/target/ppc/translate_init.c
> @@ -8841,6 +8841,11 @@ static void init_proc_POWER9(CPUPPCState *env)
>      gen_spr_power8_book4(env);
>      gen_spr_power8_rpr(env);
>  
> +    /* POWER9 Specific registers */
> +    spr_register_kvm(env, SPR_TIDR, "TIDR", NULL, NULL,
> +                     spr_read_generic, spr_write_generic,
> +                     KVM_REG_PPC_TIDR, 0);
> +
>      /* env variables */
>  #if !defined(CONFIG_USER_ONLY)
>      env->slb_nr = 32;
> 

Re: [Qemu-devel] [PATCH for-2.10 1/2] target/ppc: Implement TIDR

[Greg Kurz]

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

On Tue, 8 Aug 2017 11:13:03 +0200
Cédric Le Goater <clg@kaod.org> wrote:

> On 08/08/2017 08:08 AM, David Gibson wrote:
> > This adds a trivial implementation of the TIDR register added in
> > POWER9.  This isn't particularly important to qemu directly - it's
> > used by accelerator modules that we don't emulate.
> > 
> > However, since qemu isn't aware of it, its state is not synchronized
> > with KVM and therefore not migrated, which can be a problem.  
> 
> The Thread ID Register is defined in Power ISA 3.0B.
> 

Indeed. Available here:

https://openpowerfoundation.org/?resource_lib=power-isa-version-3-0

> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> 

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

> C. 
> 
> 
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  target/ppc/cpu.h            | 1 +
> >  target/ppc/translate_init.c | 5 +++++
> >  2 files changed, 6 insertions(+)
> > 
> > diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> > index 6ee2a26a96..f6e5413fad 100644
> > --- a/target/ppc/cpu.h
> > +++ b/target/ppc/cpu.h
> > @@ -1451,6 +1451,7 @@ void ppc_compat_add_property(Object *obj, const char *name,
> >  #define SPR_TEXASR            (0x082)
> >  #define SPR_TEXASRU           (0x083)
> >  #define SPR_UCTRL             (0x088)
> > +#define SPR_TIDR              (0x090)
> >  #define SPR_MPC_CMPA          (0x090)
> >  #define SPR_MPC_CMPB          (0x091)
> >  #define SPR_MPC_CMPC          (0x092)
> > diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
> > index 01723bdfec..94800cd29d 100644
> > --- a/target/ppc/translate_init.c
> > +++ b/target/ppc/translate_init.c
> > @@ -8841,6 +8841,11 @@ static void init_proc_POWER9(CPUPPCState *env)
> >      gen_spr_power8_book4(env);
> >      gen_spr_power8_rpr(env);
> >  
> > +    /* POWER9 Specific registers */
> > +    spr_register_kvm(env, SPR_TIDR, "TIDR", NULL, NULL,
> > +                     spr_read_generic, spr_write_generic,
> > +                     KVM_REG_PPC_TIDR, 0);
> > +
> >      /* env variables */
> >  #if !defined(CONFIG_USER_ONLY)
> >      env->slb_nr = 32;
> >   
> 


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

Re: [Qemu-devel] [Qemu-ppc] [PATCH for-2.10 1/2] target/ppc: Implement TIDR

[Thomas Huth]

On 08.08.2017 08:08, David Gibson wrote:
> This adds a trivial implementation of the TIDR register added in
> POWER9.  This isn't particularly important to qemu directly - it's
> used by accelerator modules that we don't emulate.
> 
> However, since qemu isn't aware of it, its state is not synchronized
> with KVM and therefore not migrated, which can be a problem.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  target/ppc/cpu.h            | 1 +
>  target/ppc/translate_init.c | 5 +++++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 6ee2a26a96..f6e5413fad 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1451,6 +1451,7 @@ void ppc_compat_add_property(Object *obj, const char *name,
>  #define SPR_TEXASR            (0x082)
>  #define SPR_TEXASRU           (0x083)
>  #define SPR_UCTRL             (0x088)
> +#define SPR_TIDR              (0x090)
>  #define SPR_MPC_CMPA          (0x090)
>  #define SPR_MPC_CMPB          (0x091)
>  #define SPR_MPC_CMPC          (0x092)
> diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
> index 01723bdfec..94800cd29d 100644
> --- a/target/ppc/translate_init.c
> +++ b/target/ppc/translate_init.c
> @@ -8841,6 +8841,11 @@ static void init_proc_POWER9(CPUPPCState *env)
>      gen_spr_power8_book4(env);
>      gen_spr_power8_rpr(env);
>  
> +    /* POWER9 Specific registers */
> +    spr_register_kvm(env, SPR_TIDR, "TIDR", NULL, NULL,
> +                     spr_read_generic, spr_write_generic,
> +                     KVM_REG_PPC_TIDR, 0);
> +
>      /* env variables */
>  #if !defined(CONFIG_USER_ONLY)
>      env->slb_nr = 32;
> 

In case you respin: Maybe consider to put this into a separate
gen_spr_power9() function that we could re-use on POWER10 (if appropriate)?

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

[Qemu-devel] [PATCH for-2.10 2/2] target/ppc: Add stub implementation of the PSSCR

[David Gibson]

The PSSCR register added in POWER9 controls certain power saving mode
behaviours.  Mostly, it's not relevant to TCG, however because qemu
doesn't know about it yet, it doesn't synchronize the state with KVM,
and thus it doesn't get migrated.

To fix that, this adds a minimal stub implementation of the register.
This isn't complete, even to the extent that an implementation is
possible in TCG, just enough to get migration working.  We need to
come back later and at least properly filter the various fields in the
register based on privilege level.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 target/ppc/cpu.h            | 1 +
 target/ppc/translate_init.c | 5 +++++
 2 files changed, 6 insertions(+)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index f6e5413fad..46d3dd88f6 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1771,6 +1771,7 @@ void ppc_compat_add_property(Object *obj, const char *name,
 #define SPR_IC                (0x350)
 #define SPR_VTB               (0x351)
 #define SPR_MMCRC             (0x353)
+#define SPR_PSSCR             (0x357)
 #define SPR_440_INV0          (0x370)
 #define SPR_440_INV1          (0x371)
 #define SPR_440_INV2          (0x372)
diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
index 94800cd29d..8fb407ed73 100644
--- a/target/ppc/translate_init.c
+++ b/target/ppc/translate_init.c
@@ -8846,6 +8846,11 @@ static void init_proc_POWER9(CPUPPCState *env)
                      spr_read_generic, spr_write_generic,
                      KVM_REG_PPC_TIDR, 0);
 
+    /* FIXME: Filter fields properly based on privilege level */
+    spr_register_kvm_hv(env, SPR_PSSCR, "PSSCR", NULL, NULL, NULL, NULL,
+                        spr_read_generic, spr_write_generic,
+                        KVM_REG_PPC_PSSCR, 0);
+
     /* env variables */
 #if !defined(CONFIG_USER_ONLY)
     env->slb_nr = 32;
-- 
2.13.4

Re: [Qemu-devel] [PATCH for-2.10 2/2] target/ppc: Add stub implementation of the PSSCR

[Cédric Le Goater]

On 08/08/2017 08:08 AM, David Gibson wrote:
> The PSSCR register added in POWER9 controls certain power saving mode
> behaviours.  Mostly, it's not relevant to TCG, however because qemu
> doesn't know about it yet, it doesn't synchronize the state with KVM,
> and thus it doesn't get migrated.
> 
> To fix that, this adds a minimal stub implementation of the register.
> This isn't complete, even to the extent that an implementation is
> possible in TCG, just enough to get migration working.  We need to
> come back later and at least properly filter the various fields in the
> register based on privilege level.

yes a lot of the fields are only accessible to the hypervisor, and the 
hypervisor also uses a different SPR number to access the PSSCR bits.

Reviewed-by: Cédric Le Goater <clg@kaod.org>

C.
  
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  target/ppc/cpu.h            | 1 +
>  target/ppc/translate_init.c | 5 +++++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index f6e5413fad..46d3dd88f6 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1771,6 +1771,7 @@ void ppc_compat_add_property(Object *obj, const char *name,
>  #define SPR_IC                (0x350)
>  #define SPR_VTB               (0x351)
>  #define SPR_MMCRC             (0x353)
> +#define SPR_PSSCR             (0x357)
>  #define SPR_440_INV0          (0x370)
>  #define SPR_440_INV1          (0x371)
>  #define SPR_440_INV2          (0x372)
> diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
> index 94800cd29d..8fb407ed73 100644
> --- a/target/ppc/translate_init.c
> +++ b/target/ppc/translate_init.c
> @@ -8846,6 +8846,11 @@ static void init_proc_POWER9(CPUPPCState *env)
>                       spr_read_generic, spr_write_generic,
>                       KVM_REG_PPC_TIDR, 0);
>  
> +    /* FIXME: Filter fields properly based on privilege level */
> +    spr_register_kvm_hv(env, SPR_PSSCR, "PSSCR", NULL, NULL, NULL, NULL,
> +                        spr_read_generic, spr_write_generic,
> +                        KVM_REG_PPC_PSSCR, 0);
> +
>      /* env variables */
>  #if !defined(CONFIG_USER_ONLY)
>      env->slb_nr = 32;
> 

Re: [Qemu-devel] [PATCH for-2.10 2/2] target/ppc: Add stub implementation of the PSSCR

[Greg Kurz]

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

On Tue, 8 Aug 2017 11:19:58 +0200
Cédric Le Goater <clg@kaod.org> wrote:

> On 08/08/2017 08:08 AM, David Gibson wrote:
> > The PSSCR register added in POWER9 controls certain power saving mode
> > behaviours.  Mostly, it's not relevant to TCG, however because qemu
> > doesn't know about it yet, it doesn't synchronize the state with KVM,
> > and thus it doesn't get migrated.
> > 
> > To fix that, this adds a minimal stub implementation of the register.
> > This isn't complete, even to the extent that an implementation is
> > possible in TCG, just enough to get migration working.  We need to
> > come back later and at least properly filter the various fields in the
> > register based on privilege level.  
> 
> yes a lot of the fields are only accessible to the hypervisor, and the 
> hypervisor also uses a different SPR number to access the PSSCR bits.
> 

This patch uses 0x357 (855) which is the SPR number for hypervisor state
access. But, yes, part of the register is also accessible in privileged
non-hypervisor state with SPR number 0x337 (823). This will have to be
covered later, but for now:

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

> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> 
> C.
>   
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  target/ppc/cpu.h            | 1 +
> >  target/ppc/translate_init.c | 5 +++++
> >  2 files changed, 6 insertions(+)
> > 
> > diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> > index f6e5413fad..46d3dd88f6 100644
> > --- a/target/ppc/cpu.h
> > +++ b/target/ppc/cpu.h
> > @@ -1771,6 +1771,7 @@ void ppc_compat_add_property(Object *obj, const char *name,
> >  #define SPR_IC                (0x350)
> >  #define SPR_VTB               (0x351)
> >  #define SPR_MMCRC             (0x353)
> > +#define SPR_PSSCR             (0x357)
> >  #define SPR_440_INV0          (0x370)
> >  #define SPR_440_INV1          (0x371)
> >  #define SPR_440_INV2          (0x372)
> > diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
> > index 94800cd29d..8fb407ed73 100644
> > --- a/target/ppc/translate_init.c
> > +++ b/target/ppc/translate_init.c
> > @@ -8846,6 +8846,11 @@ static void init_proc_POWER9(CPUPPCState *env)
> >                       spr_read_generic, spr_write_generic,
> >                       KVM_REG_PPC_TIDR, 0);
> >  
> > +    /* FIXME: Filter fields properly based on privilege level */
> > +    spr_register_kvm_hv(env, SPR_PSSCR, "PSSCR", NULL, NULL, NULL, NULL,
> > +                        spr_read_generic, spr_write_generic,
> > +                        KVM_REG_PPC_PSSCR, 0);
> > +
> >      /* env variables */
> >  #if !defined(CONFIG_USER_ONLY)
> >      env->slb_nr = 32;
> >   
> 


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

Re: [Qemu-devel] [PATCH for-2.10 2/2] target/ppc: Add stub implementation of the PSSCR

[David Gibson]

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

On Tue, Aug 08, 2017 at 12:54:51PM +0200, Greg Kurz wrote:
> On Tue, 8 Aug 2017 11:19:58 +0200
> Cédric Le Goater <clg@kaod.org> wrote:
> 
> > On 08/08/2017 08:08 AM, David Gibson wrote:
> > > The PSSCR register added in POWER9 controls certain power saving mode
> > > behaviours.  Mostly, it's not relevant to TCG, however because qemu
> > > doesn't know about it yet, it doesn't synchronize the state with KVM,
> > > and thus it doesn't get migrated.
> > > 
> > > To fix that, this adds a minimal stub implementation of the register.
> > > This isn't complete, even to the extent that an implementation is
> > > possible in TCG, just enough to get migration working.  We need to
> > > come back later and at least properly filter the various fields in the
> > > register based on privilege level.  
> > 
> > yes a lot of the fields are only accessible to the hypervisor, and the 
> > hypervisor also uses a different SPR number to access the PSSCR bits.
> > 
> 
> This patch uses 0x357 (855) which is the SPR number for hypervisor state
> access. But, yes, part of the register is also accessible in privileged
> non-hypervisor state with SPR number 0x337 (823). This will have to be
> covered later, but for now:

Yeah, I know.

-- 
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.10 2/2] target/ppc: Add stub implementation of the PSSCR

[Thomas Huth]

On 08.08.2017 08:08, David Gibson wrote:
> The PSSCR register added in POWER9 controls certain power saving mode
> behaviours.  Mostly, it's not relevant to TCG, however because qemu
> doesn't know about it yet, it doesn't synchronize the state with KVM,
> and thus it doesn't get migrated.
> 
> To fix that, this adds a minimal stub implementation of the register.
> This isn't complete, even to the extent that an implementation is
> possible in TCG, just enough to get migration working.  We need to
> come back later and at least properly filter the various fields in the
> register based on privilege level.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  target/ppc/cpu.h            | 1 +
>  target/ppc/translate_init.c | 5 +++++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index f6e5413fad..46d3dd88f6 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1771,6 +1771,7 @@ void ppc_compat_add_property(Object *obj, const char *name,
>  #define SPR_IC                (0x350)
>  #define SPR_VTB               (0x351)
>  #define SPR_MMCRC             (0x353)
> +#define SPR_PSSCR             (0x357)
>  #define SPR_440_INV0          (0x370)
>  #define SPR_440_INV1          (0x371)
>  #define SPR_440_INV2          (0x372)
> diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
> index 94800cd29d..8fb407ed73 100644
> --- a/target/ppc/translate_init.c
> +++ b/target/ppc/translate_init.c
> @@ -8846,6 +8846,11 @@ static void init_proc_POWER9(CPUPPCState *env)
>                       spr_read_generic, spr_write_generic,
>                       KVM_REG_PPC_TIDR, 0);
>  
> +    /* FIXME: Filter fields properly based on privilege level */
> +    spr_register_kvm_hv(env, SPR_PSSCR, "PSSCR", NULL, NULL, NULL, NULL,
> +                        spr_read_generic, spr_write_generic,
> +                        KVM_REG_PPC_PSSCR, 0);
> +
>      /* env variables */
>  #if !defined(CONFIG_USER_ONLY)
>      env->slb_nr = 32;
> 

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