[PATCH 0/3] arm64: cpuinfo: make /proc/cpuinfo more human-readable

[PATCH 0/3] arm64: cpuinfo: make /proc/cpuinfo more human-readable

[Al Stone]

As ARMv8 servers get deployed, I keep getting the same set of questions
from end-users of those systems: what do all the hex numbers mean in 
/proc/cpuinfo and could you make them so I don't have to carry a cheat
sheet with me all the time?

These patches respond to those questions.  For good or ill, some of the
automation used to manage systems in data centers (as well as many of
the humans involved) need to have text; this helps them simply slide 
into place and become usable quickly.

Patch 1/2 provides the MPIDR as basic topology info in /proc/cpuinfo
when using ACPI, perhaps until such time as the more robust ACPI
implementation is available [0]; this is helpful in automating the
selection of multi-CPU systems when many choices are available (for
example, in automated testing systems).  While it is yet another hex
value, it does provide some topology information without interfering
with what [0] will ultimately provide, and is helpful in sorting out
ACPI table issues that use the MPIDR for identifying CPUs.

Patches 2/3 and 3/3 are similar in that they provide a more human-
readable version of the info already available; this allows admin
tools to provide proper strings to display in inventory systems, for
example, or when a human is using a CI system and needs to be provided
a list of possible systems to test on.

In all of the patches, I have avoided replacing or interfering with
any existing output so as not to affect systems already in use.

Tested on AMD Seattle, APM Mustang and Cavium ThunderX systems.


[0] https://marc.info/?l=linux-pm&m=150584702021552&w=2


Al Stone (3):
  arm64: cpuinfo: add MPIDR value to /proc/cpuinfo
  arm64: cpuinfo: add human readable CPU names to /proc/cpuinfo
  arm64: cpuinfo: display product info in /proc/cpuinfo

 arch/arm64/include/asm/cpu.h |   1 +
 arch/arm64/kernel/cpuinfo.c  | 225 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 226 insertions(+)

-- 
2.13.5

[PATCH 0/3] arm64: cpuinfo: make /proc/cpuinfo more human-readable

[Al Stone]

As ARMv8 servers get deployed, I keep getting the same set of questions
from end-users of those systems: what do all the hex numbers mean in 
/proc/cpuinfo and could you make them so I don't have to carry a cheat
sheet with me all the time?

These patches respond to those questions.  For good or ill, some of the
automation used to manage systems in data centers (as well as many of
the humans involved) need to have text; this helps them simply slide 
into place and become usable quickly.

Patch 1/2 provides the MPIDR as basic topology info in /proc/cpuinfo
when using ACPI, perhaps until such time as the more robust ACPI
implementation is available [0]; this is helpful in automating the
selection of multi-CPU systems when many choices are available (for
example, in automated testing systems).  While it is yet another hex
value, it does provide some topology information without interfering
with what [0] will ultimately provide, and is helpful in sorting out
ACPI table issues that use the MPIDR for identifying CPUs.

Patches 2/3 and 3/3 are similar in that they provide a more human-
readable version of the info already available; this allows admin
tools to provide proper strings to display in inventory systems, for
example, or when a human is using a CI system and needs to be provided
a list of possible systems to test on.

In all of the patches, I have avoided replacing or interfering with
any existing output so as not to affect systems already in use.

Tested on AMD Seattle, APM Mustang and Cavium ThunderX systems.


[0] https://marc.info/?l=linux-pm&m=150584702021552&w=2


Al Stone (3):
  arm64: cpuinfo: add MPIDR value to /proc/cpuinfo
  arm64: cpuinfo: add human readable CPU names to /proc/cpuinfo
  arm64: cpuinfo: display product info in /proc/cpuinfo

 arch/arm64/include/asm/cpu.h |   1 +
 arch/arm64/kernel/cpuinfo.c  | 225 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 226 insertions(+)

-- 
2.13.5

[PATCH 1/3] arm64: cpuinfo: add MPIDR value to /proc/cpuinfo

[Al Stone]

When displaying cpuinfo on an arm64 system, include the MPIDR register
value which can be used to understand the CPU topology on a platform.
This can serve as a cross-check to the information provided in sysfs,
and has been useful in understanding CPU and NUMA allocation issues.

Signed-off-by: Al Stone <ahs3@redhat.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
---
 arch/arm64/include/asm/cpu.h | 1 +
 arch/arm64/kernel/cpuinfo.c  | 8 ++++++++
 2 files changed, 9 insertions(+)

diff --git a/arch/arm64/include/asm/cpu.h b/arch/arm64/include/asm/cpu.h
index 889226b4c6e1..ac40894df247 100644
--- a/arch/arm64/include/asm/cpu.h
+++ b/arch/arm64/include/asm/cpu.h
@@ -32,6 +32,7 @@ struct cpuinfo_arm64 {
 	u32		reg_midr;
 	u32		reg_revidr;
 
+	u64		reg_id_aa64mpidr;
 	u64		reg_id_aa64dfr0;
 	u64		reg_id_aa64dfr1;
 	u64		reg_id_aa64isar0;
diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c
index 4a6f875ac854..e505007138eb 100644
--- a/arch/arm64/kernel/cpuinfo.c
+++ b/arch/arm64/kernel/cpuinfo.c
@@ -120,6 +120,7 @@ static int c_show(struct seq_file *m, void *v)
 	for_each_online_cpu(i) {
 		struct cpuinfo_arm64 *cpuinfo = &per_cpu(cpu_data, i);
 		u32 midr = cpuinfo->reg_midr;
+		u64 mpidr = cpuinfo->reg_id_aa64mpidr;
 
 		/*
 		 * glibc reads /proc/cpuinfo to determine the number of
@@ -159,6 +160,12 @@ static int c_show(struct seq_file *m, void *v)
 		}
 		seq_puts(m, "\n");
 
+		seq_printf(m, "CPU MPIDR\t: 0x%016llx ", mpidr);
+		seq_printf(m, "(Aff3 %d Aff2 %d Aff1 %d Aff0 %d)\n",
+			   (u8) MPIDR_AFFINITY_LEVEL(mpidr, 3),
+			   (u8) MPIDR_AFFINITY_LEVEL(mpidr, 2),
+			   (u8) MPIDR_AFFINITY_LEVEL(mpidr, 1),
+			   (u8) MPIDR_AFFINITY_LEVEL(mpidr, 0));
 		seq_printf(m, "CPU implementer\t: 0x%02x\n",
 			   MIDR_IMPLEMENTOR(midr));
 		seq_printf(m, "CPU architecture: 8\n");
@@ -372,6 +379,7 @@ static void __cpuinfo_store_cpu(struct cpuinfo_arm64 *info)
 	info->reg_midr = read_cpuid_id();
 	info->reg_revidr = read_cpuid(REVIDR_EL1);
 
+	info->reg_id_aa64mpidr = read_cpuid_mpidr();
 	info->reg_id_aa64dfr0 = read_cpuid(ID_AA64DFR0_EL1);
 	info->reg_id_aa64dfr1 = read_cpuid(ID_AA64DFR1_EL1);
 	info->reg_id_aa64isar0 = read_cpuid(ID_AA64ISAR0_EL1);
-- 
2.13.5

[PATCH 1/3] arm64: cpuinfo: add MPIDR value to /proc/cpuinfo

[Al Stone]

When displaying cpuinfo on an arm64 system, include the MPIDR register
value which can be used to understand the CPU topology on a platform.
This can serve as a cross-check to the information provided in sysfs,
and has been useful in understanding CPU and NUMA allocation issues.

Signed-off-by: Al Stone <ahs3@redhat.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
---
 arch/arm64/include/asm/cpu.h | 1 +
 arch/arm64/kernel/cpuinfo.c  | 8 ++++++++
 2 files changed, 9 insertions(+)

diff --git a/arch/arm64/include/asm/cpu.h b/arch/arm64/include/asm/cpu.h
index 889226b4c6e1..ac40894df247 100644
--- a/arch/arm64/include/asm/cpu.h
+++ b/arch/arm64/include/asm/cpu.h
@@ -32,6 +32,7 @@ struct cpuinfo_arm64 {
 	u32		reg_midr;
 	u32		reg_revidr;
 
+	u64		reg_id_aa64mpidr;
 	u64		reg_id_aa64dfr0;
 	u64		reg_id_aa64dfr1;
 	u64		reg_id_aa64isar0;
diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c
index 4a6f875ac854..e505007138eb 100644
--- a/arch/arm64/kernel/cpuinfo.c
+++ b/arch/arm64/kernel/cpuinfo.c
@@ -120,6 +120,7 @@ static int c_show(struct seq_file *m, void *v)
 	for_each_online_cpu(i) {
 		struct cpuinfo_arm64 *cpuinfo = &per_cpu(cpu_data, i);
 		u32 midr = cpuinfo->reg_midr;
+		u64 mpidr = cpuinfo->reg_id_aa64mpidr;
 
 		/*
 		 * glibc reads /proc/cpuinfo to determine the number of
@@ -159,6 +160,12 @@ static int c_show(struct seq_file *m, void *v)
 		}
 		seq_puts(m, "\n");
 
+		seq_printf(m, "CPU MPIDR\t: 0x%016llx ", mpidr);
+		seq_printf(m, "(Aff3 %d Aff2 %d Aff1 %d Aff0 %d)\n",
+			   (u8) MPIDR_AFFINITY_LEVEL(mpidr, 3),
+			   (u8) MPIDR_AFFINITY_LEVEL(mpidr, 2),
+			   (u8) MPIDR_AFFINITY_LEVEL(mpidr, 1),
+			   (u8) MPIDR_AFFINITY_LEVEL(mpidr, 0));
 		seq_printf(m, "CPU implementer\t: 0x%02x\n",
 			   MIDR_IMPLEMENTOR(midr));
 		seq_printf(m, "CPU architecture: 8\n");
@@ -372,6 +379,7 @@ static void __cpuinfo_store_cpu(struct cpuinfo_arm64 *info)
 	info->reg_midr = read_cpuid_id();
 	info->reg_revidr = read_cpuid(REVIDR_EL1);
 
+	info->reg_id_aa64mpidr = read_cpuid_mpidr();
 	info->reg_id_aa64dfr0 = read_cpuid(ID_AA64DFR0_EL1);
 	info->reg_id_aa64dfr1 = read_cpuid(ID_AA64DFR1_EL1);
 	info->reg_id_aa64isar0 = read_cpuid(ID_AA64ISAR0_EL1);
-- 
2.13.5

Re: [PATCH 1/3] arm64: cpuinfo: add MPIDR value to /proc/cpuinfo

[Mark Rutland]

On Tue, Sep 26, 2017 at 04:23:22PM -0600, Al Stone wrote:
> When displaying cpuinfo on an arm64 system, include the MPIDR register
> value which can be used to understand the CPU topology on a platform.
> This can serve as a cross-check to the information provided in sysfs,
> and has been useful in understanding CPU and NUMA allocation issues.

As mentioend in the cover letter, NAK to modifiying /proc/cpuinfo.

However, I do think that we could expose this elsewhere in a structured
way.

For debugging bringup issues, I think we can update our secondary
bringup messages in dmesg to log the MPIDR (as we do for the boot CPU).
I'll send a patch for that.

> @@ -159,6 +160,12 @@ static int c_show(struct seq_file *m, void *v)
>  		}
>  		seq_puts(m, "\n");
>  
> +		seq_printf(m, "CPU MPIDR\t: 0x%016llx ", mpidr);
> +		seq_printf(m, "(Aff3 %d Aff2 %d Aff1 %d Aff0 %d)\n",
> +			   (u8) MPIDR_AFFINITY_LEVEL(mpidr, 3),
> +			   (u8) MPIDR_AFFINITY_LEVEL(mpidr, 2),
> +			   (u8) MPIDR_AFFINITY_LEVEL(mpidr, 1),
> +			   (u8) MPIDR_AFFINITY_LEVEL(mpidr, 0));

Please don't decode the register like this. We're stuck doing it with
the MIDR for historical reasons, but we shouldn't do it for new
registers.

It's possible (and I suspect likely) that MPIDR will gain more fields in
future, and it creates futher ABI problems (e.g. adding them might break
applications).

If we're going to expose this, we should expose the raw value under
sysfs. Users who require this information will know how to decode it.

Thanks,
Mark.

[PATCH 1/3] arm64: cpuinfo: add MPIDR value to /proc/cpuinfo

[Mark Rutland]

On Tue, Sep 26, 2017 at 04:23:22PM -0600, Al Stone wrote:
> When displaying cpuinfo on an arm64 system, include the MPIDR register
> value which can be used to understand the CPU topology on a platform.
> This can serve as a cross-check to the information provided in sysfs,
> and has been useful in understanding CPU and NUMA allocation issues.

As mentioend in the cover letter, NAK to modifiying /proc/cpuinfo.

However, I do think that we could expose this elsewhere in a structured
way.

For debugging bringup issues, I think we can update our secondary
bringup messages in dmesg to log the MPIDR (as we do for the boot CPU).
I'll send a patch for that.

> @@ -159,6 +160,12 @@ static int c_show(struct seq_file *m, void *v)
>  		}
>  		seq_puts(m, "\n");
>  
> +		seq_printf(m, "CPU MPIDR\t: 0x%016llx ", mpidr);
> +		seq_printf(m, "(Aff3 %d Aff2 %d Aff1 %d Aff0 %d)\n",
> +			   (u8) MPIDR_AFFINITY_LEVEL(mpidr, 3),
> +			   (u8) MPIDR_AFFINITY_LEVEL(mpidr, 2),
> +			   (u8) MPIDR_AFFINITY_LEVEL(mpidr, 1),
> +			   (u8) MPIDR_AFFINITY_LEVEL(mpidr, 0));

Please don't decode the register like this. We're stuck doing it with
the MIDR for historical reasons, but we shouldn't do it for new
registers.

It's possible (and I suspect likely) that MPIDR will gain more fields in
future, and it creates futher ABI problems (e.g. adding them might break
applications).

If we're going to expose this, we should expose the raw value under
sysfs. Users who require this information will know how to decode it.

Thanks,
Mark.

[PATCH 2/3] arm64: cpuinfo: add human readable CPU names to /proc/cpuinfo

[Al Stone]

In the interest of making things easier for humans to use, add a
"CPU name" line to /proc/cpuinfo for each CPU that uses plain old
words instead of hex values.  For example, instead of printing only
CPU implementer 0x43 and CPU part 0x0A1, print also "CPU name :
Cavium ThunderX".

Note that this is not meant to be an exhaustive list of all possible
implementers or CPUs (I'm not even sure that is knowable); this patch
is intentionally limited to only those willing to provide info in
arch/arm64/include/asm/cputype.h

Signed-off-by: Al Stone <ahs3@redhat.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
---
 arch/arm64/kernel/cpuinfo.c | 84 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 84 insertions(+)

diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c
index e505007138eb..0b4261884862 100644
--- a/arch/arm64/kernel/cpuinfo.c
+++ b/arch/arm64/kernel/cpuinfo.c
@@ -75,6 +75,61 @@ static const char *const hwcap_str[] = {
 	NULL
 };
 
+struct hw_part {
+	u16	id;
+	char	*name;
+};
+
+static const struct hw_part arm_hw_part[] = {
+	{ ARM_CPU_PART_AEM_V8,		"AEMv8 Model" },
+	{ ARM_CPU_PART_FOUNDATION,	"Foundation Model" },
+	{ ARM_CPU_PART_CORTEX_A57,	"Cortex A57" },
+	{ ARM_CPU_PART_CORTEX_A53,	"Cortex A53" },
+	{ ARM_CPU_PART_CORTEX_A73,	"Cortex A73" },
+	{ (-1), "unknown" }		/* Potenza == 0, unfortunately */
+};
+
+static const struct hw_part apm_hw_part[] = {
+	{ APM_CPU_PART_POTENZA,		"Potenza" },
+	{ (-1), "unknown" }		/* Potenza == 0, unfortunately */
+};
+
+static const struct hw_part brcm_hw_part[] = {
+	{ BRCM_CPU_PART_VULCAN,		"Vulcan" },
+	{ (-1), "unknown" }		/* Potenza == 0, unfortunately */
+};
+
+static const struct hw_part cavium_hw_part[] = {
+	{ CAVIUM_CPU_PART_THUNDERX,	 "ThunderX" },
+	{ CAVIUM_CPU_PART_THUNDERX_81XX, "ThunderX 81XX" },
+	{ CAVIUM_CPU_PART_THUNDERX_83XX, "ThunderX 83XX" },
+	{ (-1), "unknown" }		/* Potenza == 0, unfortunately */
+};
+
+static const struct hw_part qcom_hw_part[] = {
+	{ QCOM_CPU_PART_FALKOR_V1,	"Falkor v1" },
+	{ (-1), "unknown" }		/* Potenza == 0, unfortunately */
+};
+
+static const struct hw_part unknown_hw_part[] = {
+	{ (-1), "unknown" }		/* Potenza == 0, unfortunately */
+};
+
+struct hw_impl {
+	u8			id;
+	const struct hw_part	*parts;
+	char			*name;
+};
+
+static const struct hw_impl hw_implementer[] = {
+	{ ARM_CPU_IMP_ARM,	arm_hw_part,	"ARM Ltd." },
+	{ ARM_CPU_IMP_APM,	apm_hw_part,	"Applied Micro" },
+	{ ARM_CPU_IMP_CAVIUM,	cavium_hw_part,	"Cavium" },
+	{ ARM_CPU_IMP_BRCM,	brcm_hw_part,	"Broadcom" },
+	{ ARM_CPU_IMP_QCOM,	qcom_hw_part,	"Qualcomm" },
+	{ 0, unknown_hw_part, "unknown" }
+};
+
 #ifdef CONFIG_COMPAT
 static const char *const compat_hwcap_str[] = {
 	"swp",
@@ -116,6 +171,9 @@ static int c_show(struct seq_file *m, void *v)
 {
 	int i, j;
 	bool compat = personality(current->personality) == PER_LINUX32;
+	u8 impl;
+	u16 part;
+	const struct hw_part *parts;
 
 	for_each_online_cpu(i) {
 		struct cpuinfo_arm64 *cpuinfo = &per_cpu(cpu_data, i);
@@ -132,6 +190,32 @@ static int c_show(struct seq_file *m, void *v)
 			seq_printf(m, "model name\t: ARMv8 Processor rev %d (%s)\n",
 				   MIDR_REVISION(midr), COMPAT_ELF_PLATFORM);
 
+		impl = (u8) MIDR_IMPLEMENTOR(midr);
+		for (j = 0; hw_implementer[j].id != 0; j++) {
+			if (hw_implementer[j].id == impl) {
+				seq_printf(m, "CPU name\t: %s ",
+					   hw_implementer[j].name);
+				parts = hw_implementer[j].parts;
+				break;
+			}
+		}
+		if (hw_implementer[j].id == 0) {
+			seq_printf(m, "CPU name\t: %s ",
+				   hw_implementer[j].name);
+			parts = hw_implementer[j].parts;
+		}
+
+		part = (u16) MIDR_PARTNUM(midr);
+		for (j = 0; parts[j].id != (-1); j++) {
+			if (parts[j].id == part) {
+				seq_printf(m, "%s\n", parts[j].name);
+				break;
+			}
+		}
+		if (parts[j].id == (-1))
+			seq_printf(m, "%s", parts[j].name);
+		seq_puts(m, "\n");
+
 		seq_printf(m, "BogoMIPS\t: %lu.%02lu\n",
 			   loops_per_jiffy / (500000UL/HZ),
 			   loops_per_jiffy / (5000UL/HZ) % 100);
-- 
2.13.5

[PATCH 2/3] arm64: cpuinfo: add human readable CPU names to /proc/cpuinfo

[Al Stone]

In the interest of making things easier for humans to use, add a
"CPU name" line to /proc/cpuinfo for each CPU that uses plain old
words instead of hex values.  For example, instead of printing only
CPU implementer 0x43 and CPU part 0x0A1, print also "CPU name :
Cavium ThunderX".

Note that this is not meant to be an exhaustive list of all possible
implementers or CPUs (I'm not even sure that is knowable); this patch
is intentionally limited to only those willing to provide info in
arch/arm64/include/asm/cputype.h

Signed-off-by: Al Stone <ahs3@redhat.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
---
 arch/arm64/kernel/cpuinfo.c | 84 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 84 insertions(+)

diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c
index e505007138eb..0b4261884862 100644
--- a/arch/arm64/kernel/cpuinfo.c
+++ b/arch/arm64/kernel/cpuinfo.c
@@ -75,6 +75,61 @@ static const char *const hwcap_str[] = {
 	NULL
 };
 
+struct hw_part {
+	u16	id;
+	char	*name;
+};
+
+static const struct hw_part arm_hw_part[] = {
+	{ ARM_CPU_PART_AEM_V8,		"AEMv8 Model" },
+	{ ARM_CPU_PART_FOUNDATION,	"Foundation Model" },
+	{ ARM_CPU_PART_CORTEX_A57,	"Cortex A57" },
+	{ ARM_CPU_PART_CORTEX_A53,	"Cortex A53" },
+	{ ARM_CPU_PART_CORTEX_A73,	"Cortex A73" },
+	{ (-1), "unknown" }		/* Potenza == 0, unfortunately */
+};
+
+static const struct hw_part apm_hw_part[] = {
+	{ APM_CPU_PART_POTENZA,		"Potenza" },
+	{ (-1), "unknown" }		/* Potenza == 0, unfortunately */
+};
+
+static const struct hw_part brcm_hw_part[] = {
+	{ BRCM_CPU_PART_VULCAN,		"Vulcan" },
+	{ (-1), "unknown" }		/* Potenza == 0, unfortunately */
+};
+
+static const struct hw_part cavium_hw_part[] = {
+	{ CAVIUM_CPU_PART_THUNDERX,	 "ThunderX" },
+	{ CAVIUM_CPU_PART_THUNDERX_81XX, "ThunderX 81XX" },
+	{ CAVIUM_CPU_PART_THUNDERX_83XX, "ThunderX 83XX" },
+	{ (-1), "unknown" }		/* Potenza == 0, unfortunately */
+};
+
+static const struct hw_part qcom_hw_part[] = {
+	{ QCOM_CPU_PART_FALKOR_V1,	"Falkor v1" },
+	{ (-1), "unknown" }		/* Potenza == 0, unfortunately */
+};
+
+static const struct hw_part unknown_hw_part[] = {
+	{ (-1), "unknown" }		/* Potenza == 0, unfortunately */
+};
+
+struct hw_impl {
+	u8			id;
+	const struct hw_part	*parts;
+	char			*name;
+};
+
+static const struct hw_impl hw_implementer[] = {
+	{ ARM_CPU_IMP_ARM,	arm_hw_part,	"ARM Ltd." },
+	{ ARM_CPU_IMP_APM,	apm_hw_part,	"Applied Micro" },
+	{ ARM_CPU_IMP_CAVIUM,	cavium_hw_part,	"Cavium" },
+	{ ARM_CPU_IMP_BRCM,	brcm_hw_part,	"Broadcom" },
+	{ ARM_CPU_IMP_QCOM,	qcom_hw_part,	"Qualcomm" },
+	{ 0, unknown_hw_part, "unknown" }
+};
+
 #ifdef CONFIG_COMPAT
 static const char *const compat_hwcap_str[] = {
 	"swp",
@@ -116,6 +171,9 @@ static int c_show(struct seq_file *m, void *v)
 {
 	int i, j;
 	bool compat = personality(current->personality) == PER_LINUX32;
+	u8 impl;
+	u16 part;
+	const struct hw_part *parts;
 
 	for_each_online_cpu(i) {
 		struct cpuinfo_arm64 *cpuinfo = &per_cpu(cpu_data, i);
@@ -132,6 +190,32 @@ static int c_show(struct seq_file *m, void *v)
 			seq_printf(m, "model name\t: ARMv8 Processor rev %d (%s)\n",
 				   MIDR_REVISION(midr), COMPAT_ELF_PLATFORM);
 
+		impl = (u8) MIDR_IMPLEMENTOR(midr);
+		for (j = 0; hw_implementer[j].id != 0; j++) {
+			if (hw_implementer[j].id == impl) {
+				seq_printf(m, "CPU name\t: %s ",
+					   hw_implementer[j].name);
+				parts = hw_implementer[j].parts;
+				break;
+			}
+		}
+		if (hw_implementer[j].id == 0) {
+			seq_printf(m, "CPU name\t: %s ",
+				   hw_implementer[j].name);
+			parts = hw_implementer[j].parts;
+		}
+
+		part = (u16) MIDR_PARTNUM(midr);
+		for (j = 0; parts[j].id != (-1); j++) {
+			if (parts[j].id == part) {
+				seq_printf(m, "%s\n", parts[j].name);
+				break;
+			}
+		}
+		if (parts[j].id == (-1))
+			seq_printf(m, "%s", parts[j].name);
+		seq_puts(m, "\n");
+
 		seq_printf(m, "BogoMIPS\t: %lu.%02lu\n",
 			   loops_per_jiffy / (500000UL/HZ),
 			   loops_per_jiffy / (5000UL/HZ) % 100);
-- 
2.13.5

Re: [PATCH 2/3] arm64: cpuinfo: add human readable CPU names to /proc/cpuinfo

[Robin Murphy]

On 26/09/17 23:23, Al Stone wrote:
> In the interest of making things easier for humans to use, add a
> "CPU name" line to /proc/cpuinfo for each CPU that uses plain old
> words instead of hex values.  For example, instead of printing only
> CPU implementer 0x43 and CPU part 0x0A1, print also "CPU name :
> Cavium ThunderX".
> 
> Note that this is not meant to be an exhaustive list of all possible
> implementers or CPUs (I'm not even sure that is knowable); this patch
> is intentionally limited to only those willing to provide info in
> arch/arm64/include/asm/cputype.h

How valuable is an incomplete interface really? If users who want to
decode MIDRs are going to have to rely on (pretty trivial ) userspace
tools anyway when their stable distro kernel doesn't know their spangly
new hardware, why does the kernel need to bother at all.

The fact is that we already do the exact same thing as x86 - we print
exactly what the ID registers say. The fact that on x86 some of those
values happen to form a readable ASCII string is a different matter.

> Signed-off-by: Al Stone <ahs3@redhat.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> ---
>  arch/arm64/kernel/cpuinfo.c | 84 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 84 insertions(+)
> 
> diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c
> index e505007138eb..0b4261884862 100644
> --- a/arch/arm64/kernel/cpuinfo.c
> +++ b/arch/arm64/kernel/cpuinfo.c
> @@ -75,6 +75,61 @@ static const char *const hwcap_str[] = {
>  	NULL
>  };
>  
> +struct hw_part {
> +	u16	id;
> +	char	*name;
> +};
> +
> +static const struct hw_part arm_hw_part[] = {
> +	{ ARM_CPU_PART_AEM_V8,		"AEMv8 Model" },
> +	{ ARM_CPU_PART_FOUNDATION,	"Foundation Model" },
> +	{ ARM_CPU_PART_CORTEX_A57,	"Cortex A57" },
> +	{ ARM_CPU_PART_CORTEX_A53,	"Cortex A53" },
> +	{ ARM_CPU_PART_CORTEX_A73,	"Cortex A73" },
> +	{ (-1), "unknown" }		/* Potenza == 0, unfortunately */

So for a fair chunk of *current* server-class hardware, we'll be
printing "unknown" already. Great.

Robin.

> +};
> +
> +static const struct hw_part apm_hw_part[] = {
> +	{ APM_CPU_PART_POTENZA,		"Potenza" },
> +	{ (-1), "unknown" }		/* Potenza == 0, unfortunately */
> +};
> +
> +static const struct hw_part brcm_hw_part[] = {
> +	{ BRCM_CPU_PART_VULCAN,		"Vulcan" },
> +	{ (-1), "unknown" }		/* Potenza == 0, unfortunately */
> +};
> +
> +static const struct hw_part cavium_hw_part[] = {
> +	{ CAVIUM_CPU_PART_THUNDERX,	 "ThunderX" },
> +	{ CAVIUM_CPU_PART_THUNDERX_81XX, "ThunderX 81XX" },
> +	{ CAVIUM_CPU_PART_THUNDERX_83XX, "ThunderX 83XX" },
> +	{ (-1), "unknown" }		/* Potenza == 0, unfortunately */
> +};
> +
> +static const struct hw_part qcom_hw_part[] = {
> +	{ QCOM_CPU_PART_FALKOR_V1,	"Falkor v1" },
> +	{ (-1), "unknown" }		/* Potenza == 0, unfortunately */
> +};
> +
> +static const struct hw_part unknown_hw_part[] = {
> +	{ (-1), "unknown" }		/* Potenza == 0, unfortunately */
> +};
> +
> +struct hw_impl {
> +	u8			id;
> +	const struct hw_part	*parts;
> +	char			*name;
> +};
> +
> +static const struct hw_impl hw_implementer[] = {
> +	{ ARM_CPU_IMP_ARM,	arm_hw_part,	"ARM Ltd." },
> +	{ ARM_CPU_IMP_APM,	apm_hw_part,	"Applied Micro" },
> +	{ ARM_CPU_IMP_CAVIUM,	cavium_hw_part,	"Cavium" },
> +	{ ARM_CPU_IMP_BRCM,	brcm_hw_part,	"Broadcom" },
> +	{ ARM_CPU_IMP_QCOM,	qcom_hw_part,	"Qualcomm" },
> +	{ 0, unknown_hw_part, "unknown" }
> +};
> +
>  #ifdef CONFIG_COMPAT
>  static const char *const compat_hwcap_str[] = {
>  	"swp",
> @@ -116,6 +171,9 @@ static int c_show(struct seq_file *m, void *v)
>  {
>  	int i, j;
>  	bool compat = personality(current->personality) == PER_LINUX32;
> +	u8 impl;
> +	u16 part;
> +	const struct hw_part *parts;
>  
>  	for_each_online_cpu(i) {
>  		struct cpuinfo_arm64 *cpuinfo = &per_cpu(cpu_data, i);
> @@ -132,6 +190,32 @@ static int c_show(struct seq_file *m, void *v)
>  			seq_printf(m, "model name\t: ARMv8 Processor rev %d (%s)\n",
>  				   MIDR_REVISION(midr), COMPAT_ELF_PLATFORM);
>  
> +		impl = (u8) MIDR_IMPLEMENTOR(midr);
> +		for (j = 0; hw_implementer[j].id != 0; j++) {
> +			if (hw_implementer[j].id == impl) {
> +				seq_printf(m, "CPU name\t: %s ",
> +					   hw_implementer[j].name);
> +				parts = hw_implementer[j].parts;
> +				break;
> +			}
> +		}
> +		if (hw_implementer[j].id == 0) {
> +			seq_printf(m, "CPU name\t: %s ",
> +				   hw_implementer[j].name);
> +			parts = hw_implementer[j].parts;
> +		}
> +
> +		part = (u16) MIDR_PARTNUM(midr);
> +		for (j = 0; parts[j].id != (-1); j++) {
> +			if (parts[j].id == part) {
> +				seq_printf(m, "%s\n", parts[j].name);
> +				break;
> +			}
> +		}
> +		if (parts[j].id == (-1))
> +			seq_printf(m, "%s", parts[j].name);
> +		seq_puts(m, "\n");
> +
>  		seq_printf(m, "BogoMIPS\t: %lu.%02lu\n",
>  			   loops_per_jiffy / (500000UL/HZ),
>  			   loops_per_jiffy / (5000UL/HZ) % 100);
> 

[PATCH 2/3] arm64: cpuinfo: add human readable CPU names to /proc/cpuinfo

[Robin Murphy]

On 26/09/17 23:23, Al Stone wrote:
> In the interest of making things easier for humans to use, add a
> "CPU name" line to /proc/cpuinfo for each CPU that uses plain old
> words instead of hex values.  For example, instead of printing only
> CPU implementer 0x43 and CPU part 0x0A1, print also "CPU name :
> Cavium ThunderX".
> 
> Note that this is not meant to be an exhaustive list of all possible
> implementers or CPUs (I'm not even sure that is knowable); this patch
> is intentionally limited to only those willing to provide info in
> arch/arm64/include/asm/cputype.h

How valuable is an incomplete interface really? If users who want to
decode MIDRs are going to have to rely on (pretty trivial ) userspace
tools anyway when their stable distro kernel doesn't know their spangly
new hardware, why does the kernel need to bother at all.

The fact is that we already do the exact same thing as x86 - we print
exactly what the ID registers say. The fact that on x86 some of those
values happen to form a readable ASCII string is a different matter.

> Signed-off-by: Al Stone <ahs3@redhat.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> ---
>  arch/arm64/kernel/cpuinfo.c | 84 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 84 insertions(+)
> 
> diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c
> index e505007138eb..0b4261884862 100644
> --- a/arch/arm64/kernel/cpuinfo.c
> +++ b/arch/arm64/kernel/cpuinfo.c
> @@ -75,6 +75,61 @@ static const char *const hwcap_str[] = {
>  	NULL
>  };
>  
> +struct hw_part {
> +	u16	id;
> +	char	*name;
> +};
> +
> +static const struct hw_part arm_hw_part[] = {
> +	{ ARM_CPU_PART_AEM_V8,		"AEMv8 Model" },
> +	{ ARM_CPU_PART_FOUNDATION,	"Foundation Model" },
> +	{ ARM_CPU_PART_CORTEX_A57,	"Cortex A57" },
> +	{ ARM_CPU_PART_CORTEX_A53,	"Cortex A53" },
> +	{ ARM_CPU_PART_CORTEX_A73,	"Cortex A73" },
> +	{ (-1), "unknown" }		/* Potenza == 0, unfortunately */

So for a fair chunk of *current* server-class hardware, we'll be
printing "unknown" already. Great.

Robin.

> +};
> +
> +static const struct hw_part apm_hw_part[] = {
> +	{ APM_CPU_PART_POTENZA,		"Potenza" },
> +	{ (-1), "unknown" }		/* Potenza == 0, unfortunately */
> +};
> +
> +static const struct hw_part brcm_hw_part[] = {
> +	{ BRCM_CPU_PART_VULCAN,		"Vulcan" },
> +	{ (-1), "unknown" }		/* Potenza == 0, unfortunately */
> +};
> +
> +static const struct hw_part cavium_hw_part[] = {
> +	{ CAVIUM_CPU_PART_THUNDERX,	 "ThunderX" },
> +	{ CAVIUM_CPU_PART_THUNDERX_81XX, "ThunderX 81XX" },
> +	{ CAVIUM_CPU_PART_THUNDERX_83XX, "ThunderX 83XX" },
> +	{ (-1), "unknown" }		/* Potenza == 0, unfortunately */
> +};
> +
> +static const struct hw_part qcom_hw_part[] = {
> +	{ QCOM_CPU_PART_FALKOR_V1,	"Falkor v1" },
> +	{ (-1), "unknown" }		/* Potenza == 0, unfortunately */
> +};
> +
> +static const struct hw_part unknown_hw_part[] = {
> +	{ (-1), "unknown" }		/* Potenza == 0, unfortunately */
> +};
> +
> +struct hw_impl {
> +	u8			id;
> +	const struct hw_part	*parts;
> +	char			*name;
> +};
> +
> +static const struct hw_impl hw_implementer[] = {
> +	{ ARM_CPU_IMP_ARM,	arm_hw_part,	"ARM Ltd." },
> +	{ ARM_CPU_IMP_APM,	apm_hw_part,	"Applied Micro" },
> +	{ ARM_CPU_IMP_CAVIUM,	cavium_hw_part,	"Cavium" },
> +	{ ARM_CPU_IMP_BRCM,	brcm_hw_part,	"Broadcom" },
> +	{ ARM_CPU_IMP_QCOM,	qcom_hw_part,	"Qualcomm" },
> +	{ 0, unknown_hw_part, "unknown" }
> +};
> +
>  #ifdef CONFIG_COMPAT
>  static const char *const compat_hwcap_str[] = {
>  	"swp",
> @@ -116,6 +171,9 @@ static int c_show(struct seq_file *m, void *v)
>  {
>  	int i, j;
>  	bool compat = personality(current->personality) == PER_LINUX32;
> +	u8 impl;
> +	u16 part;
> +	const struct hw_part *parts;
>  
>  	for_each_online_cpu(i) {
>  		struct cpuinfo_arm64 *cpuinfo = &per_cpu(cpu_data, i);
> @@ -132,6 +190,32 @@ static int c_show(struct seq_file *m, void *v)
>  			seq_printf(m, "model name\t: ARMv8 Processor rev %d (%s)\n",
>  				   MIDR_REVISION(midr), COMPAT_ELF_PLATFORM);
>  
> +		impl = (u8) MIDR_IMPLEMENTOR(midr);
> +		for (j = 0; hw_implementer[j].id != 0; j++) {
> +			if (hw_implementer[j].id == impl) {
> +				seq_printf(m, "CPU name\t: %s ",
> +					   hw_implementer[j].name);
> +				parts = hw_implementer[j].parts;
> +				break;
> +			}
> +		}
> +		if (hw_implementer[j].id == 0) {
> +			seq_printf(m, "CPU name\t: %s ",
> +				   hw_implementer[j].name);
> +			parts = hw_implementer[j].parts;
> +		}
> +
> +		part = (u16) MIDR_PARTNUM(midr);
> +		for (j = 0; parts[j].id != (-1); j++) {
> +			if (parts[j].id == part) {
> +				seq_printf(m, "%s\n", parts[j].name);
> +				break;
> +			}
> +		}
> +		if (parts[j].id == (-1))
> +			seq_printf(m, "%s", parts[j].name);
> +		seq_puts(m, "\n");
> +
>  		seq_printf(m, "BogoMIPS\t: %lu.%02lu\n",
>  			   loops_per_jiffy / (500000UL/HZ),
>  			   loops_per_jiffy / (5000UL/HZ) % 100);
> 

Re: [PATCH 2/3] arm64: cpuinfo: add human readable CPU names to /proc/cpuinfo

[Mark Rutland]

Hi Al,

On Tue, Sep 26, 2017 at 04:23:23PM -0600, Al Stone wrote:
> In the interest of making things easier for humans to use, add a
> "CPU name" line to /proc/cpuinfo for each CPU that uses plain old
> words instead of hex values.  For example, instead of printing only
> CPU implementer 0x43 and CPU part 0x0A1, print also "CPU name :
> Cavium ThunderX".
> 
> Note that this is not meant to be an exhaustive list of all possible
> implementers or CPUs (I'm not even sure that is knowable); this patch
> is intentionally limited to only those willing to provide info in
> arch/arm64/include/asm/cputype.h

As mentioned in the cover letter, and as with previous patches
attempting to do the same thing:

NAK to this patch.

In general, modifying /proc/cpuinfo comes with the risk of breaking
applications that (for better or worse) try to parse it mechanically.
Further, such modifications add to the problem by introducing yet more
ABI state applications may or may not end up depending on to function
(even if they don't require or use the information at all).

Specifically regarding mapping MIDRs to strings in the kernel, there are
a number of problems that make this undesirable, including (but not
limited to):

* As mentioned in the commit message, this cannot be complete, and is
  thus unreliable. We cannot know the set of CPUs that will exist in the
  future, and thus this practically mandates a kernel upgrade to use new
  CPUs, solely for a nicety.

* The names of CPUs can change, so there's not necessarily a single
  correct name to expose. As seen with Cortex-A12 and Cortex-A17 [1],
  where some users were upset if their CPU was considered to be a
  Cortex-A12 rather than a Cortex-A17.

  This embeds a political problem into kernel ABI.

* No matter how many times we tell them not to, applications *will* try
  to parse this. Any change (e.g. typo fixing or naming updates) will
  break some applications. An unexpectedly long CPU name may break
  parses using a small buffer.

  Worse, this could change over a kernel update, as a cpu goes from
  being:
  
    CPU name: unknown
  
  ... to being:

    CPU name: awesome super CPU 2000xx-super-mega-long-name-edition

  ... which could break applications, and some could argue that this is
  a kernel-side regression breaking userspace.

* This information can be derived from the MIDR and REVIDR. The decoded
  MIDR has always been exposed under /proc/cpuinfo, and both are exposed
  in a structured way under:

  /sys/devices/system/cpu/cpu${N}/regs/identification

  This duplicates information that way already expose, but in an
  unreliable fashion.

* This requires us to maintain an ever growing mapping of CPU IDs to
  strings.

* For similar reasons, this information is not exposed by 32-bit ARM,
  and further bifurcates our /proc/cpuinfo formats.

These problems cannot be solved kernel-side, and given this, I will
continue to NAK patches which attempt to decode the MIDR to human
readable strings.

Thanks,
Mark.

[1] https://community.arm.com/processors/b/blog/posts/arm-cortex-a17-cortex-a12-processor-update?CommentId=85c9dea4-1c9a-460c-a7b6-dcf59caab43d&pi10955=99

> 
> Signed-off-by: Al Stone <ahs3@redhat.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> ---
>  arch/arm64/kernel/cpuinfo.c | 84 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 84 insertions(+)
> 
> diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c
> index e505007138eb..0b4261884862 100644
> --- a/arch/arm64/kernel/cpuinfo.c
> +++ b/arch/arm64/kernel/cpuinfo.c
> @@ -75,6 +75,61 @@ static const char *const hwcap_str[] = {
>  	NULL
>  };
>  
> +struct hw_part {
> +	u16	id;
> +	char	*name;
> +};
> +
> +static const struct hw_part arm_hw_part[] = {
> +	{ ARM_CPU_PART_AEM_V8,		"AEMv8 Model" },
> +	{ ARM_CPU_PART_FOUNDATION,	"Foundation Model" },
> +	{ ARM_CPU_PART_CORTEX_A57,	"Cortex A57" },
> +	{ ARM_CPU_PART_CORTEX_A53,	"Cortex A53" },
> +	{ ARM_CPU_PART_CORTEX_A73,	"Cortex A73" },
> +	{ (-1), "unknown" }		/* Potenza == 0, unfortunately */
> +};
> +
> +static const struct hw_part apm_hw_part[] = {
> +	{ APM_CPU_PART_POTENZA,		"Potenza" },
> +	{ (-1), "unknown" }		/* Potenza == 0, unfortunately */
> +};
> +
> +static const struct hw_part brcm_hw_part[] = {
> +	{ BRCM_CPU_PART_VULCAN,		"Vulcan" },
> +	{ (-1), "unknown" }		/* Potenza == 0, unfortunately */
> +};
> +
> +static const struct hw_part cavium_hw_part[] = {
> +	{ CAVIUM_CPU_PART_THUNDERX,	 "ThunderX" },
> +	{ CAVIUM_CPU_PART_THUNDERX_81XX, "ThunderX 81XX" },
> +	{ CAVIUM_CPU_PART_THUNDERX_83XX, "ThunderX 83XX" },
> +	{ (-1), "unknown" }		/* Potenza == 0, unfortunately */
> +};
> +
> +static const struct hw_part qcom_hw_part[] = {
> +	{ QCOM_CPU_PART_FALKOR_V1,	"Falkor v1" },
> +	{ (-1), "unknown" }		/* Potenza == 0, unfortunately */
> +};
> +
> +static const struct hw_part unknown_hw_part[] = {
> +	{ (-1), "unknown" }		/* Potenza == 0, unfortunately */
> +};
> +
> +struct hw_impl {
> +	u8			id;
> +	const struct hw_part	*parts;
> +	char			*name;
> +};
> +
> +static const struct hw_impl hw_implementer[] = {
> +	{ ARM_CPU_IMP_ARM,	arm_hw_part,	"ARM Ltd." },
> +	{ ARM_CPU_IMP_APM,	apm_hw_part,	"Applied Micro" },
> +	{ ARM_CPU_IMP_CAVIUM,	cavium_hw_part,	"Cavium" },
> +	{ ARM_CPU_IMP_BRCM,	brcm_hw_part,	"Broadcom" },
> +	{ ARM_CPU_IMP_QCOM,	qcom_hw_part,	"Qualcomm" },
> +	{ 0, unknown_hw_part, "unknown" }
> +};
> +
>  #ifdef CONFIG_COMPAT
>  static const char *const compat_hwcap_str[] = {
>  	"swp",
> @@ -116,6 +171,9 @@ static int c_show(struct seq_file *m, void *v)
>  {
>  	int i, j;
>  	bool compat = personality(current->personality) == PER_LINUX32;
> +	u8 impl;
> +	u16 part;
> +	const struct hw_part *parts;
>  
>  	for_each_online_cpu(i) {
>  		struct cpuinfo_arm64 *cpuinfo = &per_cpu(cpu_data, i);
> @@ -132,6 +190,32 @@ static int c_show(struct seq_file *m, void *v)
>  			seq_printf(m, "model name\t: ARMv8 Processor rev %d (%s)\n",
>  				   MIDR_REVISION(midr), COMPAT_ELF_PLATFORM);
>  
> +		impl = (u8) MIDR_IMPLEMENTOR(midr);
> +		for (j = 0; hw_implementer[j].id != 0; j++) {
> +			if (hw_implementer[j].id == impl) {
> +				seq_printf(m, "CPU name\t: %s ",
> +					   hw_implementer[j].name);
> +				parts = hw_implementer[j].parts;
> +				break;
> +			}
> +		}
> +		if (hw_implementer[j].id == 0) {
> +			seq_printf(m, "CPU name\t: %s ",
> +				   hw_implementer[j].name);
> +			parts = hw_implementer[j].parts;
> +		}
> +
> +		part = (u16) MIDR_PARTNUM(midr);
> +		for (j = 0; parts[j].id != (-1); j++) {
> +			if (parts[j].id == part) {
> +				seq_printf(m, "%s\n", parts[j].name);
> +				break;
> +			}
> +		}
> +		if (parts[j].id == (-1))
> +			seq_printf(m, "%s", parts[j].name);
> +		seq_puts(m, "\n");
> +
>  		seq_printf(m, "BogoMIPS\t: %lu.%02lu\n",
>  			   loops_per_jiffy / (500000UL/HZ),
>  			   loops_per_jiffy / (5000UL/HZ) % 100);
> -- 
> 2.13.5
> 

[PATCH 2/3] arm64: cpuinfo: add human readable CPU names to /proc/cpuinfo

[Mark Rutland]

Hi Al,

On Tue, Sep 26, 2017 at 04:23:23PM -0600, Al Stone wrote:
> In the interest of making things easier for humans to use, add a
> "CPU name" line to /proc/cpuinfo for each CPU that uses plain old
> words instead of hex values.  For example, instead of printing only
> CPU implementer 0x43 and CPU part 0x0A1, print also "CPU name :
> Cavium ThunderX".
> 
> Note that this is not meant to be an exhaustive list of all possible
> implementers or CPUs (I'm not even sure that is knowable); this patch
> is intentionally limited to only those willing to provide info in
> arch/arm64/include/asm/cputype.h

As mentioned in the cover letter, and as with previous patches
attempting to do the same thing:

NAK to this patch.

In general, modifying /proc/cpuinfo comes with the risk of breaking
applications that (for better or worse) try to parse it mechanically.
Further, such modifications add to the problem by introducing yet more
ABI state applications may or may not end up depending on to function
(even if they don't require or use the information at all).

Specifically regarding mapping MIDRs to strings in the kernel, there are
a number of problems that make this undesirable, including (but not
limited to):

* As mentioned in the commit message, this cannot be complete, and is
  thus unreliable. We cannot know the set of CPUs that will exist in the
  future, and thus this practically mandates a kernel upgrade to use new
  CPUs, solely for a nicety.

* The names of CPUs can change, so there's not necessarily a single
  correct name to expose. As seen with Cortex-A12 and Cortex-A17 [1],
  where some users were upset if their CPU was considered to be a
  Cortex-A12 rather than a Cortex-A17.

  This embeds a political problem into kernel ABI.

* No matter how many times we tell them not to, applications *will* try
  to parse this. Any change (e.g. typo fixing or naming updates) will
  break some applications. An unexpectedly long CPU name may break
  parses using a small buffer.

  Worse, this could change over a kernel update, as a cpu goes from
  being:
  
    CPU name: unknown
  
  ... to being:

    CPU name: awesome super CPU 2000xx-super-mega-long-name-edition

  ... which could break applications, and some could argue that this is
  a kernel-side regression breaking userspace.

* This information can be derived from the MIDR and REVIDR. The decoded
  MIDR has always been exposed under /proc/cpuinfo, and both are exposed
  in a structured way under:

  /sys/devices/system/cpu/cpu${N}/regs/identification

  This duplicates information that way already expose, but in an
  unreliable fashion.

* This requires us to maintain an ever growing mapping of CPU IDs to
  strings.

* For similar reasons, this information is not exposed by 32-bit ARM,
  and further bifurcates our /proc/cpuinfo formats.

These problems cannot be solved kernel-side, and given this, I will
continue to NAK patches which attempt to decode the MIDR to human
readable strings.

Thanks,
Mark.

[1] https://community.arm.com/processors/b/blog/posts/arm-cortex-a17-cortex-a12-processor-update?CommentId=85c9dea4-1c9a-460c-a7b6-dcf59caab43d&pi10955=99

> 
> Signed-off-by: Al Stone <ahs3@redhat.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> ---
>  arch/arm64/kernel/cpuinfo.c | 84 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 84 insertions(+)
> 
> diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c
> index e505007138eb..0b4261884862 100644
> --- a/arch/arm64/kernel/cpuinfo.c
> +++ b/arch/arm64/kernel/cpuinfo.c
> @@ -75,6 +75,61 @@ static const char *const hwcap_str[] = {
>  	NULL
>  };
>  
> +struct hw_part {
> +	u16	id;
> +	char	*name;
> +};
> +
> +static const struct hw_part arm_hw_part[] = {
> +	{ ARM_CPU_PART_AEM_V8,		"AEMv8 Model" },
> +	{ ARM_CPU_PART_FOUNDATION,	"Foundation Model" },
> +	{ ARM_CPU_PART_CORTEX_A57,	"Cortex A57" },
> +	{ ARM_CPU_PART_CORTEX_A53,	"Cortex A53" },
> +	{ ARM_CPU_PART_CORTEX_A73,	"Cortex A73" },
> +	{ (-1), "unknown" }		/* Potenza == 0, unfortunately */
> +};
> +
> +static const struct hw_part apm_hw_part[] = {
> +	{ APM_CPU_PART_POTENZA,		"Potenza" },
> +	{ (-1), "unknown" }		/* Potenza == 0, unfortunately */
> +};
> +
> +static const struct hw_part brcm_hw_part[] = {
> +	{ BRCM_CPU_PART_VULCAN,		"Vulcan" },
> +	{ (-1), "unknown" }		/* Potenza == 0, unfortunately */
> +};
> +
> +static const struct hw_part cavium_hw_part[] = {
> +	{ CAVIUM_CPU_PART_THUNDERX,	 "ThunderX" },
> +	{ CAVIUM_CPU_PART_THUNDERX_81XX, "ThunderX 81XX" },
> +	{ CAVIUM_CPU_PART_THUNDERX_83XX, "ThunderX 83XX" },
> +	{ (-1), "unknown" }		/* Potenza == 0, unfortunately */
> +};
> +
> +static const struct hw_part qcom_hw_part[] = {
> +	{ QCOM_CPU_PART_FALKOR_V1,	"Falkor v1" },
> +	{ (-1), "unknown" }		/* Potenza == 0, unfortunately */
> +};
> +
> +static const struct hw_part unknown_hw_part[] = {
> +	{ (-1), "unknown" }		/* Potenza == 0, unfortunately */
> +};
> +
> +struct hw_impl {
> +	u8			id;
> +	const struct hw_part	*parts;
> +	char			*name;
> +};
> +
> +static const struct hw_impl hw_implementer[] = {
> +	{ ARM_CPU_IMP_ARM,	arm_hw_part,	"ARM Ltd." },
> +	{ ARM_CPU_IMP_APM,	apm_hw_part,	"Applied Micro" },
> +	{ ARM_CPU_IMP_CAVIUM,	cavium_hw_part,	"Cavium" },
> +	{ ARM_CPU_IMP_BRCM,	brcm_hw_part,	"Broadcom" },
> +	{ ARM_CPU_IMP_QCOM,	qcom_hw_part,	"Qualcomm" },
> +	{ 0, unknown_hw_part, "unknown" }
> +};
> +
>  #ifdef CONFIG_COMPAT
>  static const char *const compat_hwcap_str[] = {
>  	"swp",
> @@ -116,6 +171,9 @@ static int c_show(struct seq_file *m, void *v)
>  {
>  	int i, j;
>  	bool compat = personality(current->personality) == PER_LINUX32;
> +	u8 impl;
> +	u16 part;
> +	const struct hw_part *parts;
>  
>  	for_each_online_cpu(i) {
>  		struct cpuinfo_arm64 *cpuinfo = &per_cpu(cpu_data, i);
> @@ -132,6 +190,32 @@ static int c_show(struct seq_file *m, void *v)
>  			seq_printf(m, "model name\t: ARMv8 Processor rev %d (%s)\n",
>  				   MIDR_REVISION(midr), COMPAT_ELF_PLATFORM);
>  
> +		impl = (u8) MIDR_IMPLEMENTOR(midr);
> +		for (j = 0; hw_implementer[j].id != 0; j++) {
> +			if (hw_implementer[j].id == impl) {
> +				seq_printf(m, "CPU name\t: %s ",
> +					   hw_implementer[j].name);
> +				parts = hw_implementer[j].parts;
> +				break;
> +			}
> +		}
> +		if (hw_implementer[j].id == 0) {
> +			seq_printf(m, "CPU name\t: %s ",
> +				   hw_implementer[j].name);
> +			parts = hw_implementer[j].parts;
> +		}
> +
> +		part = (u16) MIDR_PARTNUM(midr);
> +		for (j = 0; parts[j].id != (-1); j++) {
> +			if (parts[j].id == part) {
> +				seq_printf(m, "%s\n", parts[j].name);
> +				break;
> +			}
> +		}
> +		if (parts[j].id == (-1))
> +			seq_printf(m, "%s", parts[j].name);
> +		seq_puts(m, "\n");
> +
>  		seq_printf(m, "BogoMIPS\t: %lu.%02lu\n",
>  			   loops_per_jiffy / (500000UL/HZ),
>  			   loops_per_jiffy / (5000UL/HZ) % 100);
> -- 
> 2.13.5
> 

Re: [PATCH 2/3] arm64: cpuinfo: add human readable CPU names to /proc/cpuinfo

[Timur Tabi]

On Tue, Sep 26, 2017 at 5:23 PM, Al Stone <ahs3@redhat.com> wrote:
> In the interest of making things easier for humans to use, add a
> "CPU name" line to /proc/cpuinfo for each CPU that uses plain old
> words instead of hex values.  For example, instead of printing only
> CPU implementer 0x43 and CPU part 0x0A1, print also "CPU name :
> Cavium ThunderX".
>
> Note that this is not meant to be an exhaustive list of all possible
> implementers or CPUs (I'm not even sure that is knowable); this patch
> is intentionally limited to only those willing to provide info in
> arch/arm64/include/asm/cputype.h
>
> Signed-off-by: Al Stone <ahs3@redhat.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> ---
>  arch/arm64/kernel/cpuinfo.c | 84 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 84 insertions(+)
>
> diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c
> index e505007138eb..0b4261884862 100644
> --- a/arch/arm64/kernel/cpuinfo.c
> +++ b/arch/arm64/kernel/cpuinfo.c
> @@ -75,6 +75,61 @@ static const char *const hwcap_str[] = {
>         NULL
>  };
>
> +struct hw_part {
> +       u16     id;
> +       char    *name;

const char *name ?

> +};
> +
> +static const struct hw_part arm_hw_part[] = {
> +       { ARM_CPU_PART_AEM_V8,          "AEMv8 Model" },

AEMv8?

> +       { ARM_CPU_PART_FOUNDATION,      "Foundation Model" },
> +       { ARM_CPU_PART_CORTEX_A57,      "Cortex A57" },
> +       { ARM_CPU_PART_CORTEX_A53,      "Cortex A53" },
> +       { ARM_CPU_PART_CORTEX_A73,      "Cortex A73" },
> +       { (-1), "unknown" }             /* Potenza == 0, unfortunately */

Since Potenza is an Applied Micro CPU, why is this comment here?

> +static const struct hw_part apm_hw_part[] = {
> +       { APM_CPU_PART_POTENZA,         "Potenza" },
> +       { (-1), "unknown" }             /* Potenza == 0, unfortunately */
> +};
> +
> +static const struct hw_part brcm_hw_part[] = {
> +       { BRCM_CPU_PART_VULCAN,         "Vulcan" },
> +       { (-1), "unknown" }             /* Potenza == 0, unfortunately */
> +};
> +
> +static const struct hw_part cavium_hw_part[] = {
> +       { CAVIUM_CPU_PART_THUNDERX,      "ThunderX" },
> +       { CAVIUM_CPU_PART_THUNDERX_81XX, "ThunderX 81XX" },
> +       { CAVIUM_CPU_PART_THUNDERX_83XX, "ThunderX 83XX" },
> +       { (-1), "unknown" }             /* Potenza == 0, unfortunately */
> +};
> +
> +static const struct hw_part qcom_hw_part[] = {
> +       { QCOM_CPU_PART_FALKOR_V1,      "Falkor v1" },

I guess we should upstream our other part IDs.

> +               impl = (u8) MIDR_IMPLEMENTOR(midr);
> +               for (j = 0; hw_implementer[j].id != 0; j++) {
> +                       if (hw_implementer[j].id == impl) {
> +                               seq_printf(m, "CPU name\t: %s ",
> +                                          hw_implementer[j].name);
> +                               parts = hw_implementer[j].parts;
> +                               break;
> +                       }
> +               }
> +               if (hw_implementer[j].id == 0) {
> +                       seq_printf(m, "CPU name\t: %s ",
> +                                  hw_implementer[j].name);
> +                       parts = hw_implementer[j].parts;
> +               }

Is this intended to handle Potenza?  I don't understand why a part ID
of 0 is such a problem.

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 2/3] arm64: cpuinfo: add human readable CPU names to /proc/cpuinfo

[Timur Tabi]

On Tue, Sep 26, 2017 at 5:23 PM, Al Stone <ahs3@redhat.com> wrote:
> In the interest of making things easier for humans to use, add a
> "CPU name" line to /proc/cpuinfo for each CPU that uses plain old
> words instead of hex values.  For example, instead of printing only
> CPU implementer 0x43 and CPU part 0x0A1, print also "CPU name :
> Cavium ThunderX".
>
> Note that this is not meant to be an exhaustive list of all possible
> implementers or CPUs (I'm not even sure that is knowable); this patch
> is intentionally limited to only those willing to provide info in
> arch/arm64/include/asm/cputype.h
>
> Signed-off-by: Al Stone <ahs3@redhat.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> ---
>  arch/arm64/kernel/cpuinfo.c | 84 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 84 insertions(+)
>
> diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c
> index e505007138eb..0b4261884862 100644
> --- a/arch/arm64/kernel/cpuinfo.c
> +++ b/arch/arm64/kernel/cpuinfo.c
> @@ -75,6 +75,61 @@ static const char *const hwcap_str[] = {
>         NULL
>  };
>
> +struct hw_part {
> +       u16     id;
> +       char    *name;

const char *name ?

> +};
> +
> +static const struct hw_part arm_hw_part[] = {
> +       { ARM_CPU_PART_AEM_V8,          "AEMv8 Model" },

AEMv8?

> +       { ARM_CPU_PART_FOUNDATION,      "Foundation Model" },
> +       { ARM_CPU_PART_CORTEX_A57,      "Cortex A57" },
> +       { ARM_CPU_PART_CORTEX_A53,      "Cortex A53" },
> +       { ARM_CPU_PART_CORTEX_A73,      "Cortex A73" },
> +       { (-1), "unknown" }             /* Potenza == 0, unfortunately */

Since Potenza is an Applied Micro CPU, why is this comment here?

> +static const struct hw_part apm_hw_part[] = {
> +       { APM_CPU_PART_POTENZA,         "Potenza" },
> +       { (-1), "unknown" }             /* Potenza == 0, unfortunately */
> +};
> +
> +static const struct hw_part brcm_hw_part[] = {
> +       { BRCM_CPU_PART_VULCAN,         "Vulcan" },
> +       { (-1), "unknown" }             /* Potenza == 0, unfortunately */
> +};
> +
> +static const struct hw_part cavium_hw_part[] = {
> +       { CAVIUM_CPU_PART_THUNDERX,      "ThunderX" },
> +       { CAVIUM_CPU_PART_THUNDERX_81XX, "ThunderX 81XX" },
> +       { CAVIUM_CPU_PART_THUNDERX_83XX, "ThunderX 83XX" },
> +       { (-1), "unknown" }             /* Potenza == 0, unfortunately */
> +};
> +
> +static const struct hw_part qcom_hw_part[] = {
> +       { QCOM_CPU_PART_FALKOR_V1,      "Falkor v1" },

I guess we should upstream our other part IDs.

> +               impl = (u8) MIDR_IMPLEMENTOR(midr);
> +               for (j = 0; hw_implementer[j].id != 0; j++) {
> +                       if (hw_implementer[j].id == impl) {
> +                               seq_printf(m, "CPU name\t: %s ",
> +                                          hw_implementer[j].name);
> +                               parts = hw_implementer[j].parts;
> +                               break;
> +                       }
> +               }
> +               if (hw_implementer[j].id == 0) {
> +                       seq_printf(m, "CPU name\t: %s ",
> +                                  hw_implementer[j].name);
> +                       parts = hw_implementer[j].parts;
> +               }

Is this intended to handle Potenza?  I don't understand why a part ID
of 0 is such a problem.

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 3/3] arm64: cpuinfo: display product info in /proc/cpuinfo

[Al Stone]

While it is very useful to know what CPU is being used, it is also
useful to know who made the platform being used.  On servers, this
can point to the right person to contact when a server is having
trouble.

Go get the product info -- manufacturer, product name and version --
from DMI (aka SMBIOS) and display it in /proc/cpuinfo.  To look more
like other server platforms, include the CPU type and frequency when
displaying the product info, too.

Signed-off-by: Al Stone <ahs3@redhat.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
---
 arch/arm64/kernel/cpuinfo.c | 135 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 134 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c
index 0b4261884862..6a9dbad5ee3f 100644
--- a/arch/arm64/kernel/cpuinfo.c
+++ b/arch/arm64/kernel/cpuinfo.c
@@ -19,10 +19,12 @@
 #include <asm/cpu.h>
 #include <asm/cputype.h>
 #include <asm/cpufeature.h>
+#include <asm/unaligned.h>
 
 #include <linux/bitops.h>
 #include <linux/bug.h>
 #include <linux/compat.h>
+#include <linux/dmi.h>
 #include <linux/elf.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
@@ -31,6 +33,7 @@
 #include <linux/printk.h>
 #include <linux/seq_file.h>
 #include <linux/sched.h>
+#include <linux/slab.h>
 #include <linux/smp.h>
 #include <linux/delay.h>
 #include <linux/cpuinfo.h>
@@ -167,6 +170,111 @@ static const char *const compat_hwcap2_str[] = {
 };
 #endif /* CONFIG_COMPAT */
 
+/* Details needed when extracting fields from DMI info */
+#define DMI_ENTRY_BASEBOARD_MIN_LENGTH	8
+#define DMI_ENTRY_PROCESSOR_MIN_LENGTH	48
+
+#define DMI_BASEBOARD_MANUFACTURER	0x04
+#define DMI_BASEBOARD_PRODUCT		0x05
+#define DMI_BASEBOARD_VERSION		0x06
+#define DMI_PROCESSOR_MAX_SPEED		0x14
+
+#define DMI_MAX_STRLEN			80
+
+/* Values captured from DMI info */
+static u64 dmi_max_mhz;
+static char *dmi_product_info;
+
+/* Callback function used to retrieve the max frequency from DMI */
+static void find_dmi_mhz(const struct dmi_header *dm, void *private)
+{
+	const u8 *dmi_data = (const u8 *)dm;
+	u16 *mhz = (u16 *)private;
+
+	if (dm->type == DMI_ENTRY_PROCESSOR &&
+	    dm->length >= DMI_ENTRY_PROCESSOR_MIN_LENGTH) {
+		u16 val = (u16)get_unaligned((const u16 *)
+				(dmi_data + DMI_PROCESSOR_MAX_SPEED));
+		*mhz = val > *mhz ? val : *mhz;
+	}
+}
+
+/* Look up the max frequency in DMI */
+static u64 get_dmi_max_mhz(void)
+{
+	u16 mhz = 0;
+
+	dmi_walk(find_dmi_mhz, &mhz);
+
+	/*
+	 * Real stupid fallback value, just in case there is no
+	 * actual value set.
+	 */
+	mhz = mhz ? mhz : 1;
+
+	return (u64)mhz;
+}
+
+/* Helper function for the product info callback */
+static char *copy_string_n(char *dst, char *table, int idx)
+{
+	char *d = dst;
+	char *ptr = table;
+	int ii;
+
+	/* skip the first idx-1 strings */
+	for (ii = 1; ii < idx; ii++) {
+		while (*ptr)
+			ptr++;
+		ptr++;
+	}
+
+	/* copy in the string we need */
+	while (*ptr && (d - dst) < (DMI_MAX_STRLEN - 2))
+		*d++ = *ptr++;
+
+	return d;
+}
+
+/* Callback function used to retrieve the product info DMI */
+static void find_dmi_product_info(const struct dmi_header *dm, void *private)
+{
+	const u8 *dmi_data = (const u8 *)dm;
+	char *ptr = (char *)private;
+
+	if (dm->type == DMI_ENTRY_BASEBOARD &&
+	    dm->length >= DMI_ENTRY_BASEBOARD_MIN_LENGTH) {
+		int idx;
+
+		idx = (int)get_unaligned((const u8 *)
+				(dmi_data + DMI_BASEBOARD_MANUFACTURER));
+		ptr = copy_string_n(ptr, (char *)(dmi_data + dm->length), idx);
+		*ptr++ = ' ';
+
+		idx = (int)get_unaligned((const u8 *)
+				(dmi_data + DMI_BASEBOARD_PRODUCT));
+		ptr = copy_string_n(ptr, (char *)(dmi_data + dm->length), idx);
+		*ptr++ = ' ';
+
+		idx = (int)get_unaligned((const u8 *)
+				(dmi_data + DMI_BASEBOARD_VERSION));
+		ptr = copy_string_n(ptr, (char *)(dmi_data + dm->length), idx);
+	}
+}
+
+/* Look up the baseboard info in DMI */
+static void get_dmi_product_info(void)
+{
+	if (!dmi_product_info) {
+		dmi_product_info = kcalloc(DMI_MAX_STRLEN,
+					   sizeof(char), GFP_KERNEL);
+		if (!dmi_product_info)
+			return;
+	}
+
+	dmi_walk(find_dmi_product_info, dmi_product_info);
+}
+
 static int c_show(struct seq_file *m, void *v)
 {
 	int i, j;
@@ -190,6 +298,31 @@ static int c_show(struct seq_file *m, void *v)
 			seq_printf(m, "model name\t: ARMv8 Processor rev %d (%s)\n",
 				   MIDR_REVISION(midr), COMPAT_ELF_PLATFORM);
 
+		if (IS_ENABLED(CONFIG_DMI)) {
+			seq_puts(m, "product name\t: ");
+
+			if (!dmi_product_info)
+				get_dmi_product_info();
+			if (dmi_product_info)
+				seq_printf(m, "%s", dmi_product_info);
+			else
+				seq_puts(m, "<unknown>");
+
+			seq_printf(m, ", ARM 8.%d (r%dp%d) CPU",
+				   MIDR_VARIANT(midr),
+				   MIDR_VARIANT(midr),
+				   MIDR_REVISION(midr));
+
+			if (!dmi_max_mhz)
+				dmi_max_mhz = get_dmi_max_mhz();
+			if (dmi_max_mhz)
+				seq_printf(m, " @ %d.%02dGHz\n",
+					   (int)(dmi_max_mhz / 1000),
+					   (int)(dmi_max_mhz % 1000));
+			else
+				seq_puts(m, " @ <unknown>GHz\n");
+		}
+
 		impl = (u8) MIDR_IMPLEMENTOR(midr);
 		for (j = 0; hw_implementer[j].id != 0; j++) {
 			if (hw_implementer[j].id == impl) {
@@ -208,7 +341,7 @@ static int c_show(struct seq_file *m, void *v)
 		part = (u16) MIDR_PARTNUM(midr);
 		for (j = 0; parts[j].id != (-1); j++) {
 			if (parts[j].id == part) {
-				seq_printf(m, "%s\n", parts[j].name);
+				seq_printf(m, "%s ", parts[j].name);
 				break;
 			}
 		}
-- 
2.13.5

[PATCH 3/3] arm64: cpuinfo: display product info in /proc/cpuinfo

[Al Stone]

While it is very useful to know what CPU is being used, it is also
useful to know who made the platform being used.  On servers, this
can point to the right person to contact when a server is having
trouble.

Go get the product info -- manufacturer, product name and version --
from DMI (aka SMBIOS) and display it in /proc/cpuinfo.  To look more
like other server platforms, include the CPU type and frequency when
displaying the product info, too.

Signed-off-by: Al Stone <ahs3@redhat.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
---
 arch/arm64/kernel/cpuinfo.c | 135 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 134 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c
index 0b4261884862..6a9dbad5ee3f 100644
--- a/arch/arm64/kernel/cpuinfo.c
+++ b/arch/arm64/kernel/cpuinfo.c
@@ -19,10 +19,12 @@
 #include <asm/cpu.h>
 #include <asm/cputype.h>
 #include <asm/cpufeature.h>
+#include <asm/unaligned.h>
 
 #include <linux/bitops.h>
 #include <linux/bug.h>
 #include <linux/compat.h>
+#include <linux/dmi.h>
 #include <linux/elf.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
@@ -31,6 +33,7 @@
 #include <linux/printk.h>
 #include <linux/seq_file.h>
 #include <linux/sched.h>
+#include <linux/slab.h>
 #include <linux/smp.h>
 #include <linux/delay.h>
 #include <linux/cpuinfo.h>
@@ -167,6 +170,111 @@ static const char *const compat_hwcap2_str[] = {
 };
 #endif /* CONFIG_COMPAT */
 
+/* Details needed when extracting fields from DMI info */
+#define DMI_ENTRY_BASEBOARD_MIN_LENGTH	8
+#define DMI_ENTRY_PROCESSOR_MIN_LENGTH	48
+
+#define DMI_BASEBOARD_MANUFACTURER	0x04
+#define DMI_BASEBOARD_PRODUCT		0x05
+#define DMI_BASEBOARD_VERSION		0x06
+#define DMI_PROCESSOR_MAX_SPEED		0x14
+
+#define DMI_MAX_STRLEN			80
+
+/* Values captured from DMI info */
+static u64 dmi_max_mhz;
+static char *dmi_product_info;
+
+/* Callback function used to retrieve the max frequency from DMI */
+static void find_dmi_mhz(const struct dmi_header *dm, void *private)
+{
+	const u8 *dmi_data = (const u8 *)dm;
+	u16 *mhz = (u16 *)private;
+
+	if (dm->type == DMI_ENTRY_PROCESSOR &&
+	    dm->length >= DMI_ENTRY_PROCESSOR_MIN_LENGTH) {
+		u16 val = (u16)get_unaligned((const u16 *)
+				(dmi_data + DMI_PROCESSOR_MAX_SPEED));
+		*mhz = val > *mhz ? val : *mhz;
+	}
+}
+
+/* Look up the max frequency in DMI */
+static u64 get_dmi_max_mhz(void)
+{
+	u16 mhz = 0;
+
+	dmi_walk(find_dmi_mhz, &mhz);
+
+	/*
+	 * Real stupid fallback value, just in case there is no
+	 * actual value set.
+	 */
+	mhz = mhz ? mhz : 1;
+
+	return (u64)mhz;
+}
+
+/* Helper function for the product info callback */
+static char *copy_string_n(char *dst, char *table, int idx)
+{
+	char *d = dst;
+	char *ptr = table;
+	int ii;
+
+	/* skip the first idx-1 strings */
+	for (ii = 1; ii < idx; ii++) {
+		while (*ptr)
+			ptr++;
+		ptr++;
+	}
+
+	/* copy in the string we need */
+	while (*ptr && (d - dst) < (DMI_MAX_STRLEN - 2))
+		*d++ = *ptr++;
+
+	return d;
+}
+
+/* Callback function used to retrieve the product info DMI */
+static void find_dmi_product_info(const struct dmi_header *dm, void *private)
+{
+	const u8 *dmi_data = (const u8 *)dm;
+	char *ptr = (char *)private;
+
+	if (dm->type == DMI_ENTRY_BASEBOARD &&
+	    dm->length >= DMI_ENTRY_BASEBOARD_MIN_LENGTH) {
+		int idx;
+
+		idx = (int)get_unaligned((const u8 *)
+				(dmi_data + DMI_BASEBOARD_MANUFACTURER));
+		ptr = copy_string_n(ptr, (char *)(dmi_data + dm->length), idx);
+		*ptr++ = ' ';
+
+		idx = (int)get_unaligned((const u8 *)
+				(dmi_data + DMI_BASEBOARD_PRODUCT));
+		ptr = copy_string_n(ptr, (char *)(dmi_data + dm->length), idx);
+		*ptr++ = ' ';
+
+		idx = (int)get_unaligned((const u8 *)
+				(dmi_data + DMI_BASEBOARD_VERSION));
+		ptr = copy_string_n(ptr, (char *)(dmi_data + dm->length), idx);
+	}
+}
+
+/* Look up the baseboard info in DMI */
+static void get_dmi_product_info(void)
+{
+	if (!dmi_product_info) {
+		dmi_product_info = kcalloc(DMI_MAX_STRLEN,
+					   sizeof(char), GFP_KERNEL);
+		if (!dmi_product_info)
+			return;
+	}
+
+	dmi_walk(find_dmi_product_info, dmi_product_info);
+}
+
 static int c_show(struct seq_file *m, void *v)
 {
 	int i, j;
@@ -190,6 +298,31 @@ static int c_show(struct seq_file *m, void *v)
 			seq_printf(m, "model name\t: ARMv8 Processor rev %d (%s)\n",
 				   MIDR_REVISION(midr), COMPAT_ELF_PLATFORM);
 
+		if (IS_ENABLED(CONFIG_DMI)) {
+			seq_puts(m, "product name\t: ");
+
+			if (!dmi_product_info)
+				get_dmi_product_info();
+			if (dmi_product_info)
+				seq_printf(m, "%s", dmi_product_info);
+			else
+				seq_puts(m, "<unknown>");
+
+			seq_printf(m, ", ARM 8.%d (r%dp%d) CPU",
+				   MIDR_VARIANT(midr),
+				   MIDR_VARIANT(midr),
+				   MIDR_REVISION(midr));
+
+			if (!dmi_max_mhz)
+				dmi_max_mhz = get_dmi_max_mhz();
+			if (dmi_max_mhz)
+				seq_printf(m, " @ %d.%02dGHz\n",
+					   (int)(dmi_max_mhz / 1000),
+					   (int)(dmi_max_mhz % 1000));
+			else
+				seq_puts(m, " @ <unknown>GHz\n");
+		}
+
 		impl = (u8) MIDR_IMPLEMENTOR(midr);
 		for (j = 0; hw_implementer[j].id != 0; j++) {
 			if (hw_implementer[j].id == impl) {
@@ -208,7 +341,7 @@ static int c_show(struct seq_file *m, void *v)
 		part = (u16) MIDR_PARTNUM(midr);
 		for (j = 0; parts[j].id != (-1); j++) {
 			if (parts[j].id == part) {
-				seq_printf(m, "%s\n", parts[j].name);
+				seq_printf(m, "%s ", parts[j].name);
 				break;
 			}
 		}
-- 
2.13.5

Re: [PATCH 3/3] arm64: cpuinfo: display product info in /proc/cpuinfo

[Florian Fainelli]

On 09/26/2017 03:23 PM, Al Stone wrote:
> While it is very useful to know what CPU is being used, it is also
> useful to know who made the platform being used.  On servers, this
> can point to the right person to contact when a server is having
> trouble.
> 
> Go get the product info -- manufacturer, product name and version --
> from DMI (aka SMBIOS) and display it in /proc/cpuinfo.  To look more
> like other server platforms, include the CPU type and frequency when
> displaying the product info, too.
> 
> Signed-off-by: Al Stone <ahs3@redhat.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> ---

[snip]

> +/* Look up the baseboard info in DMI */
> +static void get_dmi_product_info(void)
> +{
> +	if (!dmi_product_info) {
> +		dmi_product_info = kcalloc(DMI_MAX_STRLEN,
> +					   sizeof(char), GFP_KERNEL);
> +		if (!dmi_product_info)
> +			return;
> +	}
> +
> +	dmi_walk(find_dmi_product_info, dmi_product_info);
> +}

Don't you need all of these DMI-related functions you defined to be also
enclosed within an #if IS_ENABLED(CONFIG_DMI) otherwise chances are that
we are going to get defined but unused warnings?


>  		impl = (u8) MIDR_IMPLEMENTOR(midr);
>  		for (j = 0; hw_implementer[j].id != 0; j++) {
>  			if (hw_implementer[j].id == impl) {
> @@ -208,7 +341,7 @@ static int c_show(struct seq_file *m, void *v)
>  		part = (u16) MIDR_PARTNUM(midr);
>  		for (j = 0; parts[j].id != (-1); j++) {
>  			if (parts[j].id == part) {
> -				seq_printf(m, "%s\n", parts[j].name);
> +				seq_printf(m, "%s ", parts[j].name);
>  				break;
>  			}

Should this hunk be part of patch 3?
-- 
Florian

[PATCH 3/3] arm64: cpuinfo: display product info in /proc/cpuinfo

[Florian Fainelli]

On 09/26/2017 03:23 PM, Al Stone wrote:
> While it is very useful to know what CPU is being used, it is also
> useful to know who made the platform being used.  On servers, this
> can point to the right person to contact when a server is having
> trouble.
> 
> Go get the product info -- manufacturer, product name and version --
> from DMI (aka SMBIOS) and display it in /proc/cpuinfo.  To look more
> like other server platforms, include the CPU type and frequency when
> displaying the product info, too.
> 
> Signed-off-by: Al Stone <ahs3@redhat.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> ---

[snip]

> +/* Look up the baseboard info in DMI */
> +static void get_dmi_product_info(void)
> +{
> +	if (!dmi_product_info) {
> +		dmi_product_info = kcalloc(DMI_MAX_STRLEN,
> +					   sizeof(char), GFP_KERNEL);
> +		if (!dmi_product_info)
> +			return;
> +	}
> +
> +	dmi_walk(find_dmi_product_info, dmi_product_info);
> +}

Don't you need all of these DMI-related functions you defined to be also
enclosed within an #if IS_ENABLED(CONFIG_DMI) otherwise chances are that
we are going to get defined but unused warnings?


>  		impl = (u8) MIDR_IMPLEMENTOR(midr);
>  		for (j = 0; hw_implementer[j].id != 0; j++) {
>  			if (hw_implementer[j].id == impl) {
> @@ -208,7 +341,7 @@ static int c_show(struct seq_file *m, void *v)
>  		part = (u16) MIDR_PARTNUM(midr);
>  		for (j = 0; parts[j].id != (-1); j++) {
>  			if (parts[j].id == part) {
> -				seq_printf(m, "%s\n", parts[j].name);
> +				seq_printf(m, "%s ", parts[j].name);
>  				break;
>  			}

Should this hunk be part of patch 3?
-- 
Florian

Re: [PATCH 3/3] arm64: cpuinfo: display product info in /proc/cpuinfo

[Robin Murphy]

On 26/09/17 23:23, Al Stone wrote:
> While it is very useful to know what CPU is being used, it is also
> useful to know who made the platform being used.  On servers, this
> can point to the right person to contact when a server is having
> trouble.
> 
> Go get the product info -- manufacturer, product name and version --
> from DMI (aka SMBIOS) and display it in /proc/cpuinfo.  To look more
> like other server platforms, include the CPU type and frequency when
> displaying the product info, too.
> 
> Signed-off-by: Al Stone <ahs3@redhat.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> ---
[...]
> @@ -190,6 +298,31 @@ static int c_show(struct seq_file *m, void *v)
>  			seq_printf(m, "model name\t: ARMv8 Processor rev %d (%s)\n",
>  				   MIDR_REVISION(midr), COMPAT_ELF_PLATFORM);
>  
> +		if (IS_ENABLED(CONFIG_DMI)) {
> +			seq_puts(m, "product name\t: ");
> +
> +			if (!dmi_product_info)
> +				get_dmi_product_info();
> +			if (dmi_product_info)
> +				seq_printf(m, "%s", dmi_product_info);
> +			else
> +				seq_puts(m, "<unknown>");
> +
> +			seq_printf(m, ", ARM 8.%d (r%dp%d) CPU",
> +				   MIDR_VARIANT(midr),
> +				   MIDR_VARIANT(midr),
> +				   MIDR_REVISION(midr));

What is "ARM 8.1" meant to infer for, say, a typical Cortex-A57?

Robin.

> +
> +			if (!dmi_max_mhz)
> +				dmi_max_mhz = get_dmi_max_mhz();
> +			if (dmi_max_mhz)
> +				seq_printf(m, " @ %d.%02dGHz\n",
> +					   (int)(dmi_max_mhz / 1000),
> +					   (int)(dmi_max_mhz % 1000));
> +			else
> +				seq_puts(m, " @ <unknown>GHz\n");
> +		}
> +
>  		impl = (u8) MIDR_IMPLEMENTOR(midr);
>  		for (j = 0; hw_implementer[j].id != 0; j++) {
>  			if (hw_implementer[j].id == impl) {

[PATCH 3/3] arm64: cpuinfo: display product info in /proc/cpuinfo

[Robin Murphy]

On 26/09/17 23:23, Al Stone wrote:
> While it is very useful to know what CPU is being used, it is also
> useful to know who made the platform being used.  On servers, this
> can point to the right person to contact when a server is having
> trouble.
> 
> Go get the product info -- manufacturer, product name and version --
> from DMI (aka SMBIOS) and display it in /proc/cpuinfo.  To look more
> like other server platforms, include the CPU type and frequency when
> displaying the product info, too.
> 
> Signed-off-by: Al Stone <ahs3@redhat.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> ---
[...]
> @@ -190,6 +298,31 @@ static int c_show(struct seq_file *m, void *v)
>  			seq_printf(m, "model name\t: ARMv8 Processor rev %d (%s)\n",
>  				   MIDR_REVISION(midr), COMPAT_ELF_PLATFORM);
>  
> +		if (IS_ENABLED(CONFIG_DMI)) {
> +			seq_puts(m, "product name\t: ");
> +
> +			if (!dmi_product_info)
> +				get_dmi_product_info();
> +			if (dmi_product_info)
> +				seq_printf(m, "%s", dmi_product_info);
> +			else
> +				seq_puts(m, "<unknown>");
> +
> +			seq_printf(m, ", ARM 8.%d (r%dp%d) CPU",
> +				   MIDR_VARIANT(midr),
> +				   MIDR_VARIANT(midr),
> +				   MIDR_REVISION(midr));

What is "ARM 8.1" meant to infer for, say, a typical Cortex-A57?

Robin.

> +
> +			if (!dmi_max_mhz)
> +				dmi_max_mhz = get_dmi_max_mhz();
> +			if (dmi_max_mhz)
> +				seq_printf(m, " @ %d.%02dGHz\n",
> +					   (int)(dmi_max_mhz / 1000),
> +					   (int)(dmi_max_mhz % 1000));
> +			else
> +				seq_puts(m, " @ <unknown>GHz\n");
> +		}
> +
>  		impl = (u8) MIDR_IMPLEMENTOR(midr);
>  		for (j = 0; hw_implementer[j].id != 0; j++) {
>  			if (hw_implementer[j].id == impl) {

Re: [PATCH 3/3] arm64: cpuinfo: display product info in /proc/cpuinfo

[Mark Rutland]

Hi,

On Wed, Sep 27, 2017 at 11:42:07AM +0100, Robin Murphy wrote:
> On 26/09/17 23:23, Al Stone wrote:
> > +			seq_printf(m, ", ARM 8.%d (r%dp%d) CPU",
> > +				   MIDR_VARIANT(midr),
> > +				   MIDR_VARIANT(midr),
> > +				   MIDR_REVISION(midr));
> 
> What is "ARM 8.1" meant to infer for, say, a typical Cortex-A57?

Just to make Robin's point a little clearer, MIDR_EL1.Variant is
IMPLEMENTATION DEFINED, and doesn't describe the ARMv8.x architecture
revision.

For example, on Cortex A57 is contains the major revision number of the
CPU, and is 1 for any r1pY Cortex-A57 (e.g. those on Juno R1).

For better or worse, the architecture provides us no mechanism to
determine the architecture revision.

Thanks,
Mark.

[PATCH 3/3] arm64: cpuinfo: display product info in /proc/cpuinfo

[Mark Rutland]

Hi,

On Wed, Sep 27, 2017 at 11:42:07AM +0100, Robin Murphy wrote:
> On 26/09/17 23:23, Al Stone wrote:
> > +			seq_printf(m, ", ARM 8.%d (r%dp%d) CPU",
> > +				   MIDR_VARIANT(midr),
> > +				   MIDR_VARIANT(midr),
> > +				   MIDR_REVISION(midr));
> 
> What is "ARM 8.1" meant to infer for, say, a typical Cortex-A57?

Just to make Robin's point a little clearer, MIDR_EL1.Variant is
IMPLEMENTATION DEFINED, and doesn't describe the ARMv8.x architecture
revision.

For example, on Cortex A57 is contains the major revision number of the
CPU, and is 1 for any r1pY Cortex-A57 (e.g. those on Juno R1).

For better or worse, the architecture provides us no mechanism to
determine the architecture revision.

Thanks,
Mark.

Re: [PATCH 3/3] arm64: cpuinfo: display product info in /proc/cpuinfo

[Mark Rutland]

Hi Al,

On Tue, Sep 26, 2017 at 04:23:24PM -0600, Al Stone wrote:
> While it is very useful to know what CPU is being used, it is also
> useful to know who made the platform being used.  On servers, this
> can point to the right person to contact when a server is having
> trouble.
> 
> Go get the product info -- manufacturer, product name and version --
> from DMI (aka SMBIOS) and display it in /proc/cpuinfo.  To look more
> like other server platforms, include the CPU type and frequency when
> displaying the product info, too.

As mentioned on the cover letter, NAK to modifiying /proc/cpuinfo.

I note this is all DMI information, which I thought we already exposed
under sysfs (in an architecture-neutral format).

Is that not the case?

... or do existing tools not pick that up today?

Thanks,
Mark.

> 
> Signed-off-by: Al Stone <ahs3@redhat.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> ---
>  arch/arm64/kernel/cpuinfo.c | 135 +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 134 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c
> index 0b4261884862..6a9dbad5ee3f 100644
> --- a/arch/arm64/kernel/cpuinfo.c
> +++ b/arch/arm64/kernel/cpuinfo.c
> @@ -19,10 +19,12 @@
>  #include <asm/cpu.h>
>  #include <asm/cputype.h>
>  #include <asm/cpufeature.h>
> +#include <asm/unaligned.h>
>  
>  #include <linux/bitops.h>
>  #include <linux/bug.h>
>  #include <linux/compat.h>
> +#include <linux/dmi.h>
>  #include <linux/elf.h>
>  #include <linux/init.h>
>  #include <linux/kernel.h>
> @@ -31,6 +33,7 @@
>  #include <linux/printk.h>
>  #include <linux/seq_file.h>
>  #include <linux/sched.h>
> +#include <linux/slab.h>
>  #include <linux/smp.h>
>  #include <linux/delay.h>
>  #include <linux/cpuinfo.h>
> @@ -167,6 +170,111 @@ static const char *const compat_hwcap2_str[] = {
>  };
>  #endif /* CONFIG_COMPAT */
>  
> +/* Details needed when extracting fields from DMI info */
> +#define DMI_ENTRY_BASEBOARD_MIN_LENGTH	8
> +#define DMI_ENTRY_PROCESSOR_MIN_LENGTH	48
> +
> +#define DMI_BASEBOARD_MANUFACTURER	0x04
> +#define DMI_BASEBOARD_PRODUCT		0x05
> +#define DMI_BASEBOARD_VERSION		0x06
> +#define DMI_PROCESSOR_MAX_SPEED		0x14
> +
> +#define DMI_MAX_STRLEN			80
> +
> +/* Values captured from DMI info */
> +static u64 dmi_max_mhz;
> +static char *dmi_product_info;
> +
> +/* Callback function used to retrieve the max frequency from DMI */
> +static void find_dmi_mhz(const struct dmi_header *dm, void *private)
> +{
> +	const u8 *dmi_data = (const u8 *)dm;
> +	u16 *mhz = (u16 *)private;
> +
> +	if (dm->type == DMI_ENTRY_PROCESSOR &&
> +	    dm->length >= DMI_ENTRY_PROCESSOR_MIN_LENGTH) {
> +		u16 val = (u16)get_unaligned((const u16 *)
> +				(dmi_data + DMI_PROCESSOR_MAX_SPEED));
> +		*mhz = val > *mhz ? val : *mhz;
> +	}
> +}
> +
> +/* Look up the max frequency in DMI */
> +static u64 get_dmi_max_mhz(void)
> +{
> +	u16 mhz = 0;
> +
> +	dmi_walk(find_dmi_mhz, &mhz);
> +
> +	/*
> +	 * Real stupid fallback value, just in case there is no
> +	 * actual value set.
> +	 */
> +	mhz = mhz ? mhz : 1;
> +
> +	return (u64)mhz;
> +}
> +
> +/* Helper function for the product info callback */
> +static char *copy_string_n(char *dst, char *table, int idx)
> +{
> +	char *d = dst;
> +	char *ptr = table;
> +	int ii;
> +
> +	/* skip the first idx-1 strings */
> +	for (ii = 1; ii < idx; ii++) {
> +		while (*ptr)
> +			ptr++;
> +		ptr++;
> +	}
> +
> +	/* copy in the string we need */
> +	while (*ptr && (d - dst) < (DMI_MAX_STRLEN - 2))
> +		*d++ = *ptr++;
> +
> +	return d;
> +}
> +
> +/* Callback function used to retrieve the product info DMI */
> +static void find_dmi_product_info(const struct dmi_header *dm, void *private)
> +{
> +	const u8 *dmi_data = (const u8 *)dm;
> +	char *ptr = (char *)private;
> +
> +	if (dm->type == DMI_ENTRY_BASEBOARD &&
> +	    dm->length >= DMI_ENTRY_BASEBOARD_MIN_LENGTH) {
> +		int idx;
> +
> +		idx = (int)get_unaligned((const u8 *)
> +				(dmi_data + DMI_BASEBOARD_MANUFACTURER));
> +		ptr = copy_string_n(ptr, (char *)(dmi_data + dm->length), idx);
> +		*ptr++ = ' ';
> +
> +		idx = (int)get_unaligned((const u8 *)
> +				(dmi_data + DMI_BASEBOARD_PRODUCT));
> +		ptr = copy_string_n(ptr, (char *)(dmi_data + dm->length), idx);
> +		*ptr++ = ' ';
> +
> +		idx = (int)get_unaligned((const u8 *)
> +				(dmi_data + DMI_BASEBOARD_VERSION));
> +		ptr = copy_string_n(ptr, (char *)(dmi_data + dm->length), idx);
> +	}
> +}
> +
> +/* Look up the baseboard info in DMI */
> +static void get_dmi_product_info(void)
> +{
> +	if (!dmi_product_info) {
> +		dmi_product_info = kcalloc(DMI_MAX_STRLEN,
> +					   sizeof(char), GFP_KERNEL);
> +		if (!dmi_product_info)
> +			return;
> +	}
> +
> +	dmi_walk(find_dmi_product_info, dmi_product_info);
> +}
> +
>  static int c_show(struct seq_file *m, void *v)
>  {
>  	int i, j;
> @@ -190,6 +298,31 @@ static int c_show(struct seq_file *m, void *v)
>  			seq_printf(m, "model name\t: ARMv8 Processor rev %d (%s)\n",
>  				   MIDR_REVISION(midr), COMPAT_ELF_PLATFORM);
>  
> +		if (IS_ENABLED(CONFIG_DMI)) {
> +			seq_puts(m, "product name\t: ");
> +
> +			if (!dmi_product_info)
> +				get_dmi_product_info();
> +			if (dmi_product_info)
> +				seq_printf(m, "%s", dmi_product_info);
> +			else
> +				seq_puts(m, "<unknown>");
> +
> +			seq_printf(m, ", ARM 8.%d (r%dp%d) CPU",
> +				   MIDR_VARIANT(midr),
> +				   MIDR_VARIANT(midr),
> +				   MIDR_REVISION(midr));
> +
> +			if (!dmi_max_mhz)
> +				dmi_max_mhz = get_dmi_max_mhz();
> +			if (dmi_max_mhz)
> +				seq_printf(m, " @ %d.%02dGHz\n",
> +					   (int)(dmi_max_mhz / 1000),
> +					   (int)(dmi_max_mhz % 1000));
> +			else
> +				seq_puts(m, " @ <unknown>GHz\n");
> +		}
> +
>  		impl = (u8) MIDR_IMPLEMENTOR(midr);
>  		for (j = 0; hw_implementer[j].id != 0; j++) {
>  			if (hw_implementer[j].id == impl) {
> @@ -208,7 +341,7 @@ static int c_show(struct seq_file *m, void *v)
>  		part = (u16) MIDR_PARTNUM(midr);
>  		for (j = 0; parts[j].id != (-1); j++) {
>  			if (parts[j].id == part) {
> -				seq_printf(m, "%s\n", parts[j].name);
> +				seq_printf(m, "%s ", parts[j].name);
>  				break;
>  			}
>  		}
> -- 
> 2.13.5
> 

[PATCH 3/3] arm64: cpuinfo: display product info in /proc/cpuinfo

[Mark Rutland]

Hi Al,

On Tue, Sep 26, 2017 at 04:23:24PM -0600, Al Stone wrote:
> While it is very useful to know what CPU is being used, it is also
> useful to know who made the platform being used.  On servers, this
> can point to the right person to contact when a server is having
> trouble.
> 
> Go get the product info -- manufacturer, product name and version --
> from DMI (aka SMBIOS) and display it in /proc/cpuinfo.  To look more
> like other server platforms, include the CPU type and frequency when
> displaying the product info, too.

As mentioned on the cover letter, NAK to modifiying /proc/cpuinfo.

I note this is all DMI information, which I thought we already exposed
under sysfs (in an architecture-neutral format).

Is that not the case?

... or do existing tools not pick that up today?

Thanks,
Mark.

> 
> Signed-off-by: Al Stone <ahs3@redhat.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> ---
>  arch/arm64/kernel/cpuinfo.c | 135 +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 134 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c
> index 0b4261884862..6a9dbad5ee3f 100644
> --- a/arch/arm64/kernel/cpuinfo.c
> +++ b/arch/arm64/kernel/cpuinfo.c
> @@ -19,10 +19,12 @@
>  #include <asm/cpu.h>
>  #include <asm/cputype.h>
>  #include <asm/cpufeature.h>
> +#include <asm/unaligned.h>
>  
>  #include <linux/bitops.h>
>  #include <linux/bug.h>
>  #include <linux/compat.h>
> +#include <linux/dmi.h>
>  #include <linux/elf.h>
>  #include <linux/init.h>
>  #include <linux/kernel.h>
> @@ -31,6 +33,7 @@
>  #include <linux/printk.h>
>  #include <linux/seq_file.h>
>  #include <linux/sched.h>
> +#include <linux/slab.h>
>  #include <linux/smp.h>
>  #include <linux/delay.h>
>  #include <linux/cpuinfo.h>
> @@ -167,6 +170,111 @@ static const char *const compat_hwcap2_str[] = {
>  };
>  #endif /* CONFIG_COMPAT */
>  
> +/* Details needed when extracting fields from DMI info */
> +#define DMI_ENTRY_BASEBOARD_MIN_LENGTH	8
> +#define DMI_ENTRY_PROCESSOR_MIN_LENGTH	48
> +
> +#define DMI_BASEBOARD_MANUFACTURER	0x04
> +#define DMI_BASEBOARD_PRODUCT		0x05
> +#define DMI_BASEBOARD_VERSION		0x06
> +#define DMI_PROCESSOR_MAX_SPEED		0x14
> +
> +#define DMI_MAX_STRLEN			80
> +
> +/* Values captured from DMI info */
> +static u64 dmi_max_mhz;
> +static char *dmi_product_info;
> +
> +/* Callback function used to retrieve the max frequency from DMI */
> +static void find_dmi_mhz(const struct dmi_header *dm, void *private)
> +{
> +	const u8 *dmi_data = (const u8 *)dm;
> +	u16 *mhz = (u16 *)private;
> +
> +	if (dm->type == DMI_ENTRY_PROCESSOR &&
> +	    dm->length >= DMI_ENTRY_PROCESSOR_MIN_LENGTH) {
> +		u16 val = (u16)get_unaligned((const u16 *)
> +				(dmi_data + DMI_PROCESSOR_MAX_SPEED));
> +		*mhz = val > *mhz ? val : *mhz;
> +	}
> +}
> +
> +/* Look up the max frequency in DMI */
> +static u64 get_dmi_max_mhz(void)
> +{
> +	u16 mhz = 0;
> +
> +	dmi_walk(find_dmi_mhz, &mhz);
> +
> +	/*
> +	 * Real stupid fallback value, just in case there is no
> +	 * actual value set.
> +	 */
> +	mhz = mhz ? mhz : 1;
> +
> +	return (u64)mhz;
> +}
> +
> +/* Helper function for the product info callback */
> +static char *copy_string_n(char *dst, char *table, int idx)
> +{
> +	char *d = dst;
> +	char *ptr = table;
> +	int ii;
> +
> +	/* skip the first idx-1 strings */
> +	for (ii = 1; ii < idx; ii++) {
> +		while (*ptr)
> +			ptr++;
> +		ptr++;
> +	}
> +
> +	/* copy in the string we need */
> +	while (*ptr && (d - dst) < (DMI_MAX_STRLEN - 2))
> +		*d++ = *ptr++;
> +
> +	return d;
> +}
> +
> +/* Callback function used to retrieve the product info DMI */
> +static void find_dmi_product_info(const struct dmi_header *dm, void *private)
> +{
> +	const u8 *dmi_data = (const u8 *)dm;
> +	char *ptr = (char *)private;
> +
> +	if (dm->type == DMI_ENTRY_BASEBOARD &&
> +	    dm->length >= DMI_ENTRY_BASEBOARD_MIN_LENGTH) {
> +		int idx;
> +
> +		idx = (int)get_unaligned((const u8 *)
> +				(dmi_data + DMI_BASEBOARD_MANUFACTURER));
> +		ptr = copy_string_n(ptr, (char *)(dmi_data + dm->length), idx);
> +		*ptr++ = ' ';
> +
> +		idx = (int)get_unaligned((const u8 *)
> +				(dmi_data + DMI_BASEBOARD_PRODUCT));
> +		ptr = copy_string_n(ptr, (char *)(dmi_data + dm->length), idx);
> +		*ptr++ = ' ';
> +
> +		idx = (int)get_unaligned((const u8 *)
> +				(dmi_data + DMI_BASEBOARD_VERSION));
> +		ptr = copy_string_n(ptr, (char *)(dmi_data + dm->length), idx);
> +	}
> +}
> +
> +/* Look up the baseboard info in DMI */
> +static void get_dmi_product_info(void)
> +{
> +	if (!dmi_product_info) {
> +		dmi_product_info = kcalloc(DMI_MAX_STRLEN,
> +					   sizeof(char), GFP_KERNEL);
> +		if (!dmi_product_info)
> +			return;
> +	}
> +
> +	dmi_walk(find_dmi_product_info, dmi_product_info);
> +}
> +
>  static int c_show(struct seq_file *m, void *v)
>  {
>  	int i, j;
> @@ -190,6 +298,31 @@ static int c_show(struct seq_file *m, void *v)
>  			seq_printf(m, "model name\t: ARMv8 Processor rev %d (%s)\n",
>  				   MIDR_REVISION(midr), COMPAT_ELF_PLATFORM);
>  
> +		if (IS_ENABLED(CONFIG_DMI)) {
> +			seq_puts(m, "product name\t: ");
> +
> +			if (!dmi_product_info)
> +				get_dmi_product_info();
> +			if (dmi_product_info)
> +				seq_printf(m, "%s", dmi_product_info);
> +			else
> +				seq_puts(m, "<unknown>");
> +
> +			seq_printf(m, ", ARM 8.%d (r%dp%d) CPU",
> +				   MIDR_VARIANT(midr),
> +				   MIDR_VARIANT(midr),
> +				   MIDR_REVISION(midr));
> +
> +			if (!dmi_max_mhz)
> +				dmi_max_mhz = get_dmi_max_mhz();
> +			if (dmi_max_mhz)
> +				seq_printf(m, " @ %d.%02dGHz\n",
> +					   (int)(dmi_max_mhz / 1000),
> +					   (int)(dmi_max_mhz % 1000));
> +			else
> +				seq_puts(m, " @ <unknown>GHz\n");
> +		}
> +
>  		impl = (u8) MIDR_IMPLEMENTOR(midr);
>  		for (j = 0; hw_implementer[j].id != 0; j++) {
>  			if (hw_implementer[j].id == impl) {
> @@ -208,7 +341,7 @@ static int c_show(struct seq_file *m, void *v)
>  		part = (u16) MIDR_PARTNUM(midr);
>  		for (j = 0; parts[j].id != (-1); j++) {
>  			if (parts[j].id == part) {
> -				seq_printf(m, "%s\n", parts[j].name);
> +				seq_printf(m, "%s ", parts[j].name);
>  				break;
>  			}
>  		}
> -- 
> 2.13.5
> 

Re: [PATCH 3/3] arm64: cpuinfo: display product info in /proc/cpuinfo

[Timur Tabi]

On Tue, Sep 26, 2017 at 5:23 PM, Al Stone <ahs3@redhat.com> wrote:
> +               if (IS_ENABLED(CONFIG_DMI)) {
> +                       seq_puts(m, "product name\t: ");
> +
> +                       if (!dmi_product_info)
> +                               get_dmi_product_info();
> +                       if (dmi_product_info)
> +                               seq_printf(m, "%s", dmi_product_info);

This line prints out like this:

product name    : QUALCOMM BASEBOARD ASSEMBLY, AMBERWIN
 20-P7989-H1S   , ARM 8.0 (r0p0) CPU @ 2.500GHz

Is this a bug in our DMI table?  'dmidecode' reports this:

Handle 0x0004, DMI type 2, 17 bytes
Base Board Information
        Manufacturer: QUALCOMM
        Product Name: BASEBOARD ASSEMBLY, AMBERWIN..
        Version: 20-P7989-H1S

I'm guessing that the actual string has a CR/LF at the end of "AMBERWIN"

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 3/3] arm64: cpuinfo: display product info in /proc/cpuinfo

[Timur Tabi]

On Tue, Sep 26, 2017 at 5:23 PM, Al Stone <ahs3@redhat.com> wrote:
> +               if (IS_ENABLED(CONFIG_DMI)) {
> +                       seq_puts(m, "product name\t: ");
> +
> +                       if (!dmi_product_info)
> +                               get_dmi_product_info();
> +                       if (dmi_product_info)
> +                               seq_printf(m, "%s", dmi_product_info);

This line prints out like this:

product name    : QUALCOMM BASEBOARD ASSEMBLY, AMBERWIN
 20-P7989-H1S   , ARM 8.0 (r0p0) CPU @ 2.500GHz

Is this a bug in our DMI table?  'dmidecode' reports this:

Handle 0x0004, DMI type 2, 17 bytes
Base Board Information
        Manufacturer: QUALCOMM
        Product Name: BASEBOARD ASSEMBLY, AMBERWIN..
        Version: 20-P7989-H1S

I'm guessing that the actual string has a CR/LF at the end of "AMBERWIN"

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH 0/3] arm64: cpuinfo: make /proc/cpuinfo more human-readable

[Mark Rutland]

Hi Al,

On Tue, Sep 26, 2017 at 04:23:21PM -0600, Al Stone wrote:
> As ARMv8 servers get deployed, I keep getting the same set of questions
> from end-users of those systems: what do all the hex numbers mean in 
> /proc/cpuinfo and could you make them so I don't have to carry a cheat
> sheet with me all the time?

I appreciate that /proc/cpuinfo can be opaque to end users, but I do not
believe that this is the right solution to that problem.

There are a number of issues stemming from the face that /proc/cpuinfo
is ill-defined and overused for a number of cases. Changes to it almost
certainly violate brittle de-facto ABI details people are relying on,
and there's a very long tail on fallout resulting from this. In
addition, many niceties come at an excessive maintenance cost, and are
simply unreliable as an ABI.

So, as with all other patches modifying /proc/cpuinfo, I must NAK this
series.

For the MPIDR and product info, I think we can expose this in a more
structured way (e.g. under sysfs). IIUC the product info is all derived
from DMI -- do we not expose that already?

For the human-readable names I must NAK such patches. This is an
extremely brittle ABI that cannot be forwards compatible, and comes with
hilarious political problems. This should be managed in userspace alone.

I thought tools like lscpu and lshw were intended to gather such
information for users. What are these missing today?

Thanks,
Mark.

[PATCH 0/3] arm64: cpuinfo: make /proc/cpuinfo more human-readable

[Mark Rutland]

Hi Al,

On Tue, Sep 26, 2017 at 04:23:21PM -0600, Al Stone wrote:
> As ARMv8 servers get deployed, I keep getting the same set of questions
> from end-users of those systems: what do all the hex numbers mean in 
> /proc/cpuinfo and could you make them so I don't have to carry a cheat
> sheet with me all the time?

I appreciate that /proc/cpuinfo can be opaque to end users, but I do not
believe that this is the right solution to that problem.

There are a number of issues stemming from the face that /proc/cpuinfo
is ill-defined and overused for a number of cases. Changes to it almost
certainly violate brittle de-facto ABI details people are relying on,
and there's a very long tail on fallout resulting from this. In
addition, many niceties come at an excessive maintenance cost, and are
simply unreliable as an ABI.

So, as with all other patches modifying /proc/cpuinfo, I must NAK this
series.

For the MPIDR and product info, I think we can expose this in a more
structured way (e.g. under sysfs). IIUC the product info is all derived
from DMI -- do we not expose that already?

For the human-readable names I must NAK such patches. This is an
extremely brittle ABI that cannot be forwards compatible, and comes with
hilarious political problems. This should be managed in userspace alone.

I thought tools like lscpu and lshw were intended to gather such
information for users. What are these missing today?

Thanks,
Mark.

Re: [PATCH 0/3] arm64: cpuinfo: make /proc/cpuinfo more human-readable

[Al Stone]

On 09/27/2017 04:34 AM, Mark Rutland wrote:
> On Tue, Sep 26, 2017 at 04:23:21PM -0600, Al Stone wrote:
>> As ARMv8 servers get deployed, I keep getting the same set of questions
>> from end-users of those systems: what do all the hex numbers mean in 
>> /proc/cpuinfo and could you make them so I don't have to carry a cheat
>> sheet with me all the time?
> 
> I appreciate that /proc/cpuinfo can be opaque to end users, but I do not
> believe that this is the right solution to that problem.
> 
> There are a number of issues stemming from the face that /proc/cpuinfo
> is ill-defined and overused for a number of cases. Changes to it almost
> certainly violate brittle de-facto ABI details people are relying on,
> and there's a very long tail on fallout resulting from this. In
> addition, many niceties come at an excessive maintenance cost, and are
> simply unreliable as an ABI.

Instead of dealing with the replies to the specific patches, I'd like to deal
with the real gist of the matter first -- details can be worked out later.

While I don't disagree -- cpuinfo is enormously fragile -- what _is_ a good
solution, then?  Right now, things are not terribly useful.  Yes, it is true
/sys/firmware/dmi has info in it, but again it is all numeric (or in a file
named 'raw').  Any one who wishes to read the values will need to put some
interpretation on them somehow.  My personal preference is that the kernel
control that interpretation instead of random admin tools scattered over a raft
of different companies.

> So, as with all other patches modifying /proc/cpuinfo, I must NAK this
> series.

Hrm.  I suggest that policy needs to be rethought.  I understand the reasoning,
but it puts my end-users in a very difficult position; they now have to justify
modifying management software that is already in operation in order to purchase
and integrate ARM servers.  And while I really don't want distro-specific code,
I'd have to consider it as a possibility.

To be completely fair, though, I'm not completely fixated on cpuinfo as the only
possible answer.  It has been the most requested, however.

> For the MPIDR and product info, I think we can expose this in a more
> structured way (e.g. under sysfs). IIUC the product info is all derived
> from DMI -- do we not expose that already?

Agreed; the MPIDR I think could just as easily be in /sys/devices/cpu/* as
a hex value.  It doesn't have the value to end-users that the other info does,
either -- for debugging, definitely, but not necessarily sysadmin.  I'll put
something together for this.

The DMI info is in sysfs but it is not in a fashion normally consumed, hence
the questions from end-users, in a way.  They're not expecting to have to do
the interpretation, and they are expecting the platform to be able to tell
them what "Model: 2" means.

> For the human-readable names I must NAK such patches. This is an
> extremely brittle ABI that cannot be forwards compatible, and comes with
> hilarious political problems. This should be managed in userspace alone.
> 
> I thought tools like lscpu and lshw were intended to gather such
> information for users. What are these missing today?

All of the above human-readable parts -- lscpu reads model information from
/proc/cpuinfo, for example, and would require new code to read this info from
somewhere else, and then figure out how to interpret it.  All I get is a lovely
'2' for 'model' :).  We could obviously change the userspace tools, but I'm
guessing it just moves the political hilarity from the kernel to userspace,
correct?  It also runs the risk of even greater silliness, like perhaps lscpu
gets a product name right, but lshw calls it something different, and so on.

The other approach that occurs to me is to blend what x86 does and sysfs; i.e.,
would it be acceptable to modify cputype.h to include comments (as x86 does)
that are then munged by a script somewhat like arch/x86/kernel/cpu/mkcapflags.sh
that then produces a header file with proper human-readable content that can
then be exposed in sysfs?  I don't think we can ever remove the brittleness
completely, but perhaps this would at least mitigate it.  This would still
require userspace modifications, too, but they could at least rely on a uniform
source of data -- and perhaps a single place to focus when hilarity ensues :).
I'll bodge some patches together and maybe start over as an RFC, if there's a
chance this would be accepted.

-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------

[PATCH 0/3] arm64: cpuinfo: make /proc/cpuinfo more human-readable

[Al Stone]

On 09/27/2017 04:34 AM, Mark Rutland wrote:
> On Tue, Sep 26, 2017 at 04:23:21PM -0600, Al Stone wrote:
>> As ARMv8 servers get deployed, I keep getting the same set of questions
>> from end-users of those systems: what do all the hex numbers mean in 
>> /proc/cpuinfo and could you make them so I don't have to carry a cheat
>> sheet with me all the time?
> 
> I appreciate that /proc/cpuinfo can be opaque to end users, but I do not
> believe that this is the right solution to that problem.
> 
> There are a number of issues stemming from the face that /proc/cpuinfo
> is ill-defined and overused for a number of cases. Changes to it almost
> certainly violate brittle de-facto ABI details people are relying on,
> and there's a very long tail on fallout resulting from this. In
> addition, many niceties come at an excessive maintenance cost, and are
> simply unreliable as an ABI.

Instead of dealing with the replies to the specific patches, I'd like to deal
with the real gist of the matter first -- details can be worked out later.

While I don't disagree -- cpuinfo is enormously fragile -- what _is_ a good
solution, then?  Right now, things are not terribly useful.  Yes, it is true
/sys/firmware/dmi has info in it, but again it is all numeric (or in a file
named 'raw').  Any one who wishes to read the values will need to put some
interpretation on them somehow.  My personal preference is that the kernel
control that interpretation instead of random admin tools scattered over a raft
of different companies.

> So, as with all other patches modifying /proc/cpuinfo, I must NAK this
> series.

Hrm.  I suggest that policy needs to be rethought.  I understand the reasoning,
but it puts my end-users in a very difficult position; they now have to justify
modifying management software that is already in operation in order to purchase
and integrate ARM servers.  And while I really don't want distro-specific code,
I'd have to consider it as a possibility.

To be completely fair, though, I'm not completely fixated on cpuinfo as the only
possible answer.  It has been the most requested, however.

> For the MPIDR and product info, I think we can expose this in a more
> structured way (e.g. under sysfs). IIUC the product info is all derived
> from DMI -- do we not expose that already?

Agreed; the MPIDR I think could just as easily be in /sys/devices/cpu/* as
a hex value.  It doesn't have the value to end-users that the other info does,
either -- for debugging, definitely, but not necessarily sysadmin.  I'll put
something together for this.

The DMI info is in sysfs but it is not in a fashion normally consumed, hence
the questions from end-users, in a way.  They're not expecting to have to do
the interpretation, and they are expecting the platform to be able to tell
them what "Model: 2" means.

> For the human-readable names I must NAK such patches. This is an
> extremely brittle ABI that cannot be forwards compatible, and comes with
> hilarious political problems. This should be managed in userspace alone.
> 
> I thought tools like lscpu and lshw were intended to gather such
> information for users. What are these missing today?

All of the above human-readable parts -- lscpu reads model information from
/proc/cpuinfo, for example, and would require new code to read this info from
somewhere else, and then figure out how to interpret it.  All I get is a lovely
'2' for 'model' :).  We could obviously change the userspace tools, but I'm
guessing it just moves the political hilarity from the kernel to userspace,
correct?  It also runs the risk of even greater silliness, like perhaps lscpu
gets a product name right, but lshw calls it something different, and so on.

The other approach that occurs to me is to blend what x86 does and sysfs; i.e.,
would it be acceptable to modify cputype.h to include comments (as x86 does)
that are then munged by a script somewhat like arch/x86/kernel/cpu/mkcapflags.sh
that then produces a header file with proper human-readable content that can
then be exposed in sysfs?  I don't think we can ever remove the brittleness
completely, but perhaps this would at least mitigate it.  This would still
require userspace modifications, too, but they could at least rely on a uniform
source of data -- and perhaps a single place to focus when hilarity ensues :).
I'll bodge some patches together and maybe start over as an RFC, if there's a
chance this would be accepted.

-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3 at redhat.com
-----------------------------------

Re: [PATCH 0/3] arm64: cpuinfo: make /proc/cpuinfo more human-readable

[Timur Tabi]

On Wed, Sep 27, 2017 at 5:34 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> Hi Al,
>
> On Tue, Sep 26, 2017 at 04:23:21PM -0600, Al Stone wrote:
>> As ARMv8 servers get deployed, I keep getting the same set of questions
>> from end-users of those systems: what do all the hex numbers mean in
>> /proc/cpuinfo and could you make them so I don't have to carry a cheat
>> sheet with me all the time?
>
> I appreciate that /proc/cpuinfo can be opaque to end users, but I do not
> believe that this is the right solution to that problem.
>
> There are a number of issues stemming from the face that /proc/cpuinfo
> is ill-defined and overused for a number of cases. Changes to it almost
> certainly violate brittle de-facto ABI details people are relying on,
> and there's a very long tail on fallout resulting from this. In
> addition, many niceties come at an excessive maintenance cost, and are
> simply unreliable as an ABI.
>
> So, as with all other patches modifying /proc/cpuinfo, I must NAK this
> series.

Qualcomm Datacenter Technologies is very interested in seeing these
patches (or some variant of them) accepted.  Updates to /proc/cpuinfo
are long overdue, and I'm asking you to reconsider your objections.
We're willing to work with distro vendors to get this information
added to their products while upstream is left behind, but I hope that
won't be necessary.

I would even go so far as to say that we should be making
/proc/cpuinfo for ARM match the x86 output as closely as possible,
even using their terminology.  We should be providing information like
frequencies and product names.

Having a human-readable /proc/cpuinfo with extensive details of the
CPU spelled out is very useful, and Al's reasoning is valid.  The fact
that it's "fragile" is not as important as the fact that on x86,
/proc/cpuinfo is much more useful.

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 0/3] arm64: cpuinfo: make /proc/cpuinfo more human-readable

[Timur Tabi]

On Wed, Sep 27, 2017 at 5:34 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> Hi Al,
>
> On Tue, Sep 26, 2017 at 04:23:21PM -0600, Al Stone wrote:
>> As ARMv8 servers get deployed, I keep getting the same set of questions
>> from end-users of those systems: what do all the hex numbers mean in
>> /proc/cpuinfo and could you make them so I don't have to carry a cheat
>> sheet with me all the time?
>
> I appreciate that /proc/cpuinfo can be opaque to end users, but I do not
> believe that this is the right solution to that problem.
>
> There are a number of issues stemming from the face that /proc/cpuinfo
> is ill-defined and overused for a number of cases. Changes to it almost
> certainly violate brittle de-facto ABI details people are relying on,
> and there's a very long tail on fallout resulting from this. In
> addition, many niceties come at an excessive maintenance cost, and are
> simply unreliable as an ABI.
>
> So, as with all other patches modifying /proc/cpuinfo, I must NAK this
> series.

Qualcomm Datacenter Technologies is very interested in seeing these
patches (or some variant of them) accepted.  Updates to /proc/cpuinfo
are long overdue, and I'm asking you to reconsider your objections.
We're willing to work with distro vendors to get this information
added to their products while upstream is left behind, but I hope that
won't be necessary.

I would even go so far as to say that we should be making
/proc/cpuinfo for ARM match the x86 output as closely as possible,
even using their terminology.  We should be providing information like
frequencies and product names.

Having a human-readable /proc/cpuinfo with extensive details of the
CPU spelled out is very useful, and Al's reasoning is valid.  The fact
that it's "fragile" is not as important as the fact that on x86,
/proc/cpuinfo is much more useful.

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH 0/3] arm64: cpuinfo: make /proc/cpuinfo more human-readable

[Mark Rutland]

Hi Timur,

On Fri, Oct 13, 2017 at 08:39:09AM -0500, Timur Tabi wrote:
> On Wed, Sep 27, 2017 at 5:34 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Tue, Sep 26, 2017 at 04:23:21PM -0600, Al Stone wrote:
> >> As ARMv8 servers get deployed, I keep getting the same set of questions
> >> from end-users of those systems: what do all the hex numbers mean in
> >> /proc/cpuinfo and could you make them so I don't have to carry a cheat
> >> sheet with me all the time?
> >
> > I appreciate that /proc/cpuinfo can be opaque to end users, but I do not
> > believe that this is the right solution to that problem.
> >
> > There are a number of issues stemming from the face that /proc/cpuinfo
> > is ill-defined and overused for a number of cases. Changes to it almost
> > certainly violate brittle de-facto ABI details people are relying on,
> > and there's a very long tail on fallout resulting from this. In
> > addition, many niceties come at an excessive maintenance cost, and are
> > simply unreliable as an ABI.
> >
> > So, as with all other patches modifying /proc/cpuinfo, I must NAK this
> > series.
> 
> Qualcomm Datacenter Technologies is very interested in seeing these
> patches (or some variant of them) accepted. Updates to /proc/cpuinfo
> are long overdue, and I'm asking you to reconsider your objections.

I more than appreciate that there is information that people want access
to, and I'd really like to expose that in a consistent and structured
manner.

So please propose exposing the information elsewhere, which I would be
more than happy with for information that the kernel already has access
to.

I object to /proc/cpuinfo changes because they impose an ABI break, and
because some of the proposed changes impose new and requirements on the
kernel that cannot be maintained in a forwards-compatible manner (which
is liable to result in other ABI breaks).

For better or worse, we're stuck with the existing arch-specific format
of /proc/cpuinfo, as this is parsed by a wide variety of applications,
and any change risks breaking these.

> We're willing to work with distro vendors to get this information
> added to their products while upstream is left behind, but I hope that
> won't be necessary.

Please understand that this is an ABI break, and that is why it is being
NAK'd.

I don't have the power to stop Red Hat or other vendors from
deliberately breaking ABI, but I would very strongly recommend that they
do not.

> I would even go so far as to say that we should be making
> /proc/cpuinfo for ARM match the x86 output as closely as possible,

As has been stated before, /proc/cpuinfo is an existing arch-specific ABI.

For better or worse, we're stuck with it as-is, regardless of what could
be nicer had we done something different from the outset.

It has never been portable across architectures, and applications
relying upon it have never been portable except by chance.

> even using their terminology.  We should be providing information like
> frequencies and product names.

As has been stated before, the problem with exposing these is that we
don't have the relevant information.

We have no consistent mechanism for acquiring product names, and for
those cases where it is truly necessary to identify an implementation,
the MIDR+REVIDR provide the exact same information.

For frequencies, I'd be more than happy to expose this information when
we have it, in new files. In practice today, we do not have this
information most of the time.

> Having a human-readable /proc/cpuinfo with extensive details of the
> CPU spelled out is very useful, and Al's reasoning is valid.  The fact
> that it's "fragile" is not as important as the fact that on x86,
> /proc/cpuinfo is much more useful.

I must disagree.

We care about that fragility so that we do not break userspace, which is
the most important upstream rule.

I certainly agree that exposing the information that we have is useful,
as I have stated several times. I'm not NAKing exposing this information
elsewhere.

If you want a consistent cross-architecture interface for this
information, then you need to propose a new one. That was we can
actually solve the underlying issues, for all architectures, without
breaking ABI.

I would be *very* interested in such an interface, and would be more
than happy to help.

Thanks,
Mark.

[PATCH 0/3] arm64: cpuinfo: make /proc/cpuinfo more human-readable

[Mark Rutland]

Hi Timur,

On Fri, Oct 13, 2017 at 08:39:09AM -0500, Timur Tabi wrote:
> On Wed, Sep 27, 2017 at 5:34 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Tue, Sep 26, 2017 at 04:23:21PM -0600, Al Stone wrote:
> >> As ARMv8 servers get deployed, I keep getting the same set of questions
> >> from end-users of those systems: what do all the hex numbers mean in
> >> /proc/cpuinfo and could you make them so I don't have to carry a cheat
> >> sheet with me all the time?
> >
> > I appreciate that /proc/cpuinfo can be opaque to end users, but I do not
> > believe that this is the right solution to that problem.
> >
> > There are a number of issues stemming from the face that /proc/cpuinfo
> > is ill-defined and overused for a number of cases. Changes to it almost
> > certainly violate brittle de-facto ABI details people are relying on,
> > and there's a very long tail on fallout resulting from this. In
> > addition, many niceties come at an excessive maintenance cost, and are
> > simply unreliable as an ABI.
> >
> > So, as with all other patches modifying /proc/cpuinfo, I must NAK this
> > series.
> 
> Qualcomm Datacenter Technologies is very interested in seeing these
> patches (or some variant of them) accepted. Updates to /proc/cpuinfo
> are long overdue, and I'm asking you to reconsider your objections.

I more than appreciate that there is information that people want access
to, and I'd really like to expose that in a consistent and structured
manner.

So please propose exposing the information elsewhere, which I would be
more than happy with for information that the kernel already has access
to.

I object to /proc/cpuinfo changes because they impose an ABI break, and
because some of the proposed changes impose new and requirements on the
kernel that cannot be maintained in a forwards-compatible manner (which
is liable to result in other ABI breaks).

For better or worse, we're stuck with the existing arch-specific format
of /proc/cpuinfo, as this is parsed by a wide variety of applications,
and any change risks breaking these.

> We're willing to work with distro vendors to get this information
> added to their products while upstream is left behind, but I hope that
> won't be necessary.

Please understand that this is an ABI break, and that is why it is being
NAK'd.

I don't have the power to stop Red Hat or other vendors from
deliberately breaking ABI, but I would very strongly recommend that they
do not.

> I would even go so far as to say that we should be making
> /proc/cpuinfo for ARM match the x86 output as closely as possible,

As has been stated before, /proc/cpuinfo is an existing arch-specific ABI.

For better or worse, we're stuck with it as-is, regardless of what could
be nicer had we done something different from the outset.

It has never been portable across architectures, and applications
relying upon it have never been portable except by chance.

> even using their terminology.  We should be providing information like
> frequencies and product names.

As has been stated before, the problem with exposing these is that we
don't have the relevant information.

We have no consistent mechanism for acquiring product names, and for
those cases where it is truly necessary to identify an implementation,
the MIDR+REVIDR provide the exact same information.

For frequencies, I'd be more than happy to expose this information when
we have it, in new files. In practice today, we do not have this
information most of the time.

> Having a human-readable /proc/cpuinfo with extensive details of the
> CPU spelled out is very useful, and Al's reasoning is valid.  The fact
> that it's "fragile" is not as important as the fact that on x86,
> /proc/cpuinfo is much more useful.

I must disagree.

We care about that fragility so that we do not break userspace, which is
the most important upstream rule.

I certainly agree that exposing the information that we have is useful,
as I have stated several times. I'm not NAKing exposing this information
elsewhere.

If you want a consistent cross-architecture interface for this
information, then you need to propose a new one. That was we can
actually solve the underlying issues, for all architectures, without
breaking ABI.

I would be *very* interested in such an interface, and would be more
than happy to help.

Thanks,
Mark.

Re: [PATCH 0/3] arm64: cpuinfo: make /proc/cpuinfo more human-readable

[Al Stone]

On 10/13/2017 08:27 AM, Mark Rutland wrote:
> Hi Timur,
> 
> On Fri, Oct 13, 2017 at 08:39:09AM -0500, Timur Tabi wrote:
>> On Wed, Sep 27, 2017 at 5:34 AM, Mark Rutland <mark.rutland@arm.com> wrote:
>>> On Tue, Sep 26, 2017 at 04:23:21PM -0600, Al Stone wrote:
>>>> As ARMv8 servers get deployed, I keep getting the same set of questions
>>>> from end-users of those systems: what do all the hex numbers mean in
>>>> /proc/cpuinfo and could you make them so I don't have to carry a cheat
>>>> sheet with me all the time?
>>>
>>> I appreciate that /proc/cpuinfo can be opaque to end users, but I do not
>>> believe that this is the right solution to that problem.
>>>
>>> There are a number of issues stemming from the face that /proc/cpuinfo
>>> is ill-defined and overused for a number of cases. Changes to it almost
>>> certainly violate brittle de-facto ABI details people are relying on,
>>> and there's a very long tail on fallout resulting from this. In
>>> addition, many niceties come at an excessive maintenance cost, and are
>>> simply unreliable as an ABI.
>>>
>>> So, as with all other patches modifying /proc/cpuinfo, I must NAK this
>>> series.
>>
>> Qualcomm Datacenter Technologies is very interested in seeing these
>> patches (or some variant of them) accepted. Updates to /proc/cpuinfo
>> are long overdue, and I'm asking you to reconsider your objections.
> 
> I more than appreciate that there is information that people want access
> to, and I'd really like to expose that in a consistent and structured
> manner.
> 
> So please propose exposing the information elsewhere, which I would be
> more than happy with for information that the kernel already has access
> to.
> 
> I object to /proc/cpuinfo changes because they impose an ABI break, and
> because some of the proposed changes impose new and requirements on the
> kernel that cannot be maintained in a forwards-compatible manner (which
> is liable to result in other ABI breaks).
> 
> For better or worse, we're stuck with the existing arch-specific format
> of /proc/cpuinfo, as this is parsed by a wide variety of applications,
> and any change risks breaking these.
> 
>> We're willing to work with distro vendors to get this information
>> added to their products while upstream is left behind, but I hope that
>> won't be necessary.
> 
> Please understand that this is an ABI break, and that is why it is being
> NAK'd.
> 
> I don't have the power to stop Red Hat or other vendors from
> deliberately breaking ABI, but I would very strongly recommend that they
> do not.
> 
>> I would even go so far as to say that we should be making
>> /proc/cpuinfo for ARM match the x86 output as closely as possible,
> 
> As has been stated before, /proc/cpuinfo is an existing arch-specific ABI.
> 
> For better or worse, we're stuck with it as-is, regardless of what could
> be nicer had we done something different from the outset.
> 
> It has never been portable across architectures, and applications
> relying upon it have never been portable except by chance.
> 
>> even using their terminology.  We should be providing information like
>> frequencies and product names.
> 
> As has been stated before, the problem with exposing these is that we
> don't have the relevant information.
> 
> We have no consistent mechanism for acquiring product names, and for
> those cases where it is truly necessary to identify an implementation,
> the MIDR+REVIDR provide the exact same information.
> 
> For frequencies, I'd be more than happy to expose this information when
> we have it, in new files. In practice today, we do not have this
> information most of the time.
> 
>> Having a human-readable /proc/cpuinfo with extensive details of the
>> CPU spelled out is very useful, and Al's reasoning is valid.  The fact
>> that it's "fragile" is not as important as the fact that on x86,
>> /proc/cpuinfo is much more useful.
> 
> I must disagree.
> 
> We care about that fragility so that we do not break userspace, which is
> the most important upstream rule.
> 
> I certainly agree that exposing the information that we have is useful,
> as I have stated several times. I'm not NAKing exposing this information
> elsewhere.
> 
> If you want a consistent cross-architecture interface for this
> information, then you need to propose a new one. That was we can
> actually solve the underlying issues, for all architectures, without
> breaking ABI.
> 
> I would be *very* interested in such an interface, and would be more
> than happy to help.

I'm playing with some patches that do very similar things in sysfs, vs
proc.  Is that better :)?  Obviously, you'll have to see the patches to
properly answer that, but what I'm playing with at present is placing
this info in new entries in /sys/devices/cpu and/or /sys/devices/system,
and generating some of the content based on what's already in header files
(e.g., in cputype.h).  The idea of course is to keep this new info from
touching any existing info so we don't break compatibility -- does that
feel like a better direction, at least?

> Thanks,
> Mark.
> 


-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------

[PATCH 0/3] arm64: cpuinfo: make /proc/cpuinfo more human-readable

[Al Stone]

On 10/13/2017 08:27 AM, Mark Rutland wrote:
> Hi Timur,
> 
> On Fri, Oct 13, 2017 at 08:39:09AM -0500, Timur Tabi wrote:
>> On Wed, Sep 27, 2017 at 5:34 AM, Mark Rutland <mark.rutland@arm.com> wrote:
>>> On Tue, Sep 26, 2017 at 04:23:21PM -0600, Al Stone wrote:
>>>> As ARMv8 servers get deployed, I keep getting the same set of questions
>>>> from end-users of those systems: what do all the hex numbers mean in
>>>> /proc/cpuinfo and could you make them so I don't have to carry a cheat
>>>> sheet with me all the time?
>>>
>>> I appreciate that /proc/cpuinfo can be opaque to end users, but I do not
>>> believe that this is the right solution to that problem.
>>>
>>> There are a number of issues stemming from the face that /proc/cpuinfo
>>> is ill-defined and overused for a number of cases. Changes to it almost
>>> certainly violate brittle de-facto ABI details people are relying on,
>>> and there's a very long tail on fallout resulting from this. In
>>> addition, many niceties come at an excessive maintenance cost, and are
>>> simply unreliable as an ABI.
>>>
>>> So, as with all other patches modifying /proc/cpuinfo, I must NAK this
>>> series.
>>
>> Qualcomm Datacenter Technologies is very interested in seeing these
>> patches (or some variant of them) accepted. Updates to /proc/cpuinfo
>> are long overdue, and I'm asking you to reconsider your objections.
> 
> I more than appreciate that there is information that people want access
> to, and I'd really like to expose that in a consistent and structured
> manner.
> 
> So please propose exposing the information elsewhere, which I would be
> more than happy with for information that the kernel already has access
> to.
> 
> I object to /proc/cpuinfo changes because they impose an ABI break, and
> because some of the proposed changes impose new and requirements on the
> kernel that cannot be maintained in a forwards-compatible manner (which
> is liable to result in other ABI breaks).
> 
> For better or worse, we're stuck with the existing arch-specific format
> of /proc/cpuinfo, as this is parsed by a wide variety of applications,
> and any change risks breaking these.
> 
>> We're willing to work with distro vendors to get this information
>> added to their products while upstream is left behind, but I hope that
>> won't be necessary.
> 
> Please understand that this is an ABI break, and that is why it is being
> NAK'd.
> 
> I don't have the power to stop Red Hat or other vendors from
> deliberately breaking ABI, but I would very strongly recommend that they
> do not.
> 
>> I would even go so far as to say that we should be making
>> /proc/cpuinfo for ARM match the x86 output as closely as possible,
> 
> As has been stated before, /proc/cpuinfo is an existing arch-specific ABI.
> 
> For better or worse, we're stuck with it as-is, regardless of what could
> be nicer had we done something different from the outset.
> 
> It has never been portable across architectures, and applications
> relying upon it have never been portable except by chance.
> 
>> even using their terminology.  We should be providing information like
>> frequencies and product names.
> 
> As has been stated before, the problem with exposing these is that we
> don't have the relevant information.
> 
> We have no consistent mechanism for acquiring product names, and for
> those cases where it is truly necessary to identify an implementation,
> the MIDR+REVIDR provide the exact same information.
> 
> For frequencies, I'd be more than happy to expose this information when
> we have it, in new files. In practice today, we do not have this
> information most of the time.
> 
>> Having a human-readable /proc/cpuinfo with extensive details of the
>> CPU spelled out is very useful, and Al's reasoning is valid.  The fact
>> that it's "fragile" is not as important as the fact that on x86,
>> /proc/cpuinfo is much more useful.
> 
> I must disagree.
> 
> We care about that fragility so that we do not break userspace, which is
> the most important upstream rule.
> 
> I certainly agree that exposing the information that we have is useful,
> as I have stated several times. I'm not NAKing exposing this information
> elsewhere.
> 
> If you want a consistent cross-architecture interface for this
> information, then you need to propose a new one. That was we can
> actually solve the underlying issues, for all architectures, without
> breaking ABI.
> 
> I would be *very* interested in such an interface, and would be more
> than happy to help.

I'm playing with some patches that do very similar things in sysfs, vs
proc.  Is that better :)?  Obviously, you'll have to see the patches to
properly answer that, but what I'm playing with at present is placing
this info in new entries in /sys/devices/cpu and/or /sys/devices/system,
and generating some of the content based on what's already in header files
(e.g., in cputype.h).  The idea of course is to keep this new info from
touching any existing info so we don't break compatibility -- does that
feel like a better direction, at least?

> Thanks,
> Mark.
> 


-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3 at redhat.com
-----------------------------------

Re: [PATCH 0/3] arm64: cpuinfo: make /proc/cpuinfo more human-readable

[Mark Rutland]

Hi Al,

On Mon, Oct 16, 2017 at 05:43:19PM -0600, Al Stone wrote:
> On 10/13/2017 08:27 AM, Mark Rutland wrote:
> > I certainly agree that exposing the information that we have is useful,
> > as I have stated several times. I'm not NAKing exposing this information
> > elsewhere.
> > 
> > If you want a consistent cross-architecture interface for this
> > information, then you need to propose a new one. That was we can
> > actually solve the underlying issues, for all architectures, without
> > breaking ABI.
> > 
> > I would be *very* interested in such an interface, and would be more
> > than happy to help.
> 
> I'm playing with some patches that do very similar things in sysfs, vs
> proc.  Is that better :)?

Exposing data under sysfs is certainly better, yes. :)

> Obviously, you'll have to see the patches to
> properly answer that, but what I'm playing with at present is placing
> this info in new entries in /sys/devices/cpu and/or /sys/devices/system,
> and generating some of the content based on what's already in header files
> (e.g., in cputype.h). 

My opposition to MIDR -> string mapping applies regardless of
location...

> The idea of course is to keep this new info from touching any existing
> info so we don't break compatibility -- does that feel like a better
> direction, at least?

... but otherwise this sounds good to me!

Thanks,
Mark.

[PATCH 0/3] arm64: cpuinfo: make /proc/cpuinfo more human-readable

[Mark Rutland]

Hi Al,

On Mon, Oct 16, 2017 at 05:43:19PM -0600, Al Stone wrote:
> On 10/13/2017 08:27 AM, Mark Rutland wrote:
> > I certainly agree that exposing the information that we have is useful,
> > as I have stated several times. I'm not NAKing exposing this information
> > elsewhere.
> > 
> > If you want a consistent cross-architecture interface for this
> > information, then you need to propose a new one. That was we can
> > actually solve the underlying issues, for all architectures, without
> > breaking ABI.
> > 
> > I would be *very* interested in such an interface, and would be more
> > than happy to help.
> 
> I'm playing with some patches that do very similar things in sysfs, vs
> proc.  Is that better :)?

Exposing data under sysfs is certainly better, yes. :)

> Obviously, you'll have to see the patches to
> properly answer that, but what I'm playing with at present is placing
> this info in new entries in /sys/devices/cpu and/or /sys/devices/system,
> and generating some of the content based on what's already in header files
> (e.g., in cputype.h). 

My opposition to MIDR -> string mapping applies regardless of
location...

> The idea of course is to keep this new info from touching any existing
> info so we don't break compatibility -- does that feel like a better
> direction, at least?

... but otherwise this sounds good to me!

Thanks,
Mark.

Re: [PATCH 0/3] arm64: cpuinfo: make /proc/cpuinfo more human-readable

[Jon Masters]

Hi Mark,

On 10/20/2017 12:10 PM, Mark Rutland wrote:

> On Mon, Oct 16, 2017 at 05:43:19PM -0600, Al Stone wrote:

>> Obviously, you'll have to see the patches to
>> properly answer that, but what I'm playing with at present is placing
>> this info in new entries in /sys/devices/cpu and/or /sys/devices/system,
>> and generating some of the content based on what's already in header files
>> (e.g., in cputype.h). 
> 
> My opposition to MIDR -> string mapping applies regardless of
> location...

Well, we do need to have the name to display to the user. Two things
that absolutely need to be in a clear, easy to find place:

1). The make and model of the CPU in human readable format
2). The nominal and operating frequency of the CPU(s)

Obviously, I want this to be in /proc/cpuinfo, but if it goes elsewhere
(and other arches get behind it), then that could work. For the mapping
of name to model, I agree that hard coding stuff in the kernel is ugly.
This means that we need to either leverage DMI/SMBIOS data or add a new
API to something like PSCI (ugly, maybe dangerous) to get the info.

Here are some reasons that I care:

1). The first thing people do when they get an Arm server is to cat
/proc/cpuinfo. They then come complaining that it's not like x86. They
can't get the output their looking for and this results in bug filing,
and countless hours on phone calls discussing over and over again.
Worse, there are some parts of the stack that really need this.

2). I have seen entire Arm server SoCs get a bad review because the
person running some test couldn't get the frequency reported in a
sensible enough place that they noticed it was an early part clocked at
half the frequency. Today, they have little opportunity to notice the
frequency and a lot of chance to start spreading rumors that some part
"sucks". Sure, bogomips, other numbers are all lies. But if I had a
penny for every time someone said "man, this server only runs at 100/400
MHz! that's so slow!" (arch timer)

So we're going to clean this up. I've spoken with all of the vendors and
there's general agreement that this needs changing for Enterprise users.
But how it's done - definitely there's flexibility there. One thing that
we won't do is have no solution. While it would be very unfortunate to
carry something just in RHEL, we'll do it (hopefully in collaboration
with other distros) if this isn't resolvable upstream.

Thanks,

Jon.

-- 
Computer Architect | Sent from my Fedora powered laptop

[PATCH 0/3] arm64: cpuinfo: make /proc/cpuinfo more human-readable

[Jon Masters]

Hi Mark,

On 10/20/2017 12:10 PM, Mark Rutland wrote:

> On Mon, Oct 16, 2017 at 05:43:19PM -0600, Al Stone wrote:

>> Obviously, you'll have to see the patches to
>> properly answer that, but what I'm playing with at present is placing
>> this info in new entries in /sys/devices/cpu and/or /sys/devices/system,
>> and generating some of the content based on what's already in header files
>> (e.g., in cputype.h). 
> 
> My opposition to MIDR -> string mapping applies regardless of
> location...

Well, we do need to have the name to display to the user. Two things
that absolutely need to be in a clear, easy to find place:

1). The make and model of the CPU in human readable format
2). The nominal and operating frequency of the CPU(s)

Obviously, I want this to be in /proc/cpuinfo, but if it goes elsewhere
(and other arches get behind it), then that could work. For the mapping
of name to model, I agree that hard coding stuff in the kernel is ugly.
This means that we need to either leverage DMI/SMBIOS data or add a new
API to something like PSCI (ugly, maybe dangerous) to get the info.

Here are some reasons that I care:

1). The first thing people do when they get an Arm server is to cat
/proc/cpuinfo. They then come complaining that it's not like x86. They
can't get the output their looking for and this results in bug filing,
and countless hours on phone calls discussing over and over again.
Worse, there are some parts of the stack that really need this.

2). I have seen entire Arm server SoCs get a bad review because the
person running some test couldn't get the frequency reported in a
sensible enough place that they noticed it was an early part clocked at
half the frequency. Today, they have little opportunity to notice the
frequency and a lot of chance to start spreading rumors that some part
"sucks". Sure, bogomips, other numbers are all lies. But if I had a
penny for every time someone said "man, this server only runs at 100/400
MHz! that's so slow!" (arch timer)

So we're going to clean this up. I've spoken with all of the vendors and
there's general agreement that this needs changing for Enterprise users.
But how it's done - definitely there's flexibility there. One thing that
we won't do is have no solution. While it would be very unfortunate to
carry something just in RHEL, we'll do it (hopefully in collaboration
with other distros) if this isn't resolvable upstream.

Thanks,

Jon.

-- 
Computer Architect | Sent from my Fedora powered laptop

Re: [PATCH 0/3] arm64: cpuinfo: make /proc/cpuinfo more human-readable

[Jon Masters]

On 10/20/2017 01:24 PM, Jon Masters wrote:

> 1). The first thing people do when they get an Arm server is to cat
> /proc/cpuinfo. They then come complaining that it's not like x86. They
> can't get the output their looking for and this results in bug filing,
> and countless hours on phone calls discussing over and over again.
> Worse, there are some parts of the stack that really need this.

Within 6 hours of sending this, I get a ping about this week's "Works On
Arm" newsletter and...people reporting bugs with not getting CPU
capabilities in /proc/cpuinfo. This madness is going to end. Soon.

Jon.

-- 
Computer Architect | Sent from my Fedora powered laptop

[PATCH 0/3] arm64: cpuinfo: make /proc/cpuinfo more human-readable

[Jon Masters]

On 10/20/2017 01:24 PM, Jon Masters wrote:

> 1). The first thing people do when they get an Arm server is to cat
> /proc/cpuinfo. They then come complaining that it's not like x86. They
> can't get the output their looking for and this results in bug filing,
> and countless hours on phone calls discussing over and over again.
> Worse, there are some parts of the stack that really need this.

Within 6 hours of sending this, I get a ping about this week's "Works On
Arm" newsletter and...people reporting bugs with not getting CPU
capabilities in /proc/cpuinfo. This madness is going to end. Soon.

Jon.

-- 
Computer Architect | Sent from my Fedora powered laptop

Re: [PATCH 0/3] arm64: cpuinfo: make /proc/cpuinfo more human-readable

[Al Stone]

On 10/20/2017 10:10 AM, Mark Rutland wrote:
> Hi Al,
> 
> On Mon, Oct 16, 2017 at 05:43:19PM -0600, Al Stone wrote:
>> On 10/13/2017 08:27 AM, Mark Rutland wrote:
>>> I certainly agree that exposing the information that we have is useful,
>>> as I have stated several times. I'm not NAKing exposing this information
>>> elsewhere.
>>>
>>> If you want a consistent cross-architecture interface for this
>>> information, then you need to propose a new one. That was we can
>>> actually solve the underlying issues, for all architectures, without
>>> breaking ABI.
>>>
>>> I would be *very* interested in such an interface, and would be more
>>> than happy to help.
>>
>> I'm playing with some patches that do very similar things in sysfs, vs
>> proc.  Is that better :)?
> 
> Exposing data under sysfs is certainly better, yes. :)
> 
>> Obviously, you'll have to see the patches to
>> properly answer that, but what I'm playing with at present is placing
>> this info in new entries in /sys/devices/cpu and/or /sys/devices/system,
>> and generating some of the content based on what's already in header files
>> (e.g., in cputype.h). 
> 
> My opposition to MIDR -> string mapping applies regardless of
> location...

Harumph.  This is the one thing I get asked for most often, however (second most
is frequency).  It turns out humans are not nearly as good at indexed lookups as
computers, hence the requests.

Whatever the root of the opposition is, it needs to get fixed.  My fear is that
if it doesn't get fixed in the firmware or the kernel, it will get fixed in some
far messier, less controllable way somewhere else (more likely several somewhere
elses) and just exacerbate the problem.

>> The idea of course is to keep this new info from touching any existing
>> info so we don't break compatibility -- does that feel like a better
>> direction, at least?
> 
> ... but otherwise this sounds good to me!
> 
> Thanks,
> Mark.
> 


-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------

[PATCH 0/3] arm64: cpuinfo: make /proc/cpuinfo more human-readable

[Al Stone]

On 10/20/2017 10:10 AM, Mark Rutland wrote:
> Hi Al,
> 
> On Mon, Oct 16, 2017 at 05:43:19PM -0600, Al Stone wrote:
>> On 10/13/2017 08:27 AM, Mark Rutland wrote:
>>> I certainly agree that exposing the information that we have is useful,
>>> as I have stated several times. I'm not NAKing exposing this information
>>> elsewhere.
>>>
>>> If you want a consistent cross-architecture interface for this
>>> information, then you need to propose a new one. That was we can
>>> actually solve the underlying issues, for all architectures, without
>>> breaking ABI.
>>>
>>> I would be *very* interested in such an interface, and would be more
>>> than happy to help.
>>
>> I'm playing with some patches that do very similar things in sysfs, vs
>> proc.  Is that better :)?
> 
> Exposing data under sysfs is certainly better, yes. :)
> 
>> Obviously, you'll have to see the patches to
>> properly answer that, but what I'm playing with at present is placing
>> this info in new entries in /sys/devices/cpu and/or /sys/devices/system,
>> and generating some of the content based on what's already in header files
>> (e.g., in cputype.h). 
> 
> My opposition to MIDR -> string mapping applies regardless of
> location...

Harumph.  This is the one thing I get asked for most often, however (second most
is frequency).  It turns out humans are not nearly as good at indexed lookups as
computers, hence the requests.

Whatever the root of the opposition is, it needs to get fixed.  My fear is that
if it doesn't get fixed in the firmware or the kernel, it will get fixed in some
far messier, less controllable way somewhere else (more likely several somewhere
elses) and just exacerbate the problem.

>> The idea of course is to keep this new info from touching any existing
>> info so we don't break compatibility -- does that feel like a better
>> direction, at least?
> 
> ... but otherwise this sounds good to me!
> 
> Thanks,
> Mark.
> 


-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3 at redhat.com
-----------------------------------