Re: [PATCH V4 1/3] powerpc/mm: Add support for handling > 512TB address in SLB miss
From: Michael Ellerman <mpe@ellerman.id.au>
Date: 2018-03-16 06:35:27
Hi Aneesh, Some comments ... "Aneesh Kumar K.V" [off-list ref] writes:
For address above 512TB we allocate additional mmu context. To make it all easy address above 512TB is handled with IR/DR=3D1 and with stack frame s=
etup.
We do the additional context allocation in SLB miss handler. If the conte=
xt is
not allocated, we enable interrupts and allocate the context and retry the access which will again result in a SLB miss. Signed-off-by: Aneesh Kumar K.V <redacted>
This doesn't build for g5_defconfig:
In file included from ../arch/powerpc/include/asm/mmu.h:314:0,
from ../arch/powerpc/include/asm/lppaca.h:36,
from ../arch/powerpc/include/asm/paca.h:21,
from ../arch/powerpc/include/asm/hw_irq.h:63,
from ../arch/powerpc/include/asm/irqflags.h:12,
from ../include/linux/irqflags.h:16,
from ../include/linux/spinlock.h:54,
from ../include/linux/mmzone.h:8,
from ../arch/powerpc/include/asm/pgtable.h:7,
from ../arch/powerpc/mm/slb.c:17:
../arch/powerpc/mm/slb.c: In function =E2=80=98handle_multi_context_slb_m=
iss=E2=80=99:
../arch/powerpc/include/asm/book3s/64/mmu.h:208:25: error: array subscrip=
t is above array bounds [-Werror=3Darray-bounds]
return ctx->extended_id[index];
~~~~~~~~~~~~~~~~^~~~~~~
../arch/powerpc/mm/slb.c:417:30: error: array subscript is above array bo=
unds [-Werror=3Darray-bounds]
if (!mm->context.extended_id[index])
~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~
../arch/powerpc/mm/slb.c:418:26: error: array subscript is above array bo=
unds [-Werror=3Darray-bounds]
mm->context.extended_id[index] =3D context_id;
~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~
quoted hunk ↗ jump to hunk
diff --git a/arch/powerpc/include/asm/book3s/64/hash-4k.h b/arch/powerpc/=
include/asm/book3s/64/hash-4k.h
quoted hunk ↗ jump to hunk
index 67c5475311ee..af2ba9875f18 100644--- a/arch/powerpc/include/asm/book3s/64/hash-4k.h +++ b/arch/powerpc/include/asm/book3s/64/hash-4k.h@@ -11,6 +11,12 @@ #define H_PUD_INDEX_SIZE 9 #define H_PGD_INDEX_SIZE 9=20=20 +/* + * No of address bits below which we use the default context
Can we use "Number", "No" means no, to mean number you need "No." but that's ugly.
quoted hunk ↗ jump to hunk
+ * for slb allocation. For 4k this is 64TB. + */ +#define H_BITS_FIRST_CONTEXT 46
It's actually the number of address bits *per* context right? "Context" is also a bit of a overloaded term, eg. context !=3D mm_context_t, maybe "context id" would be clearer? So maybe H_BITS_PER_CONTEXT_ID? This values is essentially VA_BITS - CONTEXT_BITS right? Except VA_BITS is actually 65 in this case, but that's not clear at all from the code :/ It'd be really nice if this was calculated, or if the other values (VA_BITS/CONTEXT_BITS) were calculated based on it.
quoted hunk ↗ jump to hunk
diff --git a/arch/powerpc/include/asm/book3s/64/hash-64k.h b/arch/powerpc=
/include/asm/book3s/64/hash-64k.h
quoted hunk ↗ jump to hunk
index 3bcf269f8f55..0ee0fc1ad675 100644--- a/arch/powerpc/include/asm/book3s/64/hash-64k.h +++ b/arch/powerpc/include/asm/book3s/64/hash-64k.h@@ -6,6 +6,11 @@ #define H_PMD_INDEX_SIZE 10 #define H_PUD_INDEX_SIZE 7 #define H_PGD_INDEX_SIZE 8
Can we get some newlines between code?
quoted hunk ↗ jump to hunk
+/* + * No of address bits below which we use the default context + * for slb allocation. For 64k this is 512TB. + */ +#define H_BITS_FIRST_CONTEXT 49
This really is =3D=3D VA_BITS - CONTEXT_BITS.
quoted hunk ↗ jump to hunk
diff --git a/arch/powerpc/include/asm/book3s/64/mmu-hash.h b/arch/powerpc=
/include/asm/book3s/64/mmu-hash.h
quoted hunk ↗ jump to hunk
index 50ed64fba4ae..8ee83f6e9c84 100644--- a/arch/powerpc/include/asm/book3s/64/mmu-hash.h +++ b/arch/powerpc/include/asm/book3s/64/mmu-hash.h@@ -691,8 +691,8 @@ static inline int user_segment_size(unsigned long add=
r)
return MMU_SEGSIZE_256M; } =20=20 -static inline unsigned long get_vsid(unsigned long context, unsigned lon=
g ea,
- int ssize) +static inline unsigned long __get_vsid(unsigned long context, unsigned l=
ong ea,
+ int ssize)
If you're going to realign it the 'i' in int should line up with the 'u' in unsigned. Why did this become __get_vsid()? We don't have a get_vsid() that I can see?
quoted hunk ↗ jump to hunk
diff --git a/arch/powerpc/include/asm/book3s/64/mmu.h b/arch/powerpc/incl=
ude/asm/book3s/64/mmu.h
quoted hunk ↗ jump to hunk
index 777778579305..a70adbb7ec56 100644--- a/arch/powerpc/include/asm/book3s/64/mmu.h +++ b/arch/powerpc/include/asm/book3s/64/mmu.h@@ -91,7 +91,15 @@ struct slice_mask { };=20=20 typedef struct { - mm_context_id_t id; + union { + /* + * One context for each 512TB. + * First 512TB context is saved in id and is also used + * as PIDR. + */
Can you make it clearer how this is used on hash vs radix.
+ mm_context_id_t id; + mm_context_id_t extended_id[TASK_SIZE_USER64/TASK_CONTEXT_SIZE];
If my math is right this is typically 4PB / 512T =3D=3D 8 =3D=3D 64 bytes. Which is small enough to not worry about, but would be good to mention in the change log at least.
quoted hunk ↗ jump to hunk
+ }; u16 user_psize; /* page size index */ =20=20 /* Number of bits in the mm_cpumask */@@ -193,5 +201,21 @@ extern void radix_init_pseries(void); static inline void radix_init_pseries(void) { }; #endif=20=20 +static inline int get_esid_context(mm_context_t *ctx, unsigned long ea) +{ + int index =3D ea >> H_BITS_FIRST_CONTEXT;
Using "esid" in the name is a bit confusing, because the value is an EA, not an ESID. So change the name or what's passed? I think change the name, the callers both have an EA so it makes sense to pass that.
+
I know we check ea elsewhere, but a: VM_BUG_ON(index >=3D ARRAY_SIZE(extened_id)) Would make me feel better I think, and index being unsigned.
quoted hunk ↗ jump to hunk
diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/=
asm/processor.h
quoted hunk ↗ jump to hunk
index 01299cdc9806..70d65b482504 100644--- a/arch/powerpc/include/asm/processor.h +++ b/arch/powerpc/include/asm/processor.h@@ -119,9 +119,16 @@ void release_thread(struct task_struct *); */ #define TASK_SIZE_USER64 TASK_SIZE_512TB #define DEFAULT_MAP_WINDOW_USER64 TASK_SIZE_128TB +#define TASK_CONTEXT_SIZE TASK_SIZE_512TB #else #define TASK_SIZE_USER64 TASK_SIZE_64TB #define DEFAULT_MAP_WINDOW_USER64 TASK_SIZE_64TB +/* + * We don't need allocate extended context id for 4K + * page size. We limit max address on this config to + * 64TB. + */
/* * We don't need to allocate extended context ids for 4K page size, because * we limit the max effective address on this config to 64TB. */ ???
quoted hunk ↗ jump to hunk
diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/e=
xceptions-64s.S
quoted hunk ↗ jump to hunk
index 3ac87e53b3da..166b8c0f1830 100644--- a/arch/powerpc/kernel/exceptions-64s.S +++ b/arch/powerpc/kernel/exceptions-64s.S@@ -620,8 +620,12 @@ END_MMU_FTR_SECTION_IFCLR(MMU_FTR_TYPE_RADIX) ld r10,PACA_EXSLB+EX_LR(r13) lwz r9,PACA_EXSLB+EX_CCR(r13) /* get saved CR */ mtlr r10
Blank line would be nice.
quoted hunk ↗ jump to hunk
+ /* + * Large address, check whether we have to allocate new + * contexts. + */
That doesn't need to wrap. /* Large address, check whether we need to allocate new contexts. */
+ beq- 8f =20=20 - beq- 8f /* if bad address, make full stack frame */
And we ended up with two blank lines here which is probably overkill. =20
quoted hunk ↗ jump to hunk
@@ -710,7 +714,7 @@ EXC_COMMON_BEGIN(bad_addr_slb) std r10, _TRAP(r1) 2: bl save_nvgprs addi r3, r1, STACK_FRAME_OVERHEAD - bl slb_miss_bad_addr
You removed the only call to this but didn't remove the function or its prototype AFAICS.
quoted hunk ↗ jump to hunk
diff --git a/arch/powerpc/mm/mmu_context_book3s64.c b/arch/powerpc/mm/mmu=
_context_book3s64.c
quoted hunk ↗ jump to hunk
index 80acad52b006..ccc88fa7c35c 100644--- a/arch/powerpc/mm/mmu_context_book3s64.c +++ b/arch/powerpc/mm/mmu_context_book3s64.c@@ -178,6 +178,19 @@ void __destroy_context(int context_id) } EXPORT_SYMBOL_GPL(__destroy_context);=20=20 +static void destroy_contexts(mm_context_t *ctx) +{ + int index, context_id; + + spin_lock(&mmu_context_lock); + for (index =3D 0; index < (TASK_SIZE_USER64/TASK_CONTEXT_SIZE); index++=
) {
ARRAY_SIZE(ctx->extended_id) ?
quoted hunk ↗ jump to hunk
diff --git a/arch/powerpc/mm/slb.c b/arch/powerpc/mm/slb.c index 13cfe413b40d..a93887ee9b22 100644 --- a/arch/powerpc/mm/slb.c +++ b/arch/powerpc/mm/slb.c@@ -23,6 +23,7 @@ #include <asm/smp.h> #include <linux/compiler.h> #include <linux/mm_types.h> +#include <linux/context_tracking.h>
Our includes are generally not well sorted, but can you try to keep them alphabetical. So after compiler.h.
quoted hunk ↗ jump to hunk
@@ -340,3 +341,156 @@ void slb_initialize(void)=20=20 asm volatile("isync":::"memory"); } + +/* + * Only handle insert of 1TB slb entries.
What if we don't have them (enabled) ?
quoted hunk ↗ jump to hunk
+ */ +static void insert_slb_entry(unsigned long vsid, unsigned long ea, + int bpsize, int ssize)
Parameter alignment again.
quoted hunk ↗ jump to hunk
+{ + int slb_cache_index; + unsigned long flags; + enum slb_index index; + unsigned long vsid_data, esid_data;
Can you clean those up: unsigned long vsid_data, esid_data, flags; enum slb_index index; int slb_cache_index;
quoted hunk ↗ jump to hunk
+ + /* + * We are irq disabled, hence should be safe + * to access PACA.
Doesn't need to wrap.
+ */ + index =3D get_paca()->stab_rr;
^ Unneeded extra space there Blank line please.
+ /* + * simple round roubin replacement of slb.
"Simple round-robin" Although it's not really, it's round-robin but not starting at zero, starting at SLB_NUM_BOLTED.
quoted hunk ↗ jump to hunk
+ */ + if (index < mmu_slb_size) + index++; + else + index =3D SLB_NUM_BOLTED;
Blank line.
quoted hunk ↗ jump to hunk
+ get_paca()->stab_rr =3D index; + + flags =3D SLB_VSID_USER | mmu_psize_defs[bpsize].sllp; + vsid_data =3D (vsid << SLB_VSID_SHIFT_1T) | flags | + ((unsigned long) ssize << SLB_VSID_SSIZE_SHIFT);
vsid_data =3D (vsid << SLB_VSID_SHIFT_1T) | flags |
((unsigned long) ssize << SLB_VSID_SSIZE_SHIFT);
quoted hunk ↗ jump to hunk
+ esid_data =3D mk_esid_data(ea, mmu_highuser_ssize, index); + + asm volatile("slbmte %0, %1" : : "r" (vsid_data), "r" (esid_data) + : "memory");
Blank line.
quoted hunk ↗ jump to hunk
+ /* + * Now update slb cache entries + */ + slb_cache_index =3D get_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++] =3D esid_data >> 28; + }
Blank line.
quoted hunk ↗ jump to hunk
+ /* + * if we are full, just increment and return. + */ + get_paca()->slb_cache_ptr++; +} + +static void alloc_extended_context(struct mm_struct *mm, unsigned long e=
a)
quoted hunk ↗ jump to hunk
+{ + int context_id; + + int index =3D ea >> H_BITS_FIRST_CONTEXT; + + /* + * we need to do locking only here. If this value was not set before + * we will have taken an SLB miss and will reach here. The value will + * be either 0 or a valid extended context. We need to make sure two + * parallel SLB miss don't end up allocating extended_context for the + * same range. The locking below ensures that. For now we take the + * heavy mmap_sem. But can be changed to per mm_context_t custom lock + * if needed. + */ + down_read(&mm->mmap_sem);
quoted hunk ↗ jump to hunk
+ context_id =3D hash__alloc_context_id(); + if (context_id < 0) { + up_read(&mm->mmap_sem); + pagefault_out_of_memory(); + return; + }
quoted hunk ↗ jump to hunk
+ /* Check for parallel allocation after holding lock */ + if (!mm->context.extended_id[index]) + mm->context.extended_id[index] =3D context_id; + else + __destroy_context(context_id);
quoted hunk ↗ jump to hunk
+ up_read(&mm->mmap_sem); +} + +static void __handle_multi_context_slb_miss(struct pt_regs *regs, + unsigned long ea) +{ + int context, bpsize; + unsigned long vsid; + struct mm_struct *mm =3D current->mm; + + context =3D get_esid_context(&mm->context, ea); + if (!context) { + /* + * haven't allocated context yet for this range. + * Enable irq and allo context and return. We will + * take an slb miss on this again and come here with + * allocated context. + */ + /* We restore the interrupt state now */ + if (!arch_irq_disabled_regs(regs)) + local_irq_enable(); + return alloc_extended_context(mm, ea);
I guess I thought we'd do this somewhere in the slice code, when an address above the limit is first used? Doing it this way means a stray access to >=3D 512TB will get a lot further, ie. we'll come in here allocate a context etc, install an SLB entry and only then realise there's nothing mapped when we look in the hash. I'd prefer we caught it earlier if possible.
quoted hunk ↗ jump to hunk
+ } + /* + * We are always above 1TB, hence use high user segment size. + */ + vsid =3D __get_vsid(context, ea, mmu_highuser_ssize); + bpsize =3D get_slice_psize(mm, ea); + + insert_slb_entry(vsid, ea, bpsize, mmu_highuser_ssize); +} + +/* + * exception_enter() handling? FIXME!!
???
+ */ +void handle_multi_context_slb_miss(struct pt_regs *regs)
I don't love this name, maybe slb_miss_large_addr()? That would also free-up the non-underscore name for the actual core of the handler above.
quoted hunk ↗ jump to hunk
+{ + enum ctx_state prev_state =3D exception_enter(); + unsigned long ea =3D regs->dar; + + /* + * Kernel always runs with single context. Hence + * anything that request for multi context is + * considered bad slb request. + */ + if (!user_mode(regs)) + return bad_page_fault(regs, ea, SIGSEGV);
Can't copy/to_from_user() hit this case?
+ if (REGION_ID(ea) !=3D USER_REGION_ID) + goto slb_bad_addr;
Blank line please :)
+ /* + * Are we beyound what the page table layout support ?
"supports" ?
quoted hunk ↗ jump to hunk
+ */ + if ((ea & ~REGION_MASK) >=3D H_PGTABLE_RANGE) + goto slb_bad_addr; + +#ifdef CONFIG_PPC_MM_SLICES
That can only ever be true in this file, right?
quoted hunk ↗ jump to hunk
+ /* + * consider this as bad slb if we take an slb miss + * on an address above addr limit.
It's a bad "access" not a "bad slb". We should probably spell slb SLB in the comments too.
quoted hunk ↗ jump to hunk
+ */ + if (ea >=3D current->mm->context.slb_addr_limit) + goto slb_bad_addr; +#endif
Blank ..
+ /* Lower address should be handled by asm code */
"should have been handled"
quoted hunk ↗ jump to hunk
+ if (ea < (1UL << H_BITS_FIRST_CONTEXT)) + goto slb_bad_addr; + + __handle_multi_context_slb_miss(regs, ea); + exception_exit(prev_state); + return; + +slb_bad_addr: + _exception(SIGSEGV, regs, SEGV_BNDERR, ea); + exception_exit(prev_state); +}
I think this would work better if we had:
void slb_bad_addr(regs, ea)
{
_exception(SIGSEGV, regs, SEGV_BNDERR, ea);
}
...
void handle_multi_context_slb_miss(struct pt_regs *regs)
{
...
if (ea < (1UL << H_BITS_FIRST_CONTEXT)) {
slb_bad_addr(regs, ea);
goto out;
}
...
__handle_multi_context_slb_miss(regs, ea);
out:
exception_exit(prev_state);
}
quoted hunk ↗ jump to hunk
diff --git a/arch/powerpc/mm/slb_low.S b/arch/powerpc/mm/slb_low.S index 2c7c717fd2ea..c66cb06e73a1 100644 --- a/arch/powerpc/mm/slb_low.S +++ b/arch/powerpc/mm/slb_low.S@@ -75,10 +75,12 @@ ALT_MMU_FTR_SECTION_END_IFCLR(MMU_FTR_68_BIT_VA) */ _GLOBAL(slb_allocate) /* - * check for bad kernel/user address + * Check for address range for which we need to handle multi context. F=
or
+ * the default context we allocate the slb via the fast path. Fo=
r large
+ * address we branch out to C-code and look at additional contex=
t allocated. Leading white space is fubar. cheers