iommu non-strict mode for arm64
Hi all, We've gotten significant slowdowns on arm64 with 4k pages compared to 64k. The slowdowns can be alleviated by setting iommu.strict=0 or iommu.passthrough=1. Is there a reason x86 defaults to lazy iommu, while arm64 does not? Are there security implications which are specific to arm64? -- Josh ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 10/11] tools/objtool: Check for use of the ENQCMD instruction in the kernel
On Fri, Dec 17, 2021 at 10:01:35PM +, Fenghua Yu wrote: > The ENQCMD implicitly accesses the PASID_MSR to fill in the pasid field > of the descriptor being submitted to an accelerator. But there is no > precise (and stable across kernel changes) point at which the PASID_MSR > is updated from the value for one task to the next. > > Kernel code that uses accelerators must always use the ENQCMDS instruction > which does not access the PASID_MSR. > > Check for use of the ENQCMD instruction in the kernel and warn on its > usage. > > Signed-off-by: Fenghua Yu > Reviewed-by: Tony Luck > --- > v2: > - Simplify handling ENQCMD (PeterZ and Josh) Acked-by: Josh Poimboeuf -- Josh ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 7/8] tools/objtool: Check for use of the ENQCMD instruction in the kernel
On Thu, Sep 23, 2021 at 03:26:14PM +, Fenghua Yu wrote:
> > > + } else if (op2 == 0x38 && op3 == 0xf8) {
> > > + if (insn.prefixes.nbytes == 1 &&
> > > + insn.prefixes.bytes[0] == 0xf2) {
> > > + /* ENQCMD cannot be used in the kernel. */
> > > + WARN("ENQCMD instruction at %s:%lx", sec->name,
> > > + offset);
> > > +
> > > + return -1;
> > > + }
> >
> > The only concern here is if we want it to be fatal or not. But otherwise
> > this seems to be all that's required.
>
> objtool doesn't fail kernel build on this fatal warning.
>
> Returning -1 here stops checking the rest of the file and won't report any
> further warnings unless this ENQCMD warning is fixed. Not returning -1
> continues checking the rest of the file and may report more warnings.
> Seems that's the only difference b/w them.
>
> Should I keep this "return -1" or not? Please advice.
I'd say remove the "return -1" since it's not a fatal-type analysis
error and there's nothing to prevent objtool from analyzing the rest of
the file.
--
Josh
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [patch V3 00/29] stacktrace: Consolidate stack trace usage
On Thu, Apr 25, 2019 at 11:44:53AM +0200, Thomas Gleixner wrote: > This is an update to V2 which can be found here: > > https://lkml.kernel.org/r/20190418084119.056416...@linutronix.de > > Changes vs. V2: > > - Fixed the kernel-doc issue pointed out by Mike > > - Removed the '-1' oddity from the tracer > > - Restricted the tracer nesting to 4 > > - Restored the lockdep magic to prevent redundant stack traces > > - Addressed the small nitpicks here and there > > - Picked up Acked/Reviewed tags Other than the 2 minor nits: Reviewed-by: Josh Poimboeuf -- Josh ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [patch V3 21/29] tracing: Use percpu stack trace buffer more intelligently
On Thu, Apr 25, 2019 at 11:45:14AM +0200, Thomas Gleixner wrote:
> @@ -2788,29 +2798,32 @@ static void __ftrace_trace_stack(struct
>*/
> preempt_disable_notrace();
>
> - use_stack = __this_cpu_inc_return(ftrace_stack_reserve);
> + stackidx = __this_cpu_inc_return(ftrace_stack_reserve);
> +
> + /* This should never happen. If it does, yell once and skip */
> + if (WARN_ON_ONCE(stackidx >= FTRACE_KSTACK_NESTING))
> + goto out;
> +
> /*
> - * We don't need any atomic variables, just a barrier.
> - * If an interrupt comes in, we don't care, because it would
> - * have exited and put the counter back to what we want.
> - * We just need a barrier to keep gcc from moving things
> - * around.
> + * The above __this_cpu_inc_return() is 'atomic' cpu local. An
> + * interrupt will either see the value pre increment or post
> + * increment. If the interrupt happens pre increment it will have
> + * restored the counter when it returns. We just need a barrier to
> + * keep gcc from moving things around.
>*/
> barrier();
> - if (use_stack == 1) {
> - trace.entries = this_cpu_ptr(ftrace_stack.calls);
> - trace.max_entries = FTRACE_STACK_MAX_ENTRIES;
> -
> - if (regs)
> - save_stack_trace_regs(regs, );
> - else
> - save_stack_trace();
> -
> - if (trace.nr_entries > size)
> - size = trace.nr_entries;
> - } else
> - /* From now on, use_stack is a boolean */
> - use_stack = 0;
> +
> + fstack = this_cpu_ptr(ftrace_stacks.stacks) + (stackidx - 1);
nit: it would be slightly less surprising if stackidx were 0-based:
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index d3f6ec7eb729..4fc93004feab 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -2798,10 +2798,10 @@ static void __ftrace_trace_stack(struct ring_buffer
*buffer,
*/
preempt_disable_notrace();
- stackidx = __this_cpu_inc_return(ftrace_stack_reserve);
+ stackidx = __this_cpu_inc_return(ftrace_stack_reserve) - 1;
/* This should never happen. If it does, yell once and skip */
- if (WARN_ON_ONCE(stackidx >= FTRACE_KSTACK_NESTING))
+ if (WARN_ON_ONCE(stackidx > FTRACE_KSTACK_NESTING))
goto out;
/*
@@ -2813,7 +2813,7 @@ static void __ftrace_trace_stack(struct ring_buffer
*buffer,
*/
barrier();
- fstack = this_cpu_ptr(ftrace_stacks.stacks) + (stackidx - 1);
+ fstack = this_cpu_ptr(ftrace_stacks.stacks) + stackidx;
trace.entries = fstack->calls;
trace.max_entries = FTRACE_KSTACK_ENTRIES;
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [patch V2 28/29] stacktrace: Provide common infrastructure
On Fri, Apr 19, 2019 at 11:07:17AM +0200, Peter Zijlstra wrote: > On Fri, Apr 19, 2019 at 10:32:30AM +0200, Thomas Gleixner wrote: > > On Fri, 19 Apr 2019, Peter Zijlstra wrote: > > > On Thu, Apr 18, 2019 at 10:41:47AM +0200, Thomas Gleixner wrote: > > > > > > > +typedef bool (*stack_trace_consume_fn)(void *cookie, unsigned long > > > > addr, > > > > + bool reliable); > > > > > > > +void arch_stack_walk(stack_trace_consume_fn consume_entry, void > > > > *cookie, > > > > +struct task_struct *task, struct pt_regs *regs); > > > > +int arch_stack_walk_reliable(stack_trace_consume_fn consume_entry, > > > > void *cookie, > > > > +struct task_struct *task); > > > > > > This bugs me a little; ideally the _reliable() thing would not exists. > > > > > > Thomas said that the existing __save_stack_trace_reliable() is different > > > enough for the unification to be non-trivial, but maybe Josh can help > > > out? > > > > > > >From what I can see the biggest significant differences are: > > > > > > - it looks at the regs sets on the stack and for FP bails early > > > - bails for khreads and idle (after it does all the hard work!?!) That's done for a reason, see the "Success path" comments. > > > The first (FP checking for exceptions) should probably be reflected in > > > consume_fn(.reliable) anyway -- although that would mean a lot of extra > > > '?' entries where there are none today. > > > > > > And the second (KTHREAD/IDLE) is something that the generic code can > > > easily do before calling into the arch unwinder. > > > > And looking at the powerpc version of it, that has even more interesting > > extra checks in that function. > > Right, but not fundamentally different from determining @reliable I > think. > > Anyway, it would be good if someone knowledgable could have a look at > this. Yeah, we could probably do that. The flow would need to be changed a bit -- some of the errors are soft errors which most users don't care about because they just want a best effort. The soft errors can be remembered without breaking out of the loop, and just returned at the end. Most users could just ignore the return code. The only thing I'd be worried about is performance for the non-livepatch users, but I guess those checks don't look very expensive. And the x86 unwinders are already pretty slow anyway... -- Josh ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [patch V2 28/29] stacktrace: Provide common infrastructure
On Fri, Apr 19, 2019 at 09:02:11AM +0200, Peter Zijlstra wrote: > On Thu, Apr 18, 2019 at 05:42:55PM +0200, Thomas Gleixner wrote: > > On Thu, 18 Apr 2019, Josh Poimboeuf wrote: > > > > Another idea I had (but never got a chance to work on) was to extend the > > > x86 unwind interface to all arches. So instead of the callbacks, each > > > arch would implement something like this API: > > > I surely thought about that, but after staring at all incarnations of > > arch/*/stacktrace.c I just gave up. > > > > Aside of that quite some archs already have callback based unwinders > > because they use them for more than stacktracing and just have a single > > implementation of that loop. > > > > I'm fine either way. We can start with x86 and then let archs convert over > > their stuff, but I wouldn't hold my breath that this will be completed in > > the forseeable future. > > I suggested the same to Thomas early on, and I even spend the time to > convert some $random arch to the iterator interface, and while it is > indeed entirely feasible, it is _far_ more work. > > The callback thing OTOH is flexible enough to do what we want to do now, > and allows converting most archs to it without too much pain (as Thomas > said, many archs are already in this form and only need minor API > adjustments), which gets us in a far better place than we are now. > > And we can always go to iterators later on. But I think getting the > generic unwinder improved across all archs is a really important first > step here. Fair enough. -- Josh ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [patch V2 28/29] stacktrace: Provide common infrastructure
On Thu, Apr 18, 2019 at 10:41:47AM +0200, Thomas Gleixner wrote: > All architectures which support stacktrace carry duplicated code and > do the stack storage and filtering at the architecture side. > > Provide a consolidated interface with a callback function for consuming the > stack entries provided by the architecture specific stack walker. This > removes lots of duplicated code and allows to implement better filtering > than 'skip number of entries' in the future without touching any > architecture specific code. > > Signed-off-by: Thomas Gleixner > Cc: linux-a...@vger.kernel.org This is a step in the right direction, especially if it allows us to get rid of the 'skip' stuff. But I'm not crazy about the callbacks. Another idea I had (but never got a chance to work on) was to extend the x86 unwind interface to all arches. So instead of the callbacks, each arch would implement something like this API: struct unwind_state state; void unwind_start(struct unwind_state *state, struct task_struct *task, struct pt_regs *regs, unsigned long *first_frame); bool unwind_next_frame(struct unwind_state *state); inline bool unwind_done(struct unwind_state *state); Not only would it avoid the callbacks (which is a nice benefit already), it would also allow the interfaces to be used outside of the stack_trace_*() interfaces. That would come in handy in cases like the ftrace stack tracer code, which needs more than the stack_trace_*() API can give. Of course, this may be more work than what you thought you signed up for ;-) -- Josh ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [patch V2 01/29] tracing: Cleanup stack trace code
On Thu, Apr 18, 2019 at 10:41:20AM +0200, Thomas Gleixner wrote: > - Remove the extra array member of stack_dump_trace[]. It's not required as > the stack tracer stores at max array size - 1 entries so there is still > an empty slot. What is the empty slot used for? -- Josh ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC patch 28/41] dma/debug: Simplify stracktrace retrieval
On Wed, Apr 10, 2019 at 12:28:22PM +0200, Thomas Gleixner wrote:
> Replace the indirection through struct stack_trace with an invocation of
> the storage array based interface.
>
> Signed-off-by: Thomas Gleixner
> Cc: iommu@lists.linux-foundation.org
> Cc: Robin Murphy
> Cc: Christoph Hellwig
> Cc: Marek Szyprowski
> ---
> kernel/dma/debug.c | 13 +
> 1 file changed, 5 insertions(+), 8 deletions(-)
>
> --- a/kernel/dma/debug.c
> +++ b/kernel/dma/debug.c
> @@ -89,8 +89,8 @@ struct dma_debug_entry {
> int sg_mapped_ents;
> enum map_err_types map_err_type;
> #ifdef CONFIG_STACKTRACE
> - struct stack_trace stacktrace;
> - unsigned longst_entries[DMA_DEBUG_STACKTRACE_ENTRIES];
> + unsigned intst_len;
> + unsigned long st_entries[DMA_DEBUG_STACKTRACE_ENTRIES];
nit: st_entries isn't very readable. Thanks to the magic of compilers,
the characters are free, so why not call them "stacktrace_entries" and
"stacktrace_len".
--
Josh
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5 32/32] x86/mm: Add support to make use of Secure Memory Encryption
On Fri, May 19, 2017 at 01:30:05PM +0200, Borislav Petkov wrote:
> > it is called so early. I can get past it by adding:
> >
> > CFLAGS_mem_encrypt.o := $(nostackp)
> >
> > in the arch/x86/mm/Makefile, but that obviously eliminates the support
> > for the whole file. Would it be better to split out the sme_enable()
> > and other boot routines into a separate file or just apply the
> > $(nostackp) to the whole file?
>
> Josh might have a better idea here... CCed.
I'm the stack validation guy, not the stack protection guy :-)
But there is a way to disable compiler options on a per-function basis
with the gcc __optimize__ function attribute. For example:
__attribute__((__optimize__("no-stack-protector")))
--
Josh
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
