Re: [kvm-unit-tests RFC PATCH v3 5/7] lib: arm64: Add support for disabling and re-enabling VHE

[Marc Zyngier <maz@kernel.org>]

Hi Alex,

On 2020-01-31 09:52, Alexandru Elisei wrote:
> 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@arm.com>
>>> ---
>>>  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).

Fair enough. You're just propagating a dummy value, which is going to
bite you later.

> 
>> 
>>> +    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'm sorry, but if your reasoning that above that VHE's TTBR0_EL1 is the
same as nVHE's TTBR0_EL2 appears correct (they are accessing the same HW
register), this particular read of TTBR0_EL1 is *not* an EL2 register at
all. VHE is off, and you are reading an uninitialized EL1 register (and
it's easy to spot in KVM, as it has the magic poison value).

> 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.

I don't mind the extra writes, or even the confusion. But the above 
looks
totally wrong.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...