[PATCH] target/ppc: Do not check for LPCR[HAIL] on power10_v1.0 CPUs

Cédric Le Goater posted 1 patch 2 years, 11 months ago
Failed in applying to current master (apply log)
target/ppc/excp_helper.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
[PATCH] target/ppc: Do not check for LPCR[HAIL] on power10_v1.0 CPUs
Posted by Cédric Le Goater 2 years, 11 months ago
The LPCR[HAIL] bit only applies to POWER10 DD2 CPUs. On POWER10 DD1,
the ail value should be extracted using the LPCR_AIL mask like on P9.

Cc: Nicholas Piggin <npiggin@gmail.com>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 target/ppc/excp_helper.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index c0605f84d70f..ba18444f5d1e 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -198,6 +198,7 @@ static inline void ppc_excp_apply_ail(PowerPCCPU *cpu, int excp_model, int excp,
                                       target_ulong *vector)
 {
 #if defined(TARGET_PPC64)
+    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
     CPUPPCState *env = &cpu->env;
     bool mmu_all_on = ((msr >> MSR_IR) & 1) && ((msr >> MSR_DR) & 1);
     bool hv_escalation = !(msr & MSR_HVB) && (*new_msr & MSR_HVB);
@@ -240,7 +241,7 @@ static inline void ppc_excp_apply_ail(PowerPCCPU *cpu, int excp_model, int excp,
             return;
         }
 
-        if (*new_msr & MSR_HVB) {
+        if (*new_msr & MSR_HVB && pcc->lpcr_mask & LPCR_HAIL) {
             if (!(env->spr[SPR_LPCR] & LPCR_HAIL)) {
                 /* HV interrupts depend on LPCR[HAIL] */
                 return;
-- 
2.26.3


Re: [PATCH] target/ppc: Do not check for LPCR[HAIL] on power10_v1.0 CPUs
Posted by Nicholas Piggin 2 years, 11 months ago
Excerpts from Cédric Le Goater's message of May 4, 2021 7:59 pm:
> The LPCR[HAIL] bit only applies to POWER10 DD2 CPUs. On POWER10 DD1,
> the ail value should be extracted using the LPCR_AIL mask like on P9.
> 
> Cc: Nicholas Piggin <npiggin@gmail.com>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Thanks for this, my oversight for not realising the P10 CPU is DD1 
(which doesn't have HAIL).

I wonder if it could just use the POWER9 excp_model?

> ---
>  target/ppc/excp_helper.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index c0605f84d70f..ba18444f5d1e 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -198,6 +198,7 @@ static inline void ppc_excp_apply_ail(PowerPCCPU *cpu, int excp_model, int excp,
>                                        target_ulong *vector)
>  {
>  #if defined(TARGET_PPC64)
> +    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
>      CPUPPCState *env = &cpu->env;
>      bool mmu_all_on = ((msr >> MSR_IR) & 1) && ((msr >> MSR_DR) & 1);
>      bool hv_escalation = !(msr & MSR_HVB) && (*new_msr & MSR_HVB);
> @@ -240,7 +241,7 @@ static inline void ppc_excp_apply_ail(PowerPCCPU *cpu, int excp_model, int excp,
>              return;
>          }
>  
> -        if (*new_msr & MSR_HVB) {
> +        if (*new_msr & MSR_HVB && pcc->lpcr_mask & LPCR_HAIL) {
>              if (!(env->spr[SPR_LPCR] & LPCR_HAIL)) {
>                  /* HV interrupts depend on LPCR[HAIL] */
>                  return;
> -- 
> 2.26.3
> 
> 

Re: [PATCH] target/ppc: Do not check for LPCR[HAIL] on power10_v1.0 CPUs
Posted by Cédric Le Goater 2 years, 11 months ago
On 5/4/21 12:49 PM, Nicholas Piggin wrote:
> Excerpts from Cédric Le Goater's message of May 4, 2021 7:59 pm:
>> The LPCR[HAIL] bit only applies to POWER10 DD2 CPUs. On POWER10 DD1,
>> the ail value should be extracted using the LPCR_AIL mask like on P9.
>>
>> Cc: Nicholas Piggin <npiggin@gmail.com>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> 
> Thanks for this, my oversight for not realising the P10 CPU is DD1 
> (which doesn't have HAIL).
> 
> I wonder if it could just use the POWER9 excp_model?

Yes. Why not. It does bring up another problem which is how to define
(cleanly) different characteristics for CPUs of the same POWER family.

Currently, all P10s are under POWERPC_FAMILY(POWER10). This is a base 
abstract class and definitions can not depend on the PVR. See below
what needs to be done to add a custom LPCR mask for DD2 :/

We could also simply switch P10 to DD2. I would favor that instead of 
adding complexity. 

David, what is your opinion on this ? 

Thank,

C. 


Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 target/ppc/cpu-models.c |   13 +++++++++++--
 target/ppc/cpu-models.h |    1 +
 2 files changed, 12 insertions(+), 2 deletions(-)

Index: qemu-powernv-6.1.git/target/ppc/cpu-models.c
===================================================================
--- qemu-powernv-6.1.git.orig/target/ppc/cpu-models.c
+++ qemu-powernv-6.1.git/target/ppc/cpu-models.c
@@ -32,7 +32,7 @@
 /* PowerPC CPU definitions                                                 */
 #define POWERPC_DEF_PREFIX(pvr, svr, type)                                  \
     glue(glue(glue(glue(pvr, _), svr), _), type)
-#define POWERPC_DEF_SVR(_name, _desc, _pvr, _svr, _type)                    \
+#define __POWERPC_DEF_SVR(_name, _desc, _pvr, _svr, _type, _lpcr)           \
     static void                                                             \
     glue(POWERPC_DEF_PREFIX(_pvr, _svr, _type), _cpu_class_init)            \
     (ObjectClass *oc, void *data)                                           \
@@ -40,6 +40,7 @@
         DeviceClass *dc = DEVICE_CLASS(oc);                                 \
         PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);                       \
                                                                             \
+        pcc->lpcr_mask    |= _lpcr;                                         \
         pcc->pvr          = _pvr;                                           \
         pcc->svr          = _svr;                                           \
         dc->desc          = _desc;                                          \
@@ -63,6 +64,12 @@
     type_init(                                                              \
         glue(POWERPC_DEF_PREFIX(_pvr, _svr, _type), _cpu_register_types))
 
+#define POWERPC_DEF_SVR(_name, _desc, _pvr, _svr, _type)                    \
+    __POWERPC_DEF_SVR(_name, _desc, _pvr, _svr, _type, 0)
+
+#define POWERPC_DEF_LPCR(_name, _pvr, _type, _desc, _lpcr)                  \
+    __POWERPC_DEF_SVR(_name, _desc, _pvr, POWERPC_SVR_NONE, _type, _lpcr)
+
 #define POWERPC_DEF(_name, _pvr, _type, _desc)                              \
     POWERPC_DEF_SVR(_name, _desc, _pvr, POWERPC_SVR_NONE, _type)
 
@@ -776,6 +783,8 @@
                 "POWER9 v2.0")
     POWERPC_DEF("power10_v1.0",  CPU_POWERPC_POWER10_DD1,            POWER10,
                 "POWER10 v1.0")
+    POWERPC_DEF_LPCR("power10_v2.0",  CPU_POWERPC_POWER10_DD20,      POWER10,
+                     "POWER10 v2.0", LPCR_HAIL)
 #endif /* defined (TARGET_PPC64) */
 
 /***************************************************************************/
@@ -952,7 +961,7 @@ PowerPCCPUAlias ppc_cpu_aliases[] = {
     { "power8", "power8_v2.0" },
     { "power8nvl", "power8nvl_v1.0" },
     { "power9", "power9_v2.0" },
-    { "power10", "power10_v1.0" },
+    { "power10", "power10_v2.0" },
 #endif
 
     /* Generic PowerPCs */
Index: qemu-powernv-6.1.git/target/ppc/cpu-models.h
===================================================================
--- qemu-powernv-6.1.git.orig/target/ppc/cpu-models.h
+++ qemu-powernv-6.1.git/target/ppc/cpu-models.h
@@ -375,6 +375,7 @@ enum {
     CPU_POWERPC_POWER9_DD20        = 0x004E1200,
     CPU_POWERPC_POWER10_BASE       = 0x00800000,
     CPU_POWERPC_POWER10_DD1        = 0x00800100,
+    CPU_POWERPC_POWER10_DD20       = 0x00800200,
     CPU_POWERPC_970_v22            = 0x00390202,
     CPU_POWERPC_970FX_v10          = 0x00391100,
     CPU_POWERPC_970FX_v20          = 0x003C0200,

Re: [PATCH] target/ppc: Do not check for LPCR[HAIL] on power10_v1.0 CPUs
Posted by David Gibson 2 years, 10 months ago
On Tue, May 04, 2021 at 01:54:39PM +0200, Cédric Le Goater wrote:
> On 5/4/21 12:49 PM, Nicholas Piggin wrote:
> > Excerpts from Cédric Le Goater's message of May 4, 2021 7:59 pm:
> >> The LPCR[HAIL] bit only applies to POWER10 DD2 CPUs. On POWER10 DD1,
> >> the ail value should be extracted using the LPCR_AIL mask like on P9.
> >>
> >> Cc: Nicholas Piggin <npiggin@gmail.com>
> >> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> > 
> > Thanks for this, my oversight for not realising the P10 CPU is DD1 
> > (which doesn't have HAIL).
> > 
> > I wonder if it could just use the POWER9 excp_model?
> 
> Yes. Why not. It does bring up another problem which is how to define
> (cleanly) different characteristics for CPUs of the same POWER family.
> 
> Currently, all P10s are under POWERPC_FAMILY(POWER10). This is a base 
> abstract class and definitions can not depend on the PVR. See below
> what needs to be done to add a custom LPCR mask for DD2 :/
> 
> We could also simply switch P10 to DD2. I would favor that instead of 
> adding complexity.

Definitely.  I'm guessing DD1 POWER10s will never be seen outside IBM,
so I don't think we want support for them in upstream qemu at all.

> 
> David, what is your opinion on this ? 
> 
> Thank,
> 
> C. 
> 
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  target/ppc/cpu-models.c |   13 +++++++++++--
>  target/ppc/cpu-models.h |    1 +
>  2 files changed, 12 insertions(+), 2 deletions(-)
> 
> Index: qemu-powernv-6.1.git/target/ppc/cpu-models.c
> ===================================================================
> --- qemu-powernv-6.1.git.orig/target/ppc/cpu-models.c
> +++ qemu-powernv-6.1.git/target/ppc/cpu-models.c
> @@ -32,7 +32,7 @@
>  /* PowerPC CPU definitions                                                 */
>  #define POWERPC_DEF_PREFIX(pvr, svr, type)                                  \
>      glue(glue(glue(glue(pvr, _), svr), _), type)
> -#define POWERPC_DEF_SVR(_name, _desc, _pvr, _svr, _type)                    \
> +#define __POWERPC_DEF_SVR(_name, _desc, _pvr, _svr, _type, _lpcr)           \
>      static void                                                             \
>      glue(POWERPC_DEF_PREFIX(_pvr, _svr, _type), _cpu_class_init)            \
>      (ObjectClass *oc, void *data)                                           \
> @@ -40,6 +40,7 @@
>          DeviceClass *dc = DEVICE_CLASS(oc);                                 \
>          PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);                       \
>                                                                              \
> +        pcc->lpcr_mask    |= _lpcr;                                         \
>          pcc->pvr          = _pvr;                                           \
>          pcc->svr          = _svr;                                           \
>          dc->desc          = _desc;                                          \
> @@ -63,6 +64,12 @@
>      type_init(                                                              \
>          glue(POWERPC_DEF_PREFIX(_pvr, _svr, _type), _cpu_register_types))
>  
> +#define POWERPC_DEF_SVR(_name, _desc, _pvr, _svr, _type)                    \
> +    __POWERPC_DEF_SVR(_name, _desc, _pvr, _svr, _type, 0)
> +
> +#define POWERPC_DEF_LPCR(_name, _pvr, _type, _desc, _lpcr)                  \
> +    __POWERPC_DEF_SVR(_name, _desc, _pvr, POWERPC_SVR_NONE, _type, _lpcr)
> +
>  #define POWERPC_DEF(_name, _pvr, _type, _desc)                              \
>      POWERPC_DEF_SVR(_name, _desc, _pvr, POWERPC_SVR_NONE, _type)
>  
> @@ -776,6 +783,8 @@
>                  "POWER9 v2.0")
>      POWERPC_DEF("power10_v1.0",  CPU_POWERPC_POWER10_DD1,            POWER10,
>                  "POWER10 v1.0")
> +    POWERPC_DEF_LPCR("power10_v2.0",  CPU_POWERPC_POWER10_DD20,      POWER10,
> +                     "POWER10 v2.0", LPCR_HAIL)
>  #endif /* defined (TARGET_PPC64) */
>  
>  /***************************************************************************/
> @@ -952,7 +961,7 @@ PowerPCCPUAlias ppc_cpu_aliases[] = {
>      { "power8", "power8_v2.0" },
>      { "power8nvl", "power8nvl_v1.0" },
>      { "power9", "power9_v2.0" },
> -    { "power10", "power10_v1.0" },
> +    { "power10", "power10_v2.0" },
>  #endif
>  
>      /* Generic PowerPCs */
> Index: qemu-powernv-6.1.git/target/ppc/cpu-models.h
> ===================================================================
> --- qemu-powernv-6.1.git.orig/target/ppc/cpu-models.h
> +++ qemu-powernv-6.1.git/target/ppc/cpu-models.h
> @@ -375,6 +375,7 @@ enum {
>      CPU_POWERPC_POWER9_DD20        = 0x004E1200,
>      CPU_POWERPC_POWER10_BASE       = 0x00800000,
>      CPU_POWERPC_POWER10_DD1        = 0x00800100,
> +    CPU_POWERPC_POWER10_DD20       = 0x00800200,
>      CPU_POWERPC_970_v22            = 0x00390202,
>      CPU_POWERPC_970FX_v10          = 0x00391100,
>      CPU_POWERPC_970FX_v20          = 0x003C0200,
> 

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