Hi,
On 12/13/19 6:42 PM, Andrew Jones wrote:
> On Thu, Nov 28, 2019 at 06:04:15PM +0000, Alexandru Elisei wrote:
>> When the MMU is off, data accesses are to Device nGnRnE memory on arm64 [1]
>> or to Strongly-Ordered memory on arm [2]. This means that the accesses are
>> non-cacheable.
>>
>> Perform a dcache clean to PoC so we can read the newer values from the
>> cache, instead of the stale values from memory.
>>
>> Perform an invalidation so when we re-enable the MMU, we can access the
>> data written to memory while the MMU was off, instead of potentially
>> stale values from the cache.
>>
>> Data caches are PIPT and the VAs are translated using the current
>> translation tables, or an identity mapping (what Arm calls a "flat
>> mapping") when the MMU is off [1], [2]. Do the clean + invalidate when the
>> MMU is off so we don't depend on the current translation tables and we can
>> make sure that the operation applies to the entire physical memory.
>>
>> [1] ARM DDI 0487E.a, section D5.2.9
>> [2] ARM DDI 0406C.d, section B3.2.1
>>
>> Signed-off-by: Alexandru Elisei <alexandru.elisei@xxxxxxx>
>> ---
>>
>> Tested with the following hack:
>>
>> diff --git a/arm/selftest.c b/arm/selftest.c
>> index e9dc5c0cab28..7f29548bc468 100644
>> --- a/arm/selftest.c
>> +++ b/arm/selftest.c
>> @@ -350,10 +350,21 @@ static void cpu_report(void *data __unused)
>> report_info("CPU%3d: MPIDR=%010" PRIx64, cpu, mpidr);
>> }
>>
>> +#include <alloc_page.h>
>> +#include <asm/mmu.h>
>> int main(int argc, char **argv)
>> {
>> + int *x = alloc_page();
>> +
>> report_prefix_push("selftest");
>>
>> + *x = 0x42;
>> + mmu_disable();
>> + report("read back value written with MMU on", *x == 0x42);
>> + *x = 0x50;
>> + mmu_enable(current_thread_info()->pgtable);
>> + report("read back value written with MMU off", *x == 0x50);
>> +
>> if (argc < 2)
>> report_abort("no test specified");
> When applying this patch the above hack also gets applied, and I'm
> guessing that wasn't the intent. It can go above the ---, as it's useful
> information.
That is unfortunate, I didn't intend for the reproducer to get applied. I'll add
the diff to the commit message, as well as the explanation (below) for the
failures without this patch.
Thanks,
Alex
> Thanks,
> drew
>
>>
>> Without the fix, the first report fails, and the test usually hangs because
>> mmu_enable pushes the LR register on the stack before asm_mmu_enable, which
>> goes to memory, then pops it after asm_mmu_enable, and reads back garbage
>> from the dcache.
>>
>> With the fix, the two reports pass.
>>
>> lib/arm/asm/processor.h | 6 ++++++
>> lib/arm64/asm/processor.h | 6 ++++++
>> lib/arm/processor.c | 10 ++++++++++
>> lib/arm/setup.c | 2 ++
>> lib/arm64/processor.c | 11 +++++++++++
>> arm/cstart.S | 22 ++++++++++++++++++++++
>> arm/cstart64.S | 23 +++++++++++++++++++++++
>> 7 files changed, 80 insertions(+)
>>
>> diff --git a/lib/arm/asm/processor.h b/lib/arm/asm/processor.h
>> index a8c4628da818..4684fb4755b3 100644
>> --- a/lib/arm/asm/processor.h
>> +++ b/lib/arm/asm/processor.h
>> @@ -9,6 +9,11 @@
>> #include <asm/sysreg.h>
>> #include <asm/barrier.h>
>>
>> +#define CTR_DMINLINE_SHIFT 16
>> +#define CTR_DMINLINE_MASK (0xf << 16)
>> +#define CTR_DMINLINE(x) \
>> + (((x) & CTR_DMINLINE_MASK) >> CTR_DMINLINE_SHIFT)
>> +
>> enum vector {
>> EXCPTN_RST,
>> EXCPTN_UND,
>> @@ -25,6 +30,7 @@ typedef void (*exception_fn)(struct pt_regs *);
>> extern void install_exception_handler(enum vector v, exception_fn fn);
>>
>> extern void show_regs(struct pt_regs *regs);
>> +extern void init_dcache_line_size(void);
>>
>> static inline unsigned long current_cpsr(void)
>> {
>> diff --git a/lib/arm64/asm/processor.h b/lib/arm64/asm/processor.h
>> index 1d9223f728a5..fd508c02f30d 100644
>> --- a/lib/arm64/asm/processor.h
>> +++ b/lib/arm64/asm/processor.h
>> @@ -16,6 +16,11 @@
>> #define SCTLR_EL1_A (1 << 1)
>> #define SCTLR_EL1_M (1 << 0)
>>
>> +#define CTR_EL0_DMINLINE_SHIFT 16
>> +#define CTR_EL0_DMINLINE_MASK (0xf << 16)
>> +#define CTR_EL0_DMINLINE(x) \
>> + (((x) & CTR_EL0_DMINLINE_MASK) >> CTR_EL0_DMINLINE_SHIFT)
>> +
>> #ifndef __ASSEMBLY__
>> #include <asm/ptrace.h>
>> #include <asm/esr.h>
>> @@ -60,6 +65,7 @@ extern void vector_handlers_default_init(vector_fn *handlers);
>>
>> extern void show_regs(struct pt_regs *regs);
>> extern bool get_far(unsigned int esr, unsigned long *far);
>> +extern void init_dcache_line_size(void);
>>
>> static inline unsigned long current_level(void)
>> {
>> diff --git a/lib/arm/processor.c b/lib/arm/processor.c
>> index 773337e6d3b7..c57657c5ea53 100644
>> --- a/lib/arm/processor.c
>> +++ b/lib/arm/processor.c
>> @@ -25,6 +25,8 @@ static const char *vector_names[] = {
>> "rst", "und", "svc", "pabt", "dabt", "addrexcptn", "irq", "fiq"
>> };
>>
>> +unsigned int dcache_line_size;
>> +
>> void show_regs(struct pt_regs *regs)
>> {
>> unsigned long flags;
>> @@ -145,3 +147,11 @@ bool is_user(void)
>> {
>> return current_thread_info()->flags & TIF_USER_MODE;
>> }
>> +void init_dcache_line_size(void)
>> +{
>> + u32 ctr;
>> +
>> + asm volatile("mrc p15, 0, %0, c0, c0, 1" : "=r" (ctr));
>> + /* DminLine is log2 of the number of words in the smallest cache line */
>> + dcache_line_size = 1 << (CTR_DMINLINE(ctr) + 2);
>> +}
>> diff --git a/lib/arm/setup.c b/lib/arm/setup.c
>> index 4f02fca85607..54fc19a20942 100644
>> --- a/lib/arm/setup.c
>> +++ b/lib/arm/setup.c
>> @@ -20,6 +20,7 @@
>> #include <asm/thread_info.h>
>> #include <asm/setup.h>
>> #include <asm/page.h>
>> +#include <asm/processor.h>
>> #include <asm/smp.h>
>>
>> #include "io.h"
>> @@ -63,6 +64,7 @@ static void cpu_init(void)
>> ret = dt_for_each_cpu_node(cpu_set, NULL);
>> assert(ret == 0);
>> set_cpu_online(0, true);
>> + init_dcache_line_size();
>> }
>>
>> static void mem_init(phys_addr_t freemem_start)
>> diff --git a/lib/arm64/processor.c b/lib/arm64/processor.c
>> index 2a024e3f4e9d..f28066d40145 100644
>> --- a/lib/arm64/processor.c
>> +++ b/lib/arm64/processor.c
>> @@ -62,6 +62,8 @@ static const char *ec_names[EC_MAX] = {
>> [ESR_EL1_EC_BRK64] = "BRK64",
>> };
>>
>> +unsigned int dcache_line_size;
>> +
>> void show_regs(struct pt_regs *regs)
>> {
>> int i;
>> @@ -257,3 +259,12 @@ bool is_user(void)
>> {
>> return current_thread_info()->flags & TIF_USER_MODE;
>> }
>> +
>> +void init_dcache_line_size(void)
>> +{
>> + u64 ctr;
>> +
>> + ctr = read_sysreg(ctr_el0);
>> + /* DminLine is log2 of the number of words in the smallest cache line */
>> + dcache_line_size = 1 << (CTR_EL0_DMINLINE(ctr) + 2);
>> +}
>> diff --git a/arm/cstart.S b/arm/cstart.S
>> index dfef48e4dbb2..3c2a3bcde61a 100644
>> --- a/arm/cstart.S
>> +++ b/arm/cstart.S
>> @@ -188,6 +188,20 @@ asm_mmu_enable:
>>
>> mov pc, lr
>>
>> +.macro dcache_clean_inval domain, start, end, tmp1, tmp2
>> + ldr \tmp1, =dcache_line_size
>> + ldr \tmp1, [\tmp1]
>> + sub \tmp2, \tmp1, #1
>> + bic \start, \start, \tmp2
>> +9998:
>> + /* DCCIMVAC */
>> + mcr p15, 0, \start, c7, c14, 1
>> + add \start, \start, \tmp1
>> + cmp \start, \end
>> + blo 9998b
>> + dsb \domain
>> +.endm
>> +
>> .globl asm_mmu_disable
>> asm_mmu_disable:
>> /* SCTLR */
>> @@ -195,6 +209,14 @@ asm_mmu_disable:
>> bic r0, #CR_M
>> mcr p15, 0, r0, c1, c0, 0
>> isb
>> +
>> + ldr r0, =__phys_offset
>> + ldr r0, [r0]
>> + ldr r1, =__phys_end
>> + ldr r1, [r1]
>> + dcache_clean_inval sy, r0, r1, r2, r3
>> + isb
>> +
>> mov pc, lr
>>
>> /*
>> diff --git a/arm/cstart64.S b/arm/cstart64.S
>> index c98842f11e90..f41ffa3bc6c2 100644
>> --- a/arm/cstart64.S
>> +++ b/arm/cstart64.S
>> @@ -201,12 +201,35 @@ asm_mmu_enable:
>>
>> 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
>> + ldr \tmp1, [\tmp1, :lo12:dcache_line_size]
>> + sub \tmp2, \tmp1, #1
>> + bic \start, \start, \tmp2
>> +9998:
>> + dc \op , \start
>> + add \start, \start, \tmp1
>> + cmp \start, \end
>> + b.lo 9998b
>> + dsb \domain
>> +.endm
>> +
>> .globl asm_mmu_disable
>> asm_mmu_disable:
>> mrs x0, sctlr_el1
>> bic x0, x0, SCTLR_EL1_M
>> msr sctlr_el1, x0
>> isb
>> +
>> + /* 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
>> + isb
>> +
>> ret
>>
>> /*
>> --
>> 2.20.1
>>
