| Message ID | 20201211174559.26010-3-anton.ivanov@cambridgegreys.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v4,1/7] um: allow the use of glibc functions instead of builtins | expand |
> +++ b/arch/um/include/asm/fpu/api.h > @@ -0,0 +1,14 @@ > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > +#ifndef _ASM_UM_FPU_API_H > +#define _ASM_UM_FPU_API_H > + > +/* Copyright (c) 2020 Cambridge Greys Ltd > + * Copyright (c) 2020 Red Hat Inc. > + * A set of "dummy" defines to allow the direct inclusion > + * of x86 optimized copy, xor, etc routines into the > + * UML code tree. */ > + > +#define kernel_fpu_begin() (void)0 > +#define kernel_fpu_end() (void)0 I think I would prefer those to be static inlines, but YMMV. johannes > diff --git a/arch/um/include/asm/xor-x86.h b/arch/um/include/asm/xor-x86.h > new file mode 120000 > index 000000000000..beff7de6890d > --- /dev/null > +++ b/arch/um/include/asm/xor-x86.h > @@ -0,0 +1 @@ > +../../../x86/include/asm/xor.h Do these really need to be symlinks? Last I looked, it seemed that arch/x86/include/asm/ is actually in the include path? > --- a/arch/um/include/asm/xor.h > +++ b/arch/um/include/asm/xor.h > @@ -1,7 +1,22 @@ > /* SPDX-License-Identifier: GPL-2.0 */ > -#include <asm-generic/xor.h> > +#ifndef _ASM_UM_XOR_H > +#define _ASM_UM_XOR_H > + > +#ifdef CONFIG_64BIT > +#undef CONFIG_X86_32 > +#else > +#define CONFIG_X86_32 1 > +#endif > + > +#include <asm/cpufeature.h> > +#include <asm/xor-x86.h> and thus this could just be #include_next <asm/xor.h> without the symlink? > diff --git a/arch/um/include/asm/xor_32.h b/arch/um/include/asm/xor_32.h > new file mode 120000 > index 000000000000..8a0894e996d7 > --- /dev/null > +++ b/arch/um/include/asm/xor_32.h And the others don't exist in um anyway, so probably aren't needed? But maybe I'm confused about the include path, I didn't double-check now. johannes
On 11/12/2020 20:07, Johannes Berg wrote: >> +++ b/arch/um/include/asm/fpu/api.h >> @@ -0,0 +1,14 @@ >> +/* SPDX-License-Identifier: GPL-2.0-or-later */ >> +#ifndef _ASM_UM_FPU_API_H >> +#define _ASM_UM_FPU_API_H >> + >> +/* Copyright (c) 2020 Cambridge Greys Ltd >> + * Copyright (c) 2020 Red Hat Inc. >> + * A set of "dummy" defines to allow the direct inclusion >> + * of x86 optimized copy, xor, etc routines into the >> + * UML code tree. */ >> + >> +#define kernel_fpu_begin() (void)0 >> +#define kernel_fpu_end() (void)0 > I think I would prefer those to be static inlines, but YMMV. > > johannes > >> diff --git a/arch/um/include/asm/xor-x86.h b/arch/um/include/asm/xor-x86.h >> new file mode 120000 >> index 000000000000..beff7de6890d >> --- /dev/null >> +++ b/arch/um/include/asm/xor-x86.h >> @@ -0,0 +1 @@ >> +../../../x86/include/asm/xor.h > Do these really need to be symlinks? Last I looked, it seemed that > arch/x86/include/asm/ is actually in the include path? It is included, but it is included quite far down the list. We pick up a few things out of there, but if we leave them "as is" they all default to their least optimized versions. The results clearly demonstrate that too - 30% difference on 64 bit and > 100% on 32 bit. This is because we do not perform alternatives substitution. Our "alternatives" processing function in the UML startup is a noop. My idea was to override that to the extent possible and get whatever mileage is possible without that. I can give it a try to see how it looks if I use the x86 feature table and other bits which are picked up from there, but working with that is like pulling teeth without anaesthetic. On the positive side this means that we can copy the alternatives code on x86. I can give it another go. I tried early on and it was a bit painful. A. > >> --- a/arch/um/include/asm/xor.h >> +++ b/arch/um/include/asm/xor.h >> @@ -1,7 +1,22 @@ >> /* SPDX-License-Identifier: GPL-2.0 */ >> -#include <asm-generic/xor.h> >> +#ifndef _ASM_UM_XOR_H >> +#define _ASM_UM_XOR_H >> + >> +#ifdef CONFIG_64BIT >> +#undef CONFIG_X86_32 >> +#else >> +#define CONFIG_X86_32 1 >> +#endif >> + >> +#include <asm/cpufeature.h> >> +#include <asm/xor-x86.h> > and thus this could just be > > #include_next <asm/xor.h> > > without the symlink? > >> diff --git a/arch/um/include/asm/xor_32.h b/arch/um/include/asm/xor_32.h >> new file mode 120000 >> index 000000000000..8a0894e996d7 >> --- /dev/null >> +++ b/arch/um/include/asm/xor_32.h > > And the others don't exist in um anyway, so probably aren't needed? > > But maybe I'm confused about the include path, I didn't double-check > now. > > johannes > > > > _______________________________________________ > linux-um mailing list > linux-um@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-um >
On Fri, 2020-12-11 at 21:57 +0000, Anton Ivanov wrote: > > > > --- /dev/null > > > +++ b/arch/um/include/asm/xor-x86.h > > > @@ -0,0 +1 @@ > > > +../../../x86/include/asm/xor.h > > Do these really need to be symlinks? Last I looked, it seemed that > > arch/x86/include/asm/ is actually in the include path? > > It is included, but it is included quite far down the list. I see. So you're saying basically we'll get asm-generic/xor.h before the x86 version, and then we're getting the worst possible implementation, right? > We pick up a few things out of there, but if we leave them "as is" they > all default to their least optimized versions. The results clearly > demonstrate that too - 30% difference on 64 bit and > 100% on 32 bit. Right. > This is because we do not perform alternatives substitution. Our > "alternatives" processing function in the UML startup is a noop. Oh, so we *do* get x86, but compatibility with ancient CPUs? > My idea was to override that to the extent possible and get whatever > mileage is possible without that. Makes sense. > I can give it a try to see how it looks if I use the x86 feature table > and other bits which are picked up from there, but working with that is > like pulling teeth without anaesthetic. > > On the positive side this means that we can copy the alternatives code > on x86. > > I can give it another go. I tried early on and it was a bit painful. Yeah, no, not sure ... Maybe just doing something like #include "../../../x86/include/asm/xor.h" would be acceptable? It seems a bit better to me in the sense of being more obvious than the symlinks... but dunno. johannes
On 11/12/2020 22:00, Johannes Berg wrote: > On Fri, 2020-12-11 at 21:57 +0000, Anton Ivanov wrote: >>>> --- /dev/null >>>> +++ b/arch/um/include/asm/xor-x86.h >>>> @@ -0,0 +1 @@ >>>> +../../../x86/include/asm/xor.h >>> Do these really need to be symlinks? Last I looked, it seemed that >>> arch/x86/include/asm/ is actually in the include path? >> It is included, but it is included quite far down the list. > I see. So you're saying basically we'll get asm-generic/xor.h before the > x86 version, and then we're getting the worst possible implementation, > right? A x86 implementation which is at "worst case scenario defaults" and has not undergone an alternative replacement for the actual CPU features can be as bad as the generic. In fact, in some cases generic may even be better :( >> We pick up a few things out of there, but if we leave them "as is" they >> all default to their least optimized versions. The results clearly >> demonstrate that too - 30% difference on 64 bit and > 100% on 32 bit. > Right. > >> This is because we do not perform alternatives substitution. Our >> "alternatives" processing function in the UML startup is a noop. > Oh, so we *do* get x86, but compatibility with ancient CPUs? Not just ancient CPUs. Ancient BUGGY cpus. It is usually the worst case scenario implementation. > >> My idea was to override that to the extent possible and get whatever >> mileage is possible without that. > Makes sense. > >> I can give it a try to see how it looks if I use the x86 feature table >> and other bits which are picked up from there, but working with that is >> like pulling teeth without anaesthetic. >> >> On the positive side this means that we can copy the alternatives code >> on x86. >> >> I can give it another go. I tried early on and it was a bit painful. > Yeah, no, not sure ... > > Maybe just doing something like > > #include "../../../x86/include/asm/xor.h" > > would be acceptable? It seems a bit better to me in the sense of being > more obvious than the symlinks... but dunno. That (and everything else) relies on the CPU Features available macros. I "cheated" on those and created our own using just one ulong - the 5-6 bits which are relevant to us. That is the original idea behind pulling things in and symlinking - to make sure they pick OUR defs, not the whole array of features out of the x86 tree. We also need to noop or redefine a few things like fpu_exit, fpu_enter, etc. However, based on the discussion we have had so far, I should revisit this and do it ONLY where it is needed, not in all cases. I will give it another go on Monday. Looking at the results, it's definitely worth it. For me it is a question of how to do it, not "should we do it". 30% difference is in the realm of "definitely worth it". > > johannes > >
On 11/12/2020 22:00, Johannes Berg wrote: > On Fri, 2020-12-11 at 21:57 +0000, Anton Ivanov wrote: >>>> --- /dev/null >>>> +++ b/arch/um/include/asm/xor-x86.h >>>> @@ -0,0 +1 @@ >>>> +../../../x86/include/asm/xor.h >>> Do these really need to be symlinks? Last I looked, it seemed that >>> arch/x86/include/asm/ is actually in the include path? >> It is included, but it is included quite far down the list. > I see. So you're saying basically we'll get asm-generic/xor.h before the > x86 version, and then we're getting the worst possible implementation, > right? > >> We pick up a few things out of there, but if we leave them "as is" they >> all default to their least optimized versions. The results clearly >> demonstrate that too - 30% difference on 64 bit and > 100% on 32 bit. > Right. > >> This is because we do not perform alternatives substitution. Our >> "alternatives" processing function in the UML startup is a noop. > Oh, so we *do* get x86, but compatibility with ancient CPUs? I had a look at the alternatives processing in x86 once again. The exact definition of what we get to use is: "ancient, buggy CPUs in SMP mode". So in addition to using one of the worst case scenario implementations, we also do not do patching of SMP verbiage to UP where appropriate which is done on x86. I just had a go at trying to reuse the aforementioned alternatives processing "as is" from the x86 tree. This is pretty much a no-go from the start. We can't use it. It relies on "owning" int handlers and generating int instructions in places. If I understand it correctly, it will interfere with gdb by doing its own INT 3 work. Key parts of it are also "if-defed away" from us at present. IMHO - it will have to be rewritten mostly from scratch for UML. I will have a look if we can reuse the cpu feature and bug definitions instead of using our own. This will allow us to reuse the bits which relate to crypto - xor, etc as those are cases/ifdefs instead of alternatives. A. > >> My idea was to override that to the extent possible and get whatever >> mileage is possible without that. > Makes sense. > >> I can give it a try to see how it looks if I use the x86 feature table >> and other bits which are picked up from there, but working with that is >> like pulling teeth without anaesthetic. >> >> On the positive side this means that we can copy the alternatives code >> on x86. >> >> I can give it another go. I tried early on and it was a bit painful. > Yeah, no, not sure ... > > Maybe just doing something like > > #include "../../../x86/include/asm/xor.h" > > would be acceptable? It seems a bit better to me in the sense of being > more obvious than the symlinks... but dunno. > > johannes > > > _______________________________________________ > linux-um mailing list > linux-um@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-um >
On Mon, 2020-12-14 at 09:07 +0000, Anton Ivanov wrote: > > I had a look at the alternatives processing in x86 once again. The > exact definition of what we get to use is: "ancient, buggy CPUs in SMP > mode". fun. > So in addition to using one of the worst case scenario > implementations, we also do not do patching of SMP verbiage to UP > where appropriate which is done on x86. right. > I just had a go at trying to reuse the aforementioned alternatives > processing "as is" from the x86 tree. > > This is pretty much a no-go from the start. We can't use it. It relies > on "owning" int handlers and generating int instructions in places. If > I understand it correctly, it will interfere with gdb by doing its own > INT 3 work. Key parts of it are also "if-defed away" from us at > present. > > IMHO - it will have to be rewritten mostly from scratch for UML. > > I will have a look if we can reuse the cpu feature and bug definitions > instead of using our own. This will allow us to reuse the bits which > relate to crypto - xor, etc as those are cases/ifdefs instead of > alternatives. I agree. But I feel like something I said sent you on this path, and that never was my intent. I don't mind using the x86 header files in a fashion similar to what you did, I just didn't like the symlinks because it seems those would be awkward if somebody ever wants to port UML to some other architecture ... Maybe just put there header files with #include "../../x86/include/asm/xor.h" or something like that just to avoid the symlinks? johannes
On 14/12/2020 09:12, Johannes Berg wrote: > On Mon, 2020-12-14 at 09:07 +0000, Anton Ivanov wrote: >> I had a look at the alternatives processing in x86 once again. The >> exact definition of what we get to use is: "ancient, buggy CPUs in SMP >> mode". > fun. > >> So in addition to using one of the worst case scenario >> implementations, we also do not do patching of SMP verbiage to UP >> where appropriate which is done on x86. > right. > >> I just had a go at trying to reuse the aforementioned alternatives >> processing "as is" from the x86 tree. >> >> This is pretty much a no-go from the start. We can't use it. It relies >> on "owning" int handlers and generating int instructions in places. If >> I understand it correctly, it will interfere with gdb by doing its own >> INT 3 work. Key parts of it are also "if-defed away" from us at >> present. >> >> IMHO - it will have to be rewritten mostly from scratch for UML. >> >> I will have a look if we can reuse the cpu feature and bug definitions >> instead of using our own. This will allow us to reuse the bits which >> relate to crypto - xor, etc as those are cases/ifdefs instead of >> alternatives. > I agree. > > But I feel like something I said sent you on this path, and that never > was my intent. I don't mind using the x86 header files in a fashion > similar to what you did, I just didn't like the symlinks because it > seems those would be awkward if somebody ever wants to port UML to some > other architecture ... > > Maybe just put there header files with > > #include "../../x86/include/asm/xor.h" > > or something like that just to avoid the symlinks? I agree with you - that is how we should use them. We still need to have "features" code and boot time CPU detection. I am trying to figure out if it is worth it to borrow some of that instead of my original hack which reused the names with a different back-end. That is another can of worms because some of that code is actually generated at compile time by x86 so I need to figure out how to pull that into our build system. > johannes > >
diff --git a/arch/um/include/asm/cpufeature.h b/arch/um/include/asm/cpufeature.h new file mode 100644 index 000000000000..19a2394d03a4 --- /dev/null +++ b/arch/um/include/asm/cpufeature.h @@ -0,0 +1,17 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef _ASM_UM_CPUFEATURE_H +#define _ASM_UM_CPUFEATURE_H + +#include <asm/asm.h> +#include <linux/bitops.h> +#include <asm/processor-generic.h> +#include <asm/cpufeatures.h> + + +const char *host_cpu_feature_names[] = {"mmx", "xmm", "avx", "osxsave", "rep_good", "erms", "xmm2"}; +#define MAX_UM_CPU_FEATURES ARRAY_SIZE(host_cpu_feature_names) + + +#define boot_cpu_has(bit) (boot_cpu_data.host_features & bit) + +#endif /* _ASM_UM_CPUFEATURE_H */ diff --git a/arch/um/include/asm/cpufeatures.h b/arch/um/include/asm/cpufeatures.h new file mode 100644 index 000000000000..d02da39b039d --- /dev/null +++ b/arch/um/include/asm/cpufeatures.h @@ -0,0 +1,15 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef _ASM_UM_CPUFEATURES_H +#define _ASM_UM_CPUFEATURES_H + +/* Fake x86 Features of actual interest to UML */ + +#define X86_FEATURE_MMX (1 << 0) +#define X86_FEATURE_XMM (1 << 1) +#define X86_FEATURE_AVX (1 << 2) +#define X86_FEATURE_OSXSAVE (1 << 3) +#define X86_FEATURE_REP_GOOD (1 << 4) +#define X86_FEATURE_ERMS (1 << 5) +#define X86_FEATURE_XMM2 (1 << 6) + +#endif /* _ASM_UM_CPUFEATURES_H */ diff --git a/arch/um/include/asm/fpu/api.h b/arch/um/include/asm/fpu/api.h new file mode 100644 index 000000000000..2873a6f0105d --- /dev/null +++ b/arch/um/include/asm/fpu/api.h @@ -0,0 +1,14 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +#ifndef _ASM_UM_FPU_API_H +#define _ASM_UM_FPU_API_H + +/* Copyright (c) 2020 Cambridge Greys Ltd + * Copyright (c) 2020 Red Hat Inc. + * A set of "dummy" defines to allow the direct inclusion + * of x86 optimized copy, xor, etc routines into the + * UML code tree. */ + +#define kernel_fpu_begin() (void)0 +#define kernel_fpu_end() (void)0 + +#endif diff --git a/arch/um/include/asm/processor-generic.h b/arch/um/include/asm/processor-generic.h index afd9b267cf81..b8bcddbb1898 100644 --- a/arch/um/include/asm/processor-generic.h +++ b/arch/um/include/asm/processor-generic.h @@ -90,6 +90,9 @@ extern void start_thread(struct pt_regs *regs, unsigned long entry, struct cpuinfo_um { unsigned long loops_per_jiffy; int ipi_pipe[2]; + /* There is only a small set of x86 features we are interested + * in for now */ + unsigned long host_features; }; extern struct cpuinfo_um boot_cpu_data; diff --git a/arch/um/include/asm/xor-x86.h b/arch/um/include/asm/xor-x86.h new file mode 120000 index 000000000000..beff7de6890d --- /dev/null +++ b/arch/um/include/asm/xor-x86.h @@ -0,0 +1 @@ +../../../x86/include/asm/xor.h \ No newline at end of file diff --git a/arch/um/include/asm/xor.h b/arch/um/include/asm/xor.h index 36b33d62a35d..18bcb5b6189d 100644 --- a/arch/um/include/asm/xor.h +++ b/arch/um/include/asm/xor.h @@ -1,7 +1,22 @@ /* SPDX-License-Identifier: GPL-2.0 */ -#include <asm-generic/xor.h> +#ifndef _ASM_UM_XOR_H +#define _ASM_UM_XOR_H + +#ifdef CONFIG_64BIT +#undef CONFIG_X86_32 +#else +#define CONFIG_X86_32 1 +#endif + +#include <asm/cpufeature.h> +#include <asm/xor-x86.h> #include <linux/time-internal.h> +#ifdef CONFIG_UML_TIME_TRAVEL_SUPPORT +#undef XOR_SELECT_TEMPLATE /* pick an arbitrary one - measuring isn't possible with inf-cpu */ #define XOR_SELECT_TEMPLATE(x) \ (time_travel_mode == TT_MODE_INFCPU ? &xor_block_8regs : NULL) +#endif + +#endif diff --git a/arch/um/include/asm/xor_32.h b/arch/um/include/asm/xor_32.h new file mode 120000 index 000000000000..8a0894e996d7 --- /dev/null +++ b/arch/um/include/asm/xor_32.h @@ -0,0 +1 @@ +../../../x86/include/asm/xor_32.h \ No newline at end of file diff --git a/arch/um/include/asm/xor_64.h b/arch/um/include/asm/xor_64.h new file mode 120000 index 000000000000..b8d346c516bf --- /dev/null +++ b/arch/um/include/asm/xor_64.h @@ -0,0 +1 @@ +../../../x86/include/asm/xor_64.h \ No newline at end of file diff --git a/arch/um/include/asm/xor_avx.h b/arch/um/include/asm/xor_avx.h new file mode 120000 index 000000000000..370ded122095 --- /dev/null +++ b/arch/um/include/asm/xor_avx.h @@ -0,0 +1 @@ +../../../x86/include/asm/xor_avx.h \ No newline at end of file diff --git a/arch/um/include/shared/os.h b/arch/um/include/shared/os.h index f467d28fc0b4..c2ff855af603 100644 --- a/arch/um/include/shared/os.h +++ b/arch/um/include/shared/os.h @@ -187,6 +187,7 @@ int os_poll(unsigned int n, const int *fds); extern void os_early_checks(void); extern void os_check_bugs(void); extern void check_host_supports_tls(int *supports_tls, int *tls_min); +extern unsigned long check_host_cpu_features(const char **feature_names, int n); /* mem.c */ extern int create_mem_file(unsigned long long len); diff --git a/arch/um/kernel/um_arch.c b/arch/um/kernel/um_arch.c index 76b37297b7d4..b7dfc4fcc130 100644 --- a/arch/um/kernel/um_arch.c +++ b/arch/um/kernel/um_arch.c @@ -15,6 +15,7 @@ #include <linux/kmsg_dump.h> #include <asm/processor.h> +#include <asm/cpufeature.h> #include <asm/sections.h> #include <asm/setup.h> #include <as-layout.h> @@ -48,9 +49,12 @@ static void __init add_arg(char *arg) */ struct cpuinfo_um boot_cpu_data = { .loops_per_jiffy = 0, - .ipi_pipe = { -1, -1 } + .ipi_pipe = { -1, -1 }, + .host_features = 0 }; +EXPORT_SYMBOL(boot_cpu_data); + union thread_union cpu0_irqstack __section(".data..init_irqstack") = { .thread_info = INIT_THREAD_INFO(init_task) }; @@ -67,9 +71,15 @@ static int show_cpuinfo(struct seq_file *m, void *v) seq_printf(m, "model name\t: UML\n"); seq_printf(m, "mode\t\t: skas\n"); seq_printf(m, "host\t\t: %s\n", host_info); - seq_printf(m, "bogomips\t: %lu.%02lu\n\n", + seq_printf(m, "bogomips\t: %lu.%02lu\n", loops_per_jiffy/(500000/HZ), (loops_per_jiffy/(5000/HZ)) % 100); + seq_printf(m, "flags\t\t:"); + for (index = 0; index < MAX_UM_CPU_FEATURES; index++) { + if (boot_cpu_data.host_features & (1 << index)) + seq_printf(m, " %s", host_cpu_feature_names[index]); + } + seq_printf(m, "\n\n"); return 0; } @@ -275,6 +285,9 @@ int __init linux_main(int argc, char **argv) /* OS sanity checks that need to happen before the kernel runs */ os_early_checks(); + boot_cpu_data.host_features = + check_host_cpu_features(host_cpu_feature_names, MAX_UM_CPU_FEATURES); + brk_start = (unsigned long) sbrk(0); /* diff --git a/arch/um/os-Linux/start_up.c b/arch/um/os-Linux/start_up.c index f79dc338279e..be884ed86b30 100644 --- a/arch/um/os-Linux/start_up.c +++ b/arch/um/os-Linux/start_up.c @@ -321,6 +321,38 @@ static void __init check_coredump_limit(void) os_info("%llu\n", (unsigned long long)lim.rlim_max); } +unsigned long __init check_host_cpu_features(const char **feature_names, int n) +{ + FILE *cpuinfo; + char *line = NULL; + size_t len = 0; + int i; + bool done_parsing = false; + unsigned long result = 0; + + cpuinfo = fopen("/proc/cpuinfo", "r"); + if (cpuinfo == NULL) { + os_info("Failed to get host CPU features\n"); + } else { + while ((getline(&line, &len, cpuinfo)) != -1) { + if (strstr(line, "flags")) { + for (i = 0; i < n; i++) { + if (strstr(line, feature_names[i])) { + result |= (1 << i); + } + } + done_parsing = true; + } + free(line); + line = NULL; + if (done_parsing) + break; + } + fclose(cpuinfo); + } + return result; +} + void __init os_early_checks(void) { int pid;