linuxppc-dev.lists.ozlabs.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 1/2] powerpc: Introduce POWER10_DD1 feature
@ 2020-10-22  3:40 Ravi Bangoria
  2020-10-22  3:40 ` [PATCH v2 2/2] powerpc/watchpoint: Workaround P10 DD1 issue with VSX-32 byte instructions Ravi Bangoria
  2020-10-22  5:11 ` [PATCH v2 1/2] powerpc: Introduce POWER10_DD1 feature Jordan Niethe
  0 siblings, 2 replies; 6+ messages in thread
From: Ravi Bangoria @ 2020-10-22  3:40 UTC (permalink / raw)
  To: mpe
  Cc: christophe.leroy, ravi.bangoria, mikey, jniethe5, npiggin, maddy,
	paulus, naveen.n.rao, linuxppc-dev

POWER10_DD1 feature flag will be needed while adding
conditional code that applies only for Power10 DD1.

Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
---
 arch/powerpc/include/asm/cputable.h | 8 ++++++--
 arch/powerpc/kernel/dt_cpu_ftrs.c   | 3 +++
 arch/powerpc/kernel/prom.c          | 9 +++++++++
 3 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/include/asm/cputable.h b/arch/powerpc/include/asm/cputable.h
index 93bc70d4c9a1..d486f56c0d33 100644
--- a/arch/powerpc/include/asm/cputable.h
+++ b/arch/powerpc/include/asm/cputable.h
@@ -216,6 +216,7 @@ static inline void cpu_feature_keys_init(void) { }
 #define CPU_FTR_P9_RADIX_PREFETCH_BUG	LONG_ASM_CONST(0x0002000000000000)
 #define CPU_FTR_ARCH_31			LONG_ASM_CONST(0x0004000000000000)
 #define CPU_FTR_DAWR1			LONG_ASM_CONST(0x0008000000000000)
+#define CPU_FTR_POWER10_DD1		LONG_ASM_CONST(0x0010000000000000)
 
 #ifndef __ASSEMBLY__
 
@@ -479,6 +480,7 @@ static inline void cpu_feature_keys_init(void) { }
 	    CPU_FTR_DBELL | CPU_FTR_HAS_PPR | CPU_FTR_ARCH_207S | \
 	    CPU_FTR_TM_COMP | CPU_FTR_ARCH_300 | CPU_FTR_ARCH_31 | \
 	    CPU_FTR_DAWR | CPU_FTR_DAWR1)
+#define CPU_FTRS_POWER10_DD1	(CPU_FTRS_POWER10 | CPU_FTR_POWER10_DD1)
 #define CPU_FTRS_CELL	(CPU_FTR_LWSYNC | \
 	    CPU_FTR_PPCAS_ARCH_V2 | CPU_FTR_CTRL | \
 	    CPU_FTR_ALTIVEC_COMP | CPU_FTR_MMCRA | CPU_FTR_SMT | \
@@ -497,14 +499,16 @@ static inline void cpu_feature_keys_init(void) { }
 #define CPU_FTRS_POSSIBLE	\
 	    (CPU_FTRS_POWER7 | CPU_FTRS_POWER8E | CPU_FTRS_POWER8 | \
 	     CPU_FTR_ALTIVEC_COMP | CPU_FTR_VSX_COMP | CPU_FTRS_POWER9 | \
-	     CPU_FTRS_POWER9_DD2_1 | CPU_FTRS_POWER9_DD2_2 | CPU_FTRS_POWER10)
+	     CPU_FTRS_POWER9_DD2_1 | CPU_FTRS_POWER9_DD2_2 | CPU_FTRS_POWER10 | \
+	     CPU_FTRS_POWER10_DD1)
 #else
 #define CPU_FTRS_POSSIBLE	\
 	    (CPU_FTRS_PPC970 | CPU_FTRS_POWER5 | \
 	     CPU_FTRS_POWER6 | CPU_FTRS_POWER7 | CPU_FTRS_POWER8E | \
 	     CPU_FTRS_POWER8 | CPU_FTRS_CELL | CPU_FTRS_PA6T | \
 	     CPU_FTR_VSX_COMP | CPU_FTR_ALTIVEC_COMP | CPU_FTRS_POWER9 | \
-	     CPU_FTRS_POWER9_DD2_1 | CPU_FTRS_POWER9_DD2_2 | CPU_FTRS_POWER10)
+	     CPU_FTRS_POWER9_DD2_1 | CPU_FTRS_POWER9_DD2_2 | CPU_FTRS_POWER10 | \
+	     CPU_FTRS_POWER10_DD1)
 #endif /* CONFIG_CPU_LITTLE_ENDIAN */
 #endif
 #else
diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c b/arch/powerpc/kernel/dt_cpu_ftrs.c
index 1098863e17ee..b2327f2967ff 100644
--- a/arch/powerpc/kernel/dt_cpu_ftrs.c
+++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
@@ -811,6 +811,9 @@ static __init void cpufeatures_cpu_quirks(void)
 	}
 
 	update_tlbie_feature_flag(version);
+
+	if ((version & 0xffffffff) == 0x00800100)
+		cur_cpu_spec->cpu_features |= CPU_FTR_POWER10_DD1;
 }
 
 static void __init cpufeatures_setup_finished(void)
diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
index c1545f22c077..c778c81284f7 100644
--- a/arch/powerpc/kernel/prom.c
+++ b/arch/powerpc/kernel/prom.c
@@ -305,6 +305,14 @@ static void __init check_cpu_feature_properties(unsigned long node)
 	}
 }
 
+static void __init fixup_cpu_features(void)
+{
+	unsigned long version = mfspr(SPRN_PVR);
+
+	if ((version & 0xffffffff) == 0x00800100)
+		cur_cpu_spec->cpu_features |= CPU_FTR_POWER10_DD1;
+}
+
 static int __init early_init_dt_scan_cpus(unsigned long node,
 					  const char *uname, int depth,
 					  void *data)
@@ -378,6 +386,7 @@ static int __init early_init_dt_scan_cpus(unsigned long node,
 
 		check_cpu_feature_properties(node);
 		check_cpu_pa_features(node);
+		fixup_cpu_features();
 	}
 
 	identical_pvr_fixup(node);
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v2 2/2] powerpc/watchpoint: Workaround P10 DD1 issue with VSX-32 byte instructions
  2020-10-22  3:40 [PATCH v2 1/2] powerpc: Introduce POWER10_DD1 feature Ravi Bangoria
@ 2020-10-22  3:40 ` Ravi Bangoria
  2020-10-22  5:11 ` [PATCH v2 1/2] powerpc: Introduce POWER10_DD1 feature Jordan Niethe
  1 sibling, 0 replies; 6+ messages in thread
From: Ravi Bangoria @ 2020-10-22  3:40 UTC (permalink / raw)
  To: mpe
  Cc: christophe.leroy, ravi.bangoria, mikey, jniethe5, npiggin, maddy,
	paulus, naveen.n.rao, linuxppc-dev

POWER10 DD1 has an issue where it generates watchpoint exceptions when it
shouldn't. The conditions where this occur are:

 - octword op
 - ending address of DAWR range is less than starting address of op
 - those addresses need to be in the same or in two consecutive 512B
   blocks
 - 'op address + 64B' generates an address that has a carry into bit
   52 (crosses 2K boundary)

Handle such spurious exception by considering them as extraneous and
emulating/single-steeping instruction without generating an event.

Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
[Fixed build warning reported by kernel test robot]
Reported-by: kernel test robot <lkp@intel.com>
---

Dependency: VSX-32 byte emulation support patches
  https://lore.kernel.org/r/20201011050908.72173-1-ravi.bangoria@linux.ibm.com

 arch/powerpc/kernel/hw_breakpoint.c | 67 ++++++++++++++++++++++++++++-
 1 file changed, 65 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
index f4e8f21046f5..9e83dd3d2ec5 100644
--- a/arch/powerpc/kernel/hw_breakpoint.c
+++ b/arch/powerpc/kernel/hw_breakpoint.c
@@ -499,6 +499,11 @@ static bool is_larx_stcx_instr(int type)
 	return type == LARX || type == STCX;
 }
 
+static bool is_octword_vsx_instr(int type, int size)
+{
+	return ((type == LOAD_VSX || type == STORE_VSX) && size == 32);
+}
+
 /*
  * We've failed in reliably handling the hw-breakpoint. Unregister
  * it and throw a warning message to let the user know about it.
@@ -549,6 +554,58 @@ static bool stepping_handler(struct pt_regs *regs, struct perf_event **bp,
 	return true;
 }
 
+static void handle_p10dd1_spurious_exception(struct arch_hw_breakpoint **info,
+					     int *hit, unsigned long ea)
+{
+	int i;
+	unsigned long hw_end_addr;
+
+	/*
+	 * Handle spurious exception only when any bp_per_reg is set.
+	 * Otherwise this might be created by xmon and not actually a
+	 * spurious exception.
+	 */
+	for (i = 0; i < nr_wp_slots(); i++) {
+		if (!info[i])
+			continue;
+
+		hw_end_addr = ALIGN(info[i]->address + info[i]->len, HW_BREAKPOINT_SIZE);
+
+		/*
+		 * Ending address of DAWR range is less than starting
+		 * address of op.
+		 */
+		if ((hw_end_addr - 1) >= ea)
+			continue;
+
+		/*
+		 * Those addresses need to be in the same or in two
+		 * consecutive 512B blocks;
+		 */
+		if (((hw_end_addr - 1) >> 10) != (ea >> 10))
+			continue;
+
+		/*
+		 * 'op address + 64B' generates an address that has a
+		 * carry into bit 52 (crosses 2K boundary).
+		 */
+		if ((ea & 0x800) == ((ea + 64) & 0x800))
+			continue;
+
+		break;
+	}
+
+	if (i == nr_wp_slots())
+		return;
+
+	for (i = 0; i < nr_wp_slots(); i++) {
+		if (info[i]) {
+			hit[i] = 1;
+			info[i]->type |= HW_BRK_TYPE_EXTRANEOUS_IRQ;
+		}
+	}
+}
+
 int hw_breakpoint_handler(struct die_args *args)
 {
 	bool err = false;
@@ -607,8 +664,14 @@ int hw_breakpoint_handler(struct die_args *args)
 		goto reset;
 
 	if (!nr_hit) {
-		rc = NOTIFY_DONE;
-		goto out;
+		if (cpu_has_feature(CPU_FTR_POWER10_DD1) &&
+		    !IS_ENABLED(CONFIG_PPC_8xx) &&
+		    is_octword_vsx_instr(type, size)) {
+			handle_p10dd1_spurious_exception(info, hit, ea);
+		} else {
+			rc = NOTIFY_DONE;
+			goto out;
+		}
 	}
 
 	/*
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 1/2] powerpc: Introduce POWER10_DD1 feature
  2020-10-22  3:40 [PATCH v2 1/2] powerpc: Introduce POWER10_DD1 feature Ravi Bangoria
  2020-10-22  3:40 ` [PATCH v2 2/2] powerpc/watchpoint: Workaround P10 DD1 issue with VSX-32 byte instructions Ravi Bangoria
@ 2020-10-22  5:11 ` Jordan Niethe
  2020-10-22  5:33   ` Ravi Bangoria
  1 sibling, 1 reply; 6+ messages in thread
From: Jordan Niethe @ 2020-10-22  5:11 UTC (permalink / raw)
  To: Ravi Bangoria
  Cc: Christophe Leroy, Michael Neuling, Nicholas Piggin, maddy,
	Paul Mackerras, naveen.n.rao, linuxppc-dev

On Thu, Oct 22, 2020 at 2:40 PM Ravi Bangoria
<ravi.bangoria@linux.ibm.com> wrote:
>
> POWER10_DD1 feature flag will be needed while adding
> conditional code that applies only for Power10 DD1.
>
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> ---
>  arch/powerpc/include/asm/cputable.h | 8 ++++++--
>  arch/powerpc/kernel/dt_cpu_ftrs.c   | 3 +++
>  arch/powerpc/kernel/prom.c          | 9 +++++++++
>  3 files changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/cputable.h b/arch/powerpc/include/asm/cputable.h
> index 93bc70d4c9a1..d486f56c0d33 100644
> --- a/arch/powerpc/include/asm/cputable.h
> +++ b/arch/powerpc/include/asm/cputable.h
> @@ -216,6 +216,7 @@ static inline void cpu_feature_keys_init(void) { }
>  #define CPU_FTR_P9_RADIX_PREFETCH_BUG  LONG_ASM_CONST(0x0002000000000000)
>  #define CPU_FTR_ARCH_31                        LONG_ASM_CONST(0x0004000000000000)
>  #define CPU_FTR_DAWR1                  LONG_ASM_CONST(0x0008000000000000)
> +#define CPU_FTR_POWER10_DD1            LONG_ASM_CONST(0x0010000000000000)
>
>  #ifndef __ASSEMBLY__
>
> @@ -479,6 +480,7 @@ static inline void cpu_feature_keys_init(void) { }
>             CPU_FTR_DBELL | CPU_FTR_HAS_PPR | CPU_FTR_ARCH_207S | \
>             CPU_FTR_TM_COMP | CPU_FTR_ARCH_300 | CPU_FTR_ARCH_31 | \
>             CPU_FTR_DAWR | CPU_FTR_DAWR1)
> +#define CPU_FTRS_POWER10_DD1   (CPU_FTRS_POWER10 | CPU_FTR_POWER10_DD1)
>  #define CPU_FTRS_CELL  (CPU_FTR_LWSYNC | \
>             CPU_FTR_PPCAS_ARCH_V2 | CPU_FTR_CTRL | \
>             CPU_FTR_ALTIVEC_COMP | CPU_FTR_MMCRA | CPU_FTR_SMT | \
> @@ -497,14 +499,16 @@ static inline void cpu_feature_keys_init(void) { }
>  #define CPU_FTRS_POSSIBLE      \
>             (CPU_FTRS_POWER7 | CPU_FTRS_POWER8E | CPU_FTRS_POWER8 | \
>              CPU_FTR_ALTIVEC_COMP | CPU_FTR_VSX_COMP | CPU_FTRS_POWER9 | \
> -            CPU_FTRS_POWER9_DD2_1 | CPU_FTRS_POWER9_DD2_2 | CPU_FTRS_POWER10)
> +            CPU_FTRS_POWER9_DD2_1 | CPU_FTRS_POWER9_DD2_2 | CPU_FTRS_POWER10 | \
> +            CPU_FTRS_POWER10_DD1)
>  #else
>  #define CPU_FTRS_POSSIBLE      \
>             (CPU_FTRS_PPC970 | CPU_FTRS_POWER5 | \
>              CPU_FTRS_POWER6 | CPU_FTRS_POWER7 | CPU_FTRS_POWER8E | \
>              CPU_FTRS_POWER8 | CPU_FTRS_CELL | CPU_FTRS_PA6T | \
>              CPU_FTR_VSX_COMP | CPU_FTR_ALTIVEC_COMP | CPU_FTRS_POWER9 | \
> -            CPU_FTRS_POWER9_DD2_1 | CPU_FTRS_POWER9_DD2_2 | CPU_FTRS_POWER10)
> +            CPU_FTRS_POWER9_DD2_1 | CPU_FTRS_POWER9_DD2_2 | CPU_FTRS_POWER10 | \
> +            CPU_FTRS_POWER10_DD1)
>  #endif /* CONFIG_CPU_LITTLE_ENDIAN */
>  #endif
>  #else
> diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c b/arch/powerpc/kernel/dt_cpu_ftrs.c
> index 1098863e17ee..b2327f2967ff 100644
> --- a/arch/powerpc/kernel/dt_cpu_ftrs.c
> +++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
> @@ -811,6 +811,9 @@ static __init void cpufeatures_cpu_quirks(void)
>         }
>
>         update_tlbie_feature_flag(version);
> +
> +       if ((version & 0xffffffff) == 0x00800100)
> +               cur_cpu_spec->cpu_features |= CPU_FTR_POWER10_DD1;
>  }
>
>  static void __init cpufeatures_setup_finished(void)
> diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
> index c1545f22c077..c778c81284f7 100644
> --- a/arch/powerpc/kernel/prom.c
> +++ b/arch/powerpc/kernel/prom.c
> @@ -305,6 +305,14 @@ static void __init check_cpu_feature_properties(unsigned long node)
>         }
>  }
>
> +static void __init fixup_cpu_features(void)
> +{
> +       unsigned long version = mfspr(SPRN_PVR);
> +
> +       if ((version & 0xffffffff) == 0x00800100)
> +               cur_cpu_spec->cpu_features |= CPU_FTR_POWER10_DD1;
> +}
> +
I am just wondering why this is needed here, but the same thing is not
done for, say, CPU_FTR_POWER9_DD2_1?
And should we get a /* Power10 DD 1 */ added to cpu_specs[]?

>  static int __init early_init_dt_scan_cpus(unsigned long node,
>                                           const char *uname, int depth,
>                                           void *data)
> @@ -378,6 +386,7 @@ static int __init early_init_dt_scan_cpus(unsigned long node,
>
>                 check_cpu_feature_properties(node);
>                 check_cpu_pa_features(node);
> +               fixup_cpu_features();
>         }
>
>         identical_pvr_fixup(node);
> --
> 2.25.1
>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 1/2] powerpc: Introduce POWER10_DD1 feature
  2020-10-22  5:11 ` [PATCH v2 1/2] powerpc: Introduce POWER10_DD1 feature Jordan Niethe
@ 2020-10-22  5:33   ` Ravi Bangoria
  2020-10-22  6:26     ` Jordan Niethe
  0 siblings, 1 reply; 6+ messages in thread
From: Ravi Bangoria @ 2020-10-22  5:33 UTC (permalink / raw)
  To: Jordan Niethe
  Cc: Christophe Leroy, Ravi Bangoria, Michael Neuling,
	Nicholas Piggin, maddy, Paul Mackerras, naveen.n.rao,
	linuxppc-dev



On 10/22/20 10:41 AM, Jordan Niethe wrote:
> On Thu, Oct 22, 2020 at 2:40 PM Ravi Bangoria
> <ravi.bangoria@linux.ibm.com> wrote:
>>
>> POWER10_DD1 feature flag will be needed while adding
>> conditional code that applies only for Power10 DD1.
>>
>> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
>> ---
>>   arch/powerpc/include/asm/cputable.h | 8 ++++++--
>>   arch/powerpc/kernel/dt_cpu_ftrs.c   | 3 +++
>>   arch/powerpc/kernel/prom.c          | 9 +++++++++
>>   3 files changed, 18 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/cputable.h b/arch/powerpc/include/asm/cputable.h
>> index 93bc70d4c9a1..d486f56c0d33 100644
>> --- a/arch/powerpc/include/asm/cputable.h
>> +++ b/arch/powerpc/include/asm/cputable.h
>> @@ -216,6 +216,7 @@ static inline void cpu_feature_keys_init(void) { }
>>   #define CPU_FTR_P9_RADIX_PREFETCH_BUG  LONG_ASM_CONST(0x0002000000000000)
>>   #define CPU_FTR_ARCH_31                        LONG_ASM_CONST(0x0004000000000000)
>>   #define CPU_FTR_DAWR1                  LONG_ASM_CONST(0x0008000000000000)
>> +#define CPU_FTR_POWER10_DD1            LONG_ASM_CONST(0x0010000000000000)
>>
>>   #ifndef __ASSEMBLY__
>>
>> @@ -479,6 +480,7 @@ static inline void cpu_feature_keys_init(void) { }
>>              CPU_FTR_DBELL | CPU_FTR_HAS_PPR | CPU_FTR_ARCH_207S | \
>>              CPU_FTR_TM_COMP | CPU_FTR_ARCH_300 | CPU_FTR_ARCH_31 | \
>>              CPU_FTR_DAWR | CPU_FTR_DAWR1)
>> +#define CPU_FTRS_POWER10_DD1   (CPU_FTRS_POWER10 | CPU_FTR_POWER10_DD1)
>>   #define CPU_FTRS_CELL  (CPU_FTR_LWSYNC | \
>>              CPU_FTR_PPCAS_ARCH_V2 | CPU_FTR_CTRL | \
>>              CPU_FTR_ALTIVEC_COMP | CPU_FTR_MMCRA | CPU_FTR_SMT | \
>> @@ -497,14 +499,16 @@ static inline void cpu_feature_keys_init(void) { }
>>   #define CPU_FTRS_POSSIBLE      \
>>              (CPU_FTRS_POWER7 | CPU_FTRS_POWER8E | CPU_FTRS_POWER8 | \
>>               CPU_FTR_ALTIVEC_COMP | CPU_FTR_VSX_COMP | CPU_FTRS_POWER9 | \
>> -            CPU_FTRS_POWER9_DD2_1 | CPU_FTRS_POWER9_DD2_2 | CPU_FTRS_POWER10)
>> +            CPU_FTRS_POWER9_DD2_1 | CPU_FTRS_POWER9_DD2_2 | CPU_FTRS_POWER10 | \
>> +            CPU_FTRS_POWER10_DD1)
>>   #else
>>   #define CPU_FTRS_POSSIBLE      \
>>              (CPU_FTRS_PPC970 | CPU_FTRS_POWER5 | \
>>               CPU_FTRS_POWER6 | CPU_FTRS_POWER7 | CPU_FTRS_POWER8E | \
>>               CPU_FTRS_POWER8 | CPU_FTRS_CELL | CPU_FTRS_PA6T | \
>>               CPU_FTR_VSX_COMP | CPU_FTR_ALTIVEC_COMP | CPU_FTRS_POWER9 | \
>> -            CPU_FTRS_POWER9_DD2_1 | CPU_FTRS_POWER9_DD2_2 | CPU_FTRS_POWER10)
>> +            CPU_FTRS_POWER9_DD2_1 | CPU_FTRS_POWER9_DD2_2 | CPU_FTRS_POWER10 | \
>> +            CPU_FTRS_POWER10_DD1)
>>   #endif /* CONFIG_CPU_LITTLE_ENDIAN */
>>   #endif
>>   #else
>> diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c b/arch/powerpc/kernel/dt_cpu_ftrs.c
>> index 1098863e17ee..b2327f2967ff 100644
>> --- a/arch/powerpc/kernel/dt_cpu_ftrs.c
>> +++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
>> @@ -811,6 +811,9 @@ static __init void cpufeatures_cpu_quirks(void)
>>          }
>>
>>          update_tlbie_feature_flag(version);
>> +
>> +       if ((version & 0xffffffff) == 0x00800100)
>> +               cur_cpu_spec->cpu_features |= CPU_FTR_POWER10_DD1;
>>   }
>>
>>   static void __init cpufeatures_setup_finished(void)
>> diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
>> index c1545f22c077..c778c81284f7 100644
>> --- a/arch/powerpc/kernel/prom.c
>> +++ b/arch/powerpc/kernel/prom.c
>> @@ -305,6 +305,14 @@ static void __init check_cpu_feature_properties(unsigned long node)
>>          }
>>   }
>>
>> +static void __init fixup_cpu_features(void)
>> +{
>> +       unsigned long version = mfspr(SPRN_PVR);
>> +
>> +       if ((version & 0xffffffff) == 0x00800100)
>> +               cur_cpu_spec->cpu_features |= CPU_FTR_POWER10_DD1;
>> +}
>> +
> I am just wondering why this is needed here, but the same thing is not
> done for, say, CPU_FTR_POWER9_DD2_1?

When we don't use DT cpu_features (PowerVM / kvm geusts), we call
identify_cpu() twice. First with Real PVR which sets "raw" cpu_spec
as cur_cpu_spec and then 2nd time with Logical PVR (0x0f...) which
(mostly) overwrites the cur_cpu_spec with "architected" mode cpu_spec.
I don't see DD version specific entries for "architected" mode in
cpu_specs[] for any previous processors. So I've introduced this
function to tweak cpu_features.

Though, I don't know why we don't have similar thing for
CPU_FTR_POWER9_DD2_1. I've to check that.

> And should we get a /* Power10 DD 1 */ added to cpu_specs[]?

IIUC, we don't need such entry. For PowerVM / kvm guests, we overwrite
cpu_spec, so /* Power10 */ "raw" entry is sufficient. And For baremetal,
we don't use cpu_specs[] at all.

> 
>>   static int __init early_init_dt_scan_cpus(unsigned long node,
>>                                            const char *uname, int depth,
>>                                            void *data)
>> @@ -378,6 +386,7 @@ static int __init early_init_dt_scan_cpus(unsigned long node,
>>
>>                  check_cpu_feature_properties(node);
>>                  check_cpu_pa_features(node);
>> +               fixup_cpu_features();
>>          }
>>
>>          identical_pvr_fixup(node);
>> --
>> 2.25.1
>>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 1/2] powerpc: Introduce POWER10_DD1 feature
  2020-10-22  5:33   ` Ravi Bangoria
@ 2020-10-22  6:26     ` Jordan Niethe
  2020-10-23 10:14       ` Ravi Bangoria
  0 siblings, 1 reply; 6+ messages in thread
From: Jordan Niethe @ 2020-10-22  6:26 UTC (permalink / raw)
  To: Ravi Bangoria
  Cc: Christophe Leroy, Michael Neuling, Nicholas Piggin, maddy,
	Paul Mackerras, naveen.n.rao, linuxppc-dev

On Thu, Oct 22, 2020 at 4:33 PM Ravi Bangoria
<ravi.bangoria@linux.ibm.com> wrote:
>
>
>
> On 10/22/20 10:41 AM, Jordan Niethe wrote:
> > On Thu, Oct 22, 2020 at 2:40 PM Ravi Bangoria
> > <ravi.bangoria@linux.ibm.com> wrote:
> >>
> >> POWER10_DD1 feature flag will be needed while adding
> >> conditional code that applies only for Power10 DD1.
> >>
> >> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> >> ---
> >>   arch/powerpc/include/asm/cputable.h | 8 ++++++--
> >>   arch/powerpc/kernel/dt_cpu_ftrs.c   | 3 +++
> >>   arch/powerpc/kernel/prom.c          | 9 +++++++++
> >>   3 files changed, 18 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/arch/powerpc/include/asm/cputable.h b/arch/powerpc/include/asm/cputable.h
> >> index 93bc70d4c9a1..d486f56c0d33 100644
> >> --- a/arch/powerpc/include/asm/cputable.h
> >> +++ b/arch/powerpc/include/asm/cputable.h
> >> @@ -216,6 +216,7 @@ static inline void cpu_feature_keys_init(void) { }
> >>   #define CPU_FTR_P9_RADIX_PREFETCH_BUG  LONG_ASM_CONST(0x0002000000000000)
> >>   #define CPU_FTR_ARCH_31                        LONG_ASM_CONST(0x0004000000000000)
> >>   #define CPU_FTR_DAWR1                  LONG_ASM_CONST(0x0008000000000000)
> >> +#define CPU_FTR_POWER10_DD1            LONG_ASM_CONST(0x0010000000000000)
> >>
> >>   #ifndef __ASSEMBLY__
> >>
> >> @@ -479,6 +480,7 @@ static inline void cpu_feature_keys_init(void) { }
> >>              CPU_FTR_DBELL | CPU_FTR_HAS_PPR | CPU_FTR_ARCH_207S | \
> >>              CPU_FTR_TM_COMP | CPU_FTR_ARCH_300 | CPU_FTR_ARCH_31 | \
> >>              CPU_FTR_DAWR | CPU_FTR_DAWR1)
> >> +#define CPU_FTRS_POWER10_DD1   (CPU_FTRS_POWER10 | CPU_FTR_POWER10_DD1)
> >>   #define CPU_FTRS_CELL  (CPU_FTR_LWSYNC | \
> >>              CPU_FTR_PPCAS_ARCH_V2 | CPU_FTR_CTRL | \
> >>              CPU_FTR_ALTIVEC_COMP | CPU_FTR_MMCRA | CPU_FTR_SMT | \
> >> @@ -497,14 +499,16 @@ static inline void cpu_feature_keys_init(void) { }
> >>   #define CPU_FTRS_POSSIBLE      \
> >>              (CPU_FTRS_POWER7 | CPU_FTRS_POWER8E | CPU_FTRS_POWER8 | \
> >>               CPU_FTR_ALTIVEC_COMP | CPU_FTR_VSX_COMP | CPU_FTRS_POWER9 | \
> >> -            CPU_FTRS_POWER9_DD2_1 | CPU_FTRS_POWER9_DD2_2 | CPU_FTRS_POWER10)
> >> +            CPU_FTRS_POWER9_DD2_1 | CPU_FTRS_POWER9_DD2_2 | CPU_FTRS_POWER10 | \
> >> +            CPU_FTRS_POWER10_DD1)
> >>   #else
> >>   #define CPU_FTRS_POSSIBLE      \
> >>              (CPU_FTRS_PPC970 | CPU_FTRS_POWER5 | \
> >>               CPU_FTRS_POWER6 | CPU_FTRS_POWER7 | CPU_FTRS_POWER8E | \
> >>               CPU_FTRS_POWER8 | CPU_FTRS_CELL | CPU_FTRS_PA6T | \
> >>               CPU_FTR_VSX_COMP | CPU_FTR_ALTIVEC_COMP | CPU_FTRS_POWER9 | \
> >> -            CPU_FTRS_POWER9_DD2_1 | CPU_FTRS_POWER9_DD2_2 | CPU_FTRS_POWER10)
> >> +            CPU_FTRS_POWER9_DD2_1 | CPU_FTRS_POWER9_DD2_2 | CPU_FTRS_POWER10 | \
> >> +            CPU_FTRS_POWER10_DD1)
> >>   #endif /* CONFIG_CPU_LITTLE_ENDIAN */
> >>   #endif
> >>   #else
> >> diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c b/arch/powerpc/kernel/dt_cpu_ftrs.c
> >> index 1098863e17ee..b2327f2967ff 100644
> >> --- a/arch/powerpc/kernel/dt_cpu_ftrs.c
> >> +++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
> >> @@ -811,6 +811,9 @@ static __init void cpufeatures_cpu_quirks(void)
> >>          }
> >>
> >>          update_tlbie_feature_flag(version);
> >> +
> >> +       if ((version & 0xffffffff) == 0x00800100)
> >> +               cur_cpu_spec->cpu_features |= CPU_FTR_POWER10_DD1;
> >>   }
> >>
> >>   static void __init cpufeatures_setup_finished(void)
> >> diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
> >> index c1545f22c077..c778c81284f7 100644
> >> --- a/arch/powerpc/kernel/prom.c
> >> +++ b/arch/powerpc/kernel/prom.c
> >> @@ -305,6 +305,14 @@ static void __init check_cpu_feature_properties(unsigned long node)
> >>          }
> >>   }
> >>
> >> +static void __init fixup_cpu_features(void)
> >> +{
> >> +       unsigned long version = mfspr(SPRN_PVR);
> >> +
> >> +       if ((version & 0xffffffff) == 0x00800100)
> >> +               cur_cpu_spec->cpu_features |= CPU_FTR_POWER10_DD1;
> >> +}
> >> +
> > I am just wondering why this is needed here, but the same thing is not
> > done for, say, CPU_FTR_POWER9_DD2_1?
>
> When we don't use DT cpu_features (PowerVM / kvm geusts), we call
> identify_cpu() twice. First with Real PVR which sets "raw" cpu_spec
> as cur_cpu_spec and then 2nd time with Logical PVR (0x0f...) which
> (mostly) overwrites the cur_cpu_spec with "architected" mode cpu_spec.
> I don't see DD version specific entries for "architected" mode in
> cpu_specs[] for any previous processors. So I've introduced this
> function to tweak cpu_features.
>
> Though, I don't know why we don't have similar thing for
> CPU_FTR_POWER9_DD2_1. I've to check that.
>
> > And should we get a /* Power10 DD 1 */ added to cpu_specs[]?
>
> IIUC, we don't need such entry. For PowerVM / kvm guests, we overwrite
> cpu_spec, so /* Power10 */ "raw" entry is sufficient. And For baremetal,
> we don't use cpu_specs[] at all.
I think even for powernv, using dt features can be disabled by the
cmdline with dt_cpu_ftrs=off, then cpu_specs[] will then be used.
>
> >
> >>   static int __init early_init_dt_scan_cpus(unsigned long node,
> >>                                            const char *uname, int depth,
> >>                                            void *data)
> >> @@ -378,6 +386,7 @@ static int __init early_init_dt_scan_cpus(unsigned long node,
> >>
> >>                  check_cpu_feature_properties(node);
> >>                  check_cpu_pa_features(node);
> >> +               fixup_cpu_features();
> >>          }
> >>
> >>          identical_pvr_fixup(node);
> >> --
> >> 2.25.1
> >>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 1/2] powerpc: Introduce POWER10_DD1 feature
  2020-10-22  6:26     ` Jordan Niethe
@ 2020-10-23 10:14       ` Ravi Bangoria
  0 siblings, 0 replies; 6+ messages in thread
From: Ravi Bangoria @ 2020-10-23 10:14 UTC (permalink / raw)
  To: Jordan Niethe
  Cc: Christophe Leroy, Ravi Bangoria, Michael Neuling,
	Nicholas Piggin, maddy, Paul Mackerras, naveen.n.rao,
	linuxppc-dev


>>>> +static void __init fixup_cpu_features(void)
>>>> +{
>>>> +       unsigned long version = mfspr(SPRN_PVR);
>>>> +
>>>> +       if ((version & 0xffffffff) == 0x00800100)
>>>> +               cur_cpu_spec->cpu_features |= CPU_FTR_POWER10_DD1;
>>>> +}
>>>> +
>>> I am just wondering why this is needed here, but the same thing is not
>>> done for, say, CPU_FTR_POWER9_DD2_1?
>>
>> When we don't use DT cpu_features (PowerVM / kvm geusts), we call
>> identify_cpu() twice. First with Real PVR which sets "raw" cpu_spec
>> as cur_cpu_spec and then 2nd time with Logical PVR (0x0f...) which
>> (mostly) overwrites the cur_cpu_spec with "architected" mode cpu_spec.
>> I don't see DD version specific entries for "architected" mode in
>> cpu_specs[] for any previous processors. So I've introduced this
>> function to tweak cpu_features.
>>
>> Though, I don't know why we don't have similar thing for
>> CPU_FTR_POWER9_DD2_1. I've to check that.
>>
>>> And should we get a /* Power10 DD 1 */ added to cpu_specs[]?
>>
>> IIUC, we don't need such entry. For PowerVM / kvm guests, we overwrite
>> cpu_spec, so /* Power10 */ "raw" entry is sufficient. And For baremetal,
>> we don't use cpu_specs[] at all.
> I think even for powernv, using dt features can be disabled by the
> cmdline with dt_cpu_ftrs=off, then cpu_specs[] will then be used.

Ok... with dt_cpu_ftrs=off, we seem to be using raw mode cpu_specs[] entry on
baremetal. So yeah, I'll add /* Power10 DD1 */ raw mode entry into cpu_specs[].
Thanks for pointing it out.

-Ravi

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2020-10-23 10:16 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-22  3:40 [PATCH v2 1/2] powerpc: Introduce POWER10_DD1 feature Ravi Bangoria
2020-10-22  3:40 ` [PATCH v2 2/2] powerpc/watchpoint: Workaround P10 DD1 issue with VSX-32 byte instructions Ravi Bangoria
2020-10-22  5:11 ` [PATCH v2 1/2] powerpc: Introduce POWER10_DD1 feature Jordan Niethe
2020-10-22  5:33   ` Ravi Bangoria
2020-10-22  6:26     ` Jordan Niethe
2020-10-23 10:14       ` Ravi Bangoria

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).