[PATCH 1/4] powerpc/64s: Always set mmu_slb_size using slb_set_size()

It's easier to reason about the code if we only set mmu_slb_size in
one place, so convert open-coded assignments to use slb_set_size().

Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/kernel/prom.c      | 2 +-
 arch/powerpc/mm/pgtable-radix.c | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
index 4181ec715f88..14693f8ccb80 100644
--- a/arch/powerpc/kernel/prom.c
+++ b/arch/powerpc/kernel/prom.c

@@ -238,7 +238,7 @@ static void __init init_mmu_slb_size(unsigned long node)
 			of_get_flat_dt_prop(node, "ibm,slb-size", NULL);
 
 	if (slb_size_ptr)
-		mmu_slb_size = be32_to_cpup(slb_size_ptr);
+		slb_set_size(be32_to_cpup(slb_size_ptr));
 }
 #else
 #define init_mmu_slb_size(node) do { } while(0)

diff --git a/arch/powerpc/mm/pgtable-radix.c b/arch/powerpc/mm/pgtable-radix.c
index 931156069a81..949fbc96b237 100644
--- a/arch/powerpc/mm/pgtable-radix.c
+++ b/arch/powerpc/mm/pgtable-radix.c

@@ -328,7 +328,8 @@ void __init radix_init_pgtable(void)
 	struct memblock_region *reg;
 
 	/* We don't support slb for radix */
-	mmu_slb_size = 0;
+	slb_set_size(0);
+
 	/*
 	 * Create the linear mapping, using standard page size for now
 	 */

-- 
2.20.1

The recent rewrite of the SLB code into C included the assumption that
all CPUs we run on have at least 32 SLB entries. This is currently
true but a bit fragile as the SLB size is actually defined by the
device tree and so could theoretically change at any time.

The assumption is encoded in the fact that we use U32_MAX as the value
for a full SLB bitmap. Instead, calculate what the full bitmap would
be based on the SLB size we're given and store it. This still requires
the SLB size to be a power of 2.

Fixes: 126b11b294d1 ("powerpc/64s/hash: Add SLB allocation status bitmaps")
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/mm/slb.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/mm/slb.c b/arch/powerpc/mm/slb.c
index bc3914d54e26..61450a9cf30d 100644
--- a/arch/powerpc/mm/slb.c
+++ b/arch/powerpc/mm/slb.c

@@ -506,9 +506,16 @@ void switch_slb(struct task_struct *tsk, struct mm_struct *mm)
 	asm volatile("isync" : : : "memory");
 }
 
+static u32 slb_full_bitmap;
+
 void slb_set_size(u16 size)
 {
 	mmu_slb_size = size;
+
+	if (size >= 32)
+		slb_full_bitmap = U32_MAX;
+	else
+		slb_full_bitmap = (1ul << size) - 1;
 }
 
 void slb_initialize(void)

@@ -611,7 +618,7 @@ 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 (local_paca->slb_used_bitmap != U32_MAX) {
+	if (local_paca->slb_used_bitmap != slb_full_bitmap) {
 		index = ffz(local_paca->slb_used_bitmap);
 		local_paca->slb_used_bitmap |= 1U << index;
 		if (kernel)

-- 
2.20.1

On machines with 1TB segments and a 32-entry SLB it's quite hard to
cause sufficient SLB pressure to trigger bugs caused due to badly
timed SLB faults.

We have seen this in the past and a few years ago added the
disable_1tb_segments command line option to force the use of 256MB
segments. However even this allows some bugs to slip through testing
if the SLB entry in question was recently accessed.

So add a new command line parameter for debugging which shrinks the
SLB to the minimum size we can support. Currently that size is 3, two
bolted SLBs and 1 for dynamic use. This creates the maximal SLB
pressure while still allowing the kernel to operate.

Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/mm/slb.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/powerpc/mm/slb.c b/arch/powerpc/mm/slb.c
index 61450a9cf30d..0f33e28f97da 100644
--- a/arch/powerpc/mm/slb.c
+++ b/arch/powerpc/mm/slb.c

@@ -506,10 +506,24 @@ void switch_slb(struct task_struct *tsk, struct mm_struct *mm)
 	asm volatile("isync" : : : "memory");
 }
 
+static bool shrink_slb = false;
+
+static int __init parse_shrink_slb(char *p)
+{
+	shrink_slb = true;
+	slb_set_size(0);
+
+	return 0;
+}
+early_param("shrink_slb", parse_shrink_slb);
+
 static u32 slb_full_bitmap;
 
 void slb_set_size(u16 size)
 {
+	if (shrink_slb)
+		size = SLB_NUM_BOLTED + 1;
+
 	mmu_slb_size = size;
 
 	if (size >= 32)

-- 
2.20.1

The SLB initialisation code is spread around a bit between prom.c and
hash_utils_64.c. Consolidate it all in hash_utils_64.c.

This slightly changes the timing of when mmu_slb_size is initialised,
but that should have no effect.

Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/kernel/prom.c      | 16 ----------------
 arch/powerpc/mm/hash_utils_64.c | 15 ++++++++++-----
 2 files changed, 10 insertions(+), 21 deletions(-)

diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
index 14693f8ccb80..018ededd1948 100644
--- a/arch/powerpc/kernel/prom.c
+++ b/arch/powerpc/kernel/prom.c

@@ -229,21 +229,6 @@ static void __init check_cpu_pa_features(unsigned long node)
 		      ibm_pa_features, ARRAY_SIZE(ibm_pa_features));
 }
 
-#ifdef CONFIG_PPC_BOOK3S_64
-static void __init init_mmu_slb_size(unsigned long node)
-{
-	const __be32 *slb_size_ptr;
-
-	slb_size_ptr = of_get_flat_dt_prop(node, "slb-size", NULL) ? :
-			of_get_flat_dt_prop(node, "ibm,slb-size", NULL);
-
-	if (slb_size_ptr)
-		slb_set_size(be32_to_cpup(slb_size_ptr));
-}
-#else
-#define init_mmu_slb_size(node) do { } while(0)
-#endif
-
 static struct feature_property {
 	const char *name;
 	u32 min_value;

@@ -379,7 +364,6 @@ static int __init early_init_dt_scan_cpus(unsigned long node,
 	}
 
 	identical_pvr_fixup(node);
-	init_mmu_slb_size(node);
 
 #ifdef CONFIG_PPC64
 	if (nthreads == 1)

diff --git a/arch/powerpc/mm/hash_utils_64.c b/arch/powerpc/mm/hash_utils_64.c
index 4aa0797000f7..33ce76be17de 100644
--- a/arch/powerpc/mm/hash_utils_64.c
+++ b/arch/powerpc/mm/hash_utils_64.c

@@ -344,9 +344,8 @@ static int __init parse_disable_1tb_segments(char *p)
 }
 early_param("disable_1tb_segments", parse_disable_1tb_segments);
 
-static int __init htab_dt_scan_seg_sizes(unsigned long node,
-					 const char *uname, int depth,
-					 void *data)
+static int __init htab_dt_scan_slb(unsigned long node, const char *uname,
+				   int depth, void *data)
 {
 	const char *type = of_get_flat_dt_prop(node, "device_type", NULL);
 	const __be32 *prop;

@@ -356,6 +355,12 @@ static int __init htab_dt_scan_seg_sizes(unsigned long node,
 	if (type == NULL || strcmp(type, "cpu") != 0)
 		return 0;
 
+	prop = of_get_flat_dt_prop(node, "slb-size", NULL);
+	if (!prop)
+		prop = of_get_flat_dt_prop(node, "ibm,slb-size", NULL);
+	if (prop)
+		slb_set_size(be32_to_cpup(prop));
+
 	prop = of_get_flat_dt_prop(node, "ibm,processor-segment-sizes", &size);
 	if (prop == NULL)
 		return 0;

@@ -954,8 +959,8 @@ static void __init htab_initialize(void)
 
 void __init hash__early_init_devtree(void)
 {
-	/* Initialize segment sizes */
-	of_scan_flat_dt(htab_dt_scan_seg_sizes, NULL);
+	/* Initialize SLB size and segment sizes */
+	of_scan_flat_dt(htab_dt_scan_slb, NULL);
 
 	/* Initialize page sizes */
 	htab_scan_page_sizes();

-- 
2.20.1

On Thu, Jan 17, 2019 at 11:13:26PM +1100, Michael Ellerman wrote:

The recent rewrite of the SLB code into C included the assumption that
all CPUs we run on have at least 32 SLB entries. This is currently
true but a bit fragile as the SLB size is actually defined by the
device tree and so could theoretically change at any time.

It also is guaranteed by the architecture, since at least 2.02, FWIW.


Segher

Segher Boessenkool [off-list ref] writes:

On Thu, Jan 17, 2019 at 11:13:26PM +1100, Michael Ellerman wrote:

quoted

The recent rewrite of the SLB code into C included the assumption that
all CPUs we run on have at least 32 SLB entries. This is currently
true but a bit fragile as the SLB size is actually defined by the
device tree and so could theoretically change at any time.

It also is guaranteed by the architecture, since at least 2.02, FWIW.

True. Actually 2.00 says at least 32.

Unfortunately we don't live in a world where "the architecture
guarantees it" has any bearing on reality :)

But given it *should* always be at least 32 maybe I should optimise for
that case. We could use a static key to skip the U32_MAX comparison and
go down the else path.

cheers

On Fri, Jan 18, 2019 at 11:28:24PM +1100, Michael Ellerman wrote:

Segher Boessenkool [off-list ref] writes:

quoted

On Thu, Jan 17, 2019 at 11:13:26PM +1100, Michael Ellerman wrote:

quoted

The recent rewrite of the SLB code into C included the assumption that
all CPUs we run on have at least 32 SLB entries. This is currently
true but a bit fragile as the SLB size is actually defined by the
device tree and so could theoretically change at any time.

It also is guaranteed by the architecture, since at least 2.02, FWIW.

True. Actually 2.00 says at least 32.

Unfortunately we don't live in a world where "the architecture
guarantees it" has any bearing on reality :)

It's a pretty strong hint.  I don't remember any hardware where it is not
true, either.  (That might be selective memory ;-) )

But given it *should* always be at least 32 maybe I should optimise for
that case. We could use a static key to skip the U32_MAX comparison and
go down the else path.

Ah that sounds like a good idea :-)


Segher

On Thu, 17 Jan 2019 23:13:28 +1100
Michael Ellerman [off-list ref] wrote:

On machines with 1TB segments and a 32-entry SLB it's quite hard to
cause sufficient SLB pressure to trigger bugs caused due to badly
timed SLB faults.

We have seen this in the past and a few years ago added the
disable_1tb_segments command line option to force the use of 256MB
segments. However even this allows some bugs to slip through testing
if the SLB entry in question was recently accessed.

So add a new command line parameter for debugging which shrinks the
SLB to the minimum size we can support. Currently that size is 3, two
bolted SLBs and 1 for dynamic use. This creates the maximal SLB

Doesn't this violate the power of 2 requirement stated in 2/4?

Thanks

Michal

Michal Suchánek [off-list ref] writes:

On Thu, 17 Jan 2019 23:13:28 +1100
Michael Ellerman [off-list ref] wrote:

quoted

On machines with 1TB segments and a 32-entry SLB it's quite hard to
cause sufficient SLB pressure to trigger bugs caused due to badly
timed SLB faults.

We have seen this in the past and a few years ago added the
disable_1tb_segments command line option to force the use of 256MB
segments. However even this allows some bugs to slip through testing
if the SLB entry in question was recently accessed.

So add a new command line parameter for debugging which shrinks the
SLB to the minimum size we can support. Currently that size is 3, two
bolted SLBs and 1 for dynamic use. This creates the maximal SLB

Doesn't this violate the power of 2 requirement stated in 2/4?

Yes. Good point. This was originally a hack patch in my tree, back when
SLB_NUM_BOLTED was 3 and before Nick even added the slb_used_bitmap, so
back then it was a power of 2 but it also didn't matter :)

I think I'll rework the slb_full_bitmap patch anyway and remove the
power of 2 requirement, so then this patch will be OK.

Thanks for the review!

cheers

Michael Ellerman's on January 19, 2019 8:27 pm:

Michal Suchánek [off-list ref] writes:

quoted

On Thu, 17 Jan 2019 23:13:28 +1100
Michael Ellerman [off-list ref] wrote:

quoted

On machines with 1TB segments and a 32-entry SLB it's quite hard to
cause sufficient SLB pressure to trigger bugs caused due to badly
timed SLB faults.

We have seen this in the past and a few years ago added the
disable_1tb_segments command line option to force the use of 256MB
segments. However even this allows some bugs to slip through testing
if the SLB entry in question was recently accessed.

So add a new command line parameter for debugging which shrinks the
SLB to the minimum size we can support. Currently that size is 3, two
bolted SLBs and 1 for dynamic use. This creates the maximal SLB

Doesn't this violate the power of 2 requirement stated in 2/4?

Yes. Good point. This was originally a hack patch in my tree, back when
SLB_NUM_BOLTED was 3 and before Nick even added the slb_used_bitmap, so
back then it was a power of 2 but it also didn't matter :)

I think I'll rework the slb_full_bitmap patch anyway and remove the
power of 2 requirement, so then this patch will be OK.

Thanks, good patch and really will help testing. Sometimes even if you 
can get enough pressure with the 256MB segments, you actually want to 
test 1TB segment code paths too.

Thanks,
Nick

Michael Ellerman [off-list ref] writes:

It's easier to reason about the code if we only set mmu_slb_size in
one place, so convert open-coded assignments to use slb_set_size().

Reviewed-by: Aneesh Kumar K.V <redacted>

Michael Ellerman [off-list ref] writes:

The recent rewrite of the SLB code into C included the assumption that
all CPUs we run on have at least 32 SLB entries. This is currently
true but a bit fragile as the SLB size is actually defined by the
device tree and so could theoretically change at any time.

The assumption is encoded in the fact that we use U32_MAX as the value
for a full SLB bitmap. Instead, calculate what the full bitmap would
be based on the SLB size we're given and store it. This still requires
the SLB size to be a power of 2.

So if it is less than 32 we want to make sure we don't allocate an index
above 32. Is this the reason for that radom assert_slb_presence?

Reviewed-by: Aneesh Kumar K.V <redacted> 

Michael Ellerman [off-list ref] writes:

The SLB initialisation code is spread around a bit between prom.c and
hash_utils_64.c. Consolidate it all in hash_utils_64.c.

This slightly changes the timing of when mmu_slb_size is initialised,
but that should have no effect.

Reviewed-by: Aneesh Kumar K.V <redacted>

Michael Ellerman [off-list ref] writes:

On machines with 1TB segments and a 32-entry SLB it's quite hard to
cause sufficient SLB pressure to trigger bugs caused due to badly
timed SLB faults.

We have seen this in the past and a few years ago added the
disable_1tb_segments command line option to force the use of 256MB
segments. However even this allows some bugs to slip through testing
if the SLB entry in question was recently accessed.

So add a new command line parameter for debugging which shrinks the
SLB to the minimum size we can support. Currently that size is 3, two
bolted SLBs and 1 for dynamic use. This creates the maximal SLB
pressure while still allowing the kernel to operate.

Should we put this within DEBUG_VM? 

Reviewed-by: Aneesh Kumar K.V <redacted> 

Why do we need call slb_set_size(0) here? htab_dt_scan_seg_sizes should 
find the shirnk_slb = true?

+
+	return 0;
+}
+early_param("shrink_slb", parse_shrink_slb);
+
 static u32 slb_full_bitmap;
 
 void slb_set_size(u16 size)
 {
+	if (shrink_slb)
+		size = SLB_NUM_BOLTED + 1;
+
 	mmu_slb_size = size;
 
 	if (size >= 32)
-- 
2.20.1

-aneesh