* Re: [PATCH 1/4] hugetlbfs: add arch_hugetlb_valid_size
From: Christian Borntraeger @ 2020-03-25 9:38 UTC (permalink / raw)
To: Longpeng (Mike, Cloud Infrastructure Service Product Dept.),
Mike Kravetz, Dave Hansen, linux-mm, linux-kernel,
linux-arm-kernel, linuxppc-dev, linux-riscv, linux-s390,
sparclinux, linux-doc
Cc: Albert Ou, Andrew Morton, Vasily Gorbik, Jonathan Corbet,
Catalin Marinas, Dave Hansen, Heiko Carstens, Ingo Molnar,
Palmer Dabbelt, Paul Walmsley, Paul Mackerras, Thomas Gleixner,
Will Deacon, David S.Miller
In-Reply-To: <5ea6313e-ec4f-a043-632b-ef2901ce2cc9@huawei.com>
On 25.03.20 03:58, Longpeng (Mike, Cloud Infrastructure Service Product Dept.) wrote:
[...]
> Hi Mike,
>
> Inspired by Dave's opinion, it seems the x86-specific hugepages_supported should
> also need to use cpu_feature_enabled instead.
>
> Also, I wonder if the hugepages_supported is correct ? There're two arch
> specific hugepages_supported:
> x86:
> #define hugepages_supported() boot_cpu_has(X86_FEATURE_PSE)
> and
> s390:
> #define hugepages_supported() (MACHINE_HAS_EDAT1)
>
> Is it possible that x86 has X86_FEATURE_GBPAGES but hasn't X86_FEATURE_GBPAGES
> or s390 has MACHINE_HAS_EDAT2 but hasn't MACHINE_HAS_EDAT1 ?
The s390 architecture says that
When EDAT-2 applies, the following function is available in the DAT process:
- EDAT-1 applies.
[..]
So if the machine has EDAT-2 it also has EDAT-1.
^ permalink raw reply
* [PATCH 1/2] Fix 2 "[v3, 25/32] powerpc/64: system call implement entry/exit logic in C"
From: Nicholas Piggin @ 2020-03-25 9:30 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Nicholas Piggin
This fixes 4 issues caught by TM selftests. First was a tm-syscall bug
that hit due to tabort_syscall being called after interrupts were reconciled
(in a subsequent patch), which led to interrupts being enabled before
tabort_syscall was called. Rather than going through an un-reconciling
interrupts for the return, I just go back to putting the test early in
asm, the C-ification of that wasn't a big win anyway.
Second is the syscall return _TIF_USER_WORK_MASK check would go into an
infinite loop if _TIF_RESTORE_TM became set. The asm code uses
_TIF_USER_WORK_MASK to brach to slowpath which includes restore_tm_state.
Third is system call return was not calling restore_tm_state, I missed
this completely (alhtough it's in the return from interrupt C conversion
because when the asm syscall code encountered problems it would branch
to the interrupt return code.
Fourth is MSR_VEC missing from restore_math, which was caught by
tm-unavailable selftest taking an unexpected facility unavailable
interrupt when testing VSX unavailble exception with MSR.FP=1 MSR.VEC=1.
Fourth case also has a fixup in a subsequent patch.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/kernel/entry_64.S | 12 +++++++++---
arch/powerpc/kernel/syscall_64.c | 25 +++++++++++++------------
2 files changed, 22 insertions(+), 15 deletions(-)
diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
index dc7fd3196d20..403224acdaa8 100644
--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S
@@ -64,6 +64,12 @@ exception_marker:
.globl system_call_common
system_call_common:
+#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
+BEGIN_FTR_SECTION
+ extrdi. r10, r12, 1, (63-MSR_TS_T_LG) /* transaction active? */
+ bne .Ltabort_syscall
+END_FTR_SECTION_IFSET(CPU_FTR_TM)
+#endif
_ASM_NOKPROBE_SYMBOL(system_call_common)
mr r10,r1
ld r1,PACAKSAVE(r13)
@@ -179,7 +185,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
b .Lsyscall_restore_regs_cont
#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
-_GLOBAL(tabort_syscall) /* (unsigned long nip, unsigned long msr) */
+.Ltabort_syscall:
/* Firstly we need to enable TM in the kernel */
mfmsr r10
li r9, 1
@@ -199,8 +205,8 @@ _GLOBAL(tabort_syscall) /* (unsigned long nip, unsigned long msr) */
li r9, MSR_RI
andc r10, r10, r9
mtmsrd r10, 1
- mtspr SPRN_SRR0, r3
- mtspr SPRN_SRR1, r4
+ mtspr SPRN_SRR0, r11
+ mtspr SPRN_SRR1, r12
RFI_TO_USER
b . /* prevent speculative execution */
#endif
diff --git a/arch/powerpc/kernel/syscall_64.c b/arch/powerpc/kernel/syscall_64.c
index ffd601d87065..56533a26f3b7 100644
--- a/arch/powerpc/kernel/syscall_64.c
+++ b/arch/powerpc/kernel/syscall_64.c
@@ -15,8 +15,6 @@
#include <asm/time.h>
#include <asm/unistd.h>
-extern void __noreturn tabort_syscall(unsigned long nip, unsigned long msr);
-
typedef long (*syscall_fn)(long, long, long, long, long, long);
/* Has to run notrace because it is entered "unreconciled" */
@@ -32,10 +30,6 @@ notrace long system_call_exception(long r3, long r4, long r5, long r6, long r7,
BUG_ON(!FULL_REGS(regs));
BUG_ON(regs->softe != IRQS_ENABLED);
- if (IS_ENABLED(CONFIG_PPC_TRANSACTIONAL_MEM) &&
- unlikely(regs->msr & MSR_TS_T))
- tabort_syscall(regs->nip, regs->msr);
-
account_cpu_user_entry();
#ifdef CONFIG_PPC_SPLPAR
@@ -161,7 +155,7 @@ notrace unsigned long syscall_exit_prepare(unsigned long r3,
again:
local_irq_disable();
ti_flags = READ_ONCE(*ti_flagsp);
- while (unlikely(ti_flags & _TIF_USER_WORK_MASK)) {
+ while (unlikely(ti_flags & (_TIF_USER_WORK_MASK & ~_TIF_RESTORE_TM))) {
local_irq_enable();
if (ti_flags & _TIF_NEED_RESCHED) {
schedule();
@@ -180,13 +174,20 @@ notrace unsigned long syscall_exit_prepare(unsigned long r3,
}
if (IS_ENABLED(CONFIG_PPC_BOOK3S) && IS_ENABLED(CONFIG_PPC_FPU)) {
- unsigned long mathflags = MSR_FP;
+ if (IS_ENABLED(CONFIG_PPC_TRANSACTIONAL_MEM) &&
+ unlikely((ti_flags & _TIF_RESTORE_TM))) {
+ restore_tm_state(regs);
+ } else {
+ unsigned long mathflags = MSR_FP;
- if (IS_ENABLED(CONFIG_ALTIVEC))
- mathflags |= MSR_VEC;
+ if (cpu_has_feature(CPU_FTR_VSX))
+ mathflags |= MSR_VEC | MSR_VSX;
+ else if (cpu_has_feature(CPU_FTR_ALTIVEC))
+ mathflags |= MSR_VEC;
- if ((regs->msr & mathflags) != mathflags)
- restore_math(regs);
+ if ((regs->msr & mathflags) != mathflags)
+ restore_math(regs);
+ }
}
/* This must be done with RI=1 because tracing may touch vmaps */
--
2.23.0
^ permalink raw reply related
* [PATCH 2/2] Fix 3 "[v3, 28/32] powerpc/64s: interrupt implement exit logic in C"
From: Nicholas Piggin @ 2020-03-25 9:30 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Nicholas Piggin
In-Reply-To: <20200325093014.149962-1-npiggin@gmail.com>
This fixes the interrupt-return part of the MSR_VSX restore bug caught
by tm-unavailable selftest.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/kernel/syscall_64.c | 24 +++++++++++++-----------
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/arch/powerpc/kernel/syscall_64.c b/arch/powerpc/kernel/syscall_64.c
index 56533a26f3b7..a2995909b83b 100644
--- a/arch/powerpc/kernel/syscall_64.c
+++ b/arch/powerpc/kernel/syscall_64.c
@@ -251,19 +251,21 @@ notrace unsigned long interrupt_exit_user_prepare(struct pt_regs *regs, unsigned
ti_flags = READ_ONCE(*ti_flagsp);
}
- if (IS_ENABLED(CONFIG_PPC_BOOK3S)) {
- unsigned long mathflags = 0;
-
- if (IS_ENABLED(CONFIG_PPC_FPU))
- mathflags |= MSR_FP;
- if (IS_ENABLED(CONFIG_ALTIVEC))
- mathflags |= MSR_VEC;
-
+ if (IS_ENABLED(CONFIG_PPC_BOOK3S) && IS_ENABLED(CONFIG_PPC_FPU)) {
if (IS_ENABLED(CONFIG_PPC_TRANSACTIONAL_MEM) &&
- (ti_flags & _TIF_RESTORE_TM))
+ unlikely((ti_flags & _TIF_RESTORE_TM))) {
restore_tm_state(regs);
- else if ((regs->msr & mathflags) != mathflags)
- restore_math(regs);
+ } else {
+ unsigned long mathflags = MSR_FP;
+
+ if (cpu_has_feature(CPU_FTR_VSX))
+ mathflags |= MSR_VEC | MSR_VSX;
+ else if (cpu_has_feature(CPU_FTR_ALTIVEC))
+ mathflags |= MSR_VEC;
+
+ if ((regs->msr & mathflags) != mathflags)
+ restore_math(regs);
+ }
}
trace_hardirqs_on();
--
2.23.0
^ permalink raw reply related
* Re: [PATCH] powerpc/boot: Delete unneeded .globl _zimage_start
From: Alan Modra @ 2020-03-25 8:37 UTC (permalink / raw)
To: Joel Stanley
Cc: Nick Desaulniers, linuxppc-dev, Fangrui Song, clang-built-linux
In-Reply-To: <CACPK8Xf70Yq2szW110G_2f2Q3J6inVqQY4Y-y0tggkVmT7tmfw@mail.gmail.com>
On Wed, Mar 25, 2020 at 05:22:31AM +0000, Joel Stanley wrote:
> On Wed, 25 Mar 2020 at 05:19, Fangrui Song <maskray@google.com> wrote:
> >
> > .globl sets the symbol binding to STB_GLOBAL while .weak sets the
> > binding to STB_WEAK. They should not be used together. It is accidetal
> > rather then intentional that GNU as let .weak override .globl while
> > clang integrated assembler let the last win.
No, it isn't accidental. gas deliberately lets .weak override .globl.
Since 1996-07-26, git commit 5ca547dc239
I'm fine with the patch so far as it is true that there is no need for
both .globl and .weak (and it looks silly to have both), but the
explanation isn't true. The patch is needed because the clang
assembler is incompatible with gas in this detail.
> > Fixes: cd197ffcf10b "[POWERPC] zImage: Cleanup and improve zImage entry point"
> > Fixes: ee9d21b3b358 "powerpc/boot: Ensure _zimage_start is a weak symbol"
> > Link: https://github.com/ClangBuiltLinux/linux/issues/937
> > Signed-off-by: Fangrui Song <maskray@google.com>
> > Cc: Joel Stanley <joel@jms.id.au>
> > Cc: Michael Ellerman <mpe@ellerman.id.au>
> > Cc: Nick Desaulniers <ndesaulniers@google.com>
> > Cc: clang-built-linux@googlegroups.com
> > ---
> > arch/powerpc/boot/crt0.S | 3 ---
> > 1 file changed, 3 deletions(-)
> >
> > diff --git a/arch/powerpc/boot/crt0.S b/arch/powerpc/boot/crt0.S
> > index 92608f34d312..1d83966f5ef6 100644
> > --- a/arch/powerpc/boot/crt0.S
> > +++ b/arch/powerpc/boot/crt0.S
> > @@ -44,9 +44,6 @@ p_end: .long _end
> > p_pstack: .long _platform_stack_top
> > #endif
> >
> > - .globl _zimage_start
> > - /* Clang appears to require the .weak directive to be after the symbol
> > - * is defined. See https://bugs.llvm.org/show_bug.cgi?id=38921 */
> > .weak _zimage_start
> > _zimage_start:
>
> Your explanation makes sense to me. I've added Alan to cc for his review.
>
> Reviewed-by: Joel Stanley <joel@jms.id.au>
>
> Thanks for the patch.
>
> Cheers,
>
> Joel
>
> > .globl _zimage_start_lib
> > --
> > 2.25.1.696.g5e7596f4ac-goog
--
Alan Modra
Australia Development Lab, IBM
^ permalink raw reply
* Re: [patch V3 03/20] usb: gadget: Use completion interface instead of open coding it
From: Felipe Balbi @ 2020-03-25 8:37 UTC (permalink / raw)
To: Thomas Gleixner, LKML
Cc: Randy Dunlap, linux-ia64, Peter Zijlstra, linux-pci,
Sebastian Siewior, platform-driver-x86, Guo Ren, Joel Fernandes,
Vincent Chen, Ingo Molnar, Jonathan Corbet, Davidlohr Bueso,
kbuild test robot, Brian Cain, linux-acpi, Paul E . McKenney,
linux-hexagon, Rafael J. Wysocki, linux-csky, Linus Torvalds,
Darren Hart, Zhang Rui, Len Brown, Fenghua Yu, Arnd Bergmann,
linux-pm, linuxppc-dev, Greentime Hu, Bjorn Helgaas,
Kurt Schwemmer, Kalle Valo, Michal Simek, Tony Luck, Nick Hu,
Geoff Levand, Greg Kroah-Hartman, linux-usb, linux-wireless,
Oleg Nesterov, Davidlohr Bueso, netdev, Logan Gunthorpe,
David S. Miller, Andy Shevchenko
In-Reply-To: <20200321113241.043380271@linutronix.de>
[-- Attachment #1: Type: text/plain, Size: 1032 bytes --]
Thomas Gleixner <tglx@linutronix.de> writes:
> From: Thomas Gleixner <tglx@linutronix.de>
>
> ep_io() uses a completion on stack and open codes the waiting with:
>
> wait_event_interruptible (done.wait, done.done);
> and
> wait_event (done.wait, done.done);
>
> This waits in non-exclusive mode for complete(), but there is no reason to
> do so because the completion can only be waited for by the task itself and
> complete() wakes exactly one exlusive waiter.
>
> Replace the open coded implementation with the corresponding
> wait_for_completion*() functions.
>
> No functional change.
>
> Reported-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Felipe Balbi <balbi@kernel.org>
> Cc: linux-usb@vger.kernel.org
Do you want to carry it via your tree? If so:
Acked-by: Felipe Balbi <balbi@kernel.org>
Otherwise, let me know and I'll pick this patch.
--
balbi
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply
* Re: [PATCH 1/2] dma-mapping: add a dma_ops_bypass flag to struct device
From: Christoph Hellwig @ 2020-03-25 8:37 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: Greg Kroah-Hartman, Joerg Roedel, Robin Murphy, linux-kernel,
iommu, Aneesh Kumar K.V, linuxppc-dev, Christoph Hellwig,
Lu Baolu
In-Reply-To: <41975da3-3a4a-fc3c-2b90-8d607cf220e6@ozlabs.ru>
On Wed, Mar 25, 2020 at 03:51:36PM +1100, Alexey Kardashevskiy wrote:
> >> This is for persistent memory which you can DMA to/from but yet it does
> >> not appear in the system as a normal memory and therefore requires
> >> special handling anyway (O_DIRECT or DAX, I do not know the exact
> >> mechanics). All other devices in the system should just run as usual,
> >> i.e. use 1:1 mapping if possible.
> >
> > On other systems (x86 and arm) pmem as long as it is page backed does
> > not require any special handling. This must be some weird way powerpc
> > fucked up again, and I suspect you'll have to suffer from it.
>
>
> It does not matter if it is backed by pages or not, the problem may also
> appear if we wanted for example p2p PCI via IOMMU (between PHBs) and
> MMIO might be mapped way too high in the system address space and make
> 1:1 impossible.
How can it be mapped too high for a direct mapping with a 64-bit DMA
mask?
^ permalink raw reply
* Re: [PATCH] mm/sparse: Fix kernel crash with pfn_section_valid check
From: Baoquan He @ 2020-03-25 8:36 UTC (permalink / raw)
To: Aneesh Kumar K.V
Cc: Sachin Sant, david, linux-kernel, mhocko, linux-mm, akpm,
linuxppc-dev, dan.j.williams
In-Reply-To: <5cdf5334-7fbb-8427-1918-ed67d5f23834@linux.ibm.com>
On 03/25/20 at 01:42pm, Aneesh Kumar K.V wrote:
> On 3/25/20 1:07 PM, Baoquan He wrote:
> > On 03/25/20 at 03:06pm, Baoquan He wrote:
> > > On 03/25/20 at 08:49am, Aneesh Kumar K.V wrote:
> >
> > > > mm/sparse.c | 2 ++
> > > > 1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/mm/sparse.c b/mm/sparse.c
> > > > index aadb7298dcef..3012d1f3771a 100644
> > > > --- a/mm/sparse.c
> > > > +++ b/mm/sparse.c
> > > > @@ -781,6 +781,8 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages,
> > > > ms->usage = NULL;
> > > > }
> > > > memmap = sparse_decode_mem_map(ms->section_mem_map, section_nr);
> > > > + /* Mark the section invalid */
> > > > + ms->section_mem_map &= ~SECTION_HAS_MEM_MAP;
> > >
> > > Not sure if we should add checking in valid_section() or pfn_valid(),
> > > e.g check ms->usage validation too. Otherwise, this fix looks good to
> > > me.
> >
> > With SPASEMEM_VMEMAP enabled, we should do validation check on ms->usage
> > before checking any subsection is valid. Since now we do have case
> > in which ms->usage is released, people still try to check it.
> >
> > diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> > index f0a2c184eb9a..d79bd938852e 100644
> > --- a/include/linux/mmzone.h
> > +++ b/include/linux/mmzone.h
> > @@ -1306,6 +1306,8 @@ static inline int pfn_section_valid(struct mem_section *ms, unsigned long pfn)
> > {
> > int idx = subsection_map_index(pfn);
> > + if (!ms->usage)
> > + return 0;
> > return test_bit(idx, ms->usage->subsection_map);
> > }
> > #else
> >
>
> We always check for section valid, before we check if pfn_section_valid().
>
> static inline int pfn_valid(unsigned long pfn)
>
> struct mem_section *ms;
>
> if (pfn_to_section_nr(pfn) >= NR_MEM_SECTIONS)
> return 0;
> ms = __nr_to_section(pfn_to_section_nr(pfn));
> if (!valid_section(ms))
> return 0;
> /*
> * Traditionally early sections always returned pfn_valid() for
> * the entire section-sized span.
> */
> return early_section(ms) || pfn_section_valid(ms, pfn);
> }
>
>
> IMHO adding that if (!ms->usage) is redundant.
Yeah, I tend to agree. Consider this happens in the only small window
between ms->usage releasing and ms->section_mem_map releasing when
removing a section. Just thought adding this check to enhance it even
though we have had your fix, because we only check ms->section_mem_map
in valid_section(). Anyway, your fix looks good to me, see if other
people have any comment.
Thanks
Baoquan
^ permalink raw reply
* Re: [PATCH] mm/sparse: Fix kernel crash with pfn_section_valid check
From: Aneesh Kumar K.V @ 2020-03-25 8:12 UTC (permalink / raw)
To: Baoquan He
Cc: Sachin Sant, linux-kernel, linux-mm, akpm, linuxppc-dev,
dan.j.williams
In-Reply-To: <20200325073707.GI3039@MiWiFi-R3L-srv>
On 3/25/20 1:07 PM, Baoquan He wrote:
> On 03/25/20 at 03:06pm, Baoquan He wrote:
>> On 03/25/20 at 08:49am, Aneesh Kumar K.V wrote:
>
>>> mm/sparse.c | 2 ++
>>> 1 file changed, 2 insertions(+)
>>>
>>> diff --git a/mm/sparse.c b/mm/sparse.c
>>> index aadb7298dcef..3012d1f3771a 100644
>>> --- a/mm/sparse.c
>>> +++ b/mm/sparse.c
>>> @@ -781,6 +781,8 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages,
>>> ms->usage = NULL;
>>> }
>>> memmap = sparse_decode_mem_map(ms->section_mem_map, section_nr);
>>> + /* Mark the section invalid */
>>> + ms->section_mem_map &= ~SECTION_HAS_MEM_MAP;
>>
>> Not sure if we should add checking in valid_section() or pfn_valid(),
>> e.g check ms->usage validation too. Otherwise, this fix looks good to
>> me.
>
> With SPASEMEM_VMEMAP enabled, we should do validation check on ms->usage
> before checking any subsection is valid. Since now we do have case
> in which ms->usage is released, people still try to check it.
>
> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> index f0a2c184eb9a..d79bd938852e 100644
> --- a/include/linux/mmzone.h
> +++ b/include/linux/mmzone.h
> @@ -1306,6 +1306,8 @@ static inline int pfn_section_valid(struct mem_section *ms, unsigned long pfn)
> {
> int idx = subsection_map_index(pfn);
>
> + if (!ms->usage)
> + return 0;
> return test_bit(idx, ms->usage->subsection_map);
> }
> #else
>
We always check for section valid, before we check if pfn_section_valid().
static inline int pfn_valid(unsigned long pfn)
struct mem_section *ms;
if (pfn_to_section_nr(pfn) >= NR_MEM_SECTIONS)
return 0;
ms = __nr_to_section(pfn_to_section_nr(pfn));
if (!valid_section(ms))
return 0;
/*
* Traditionally early sections always returned pfn_valid() for
* the entire section-sized span.
*/
return early_section(ms) || pfn_section_valid(ms, pfn);
}
IMHO adding that if (!ms->usage) is redundant.
-aneesh
^ permalink raw reply
* Re: [PATCH] mm/sparse: Fix kernel crash with pfn_section_valid check
From: Baoquan He @ 2020-03-25 7:37 UTC (permalink / raw)
To: Aneesh Kumar K.V
Cc: Sachin Sant, linux-kernel, linux-mm, akpm, linuxppc-dev,
dan.j.williams
In-Reply-To: <20200325070643.GH3039@MiWiFi-R3L-srv>
On 03/25/20 at 03:06pm, Baoquan He wrote:
> On 03/25/20 at 08:49am, Aneesh Kumar K.V wrote:
> > mm/sparse.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/mm/sparse.c b/mm/sparse.c
> > index aadb7298dcef..3012d1f3771a 100644
> > --- a/mm/sparse.c
> > +++ b/mm/sparse.c
> > @@ -781,6 +781,8 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages,
> > ms->usage = NULL;
> > }
> > memmap = sparse_decode_mem_map(ms->section_mem_map, section_nr);
> > + /* Mark the section invalid */
> > + ms->section_mem_map &= ~SECTION_HAS_MEM_MAP;
>
> Not sure if we should add checking in valid_section() or pfn_valid(),
> e.g check ms->usage validation too. Otherwise, this fix looks good to
> me.
With SPASEMEM_VMEMAP enabled, we should do validation check on ms->usage
before checking any subsection is valid. Since now we do have case
in which ms->usage is released, people still try to check it.
diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index f0a2c184eb9a..d79bd938852e 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -1306,6 +1306,8 @@ static inline int pfn_section_valid(struct mem_section *ms, unsigned long pfn)
{
int idx = subsection_map_index(pfn);
+ if (!ms->usage)
+ return 0;
return test_bit(idx, ms->usage->subsection_map);
}
#else
^ permalink raw reply related
* Re: [PATCH] mm/sparse: Fix kernel crash with pfn_section_valid check
From: Baoquan He @ 2020-03-25 7:06 UTC (permalink / raw)
To: Aneesh Kumar K.V
Cc: Sachin Sant, linux-kernel, linux-mm, akpm, linuxppc-dev,
dan.j.williams
In-Reply-To: <20200325031914.107660-1-aneesh.kumar@linux.ibm.com>
On 03/25/20 at 08:49am, Aneesh Kumar K.V wrote:
> Fixes the below crash
>
> BUG: Kernel NULL pointer dereference on read at 0x00000000
> Faulting instruction address: 0xc000000000c3447c
> Oops: Kernel access of bad area, sig: 11 [#1]
> LE PAGE_SIZE=64K MMU=Hash SMP NR_CPUS=2048 NUMA pSeries
> CPU: 11 PID: 7519 Comm: lt-ndctl Not tainted 5.6.0-rc7-autotest #1
> ...
> NIP [c000000000c3447c] vmemmap_populated+0x98/0xc0
> LR [c000000000088354] vmemmap_free+0x144/0x320
> Call Trace:
> section_deactivate+0x220/0x240
> __remove_pages+0x118/0x170
> arch_remove_memory+0x3c/0x150
> memunmap_pages+0x1cc/0x2f0
> devm_action_release+0x30/0x50
> release_nodes+0x2f8/0x3e0
> device_release_driver_internal+0x168/0x270
> unbind_store+0x130/0x170
> drv_attr_store+0x44/0x60
> sysfs_kf_write+0x68/0x80
> kernfs_fop_write+0x100/0x290
> __vfs_write+0x3c/0x70
> vfs_write+0xcc/0x240
> ksys_write+0x7c/0x140
> system_call+0x5c/0x68
>
> With commit: d41e2f3bd546 ("mm/hotplug: fix hot remove failure in SPARSEMEM|!VMEMMAP case")
> section_mem_map is set to NULL after depopulate_section_mem(). This
> was done so that pfn_page() can work correctly with kernel config that disables
> SPARSEMEM_VMEMMAP. With that config pfn_to_page does
>
> __section_mem_map_addr(__sec) + __pfn;
> where
>
> static inline struct page *__section_mem_map_addr(struct mem_section *section)
> {
> unsigned long map = section->section_mem_map;
> map &= SECTION_MAP_MASK;
> return (struct page *)map;
> }
>
> Now with SPASEMEM_VMEMAP enabled, mem_section->usage->subsection_map is used to
> check the pfn validity (pfn_valid()). Since section_deactivate release
> mem_section->usage if a section is fully deactivated, pfn_valid() check after
> a subsection_deactivate cause a kernel crash.
>
> static inline int pfn_valid(unsigned long pfn)
> {
> ...
> return early_section(ms) || pfn_section_valid(ms, pfn);
> }
>
> where
>
> static inline int pfn_section_valid(struct mem_section *ms, unsigned long pfn)
> {
> int idx = subsection_map_index(pfn);
>
> return test_bit(idx, ms->usage->subsection_map);
> }
>
> Avoid this by clearing SECTION_HAS_MEM_MAP when mem_section->usage is freed.
>
> Fixes: d41e2f3bd546 ("mm/hotplug: fix hot remove failure in SPARSEMEM|!VMEMMAP case")
> Cc: Baoquan He <bhe@redhat.com>
> Reported-by: Sachin Sant <sachinp@linux.vnet.ibm.com>
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
Maybe add Sachin's Tested-by, Sachin has tested and confirmed this fix
works.
> ---
> mm/sparse.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/mm/sparse.c b/mm/sparse.c
> index aadb7298dcef..3012d1f3771a 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -781,6 +781,8 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages,
> ms->usage = NULL;
> }
> memmap = sparse_decode_mem_map(ms->section_mem_map, section_nr);
> + /* Mark the section invalid */
> + ms->section_mem_map &= ~SECTION_HAS_MEM_MAP;
Not sure if we should add checking in valid_section() or pfn_valid(),
e.g check ms->usage validation too. Otherwise, this fix looks good to
me.
Reviewed-by: Baoquan He <bhe@redhat.com>
^ permalink raw reply
* Re: [PATCH] powerpc/boot: Delete unneeded .globl _zimage_start
From: Segher Boessenkool @ 2020-03-25 6:23 UTC (permalink / raw)
To: Fangrui Song
Cc: Nick Desaulniers, linuxppc-dev, Joel Stanley, clang-built-linux
In-Reply-To: <20200325051820.163253-1-maskray@google.com>
On Tue, Mar 24, 2020 at 10:18:20PM -0700, Fangrui Song wrote:
> .globl sets the symbol binding to STB_GLOBAL while .weak sets the
> binding to STB_WEAK. They should not be used together. It is accidetal
> rather then intentional that GNU as let .weak override .globl while
> clang integrated assembler let the last win.
Nothing is "overridden".
The as manual says (.weak):
This directive sets the weak attribute on the comma separated list of
symbol 'names'. If the symbols do not already exist, they will be
created.
so this behaviour is obviously as intended (or was later documented in
any case), so LLVM has a bug to fix (whether you like this (much saner)
behaviour or not).
Segher
^ permalink raw reply
* Re: Argh, can't find dcache properties !
From: Michael Ellerman @ 2020-03-25 6:01 UTC (permalink / raw)
To: Qian Cai, Chris Packham
Cc: Hamish Martin, linux-kernel@vger.kernel.org, paulus@samba.org,
tglx@linutronix.de, linuxppc-dev@lists.ozlabs.org
In-Reply-To: <48F1D8CF-13A1-4348-8973-81503782A451@lca.pw>
Qian Cai <cai@lca.pw> writes:
>> On Mar 24, 2020, at 4:06 PM, Chris Packham <Chris.Packham@alliedtelesis.co.nz> wrote:
>> On Tue, 2020-03-24 at 15:47 +1100, Michael Ellerman wrote:
>>> Chris Packham <Chris.Packham@alliedtelesis.co.nz> writes:
>>>> Hi All,
>>>>
>>>> Just booting up v5.5.11 on a Freescale T2080RDB and I'm seeing the
>>>> following mesage.
>>>>
>>>> kern.warning linuxbox kernel: Argh, can't find dcache properties !
>>>> kern.warning linuxbox kernel: Argh, can't find icache properties !
...
>
> BTW, POWER9 PowerNV would have the same thing.
Ugh, you're right.
Because we're missing the cache-line-size properties, even though they
are optional when the block & line size are the same.
# find /proc/device-tree/cpus/PowerPC\,POWER9@0/ -name '*cache*'
/proc/device-tree/cpus/PowerPC,POWER9@0/l2-cache
/proc/device-tree/cpus/PowerPC,POWER9@0/d-cache-block-size
/proc/device-tree/cpus/PowerPC,POWER9@0/d-cache-size
/proc/device-tree/cpus/PowerPC,POWER9@0/i-cache-size
/proc/device-tree/cpus/PowerPC,POWER9@0/d-cache-sets
/proc/device-tree/cpus/PowerPC,POWER9@0/i-cache-block-size
/proc/device-tree/cpus/PowerPC,POWER9@0/i-cache-sets
skiboot even explicitly omits them:
if (cache->icache_line_size != cache->icache_block_size)
dt_add_property_cells(cpu, "i-cache-line-size",
be32_to_cpu(cache->icache_line_size));
if (cache->l1_dcache_line_size != cache->dcache_block_size)
dt_add_property_cells(cpu, "d-cache-line-size",
be32_to_cpu(cache->l1_dcache_line_size));
Looks like it was broken ~3 years ago, in:
bd067f83b084 ("powerpc/64: Fix naming of cache block vs. cache line")
Previously we did:
lsizep = of_get_property(np, "d-cache-block-size",
NULL);
/* fallback if block size missing */
if (lsizep == NULL)
lsizep = of_get_property(np,
"d-cache-line-size",
NULL);
if (lsizep != NULL)
lsize = be32_to_cpu(*lsizep);
if (sizep == NULL || lsizep == NULL)
DBG("Argh, can't find dcache properties ! "
"sizep: %p, lsizep: %p\n", sizep, lsizep);
ie. fallback from block size to line size, and only print if both are missing.
That commit changed the names and the logic, but not in a consistent
fashion, making "d-cache-line-size" required to avoid the Argh:
bsizep = of_get_property(np, "d-cache-block-size",
NULL);
lsizep = of_get_property(np, "d-cache-line-size",
NULL);
if (bsizep == NULL)
bsizep = lsizep;
if (lsizep != NULL)
lsize = be32_to_cpu(*lsizep);
if (bsizep != NULL)
bsize = be32_to_cpu(*bsizep);
if (sizep == NULL || bsizep == NULL || lsizep == NULL)
DBG("Argh, can't find dcache properties ! "
"sizep: %p, bsizep: %p, lsizep: %p\n",
sizep, bsizep, lsizep);
Back then we fell back to cur_cpu_spec->dcache_bsize, which should be
correct. But since then we introduced the device tree CPU features
parsing, which does:
static struct cpu_spec __initdata base_cpu_spec = {
...
.icache_bsize = 32, /* minimum block size, fixed by */
.dcache_bsize = 32, /* cache info init. */
So on systems with new enough skiboot we now default to 32, which is
wrong on Power9.
Luckily this info is not used by the sysfs cache files, because that
code doesn't use the values we parse here, it goes and looks at the
device tree itself. Which is pretty gross but actually saves us in this
case.
These values do end up in the vdso_data, and I can see the wrong values
in the vdso_data:
1c:mon> d4 c000000002390000
c000000002390000 54535953 46434d45 50503a47 00343643 eyecatcher
c000000002390010 00000001 00000001 00000100 004e1202 major minor platform processor
c000000002390020 000000b0 00000000 00000000 0000003c processorCount physicalMemorySize
c000000002390030 57b7623f 0000ac10 1e848000 00000000 tb_orig_stamp tb_ticks_per_sec
c000000002390040 de6d9e42 008637af 8de66bca 0005e7ae tb_to_xs stamp_xsec
c000000002390050 000ff730 00000000 00000000 00000000 tb_update_count tz_minuteswest tz_dsttime
c000000002390060 00008000 00000020 00008000 00000020 dcache_size line_size icache_size icache_line_size
^
32 != 128
And that appears to flow through to glibc, ie. I see:
_SC_LEVEL1_ICACHE_LINESIZE = 32
_SC_LEVEL1_DCACHE_LINESIZE = 32
_SC_LEVEL2_CACHE_LINESIZE = 32
_SC_LEVEL3_CACHE_LINESIZE = 32
So excuse me while I go and swear at something.
cheers
^ permalink raw reply
* Re: [PATCH] powerpc/boot: Delete unneeded .globl _zimage_start
From: Joel Stanley @ 2020-03-25 5:22 UTC (permalink / raw)
To: Fangrui Song, Alan Modra
Cc: Nick Desaulniers, linuxppc-dev, clang-built-linux
In-Reply-To: <20200325051820.163253-1-maskray@google.com>
On Wed, 25 Mar 2020 at 05:19, Fangrui Song <maskray@google.com> wrote:
>
> .globl sets the symbol binding to STB_GLOBAL while .weak sets the
> binding to STB_WEAK. They should not be used together. It is accidetal
> rather then intentional that GNU as let .weak override .globl while
> clang integrated assembler let the last win.
>
> Fixes: cd197ffcf10b "[POWERPC] zImage: Cleanup and improve zImage entry point"
> Fixes: ee9d21b3b358 "powerpc/boot: Ensure _zimage_start is a weak symbol"
> Link: https://github.com/ClangBuiltLinux/linux/issues/937
> Signed-off-by: Fangrui Song <maskray@google.com>
> Cc: Joel Stanley <joel@jms.id.au>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Nick Desaulniers <ndesaulniers@google.com>
> Cc: clang-built-linux@googlegroups.com
> ---
> arch/powerpc/boot/crt0.S | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/arch/powerpc/boot/crt0.S b/arch/powerpc/boot/crt0.S
> index 92608f34d312..1d83966f5ef6 100644
> --- a/arch/powerpc/boot/crt0.S
> +++ b/arch/powerpc/boot/crt0.S
> @@ -44,9 +44,6 @@ p_end: .long _end
> p_pstack: .long _platform_stack_top
> #endif
>
> - .globl _zimage_start
> - /* Clang appears to require the .weak directive to be after the symbol
> - * is defined. See https://bugs.llvm.org/show_bug.cgi?id=38921 */
> .weak _zimage_start
> _zimage_start:
Your explanation makes sense to me. I've added Alan to cc for his review.
Reviewed-by: Joel Stanley <joel@jms.id.au>
Thanks for the patch.
Cheers,
Joel
> .globl _zimage_start_lib
> --
> 2.25.1.696.g5e7596f4ac-goog
>
^ permalink raw reply
* [PATCH] powerpc/boot: Delete unneeded .globl _zimage_start
From: Fangrui Song @ 2020-03-25 5:18 UTC (permalink / raw)
To: linuxppc-dev
Cc: Nick Desaulniers, Joel Stanley, Fangrui Song, clang-built-linux
.globl sets the symbol binding to STB_GLOBAL while .weak sets the
binding to STB_WEAK. They should not be used together. It is accidetal
rather then intentional that GNU as let .weak override .globl while
clang integrated assembler let the last win.
Fixes: cd197ffcf10b "[POWERPC] zImage: Cleanup and improve zImage entry point"
Fixes: ee9d21b3b358 "powerpc/boot: Ensure _zimage_start is a weak symbol"
Link: https://github.com/ClangBuiltLinux/linux/issues/937
Signed-off-by: Fangrui Song <maskray@google.com>
Cc: Joel Stanley <joel@jms.id.au>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Nick Desaulniers <ndesaulniers@google.com>
Cc: clang-built-linux@googlegroups.com
---
arch/powerpc/boot/crt0.S | 3 ---
1 file changed, 3 deletions(-)
diff --git a/arch/powerpc/boot/crt0.S b/arch/powerpc/boot/crt0.S
index 92608f34d312..1d83966f5ef6 100644
--- a/arch/powerpc/boot/crt0.S
+++ b/arch/powerpc/boot/crt0.S
@@ -44,9 +44,6 @@ p_end: .long _end
p_pstack: .long _platform_stack_top
#endif
- .globl _zimage_start
- /* Clang appears to require the .weak directive to be after the symbol
- * is defined. See https://bugs.llvm.org/show_bug.cgi?id=38921 */
.weak _zimage_start
_zimage_start:
.globl _zimage_start_lib
--
2.25.1.696.g5e7596f4ac-goog
^ permalink raw reply related
* Re: [PATCH 1/2] dma-mapping: add a dma_ops_bypass flag to struct device
From: Alexey Kardashevskiy @ 2020-03-25 4:51 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Aneesh Kumar K.V, Joerg Roedel, Robin Murphy, linux-kernel, iommu,
Greg Kroah-Hartman, linuxppc-dev, Lu Baolu
In-Reply-To: <20200324075402.GJ23447@lst.de>
On 24/03/2020 18:54, Christoph Hellwig wrote:
> On Tue, Mar 24, 2020 at 02:05:54PM +1100, Alexey Kardashevskiy wrote:
>> This is for persistent memory which you can DMA to/from but yet it does
>> not appear in the system as a normal memory and therefore requires
>> special handling anyway (O_DIRECT or DAX, I do not know the exact
>> mechanics). All other devices in the system should just run as usual,
>> i.e. use 1:1 mapping if possible.
>
> On other systems (x86 and arm) pmem as long as it is page backed does
> not require any special handling. This must be some weird way powerpc
> fucked up again, and I suspect you'll have to suffer from it.
It does not matter if it is backed by pages or not, the problem may also
appear if we wanted for example p2p PCI via IOMMU (between PHBs) and
MMIO might be mapped way too high in the system address space and make
1:1 impossible.
--
Alexey
^ permalink raw reply
* [PATCH] powerpc/tm: Document h/rfid and mtmsrd quirk
From: Michael Neuling @ 2020-03-25 4:05 UTC (permalink / raw)
To: mpe; +Cc: Michael Neuling, linuxppc-dev, npiggin, Gustavo Romero
The ISA has a quirk that's useful for the Linux implementation.
Document it here so others are less likely to trip over it.
Signed-off-by: Michael Neuling <mikey@neuling.org>
Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
---
.../powerpc/transactional_memory.rst | 27 +++++++++++++++++++
1 file changed, 27 insertions(+)
diff --git a/Documentation/powerpc/transactional_memory.rst b/Documentation/powerpc/transactional_memory.rst
index 09955103ac..74ae71001a 100644
--- a/Documentation/powerpc/transactional_memory.rst
+++ b/Documentation/powerpc/transactional_memory.rst
@@ -245,3 +245,30 @@ POWER9N DD2.2.
Guest migration from POWER8 to POWER9 will work with POWER9N DD2.2 and
POWER9C DD1.2. Since earlier POWER9 processors don't support TM
emulation, migration from POWER8 to POWER9 is not supported there.
+
+Kernel implementation
+=====================
+
+h/rfid mtmsrd quirk
+===================
+
+As defined in the ISA, rfid has a quirk which is useful in early
+exception handling. When in a userspace transaction and we enter the
+kernel via some exception, MSR will end up as TM=0 and TS=01 (ie. TM
+off but TM suspended). Regularly the kernel will want change bits in
+the MSR and will perform an rfid to do this. In this case rfid can
+have SRR0 TM = 0 and TS = 00 (ie. TM off and non transaction) and the
+resulting MSR will retain TM = 0 and TS=01 from before (ie. stay in
+suspend). This is a quirk in the architecture as this would normally
+be a transition from TS=01 to TS=00 (ie. suspend -> non transactional)
+which is an illegal transition.
+
+This quirk is described the architecture in the definition of rfid
+with these lines:
+
+ if (MSR 29:31 ¬ = 0b010 | SRR1 29:31 ¬ = 0b000) then
+ MSR 29:31 <- SRR1 29:31
+
+hrfid and mtmsrd have the same quirk.
+
+The Linux kernel uses this quirk in it's early exception handling.
--
2.25.1
^ permalink raw reply related
* Re: [PATCH v4 3/9] powerpc/vas: Add VAS user space API
From: Haren Myneni @ 2020-03-25 3:48 UTC (permalink / raw)
To: Michael Ellerman
Cc: mikey, herbert, npiggin, linux-crypto, sukadev, linuxppc-dev,
Daniel Axtens
In-Reply-To: <87zhc6xvuk.fsf@mpe.ellerman.id.au>
On Tue, 2020-03-24 at 14:41 +1100, Michael Ellerman wrote:
> Daniel Axtens <dja@axtens.net> writes:
> > Michael Ellerman <mpe@ellerman.id.au> writes:
> >> Daniel Axtens <dja@axtens.net> writes:
> >>> Haren Myneni <haren@linux.ibm.com> writes:
> >>>> diff --git a/arch/powerpc/platforms/powernv/vas-api.c b/arch/powerpc/platforms/powernv/vas-api.c
> >>>> new file mode 100644
> >>>> index 0000000..7d049af
> >>>> --- /dev/null
> >>>> +++ b/arch/powerpc/platforms/powernv/vas-api.c
> >>>> @@ -0,0 +1,257 @@
> >> ...
> >>>> +
> >>>> +static int coproc_mmap(struct file *fp, struct vm_area_struct *vma)
> >>>> +{
> >>>> + struct vas_window *txwin = fp->private_data;
> >>>> + unsigned long pfn;
> >>>> + u64 paste_addr;
> >>>> + pgprot_t prot;
> >>>> + int rc;
> >>>> +
> >>>> + if ((vma->vm_end - vma->vm_start) > PAGE_SIZE) {
> >>>
> >>> I think you said this should be 4096 rather than 64k, regardless of what
> >>> PAGE_SIZE you are compiled with?
> >>
> >> You can't mmap less than a page, a page is PAGE_SIZE bytes.
> >>
> >> So if that checked for 4K explicitly it would prevent mmap on 64K
> >> kernels always, which seems like not what you want?
> >
> > Ah. My bad. Carry on then :)
>
> Well you were just quoting something from Haren, so I think it's over to
> him.
Sorry my mistake.
I should change in documentation. vas_win_paste_addr() always returns 1
page. Restriction should be PAGE_SIZE.
>
> cheers
^ permalink raw reply
* Re: [PATCH v8 12/14] powerpc/vas: Return credits after handling fault
From: Haren Myneni @ 2020-03-25 3:35 UTC (permalink / raw)
To: Nicholas Piggin; +Cc: mikey, ajd, hch, oohall, sukadev, linuxppc-dev, herbert
In-Reply-To: <1584931224.oij3nwbo4z.astroid@bobo.none>
On Mon, 2020-03-23 at 12:44 +1000, Nicholas Piggin wrote:
> Haren Myneni's on March 19, 2020 4:19 pm:
> >
> > NX expects OS to return credit for send window after processing each
> > fault. Also credit has to be returned even for fault window.
>
> And this should be merged in the fault handler function.
credits are assigned and used per VAS window - default value is 1024 for
user space windows, and fault_fifo_size/CRb_SIZE for fault window.
When user space submits request, credit is taken on specific window (by
VAS). After successful processing of this request, NX return credit. In
case if NX sees fault, expects OS return credit for the corresponding
user space window after handling fault CRB.
Similarly NX takes credit on fault window after pasting fault CRB and
expects return credit after handling fault CRB. NX workbook has on
credits usage and How this credit system works.
Thought vas_return_credit() is unique function and added as separate
patch so that easy to review.
>
> >
> > Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
> > Signed-off-by: Haren Myneni <haren@linux.ibm.com>
> > ---
> > arch/powerpc/platforms/powernv/vas-fault.c | 9 +++++++++
> > arch/powerpc/platforms/powernv/vas-window.c | 17 +++++++++++++++++
> > arch/powerpc/platforms/powernv/vas.h | 1 +
> > 3 files changed, 27 insertions(+)
> >
> > diff --git a/arch/powerpc/platforms/powernv/vas-fault.c b/arch/powerpc/platforms/powernv/vas-fault.c
> > index 40e1de4..292f7ba 100644
> > --- a/arch/powerpc/platforms/powernv/vas-fault.c
> > +++ b/arch/powerpc/platforms/powernv/vas-fault.c
> > @@ -238,6 +238,10 @@ irqreturn_t vas_fault_thread_fn(int irq, void *data)
> > memcpy(crb, fifo, CRB_SIZE);
> > entry->stamp.nx.pswid = cpu_to_be32(FIFO_INVALID_ENTRY);
> > entry->ccw |= cpu_to_be32(CCW0_INVALID);
> > + /*
> > + * Return credit for the fault window.
> > + */
>
> None of the comments in this patch are useful.
>
> > + vas_return_credit(vinst->fault_win, 0);
>
> Can you use true/false for bools?
>
> > mutex_unlock(&vinst->mutex);
> >
> > pr_devel("VAS[%d] fault_fifo %p, fifo %p, fault_crbs %d\n",
> > @@ -267,6 +271,11 @@ irqreturn_t vas_fault_thread_fn(int irq, void *data)
> > }
> >
> > update_csb(window, crb);
> > + /*
> > + * Return credit for send window after processing
> > + * fault CRB.
> > + */
>
> Any chance of a little bit of explanation how the credit system works?
> Or is it in the code somewhere already?
Sure will add few comments on credit usage.
>
> I don't suppose there is a chance to batch credit updates with multiple
> faults? (maybe the MMIO is insignificant)
Yes, we return credit after processing each CRB. In the case of fault
window, NX can continue pasting fault CRB whenever the credit is
available.
Thanks
Haren
>
> > + vas_return_credit(window, 1);
> > } while (true);
> > }
> >
>
> Thanks,
> Nick
^ permalink raw reply
* [PATCH] mm/sparse: Fix kernel crash with pfn_section_valid check
From: Aneesh Kumar K.V @ 2020-03-25 3:19 UTC (permalink / raw)
To: linux-mm, akpm
Cc: Sachin Sant, Baoquan He, linux-kernel, Aneesh Kumar K.V,
linuxppc-dev
Fixes the below crash
BUG: Kernel NULL pointer dereference on read at 0x00000000
Faulting instruction address: 0xc000000000c3447c
Oops: Kernel access of bad area, sig: 11 [#1]
LE PAGE_SIZE=64K MMU=Hash SMP NR_CPUS=2048 NUMA pSeries
CPU: 11 PID: 7519 Comm: lt-ndctl Not tainted 5.6.0-rc7-autotest #1
...
NIP [c000000000c3447c] vmemmap_populated+0x98/0xc0
LR [c000000000088354] vmemmap_free+0x144/0x320
Call Trace:
section_deactivate+0x220/0x240
__remove_pages+0x118/0x170
arch_remove_memory+0x3c/0x150
memunmap_pages+0x1cc/0x2f0
devm_action_release+0x30/0x50
release_nodes+0x2f8/0x3e0
device_release_driver_internal+0x168/0x270
unbind_store+0x130/0x170
drv_attr_store+0x44/0x60
sysfs_kf_write+0x68/0x80
kernfs_fop_write+0x100/0x290
__vfs_write+0x3c/0x70
vfs_write+0xcc/0x240
ksys_write+0x7c/0x140
system_call+0x5c/0x68
With commit: d41e2f3bd546 ("mm/hotplug: fix hot remove failure in SPARSEMEM|!VMEMMAP case")
section_mem_map is set to NULL after depopulate_section_mem(). This
was done so that pfn_page() can work correctly with kernel config that disables
SPARSEMEM_VMEMMAP. With that config pfn_to_page does
__section_mem_map_addr(__sec) + __pfn;
where
static inline struct page *__section_mem_map_addr(struct mem_section *section)
{
unsigned long map = section->section_mem_map;
map &= SECTION_MAP_MASK;
return (struct page *)map;
}
Now with SPASEMEM_VMEMAP enabled, mem_section->usage->subsection_map is used to
check the pfn validity (pfn_valid()). Since section_deactivate release
mem_section->usage if a section is fully deactivated, pfn_valid() check after
a subsection_deactivate cause a kernel crash.
static inline int pfn_valid(unsigned long pfn)
{
...
return early_section(ms) || pfn_section_valid(ms, pfn);
}
where
static inline int pfn_section_valid(struct mem_section *ms, unsigned long pfn)
{
int idx = subsection_map_index(pfn);
return test_bit(idx, ms->usage->subsection_map);
}
Avoid this by clearing SECTION_HAS_MEM_MAP when mem_section->usage is freed.
Fixes: d41e2f3bd546 ("mm/hotplug: fix hot remove failure in SPARSEMEM|!VMEMMAP case")
Cc: Baoquan He <bhe@redhat.com>
Reported-by: Sachin Sant <sachinp@linux.vnet.ibm.com>
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
mm/sparse.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/mm/sparse.c b/mm/sparse.c
index aadb7298dcef..3012d1f3771a 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -781,6 +781,8 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages,
ms->usage = NULL;
}
memmap = sparse_decode_mem_map(ms->section_mem_map, section_nr);
+ /* Mark the section invalid */
+ ms->section_mem_map &= ~SECTION_HAS_MEM_MAP;
}
if (section_is_early && memmap)
--
2.25.1
^ permalink raw reply related
* [PATCH v2] powerpc/setup_64: Set cache-line-size based on cache-block-size
From: Chris Packham @ 2020-03-25 3:18 UTC (permalink / raw)
To: benh, paulus, mpe, christophe.leroy, tglx, oss
Cc: Hamish Martin, Chris Packham, linuxppc-dev, linux-kernel
In-Reply-To: <dd342c71e03e654a8786302d82f9662004418c6e.camel@alliedtelesis.co.nz>
If {i,d}-cache-block-size is set and {i,d}-cache-line-size is not, use
the block-size value for both. Per the devicetree spec cache-line-size
is only needed if it differs from the block size.
Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
It looks as though the bsizep = lsizep is not required per the spec but it's
probably safer to retain it.
Changes in v2:
- Scott pointed out that u-boot should be filling in the cache properties
(which it does). But it does not specify a cache-line-size because it
provides a cache-block-size and the spec says you don't have to if they are
the same. So the error is in the parsing not in the devicetree itself.
arch/powerpc/kernel/setup_64.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
index e05e6dd67ae6..dd8a238b54b8 100644
--- a/arch/powerpc/kernel/setup_64.c
+++ b/arch/powerpc/kernel/setup_64.c
@@ -516,6 +516,8 @@ static bool __init parse_cache_info(struct device_node *np,
lsizep = of_get_property(np, propnames[3], NULL);
if (bsizep == NULL)
bsizep = lsizep;
+ if (lsizep == NULL)
+ lsizep = bsizep;
if (lsizep != NULL)
lsize = be32_to_cpu(*lsizep);
if (bsizep != NULL)
--
2.25.1
^ permalink raw reply related
* Re: [PATCH 3/7] docs: fix broken references to text files
From: Stephen Boyd @ 2020-03-25 2:42 UTC (permalink / raw)
To: Linux Doc Mailing List, Mauro Carvalho Chehab
Cc: linux-arch, linux-nfs, kvm, Jonathan Corbet,
Mauro Carvalho Chehab, netdev, linux-unionfs, kvm-ppc, linux-mm,
dri-devel, linux-fsdevel, linuxppc-dev, kvmarm, linux-arm-kernel,
linux-rdma
In-Reply-To: <5cfeed6df208b74913312a1c97235ee615180f91.1582361737.git.mchehab+huawei@kernel.org>
Quoting Mauro Carvalho Chehab (2020-02-22 01:00:03)
> Several references got broken due to txt to ReST conversion.
>
> Several of them can be automatically fixed with:
>
> scripts/documentation-file-ref-check --fix
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> ---
> drivers/hwtracing/coresight/Kconfig | 2 +-
>
> diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
> index 6ff30e25af55..6d42a6d3766f 100644
> --- a/drivers/hwtracing/coresight/Kconfig
> +++ b/drivers/hwtracing/coresight/Kconfig
> @@ -107,7 +107,7 @@ config CORESIGHT_CPU_DEBUG
> can quickly get to know program counter (PC), secure state,
> exception level, etc. Before use debugging functionality, platform
> needs to ensure the clock domain and power domain are enabled
> - properly, please refer Documentation/trace/coresight-cpu-debug.rst
> + properly, please refer Documentation/trace/coresight/coresight-cpu-debug.rst
> for detailed description and the example for usage.
>
> endif
I ran into this today and almost sent a patch. Can you split this patch
up into more pieces and send it off to the respective subsystem
maintainers?
^ permalink raw reply
* Re: [PATCH v8 11/14] powerpc/vas: Do not use default credits for receive window
From: Haren Myneni @ 2020-03-25 3:04 UTC (permalink / raw)
To: Nicholas Piggin; +Cc: mikey, ajd, hch, oohall, sukadev, linuxppc-dev, herbert
In-Reply-To: <1584931140.88q80wx2at.astroid@bobo.none>
On Mon, 2020-03-23 at 12:40 +1000, Nicholas Piggin wrote:
> Haren Myneni's on March 19, 2020 4:18 pm:
> >
> > System checkstops if RxFIFO overruns with more requests than the
> > maximum possible number of CRBs allowed in FIFO at any time. So
> > max credits value (rxattr.wcreds_max) is set and is passed to
> > vas_rx_win_open() by the the driver.
>
> This seems like it should be a bug fix or merged in the NX fault
> window register patch or something.
Yes, it is a bug fix and can affect with any VAS windows, Not related to
NX fault window. Hence added as separate patch.
>
> Thanks,
> Nick
>
^ permalink raw reply
* Re: [PATCH v8 06/14] powerpc/vas: Setup thread IRQ handler per VAS instance
From: Haren Myneni @ 2020-03-25 2:58 UTC (permalink / raw)
To: Nicholas Piggin; +Cc: mikey, ajd, hch, oohall, sukadev, linuxppc-dev, herbert
In-Reply-To: <1584926621.6b1zd5e2im.astroid@bobo.none>
On Mon, 2020-03-23 at 12:23 +1000, Nicholas Piggin wrote:
> Haren Myneni's on March 19, 2020 4:15 pm:
> >
> > Setup thread IRQ handler per each VAS instance. When NX sees a fault
> > on CRB, kernel gets an interrupt and vas_fault_handler will be
> > executed to process fault CRBs. Read all valid CRBs from fault FIFO,
> > determine the corresponding send window from CRB and process fault
> > requests.
>
> Perhaps some more overview/why.
>
> "If NX encounters a translation error when accessing the CRB or one
> of addresses in the request, it raises an interrupt on the CPU to
> handle the fault.
>
>
> >
> > Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
> > Signed-off-by: Haren Myneni <haren@linux.ibm.com>
> > ---
> > arch/powerpc/platforms/powernv/vas-fault.c | 90 +++++++++++++++++++++++++++++
> > arch/powerpc/platforms/powernv/vas-window.c | 60 +++++++++++++++++++
> > arch/powerpc/platforms/powernv/vas.c | 49 +++++++++++++++-
> > arch/powerpc/platforms/powernv/vas.h | 6 ++
> > 4 files changed, 204 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/powerpc/platforms/powernv/vas-fault.c b/arch/powerpc/platforms/powernv/vas-fault.c
> > index 4044998..1c6d5cc 100644
> > --- a/arch/powerpc/platforms/powernv/vas-fault.c
> > +++ b/arch/powerpc/platforms/powernv/vas-fault.c
> > @@ -11,6 +11,7 @@
> > #include <linux/slab.h>
> > #include <linux/uaccess.h>
> > #include <linux/kthread.h>
> > +#include <linux/mmu_context.h>
> > #include <asm/icswx.h>
> >
> > #include "vas.h"
> > @@ -25,6 +26,95 @@
> > #define VAS_FAULT_WIN_FIFO_SIZE (4 << 20)
> >
> > /*
> > + * Process valid CRBs in fault FIFO.
> > + */
> > +irqreturn_t vas_fault_thread_fn(int irq, void *data)
>
> Are page faults the only reason why VAS would raise this interrupt? Is
> NX really the only possible user of this, so you can have NX specifics
> in here?
Yes, When NX sees page faults, it generates interrupts on specific VAS
instance. Right now NX is the only user. So trying to make as
generalized as possible.
>
> > +{
> > + struct vas_instance *vinst = data;
> > + struct coprocessor_request_block *crb, *entry;
> > + struct coprocessor_request_block buf;
> > + struct vas_window *window;
> > + unsigned long flags;
> > + void *fifo;
> > +
> > + crb = &buf;
>
> The below comment could just be moved to replace the one at the top of the
> function. Can you explain slightly more about how the faults work, and
> be more clear about what the coprocessor does versus what the host does? The
> use of VAS and NX is a bit confusing too. VAS doesn't interrupt with
> page faults, does it? NX has the page fault(s), and it requests VAS to
> interrupt the host?
NX is the one who raises interrupt on specific VAS port that is
registered with fault window. I will make it clear in the comment.
>
> > +
> > + /*
> > + * VAS can interrupt with multiple page faults. So process all
> > + * valid CRBs within fault FIFO until reaches invalid CRB.
>
> When NX encounters a fault accessing a memory address for a particular
> CRB, it updates the nx_fault_stamp field in the CRB (to what?), and
> copies the CRB to the fault FIFO memory, then raises an interrupt on the
> CPU (memory ordering on the store and load sides are provided how?). NX
> can store multiple faults into the FIFO per interrupt (does it proceed
> asynchronously after the interrupt? what's the stopping condition?).
User space fills CRB and sends request (CRB). NX processes the request
and update CSB (in CRB struct). If NX sees any page fault either on
request buffers or csb address, updates nx_fault_stamp struct (in user
space CRB) and pastes CRB in fault_fifo. Then raises interrupt on port
defined in fault_window (which is per VAS instance).
NX can raise single interrupt for multiple faults and can paste in fault
FIFO. But OS and NX use credits to control fault FIFO.
Initially FIFO_SIZE/CRB_SIZE credits are available for fault window. For
example, When NX pastes CRB in fault fifo, credits will be reduced by 1.
It can continue paste CRBs in FIFO until credits reached to 0.
When OS handles the fault CRB, increments index in FIFO and returns the
credit so that NX knows one more CRB slot is available.
struct coprocessor_request_block {
__be32 ccw;
__be32 flags;
__be64 csb_addr;
struct data_descriptor_entry source;
struct data_descriptor_entry target;
struct coprocessor_completion_block ccb;
union {
struct nx_fault_stamp nx;
u8 reserved[16];
} stamp;
u8 reserved[32];
struct coprocessor_status_block csb;
} __packed __aligned(CRB_ALIGN);
>
> When the CPU takes this interrupt, it reads the faulting CRBs from the
> FIFO and processes them in order until it reaches an invalid entry, FIFO
> empty (memory ordering how?). After each FIFO entry is processed, store
> to mark them as invalid. (How does NX resume after this?)
NX should do atomic copy of CRB in fault FIFO. But we had barrier (as
part of spin_unlock()) for the safe side which is suggested by HW team.
NX stops pasting CRBs if credits are not available and start when credit
is returned by OS after handling fault.
>
> How is the fault actually even "handled" here? Nothing seems to be
> actually done for them.
>
> > + * NX updates nx_fault_stamp in CRB and pastes in fault FIFO.
> > + * kernel retrives send window from parition send window ID
> > + * (pswid) in nx_fault_stamp. So pswid should be valid and
> > + * ccw[0] (in be) should be zero since this bit is reserved.
> > + * If user space touches this bit, NX returns with "CRB format
> > + * error".
> > + *
> > + * After reading CRB entry, invalidate it with pswid (set
> > + * 0xffffffff) and ccw[0] (set to 1).
>
> Al this is very busy and hard to decipher unambiguously. It should read
> more like a spec, a precise sequence of things happening.
Sure, will make it clear
>
> > + *
> > + * In case kernel receives another interrupt with different page
> > + * fault, CRBs are already processed by the previous handling. So
> > + * will be returned from this function when it sees invalid CRB.
> > + */
>
> Ambiguous at best. Assuming the NX continues running asynchronously and
> it's a usual kind of FIFO, I assume this means if the kernel gets
> another interrupt for a page fault corresponding to a FIFO entry that
> has already been processed by this fault.
>
>
> > + do {
>
> Can you make this 'while (true)' or 'for (;;)' so you don't need to go
> to the bottom to see it's an infinite loop.
>
> > + mutex_lock(&vinst->mutex);
>
> What does this protect? Threaded handlers don't run concurrently for the
> same request_threaded_irq?
We can remove this mutex_lock.
>
> > +
> > + spin_lock_irqsave(&vinst->fault_lock, flags);
> > + /*
> > + * Advance the fault fifo pointer to next CRB.
>
> The code below the comment isn't advancing the fault fifo pointer, it's
> grabbing the current one. The pointer (fault_crbs) is advanced later.
> You presumabl don't want to advance over an invalid entry.
Advancing to next entry which is invalid means start processing from
this entry in the next fault.
>
> > + * Use CRB_SIZE rather than sizeof(*crb) since the latter is
> > + * aligned to CRB_ALIGN (256) but the CRB written to by VAS is
> > + * only CRB_SIZE in len.
> > + */
> > + fifo = vinst->fault_fifo + (vinst->fault_crbs * CRB_SIZE);
> > + entry = fifo;
>
> Don't think you should really do this. It may be harmless in this case,
> but the compiler expects the type to be aligned. Make it another type,
> like coprocessor_fault_block or something?
Can add like "entry = (struct coprocessor_request_block *)fifo;
>
> > +
> > + if ((entry->stamp.nx.pswid == cpu_to_be32(FIFO_INVALID_ENTRY))
> > + || (entry->ccw & cpu_to_be32(CCW0_INVALID))) {
> > + atomic_set(&vinst->faults_in_progress, 0);
> > + spin_unlock_irqrestore(&vinst->fault_lock, flags);
>
> So what does the fault_lock protect? The only data it protects is
> faults_in_progress (vs the hard interrupt handler), which doesn't
> achieve anything by itself, so I guess it also prevents the hard irq
> handler from returning until the handler here has checked that the
> fault FIFO is empty then returns IRQ_HANDLED? That seems fine (so long
> as memory ordering details are okay), but it should be documented
> that way.
In the case of using default_handler, wakes up thread if it is not in
progress and checks whether interrupts are handled with in some
duration. If un_handled interrupts reached 99000, display bad_irq trace
and disables IRQ. In our case we do not have one fault per interrupt.
We ran some test case which continuously generates NX faults, the
handler thread is busy processing fault CRBs in FIFO and the later
interrupts are not handled.
So added own handler which checks whether fault_thread is in progress.
If so returned IRQ_HANDLED. fault_lock is used to check valid entry
section and check faults_in_progress in handler.
Added comment in vas_fault_handler().
>
> Also why is the hard handler in a different file? Makes it harder to
> see how this works at a glance.
OK, Will change. I thought IRQ handler per VAS is added in vas.c since
it has VAS initialization and vas_fault.c is only for fault handling.
>
> faults_in_progress does not have to be atomic because it's always
> accessed under the lock. And IMO it should have a better name. If the
> NX can be causing more faults as we go, it really doesn't indicate
> anything about faults. It's whether or not the threaded handler is
> currently woken and processing faults.
Used atomic since not using spin_lock/unlock for failing case when
window (from CRB) is not valid.
Sure, How about fault_thread_in_progress? As it is long name, used
faults_in_progress.
>
> > + mutex_unlock(&vinst->mutex);
> > + return IRQ_HANDLED;
> > + }
> > +
> > + spin_unlock_irqrestore(&vinst->fault_lock, flags);
> > + vinst->fault_crbs++;
> > + if (vinst->fault_crbs == (vinst->fault_fifo_size / CRB_SIZE))
> > + vinst->fault_crbs = 0;
> > +
> > + memcpy(crb, fifo, CRB_SIZE);
> > + entry->stamp.nx.pswid = cpu_to_be32(FIFO_INVALID_ENTRY);
> > + entry->ccw |= cpu_to_be32(CCW0_INVALID);
> > + mutex_unlock(&vinst->mutex);
> > +
> > + pr_devel("VAS[%d] fault_fifo %p, fifo %p, fault_crbs %d\n",
> > + vinst->vas_id, vinst->fault_fifo, fifo,
> > + vinst->fault_crbs);
> > +
> > + window = vas_pswid_to_window(vinst,
> > + be32_to_cpu(crb->stamp.nx.pswid));
> > +
> > + if (IS_ERR(window)) {
> > + /*
> > + * We got an interrupt about a specific send
> > + * window but we can't find that window and we can't
> > + * even clean it up (return credit).
> > + * But we should not get here.
> > + */
> > + pr_err("VAS[%d] fault_fifo %p, fifo %p, pswid 0x%x, fault_crbs %d bad CRB?\n",
> > + vinst->vas_id, vinst->fault_fifo, fifo,
> > + be32_to_cpu(crb->stamp.nx.pswid),
> > + vinst->fault_crbs);
> > +
> > + WARN_ON_ONCE(1);
> > + atomic_set(&vinst->faults_in_progress, 0);
> > + return IRQ_HANDLED;
>
> Shouldn't get here but you have a handler for it, so it should try to
> be graceful. Keep processing the rest of the FIFO until it's empty
> otherwise you have a missed wakeup here? Probably less code too, just
> delete the last 2 lines.
Derive window address from pswid which is pasted by NX. So if window
address is not valid means a bug, we should not be reached this. If
getting this failure, printed data in FIFO for around 10 CRBs. Not sure
whether proceeding with this failure. We may end up receiving lots of
messages on the console if we see similar failures in later CRBs.
Thinking may be disable IRQ so that kernel will not receive any
interrupts. I have not tested this case. I can change it to continue now
and Can I add disable IRQ as TODO?
Thanks for your detailed review.
>
> Thanks,
> Nick
>
> > + }
> > +
> > + } while (true);
> > +}
> > +
> > +/*
> > * Fault window is opened per VAS instance. NX pastes fault CRB in fault
> > * FIFO upon page faults.
> > */
> > diff --git a/arch/powerpc/platforms/powernv/vas-window.c b/arch/powerpc/platforms/powernv/vas-window.c
> > index 1783fa9..1f31c18 100644
> > --- a/arch/powerpc/platforms/powernv/vas-window.c
> > +++ b/arch/powerpc/platforms/powernv/vas-window.c
> > @@ -1040,6 +1040,15 @@ struct vas_window *vas_tx_win_open(int vasid, enum vas_cop_type cop,
> > }
> > } else {
> > /*
> > + * Interrupt hanlder or fault window setup failed. Means
> > + * NX can not generate fault for page fault. So not
> > + * opening for user space tx window.
> > + */
> > + if (!vinst->virq) {
> > + rc = -ENODEV;
> > + goto free_window;
> > + }
> > + /*
> > * A user mapping must ensure that context switch issues
> > * CP_ABORT for this thread.
> > */
> > @@ -1254,3 +1263,54 @@ int vas_win_close(struct vas_window *window)
> > return 0;
> > }
> > EXPORT_SYMBOL_GPL(vas_win_close);
> > +
> > +struct vas_window *vas_pswid_to_window(struct vas_instance *vinst,
> > + uint32_t pswid)
> > +{
> > + struct vas_window *window;
> > + int winid;
> > +
> > + if (!pswid) {
> > + pr_devel("%s: called for pswid 0!\n", __func__);
> > + return ERR_PTR(-ESRCH);
> > + }
> > +
> > + decode_pswid(pswid, NULL, &winid);
> > +
> > + if (winid >= VAS_WINDOWS_PER_CHIP)
> > + return ERR_PTR(-ESRCH);
> > +
> > + /*
> > + * If application closes the window before the hardware
> > + * returns the fault CRB, we should wait in vas_win_close()
> > + * for the pending requests. so the window must be active
> > + * and the process alive.
> > + *
> > + * If its a kernel process, we should not get any faults and
> > + * should not get here.
> > + */
> > + window = vinst->windows[winid];
> > +
> > + if (!window) {
> > + pr_err("PSWID decode: Could not find window for winid %d pswid %d vinst 0x%p\n",
> > + winid, pswid, vinst);
> > + return NULL;
> > + }
> > +
> > + /*
> > + * Do some sanity checks on the decoded window. Window should be
> > + * NX GZIP user send window. FTW windows should not incur faults
> > + * since their CRBs are ignored (not queued on FIFO or processed
> > + * by NX).
> > + */
> > + if (!window->tx_win || !window->user_win || !window->nx_win ||
> > + window->cop == VAS_COP_TYPE_FAULT ||
> > + window->cop == VAS_COP_TYPE_FTW) {
> > + pr_err("PSWID decode: id %d, tx %d, user %d, nx %d, cop %d\n",
> > + winid, window->tx_win, window->user_win,
> > + window->nx_win, window->cop);
> > + WARN_ON(1);
> > + }
> > +
> > + return window;
> > +}
> > diff --git a/arch/powerpc/platforms/powernv/vas.c b/arch/powerpc/platforms/powernv/vas.c
> > index 557c8e4..3d9ba58 100644
> > --- a/arch/powerpc/platforms/powernv/vas.c
> > +++ b/arch/powerpc/platforms/powernv/vas.c
> > @@ -14,6 +14,8 @@
> > #include <linux/of_platform.h>
> > #include <linux/of_address.h>
> > #include <linux/of.h>
> > +#include <linux/irqdomain.h>
> > +#include <linux/interrupt.h>
> > #include <asm/prom.h>
> > #include <asm/xive.h>
> >
> > @@ -24,9 +26,53 @@
> >
> > static DEFINE_PER_CPU(int, cpu_vas_id);
> >
> > +static irqreturn_t vas_fault_handler(int irq, void *dev_id)
> > +{
> > + struct vas_instance *vinst = dev_id;
> > + irqreturn_t ret = IRQ_WAKE_THREAD;
> > + unsigned long flags;
> > +
> > + /*
> > + * NX can generate an interrupt for multiple faults. So the
> > + * fault handler thread process all CRBs until finds invalid
> > + * entry. In case if NX sees continuous faults, it is possible
> > + * that the thread function entered with the first interrupt
> > + * can execute and process all valid CRBs.
> > + * So wake up thread only if the fault thread is not in progress.
> > + */
> > + spin_lock_irqsave(&vinst->fault_lock, flags);
> > +
> > + if (atomic_read(&vinst->faults_in_progress))
> > + ret = IRQ_HANDLED;
> > + else
> > + atomic_set(&vinst->faults_in_progress, 1);
> > +
> > + spin_unlock_irqrestore(&vinst->fault_lock, flags);
> > +
> > + return ret;
> > +}
> > +
> > static int vas_irq_fault_window_setup(struct vas_instance *vinst)
> > {
> > - return vas_setup_fault_window(vinst);
> > + char devname[64];
> > + int rc = 0;
> > +
> > + snprintf(devname, sizeof(devname), "vas-%d", vinst->vas_id);
> > + rc = request_threaded_irq(vinst->virq, vas_fault_handler,
> > + vas_fault_thread_fn, 0, devname, vinst);
> > +
> > + if (rc) {
> > + pr_err("VAS[%d]: Request IRQ(%d) failed with %d\n",
> > + vinst->vas_id, vinst->virq, rc);
> > + goto out;
> > + }
> > +
> > + rc = vas_setup_fault_window(vinst);
> > + if (rc)
> > + free_irq(vinst->virq, vinst);
> > +
> > +out:
> > + return rc;
> > }
> >
> > static int init_vas_instance(struct platform_device *pdev)
> > @@ -109,6 +155,7 @@ static int init_vas_instance(struct platform_device *pdev)
> > list_add(&vinst->node, &vas_instances);
> > mutex_unlock(&vas_mutex);
> >
> > + spin_lock_init(&vinst->fault_lock);
> > /*
> > * IRQ and fault handling setup is needed only for user space
> > * send windows.
> > diff --git a/arch/powerpc/platforms/powernv/vas.h b/arch/powerpc/platforms/powernv/vas.h
> > index 6c4baf5..ecae7cd 100644
> > --- a/arch/powerpc/platforms/powernv/vas.h
> > +++ b/arch/powerpc/platforms/powernv/vas.h
> > @@ -326,7 +326,10 @@ struct vas_instance {
> >
> > u64 irq_port;
> > int virq;
> > + int fault_crbs;
> > int fault_fifo_size;
> > + atomic_t faults_in_progress;
> > + spinlock_t fault_lock;
> > void *fault_fifo;
> > struct vas_window *fault_win; /* Fault window */
> >
> > @@ -424,6 +427,9 @@ struct vas_winctx {
> > extern void vas_window_init_dbgdir(struct vas_window *win);
> > extern void vas_window_free_dbgdir(struct vas_window *win);
> > extern int vas_setup_fault_window(struct vas_instance *vinst);
> > +extern irqreturn_t vas_fault_thread_fn(int irq, void *data);
> > +extern struct vas_window *vas_pswid_to_window(struct vas_instance *vinst,
> > + uint32_t pswid);
> >
> > static inline void vas_log_write(struct vas_window *win, char *name,
> > void *regptr, u64 val)
> > --
> > 1.8.3.1
> >
> >
> >
> >
^ permalink raw reply
* Re: [PATCH 1/4] hugetlbfs: add arch_hugetlb_valid_size
From: Longpeng (Mike, Cloud Infrastructure Service Product Dept.) @ 2020-03-25 2:58 UTC (permalink / raw)
To: Mike Kravetz, Dave Hansen, linux-mm, linux-kernel,
linux-arm-kernel, linuxppc-dev, linux-riscv, linux-s390,
sparclinux, linux-doc
Cc: Albert Ou, Andrew Morton, Vasily Gorbik, Jonathan Corbet,
Catalin Marinas, Dave Hansen, Heiko Carstens,
Christian Borntraeger, Ingo Molnar, Palmer Dabbelt, Paul Walmsley,
Paul Mackerras, Thomas Gleixner, Will Deacon, David S.Miller
In-Reply-To: <5aceea6a-8dc0-a44b-80c6-94511b5c75ca@oracle.com>
On 2020/3/19 6:52, Mike Kravetz wrote:
> On 3/18/20 3:15 PM, Dave Hansen wrote:
>> Hi Mike,
>>
>> The series looks like a great idea to me. One nit on the x86 bits,
>> though...
>>
>>> diff --git a/arch/x86/mm/hugetlbpage.c b/arch/x86/mm/hugetlbpage.c
>>> index 5bfd5aef5378..51e6208fdeec 100644
>>> --- a/arch/x86/mm/hugetlbpage.c
>>> +++ b/arch/x86/mm/hugetlbpage.c
>>> @@ -181,16 +181,25 @@ hugetlb_get_unmapped_area(struct file *file, unsigned long addr,
>>> #endif /* CONFIG_HUGETLB_PAGE */
>>>
>>> #ifdef CONFIG_X86_64
>>> +bool __init arch_hugetlb_valid_size(unsigned long long size)
>>> +{
>>> + if (size == PMD_SIZE)
>>> + return true;
>>> + else if (size == PUD_SIZE && boot_cpu_has(X86_FEATURE_GBPAGES))
>>> + return true;
>>> + else
>>> + return false;
>>> +}
>>
>> I'm pretty sure it's possible to have a system without 2M/PMD page
>> support. We even have a handy-dandy comment about it in
>> arch/x86/include/asm/required-features.h:
>>
>> #ifdef CONFIG_X86_64
>> #ifdef CONFIG_PARAVIRT
>> /* Paravirtualized systems may not have PSE or PGE available */
>> #define NEED_PSE 0
>> ...
>>
>> I *think* you need an X86_FEATURE_PSE check here to be totally correct.
>>
>> if (size == PMD_SIZE && cpu_feature_enabled(X86_FEATURE_PSE))
>> return true;
>>
>> BTW, I prefer cpu_feature_enabled() to boot_cpu_has() because it
>> includes disabled-features checking. I don't think any of it matters
>> for these specific features, but I generally prefer it on principle.
>
> Sounds good. I'll incorporate those changes into a v2, unless someone
> else with has a different opinion.
>
> BTW, this patch should not really change the way the code works today.
> It is mostly a movement of code. Unless I am missing something, the
> existing code will always allow setup of PMD_SIZE hugetlb pages.
>
Hi Mike,
Inspired by Dave's opinion, it seems the x86-specific hugepages_supported should
also need to use cpu_feature_enabled instead.
Also, I wonder if the hugepages_supported is correct ? There're two arch
specific hugepages_supported:
x86:
#define hugepages_supported() boot_cpu_has(X86_FEATURE_PSE)
and
s390:
#define hugepages_supported() (MACHINE_HAS_EDAT1)
Is it possible that x86 has X86_FEATURE_GBPAGES but hasn't X86_FEATURE_GBPAGES
or s390 has MACHINE_HAS_EDAT2 but hasn't MACHINE_HAS_EDAT1 ?
---
Regards,
Longpeng(Mike)
^ permalink raw reply
* Re: [RFC PATCH 0/3] Use per-CPU temporary mappings for patching
From: Andrew Donnellan @ 2020-03-25 2:51 UTC (permalink / raw)
To: Christopher M. Riedl, linuxppc-dev
In-Reply-To: <20200323045205.20314-1-cmr@informatik.wtf>
On 23/3/20 3:52 pm, Christopher M. Riedl wrote:
> When compiled with CONFIG_STRICT_KERNEL_RWX, the kernel must create
> temporary mappings when patching itself. These mappings temporarily
> override the strict RWX text protections to permit a write. Currently,
> powerpc allocates a per-CPU VM area for patching. Patching occurs as
> follows:
>
> 1. Map page of text to be patched to per-CPU VM area w/
> PAGE_KERNEL protection
> 2. Patch text
> 3. Remove the temporary mapping
>
> While the VM area is per-CPU, the mapping is actually inserted into the
> kernel page tables. Presumably, this could allow another CPU to access
> the normally write-protected text - either malicously or accidentally -
> via this same mapping if the address of the VM area is known. Ideally,
> the mapping should be kept local to the CPU doing the patching (or any
> other sensitive operations requiring temporarily overriding memory
> protections) [0].
>
> x86 introduced "temporary mm" structs which allow the creation of
> mappings local to a particular CPU [1]. This series intends to bring the
> notion of a temporary mm to powerpc and harden powerpc by using such a
> mapping for patching a kernel with strict RWX permissions.
>
> The first patch introduces the temporary mm struct and API for powerpc
> along with a new function to retrieve a current hw breakpoint.
>
> The second patch uses the `poking_init` init hook added by the x86
> patches to initialize a temporary mm and patching address. The patching
> address is randomized between 0 and DEFAULT_MAP_WINDOW-PAGE_SIZE. The
> upper limit is necessary due to how the hash MMU operates - by default
> the space above DEFAULT_MAP_WINDOW is not available. For now, both hash
> and radix randomize inside this range. The number of possible random
> addresses is dependent on PAGE_SIZE and limited by DEFAULT_MAP_WINDOW.
>
> Bits of entropy with 64K page size on BOOK3S_64:
>
> bits-o-entropy = log2(DEFAULT_MAP_WINDOW_USER64 / PAGE_SIZE)
>
> PAGE_SIZE=64K, DEFAULT_MAP_WINDOW_USER64=128TB
> bits-o-entropy = log2(128TB / 64K)
> bits-o-entropy = 31
>
> Currently, randomization occurs only once during initialization at boot.
>
> The third patch replaces the VM area with the temporary mm in the
> patching code. The page for patching has to be mapped PAGE_SHARED with
> the hash MMU since hash prevents the kernel from accessing userspace
> pages with PAGE_PRIVILEGED bit set. There is on-going work on my side to
> explore if this is actually necessary in the hash codepath.
>
> Testing so far is limited to booting on QEMU (power8 and power9 targets)
> and a POWER8 VM along with setting some simple xmon breakpoints (which
> makes use of code-patching). A POC lkdtm test is in-progress to actually
> exploit the existing vulnerability (ie. the mapping during patching is
> exposed in kernel page tables and accessible by other CPUS) - this will
> accompany a future v1 of this series.
>
> [0]: https://github.com/linuxppc/issues/issues/224
> [1]: https://lore.kernel.org/kernel-hardening/20190426232303.28381-1-nadav.amit@gmail.com/
>
> Christopher M. Riedl (3):
> powerpc/mm: Introduce temporary mm
> powerpc/lib: Initialize a temporary mm for code patching
> powerpc/lib: Use a temporary mm for code patching
>
> arch/powerpc/include/asm/debug.h | 1 +
> arch/powerpc/include/asm/mmu_context.h | 56 +++++++++-
> arch/powerpc/kernel/process.c | 5 +
> arch/powerpc/lib/code-patching.c | 140 ++++++++++++++-----------
> 4 files changed, 137 insertions(+), 65 deletions(-)
This series causes a build failure with ppc64e_defconfig
https://openpower.xyz/job/snowpatch/job/snowpatch-linux-sparse/16478//artifact/linux/report.txt
--
Andrew Donnellan OzLabs, ADL Canberra
ajd@linux.ibm.com IBM Australia Limited
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox