* Re: drivers binding to device node with multiple compatible strings
From: Rob Herring @ 2018-10-02 14:19 UTC (permalink / raw)
To: Yang-Leo Li
Cc: devicetree, linuxppc-dev, linux-kernel@vger.kernel.org,
Frank Rowand,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
In-Reply-To: <CADRPPNQam-QUfqDMvu42KS+50VaHfGPhCZj-1jRHQ-Od0fbxuQ@mail.gmail.com>
On Fri, Sep 28, 2018 at 4:01 PM Li Yang <leoyang.li@nxp.com> wrote:
>
> On Fri, Sep 28, 2018 at 3:07 PM Rob Herring <robh+dt@kernel.org> wrote:
> >
> > On Thu, Sep 27, 2018 at 5:25 PM Li Yang <leoyang.li@nxp.com> wrote:
> > >
> > > Hi Rob and Grant,
> > >
> > > Various device tree specs are recommending to include all the
> > > potential compatible strings in the device node, with the order from
> > > most specific to most general. But it looks like Linux kernel doesn't
> > > provide a way to bind the device to the most specific driver, however,
> > > the first registered compatible driver will be bound.
> > >
> > > As more and more generic drivers are added to the Linux kernel, they
> > > are competing with the more specific vendor drivers and causes problem
> > > when both are built into the kernel. I'm wondering if there is a
> > > generic solution (or in plan) to make the most specific driver bound
> > > to the device. Or we have to disable the more general driver or
> > > remove the more general compatible string from the device tree?
> >
> > It's been a known limitation for a long time. However, in practice it
> > doesn't seem to be a common problem. Perhaps folks just remove the
> > less specific compatible from their DT (though that's not ideal). For
> > most modern bindings, there's so many other resources beyond
> > compatible (clocks, resets, pinctrl, etc.) that there are few generic
> > drivers that can work.
> >
> > I guess if we want to fix this, we'd need to have weighted matching in
> > the driver core and unbind drivers when we get a better match. Though
> > it could get messy if the better driver probe fails. Then we've got to
> > rebind to the original driver.
>
> Probably we can populate the platform devices from device tree after
> the device_init phase? So that all built-in drivers are already
> registered when the devices are created and we can try find the best
> match in one go? For more specific loadable modules we probably need
> to unbind from the old driver and bind to the new one. But I agree
> with you that it could be messy.
>
> >
> > Do you have a specific case where you hit this?
>
> Maybe not a new issue but "snps,dw-pcie" is competing with various
> "fsl,<chip>-pcie" compatibles.
Having "snps,dw-pcie" is pretty useless IMO. There's lots of versions
of the IP and variations on how it is integrated by various SoC
vendors.
> Also a specific PHY
> "ethernet-phy-idAAAA.BBBB" with generic "ethernet-phy-ieee802.3-c45".
MDIO device probing works a bit differently, so I don't think there's
a problem there. Drivers for specific phys should have a
.match_phy_device() callback. I could be wrong as I'm not all that
familiar with the MDIO bus code.
Rob
^ permalink raw reply
* [PATCH v2 0/9] Fixes for SLB to C series
From: Nicholas Piggin @ 2018-10-02 14:27 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Aneesh Kumar K . V, Nicholas Piggin
This is another spin of the fixes. Also painfully re-discovered
that we need https://patchwork.ozlabs.org/patch/962327/, as it
prevents POWER8 NUMA from booting (initial stack SLB for the
!0 node CPUs gets cleared by the initial TLB flush without it,
and the SLB handler now uses the stack...)
Question is whether to revert the series and try again next time.
I'm inclined to maybe say revert because it's been holdig up the
tree a bit and also some of these patches like the PPR fixes are
pretty complicated and should really be done as pre-requisites rather
than fixes.
Thanks,
Nick
Nicholas Piggin (9):
Revert "powerpc/64s/hash: remove user SLB data from the paca"
powerpc/64: add struct int_regs to save additional registers on stack
powerpc/64: interrupts save PPR on stack rather than thread_struct
powerpc/64s/hash: Fix preloading of SLB entries
powerpc/64s/hash: add more barriers for slb preloading
powerpc/64s/hash: SLB fix preemption issues
powerpc/64s/hash: Fix false positive preempt warnings
powerpc/64s/hash: slb_flush_and_rebolt simplify
powerpc/64s/hash: add some slb debugging tests
arch/powerpc/include/asm/book3s/64/mmu-hash.h | 3 +-
arch/powerpc/include/asm/exception-64s.h | 9 +-
arch/powerpc/include/asm/paca.h | 16 ++
arch/powerpc/include/asm/processor.h | 12 +-
arch/powerpc/include/asm/ptrace.h | 18 +-
arch/powerpc/kernel/asm-offsets.c | 32 ++-
arch/powerpc/kernel/entry_64.S | 15 +-
arch/powerpc/kernel/paca.c | 22 ++
arch/powerpc/kernel/process.c | 54 ++---
arch/powerpc/kernel/ptrace.c | 4 +-
arch/powerpc/kernel/stacktrace.c | 2 +-
arch/powerpc/kernel/swsusp_asm64.S | 2 +-
arch/powerpc/mm/hash_utils_64.c | 46 +++-
arch/powerpc/mm/mmu_context.c | 3 +-
arch/powerpc/mm/slb.c | 203 ++++++++++++------
arch/powerpc/mm/slice.c | 29 ++-
16 files changed, 314 insertions(+), 156 deletions(-)
--
2.18.0
^ permalink raw reply
* [PATCH v2 1/9] Revert "powerpc/64s/hash: remove user SLB data from the paca"
From: Nicholas Piggin @ 2018-10-02 14:27 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Aneesh Kumar K . V, Nicholas Piggin
In-Reply-To: <20181002142759.6244-1-npiggin@gmail.com>
This reverts commit 8fed04d0f6aedf99b3d811ba58d38bb7f938a47a.
There are a number of problems with this patch, there are minor bugs
like vmalloc_sllp update no longer being picked up into pacas, but
more fundamentally the SLB flush can not be broadcast out to other
CPUs because it must be done in places where interrupts can be
disabled, which would deadlock.
Rather than try put some bandaids on this, the best fix is to revert
it. I think it's a worthwhile approach, but it should be solved by
demotig things eagerly at setup or map time, rather than in low level
hash faults. Most cases are rarely used by software or apply to very
old hardware.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/include/asm/book3s/64/mmu-hash.h | 1 -
arch/powerpc/include/asm/paca.h | 13 ++++++
arch/powerpc/kernel/asm-offsets.c | 9 ++++
arch/powerpc/kernel/paca.c | 22 +++++++++
arch/powerpc/mm/hash_utils_64.c | 46 ++++++++++++++-----
arch/powerpc/mm/mmu_context.c | 3 +-
arch/powerpc/mm/slb.c | 20 +-------
arch/powerpc/mm/slice.c | 29 ++++++++----
8 files changed, 103 insertions(+), 40 deletions(-)
diff --git a/arch/powerpc/include/asm/book3s/64/mmu-hash.h b/arch/powerpc/include/asm/book3s/64/mmu-hash.h
index bbeaf6adf93c..14e552ea5e52 100644
--- a/arch/powerpc/include/asm/book3s/64/mmu-hash.h
+++ b/arch/powerpc/include/asm/book3s/64/mmu-hash.h
@@ -503,7 +503,6 @@ struct slb_entry {
};
extern void slb_initialize(void);
-extern void core_flush_all_slbs(struct mm_struct *mm);
extern void slb_flush_and_rebolt(void);
void slb_flush_all_realmode(void);
void __slb_restore_bolted_realmode(void);
diff --git a/arch/powerpc/include/asm/paca.h b/arch/powerpc/include/asm/paca.h
index 6d6b3706232c..4838149ee07b 100644
--- a/arch/powerpc/include/asm/paca.h
+++ b/arch/powerpc/include/asm/paca.h
@@ -146,6 +146,18 @@ struct paca_struct {
struct tlb_core_data tcd;
#endif /* CONFIG_PPC_BOOK3E */
+#ifdef CONFIG_PPC_BOOK3S
+ mm_context_id_t mm_ctx_id;
+#ifdef CONFIG_PPC_MM_SLICES
+ unsigned char mm_ctx_low_slices_psize[BITS_PER_LONG / BITS_PER_BYTE];
+ unsigned char mm_ctx_high_slices_psize[SLICE_ARRAY_SIZE];
+ unsigned long mm_ctx_slb_addr_limit;
+#else
+ u16 mm_ctx_user_psize;
+ u16 mm_ctx_sllp;
+#endif
+#endif
+
/*
* then miscellaneous read-write fields
*/
@@ -248,6 +260,7 @@ struct paca_struct {
#endif /* CONFIG_PPC_BOOK3S_64 */
} ____cacheline_aligned;
+extern void copy_mm_to_paca(struct mm_struct *mm);
extern struct paca_struct **paca_ptrs;
extern void initialise_paca(struct paca_struct *new_paca, int cpu);
extern void setup_paca(struct paca_struct *new_paca);
diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c
index ba9d0fc98730..27c78b6c9909 100644
--- a/arch/powerpc/kernel/asm-offsets.c
+++ b/arch/powerpc/kernel/asm-offsets.c
@@ -180,6 +180,15 @@ int main(void)
OFFSET(PACAIRQSOFTMASK, paca_struct, irq_soft_mask);
OFFSET(PACAIRQHAPPENED, paca_struct, irq_happened);
OFFSET(PACA_FTRACE_ENABLED, paca_struct, ftrace_enabled);
+#ifdef CONFIG_PPC_BOOK3S
+ OFFSET(PACACONTEXTID, paca_struct, mm_ctx_id);
+#ifdef CONFIG_PPC_MM_SLICES
+ OFFSET(PACALOWSLICESPSIZE, paca_struct, mm_ctx_low_slices_psize);
+ OFFSET(PACAHIGHSLICEPSIZE, paca_struct, mm_ctx_high_slices_psize);
+ OFFSET(PACA_SLB_ADDR_LIMIT, paca_struct, mm_ctx_slb_addr_limit);
+ DEFINE(MMUPSIZEDEFSIZE, sizeof(struct mmu_psize_def));
+#endif /* CONFIG_PPC_MM_SLICES */
+#endif
#ifdef CONFIG_PPC_BOOK3E
OFFSET(PACAPGD, paca_struct, pgd);
diff --git a/arch/powerpc/kernel/paca.c b/arch/powerpc/kernel/paca.c
index 0cf84e30d1cd..0ee3e6d50f28 100644
--- a/arch/powerpc/kernel/paca.c
+++ b/arch/powerpc/kernel/paca.c
@@ -258,3 +258,25 @@ void __init free_unused_pacas(void)
printk(KERN_DEBUG "Allocated %u bytes for %u pacas\n",
paca_ptrs_size + paca_struct_size, nr_cpu_ids);
}
+
+void copy_mm_to_paca(struct mm_struct *mm)
+{
+#ifdef CONFIG_PPC_BOOK3S
+ mm_context_t *context = &mm->context;
+
+ get_paca()->mm_ctx_id = context->id;
+#ifdef CONFIG_PPC_MM_SLICES
+ VM_BUG_ON(!mm->context.slb_addr_limit);
+ get_paca()->mm_ctx_slb_addr_limit = mm->context.slb_addr_limit;
+ memcpy(&get_paca()->mm_ctx_low_slices_psize,
+ &context->low_slices_psize, sizeof(context->low_slices_psize));
+ memcpy(&get_paca()->mm_ctx_high_slices_psize,
+ &context->high_slices_psize, TASK_SLICE_ARRAY_SZ(mm));
+#else /* CONFIG_PPC_MM_SLICES */
+ get_paca()->mm_ctx_user_psize = context->user_psize;
+ get_paca()->mm_ctx_sllp = context->sllp;
+#endif
+#else /* !CONFIG_PPC_BOOK3S */
+ return;
+#endif
+}
diff --git a/arch/powerpc/mm/hash_utils_64.c b/arch/powerpc/mm/hash_utils_64.c
index 88c95dc8b141..f23a89d8e4ce 100644
--- a/arch/powerpc/mm/hash_utils_64.c
+++ b/arch/powerpc/mm/hash_utils_64.c
@@ -1088,16 +1088,16 @@ unsigned int hash_page_do_lazy_icache(unsigned int pp, pte_t pte, int trap)
}
#ifdef CONFIG_PPC_MM_SLICES
-static unsigned int get_psize(struct mm_struct *mm, unsigned long addr)
+static unsigned int get_paca_psize(unsigned long addr)
{
unsigned char *psizes;
unsigned long index, mask_index;
if (addr < SLICE_LOW_TOP) {
- psizes = mm->context.low_slices_psize;
+ psizes = get_paca()->mm_ctx_low_slices_psize;
index = GET_LOW_SLICE_INDEX(addr);
} else {
- psizes = mm->context.high_slices_psize;
+ psizes = get_paca()->mm_ctx_high_slices_psize;
index = GET_HIGH_SLICE_INDEX(addr);
}
mask_index = index & 0x1;
@@ -1105,9 +1105,9 @@ static unsigned int get_psize(struct mm_struct *mm, unsigned long addr)
}
#else
-unsigned int get_psize(struct mm_struct *mm, unsigned long addr)
+unsigned int get_paca_psize(unsigned long addr)
{
- return mm->context.user_psize;
+ return get_paca()->mm_ctx_user_psize;
}
#endif
@@ -1118,11 +1118,15 @@ unsigned int get_psize(struct mm_struct *mm, unsigned long addr)
#ifdef CONFIG_PPC_64K_PAGES
void demote_segment_4k(struct mm_struct *mm, unsigned long addr)
{
- if (get_psize(mm, addr) == MMU_PAGE_4K)
+ if (get_slice_psize(mm, addr) == MMU_PAGE_4K)
return;
slice_set_range_psize(mm, addr, 1, MMU_PAGE_4K);
copro_flush_all_slbs(mm);
- core_flush_all_slbs(mm);
+ if ((get_paca_psize(addr) != MMU_PAGE_4K) && (current->mm == mm)) {
+
+ copy_mm_to_paca(mm);
+ slb_flush_and_rebolt();
+ }
}
#endif /* CONFIG_PPC_64K_PAGES */
@@ -1187,6 +1191,22 @@ void hash_failure_debug(unsigned long ea, unsigned long access,
trap, vsid, ssize, psize, lpsize, pte);
}
+static void check_paca_psize(unsigned long ea, struct mm_struct *mm,
+ int psize, bool user_region)
+{
+ if (user_region) {
+ if (psize != get_paca_psize(ea)) {
+ copy_mm_to_paca(mm);
+ slb_flush_and_rebolt();
+ }
+ } else if (get_paca()->vmalloc_sllp !=
+ mmu_psize_defs[mmu_vmalloc_psize].sllp) {
+ get_paca()->vmalloc_sllp =
+ mmu_psize_defs[mmu_vmalloc_psize].sllp;
+ slb_vmalloc_update();
+ }
+}
+
/* Result code is:
* 0 - handled
* 1 - normal page fault
@@ -1219,7 +1239,7 @@ int hash_page_mm(struct mm_struct *mm, unsigned long ea,
rc = 1;
goto bail;
}
- psize = get_psize(mm, ea);
+ psize = get_slice_psize(mm, ea);
ssize = user_segment_size(ea);
vsid = get_user_vsid(&mm->context, ea, ssize);
break;
@@ -1307,6 +1327,9 @@ int hash_page_mm(struct mm_struct *mm, unsigned long ea,
WARN_ON(1);
}
#endif
+ if (current->mm == mm)
+ check_paca_psize(ea, mm, psize, user_region);
+
goto bail;
}
@@ -1341,14 +1364,15 @@ int hash_page_mm(struct mm_struct *mm, unsigned long ea,
"to 4kB pages because of "
"non-cacheable mapping\n");
psize = mmu_vmalloc_psize = MMU_PAGE_4K;
- slb_vmalloc_update();
copro_flush_all_slbs(mm);
- core_flush_all_slbs(mm);
}
}
#endif /* CONFIG_PPC_64K_PAGES */
+ if (current->mm == mm)
+ check_paca_psize(ea, mm, psize, user_region);
+
#ifdef CONFIG_PPC_64K_PAGES
if (psize == MMU_PAGE_64K)
rc = __hash_page_64K(ea, access, vsid, ptep, trap,
@@ -1436,7 +1460,7 @@ int __hash_page(unsigned long ea, unsigned long msr, unsigned long trap,
#ifdef CONFIG_PPC_MM_SLICES
static bool should_hash_preload(struct mm_struct *mm, unsigned long ea)
{
- int psize = get_psize(mm, ea);
+ int psize = get_slice_psize(mm, ea);
/* We only prefault standard pages for now */
if (unlikely(psize != mm->context.user_psize))
diff --git a/arch/powerpc/mm/mmu_context.c b/arch/powerpc/mm/mmu_context.c
index 28ae2835db3d..f84e14f23e50 100644
--- a/arch/powerpc/mm/mmu_context.c
+++ b/arch/powerpc/mm/mmu_context.c
@@ -54,7 +54,8 @@ void switch_mm_irqs_off(struct mm_struct *prev, struct mm_struct *next,
* MMU context id, which is then moved to SPRN_PID.
*
* For the hash MMU it is either the first load from slb_cache
- * in switch_slb(), and/or load of MMU context id.
+ * in switch_slb(), and/or the store of paca->mm_ctx_id in
+ * copy_mm_to_paca().
*
* On the other side, the barrier is in mm/tlb-radix.c for
* radix which orders earlier stores to clear the PTEs vs
diff --git a/arch/powerpc/mm/slb.c b/arch/powerpc/mm/slb.c
index b438220c4336..74c3b6f8e9b7 100644
--- a/arch/powerpc/mm/slb.c
+++ b/arch/powerpc/mm/slb.c
@@ -430,6 +430,8 @@ void switch_slb(struct task_struct *tsk, struct mm_struct *mm)
}
get_paca()->slb_used_bitmap = get_paca()->slb_kern_bitmap;
+ copy_mm_to_paca(mm);
+
/*
* We gradually age out SLBs after a number of context switches to
* reduce reload overhead of unused entries (like we do with FP/VEC
@@ -460,24 +462,6 @@ void slb_set_size(u16 size)
mmu_slb_size = size;
}
-static void cpu_flush_slb(void *parm)
-{
- struct mm_struct *mm = parm;
- unsigned long flags;
-
- if (mm != current->active_mm)
- return;
-
- local_irq_save(flags);
- slb_flush_and_rebolt();
- local_irq_restore(flags);
-}
-
-void core_flush_all_slbs(struct mm_struct *mm)
-{
- on_each_cpu(cpu_flush_slb, mm, 1);
-}
-
void slb_initialize(void)
{
unsigned long linear_llp, vmalloc_llp, io_llp;
diff --git a/arch/powerpc/mm/slice.c b/arch/powerpc/mm/slice.c
index fc5b3a1ec666..546dd07c8083 100644
--- a/arch/powerpc/mm/slice.c
+++ b/arch/powerpc/mm/slice.c
@@ -207,6 +207,23 @@ static bool slice_check_range_fits(struct mm_struct *mm,
return true;
}
+static void slice_flush_segments(void *parm)
+{
+#ifdef CONFIG_PPC64
+ struct mm_struct *mm = parm;
+ unsigned long flags;
+
+ if (mm != current->active_mm)
+ return;
+
+ copy_mm_to_paca(current->active_mm);
+
+ local_irq_save(flags);
+ slb_flush_and_rebolt();
+ local_irq_restore(flags);
+#endif
+}
+
static void slice_convert(struct mm_struct *mm,
const struct slice_mask *mask, int psize)
{
@@ -272,9 +289,6 @@ static void slice_convert(struct mm_struct *mm,
spin_unlock_irqrestore(&slice_convert_lock, flags);
copro_flush_all_slbs(mm);
-#ifdef CONFIG_PPC64
- core_flush_all_slbs(mm);
-#endif
}
/*
@@ -488,9 +502,8 @@ unsigned long slice_get_unmapped_area(unsigned long addr, unsigned long len,
* be already initialised beyond the old address limit.
*/
mm->context.slb_addr_limit = high_limit;
-#ifdef CONFIG_PPC64
- core_flush_all_slbs(mm);
-#endif
+
+ on_each_cpu(slice_flush_segments, mm, 1);
}
/* Sanity checks */
@@ -652,10 +665,8 @@ unsigned long slice_get_unmapped_area(unsigned long addr, unsigned long len,
(SLICE_NUM_HIGH &&
!bitmap_empty(potential_mask.high_slices, SLICE_NUM_HIGH))) {
slice_convert(mm, &potential_mask, psize);
-#ifdef CONFIG_PPC64
if (psize > MMU_PAGE_BASE)
- core_flush_all_slbs(mm);
-#endif
+ on_each_cpu(slice_flush_segments, mm, 1);
}
return newaddr;
--
2.18.0
^ permalink raw reply related
* [PATCH v2 2/9] powerpc/64: add struct int_regs to save additional registers on stack
From: Nicholas Piggin @ 2018-10-02 14:27 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Aneesh Kumar K . V, Nicholas Piggin
In-Reply-To: <20181002142759.6244-1-npiggin@gmail.com>
struct pt_regs is part of the user ABI and also the fundametal
structure for saving registers at interrupt.
The generic kernel code makes it difficult to completely decouple
these, but it's easy enough to add additional space required to save
more registers. Create a struct int_stack with struct pt_regs at
offset 0.
This is required for a following fix to save the PPR SPR on stack.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/include/asm/ptrace.h | 17 +++++++---
arch/powerpc/kernel/asm-offsets.c | 21 ++++++++-----
arch/powerpc/kernel/process.c | 52 ++++++++++++++++---------------
arch/powerpc/kernel/stacktrace.c | 2 +-
4 files changed, 53 insertions(+), 39 deletions(-)
diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
index 447cbd1bee99..1a98cd8c49f6 100644
--- a/arch/powerpc/include/asm/ptrace.h
+++ b/arch/powerpc/include/asm/ptrace.h
@@ -26,7 +26,6 @@
#include <uapi/asm/ptrace.h>
#include <asm/asm-const.h>
-
#ifdef __powerpc64__
/*
@@ -44,7 +43,7 @@
#define STACK_FRAME_OVERHEAD 112 /* size of minimum stack frame */
#define STACK_FRAME_LR_SAVE 2 /* Location of LR in stack frame */
#define STACK_FRAME_REGS_MARKER ASM_CONST(0x7265677368657265)
-#define STACK_INT_FRAME_SIZE (sizeof(struct pt_regs) + \
+#define STACK_INT_FRAME_SIZE (sizeof(struct int_regs) + \
STACK_FRAME_OVERHEAD + KERNEL_REDZONE_SIZE)
#define STACK_FRAME_MARKER 12
@@ -76,6 +75,11 @@
#ifndef __ASSEMBLY__
+struct int_regs {
+ /* pt_regs must be offset 0 so r1 + STACK_FRAME_OVERHEAD points to it */
+ struct pt_regs pt_regs;
+};
+
#define GET_IP(regs) ((regs)->nip)
#define GET_USP(regs) ((regs)->gpr[1])
#define GET_FP(regs) (0)
@@ -119,8 +123,11 @@ extern int ptrace_get_reg(struct task_struct *task, int regno,
extern int ptrace_put_reg(struct task_struct *task, int regno,
unsigned long data);
-#define current_pt_regs() \
- ((struct pt_regs *)((unsigned long)current_thread_info() + THREAD_SIZE) - 1)
+#define current_int_regs() \
+ ((struct int_regs *)((unsigned long)current_thread_info() + THREAD_SIZE) - 1)
+
+#define current_pt_regs() (¤t_int_regs()->pt_regs)
+
/*
* We use the least-significant bit of the trap field to indicate
* whether we have saved the full set of registers, or only a
@@ -137,7 +144,7 @@ extern int ptrace_put_reg(struct task_struct *task, int regno,
#define TRAP(regs) ((regs)->trap & ~0xF)
#ifdef __powerpc64__
#define NV_REG_POISON 0xdeadbeefdeadbeefUL
-#define CHECK_FULL_REGS(regs) BUG_ON(regs->trap & 1)
+#define CHECK_FULL_REGS(regs) BUG_ON((regs)->trap & 1)
#else
#define NV_REG_POISON 0xdeadbeef
#define CHECK_FULL_REGS(regs) \
diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c
index 27c78b6c9909..509a113f97dc 100644
--- a/arch/powerpc/kernel/asm-offsets.c
+++ b/arch/powerpc/kernel/asm-offsets.c
@@ -72,8 +72,13 @@
#include <asm/fixmap.h>
#endif
-#define STACK_PT_REGS_OFFSET(sym, val) \
- DEFINE(sym, STACK_FRAME_OVERHEAD + offsetof(struct pt_regs, val))
+#define STACK_INT_REGS_OFFSET(sym, val) \
+ DEFINE(sym, STACK_FRAME_OVERHEAD + offsetof(struct int_regs, val))
+
+#define STACK_PT_REGS_OFFSET(sym, val) \
+ DEFINE(sym, STACK_FRAME_OVERHEAD + \
+ offsetof(struct int_regs, pt_regs) + \
+ offsetof(struct pt_regs, val))
int main(void)
{
@@ -150,7 +155,7 @@ int main(void)
OFFSET(THREAD_CKFPSTATE, thread_struct, ckfp_state.fpr);
/* Local pt_regs on stack for Transactional Memory funcs. */
DEFINE(TM_FRAME_SIZE, STACK_FRAME_OVERHEAD +
- sizeof(struct pt_regs) + 16);
+ sizeof(struct int_regs) + 16);
#endif /* CONFIG_PPC_TRANSACTIONAL_MEM */
OFFSET(TI_FLAGS, thread_info, flags);
@@ -273,11 +278,11 @@ int main(void)
/* Interrupt register frame */
DEFINE(INT_FRAME_SIZE, STACK_INT_FRAME_SIZE);
- DEFINE(SWITCH_FRAME_SIZE, STACK_FRAME_OVERHEAD + sizeof(struct pt_regs));
+ DEFINE(SWITCH_FRAME_SIZE, STACK_FRAME_OVERHEAD + sizeof(struct int_regs));
#ifdef CONFIG_PPC64
/* Create extra stack space for SRR0 and SRR1 when calling prom/rtas. */
- DEFINE(PROM_FRAME_SIZE, STACK_FRAME_OVERHEAD + sizeof(struct pt_regs) + 16);
- DEFINE(RTAS_FRAME_SIZE, STACK_FRAME_OVERHEAD + sizeof(struct pt_regs) + 16);
+ DEFINE(PROM_FRAME_SIZE, STACK_FRAME_OVERHEAD + sizeof(struct int_regs) + 16);
+ DEFINE(RTAS_FRAME_SIZE, STACK_FRAME_OVERHEAD + sizeof(struct int_regs) + 16);
#endif /* CONFIG_PPC64 */
STACK_PT_REGS_OFFSET(GPR0, gpr[0]);
STACK_PT_REGS_OFFSET(GPR1, gpr[1]);
@@ -324,8 +329,8 @@ int main(void)
STACK_PT_REGS_OFFSET(SOFTE, softe);
/* These _only_ to be used with {PROM,RTAS}_FRAME_SIZE!!! */
- DEFINE(_SRR0, STACK_FRAME_OVERHEAD+sizeof(struct pt_regs));
- DEFINE(_SRR1, STACK_FRAME_OVERHEAD+sizeof(struct pt_regs)+8);
+ DEFINE(_SRR0, STACK_FRAME_OVERHEAD+sizeof(struct int_regs));
+ DEFINE(_SRR1, STACK_FRAME_OVERHEAD+sizeof(struct int_regs)+8);
#endif /* CONFIG_PPC64 */
#if defined(CONFIG_PPC32)
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 03c2e1f134bc..532e9a83e526 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -1627,7 +1627,7 @@ static void setup_ksp_vsid(struct task_struct *p, unsigned long sp)
int copy_thread(unsigned long clone_flags, unsigned long usp,
unsigned long kthread_arg, struct task_struct *p)
{
- struct pt_regs *childregs, *kregs;
+ struct int_regs *childregs, *kregs;
extern void ret_from_fork(void);
extern void ret_from_kernel_thread(void);
void (*f)(void);
@@ -1637,44 +1637,44 @@ int copy_thread(unsigned long clone_flags, unsigned long usp,
klp_init_thread_info(ti);
/* Copy registers */
- sp -= sizeof(struct pt_regs);
- childregs = (struct pt_regs *) sp;
+ sp -= sizeof(struct int_regs);
+ childregs = (struct int_regs *) sp;
if (unlikely(p->flags & PF_KTHREAD)) {
/* kernel thread */
- memset(childregs, 0, sizeof(struct pt_regs));
- childregs->gpr[1] = sp + sizeof(struct pt_regs);
+ memset(childregs, 0, sizeof(struct int_regs));
+ childregs->pt_regs.gpr[1] = sp + sizeof(struct int_regs);
/* function */
if (usp)
- childregs->gpr[14] = ppc_function_entry((void *)usp);
+ childregs->pt_regs.gpr[14] = ppc_function_entry((void *)usp);
#ifdef CONFIG_PPC64
clear_tsk_thread_flag(p, TIF_32BIT);
- childregs->softe = IRQS_ENABLED;
+ childregs->pt_regs.softe = IRQS_ENABLED;
#endif
- childregs->gpr[15] = kthread_arg;
+ childregs->pt_regs.gpr[15] = kthread_arg;
p->thread.regs = NULL; /* no user register state */
ti->flags |= _TIF_RESTOREALL;
f = ret_from_kernel_thread;
} else {
/* user thread */
- struct pt_regs *regs = current_pt_regs();
- CHECK_FULL_REGS(regs);
+ struct int_regs *regs = current_int_regs();
+ CHECK_FULL_REGS(®s->pt_regs);
*childregs = *regs;
if (usp)
- childregs->gpr[1] = usp;
- p->thread.regs = childregs;
- childregs->gpr[3] = 0; /* Result from fork() */
+ childregs->pt_regs.gpr[1] = usp;
+ p->thread.regs = &childregs->pt_regs;
+ childregs->pt_regs.gpr[3] = 0; /* Result from fork() */
if (clone_flags & CLONE_SETTLS) {
#ifdef CONFIG_PPC64
if (!is_32bit_task())
- childregs->gpr[13] = childregs->gpr[6];
+ childregs->pt_regs.gpr[13] = childregs->pt_regs.gpr[6];
else
#endif
- childregs->gpr[2] = childregs->gpr[6];
+ childregs->pt_regs.gpr[2] = childregs->pt_regs.gpr[6];
}
f = ret_from_fork;
}
- childregs->msr &= ~(MSR_FP|MSR_VEC|MSR_VSX);
+ childregs->pt_regs.msr &= ~(MSR_FP|MSR_VEC|MSR_VSX);
sp -= STACK_FRAME_OVERHEAD;
/*
@@ -1686,8 +1686,8 @@ int copy_thread(unsigned long clone_flags, unsigned long usp,
* system call, using the stack frame created above.
*/
((unsigned long *)sp)[0] = 0;
- sp -= sizeof(struct pt_regs);
- kregs = (struct pt_regs *) sp;
+ sp -= sizeof(struct int_regs);
+ kregs = (struct int_regs *) sp;
sp -= STACK_FRAME_OVERHEAD;
p->thread.ksp = sp;
#ifdef CONFIG_PPC32
@@ -1715,7 +1715,7 @@ int copy_thread(unsigned long clone_flags, unsigned long usp,
p->thread.tidr = 0;
#endif
- kregs->nip = ppc_function_entry(f);
+ kregs->pt_regs.nip = ppc_function_entry(f);
return 0;
}
@@ -1739,8 +1739,8 @@ void start_thread(struct pt_regs *regs, unsigned long start, unsigned long sp)
* set. Do it now.
*/
if (!current->thread.regs) {
- struct pt_regs *regs = task_stack_page(current) + THREAD_SIZE;
- current->thread.regs = regs - 1;
+ struct int_regs *iregs = task_stack_page(current) + THREAD_SIZE;
+ current->thread.regs = &(iregs - 1)->pt_regs;
}
#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
@@ -2106,11 +2106,13 @@ void show_stack(struct task_struct *tsk, unsigned long *stack)
*/
if (validate_sp(sp, tsk, STACK_INT_FRAME_SIZE)
&& stack[STACK_FRAME_MARKER] == STACK_FRAME_REGS_MARKER) {
- struct pt_regs *regs = (struct pt_regs *)
- (sp + STACK_FRAME_OVERHEAD);
- lr = regs->link;
+ struct int_regs *regs;
+ regs = (struct int_regs *)(sp + STACK_FRAME_OVERHEAD);
+ lr = regs->pt_regs.link;
printk("--- interrupt: %lx at %pS\n LR = %pS\n",
- regs->trap, (void *)regs->nip, (void *)lr);
+ regs->pt_regs.trap,
+ (void *)regs->pt_regs.nip,
+ (void *)lr);
firstframe = 1;
}
diff --git a/arch/powerpc/kernel/stacktrace.c b/arch/powerpc/kernel/stacktrace.c
index e2c50b55138f..463dedc4d664 100644
--- a/arch/powerpc/kernel/stacktrace.c
+++ b/arch/powerpc/kernel/stacktrace.c
@@ -120,7 +120,7 @@ save_stack_trace_tsk_reliable(struct task_struct *tsk,
* an unreliable stack trace until it's been
* _switch()'ed to for the first time.
*/
- stack_end -= STACK_FRAME_OVERHEAD + sizeof(struct pt_regs);
+ stack_end -= STACK_FRAME_OVERHEAD + sizeof(struct int_regs);
} else {
/*
* idle tasks have a custom stack layout,
--
2.18.0
^ permalink raw reply related
* [PATCH v2 3/9] powerpc/64: interrupts save PPR on stack rather than thread_struct
From: Nicholas Piggin @ 2018-10-02 14:27 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Aneesh Kumar K . V, Nicholas Piggin
In-Reply-To: <20181002142759.6244-1-npiggin@gmail.com>
PPR is the odd register out when it comes to interrupt handling,
it is saved in current->thread.ppr while all others are saved on
the stack.
The difficulty with this is that accessing thread.ppr can cause a
SLB fault, but the SLB fault handler implementation in C change had
assumed the normal exception entry handlers would not cause an SLB
fault.
Fix this by allocating room in the interrupt stack to save PPR.
Fixes: 5e46e29e6a97 ("powerpc/64s/hash: convert SLB miss handlers to C")
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/include/asm/exception-64s.h | 9 ++++-----
arch/powerpc/include/asm/processor.h | 12 +++++++-----
arch/powerpc/include/asm/ptrace.h | 1 +
arch/powerpc/kernel/asm-offsets.c | 2 +-
arch/powerpc/kernel/entry_64.S | 15 +++++----------
arch/powerpc/kernel/process.c | 2 +-
arch/powerpc/kernel/ptrace.c | 4 ++--
7 files changed, 21 insertions(+), 24 deletions(-)
diff --git a/arch/powerpc/include/asm/exception-64s.h b/arch/powerpc/include/asm/exception-64s.h
index 47578b79f0fb..3b4767ed3ec5 100644
--- a/arch/powerpc/include/asm/exception-64s.h
+++ b/arch/powerpc/include/asm/exception-64s.h
@@ -228,11 +228,10 @@
* PPR save/restore macros used in exceptions_64s.S
* Used for P7 or later processors
*/
-#define SAVE_PPR(area, ra, rb) \
+#define SAVE_PPR(area, ra) \
BEGIN_FTR_SECTION_NESTED(940) \
- ld ra,PACACURRENT(r13); \
- ld rb,area+EX_PPR(r13); /* Read PPR from paca */ \
- std rb,TASKTHREADPPR(ra); \
+ ld ra,area+EX_PPR(r13); /* Read PPR from paca */ \
+ std ra,_PPR(r1); \
END_FTR_SECTION_NESTED(CPU_FTR_HAS_PPR,CPU_FTR_HAS_PPR,940)
#define RESTORE_PPR_PACA(area, ra) \
@@ -500,7 +499,7 @@ END_FTR_SECTION_NESTED(ftr,ftr,943)
3: EXCEPTION_PROLOG_COMMON_1(); \
beq 4f; /* if from kernel mode */ \
ACCOUNT_CPU_USER_ENTRY(r13, r9, r10); \
- SAVE_PPR(area, r9, r10); \
+ SAVE_PPR(area, r9); \
4: EXCEPTION_PROLOG_COMMON_2(area) \
EXCEPTION_PROLOG_COMMON_3(n) \
ACCOUNT_STOLEN_TIME
diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h
index 350c584ca179..07251598056c 100644
--- a/arch/powerpc/include/asm/processor.h
+++ b/arch/powerpc/include/asm/processor.h
@@ -32,9 +32,9 @@
/* Default SMT priority is set to 3. Use 11- 13bits to save priority. */
#define PPR_PRIORITY 3
#ifdef __ASSEMBLY__
-#define INIT_PPR (PPR_PRIORITY << 50)
+#define DEFAULT_PPR (PPR_PRIORITY << 50)
#else
-#define INIT_PPR ((u64)PPR_PRIORITY << 50)
+#define DEFAULT_PPR ((u64)PPR_PRIORITY << 50)
#endif /* __ASSEMBLY__ */
#endif /* CONFIG_PPC64 */
@@ -247,7 +247,11 @@ struct thread_struct {
#ifdef CONFIG_PPC64
unsigned long ksp_vsid;
#endif
- struct pt_regs *regs; /* Pointer to saved register state */
+ union {
+ struct int_regs *iregs; /* Pointer to saved register state */
+ struct pt_regs *regs; /* Pointer to saved register state */
+ };
+
mm_segment_t addr_limit; /* for get_fs() validation */
#ifdef CONFIG_BOOKE
/* BookE base exception scratch space; align on cacheline */
@@ -342,7 +346,6 @@ struct thread_struct {
* onwards.
*/
int dscr_inherit;
- unsigned long ppr; /* used to save/restore SMT priority */
unsigned long tidr;
#endif
#ifdef CONFIG_PPC_BOOK3S_64
@@ -390,7 +393,6 @@ struct thread_struct {
.regs = (struct pt_regs *)INIT_SP - 1, /* XXX bogus, I think */ \
.addr_limit = KERNEL_DS, \
.fpexc_mode = 0, \
- .ppr = INIT_PPR, \
.fscr = FSCR_TAR | FSCR_EBB \
}
#endif
diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
index 1a98cd8c49f6..9a5a1cc85bd0 100644
--- a/arch/powerpc/include/asm/ptrace.h
+++ b/arch/powerpc/include/asm/ptrace.h
@@ -78,6 +78,7 @@
struct int_regs {
/* pt_regs must be offset 0 so r1 + STACK_FRAME_OVERHEAD points to it */
struct pt_regs pt_regs;
+ unsigned long ppr;
};
#define GET_IP(regs) ((regs)->nip)
diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c
index 509a113f97dc..56241174ea94 100644
--- a/arch/powerpc/kernel/asm-offsets.c
+++ b/arch/powerpc/kernel/asm-offsets.c
@@ -88,7 +88,6 @@ int main(void)
#ifdef CONFIG_PPC64
DEFINE(SIGSEGV, SIGSEGV);
DEFINE(NMI_MASK, NMI_MASK);
- OFFSET(TASKTHREADPPR, task_struct, thread.ppr);
#else
OFFSET(THREAD_INFO, task_struct, stack);
DEFINE(THREAD_INFO_GAP, _ALIGN_UP(sizeof(struct thread_info), 16));
@@ -283,6 +282,7 @@ int main(void)
/* Create extra stack space for SRR0 and SRR1 when calling prom/rtas. */
DEFINE(PROM_FRAME_SIZE, STACK_FRAME_OVERHEAD + sizeof(struct int_regs) + 16);
DEFINE(RTAS_FRAME_SIZE, STACK_FRAME_OVERHEAD + sizeof(struct int_regs) + 16);
+ STACK_INT_REGS_OFFSET(_PPR, ppr);
#endif /* CONFIG_PPC64 */
STACK_PT_REGS_OFFSET(GPR0, gpr[0]);
STACK_PT_REGS_OFFSET(GPR1, gpr[1]);
diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
index 77a888bfcb53..ce448f753d2e 100644
--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S
@@ -386,10 +386,9 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
4: /* Anything else left to do? */
BEGIN_FTR_SECTION
- lis r3,INIT_PPR@highest /* Set thread.ppr = 3 */
- ld r10,PACACURRENT(r13)
+ lis r3,DEFAULT_PPR@highest /* Set default PPR */
sldi r3,r3,32 /* bits 11-13 are used for ppr */
- std r3,TASKTHREADPPR(r10)
+ std r3,_PPR(r1)
END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
andi. r0,r9,(_TIF_SYSCALL_DOTRACE|_TIF_SINGLESTEP)
@@ -938,12 +937,6 @@ fast_exception_return:
andi. r0,r3,MSR_RI
beq- .Lunrecov_restore
- /* Load PPR from thread struct before we clear MSR:RI */
-BEGIN_FTR_SECTION
- ld r2,PACACURRENT(r13)
- ld r2,TASKTHREADPPR(r2)
-END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
-
/*
* Clear RI before restoring r13. If we are returning to
* userspace and we take an exception after restoring r13,
@@ -964,7 +957,9 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
andi. r0,r3,MSR_PR
beq 1f
BEGIN_FTR_SECTION
- mtspr SPRN_PPR,r2 /* Restore PPR */
+ /* Restore PPR */
+ ld r2,_PPR(r1)
+ mtspr SPRN_PPR,r2
END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
ACCOUNT_CPU_USER_EXIT(r13, r2, r4)
REST_GPR(13, r1)
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 532e9a83e526..18a14fb8f759 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -1711,7 +1711,7 @@ int copy_thread(unsigned long clone_flags, unsigned long usp,
p->thread.dscr = mfspr(SPRN_DSCR);
}
if (cpu_has_feature(CPU_FTR_HAS_PPR))
- p->thread.ppr = INIT_PPR;
+ childregs->ppr = DEFAULT_PPR;
p->thread.tidr = 0;
#endif
diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index 9667666eb18e..57c04176f686 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -1609,7 +1609,7 @@ static int ppr_get(struct task_struct *target,
void *kbuf, void __user *ubuf)
{
return user_regset_copyout(&pos, &count, &kbuf, &ubuf,
- &target->thread.ppr, 0, sizeof(u64));
+ &target->thread.iregs->ppr, 0, sizeof(u64));
}
static int ppr_set(struct task_struct *target,
@@ -1618,7 +1618,7 @@ static int ppr_set(struct task_struct *target,
const void *kbuf, const void __user *ubuf)
{
return user_regset_copyin(&pos, &count, &kbuf, &ubuf,
- &target->thread.ppr, 0, sizeof(u64));
+ &target->thread.iregs->ppr, 0, sizeof(u64));
}
static int dscr_get(struct task_struct *target,
--
2.18.0
^ permalink raw reply related
* [PATCH v2 4/9] powerpc/64s/hash: Fix preloading of SLB entries
From: Nicholas Piggin @ 2018-10-02 14:27 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Aneesh Kumar K . V, Nicholas Piggin
In-Reply-To: <20181002142759.6244-1-npiggin@gmail.com>
slb_setup_new_exec and preload_new_slb_context assumed if an address
missed the preload cache, then it would not be in the SLB and could
be added. This is wrong if the preload cache has started to overflow.
This can cause SLB multi-hits on user addresses.
That assumption came from an earlier version of the patch which
cleared the preload cache when copying the task, but even that was
technically wrong because some user accesses occur before these
preloads, and the preloads themselves could overflow the cache
depending on the size.
Fixes: 89ca4e126a3f ("powerpc/64s/hash: Add a SLB preload cache")
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/mm/slb.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/arch/powerpc/mm/slb.c b/arch/powerpc/mm/slb.c
index 74c3b6f8e9b7..a5bd3c02d432 100644
--- a/arch/powerpc/mm/slb.c
+++ b/arch/powerpc/mm/slb.c
@@ -311,6 +311,13 @@ void slb_setup_new_exec(void)
struct mm_struct *mm = current->mm;
unsigned long exec = 0x10000000;
+ /*
+ * preload cache can only be used to determine whether a SLB
+ * entry exists if it does not start to overflow.
+ */
+ if (ti->slb_preload_nr + 2 > SLB_PRELOAD_NR)
+ return;
+
/*
* We have no good place to clear the slb preload cache on exec,
* flush_thread is about the earliest arch hook but that happens
@@ -345,6 +352,10 @@ void preload_new_slb_context(unsigned long start, unsigned long sp)
struct mm_struct *mm = current->mm;
unsigned long heap = mm->start_brk;
+ /* see above */
+ if (ti->slb_preload_nr + 3 > SLB_PRELOAD_NR)
+ return;
+
/* Userspace entry address. */
if (!is_kernel_addr(start)) {
if (preload_add(ti, start))
--
2.18.0
^ permalink raw reply related
* [PATCH v2 5/9] powerpc/64s/hash: add more barriers for slb preloading
From: Nicholas Piggin @ 2018-10-02 14:27 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Aneesh Kumar K . V, Nicholas Piggin
In-Reply-To: <20181002142759.6244-1-npiggin@gmail.com>
In several places, more care has to be taken to prevent compiler or
CPU re-ordering of memory accesses into critical sections that must
not take SLB faults. Barriers are explained in the comments.
Fixes: 5e46e29e6a97 ("powerpc/64s/hash: convert SLB miss handlers to C")
Fixes: 89ca4e126a3f ("powerpc/64s/hash: Add a SLB preload cache")
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/mm/slb.c | 37 +++++++++++++++++++++++++++++++------
1 file changed, 31 insertions(+), 6 deletions(-)
diff --git a/arch/powerpc/mm/slb.c b/arch/powerpc/mm/slb.c
index a5bd3c02d432..8c38659f1b6b 100644
--- a/arch/powerpc/mm/slb.c
+++ b/arch/powerpc/mm/slb.c
@@ -344,6 +344,9 @@ void slb_setup_new_exec(void)
if (preload_add(ti, mm->mmap_base))
slb_allocate_user(mm, mm->mmap_base);
}
+
+ /* see switch_slb */
+ asm volatile("isync" : : : "memory");
}
void preload_new_slb_context(unsigned long start, unsigned long sp)
@@ -373,6 +376,9 @@ void preload_new_slb_context(unsigned long start, unsigned long sp)
if (preload_add(ti, heap))
slb_allocate_user(mm, heap);
}
+
+ /* see switch_slb */
+ asm volatile("isync" : : : "memory");
}
@@ -389,6 +395,7 @@ void switch_slb(struct task_struct *tsk, struct mm_struct *mm)
* which would update the slb_cache/slb_cache_ptr fields in the PACA.
*/
hard_irq_disable();
+ asm volatile("isync" : : : "memory");
if (cpu_has_feature(CPU_FTR_ARCH_300)) {
/*
* SLBIA IH=3 invalidates all Class=1 SLBEs and their
@@ -396,7 +403,7 @@ void switch_slb(struct task_struct *tsk, struct mm_struct *mm)
* switch_slb wants. So ARCH_300 does not use the slb
* cache.
*/
- asm volatile("isync ; " PPC_SLBIA(3)" ; isync");
+ asm volatile(PPC_SLBIA(3));
} else {
unsigned long offset = get_paca()->slb_cache_ptr;
@@ -404,7 +411,6 @@ void switch_slb(struct task_struct *tsk, struct mm_struct *mm)
offset <= SLB_CACHE_ENTRIES) {
unsigned long slbie_data = 0;
- asm volatile("isync" : : : "memory");
for (i = 0; i < offset; i++) {
/* EA */
slbie_data = (unsigned long)
@@ -419,7 +425,6 @@ void switch_slb(struct task_struct *tsk, struct mm_struct *mm)
if (!cpu_has_feature(CPU_FTR_ARCH_207S) && offset == 1)
asm volatile("slbie %0" : : "r" (slbie_data));
- asm volatile("isync" : : : "memory");
} else {
struct slb_shadow *p = get_slb_shadow();
unsigned long ksp_esid_data =
@@ -427,8 +432,7 @@ void switch_slb(struct task_struct *tsk, struct mm_struct *mm)
unsigned long ksp_vsid_data =
be64_to_cpu(p->save_area[KSTACK_INDEX].vsid);
- asm volatile("isync\n"
- PPC_SLBIA(1) "\n"
+ asm volatile(PPC_SLBIA(1) "\n"
"slbmte %0,%1\n"
"isync"
:: "r"(ksp_vsid_data),
@@ -466,6 +470,13 @@ void switch_slb(struct task_struct *tsk, struct mm_struct *mm)
slb_allocate_user(mm, ea);
}
+
+ /*
+ * Synchronize slbmte preloads with possible subsequent user memory
+ * address accesses by the kernel (user mode won't happen until
+ * rfid, which is safe).
+ */
+ asm volatile("isync" : : : "memory");
}
void slb_set_size(u16 size)
@@ -609,6 +620,17 @@ static long slb_insert_entry(unsigned long ea, unsigned long context,
if (!vsid)
return -EFAULT;
+ /*
+ * There must not be a kernel SLB fault in alloc_slb_index or before
+ * slbmte here or the allocation bitmaps could get out of whack with
+ * the SLB.
+ *
+ * User SLB faults or preloads take this path which might get inlined
+ * into the caller, so add compiler barriers here to ensure unsafe
+ * memory accesses do not come between
+ */
+ barrier();
+
index = alloc_slb_index(kernel);
vsid_data = __mk_vsid_data(vsid, ssize, flags);
@@ -617,10 +639,13 @@ static long slb_insert_entry(unsigned long ea, unsigned long context,
/*
* No need for an isync before or after this slbmte. The exception
* we enter with and the rfid we exit with are context synchronizing.
- * Also we only handle user segments here.
+ * User preloads should add isync afterwards in case the kernel
+ * accesses user memory before it returns to userspace with rfid.
*/
asm volatile("slbmte %0, %1" : : "r" (vsid_data), "r" (esid_data));
+ barrier();
+
if (!kernel)
slb_cache_update(esid_data);
--
2.18.0
^ permalink raw reply related
* [PATCH v2 6/9] powerpc/64s/hash: SLB fix preemption issues
From: Nicholas Piggin @ 2018-10-02 14:27 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Aneesh Kumar K . V, Nicholas Piggin
In-Reply-To: <20181002142759.6244-1-npiggin@gmail.com>
slb_setup_new_exec and preload_new_slb_context run with interrupts
and preemption enabled, which can be corrupted by re-entrant interrupt
or process touching SLB preload cache or SLB allocator.
Hard disable interrupts over these regions.
Fixes: 5e46e29e6a97 ("powerpc/64s/hash: convert SLB miss handlers to C")
Fixes: 89ca4e126a3f ("powerpc/64s/hash: Add a SLB preload cache")
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/mm/slb.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/arch/powerpc/mm/slb.c b/arch/powerpc/mm/slb.c
index 8c38659f1b6b..b5a33da875b5 100644
--- a/arch/powerpc/mm/slb.c
+++ b/arch/powerpc/mm/slb.c
@@ -311,6 +311,8 @@ void slb_setup_new_exec(void)
struct mm_struct *mm = current->mm;
unsigned long exec = 0x10000000;
+ WARN_ON(irqs_disabled());
+
/*
* preload cache can only be used to determine whether a SLB
* entry exists if it does not start to overflow.
@@ -318,6 +320,8 @@ void slb_setup_new_exec(void)
if (ti->slb_preload_nr + 2 > SLB_PRELOAD_NR)
return;
+ hard_irq_disable();
+
/*
* We have no good place to clear the slb preload cache on exec,
* flush_thread is about the earliest arch hook but that happens
@@ -347,6 +351,8 @@ void slb_setup_new_exec(void)
/* see switch_slb */
asm volatile("isync" : : : "memory");
+
+ local_irq_enable();
}
void preload_new_slb_context(unsigned long start, unsigned long sp)
@@ -355,10 +361,14 @@ void preload_new_slb_context(unsigned long start, unsigned long sp)
struct mm_struct *mm = current->mm;
unsigned long heap = mm->start_brk;
+ WARN_ON(irqs_disabled());
+
/* see above */
if (ti->slb_preload_nr + 3 > SLB_PRELOAD_NR)
return;
+ hard_irq_disable();
+
/* Userspace entry address. */
if (!is_kernel_addr(start)) {
if (preload_add(ti, start))
@@ -379,6 +389,8 @@ void preload_new_slb_context(unsigned long start, unsigned long sp)
/* see switch_slb */
asm volatile("isync" : : : "memory");
+
+ local_irq_enable();
}
--
2.18.0
^ permalink raw reply related
* [PATCH v2 7/9] powerpc/64s/hash: Fix false positive preempt warnings
From: Nicholas Piggin @ 2018-10-02 14:27 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Aneesh Kumar K . V, Nicholas Piggin
In-Reply-To: <20181002142759.6244-1-npiggin@gmail.com>
Fixes: 5e46e29e6a97 ("powerpc/64s/hash: convert SLB miss handlers to C")
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/mm/slb.c | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/arch/powerpc/mm/slb.c b/arch/powerpc/mm/slb.c
index b5a33da875b5..c273d0baf4fc 100644
--- a/arch/powerpc/mm/slb.c
+++ b/arch/powerpc/mm/slb.c
@@ -558,21 +558,21 @@ static void slb_cache_update(unsigned long esid_data)
/*
* Now update slb cache entries
*/
- slb_cache_index = get_paca()->slb_cache_ptr;
+ slb_cache_index = local_paca->slb_cache_ptr;
if (slb_cache_index < SLB_CACHE_ENTRIES) {
/*
* We have space in slb cache for optimized switch_slb().
* Top 36 bits from esid_data as per ISA
*/
- get_paca()->slb_cache[slb_cache_index++] = esid_data >> 28;
- get_paca()->slb_cache_ptr++;
+ local_paca->slb_cache[slb_cache_index++] = esid_data >> 28;
+ local_paca->slb_cache_ptr++;
} else {
/*
* Our cache is full and the current cache content strictly
* doesn't indicate the active SLB conents. Bump the ptr
* so that switch_slb() will ignore the cache.
*/
- get_paca()->slb_cache_ptr = SLB_CACHE_ENTRIES + 1;
+ local_paca->slb_cache_ptr = SLB_CACHE_ENTRIES + 1;
}
}
@@ -596,24 +596,24 @@ static enum slb_index alloc_slb_index(bool kernel)
* POWER7/8/9 have 32 SLB entries, this could be expanded if a
* future CPU has more.
*/
- if (get_paca()->slb_used_bitmap != U32_MAX) {
- index = ffz(get_paca()->slb_used_bitmap);
- get_paca()->slb_used_bitmap |= 1U << index;
+ if (local_paca->slb_used_bitmap != U32_MAX) {
+ index = ffz(local_paca->slb_used_bitmap);
+ local_paca->slb_used_bitmap |= 1U << index;
if (kernel)
- get_paca()->slb_kern_bitmap |= 1U << index;
+ local_paca->slb_kern_bitmap |= 1U << index;
} else {
/* round-robin replacement of slb starting at SLB_NUM_BOLTED. */
- index = get_paca()->stab_rr;
+ index = local_paca->stab_rr;
if (index < (mmu_slb_size - 1))
index++;
else
index = SLB_NUM_BOLTED;
- get_paca()->stab_rr = index;
+ local_paca->stab_rr = index;
if (index < 32) {
if (kernel)
- get_paca()->slb_kern_bitmap |= 1U << index;
+ local_paca->slb_kern_bitmap |= 1U << index;
else
- get_paca()->slb_kern_bitmap &= ~(1U << index);
+ local_paca->slb_kern_bitmap &= ~(1U << index);
}
}
BUG_ON(index < SLB_NUM_BOLTED);
--
2.18.0
^ permalink raw reply related
* [PATCH v2 8/9] powerpc/64s/hash: slb_flush_and_rebolt simplify
From: Nicholas Piggin @ 2018-10-02 14:27 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Aneesh Kumar K . V, Nicholas Piggin
In-Reply-To: <20181002142759.6244-1-npiggin@gmail.com>
slb_flush_and_rebolt is misleading, it is called in virtual mode, so
it can not possibly change the stack, so it should not be touching the
shadow area. And since vmalloc is no longer bolted, it should not
change any bolted mappings at all.
Change the name to slb_flush_and_restore_bolted, and have it just load
the kernel stack from what's currently in the shadow SLB area.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/include/asm/book3s/64/mmu-hash.h | 2 +-
arch/powerpc/kernel/swsusp_asm64.S | 2 +-
arch/powerpc/mm/hash_utils_64.c | 4 +-
arch/powerpc/mm/slb.c | 46 +++++++------------
arch/powerpc/mm/slice.c | 2 +-
5 files changed, 21 insertions(+), 35 deletions(-)
diff --git a/arch/powerpc/include/asm/book3s/64/mmu-hash.h b/arch/powerpc/include/asm/book3s/64/mmu-hash.h
index 14e552ea5e52..60cda8fb0677 100644
--- a/arch/powerpc/include/asm/book3s/64/mmu-hash.h
+++ b/arch/powerpc/include/asm/book3s/64/mmu-hash.h
@@ -503,7 +503,7 @@ struct slb_entry {
};
extern void slb_initialize(void);
-extern void slb_flush_and_rebolt(void);
+void slb_flush_and_restore_bolted(void);
void slb_flush_all_realmode(void);
void __slb_restore_bolted_realmode(void);
void slb_restore_bolted_realmode(void);
diff --git a/arch/powerpc/kernel/swsusp_asm64.S b/arch/powerpc/kernel/swsusp_asm64.S
index f83bf6f72cb0..185216becb8b 100644
--- a/arch/powerpc/kernel/swsusp_asm64.S
+++ b/arch/powerpc/kernel/swsusp_asm64.S
@@ -262,7 +262,7 @@ END_FW_FTR_SECTION_IFCLR(FW_FEATURE_LPAR)
addi r1,r1,-128
#ifdef CONFIG_PPC_BOOK3S_64
- bl slb_flush_and_rebolt
+ bl slb_flush_and_restore_bolted
#endif
bl do_after_copyback
addi r1,r1,128
diff --git a/arch/powerpc/mm/hash_utils_64.c b/arch/powerpc/mm/hash_utils_64.c
index f23a89d8e4ce..974d0332e1d8 100644
--- a/arch/powerpc/mm/hash_utils_64.c
+++ b/arch/powerpc/mm/hash_utils_64.c
@@ -1125,7 +1125,7 @@ void demote_segment_4k(struct mm_struct *mm, unsigned long addr)
if ((get_paca_psize(addr) != MMU_PAGE_4K) && (current->mm == mm)) {
copy_mm_to_paca(mm);
- slb_flush_and_rebolt();
+ slb_flush_and_restore_bolted();
}
}
#endif /* CONFIG_PPC_64K_PAGES */
@@ -1197,7 +1197,7 @@ static void check_paca_psize(unsigned long ea, struct mm_struct *mm,
if (user_region) {
if (psize != get_paca_psize(ea)) {
copy_mm_to_paca(mm);
- slb_flush_and_rebolt();
+ slb_flush_and_restore_bolted();
}
} else if (get_paca()->vmalloc_sllp !=
mmu_psize_defs[mmu_vmalloc_psize].sllp) {
diff --git a/arch/powerpc/mm/slb.c b/arch/powerpc/mm/slb.c
index c273d0baf4fc..e3c5280f43bd 100644
--- a/arch/powerpc/mm/slb.c
+++ b/arch/powerpc/mm/slb.c
@@ -115,8 +115,6 @@ void __slb_restore_bolted_realmode(void)
/*
* Insert the bolted entries into an empty SLB.
- * This is not the same as rebolt because the bolted segments are not
- * changed, just loaded from the shadow area.
*/
void slb_restore_bolted_realmode(void)
{
@@ -135,12 +133,15 @@ void slb_flush_all_realmode(void)
asm volatile("slbmte %0,%0; slbia" : : "r" (0));
}
-void slb_flush_and_rebolt(void)
+/*
+ * This flushes non-bolted entries, it can be run in virtual mode. Must
+ * be called with interrupts disabled.
+ */
+void slb_flush_and_restore_bolted(void)
{
- /* If you change this make sure you change SLB_NUM_BOLTED
- * and PR KVM appropriately too. */
- unsigned long linear_llp, lflags;
- unsigned long ksp_esid_data, ksp_vsid_data;
+ struct slb_shadow *p = get_slb_shadow();
+
+ BUILD_BUG_ON(SLB_NUM_BOLTED != 2);
WARN_ON(!irqs_disabled());
@@ -150,30 +151,12 @@ void slb_flush_and_rebolt(void)
*/
hard_irq_disable();
- linear_llp = mmu_psize_defs[mmu_linear_psize].sllp;
- lflags = SLB_VSID_KERNEL | linear_llp;
-
- ksp_esid_data = mk_esid_data(get_paca()->kstack, mmu_kernel_ssize, KSTACK_INDEX);
- if ((ksp_esid_data & ~0xfffffffUL) <= PAGE_OFFSET) {
- ksp_esid_data &= ~SLB_ESID_V;
- ksp_vsid_data = 0;
- slb_shadow_clear(KSTACK_INDEX);
- } else {
- /* Update stack entry; others don't change */
- slb_shadow_update(get_paca()->kstack, mmu_kernel_ssize, lflags, KSTACK_INDEX);
- ksp_vsid_data =
- be64_to_cpu(get_slb_shadow()->save_area[KSTACK_INDEX].vsid);
- }
-
- /* We need to do this all in asm, so we're sure we don't touch
- * the stack between the slbia and rebolting it. */
asm volatile("isync\n"
"slbia\n"
- /* Slot 1 - kernel stack */
- "slbmte %0,%1\n"
- "isync"
- :: "r"(ksp_vsid_data),
- "r"(ksp_esid_data)
+ "slbmte %0, %1\n"
+ "isync\n"
+ :: "r" (be64_to_cpu(p->save_area[KSTACK_INDEX].vsid)),
+ "r" (be64_to_cpu(p->save_area[KSTACK_INDEX].esid))
: "memory");
get_paca()->slb_cache_ptr = 0;
@@ -254,7 +237,10 @@ void slb_dump_contents(struct slb_entry *slb_ptr)
void slb_vmalloc_update(void)
{
- slb_flush_and_rebolt();
+ /*
+ * vmalloc is not bolted, so just have to flush non-bolted.
+ */
+ slb_flush_and_restore_bolted();
}
static bool preload_hit(struct thread_info *ti, unsigned long esid)
diff --git a/arch/powerpc/mm/slice.c b/arch/powerpc/mm/slice.c
index 546dd07c8083..50ba3d0456a5 100644
--- a/arch/powerpc/mm/slice.c
+++ b/arch/powerpc/mm/slice.c
@@ -219,7 +219,7 @@ static void slice_flush_segments(void *parm)
copy_mm_to_paca(current->active_mm);
local_irq_save(flags);
- slb_flush_and_rebolt();
+ slb_flush_and_restore_bolted();
local_irq_restore(flags);
#endif
}
--
2.18.0
^ permalink raw reply related
* [PATCH v2 9/9] powerpc/64s/hash: add some slb debugging tests
From: Nicholas Piggin @ 2018-10-02 14:27 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Aneesh Kumar K . V, Nicholas Piggin
In-Reply-To: <20181002142759.6244-1-npiggin@gmail.com>
This adds CONFIG_DEBUG_VM checks to ensure:
- The kernel stack is in the SLB after it's flushed and bolted.
- We don't insert an SLB for an address that is aleady in the SLB.
- The kernel SLB miss handler does not take an SLB miss.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/include/asm/paca.h | 3 ++
arch/powerpc/mm/slb.c | 53 +++++++++++++++++++++++++++++++--
2 files changed, 53 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/include/asm/paca.h b/arch/powerpc/include/asm/paca.h
index 4838149ee07b..2bfbd8811b72 100644
--- a/arch/powerpc/include/asm/paca.h
+++ b/arch/powerpc/include/asm/paca.h
@@ -115,6 +115,9 @@ struct paca_struct {
u16 vmalloc_sllp;
u8 slb_cache_ptr;
u8 stab_rr; /* stab/slb round-robin counter */
+#ifdef CONFIG_DEBUG_VM
+ u8 in_kernel_slb_handler;
+#endif
u32 slb_used_bitmap; /* Bitmaps for first 32 SLB entries. */
u32 slb_kern_bitmap;
u32 slb_cache[SLB_CACHE_ENTRIES];
diff --git a/arch/powerpc/mm/slb.c b/arch/powerpc/mm/slb.c
index e3c5280f43bd..703c344f6751 100644
--- a/arch/powerpc/mm/slb.c
+++ b/arch/powerpc/mm/slb.c
@@ -58,6 +58,30 @@ static inline unsigned long mk_vsid_data(unsigned long ea, int ssize,
return __mk_vsid_data(get_kernel_vsid(ea, ssize), ssize, flags);
}
+static void assert_slb_exists(unsigned long ea)
+{
+#ifdef CONFIG_DEBUG_VM
+ unsigned long tmp;
+
+ WARN_ON_ONCE(mfmsr() & MSR_EE);
+
+ asm volatile("slbfee. %0, %1" : "=r"(tmp) : "r"(ea) : "cr0");
+ WARN_ON(tmp == 0);
+#endif
+}
+
+static void assert_slb_notexists(unsigned long ea)
+{
+#ifdef CONFIG_DEBUG_VM
+ unsigned long tmp;
+
+ WARN_ON_ONCE(mfmsr() & MSR_EE);
+
+ asm volatile("slbfee. %0, %1" : "=r"(tmp) : "r"(ea) : "cr0");
+ WARN_ON(tmp != 0);
+#endif
+}
+
static inline void slb_shadow_update(unsigned long ea, int ssize,
unsigned long flags,
enum slb_index index)
@@ -90,6 +114,7 @@ static inline void create_shadowed_slbe(unsigned long ea, int ssize,
*/
slb_shadow_update(ea, ssize, flags, index);
+ assert_slb_notexists(ea);
asm volatile("slbmte %0,%1" :
: "r" (mk_vsid_data(ea, ssize, flags)),
"r" (mk_esid_data(ea, ssize, index))
@@ -111,6 +136,8 @@ void __slb_restore_bolted_realmode(void)
: "r" (be64_to_cpu(p->save_area[index].vsid)),
"r" (be64_to_cpu(p->save_area[index].esid)));
}
+
+ assert_slb_exists(local_paca->kstack);
}
/*
@@ -158,6 +185,7 @@ void slb_flush_and_restore_bolted(void)
:: "r" (be64_to_cpu(p->save_area[KSTACK_INDEX].vsid)),
"r" (be64_to_cpu(p->save_area[KSTACK_INDEX].esid))
: "memory");
+ assert_slb_exists(get_paca()->kstack);
get_paca()->slb_cache_ptr = 0;
@@ -410,9 +438,17 @@ void switch_slb(struct task_struct *tsk, struct mm_struct *mm)
unsigned long slbie_data = 0;
for (i = 0; i < offset; i++) {
- /* EA */
- slbie_data = (unsigned long)
+ unsigned long ea;
+
+ ea = (unsigned long)
get_paca()->slb_cache[i] << SID_SHIFT;
+ /*
+ * Could assert_slb_exists here, but hypervisor
+ * or machine check could have come in and
+ * removed the entry at this point.
+ */
+
+ slbie_data = ea;
slbie_data |= user_segment_size(slbie_data)
<< SLBIE_SSIZE_SHIFT;
slbie_data |= SLBIE_C; /* user slbs have C=1 */
@@ -640,6 +676,7 @@ static long slb_insert_entry(unsigned long ea, unsigned long context,
* User preloads should add isync afterwards in case the kernel
* accesses user memory before it returns to userspace with rfid.
*/
+ assert_slb_notexists(ea);
asm volatile("slbmte %0, %1" : : "r" (vsid_data), "r" (esid_data));
barrier();
@@ -740,7 +777,17 @@ long do_slb_fault(struct pt_regs *regs, unsigned long ea)
* if they go via fast_exception_return too.
*/
if (id >= KERNEL_REGION_ID) {
- return slb_allocate_kernel(ea, id);
+ long err;
+#ifdef CONFIG_DEBUG_VM
+ /* Catch recursive kernel SLB faults. */
+ BUG_ON(local_paca->in_kernel_slb_handler);
+ local_paca->in_kernel_slb_handler = 1;
+#endif
+ err = slb_allocate_kernel(ea, id);
+#ifdef CONFIG_DEBUG_VM
+ local_paca->in_kernel_slb_handler = 0;
+#endif
+ return err;
} else {
struct mm_struct *mm = current->mm;
long err;
--
2.18.0
^ permalink raw reply related
* Re: [PATCH] migration/mm: Add WARN_ON to try_offline_node
From: Michael Bringmann @ 2018-10-02 14:51 UTC (permalink / raw)
To: Tyrel Datwyler, Michal Hocko
Cc: Thomas Falcon, Kees Cook, Mathieu Malaterre, linux-kernel,
Nicholas Piggin, Pavel Tatashin, linux-mm,
Mauricio Faria de Oliveira, Juliet Kim, Thiago Jung Bauermann,
Nathan Fontenot, Andrew Morton, YASUAKI ISHIMATSU, linuxppc-dev,
Dan Williams, Oscar Salvador
In-Reply-To: <bdbca329-7d35-0535-1737-94a06a19ae28@linux.vnet.ibm.com>
See below.
On 10/01/2018 06:20 PM, Tyrel Datwyler wrote:
> On 10/01/2018 01:27 PM, Michal Hocko wrote:
>> On Mon 01-10-18 13:56:25, Michael Bringmann wrote:
>>> In some LPAR migration scenarios, device-tree modifications are
>>> made to the affinity of the memory in the system. For instance,
>>> it may occur that memory is installed to nodes 0,3 on a source
>>> system, and to nodes 0,2 on a target system. Node 2 may not
>>> have been initialized/allocated on the target system.
>>>
>>> After migration, if a RTAS PRRN memory remove is made to a
>>> memory block that was in node 3 on the source system, then
>>> try_offline_node tries to remove it from node 2 on the target.
>>> The NODE_DATA(2) block would not be initialized on the target,
>>> and there is no validation check in the current code to prevent
>>> the use of a NULL pointer.
>>
>> I am not familiar with ppc and the above doesn't really help me
>> much. Sorry about that. But from the above it is not clear to me whether
>> it is the caller which does something unexpected or the hotplug code
>> being not robust enough. From your changelog I would suggest the later
>> but why don't we see the same problem for other archs? Is this a problem
>> of unrolling a partial failure?
>>
>> dlpar_remove_lmb does the following
>>
>> nid = memory_add_physaddr_to_nid(lmb->base_addr);
>>
>> remove_memory(nid, lmb->base_addr, block_sz);
>>
>> /* Update memory regions for memory remove */
>> memblock_remove(lmb->base_addr, block_sz);
>>
>> dlpar_remove_device_tree_lmb(lmb);
>>
>> Is the whole operation correct when remove_memory simply backs off
>> silently. Why don't we have to care about memblock resp
>> dlpar_remove_device_tree_lmb parts? In other words how come the physical
>> memory range is valid while the node association is not?
>>
>
> I think the issue here is a race between the LPM code updating affinity and PRRN events being processed. Does your other patch[1] not fix the issue? Or is it that the LPM affinity updates don't do any of the initialization/allocation you mentioned?
This patch addresses the specific case where PRRN changes to CPU or memory are occurring on a system that also observes affinity changes during migration. Yes, there is a race condition -- if the PRRN events reliably occurred before the device-tree was updated, this error would not occur. However, they overlap or occur after the changes during most LPMs observed.
When the device-tree affinity attributes have changed for memory, the 'nid' affinity calculated points to a different node for the memory block than the one used to install it, previously on the source system. The newly calculated 'nid' affinity may not yet be initialized on the target system. The current memory tracking mechanisms do not record the node to which a memory block was associated when it was added. Nathan is looking at adding this feature to the new implementation of LMBs, but it is not there yet, and won't be present in earlier kernels without backporting a significant number of changes.
My other patch[1] is more intended to address locking and CPU update dependencies between PRRN changes and RTAS requests which did not necessarly involve memory updates. The node to memory association problem after migration would be present even with this issue resolved.
>
> -Tyrel
>
> [1] https://lore.kernel.org/linuxppc-dev/20181001185603.11373.61650.stgit@ltcalpine2-lp9.aus.stglabs.ibm.com/T/#u
>
Michael
--
Michael W. Bringmann
Linux Technology Center
IBM Corporation
Tie-Line 363-5196
External: (512) 286-5196
Cell: (512) 466-0650
mwb@linux.vnet.ibm.com
^ permalink raw reply
* Re: [PATCH] migration/mm: Add WARN_ON to try_offline_node
From: Michal Hocko @ 2018-10-02 14:59 UTC (permalink / raw)
To: Michael Bringmann
Cc: Thomas Falcon, Kees Cook, Mathieu Malaterre, Pavel Tatashin,
Nicholas Piggin, linux-kernel, linux-mm,
Mauricio Faria de Oliveira, Juliet Kim, Tyrel Datwyler,
Thiago Jung Bauermann, Nathan Fontenot, Andrew Morton,
YASUAKI ISHIMATSU, linuxppc-dev, Dan Williams, Oscar Salvador
In-Reply-To: <df95f828-1963-d8b9-ab58-6d29d2d152d2@linux.vnet.ibm.com>
On Tue 02-10-18 09:51:40, Michael Bringmann wrote:
[...]
> When the device-tree affinity attributes have changed for memory,
> the 'nid' affinity calculated points to a different node for the
> memory block than the one used to install it, previously on the
> source system. The newly calculated 'nid' affinity may not yet
> be initialized on the target system. The current memory tracking
> mechanisms do not record the node to which a memory block was
> associated when it was added. Nathan is looking at adding this
> feature to the new implementation of LMBs, but it is not there
> yet, and won't be present in earlier kernels without backporting a
> significant number of changes.
Then the patch you have proposed here just papers over a real issue, no?
IIUC then you simply do not remove the memory if you lose the race.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH] migration/mm: Add WARN_ON to try_offline_node
From: Michael Bringmann @ 2018-10-02 15:14 UTC (permalink / raw)
To: Michal Hocko
Cc: Thomas Falcon, Kees Cook, Mathieu Malaterre, Pavel Tatashin,
Nicholas Piggin, linux-kernel, linux-mm,
Mauricio Faria de Oliveira, Juliet Kim, Tyrel Datwyler,
Thiago Jung Bauermann, Nathan Fontenot, Andrew Morton,
YASUAKI ISHIMATSU, linuxppc-dev, Dan Williams, Oscar Salvador
In-Reply-To: <20181002145922.GZ18290@dhcp22.suse.cz>
On 10/02/2018 09:59 AM, Michal Hocko wrote:
> On Tue 02-10-18 09:51:40, Michael Bringmann wrote:
> [...]
>> When the device-tree affinity attributes have changed for memory,
>> the 'nid' affinity calculated points to a different node for the
>> memory block than the one used to install it, previously on the
>> source system. The newly calculated 'nid' affinity may not yet
>> be initialized on the target system. The current memory tracking
>> mechanisms do not record the node to which a memory block was
>> associated when it was added. Nathan is looking at adding this
>> feature to the new implementation of LMBs, but it is not there
>> yet, and won't be present in earlier kernels without backporting a
>> significant number of changes.
>
> Then the patch you have proposed here just papers over a real issue, no?
> IIUC then you simply do not remove the memory if you lose the race.
The problem occurs when removing memory after an affinity change references a node that was previously unreferenced. Other code in 'kernel/mm/memory_hotplug.c' deals with initializing an empty node when adding memory to a system. The 'removing memory' case is specific to systems that perform LPM and allow device-tree changes. The powerpc kernel does not have the option of accepting some PRRN requests and accepting others. It must perform them all.
The kernel/mm code that removes memory blocks does not (before this patch) recognize that the affinity of a memory block could have changed to a previously unused node. If every path to try_offline_node made such a check, then this patch would be unnecessary. However, putting a patch at a single location to check for a relatively rare occurrence, would seem to be a more efficient implementation.
Michael
--
Michael W. Bringmann
Linux Technology Center
IBM Corporation
Tie-Line 363-5196
External: (512) 286-5196
Cell: (512) 466-0650
mwb@linux.vnet.ibm.com
^ permalink raw reply
* Re: [PATCH RFC] mm/memory_hotplug: Introduce memory block types
From: David Hildenbrand @ 2018-10-02 15:25 UTC (permalink / raw)
To: Michal Hocko
Cc: Kate Stewart, Rich Felker, linux-ia64, linux-sh, Peter Zijlstra,
Dave Hansen, Heiko Carstens, linux-mm, Pavel Tatashin,
Paul Mackerras, H. Peter Anvin, Rashmica Gupta, K. Y. Srinivasan,
Boris Ostrovsky, linux-s390, Michael Neuling, Stephen Hemminger,
Yoshinori Sato, linux-acpi, Ingo Molnar, xen-devel, Rob Herring,
Len Brown, Fenghua Yu, Stephen Rothwell, mike.travis@hpe.com,
Haiyang Zhang, Dan Williams, Jonathan Neuschäfer,
Nicholas Piggin, Joe Perches, Jérôme Glisse,
Mike Rapoport, Borislav Petkov, Andy Lutomirski, Thomas Gleixner,
Joonsoo Kim, Oscar Salvador, Juergen Gross, Tony Luck,
Mathieu Malaterre, Greg Kroah-Hartman, Rafael J. Wysocki,
linux-kernel, Mauricio Faria de Oliveira, Philippe Ombredanne,
Martin Schwidefsky, devel, Andrew Morton, linuxppc-dev,
Kirill A. Shutemov
In-Reply-To: <20181002134734.GT18290@dhcp22.suse.cz>
On 02/10/2018 15:47, Michal Hocko wrote:
> On Mon 01-10-18 11:34:25, David Hildenbrand wrote:
>> On 01/10/2018 10:40, Michal Hocko wrote:
>>> On Fri 28-09-18 17:03:57, David Hildenbrand wrote:
>>> [...]
>>>
>>> I haven't read the patch itself but I just wanted to note one thing
>>> about this part
>>>
>>>> For paravirtualized devices it is relevant that memory is onlined as
>>>> quickly as possible after adding - and that it is added to the NORMAL
>>>> zone. Otherwise, it could happen that too much memory in a row is added
>>>> (but not onlined), resulting in out-of-memory conditions due to the
>>>> additional memory for "struct pages" and friends. MOVABLE zone as well
>>>> as delays might be very problematic and lead to crashes (e.g. zone
>>>> imbalance).
>>>
>>> I have proposed (but haven't finished this due to other stuff) a
>>> solution for this. Newly added memory can host memmaps itself and then
>>> you do not have the problem in the first place. For vmemmap it would
>>> have an advantage that you do not really have to beg for 2MB pages to
>>> back the whole section but you would get it for free because the initial
>>> part of the section is by definition properly aligned and unused.
>>
>> So the plan is to "host metadata for new memory on the memory itself".
>> Just want to note that this is basically impossible for s390x with the
>> current mechanisms. (added memory is dead, until onlining notifies the
>> hypervisor and memory is allocated). It will also be problematic for
>> paravirtualized memory devices (e.g. XEN's "not backed by the
>> hypervisor" hacks).
>
> OK, I understand that not all usecases can use self memmap hosting
> others do not have much choice left though. You have to allocate from
> somewhere. Well and alternative would be to have no memmap until
> onlining but I am not sure how much work that would be.
>
>> This would only be possible for memory DIMMs, memory that is completely
>> accessible as far as I can see. Or at least, some specified "first part"
>> is accessible.
>>
>> Other problems are other metadata like extended struct pages and friends.
>
> I wouldn't really worry about extended struct pages. Those should be
> used for debugging purposes mostly. Ot at least that was the case last
> time I've checked.
Yes, I guess that is true. Being able to add and online memory without
the need for additional (external) memory would be the ultimate goal,
but highly complicated. But steps into that direction is a good idea.
>
>> (I really like the idea of adding memory without allocating memory in
>> the hypervisor in the first place, please keep me tuned).
>>
>> And please note: This solves some problematic part ("adding too much
>> memory to the movable zone or not onlining it"), but not the issue of
>> zone imbalance in the first place. And not one issue I try to tackle
>> here: don't add paravirtualized memory to the movable zone.
>
> Zone imbalance is an inherent problem of the highmem zone. It is
> essentially the highmem zone we all loved so much back in 32b days.
> Yes the movable zone doesn't have any addressing limitations so it is a
> bit more relaxed but considering the hotplug scenarios I have seen so
> far people just want to have full NUMA nodes movable to allow replacing
> DIMMs. And then we are back to square one and the zone imbalance issue.
> You have those regardless where memmaps are allocated from.
Unfortunately yes. And things get more complicated as you are adding a
whole DIMMs and get notifications in the granularity of memory blocks.
Usually you are not interested in onlining any memory block of that DIMM
as MOVABLE as soon as you would have to online one memory block of that
DIMM as NORMAL - because that can already block the whole DIMM.
>
>>> I yet have to think about the whole proposal but I am missing the most
>>> important part. _Who_ is going to use the new exported information and
>>> for what purpose. You said that distributions have hard time to
>>> distinguish different types of onlinining policies but isn't this
>>> something that is inherently usecase specific?
>>>
>>
>> Let's think about a distribution. We have a clash of use cases here
>> (just what you describe). What I propose solves one part of it ("handle
>> what you know how to handle right in the kernel").
>>
>> 1. Users of DIMMs usually expect that they can be unplugged again. That
>> is why you want to control how to online memory in user space (== add it
>> to the movable zone).
>
> Which is only true if you really want to hotremove them. I am not going
> to tell how much I believe in this usecase but movable policy is not
> generally applicable here.
Customers expect this to work and the both of us know that we can't make
any guarantees. At least MOVABLE makes it more likely to work. NORMAL is
basically impossible.
>
>> 2. Users of standby memory (s390) expect that memory will never be
>> onlined automatically. It will be onlined manually.
>
> yeah
>
>> 3. Users of paravirtualized devices (esp. Hyper-V) don't care about
>> memory unplug in the sense of MOVABLE at all. They (or Hyper-V!) will
>> add a whole bunch of memory and expect that everything works fine. So
>> that memory is onlined immediately and that memory is added to the
>> NORMAL zone. Users never want the MOVABLE zone.
>
> Then the immediate question would be why to use memory hotplug for that
> at all? Why don't you simply start with a huge pre-allocated physical
> address space and balloon memory in an out per demand. Why do you want
> to inject new memory during the runtime?
Let's assume you have a guest with 20GB size and eventually want to
allow to grow it to 4TB. You would have to allocate metadata for 4TB
right from the beginning. That's definitely now what we want. That is
why memory hotplug is used by e.g. XEN or Hyper-V. With Hyper-V, the
hypervisor even tells you at which places additional memory has been
made available.
>
>> 1. is a reason why distributions usually don't configure
>> "MEMORY_HOTPLUG_DEFAULT_ONLINE", because you really want the option for
>> MOVABLE zone. That however implies, that e.g. for x86, you have to
>> handle all new memory in user space, especially also HyperV memory.
>> There, you then have to check for things like "isHyperV()" to decide
>> "oh, yes, this should definitely not go to the MOVABLE zone".
>
> Why do you need a generic hotplug rule in the first place? Why don't you
> simply provide different set of rules for different usecases? Let users
> decide which usecase they prefer rather than try to be clever which
> almost always hits weird corner cases.
>
Memory hotplug has to work as reliable as we can out of the box. Letting
the user make simple decisions like "oh, I am on hyper-V, I want to
online memory to the normal zone" does not feel right. But yes, we
should definitely allow to make modifications. So some sane default rule
+ possible modification is usually a good idea.
I think Dave has a point with using MOVABLE for huge page use cases. And
there might be other corner cases as you correctly state.
I wonder if this patch itself minus modifying online/offline might make
sense. We can then implement simple rules in user space
if (normal) {
/* customers expect hotplugged DIMMs to be unpluggable */
online_movable();
} else if (paravirt) {
/* paravirt memory should as default always go to the NORMAL */
online();
} else {
/* standby memory will never get onlined automatically */
}
Compared to having to guess what is to be done (isKVM(), isHyperV,
isS390 ...) and failing once this is no longer unique (e.g. virtio-mem
and ACPI support for x86 KVM).
--
Thanks,
David / dhildenb
^ permalink raw reply
* [PATCH v2] powerpc/pseries: Track LMB nid instead of using device tree
From: Nathan Fontenot @ 2018-10-02 15:35 UTC (permalink / raw)
To: linuxppc-dev
When removing memory we need to remove the memory from the node
it was added to instead of looking up the node it should be in
in the device tree.
During testing we have seen scenarios where the affinity for a
LMB changes due to a partition migration or PRRN event. In these
cases the node the LMB exists in may not match the node the device
tree indicates it belongs in. This can lead to a system crash
when trying to DLPAR remove the LMB after a migration or PRRN
event. The current code looks up the node in the device tree to
remove the LMB from, the crash occurs when we try to offline this
node and it does not have any data, i.e. node_data[nid] == NULL.
36:mon> e
cpu 0x36: Vector: 300 (Data Access) at [c0000001828b7810]
pc: c00000000036d08c: try_offline_node+0x2c/0x1b0
lr: c0000000003a14ec: remove_memory+0xbc/0x110
sp: c0000001828b7a90
msr: 800000000280b033
dar: 9a28
dsisr: 40000000
current = 0xc0000006329c4c80
paca = 0xc000000007a55200 softe: 0 irq_happened: 0x01
pid = 76926, comm = kworker/u320:3
36:mon> t
[link register ] c0000000003a14ec remove_memory+0xbc/0x110
[c0000001828b7a90] c00000000006a1cc arch_remove_memory+0x9c/0xd0 (unreliable)
[c0000001828b7ad0] c0000000003a14e0 remove_memory+0xb0/0x110
[c0000001828b7b20] c0000000000c7db4 dlpar_remove_lmb+0x94/0x160
[c0000001828b7b60] c0000000000c8ef8 dlpar_memory+0x7e8/0xd10
[c0000001828b7bf0] c0000000000bf828 handle_dlpar_errorlog+0xf8/0x160
[c0000001828b7c60] c0000000000bf8cc pseries_hp_work_fn+0x3c/0xa0
[c0000001828b7c90] c000000000128cd8 process_one_work+0x298/0x5a0
[c0000001828b7d20] c000000000129068 worker_thread+0x88/0x620
[c0000001828b7dc0] c00000000013223c kthread+0x1ac/0x1c0
[c0000001828b7e30] c00000000000b45c ret_from_kernel_thread+0x5c/0x80
To resolve this we need to track the node a LMB belongs to when
it is added to the system so we can remove it from that node instead
of the node that the device tree indicates it should belong to.
Signed-off-by: Nathan Fontenot <nfont@linux.vnet.ibm.com>
---
Updates:
V2: Fix builds to only track nid for LMBs when MEMORY_HOTPLUG
is configured.
arch/powerpc/include/asm/drmem.h | 21 +++++++++++++++++++++
arch/powerpc/mm/drmem.c | 6 +++++-
arch/powerpc/platforms/pseries/hotplug-memory.c | 17 ++++++++---------
3 files changed, 34 insertions(+), 10 deletions(-)
diff --git a/arch/powerpc/include/asm/drmem.h b/arch/powerpc/include/asm/drmem.h
index 7c1d8e74b25d..7f3279b014db 100644
--- a/arch/powerpc/include/asm/drmem.h
+++ b/arch/powerpc/include/asm/drmem.h
@@ -17,6 +17,9 @@ struct drmem_lmb {
u32 drc_index;
u32 aa_index;
u32 flags;
+#ifdef CONFIG_MEMORY_HOTPLUG
+ int nid;
+#endif
};
struct drmem_lmb_info {
@@ -104,4 +107,22 @@ static inline void invalidate_lmb_associativity_index(struct drmem_lmb *lmb)
lmb->aa_index = 0xffffffff;
}
+#ifdef CONFIG_MEMORY_HOTPLUG
+static inline void lmb_set_nid(struct drmem_lmb *lmb)
+{
+ lmb->nid = memory_add_physaddr_to_nid(lmb->base_addr);
+}
+static inline void lmb_clear_nid(struct drmem_lmb *lmb)
+{
+ lmb->nid = -1;
+}
+#else
+static inline void lmb_set_nid(struct drmem_lmb *lmb)
+{
+}
+static inline void lmb_clear_nid(struct drmem_lmb *lmb)
+{
+}
+#endif
+
#endif /* _ASM_POWERPC_LMB_H */
diff --git a/arch/powerpc/mm/drmem.c b/arch/powerpc/mm/drmem.c
index 3f1803672c9b..641891df2046 100644
--- a/arch/powerpc/mm/drmem.c
+++ b/arch/powerpc/mm/drmem.c
@@ -366,8 +366,10 @@ static void __init init_drmem_v1_lmbs(const __be32 *prop)
if (!drmem_info->lmbs)
return;
- for_each_drmem_lmb(lmb)
+ for_each_drmem_lmb(lmb) {
read_drconf_v1_cell(lmb, &prop);
+ lmb_set_nid(lmb);
+ }
}
static void __init init_drmem_v2_lmbs(const __be32 *prop)
@@ -412,6 +414,8 @@ static void __init init_drmem_v2_lmbs(const __be32 *prop)
lmb->aa_index = dr_cell.aa_index;
lmb->flags = dr_cell.flags;
+
+ lmb_set_nid(lmb);
}
}
}
diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
index 9a15d39995e5..58feca665b8c 100644
--- a/arch/powerpc/platforms/pseries/hotplug-memory.c
+++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
@@ -382,7 +382,7 @@ static int dlpar_add_lmb(struct drmem_lmb *);
static int dlpar_remove_lmb(struct drmem_lmb *lmb)
{
unsigned long block_sz;
- int nid, rc;
+ int rc;
if (!lmb_is_removable(lmb))
return -EINVAL;
@@ -392,14 +392,14 @@ static int dlpar_remove_lmb(struct drmem_lmb *lmb)
return rc;
block_sz = pseries_memory_block_size();
- nid = memory_add_physaddr_to_nid(lmb->base_addr);
- remove_memory(nid, lmb->base_addr, block_sz);
+ remove_memory(lmb->nid, lmb->base_addr, block_sz);
/* Update memory regions for memory remove */
memblock_remove(lmb->base_addr, block_sz);
invalidate_lmb_associativity_index(lmb);
+ lmb_clear_nid(lmb);
lmb->flags &= ~DRCONF_MEM_ASSIGNED;
return 0;
@@ -656,7 +656,7 @@ static int dlpar_memory_remove_by_ic(u32 lmbs_to_remove, u32 drc_index)
static int dlpar_add_lmb(struct drmem_lmb *lmb)
{
unsigned long block_sz;
- int nid, rc;
+ int rc;
if (lmb->flags & DRCONF_MEM_ASSIGNED)
return -EINVAL;
@@ -667,13 +667,11 @@ static int dlpar_add_lmb(struct drmem_lmb *lmb)
return rc;
}
+ lmb_set_nid(lmb);
block_sz = memory_block_size_bytes();
- /* Find the node id for this address */
- nid = memory_add_physaddr_to_nid(lmb->base_addr);
-
/* Add the memory */
- rc = add_memory(nid, lmb->base_addr, block_sz);
+ rc = add_memory(lmb->nid, lmb->base_addr, block_sz);
if (rc) {
invalidate_lmb_associativity_index(lmb);
return rc;
@@ -681,8 +679,9 @@ static int dlpar_add_lmb(struct drmem_lmb *lmb)
rc = dlpar_online_lmb(lmb);
if (rc) {
- remove_memory(nid, lmb->base_addr, block_sz);
+ remove_memory(lmb->nid, lmb->base_addr, block_sz);
invalidate_lmb_associativity_index(lmb);
+ lmb_clear_nid(lmb);
} else {
lmb->flags |= DRCONF_MEM_ASSIGNED;
}
^ permalink raw reply related
* Re: [PATCH] migration/mm: Add WARN_ON to try_offline_node
From: Michal Hocko @ 2018-10-02 16:04 UTC (permalink / raw)
To: Michael Bringmann
Cc: Thomas Falcon, Kees Cook, Mathieu Malaterre, Pavel Tatashin,
Nicholas Piggin, linux-kernel, linux-mm,
Mauricio Faria de Oliveira, Juliet Kim, Tyrel Datwyler,
Thiago Jung Bauermann, Nathan Fontenot, Andrew Morton,
YASUAKI ISHIMATSU, linuxppc-dev, Dan Williams, Oscar Salvador
In-Reply-To: <d338b385-626b-0e79-9944-708178fe245d@linux.vnet.ibm.com>
On Tue 02-10-18 10:14:49, Michael Bringmann wrote:
> On 10/02/2018 09:59 AM, Michal Hocko wrote:
> > On Tue 02-10-18 09:51:40, Michael Bringmann wrote:
> > [...]
> >> When the device-tree affinity attributes have changed for memory,
> >> the 'nid' affinity calculated points to a different node for the
> >> memory block than the one used to install it, previously on the
> >> source system. The newly calculated 'nid' affinity may not yet
> >> be initialized on the target system. The current memory tracking
> >> mechanisms do not record the node to which a memory block was
> >> associated when it was added. Nathan is looking at adding this
> >> feature to the new implementation of LMBs, but it is not there
> >> yet, and won't be present in earlier kernels without backporting a
> >> significant number of changes.
> >
> > Then the patch you have proposed here just papers over a real issue, no?
> > IIUC then you simply do not remove the memory if you lose the race.
>
> The problem occurs when removing memory after an affinity change
> references a node that was previously unreferenced. Other code
> in 'kernel/mm/memory_hotplug.c' deals with initializing an empty
> node when adding memory to a system. The 'removing memory' case is
> specific to systems that perform LPM and allow device-tree changes.
> The powerpc kernel does not have the option of accepting some PRRN
> requests and accepting others. It must perform them all.
I am sorry, but you are still too cryptic for me. Either there is a
correctness issue and the the patch doesn't really fix anything or the
final race doesn't make any difference and then the ppc code should be
explicit about that. Checking the node inside the hotplug core code just
looks as a wrong layer to mitigate an arch specific problem. I am not
saying the patch is a no-go but if anything we want a big fat comment
explaining how this is possible because right now it just points to an
incorrect API usage.
That being said, this sounds pretty much ppc specific problem and I
would _prefer_ it to be handled there (along with a big fat comment of
course).
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH] migration/mm: Add WARN_ON to try_offline_node
From: Michael Bringmann @ 2018-10-02 18:13 UTC (permalink / raw)
To: Michal Hocko
Cc: Thomas Falcon, Kees Cook, Mathieu Malaterre, linux-kernel,
Nicholas Piggin, Pavel Tatashin, linux-mm,
Mauricio Faria de Oliveira, Juliet Kim, Tyrel Datwyler,
Thiago Jung Bauermann, Nathan Fontenot, Andrew Morton,
YASUAKI ISHIMATSU, linuxppc-dev, Dan Williams, Oscar Salvador
In-Reply-To: <20181002160446.GA18290@dhcp22.suse.cz>
On 10/02/2018 11:04 AM, Michal Hocko wrote:
> On Tue 02-10-18 10:14:49, Michael Bringmann wrote:
>> On 10/02/2018 09:59 AM, Michal Hocko wrote:
>>> On Tue 02-10-18 09:51:40, Michael Bringmann wrote:
>>> [...]
>>>> When the device-tree affinity attributes have changed for memory,
>>>> the 'nid' affinity calculated points to a different node for the
>>>> memory block than the one used to install it, previously on the
>>>> source system. The newly calculated 'nid' affinity may not yet
>>>> be initialized on the target system. The current memory tracking
>>>> mechanisms do not record the node to which a memory block was
>>>> associated when it was added. Nathan is looking at adding this
>>>> feature to the new implementation of LMBs, but it is not there
>>>> yet, and won't be present in earlier kernels without backporting a
>>>> significant number of changes.
>>>
>>> Then the patch you have proposed here just papers over a real issue, no?
>>> IIUC then you simply do not remove the memory if you lose the race.
>>
>> The problem occurs when removing memory after an affinity change
>> references a node that was previously unreferenced. Other code
>> in 'kernel/mm/memory_hotplug.c' deals with initializing an empty
>> node when adding memory to a system. The 'removing memory' case is
>> specific to systems that perform LPM and allow device-tree changes.
>> The powerpc kernel does not have the option of accepting some PRRN
>> requests and accepting others. It must perform them all.
>
> I am sorry, but you are still too cryptic for me. Either there is a
> correctness issue and the the patch doesn't really fix anything or the
> final race doesn't make any difference and then the ppc code should be
> explicit about that. Checking the node inside the hotplug core code just
> looks as a wrong layer to mitigate an arch specific problem. I am not
> saying the patch is a no-go but if anything we want a big fat comment
> explaining how this is possible because right now it just points to an
> incorrect API usage.
>
> That being said, this sounds pretty much ppc specific problem and I
> would _prefer_ it to be handled there (along with a big fat comment of
> course).
Let me try again. Regardless of the path to which we get to this condition,
we currently crash the kernel. This patch changes that to a WARN_ON notice
and continues executing the kernel without shutting down the system. I saw
the problem during powerpc testing, because that is the focus of my work.
There are other paths to this function besides powerpc. I feel that the
kernel should keep running instead of halting.
Regards,
--
Michael W. Bringmann
Linux Technology Center
IBM Corporation
Tie-Line 363-5196
External: (512) 286-5196
Cell: (512) 466-0650
mwb@linux.vnet.ibm.com
^ permalink raw reply
* Re: [PATCH] powerpc/mobility: Extend start/stop topology update scope
From: Nathan Fontenot @ 2018-10-02 19:32 UTC (permalink / raw)
To: Michael Bringmann, linuxppc-dev; +Cc: Juliet Kim, Thomas Falcon, Tyrel Datwyler
In-Reply-To: <20181001185603.11373.61650.stgit@ltcalpine2-lp9.aus.stglabs.ibm.com>
On 10/01/2018 01:56 PM, Michael Bringmann wrote:
> The PPC mobility code may receive RTAS requests to perform PRRN
> topology changes at any time, including during LPAR migration
> operations. In some configurations where the affinity of CPUs
> or memory is being changed on that platform, the PRRN requests
> may apply or refer to outdated information prior to the complete
> update of the device-tree. This patch changes the duration for
> which topology updates are suppressed during LPAR migrations from
> just the rtas_ibm_suspend_me / 'ibm,suspend-me' call(s) to cover
> the entire 'migration_store' operation to allow all changes to
> the device-tree to be applied prior to accepting and applying any
> PRRN requests.
>
> For tracking purposes, pr_info notices are added to the functions
> start_topology_update() and stop_topology_update() of 'numa.c'.
>
> Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
Reviewed-by: Nathan Fontenot <nfont@linux.vnet.ibm.com>
> ---
> arch/powerpc/kernel/rtas.c | 4 ----
> arch/powerpc/mm/numa.c | 6 ++++++
> arch/powerpc/platforms/pseries/mobility.c | 5 +++++
> 3 files changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
> index 8afd146..28d8b57 100644
> --- a/arch/powerpc/kernel/rtas.c
> +++ b/arch/powerpc/kernel/rtas.c
> @@ -981,8 +981,6 @@ int rtas_ibm_suspend_me(u64 handle)
> goto out;
> }
>
> - stop_topology_update();
> -
> /* Call function on all CPUs. One of us will make the
> * rtas call
> */
> @@ -994,8 +992,6 @@ int rtas_ibm_suspend_me(u64 handle)
> if (atomic_read(&data.error) != 0)
> printk(KERN_ERR "Error doing global join\n");
>
> - start_topology_update();
> -
> /* Take down CPUs not online prior to suspend */
> cpuret = rtas_offline_cpus_mask(offline_mask);
> if (cpuret)
> diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
> index b5a71ba..0ade0a1 100644
> --- a/arch/powerpc/mm/numa.c
> +++ b/arch/powerpc/mm/numa.c
> @@ -1518,6 +1518,10 @@ int start_topology_update(void)
> }
> }
>
> + pr_info("Starting topology update%s%s\n",
> + (prrn_enabled ? " prrn_enabled" : ""),
> + (vphn_enabled ? " vphn_enabled" : ""));
> +
> return rc;
> }
>
> @@ -1539,6 +1543,8 @@ int stop_topology_update(void)
> rc = del_timer_sync(&topology_timer);
> }
>
> + pr_info("Stopping topology update\n");
> +
> return rc;
> }
>
> diff --git a/arch/powerpc/platforms/pseries/mobility.c b/arch/powerpc/platforms/pseries/mobility.c
> index 23fb9ac..49ebefd 100644
> --- a/arch/powerpc/platforms/pseries/mobility.c
> +++ b/arch/powerpc/platforms/pseries/mobility.c
> @@ -373,6 +373,8 @@ static ssize_t migration_store(struct class *class,
> if (rc)
> return rc;
>
> + stop_topology_update();
> +
> do {
> rc = rtas_ibm_suspend_me(streamid);
> if (rc == -EAGAIN)
> @@ -383,6 +385,9 @@ static ssize_t migration_store(struct class *class,
> return rc;
>
> post_mobility_fixup();
> +
> + start_topology_update();
> +
> return count;
> }
>
>
^ permalink raw reply
* Re: [PATCH v2 0/5] soc/fsl/qbman: DPAA QBMan fixes and additions
From: Li Yang @ 2018-10-02 19:45 UTC (permalink / raw)
To: madalin.bucur, Olof Johansson, Arnd Bergmann
Cc: Catalin Marinas, Roy Pledge, lkml, Scott Wood, claudiu.manoil,
linuxppc-dev,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
In-Reply-To: <AM6PR04MB40080234BA68140933F6B733ECE80@AM6PR04MB4008.eurprd04.prod.outlook.com>
On Tue, Oct 2, 2018 at 1:29 AM Madalin-cristian Bucur
<madalin.bucur@nxp.com> wrote:
>
> > -----Original Message-----
> > From: Li Yang [mailto:leoyang.li@nxp.com]
> > Sent: Tuesday, October 2, 2018 1:30 AM
> > To: Madalin-cristian Bucur <madalin.bucur@nxp.com>
> > Cc: Roy Pledge <roy.pledge@nxp.com>; Claudiu Manoil
> > <claudiu.manoil@nxp.com>; Catalin Marinas <catalin.marinas@arm.com>; Scott
> > Wood <oss@buserror.net>; moderated list:ARM/FREESCALE IMX / MXC ARM
> > ARCHITECTURE <linux-arm-kernel@lists.infradead.org>; linuxppc-dev
> > <linuxppc-dev@lists.ozlabs.org>; lkml <linux-kernel@vger.kernel.org>
> > Subject: Re: [PATCH v2 0/5] soc/fsl/qbman: DPAA QBMan fixes and additions
> >
> > On Fri, Sep 28, 2018 at 3:44 AM Madalin Bucur <madalin.bucur@nxp.com>
> > wrote:
> > >
> >
> > Applied 1-4 to for-next while waiting for clarification on 5/5. And
> > updated the prefix to "soc: fsl:" style to be aligned with arm-soc
> > convention. Please try to use that style in the future for soc/fsl
> > patches.
>
> Thank you, I've sent an email about the APIs.
> I'm not sure we need to align the prefix to arm-soc as the soc/fsl does not
> service only ARM but also PPC based SoCs and historically we've been using
> the soc/* format.
There is no kernel wide guideline about the format of subsystem prefix
in the patch subject. Different subsystems have their own
preferrences. Soc is not considered as a separate subsystem, so we
followed the convention of the architectural subsystem that we merge
patches through. Since we normally get soc patches through the
arm-soc tree right now, I think it would be better to follow the
convention of arm-soc to make them not looking too different in the
arm-soc pull requests. Not sure how sensetive ARM-SOC maintainers
feel about this though.
Regards,
Leo
^ permalink raw reply
* Re: [PATCH] migration/mm: Add WARN_ON to try_offline_node
From: Tyrel Datwyler @ 2018-10-02 19:45 UTC (permalink / raw)
To: Michael Bringmann, Michal Hocko
Cc: Thomas Falcon, Kees Cook, Mathieu Malaterre, Pavel Tatashin,
Nicholas Piggin, linux-kernel, linux-mm,
Mauricio Faria de Oliveira, Juliet Kim, Thiago Jung Bauermann,
Nathan Fontenot, Andrew Morton, YASUAKI ISHIMATSU, linuxppc-dev,
Dan Williams, Oscar Salvador
In-Reply-To: <e7dd66c1-d196-3a14-0115-acdaf538ebfd@linux.vnet.ibm.com>
On 10/02/2018 11:13 AM, Michael Bringmann wrote:
>
>
> On 10/02/2018 11:04 AM, Michal Hocko wrote:
>> On Tue 02-10-18 10:14:49, Michael Bringmann wrote:
>>> On 10/02/2018 09:59 AM, Michal Hocko wrote:
>>>> On Tue 02-10-18 09:51:40, Michael Bringmann wrote:
>>>> [...]
>>>>> When the device-tree affinity attributes have changed for memory,
>>>>> the 'nid' affinity calculated points to a different node for the
>>>>> memory block than the one used to install it, previously on the
>>>>> source system. The newly calculated 'nid' affinity may not yet
>>>>> be initialized on the target system. The current memory tracking
>>>>> mechanisms do not record the node to which a memory block was
>>>>> associated when it was added. Nathan is looking at adding this
>>>>> feature to the new implementation of LMBs, but it is not there
>>>>> yet, and won't be present in earlier kernels without backporting a
>>>>> significant number of changes.
>>>>
>>>> Then the patch you have proposed here just papers over a real issue, no?
>>>> IIUC then you simply do not remove the memory if you lose the race.
>>>
>>> The problem occurs when removing memory after an affinity change
>>> references a node that was previously unreferenced. Other code
>>> in 'kernel/mm/memory_hotplug.c' deals with initializing an empty
>>> node when adding memory to a system. The 'removing memory' case is
>>> specific to systems that perform LPM and allow device-tree changes.
>>> The powerpc kernel does not have the option of accepting some PRRN
>>> requests and accepting others. It must perform them all.
>>
>> I am sorry, but you are still too cryptic for me. Either there is a
>> correctness issue and the the patch doesn't really fix anything or the
>> final race doesn't make any difference and then the ppc code should be
>> explicit about that. Checking the node inside the hotplug core code just
>> looks as a wrong layer to mitigate an arch specific problem. I am not
>> saying the patch is a no-go but if anything we want a big fat comment
>> explaining how this is possible because right now it just points to an
>> incorrect API usage.
>>
>> That being said, this sounds pretty much ppc specific problem and I
>> would _prefer_ it to be handled there (along with a big fat comment of
>> course).
>
> Let me try again. Regardless of the path to which we get to this condition,
> we currently crash the kernel. This patch changes that to a WARN_ON notice
> and continues executing the kernel without shutting down the system. I saw
> the problem during powerpc testing, because that is the focus of my work.
> There are other paths to this function besides powerpc. I feel that the
> kernel should keep running instead of halting.
This is still basically a hack to get around a known race. In itself this patch is still worth while in that we shouldn't crash the kernel on a null pointer dereference. However, I think the actual problem still needs to be addressed. We shouldn't run any PRRN events for the source system on the target after a migration. The device tree update should have taken care of telling us about new affinities and what not. Can we just throw out any queued PRRN events when we wake up on the target?
-Tyrel
>
> Regards,
>
^ permalink raw reply
* Re: [PATCH v2 5/5] soc/fsl_qbman: export coalesce change API
From: Li Yang @ 2018-10-02 20:07 UTC (permalink / raw)
To: madalin.bucur
Cc: Catalin Marinas, Roy Pledge, lkml, Scott Wood, claudiu.manoil,
Netdev, linuxppc-dev,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
In-Reply-To: <AM6PR04MB400877EC0C140A7D919AEF41ECE80@AM6PR04MB4008.eurprd04.prod.outlook.com>
On Tue, Oct 2, 2018 at 1:08 AM Madalin-cristian Bucur
<madalin.bucur@nxp.com> wrote:
>
> > -----Original Message-----
> > From: Li Yang [mailto:leoyang.li@nxp.com]
> > Sent: Tuesday, October 2, 2018 12:50 AM
> > To: Madalin-cristian Bucur <madalin.bucur@nxp.com>
> > Cc: Roy Pledge <roy.pledge@nxp.com>; Claudiu Manoil
> > <claudiu.manoil@nxp.com>; Catalin Marinas <catalin.marinas@arm.com>; Scott
> > Wood <oss@buserror.net>; moderated list:ARM/FREESCALE IMX / MXC ARM
> > ARCHITECTURE <linux-arm-kernel@lists.infradead.org>; linuxppc-dev
> > <linuxppc-dev@lists.ozlabs.org>; lkml <linux-kernel@vger.kernel.org>
> > Subject: Re: [PATCH v2 5/5] soc/fsl_qbman: export coalesce change API
> >
> > On Fri, Sep 28, 2018 at 3:45 AM Madalin Bucur <madalin.bucur@nxp.com>
> > wrote:
> > >
> > > Export the API required to control the QMan portal interrupt coalescing
> > > settings.
> >
> > These are new APIs not just old APIs being exported. What is the user
> > of these APIs? Is the user being submitted? We cannot have APIs in
> > kernel that has no users.
>
> Hi,
>
> These are new APIs that will be used in the DPAA Ethernet driver.
> Changes for the DPAA QBMan and DPAA Ethernet follow different paths upstream
> so the Ethernet driver patch that makes use of these APIs will be sent when
> these changes reach the net-next/master tree.
Applied for next after changing from "export API" to "add API" in the
introduction and updated the subsystem prefix.
Regards,
Leo
^ permalink raw reply
* Re: [PATCH v03 1/5] powerpc/drmem: Export 'dynamic-memory' loader
From: Tyrel Datwyler @ 2018-10-02 20:56 UTC (permalink / raw)
To: Michael Bringmann, linuxppc-dev
Cc: Nathan Fontenot, Juliet Kim, Thomas Falcon
In-Reply-To: <20181001125924.2676.54786.stgit@ltcalpine2-lp9.aus.stglabs.ibm.com>
On 10/01/2018 05:59 AM, Michael Bringmann wrote:
> powerpc/drmem: Export many of the functions of DRMEM to parse
> "ibm,dynamic-memory" and "ibm,dynamic-memory-v2" during hotplug
> operations and for Post Migration events.
>
> Also modify the DRMEM initialization code to allow it to,
>
> * Be called after system initialization
> * Provide a separate user copy of the LMB array that is produces
> * Free the user copy upon request
>
> In addition, a couple of changes were made to make the creation
> of additional copies of the LMB array more useful including,
>
> * Add new iterator to work through a pair of drmem_info arrays.
> * Modify DRMEM code to replace usages of dt_root_addr_cells, and
> dt_mem_next_cell, as these are only available at first boot.
>
> Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
> ---
> arch/powerpc/include/asm/drmem.h | 15 ++++++++
> arch/powerpc/mm/drmem.c | 75 ++++++++++++++++++++++++++++----------
> 2 files changed, 70 insertions(+), 20 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/drmem.h b/arch/powerpc/include/asm/drmem.h
> index ce242b9..b0e70fd 100644
> --- a/arch/powerpc/include/asm/drmem.h
> +++ b/arch/powerpc/include/asm/drmem.h
> @@ -35,6 +35,18 @@ struct drmem_lmb_info {
> &drmem_info->lmbs[0], \
> &drmem_info->lmbs[drmem_info->n_lmbs - 1])
>
> +#define for_each_dinfo_lmb(dinfo, lmb) \
> + for_each_drmem_lmb_in_range((lmb), \
> + &dinfo->lmbs[0], \
> + &dinfo->lmbs[dinfo->n_lmbs - 1])
> +
> +#define for_each_pair_dinfo_lmb(dinfo1, lmb1, dinfo2, lmb2) \
> + for ((lmb1) = (&dinfo1->lmbs[0]), \
> + (lmb2) = (&dinfo2->lmbs[0]); \
> + ((lmb1) <= (&dinfo1->lmbs[dinfo1->n_lmbs - 1])) && \
> + ((lmb2) <= (&dinfo2->lmbs[dinfo2->n_lmbs - 1])); \
> + (lmb1)++, (lmb2)++)
> +
> /*
> * The of_drconf_cell_v1 struct defines the layout of the LMB data
> * specified in the ibm,dynamic-memory device tree property.
> @@ -94,6 +106,9 @@ void __init walk_drmem_lmbs(struct device_node *dn,
> void (*func)(struct drmem_lmb *, const __be32 **));
> int drmem_update_dt(void);
>
> +struct drmem_lmb_info *drmem_lmbs_init(struct property *prop);
> +void drmem_lmbs_free(struct drmem_lmb_info *dinfo);
> +
> #ifdef CONFIG_PPC_PSERIES
> void __init walk_drmem_lmbs_early(unsigned long node,
> void (*func)(struct drmem_lmb *, const __be32 **));
> diff --git a/arch/powerpc/mm/drmem.c b/arch/powerpc/mm/drmem.c
> index 3f18036..13d2abb 100644
> --- a/arch/powerpc/mm/drmem.c
> +++ b/arch/powerpc/mm/drmem.c
> @@ -20,6 +20,7 @@
>
> static struct drmem_lmb_info __drmem_info;
> struct drmem_lmb_info *drmem_info = &__drmem_info;
> +static int n_root_addr_cells;
What is the point of this new global? I see two places were it gets initialized if it is null, and both those initializers simply set it to "dt_root_addr_cells". I also checked the rest of the patches in the series and none of those even reference this variable.
>
> u64 drmem_lmb_memory_max(void)
> {
> @@ -193,12 +194,13 @@ int drmem_update_dt(void)
> return rc;
> }
>
> -static void __init read_drconf_v1_cell(struct drmem_lmb *lmb,
> +static void read_drconf_v1_cell(struct drmem_lmb *lmb,
> const __be32 **prop)
> {
> const __be32 *p = *prop;
>
> - lmb->base_addr = dt_mem_next_cell(dt_root_addr_cells, &p);
> + lmb->base_addr = of_read_number(p, n_root_addr_cells);
> + p += n_root_addr_cells;
Unnecessary code churn do to the introduction of "n_root_addr_cells".
> lmb->drc_index = of_read_number(p++, 1);
>
> p++; /* skip reserved field */
> @@ -209,7 +211,7 @@ static void __init read_drconf_v1_cell(struct drmem_lmb *lmb,
> *prop = p;
> }
>
> -static void __init __walk_drmem_v1_lmbs(const __be32 *prop, const __be32 *usm,
> +static void __walk_drmem_v1_lmbs(const __be32 *prop, const __be32 *usm,
> void (*func)(struct drmem_lmb *, const __be32 **))
> {
> struct drmem_lmb lmb;
> @@ -225,13 +227,14 @@ static void __init __walk_drmem_v1_lmbs(const __be32 *prop, const __be32 *usm,
> }
> }
>
> -static void __init read_drconf_v2_cell(struct of_drconf_cell_v2 *dr_cell,
> +static void read_drconf_v2_cell(struct of_drconf_cell_v2 *dr_cell,
> const __be32 **prop)
> {
> const __be32 *p = *prop;
>
> dr_cell->seq_lmbs = of_read_number(p++, 1);
> - dr_cell->base_addr = dt_mem_next_cell(dt_root_addr_cells, &p);
> + dr_cell->base_addr = of_read_number(p, n_root_addr_cells);
> + p += n_root_addr_cells;
Same comment as above.
> dr_cell->drc_index = of_read_number(p++, 1);
> dr_cell->aa_index = of_read_number(p++, 1);
> dr_cell->flags = of_read_number(p++, 1);
> @@ -239,7 +242,7 @@ static void __init read_drconf_v2_cell(struct of_drconf_cell_v2 *dr_cell,
> *prop = p;
> }
>
> -static void __init __walk_drmem_v2_lmbs(const __be32 *prop, const __be32 *usm,
> +static void __walk_drmem_v2_lmbs(const __be32 *prop, const __be32 *usm,
> void (*func)(struct drmem_lmb *, const __be32 **))
> {
> struct of_drconf_cell_v2 dr_cell;
> @@ -275,6 +278,9 @@ void __init walk_drmem_lmbs_early(unsigned long node,
> const __be32 *prop, *usm;
> int len;
>
> + if (n_root_addr_cells == 0)
> + n_root_addr_cells = dt_root_addr_cells;
> +
As I mentioned initially whats the point? Why not just use "dt_root_addr_cells"?
> prop = of_get_flat_dt_prop(node, "ibm,lmb-size", &len);
> if (!prop || len < dt_root_size_cells * sizeof(__be32))
> return;
> @@ -353,24 +359,26 @@ void __init walk_drmem_lmbs(struct device_node *dn,
> }
> }
>
> -static void __init init_drmem_v1_lmbs(const __be32 *prop)
> +static void init_drmem_v1_lmbs(const __be32 *prop,
> + struct drmem_lmb_info *dinfo)
> {
> struct drmem_lmb *lmb;
>
> - drmem_info->n_lmbs = of_read_number(prop++, 1);
> - if (drmem_info->n_lmbs == 0)
> + dinfo->n_lmbs = of_read_number(prop++, 1);
> + if (dinfo->n_lmbs == 0)
> return;
>
> - drmem_info->lmbs = kcalloc(drmem_info->n_lmbs, sizeof(*lmb),
> + dinfo->lmbs = kcalloc(dinfo->n_lmbs, sizeof(*lmb),
> GFP_KERNEL);
> - if (!drmem_info->lmbs)
> + if (!dinfo->lmbs)
> return;
>
> - for_each_drmem_lmb(lmb)
> + for_each_dinfo_lmb(dinfo, lmb)
> read_drconf_v1_cell(lmb, &prop);
> }
>
> -static void __init init_drmem_v2_lmbs(const __be32 *prop)
> +static void init_drmem_v2_lmbs(const __be32 *prop,
> + struct drmem_lmb_info *dinfo)
> {
> struct drmem_lmb *lmb;
> struct of_drconf_cell_v2 dr_cell;
> @@ -386,12 +394,12 @@ static void __init init_drmem_v2_lmbs(const __be32 *prop)
> p = prop;
> for (i = 0; i < lmb_sets; i++) {
> read_drconf_v2_cell(&dr_cell, &p);
> - drmem_info->n_lmbs += dr_cell.seq_lmbs;
> + dinfo->n_lmbs += dr_cell.seq_lmbs;
> }
>
> - drmem_info->lmbs = kcalloc(drmem_info->n_lmbs, sizeof(*lmb),
> + dinfo->lmbs = kcalloc(dinfo->n_lmbs, sizeof(*lmb),
> GFP_KERNEL);
> - if (!drmem_info->lmbs)
> + if (!dinfo->lmbs)
> return;
>
> /* second pass, read in the LMB information */
> @@ -402,10 +410,10 @@ static void __init init_drmem_v2_lmbs(const __be32 *prop)
> read_drconf_v2_cell(&dr_cell, &p);
>
> for (j = 0; j < dr_cell.seq_lmbs; j++) {
> - lmb = &drmem_info->lmbs[lmb_index++];
> + lmb = &dinfo->lmbs[lmb_index++];
>
> lmb->base_addr = dr_cell.base_addr;
> - dr_cell.base_addr += drmem_info->lmb_size;
> + dr_cell.base_addr += dinfo->lmb_size;
>
> lmb->drc_index = dr_cell.drc_index;
> dr_cell.drc_index++;
> @@ -416,11 +424,38 @@ static void __init init_drmem_v2_lmbs(const __be32 *prop)
> }
> }
>
> +void drmem_lmbs_free(struct drmem_lmb_info *dinfo)
> +{
> + if (dinfo) {
> + kfree(dinfo->lmbs);
> + kfree(dinfo);
> + }
> +}
> +
> +struct drmem_lmb_info *drmem_lmbs_init(struct property *prop)
> +{
> + struct drmem_lmb_info *dinfo;
> +
> + dinfo = kzalloc(sizeof(*dinfo), GFP_KERNEL);
> + if (!dinfo)
> + return NULL;
> +
> + if (!strcmp("ibm,dynamic-memory", prop->name))
> + init_drmem_v1_lmbs(prop->value, dinfo);
> + else if (!strcmp("ibm,dynamic-memory-v2", prop->name))
> + init_drmem_v2_lmbs(prop->value, dinfo);
> +
> + return dinfo;
> +}
> +
> static int __init drmem_init(void)
> {
> struct device_node *dn;
> const __be32 *prop;
>
> + if (n_root_addr_cells == 0)
> + n_root_addr_cells = dt_root_addr_cells;
> +
See previous comment.
-Tyrel
> dn = of_find_node_by_path("/ibm,dynamic-reconfiguration-memory");
> if (!dn) {
> pr_info("No dynamic reconfiguration memory found\n");
> @@ -434,11 +469,11 @@ static int __init drmem_init(void)
>
> prop = of_get_property(dn, "ibm,dynamic-memory", NULL);
> if (prop) {
> - init_drmem_v1_lmbs(prop);
> + init_drmem_v1_lmbs(prop, drmem_info);
> } else {
> prop = of_get_property(dn, "ibm,dynamic-memory-v2", NULL);
> if (prop)
> - init_drmem_v2_lmbs(prop);
> + init_drmem_v2_lmbs(prop, drmem_info);
> }
>
> of_node_put(dn);
>
^ permalink raw reply
* Re: [RFC 00/15] PCI: turn some __weak functions into callbacks
From: Bjorn Helgaas @ 2018-10-02 20:59 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Lorenzo Pieralisi, linux-pci, linux-kernel, Christoph Hellwig,
linux-acpi, Bjorn Helgaas, linuxppc-dev
In-Reply-To: <20180817102645.3839621-1-arnd@arndb.de>
On Fri, Aug 17, 2018 at 12:26:30PM +0200, Arnd Bergmann wrote:
> Hi Bjorn and others,
>
> Triggered by Christoph's patches, I had another go at converting
> all of the remaining pci host bridge implementations to be based
> on pci_alloc_host_bridge and a separate registration function.
>
> This is made possible through work from Lorenzo and others to
> convert many of the existing drivers, as well as the removal
> of some of the older architectures that nobody used.
>
> I'm adding a bit of duplication into the less maintained code
> here, but it makes everything more consistent, and gives an
> easy place to hook up callback functions etc.
>
> The three parts of this series are:
>
> a) push up the registration into the callers (this is where
> code gets added)
> b) clean up some of the more common host bridge
> implementations again to integrate that code better.
> This could be done for the rest as well, or we could just
> leave them alone.
> c) start moving the __weak functions into callbacks in
> pci_host_bridge. This is intentionally incomplete, since
> it is a lot of work to do it for all those functions,
> and I want to get consensus on the approach first, as well
> as maybe get other developers to help out with the rest.
>
> Please have a look.
>
> Arnd
>
> [1] https://lore.kernel.org/lkml/4288331.jNpl6KXlNO@wuerfel/
> [2] https://patchwork.kernel.org/patch/10555657/
>
> Arnd Bergmann (15):
> PCI: clean up legacy host bridge scan functions
> PCI: move pci_scan_bus into callers
> PCI: move pci_scan_root_bus into callers
> PCI: export pci_register_host_bridge
> PCI: move pci_create_root_bus into callers
> powerpc/pci: fold pci_create_root_bus into pcibios_scan_phb
> PCI/ACPI: clean up acpi_pci_root_create()
> x86: PCI: clean up pcibios_scan_root()
> PCI: xenfront: clean up pcifront_scan_root()
> sparc/PCI: simplify pci_scan_one_pbm
> PCI: hyperv: convert to pci_scan_root_bus_bridge
> PCI: make pcibios_bus_add_device() a callback function
> PCI: turn pcibios_alloc_irq into a callback
> PCI: make pcibios_root_bridge_prepare a callback
> PCI: make pcibios_add_bus/remove_bus callbacks
>
> arch/arm64/kernel/pci.c | 40 ++-----
> arch/ia64/pci/pci.c | 25 +----
> arch/ia64/sn/kernel/io_init.c | 27 +++++
> arch/microblaze/pci/pci-common.c | 27 +++++
> arch/powerpc/include/asm/pci-bridge.h | 3 +
> arch/powerpc/kernel/pci-common.c | 60 +++++------
> arch/s390/pci/pci.c | 30 +++++-
> arch/sh/drivers/pci/pci.c | 1 +
> arch/sh/drivers/pci/pcie-sh7786.c | 3 +-
> arch/sh/include/asm/pci.h | 2 +
> arch/sparc/kernel/pci.c | 40 ++++---
> arch/sparc/kernel/pcic.c | 35 ++++++
> arch/x86/pci/acpi.c | 15 +--
> arch/x86/pci/common.c | 42 ++++----
> arch/xtensa/kernel/pci.c | 27 +++++
> drivers/acpi/pci_root.c | 43 +++++---
> drivers/parisc/dino.c | 28 +++++
> drivers/parisc/lba_pci.c | 28 +++++
> drivers/pci/bus.c | 8 +-
> drivers/pci/controller/pci-hyperv.c | 47 ++++----
> drivers/pci/controller/vmd.c | 30 +++++-
> drivers/pci/hotplug/ibmphp_core.c | 35 ++++++
> drivers/pci/pci-driver.c | 13 ++-
> drivers/pci/probe.c | 150 +++++++++-----------------
> drivers/pci/xen-pcifront.c | 40 +++----
> include/linux/acpi.h | 2 +
> include/linux/pci.h | 17 ++-
> 27 files changed, 514 insertions(+), 304 deletions(-)
Sorry for the late response to this.
I think I'm generally on-board with this. I admit I'm a little
hesitant about adding 200 lines of code when this is really more
"cleanup" than new functionality, but I think a lot of that is because
this series contains costs (e.g., duplicating code) for everybody but
only has the corresponding benefits for a few (ACPI, x86, xenfront).
Those cases are much closer to parity in terms of lines added/removed.
I saw some minor comments that suggested you had some updates, so I'll
watch for an updated posting.
Bjorn
^ permalink raw reply
* Re: [PATCH v03 3/5] migration/memory: Add hotplug READD_MULTIPLE
From: Tyrel Datwyler @ 2018-10-02 21:03 UTC (permalink / raw)
To: Michael Bringmann, linuxppc-dev
Cc: Nathan Fontenot, Juliet Kim, Thomas Falcon
In-Reply-To: <20181001125952.2676.35168.stgit@ltcalpine2-lp9.aus.stglabs.ibm.com>
On 10/01/2018 05:59 AM, Michael Bringmann wrote:
> migration/memory: This patch adds a new pseries hotplug action
> for CPU and memory operations, PSERIES_HP_ELOG_ACTION_READD_MULTIPLE.
> This is a variant of the READD operation which performs the action
> upon multiple instances of the resource at one time. The operation
> is to be triggered by device-tree analysis of updates by RTAS events
> analyzed by 'migation_store' during post-migration processing. It
> will be used for memory updates, initially.
>
> Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
> ---
> arch/powerpc/include/asm/rtas.h | 1 +
> arch/powerpc/mm/drmem.c | 1 +
> 2 files changed, 2 insertions(+)
>
> diff --git a/arch/powerpc/include/asm/rtas.h b/arch/powerpc/include/asm/rtas.h
> index 71e393c..e510d82 100644
> --- a/arch/powerpc/include/asm/rtas.h
> +++ b/arch/powerpc/include/asm/rtas.h
> @@ -320,6 +320,7 @@ struct pseries_hp_errorlog {
> #define PSERIES_HP_ELOG_ACTION_ADD 1
> #define PSERIES_HP_ELOG_ACTION_REMOVE 2
> #define PSERIES_HP_ELOG_ACTION_READD 3
> +#define PSERIES_HP_ELOG_ACTION_READD_MULTIPLE 4
I'm confused, you have only added a define and not the actual implementation. I really think this should be squashed into your 4th patch where the operation is actually implemented.
>
> #define PSERIES_HP_ELOG_ID_DRC_NAME 1
> #define PSERIES_HP_ELOG_ID_DRC_INDEX 2
> diff --git a/arch/powerpc/mm/drmem.c b/arch/powerpc/mm/drmem.c
> index fd2cae92..2228586 100644
> --- a/arch/powerpc/mm/drmem.c
> +++ b/arch/powerpc/mm/drmem.c
> @@ -422,6 +422,7 @@ static void init_drmem_v2_lmbs(const __be32 *prop,
>
> lmb->aa_index = dr_cell.aa_index;
> lmb->flags = dr_cell.flags;
> + lmb->internal_flags = 0;
And this should have been squashed into the previous patch where you added the internal_flags field to the lmb struct.
-Tyrel
> }
> }
> }
>
^ 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