diff mbox

[PATCHv4,5/5] arm64: cpuinfo: print info for all CPUs

Message ID 20140718135738.GA17328@leverpostej
State New
Headers show

Commit Message

Mark Rutland July 18, 2014, 1:57 p.m. UTC 
On Fri, Jul 18, 2014 at 10:53:12AM +0100, Will Deacon wrote:
> On Fri, Jul 18, 2014 at 10:27:44AM +0100, Catalin Marinas wrote:
> > On Thu, Jul 17, 2014 at 06:28:58PM +0100, Will Deacon wrote:
> > > On Thu, Jul 17, 2014 at 06:10:58PM +0100, Catalin Marinas wrote:
> > > > On Thu, Jul 17, 2014 at 02:55:37PM +0100, Peter Maydell wrote:
> > > > > On 17 July 2014 13:35, Will Deacon <will.deacon@arm.com> wrote:
> > > > > > We're not denying the possibility of heterogeneity, we're trying to expose a
> > > > > > consistent view of the system to userspace. Differences between cores should
> > > > > > be dealt with by the kernel (e.g. IKS, HMP scheduling), not blindly
> > > > > > passed off to userspace.
> > > > > 
> > > > > On that basis, why report anything at all about invididual cores?
> > > > > Just have /proc/cpuinfo report "number of processors: 4" and
> > > > > no per-CPU information at all...
> > > > 
> > > > We lost a lot of time on this already (given the internal threads). So
> > > > my proposal is to go ahead with Mark's patch with per-CPU features. They
> > > > currently just include the same elf_hwcap multiple times. If we ever
> > > > need to present different features, the conditions would be:
> > > > 
> > > > 1. Never report more than elf_hwcap
> > > > 2. elf_hwcap can only include non-symmetric features *if* Linux gets a
> > > >    way to transparently handle migration or emulation
> > > 
> > > ... making the point of a per-cpu field entirely pointless ;)
> > 
> > Well, if we can support such features in a transparent way,
> > /proc/cpuinfo becomes more informative (e.g. user wondering why a
> > process runs only on certain CPUs).
> > 
> > But to be clear (and I think we are aligned), I don't trust user space
> > to parse all processors in /proc/cpuinfo and make an informed selection
> > of CPU affinity to avoid SIGILL.
> > 
> > Yet another option would be to have a single features/hwcap line and
> > present the extra features in a human (and only human) readable form
> > (e.g. some haiku that changes with every kernel release ;)).
> 
> Or just have the single features line, then the per-cpu line can be called
> `flags' or something, like Ard suggested. If userspace decides to parse
> flags, it deserves all the pain that it gets.

Ok. I think retaining the current (common) features line makes sense if
it's clearly separated from any particular CPU. If we need per-cpu
information later, this can be in addition to the common line.

That should enable software which only does mildy crazy things with
values from /proc/cpuinfo to work even if we print per-cpu information.
Anything trying to handle per-cpu differences will need rework
regardless, and that discussion can be had when we actually have crazily
heterogeneous systems.

Therefore, how about the below?

Thanks,
Mark.

---->8----
From 7dc7b5e5c4a54f56a5a7e59d2dd0de009dc9b9d0 Mon Sep 17 00:00:00 2001
From: Mark Rutland <mark.rutland@arm.com>
Date: Thu, 26 Jun 2014 16:18:44 +0100
Subject: [PATCH] arm64: cpuinfo: print info for all CPUs

Currently reading /proc/cpuinfo will result in information being read
out of the MIDR_EL1 of the current CPU, and the information is not
associated with any particular logical CPU number.

This is problematic for systems with heterogeneous CPUs (i.e.
big.LITTLE) where MIDR fields will vary across CPUs, and the output will
differ depending on the executing CPU.

This patch reorganises the code responsible for /proc/cpuinfo to print
information per-cpu. In the process, we perform several cleanups:

* Property names are coerced to lower-case (to match "processor" as per
  glibc's expectations).
* Property names are simplified and made to match the MIDR field names.
* Revision is changed to hex as with every other field.
* The meaningless Architecture property is removed.
* The ripe-for-abuse Machine field is removed.

The features field (a human-readable representation of the hwcaps)
remains printed once, as this is expected to remain in use as the
globally support CPU features. To enable the possibility of the addition
of per-cpu HW feature information later, this is printed before any
CPU-specific information.

Comments are added to guide userspace developers in the right direction
(using the hwcaps provided in auxval). Hopefully where userspace
applications parse /proc/cpuinfo rather than using the readily available
hwcaps, they limit themselves to reading said first line.

If CPU features differ from each other, the previously installed sanity
checks will give us some advance notice with warnings and
TAINT_CPU_OUT_OF_SPEC. If we are lucky, we will never see such systems.
Rework will be required in many places to support such systems anyway.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Marcus Shawcroft <marcus.shawcroft@arm.com>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Will Deacon <will.deacon@arm.com>
---
 arch/arm64/kernel/setup.c | 37 ++++++++++++++++++-------------------
 1 file changed, 18 insertions(+), 19 deletions(-)

Comments

Peter Maydell July 18, 2014, 3:52 p.m. UTC | #1 
On 18 July 2014 14:57, Mark Rutland <mark.rutland@arm.com> wrote:
> Comments are added to guide userspace developers in the right direction
> (using the hwcaps provided in auxval). Hopefully where userspace
> applications parse /proc/cpuinfo rather than using the readily available
> hwcaps, they limit themselves to reading said first line.

Comments in the kernel sources aren't going to guide
anybody except kernel developers. I was expecting from
this commit message that you were going to emit actual
comments in /proc/cpuinfo...

-- PMM
Mark Rutland July 18, 2014, 3:58 p.m. UTC | #2 
On Fri, Jul 18, 2014 at 04:52:37PM +0100, Peter Maydell wrote:
> On 18 July 2014 14:57, Mark Rutland <mark.rutland@arm.com> wrote:
> > Comments are added to guide userspace developers in the right direction
> > (using the hwcaps provided in auxval). Hopefully where userspace
> > applications parse /proc/cpuinfo rather than using the readily available
> > hwcaps, they limit themselves to reading said first line.
> 
> Comments in the kernel sources aren't going to guide
> anybody except kernel developers.

That's not entirely true, some people skim the kernel sources to figure
out how they're meant to use syscalls and such (though admitedly this
isn't all that common).

> I was expecting from this commit message that you were going to emit
> actual comments in /proc/cpuinfo...

I don't think that's a good idea, and I can only see that reading when I
squint quite hard. ;)

Thanks,
Mark
Peter Maydell July 18, 2014, 4:18 p.m. UTC | #3 
On 18 July 2014 16:58, Mark Rutland <mark.rutland@arm.com> wrote:
> On Fri, Jul 18, 2014 at 04:52:37PM +0100, Peter Maydell wrote:
>> Comments in the kernel sources aren't going to guide
>> anybody except kernel developers.
>
> That's not entirely true, some people skim the kernel sources to figure
> out how they're meant to use syscalls and such (though admitedly this
> isn't all that common).

Every time anybody has to do that it means you've failed to document
something...

>> I was expecting from this commit message that you were going to emit
>> actual comments in /proc/cpuinfo...
>
> I don't think that's a good idea, and I can only see that reading when I
> squint quite hard. ;)

You have to admit it would put the documentation right where
the people looking at cpuinfo can find it :-)

How about a patch to Documentation/filesystems/proc.txt ?

thanks
-- PMM
Mark Rutland July 18, 2014, 4:41 p.m. UTC | #4 
On Fri, Jul 18, 2014 at 05:18:27PM +0100, Peter Maydell wrote:
> On 18 July 2014 16:58, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Fri, Jul 18, 2014 at 04:52:37PM +0100, Peter Maydell wrote:
> >> Comments in the kernel sources aren't going to guide
> >> anybody except kernel developers.
> >
> > That's not entirely true, some people skim the kernel sources to figure
> > out how they're meant to use syscalls and such (though admitedly this
> > isn't all that common).
> 
> Every time anybody has to do that it means you've failed to document
> something...

...and in this case, the thing to document is the hwcaps.

> >> I was expecting from this commit message that you were going to emit
> >> actual comments in /proc/cpuinfo...
> >
> > I don't think that's a good idea, and I can only see that reading when I
> > squint quite hard. ;)
> 
> You have to admit it would put the documentation right where
> the people looking at cpuinfo can find it :-)

Sure :)

> How about a patch to Documentation/filesystems/proc.txt ?

Currently there seems to be a single relevant line, and it doesn't seem
to be up-to-date for SMP:

	cpuinfo     Info about the CPU

It might make sense to have something under Documentation/arm64, but I
don't know what precisely.

Cheers,
Mark.
Christopher Covington July 18, 2014, 8:24 p.m. UTC | #5 
On 07/18/2014 12:41 PM, Mark Rutland wrote:
> On Fri, Jul 18, 2014 at 05:18:27PM +0100, Peter Maydell wrote:
>> On 18 July 2014 16:58, Mark Rutland <mark.rutland@arm.com> wrote:
>>> On Fri, Jul 18, 2014 at 04:52:37PM +0100, Peter Maydell wrote:
>>>> Comments in the kernel sources aren't going to guide
>>>> anybody except kernel developers.
>>>
>>> That's not entirely true, some people skim the kernel sources to figure
>>> out how they're meant to use syscalls and such (though admitedly this
>>> isn't all that common).
>>
>> Every time anybody has to do that it means you've failed to document
>> something...
> 
> ...and in this case, the thing to document is the hwcaps.
> 
>>>> I was expecting from this commit message that you were going to emit
>>>> actual comments in /proc/cpuinfo...
>>>
>>> I don't think that's a good idea, and I can only see that reading when I
>>> squint quite hard. ;)
>>
>> You have to admit it would put the documentation right where
>> the people looking at cpuinfo can find it :-)
> 
> Sure :)
> 
>> How about a patch to Documentation/filesystems/proc.txt ?
> 
> Currently there seems to be a single relevant line, and it doesn't seem
> to be up-to-date for SMP:
> 
> 	cpuinfo     Info about the CPU
> 
> It might make sense to have something under Documentation/arm64, but I
> don't know what precisely.

If you're looking to communicate information to users I would highly recommend
including an update to the Linux man page.

http://man7.org/linux/man-pages/man5/proc.5.html

https://www.kernel.org/doc/man-pages/contributing.html

Regards,
Christopher
diff mbox

Patch

diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
index edb146d..aaf1ddb 100644
--- a/arch/arm64/kernel/setup.c
+++ b/arch/arm64/kernel/setup.c
@@ -450,10 +450,21 @@  static int c_show(struct seq_file *m, void *v)
 {
 	int i;
 
-	seq_printf(m, "Processor\t: %s rev %d (%s)\n",
-		   cpu_name, read_cpuid_id() & 15, ELF_PLATFORM);
+	/*
+	 * Dump out the common processor features in a single line. Userspace
+	 * should read the hwcaps with getauxval(AT_HWCAP) rather than
+	 * attempting to parse this.
+	 */
+	seq_puts(m, "features\t:");
+	for (i = 0; hwcap_str[i]; i++)
+		if (elf_hwcap & (1 << i))
+			seq_printf(m, " %s", hwcap_str[i]);
+	seq_puts(m, "\n\n");
 
 	for_each_online_cpu(i) {
+		struct cpuinfo_arm64 *cpuinfo = &per_cpu(cpu_data, i);
+		u32 midr = cpuinfo->reg_midr;
+
 		/*
 		 * glibc reads /proc/cpuinfo to determine the number of
 		 * online processors, looking for lines beginning with
@@ -462,25 +473,13 @@  static int c_show(struct seq_file *m, void *v)
 #ifdef CONFIG_SMP
 		seq_printf(m, "processor\t: %d\n", i);
 #endif
+		seq_printf(m, "implementer\t: 0x%02x\n",
+			   MIDR_IMPLEMENTOR(midr));
+		seq_printf(m, "variant\t\t: 0x%x\n", MIDR_VARIANT(midr));
+		seq_printf(m, "partnum\t\t: 0x%03x\n", MIDR_PARTNUM(midr));
+		seq_printf(m, "revision\t: 0x%x\n\n", MIDR_REVISION(midr));
 	}
 
-	/* dump out the processor features */
-	seq_puts(m, "Features\t: ");
-
-	for (i = 0; hwcap_str[i]; i++)
-		if (elf_hwcap & (1 << i))
-			seq_printf(m, "%s ", hwcap_str[i]);
-
-	seq_printf(m, "\nCPU implementer\t: 0x%02x\n", read_cpuid_id() >> 24);
-	seq_printf(m, "CPU architecture: AArch64\n");
-	seq_printf(m, "CPU variant\t: 0x%x\n", (read_cpuid_id() >> 20) & 15);
-	seq_printf(m, "CPU part\t: 0x%03x\n", (read_cpuid_id() >> 4) & 0xfff);
-	seq_printf(m, "CPU revision\t: %d\n", read_cpuid_id() & 15);
-
-	seq_puts(m, "\n");
-
-	seq_printf(m, "Hardware\t: %s\n", machine_name);
-
 	return 0;
 }