diff mbox series

[1/2] perf tools riscv: Allow get_cpuid return empty MARCH and MIMP

Message ID IA1PR20MB4953E8FFED81D5733DDFDA2DBB799@IA1PR20MB4953.namprd20.prod.outlook.com
State New
Headers
Series perf: add T-HEAD C9xx series cpu support |

Commit Message

Inochi Amaoto May 16, 2023, 2:37 a.m. UTC 
  The T-HEAD C9xx series CPU only has MVENDOR defined, and left MARCH
and MIMP unimplemented.

To make perf support T-HEAD C9xx events. remove the restriction of
the MARCH and MIMP.

Signed-off-by: Inochi Amaoto <inochiama@outlook.com>
---
 tools/perf/arch/riscv/util/header.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

Comments

Inochi Amaoto May 16, 2023, 9:43 a.m. UTC | #1 
> > The T-HEAD C9xx series CPU only has MVENDOR defined, and left MARCH
> > and MIMP unimplemented.
>
> According to the docs you can still read them, but it's hardwired to
> 64h0.
>
> How it's supposed to distinguish c906 and c910 for example ?

It is unnecessary to distinguish c9xx, their event index is compatible.
The dtb and opensbi will final decide which event can be used.

> What does /proc/cpuinfo shows on c9xx ? Why can't we use zeroes ?

The content is as follows.

processor     : 0
hart          : 0
isa           : rv64imafdc
mmu           : sv39
uarch         : thead,c910
mvendorid     : 0x5b7
marchid       : 0x0
mimpid        : 0x0

The `mvendorid`, `marchid`, `mimpid` are the same across allwinner D1 (C906),
T-HEAD th1520 (C910) and the sophgo mango (C920). It seems T-HEAD use MCPUID
CSR to store CPU info. But this is not standard and not shown in cpuinfo.
Nikita Shubin May 16, 2023, 10:28 a.m. UTC | #2 
Hello Inochi Amaoto!

On Tue, 2023-05-16 at 10:37 +0800, Inochi Amaoto wrote:
> The T-HEAD C9xx series CPU only has MVENDOR defined, and left MARCH
> and MIMP unimplemented.

According to the docs you can still read them, but it's hardwired to
64h0.

How it's supposed to distinguish c906 and c910 for example ?

> 
> To make perf support T-HEAD C9xx events. remove the restriction of
> the MARCH and MIMP.
> 
> Signed-off-by: Inochi Amaoto <inochiama@outlook.com>
> ---
>  tools/perf/arch/riscv/util/header.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/tools/perf/arch/riscv/util/header.c
> b/tools/perf/arch/riscv/util/header.c
> index 4a41856938a8..031899c627f6 100644
> --- a/tools/perf/arch/riscv/util/header.c
> +++ b/tools/perf/arch/riscv/util/header.c
> @@ -55,18 +55,13 @@ static char *_get_cpuid(void)
>                                 goto free;
>                 } else if (!strncmp(line, CPUINFO_MARCH,
> strlen(CPUINFO_MARCH))) {
>                         marchid = _get_field(line);
> -                       if (!marchid)
> -                               goto free;
>                 } else if (!strncmp(line, CPUINFO_MIMP,
> strlen(CPUINFO_MIMP))) {
>                         mimpid = _get_field(line);
> -                       if (!mimpid)
> -                               goto free;
> -
>                         break;
>                 }
>         }

What does /proc/cpuinfo shows on c9xx ? Why can't we use zeroes ?

>  
> -       if (!mvendorid || !marchid || !mimpid)
> +       if (!mvendorid)
>                 goto free;
>  
>         if (asprintf(&cpuid, "%s-%s-%s", mvendorid, marchid, mimpid)
> < 0)
Nikita Shubin May 16, 2023, 2:15 p.m. UTC | #3 
On Tue, 2023-05-16 at 17:43 +0800, Inochi Amaoto wrote:
> > > The T-HEAD C9xx series CPU only has MVENDOR defined, and left
> > > MARCH
> > > and MIMP unimplemented.
> > 
> > According to the docs you can still read them, but it's hardwired
> > to
> > 64h0.
> > 
> > How it's supposed to distinguish c906 and c910 for example ?
> 
> It is unnecessary to distinguish c9xx, their event index is
> compatible.
> The dtb and opensbi will final decide which event can be used.
> 
> > What does /proc/cpuinfo shows on c9xx ? Why can't we use zeroes ?
> 
> The content is as follows.
> 
> processor     : 0
> hart          : 0
> isa           : rv64imafdc
> mmu           : sv39
> uarch         : thead,c910
> mvendorid     : 0x5b7
> marchid       : 0x0
> mimpid        : 0x0

Then why do you need first patch then ?

marchid, mimpid will never be NULL they "0x0" and "0x0" strings
respectively.

How have you tested it ? 

There no way "0x5b7-0x0000000000000000-0x[[:xdigit:]]+" will match
"0x5b7-0x0-0x0" which cpuid in your case.

Just drop this patch.

Anyway "PAGER=cat perf list pmu" gives me an empty list on licheerv.

> 
> The `mvendorid`, `marchid`, `mimpid` are the same across allwinner D1
> (C906),
> T-HEAD th1520 (C910) and the sophgo mango (C920). It seems T-HEAD use
> MCPUID
> CSR to store CPU info. But this is not standard and not shown in
> cpuinfo.
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
Inochi Amaoto May 17, 2023, 5:06 a.m. UTC | #4 
> Then why do you need first patch then ?
>
> marchid, mimpid will never be NULL they "0x0" and "0x0" strings
> respectively.
>
> How have you tested it ?
>
> There no way "0x5b7-0x0000000000000000-0x[[:xdigit:]]+" will match
> "0x5b7-0x0-0x0" which cpuid in your case.
>
> Just drop this patch.
>
> Anyway "PAGER=cat perf list pmu" gives me an empty list on licheerv.

Sorry for this mistake, I mistook the type of the MIMP and MARCH as
unsigned long. And I write wrong MARCH id in my test container.

Anyway, I agree to drop this patch as there is no need.
diff mbox series

Patch

diff --git a/tools/perf/arch/riscv/util/header.c b/tools/perf/arch/riscv/util/header.c
index 4a41856938a8..031899c627f6 100644
--- a/tools/perf/arch/riscv/util/header.c
+++ b/tools/perf/arch/riscv/util/header.c
@@ -55,18 +55,13 @@  static char *_get_cpuid(void)
 				goto free;
 		} else if (!strncmp(line, CPUINFO_MARCH, strlen(CPUINFO_MARCH))) {
 			marchid = _get_field(line);
-			if (!marchid)
-				goto free;
 		} else if (!strncmp(line, CPUINFO_MIMP, strlen(CPUINFO_MIMP))) {
 			mimpid = _get_field(line);
-			if (!mimpid)
-				goto free;
-
 			break;
 		}
 	}
 
-	if (!mvendorid || !marchid || !mimpid)
+	if (!mvendorid)
 		goto free;
 
 	if (asprintf(&cpuid, "%s-%s-%s", mvendorid, marchid, mimpid) < 0)