Re: [PATCH 1/2] compiler_types: Ensure __diag_clang() is always available
Hi,
On Tue, Mar 19, 2024 at 09:07:52AM -0700, Nathan Chancellor wrote:
> Attempting to use __diag_clang() and build with GCC results in a build
> error:
>
> include/linux/compiler_types.h:468:38: error: 'ignore' undeclared (first
> use in this function); did you mean 'inode'?
> 468 | __diag_ ## compiler(version, ignore, option)
> | ^~
>
> This error occurs because __diag_clang() is only defined in
> compiler-clang.h, which is only included when using clang as the
> compiler. This error has not been seen before because __diag_clang() has
> only been used in __diag_ignore_all(), which is defined in both
> compiler-clang.h and compiler-gcc.h.
>
> Add an empty stub for __diag_clang() in compiler_types.h, so that it is
> always defined and just becomes a no-op when using GCC.
>
> Fixes: f014a00bbeb0 ("compiler-clang.h: Add __diag infrastructure for clang")
> Signed-off-by: Nathan Chancellor
Reviewed-by: Justin Stitt
> ---
> include/linux/compiler_types.h | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
> index 3e64ec0f7ac8..fb0c3ff5497d 100644
> --- a/include/linux/compiler_types.h
> +++ b/include/linux/compiler_types.h
> @@ -461,6 +461,10 @@ struct ftrace_likely_data {
> #define __diag_GCC(version, severity, string)
> #endif
>
> +#ifndef __diag_clang
> +#define __diag_clang(version, severity, string)
> +#endif
> +
> #define __diag_push()__diag(push)
> #define __diag_pop() __diag(pop)
>
>
> --
> 2.44.0
Thanks
Justin
Re: [PATCH 2/2] tracing: Ignore -Wstring-compare with diagnostic macros
On Tue, Mar 19, 2024 at 9:08 AM Nathan Chancellor wrote:
>
> Commit b1afefa62ca9 ("tracing: Use strcmp() in __assign_str() WARN_ON()
> check") addressed a clang warning, -Wstring-compare, with the use of
> __builtin_constant_p() to dispatch to strcmp() if the source string is a
> string literal and a direct comparison if not. Unfortunately, even with
> this change, the warning is still present because __builtin_constant_p()
> is not evaluated at this stage of the pipeline, so clang still thinks
> the else branch could occur for this situation:
>
> include/trace/events/sunrpc.h:705:4: error: result of comparison against a
> string literal is unspecified (use an explicit string comparison function
> instead) [-Werror,-Wstring-compare]
> ...
> include/trace/stages/stage6_event_callback.h:40:15: note: expanded from
> macro '__assign_str'
> 40 | (src) != __data_offsets.dst##_ptr_);
> \
> |^
> ...
>
> Use the compiler diagnostic macros to disable this warning around the
> WARN_ON_ONCE() expression since a string comparison function, strcmp(),
> will always be used for the comparison of string literals.
>
> Fixes: b1afefa62ca9 ("tracing: Use strcmp() in __assign_str() WARN_ON()
> check")
> Reported-by: Linux Kernel Functional Testing
> Closes:
> https://lore.kernel.org/all/CA+G9fYs=otkazs6g1p1ewadfr0qoe6lgovsohqkxmfxoteo...@mail.gmail.com/
> Signed-off-by: Nathan Chancellor
> ---
> include/trace/stages/stage6_event_callback.h | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/include/trace/stages/stage6_event_callback.h
> b/include/trace/stages/stage6_event_callback.h
> index 83da83a0c14f..56a4eea5a48e 100644
> --- a/include/trace/stages/stage6_event_callback.h
> +++ b/include/trace/stages/stage6_event_callback.h
> @@ -35,9 +35,14 @@
> do {\
> char *__str__ = __get_str(dst); \
> int __len__ = __get_dynamic_array_len(dst) - 1; \
> + __diag_push(); \
> + __diag_ignore(clang, 11, "-Wstring-compare",\
> + "__builtin_constant_p() ensures strcmp()" \
> + "will be used for string literals"); \
> WARN_ON_ONCE(__builtin_constant_p(src) ?\
> strcmp((src), __data_offsets.dst##_ptr_) : \
> (src) != __data_offsets.dst##_ptr_); \
What exactly is the point of the literal string comparison? Why
doesn't strcmp do the trick?
> + __diag_pop(); \
> memcpy(__str__, __data_offsets.dst##_ptr_ ? : \
>EVENT_NULL_STR, __len__);\
> __str__[__len__] = '\0';\
>
> --
> 2.44.0
>
Re: ARM Ftrace Function Graph Fails With UNWINDER_FRAME_POINTER
On 12/2/2023 1:26 AM, Ard Biesheuvel wrote:
On Sat, 2 Dec 2023 at 09:49, Justin Chen wrote:
On 12/1/23 10:53 PM, Ard Biesheuvel wrote:
On Fri, 1 Dec 2023 at 23:59, Justin Chen wrote:
On 12/1/23 10:07 AM, Steven Rostedt wrote:
On Fri, 1 Dec 2023 09:25:59 -0800
Justin Chen wrote:
It appears the sub instruction at 0x6dd0 correctly accounts for the
extra 8 bytes, so the frame pointer is valid. So it is our assumption
that there are no gaps between the stack frames is invalid.
Thanks for the assistance. The gap between the stack frame depends on
the function. Most do not have a gap. Some have 8 (as shown above), some
have 12. A single assumption here is not going to work. I'm having a
hard time finding out the reasoning for this gap. I tried disabling a
bunch of gcc flags as well as -O2 and the gap still exists.
That code was originally added because of some strange things that gcc did
with mcount (for example, it made a copy of the stack frame that it passed
to mcount, where the function graph tracer replaced the copy of the return
stack making the shadow stack go out of sync and crash). This was very hard
to debug and I added this code to detect it if it happened again.
Well it's been over a decade since that happened (2009).
71e308a239c09 ("function-graph: add stack frame test")
I'm happy assuming that the compiler folks are aware of our tricks with
hijacking return calls and I don't expect it to happen again. We can just
rip out those checks. That is, if it's only causing false positives, I
don't think it's worth keeping around.
Has it detected any real issues on the Arm platforms?
-- Steve
I am not familiar enough to make a call. But from my limited testing
with ARM, I didn't see any issues. If you would like me to, I can submit
a patch to remove the check entirely. Or maybe only disable it for ARM?
Please try the fix I proposed first.
Just tested it. Seems to do the trick.
Thanks
Either solution works for me.
Given that this discussion is taking place in the context of the
report of an issue identified by GRAPH_FP_TEST, I don't see how
removing that would be a reasonable conclusion.
Fair enough. I will submit a patch. Thanks for the help.
FWIW I also experimented with LLVM, looks like function_graph just
crashes regardless of the issue being discussed. The disassemble of
LLVM[1] does something completely different.
LLVM does not support CONFIG_UNWINDER_FRAME_POINTER so the fact that
the prologue looks different is expected.
In the case below, the FP {r11} is correctly made to point to a {r11,
lr} tuple on the stack, so the codegen is correct AFAICT. But IIRC we
rely on unwind information for ftrace in this case, not the frame
pointer.
Could you be more specific about the crash?
It just hangs with no prints. I can instrument the hang and see what I
can find.
Justin
[1]
LLVM dump
c0c6faa0 :
c0c6faa0: f0 4f 2d e9 push{r4, r5, r6, r7, r8, r9, r10, r11, lr}
c0c6faa4: 1c b0 8d e2 add r11, sp, #28
c0c6faa8: ac d0 4d e2 sub sp, sp, #172
c0c6faac: 00 70 a0 e1 mov r7, r0
c0c6fab0: c8 0c 04 e3 movwr0, #19656
c0c6fab4: 80 02 4c e3 movtr0, #49792
c0c6fab8: 03 50 a0 e1 mov r5, r3
c0c6fabc: 00 00 90 e5 ldr r0, [r0]
c0c6fac0: 02 a0 a0 e1 mov r10, r2
c0c6fac4: 20 00 0b e5 str r0, [r11, #-32]
c0c6fac8: 00 40 2d e9 stmdb sp!, {lr}
c0c6facc: 4b 8b d6 eb bl 0xc0212800 <__gnu_mcount_nc> @ imm =
#-10867412
smime.p7s
Description: S/MIME Cryptographic Signature
Re: ARM Ftrace Function Graph Fails With UNWINDER_FRAME_POINTER
On 12/1/23 10:53 PM, Ard Biesheuvel wrote:
On Fri, 1 Dec 2023 at 23:59, Justin Chen wrote:
On 12/1/23 10:07 AM, Steven Rostedt wrote:
On Fri, 1 Dec 2023 09:25:59 -0800
Justin Chen wrote:
It appears the sub instruction at 0x6dd0 correctly accounts for the
extra 8 bytes, so the frame pointer is valid. So it is our assumption
that there are no gaps between the stack frames is invalid.
Thanks for the assistance. The gap between the stack frame depends on
the function. Most do not have a gap. Some have 8 (as shown above), some
have 12. A single assumption here is not going to work. I'm having a
hard time finding out the reasoning for this gap. I tried disabling a
bunch of gcc flags as well as -O2 and the gap still exists.
That code was originally added because of some strange things that gcc did
with mcount (for example, it made a copy of the stack frame that it passed
to mcount, where the function graph tracer replaced the copy of the return
stack making the shadow stack go out of sync and crash). This was very hard
to debug and I added this code to detect it if it happened again.
Well it's been over a decade since that happened (2009).
71e308a239c09 ("function-graph: add stack frame test")
I'm happy assuming that the compiler folks are aware of our tricks with
hijacking return calls and I don't expect it to happen again. We can just
rip out those checks. That is, if it's only causing false positives, I
don't think it's worth keeping around.
Has it detected any real issues on the Arm platforms?
-- Steve
I am not familiar enough to make a call. But from my limited testing
with ARM, I didn't see any issues. If you would like me to, I can submit
a patch to remove the check entirely. Or maybe only disable it for ARM?
Please try the fix I proposed first.
Just tested it. Seems to do the trick. Either solution works for me.
FWIW I also experimented with LLVM, looks like function_graph just
crashes regardless of the issue being discussed. The disassemble of
LLVM[1] does something completely different.
Thanks,
Justin
[1]
LLVM dump
c0c6faa0 :
c0c6faa0: f0 4f 2d e9 push{r4, r5, r6, r7, r8, r9, r10, r11, lr}
c0c6faa4: 1c b0 8d e2 add r11, sp, #28
c0c6faa8: ac d0 4d e2 sub sp, sp, #172
c0c6faac: 00 70 a0 e1 mov r7, r0
c0c6fab0: c8 0c 04 e3 movwr0, #19656
c0c6fab4: 80 02 4c e3 movtr0, #49792
c0c6fab8: 03 50 a0 e1 mov r5, r3
c0c6fabc: 00 00 90 e5 ldr r0, [r0]
c0c6fac0: 02 a0 a0 e1 mov r10, r2
c0c6fac4: 20 00 0b e5 str r0, [r11, #-32]
c0c6fac8: 00 40 2d e9 stmdb sp!, {lr}
c0c6facc: 4b 8b d6 eb bl 0xc0212800 <__gnu_mcount_nc> @ imm =
#-10867412
smime.p7s
Description: S/MIME Cryptographic Signature
Re: ARM Ftrace Function Graph Fails With UNWINDER_FRAME_POINTER
On 12/1/23 10:07 AM, Steven Rostedt wrote:
On Fri, 1 Dec 2023 09:25:59 -0800
Justin Chen wrote:
It appears the sub instruction at 0x6dd0 correctly accounts for the
extra 8 bytes, so the frame pointer is valid. So it is our assumption
that there are no gaps between the stack frames is invalid.
Thanks for the assistance. The gap between the stack frame depends on
the function. Most do not have a gap. Some have 8 (as shown above), some
have 12. A single assumption here is not going to work. I'm having a
hard time finding out the reasoning for this gap. I tried disabling a
bunch of gcc flags as well as -O2 and the gap still exists.
That code was originally added because of some strange things that gcc did
with mcount (for example, it made a copy of the stack frame that it passed
to mcount, where the function graph tracer replaced the copy of the return
stack making the shadow stack go out of sync and crash). This was very hard
to debug and I added this code to detect it if it happened again.
Well it's been over a decade since that happened (2009).
71e308a239c09 ("function-graph: add stack frame test")
I'm happy assuming that the compiler folks are aware of our tricks with
hijacking return calls and I don't expect it to happen again. We can just
rip out those checks. That is, if it's only causing false positives, I
don't think it's worth keeping around.
Has it detected any real issues on the Arm platforms?
-- Steve
I am not familiar enough to make a call. But from my limited testing
with ARM, I didn't see any issues. If you would like me to, I can submit
a patch to remove the check entirely. Or maybe only disable it for ARM?
Thanks,
Justin
smime.p7s
Description: S/MIME Cryptographic Signature
Re: ARM Ftrace Function Graph Fails With UNWINDER_FRAME_POINTER
On 12/1/2023 1:12 AM, Ard Biesheuvel wrote:
On Fri, 1 Dec 2023 at 00:48, Justin Chen wrote:
Hello,
Ran into an odd bug that I am unsure what the solution is. Tested a few
kernels versions and they all fail the same.
FUNCTION_GRAPH_FP_TEST was enabled with 953f534a7ed6 ("ARM: ftrace:
enable HAVE_FUNCTION_GRAPH_FP_TEST"). This test fails when
UNWINDER_FRAME_POINTER is enabled. Enable function_graph tracer and you
should see a failure similar to below.
[ 63.817239] [ cut here ]
[ 63.822006] WARNING: CPU: 3 PID: 1185 at kernel/trace/fgraph.c:195
ftrace_return_to_handler+0x228/0x374
[ 63.831645] Bad frame pointer: expected d1e0df40, received d1e0df48
[ 63.831645] from func packet_setsockopt return to c0b558f4
[ 63.843801] Modules linked in: bdc udc_core
[ 63.848246] CPU: 3 PID: 1185 Comm: udhcpc Not tainted
6.1.53-0.1pre-gf0bc552d12f8 #33
[ 63.856209] Hardware name: Broadcom STB (Flattened Device Tree)
[ 63.862227] Backtrace:
[ 63.864761] dump_backtrace from show_stack+0x20/0x24
[ 63.869982] r7:c031cd8c r6:0009 r5:0013 r4:c11c7fac
[ 63.875736] show_stack from dump_stack_lvl+0x48/0x54
[ 63.880929] dump_stack_lvl from dump_stack+0x18/0x1c
[ 63.886111] r5:00c3 r4:c11bd92c
[ 63.889764] dump_stack from __warn+0x88/0x130
[ 63.894339] __warn from warn_slowpath_fmt+0x140/0x198
[ 63.899631] r8:d1e0deac r7:c11bd958 r6:c031cd8c r5:c11bd92c r4:
[ 63.906431] warn_slowpath_fmt from ftrace_return_to_handler+0x228/0x374
[ 63.913294] r8:c3a8d840 r7:0002 r6:d1e0df48 r5:c2377a94 r4:c269a400
[ 63.920095] ftrace_return_to_handler from return_to_handler+0xc/0x18
[ 63.926699] r8:c0cd8ed0 r7:0008 r6:c418c500 r5:0004 r4:0107
[ 63.933500] __sys_setsockopt from return_to_handler+0x0/0x18
[ 63.939415] r8:c02002bc r7:0126 r6:0003 r5: r4:0004
[ 63.946217] sys_setsockopt from return_to_handler+0x0/0x18
[ 63.952053] ---[ end trace ]---
Sure enough the top of the parent stack is off by 8. (Tested with
gcc6.3/gcc8.3/gcc12.3)
6dcc :
6dcc: e1a0c00dmov ip, sp
6dd0: e24dd008sub sp, sp, #8 <==
6dd4: e92ddff0push{r4, r5, r6, r7, r8, r9, sl,
fp, ip, lr, pc}
6dd8: e24cb00csub fp, ip, #12
6ddc: e24dd06csub sp, sp, #108@ 0x6c
6de0: e52de004push{lr}@ (str lr, [sp,
#-4]!)
6de4: ebfebl 0 <__gnu_mcount_nc>
I'm not quite sure why gcc is putting this extra 8 byte frame (maybe
some optimization?), but it isn't being accounted for thus the
FUNCTION_GRAPH_FP_TEST for arm fails. Note that only some functions do
this. Function graph works with FUNCTION_GRAPH_FP_TEST disabled, so it
looks the test is hitting false positives.
Thanks for the report.
It appears the sub instruction at 0x6dd0 correctly accounts for the
extra 8 bytes, so the frame pointer is valid. So it is our assumption
that there are no gaps between the stack frames is invalid.
Thanks for the assistance. The gap between the stack frame depends on
the function. Most do not have a gap. Some have 8 (as shown above), some
have 12. A single assumption here is not going to work. I'm having a
hard time finding out the reasoning for this gap. I tried disabling a
bunch of gcc flags as well as -O2 and the gap still exists.
Thanks,
Justin
Could you try the following change please?
--- a/arch/arm/kernel/ftrace.c
+++ b/arch/arm/kernel/ftrace.c
@@ -235,8 +235,12 @@
return;
if (IS_ENABLED(CONFIG_UNWINDER_FRAME_POINTER)) {
- /* FP points one word below parent's top of stack */
- frame_pointer += 4;
+ /*
+* The top of stack of the parent is recorded in the stack
+* frame at offset [fp, #-8].
+*/
+ get_kernel_nofault(frame_pointer,
+ (unsigned long *)(frame_pointer - 8));
} else {
struct stackframe frame = {
.fp = frame_pointer,
smime.p7s
Description: S/MIME Cryptographic Signature
ARM Ftrace Function Graph Fails With UNWINDER_FRAME_POINTER
Hello,
Ran into an odd bug that I am unsure what the solution is. Tested a few
kernels versions and they all fail the same.
FUNCTION_GRAPH_FP_TEST was enabled with 953f534a7ed6 ("ARM: ftrace:
enable HAVE_FUNCTION_GRAPH_FP_TEST"). This test fails when
UNWINDER_FRAME_POINTER is enabled. Enable function_graph tracer and you
should see a failure similar to below.
[ 63.817239] [ cut here ]
[ 63.822006] WARNING: CPU: 3 PID: 1185 at kernel/trace/fgraph.c:195
ftrace_return_to_handler+0x228/0x374
[ 63.831645] Bad frame pointer: expected d1e0df40, received d1e0df48
[ 63.831645] from func packet_setsockopt return to c0b558f4
[ 63.843801] Modules linked in: bdc udc_core
[ 63.848246] CPU: 3 PID: 1185 Comm: udhcpc Not tainted
6.1.53-0.1pre-gf0bc552d12f8 #33
[ 63.856209] Hardware name: Broadcom STB (Flattened Device Tree)
[ 63.862227] Backtrace:
[ 63.864761] dump_backtrace from show_stack+0x20/0x24
[ 63.869982] r7:c031cd8c r6:0009 r5:0013 r4:c11c7fac
[ 63.875736] show_stack from dump_stack_lvl+0x48/0x54
[ 63.880929] dump_stack_lvl from dump_stack+0x18/0x1c
[ 63.886111] r5:00c3 r4:c11bd92c
[ 63.889764] dump_stack from __warn+0x88/0x130
[ 63.894339] __warn from warn_slowpath_fmt+0x140/0x198
[ 63.899631] r8:d1e0deac r7:c11bd958 r6:c031cd8c r5:c11bd92c r4:
[ 63.906431] warn_slowpath_fmt from ftrace_return_to_handler+0x228/0x374
[ 63.913294] r8:c3a8d840 r7:0002 r6:d1e0df48 r5:c2377a94 r4:c269a400
[ 63.920095] ftrace_return_to_handler from return_to_handler+0xc/0x18
[ 63.926699] r8:c0cd8ed0 r7:0008 r6:c418c500 r5:0004 r4:0107
[ 63.933500] __sys_setsockopt from return_to_handler+0x0/0x18
[ 63.939415] r8:c02002bc r7:0126 r6:0003 r5: r4:0004
[ 63.946217] sys_setsockopt from return_to_handler+0x0/0x18
[ 63.952053] ---[ end trace ]---
Sure enough the top of the parent stack is off by 8. (Tested with
gcc6.3/gcc8.3/gcc12.3)
6dcc :
6dcc: e1a0c00d mov ip, sp
6dd0: e24dd008 sub sp, sp, #8 <==
6dd4: e92ddff0 push {r4, r5, r6, r7, r8, r9, sl,
fp, ip, lr, pc}
6dd8: e24cb00c sub fp, ip, #12
6ddc: e24dd06c sub sp, sp, #108 @ 0x6c
6de0: e52de004 push {lr} @ (str lr, [sp,
#-4]!)
6de4: ebfe bl 0 <__gnu_mcount_nc>
I'm not quite sure why gcc is putting this extra 8 byte frame (maybe
some optimization?), but it isn't being accounted for thus the
FUNCTION_GRAPH_FP_TEST for arm fails. Note that only some functions do
this. Function graph works with FUNCTION_GRAPH_FP_TEST disabled, so it
looks the test is hitting false positives.
Thanks,
Justin
smime.p7s
Description: S/MIME Cryptographic Signature
Re: [PATCH v2 0/2] get_maintainer: add patch-only keyword matching
On Fri, Sep 29, 2023 at 11:50 AM Joe Perches wrote: > > On Fri, 2023-09-29 at 11:07 +0900, Justin Stitt wrote: > > On Fri, Sep 29, 2023 at 12:52 AM Nick Desaulniers > > wrote: > > > > > > On Wed, Sep 27, 2023 at 11:09 PM Joe Perches wrote: > > > > > > > > On Thu, 2023-09-28 at 14:31 +0900, Justin Stitt wrote: > > > > > On Thu, Sep 28, 2023 at 2:01 PM Joe Perches wrote: > > > > > > > > > > > > On Thu, 2023-09-28 at 04:23 +, Justin Stitt wrote: > > > > > > > Changes in v2: > > > > > > > - remove formatting pass (thanks Joe) (but seriously the > > > > > > > formatting is > > > > > > > bad, is there opportunity to get a formatting pass in here at > > > > > > > some > > > > > > > point?) > > > > > > > > LG G7 Battery Replacement | Guide | Is it actually a Samsung? I t > > > > > > Why? What is it that makes you believe the formatting is bad? > > > > > > > > > > > > > > > > Investigating further, it looked especially bad in my editor. There is > > > > > a mixture of > > > > > tabs and spaces and my vim tabstop is set to 4 for pl files. Setting > > > > > this to > > > > > 8 is a whole lot better. But I still see some weird spacing > > > > > > > > > > > > > Yes, it's a bit odd indentation. > > > > It's emacs default perl format. > > > > 4 space indent with 8 space tabs, maximal tab fill. > > > > > > > > > > Oh! What?! That's the most surprising convention I've ever heard of > > > (after the GNU C coding style). Yet another thing to hold against > > > perl I guess. :P > > > > > > I have my editor setup to highlight tabs vs spaces via visual cues, so > > > that I don't mess up kernel coding style. (`git clang-format HEAD~` > > > after a commit helps). scripts/get_maintainer.pl has some serious > > > inconsistencies to the point where I'm not sure what it should or was > > > meant to be. Now that you mention it, I see it, and it does seem > > > consistent in that regard. > > > > > > Justin, is your formatter configurable to match that convention? > > > Maybe it's still useful, as long as you configure it to stick to the > > > pre-existing convention. > > > > Negative, all the perl formatters I've tried will convert everything to > > spaces. > > The best I've seen is perltidy. > > > > https://gist.github.com/JustinStitt/347385921c80a5212c2672075aa769b6 > > emacs with cperl mode works fine. > > I don't know much about vim, but when I open this file in vim > it looks perfectly normal and it's apparently properly syntax > highlighted. > I believe a :set tabstop=2 will make it look weird. But really, this whole formatting thing is a non-issue for me personally once I discovered what the problem was. I'm not sure this file attracts nearly enough eyes to warrant an eager formatting attempt as I was previously preaching for. Nick and I using vim with special tab handling are most definitely the exception and for most folks this file probably looks fine.
Re: [PATCH v2 0/2] get_maintainer: add patch-only keyword matching
On Fri, Sep 29, 2023 at 12:52 AM Nick Desaulniers wrote: > > On Wed, Sep 27, 2023 at 11:09 PM Joe Perches wrote: > > > > On Thu, 2023-09-28 at 14:31 +0900, Justin Stitt wrote: > > > On Thu, Sep 28, 2023 at 2:01 PM Joe Perches wrote: > > > > > > > > On Thu, 2023-09-28 at 04:23 +, Justin Stitt wrote: > > > > > Changes in v2: > > > > > - remove formatting pass (thanks Joe) (but seriously the formatting is > > > > > bad, is there opportunity to get a formatting pass in here at some > > > > > point?) > > > > > > > > Why? What is it that makes you believe the formatting is bad? > > > > > > > > > > Investigating further, it looked especially bad in my editor. There is > > > a mixture of > > > tabs and spaces and my vim tabstop is set to 4 for pl files. Setting this > > > to > > > 8 is a whole lot better. But I still see some weird spacing > > > > > > > Yes, it's a bit odd indentation. > > It's emacs default perl format. > > 4 space indent with 8 space tabs, maximal tab fill. > > > > Oh! What?! That's the most surprising convention I've ever heard of > (after the GNU C coding style). Yet another thing to hold against > perl I guess. :P > > I have my editor setup to highlight tabs vs spaces via visual cues, so > that I don't mess up kernel coding style. (`git clang-format HEAD~` > after a commit helps). scripts/get_maintainer.pl has some serious > inconsistencies to the point where I'm not sure what it should or was > meant to be. Now that you mention it, I see it, and it does seem > consistent in that regard. > > Justin, is your formatter configurable to match that convention? > Maybe it's still useful, as long as you configure it to stick to the > pre-existing convention. Negative, all the perl formatters I've tried will convert everything to spaces. The best I've seen is perltidy. https://gist.github.com/JustinStitt/347385921c80a5212c2672075aa769b6 > -- > Thanks, > ~Nick Desaulniers
Re: [PATCH v2 1/2] get_maintainer: add patch-only keyword-matching
On Thu, Sep 28, 2023 at 1:46 PM Joe Perches wrote:
>
> On Thu, 2023-09-28 at 04:23 +, Justin Stitt wrote:
> > Add the "D:" type which behaves the same as "K:" but will only match
> > content present in a patch file.
> >
> > To illustrate:
> >
> > Imagine this entry in MAINTAINERS:
> >
> > NEW REPUBLIC
> > M: Han Solo
> > W: https://www.jointheresistance.org
> > D: \bstrncpy\b
> >
> > Our maintainer, Han, will only be added to the recipients if a patch
> > file is passed to get_maintainer (like what b4 does):
> > $ ./scripts/get_maintainer.pl 0004-some-change.patch
> >
> > If the above patch has a `strncpy` present in the subject, commit log or
> > diff then Han will be to/cc'd.
> >
> > However, in the event of a file from the tree given like:
> > $ ./scripts/get_maintainer.pl ./lib/string.c
> >
> > Han will not be noisily to/cc'd (like a K: type would in this
> > circumstance)
> >
> > Signed-off-by: Justin Stitt
> > ---
> > MAINTAINERS | 5 +
> > scripts/get_maintainer.pl | 12 ++--
> > 2 files changed, 15 insertions(+), 2 deletions(-)
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index b19995690904..94e431daa7c2 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -59,6 +59,11 @@ Descriptions of section entries and preferred order
> > matches patches or files that contain one or more of the words
> > printk, pr_info or pr_err
> > One regex pattern per line. Multiple K: lines acceptable.
> > + D: *Diff content regex* (perl extended) pattern match that applies only
> > to
> > + patches and not entire files (e.g. when using the get_maintainers.pl
> > + script).
> > + Usage same as K:.
> > +
> >
> > Maintainers List
> >
> > diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
> > index ab123b498fd9..a3e697926ddd 100755
> > --- a/scripts/get_maintainer.pl
> > +++ b/scripts/get_maintainer.pl
> > @@ -342,6 +342,7 @@ if ($tree && !top_of_kernel_tree($lk_path)) {
> >
> > my @typevalue = ();
> > my %keyword_hash;
> > +my %patch_keyword_hash;
> > my @mfiles = ();
> > my @self_test_info = ();
> >
> > @@ -369,8 +370,10 @@ sub read_maintainer_file {
> > $value =~ s@([^/])$@$1/@;
> > }
> > } elsif ($type eq "K") {
> > - $keyword_hash{@typevalue} = $value;
> > - }
> > + $keyword_hash{@typevalue} = $value;
> > + } elsif ($type eq "D") {
> > + $patch_keyword_hash{@typevalue} = $value;
> > + }
> > push(@typevalue, "$type:$value");
> > } elsif (!(/^\s*$/ || /^\s*\#/)) {
> > push(@typevalue, $line);
> > @@ -607,6 +610,11 @@ foreach my $file (@ARGV) {
> > push(@keyword_tvi, $line);
> > }
> > }
> > +foreach my $line(keys %patch_keyword_hash) {
> > + if ($patch_line =~ m/${patch_prefix}$patch_keyword_hash{$line}/x) {
> > +push(@keyword_tvi, $line);
> > + }
> > +}
> > }
> > }
> > close($patch);
> >
>
>
> My opinion: Nack.
>
> I think something like this would be better
> as it avoids duplication of K and D content.
If I understand correctly, this puts the onus on the get_maintainer users
to select the right argument whereas adding "D:", albeit with some
duplicate code, allows maintainers themselves to decide in exactly
which context they receive mail.
Adding a command line flag means sometimes K: is treated one
way and sometimes treated a different way depending on
the specific incantation.
This could all be a moot point, though, as I believe Konstantin
is trying to separate out the whole idea of a patch-sender needing
to specify the recipients of a patch. Instead some middleware would
capture mail and delegate automatically based on some queries
set up by maintainers.
Exciting idea, I wonder what the progress is on that?
Cc: Konstantin Ryabitsev
[1]: https://lore.kernel.org/all/20230726-june-mocha-ad6809@meerkat/
> ---
> scripts/get_maintainer.pl | 16 +---
> 1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
> index ab123b498fd9..07e7d744cadb 100755
> --- a/scripts/get_maintainer.pl
> +++ b/scripts/get_maintainer.pl
> @@ -57,6 +57,7 @@ my $subsystem = 0;
> my $st
[PATCH v2 2/2] MAINTAINERS: migrate some K to D
Let's get the ball rolling with some changes from K to D. Ultimately, if it turns out that 100% of K users want to change to D then really the behavior of K could just be changed. Signed-off-by: Justin Stitt Original-author: Kees Cook --- MAINTAINERS | 16 +--- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/MAINTAINERS b/MAINTAINERS index 94e431daa7c2..80ffdaa8f044 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -5038,7 +5038,7 @@ F:Documentation/kbuild/llvm.rst F: include/linux/compiler-clang.h F: scripts/Makefile.clang F: scripts/clang-tools/ -K: \b(?i:clang|llvm)\b +D: \b(?i:clang|llvm)\b CLK API M: Russell King @@ -8149,7 +8149,7 @@ F:lib/strcat_kunit.c F: lib/strscpy_kunit.c F: lib/test_fortify/* F: scripts/test_fortify.sh -K: \b__NO_FORTIFY\b +D: \b__NO_FORTIFY\b FPGA DFL DRIVERS M: Wu Hao @@ -11405,8 +11405,10 @@ F: Documentation/ABI/testing/sysfs-kernel-warn_count F: include/linux/overflow.h F: include/linux/randomize_kstack.h F: mm/usercopy.c -K: \b(add|choose)_random_kstack_offset\b -K: \b__check_(object_size|heap_object)\b +D: \b(add|choose)_random_kstack_offset\b +D: \b__check_(object_size|heap_object)\b +D: \b__counted_by\b + KERNEL JANITORS L: kernel-janit...@vger.kernel.org @@ -17290,7 +17292,7 @@ F: drivers/acpi/apei/erst.c F: drivers/firmware/efi/efi-pstore.c F: fs/pstore/ F: include/linux/pstore* -K: \b(pstore|ramoops) +D: \b(pstore|ramoops) PTP HARDWARE CLOCK SUPPORT M: Richard Cochran @@ -19231,8 +19233,8 @@ F: include/uapi/linux/seccomp.h F: kernel/seccomp.c F: tools/testing/selftests/kselftest_harness.h F: tools/testing/selftests/seccomp/* -K: \bsecure_computing -K: \bTIF_SECCOMP\b +D: \bsecure_computing +D: \bTIF_SECCOMP\b SECURE DIGITAL HOST CONTROLLER INTERFACE (SDHCI) Broadcom BRCMSTB DRIVER M: Kamal Dasu -- 2.42.0.582.g8ccd20d70d-goog
[PATCH v2 1/2] get_maintainer: add patch-only keyword-matching
Add the "D:" type which behaves the same as "K:" but will only match
content present in a patch file.
To illustrate:
Imagine this entry in MAINTAINERS:
NEW REPUBLIC
M: Han Solo
W: https://www.jointheresistance.org
D: \bstrncpy\b
Our maintainer, Han, will only be added to the recipients if a patch
file is passed to get_maintainer (like what b4 does):
$ ./scripts/get_maintainer.pl 0004-some-change.patch
If the above patch has a `strncpy` present in the subject, commit log or
diff then Han will be to/cc'd.
However, in the event of a file from the tree given like:
$ ./scripts/get_maintainer.pl ./lib/string.c
Han will not be noisily to/cc'd (like a K: type would in this
circumstance)
Signed-off-by: Justin Stitt
---
MAINTAINERS | 5 +
scripts/get_maintainer.pl | 12 ++--
2 files changed, 15 insertions(+), 2 deletions(-)
diff --git a/MAINTAINERS b/MAINTAINERS
index b19995690904..94e431daa7c2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -59,6 +59,11 @@ Descriptions of section entries and preferred order
matches patches or files that contain one or more of the words
printk, pr_info or pr_err
One regex pattern per line. Multiple K: lines acceptable.
+ D: *Diff content regex* (perl extended) pattern match that applies only to
+ patches and not entire files (e.g. when using the get_maintainers.pl
+ script).
+ Usage same as K:.
+
Maintainers List
diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
index ab123b498fd9..a3e697926ddd 100755
--- a/scripts/get_maintainer.pl
+++ b/scripts/get_maintainer.pl
@@ -342,6 +342,7 @@ if ($tree && !top_of_kernel_tree($lk_path)) {
my @typevalue = ();
my %keyword_hash;
+my %patch_keyword_hash;
my @mfiles = ();
my @self_test_info = ();
@@ -369,8 +370,10 @@ sub read_maintainer_file {
$value =~ s@([^/])$@$1/@;
}
} elsif ($type eq "K") {
- $keyword_hash{@typevalue} = $value;
- }
+ $keyword_hash{@typevalue} = $value;
+ } elsif ($type eq "D") {
+ $patch_keyword_hash{@typevalue} = $value;
+ }
push(@typevalue, "$type:$value");
} elsif (!(/^\s*$/ || /^\s*\#/)) {
push(@typevalue, $line);
@@ -607,6 +610,11 @@ foreach my $file (@ARGV) {
push(@keyword_tvi, $line);
}
}
+foreach my $line(keys %patch_keyword_hash) {
+ if ($patch_line =~ m/${patch_prefix}$patch_keyword_hash{$line}/x) {
+push(@keyword_tvi, $line);
+ }
+}
}
}
close($patch);
--
2.42.0.582.g8ccd20d70d-goog
[PATCH v2 0/2] get_maintainer: add patch-only keyword matching
This series aims to add "D:" which behaves exactly the same as "K:" but works only on patch files. The goal of this is to reduce noise when folks use get_maintainer on tree files as opposed to patches. "D:" should help maintainers reduce noise in their inboxes, especially when matching omnipresent keywords like [1]. In the event of [1] Kees would be to/cc'd from folks running get_maintainer on _any_ file containing "__counted_by". The number of these files is rising and I fear for his inbox as his goal, as I understand it, is to simply monitor the introduction of new __counted_by annotations to ensure accurate semantics. Joe mentioned in v1 that perhaps K: should be reworked to only consider patch files. I wonder, though, if folks are intentionally using the current behavior of K: and thus would be peeved from a change there. I see this series as, at the very least, a gentle migration in behavior which is purely opt-in and at some point could eagerly be merged with K:. [1]: https://lore.kernel.org/all/20230925172037.work.853-k...@kernel.org/ Signed-off-by: Justin Stitt --- Changes in v2: - remove bits about non-patch usage being bad (thanks Greg, Kees, et al.) - remove formatting pass (thanks Joe) (but seriously the formatting is bad, is there opportunity to get a formatting pass in here at some point?) - add some migration from K to D (thanks Kees, Nick) - Link to v1: https://lore.kernel.org/r/20230927-get_maintainer_add_d-v1-0-28c207229...@google.com --- Justin Stitt (2): get_maintainer: add patch-only keyword-matching MAINTAINERS: migrate some K to D MAINTAINERS | 21 ++--- scripts/get_maintainer.pl | 12 ++-- 2 files changed, 24 insertions(+), 9 deletions(-) --- base-commit: 6465e260f48790807eef06b583b38ca9789b6072 change-id: 20230926-get_maintainer_add_d-07424a814e72 Best regards, -- Justin Stitt
Re: [PATCH 3/3] get_maintainer: add patch-only pattern matching type
On Wed, Sep 27, 2023 at 3:14 PM Greg KH wrote: > > On Wed, Sep 27, 2023 at 03:19:16AM +, Justin Stitt wrote: > > Note that folks really shouldn't be using get_maintainer on tree files > > anyways [1]. > > That's not true, Linus and I use it on a daily basis this way, it's part > of our normal workflow, AND the workflow of the kernel security team. > > So please don't take that valid use-case away from us. Fair. I'm on the side of keeping the "K:'' behavior the way it is and that's why I'm proposing adding "D:" to provide a more granular content matching type operating strictly on patches. It's purely opt-in. The patch I linked mentioned steering folks away from using tree files but not necessarily removing the behavior. > > thanks, > > greg k-h Thanks Justin
[PATCH 3/3] get_maintainer: add patch-only pattern matching type
Add the "D:" type which behaves the same as "K:" but will only match
content present in a patch file.
To illustrate:
Imagine this entry in MAINTAINERS:
NEW REPUBLIC
M: Han Solo
W: https://www.jointheresistance.org
D: \bstrncpy\b
Our maintainer, Han, will only be added to the recipients if a patch
file is passed to get_maintainer (like what b4 does):
$ ./scripts/get_maintainer.pl 0004-some-change.patch
If the above patch has a `strncpy` present in the subject, commit log or
diff then Han will be to/cc'd.
However, in the event of a file from the tree given like:
$ ./scripts/get_maintainer.pl ./lib/string.c
Han will not be noisily to/cc'd (like a K: type would in this
circumstance)
Note that folks really shouldn't be using get_maintainer on tree files
anyways [1].
[1]: https://lore.kernel.org/all/20230726151515.1650519-1-k...@kernel.org/
---
scripts/get_maintainer.pl | 9 +
1 file changed, 9 insertions(+)
diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
index e679eac96793..f290bf0948f1 100755
--- a/scripts/get_maintainer.pl
+++ b/scripts/get_maintainer.pl
@@ -309,6 +309,7 @@ if ( $tree && !top_of_kernel_tree($lk_path) ) {
my @typevalue = ();
my %keyword_hash;
+my %patch_keyword_hash;
my @mfiles = ();
my @self_test_info = ();
@@ -339,6 +340,9 @@ sub read_maintainer_file {
elsif ( $type eq "K" ) {
$keyword_hash{@typevalue} = $value;
}
+elsif ( $type eq "D" ) {
+$patch_keyword_hash{@typevalue} = $value;
+}
push( @typevalue, "$type:$value" );
}
elsif ( !( /^\s*$/ || /^\s*\#/ ) ) {
@@ -591,6 +595,11 @@ foreach my $file (@ARGV) {
push( @keyword_tvi, $line );
}
}
+foreach my $line ( keys %patch_keyword_hash ) {
+if ($patch_line =~
m/${patch_prefix}$patch_keyword_hash{$line}/x ) {
+push( @keyword_tvi, $line );
+}
+}
}
}
close($patch);
--
2.42.0.582.g8ccd20d70d-goog
[PATCH 1/3] MAINTAINERS: add documentation for D:
Document what "D:" does. This is more or less the same as what "K:" does but only works for patch files. See [3/3] for more info and an illustrative example. --- MAINTAINERS | 3 +++ 1 file changed, 3 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index b19995690904..de68d2c0cf29 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -59,6 +59,9 @@ Descriptions of section entries and preferred order matches patches or files that contain one or more of the words printk, pr_info or pr_err One regex pattern per line. Multiple K: lines acceptable. + D: *Content regex* (perl extended) pattern match patches only. + Usage same as K:. + Maintainers List -- 2.42.0.582.g8ccd20d70d-goog
Re: [PATCH 1/3] MAINTAINERS: add documentation for D:
On Wed, Sep 27, 2023 at 12:27 PM Joe Perches wrote: > > On Wed, 2023-09-27 at 03:19 +, Justin Stitt wrote: > > Document what "D:" does. > > > > This is more or less the same as what "K:" does but only works for patch > > files. > > Nack. I'd rather just add a !$file test to K: patterns. Are there no legitimate use cases for K:'s current behavior to warrant keeping it around? >
[PATCH 0/3] get_maintainer: add patch-only keyword matching
This series aims to add "D:" which behaves exactly the same as "K:" but works only on patch files. The goal of this is to reduce noise when folks use get_maintainer on tree files as opposed to patches. This use case should be steered away from [1] but "D:" should help maintainers reduce noise in their inboxes regardless, especially when matching omnipresent keywords like [2]. In the event of [2] Kees would be to/cc'd from folks running get_maintainer on _any_ file containing "__counted_by". The number of these files is rising and I fear for his inbox as his goal, as I understand it, is to simply monitor the introduction of new __counted_by annotations to ensure accurate semantics. See [3/3] for an illustrative example. This series also includes a formatting pass over get_maintainer because I personally found it difficult to parse with the human eye. [1]: https://lore.kernel.org/all/20230726151515.1650519-1-k...@kernel.org/ [2]: https://lore.kernel.org/all/20230925172037.work.853-k...@kernel.org/ Signed-off-by: Justin Stitt --- Justin Stitt (3): MAINTAINERS: add documentation for D: get_maintainer: run perltidy get_maintainer: add patch-only pattern matching type MAINTAINERS |3 + scripts/get_maintainer.pl | 3334 +++-- 2 files changed, 1718 insertions(+), 1619 deletions(-) --- base-commit: 6465e260f48790807eef06b583b38ca9789b6072 change-id: 20230926-get_maintainer_add_d-07424a814e72 Best regards, -- Justin Stitt
[PATCH] HID: uhid: refactor deprecated strncpy
`strncpy` is deprecated for use on NUL-terminated destination strings [1].
We should prefer more robust and less ambiguous string interfaces.
A suitable replacement is `strscpy` [2] due to the fact that it
guarantees NUL-termination on the destination buffer without
unnecessarily NUL-padding.
Looking at: Commit 4d26d1d1e806 ("Revert "HID: uhid: use strlcpy() instead of
strncpy()"")
we see referenced the fact that many attempts have been made to change
these strncpy's into strlcpy to no success. I think strscpy is an
objectively better interface here as it doesn't unnecessarily NUL-pad
the destination buffer whilst allowing us to drop the `len = min(...)`
pattern as strscpy will implicitly limit the number of bytes copied by
the smaller of its dest and src arguments.
So while the existing code may not have a bug (i.e: overread problems)
we should still favor strscpy due to readability (plus a very slight
performance boost).
Link:
https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
[1]
Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2]
Link: https://github.com/KSPP/linux/issues/90
Cc: linux-harden...@vger.kernel.org
Cc: Kees Cook
Signed-off-by: Justin Stitt
---
drivers/hid/uhid.c | 12
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
index 4588d2cd4ea4..00e1566ad37b 100644
--- a/drivers/hid/uhid.c
+++ b/drivers/hid/uhid.c
@@ -490,7 +490,7 @@ static int uhid_dev_create2(struct uhid_device *uhid,
const struct uhid_event *ev)
{
struct hid_device *hid;
- size_t rd_size, len;
+ size_t rd_size;
void *rd_data;
int ret;
@@ -514,13 +514,9 @@ static int uhid_dev_create2(struct uhid_device *uhid,
goto err_free;
}
- /* @hid is zero-initialized, strncpy() is correct, strlcpy() not */
- len = min(sizeof(hid->name), sizeof(ev->u.create2.name)) - 1;
- strncpy(hid->name, ev->u.create2.name, len);
- len = min(sizeof(hid->phys), sizeof(ev->u.create2.phys)) - 1;
- strncpy(hid->phys, ev->u.create2.phys, len);
- len = min(sizeof(hid->uniq), sizeof(ev->u.create2.uniq)) - 1;
- strncpy(hid->uniq, ev->u.create2.uniq, len);
+ strscpy(hid->name, ev->u.create2.name, sizeof(hid->name));
+ strscpy(hid->phys, ev->u.create2.phys, sizeof(hid->phys));
+ strscpy(hid->uniq, ev->u.create2.uniq, sizeof(hid->uniq));
hid->ll_driver = _hid_driver;
hid->bus = ev->u.create2.bus;
---
base-commit: 3669558bdf354cd352be955ef2764cde6a9bf5ec
change-id: 20230914-strncpy-drivers-hid-uhid-c-a465f3e784dd
Best regards,
--
Justin Stitt
[PATCH] HID: prodikeys: refactor deprecated strncpy
`strncpy` is deprecated for use on NUL-terminated destination strings [1]. We should prefer more robust and less ambiguous string interfaces. A suitable replacement is `strscpy` [2] due to the fact that it guarantees NUL-termination on the destination buffer without unnecessarily NUL-padding. Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2] Link: https://github.com/KSPP/linux/issues/90 Cc: linux-harden...@vger.kernel.org Signed-off-by: Justin Stitt --- Note: for some reason if NUL-padding is needed let's opt for `strscpy_pad()` --- drivers/hid/hid-prodikeys.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/hid/hid-prodikeys.c b/drivers/hid/hid-prodikeys.c index e4e9471d0f1e..c16d2ba6ea16 100644 --- a/drivers/hid/hid-prodikeys.c +++ b/drivers/hid/hid-prodikeys.c @@ -639,9 +639,9 @@ static int pcmidi_snd_initialise(struct pcmidi_snd *pm) goto fail; } - strncpy(card->driver, shortname, sizeof(card->driver)); - strncpy(card->shortname, shortname, sizeof(card->shortname)); - strncpy(card->longname, longname, sizeof(card->longname)); + strscpy(card->driver, shortname, sizeof(card->driver)); + strscpy(card->shortname, shortname, sizeof(card->shortname)); + strscpy(card->longname, longname, sizeof(card->longname)); /* Set up rawmidi */ err = snd_rawmidi_new(card, card->shortname, 0, @@ -652,7 +652,7 @@ static int pcmidi_snd_initialise(struct pcmidi_snd *pm) goto fail; } pm->rwmidi = rwmidi; - strncpy(rwmidi->name, card->shortname, sizeof(rwmidi->name)); + strscpy(rwmidi->name, card->shortname, sizeof(rwmidi->name)); rwmidi->info_flags = SNDRV_RAWMIDI_INFO_INPUT; rwmidi->private_data = pm; --- base-commit: 3669558bdf354cd352be955ef2764cde6a9bf5ec change-id: 20230914-strncpy-drivers-hid-hid-prodikeys-c-cf42614a21d4 Best regards, -- Justin Stitt
[PATCH] firmware: ti_sci: refactor deprecated strncpy
`strncpy` is deprecated for use on NUL-terminated destination strings [1]. We should prefer more robust and less ambiguous string interfaces. A suitable replacement is `strscpy` [2] due to the fact that it guarantees NUL-termination on the destination buffer. It does not seem like `ver->firmware_description` requires NUL-padding (which is a behavior that strncpy provides) but if it does let's opt for `strscpy_pad()`. Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2] Link: https://github.com/KSPP/linux/issues/90 Cc: linux-harden...@vger.kernel.org Signed-off-by: Justin Stitt --- Note: build-tested only. --- drivers/firmware/ti_sci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/firmware/ti_sci.c b/drivers/firmware/ti_sci.c index 26a37f47f4ca..ce546f391959 100644 --- a/drivers/firmware/ti_sci.c +++ b/drivers/firmware/ti_sci.c @@ -485,7 +485,7 @@ static int ti_sci_cmd_get_revision(struct ti_sci_info *info) ver->abi_major = rev_info->abi_major; ver->abi_minor = rev_info->abi_minor; ver->firmware_revision = rev_info->firmware_revision; - strncpy(ver->firmware_description, rev_info->firmware_description, + strscpy(ver->firmware_description, rev_info->firmware_description, sizeof(ver->firmware_description)); fail: --- base-commit: 3669558bdf354cd352be955ef2764cde6a9bf5ec change-id: 20230913-strncpy-drivers-firmware-ti_sci-c-22667413c18f Best regards, -- Justin Stitt
[PATCH] firmware: tegra: bpmp: refactor deprecated strncpy
`strncpy` is deprecated for use on NUL-terminated destination strings [1].
We should prefer more robust and less ambiguous string interfaces.
It seems like the filename stored at `namevirt` is expected to be
NUL-terminated.
A suitable replacement is `strscpy_pad` due to the fact that it
guarantees NUL-termination on the destination buffer whilst maintaining
the NUL-padding behavior that strncpy provides.
Link:
https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
[1]
Link: https://github.com/KSPP/linux/issues/90
Cc: linux-harden...@vger.kernel.org
Signed-off-by: Justin Stitt
---
Note: compile tested only.
---
drivers/firmware/tegra/bpmp-debugfs.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/firmware/tegra/bpmp-debugfs.c
b/drivers/firmware/tegra/bpmp-debugfs.c
index 6dfe3d34109e..bbcdd9fed3fb 100644
--- a/drivers/firmware/tegra/bpmp-debugfs.c
+++ b/drivers/firmware/tegra/bpmp-debugfs.c
@@ -610,7 +610,7 @@ static int debugfs_show(struct seq_file *m, void *p)
}
len = strlen(filename);
- strncpy(namevirt, filename, namesize);
+ strscpy_pad(namevirt, filename, namesize);
err = mrq_debugfs_read(bpmp, namephys, len, dataphys, datasize,
);
@@ -661,7 +661,7 @@ static ssize_t debugfs_store(struct file *file, const char
__user *buf,
}
len = strlen(filename);
- strncpy(namevirt, filename, namesize);
+ strscpy_pad(namevirt, filename, namesize);
if (copy_from_user(datavirt, buf, count)) {
err = -EFAULT;
---
base-commit: 3669558bdf354cd352be955ef2764cde6a9bf5ec
change-id: 20230913-strncpy-drivers-firmware-tegra-bpmp-debugfs-c-54f7baaf32c0
Best regards,
--
Justin Stitt
[PATCH v3] EDAC/mc_sysfs: refactor deprecated strncpy
`strncpy` is deprecated for use on NUL-terminated destination strings [1]. We should prefer more robust and less ambiguous string interfaces. A suitable replacement is `strscpy` [2] due to the fact that it guarantees NUL-termination on the destination buffer without needlessly NUL-padding. Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2] Link: https://github.com/KSPP/linux/issues/90 Cc: linux-harden...@vger.kernel.org Signed-off-by: Justin Stitt --- Changes in v3: - prefer strscpy to strscpy_pad (thanks Tony) - Link to v2: https://lore.kernel.org/r/20230913-strncpy-drivers-edac-edac_mc_sysfs-c-v2-1-2d2e6bd43...@google.com Changes in v2: - included refactor of another strncpy in same file - Link to v1: https://lore.kernel.org/r/20230913-strncpy-drivers-edac-edac_mc_sysfs-c-v1-1-d232891b0...@google.com --- Note: build-tested only. --- drivers/edac/edac_mc_sysfs.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c index 15f63452a9be..9a5b4bbd8191 100644 --- a/drivers/edac/edac_mc_sysfs.c +++ b/drivers/edac/edac_mc_sysfs.c @@ -229,8 +229,7 @@ static ssize_t channel_dimm_label_store(struct device *dev, if (copy_count == 0 || copy_count >= sizeof(rank->dimm->label)) return -EINVAL; - strncpy(rank->dimm->label, data, copy_count); - rank->dimm->label[copy_count] = '\0'; + strscpy(rank->dimm->label, data, copy_count); return count; } @@ -535,7 +534,7 @@ static ssize_t dimmdev_label_store(struct device *dev, if (copy_count == 0 || copy_count >= sizeof(dimm->label)) return -EINVAL; - strncpy(dimm->label, data, copy_count); + strscpy(dimm->label, data, copy_count); dimm->label[copy_count] = '\0'; return count; --- base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c change-id: 20230913-strncpy-drivers-edac-edac_mc_sysfs-c-e619b00124a3 Best regards, -- Justin Stitt
Re: [PATCH] EDAC/mc_sysfs: refactor deprecated strncpy
On Wed, Sep 13, 2023 at 8:13 AM Luck, Tony wrote: > > > `strncpy` is deprecated for use on NUL-terminated destination strings [1]. > > > > We should prefer more robust and less ambiguous string interfaces. > > > > A suitable replacement is `strscpy_pad` [2] due to the fact that it > > guarantees > > NUL-termination on the destination buffer whilst maintaining the > > NUL-padding behavior that `strncpy` provides. This may not be strictly > > necessary but as I couldn't understand what this code does I wanted to > > ensure that the functionality is the same. > > > > Link: > > https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings > > [1] > > Link: > > https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2] > > Link: https://github.com/KSPP/linux/issues/90 > > Cc: linux-harden...@vger.kernel.org > > Signed-off-by: Justin Stitt > > --- > > Note: build-tested only. > > --- > > drivers/edac/edac_mc_sysfs.c | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c > > index 15f63452a9be..b303309a63cf 100644 > > --- a/drivers/edac/edac_mc_sysfs.c > > +++ b/drivers/edac/edac_mc_sysfs.c > > @@ -229,8 +229,7 @@ static ssize_t channel_dimm_label_store(struct device > > *dev, > > if (copy_count == 0 || copy_count >= sizeof(rank->dimm->label)) > > return -EINVAL; > > > > - strncpy(rank->dimm->label, data, copy_count); > > - rank->dimm->label[copy_count] = '\0'; > > + strscpy_pad(rank->dimm->label, data, copy_count); > > That doc page says the problem with strncpy() is that it doesn't guarantee to > NUL terminate the target string. But this code is aware of that limitation and > zaps a '\0' at the end to be sure. > > So this code doesn't suffer from the potential problems. Right, the original code did not have an existing bug due to the reason you mentioned. However, I'm pretty keen on eliminating uses of this interface treewide as there is always a more robust and less ambiguous option. > > If it is going to be fixed, then some further analysis of the original code > would be wise. Just replacing with strscpy_pad() means the code probably > still suffers from the "needless performance penalty" also mentioned in > the deprecation document. Got it, sending a v2 that prefers `strscpy` to `strscpy_pad` resolving the performance issue. > > -Tony > Thanks for the timely review! Justin
[PATCH v2] ipmi: refactor deprecated strncpy
`strncpy` is deprecated for use on NUL-terminated destination strings [1].
In this case, strncpy is being used specifically for its NUL-padding
behavior (and has been commented as such). Moreover, the destination
string is not required to be NUL-terminated [2].
We can use a more robust and less ambiguous interface in
`memcpy_and_pad` which makes the code more readable and even eliminates
the need for that comment.
Let's also use `strnlen` instead of `strlen()` with an upper-bounds
check as this is intrinsically a part of `strnlen`.
Also included in this patch is a simple 1:1 change of `strncpy` to
`strscpy` for ipmi_ssif.c. If NUL-padding is wanted here as well then we
should opt again for `strscpy_pad`.
Link:
https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
[1]
Link: https://lore.kernel.org/all/zqeadybl0uz1n...@mail.minyard.net/ [2]
Link: https://github.com/KSPP/linux/issues/90
Cc: linux-harden...@vger.kernel.org
Cc: Kees Cook
Signed-off-by: Justin Stitt
---
Changes in v2:
- use memcpy_and_pad (thanks Corey)
- Link to v1:
https://lore.kernel.org/r/20230912-strncpy-drivers-char-ipmi-ipmi-v1-1-cc43e0d1c...@google.com
---
drivers/char/ipmi/ipmi_msghandler.c | 11 +++
drivers/char/ipmi/ipmi_ssif.c | 2 +-
2 files changed, 4 insertions(+), 9 deletions(-)
diff --git a/drivers/char/ipmi/ipmi_msghandler.c
b/drivers/char/ipmi/ipmi_msghandler.c
index 186f1fee7534..d6f14279684d 100644
--- a/drivers/char/ipmi/ipmi_msghandler.c
+++ b/drivers/char/ipmi/ipmi_msghandler.c
@@ -5377,20 +5377,15 @@ static void send_panic_events(struct ipmi_smi *intf,
char *str)
j = 0;
while (*p) {
- int size = strlen(p);
+ int size = strnlen(p, 11);
- if (size > 11)
- size = 11;
data[0] = 0;
data[1] = 0;
data[2] = 0xf0; /* OEM event without timestamp. */
data[3] = intf->addrinfo[0].address;
data[4] = j++; /* sequence # */
- /*
-* Always give 11 bytes, so strncpy will fill
-* it with zeroes for me.
-*/
- strncpy(data+5, p, 11);
+
+ memcpy_and_pad(data+5, 11, p, size, '\0');
p += size;
ipmi_panic_request_and_wait(intf, , );
diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
index 3b921c78ba08..edcb83765dce 100644
--- a/drivers/char/ipmi/ipmi_ssif.c
+++ b/drivers/char/ipmi/ipmi_ssif.c
@@ -1940,7 +1940,7 @@ static int new_ssif_client(int addr, char *adapter_name,
}
}
- strncpy(addr_info->binfo.type, DEVICE_NAME,
+ strscpy(addr_info->binfo.type, DEVICE_NAME,
sizeof(addr_info->binfo.type));
addr_info->binfo.addr = addr;
addr_info->binfo.platform_data = addr_info;
---
base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
change-id: 20230912-strncpy-drivers-char-ipmi-ipmi-dda47b3773fd
Best regards,
--
Justin Stitt
Re: [PATCH] EDAC/mc_sysfs: refactor deprecated strncpy
On Tue, Sep 12, 2023 at 6:26 PM Justin Stitt wrote: > > `strncpy` is deprecated for use on NUL-terminated destination strings [1]. > > We should prefer more robust and less ambiguous string interfaces. > > A suitable replacement is `strscpy_pad` [2] due to the fact that it guarantees > NUL-termination on the destination buffer whilst maintaining the > NUL-padding behavior that `strncpy` provides. This may not be strictly > necessary but as I couldn't understand what this code does I wanted to > ensure that the functionality is the same. > > Link: > https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings > [1] > Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html > [2] > Link: https://github.com/KSPP/linux/issues/90 > Cc: linux-harden...@vger.kernel.org > Signed-off-by: Justin Stitt > --- > Note: build-tested only. > --- > drivers/edac/edac_mc_sysfs.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c > index 15f63452a9be..b303309a63cf 100644 > --- a/drivers/edac/edac_mc_sysfs.c > +++ b/drivers/edac/edac_mc_sysfs.c > @@ -229,8 +229,7 @@ static ssize_t channel_dimm_label_store(struct device > *dev, > if (copy_count == 0 || copy_count >= sizeof(rank->dimm->label)) > return -EINVAL; > > - strncpy(rank->dimm->label, data, copy_count); > - rank->dimm->label[copy_count] = '\0'; > + strscpy_pad(rank->dimm->label, data, copy_count); > > return count; > } > > --- > base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c > change-id: 20230913-strncpy-drivers-edac-edac_mc_sysfs-c-e619b00124a3 > > Best regards, > -- > Justin Stitt > I typo'd my grep and initially missed refactoring another instance of strncpy in this same file. v2 [1] resolves this. [1]: https://lore.kernel.org/r/20230913-strncpy-drivers-edac-edac_mc_sysfs-c-v2-1-2d2e6bd43...@google.com
[PATCH v2] EDAC/mc_sysfs: refactor deprecated strncpy
`strncpy` is deprecated for use on NUL-terminated destination strings [1]. We should prefer more robust and less ambiguous string interfaces. A suitable replacement is `strscpy_pad` [2] due to the fact that it guarantees NUL-termination on the destination buffer whilst maintaining the NUL-padding behavior that `strncpy` provides. This may not be strictly necessary but as I couldn't understand what this code does I wanted to ensure that the functionality is the same. Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2] Link: https://github.com/KSPP/linux/issues/90 Cc: linux-harden...@vger.kernel.org Signed-off-by: Justin Stitt --- Changes in v2: - included refactor of another strncpy in same file - Link to v1: https://lore.kernel.org/r/20230913-strncpy-drivers-edac-edac_mc_sysfs-c-v1-1-d232891b0...@google.com --- Note: build-tested only. --- drivers/edac/edac_mc_sysfs.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c index 15f63452a9be..ce025a20288c 100644 --- a/drivers/edac/edac_mc_sysfs.c +++ b/drivers/edac/edac_mc_sysfs.c @@ -229,8 +229,7 @@ static ssize_t channel_dimm_label_store(struct device *dev, if (copy_count == 0 || copy_count >= sizeof(rank->dimm->label)) return -EINVAL; - strncpy(rank->dimm->label, data, copy_count); - rank->dimm->label[copy_count] = '\0'; + strscpy_pad(rank->dimm->label, data, copy_count); return count; } @@ -535,7 +534,7 @@ static ssize_t dimmdev_label_store(struct device *dev, if (copy_count == 0 || copy_count >= sizeof(dimm->label)) return -EINVAL; - strncpy(dimm->label, data, copy_count); + strscpy_pad(dimm->label, data, copy_count); dimm->label[copy_count] = '\0'; return count; --- base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c change-id: 20230913-strncpy-drivers-edac-edac_mc_sysfs-c-e619b00124a3 Best regards, -- Justin Stitt
[PATCH] EDAC/mc_sysfs: refactor deprecated strncpy
`strncpy` is deprecated for use on NUL-terminated destination strings [1]. We should prefer more robust and less ambiguous string interfaces. A suitable replacement is `strscpy_pad` [2] due to the fact that it guarantees NUL-termination on the destination buffer whilst maintaining the NUL-padding behavior that `strncpy` provides. This may not be strictly necessary but as I couldn't understand what this code does I wanted to ensure that the functionality is the same. Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2] Link: https://github.com/KSPP/linux/issues/90 Cc: linux-harden...@vger.kernel.org Signed-off-by: Justin Stitt --- Note: build-tested only. --- drivers/edac/edac_mc_sysfs.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c index 15f63452a9be..b303309a63cf 100644 --- a/drivers/edac/edac_mc_sysfs.c +++ b/drivers/edac/edac_mc_sysfs.c @@ -229,8 +229,7 @@ static ssize_t channel_dimm_label_store(struct device *dev, if (copy_count == 0 || copy_count >= sizeof(rank->dimm->label)) return -EINVAL; - strncpy(rank->dimm->label, data, copy_count); - rank->dimm->label[copy_count] = '\0'; + strscpy_pad(rank->dimm->label, data, copy_count); return count; } --- base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c change-id: 20230913-strncpy-drivers-edac-edac_mc_sysfs-c-e619b00124a3 Best regards, -- Justin Stitt
[PATCH] dax: refactor deprecated strncpy
`strncpy` is deprecated for use on NUL-terminated destination strings [1].
We should prefer more robust and less ambiguous string interfaces.
`dax_id->dev_name` is expected to be NUL-terminated and has been zero-allocated.
A suitable replacement is `strscpy` [2] due to the fact that it
guarantees NUL-termination on the destination buffer. Moreover, due to
`dax_id` being zero-allocated the padding behavior of `strncpy` is not
needed and a simple 1:1 replacement of strncpy -> strscpy should
suffice.
Link:
https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
[1]
Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2]
Link: https://github.com/KSPP/linux/issues/90
Cc: linux-harden...@vger.kernel.org
Signed-off-by: Justin Stitt
---
Note: build-tested only.
---
drivers/dax/bus.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/dax/bus.c b/drivers/dax/bus.c
index 0ee96e6fc426..1659b787b65f 100644
--- a/drivers/dax/bus.c
+++ b/drivers/dax/bus.c
@@ -103,7 +103,7 @@ static ssize_t do_id_store(struct device_driver *drv, const
char *buf,
if (action == ID_ADD) {
dax_id = kzalloc(sizeof(*dax_id), GFP_KERNEL);
if (dax_id) {
- strncpy(dax_id->dev_name, buf, DAX_NAME_LEN);
+ strscpy(dax_id->dev_name, buf, DAX_NAME_LEN);
list_add(_id->list, _drv->ids);
} else
rc = -ENOMEM;
---
base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
change-id: 20230913-strncpy-drivers-dax-bus-c-f12e3153e44b
Best regards,
--
Justin Stitt
Re: [PATCH] ipmi: refactor deprecated strncpy
On Tue, Sep 12, 2023 at 5:55 PM Justin Stitt wrote:
>
> On Tue, Sep 12, 2023 at 5:19 PM Corey Minyard wrote:
> >
> > On Tue, Sep 12, 2023 at 11:43:05PM +, Justin Stitt wrote:
> > > `strncpy` is deprecated for use on NUL-terminated destination strings [1].
> > >
> > > In this case, strncpy is being used specifically for its NUL-padding
> > > behavior (and has been commented as such). We can use a more robust and
> > > less ambiguous interface in `strscpy_pad` which makes the code more
> > > readable and even eliminates the need for that comment.
> > >
> > > Let's also use `strnlen` instead of `strlen()` with an upper-bounds
> > > check as this is intrinsically a part of `strnlen`.
> > >
> > > Also included in this patch is a simple 1:1 change of `strncpy` to
> > > `strscpy` for ipmi_ssif.c. If NUL-padding is wanted here as well then we
> > > should opt again for `strscpy_pad`.
> > >
> > > Link:
> > > https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
> > > [1]
> > > Link: https://github.com/KSPP/linux/issues/90
> > > Cc: linux-harden...@vger.kernel.org
> > > Cc: Kees Cook
> > > Signed-off-by: Justin Stitt
> > > ---
> > > drivers/char/ipmi/ipmi_msghandler.c | 11 +++
> > > drivers/char/ipmi/ipmi_ssif.c | 2 +-
> > > 2 files changed, 4 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/char/ipmi/ipmi_msghandler.c
> > > b/drivers/char/ipmi/ipmi_msghandler.c
> > > index 186f1fee7534..04f7622cb703 100644
> > > --- a/drivers/char/ipmi/ipmi_msghandler.c
> > > +++ b/drivers/char/ipmi/ipmi_msghandler.c
> > > @@ -5377,20 +5377,15 @@ static void send_panic_events(struct ipmi_smi
> > > *intf, char *str)
> > >
> > > j = 0;
> > > while (*p) {
> > > - int size = strlen(p);
> > > + int size = strnlen(p, 11);
> > >
> > > - if (size > 11)
> > > - size = 11;
> > > data[0] = 0;
> > > data[1] = 0;
> > > data[2] = 0xf0; /* OEM event without timestamp. */
> > > data[3] = intf->addrinfo[0].address;
> > > data[4] = j++; /* sequence # */
> > > - /*
> > > - * Always give 11 bytes, so strncpy will fill
> > > - * it with zeroes for me.
> > > - */
> > > - strncpy(data+5, p, 11);
> > > +
> > > + strscpy_pad(data+5, p, 11);
> >
> > This is incorrect, the destination should *not* be nil terminated if the
> > destination is full. strncpy does exactly what is needed here.
>
> Could we use `memcpy_and_pad()` as this matches the behavior of
> strncpy in this case? I understand strncpy works here but I'm really
> keen on snuffing out all its uses -- treewide.
^ I mean something like the following:
|memcpy_and_pad(data+5, 11, p, size, '\0');
as this is explicit in its behavior.
>
> >
> > A comment should be added here, this is not the first time this has been
> > brought up.
> >
> > > p += size;
> > >
> > > ipmi_panic_request_and_wait(intf, , );
> > > diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
> > > index 3b921c78ba08..edcb83765dce 100644
> > > --- a/drivers/char/ipmi/ipmi_ssif.c
> > > +++ b/drivers/char/ipmi/ipmi_ssif.c
> > > @@ -1940,7 +1940,7 @@ static int new_ssif_client(int addr, char
> > > *adapter_name,
> > > }
> > > }
> > >
> > > - strncpy(addr_info->binfo.type, DEVICE_NAME,
> > > + strscpy(addr_info->binfo.type, DEVICE_NAME,
> > > sizeof(addr_info->binfo.type));
> >
> > This one is good.
> >
> > -corey
> >
> > > addr_info->binfo.addr = addr;
> > > addr_info->binfo.platform_data = addr_info;
> > >
> > > ---
> > > base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
> > > change-id: 20230912-strncpy-drivers-char-ipmi-ipmi-dda47b3773fd
> > >
> > > Best regards,
> > > --
> > > Justin Stitt
> > >
Re: [PATCH] ipmi: refactor deprecated strncpy
On Tue, Sep 12, 2023 at 5:19 PM Corey Minyard wrote:
>
> On Tue, Sep 12, 2023 at 11:43:05PM +, Justin Stitt wrote:
> > `strncpy` is deprecated for use on NUL-terminated destination strings [1].
> >
> > In this case, strncpy is being used specifically for its NUL-padding
> > behavior (and has been commented as such). We can use a more robust and
> > less ambiguous interface in `strscpy_pad` which makes the code more
> > readable and even eliminates the need for that comment.
> >
> > Let's also use `strnlen` instead of `strlen()` with an upper-bounds
> > check as this is intrinsically a part of `strnlen`.
> >
> > Also included in this patch is a simple 1:1 change of `strncpy` to
> > `strscpy` for ipmi_ssif.c. If NUL-padding is wanted here as well then we
> > should opt again for `strscpy_pad`.
> >
> > Link:
> > https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
> > [1]
> > Link: https://github.com/KSPP/linux/issues/90
> > Cc: linux-harden...@vger.kernel.org
> > Cc: Kees Cook
> > Signed-off-by: Justin Stitt
> > ---
> > drivers/char/ipmi/ipmi_msghandler.c | 11 +++
> > drivers/char/ipmi/ipmi_ssif.c | 2 +-
> > 2 files changed, 4 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/char/ipmi/ipmi_msghandler.c
> > b/drivers/char/ipmi/ipmi_msghandler.c
> > index 186f1fee7534..04f7622cb703 100644
> > --- a/drivers/char/ipmi/ipmi_msghandler.c
> > +++ b/drivers/char/ipmi/ipmi_msghandler.c
> > @@ -5377,20 +5377,15 @@ static void send_panic_events(struct ipmi_smi
> > *intf, char *str)
> >
> > j = 0;
> > while (*p) {
> > - int size = strlen(p);
> > + int size = strnlen(p, 11);
> >
> > - if (size > 11)
> > - size = 11;
> > data[0] = 0;
> > data[1] = 0;
> > data[2] = 0xf0; /* OEM event without timestamp. */
> > data[3] = intf->addrinfo[0].address;
> > data[4] = j++; /* sequence # */
> > - /*
> > - * Always give 11 bytes, so strncpy will fill
> > - * it with zeroes for me.
> > - */
> > - strncpy(data+5, p, 11);
> > +
> > + strscpy_pad(data+5, p, 11);
>
> This is incorrect, the destination should *not* be nil terminated if the
> destination is full. strncpy does exactly what is needed here.
Could we use `memcpy_and_pad()` as this matches the behavior of
strncpy in this case? I understand strncpy works here but I'm really
keen on snuffing out all its uses -- treewide.
>
> A comment should be added here, this is not the first time this has been
> brought up.
>
> > p += size;
> >
> > ipmi_panic_request_and_wait(intf, , );
> > diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
> > index 3b921c78ba08..edcb83765dce 100644
> > --- a/drivers/char/ipmi/ipmi_ssif.c
> > +++ b/drivers/char/ipmi/ipmi_ssif.c
> > @@ -1940,7 +1940,7 @@ static int new_ssif_client(int addr, char
> > *adapter_name,
> > }
> > }
> >
> > - strncpy(addr_info->binfo.type, DEVICE_NAME,
> > + strscpy(addr_info->binfo.type, DEVICE_NAME,
> > sizeof(addr_info->binfo.type));
>
> This one is good.
>
> -corey
>
> > addr_info->binfo.addr = addr;
> > addr_info->binfo.platform_data = addr_info;
> >
> > ---
> > base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
> > change-id: 20230912-strncpy-drivers-char-ipmi-ipmi-dda47b3773fd
> >
> > Best regards,
> > --
> > Justin Stitt
> >
[PATCH] cpuidle: dt: refactor deprecated strncpy
`strncpy` is deprecated for use on NUL-terminated destination strings [1]. We should prefer more robust and less ambiguous string interfaces. A suitable replacement is `strscpy` [2] due to the fact that it guarantees NUL-termination on the destination buffer. With this, we can also drop the now unnecessary `CPUIDLE_(NAME|DESC)_LEN - 1` pieces. Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2] Link: https://github.com/KSPP/linux/issues/90 Cc: linux-harden...@vger.kernel.org Signed-off-by: Justin Stitt --- Note: build-tested only --- drivers/cpuidle/dt_idle_states.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/cpuidle/dt_idle_states.c b/drivers/cpuidle/dt_idle_states.c index 12fec92a85fd..97feb7d8fb23 100644 --- a/drivers/cpuidle/dt_idle_states.c +++ b/drivers/cpuidle/dt_idle_states.c @@ -84,8 +84,8 @@ static int init_state_node(struct cpuidle_state *idle_state, * replace with kstrdup and pointer assignment when name * and desc become string pointers */ - strncpy(idle_state->name, state_node->name, CPUIDLE_NAME_LEN - 1); - strncpy(idle_state->desc, desc, CPUIDLE_DESC_LEN - 1); + strscpy(idle_state->name, state_node->name, CPUIDLE_NAME_LEN); + strscpy(idle_state->desc, desc, CPUIDLE_DESC_LEN); return 0; } --- base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c change-id: 20230913-strncpy-drivers-cpuidle-dt_idle_states-c-c84ea03c1379 Best regards, -- Justin Stitt
[PATCH] cpufreq: refactor deprecated strncpy
`strncpy` is deprecated for use on NUL-terminated destination strings [1]. We should prefer more robust and less ambiguous string interfaces. Both `policy->last_governor` and `default_governor` are expected to be NUL-terminated which is shown by their heavy usage with other string apis like `strcmp`. A suitable replacement is `strscpy` [2] due to the fact that it guarantees NUL-termination on the destination buffer. Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2] Link: https://github.com/KSPP/linux/issues/90 Cc: linux-harden...@vger.kernel.org Signed-off-by: Justin Stitt --- Note: build-tested --- drivers/cpufreq/cpufreq.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 50bbc969ffe5..3eb851a03fce 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1607,7 +1607,7 @@ static void __cpufreq_offline(unsigned int cpu, struct cpufreq_policy *policy) } if (has_target()) - strncpy(policy->last_governor, policy->governor->name, + strscpy(policy->last_governor, policy->governor->name, CPUFREQ_NAME_LEN); else policy->last_policy = policy->policy; @@ -2951,7 +2951,7 @@ static int __init cpufreq_core_init(void) BUG_ON(!cpufreq_global_kobject); if (!strlen(default_governor)) - strncpy(default_governor, gov->name, CPUFREQ_NAME_LEN); + strscpy(default_governor, gov->name, CPUFREQ_NAME_LEN); return 0; } --- base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c change-id: 20230912-strncpy-drivers-cpufreq-cpufreq-c-1d800044b007 Best regards, -- Justin Stitt
[PATCH] ipmi: refactor deprecated strncpy
`strncpy` is deprecated for use on NUL-terminated destination strings [1].
In this case, strncpy is being used specifically for its NUL-padding
behavior (and has been commented as such). We can use a more robust and
less ambiguous interface in `strscpy_pad` which makes the code more
readable and even eliminates the need for that comment.
Let's also use `strnlen` instead of `strlen()` with an upper-bounds
check as this is intrinsically a part of `strnlen`.
Also included in this patch is a simple 1:1 change of `strncpy` to
`strscpy` for ipmi_ssif.c. If NUL-padding is wanted here as well then we
should opt again for `strscpy_pad`.
Link:
https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
[1]
Link: https://github.com/KSPP/linux/issues/90
Cc: linux-harden...@vger.kernel.org
Cc: Kees Cook
Signed-off-by: Justin Stitt
---
drivers/char/ipmi/ipmi_msghandler.c | 11 +++
drivers/char/ipmi/ipmi_ssif.c | 2 +-
2 files changed, 4 insertions(+), 9 deletions(-)
diff --git a/drivers/char/ipmi/ipmi_msghandler.c
b/drivers/char/ipmi/ipmi_msghandler.c
index 186f1fee7534..04f7622cb703 100644
--- a/drivers/char/ipmi/ipmi_msghandler.c
+++ b/drivers/char/ipmi/ipmi_msghandler.c
@@ -5377,20 +5377,15 @@ static void send_panic_events(struct ipmi_smi *intf,
char *str)
j = 0;
while (*p) {
- int size = strlen(p);
+ int size = strnlen(p, 11);
- if (size > 11)
- size = 11;
data[0] = 0;
data[1] = 0;
data[2] = 0xf0; /* OEM event without timestamp. */
data[3] = intf->addrinfo[0].address;
data[4] = j++; /* sequence # */
- /*
-* Always give 11 bytes, so strncpy will fill
-* it with zeroes for me.
-*/
- strncpy(data+5, p, 11);
+
+ strscpy_pad(data+5, p, 11);
p += size;
ipmi_panic_request_and_wait(intf, , );
diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
index 3b921c78ba08..edcb83765dce 100644
--- a/drivers/char/ipmi/ipmi_ssif.c
+++ b/drivers/char/ipmi/ipmi_ssif.c
@@ -1940,7 +1940,7 @@ static int new_ssif_client(int addr, char *adapter_name,
}
}
- strncpy(addr_info->binfo.type, DEVICE_NAME,
+ strscpy(addr_info->binfo.type, DEVICE_NAME,
sizeof(addr_info->binfo.type));
addr_info->binfo.addr = addr;
addr_info->binfo.platform_data = addr_info;
---
base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
change-id: 20230912-strncpy-drivers-char-ipmi-ipmi-dda47b3773fd
Best regards,
--
Justin Stitt
[PATCH] bus: fsl-mc: refactor deprecated strncpy
`strncpy` is deprecated for use on NUL-terminated destination strings [1]. We need to prefer more robust and less ambiguous string interfaces. `obj_desc->(type|label)` are expected to be NUL-terminated strings as per "include/linux/fsl/mc.h +143" | ... | * struct fsl_mc_obj_desc - Object descriptor | * @type: Type of object: NULL terminated string | ... It seems `cmd_params->obj_type` is also expected to be a NUL-terminated string. A suitable replacement is `strscpy_pad` due to the fact that it guarantees NUL-termination on the destination buffer whilst keeping the NUL-padding behavior that `strncpy` provides. Padding may not strictly be necessary but let's opt to keep it as this ensures no functional change. Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] Link: https://github.com/KSPP/linux/issues/90 Cc: linux-harden...@vger.kernel.org Cc: Kees Cook Signed-off-by: Justin Stitt --- Note: build-tested --- drivers/bus/fsl-mc/dprc.c | 12 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/drivers/bus/fsl-mc/dprc.c b/drivers/bus/fsl-mc/dprc.c index d129338b8bc0..dd1b5c0fb7e2 100644 --- a/drivers/bus/fsl-mc/dprc.c +++ b/drivers/bus/fsl-mc/dprc.c @@ -450,10 +450,8 @@ int dprc_get_obj(struct fsl_mc_io *mc_io, obj_desc->ver_major = le16_to_cpu(rsp_params->version_major); obj_desc->ver_minor = le16_to_cpu(rsp_params->version_minor); obj_desc->flags = le16_to_cpu(rsp_params->flags); - strncpy(obj_desc->type, rsp_params->type, 16); - obj_desc->type[15] = '\0'; - strncpy(obj_desc->label, rsp_params->label, 16); - obj_desc->label[15] = '\0'; + strscpy_pad(obj_desc->type, rsp_params->type, 16); + strscpy_pad(obj_desc->label, rsp_params->label, 16); return 0; } EXPORT_SYMBOL_GPL(dprc_get_obj); @@ -491,8 +489,7 @@ int dprc_set_obj_irq(struct fsl_mc_io *mc_io, cmd_params->irq_addr = cpu_to_le64(irq_cfg->paddr); cmd_params->irq_num = cpu_to_le32(irq_cfg->irq_num); cmd_params->obj_id = cpu_to_le32(obj_id); - strncpy(cmd_params->obj_type, obj_type, 16); - cmd_params->obj_type[15] = '\0'; + strscpy_pad(cmd_params->obj_type, obj_type, 16); /* send command to mc*/ return mc_send_command(mc_io, ); @@ -564,8 +561,7 @@ int dprc_get_obj_region(struct fsl_mc_io *mc_io, cmd_params = (struct dprc_cmd_get_obj_region *)cmd.params; cmd_params->obj_id = cpu_to_le32(obj_id); cmd_params->region_index = region_index; - strncpy(cmd_params->obj_type, obj_type, 16); - cmd_params->obj_type[15] = '\0'; + strscpy_pad(cmd_params->obj_type, obj_type, 16); /* send command to mc*/ err = mc_send_command(mc_io, ); --- base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c change-id: 20230912-strncpy-drivers-bus-fsl-mc-dprc-c-bc7d818386ec Best regards, -- Justin Stitt
Re: [PATCH] um,ethertap: refactor deprecated strncpy
On Tue, Sep 12, 2023 at 12:36 AM Geert Uytterhoeven
wrote:
>
> Hi Justin,
>
> Thanks for your patch!
>
> On Mon, Sep 11, 2023 at 7:53 PM Justin Stitt wrote:
> > `strncpy` is deprecated for use on NUL-terminated destination strings [1].
> >
> > `gate_buf` should always be NUL-terminated and does not require
> > NUL-padding. It is used as a string arg inside an argv array given to
>
> Can you please explain why it does not require NUL-padding?
> It looks like this buffer is passed eventually to a user space
> application, thus possibly leaking uninitialized stack data.
It looks like it's being passed as a list of command-line arguments in
`run_helper()`. Should this be NUL-padded due to its eventual use in
user space? If we think yes I can send a v2. Thanks for pointing this
out.
>
> > `run_helper()`. Due to this, let's use `strscpy` as it guarantees
> > NUL-terminated on the destination buffer preventing potential buffer
> > overreads [2].
> >
> > This exact invocation was changed from `strcpy` to `strncpy` in commit
> > 7879b1d94badb ("um,ethertap: use strncpy") back in 2015. Let's continue
> > hardening our `str*cpy` apis and use the newer and safer `strscpy`!
> >
> > Link:
> > www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings[1]
> > Link:
> > https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2]
> > Link: https://github.com/KSPP/linux/issues/90
> > Cc: linux-harden...@vger.kernel.org
> > Cc: Kees Cook
> > Signed-off-by: Justin Stitt
> > ---
> > arch/um/os-Linux/drivers/ethertap_user.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/um/os-Linux/drivers/ethertap_user.c
> > b/arch/um/os-Linux/drivers/ethertap_user.c
> > index 9483021d86dd..3363851a4ae8 100644
> > --- a/arch/um/os-Linux/drivers/ethertap_user.c
> > +++ b/arch/um/os-Linux/drivers/ethertap_user.c
> > @@ -105,7 +105,7 @@ static int etap_tramp(char *dev, char *gate, int
> > control_me,
> > sprintf(data_fd_buf, "%d", data_remote);
> > sprintf(version_buf, "%d", UML_NET_VERSION);
> > if (gate != NULL) {
> > - strncpy(gate_buf, gate, 15);
> > + strscpy(gate_buf, gate, sizeof(gate_buf));
> > args = setup_args;
> > }
> > else args = nosetup_args;
> >
> > ---
> > base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
> > change-id:
> > 20230911-strncpy-arch-um-os-linux-drivers-ethertap_user-c-859160d13f59
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> ge...@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like
> that.
> -- Linus Torvalds
Re: [PATCH] tpm: Fix typo in tpmrm class definition
On Tue, Sep 12, 2023 at 4:41 AM Jarkko Sakkinen wrote:
>
> On Tue Sep 12, 2023 at 1:32 AM EEST, Justin M. Forbes wrote:
> > Commit d2e8071bed0be ("tpm: make all 'class' structures const")
> > unfortunately had a typo for the name on tpmrm.
> >
> > Fixes: d2e8071bed0b ("tpm: make all 'class' structures const")
> > Signed-off-by: Justin M. Forbes
> > ---
> > drivers/char/tpm/tpm-chip.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> > index 23f6f2eda84c..42b1062e33cd 100644
> > --- a/drivers/char/tpm/tpm-chip.c
> > +++ b/drivers/char/tpm/tpm-chip.c
> > @@ -33,7 +33,7 @@ const struct class tpm_class = {
> > .shutdown_pre = tpm_class_shutdown,
> > };
> > const struct class tpmrm_class = {
> > - .name = "tmprm",
> > + .name = "tpmrm",
> > };
> > dev_t tpm_devt;
> >
> > --
> > 2.41.0
>
> Unfortunately your patch does not apply:
Fixed with the V2 I just sent out. Seems I had suppress-blank-empty =
true in a config file somewhere. Apologies for wasting your time.
Justin
> $ git-tip
> 0bb80ecc33a8 (HEAD -> next, tag: v6.6-rc1, upstream/master, origin/next)
> Linux 6.6-rc1
>
> $ b4 am 20230911223238.3495955-1-jfor...@fedoraproject.org
> Analyzing 1 messages in the thread
> Checking attestation on all messages, may take a moment...
> ---
> ✓ [PATCH] tpm: Fix typo in tpmrm class definition
> ---
> ✓ Signed: DKIM/linuxtx.org (From: jfor...@fedoraproject.org)
> ---
> Total patches: 1
> ---
> Link:
> https://lore.kernel.org/r/20230911223238.3495955-1-jfor...@fedoraproject.org
> Base: applies clean to current tree
>git checkout -b 20230911_jforbes_fedoraproject_org HEAD
>git am ./20230911_jforbes_tpm_fix_typo_in_tpmrm_class_definition.mbx
>
> $ git am -3 20230911_jforbes_tpm_fix_typo_in_tpmrm_class_definition.mbx
> Applying: tpm: Fix typo in tpmrm class definition
> error: corrupt patch at line 18
> error: could not build fake ancestor
> Patch failed at 0001 tpm: Fix typo in tpmrm class definition
> hint: Use 'git am --show-current-patch=diff' to see the failed patch
> When you have resolved this problem, run "git am --continue".
> If you prefer to skip this patch, run "git am --skip" instead.
> To restore the original branch and stop patching, run "git am --abort".
>
> BR, Jarkko
[PATCH v2] tpm: Fix typo in tpmrm class definition
Commit d2e8071bed0be ("tpm: make all 'class' structures const")
unfortunately had a typo for the name on tpmrm.
Fixes: d2e8071bed0b ("tpm: make all 'class' structures const")
Signed-off-by: Justin M. Forbes
---
drivers/char/tpm/tpm-chip.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 23f6f2eda84c..42b1062e33cd 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -33,7 +33,7 @@ const struct class tpm_class = {
.shutdown_pre = tpm_class_shutdown,
};
const struct class tpmrm_class = {
- .name = "tmprm",
+ .name = "tpmrm",
};
dev_t tpm_devt;
--
2.41.0
Re: [PATCH] x86/tdx: refactor deprecated strncpy
On Mon, Sep 11, 2023 at 11:51 AM Dave Hansen wrote: > > On 9/11/23 11:27, Justin Stitt wrote: > > `strncpy` is deprecated and we should prefer more robust string apis. > > I dunno. It actually seems like a pretty good fit here. > > > In this case, `message.str` is not expected to be NUL-terminated as it > > is simply a buffer of characters residing in a union which allows for > > named fields representing 8 bytes each. There is only one caller of > > `tdx_panic()` and they use a 59-length string for `msg`: > > | const char *msg = "TD misconfiguration: SEPT_VE_DISABLE attribute > > must be set."; > > I'm not really following this logic. > > We need to do the following: > > 1. Make sure not to over write past the end of 'message' > 2. Make sure not to over read past the end of 'msg' > 3. Make sure not to leak stack data into the hypercall registers >in the case of short strings. > > strncpy() does #1, #2 and #3 just fine. Right, to be clear, I do not suspect a bug in the current implementation. Rather, let's move towards a less ambiguous interface for maintainability's sake > > The resulting string does *NOT* need to be NULL-terminated. See the > comment: > > /* VMM assumes '\0' in byte 65, if the message took all 64 bytes */ > > Are there cases where strncpy() doesn't NULL-terminate the string other > than when the buffer is full? > > I actually didn't realize that strncpy() pads its output up to the full > size. I wonder if Kirill used it intentionally or whether he got lucky > here. :) Big reason to use strtomem_pad as it is more obvious about what it does. I'd love more thoughts/testing here.
[PATCH] auxdisplay: panel: refactor deprecated strncpy
`strncpy` is deprecated and as such we should prefer more robust and
less ambiguous interfaces.
In this case, all of `press_str`, `repeat_str` and `release_str` are
explicitly marked as nonstring:
| struct {/* valid when type == INPUT_TYPE_KBD */
| char press_str[sizeof(void *) + sizeof(int)] __nonstring;
| char repeat_str[sizeof(void *) + sizeof(int)] __nonstring;
| char release_str[sizeof(void *) + sizeof(int)] __nonstring;
| } kbd;
... which makes `strtomem_pad` a suitable replacement as it is
functionally the same whilst being more obvious about its behavior.
Link:
https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
[1]
Link: https://github.com/KSPP/linux/issues/90
Cc: linux-harden...@vger.kernel.org
Cc: Kees Cook
Signed-off-by: Justin Stitt
---
Note: build-tested
---
drivers/auxdisplay/panel.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/auxdisplay/panel.c b/drivers/auxdisplay/panel.c
index eba04c0de7eb..e20d35bdf5fe 100644
--- a/drivers/auxdisplay/panel.c
+++ b/drivers/auxdisplay/panel.c
@@ -1449,10 +1449,9 @@ static struct logical_input *panel_bind_key(const char
*name, const char *press,
key->rise_time = 1;
key->fall_time = 1;
- strncpy(key->u.kbd.press_str, press, sizeof(key->u.kbd.press_str));
- strncpy(key->u.kbd.repeat_str, repeat, sizeof(key->u.kbd.repeat_str));
- strncpy(key->u.kbd.release_str, release,
- sizeof(key->u.kbd.release_str));
+ strtomem_pad(key->u.kbd.press_str, press, '\0');
+ strtomem_pad(key->u.kbd.repeat_str, repeat, '\0');
+ strtomem_pad(key->u.kbd.release_str, release, '\0');
list_add(>list, _inputs);
return key;
}
---
base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
change-id: 20230911-strncpy-drivers-auxdisplay-panel-c-83bce51f32cb
Best regards,
--
Justin Stitt
[PATCH] tpm: Fix typo in tpmrm class definition
Commit d2e8071bed0be ("tpm: make all 'class' structures const")
unfortunately had a typo for the name on tpmrm.
Fixes: d2e8071bed0b ("tpm: make all 'class' structures const")
Signed-off-by: Justin M. Forbes
---
drivers/char/tpm/tpm-chip.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 23f6f2eda84c..42b1062e33cd 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -33,7 +33,7 @@ const struct class tpm_class = {
.shutdown_pre = tpm_class_shutdown,
};
const struct class tpmrm_class = {
- .name = "tmprm",
+ .name = "tpmrm",
};
dev_t tpm_devt;
--
2.41.0
Re: [PATCH] Fix typo in tpmrm class definition
On Mon, Sep 11, 2023 at 5:09 PM Jarkko Sakkinen wrote:
>
> On Fri Sep 8, 2023 at 5:06 PM EEST, Justin M. Forbes wrote:
> > Commit d2e8071bed0be ("tpm: make all 'class' structures const")
> > unfortunately had a typo for the name on tpmrm.
> >
> > Fixes: d2e8071bed0b ("tpm: make all 'class' structures const")
> > Signed-off-by: Justin M. Forbes
> > ---
> > drivers/char/tpm/tpm-chip.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> > index 23f6f2eda84c..42b1062e33cd 100644
> > --- a/drivers/char/tpm/tpm-chip.c
> > +++ b/drivers/char/tpm/tpm-chip.c
> > @@ -33,7 +33,7 @@ const struct class tpm_class = {
> > .shutdown_pre = tpm_class_shutdown,
> > };
> > const struct class tpmrm_class = {
> > - .name = "tmprm",
> > + .name = "tpmrm",
> > };
> > dev_t tpm_devt;
> >
> > --
> > 2.41.0
>
> I have issues applying the patch:
Sorry, not sure what the issue is, but I did a git am of it myself to
a fresh checkout of linus' tree and just recreated and sent it. So,
new thread, but hopefully the patch will apply
Justin
>
> $ git am -3 20230908_jforbes_fix_typo_in_tpmrm_class_definition.mbx
> Applying: Fix typo in tpmrm class definition
> error: corrupt patch at line 18
> error: could not build fake ancestor
> Patch failed at 0001 Fix typo in tpmrm class definition
> hint: Use 'git am --show-current-patch=diff' to see the failed patch
> When you have resolved this problem, run "git am --continue".
> If you prefer to skip this patch, run "git am --skip" instead.
> To restore the original branch and stop patching, run "git am --abort".
>
> $ git log -2
> commit ba46245183940de39e42c8456b85ceaf3519b764 (HEAD -> master,
> origin/master, origin/HEAD)
> Author: Sumit Garg
> Date: Tue Aug 22 16:59:33 2023 +0530
>
> KEYS: trusted: tee: Refactor register SHM usage
>
> The OP-TEE driver using the old SMC based ABI permits overlapping shared
> buffers, but with the new FF-A based ABI each physical page may only
> be registered once.
>
> As the key and blob buffer are allocated adjancently, there is no need
> for redundant register shared memory invocation. Also, it is incompatibile
> with FF-A based ABI limitation. So refactor register shared memory
> implementation to use only single invocation to register both key and blob
> buffers.
>
> [jarkko: Added cc to stable.]
> Cc: sta...@vger.kernel.org # v5.16+
> Fixes: 4615e5a34b95 ("optee: add FF-A support")
> Reported-by: Jens Wiklander
> Signed-off-by: Sumit Garg
> Tested-by: Jens Wiklander
> Reviewed-by: Jens Wiklander
> Signed-off-by: Jarkko Sakkinen
>
> commit 0bb80ecc33a8fb5a682236443c1e740d5c917d1d (tag: v6.6-rc1,
> upstream/master, origin/next, next)
> Author: Linus Torvalds
> Date: Sun Sep 10 16:28:41 2023 -0700
>
> Linux 6.6-rc1
>
> BR, Jarkko
>
[PATCH] ACPI: OSI: refactor deprecated strncpy
`strncpy` is deprecated for use on NUL-terminated destination strings [1].
We know `osi->string` is a NUL-terminated string due to its eventual use
in `acpi_install_interface()` and `acpi_remove_interface()` which expect
a `acpi_string` which has been specifically typedef'd as:
| typedef char *acpi_string; /* Null terminated ASCII string */
... and which also has other string functions used on it like `strlen`.
Furthermore, padding is not needed in this instance either.
Due to the reasoning above a suitable replacement is `strscpy` [2] since
it guarantees NUL-termination on the destination buffer and doesn't
unnecessarily NUL-pad.
While there is unlikely to be a buffer overread (or other related bug)
in this case, we should still favor a more robust and less ambiguous
interface.
Link:
https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
[1]
Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2]
Link: https://github.com/KSPP/linux/issues/90
Cc: linux-harden...@vger.kernel.org
Cc: Kees Cook
Signed-off-by: Justin Stitt
---
Note: build-tested
---
drivers/acpi/osi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/acpi/osi.c b/drivers/acpi/osi.c
index d4405e1ca9b9..df9328c850bd 100644
--- a/drivers/acpi/osi.c
+++ b/drivers/acpi/osi.c
@@ -110,7 +110,7 @@ void __init acpi_osi_setup(char *str)
break;
} else if (osi->string[0] == '\0') {
osi->enable = enable;
- strncpy(osi->string, str, OSI_STRING_LENGTH_MAX);
+ strscpy(osi->string, str, OSI_STRING_LENGTH_MAX);
break;
}
}
---
base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
change-id: 20230911-strncpy-drivers-acpi-osi-c-c801b7427987
Best regards,
--
Justin Stitt
[PATCH] x86/tdx: refactor deprecated strncpy
`strncpy` is deprecated and we should prefer more robust string apis. In this case, `message.str` is not expected to be NUL-terminated as it is simply a buffer of characters residing in a union which allows for named fields representing 8 bytes each. There is only one caller of `tdx_panic()` and they use a 59-length string for `msg`: | const char *msg = "TD misconfiguration: SEPT_VE_DISABLE attribute must be set."; Given all this information, let's use `strtomem_pad` as this matches the functionality of `strncpy` in this instances whilst being a more robust and easier to understand interface. Link: www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings[1] Link: https://github.com/KSPP/linux/issues/90 Cc: linux-harden...@vger.kernel.org Cc: Kees Cook Cc: Nick Desaulniers Signed-off-by: Justin Stitt --- Note: build-tested --- arch/x86/coco/tdx/tdx.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c index 1d6b863c42b0..2e1be592c220 100644 --- a/arch/x86/coco/tdx/tdx.c +++ b/arch/x86/coco/tdx/tdx.c @@ -119,7 +119,7 @@ static void __noreturn tdx_panic(const char *msg) } message; /* VMM assumes '\0' in byte 65, if the message took all 64 bytes */ - strncpy(message.str, msg, 64); + strtomem_pad(message.str, msg, '\0'); args.r8 = message.r8; args.r9 = message.r9; --- base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c change-id: 20230911-strncpy-arch-x86-coco-tdx-tdx-c-98b0b966bb8d Best regards, -- Justin Stitt
[PATCH] xen/efi: refactor deprecated strncpy
`strncpy` is deprecated for use on NUL-terminated destination strings [1]. `efi_loader_signature` has space for 4 bytes. We are copying "Xen" (3 bytes) plus a NUL-byte which makes 4 total bytes. With that being said, there is currently not a bug with the current `strncpy()` implementation in terms of buffer overreads but we should favor a more robust string interface either way. A suitable replacement is `strscpy` [2] due to the fact that it guarantees NUL-termination on the destination buffer while being functionally the same in this case. Link: www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings[1] Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2] Link: https://github.com/KSPP/linux/issues/90 Cc: linux-harden...@vger.kernel.org Cc: Kees Cook Signed-off-by: Justin Stitt --- Note: build-tested --- arch/x86/xen/efi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/x86/xen/efi.c b/arch/x86/xen/efi.c index 863d0d6b3edc..7250d0e0e1a9 100644 --- a/arch/x86/xen/efi.c +++ b/arch/x86/xen/efi.c @@ -138,7 +138,7 @@ void __init xen_efi_init(struct boot_params *boot_params) if (efi_systab_xen == NULL) return; - strncpy((char *)_params->efi_info.efi_loader_signature, "Xen", + strscpy((char *)_params->efi_info.efi_loader_signature, "Xen", sizeof(boot_params->efi_info.efi_loader_signature)); boot_params->efi_info.efi_systab = (__u32)__pa(efi_systab_xen); boot_params->efi_info.efi_systab_hi = (__u32)(__pa(efi_systab_xen) >> 32); --- base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c change-id: 20230911-strncpy-arch-x86-xen-efi-c-14292f5a79ee Best regards, -- Justin Stitt
[PATCH] um,ethertap: refactor deprecated strncpy
`strncpy` is deprecated for use on NUL-terminated destination strings [1].
`gate_buf` should always be NUL-terminated and does not require
NUL-padding. It is used as a string arg inside an argv array given to
`run_helper()`. Due to this, let's use `strscpy` as it guarantees
NUL-terminated on the destination buffer preventing potential buffer
overreads [2].
This exact invocation was changed from `strcpy` to `strncpy` in commit
7879b1d94badb ("um,ethertap: use strncpy") back in 2015. Let's continue
hardening our `str*cpy` apis and use the newer and safer `strscpy`!
Link:
www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings[1]
Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2]
Link: https://github.com/KSPP/linux/issues/90
Cc: linux-harden...@vger.kernel.org
Cc: Kees Cook
Signed-off-by: Justin Stitt
---
arch/um/os-Linux/drivers/ethertap_user.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/um/os-Linux/drivers/ethertap_user.c
b/arch/um/os-Linux/drivers/ethertap_user.c
index 9483021d86dd..3363851a4ae8 100644
--- a/arch/um/os-Linux/drivers/ethertap_user.c
+++ b/arch/um/os-Linux/drivers/ethertap_user.c
@@ -105,7 +105,7 @@ static int etap_tramp(char *dev, char *gate, int control_me,
sprintf(data_fd_buf, "%d", data_remote);
sprintf(version_buf, "%d", UML_NET_VERSION);
if (gate != NULL) {
- strncpy(gate_buf, gate, 15);
+ strscpy(gate_buf, gate, sizeof(gate_buf));
args = setup_args;
}
else args = nosetup_args;
---
base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
change-id:
20230911-strncpy-arch-um-os-linux-drivers-ethertap_user-c-859160d13f59
Best regards,
--
Justin Stitt
RE: [PATCH v2] device-dax: use fallback nid when numa node is invalid
> -Original Message-
> From: Dan Williams
> Sent: Wednesday, September 15, 2021 1:16 PM
> To: Justin He
> Cc: Vishal Verma ; Dave Jiang
> ; David Hildenbrand ; Linux NVDIMM
> ; Linux Kernel Mailing List ker...@vger.kernel.org>; nd
> Subject: Re: [PATCH v2] device-dax: use fallback nid when numa node is
> invalid
>
> On Mon, Sep 13, 2021 at 7:06 PM Justin He wrote:
> >
> > Hi Dan,
> >
> > > -Original Message-
> > > From: Dan Williams
> > > Sent: Friday, September 10, 2021 11:42 PM
> > > To: Justin He
> > > Cc: Vishal Verma ; Dave Jiang
> > > ; David Hildenbrand ; Linux
> NVDIMM
> > > ; Linux Kernel Mailing List > > ker...@vger.kernel.org>
> > > Subject: Re: [PATCH v2] device-dax: use fallback nid when numa node is
> > > invalid
> > >
> > > On Fri, Sep 10, 2021 at 5:46 AM Jia He wrote:
> > > >
> > > > Previously, numa_off was set unconditionally in dummy_numa_init()
> > > > even with a fake numa node. Then ACPI sets node id as NUMA_NO_NODE(-1)
> > > > after acpi_map_pxm_to_node() because it regards numa_off as turning
> > > > off the numa node. Hence dev_dax->target_node is NUMA_NO_NODE on
> > > > arm64 with fake numa case.
> > > >
> > > > Without this patch, pmem can't be probed as RAM devices on arm64 if
> > > > SRAT table isn't present:
> > > > $ndctl create-namespace -fe namespace0.0 --mode=devdax --map=dev -s
> 1g
> > > -a 64K
> > > > kmem dax0.0: rejecting DAX region [mem 0x24040-0x2bfff]
> with
> > > invalid node: -1
> > > > kmem: probe of dax0.0 failed with error -22
> > > >
> > > > This fixes it by using fallback memory_add_physaddr_to_nid() as nid.
> > > >
> > > > Suggested-by: David Hildenbrand
> > > > Signed-off-by: Jia He
> > > > ---
> > > > v2: - rebase it based on David's "memory group" patch.
> > > > - drop the changes in dev_dax_kmem_remove() since nid had been
> > > > removed in remove_memory().
> > > > drivers/dax/kmem.c | 31 +--
> > > > 1 file changed, 17 insertions(+), 14 deletions(-)
> > > >
> > > > diff --git a/drivers/dax/kmem.c b/drivers/dax/kmem.c
> > > > index a37622060fff..e4836eb7539e 100644
> > > > --- a/drivers/dax/kmem.c
> > > > +++ b/drivers/dax/kmem.c
> > > > @@ -47,20 +47,7 @@ static int dev_dax_kmem_probe(struct dev_dax
> *dev_dax)
> > > > unsigned long total_len = 0;
> > > > struct dax_kmem_data *data;
> > > > int i, rc, mapped = 0;
> > > > - int numa_node;
> > > > -
> > > > - /*
> > > > -* Ensure good NUMA information for the persistent memory.
> > > > -* Without this check, there is a risk that slow memory
> > > > -* could be mixed in a node with faster memory, causing
> > > > -* unavoidable performance issues.
> > > > -*/
> > > > - numa_node = dev_dax->target_node;
> > > > - if (numa_node < 0) {
> > > > - dev_warn(dev, "rejecting DAX region with invalid
> > > node: %d\n",
> > > > - numa_node);
> > > > - return -EINVAL;
> > > > - }
> > > > + int numa_node = dev_dax->target_node;
> > > >
> > > > for (i = 0; i < dev_dax->nr_range; i++) {
> > > > struct range range;
> > > > @@ -71,6 +58,22 @@ static int dev_dax_kmem_probe(struct dev_dax
> *dev_dax)
> > > > i, range.start, range.end);
> > > > continue;
> > > > }
> > > > +
> > > > + /*
> > > > +* Ensure good NUMA information for the persistent
> > > memory.
> > > > +* Without this check, there is a risk but not fatal
> > > that slow
> > > > +* memory could be mixed in a node with faster memory,
> > > causing
> > > > +* unavoidable performance issues. Warn this and use
> > > fallback
> > > > +* node id.
> > > > +*/
> > > > + if (numa_node < 0) {
RE: [PATCH v2] device-dax: use fallback nid when numa node is invalid
Hi Dan,
> -Original Message-
> From: Dan Williams
> Sent: Friday, September 10, 2021 11:42 PM
> To: Justin He
> Cc: Vishal Verma ; Dave Jiang
> ; David Hildenbrand ; Linux NVDIMM
> ; Linux Kernel Mailing List ker...@vger.kernel.org>
> Subject: Re: [PATCH v2] device-dax: use fallback nid when numa node is
> invalid
>
> On Fri, Sep 10, 2021 at 5:46 AM Jia He wrote:
> >
> > Previously, numa_off was set unconditionally in dummy_numa_init()
> > even with a fake numa node. Then ACPI sets node id as NUMA_NO_NODE(-1)
> > after acpi_map_pxm_to_node() because it regards numa_off as turning
> > off the numa node. Hence dev_dax->target_node is NUMA_NO_NODE on
> > arm64 with fake numa case.
> >
> > Without this patch, pmem can't be probed as RAM devices on arm64 if
> > SRAT table isn't present:
> > $ndctl create-namespace -fe namespace0.0 --mode=devdax --map=dev -s 1g
> -a 64K
> > kmem dax0.0: rejecting DAX region [mem 0x24040-0x2bfff] with
> invalid node: -1
> > kmem: probe of dax0.0 failed with error -22
> >
> > This fixes it by using fallback memory_add_physaddr_to_nid() as nid.
> >
> > Suggested-by: David Hildenbrand
> > Signed-off-by: Jia He
> > ---
> > v2: - rebase it based on David's "memory group" patch.
> > - drop the changes in dev_dax_kmem_remove() since nid had been
> > removed in remove_memory().
> > drivers/dax/kmem.c | 31 +--
> > 1 file changed, 17 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/dax/kmem.c b/drivers/dax/kmem.c
> > index a37622060fff..e4836eb7539e 100644
> > --- a/drivers/dax/kmem.c
> > +++ b/drivers/dax/kmem.c
> > @@ -47,20 +47,7 @@ static int dev_dax_kmem_probe(struct dev_dax *dev_dax)
> > unsigned long total_len = 0;
> > struct dax_kmem_data *data;
> > int i, rc, mapped = 0;
> > - int numa_node;
> > -
> > - /*
> > -* Ensure good NUMA information for the persistent memory.
> > -* Without this check, there is a risk that slow memory
> > -* could be mixed in a node with faster memory, causing
> > -* unavoidable performance issues.
> > -*/
> > - numa_node = dev_dax->target_node;
> > - if (numa_node < 0) {
> > - dev_warn(dev, "rejecting DAX region with invalid
> node: %d\n",
> > - numa_node);
> > - return -EINVAL;
> > - }
> > + int numa_node = dev_dax->target_node;
> >
> > for (i = 0; i < dev_dax->nr_range; i++) {
> > struct range range;
> > @@ -71,6 +58,22 @@ static int dev_dax_kmem_probe(struct dev_dax *dev_dax)
> > i, range.start, range.end);
> > continue;
> > }
> > +
> > + /*
> > +* Ensure good NUMA information for the persistent
> memory.
> > +* Without this check, there is a risk but not fatal
> that slow
> > +* memory could be mixed in a node with faster memory,
> causing
> > +* unavoidable performance issues. Warn this and use
> fallback
> > +* node id.
> > +*/
> > + if (numa_node < 0) {
> > + int new_node =
> memory_add_physaddr_to_nid(range.start);
> > +
> > + dev_info(dev, "changing nid from %d to %d for
> DAX region [%#llx-%#llx]\n",
> > +numa_node, new_node, range.start,
> range.end);
> > + numa_node = new_node;
> > + }
> > +
> > total_len += range_len();
>
> This fallback change belongs where the parent region for the namespace
> adopts its target_node, because it's not clear
> memory_add_physaddr_to_nid() is the right fallback in all situations.
> Here is where this setting is happening currently:
>
> drivers/acpi/nfit/core.c:3004: ndr_desc->target_node =
> pxm_to_node(spa->proximity_domain);
On my local arm64 guest('virt' machine type), the target_node is
set to -1 at this line.
That is:
The condition "spa->flags & ACPI_NFIT_PROXIMITY_VALID" is hit.
> drivers/acpi/nfit/core.c:3007: ndr_desc->target_node =
> NUMA_NO_NODE;
> drivers/nvdimm/e820.c:29: ndr_desc.target_node = nid;
> drivers/nvdimm/of_pmem.c:58:ndr_desc.target_node =
> ndr_desc.numa_node;
>
[tip: x86/platform] x86/platform/uv: Fix indentation warning in Documentation/ABI/testing/sysfs-firmware-sgi_uv
The following commit has been merged into the x86/platform branch of tip:
Commit-ID: e93d757c3f33c8a09f4aae579da4dc4500707471
Gitweb:
https://git.kernel.org/tip/e93d757c3f33c8a09f4aae579da4dc4500707471
Author:Justin Ernst
AuthorDate:Fri, 19 Feb 2021 12:28:52 -06:00
Committer: Borislav Petkov
CommitterDate: Sat, 06 Mar 2021 12:28:35 +01:00
x86/platform/uv: Fix indentation warning in
Documentation/ABI/testing/sysfs-firmware-sgi_uv
Commit
c9624cb7db1c ("x86/platform/uv: Update sysfs documentation")
misplaced the first line of a codeblock section, causing the reported
warning message:
Documentation/ABI/testing/sysfs-firmware-sgi_uv:2: WARNING: Unexpected
indentation.
Move the misplaced line below the required blank line to remove the
warning message.
Fixes: c9624cb7db1c ("x86/platform/uv: Update sysfs documentation")
Reported-by: Stephen Rothwell
Signed-off-by: Justin Ernst
Signed-off-by: Borislav Petkov
Acked-by: Mike Travis
Link: https://lkml.kernel.org/r/20210219182852.385297-1-justin.er...@hpe.com
---
Documentation/ABI/testing/sysfs-firmware-sgi_uv | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/ABI/testing/sysfs-firmware-sgi_uv
b/Documentation/ABI/testing/sysfs-firmware-sgi_uv
index 637c668..12ed843 100644
--- a/Documentation/ABI/testing/sysfs-firmware-sgi_uv
+++ b/Documentation/ABI/testing/sysfs-firmware-sgi_uv
@@ -39,8 +39,8 @@ Description:
The uv_type entry contains the hub revision number.
This value can be used to identify the UV system version::
- "0.*" = Hubless UV ('*' is subtype)
+ "0.*" = Hubless UV ('*' is subtype)
"3.0" = UV2
"5.0" = UV3
"7.0" = UV4
NAND Flash Issue Need Help!
ading file 'kernel/uImage' kernel/uImage not found! Strange that I need to run same command twice and that uImage also can not be loaded from SystemB Log file for all I did is attached. Justin Mitchell Thank you !!
RE: [PATCH] KVM: arm64: Fix unaligned addr case in mmu walking
Hi Marc
> -Original Message-
> From: Will Deacon
> Sent: Thursday, March 4, 2021 5:13 AM
> To: Marc Zyngier
> Cc: Justin He ; kvm...@lists.cs.columbia.edu; James
> Morse ; Julien Thierry ;
> Suzuki Poulose ; Catalin Marinas
> ; Gavin Shan ; Yanan Wang
> ; Quentin Perret ; linux-arm-
> ker...@lists.infradead.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] KVM: arm64: Fix unaligned addr case in mmu walking
>
> On Wed, Mar 03, 2021 at 07:07:37PM +, Marc Zyngier wrote:
> > From e0524b41a71e0f17d6dc8f197e421e677d584e72 Mon Sep 17 00:00:00 2001
> > From: Jia He
> > Date: Wed, 3 Mar 2021 10:42:25 +0800
> > Subject: [PATCH] KVM: arm64: Fix range alignment when walking page tables
> >
> > When walking the page tables at a given level, and if the start
> > address for the range isn't aligned for that level, we propagate
> > the misalignment on each iteration at that level.
> >
> > This results in the walker ignoring a number of entries (depending
> > on the original misalignment) on each subsequent iteration.
> >
> > Properly aligning the address at the before the next iteration
>
> "at the before the next" ???
>
> > addresses the issue.
> >
> > Cc: sta...@vger.kernel.org
> > Reported-by: Howard Zhang
> > Signed-off-by: Jia He
> > Fixes: b1e57de62cfb ("KVM: arm64: Add stand-alone page-table walker
> infrastructure")
> > [maz: rewrite commit message]
> > Signed-off-by: Marc Zyngier
> > Link: https://lore.kernel.org/r/20210303024225.2591-1-justin...@arm.com
> > ---
> > arch/arm64/kvm/hyp/pgtable.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
> > index 4d177ce1d536..124cd2f93020 100644
> > --- a/arch/arm64/kvm/hyp/pgtable.c
> > +++ b/arch/arm64/kvm/hyp/pgtable.c
> > @@ -223,7 +223,7 @@ static inline int __kvm_pgtable_visit(struct
> kvm_pgtable_walk_data *data,
> > goto out;
> >
> > if (!table) {
> > - data->addr += kvm_granule_size(level);
> > + data->addr = ALIGN(data->addr, kvm_granule_size(level));
What if previous data->addr is already aligned with kvm_granule_size(level)?
Hence a deadloop? Am I missing anything else?
--
Cheers,
Justin (Jia He)
> > goto out;
> > }
>
> If Jia is happy with it, please feel free to add:
>
> Acked-by: Will Deacon
>
> Will
RE: [PATCH] KVM: arm64: Fix unaligned addr case in mmu walking
Hi Quentin and Marc
I noticed Marc had sent out new version on behalf of me, thanks for the help.
I hated the time difference, sorry for the late.
Just answer the comments below to make it clear.
> -Original Message-
> From: Quentin Perret
> Sent: Wednesday, March 3, 2021 7:09 PM
> To: Marc Zyngier
> Cc: Justin He ; kvm...@lists.cs.columbia.edu; James
> Morse ; Julien Thierry ;
> Suzuki Poulose ; Catalin Marinas
> ; Will Deacon ; Gavin Shan
> ; Yanan Wang ; linux-arm-
> ker...@lists.infradead.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] KVM: arm64: Fix unaligned addr case in mmu walking
>
> On Wednesday 03 Mar 2021 at 09:54:25 (+), Marc Zyngier wrote:
> > Hi Jia,
> >
> > On Wed, 03 Mar 2021 02:42:25 +,
> > Jia He wrote:
> > >
> > > If the start addr is not aligned with the granule size of that level.
> > > loop step size should be adjusted to boundary instead of simple
> > > kvm_granual_size(level) increment. Otherwise, some mmu entries might
> miss
> > > the chance to be walked through.
> > > E.g. Assume the unmap range [data->addr, data->end] is
> > > [0xff00ab2000,0xff00cb2000] in level 2 walking and NOT block mapping.
> >
> > When does this occur? Upgrade from page mappings to block? Swap out?
> >
> > > And the 1st part of that pmd entry is [0xff00ab2000,0xff00c0]. The
> > > pmd value is 0x83fbd2c1002 (not valid entry). In this case, data->addr
> > > should be adjusted to 0xff00c0 instead of 0xff00cb2000.
> >
> > Let me see if I understand this. Assuming 4k pages, the region
> > described above spans *two* 2M entries:
> >
> > (a) ff00ab2000-ff00c0, part of ff00a0-ff00c0
> > (b) ff00c0-ff00db2000, part of ff00c0-ff00e0
> >
> > (a) has no valid mapping, but (b) does. Because we fail to correctly
> > align on a block boundary when skipping (a), we also skip (b), which
> > is then left mapped.
> >
> > Did I get it right? If so, yes, this is... annoying.
> >
Yes, exactly the case
> > Understanding the circumstances this triggers in would be most
> > interesting. This current code seems to assume that we get ranges
> > aligned to mapping boundaries, but I seem to remember that the old
> > code did use the stage2_*_addr_end() helpers to deal with this case.
> >
> > Will: I don't think things have changed in that respect, right?
>
> Indeed we should still use stage2_*_addr_end(), especially in the unmap
> path that is mentioned here, so it would be helpful to have a little bit
> more context.
Yes, stage2_pgd_addr_end() was still there but the stage2_pmd_addr_end() was
removed.
>
> > > Without this fix, userspace "segment fault" error can be easily
> > > triggered by running simple gVisor runsc cases on an Ampere Altra
> > > server:
> > > docker run --runtime=runsc -it --rm ubuntu /bin/bash
> > >
> > > In container:
> > > for i in `seq 1 100`;do ls;done
> >
> > The workload on its own isn't that interesting. What I'd like to
> > understand is what happens on the host during that time.
Okay
> >
> > >
> > > Reported-by: Howard Zhang
> > > Signed-off-by: Jia He
> > > ---
> > > arch/arm64/kvm/hyp/pgtable.c | 1 +
> > > 1 file changed, 1 insertion(+)
> > >
> > > diff --git a/arch/arm64/kvm/hyp/pgtable.c
> b/arch/arm64/kvm/hyp/pgtable.c
> > > index bdf8e55ed308..4d99d07c610c 100644
> > > --- a/arch/arm64/kvm/hyp/pgtable.c
> > > +++ b/arch/arm64/kvm/hyp/pgtable.c
> > > @@ -225,6 +225,7 @@ static inline int __kvm_pgtable_visit(struct
> kvm_pgtable_walk_data *data,
> > > goto out;
> > >
> > > if (!table) {
> > > + data->addr = ALIGN_DOWN(data->addr, kvm_granule_size(level));
> > > data->addr += kvm_granule_size(level);
> > > goto out;
> > > }
> >
> > It otherwise looks good to me. Quentin, Will: unless you object to
> > this, I plan to take it in the next round of fixes with
>
> Though I'm still unsure how we hit that today, the change makes sense on
> its own I think, so no objection from me.
>
> Thanks,
> Quentin
[tip: x86/platform] x86/platform/uv: Fix indentation warning in Documentation/ABI/testing/sysfs-firmware-sgi_uv
The following commit has been merged into the x86/platform branch of tip:
Commit-ID: 2430915f8291212f2bd2155176b817c34a18a2b1
Gitweb:
https://git.kernel.org/tip/2430915f8291212f2bd2155176b817c34a18a2b1
Author:Justin Ernst
AuthorDate:Fri, 19 Feb 2021 12:28:52 -06:00
Committer: Borislav Petkov
CommitterDate: Mon, 01 Mar 2021 11:14:25 +01:00
x86/platform/uv: Fix indentation warning in
Documentation/ABI/testing/sysfs-firmware-sgi_uv
Commit
c9624cb7db1c ("x86/platform/uv: Update sysfs documentation")
misplaced the first line of a codeblock section, causing the reported
warning message:
Documentation/ABI/testing/sysfs-firmware-sgi_uv:2: WARNING: Unexpected
indentation.
Move the misplaced line below the required blank line to remove the
warning message.
Fixes: c9624cb7db1c ("x86/platform/uv: Update sysfs documentation")
Reported-by: Stephen Rothwell
Signed-off-by: Justin Ernst
Signed-off-by: Borislav Petkov
Acked-by: Mike Travis
Link: https://lkml.kernel.org/r/20210219182852.385297-1-justin.er...@hpe.com
---
Documentation/ABI/testing/sysfs-firmware-sgi_uv | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/ABI/testing/sysfs-firmware-sgi_uv
b/Documentation/ABI/testing/sysfs-firmware-sgi_uv
index 637c668..12ed843 100644
--- a/Documentation/ABI/testing/sysfs-firmware-sgi_uv
+++ b/Documentation/ABI/testing/sysfs-firmware-sgi_uv
@@ -39,8 +39,8 @@ Description:
The uv_type entry contains the hub revision number.
This value can be used to identify the UV system version::
- "0.*" = Hubless UV ('*' is subtype)
+ "0.*" = Hubless UV ('*' is subtype)
"3.0" = UV2
"5.0" = UV3
"7.0" = UV4
RE: linux-next: build warning in Linus' tree
> Hi all,
>
> On Thu, 18 Feb 2021 22:47:57 + "Ernst, Justin"
> wrote:
> >
> > Hi,
> > We made a special effort to squash the unexpected indentation warnings in
> > c159376490ee
> (https://lore.kernel.org/lkml/20201130214304.369348-1-justin.er...@hpe.com/),
> so I was surprised to
> see this again.
> > Commit:
> >
> > c9624cb7db1c ("x86/platform/uv: Update sysfs documentation")
> >
> > is the culprit here. I suspect it was written and submitted before we made
> > the effort to fix the
> Unexpected indentation in c159376490ee, so it misplaced the first line of a
> codeblock, the original
> problem that was reported and fixed.
> >
> > The fix:
> >
> > diff --git a/Documentation/ABI/testing/sysfs-firmware-sgi_uv
> > b/Documentation/ABI/testing/sysfs-
> firmware-sgi_uv
> > index 637c668cbe45..12ed843e1d3e 100644
> > --- a/Documentation/ABI/testing/sysfs-firmware-sgi_uv
> > +++ b/Documentation/ABI/testing/sysfs-firmware-sgi_uv
> > @@ -39,8 +39,8 @@ Description:
> >
> > The uv_type entry contains the hub revision number.
> > This value can be used to identify the UV system version::
> > - "0.*" = Hubless UV ('*' is subtype)
> >
> > + "0.*" = Hubless UV ('*' is subtype)
> > "3.0" = UV2
> > "5.0" = UV3
> > "7.0" = UV4
> >
> > Thanks,
> > Justin
> >
> > > Building Linus' tree, today's linux-next build (htmldocs) produced
> > > this warning:
> > >
> > > Documentation/ABI/testing/sysfs-firmware-sgi_uv:2: WARNING: Unexpected
> > > indentation.
> > >
> > > Introduced by commit
> > >
> > > c159376490ee ("x86/platform/uv: Update ABI documentation of
> > > /sys/firmware/sgi_uv/")
> > >
> > > Or maybe an ealier one.
> > >
> > > This has been around for some time.
>
> I am still seeing this warning.
I submitted a patch here:
https://lore.kernel.org/lkml/20210219182852.385297-1-justin.er...@hpe.com/
Thanks,
Justin Ernst
>
> --
> Cheers,
> Stephen Rothwell
[PATCH] x86/platform/uv: Fix indentation warning in Documentation/ABI/testing/sysfs-firmware-sgi_uv
commit c9624cb7db1c ("x86/platform/uv: Update sysfs documentation")
misplaced the first line of a codeblock section, causing the reported
warning message:
Documentation/ABI/testing/sysfs-firmware-sgi_uv:2: WARNING: Unexpected
indentation.
Move the misplaced line below the required blank line to remove the
warning message.
Reported-by: Stephen Rothwell
Fixes: c9624cb7db1c ("x86/platform/uv: Update sysfs documentation")
Acked-by: Mike Travis
Signed-off-by: Justin Ernst
---
Documentation/ABI/testing/sysfs-firmware-sgi_uv | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/ABI/testing/sysfs-firmware-sgi_uv
b/Documentation/ABI/testing/sysfs-firmware-sgi_uv
index 637c668cbe45..12ed843e1d3e 100644
--- a/Documentation/ABI/testing/sysfs-firmware-sgi_uv
+++ b/Documentation/ABI/testing/sysfs-firmware-sgi_uv
@@ -39,8 +39,8 @@ Description:
The uv_type entry contains the hub revision number.
This value can be used to identify the UV system version::
- "0.*" = Hubless UV ('*' is subtype)
+ "0.*" = Hubless UV ('*' is subtype)
"3.0" = UV2
"5.0" = UV3
"7.0" = UV4
--
2.26.2
RE: linux-next: build warning in Linus' tree
Hi,
We made a special effort to squash the unexpected indentation warnings in
c159376490ee
(https://lore.kernel.org/lkml/20201130214304.369348-1-justin.er...@hpe.com/),
so I was surprised to see this again.
Commit:
c9624cb7db1c ("x86/platform/uv: Update sysfs documentation")
is the culprit here. I suspect it was written and submitted before we made the
effort to fix the Unexpected indentation in c159376490ee, so it misplaced the
first line of a codeblock, the original problem that was reported and fixed.
The fix:
diff --git a/Documentation/ABI/testing/sysfs-firmware-sgi_uv
b/Documentation/ABI/testing/sysfs-firmware-sgi_uv
index 637c668cbe45..12ed843e1d3e 100644
--- a/Documentation/ABI/testing/sysfs-firmware-sgi_uv
+++ b/Documentation/ABI/testing/sysfs-firmware-sgi_uv
@@ -39,8 +39,8 @@ Description:
The uv_type entry contains the hub revision number.
This value can be used to identify the UV system version::
- "0.*" = Hubless UV ('*' is subtype)
+ "0.*" = Hubless UV ('*' is subtype)
"3.0" = UV2
"5.0" = UV3
"7.0" = UV4
Thanks,
Justin
> Hi all,
>
> Building Linus' tree, today's linux-next build (htmldocs) produced
> this warning:
>
> Documentation/ABI/testing/sysfs-firmware-sgi_uv:2: WARNING: Unexpected
> indentation.
>
> Introduced by commit
>
> c159376490ee ("x86/platform/uv: Update ABI documentation of
> /sys/firmware/sgi_uv/")
>
> Or maybe an ealier one.
>
> This has been around for some time.
>
> --
> Cheers,
> Stephen Rothwell
Re: [PATCH RFC] gcc-plugins: Handle GCC version mismatch for OOT modules
On Tue, Jan 26, 2021 at 11:07 AM Greg KH wrote: > > On Tue, Jan 26, 2021 at 10:19:34AM -0600, Josh Poimboeuf wrote: > > On Tue, Jan 26, 2021 at 10:15:52AM -0600, Justin Forbes wrote: > > > On Tue, Jan 26, 2021 at 10:05 AM Peter Zijlstra > > > wrote: > > > > > > > > On Tue, Jan 26, 2021 at 09:46:51AM -0600, Josh Poimboeuf wrote: > > > > > On Tue, Jan 26, 2021 at 04:15:37PM +0100, Peter Zijlstra wrote: > > > > > > On Tue, Jan 26, 2021 at 08:51:55AM -0600, Josh Poimboeuf wrote: > > > > > > > User space mixes compiler versions all the time. The C ABI is > > > > > > > stable. > > > > > > > > > > > > > > What specifically is the harder issue you're referring to? > > > > > > > > > > > > I don't think the C ABI captures nearly enough. Imagine trying to > > > > > > mix a > > > > > > compiler with and without asm-goto support (ok, we fail to build > > > > > > without > > > > > > by now, but just imagine). > > > > > > > > > > > > No C ABI violated, but having that GCC extention vs not having it > > > > > > radically changes the kernel ABI. > > > > > > > > > > > > I think I'm with Greg here, just don't do it. > > > > > > > > > > Ok, thank you for an actual example. asm goto is a good one. > > > > > > > > > > But it's not a cut-and-dry issue. Otherwise how could modversions > > > > > possibly work? > > > > > > > > > > So yes, we should enforce GCC versions, but I still haven't seen a > > > > > reason it should be more than just "same compiler and *major* > > > > > version". > > > > > > > > Why bother? rebuilding the kernel and all modules is a matter of 10 > > > > minutes at most on a decently beefy build box. > > > > > > > > What actual problem are we trying to solve here? > > > > > > This is true for those of us used to working with source and building > > > by hand. For users who want everything packaged, rebuilding a kernel > > > package for install is considerably longer, and for distros providing > > > builds for multiple arches, we are looking at a couple of hours at > > > best. From a Fedora standpoint, I am perfectly fine with it failing > > > if someone tries to build a module on gcc10 when the kernel was built > > > with gcc11. It's tedious when the kernel was built with gcc11 > > > yesterday, and a new gcc11 build today means that kernel needs to be > > > rebuilt. > > > > Right. It's a problem for distro users. The compiler and kernel are in > > separate packages, with separate release cadences. The latest compiler > > version may not exactly match what was used to build the latest kernel. > > Given that distros _should_ be updating their kernel faster than the > compiler updates, what's the real issue here? You build a kernel, and > all external modules, at the same time. If you want to build them at > different times, you make your build system ensure they were the same > compiler so that you are "bug compatible". > > And yes, it might be a pain if gcc11 gets updated every other day, but > as someone living with a "rolling-distro" you get used to it, otherwise > you just "pin" the build tools and keep that from happening. > > This isn't a new thing, we've been doing this for decades, why is this > surprising? We definitely build considerably more kernels than toolchains. From a rawhide standpoint though, a number of testers are willing to test RC releases, but are not willing to run debug kernels, so they installed rc4 yesterday, but will not install today's snapshot. I will build 3-5 new kernels before they update to rc5. We have been doing things this way for over a decade. It has never been a problem until we turned on CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL. Suddenly I am getting complaints.
Re: [PATCH RFC] gcc-plugins: Handle GCC version mismatch for OOT modules
On Tue, Jan 26, 2021 at 10:05 AM Peter Zijlstra wrote: > > On Tue, Jan 26, 2021 at 09:46:51AM -0600, Josh Poimboeuf wrote: > > On Tue, Jan 26, 2021 at 04:15:37PM +0100, Peter Zijlstra wrote: > > > On Tue, Jan 26, 2021 at 08:51:55AM -0600, Josh Poimboeuf wrote: > > > > User space mixes compiler versions all the time. The C ABI is stable. > > > > > > > > What specifically is the harder issue you're referring to? > > > > > > I don't think the C ABI captures nearly enough. Imagine trying to mix a > > > compiler with and without asm-goto support (ok, we fail to build without > > > by now, but just imagine). > > > > > > No C ABI violated, but having that GCC extention vs not having it > > > radically changes the kernel ABI. > > > > > > I think I'm with Greg here, just don't do it. > > > > Ok, thank you for an actual example. asm goto is a good one. > > > > But it's not a cut-and-dry issue. Otherwise how could modversions > > possibly work? > > > > So yes, we should enforce GCC versions, but I still haven't seen a > > reason it should be more than just "same compiler and *major* version". > > Why bother? rebuilding the kernel and all modules is a matter of 10 > minutes at most on a decently beefy build box. > > What actual problem are we trying to solve here? This is true for those of us used to working with source and building by hand. For users who want everything packaged, rebuilding a kernel package for install is considerably longer, and for distros providing builds for multiple arches, we are looking at a couple of hours at best. From a Fedora standpoint, I am perfectly fine with it failing if someone tries to build a module on gcc10 when the kernel was built with gcc11. It's tedious when the kernel was built with gcc11 yesterday, and a new gcc11 build today means that kernel needs to be rebuilt.
Re: [PATCH RFC] gcc-plugins: Handle GCC version mismatch for OOT modules
On Tue, Jan 26, 2021 at 2:21 AM Greg KH wrote: > > On Mon, Jan 25, 2021 at 04:07:57PM -0600, Josh Poimboeuf wrote: > > On Tue, Jan 26, 2021 at 06:44:35AM +0900, Masahiro Yamada wrote: > > > > > If people use a different compiler, they must be > > > > > prepared for any possible problem. > > > > > > > > > > Using different compiler flags for in-tree and out-of-tree > > > > > is even more dangerous. > > > > > > > > > > For example, CONFIG_GCC_PLUGIN_RANDSTRUCT is enabled > > > > > for in-tree build, and then disabled for out-of-tree modules, > > > > > the struct layout will mismatch, won't it? > > > > > > > > If you read the patch you'll notice that it handles that case, when it's > > > > caused by GCC mismatch. > > > > > > > > However, as alluded to in the [1] footnote, it doesn't handle the case > > > > where the OOT build system doesn't have gcc-plugin-devel installed. > > > > Then CONFIG_GCC_PLUGIN_RANDSTRUCT gets silently disabled and the build > > > > succeeds! That happens even without a GCC mismatch. > > > > > > > > > Ah, sorry. > > > > > > I responded too early before reading the patch fully. > > > > > > But, I do not like to make RANDSTRUCT a special case. > > > > > > I'd rather want to stop building for any plugin. > > > > Other than RANDSTRUCT there doesn't seem to be any problem with > > disabling them (and printing a warning) in the OOT build. Why not give > > users that option? It's harmless, and will make distro's (and their > > users') lives easier. > > > > Either GCC mismatch is ok, or it's not. Let's not half-enforce it. > > As I said earlier, it's not ok, we can not support it at all. > Support and enforce are 2 completely different things. To shed a bit more light on this, the real issue that prompted this was breaking CI systems. As we enabled gcc plugins in Fedora, and the toolchain folks went through 3 different snapshots of gcc 11 in a week. Any CI process that built an out of tree module failed. I don't think this is nearly as much of a concern for stable distros, as it is for CI in development cycles. Justin
RE: [RFC PATCH 0/2] Avoid booting stall caused by idmap_kpti_install_ng_mappings
Hi Marc
> -Original Message-
> From: Justin He
> Sent: Wednesday, January 20, 2021 11:56 PM
> To: Marc Zyngier
> Cc: Catalin Marinas ; Will Deacon
> ; linux-arm-ker...@lists.infradead.org; linux-
> ker...@vger.kernel.org; Anshuman Khandual ;
> Suzuki Poulose ; Mark Rutland
> ; Gustavo A. R. Silva ;
> Richard Henderson ; Dave P Martin
> ; Steven Price ; Andrew Morton
> ; Mike Rapoport ; Ard
> Biesheuvel ; Gavin Shan ; Kefeng Wang
> ; Mark Brown ; Cristian
> Marussi
> Subject: RE: [RFC PATCH 0/2] Avoid booting stall caused by
> idmap_kpti_install_ng_mappings
>
> Hi Marc
>
> > -Original Message-
> > From: Marc Zyngier
> > Sent: Wednesday, January 20, 2021 6:58 PM
> > To: Justin He
> > Cc: Catalin Marinas ; Will Deacon
> > ; linux-arm-ker...@lists.infradead.org; linux-
> > ker...@vger.kernel.org; Anshuman Khandual ;
> > Suzuki Poulose ; Mark Rutland
> > ; Gustavo A. R. Silva ;
> > Richard Henderson ; Dave P Martin
> > ; Steven Price ; Andrew Morton
> > ; Mike Rapoport ; Ard
> > Biesheuvel ; Gavin Shan ; Kefeng Wang
> > ; Mark Brown ; Cristian
> > Marussi
> > Subject: Re: [RFC PATCH 0/2] Avoid booting stall caused by
> > idmap_kpti_install_ng_mappings
> >
> > Hi Justin,
> >
> > On 2021-01-20 04:51, Justin He wrote:
> > > Hi,
> > > Kindly ping
> > >
> > >> -Original Message-
> > >> From: Jia He
> > >> Sent: Wednesday, January 13, 2021 9:41 AM
> > >> To: Catalin Marinas ; Will Deacon
> > >> ; linux-arm-ker...@lists.infradead.org; linux-
> > >> ker...@vger.kernel.org
> > >> Cc: Anshuman Khandual ; Suzuki Poulose
> > >> ; Justin He ; Mark Rutland
> > >> ; Gustavo A. R. Silva ;
> > >> Richard Henderson ; Dave P Martin
> > >> ; Steven Price ; Andrew
> > >> Morton
> > >> ; Mike Rapoport ; Ard
> > >> Biesheuvel ; Gavin Shan ; Kefeng
> > >> Wang
> > >> ; Mark Brown ; Marc
> > >> Zyngier
> > >> ; Cristian Marussi
> > >> Subject: [RFC PATCH 0/2] Avoid booting stall caused by
> > >>
> > >> There is a 10s stall in idmap_kpti_install_ng_mappings when kernel
> > >> boots
> > >> on a Ampere EMAG server.
> > >>
> > >> Commit f992b4dfd58b ("arm64: kpti: Add ->enable callback to remap
> > >> swapper using nG mappings") updates the nG bit runtime if kpti is
> > >> required.
> > >>
> > >> But things get worse if rodata=full in map_mem(). NO_BLOCK_MAPPINGS |
> > >> NO_CONT_MAPPINGS is required when creating pagetable mapping. Hence
> > >> all
> > >> ptes are fully mapped in this case. On a Ampere EMAG server with 256G
> > >> memory(pagesize=4k), it causes the 10s stall.
> > >>
> > >> After moving init_cpu_features() ahead of early_fixmap_init(), we can
> > >> use
> > >> cpu_have_const_cap earlier than before. Hence we can avoid this stall
> > >> by updating arm64_use_ng_mappings.
> > >>
> > >> After this patch series, it reduces the kernel boot time from 14.7s to
> > >> 4.1s:
> > >> Before:
> > >> [ 14.757569] Freeing initrd memory: 60752K
> > >> After:
> > >> [4.138819] Freeing initrd memory: 60752K
> > >>
> > >> Set it as RFC because I want to resolve any other points which I have
> > >> misconerned.
> >
> > But you don't really explain *why* having the CPU Feature discovery
> > early helps at all. Is that so that you can bypass the idmap mapping?
>
> Adding nG bits can be avoided by having the discovery of boot cpu feature
> earlier since the nG bit had been set in PTE_MAYBE_NG/PMD_MAYBE_NG
>
> Before this patch:
> 1. kernel will firstly create mapping in setup_arch->paging_init->map_mem
> -> __map_memblock
> 2. Then if kpti is required, kernel will add nG bits for each pte entry.
> 3. In extreme case, e.g. physical memory is 256G,rodata=full, and pagesize
> is 4K, the nG bits updating in step 2 takes about 10s.
>
> > I'd expect something that explain the problem instead of paraphrasing
> > the patches.
> >
> > Another thing is whether you have tested this on some ThunderX HW
>
> I will find a TX1 as you told to see any difference.
>
>
I fortunately found a cavium TX1.
Seems that unmap_kernel_at_el0 is false:
...
[0.00] Machine model: Cavium ThunderX CN88XX board
...
[0.00] CPU features: kernel page table isolation forced OFF by
ARM64_WORKAROUND_CAVIUM_27456
...
Hence no such stall *before* and *after* this patch set because kpti is not
enabled.
--
Cheers,
Justin (Jia He)
RE: [RFC PATCH 0/2] Avoid booting stall caused by idmap_kpti_install_ng_mappings
Hi Marc
> -Original Message-
> From: Marc Zyngier
> Sent: Wednesday, January 20, 2021 6:58 PM
> To: Justin He
> Cc: Catalin Marinas ; Will Deacon
> ; linux-arm-ker...@lists.infradead.org; linux-
> ker...@vger.kernel.org; Anshuman Khandual ;
> Suzuki Poulose ; Mark Rutland
> ; Gustavo A. R. Silva ;
> Richard Henderson ; Dave P Martin
> ; Steven Price ; Andrew Morton
> ; Mike Rapoport ; Ard
> Biesheuvel ; Gavin Shan ; Kefeng Wang
> ; Mark Brown ; Cristian
> Marussi
> Subject: Re: [RFC PATCH 0/2] Avoid booting stall caused by
> idmap_kpti_install_ng_mappings
>
> Hi Justin,
>
> On 2021-01-20 04:51, Justin He wrote:
> > Hi,
> > Kindly ping
> >
> >> -Original Message-
> >> From: Jia He
> >> Sent: Wednesday, January 13, 2021 9:41 AM
> >> To: Catalin Marinas ; Will Deacon
> >> ; linux-arm-ker...@lists.infradead.org; linux-
> >> ker...@vger.kernel.org
> >> Cc: Anshuman Khandual ; Suzuki Poulose
> >> ; Justin He ; Mark Rutland
> >> ; Gustavo A. R. Silva ;
> >> Richard Henderson ; Dave P Martin
> >> ; Steven Price ; Andrew
> >> Morton
> >> ; Mike Rapoport ; Ard
> >> Biesheuvel ; Gavin Shan ; Kefeng
> >> Wang
> >> ; Mark Brown ; Marc
> >> Zyngier
> >> ; Cristian Marussi
> >> Subject: [RFC PATCH 0/2] Avoid booting stall caused by
> >>
> >> There is a 10s stall in idmap_kpti_install_ng_mappings when kernel
> >> boots
> >> on a Ampere EMAG server.
> >>
> >> Commit f992b4dfd58b ("arm64: kpti: Add ->enable callback to remap
> >> swapper using nG mappings") updates the nG bit runtime if kpti is
> >> required.
> >>
> >> But things get worse if rodata=full in map_mem(). NO_BLOCK_MAPPINGS |
> >> NO_CONT_MAPPINGS is required when creating pagetable mapping. Hence
> >> all
> >> ptes are fully mapped in this case. On a Ampere EMAG server with 256G
> >> memory(pagesize=4k), it causes the 10s stall.
> >>
> >> After moving init_cpu_features() ahead of early_fixmap_init(), we can
> >> use
> >> cpu_have_const_cap earlier than before. Hence we can avoid this stall
> >> by updating arm64_use_ng_mappings.
> >>
> >> After this patch series, it reduces the kernel boot time from 14.7s to
> >> 4.1s:
> >> Before:
> >> [ 14.757569] Freeing initrd memory: 60752K
> >> After:
> >> [4.138819] Freeing initrd memory: 60752K
> >>
> >> Set it as RFC because I want to resolve any other points which I have
> >> misconerned.
>
> But you don't really explain *why* having the CPU Feature discovery
> early helps at all. Is that so that you can bypass the idmap mapping?
Adding nG bits can be avoided by having the discovery of boot cpu feature
earlier since the nG bit had been set in PTE_MAYBE_NG/PMD_MAYBE_NG
Before this patch:
1. kernel will firstly create mapping in setup_arch->paging_init->map_mem
-> __map_memblock
2. Then if kpti is required, kernel will add nG bits for each pte entry.
3. In extreme case, e.g. physical memory is 256G,rodata=full, and pagesize
is 4K, the nG bits updating in step 2 takes about 10s.
> I'd expect something that explain the problem instead of paraphrasing
> the patches.
>
> Another thing is whether you have tested this on some ThunderX HW
I will find a TX1 as you told to see any difference.
--
Cheers,
Justin (Jia He)
> (the first version, not TX2), as this is the whole reason for this
> code...
>
> Thanks,
>
> M.
> --
> Jazz is not dead. It just smells funny...
RE: [RFC PATCH 0/2] Avoid booting stall caused by idmap_kpti_install_ng_mappings
Hi,
Kindly ping
> -Original Message-
> From: Jia He
> Sent: Wednesday, January 13, 2021 9:41 AM
> To: Catalin Marinas ; Will Deacon
> ; linux-arm-ker...@lists.infradead.org; linux-
> ker...@vger.kernel.org
> Cc: Anshuman Khandual ; Suzuki Poulose
> ; Justin He ; Mark Rutland
> ; Gustavo A. R. Silva ;
> Richard Henderson ; Dave P Martin
> ; Steven Price ; Andrew Morton
> ; Mike Rapoport ; Ard
> Biesheuvel ; Gavin Shan ; Kefeng Wang
> ; Mark Brown ; Marc Zyngier
> ; Cristian Marussi
> Subject: [RFC PATCH 0/2] Avoid booting stall caused by
>
> There is a 10s stall in idmap_kpti_install_ng_mappings when kernel boots
> on a Ampere EMAG server.
>
> Commit f992b4dfd58b ("arm64: kpti: Add ->enable callback to remap
> swapper using nG mappings") updates the nG bit runtime if kpti is
> required.
>
> But things get worse if rodata=full in map_mem(). NO_BLOCK_MAPPINGS |
> NO_CONT_MAPPINGS is required when creating pagetable mapping. Hence all
> ptes are fully mapped in this case. On a Ampere EMAG server with 256G
> memory(pagesize=4k), it causes the 10s stall.
>
> After moving init_cpu_features() ahead of early_fixmap_init(), we can use
> cpu_have_const_cap earlier than before. Hence we can avoid this stall
> by updating arm64_use_ng_mappings.
>
> After this patch series, it reduces the kernel boot time from 14.7s to
> 4.1s:
> Before:
> [ 14.757569] Freeing initrd memory: 60752K
> After:
> [4.138819] Freeing initrd memory: 60752K
>
> Set it as RFC because I want to resolve any other points which I have
> misconerned.
>
> Jia He (2):
> arm64/cpuinfo: Move init_cpu_features() ahead of early_fixmap_init()
> arm64: kpti: Update arm64_use_ng_mappings before pagetable mapping
>
> arch/arm64/include/asm/cpu.h | 1 +
> arch/arm64/kernel/cpuinfo.c | 13 ++---
> arch/arm64/kernel/setup.c| 18 +-
> arch/arm64/kernel/smp.c | 3 +--
> 4 files changed, 25 insertions(+), 10 deletions(-)
>
> --
> 2.17.1
RE: [PATCH 04/10] ABI: sysfs-firmware-sgi_uv
> From: Mauro Carvalho Chehab [mailto:mche...@kernel.org] On Behalf Of Mauro
> Carvalho Chehab
> Sent: Thursday, January 14, 2021 1:54 AM
> Subject: [PATCH 04/10] ABI: sysfs-firmware-sgi_uv
>
> Add a missing blank line required to identify a literal block,
> fixing this warning:
>
> .../Documentation/ABI/testing/sysfs-firmware-sgi_uv:2: WARNING:
> Unexpected indentation.
>
> Signed-off-by: Mauro Carvalho Chehab
Thanks for finding and fixing this. I was able to replicate the warning and
confirm the fix.
Reviewed-by: Justin Ernst
> ---
> Documentation/ABI/testing/sysfs-firmware-sgi_uv | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-firmware-sgi_uv
> b/Documentation/ABI/testing/sysfs-
> firmware-sgi_uv
> index 637c668cbe45..b0f79a1d14b3 100644
> --- a/Documentation/ABI/testing/sysfs-firmware-sgi_uv
> +++ b/Documentation/ABI/testing/sysfs-firmware-sgi_uv
> @@ -39,6 +39,7 @@ Description:
>
> The uv_type entry contains the hub revision number.
> This value can be used to identify the UV system version::
> +
> "0.*" = Hubless UV ('*' is subtype)
>
> "3.0" = UV2
> --
> 2.29.2
Re: [PATCH] mm/filemap: add static for function __add_to_page_cache_locked
On Mon, Dec 7, 2020 at 2:16 AM Michal Kubecek wrote: > > On Thu, Nov 12, 2020 at 08:18:57AM +0800, Alex Shi wrote: > > > > > > 在 2020/11/11 上午3:50, Andrew Morton 写道: > > > On Tue, 10 Nov 2020 08:39:24 +0530 Souptick Joarder > > > wrote: > > > > > >> On Fri, Nov 6, 2020 at 4:55 PM Alex Shi > > >> wrote: > > >>> > > >>> Otherwise it cause gcc warning: > > >>> ^~~ > > >>> ../mm/filemap.c:830:14: warning: no previous prototype for > > >>> ‘__add_to_page_cache_locked’ [-Wmissing-prototypes] > > >>> noinline int __add_to_page_cache_locked(struct page *page, > > >>> ^~ > > >> > > >> Is CONFIG_DEBUG_INFO_BTF enabled in your .config ? > > > > > > hm, yes. > > > > When the config enabled, compiling looks good untill pahole tool > > used to get BTF info, but I still failed on a right version pahole > > > 1.16. Sorry. > > > > > > > >>> > > >>> Signed-off-by: Alex Shi > > >>> Cc: Andrew Morton > > >>> Cc: linux...@kvack.org > > >>> Cc: linux-kernel@vger.kernel.org > > >>> --- > > >>> mm/filemap.c | 2 +- > > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > > >>> > > >>> diff --git a/mm/filemap.c b/mm/filemap.c > > >>> index d90614f501da..249cf489f5df 100644 > > >>> --- a/mm/filemap.c > > >>> +++ b/mm/filemap.c > > >>> @@ -827,7 +827,7 @@ int replace_page_cache_page(struct page *old, > > >>> struct page *new, gfp_t gfp_mask) > > >>> } > > >>> EXPORT_SYMBOL_GPL(replace_page_cache_page); > > >>> > > >>> -noinline int __add_to_page_cache_locked(struct page *page, > > >>> +static noinline int __add_to_page_cache_locked(struct page *page, > > >>> struct address_space *mapping, > > >>> pgoff_t offset, gfp_t gfp, > > >>> void **shadowp) > > > > > > It's unclear to me whether BTF_ID() requires that the target symbol be > > > non-static. It doesn't actually reference the symbol: > > > > > > #define BTF_ID(prefix, name) \ > > > __BTF_ID(__ID(__BTF_ID__##prefix##__##name##__)) > > > > > > > The above usage make me thought BTF don't require the symbol, though > > the symbol still exist in vmlinux with 'static'. > > > > So any comments of this, Alexei? > > It's probably more complicated: our v5.10-rc7 builds with > CONFIG_DEBUG_INFO_BTF=y fail on ppc64 and ppc64le with > > BTFIDS vmlinux >FAILED unresolved symbol __add_to_page_cache_locked > > > but succeed on x86_64, i586, aarch64 and s390x. So far I don't see why > this should depend on architecture. > Fedora is failing with rc7 on the same issue on PPC only. Justin
[tip: x86/platform] x86/platform/uv: Update ABI documentation of /sys/firmware/sgi_uv/
The following commit has been merged into the x86/platform branch of tip: Commit-ID: c159376490eef39f0f2cb1ce5dd38a6d41c859b4 Gitweb: https://git.kernel.org/tip/c159376490eef39f0f2cb1ce5dd38a6d41c859b4 Author:Justin Ernst AuthorDate:Wed, 25 Nov 2020 11:54:43 -06:00 Committer: Borislav Petkov CommitterDate: Tue, 01 Dec 2020 13:59:07 +01:00 x86/platform/uv: Update ABI documentation of /sys/firmware/sgi_uv/ Update the ABI documentation to describe the sysfs interface provided by the new uv_sysfs platform driver. [ bp: Merge in kernel-doc warning fixes, see second Link: below. ] Signed-off-by: Justin Ernst Signed-off-by: Borislav Petkov Reviewed-by: Steve Wahl Acked-by: Hans de Goede Link: https://lkml.kernel.org/r/20201125175444.279074-5-justin.er...@hpe.com Link: https://lkml.kernel.org/r/20201130214304.369348-1-justin.er...@hpe.com --- Documentation/ABI/testing/sysfs-firmware-sgi_uv | 144 +-- 1 file changed, 130 insertions(+), 14 deletions(-) diff --git a/Documentation/ABI/testing/sysfs-firmware-sgi_uv b/Documentation/ABI/testing/sysfs-firmware-sgi_uv index 66800ba..351b1f4 100644 --- a/Documentation/ABI/testing/sysfs-firmware-sgi_uv +++ b/Documentation/ABI/testing/sysfs-firmware-sgi_uv @@ -1,27 +1,143 @@ What: /sys/firmware/sgi_uv/ -Date: August 2008 -Contact: Russ Anderson +Date: September 2020 +Contact: Justin Ernst Description: The /sys/firmware/sgi_uv directory contains information - about the SGI UV platform. + about the UV platform. - Under that directory are a number of files:: + Under that directory are a number of read-only attributes:: partition_id coherence_id + uv_type The partition_id entry contains the partition id. - SGI UV systems can be partitioned into multiple physical + UV systems can be partitioned into multiple physical machines, which each partition running a unique copy - of the operating system. Each partition will have a unique - partition id. To display the partition id, use the command:: - - cat /sys/firmware/sgi_uv/partition_id + of the operating system. Each partition will have a unique + partition id. The coherence_id entry contains the coherence id. - A partitioned SGI UV system can have one or more coherence - domain. The coherence id indicates which coherence domain - this partition is in. To display the coherence id, use the - command:: + A partitioned UV system can have one or more coherence + domains. The coherence id indicates which coherence domain + this partition is in. + + The uv_type entry contains the hub revision number. + This value can be used to identify the UV system version:: + + "3.0" = UV2 + "5.0" = UV3 + "7.0" = UV4 + "7.1" = UV4a + "9.0" = UV5 + + The /sys/firmware/sgi_uv directory also contains two directories:: + + hubs/ + pcibuses/ + + The hubs directory contains a number of hub objects, each representing + a UV Hub visible to the BIOS. Each hub object's name is appended by a + unique ordinal value (ex. /sys/firmware/sgi_uv/hubs/hub_5) + + Each hub object directory contains a number of read-only attributes:: + + cnode + location + name + nasid + shared + this_partition + + The cnode entry contains the cnode number of the corresponding hub. + If a cnode value is not applicable, the value returned will be -1. + + The location entry contains the location string of the corresponding hub. + This value is used to physically identify a hub within a system. + + The name entry contains the name of the corresponding hub. This name can + be two variants:: + + "UVHub x.x" = A 'node' ASIC, connecting a CPU to the interconnect + fabric. The 'x.x' value represents the ASIC revision. + (ex. 'UVHub 5.0') + + "NLxRouter" = A 'router ASIC, only connecting other ASICs to + the interconnect fabric. The 'x' value representing + the fabric technology version. (ex. 'NL8Router') + +
[tip: x86/platform] x86/platform/uv: Update MAINTAINERS for uv_sysfs driver
The following commit has been merged into the x86/platform branch of tip: Commit-ID: 6043082c96844fa3a047896212e2da0adc1dde81 Gitweb: https://git.kernel.org/tip/6043082c96844fa3a047896212e2da0adc1dde81 Author:Justin Ernst AuthorDate:Wed, 25 Nov 2020 11:54:44 -06:00 Committer: Borislav Petkov CommitterDate: Tue, 01 Dec 2020 13:59:20 +01:00 x86/platform/uv: Update MAINTAINERS for uv_sysfs driver Add an entry and email address for the new uv_sysfs driver and its maintainer. Signed-off-by: Justin Ernst Signed-off-by: Borislav Petkov Acked-by: Hans de Goede Acked-by: Steve Wahl Link: https://lkml.kernel.org/r/20201125175444.279074-6-justin.er...@hpe.com --- MAINTAINERS | 6 ++ 1 file changed, 6 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index a008b70..bcf83e1 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -18354,6 +18354,12 @@ F: include/uapi/linux/uuid.h F: lib/test_uuid.c F: lib/uuid.c +UV SYSFS DRIVER +M: Justin Ernst +L: platform-driver-...@vger.kernel.org +S: Maintained +F: drivers/platform/x86/uv_sysfs.c + UVESAFB DRIVER M: Michal Januszewski L: linux-fb...@vger.kernel.org
RE: linux-next: build warnings after merge of the tip tree
> On Mon, Nov 30, 2020 at 06:05:03PM +1100, Stephen Rothwell wrote:
> > Hi all,
> >
> > After merging the tip tree, today's linux-next build (htmldocs) produced
> > these warnings:
> >
> > Documentation/ABI/testing/sysfs-firmware-sgi_uv:2: WARNING: Unexpected
> > indentation.
> > Documentation/ABI/testing/sysfs-firmware-sgi_uv:2: WARNING: Unexpected
> > indentation.
> > Documentation/ABI/testing/sysfs-firmware-sgi_uv:2: WARNING: Unexpected
> > indentation.
> >
> > Introduced by commit
> >
> > 7ac2f1017115 ("x86/platform/uv: Update ABI documentation of
> > /sys/firmware/sgi_uv/")
>
> Yah, I can reproduce but I have no clue what sphinx wants from me. Line
> 2 looks ok which could mean that the warning line it points to is bogus.
>
> Justin, this is all yours. :)
After scratching my head for a while, I found that the issue was missing empty
lines before three different code-block sections.
The line number is definitely bogus, but I wasn't able to discover why.
You can find my patch at: https://lkml.org/lkml/2020/11/30/1196
The patch depends on the v2 patch set Mike Travis
submitted, which hasn't made it to tip yet.
Thanks,
-Justin
>
> Thx.
>
> --
> Regards/Gruss,
> Boris.
>
> SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 36809, AG
> Nürnberg
[PATCH] x86/platform/uv: Fix/cleanup ABI documentation of /sys/firmware/sgi_uv/
With the introduction of commit
7ac2f1017115 ("x86/platform/uv: Update ABI documentation of
/sys/firmware/sgi_uv/")
3 new warnings were reported from the htmldocs build:
Documentation/ABI/testing/sysfs-firmware-sgi_uv:2: WARNING: Unexpected
indentation.
Documentation/ABI/testing/sysfs-firmware-sgi_uv:2: WARNING: Unexpected
indentation.
Documentation/ABI/testing/sysfs-firmware-sgi_uv:2: WARNING: Unexpected
indentation.
The line number seems to be irrelevant to the location of the cause for warning.
Three new empty lines were added before codeblock sections to remove the
warnings.
Lines directly before codeblock sections with one ':' get a second ':' to match
required formating.
Change an 'If' to 'It'.
This patch depends on a patch in the prior v2 patchset sent by Mike Travis
x86/platform/uv: Update MAINTAINERS for uv_sysfs driver
Signed-off-by: Justin Ernst
---
.../ABI/testing/sysfs-firmware-sgi_uv | 33 ++-
1 file changed, 18 insertions(+), 15 deletions(-)
diff --git a/Documentation/ABI/testing/sysfs-firmware-sgi_uv
b/Documentation/ABI/testing/sysfs-firmware-sgi_uv
index 1994d2621eaa..62aeb2a023f1 100644
--- a/Documentation/ABI/testing/sysfs-firmware-sgi_uv
+++ b/Documentation/ABI/testing/sysfs-firmware-sgi_uv
@@ -5,7 +5,7 @@ Description:
The /sys/firmware/sgi_uv directory contains information
about the UV platform.
- Under that directory are a number of read-only attributes:
+ Under that directory are a number of read-only attributes::
archtype
hub_type
@@ -16,7 +16,7 @@ Description:
The archtype entry contains the UV architecture type that
is used to select arch-dependent addresses and features.
- If can be set via the OEM_ID in the ACPI MADT table or by
+ It can be set via the OEM_ID in the ACPI MADT table or by
UVsystab entry both passed from UV BIOS.
The hub_type entry is used to select the type of hub which is
@@ -38,7 +38,8 @@ Description:
this partition is in.
The uv_type entry contains the hub revision number.
- This value can be used to identify the UV system version:
+ This value can be used to identify the UV system version::
+
"0.*" = Hubless UV ('*' is subtype)
"3.0" = UV2
"5.0" = UV3
@@ -46,7 +47,7 @@ Description:
"7.1" = UV4a
"9.0" = UV5
- The /sys/firmware/sgi_uv directory also contains two
directories:
+ The /sys/firmware/sgi_uv directory also contains two
directories::
hubs/
pcibuses/
@@ -55,7 +56,7 @@ Description:
a UV Hub visible to the BIOS. Each hub object's name is
appended by a
unique ordinal value (ex. /sys/firmware/sgi_uv/hubs/hub_5)
- Each hub object directory contains a number of read-only
attributes:
+ Each hub object directory contains a number of read-only
attributes::
cnode
location
@@ -71,13 +72,15 @@ Description:
This value is used to physically identify a hub within a system.
The name entry contains the name of the corresponding hub. This
name can
- be two variants:
+ be two variants::
+
"UVHub x.x" = A 'node' ASIC, connecting a CPU to the
interconnect
- fabric. The 'x.x' value represents the ASIC
revision.
- (ex. 'UVHub 5.0')
+ fabric. The 'x.x' value represents the ASIC revision.
+ (ex. 'UVHub 5.0')
+
"NLxRouter" = A 'router ASIC, only connecting other
ASICs to
- the interconnect fabric. The 'x' value
representing
- the fabric technology version. (ex. 'NL8Router')
+ the interconnect fabric. The 'x' value representing
+ the fabric technology version. (ex. 'NL8Router')
The nasid entry contains the nasid number of the corresponding
hub.
If a nasid value is not applicable, the value returned will be
-1.
@@ -93,7 +96,7 @@ Description:
A port object's name is appended by a unique ordinal value
(ex. /sys/firmware/sgi_uv/hubs/hub_5/port_3)
- Each port object directory contains a number of read-only
attributes:
+ Each port object directory contains a number of read-only
attributes::
conn_hub
conn_port
@@ -116
[tip: x86/platform] x86/platform/uv: Add and export uv_bios_* functions
The following commit has been merged into the x86/platform branch of tip:
Commit-ID: 9a3c425cfdfee169622f1cb1a974b2f287e5560c
Gitweb:
https://git.kernel.org/tip/9a3c425cfdfee169622f1cb1a974b2f287e5560c
Author:Justin Ernst
AuthorDate:Wed, 25 Nov 2020 11:54:41 -06:00
Committer: Borislav Petkov
CommitterDate: Thu, 26 Nov 2020 12:50:44 +01:00
x86/platform/uv: Add and export uv_bios_* functions
Add additional uv_bios_call() variant functions to expose information
needed by the new uv_sysfs driver. This includes the addition of several
new data types defined by UV BIOS and used in the new functions.
Signed-off-by: Justin Ernst
Signed-off-by: Borislav Petkov
Reviewed-by: Steve Wahl
Acked-by: Hans de Goede
Link: https://lkml.kernel.org/r/20201125175444.279074-3-justin.er...@hpe.com
---
arch/x86/include/asm/uv/bios.h | 49 ++-
arch/x86/include/asm/uv/uv_geo.h | 103 ++-
arch/x86/platform/uv/bios_uv.c | 55 -
3 files changed, 207 insertions(+)
create mode 100644 arch/x86/include/asm/uv/uv_geo.h
diff --git a/arch/x86/include/asm/uv/bios.h b/arch/x86/include/asm/uv/bios.h
index 08b3d81..01ba080 100644
--- a/arch/x86/include/asm/uv/bios.h
+++ b/arch/x86/include/asm/uv/bios.h
@@ -28,6 +28,20 @@ enum uv_bios_cmd {
UV_BIOS_SET_LEGACY_VGA_TARGET
};
+#define UV_BIOS_EXTRA 0x1
+#define UV_BIOS_GET_PCI_TOPOLOGY 0x10001
+#define UV_BIOS_GET_GEOINFO0x10003
+
+#define UV_BIOS_EXTRA_OP_MEM_COPYIN0x1000
+#define UV_BIOS_EXTRA_OP_MEM_COPYOUT 0x2000
+#define UV_BIOS_EXTRA_OP_MASK 0x0fff
+#define UV_BIOS_EXTRA_GET_HEAPSIZE 1
+#define UV_BIOS_EXTRA_INSTALL_HEAP 2
+#define UV_BIOS_EXTRA_MASTER_NASID 3
+#define UV_BIOS_EXTRA_OBJECT_COUNT (10|UV_BIOS_EXTRA_OP_MEM_COPYOUT)
+#define UV_BIOS_EXTRA_ENUM_OBJECTS (12|UV_BIOS_EXTRA_OP_MEM_COPYOUT)
+#define UV_BIOS_EXTRA_ENUM_PORTS (13|UV_BIOS_EXTRA_OP_MEM_COPYOUT)
+
/*
* Status values returned from a BIOS call.
*/
@@ -109,6 +123,32 @@ struct uv_systab {
} entry[1]; /* additional entries follow */
};
extern struct uv_systab *uv_systab;
+
+#define UV_BIOS_MAXSTRING128
+struct uv_bios_hub_info {
+ unsigned int id;
+ union {
+ struct {
+ unsigned long long this_part:1;
+ unsigned long long is_shared:1;
+ unsigned long long is_disabled:1;
+ } fields;
+ struct {
+ unsigned long long flags;
+ unsigned long long reserved;
+ } b;
+ } f;
+ char name[UV_BIOS_MAXSTRING];
+ char location[UV_BIOS_MAXSTRING];
+ unsigned int ports;
+};
+
+struct uv_bios_port_info {
+ unsigned int port;
+ unsigned int conn_id;
+ unsigned int conn_port;
+};
+
/* (... end of definitions from UV BIOS ...) */
enum {
@@ -142,6 +182,15 @@ extern s64 uv_bios_change_memprotect(u64, u64, enum
uv_memprotect);
extern s64 uv_bios_reserved_page_pa(u64, u64 *, u64 *, u64 *);
extern int uv_bios_set_legacy_vga_target(bool decode, int domain, int bus);
+extern s64 uv_bios_get_master_nasid(u64 sz, u64 *nasid);
+extern s64 uv_bios_get_heapsize(u64 nasid, u64 sz, u64 *heap_sz);
+extern s64 uv_bios_install_heap(u64 nasid, u64 sz, u64 *heap);
+extern s64 uv_bios_obj_count(u64 nasid, u64 sz, u64 *objcnt);
+extern s64 uv_bios_enum_objs(u64 nasid, u64 sz, u64 *objbuf);
+extern s64 uv_bios_enum_ports(u64 nasid, u64 obj_id, u64 sz, u64 *portbuf);
+extern s64 uv_bios_get_geoinfo(u64 nasid, u64 sz, u64 *geo);
+extern s64 uv_bios_get_pci_topology(u64 sz, u64 *buf);
+
extern int uv_bios_init(void);
extern unsigned long get_uv_systab_phys(bool msg);
diff --git a/arch/x86/include/asm/uv/uv_geo.h b/arch/x86/include/asm/uv/uv_geo.h
new file mode 100644
index 000..f241451
--- /dev/null
+++ b/arch/x86/include/asm/uv/uv_geo.h
@@ -0,0 +1,103 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License. See the file "COPYING" in the main directory of this archive
+ * for more details.
+ *
+ * Copyright (C) 2020 Hewlett Packard Enterprise Development LP. All rights
reserved.
+ */
+
+#ifndef _ASM_UV_GEO_H
+#define _ASM_UV_GEO_H
+
+/* Type declaractions */
+
+/* Size of a geoid_s structure (must be before decl. of geoid_u) */
+#define GEOID_SIZE 8
+
+/* Fields common to all substructures */
+struct geo_common_s {
+ unsigned char type; /* What type of h/w is named by this
geoid_s */
+ unsigned char blade;
+ unsigned char slot; /* slot is IRU */
+ unsigned char upos;
+ unsigned char rack;
+};
+
+/* Additional fields for particular types of hardware */
+struct geo_node_s {
+ struct geo_commo
[tip: x86/platform] x86/platform/uv: Add new uv_sysfs platform driver
The following commit has been merged into the x86/platform branch of tip:
Commit-ID: 4fc2cf1f2daf8303000efb7c9dc0307ea638a8f3
Gitweb:
https://git.kernel.org/tip/4fc2cf1f2daf8303000efb7c9dc0307ea638a8f3
Author:Justin Ernst
AuthorDate:Wed, 25 Nov 2020 11:54:42 -06:00
Committer: Borislav Petkov
CommitterDate: Thu, 26 Nov 2020 14:46:11 +01:00
x86/platform/uv: Add new uv_sysfs platform driver
Add the uv_sysfs driver to construct a read-only sysfs interface at
/sys/firmware/sgi_uv/ to expose information gathered from UV BIOS. This
information includes:
* UV Hub descriptions, including physical location
* Cabling layout between hubs on the fabric
* PCI topology, including physical location of PCI cards
Together, the information provides a robust physical description of a UV
system, useful for correlating to performance data or performing remote
support.
Signed-off-by: Justin Ernst
Signed-off-by: Borislav Petkov
Reviewed-by: Steve Wahl
Acked-by: Hans de Goede
Link: https://lkml.kernel.org/r/20201125175444.279074-4-justin.er...@hpe.com
---
drivers/platform/x86/Kconfig| 11 +-
drivers/platform/x86/Makefile | 3 +-
drivers/platform/x86/uv_sysfs.c | 862 +++-
3 files changed, 876 insertions(+)
create mode 100644 drivers/platform/x86/uv_sysfs.c
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 0d91d13..ba34153 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -78,6 +78,17 @@ config HUAWEI_WMI
To compile this driver as a module, choose M here: the module
will be called huawei-wmi.
+config UV_SYSFS
+ tristate "Sysfs structure for UV systems"
+ depends on X86_UV
+ depends on SYSFS
+ help
+ This driver supports a sysfs tree describing information about
+ UV systems at /sys/firmware/sgi_uv/.
+
+ To compile this driver as a module, choose M here: the module will
+ be called uv_sysfs.
+
config INTEL_WMI_SBL_FW_UPDATE
tristate "Intel WMI Slim Bootloader firmware update signaling driver"
depends on ACPI_WMI
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 5f823f7..a34875d 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -62,6 +62,9 @@ obj-$(CONFIG_HP_WIRELESS) += hp-wireless.o
obj-$(CONFIG_HP_WMI) += hp-wmi.o
obj-$(CONFIG_TC1100_WMI) += tc1100-wmi.o
+# Hewlett Packard Enterprise
+obj-$(CONFIG_UV_SYSFS) += uv_sysfs.o
+
# IBM Thinkpad and Lenovo
obj-$(CONFIG_IBM_RTL) += ibm_rtl.o
obj-$(CONFIG_IDEAPAD_LAPTOP) += ideapad-laptop.o
diff --git a/drivers/platform/x86/uv_sysfs.c b/drivers/platform/x86/uv_sysfs.c
new file mode 100644
index 000..54c3425
--- /dev/null
+++ b/drivers/platform/x86/uv_sysfs.c
@@ -0,0 +1,862 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * This file supports the /sys/firmware/sgi_uv topology tree on HPE UV.
+ *
+ * Copyright (c) 2020 Hewlett Packard Enterprise. All Rights Reserved.
+ * Copyright (c) Justin Ernst
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define INVALID_CNODE -1
+
+struct kobject *sgi_uv_kobj;
+struct kset *uv_pcibus_kset;
+struct kset *uv_hubs_kset;
+static struct uv_bios_hub_info *hub_buf;
+static struct uv_bios_port_info **port_buf;
+static struct uv_hub **uv_hubs;
+static struct uv_pci_top_obj **uv_pci_objs;
+static int num_pci_lines;
+static int num_cnodes;
+static int *prev_obj_to_cnode;
+static int uv_bios_obj_cnt;
+static signed short uv_master_nasid = -1;
+static void *uv_biosheap;
+
+static const char *uv_type_string(void)
+{
+ if (is_uv5_hub())
+ return "9.0";
+ else if (is_uv4a_hub())
+ return "7.1";
+ else if (is_uv4_hub())
+ return "7.0";
+ else if (is_uv3_hub())
+ return "5.0";
+ else if (is_uv2_hub())
+ return "3.0";
+ else
+ return "unknown";
+}
+
+static int ordinal_to_nasid(int ordinal)
+{
+ if (ordinal < num_cnodes && ordinal >= 0)
+ return UV_PNODE_TO_NASID(uv_blade_to_pnode(ordinal));
+ else
+ return -1;
+}
+
+static union geoid_u cnode_to_geoid(int cnode)
+{
+ union geoid_u geoid;
+
+ uv_bios_get_geoinfo(ordinal_to_nasid(cnode), (u64)sizeof(union
geoid_u), (u64 *));
+ return geoid;
+}
+
+static int location_to_bpos(char *location, int *rack, int *slot, int *blade)
+{
+ char type, r, b, h;
+ int idb, idh;
+
+ if (sscanf(location, "%c%03d%c%02d%c%2d%c%d",
+, rack, , slot, , , , ) != 8)
+ return -1;
+ *blade = idb * 2 + idh;
+
+ return 0;
+}
+
+static int cache_obj_to_cnode(struct uv_bios_hub_info *obj)
+
[tip: x86/platform] x86/platform/uv: Update MAINTAINERS for uv_sysfs driver
The following commit has been merged into the x86/platform branch of tip: Commit-ID: caf371103ea17de58251714131b06682d86b0df8 Gitweb: https://git.kernel.org/tip/caf371103ea17de58251714131b06682d86b0df8 Author:Justin Ernst AuthorDate:Wed, 25 Nov 2020 11:54:44 -06:00 Committer: Borislav Petkov CommitterDate: Thu, 26 Nov 2020 17:17:18 +01:00 x86/platform/uv: Update MAINTAINERS for uv_sysfs driver Add an entry and email address for the new uv_sysfs driver and its maintainer. Signed-off-by: Justin Ernst Signed-off-by: Borislav Petkov Acked-by: Hans de Goede Acked-by: Steve Wahl Link: https://lkml.kernel.org/r/20201125175444.279074-6-justin.er...@hpe.com --- MAINTAINERS | 6 ++ 1 file changed, 6 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index a008b70..bcf83e1 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -18354,6 +18354,12 @@ F: include/uapi/linux/uuid.h F: lib/test_uuid.c F: lib/uuid.c +UV SYSFS DRIVER +M: Justin Ernst +L: platform-driver-...@vger.kernel.org +S: Maintained +F: drivers/platform/x86/uv_sysfs.c + UVESAFB DRIVER M: Michal Januszewski L: linux-fb...@vger.kernel.org
[tip: x86/platform] x86/platform/uv: Update ABI documentation of /sys/firmware/sgi_uv/
The following commit has been merged into the x86/platform branch of tip: Commit-ID: 7ac2f1017115ece5288465da2906ad23b8b07a65 Gitweb: https://git.kernel.org/tip/7ac2f1017115ece5288465da2906ad23b8b07a65 Author:Justin Ernst AuthorDate:Wed, 25 Nov 2020 11:54:43 -06:00 Committer: Borislav Petkov CommitterDate: Thu, 26 Nov 2020 17:16:45 +01:00 x86/platform/uv: Update ABI documentation of /sys/firmware/sgi_uv/ Update the ABI documentation to describe the sysfs interface provided by the new uv_sysfs platform driver. Signed-off-by: Justin Ernst Signed-off-by: Borislav Petkov Reviewed-by: Steve Wahl Acked-by: Hans de Goede Link: https://lkml.kernel.org/r/20201125175444.279074-5-justin.er...@hpe.com --- Documentation/ABI/testing/sysfs-firmware-sgi_uv | 141 +-- 1 file changed, 127 insertions(+), 14 deletions(-) diff --git a/Documentation/ABI/testing/sysfs-firmware-sgi_uv b/Documentation/ABI/testing/sysfs-firmware-sgi_uv index 66800ba..50e25ce 100644 --- a/Documentation/ABI/testing/sysfs-firmware-sgi_uv +++ b/Documentation/ABI/testing/sysfs-firmware-sgi_uv @@ -1,27 +1,140 @@ What: /sys/firmware/sgi_uv/ -Date: August 2008 -Contact: Russ Anderson +Date: September 2020 +Contact: Justin Ernst Description: The /sys/firmware/sgi_uv directory contains information - about the SGI UV platform. + about the UV platform. - Under that directory are a number of files:: + Under that directory are a number of read-only attributes: partition_id coherence_id + uv_type The partition_id entry contains the partition id. - SGI UV systems can be partitioned into multiple physical + UV systems can be partitioned into multiple physical machines, which each partition running a unique copy - of the operating system. Each partition will have a unique - partition id. To display the partition id, use the command:: - - cat /sys/firmware/sgi_uv/partition_id + of the operating system. Each partition will have a unique + partition id. The coherence_id entry contains the coherence id. - A partitioned SGI UV system can have one or more coherence - domain. The coherence id indicates which coherence domain - this partition is in. To display the coherence id, use the - command:: + A partitioned UV system can have one or more coherence + domains. The coherence id indicates which coherence domain + this partition is in. + + The uv_type entry contains the hub revision number. + This value can be used to identify the UV system version: + "3.0" = UV2 + "5.0" = UV3 + "7.0" = UV4 + "7.1" = UV4a + "9.0" = UV5 + + The /sys/firmware/sgi_uv directory also contains two directories: + + hubs/ + pcibuses/ + + The hubs directory contains a number of hub objects, each representing + a UV Hub visible to the BIOS. Each hub object's name is appended by a + unique ordinal value (ex. /sys/firmware/sgi_uv/hubs/hub_5) + + Each hub object directory contains a number of read-only attributes: + + cnode + location + name + nasid + shared + this_partition + + The cnode entry contains the cnode number of the corresponding hub. + If a cnode value is not applicable, the value returned will be -1. + + The location entry contains the location string of the corresponding hub. + This value is used to physically identify a hub within a system. + + The name entry contains the name of the corresponding hub. This name can + be two variants: + "UVHub x.x" = A 'node' ASIC, connecting a CPU to the interconnect + fabric. The 'x.x' value represents the ASIC revision. + (ex. 'UVHub 5.0') + "NLxRouter" = A 'router ASIC, only connecting other ASICs to + the interconnect fabric. The 'x' value representing + the fabric technology version. (ex. 'NL8Router') + + The nasid entry contains the nasid number of the corresponding hub. + If a nasid value is not applicable, the value retur
[tip: x86/platform] x86/platform/uv: Remove existing /sys/firmware/sgi_uv/ interface
The following commit has been merged into the x86/platform branch of tip:
Commit-ID: 8f061abbf543355d77fac5c23521b6b452da6310
Gitweb:
https://git.kernel.org/tip/8f061abbf543355d77fac5c23521b6b452da6310
Author:Justin Ernst
AuthorDate:Wed, 25 Nov 2020 11:54:40 -06:00
Committer: Borislav Petkov
CommitterDate: Thu, 26 Nov 2020 12:08:17 +01:00
x86/platform/uv: Remove existing /sys/firmware/sgi_uv/ interface
Remove existing interface at /sys/firmware/sgi_uv/, created by
arch/x86/platform/uv/uv_sysfs.c
This interface includes:
/sys/firmware/sgi_uv/coherence_id
/sys/firmware/sgi_uv/partition_id
Both coherence_id and partition_id will be re-introduced via a
new uv_sysfs driver.
Signed-off-by: Justin Ernst
Signed-off-by: Borislav Petkov
Reviewed-by: Steve Wahl
Acked-by: Hans de Goede
Link: https://lkml.kernel.org/r/20201125175444.279074-2-justin.er...@hpe.com
---
arch/x86/platform/uv/Makefile | 2 +-
arch/x86/platform/uv/uv_sysfs.c | 63 +
2 files changed, 1 insertion(+), 64 deletions(-)
delete mode 100644 arch/x86/platform/uv/uv_sysfs.c
diff --git a/arch/x86/platform/uv/Makefile b/arch/x86/platform/uv/Makefile
index 224ff05..1441dda 100644
--- a/arch/x86/platform/uv/Makefile
+++ b/arch/x86/platform/uv/Makefile
@@ -1,2 +1,2 @@
# SPDX-License-Identifier: GPL-2.0-only
-obj-$(CONFIG_X86_UV) += bios_uv.o uv_irq.o uv_sysfs.o uv_time.o
uv_nmi.o
+obj-$(CONFIG_X86_UV) += bios_uv.o uv_irq.o uv_time.o uv_nmi.o
diff --git a/arch/x86/platform/uv/uv_sysfs.c b/arch/x86/platform/uv/uv_sysfs.c
deleted file mode 100644
index 266773e..000
--- a/arch/x86/platform/uv/uv_sysfs.c
+++ /dev/null
@@ -1,63 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-or-later
-/*
- * This file supports the /sys/firmware/sgi_uv interfaces for SGI UV.
- *
- * Copyright (c) 2008 Silicon Graphics, Inc. All Rights Reserved.
- * Copyright (c) Russ Anderson
- */
-
-#include
-#include
-#include
-
-struct kobject *sgi_uv_kobj;
-
-static ssize_t partition_id_show(struct kobject *kobj,
- struct kobj_attribute *attr, char *buf)
-{
- return snprintf(buf, PAGE_SIZE, "%ld\n", sn_partition_id);
-}
-
-static ssize_t coherence_id_show(struct kobject *kobj,
- struct kobj_attribute *attr, char *buf)
-{
- return snprintf(buf, PAGE_SIZE, "%ld\n", sn_coherency_id);
-}
-
-static struct kobj_attribute partition_id_attr =
- __ATTR(partition_id, S_IRUGO, partition_id_show, NULL);
-
-static struct kobj_attribute coherence_id_attr =
- __ATTR(coherence_id, S_IRUGO, coherence_id_show, NULL);
-
-
-static int __init sgi_uv_sysfs_init(void)
-{
- unsigned long ret;
-
- if (!is_uv_system())
- return -ENODEV;
-
- if (!sgi_uv_kobj)
- sgi_uv_kobj = kobject_create_and_add("sgi_uv", firmware_kobj);
- if (!sgi_uv_kobj) {
- printk(KERN_WARNING "kobject_create_and_add sgi_uv failed\n");
- return -EINVAL;
- }
-
- ret = sysfs_create_file(sgi_uv_kobj, _id_attr.attr);
- if (ret) {
- printk(KERN_WARNING "sysfs_create_file partition_id failed\n");
- return ret;
- }
-
- ret = sysfs_create_file(sgi_uv_kobj, _id_attr.attr);
- if (ret) {
- printk(KERN_WARNING "sysfs_create_file coherence_id failed\n");
- return ret;
- }
-
- return 0;
-}
-
-device_initcall(sgi_uv_sysfs_init);
[PATCH v3 2/5] x86/platform/uv: Add and export uv_bios_* functions
Add additional uv_bios_call variant functions to expose information
needed by the new uv_sysfs driver. This includes the addition of several
new data types defined by UV BIOS and used in the new functions.
Signed-off-by: Justin Ernst
Reviewed-by: Steve Wahl
---
arch/x86/include/asm/uv/bios.h | 49 +++
arch/x86/include/asm/uv/uv_geo.h | 103 +++
arch/x86/platform/uv/bios_uv.c | 55 +
3 files changed, 207 insertions(+)
create mode 100644 arch/x86/include/asm/uv/uv_geo.h
diff --git a/arch/x86/include/asm/uv/bios.h b/arch/x86/include/asm/uv/bios.h
index 08b3d810dfba..01ba080887b3 100644
--- a/arch/x86/include/asm/uv/bios.h
+++ b/arch/x86/include/asm/uv/bios.h
@@ -28,6 +28,20 @@ enum uv_bios_cmd {
UV_BIOS_SET_LEGACY_VGA_TARGET
};
+#define UV_BIOS_EXTRA 0x1
+#define UV_BIOS_GET_PCI_TOPOLOGY 0x10001
+#define UV_BIOS_GET_GEOINFO0x10003
+
+#define UV_BIOS_EXTRA_OP_MEM_COPYIN0x1000
+#define UV_BIOS_EXTRA_OP_MEM_COPYOUT 0x2000
+#define UV_BIOS_EXTRA_OP_MASK 0x0fff
+#define UV_BIOS_EXTRA_GET_HEAPSIZE 1
+#define UV_BIOS_EXTRA_INSTALL_HEAP 2
+#define UV_BIOS_EXTRA_MASTER_NASID 3
+#define UV_BIOS_EXTRA_OBJECT_COUNT (10|UV_BIOS_EXTRA_OP_MEM_COPYOUT)
+#define UV_BIOS_EXTRA_ENUM_OBJECTS (12|UV_BIOS_EXTRA_OP_MEM_COPYOUT)
+#define UV_BIOS_EXTRA_ENUM_PORTS (13|UV_BIOS_EXTRA_OP_MEM_COPYOUT)
+
/*
* Status values returned from a BIOS call.
*/
@@ -109,6 +123,32 @@ struct uv_systab {
} entry[1]; /* additional entries follow */
};
extern struct uv_systab *uv_systab;
+
+#define UV_BIOS_MAXSTRING128
+struct uv_bios_hub_info {
+ unsigned int id;
+ union {
+ struct {
+ unsigned long long this_part:1;
+ unsigned long long is_shared:1;
+ unsigned long long is_disabled:1;
+ } fields;
+ struct {
+ unsigned long long flags;
+ unsigned long long reserved;
+ } b;
+ } f;
+ char name[UV_BIOS_MAXSTRING];
+ char location[UV_BIOS_MAXSTRING];
+ unsigned int ports;
+};
+
+struct uv_bios_port_info {
+ unsigned int port;
+ unsigned int conn_id;
+ unsigned int conn_port;
+};
+
/* (... end of definitions from UV BIOS ...) */
enum {
@@ -142,6 +182,15 @@ extern s64 uv_bios_change_memprotect(u64, u64, enum
uv_memprotect);
extern s64 uv_bios_reserved_page_pa(u64, u64 *, u64 *, u64 *);
extern int uv_bios_set_legacy_vga_target(bool decode, int domain, int bus);
+extern s64 uv_bios_get_master_nasid(u64 sz, u64 *nasid);
+extern s64 uv_bios_get_heapsize(u64 nasid, u64 sz, u64 *heap_sz);
+extern s64 uv_bios_install_heap(u64 nasid, u64 sz, u64 *heap);
+extern s64 uv_bios_obj_count(u64 nasid, u64 sz, u64 *objcnt);
+extern s64 uv_bios_enum_objs(u64 nasid, u64 sz, u64 *objbuf);
+extern s64 uv_bios_enum_ports(u64 nasid, u64 obj_id, u64 sz, u64 *portbuf);
+extern s64 uv_bios_get_geoinfo(u64 nasid, u64 sz, u64 *geo);
+extern s64 uv_bios_get_pci_topology(u64 sz, u64 *buf);
+
extern int uv_bios_init(void);
extern unsigned long get_uv_systab_phys(bool msg);
diff --git a/arch/x86/include/asm/uv/uv_geo.h b/arch/x86/include/asm/uv/uv_geo.h
new file mode 100644
index ..f241451035fb
--- /dev/null
+++ b/arch/x86/include/asm/uv/uv_geo.h
@@ -0,0 +1,103 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License. See the file "COPYING" in the main directory of this archive
+ * for more details.
+ *
+ * Copyright (C) 2020 Hewlett Packard Enterprise Development LP. All rights
reserved.
+ */
+
+#ifndef _ASM_UV_GEO_H
+#define _ASM_UV_GEO_H
+
+/* Type declaractions */
+
+/* Size of a geoid_s structure (must be before decl. of geoid_u) */
+#define GEOID_SIZE 8
+
+/* Fields common to all substructures */
+struct geo_common_s {
+ unsigned char type; /* What type of h/w is named by this
geoid_s */
+ unsigned char blade;
+ unsigned char slot; /* slot is IRU */
+ unsigned char upos;
+ unsigned char rack;
+};
+
+/* Additional fields for particular types of hardware */
+struct geo_node_s {
+ struct geo_common_s common; /* No additional fields needed
*/
+};
+
+struct geo_rtr_s {
+ struct geo_common_s common; /* No additional fields needed
*/
+};
+
+struct geo_iocntl_s {
+ struct geo_common_s common; /* No additional fields needed
*/
+};
+
+struct geo_pcicard_s {
+ struct geo_iocntl_s common;
+ char bus; /* Bus/widget number */
+ char slot; /* PCI slot number */
+};
+
+/* Subcomponents of a node */
+struct geo_cpu_s {
+
[PATCH v3 3/5] x86/platform/uv: Add new uv_sysfs platform driver
Add the uv_sysfs driver to construct a read-only sysfs interface at
/sys/firmware/sgi_uv/ to expose information gathered from UV BIOS.
This information includes:
UV Hub descriptions, including physical location
Cabling layout between hubs on the fabric
PCI topology, including physical location of PCI cards
Together, the information provides a robust physical description of a
UV system, useful for correlating to performance data or performing
remote support.
Signed-off-by: Justin Ernst
Reviewed-by: Steve Wahl
---
drivers/platform/x86/Kconfig| 11 +
drivers/platform/x86/Makefile | 3 +
drivers/platform/x86/uv_sysfs.c | 862
3 files changed, 876 insertions(+)
create mode 100644 drivers/platform/x86/uv_sysfs.c
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 0d91d136bc3b..ba34153571b8 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -78,6 +78,17 @@ config HUAWEI_WMI
To compile this driver as a module, choose M here: the module
will be called huawei-wmi.
+config UV_SYSFS
+ tristate "Sysfs structure for UV systems"
+ depends on X86_UV
+ depends on SYSFS
+ help
+ This driver supports a sysfs tree describing information about
+ UV systems at /sys/firmware/sgi_uv/.
+
+ To compile this driver as a module, choose M here: the module will
+ be called uv_sysfs.
+
config INTEL_WMI_SBL_FW_UPDATE
tristate "Intel WMI Slim Bootloader firmware update signaling driver"
depends on ACPI_WMI
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 5f823f7eff45..a34875d833dd 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -62,6 +62,9 @@ obj-$(CONFIG_HP_WIRELESS) += hp-wireless.o
obj-$(CONFIG_HP_WMI) += hp-wmi.o
obj-$(CONFIG_TC1100_WMI) += tc1100-wmi.o
+# Hewlett Packard Enterprise
+obj-$(CONFIG_UV_SYSFS) += uv_sysfs.o
+
# IBM Thinkpad and Lenovo
obj-$(CONFIG_IBM_RTL) += ibm_rtl.o
obj-$(CONFIG_IDEAPAD_LAPTOP) += ideapad-laptop.o
diff --git a/drivers/platform/x86/uv_sysfs.c b/drivers/platform/x86/uv_sysfs.c
new file mode 100644
index ..54c342579f1c
--- /dev/null
+++ b/drivers/platform/x86/uv_sysfs.c
@@ -0,0 +1,862 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * This file supports the /sys/firmware/sgi_uv topology tree on HPE UV.
+ *
+ * Copyright (c) 2020 Hewlett Packard Enterprise. All Rights Reserved.
+ * Copyright (c) Justin Ernst
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define INVALID_CNODE -1
+
+struct kobject *sgi_uv_kobj;
+struct kset *uv_pcibus_kset;
+struct kset *uv_hubs_kset;
+static struct uv_bios_hub_info *hub_buf;
+static struct uv_bios_port_info **port_buf;
+static struct uv_hub **uv_hubs;
+static struct uv_pci_top_obj **uv_pci_objs;
+static int num_pci_lines;
+static int num_cnodes;
+static int *prev_obj_to_cnode;
+static int uv_bios_obj_cnt;
+static signed short uv_master_nasid = -1;
+static void *uv_biosheap;
+
+static const char *uv_type_string(void)
+{
+ if (is_uv5_hub())
+ return "9.0";
+ else if (is_uv4a_hub())
+ return "7.1";
+ else if (is_uv4_hub())
+ return "7.0";
+ else if (is_uv3_hub())
+ return "5.0";
+ else if (is_uv2_hub())
+ return "3.0";
+ else
+ return "unknown";
+}
+
+static int ordinal_to_nasid(int ordinal)
+{
+ if (ordinal < num_cnodes && ordinal >= 0)
+ return UV_PNODE_TO_NASID(uv_blade_to_pnode(ordinal));
+ else
+ return -1;
+}
+
+static union geoid_u cnode_to_geoid(int cnode)
+{
+ union geoid_u geoid;
+
+ uv_bios_get_geoinfo(ordinal_to_nasid(cnode), (u64)sizeof(union
geoid_u), (u64 *));
+ return geoid;
+}
+
+static int location_to_bpos(char *location, int *rack, int *slot, int *blade)
+{
+ char type, r, b, h;
+ int idb, idh;
+
+ if (sscanf(location, "%c%03d%c%02d%c%2d%c%d",
+, rack, , slot, , , , ) != 8)
+ return -1;
+ *blade = idb * 2 + idh;
+
+ return 0;
+}
+
+static int cache_obj_to_cnode(struct uv_bios_hub_info *obj)
+{
+ int cnode;
+ union geoid_u geoid;
+ int obj_rack, obj_slot, obj_blade;
+ int rack, slot, blade;
+
+ if (!obj->f.fields.this_part && !obj->f.fields.is_shared)
+ return 0;
+
+ if (location_to_bpos(obj->location, _rack, _slot, _blade))
+ return -1;
+
+ for (cnode = 0; cnode < num_cnodes; cnode++) {
+ geoid = cnode_to_geoid(cnode);
+ rack = geo_rack(geoid);
+ slot = geo_slot(geoi
[PATCH v3 4/5] x86/platform/uv: Update ABI documentation of /sys/firmware/sgi_uv/
Update the ABI documentation to describe the sysfs interface provided by the new uv_sysfs platform driver. Signed-off-by: Justin Ernst Reviewed-by: Steve Wahl --- .../ABI/testing/sysfs-firmware-sgi_uv | 141 -- 1 file changed, 127 insertions(+), 14 deletions(-) diff --git a/Documentation/ABI/testing/sysfs-firmware-sgi_uv b/Documentation/ABI/testing/sysfs-firmware-sgi_uv index 66800baab096..50e25ce80fa2 100644 --- a/Documentation/ABI/testing/sysfs-firmware-sgi_uv +++ b/Documentation/ABI/testing/sysfs-firmware-sgi_uv @@ -1,27 +1,140 @@ What: /sys/firmware/sgi_uv/ -Date: August 2008 -Contact: Russ Anderson +Date: September 2020 +Contact: Justin Ernst Description: The /sys/firmware/sgi_uv directory contains information - about the SGI UV platform. + about the UV platform. - Under that directory are a number of files:: + Under that directory are a number of read-only attributes: partition_id coherence_id + uv_type The partition_id entry contains the partition id. - SGI UV systems can be partitioned into multiple physical + UV systems can be partitioned into multiple physical machines, which each partition running a unique copy - of the operating system. Each partition will have a unique - partition id. To display the partition id, use the command:: - - cat /sys/firmware/sgi_uv/partition_id + of the operating system. Each partition will have a unique + partition id. The coherence_id entry contains the coherence id. - A partitioned SGI UV system can have one or more coherence - domain. The coherence id indicates which coherence domain - this partition is in. To display the coherence id, use the - command:: + A partitioned UV system can have one or more coherence + domains. The coherence id indicates which coherence domain + this partition is in. + + The uv_type entry contains the hub revision number. + This value can be used to identify the UV system version: + "3.0" = UV2 + "5.0" = UV3 + "7.0" = UV4 + "7.1" = UV4a + "9.0" = UV5 + + The /sys/firmware/sgi_uv directory also contains two directories: + + hubs/ + pcibuses/ + + The hubs directory contains a number of hub objects, each representing + a UV Hub visible to the BIOS. Each hub object's name is appended by a + unique ordinal value (ex. /sys/firmware/sgi_uv/hubs/hub_5) + + Each hub object directory contains a number of read-only attributes: + + cnode + location + name + nasid + shared + this_partition + + The cnode entry contains the cnode number of the corresponding hub. + If a cnode value is not applicable, the value returned will be -1. + + The location entry contains the location string of the corresponding hub. + This value is used to physically identify a hub within a system. + + The name entry contains the name of the corresponding hub. This name can + be two variants: + "UVHub x.x" = A 'node' ASIC, connecting a CPU to the interconnect + fabric. The 'x.x' value represents the ASIC revision. + (ex. 'UVHub 5.0') + "NLxRouter" = A 'router ASIC, only connecting other ASICs to + the interconnect fabric. The 'x' value representing + the fabric technology version. (ex. 'NL8Router') + + The nasid entry contains the nasid number of the corresponding hub. + If a nasid value is not applicable, the value returned will be -1. + + The shared entry contains a boolean value describing whether the + corresponding hub is shared between system partitions. + + The this_partition entry contains a boolean value describing whether + the corresponding hub is local to the current partition. + + Each hub object directory also contains a number of port objects, + each representing a fabric port on the corresponding hub. + A port object's name is appended by a unique ord
[PATCH v3 5/5] x86/platform/uv: Update MAINTAINERS for uv_sysfs driver
Add an entry and email address for the new uv_sysfs driver and its maintainer. Signed-off-by: Justin Ernst Acked-by: Steve Wahl --- MAINTAINERS | 6 ++ 1 file changed, 6 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index b43b59542d15..f693d2d97203 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -18361,6 +18361,12 @@ F: include/uapi/linux/uuid.h F: lib/test_uuid.c F: lib/uuid.c +UV SYSFS DRIVER +M: Justin Ernst +L: platform-driver-...@vger.kernel.org +S: Maintained +F: drivers/platform/x86/uv_sysfs.c + UVESAFB DRIVER M: Michal Januszewski L: linux-fb...@vger.kernel.org -- 2.26.2
[PATCH v3 0/5] x86/platform/uv: Add uv_sysfs platform driver
Introduce a new platform driver to gather topology information from UV systems and expose that information via a sysfs interface at /sys/firmware/sgi_uv/. This is version 3 with these changes since version 2: * Export sn_coherency_id to fix build failure when UV_SYSFS=m, caused by re-introduction of /sys/firmware/sgi_uv/coherence_id in v2. * Fix a null pointer dereference in drivers/platform/x86/uv_sysfs.c:uv_ports_exit() caused by calling kobject_put() on an out of range index value. Version 2 included these changes since version 1: * Re-introduced /sys/firmware/sgi_uv/coherence_id file in the new driver after removing it in Patch 1/5. This keeps the userspace API unbroken. Justin Ernst (5): x86/platform/uv: Remove existing /sys/firmware/sgi_uv/ interface x86/platform/uv: Add and export uv_bios_* functions x86/platform/uv: Add new uv_sysfs platform driver x86/platform/uv: Update ABI documentation of /sys/firmware/sgi_uv/ x86/platform/uv: Update MAINTAINERS for uv_sysfs driver .../ABI/testing/sysfs-firmware-sgi_uv | 141 ++- MAINTAINERS | 6 + arch/x86/include/asm/uv/bios.h| 49 + arch/x86/include/asm/uv/uv_geo.h | 103 +++ arch/x86/platform/uv/Makefile | 2 +- arch/x86/platform/uv/bios_uv.c| 55 ++ arch/x86/platform/uv/uv_sysfs.c | 63 -- drivers/platform/x86/Kconfig | 11 + drivers/platform/x86/Makefile | 3 + drivers/platform/x86/uv_sysfs.c | 862 ++ 10 files changed, 1217 insertions(+), 78 deletions(-) create mode 100644 arch/x86/include/asm/uv/uv_geo.h delete mode 100644 arch/x86/platform/uv/uv_sysfs.c create mode 100644 drivers/platform/x86/uv_sysfs.c -- 2.26.2
[PATCH v3 1/5] x86/platform/uv: Remove existing /sys/firmware/sgi_uv/ interface
Remove existing interface at /sys/firmware/sgi_uv/, created by
arch/x86/platform/uv/uv_sysfs.c
This interface includes:
/sys/firmware/sgi_uv/coherence_id
/sys/firmware/sgi_uv/partition_id
Both coherence_id and partition_id will be re-introduced via a
new uv_sysfs driver.
Signed-off-by: Justin Ernst
Reviewed-by: Steve Wahl
---
arch/x86/platform/uv/Makefile | 2 +-
arch/x86/platform/uv/uv_sysfs.c | 63 -
2 files changed, 1 insertion(+), 64 deletions(-)
delete mode 100644 arch/x86/platform/uv/uv_sysfs.c
diff --git a/arch/x86/platform/uv/Makefile b/arch/x86/platform/uv/Makefile
index 224ff0504890..1441dda8edf7 100644
--- a/arch/x86/platform/uv/Makefile
+++ b/arch/x86/platform/uv/Makefile
@@ -1,2 +1,2 @@
# SPDX-License-Identifier: GPL-2.0-only
-obj-$(CONFIG_X86_UV) += bios_uv.o uv_irq.o uv_sysfs.o uv_time.o
uv_nmi.o
+obj-$(CONFIG_X86_UV) += bios_uv.o uv_irq.o uv_time.o uv_nmi.o
diff --git a/arch/x86/platform/uv/uv_sysfs.c b/arch/x86/platform/uv/uv_sysfs.c
deleted file mode 100644
index 266773e2fb37..
--- a/arch/x86/platform/uv/uv_sysfs.c
+++ /dev/null
@@ -1,63 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-or-later
-/*
- * This file supports the /sys/firmware/sgi_uv interfaces for SGI UV.
- *
- * Copyright (c) 2008 Silicon Graphics, Inc. All Rights Reserved.
- * Copyright (c) Russ Anderson
- */
-
-#include
-#include
-#include
-
-struct kobject *sgi_uv_kobj;
-
-static ssize_t partition_id_show(struct kobject *kobj,
- struct kobj_attribute *attr, char *buf)
-{
- return snprintf(buf, PAGE_SIZE, "%ld\n", sn_partition_id);
-}
-
-static ssize_t coherence_id_show(struct kobject *kobj,
- struct kobj_attribute *attr, char *buf)
-{
- return snprintf(buf, PAGE_SIZE, "%ld\n", sn_coherency_id);
-}
-
-static struct kobj_attribute partition_id_attr =
- __ATTR(partition_id, S_IRUGO, partition_id_show, NULL);
-
-static struct kobj_attribute coherence_id_attr =
- __ATTR(coherence_id, S_IRUGO, coherence_id_show, NULL);
-
-
-static int __init sgi_uv_sysfs_init(void)
-{
- unsigned long ret;
-
- if (!is_uv_system())
- return -ENODEV;
-
- if (!sgi_uv_kobj)
- sgi_uv_kobj = kobject_create_and_add("sgi_uv", firmware_kobj);
- if (!sgi_uv_kobj) {
- printk(KERN_WARNING "kobject_create_and_add sgi_uv failed\n");
- return -EINVAL;
- }
-
- ret = sysfs_create_file(sgi_uv_kobj, _id_attr.attr);
- if (ret) {
- printk(KERN_WARNING "sysfs_create_file partition_id failed\n");
- return ret;
- }
-
- ret = sysfs_create_file(sgi_uv_kobj, _id_attr.attr);
- if (ret) {
- printk(KERN_WARNING "sysfs_create_file coherence_id failed\n");
- return ret;
- }
-
- return 0;
-}
-
-device_initcall(sgi_uv_sysfs_init);
--
2.26.2
RE: [PATCH v2 0/5] Add uv_sysfs platform driver
Hans,
Thank you for your Ack of my patch set.
I've found a couple bugs that need fixing:
1. In my re-introduction of /sys/firmware/sgi_uv/coherence_id, I failed to
export the associated sn_coherency_id variable, causing the build to fail when
UV_SYSFS=m
2. A null pointer dereference in
drivers/platform/x86/uv_sysfs.c:uv_ports_exit() caused by calling kobject_put()
on an out of range index value.
I will be resubmitting the patch series shortly as v3.
I apologize for the hiccup.
Thanks,
Justin
> -Original Message-
> From: Hans de Goede [mailto:hdego...@redhat.com]
> Sent: Tuesday, November 24, 2020 5:30 AM
> To: Ernst, Justin ; Borislav Petkov ;
> Ingo Molnar
> ; Mark Gross ; Thomas Gleixner
> ; Wahl,
> Steve ; x...@kernel.org
> Cc: Andy Shevchenko ; Darren Hart ;
> Sivanich, Dimitri
> ; H . Peter Anvin ; Anderson, Russ
> ;
> linux-kernel@vger.kernel.org; platform-driver-...@vger.kernel.org; Cezary
> Rojewski
> ; Ilya Dryomov ; Jonathan
> Cameron
> ; Mauro Carvalho Chehab
> ; Vaibhav Jain
>
> Subject: Re: [PATCH v2 0/5] Add uv_sysfs platform driver
>
> Hi,
>
> Quick self intro for the x86/tip maintainers: I have take over
> drivers/platform/x86 maintainership from Andy.
>
> On 11/18/20 5:47 PM, Justin Ernst wrote:
> > Introduce a new platform driver to gather topology information from UV
> > systems
> > and expose that information via a sysfs interface at /sys/firmware/sgi_uv/.
> >
> > This is version 2 with these changes since version 1:
> >
> > * Re-introduced /sys/firmware/sgi_uv/coherence_id file in the new driver
> > after
> > removing it in Patch 1/5. This keeps the userspace API unbroken.
> >
> > Justin Ernst (5):
> > x86/platform/uv: Remove existing /sys/firmware/sgi_uv/ interface
> > x86/platform/uv: Add and export uv_bios_* functions
> > x86/platform/uv: Add new uv_sysfs platform driver
> > x86/platform/uv: Update ABI documentation of /sys/firmware/sgi_uv/
> > x86/platform/uv: Update MAINTAINERS for uv_sysfs driver>
> > .../ABI/testing/sysfs-firmware-sgi_uv | 141 ++-
> > MAINTAINERS | 6 +
> > arch/x86/include/asm/uv/bios.h| 49 +
> > arch/x86/include/asm/uv/uv_geo.h | 103 +++
> > arch/x86/platform/uv/Makefile | 2 +-
> > arch/x86/platform/uv/bios_uv.c| 54 ++
> > arch/x86/platform/uv/uv_sysfs.c | 63 --
> > drivers/platform/x86/Kconfig | 11 +
> > drivers/platform/x86/Makefile | 3 +
> > drivers/platform/x86/uv_sysfs.c | 862 ++
> > 10 files changed, 1216 insertions(+), 78 deletions(-)
> > create mode 100644 arch/x86/include/asm/uv/uv_geo.h
> > delete mode 100644 arch/x86/platform/uv/uv_sysfs.c
> > create mode 100644 drivers/platform/x86/uv_sysfs.c
>
> So this touches files under both arch/x86/ and drivers/platform/x86/ ,
> I believe this is best merged through the x86/tip tree and I don't
> expect conflicts for the drivers/platform/x86/{Kconfig,Makefile} changes.
>
> So here is my ack for merging this series through the x86/tip tree:
>
> Acked-by: Hans de Goede
>
> Regards,
>
> Hans
>
RE: [PATCH] vfio iommu type1: Bypass the vma permission check in vfio_pin_pages_remote()
Hi Peter > -Original Message- > From: Peter Xu > Sent: Wednesday, November 25, 2020 2:12 AM > To: Justin He > Cc: Alex Williamson ; Cornelia Huck > ; k...@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH] vfio iommu type1: Bypass the vma permission check in > vfio_pin_pages_remote() > > Hi, Jia, > > On Thu, Nov 19, 2020 at 10:27:37PM +0800, Jia He wrote: > > The permission of vfio iommu is different and incompatible with vma > > permission. If the iotlb->perm is IOMMU_NONE (e.g. qemu side), qemu will > > simply call unmap ioctl() instead of mapping. Hence vfio_dma_map() can't > > map a dma region with NONE permission. > > > > This corner case will be exposed in coming virtio_fs cache_size > > commit [1] > > - mmap(NULL, size, PROT_NONE, MAP_ANONYMOUS | MAP_PRIVATE, -1, 0); > >memory_region_init_ram_ptr() > > - re-mmap the above area with read/write authority. > > If iiuc here we'll remap the above PROT_NONE into PROT_READ|PROT_WRITE, > then... > > > - vfio_dma_map() will be invoked when vfio device is hotplug added. > > ... here I'm slightly confused on why VFIO_IOMMU_MAP_DMA would encounter > vma > check fail - aren't they already get rw permissions? No, we haven't got the vma rw permission yet, but the default permission in this case is rw by default. When qemu side invoke vfio_dma_map(), the rw of iommu will be automatically added [1] [2] (currently map a NONE region is not supported in qemu vfio). [1] https://git.qemu.org/?p=qemu.git;a=blob;f=hw/vfio/common.c;h=6ff1daa763f87a1ed5351bcc19aeb027c43b8a8f;hb=HEAD#l479 [2] https://git.qemu.org/?p=qemu.git;a=blob;f=hw/vfio/common.c;h=6ff1daa763f87a1ed5351bcc19aeb027c43b8a8f;hb=HEAD#l486 But at kernel side, the vma permission is created by PROT_NONE. Then the check in check_vma_flags() at [3] will be failed. [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/mm/gup.c#n929 > > I'd appreciate if you could explain why vfio needs to dma map some > PROT_NONE Virtiofs will map a PROT_NONE cache window region firstly, then remap the sub region of that cache window with read or write permission. I guess this might be an security concern. Just CC virtiofs expert Stefan to answer it more accurately. -- Cheers, Justin (Jia He) > pages after all, and whether QEMU would be able to postpone the vfio map of > those PROT_NONE pages until they got to become with RW permissions. > > Thanks, > > -- > Peter Xu IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
RE: [PATCH] vfio iommu type1: Bypass the vma permission check in vfio_pin_pages_remote()
Hi Alex, thanks for the comments.
See mine below:
> -Original Message-
> From: Alex Williamson
> Sent: Friday, November 20, 2020 1:05 AM
> To: Justin He
> Cc: Cornelia Huck ; k...@vger.kernel.org; linux-
> ker...@vger.kernel.org
> Subject: Re: [PATCH] vfio iommu type1: Bypass the vma permission check in
> vfio_pin_pages_remote()
>
> On Thu, 19 Nov 2020 22:27:37 +0800
> Jia He wrote:
>
> > The permission of vfio iommu is different and incompatible with vma
> > permission. If the iotlb->perm is IOMMU_NONE (e.g. qemu side), qemu will
> > simply call unmap ioctl() instead of mapping. Hence vfio_dma_map() can't
> > map a dma region with NONE permission.
> >
> > This corner case will be exposed in coming virtio_fs cache_size
> > commit [1]
> > - mmap(NULL, size, PROT_NONE, MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
> >memory_region_init_ram_ptr()
> > - re-mmap the above area with read/write authority.
> > - vfio_dma_map() will be invoked when vfio device is hotplug added.
> >
> > qemu:
> > vfio_listener_region_add()
> > vfio_dma_map(..., readonly=false)
> > map.flags is set to VFIO_DMA_MAP_FLAG_READ|VFIO_..._WRITE
> > ioctl(VFIO_IOMMU_MAP_DMA)
> >
> > kernel:
> > vfio_dma_do_map()
> > vfio_pin_map_dma()
> > vfio_pin_pages_remote()
> > vaddr_get_pfn()
> > ...
> > check_vma_flags() failed! because
> > vm_flags hasn't VM_WRITE && gup_flags
> > has FOLL_WRITE
> >
> > It will report error in qemu log when hotplug adding(vfio) a nvme disk
> > to qemu guest on an Ampere EMAG server:
> > "VFIO_MAP_DMA failed: Bad address"
>
> I don't fully understand the argument here, I think this is suggesting
> that because QEMU won't call VFIO_IOMMU_MAP_DMA on a region that has
> NONE permission, the kernel can ignore read/write permission by using
> FOLL_FORCE. Not only is QEMU not the only userspace driver for vfio,
> but regardless of that, we can't trust the behavior of any given
> userspace driver. Bypassing the permission check with FOLL_FORCE seems
> like it's placing the trust in the user, which seems like a security
> issue. Thanks,
Yes, this might have side impact on security.
But besides this simple fix(adding FOLL_FORCE), do you think it is a good
idea that:
Qemu provides a special vfio_dma_map_none_perm() to allow mapping a
region with NONE permission?
Thanks for any suggestion.
--
Cheers,
Justin (Jia He)
>
> Alex
>
>
> > [1] https://gitlab.com/virtio-fs/qemu/-/blob/virtio-fs-
> dev/hw/virtio/vhost-user-fs.c#L502
> >
> > Signed-off-by: Jia He
> > ---
> > drivers/vfio/vfio_iommu_type1.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/vfio/vfio_iommu_type1.c
> b/drivers/vfio/vfio_iommu_type1.c
> > index 67e827638995..33faa6b7dbd4 100644
> > --- a/drivers/vfio/vfio_iommu_type1.c
> > +++ b/drivers/vfio/vfio_iommu_type1.c
> > @@ -453,7 +453,8 @@ static int vaddr_get_pfn(struct mm_struct *mm,
> unsigned long vaddr,
> > flags |= FOLL_WRITE;
> >
> > mmap_read_lock(mm);
> > -ret = pin_user_pages_remote(mm, vaddr, 1, flags | FOLL_LONGTERM,
> > +ret = pin_user_pages_remote(mm, vaddr, 1,
> > +flags | FOLL_LONGTERM | FOLL_FORCE,
> > page, NULL, NULL);
> > if (ret == 1) {
> > *pfn = page_to_pfn(page[0]);
IMPORTANT NOTICE: The contents of this email and any attachments are
confidential and may also be privileged. If you are not the intended recipient,
please notify the sender immediately and do not disclose the contents to any
other person, use it for any purpose, or store or copy the information in any
medium. Thank you.
RE: [PATCH net] vsock/virtio: discard packets only when socket is really closed
> -Original Message-
> From: Stefano Garzarella
> Sent: Friday, November 20, 2020 6:48 PM
> To: net...@vger.kernel.org
> Cc: Sergio Lopez ; David S. Miller ;
> Stefano Garzarella ; Justin He ;
> k...@vger.kernel.org; linux-kernel@vger.kernel.org; Stefan Hajnoczi
> ; virtualizat...@lists.linux-foundation.org; Jakub
> Kicinski
> Subject: [PATCH net] vsock/virtio: discard packets only when socket is
> really closed
>
> Starting from commit 8692cefc433f ("virtio_vsock: Fix race condition
> in virtio_transport_recv_pkt"), we discard packets in
> virtio_transport_recv_pkt() if the socket has been released.
>
> When the socket is connected, we schedule a delayed work to wait the
> RST packet from the other peer, also if SHUTDOWN_MASK is set in
> sk->sk_shutdown.
> This is done to complete the virtio-vsock shutdown algorithm, releasing
> the port assigned to the socket definitively only when the other peer
> has consumed all the packets.
>
> If we discard the RST packet received, the socket will be closed only
> when the VSOCK_CLOSE_TIMEOUT is reached.
>
> Sergio discovered the issue while running ab(1) HTTP benchmark using
> libkrun [1] and observing a latency increase with that commit.
>
> To avoid this issue, we discard packet only if the socket is really
> closed (SOCK_DONE flag is set).
> We also set SOCK_DONE in virtio_transport_release() when we don't need
> to wait any packets from the other peer (we didn't schedule the delayed
> work). In this case we remove the socket from the vsock lists, releasing
> the port assigned.
>
> [1] https://github.com/containers/libkrun
>
> Fixes: 8692cefc433f ("virtio_vsock: Fix race condition in
> virtio_transport_recv_pkt")
Acked-by: Jia He
--
Cheers,
Justin (Jia He)
> Cc: justin...@arm.com
> Reported-by: Sergio Lopez
> Tested-by: Sergio Lopez
> Signed-off-by: Stefano Garzarella
> ---
> net/vmw_vsock/virtio_transport_common.c | 8 +---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/net/vmw_vsock/virtio_transport_common.c
> b/net/vmw_vsock/virtio_transport_common.c
> index 0edda1edf988..5956939eebb7 100644
> --- a/net/vmw_vsock/virtio_transport_common.c
> +++ b/net/vmw_vsock/virtio_transport_common.c
> @@ -841,8 +841,10 @@ void virtio_transport_release(struct vsock_sock *vsk)
> virtio_transport_free_pkt(pkt);
> }
>
> -if (remove_sock)
> +if (remove_sock) {
> +sock_set_flag(sk, SOCK_DONE);
> vsock_remove_sock(vsk);
> +}
> }
> EXPORT_SYMBOL_GPL(virtio_transport_release);
>
> @@ -1132,8 +1134,8 @@ void virtio_transport_recv_pkt(struct
> virtio_transport *t,
>
> lock_sock(sk);
>
> -/* Check if sk has been released before lock_sock */
> -if (sk->sk_shutdown == SHUTDOWN_MASK) {
> +/* Check if sk has been closed before lock_sock */
> +if (sock_flag(sk, SOCK_DONE)) {
> (void)virtio_transport_reset_no_sock(t, pkt);
> release_sock(sk);
> sock_put(sk);
> --
> 2.26.2
IMPORTANT NOTICE: The contents of this email and any attachments are
confidential and may also be privileged. If you are not the intended recipient,
please notify the sender immediately and do not disclose the contents to any
other person, use it for any purpose, or store or copy the information in any
medium. Thank you.
[PATCH v2 0/5] Add uv_sysfs platform driver
Introduce a new platform driver to gather topology information from UV systems and expose that information via a sysfs interface at /sys/firmware/sgi_uv/. This is version 2 with these changes since version 1: * Re-introduced /sys/firmware/sgi_uv/coherence_id file in the new driver after removing it in Patch 1/5. This keeps the userspace API unbroken. Justin Ernst (5): x86/platform/uv: Remove existing /sys/firmware/sgi_uv/ interface x86/platform/uv: Add and export uv_bios_* functions x86/platform/uv: Add new uv_sysfs platform driver x86/platform/uv: Update ABI documentation of /sys/firmware/sgi_uv/ x86/platform/uv: Update MAINTAINERS for uv_sysfs driver .../ABI/testing/sysfs-firmware-sgi_uv | 141 ++- MAINTAINERS | 6 + arch/x86/include/asm/uv/bios.h| 49 + arch/x86/include/asm/uv/uv_geo.h | 103 +++ arch/x86/platform/uv/Makefile | 2 +- arch/x86/platform/uv/bios_uv.c| 54 ++ arch/x86/platform/uv/uv_sysfs.c | 63 -- drivers/platform/x86/Kconfig | 11 + drivers/platform/x86/Makefile | 3 + drivers/platform/x86/uv_sysfs.c | 862 ++ 10 files changed, 1216 insertions(+), 78 deletions(-) create mode 100644 arch/x86/include/asm/uv/uv_geo.h delete mode 100644 arch/x86/platform/uv/uv_sysfs.c create mode 100644 drivers/platform/x86/uv_sysfs.c base-commit: 4ef8451b332662d004df269d4cdeb7d9f31419b5 -- 2.26.2
[PATCH v2 4/5] x86/platform/uv: Update ABI documentation of /sys/firmware/sgi_uv/
Update the ABI documentation to describe the sysfs interface provided by the new uv_sysfs platform driver. Signed-off-by: Justin Ernst Reviewed-by: Steve Wahl --- .../ABI/testing/sysfs-firmware-sgi_uv | 141 -- 1 file changed, 127 insertions(+), 14 deletions(-) diff --git a/Documentation/ABI/testing/sysfs-firmware-sgi_uv b/Documentation/ABI/testing/sysfs-firmware-sgi_uv index 66800baab096..50e25ce80fa2 100644 --- a/Documentation/ABI/testing/sysfs-firmware-sgi_uv +++ b/Documentation/ABI/testing/sysfs-firmware-sgi_uv @@ -1,27 +1,140 @@ What: /sys/firmware/sgi_uv/ -Date: August 2008 -Contact: Russ Anderson +Date: September 2020 +Contact: Justin Ernst Description: The /sys/firmware/sgi_uv directory contains information - about the SGI UV platform. + about the UV platform. - Under that directory are a number of files:: + Under that directory are a number of read-only attributes: partition_id coherence_id + uv_type The partition_id entry contains the partition id. - SGI UV systems can be partitioned into multiple physical + UV systems can be partitioned into multiple physical machines, which each partition running a unique copy - of the operating system. Each partition will have a unique - partition id. To display the partition id, use the command:: - - cat /sys/firmware/sgi_uv/partition_id + of the operating system. Each partition will have a unique + partition id. The coherence_id entry contains the coherence id. - A partitioned SGI UV system can have one or more coherence - domain. The coherence id indicates which coherence domain - this partition is in. To display the coherence id, use the - command:: + A partitioned UV system can have one or more coherence + domains. The coherence id indicates which coherence domain + this partition is in. + + The uv_type entry contains the hub revision number. + This value can be used to identify the UV system version: + "3.0" = UV2 + "5.0" = UV3 + "7.0" = UV4 + "7.1" = UV4a + "9.0" = UV5 + + The /sys/firmware/sgi_uv directory also contains two directories: + + hubs/ + pcibuses/ + + The hubs directory contains a number of hub objects, each representing + a UV Hub visible to the BIOS. Each hub object's name is appended by a + unique ordinal value (ex. /sys/firmware/sgi_uv/hubs/hub_5) + + Each hub object directory contains a number of read-only attributes: + + cnode + location + name + nasid + shared + this_partition + + The cnode entry contains the cnode number of the corresponding hub. + If a cnode value is not applicable, the value returned will be -1. + + The location entry contains the location string of the corresponding hub. + This value is used to physically identify a hub within a system. + + The name entry contains the name of the corresponding hub. This name can + be two variants: + "UVHub x.x" = A 'node' ASIC, connecting a CPU to the interconnect + fabric. The 'x.x' value represents the ASIC revision. + (ex. 'UVHub 5.0') + "NLxRouter" = A 'router ASIC, only connecting other ASICs to + the interconnect fabric. The 'x' value representing + the fabric technology version. (ex. 'NL8Router') + + The nasid entry contains the nasid number of the corresponding hub. + If a nasid value is not applicable, the value returned will be -1. + + The shared entry contains a boolean value describing whether the + corresponding hub is shared between system partitions. + + The this_partition entry contains a boolean value describing whether + the corresponding hub is local to the current partition. + + Each hub object directory also contains a number of port objects, + each representing a fabric port on the corresponding hub. + A port object's name is appended by a unique ord
[PATCH v2 5/5] x86/platform/uv: Update MAINTAINERS for uv_sysfs driver
Add an entry and email address for the new uv_sysfs driver and its maintainer. Signed-off-by: Justin Ernst Acked-by: Steve Wahl --- MAINTAINERS | 6 ++ 1 file changed, 6 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index b43b59542d15..f693d2d97203 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -18361,6 +18361,12 @@ F: include/uapi/linux/uuid.h F: lib/test_uuid.c F: lib/uuid.c +UV SYSFS DRIVER +M: Justin Ernst +L: platform-driver-...@vger.kernel.org +S: Maintained +F: drivers/platform/x86/uv_sysfs.c + UVESAFB DRIVER M: Michal Januszewski L: linux-fb...@vger.kernel.org -- 2.26.2
[PATCH v2 1/5] x86/platform/uv: Remove existing /sys/firmware/sgi_uv/ interface
Remove existing interface at /sys/firmware/sgi_uv/, created by
arch/x86/platform/uv/uv_sysfs.c
This interface includes:
/sys/firmware/sgi_uv/coherence_id
/sys/firmware/sgi_uv/partition_id
Both coherence_id and partition_id will be re-introduced via a
new uv_sysfs driver.
Signed-off-by: Justin Ernst
Reviewed-by: Steve Wahl
---
arch/x86/platform/uv/Makefile | 2 +-
arch/x86/platform/uv/uv_sysfs.c | 63 -
2 files changed, 1 insertion(+), 64 deletions(-)
delete mode 100644 arch/x86/platform/uv/uv_sysfs.c
diff --git a/arch/x86/platform/uv/Makefile b/arch/x86/platform/uv/Makefile
index 224ff0504890..1441dda8edf7 100644
--- a/arch/x86/platform/uv/Makefile
+++ b/arch/x86/platform/uv/Makefile
@@ -1,2 +1,2 @@
# SPDX-License-Identifier: GPL-2.0-only
-obj-$(CONFIG_X86_UV) += bios_uv.o uv_irq.o uv_sysfs.o uv_time.o
uv_nmi.o
+obj-$(CONFIG_X86_UV) += bios_uv.o uv_irq.o uv_time.o uv_nmi.o
diff --git a/arch/x86/platform/uv/uv_sysfs.c b/arch/x86/platform/uv/uv_sysfs.c
deleted file mode 100644
index 266773e2fb37..
--- a/arch/x86/platform/uv/uv_sysfs.c
+++ /dev/null
@@ -1,63 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-or-later
-/*
- * This file supports the /sys/firmware/sgi_uv interfaces for SGI UV.
- *
- * Copyright (c) 2008 Silicon Graphics, Inc. All Rights Reserved.
- * Copyright (c) Russ Anderson
- */
-
-#include
-#include
-#include
-
-struct kobject *sgi_uv_kobj;
-
-static ssize_t partition_id_show(struct kobject *kobj,
- struct kobj_attribute *attr, char *buf)
-{
- return snprintf(buf, PAGE_SIZE, "%ld\n", sn_partition_id);
-}
-
-static ssize_t coherence_id_show(struct kobject *kobj,
- struct kobj_attribute *attr, char *buf)
-{
- return snprintf(buf, PAGE_SIZE, "%ld\n", sn_coherency_id);
-}
-
-static struct kobj_attribute partition_id_attr =
- __ATTR(partition_id, S_IRUGO, partition_id_show, NULL);
-
-static struct kobj_attribute coherence_id_attr =
- __ATTR(coherence_id, S_IRUGO, coherence_id_show, NULL);
-
-
-static int __init sgi_uv_sysfs_init(void)
-{
- unsigned long ret;
-
- if (!is_uv_system())
- return -ENODEV;
-
- if (!sgi_uv_kobj)
- sgi_uv_kobj = kobject_create_and_add("sgi_uv", firmware_kobj);
- if (!sgi_uv_kobj) {
- printk(KERN_WARNING "kobject_create_and_add sgi_uv failed\n");
- return -EINVAL;
- }
-
- ret = sysfs_create_file(sgi_uv_kobj, _id_attr.attr);
- if (ret) {
- printk(KERN_WARNING "sysfs_create_file partition_id failed\n");
- return ret;
- }
-
- ret = sysfs_create_file(sgi_uv_kobj, _id_attr.attr);
- if (ret) {
- printk(KERN_WARNING "sysfs_create_file coherence_id failed\n");
- return ret;
- }
-
- return 0;
-}
-
-device_initcall(sgi_uv_sysfs_init);
--
2.26.2
[PATCH v2 3/5] x86/platform/uv: Add new uv_sysfs platform driver
Add the uv_sysfs driver to construct a read-only sysfs interface at
/sys/firmware/sgi_uv/ to expose information gathered from UV BIOS.
This information includes:
UV Hub descriptions, including physical location
Cabling layout between hubs on the fabric
PCI topology, including physical location of PCI cards
Together, the information provides a robust physical description of a
UV system, useful for correlating to performance data or performing
remote support.
Signed-off-by: Justin Ernst
Reviewed-by: Steve Wahl
---
drivers/platform/x86/Kconfig| 11 +
drivers/platform/x86/Makefile | 3 +
drivers/platform/x86/uv_sysfs.c | 862
3 files changed, 876 insertions(+)
create mode 100644 drivers/platform/x86/uv_sysfs.c
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 0d91d136bc3b..ba34153571b8 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -78,6 +78,17 @@ config HUAWEI_WMI
To compile this driver as a module, choose M here: the module
will be called huawei-wmi.
+config UV_SYSFS
+ tristate "Sysfs structure for UV systems"
+ depends on X86_UV
+ depends on SYSFS
+ help
+ This driver supports a sysfs tree describing information about
+ UV systems at /sys/firmware/sgi_uv/.
+
+ To compile this driver as a module, choose M here: the module will
+ be called uv_sysfs.
+
config INTEL_WMI_SBL_FW_UPDATE
tristate "Intel WMI Slim Bootloader firmware update signaling driver"
depends on ACPI_WMI
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 5f823f7eff45..a34875d833dd 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -62,6 +62,9 @@ obj-$(CONFIG_HP_WIRELESS) += hp-wireless.o
obj-$(CONFIG_HP_WMI) += hp-wmi.o
obj-$(CONFIG_TC1100_WMI) += tc1100-wmi.o
+# Hewlett Packard Enterprise
+obj-$(CONFIG_UV_SYSFS) += uv_sysfs.o
+
# IBM Thinkpad and Lenovo
obj-$(CONFIG_IBM_RTL) += ibm_rtl.o
obj-$(CONFIG_IDEAPAD_LAPTOP) += ideapad-laptop.o
diff --git a/drivers/platform/x86/uv_sysfs.c b/drivers/platform/x86/uv_sysfs.c
new file mode 100644
index ..02206401f0ed
--- /dev/null
+++ b/drivers/platform/x86/uv_sysfs.c
@@ -0,0 +1,862 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * This file supports the /sys/firmware/sgi_uv topology tree on HPE UV.
+ *
+ * Copyright (c) 2020 Hewlett Packard Enterprise. All Rights Reserved.
+ * Copyright (c) Justin Ernst
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define INVALID_CNODE -1
+
+struct kobject *sgi_uv_kobj;
+struct kset *uv_pcibus_kset;
+struct kset *uv_hubs_kset;
+static struct uv_bios_hub_info *hub_buf;
+static struct uv_bios_port_info **port_buf;
+static struct uv_hub **uv_hubs;
+static struct uv_pci_top_obj **uv_pci_objs;
+static int num_pci_lines;
+static int num_cnodes;
+static int *prev_obj_to_cnode;
+static int uv_bios_obj_cnt;
+static signed short uv_master_nasid = -1;
+static void *uv_biosheap;
+
+static const char *uv_type_string(void)
+{
+ if (is_uv5_hub())
+ return "9.0";
+ else if (is_uv4a_hub())
+ return "7.1";
+ else if (is_uv4_hub())
+ return "7.0";
+ else if (is_uv3_hub())
+ return "5.0";
+ else if (is_uv2_hub())
+ return "3.0";
+ else
+ return "unknown";
+}
+
+static int ordinal_to_nasid(int ordinal)
+{
+ if (ordinal < num_cnodes && ordinal >= 0)
+ return UV_PNODE_TO_NASID(uv_blade_to_pnode(ordinal));
+ else
+ return -1;
+}
+
+static union geoid_u cnode_to_geoid(int cnode)
+{
+ union geoid_u geoid;
+
+ uv_bios_get_geoinfo(ordinal_to_nasid(cnode), (u64)sizeof(union
geoid_u), (u64 *));
+ return geoid;
+}
+
+static int location_to_bpos(char *location, int *rack, int *slot, int *blade)
+{
+ char type, r, b, h;
+ int idb, idh;
+
+ if (sscanf(location, "%c%03d%c%02d%c%2d%c%d",
+, rack, , slot, , , , ) != 8)
+ return -1;
+ *blade = idb * 2 + idh;
+
+ return 0;
+}
+
+static int cache_obj_to_cnode(struct uv_bios_hub_info *obj)
+{
+ int cnode;
+ union geoid_u geoid;
+ int obj_rack, obj_slot, obj_blade;
+ int rack, slot, blade;
+
+ if (!obj->f.fields.this_part && !obj->f.fields.is_shared)
+ return 0;
+
+ if (location_to_bpos(obj->location, _rack, _slot, _blade))
+ return -1;
+
+ for (cnode = 0; cnode < num_cnodes; cnode++) {
+ geoid = cnode_to_geoid(cnode);
+ rack = geo_rack(geoid);
+ slot = geo_slot(geoi
[PATCH v2 2/5] x86/platform/uv: Add and export uv_bios_* functions
Add additional uv_bios_call variant functions to expose information
needed by the new uv_sysfs driver. This includes the addition of several
new data types defined by UV BIOS and used in the new functions.
Signed-off-by: Justin Ernst
Reviewed-by: Steve Wahl
---
arch/x86/include/asm/uv/bios.h | 49 +++
arch/x86/include/asm/uv/uv_geo.h | 103 +++
arch/x86/platform/uv/bios_uv.c | 54
3 files changed, 206 insertions(+)
create mode 100644 arch/x86/include/asm/uv/uv_geo.h
diff --git a/arch/x86/include/asm/uv/bios.h b/arch/x86/include/asm/uv/bios.h
index 08b3d810dfba..01ba080887b3 100644
--- a/arch/x86/include/asm/uv/bios.h
+++ b/arch/x86/include/asm/uv/bios.h
@@ -28,6 +28,20 @@ enum uv_bios_cmd {
UV_BIOS_SET_LEGACY_VGA_TARGET
};
+#define UV_BIOS_EXTRA 0x1
+#define UV_BIOS_GET_PCI_TOPOLOGY 0x10001
+#define UV_BIOS_GET_GEOINFO0x10003
+
+#define UV_BIOS_EXTRA_OP_MEM_COPYIN0x1000
+#define UV_BIOS_EXTRA_OP_MEM_COPYOUT 0x2000
+#define UV_BIOS_EXTRA_OP_MASK 0x0fff
+#define UV_BIOS_EXTRA_GET_HEAPSIZE 1
+#define UV_BIOS_EXTRA_INSTALL_HEAP 2
+#define UV_BIOS_EXTRA_MASTER_NASID 3
+#define UV_BIOS_EXTRA_OBJECT_COUNT (10|UV_BIOS_EXTRA_OP_MEM_COPYOUT)
+#define UV_BIOS_EXTRA_ENUM_OBJECTS (12|UV_BIOS_EXTRA_OP_MEM_COPYOUT)
+#define UV_BIOS_EXTRA_ENUM_PORTS (13|UV_BIOS_EXTRA_OP_MEM_COPYOUT)
+
/*
* Status values returned from a BIOS call.
*/
@@ -109,6 +123,32 @@ struct uv_systab {
} entry[1]; /* additional entries follow */
};
extern struct uv_systab *uv_systab;
+
+#define UV_BIOS_MAXSTRING128
+struct uv_bios_hub_info {
+ unsigned int id;
+ union {
+ struct {
+ unsigned long long this_part:1;
+ unsigned long long is_shared:1;
+ unsigned long long is_disabled:1;
+ } fields;
+ struct {
+ unsigned long long flags;
+ unsigned long long reserved;
+ } b;
+ } f;
+ char name[UV_BIOS_MAXSTRING];
+ char location[UV_BIOS_MAXSTRING];
+ unsigned int ports;
+};
+
+struct uv_bios_port_info {
+ unsigned int port;
+ unsigned int conn_id;
+ unsigned int conn_port;
+};
+
/* (... end of definitions from UV BIOS ...) */
enum {
@@ -142,6 +182,15 @@ extern s64 uv_bios_change_memprotect(u64, u64, enum
uv_memprotect);
extern s64 uv_bios_reserved_page_pa(u64, u64 *, u64 *, u64 *);
extern int uv_bios_set_legacy_vga_target(bool decode, int domain, int bus);
+extern s64 uv_bios_get_master_nasid(u64 sz, u64 *nasid);
+extern s64 uv_bios_get_heapsize(u64 nasid, u64 sz, u64 *heap_sz);
+extern s64 uv_bios_install_heap(u64 nasid, u64 sz, u64 *heap);
+extern s64 uv_bios_obj_count(u64 nasid, u64 sz, u64 *objcnt);
+extern s64 uv_bios_enum_objs(u64 nasid, u64 sz, u64 *objbuf);
+extern s64 uv_bios_enum_ports(u64 nasid, u64 obj_id, u64 sz, u64 *portbuf);
+extern s64 uv_bios_get_geoinfo(u64 nasid, u64 sz, u64 *geo);
+extern s64 uv_bios_get_pci_topology(u64 sz, u64 *buf);
+
extern int uv_bios_init(void);
extern unsigned long get_uv_systab_phys(bool msg);
diff --git a/arch/x86/include/asm/uv/uv_geo.h b/arch/x86/include/asm/uv/uv_geo.h
new file mode 100644
index ..f241451035fb
--- /dev/null
+++ b/arch/x86/include/asm/uv/uv_geo.h
@@ -0,0 +1,103 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License. See the file "COPYING" in the main directory of this archive
+ * for more details.
+ *
+ * Copyright (C) 2020 Hewlett Packard Enterprise Development LP. All rights
reserved.
+ */
+
+#ifndef _ASM_UV_GEO_H
+#define _ASM_UV_GEO_H
+
+/* Type declaractions */
+
+/* Size of a geoid_s structure (must be before decl. of geoid_u) */
+#define GEOID_SIZE 8
+
+/* Fields common to all substructures */
+struct geo_common_s {
+ unsigned char type; /* What type of h/w is named by this
geoid_s */
+ unsigned char blade;
+ unsigned char slot; /* slot is IRU */
+ unsigned char upos;
+ unsigned char rack;
+};
+
+/* Additional fields for particular types of hardware */
+struct geo_node_s {
+ struct geo_common_s common; /* No additional fields needed
*/
+};
+
+struct geo_rtr_s {
+ struct geo_common_s common; /* No additional fields needed
*/
+};
+
+struct geo_iocntl_s {
+ struct geo_common_s common; /* No additional fields needed
*/
+};
+
+struct geo_pcicard_s {
+ struct geo_iocntl_s common;
+ char bus; /* Bus/widget number */
+ char slot; /* PCI slot number */
+};
+
+/* Subcomponents of a node */
+struct geo_cpu_s {
+
RE: [PATCH 0/5] Add uv_sysfs platform driver
> Hi, > > On 11/17/20 9:42 PM, Justin Ernst wrote: > > Introduce a new platform driver to gather topology information from UV > > systems > > and expose that information via a sysfs interface at /sys/firmware/sgi_uv/. > > > > Justin Ernst (5): > > x86/platform/uv: Remove existing /sys/firmware/sgi_uv/ interface > > x86/platform/uv: Add and export uv_bios_* functions > > x86/platform/uv: Add new uv_sysfs platform driver > > x86/platform/uv: Update ABI documentation of /sys/firmware/sgi_uv/ > > x86/platform/uv: Update MAINTAINERS for uv_sysfs driver > > So patch 1/1 drops the existing > > /sys/firmware/sgi_uv/coherence_id > /sys/firmware/sgi_uv/partition_id > > sysfs API, then according to patch 4/5 patch 3/5 reintroduces > the /sys/firmware/sgi_uv/partition_id API, but the > /sys/firmware/sgi_uv/coherence_id > file is gone for ever ? > > I'm not sure what userspace bits (may) depend on this but without more info > this looks like a clear violation of the do not break userspace APIs rule. > > So, based on this, I have to nack this series in its current state. > > Now if there is a strong believe there are 0 (not a few, but _zero_) users > out there who rely on the /sys/firmware/sgi_uv/coherence_id file then this > might be ok. But then there needs to be a technical analysis of why this is > ok in the commit message of the patch dropping this sysfs file. > > Also the commit message of patch 1/5 should mention that > /sys/firmware/sgi_uv/partition_id will be re-introduced later through > another driver. Hello Hans, I will resubmit these patches without the API breakage, reintroducing the coherence_id file in the new driver. Thank you for taking the time to look over my patch set. > > Regards, > > Hans
[PATCH 3/5] x86/platform/uv: Add new uv_sysfs platform driver
Add the uv_sysfs driver to construct a read-only sysfs interface at
/sys/firmware/sgi_uv/ to expose information gathered from UV BIOS.
This information includes:
UV Hub descriptions, including physical location
Cabling layout between hubs on the fabric
PCI topology, including physical location of PCI cards
Together, the information provides a robust physical description of a
UV system, useful for correlating to performance data or performing
remote support.
Signed-off-by: Justin Ernst
Reviewed-by: Steve Wahl
---
drivers/platform/x86/Kconfig| 11 +
drivers/platform/x86/Makefile | 3 +
drivers/platform/x86/uv_sysfs.c | 853
3 files changed, 867 insertions(+)
create mode 100644 drivers/platform/x86/uv_sysfs.c
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 0d91d136bc3b..ba34153571b8 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -78,6 +78,17 @@ config HUAWEI_WMI
To compile this driver as a module, choose M here: the module
will be called huawei-wmi.
+config UV_SYSFS
+ tristate "Sysfs structure for UV systems"
+ depends on X86_UV
+ depends on SYSFS
+ help
+ This driver supports a sysfs tree describing information about
+ UV systems at /sys/firmware/sgi_uv/.
+
+ To compile this driver as a module, choose M here: the module will
+ be called uv_sysfs.
+
config INTEL_WMI_SBL_FW_UPDATE
tristate "Intel WMI Slim Bootloader firmware update signaling driver"
depends on ACPI_WMI
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 5f823f7eff45..a34875d833dd 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -62,6 +62,9 @@ obj-$(CONFIG_HP_WIRELESS) += hp-wireless.o
obj-$(CONFIG_HP_WMI) += hp-wmi.o
obj-$(CONFIG_TC1100_WMI) += tc1100-wmi.o
+# Hewlett Packard Enterprise
+obj-$(CONFIG_UV_SYSFS) += uv_sysfs.o
+
# IBM Thinkpad and Lenovo
obj-$(CONFIG_IBM_RTL) += ibm_rtl.o
obj-$(CONFIG_IDEAPAD_LAPTOP) += ideapad-laptop.o
diff --git a/drivers/platform/x86/uv_sysfs.c b/drivers/platform/x86/uv_sysfs.c
new file mode 100644
index ..6de360370f0e
--- /dev/null
+++ b/drivers/platform/x86/uv_sysfs.c
@@ -0,0 +1,853 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * This file supports the /sys/firmware/sgi_uv topology tree on HPE UV.
+ *
+ * Copyright (c) 2020 Hewlett Packard Enterprise. All Rights Reserved.
+ * Copyright (c) Justin Ernst
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define INVALID_CNODE -1
+
+struct kobject *sgi_uv_kobj;
+struct kset *uv_pcibus_kset;
+struct kset *uv_hubs_kset;
+static struct uv_bios_hub_info *hub_buf;
+static struct uv_bios_port_info **port_buf;
+static struct uv_hub **uv_hubs;
+static struct uv_pci_top_obj **uv_pci_objs;
+static int num_pci_lines;
+static int num_cnodes;
+static int *prev_obj_to_cnode;
+static int uv_bios_obj_cnt;
+static signed short uv_master_nasid = -1;
+static void *uv_biosheap;
+
+static const char *uv_type_string(void)
+{
+ if (is_uv5_hub())
+ return "9.0";
+ else if (is_uv4a_hub())
+ return "7.1";
+ else if (is_uv4_hub())
+ return "7.0";
+ else if (is_uv3_hub())
+ return "5.0";
+ else if (is_uv2_hub())
+ return "3.0";
+ else
+ return "unknown";
+}
+
+static int ordinal_to_nasid(int ordinal)
+{
+ if (ordinal < num_cnodes && ordinal >= 0)
+ return UV_PNODE_TO_NASID(uv_blade_to_pnode(ordinal));
+ else
+ return -1;
+}
+
+static union geoid_u cnode_to_geoid(int cnode)
+{
+ union geoid_u geoid;
+
+ uv_bios_get_geoinfo(ordinal_to_nasid(cnode), (u64)sizeof(union
geoid_u), (u64 *));
+ return geoid;
+}
+
+static int location_to_bpos(char *location, int *rack, int *slot, int *blade)
+{
+ char type, r, b, h;
+ int idb, idh;
+
+ if (sscanf(location, "%c%03d%c%02d%c%2d%c%d",
+, rack, , slot, , , , ) != 8)
+ return -1;
+ *blade = idb * 2 + idh;
+
+ return 0;
+}
+
+static int cache_obj_to_cnode(struct uv_bios_hub_info *obj)
+{
+ int cnode;
+ union geoid_u geoid;
+ int obj_rack, obj_slot, obj_blade;
+ int rack, slot, blade;
+
+ if (!obj->f.fields.this_part && !obj->f.fields.is_shared)
+ return 0;
+
+ if (location_to_bpos(obj->location, _rack, _slot, _blade))
+ return -1;
+
+ for (cnode = 0; cnode < num_cnodes; cnode++) {
+ geoid = cnode_to_geoid(cnode);
+ rack = geo_rack(geoid);
+ slot = geo_slot(geoi
[PATCH 2/5] x86/platform/uv: Add and export uv_bios_* functions
Add additional uv_bios_call variant functions to expose information
needed by the new uv_sysfs driver. This includes the addition of several
new data types defined by UV BIOS and used in the new functions.
Signed-off-by: Justin Ernst
Reviewed-by: Steve Wahl
---
arch/x86/include/asm/uv/bios.h | 49 +++
arch/x86/include/asm/uv/uv_geo.h | 103 +++
arch/x86/platform/uv/bios_uv.c | 54
3 files changed, 206 insertions(+)
create mode 100644 arch/x86/include/asm/uv/uv_geo.h
diff --git a/arch/x86/include/asm/uv/bios.h b/arch/x86/include/asm/uv/bios.h
index 08b3d810dfba..01ba080887b3 100644
--- a/arch/x86/include/asm/uv/bios.h
+++ b/arch/x86/include/asm/uv/bios.h
@@ -28,6 +28,20 @@ enum uv_bios_cmd {
UV_BIOS_SET_LEGACY_VGA_TARGET
};
+#define UV_BIOS_EXTRA 0x1
+#define UV_BIOS_GET_PCI_TOPOLOGY 0x10001
+#define UV_BIOS_GET_GEOINFO0x10003
+
+#define UV_BIOS_EXTRA_OP_MEM_COPYIN0x1000
+#define UV_BIOS_EXTRA_OP_MEM_COPYOUT 0x2000
+#define UV_BIOS_EXTRA_OP_MASK 0x0fff
+#define UV_BIOS_EXTRA_GET_HEAPSIZE 1
+#define UV_BIOS_EXTRA_INSTALL_HEAP 2
+#define UV_BIOS_EXTRA_MASTER_NASID 3
+#define UV_BIOS_EXTRA_OBJECT_COUNT (10|UV_BIOS_EXTRA_OP_MEM_COPYOUT)
+#define UV_BIOS_EXTRA_ENUM_OBJECTS (12|UV_BIOS_EXTRA_OP_MEM_COPYOUT)
+#define UV_BIOS_EXTRA_ENUM_PORTS (13|UV_BIOS_EXTRA_OP_MEM_COPYOUT)
+
/*
* Status values returned from a BIOS call.
*/
@@ -109,6 +123,32 @@ struct uv_systab {
} entry[1]; /* additional entries follow */
};
extern struct uv_systab *uv_systab;
+
+#define UV_BIOS_MAXSTRING128
+struct uv_bios_hub_info {
+ unsigned int id;
+ union {
+ struct {
+ unsigned long long this_part:1;
+ unsigned long long is_shared:1;
+ unsigned long long is_disabled:1;
+ } fields;
+ struct {
+ unsigned long long flags;
+ unsigned long long reserved;
+ } b;
+ } f;
+ char name[UV_BIOS_MAXSTRING];
+ char location[UV_BIOS_MAXSTRING];
+ unsigned int ports;
+};
+
+struct uv_bios_port_info {
+ unsigned int port;
+ unsigned int conn_id;
+ unsigned int conn_port;
+};
+
/* (... end of definitions from UV BIOS ...) */
enum {
@@ -142,6 +182,15 @@ extern s64 uv_bios_change_memprotect(u64, u64, enum
uv_memprotect);
extern s64 uv_bios_reserved_page_pa(u64, u64 *, u64 *, u64 *);
extern int uv_bios_set_legacy_vga_target(bool decode, int domain, int bus);
+extern s64 uv_bios_get_master_nasid(u64 sz, u64 *nasid);
+extern s64 uv_bios_get_heapsize(u64 nasid, u64 sz, u64 *heap_sz);
+extern s64 uv_bios_install_heap(u64 nasid, u64 sz, u64 *heap);
+extern s64 uv_bios_obj_count(u64 nasid, u64 sz, u64 *objcnt);
+extern s64 uv_bios_enum_objs(u64 nasid, u64 sz, u64 *objbuf);
+extern s64 uv_bios_enum_ports(u64 nasid, u64 obj_id, u64 sz, u64 *portbuf);
+extern s64 uv_bios_get_geoinfo(u64 nasid, u64 sz, u64 *geo);
+extern s64 uv_bios_get_pci_topology(u64 sz, u64 *buf);
+
extern int uv_bios_init(void);
extern unsigned long get_uv_systab_phys(bool msg);
diff --git a/arch/x86/include/asm/uv/uv_geo.h b/arch/x86/include/asm/uv/uv_geo.h
new file mode 100644
index ..f241451035fb
--- /dev/null
+++ b/arch/x86/include/asm/uv/uv_geo.h
@@ -0,0 +1,103 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License. See the file "COPYING" in the main directory of this archive
+ * for more details.
+ *
+ * Copyright (C) 2020 Hewlett Packard Enterprise Development LP. All rights
reserved.
+ */
+
+#ifndef _ASM_UV_GEO_H
+#define _ASM_UV_GEO_H
+
+/* Type declaractions */
+
+/* Size of a geoid_s structure (must be before decl. of geoid_u) */
+#define GEOID_SIZE 8
+
+/* Fields common to all substructures */
+struct geo_common_s {
+ unsigned char type; /* What type of h/w is named by this
geoid_s */
+ unsigned char blade;
+ unsigned char slot; /* slot is IRU */
+ unsigned char upos;
+ unsigned char rack;
+};
+
+/* Additional fields for particular types of hardware */
+struct geo_node_s {
+ struct geo_common_s common; /* No additional fields needed
*/
+};
+
+struct geo_rtr_s {
+ struct geo_common_s common; /* No additional fields needed
*/
+};
+
+struct geo_iocntl_s {
+ struct geo_common_s common; /* No additional fields needed
*/
+};
+
+struct geo_pcicard_s {
+ struct geo_iocntl_s common;
+ char bus; /* Bus/widget number */
+ char slot; /* PCI slot number */
+};
+
+/* Subcomponents of a node */
+struct geo_cpu_s {
+
[PATCH 5/5] x86/platform/uv: Update MAINTAINERS for uv_sysfs driver
Add an entry and email address for the new uv_sysfs driver and its maintainer. Signed-off-by: Justin Ernst Acked-by: Steve Wahl --- MAINTAINERS | 6 ++ 1 file changed, 6 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index b43b59542d15..f693d2d97203 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -18361,6 +18361,12 @@ F: include/uapi/linux/uuid.h F: lib/test_uuid.c F: lib/uuid.c +UV SYSFS DRIVER +M: Justin Ernst +L: platform-driver-...@vger.kernel.org +S: Maintained +F: drivers/platform/x86/uv_sysfs.c + UVESAFB DRIVER M: Michal Januszewski L: linux-fb...@vger.kernel.org -- 2.26.2
[PATCH 1/5] x86/platform/uv: Remove existing /sys/firmware/sgi_uv/ interface
Remove existing interface at /sys/firmware/sgi_uv/, created by
arch/x86/platform/uv/uv_sysfs.c
This interface includes:
/sys/firmware/sgi_uv/coherence_id
/sys/firmware/sgi_uv/partition_id
Signed-off-by: Justin Ernst
Reviewed-by: Steve Wahl
---
arch/x86/platform/uv/Makefile | 2 +-
arch/x86/platform/uv/uv_sysfs.c | 63 -
2 files changed, 1 insertion(+), 64 deletions(-)
delete mode 100644 arch/x86/platform/uv/uv_sysfs.c
diff --git a/arch/x86/platform/uv/Makefile b/arch/x86/platform/uv/Makefile
index 224ff0504890..1441dda8edf7 100644
--- a/arch/x86/platform/uv/Makefile
+++ b/arch/x86/platform/uv/Makefile
@@ -1,2 +1,2 @@
# SPDX-License-Identifier: GPL-2.0-only
-obj-$(CONFIG_X86_UV) += bios_uv.o uv_irq.o uv_sysfs.o uv_time.o
uv_nmi.o
+obj-$(CONFIG_X86_UV) += bios_uv.o uv_irq.o uv_time.o uv_nmi.o
diff --git a/arch/x86/platform/uv/uv_sysfs.c b/arch/x86/platform/uv/uv_sysfs.c
deleted file mode 100644
index 266773e2fb37..
--- a/arch/x86/platform/uv/uv_sysfs.c
+++ /dev/null
@@ -1,63 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-or-later
-/*
- * This file supports the /sys/firmware/sgi_uv interfaces for SGI UV.
- *
- * Copyright (c) 2008 Silicon Graphics, Inc. All Rights Reserved.
- * Copyright (c) Russ Anderson
- */
-
-#include
-#include
-#include
-
-struct kobject *sgi_uv_kobj;
-
-static ssize_t partition_id_show(struct kobject *kobj,
- struct kobj_attribute *attr, char *buf)
-{
- return snprintf(buf, PAGE_SIZE, "%ld\n", sn_partition_id);
-}
-
-static ssize_t coherence_id_show(struct kobject *kobj,
- struct kobj_attribute *attr, char *buf)
-{
- return snprintf(buf, PAGE_SIZE, "%ld\n", sn_coherency_id);
-}
-
-static struct kobj_attribute partition_id_attr =
- __ATTR(partition_id, S_IRUGO, partition_id_show, NULL);
-
-static struct kobj_attribute coherence_id_attr =
- __ATTR(coherence_id, S_IRUGO, coherence_id_show, NULL);
-
-
-static int __init sgi_uv_sysfs_init(void)
-{
- unsigned long ret;
-
- if (!is_uv_system())
- return -ENODEV;
-
- if (!sgi_uv_kobj)
- sgi_uv_kobj = kobject_create_and_add("sgi_uv", firmware_kobj);
- if (!sgi_uv_kobj) {
- printk(KERN_WARNING "kobject_create_and_add sgi_uv failed\n");
- return -EINVAL;
- }
-
- ret = sysfs_create_file(sgi_uv_kobj, _id_attr.attr);
- if (ret) {
- printk(KERN_WARNING "sysfs_create_file partition_id failed\n");
- return ret;
- }
-
- ret = sysfs_create_file(sgi_uv_kobj, _id_attr.attr);
- if (ret) {
- printk(KERN_WARNING "sysfs_create_file coherence_id failed\n");
- return ret;
- }
-
- return 0;
-}
-
-device_initcall(sgi_uv_sysfs_init);
--
2.26.2
[PATCH 4/5] x86/platform/uv: Update ABI documentation of /sys/firmware/sgi_uv/
Update the ABI documentation to describe the sysfs interface provided by the new uv_sysfs platform driver. Signed-off-by: Justin Ernst Reviewed-by: Steve Wahl --- .../ABI/testing/sysfs-firmware-sgi_uv | 137 -- 1 file changed, 122 insertions(+), 15 deletions(-) diff --git a/Documentation/ABI/testing/sysfs-firmware-sgi_uv b/Documentation/ABI/testing/sysfs-firmware-sgi_uv index 66800baab096..ca5cdccdb1a8 100644 --- a/Documentation/ABI/testing/sysfs-firmware-sgi_uv +++ b/Documentation/ABI/testing/sysfs-firmware-sgi_uv @@ -1,27 +1,134 @@ What: /sys/firmware/sgi_uv/ -Date: August 2008 -Contact: Russ Anderson +Date: September 2020 +Contact: Justin Ernst Description: The /sys/firmware/sgi_uv directory contains information - about the SGI UV platform. + about the UV platform. - Under that directory are a number of files:: + Under that directory are a number of read-only attributes: partition_id - coherence_id + uv_type The partition_id entry contains the partition id. - SGI UV systems can be partitioned into multiple physical + UV systems can be partitioned into multiple physical machines, which each partition running a unique copy - of the operating system. Each partition will have a unique - partition id. To display the partition id, use the command:: + of the operating system. Each partition will have a unique + partition id. - cat /sys/firmware/sgi_uv/partition_id + The uv_type entry contains the hub revision number. + This value can be used to identify the UV system version: + "3.0" = UV2 + "5.0" = UV3 + "7.0" = UV4 + "7.1" = UV4a + "9.0" = UV5 - The coherence_id entry contains the coherence id. - A partitioned SGI UV system can have one or more coherence - domain. The coherence id indicates which coherence domain - this partition is in. To display the coherence id, use the - command:: + The /sys/firmware/sgi_uv directory also contains two directories: - cat /sys/firmware/sgi_uv/coherence_id + hubs/ + pcibuses/ + + The hubs directory contains a number of hub objects, each representing + a UV Hub visible to the BIOS. Each hub object's name is appended by a + unique ordinal value (ex. /sys/firmware/sgi_uv/hubs/hub_5) + + Each hub object directory contains a number of read-only attributes: + + cnode + location + name + nasid + shared + this_partition + + The cnode entry contains the cnode number of the corresponding hub. + If a cnode value is not applicable, the value returned will be -1. + + The location entry contains the location string of the corresponding hub. + This value is used to physically identify a hub within a system. + + The name entry contains the name of the corresponding hub. This name can + be two variants: + "UVHub x.x" = A 'node' ASIC, connecting a CPU to the interconnect + fabric. The 'x.x' value represents the ASIC revision. + (ex. 'UVHub 5.0') + "NLxRouter" = A 'router ASIC, only connecting other ASICs to + the interconnect fabric. The 'x' value representing + the fabric technology version. (ex. 'NL8Router') + + The nasid entry contains the nasid number of the corresponding hub. + If a nasid value is not applicable, the value returned will be -1. + + The shared entry contains a boolean value describing whether the + corresponding hub is shared between system partitions. + + The this_partition entry contains a boolean value describing whether + the corresponding hub is local to the current partition. + + Each hub object directory also contains a number of port objects, + each representing a fabric port on the corresponding hub. + A port object's name is appended by a unique ordinal value + (ex. /sys/firmware/sgi_uv/hubs/hub_5/port_3) + + Each port object directory contains
[PATCH 0/5] Add uv_sysfs platform driver
Introduce a new platform driver to gather topology information from UV systems and expose that information via a sysfs interface at /sys/firmware/sgi_uv/. Justin Ernst (5): x86/platform/uv: Remove existing /sys/firmware/sgi_uv/ interface x86/platform/uv: Add and export uv_bios_* functions x86/platform/uv: Add new uv_sysfs platform driver x86/platform/uv: Update ABI documentation of /sys/firmware/sgi_uv/ x86/platform/uv: Update MAINTAINERS for uv_sysfs driver .../ABI/testing/sysfs-firmware-sgi_uv | 137 ++- MAINTAINERS | 6 + arch/x86/include/asm/uv/bios.h| 49 + arch/x86/include/asm/uv/uv_geo.h | 103 +++ arch/x86/platform/uv/Makefile | 2 +- arch/x86/platform/uv/bios_uv.c| 54 ++ arch/x86/platform/uv/uv_sysfs.c | 63 -- drivers/platform/x86/Kconfig | 11 + drivers/platform/x86/Makefile | 3 + drivers/platform/x86/uv_sysfs.c | 853 ++ 10 files changed, 1202 insertions(+), 79 deletions(-) create mode 100644 arch/x86/include/asm/uv/uv_geo.h delete mode 100644 arch/x86/platform/uv/uv_sysfs.c create mode 100644 drivers/platform/x86/uv_sysfs.c base-commit: 4ef8451b332662d004df269d4cdeb7d9f31419b5 -- 2.26.2
5.9.3: "md0:" is showing in dmesg/printk but with no other information is provided
Kernel: 5.9.3 Arch: x86_64 These are showing up in dmesg every so often and they are not associated with any type of message/alert or user associated action. What is causing this & why are there no details associated with this message? [Wed Nov 4 17:05:56 2020] md0: [Thu Nov 5 08:23:32 2020] md0: [Thu Nov 5 13:12:00 2020] md0: [Sat Nov 7 06:53:59 2020] md0: [Sat Nov 7 06:54:07 2020] md0: [Tue Nov 10 08:09:27 2020] md0: [Wed Nov 11 12:43:06 2020] md0: Regards, Justin.
Re: [PATCH 5.8 35/99] tools/libbpf: Avoid counting local symbols in ABI check
On Wed, Sep 30, 2020 at 12:02 AM Tony Ambardar wrote:
>
> [adding Michael Ellerman, linux-ppc maintainer]
>
> Hello Justin,
>
> On Tue, 29 Sep 2020 at 14:54, Justin Forbes wrote:
> >
> > On Tue, Sep 29, 2020 at 6:53 AM Greg Kroah-Hartman
> > wrote:
> > >
> > > From: Tony Ambardar
> > >
> > > [ Upstream commit 746f534a4809e07f427f7d13d10f3a6a9641e5c3 ]
> > >
> > > Encountered the following failure building libbpf from kernel 5.8.5
> > > sources
> > > with GCC 8.4.0 and binutils 2.34: (long paths shortened)
> > >
> > > Warning: Num of global symbols in sharedobjs/libbpf-in.o (234) does NOT
> > > match with num of versioned symbols in libbpf.so (236). Please make sure
> > > all LIBBPF_API symbols are versioned in libbpf.map.
> > > # --- libbpf_global_syms.tmp2020-09-02 07:30:58.920084380 +
> > > # +++ libbpf_versioned_syms.tmp 2020-09-02 07:30:58.924084388 +
> > > @@ -1,3 +1,5 @@
> > > +_fini
> > > +_init
> > >bpf_btf_get_fd_by_id
> > >bpf_btf_get_next_id
> > >bpf_create_map
> > > make[4]: *** [Makefile:210: check_abi] Error 1
> > >
> > > Investigation shows _fini and _init are actually local symbols counted
> > > amongst global ones:
> > >
> > > $ readelf --dyn-syms --wide libbpf.so|head -10
> > >
> > > Symbol table '.dynsym' contains 343 entries:
> > > Num:Value Size TypeBind Vis Ndx Name
> > >0: 0 NOTYPE LOCAL DEFAULT UND
> > >1: 4098 0 SECTION LOCAL DEFAULT 11
> > >2: 4098 8 FUNCLOCAL DEFAULT 11 _init@@LIBBPF_0.0.1
> > >3: 00023040 8 FUNCLOCAL DEFAULT 14 _fini@@LIBBPF_0.0.1
> > >4: 0 OBJECT GLOBAL DEFAULT ABS LIBBPF_0.0.4
> > >5: 0 OBJECT GLOBAL DEFAULT ABS LIBBPF_0.0.1
> > >6: ffa4 8 FUNCGLOBAL DEFAULT 12
> > > bpf_object__find_map_by_offset@@LIBBPF_0.0.1
> > >
> > > A previous commit filtered global symbols in sharedobjs/libbpf-in.o. Do
> > > the
> > > same with the libbpf.so DSO for consistent comparison.
> > >
> > > Fixes: 306b267cb3c4 ("libbpf: Verify versioned symbols")
> > > Signed-off-by: Tony Ambardar
> > > Signed-off-by: Alexei Starovoitov
> > > Acked-by: Andrii Nakryiko
> > > Link:
> > > https://lore.kernel.org/bpf/20200905214831.1565465-1-tony.ambar...@gmail.com
> > > Signed-off-by: Sasha Levin
> >
> > This seems to work everywhere else, but breaks PPC64LE.
> >
>
> I also ran into a PPC build error while working on some bpf problems,
> but it seemed
> like a pre-existing PPC issue. I did submit an upstream fix, which is
> marked for stable
> and being reviewed by Michael. See here for discussion and the patch:
> https://lkml.org/lkml/2020/9/17/668.
>
> Is that the same problem you encountered? Does that patch address your issue?
It is not, the issue I see is:
Warning: Num of global symbols in sharedobjs/libbpf-in.o (259) does
NOT match with num of versioned symbols in libbpf.so (50). Please make
sure all LIBBPF_API symbols are versioned in libbpf.map.
I only see it on ppc64le with this patch, all other arch that Fedora
builds are fine (x86_64, i686, aarch64, armv7, s390). If I revert
this patch, all builds succeed. We are using gcc 10.2.1 though.
Justin
>
> Thanks,
> Tony
>
> > Justin
> >
> > > ---
> > > tools/lib/bpf/Makefile |2 ++
> > > 1 file changed, 2 insertions(+)
> > >
> > > --- a/tools/lib/bpf/Makefile
> > > +++ b/tools/lib/bpf/Makefile
> > > @@ -152,6 +152,7 @@ GLOBAL_SYM_COUNT = $(shell readelf -s --
> > >awk '/GLOBAL/ && /DEFAULT/ && !/UND/ {print
> > > $$NF}' | \
> > >sort -u | wc -l)
> > > VERSIONED_SYM_COUNT = $(shell readelf --dyn-syms --wide
> > > $(OUTPUT)libbpf.so | \
> > > + awk '/GLOBAL/ && /DEFAULT/ && !/UND/ {print
> > > $$NF}' | \
> > > grep -Eo '[^ ]+@LIBBPF_' | cut -d@ -f1 |
> > > sort -u | wc -l)
> > >
> > > CMD_TARGETS = $(LIB_TARGET) $(PC_FILE)
> > > @@ -219,6 +220,7 @@ check_abi: $(OUTPUT)libbpf.so
> > > awk '/GLOBAL/ && /DEFAULT/ && !/UND/ {print $$NF}'| \
> > > sort -u > $(OUTPUT)libbpf_global_syms.tmp; \
> > > readelf --dyn-syms --wide $(OUTPUT)libbpf.so | \
> > > + awk '/GLOBAL/ && /DEFAULT/ && !/UND/ {print $$NF}'| \
> > > grep -Eo '[^ ]+@LIBBPF_' | cut -d@ -f1 | \
> > > sort -u > $(OUTPUT)libbpf_versioned_syms.tmp;\
> > > diff -u $(OUTPUT)libbpf_global_syms.tmp \
> > >
> > >
Re: [PATCH 5.8 35/99] tools/libbpf: Avoid counting local symbols in ABI check
On Tue, Sep 29, 2020 at 6:53 AM Greg Kroah-Hartman
wrote:
>
> From: Tony Ambardar
>
> [ Upstream commit 746f534a4809e07f427f7d13d10f3a6a9641e5c3 ]
>
> Encountered the following failure building libbpf from kernel 5.8.5 sources
> with GCC 8.4.0 and binutils 2.34: (long paths shortened)
>
> Warning: Num of global symbols in sharedobjs/libbpf-in.o (234) does NOT
> match with num of versioned symbols in libbpf.so (236). Please make sure
> all LIBBPF_API symbols are versioned in libbpf.map.
> # --- libbpf_global_syms.tmp2020-09-02 07:30:58.920084380 +
> # +++ libbpf_versioned_syms.tmp 2020-09-02 07:30:58.924084388 +
> @@ -1,3 +1,5 @@
> +_fini
> +_init
>bpf_btf_get_fd_by_id
>bpf_btf_get_next_id
>bpf_create_map
> make[4]: *** [Makefile:210: check_abi] Error 1
>
> Investigation shows _fini and _init are actually local symbols counted
> amongst global ones:
>
> $ readelf --dyn-syms --wide libbpf.so|head -10
>
> Symbol table '.dynsym' contains 343 entries:
> Num:Value Size TypeBind Vis Ndx Name
>0: 0 NOTYPE LOCAL DEFAULT UND
>1: 4098 0 SECTION LOCAL DEFAULT 11
>2: 4098 8 FUNCLOCAL DEFAULT 11 _init@@LIBBPF_0.0.1
>3: 00023040 8 FUNCLOCAL DEFAULT 14 _fini@@LIBBPF_0.0.1
>4: 0 OBJECT GLOBAL DEFAULT ABS LIBBPF_0.0.4
>5: 0 OBJECT GLOBAL DEFAULT ABS LIBBPF_0.0.1
>6: ffa4 8 FUNCGLOBAL DEFAULT 12
> bpf_object__find_map_by_offset@@LIBBPF_0.0.1
>
> A previous commit filtered global symbols in sharedobjs/libbpf-in.o. Do the
> same with the libbpf.so DSO for consistent comparison.
>
> Fixes: 306b267cb3c4 ("libbpf: Verify versioned symbols")
> Signed-off-by: Tony Ambardar
> Signed-off-by: Alexei Starovoitov
> Acked-by: Andrii Nakryiko
> Link:
> https://lore.kernel.org/bpf/20200905214831.1565465-1-tony.ambar...@gmail.com
> Signed-off-by: Sasha Levin
This seems to work everywhere else, but breaks PPC64LE.
Justin
> ---
> tools/lib/bpf/Makefile |2 ++
> 1 file changed, 2 insertions(+)
>
> --- a/tools/lib/bpf/Makefile
> +++ b/tools/lib/bpf/Makefile
> @@ -152,6 +152,7 @@ GLOBAL_SYM_COUNT = $(shell readelf -s --
>awk '/GLOBAL/ && /DEFAULT/ && !/UND/ {print $$NF}'
> | \
>sort -u | wc -l)
> VERSIONED_SYM_COUNT = $(shell readelf --dyn-syms --wide $(OUTPUT)libbpf.so |
> \
> + awk '/GLOBAL/ && /DEFAULT/ && !/UND/ {print
> $$NF}' | \
> grep -Eo '[^ ]+@LIBBPF_' | cut -d@ -f1 | sort
> -u | wc -l)
>
> CMD_TARGETS = $(LIB_TARGET) $(PC_FILE)
> @@ -219,6 +220,7 @@ check_abi: $(OUTPUT)libbpf.so
> awk '/GLOBAL/ && /DEFAULT/ && !/UND/ {print $$NF}'| \
> sort -u > $(OUTPUT)libbpf_global_syms.tmp; \
> readelf --dyn-syms --wide $(OUTPUT)libbpf.so | \
> + awk '/GLOBAL/ && /DEFAULT/ && !/UND/ {print $$NF}'| \
> grep -Eo '[^ ]+@LIBBPF_' | cut -d@ -f1 | \
> sort -u > $(OUTPUT)libbpf_versioned_syms.tmp;\
> diff -u $(OUTPUT)libbpf_global_syms.tmp \
>
>
Re: crypto: aegis128: error: incompatible types when initializing type 'unsigned char' using type 'uint8x16_t'
On Mon, Jul 27, 2020 at 8:05 AM Andrea Righi wrote: > > I'm experiencing this build error on arm64 after updating to gcc 10: > > crypto/aegis128-neon-inner.c: In function 'crypto_aegis128_init_neon': > crypto/aegis128-neon-inner.c:151:3: error: incompatible types when > initializing type 'unsigned char' using type 'uint8x16_t' > 151 | k ^ vld1q_u8(const0), > | ^ > crypto/aegis128-neon-inner.c:152:3: error: incompatible types when > initializing type 'unsigned char' using type 'uint8x16_t' > 152 | k ^ vld1q_u8(const1), > | ^ > > Anybody knows if there's a fix for this already? Otherwise I'll take a look > at it. I hit it and have been working with Jakub on the issue. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96377 Justin
