| Message ID | 20180715035342.11371-1-olof@lixom.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Olof, On 15/07/18 04:53, Olof Johansson wrote: > There's some use in printing out what the implementer and part numbers > decode to for cases where they're known. > > I filled in the table based on public information; mostly from ARM TRMs > and other tools (and some of the SSBD tables in the kernel, etc). > > Apple IDs came from > https://github.com/apple/darwin-xnu/blob/master/osfmk/arm/cpuid.h > > Signed-off-by: Olof Johansson <olof@lixom.net> > --- > arch/arm64/kernel/cpuinfo.c | 79 +++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 76 insertions(+), 3 deletions(-) [...] The same thing pops up every so often. And the answer is the same each time: - it breaks an existing userspace ABI by changing the format of cpuinfo - it is unmaintainable in the long run - userspace already has this information by simply running "lscpu" What has changed? M.
On Mon, Jul 16, 2018 at 2:17 AM, Marc Zyngier <marc.zyngier@arm.com> wrote: > Hi Olof, > > On 15/07/18 04:53, Olof Johansson wrote: >> There's some use in printing out what the implementer and part numbers >> decode to for cases where they're known. >> >> I filled in the table based on public information; mostly from ARM TRMs >> and other tools (and some of the SSBD tables in the kernel, etc). >> >> Apple IDs came from >> https://github.com/apple/darwin-xnu/blob/master/osfmk/arm/cpuid.h >> >> Signed-off-by: Olof Johansson <olof@lixom.net> >> --- >> arch/arm64/kernel/cpuinfo.c | 79 +++++++++++++++++++++++++++++++++++++++++++-- >> 1 file changed, 76 insertions(+), 3 deletions(-) > > [...] > > The same thing pops up every so often. And the answer is the same each time: Ever thought about why it pops up? > - it breaks an existing userspace ABI by changing the format of cpuinfo Most tools likely rely on a per-line format, and in this case they're likely interpreting the contents after the ':' as an integer. Adding something to the line might or might not break them, agreed. How about introducing a "CPU model" line similar to x86's cleartext one? > - it is unmaintainable in the long run False. Chances are you already need details more fine-grained than this for errata and quirks, we already do for SSBD. And if it lacks an entry it's not the end of the world. > - userspace already has this information by simply running "lscpu" > > What has changed? What has changed is that ARM(64) is moving from a custom-kernel-custom-userspace world to one where you might be upreving kernels and userspace separately, and you might be using an older userspace with a newer machine and a newer kernel. Having to update lscpu is an extra step and extra friction to making it behave nicely. There's also a growing expectation of the system to behave more like x86, especially when it comes to trivial ways of detecting what kind of machine you are running on. On x86 it's been trivial to look at /proc/cpuinfo since it is provided by the platform there. Nobody wants to change their implementation from having to read a file to exec a program, dealing with errors, and parsing its output (sure, easier from scripts, but more awkward from real source). Having the information exported from the kernel is a reasonable way to do it. The other extreme is that the kernel should just ever have exported the raw MIDR value. -Olof
Hi Olof, On Mon, Jul 16, 2018 at 07:34:05AM -0700, Olof Johansson wrote: > On Mon, Jul 16, 2018 at 2:17 AM, Marc Zyngier <marc.zyngier@arm.com> wrote: > > On 15/07/18 04:53, Olof Johansson wrote: > >> There's some use in printing out what the implementer and part numbers > >> decode to for cases where they're known. > >> > >> I filled in the table based on public information; mostly from ARM TRMs > >> and other tools (and some of the SSBD tables in the kernel, etc). > >> > >> Apple IDs came from > >> https://github.com/apple/darwin-xnu/blob/master/osfmk/arm/cpuid.h I appreciate that the format of /proc/cpuinfo isn't as nice as one might like as a human reader. For better or worse, the vast majority of readers of /proc/cpuinfo are applications (be they scripts or binaries), and we must treat the format and contents of /proc/cpuinfo as ABI. We've been burned by /proc/cpuinfo format changes in the past, so we want to minimize the potential for ABI issues. This change is only useful to human readers of /proc/cpuinfo, mandates perpetual maintenance work, and comes with a very strong risk of subtle ABI breakage (both for this patch and future patches it implies will be necessary). So sorry, but I must NAK this patch, as with prior patches changing the format of /proc/cpuinfo. > > The same thing pops up every so often. And the answer is the same each time: > > Ever thought about why it pops up? In some cases because the person in question isn't sure what hardware they're on. In most cases because people are used to x86, and haven't considered what exactly they're trying to achieve. We've repeatedly been asked for strings *by application developers*, despite the MIDR being sufficient and already available. Generally, developers are happy to use MIDRs once they realise this. Having the string leads people to try to parse it, even though it cannot be reliable. > > - it breaks an existing userspace ABI by changing the format of cpuinfo > > Most tools likely rely on a per-line format, and in this case they're > likely interpreting the contents after the ':' as an integer. Adding > something to the line might or might not break them, agreed. > > How about introducing a "CPU model" line similar to x86's cleartext one? > > > - it is unmaintainable in the long run > > False. Chances are you already need details more fine-grained than > this for errata and quirks, we already do for SSBD. For SSBD, we don't look at the MIDR, and rely solely on the FW to identify the presence of a workaround. Going forwards, we'll do likewise as this is the only way to be future-proof and generic. > And if it lacks an entry it's not the end of the world. I don't think we can be that relaxed about this; it's quite easy to see that people *will* write software that relies on the string (or properties thereof) in ways that we cannot manage. People will do silly things like try to read the string into a fixed-size buffer. That works when the string is "Unknown", but falls apart on the next kernel release when it's "awesome super CPU 2000xx-super-mega-long-name-edition". We don't discover this problem for months, whereupon other applications are reliant upon the new string, and now we're stuck with a horrible ABI bug that we can't fix. > > - userspace already has this information by simply running "lscpu" > > > > What has changed? > > What has changed is that ARM(64) is moving from a > custom-kernel-custom-userspace world to one where you might be > upreving kernels and userspace separately, and you might be using an > older userspace with a newer machine and a newer kernel. Sure, and this is exactly why we're trying to ensure that the behaviour is *guaranteed* to be consistent across kernels -- so whichever kernel is used, userspace should behave the same. If userspace *needs* to know about a CPU, it can identify the CPU based on the MIDR (and REVIDR), and this will work regardless of kernel version. GCC does this today. If userspace *wants* to know about a CPU (e.g. to expose a human-friendly name), then a userspace database is fine. In the majority of cases it's *vastly* easier to update a userspace application than it is to update the kernel, and it's always possible to build the latest binary yourself. There are a number of cases where using strings is going to be problematic in practice, e.g. * Every out-of-tree SoC port is going to invent values that may not match the values mainline settles on. Then people will complain that their software only works on one or the other, because it makes some decision base on the model name. * Every distro is going to differ in the set of IDs they backport, if any. If your software relies on the MIDR, it will work everywhere. If it tries to parse the string, it will behave erratically across distros. * We may get the matching logic for wrong, and be overly permissive when matching MIDR -> string. e.g. we ignore the low 4 bits for some range of MIDRs, but the vendor only uses 0x0-0xc, and assigns 0xd-0xf a different name. * Parts (and vendors) don't always have a fix name. What happens when a vendor renames their product (or themselves)? Remember Cortex-A12? > There's also a growing expectation of the system to behave more like > x86, especially when it comes to trivial ways of detecting what kind > of machine you are running on. On x86 it's been trivial to look at > /proc/cpuinfo since it is provided by the platform there. It's trivial because *the CPU provides this*, via CPUID. See get_model_name() in arch/x86/kernel/cpu/common.c. That is how x86 can expose a model name in a future-proof manner, and without a maintenance nightmare. On arm, we have the MIDR_EL1 and REVIDR_EL1. We simply do not have the same information, and trying to guess at it is a losing game. > Nobody wants to change their implementation from having to read a file > to exec a program, dealing with errors, and parsing its output (sure, > easier from scripts, but more awkward from real source). Unfortunately, this ship has long sailed. The format of /proc/cpuinfo differs across architectures, and it is not something that can be relied upon by portable applications. > Having the information exported from the kernel is a reasonable way to > do it. While I can appreciate this may seem simple, it comes with far more problems than it solves. > The other extreme is that the kernel should just ever have exported > the raw MIDR value. We expose the raw MIDR and REVIDR values under sysfs. We expose the decomposed value (in the format defined by the architecture) because we have to for compatibility with existing software. To be honest, my personal preference would be to delete /proc/cpuinfo if we could. Thanks, Mark.
On 16/07/18 15:34, Olof Johansson wrote: [...] > There's also a growing expectation of the system to behave more like > x86, especially when it comes to trivial ways of detecting what kind > of machine you are running on. On x86 it's been trivial to look at > /proc/cpuinfo since it is provided by the platform there. Nobody wants > to change their implementation from having to read a file to exec a > program, dealing with errors, and parsing its output (sure, easier > from scripts, but more awkward from real source). The thing with the "be more like x86" argument is that it never actually stands up to scrutiny. What is proposed here (again) is for the kernel to decode the raw MIDR description of the *CPU core microarchitecture* to a human-readable string. What does the nearest Intel box say about its microarchitecture name? cpu family : 6 model : 79 OK, I had to go and look up that that apparently implies "Broadwell", so the x86 kernel is in fact no more helpful in that regard than arm64. Furthermore, telling the user that they have have 4 "Cortex-A53" cores vs. 4 "part: 0xd03" cores doesn't actually give them any more meaningful information about their *system*, because that example reflects about half the low-to-mid-range SoCs on the market today. What the "model name" on x86 describes is a specific processor SKU, of which Arm has no *architectural* equivalent, for reasons which should hopefully be obvious from the fundamental differences in the business models driving the respective architectures. And yet whenever /proc/cpuinfo/ comes up, nobody ever seems to ask for some kind of SoC identifier in there; it's always just decoding MIDR into a microarchitecture name, which implies that all these people want is to see a string for the sake of seeing a string, regardless of how meaningful it actually might be (and yes, I do realise that in *some* cases a CPU core is essentially unique to a particular SoC, but we have to look at the big picture here). In summary; when a small minority of users are complaining that oranges aren't apples, the sensible response is not to start debating whether to paint the oranges green or red. Robin.
diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c index e9ab7b3..9a7c25d 100644 --- a/arch/arm64/kernel/cpuinfo.c +++ b/arch/arm64/kernel/cpuinfo.c @@ -121,6 +121,67 @@ static const char *const compat_hwcap2_str[] = { }; #endif /* CONFIG_COMPAT */ +struct midr_info { + unsigned int mask; + unsigned int val; + char *id; +}; + +static const struct midr_info midr_part[] = { + // ARM + { .mask = 0xff00fff0, .val = 0x4100d030, .id = "Cortex-A53" }, + { .mask = 0xff00fff0, .val = 0x4100d040, .id = "Cortex-A35" }, + { .mask = 0xff00fff0, .val = 0x4100d070, .id = "Cortex-A57" }, + { .mask = 0xff00fff0, .val = 0x4100d080, .id = "Cortex-A72" }, + { .mask = 0xff00fff0, .val = 0x4100d090, .id = "Cortex-A73" }, + { .mask = 0xff00fff0, .val = 0x4100d0a0, .id = "Cortex-A75" }, + { .mask = 0xff00fff0, .val = 0x4100d0f0, .id = "Cortex-A55" }, + // Broadcom + { .mask = 0xff00fff0, .val = 0x42001000, .id = "Brahma-B53" }, + { .mask = 0xff00fff0, .val = 0x42005160, .id = "ThunderX2" }, + // Cavium + { .mask = 0xff00fff0, .val = 0x43000a00, .id = "ThunderX" }, + { .mask = 0xff00fff0, .val = 0x43000a10, .id = "ThunderX 88xx" }, + { .mask = 0xff00fff0, .val = 0x43000a20, .id = "ThunderX 81xx" }, + { .mask = 0xff00fff0, .val = 0x43000a30, .id = "ThunderX 83xx" }, + { .mask = 0xff00fff0, .val = 0x43000af0, .id = "ThunderX2 99xx" }, + // Nvidia + { .mask = 0xff00fff0, .val = 0x4e000000, .id = "Denver" }, + { .mask = 0xff00fff0, .val = 0x4e000030, .id = "Denver 2" }, + // Applied Micro + { .mask = 0xff00fff0, .val = 0x50000000, .id = "X-Gene" }, + // Qualcomm + { .mask = 0xff00fff0, .val = 0x51002010, .id = "Kryo" }, + { .mask = 0xff00fff0, .val = 0x51002050, .id = "Kryo" }, + { .mask = 0xff00fff0, .val = 0x51002110, .id = "Kryo" }, + { .mask = 0xff00fff0, .val = 0x51008000, .id = "Falkor V1/Kryo" }, + { .mask = 0xff00fff0, .val = 0x51008010, .id = "Kryo V2" }, + { .mask = 0xff00fff0, .val = 0x5100c000, .id = "Falkor" }, + { .mask = 0xff00fff0, .val = 0x5100c010, .id = "Saphira" }, + // Samsung + { .mask = 0xff00fff0, .val = 0x53000010, .id = "M1" }, + // Apple + { .mask = 0xff00fff0, .val = 0x61000010, .id = "Cyclone" }, + { .mask = 0xff00fff0, .val = 0x61000020, .id = "Typhoon" }, + { .mask = 0xff00fff0, .val = 0x61000030, .id = "Typhoon/Capri" }, + { .mask = 0xff00fff0, .val = 0x61000040, .id = "Twister" }, + { .mask = 0xff00fff0, .val = 0x61000050, .id = "Twister/Elba/Malta" }, + { .mask = 0xff00fff0, .val = 0x61000060, .id = "Hurricane" }, + { .mask = 0xff00fff0, .val = 0x61000070, .id = "Hurricane/Myst" }, +}; + +static const struct midr_info midr_impl[] = { + { .mask = 0xff000000, .val = 0x41000000, .id = "ARM" }, + { .mask = 0xff000000, .val = 0x42000000, .id = "Broadcom" }, + { .mask = 0xff000000, .val = 0x43000000, .id = "Cavium" }, + { .mask = 0xff000000, .val = 0x4d000000, .id = "Motorola" }, + { .mask = 0xff000000, .val = 0x4e000000, .id = "Nvidia" }, + { .mask = 0xff000000, .val = 0x50000000, .id = "Applied Micro" }, + { .mask = 0xff000000, .val = 0x51000000, .id = "Qualcomm" }, + { .mask = 0xff000000, .val = 0x53000000, .id = "Samsung" }, + { .mask = 0xff000000, .val = 0x61000000, .id = "Apple" }, +}; + static int c_show(struct seq_file *m, void *v) { int i, j; @@ -129,6 +190,16 @@ 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; + char *impl = NULL; + char *part = NULL; + + for (j = 0; !impl && j < ARRAY_SIZE(midr_impl); j++) + if ((midr & midr_impl[j].mask) == midr_impl[j].val) + impl = midr_impl[j].id; + + for (j = 0; !part && j < ARRAY_SIZE(midr_part); j++) + if ((midr & midr_part[j].mask) == midr_part[j].val) + part = midr_part[j].id; /* * glibc reads /proc/cpuinfo to determine the number of @@ -168,11 +239,12 @@ static int c_show(struct seq_file *m, void *v) } seq_puts(m, "\n"); - seq_printf(m, "CPU implementer\t: 0x%02x\n", - MIDR_IMPLEMENTOR(midr)); + seq_printf(m, "CPU implementer\t: 0x%02x (%s)\n", + MIDR_IMPLEMENTOR(midr), impl ? : "unknown"); seq_printf(m, "CPU architecture: 8\n"); seq_printf(m, "CPU variant\t: 0x%x\n", MIDR_VARIANT(midr)); - seq_printf(m, "CPU part\t: 0x%03x\n", MIDR_PARTNUM(midr)); + seq_printf(m, "CPU part\t: 0x%03x (%s)\n", MIDR_PARTNUM(midr), + part ? : "unknown"); seq_printf(m, "CPU revision\t: %d\n\n", MIDR_REVISION(midr)); } @@ -300,6 +372,7 @@ static int __init cpuinfo_regs_init(void) } return 0; } + static void cpuinfo_detect_icache_policy(struct cpuinfo_arm64 *info) { unsigned int cpu = smp_processor_id();
There's some use in printing out what the implementer and part numbers decode to for cases where they're known. I filled in the table based on public information; mostly from ARM TRMs and other tools (and some of the SSBD tables in the kernel, etc). Apple IDs came from https://github.com/apple/darwin-xnu/blob/master/osfmk/arm/cpuid.h Signed-off-by: Olof Johansson <olof@lixom.net> --- arch/arm64/kernel/cpuinfo.c | 79 +++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 76 insertions(+), 3 deletions(-)