Hi, Thank you for testing the patches! On 1/30/20 5:40 PM, Marc Zyngier wrote: > Hi Alexandru, > > On 2020-01-02 13:46, Alexandru Elisei wrote: >> Add a function to disable VHE and another one to re-enable VHE. Both >> functions work under the assumption that the CPU had VHE mode enabled at >> boot. >> >> Minimal support to run with VHE has been added to the TLB invalidate >> functions and to the exception handling code. >> >> Since we're touch the assembly enable/disable MMU code, let's take this >> opportunity to replace a magic number with the proper define. > > I've been using this test case to debug my NV code... only to realize > after a few hours of banging my head on the wall that it is the test > that needed debugging, see below... ;-) > >> >> Signed-off-by: Alexandru Elisei <alexandru.elisei@xxxxxxx> >> --- >> lib/arm64/asm/mmu.h | 11 ++- >> lib/arm64/asm/pgtable-hwdef.h | 53 ++++++++--- >> lib/arm64/asm/processor.h | 19 +++- >> lib/arm64/processor.c | 37 +++++++- >> arm/cstart64.S | 204 ++++++++++++++++++++++++++++++++++++++++-- >> 5 files changed, 300 insertions(+), 24 deletions(-) > > [...] > >> --- a/arm/cstart64.S >> +++ b/arm/cstart64.S >> @@ -104,6 +104,13 @@ exceptions_init: >> >> .text >> >> +exceptions_init_nvhe: >> + adrp x0, vector_table_nvhe >> + add x0, x0, :lo12:vector_table_nvhe >> + msr vbar_el2, x0 >> + isb >> + ret >> + >> .globl get_mmu_off >> get_mmu_off: >> adrp x0, auxinfo >> @@ -203,7 +210,7 @@ asm_mmu_enable: >> TCR_IRGN_WBWA | TCR_ORGN_WBWA | \ >> TCR_SHARED >> mrs x2, id_aa64mmfr0_el1 >> - bfi x1, x2, #32, #3 >> + bfi x1, x2, #TCR_EL1_IPS_SHIFT, #3 >> msr tcr_el1, x1 >> >> /* MAIR */ >> @@ -228,6 +235,41 @@ asm_mmu_enable: >> >> ret >> >> +asm_mmu_enable_nvhe: > > Note the "_nvhe" suffix, which implies that... > >> + tlbi alle2 >> + dsb nsh >> + >> + /* TCR */ >> + ldr x1, =TCR_EL2_RES1 | \ >> + TCR_T0SZ(VA_BITS) | \ >> + TCR_TG0_64K | \ >> + TCR_IRGN0_WBWA | TCR_ORGN0_WBWA | \ >> + TCR_SH0_IS >> + mrs x2, id_aa64mmfr0_el1 >> + bfi x1, x2, #TCR_EL2_PS_SHIFT, #3 >> + msr tcr_el2, x1 >> + >> + /* Same MAIR and TTBR0 as in VHE mode */ >> + ldr x1, =MAIR(0x00, MT_DEVICE_nGnRnE) | \ >> + MAIR(0x04, MT_DEVICE_nGnRE) | \ >> + MAIR(0x0c, MT_DEVICE_GRE) | \ >> + MAIR(0x44, MT_NORMAL_NC) | \ >> + MAIR(0xff, MT_NORMAL) >> + msr mair_el1, x1 > > ... this should be mair_el2... > >> + >> + msr ttbr0_el1, x0 > > ... and this should be ttbr0_el2. The code is definitely confusing, but not because it's wrong, but because it's doing something useless. From DDI 04876E.a, page D13-3374, the pseudocode for writing to ttbr0_el1: [..] elsif PSTATE.EL == EL2 then if HCR_EL2.E2H == '1' then TTBR0_EL2 = X[t]; else TTBR0_EL1 = X[t]; [..] We want to use the same ttbr0_el2 and mair_el2 values that we were using when VHE was on. We programmed those values when VHE was on, so we actually wrote them to ttbr0_el2 and mair_el2. We don't need to write them again now, in fact, all the previous versions of the series didn't even have the above useless writes (I assume it was a copy-and-paste mistake when I split the fixes from the el2 patches). > >> + isb >> + >> + /* SCTLR */ >> + ldr x1, =SCTLR_EL2_RES1 | \ >> + SCTLR_EL2_C | \ >> + SCTLR_EL2_I | \ >> + SCTLR_EL2_M >> + msr sctlr_el2, x1 >> + isb >> + >> + ret >> + >> /* Taken with small changes from arch/arm64/incluse/asm/assembler.h */ >> .macro dcache_by_line_op op, domain, start, end, tmp1, tmp2 >> adrp \tmp1, dcache_line_size >> @@ -242,21 +284,61 @@ asm_mmu_enable: >> dsb \domain >> .endm >> >> +clean_inval_cache: >> + adrp x0, __phys_offset >> + ldr x0, [x0, :lo12:__phys_offset] >> + adrp x1, __phys_end >> + ldr x1, [x1, :lo12:__phys_end] >> + dcache_by_line_op civac, sy, x0, x1, x2, x3 >> + isb >> + ret >> + >> .globl asm_mmu_disable >> asm_mmu_disable: >> mrs x0, sctlr_el1 >> bic x0, x0, SCTLR_EL1_M >> msr sctlr_el1, x0 >> isb >> + b clean_inval_cache >> >> - /* Clean + invalidate the entire memory */ >> - adrp x0, __phys_offset >> - ldr x0, [x0, :lo12:__phys_offset] >> - adrp x1, __phys_end >> - ldr x1, [x1, :lo12:__phys_end] >> - dcache_by_line_op civac, sy, x0, x1, x2, x3 >> +asm_mmu_disable_nvhe: >> + mrs x0, sctlr_el2 >> + bic x0, x0, SCTLR_EL2_M >> + msr sctlr_el2, x0 >> + isb >> + b clean_inval_cache >> + >> +.globl asm_disable_vhe >> +asm_disable_vhe: >> + str x30, [sp, #-16]! >> + >> + bl asm_mmu_disable >> + msr hcr_el2, xzr >> + isb > > At this stage, VHE is off... > >> + bl exceptions_init_nvhe >> + /* Make asm_mmu_enable_nvhe happy by having TTBR0 value in x0. */ >> + mrs x0, ttbr0_el1 > > ... so this is going to sample the wrong TTBR. It really should be > TTBR0_EL2! Not really, asm_mmu_enable has one parameter, the PA for the translation tables in register x0, and we are going to use the same translation tables with VHE off that we were using with VHE on. Hence the read.//It could have easily been mrs x0,ttbr0_el2, since they have the same value, which we want to reuse. I think this confusion stems from the fact that I'm trying to write the registers again in asm_mmu_enable_nvhe, when we don't have to. And writing to the wrong registers makes the confusion even worse. > >> + isb > > nit: this ISB is useless, as you will have a dependency on x0 anyway. True, I'll remove it. Thanks, Alex > > With these fixes (and a few more terrible hacks to synchronize HCR_EL2 > on ARMv8.4-NV), I can run this test reliably. > > Thanks, > > M.
