All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] x86/perf/zhaoxin: Add stepping check for ZX-C
@ 2023-02-02  9:17 silviazhao-oc
  2023-02-02 23:53 ` Kevin Brace
  2023-02-04 13:44 ` Borislav Petkov
  0 siblings, 2 replies; 10+ messages in thread
From: silviazhao-oc @ 2023-02-02  9:17 UTC (permalink / raw)
  To: peterz, mingo, acme, mark.rutland, alexander.shishkin, jolsa,
	namhyung, tglx, bp, dave.hansen, x86, hpa, linux-perf-users,
	linux-kernel
  Cc: cobechen, louisqi, silviazhao, tonywwang, kevinbrace, 8vvbbqzo567a

Nano processor may not fully support rdpmc instruction, it works well
for reading general pmc counter, but will lead GP(general protection)
when accessing fixed pmc counter. Furthermore, family/mode information
is same between Nano processor and ZX-C processor, it leads to zhaoxin
pmu driver is wrongly loaded for Nano processor, which resulting boot
kernal fail.

To solve this problem, stepping information will be checked to distinguish
between Nano processor and ZX-C processor.

Fixes: 3a4ac121c2ca (“x86/perf: Add hardware performance events support for Zhaoxin CPU”)
Reported-by: Arjan <8vvbbqzo567a@nospam.xutrox.com>
Link: https://bugzilla.kernel.org/show_bug.cgi?id=212389
Reported-by: Kevin Brace <kevinbrace@gmx.com>

Signed-off-by: silviazhao-oc <silviazhao-oc@zhaoxin.com>
---
 arch/x86/events/zhaoxin/core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/events/zhaoxin/core.c b/arch/x86/events/zhaoxin/core.c
index 949d845c922b..cef1de251613 100644
--- a/arch/x86/events/zhaoxin/core.c
+++ b/arch/x86/events/zhaoxin/core.c
@@ -541,7 +541,8 @@ __init int zhaoxin_pmu_init(void)
 
 	switch (boot_cpu_data.x86) {
 	case 0x06:
-		if (boot_cpu_data.x86_model == 0x0f || boot_cpu_data.x86_model == 0x19) {
+		if ((boot_cpu_data.x86_model == 0x0f && boot_cpu_data.x86_stepping >= 0x0e) ||
+			boot_cpu_data.x86_model == 0x19) {
 
 			x86_pmu.max_period = x86_pmu.cntval_mask >> 1;
 
-- 
2.17.1


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

* Re: [PATCH] x86/perf/zhaoxin: Add stepping check for ZX-C
  2023-02-02  9:17 [PATCH] x86/perf/zhaoxin: Add stepping check for ZX-C silviazhao-oc
@ 2023-02-02 23:53 ` Kevin Brace
  2023-02-03  3:12   ` silviazhaooc
  2023-02-04 13:44 ` Borislav Petkov
  1 sibling, 1 reply; 10+ messages in thread
From: Kevin Brace @ 2023-02-02 23:53 UTC (permalink / raw)
  To: silviazhao-oc
  Cc: peterz, mingo, acme, mark.rutland, alexander.shishkin, jolsa,
	namhyung, tglx, bp, dave.hansen, x86, hpa, linux-perf-users,
	linux-kernel, cobechen, louisqi, silviazhao, tonywwang,
	8vvbbqzo567a

Hi Silvia,

Thank you very much for resending the VIA / Zhaoxin PMU patch and for keeping me in the loop.
I observed this bug on ECS (Elitegroup Computer Systems) VX900-I mainboard.
The mainboard contains one VIA Nano L2007 processor (1.6 GHz) soldered on the PCB.
Although I have not independently verified it, CPUID steppings for VIA Nano 1000 / 2000 series (Centaur Technology code name: CNA) are supposedly 2 and 3.
CPUID stepppings for VIA Nano 3000 series (Centaur Technology code name: CNB) is 8.
CPUID stepppings for VIA Nano x2 (Centaur Technology code name: CNC) is 10.

https://www.reddit.com/r/VIA/comments/dy71bn/via_centaurs_new_cpu_is_a_8core_x86_cpu_with_an/

I have not checked the actual CPUID steppings, but I have confirmed that the current code without the fix works okay with Nano 3000 series and Nano x2, but definitely not Nano 2000 series.
For Nano 3000 series test, I used VIA Embedded EPIA M830 mainboard.
For Nano x2 test, I used HP T510 thin client.
Based on my observations, it appears that Centaur CNA contains a bug reading some performance counters, so not to cause inconveniences with users of Nano 1000 / 2000 series processors, the patch should limit / prevent reading performance counters on these processors.
I think the code for the fix should reflect this the following way.

_______________________________________________________________
 	switch (boot_cpu_data.x86) {
 	case 0x06:
-		if (boot_cpu_data.x86_model == 0x0f || boot_cpu_data.x86_model == 0x19) {
+		if ((boot_cpu_data.x86_model == 0x0f && boot_cpu_data.x86_stepping >= 0x08) ||
+			boot_cpu_data.x86_model == 0x19) {

 			x86_pmu.max_period = x86_pmu.cntval_mask >> 1;

_______________________________________________________________

The above code should exclude Nano 1000 / 2000 series processors properly.
I lost easy access to ECS VX900-I mainboard for now (I need to look around for it. I do own another copy of it.), so I cannot confirm if the fix is properly working.
I still have easy access to EPIA M830 mainboard.
    I welcome anyone's feedback on this issue.
This fix should go into the current kernel in development since it is a show stopper for users of Nano 1000 / 2000 series processors.
If the fix is adopted, please backport it to previous releases of the kernel.
I wasted about 2 weeks on this issue, and this fix should have never been ignored for such a long time.

Regards,

Kevin Brace
Brace Computer Laboratory blog
https://bracecomputerlab.com


> Sent: Thursday, February 02, 2023 at 3:17 AM
> From: "silviazhao-oc" <silviazhao-oc@zhaoxin.com>
> To: peterz@infradead.org, mingo@redhat.com, acme@kernel.org, mark.rutland@arm.com, alexander.shishkin@linux.intel.com, jolsa@kernel.org, namhyung@kernel.org, tglx@linutronix.de, bp@alien8.de, dave.hansen@linux.intel.com, x86@kernel.org, hpa@zytor.com, linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org
> Cc: cobechen@zhaoxin.com, louisqi@zhaoxin.com, silviazhao@zhaoxin.com, tonywwang@zhaoxin.com, kevinbrace@gmx.com, 8vvbbqzo567a@nospam.xutrox.com
> Subject: [PATCH] x86/perf/zhaoxin: Add stepping check for ZX-C
>
> Nano processor may not fully support rdpmc instruction, it works well
> for reading general pmc counter, but will lead GP(general protection)
> when accessing fixed pmc counter. Furthermore, family/mode information
> is same between Nano processor and ZX-C processor, it leads to zhaoxin
> pmu driver is wrongly loaded for Nano processor, which resulting boot
> kernal fail.
> 
> To solve this problem, stepping information will be checked to distinguish
> between Nano processor and ZX-C processor.
> 
> Fixes: 3a4ac121c2ca (“x86/perf: Add hardware performance events support for Zhaoxin CPU”)
> Reported-by: Arjan <8vvbbqzo567a@nospam.xutrox.com>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=212389
> Reported-by: Kevin Brace <kevinbrace@gmx.com>
> 
> Signed-off-by: silviazhao-oc <silviazhao-oc@zhaoxin.com>
> ---
>  arch/x86/events/zhaoxin/core.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/events/zhaoxin/core.c b/arch/x86/events/zhaoxin/core.c
> index 949d845c922b..cef1de251613 100644
> --- a/arch/x86/events/zhaoxin/core.c
> +++ b/arch/x86/events/zhaoxin/core.c
> @@ -541,7 +541,8 @@ __init int zhaoxin_pmu_init(void)
>  
>  	switch (boot_cpu_data.x86) {
>  	case 0x06:
> -		if (boot_cpu_data.x86_model == 0x0f || boot_cpu_data.x86_model == 0x19) {
> +		if ((boot_cpu_data.x86_model == 0x0f && boot_cpu_data.x86_stepping >= 0x0e) ||
> +			boot_cpu_data.x86_model == 0x19) {
>  
>  			x86_pmu.max_period = x86_pmu.cntval_mask >> 1;
>  
> -- 
> 2.17.1
> 
>

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

* Re: [PATCH] x86/perf/zhaoxin: Add stepping check for ZX-C
  2023-02-02 23:53 ` Kevin Brace
@ 2023-02-03  3:12   ` silviazhaooc
  0 siblings, 0 replies; 10+ messages in thread
From: silviazhaooc @ 2023-02-03  3:12 UTC (permalink / raw)
  To: Kevin Brace
  Cc: peterz, mingo, acme, mark.rutland, alexander.shishkin, jolsa,
	namhyung, tglx, bp, dave.hansen, x86, hpa, linux-perf-users,
	linux-kernel, cobechen, louisqi, silviazhao, tonywwang,
	8vvbbqzo567a

Hi Kevin,

	Thanks for your kindly reply.

	Since VIA Nano 1000/2000/3000 series are really very old CPU, and we 
can't find related mainboard for full verification. We suggest not to 
support all Nano series for PMC driver.

On 2023/2/3 07:53, Kevin Brace wrote:
> Hi Silvia,
> 
> Thank you very much for resending the VIA / Zhaoxin PMU patch and for keeping me in the loop.
> I observed this bug on ECS (Elitegroup Computer Systems) VX900-I mainboard.
> The mainboard contains one VIA Nano L2007 processor (1.6 GHz) soldered on the PCB.
> Although I have not independently verified it, CPUID steppings for VIA Nano 1000 / 2000 series (Centaur Technology code name: CNA) are supposedly 2 and 3.
> CPUID stepppings for VIA Nano 3000 series (Centaur Technology code name: CNB) is 8.
> CPUID stepppings for VIA Nano x2 (Centaur Technology code name: CNC) is 10.
> 
> https://www.reddit.com/r/VIA/comments/dy71bn/via_centaurs_new_cpu_is_a_8core_x86_cpu_with_an/
> 
> I have not checked the actual CPUID steppings, but I have confirmed that the current code without the fix works okay with Nano 3000 series and Nano x2, but definitely not Nano 2000 series.
> For Nano 3000 series test, I used VIA Embedded EPIA M830 mainboard.
> For Nano x2 test, I used HP T510 thin client.
> Based on my observations, it appears that Centaur CNA contains a bug reading some performance counters, so not to cause inconveniences with users of Nano 1000 / 2000 series processors, the patch should limit / prevent reading performance counters on these processors.
> I think the code for the fix should reflect this the following way.
> 
> _______________________________________________________________
>   	switch (boot_cpu_data.x86) {
>   	case 0x06:
> -		if (boot_cpu_data.x86_model == 0x0f || boot_cpu_data.x86_model == 0x19) {
> +		if ((boot_cpu_data.x86_model == 0x0f && boot_cpu_data.x86_stepping >= 0x08) ||
> +			boot_cpu_data.x86_model == 0x19) {
> 
>   			x86_pmu.max_period = x86_pmu.cntval_mask >> 1;
> 
> _______________________________________________________________
> 
> The above code should exclude Nano 1000 / 2000 series processors properly.
> I lost easy access to ECS VX900-I mainboard for now (I need to look around for it. I do own another copy of it.), so I cannot confirm if the fix is properly working.
> I still have easy access to EPIA M830 mainboard.
>      I welcome anyone's feedback on this issue.
> This fix should go into the current kernel in development since it is a show stopper for users of Nano 1000 / 2000 series processors.
> If the fix is adopted, please backport it to previous releases of the kernel.
> I wasted about 2 weeks on this issue, and this fix should have never been ignored for such a long time.
> 
> Regards,
> 
> Kevin Brace
> Brace Computer Laboratory blog
> https://bracecomputerlab.com
> 
> 
>> Sent: Thursday, February 02, 2023 at 3:17 AM
>> From: "silviazhao-oc" <silviazhao-oc@zhaoxin.com>
>> To: peterz@infradead.org, mingo@redhat.com, acme@kernel.org, mark.rutland@arm.com, alexander.shishkin@linux.intel.com, jolsa@kernel.org, namhyung@kernel.org, tglx@linutronix.de, bp@alien8.de, dave.hansen@linux.intel.com, x86@kernel.org, hpa@zytor.com, linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org
>> Cc: cobechen@zhaoxin.com, louisqi@zhaoxin.com, silviazhao@zhaoxin.com, tonywwang@zhaoxin.com, kevinbrace@gmx.com, 8vvbbqzo567a@nospam.xutrox.com
>> Subject: [PATCH] x86/perf/zhaoxin: Add stepping check for ZX-C
>>
>> Nano processor may not fully support rdpmc instruction, it works well
>> for reading general pmc counter, but will lead GP(general protection)
>> when accessing fixed pmc counter. Furthermore, family/mode information
>> is same between Nano processor and ZX-C processor, it leads to zhaoxin
>> pmu driver is wrongly loaded for Nano processor, which resulting boot
>> kernal fail.
>>
>> To solve this problem, stepping information will be checked to distinguish
>> between Nano processor and ZX-C processor.
>>
>> Fixes: 3a4ac121c2ca (“x86/perf: Add hardware performance events support for Zhaoxin CPU”)
>> Reported-by: Arjan <8vvbbqzo567a@nospam.xutrox.com>
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=212389
>> Reported-by: Kevin Brace <kevinbrace@gmx.com>
>>
>> Signed-off-by: silviazhao-oc <silviazhao-oc@zhaoxin.com>
>> ---
>>   arch/x86/events/zhaoxin/core.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/events/zhaoxin/core.c b/arch/x86/events/zhaoxin/core.c
>> index 949d845c922b..cef1de251613 100644
>> --- a/arch/x86/events/zhaoxin/core.c
>> +++ b/arch/x86/events/zhaoxin/core.c
>> @@ -541,7 +541,8 @@ __init int zhaoxin_pmu_init(void)
>>   
>>   	switch (boot_cpu_data.x86) {
>>   	case 0x06:
>> -		if (boot_cpu_data.x86_model == 0x0f || boot_cpu_data.x86_model == 0x19) {
>> +		if ((boot_cpu_data.x86_model == 0x0f && boot_cpu_data.x86_stepping >= 0x0e) ||
>> +			boot_cpu_data.x86_model == 0x19) {
>>   
>>   			x86_pmu.max_period = x86_pmu.cntval_mask >> 1;
>>   
>> -- 
>> 2.17.1
>>
>>

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

* Re: [PATCH] x86/perf/zhaoxin: Add stepping check for ZX-C
  2023-02-02  9:17 [PATCH] x86/perf/zhaoxin: Add stepping check for ZX-C silviazhao-oc
  2023-02-02 23:53 ` Kevin Brace
@ 2023-02-04 13:44 ` Borislav Petkov
  2023-02-06  8:26   ` silviazhaooc
  1 sibling, 1 reply; 10+ messages in thread
From: Borislav Petkov @ 2023-02-04 13:44 UTC (permalink / raw)
  To: silviazhao-oc
  Cc: peterz, mingo, acme, mark.rutland, alexander.shishkin, jolsa,
	namhyung, tglx, dave.hansen, x86, hpa, linux-perf-users,
	linux-kernel, cobechen, louisqi, silviazhao, tonywwang,
	kevinbrace, 8vvbbqzo567a

On Thu, Feb 02, 2023 at 05:17:38PM +0800, silviazhao-oc wrote:
> Nano processor may not fully support rdpmc instruction, it works well
> for reading general pmc counter, but will lead GP(general protection)
> when accessing fixed pmc counter. Furthermore, family/mode information
> is same between Nano processor and ZX-C processor, it leads to zhaoxin
> pmu driver is wrongly loaded for Nano processor, which resulting boot
> kernal fail.
> 
> To solve this problem, stepping information will be checked to distinguish
> between Nano processor and ZX-C processor.
> 
> Fixes: 3a4ac121c2ca (“x86/perf: Add hardware performance events support for Zhaoxin CPU”)
> Reported-by: Arjan <8vvbbqzo567a@nospam.xutrox.com>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=212389
> Reported-by: Kevin Brace <kevinbrace@gmx.com>
> 
> Signed-off-by: silviazhao-oc <silviazhao-oc@zhaoxin.com>

Please use your proper name in the Signed-off-by.

> ---
>  arch/x86/events/zhaoxin/core.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/events/zhaoxin/core.c b/arch/x86/events/zhaoxin/core.c
> index 949d845c922b..cef1de251613 100644
> --- a/arch/x86/events/zhaoxin/core.c
> +++ b/arch/x86/events/zhaoxin/core.c
> @@ -541,7 +541,8 @@ __init int zhaoxin_pmu_init(void)
>  
>  	switch (boot_cpu_data.x86) {
>  	case 0x06:
> -		if (boot_cpu_data.x86_model == 0x0f || boot_cpu_data.x86_model == 0x19) {
> +		if ((boot_cpu_data.x86_model == 0x0f && boot_cpu_data.x86_stepping >= 0x0e) ||
> +			boot_cpu_data.x86_model == 0x19) {
>  
>  			x86_pmu.max_period = x86_pmu.cntval_mask >> 1;

Last time we talked:

https://lore.kernel.org/r/3c7da7fd-402f-c74f-c96c-0e88828eab58@zhaoxin.com

you said that Nano #GPs when trying to RDPMC the fixed counters. Which
sounds like an erratum.

We do those by adding a X86_BUG flag, set that flag for Nano and then
test it where needed. Grep the source tree for examples.

Please do that above too unstead of testing steppings.

Also, I'd like to know why do steppings < 0xe mean Nano and why isn't
there a more reliable way to detect it?

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

* Re: [PATCH] x86/perf/zhaoxin: Add stepping check for ZX-C
  2023-02-04 13:44 ` Borislav Petkov
@ 2023-02-06  8:26   ` silviazhaooc
  2023-02-06  9:48     ` Borislav Petkov
  0 siblings, 1 reply; 10+ messages in thread
From: silviazhaooc @ 2023-02-06  8:26 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: peterz, mingo, acme, mark.rutland, alexander.shishkin, jolsa,
	namhyung, tglx, dave.hansen, x86, hpa, linux-perf-users,
	linux-kernel, cobechen, louisqi, silviazhao, tonywwang,
	kevinbrace, 8vvbbqzo567a



On 2023/2/4 21:44, Borislav Petkov wrote:
> On Thu, Feb 02, 2023 at 05:17:38PM +0800, silviazhao-oc wrote:
>> Nano processor may not fully support rdpmc instruction, it works well
>> for reading general pmc counter, but will lead GP(general protection)
>> when accessing fixed pmc counter. Furthermore, family/mode information
>> is same between Nano processor and ZX-C processor, it leads to zhaoxin
>> pmu driver is wrongly loaded for Nano processor, which resulting boot
>> kernal fail.
>>
>> To solve this problem, stepping information will be checked to distinguish
>> between Nano processor and ZX-C processor.
>>
>> Fixes: 3a4ac121c2ca (“x86/perf: Add hardware performance events support for Zhaoxin CPU”)
>> Reported-by: Arjan <8vvbbqzo567a@nospam.xutrox.com>
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=212389
>> Reported-by: Kevin Brace <kevinbrace@gmx.com>
>>
>> Signed-off-by: silviazhao-oc <silviazhao-oc@zhaoxin.com>
> 
> Please use your proper name in the Signed-off-by.
> 
Due to our company’s email policy, email address with oc suffix is used 
for sending email without confidentiality statement at the end of the 
mail body.

I will remove –oc from my name later.

>> ---
>>   arch/x86/events/zhaoxin/core.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/events/zhaoxin/core.c b/arch/x86/events/zhaoxin/core.c
>> index 949d845c922b..cef1de251613 100644
>> --- a/arch/x86/events/zhaoxin/core.c
>> +++ b/arch/x86/events/zhaoxin/core.c
>> @@ -541,7 +541,8 @@ __init int zhaoxin_pmu_init(void)
>>   
>>   	switch (boot_cpu_data.x86) {
>>   	case 0x06:
>> -		if (boot_cpu_data.x86_model == 0x0f || boot_cpu_data.x86_model == 0x19) {
>> +		if ((boot_cpu_data.x86_model == 0x0f && boot_cpu_data.x86_stepping >= 0x0e) ||
>> +			boot_cpu_data.x86_model == 0x19) {
>>   
>>   			x86_pmu.max_period = x86_pmu.cntval_mask >> 1;
> 
> Last time we talked:
> 
> https://lore.kernel.org/r/3c7da7fd-402f-c74f-c96c-0e88828eab58@zhaoxin.com
> 
> you said that Nano #GPs when trying to RDPMC the fixed counters. Which
> sounds like an erratum.
> 
> We do those by adding a X86_BUG flag, set that flag for Nano and then
> test it where needed. Grep the source tree for examples.
> 
> Please do that above too unstead of testing steppings.
> 
Nano series CPUs are too old, we have not fully verified their PMC 
functions, furthermore our original development plan is to support PMC 
from ZXC, not including Nano series.

In that way, I think it’s better to exclude Nano series CPU in the PMC 
driver.

> Also, I'd like to know why do steppings < 0xe mean Nano and why isn't
> there a more reliable way to detect it?
> 

Generally, CPUs are identified by FMS(Family/Model/Stepping) information.

As you said, it is not a good practice to use stepping information to 
distinguish different CPU series.

But due to some unknown historical reasons, the FMS of Nano and ZXC are 
only different in stepping.

I have considered about using the “Model name string” to distinguish 
them, but it doesn't seem to be a common way in Linux kernel.

> Thx.
> 

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

* Re: [PATCH] x86/perf/zhaoxin: Add stepping check for ZX-C
  2023-02-06  8:26   ` silviazhaooc
@ 2023-02-06  9:48     ` Borislav Petkov
  2023-02-06 10:55       ` silviazhaooc
  0 siblings, 1 reply; 10+ messages in thread
From: Borislav Petkov @ 2023-02-06  9:48 UTC (permalink / raw)
  To: silviazhaooc
  Cc: peterz, mingo, acme, mark.rutland, alexander.shishkin, jolsa,
	namhyung, tglx, dave.hansen, x86, hpa, linux-perf-users,
	linux-kernel, cobechen, louisqi, silviazhao, tonywwang,
	kevinbrace, 8vvbbqzo567a

On Mon, Feb 06, 2023 at 04:26:25PM +0800, silviazhaooc wrote:
> Due to our company’s email policy, email address with oc suffix is used for
> sending email without confidentiality statement at the end of the mail body.
> 
> I will remove –oc from my name later.

Yes, please. The email address is fine but the name doesn't have to have
that funky "-oc" thing.

> But due to some unknown historical reasons, the FMS of Nano and ZXC are only
> different in stepping.
> 
> I have considered about using the “Model name string” to distinguish them,
> but it doesn't seem to be a common way in Linux kernel.

I don't mind you using steppings to differentiate the two as long as
this is not going to change all of a sudden and that differentiation is
unambiguous.

If not, you will have to use name strings as you don't have any other
choice.

Whatever you do, pls define a new X86_BUG_ flag, set it only on Nano and
then test it in the PMU init code.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

* Re: [PATCH] x86/perf/zhaoxin: Add stepping check for ZX-C
  2023-02-06  9:48     ` Borislav Petkov
@ 2023-02-06 10:55       ` silviazhaooc
  2023-02-06 11:13         ` Borislav Petkov
  0 siblings, 1 reply; 10+ messages in thread
From: silviazhaooc @ 2023-02-06 10:55 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: peterz, mingo, acme, mark.rutland, alexander.shishkin, jolsa,
	namhyung, tglx, dave.hansen, x86, hpa, linux-perf-users,
	linux-kernel, cobechen, louisqi, silviazhao, tonywwang,
	kevinbrace, 8vvbbqzo567a

Hi Boris,

Thanks for your reply.

As I mentioned before, Nano has several series. We cannot test if all of 
them have the bug. Besides, AFAIK Nano's hardware support for PMC has 
not externally announced. So setting a new X86_BUG_ flag to Nano is 
inappropriate.

I still think exclude PMC support in driver is more appropriate.

Looking forward to your comments.


On 2023/2/6 17:48, Borislav Petkov wrote:
> On Mon, Feb 06, 2023 at 04:26:25PM +0800, silviazhaooc wrote:
>> Due to our company’s email policy, email address with oc suffix is used for
>> sending email without confidentiality statement at the end of the mail body.
>>
>> I will remove –oc from my name later.
> 
> Yes, please. The email address is fine but the name doesn't have to have
> that funky "-oc" thing.
> 
>> But due to some unknown historical reasons, the FMS of Nano and ZXC are only
>> different in stepping.
>>
>> I have considered about using the “Model name string” to distinguish them,
>> but it doesn't seem to be a common way in Linux kernel.
> 
> I don't mind you using steppings to differentiate the two as long as
> this is not going to change all of a sudden and that differentiation is
> unambiguous.
> 
> If not, you will have to use name strings as you don't have any other
> choice.
> 
> Whatever you do, pls define a new X86_BUG_ flag, set it only on Nano and
> then test it in the PMU init code.
> 
> Thx.
> 

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

* Re: [PATCH] x86/perf/zhaoxin: Add stepping check for ZX-C
  2023-02-06 10:55       ` silviazhaooc
@ 2023-02-06 11:13         ` Borislav Petkov
  2023-02-07  8:42           ` silviazhaooc
  0 siblings, 1 reply; 10+ messages in thread
From: Borislav Petkov @ 2023-02-06 11:13 UTC (permalink / raw)
  To: silviazhaooc
  Cc: peterz, mingo, acme, mark.rutland, alexander.shishkin, jolsa,
	namhyung, tglx, dave.hansen, x86, hpa, linux-perf-users,
	linux-kernel, cobechen, louisqi, silviazhao, tonywwang,
	kevinbrace, 8vvbbqzo567a

Hi Silvia,

On Mon, Feb 06, 2023 at 06:55:21PM +0800, silviazhaooc wrote:
> Thanks for your reply.

You're welcome.

First of all, please do not top-post when replying on a public mailing
list but put your reply under the text you're replying to. Like the rest
of us do.

> As I mentioned before, Nano has several series. We cannot test if all of
> them have the bug.

If you cannot test if all of them have the bug, then testing the
stepping as you do is wrong too.

You need an unambiguous way to differentiate between ZXC and Nano CPUs.

If steppings >= 0xe belong solely to ZXC, then state that in a comment
above it so that you can exclude Nano.

If Nano starts using those steppings later, though, then that check will
become wrong too.

So I need a statement: "this is how you detect a ZXC CPU unambiguously"
and then use that method when enabling PMU support on it and *only* on
it.

Makes more sense?

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

* Re: [PATCH] x86/perf/zhaoxin: Add stepping check for ZX-C
  2023-02-06 11:13         ` Borislav Petkov
@ 2023-02-07  8:42           ` silviazhaooc
  2023-02-07 18:30             ` Borislav Petkov
  0 siblings, 1 reply; 10+ messages in thread
From: silviazhaooc @ 2023-02-07  8:42 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: peterz, mingo, acme, mark.rutland, alexander.shishkin, jolsa,
	namhyung, tglx, dave.hansen, x86, hpa, linux-perf-users,
	linux-kernel, cobechen, louisqi, silviazhao, tonywwang,
	kevinbrace, 8vvbbqzo567a


On 2023/2/6 19:13, Borislav Petkov wrote:
> Hi Silvia,
> 
> On Mon, Feb 06, 2023 at 06:55:21PM +0800, silviazhaooc wrote:
>> Thanks for your reply.
> 
> You're welcome.
> 
> First of all, please do not top-post when replying on a public mailing
> list but put your reply under the text you're replying to. Like the rest
> of us do.
> 
Sorry, I'm a newbie in Linux. Thanks for your reminding.

>> As I mentioned before, Nano has several series. We cannot test if all of
>> them have the bug.
> 
> If you cannot test if all of them have the bug, then testing the
> stepping as you do is wrong too.
> 
> You need an unambiguous way to differentiate between ZXC and Nano CPUs.
> 
> If steppings >= 0xe belong solely to ZXC, then state that in a comment
> above it so that you can exclude Nano.
> 
> If Nano starts using those steppings later, though, then that check will
> become wrong too.
> 
> So I need a statement: "this is how you detect a ZXC CPU unambiguously"
> and then use that method when enabling PMU support on it and *only* on
> it.
> 
> Makes more sense?
> 
Yes, that makes sense.

I have carefully checked our product manual for Nano and ZXC FMS.

For ZXC, there are 2 kinds of FMS:

1. Family=6, Model=0x19, Stepping=0-3

2. Family=6, Model=F, Stepping=E-F

For Nano, there is only one kind of FMS:

Family=6, Model=F, Stepping=[0-A]/[C-D]

So model = 0xf, steppings >= 0xe or model = 0x19 belong solely to ZXC.
Nano is an old CPU series which has been stopped production several 
years ago. It will not use the steppings which belong to ZXC.This is an 
unambiguous way to differentiate between ZXC and Nano CPUs.

Do I need to add the statements in the source code and re-commit the patch?

Thx.

> Thx.
> 

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

* Re: [PATCH] x86/perf/zhaoxin: Add stepping check for ZX-C
  2023-02-07  8:42           ` silviazhaooc
@ 2023-02-07 18:30             ` Borislav Petkov
  0 siblings, 0 replies; 10+ messages in thread
From: Borislav Petkov @ 2023-02-07 18:30 UTC (permalink / raw)
  To: silviazhaooc
  Cc: peterz, mingo, acme, mark.rutland, alexander.shishkin, jolsa,
	namhyung, tglx, dave.hansen, x86, hpa, linux-perf-users,
	linux-kernel, cobechen, louisqi, silviazhao, tonywwang,
	kevinbrace, 8vvbbqzo567a

On Tue, Feb 07, 2023 at 04:42:26PM +0800, silviazhaooc wrote:
> Sorry, I'm a newbie in Linux. Thanks for your reminding.

No worries. :)

> I have carefully checked our product manual for Nano and ZXC FMS.
> 
> For ZXC, there are 2 kinds of FMS:
> 
> 1. Family=6, Model=0x19, Stepping=0-3
> 
> 2. Family=6, Model=F, Stepping=E-F
> 
> For Nano, there is only one kind of FMS:
> 
> Family=6, Model=F, Stepping=[0-A]/[C-D]
> 
> So model = 0xf, steppings >= 0xe or model = 0x19 belong solely to ZXC.
> Nano is an old CPU series which has been stopped production several years
> ago.

Good, which sounds like there won't be any more Nano steppings.

> It will not use the steppings which belong to ZXC.This is an
> unambiguous way to differentiate between ZXC and Nano CPUs.
> 
> Do I need to add the statements in the source code and re-commit the patch?

Yes please. That would explain in a clear way why it is ok to test those
models/steppings. If it turns out that we need this Nano - ZXC
distinction more often, we can do something more involved later.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

end of thread, other threads:[~2023-02-07 18:30 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-02  9:17 [PATCH] x86/perf/zhaoxin: Add stepping check for ZX-C silviazhao-oc
2023-02-02 23:53 ` Kevin Brace
2023-02-03  3:12   ` silviazhaooc
2023-02-04 13:44 ` Borislav Petkov
2023-02-06  8:26   ` silviazhaooc
2023-02-06  9:48     ` Borislav Petkov
2023-02-06 10:55       ` silviazhaooc
2023-02-06 11:13         ` Borislav Petkov
2023-02-07  8:42           ` silviazhaooc
2023-02-07 18:30             ` Borislav Petkov

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.