* [kvm-unit-tests PATCH v2 0/6] Misc assembly fixes and cleanups @ 2021-03-22 15:06 Alexandru Elisei 2021-03-22 15:06 ` [kvm-unit-tests PATCH v2 1/6] arm64: Remove unnecessary ISB when writing to SPSel Alexandru Elisei ` (6 more replies) 0 siblings, 7 replies; 9+ messages in thread From: Alexandru Elisei @ 2021-03-22 15:06 UTC (permalink / raw) To: drjones, kvm, kvmarm; +Cc: andre.przywara This series is mostly fixes and cleanups for things I found when playing with EFI support. Most of them I hope are fairly self-explanatory. What is clearly aimed at running on baremetal is patch #2 ("arm/arm64: Remove dcache_line_size global variable"), which is needed because the startup environment is different for EFI apps and we're going to need to do cache maintenance before setup() is run. Patch #4 ("lib: arm64: Consolidate register definitions to sysreg.h") is there to make importing register definitions and other header files from Linux (like el2_setup.h) easier by switching to the same layout. And arm is already using sysreg.h for SCTLR fields. Changes in v2: * Gathered Reviewed-by tags, thank you! * For patch #2 ("arm/arm64: Remove dcache_line_size global variable"), I've modified the commit message to mention the change in parameters for dcache_by_line_op, I've added the proper header guards to lib/arm/asm/assembler.h and I've changed raw_dcache_line_size to use ubfx instead of ubfm. Alexandru Elisei (6): arm64: Remove unnecessary ISB when writing to SPSel arm/arm64: Remove dcache_line_size global variable arm/arm64: Remove unnecessary ISB when doing dcache maintenance lib: arm64: Consolidate register definitions to sysreg.h arm64: Configure SCTLR_EL1 at boot arm64: Disable TTBR1_EL1 translation table walks lib/arm/asm/assembler.h | 53 ++++++++++++++++++++++++++++++++++ lib/arm/asm/processor.h | 7 ----- lib/arm64/asm/arch_gicv3.h | 6 ---- lib/arm64/asm/assembler.h | 54 +++++++++++++++++++++++++++++++++++ lib/arm64/asm/pgtable-hwdef.h | 1 + lib/arm64/asm/processor.h | 17 ----------- lib/arm64/asm/sysreg.h | 24 ++++++++++++++++ lib/arm/setup.c | 7 ----- arm/cstart.S | 19 ++---------- arm/cstart64.S | 28 +++++++----------- 10 files changed, 145 insertions(+), 71 deletions(-) create mode 100644 lib/arm/asm/assembler.h create mode 100644 lib/arm64/asm/assembler.h -- 2.31.0 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [kvm-unit-tests PATCH v2 1/6] arm64: Remove unnecessary ISB when writing to SPSel 2021-03-22 15:06 [kvm-unit-tests PATCH v2 0/6] Misc assembly fixes and cleanups Alexandru Elisei @ 2021-03-22 15:06 ` Alexandru Elisei 2021-03-22 15:06 ` [kvm-unit-tests PATCH v2 2/6] arm/arm64: Remove dcache_line_size global variable Alexandru Elisei ` (5 subsequent siblings) 6 siblings, 0 replies; 9+ messages in thread From: Alexandru Elisei @ 2021-03-22 15:06 UTC (permalink / raw) To: drjones, kvm, kvmarm; +Cc: andre.przywara Software can use the SPSel operand to write directly to PSTATE.SP. According to ARM DDI 0487F.b, page D1-2332, writes to PSTATE are self-synchronizing and no ISB is needed: "Writes to the PSTATE fields have side-effects on various aspects of the PE operation. All of these side-effects are guaranteed: - Not to be visible to earlier instructions in the execution stream. - To be visible to later instructions in the execution stream." Reviewed-by: Andre Przywara <andre.przywara@arm.com> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> --- arm/cstart64.S | 1 - 1 file changed, 1 deletion(-) diff --git a/arm/cstart64.S b/arm/cstart64.S index 0428014aa58a..fc1930bcdb53 100644 --- a/arm/cstart64.S +++ b/arm/cstart64.S @@ -54,7 +54,6 @@ start: /* set up stack */ mov x4, #1 msr spsel, x4 - isb adrp x4, stackptr add sp, x4, :lo12:stackptr -- 2.31.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [kvm-unit-tests PATCH v2 2/6] arm/arm64: Remove dcache_line_size global variable 2021-03-22 15:06 [kvm-unit-tests PATCH v2 0/6] Misc assembly fixes and cleanups Alexandru Elisei 2021-03-22 15:06 ` [kvm-unit-tests PATCH v2 1/6] arm64: Remove unnecessary ISB when writing to SPSel Alexandru Elisei @ 2021-03-22 15:06 ` Alexandru Elisei 2021-03-23 16:32 ` Andre Przywara 2021-03-22 15:06 ` [kvm-unit-tests PATCH v2 3/6] arm/arm64: Remove unnecessary ISB when doing dcache maintenance Alexandru Elisei ` (4 subsequent siblings) 6 siblings, 1 reply; 9+ messages in thread From: Alexandru Elisei @ 2021-03-22 15:06 UTC (permalink / raw) To: drjones, kvm, kvmarm; +Cc: andre.przywara Compute the dcache line size when doing dcache maintenance instead of using a global variable computed in setup(), which allows us to do dcache maintenance at any point in the boot process. This will be useful for running as an EFI app and it also aligns our implementation to that of the Linux kernel. As a result, the dcache_by_line_op assembly has been modified to take a range described by start address and size, instead of start and end addresses. For consistency, the arm code has been similary modified. Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> --- lib/arm/asm/assembler.h | 53 ++++++++++++++++++++++++++++++++++++++ lib/arm/asm/processor.h | 7 ----- lib/arm64/asm/assembler.h | 54 +++++++++++++++++++++++++++++++++++++++ lib/arm64/asm/processor.h | 7 ----- lib/arm/setup.c | 7 ----- arm/cstart.S | 18 +++---------- arm/cstart64.S | 16 ++---------- 7 files changed, 112 insertions(+), 50 deletions(-) create mode 100644 lib/arm/asm/assembler.h create mode 100644 lib/arm64/asm/assembler.h diff --git a/lib/arm/asm/assembler.h b/lib/arm/asm/assembler.h new file mode 100644 index 000000000000..dfd3c51bf6ad --- /dev/null +++ b/lib/arm/asm/assembler.h @@ -0,0 +1,53 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * Based on several files from Linux version v5.10: arch/arm/mm/proc-macros.S, + * arch/arm/mm/proc-v7.S. + */ + +#ifndef __ASSEMBLY__ +#error "Only include this from assembly code" +#endif + +#ifndef __ASM_ASSEMBLER_H +#define __ASM_ASSEMBLER_H + +/* + * dcache_line_size - get the minimum D-cache line size from the CTR register + * on ARMv7. + */ + .macro dcache_line_size, reg, tmp + mrc p15, 0, \tmp, c0, c0, 1 // read ctr + lsr \tmp, \tmp, #16 + and \tmp, \tmp, #0xf // cache line size encoding + mov \reg, #4 // bytes per word + mov \reg, \reg, lsl \tmp // actual cache line size + .endm + +/* + * Macro to perform a data cache maintenance for the interval + * [addr, addr + size). + * + * op: operation to execute + * domain domain used in the dsb instruction + * addr: starting virtual address of the region + * size: size of the region + * Corrupts: addr, size, tmp1, tmp2 + */ + .macro dcache_by_line_op op, domain, addr, size, tmp1, tmp2 + dcache_line_size \tmp1, \tmp2 + add \size, \addr, \size + sub \tmp2, \tmp1, #1 + bic \addr, \addr, \tmp2 +9998: + .ifc \op, dccimvac + mcr p15, 0, \addr, c7, c14, 1 + .else + .err + .endif + add \addr, \addr, \tmp1 + cmp \addr, \size + blo 9998b + dsb \domain + .endm + +#endif /* __ASM_ASSEMBLER_H */ diff --git a/lib/arm/asm/processor.h b/lib/arm/asm/processor.h index 273366d1fe1c..3c36eac903f0 100644 --- a/lib/arm/asm/processor.h +++ b/lib/arm/asm/processor.h @@ -9,11 +9,6 @@ #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, @@ -89,6 +84,4 @@ static inline u32 get_ctr(void) return read_sysreg(CTR); } -extern unsigned long dcache_line_size; - #endif /* _ASMARM_PROCESSOR_H_ */ diff --git a/lib/arm64/asm/assembler.h b/lib/arm64/asm/assembler.h new file mode 100644 index 000000000000..0a6ab9720bdd --- /dev/null +++ b/lib/arm64/asm/assembler.h @@ -0,0 +1,54 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * Based on the file arch/arm64/include/asm/assembled.h from Linux v5.10, which + * in turn is based on arch/arm/include/asm/assembler.h and + * arch/arm/mm/proc-macros.S + * + * Copyright (C) 1996-2000 Russell King + * Copyright (C) 2012 ARM Ltd. + */ + +#ifndef __ASSEMBLY__ +#error "Only include this from assembly code" +#endif + +#ifndef __ASM_ASSEMBLER_H +#define __ASM_ASSEMBLER_H + +/* + * raw_dcache_line_size - get the minimum D-cache line size on this CPU + * from the CTR register. + */ + .macro raw_dcache_line_size, reg, tmp + mrs \tmp, ctr_el0 // read CTR + ubfx \tmp, \tmp, #16, #4 // cache line size encoding + mov \reg, #4 // bytes per word + lsl \reg, \reg, \tmp // actual cache line size + .endm + +/* + * Macro to perform a data cache maintenance for the interval + * [addr, addr + size). Use the raw value for the dcache line size because + * kvm-unit-tests has no concept of scheduling. + * + * op: operation passed to dc instruction + * domain: domain used in dsb instruciton + * addr: starting virtual address of the region + * size: size of the region + * Corrupts: addr, size, tmp1, tmp2 + */ + + .macro dcache_by_line_op op, domain, addr, size, tmp1, tmp2 + raw_dcache_line_size \tmp1, \tmp2 + add \size, \addr, \size + sub \tmp2, \tmp1, #1 + bic \addr, \addr, \tmp2 +9998: + dc \op, \addr + add \addr, \addr, \tmp1 + cmp \addr, \size + b.lo 9998b + dsb \domain + .endm + +#endif /* __ASM_ASSEMBLER_H */ diff --git a/lib/arm64/asm/processor.h b/lib/arm64/asm/processor.h index 771b2d1e0c94..cdc2463e1981 100644 --- a/lib/arm64/asm/processor.h +++ b/lib/arm64/asm/processor.h @@ -16,11 +16,6 @@ #define SCTLR_EL1_A (1 << 1) #define SCTLR_EL1_M (1 << 0) -#define CTR_DMINLINE_SHIFT 16 -#define CTR_DMINLINE_MASK (0xf << 16) -#define CTR_DMINLINE(x) \ - (((x) & CTR_DMINLINE_MASK) >> CTR_DMINLINE_SHIFT) - #ifndef __ASSEMBLY__ #include <asm/ptrace.h> #include <asm/esr.h> @@ -115,8 +110,6 @@ static inline u64 get_ctr(void) return read_sysreg(ctr_el0); } -extern unsigned long dcache_line_size; - static inline unsigned long get_id_aa64mmfr0_el1(void) { return read_sysreg(id_aa64mmfr0_el1); diff --git a/lib/arm/setup.c b/lib/arm/setup.c index 066524f8bf61..751ba980000a 100644 --- a/lib/arm/setup.c +++ b/lib/arm/setup.c @@ -42,8 +42,6 @@ static struct mem_region __initial_mem_regions[NR_INITIAL_MEM_REGIONS + 1]; struct mem_region *mem_regions = __initial_mem_regions; phys_addr_t __phys_offset, __phys_end; -unsigned long dcache_line_size; - int mpidr_to_cpu(uint64_t mpidr) { int i; @@ -72,11 +70,6 @@ static void cpu_init(void) ret = dt_for_each_cpu_node(cpu_set, NULL); assert(ret == 0); set_cpu_online(0, true); - /* - * DminLine is log2 of the number of words in the smallest cache line; a - * word is 4 bytes. - */ - dcache_line_size = 1 << (CTR_DMINLINE(get_ctr()) + 2); } unsigned int mem_region_get_flags(phys_addr_t paddr) diff --git a/arm/cstart.S b/arm/cstart.S index ef936ae2f874..954748b00f64 100644 --- a/arm/cstart.S +++ b/arm/cstart.S @@ -7,6 +7,7 @@ */ #define __ASSEMBLY__ #include <auxinfo.h> +#include <asm/assembler.h> #include <asm/thread_info.h> #include <asm/asm-offsets.h> #include <asm/pgtable-hwdef.h> @@ -197,20 +198,6 @@ 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 */ @@ -223,7 +210,8 @@ asm_mmu_disable: ldr r0, [r0] ldr r1, =__phys_end ldr r1, [r1] - dcache_clean_inval sy, r0, r1, r2, r3 + sub r1, r1, r0 + dcache_by_line_op dccimvac, sy, r0, r1, r2, r3 isb mov pc, lr diff --git a/arm/cstart64.S b/arm/cstart64.S index fc1930bcdb53..046bd3914098 100644 --- a/arm/cstart64.S +++ b/arm/cstart64.S @@ -8,6 +8,7 @@ #define __ASSEMBLY__ #include <auxinfo.h> #include <asm/asm-offsets.h> +#include <asm/assembler.h> #include <asm/ptrace.h> #include <asm/processor.h> #include <asm/page.h> @@ -204,20 +205,6 @@ 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 @@ -230,6 +217,7 @@ asm_mmu_disable: ldr x0, [x0, :lo12:__phys_offset] adrp x1, __phys_end ldr x1, [x1, :lo12:__phys_end] + sub x1, x1, x0 dcache_by_line_op civac, sy, x0, x1, x2, x3 isb -- 2.31.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [kvm-unit-tests PATCH v2 2/6] arm/arm64: Remove dcache_line_size global variable 2021-03-22 15:06 ` [kvm-unit-tests PATCH v2 2/6] arm/arm64: Remove dcache_line_size global variable Alexandru Elisei @ 2021-03-23 16:32 ` Andre Przywara 0 siblings, 0 replies; 9+ messages in thread From: Andre Przywara @ 2021-03-23 16:32 UTC (permalink / raw) To: Alexandru Elisei; +Cc: drjones, kvm, kvmarm On Mon, 22 Mar 2021 15:06:37 +0000 Alexandru Elisei <alexandru.elisei@arm.com> wrote: > Compute the dcache line size when doing dcache maintenance instead of using > a global variable computed in setup(), which allows us to do dcache > maintenance at any point in the boot process. This will be useful for > running as an EFI app and it also aligns our implementation to that of the > Linux kernel. As a result, the dcache_by_line_op assembly has been modified > to take a range described by start address and size, instead of start and > end addresses. > > For consistency, the arm code has been similary modified. > > Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> Thanks for the changes, looks good now. Reviewed-by: Andre Przywara <andre.przywara@arm.com> Cheers, Andre > --- > lib/arm/asm/assembler.h | 53 ++++++++++++++++++++++++++++++++++++++ > lib/arm/asm/processor.h | 7 ----- > lib/arm64/asm/assembler.h | 54 +++++++++++++++++++++++++++++++++++++++ > lib/arm64/asm/processor.h | 7 ----- > lib/arm/setup.c | 7 ----- > arm/cstart.S | 18 +++---------- > arm/cstart64.S | 16 ++---------- > 7 files changed, 112 insertions(+), 50 deletions(-) > create mode 100644 lib/arm/asm/assembler.h > create mode 100644 lib/arm64/asm/assembler.h > > diff --git a/lib/arm/asm/assembler.h b/lib/arm/asm/assembler.h > new file mode 100644 > index 000000000000..dfd3c51bf6ad > --- /dev/null > +++ b/lib/arm/asm/assembler.h > @@ -0,0 +1,53 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Based on several files from Linux version v5.10: arch/arm/mm/proc-macros.S, > + * arch/arm/mm/proc-v7.S. > + */ > + > +#ifndef __ASSEMBLY__ > +#error "Only include this from assembly code" > +#endif > + > +#ifndef __ASM_ASSEMBLER_H > +#define __ASM_ASSEMBLER_H > + > +/* > + * dcache_line_size - get the minimum D-cache line size from the CTR register > + * on ARMv7. > + */ > + .macro dcache_line_size, reg, tmp > + mrc p15, 0, \tmp, c0, c0, 1 // read ctr > + lsr \tmp, \tmp, #16 > + and \tmp, \tmp, #0xf // cache line size encoding > + mov \reg, #4 // bytes per word > + mov \reg, \reg, lsl \tmp // actual cache line size > + .endm > + > +/* > + * Macro to perform a data cache maintenance for the interval > + * [addr, addr + size). > + * > + * op: operation to execute > + * domain domain used in the dsb instruction > + * addr: starting virtual address of the region > + * size: size of the region > + * Corrupts: addr, size, tmp1, tmp2 > + */ > + .macro dcache_by_line_op op, domain, addr, size, tmp1, tmp2 > + dcache_line_size \tmp1, \tmp2 > + add \size, \addr, \size > + sub \tmp2, \tmp1, #1 > + bic \addr, \addr, \tmp2 > +9998: > + .ifc \op, dccimvac > + mcr p15, 0, \addr, c7, c14, 1 > + .else > + .err > + .endif > + add \addr, \addr, \tmp1 > + cmp \addr, \size > + blo 9998b > + dsb \domain > + .endm > + > +#endif /* __ASM_ASSEMBLER_H */ > diff --git a/lib/arm/asm/processor.h b/lib/arm/asm/processor.h > index 273366d1fe1c..3c36eac903f0 100644 > --- a/lib/arm/asm/processor.h > +++ b/lib/arm/asm/processor.h > @@ -9,11 +9,6 @@ > #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, > @@ -89,6 +84,4 @@ static inline u32 get_ctr(void) > return read_sysreg(CTR); > } > > -extern unsigned long dcache_line_size; > - > #endif /* _ASMARM_PROCESSOR_H_ */ > diff --git a/lib/arm64/asm/assembler.h b/lib/arm64/asm/assembler.h > new file mode 100644 > index 000000000000..0a6ab9720bdd > --- /dev/null > +++ b/lib/arm64/asm/assembler.h > @@ -0,0 +1,54 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * Based on the file arch/arm64/include/asm/assembled.h from Linux v5.10, which > + * in turn is based on arch/arm/include/asm/assembler.h and > + * arch/arm/mm/proc-macros.S > + * > + * Copyright (C) 1996-2000 Russell King > + * Copyright (C) 2012 ARM Ltd. > + */ > + > +#ifndef __ASSEMBLY__ > +#error "Only include this from assembly code" > +#endif > + > +#ifndef __ASM_ASSEMBLER_H > +#define __ASM_ASSEMBLER_H > + > +/* > + * raw_dcache_line_size - get the minimum D-cache line size on this CPU > + * from the CTR register. > + */ > + .macro raw_dcache_line_size, reg, tmp > + mrs \tmp, ctr_el0 // read CTR > + ubfx \tmp, \tmp, #16, #4 // cache line size encoding > + mov \reg, #4 // bytes per word > + lsl \reg, \reg, \tmp // actual cache line size > + .endm > + > +/* > + * Macro to perform a data cache maintenance for the interval > + * [addr, addr + size). Use the raw value for the dcache line size because > + * kvm-unit-tests has no concept of scheduling. > + * > + * op: operation passed to dc instruction > + * domain: domain used in dsb instruciton > + * addr: starting virtual address of the region > + * size: size of the region > + * Corrupts: addr, size, tmp1, tmp2 > + */ > + > + .macro dcache_by_line_op op, domain, addr, size, tmp1, tmp2 > + raw_dcache_line_size \tmp1, \tmp2 > + add \size, \addr, \size > + sub \tmp2, \tmp1, #1 > + bic \addr, \addr, \tmp2 > +9998: > + dc \op, \addr > + add \addr, \addr, \tmp1 > + cmp \addr, \size > + b.lo 9998b > + dsb \domain > + .endm > + > +#endif /* __ASM_ASSEMBLER_H */ > diff --git a/lib/arm64/asm/processor.h b/lib/arm64/asm/processor.h > index 771b2d1e0c94..cdc2463e1981 100644 > --- a/lib/arm64/asm/processor.h > +++ b/lib/arm64/asm/processor.h > @@ -16,11 +16,6 @@ > #define SCTLR_EL1_A (1 << 1) > #define SCTLR_EL1_M (1 << 0) > > -#define CTR_DMINLINE_SHIFT 16 > -#define CTR_DMINLINE_MASK (0xf << 16) > -#define CTR_DMINLINE(x) \ > - (((x) & CTR_DMINLINE_MASK) >> CTR_DMINLINE_SHIFT) > - > #ifndef __ASSEMBLY__ > #include <asm/ptrace.h> > #include <asm/esr.h> > @@ -115,8 +110,6 @@ static inline u64 get_ctr(void) > return read_sysreg(ctr_el0); > } > > -extern unsigned long dcache_line_size; > - > static inline unsigned long get_id_aa64mmfr0_el1(void) > { > return read_sysreg(id_aa64mmfr0_el1); > diff --git a/lib/arm/setup.c b/lib/arm/setup.c > index 066524f8bf61..751ba980000a 100644 > --- a/lib/arm/setup.c > +++ b/lib/arm/setup.c > @@ -42,8 +42,6 @@ static struct mem_region __initial_mem_regions[NR_INITIAL_MEM_REGIONS + 1]; > struct mem_region *mem_regions = __initial_mem_regions; > phys_addr_t __phys_offset, __phys_end; > > -unsigned long dcache_line_size; > - > int mpidr_to_cpu(uint64_t mpidr) > { > int i; > @@ -72,11 +70,6 @@ static void cpu_init(void) > ret = dt_for_each_cpu_node(cpu_set, NULL); > assert(ret == 0); > set_cpu_online(0, true); > - /* > - * DminLine is log2 of the number of words in the smallest cache line; a > - * word is 4 bytes. > - */ > - dcache_line_size = 1 << (CTR_DMINLINE(get_ctr()) + 2); > } > > unsigned int mem_region_get_flags(phys_addr_t paddr) > diff --git a/arm/cstart.S b/arm/cstart.S > index ef936ae2f874..954748b00f64 100644 > --- a/arm/cstart.S > +++ b/arm/cstart.S > @@ -7,6 +7,7 @@ > */ > #define __ASSEMBLY__ > #include <auxinfo.h> > +#include <asm/assembler.h> > #include <asm/thread_info.h> > #include <asm/asm-offsets.h> > #include <asm/pgtable-hwdef.h> > @@ -197,20 +198,6 @@ 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 */ > @@ -223,7 +210,8 @@ asm_mmu_disable: > ldr r0, [r0] > ldr r1, =__phys_end > ldr r1, [r1] > - dcache_clean_inval sy, r0, r1, r2, r3 > + sub r1, r1, r0 > + dcache_by_line_op dccimvac, sy, r0, r1, r2, r3 > isb > > mov pc, lr > diff --git a/arm/cstart64.S b/arm/cstart64.S > index fc1930bcdb53..046bd3914098 100644 > --- a/arm/cstart64.S > +++ b/arm/cstart64.S > @@ -8,6 +8,7 @@ > #define __ASSEMBLY__ > #include <auxinfo.h> > #include <asm/asm-offsets.h> > +#include <asm/assembler.h> > #include <asm/ptrace.h> > #include <asm/processor.h> > #include <asm/page.h> > @@ -204,20 +205,6 @@ 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 > @@ -230,6 +217,7 @@ asm_mmu_disable: > ldr x0, [x0, :lo12:__phys_offset] > adrp x1, __phys_end > ldr x1, [x1, :lo12:__phys_end] > + sub x1, x1, x0 > dcache_by_line_op civac, sy, x0, x1, x2, x3 > isb > ^ permalink raw reply [flat|nested] 9+ messages in thread
* [kvm-unit-tests PATCH v2 3/6] arm/arm64: Remove unnecessary ISB when doing dcache maintenance 2021-03-22 15:06 [kvm-unit-tests PATCH v2 0/6] Misc assembly fixes and cleanups Alexandru Elisei 2021-03-22 15:06 ` [kvm-unit-tests PATCH v2 1/6] arm64: Remove unnecessary ISB when writing to SPSel Alexandru Elisei 2021-03-22 15:06 ` [kvm-unit-tests PATCH v2 2/6] arm/arm64: Remove dcache_line_size global variable Alexandru Elisei @ 2021-03-22 15:06 ` Alexandru Elisei 2021-03-22 15:06 ` [kvm-unit-tests PATCH v2 4/6] lib: arm64: Consolidate register definitions to sysreg.h Alexandru Elisei ` (3 subsequent siblings) 6 siblings, 0 replies; 9+ messages in thread From: Alexandru Elisei @ 2021-03-22 15:06 UTC (permalink / raw) To: drjones, kvm, kvmarm; +Cc: andre.przywara The dcache_by_line_op macro executes a DSB to complete the cache maintenance operations. According to ARM DDI 0487G.a, page B2-150: "In addition, no instruction that appears in program order after the DSB instruction can alter any state of the system or perform any part of its functionality until the DSB completes other than: - Being fetched from memory and decoded. - Reading the general-purpose, SIMD and floating-point, Special-purpose, or System registers that are directly or indirectly read without causing side-effects." Similar definition for ARM in ARM DDI 0406C.d, page A3-150: "In addition, no instruction that appears in program order after the DSB instruction can execute until the DSB completes." This means that we don't need the ISB to prevent reordering of the cache maintenance instructions. We are also not doing icache maintenance, where an ISB would be required for the PE to discard instructions speculated before the invalidation. In conclusion, the ISB is unnecessary, so remove it. Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> --- arm/cstart.S | 1 - arm/cstart64.S | 1 - 2 files changed, 2 deletions(-) diff --git a/arm/cstart.S b/arm/cstart.S index 954748b00f64..2d62c1e6d40d 100644 --- a/arm/cstart.S +++ b/arm/cstart.S @@ -212,7 +212,6 @@ asm_mmu_disable: ldr r1, [r1] sub r1, r1, r0 dcache_by_line_op dccimvac, sy, r0, r1, r2, r3 - isb mov pc, lr diff --git a/arm/cstart64.S b/arm/cstart64.S index 046bd3914098..c1deff842f03 100644 --- a/arm/cstart64.S +++ b/arm/cstart64.S @@ -219,7 +219,6 @@ asm_mmu_disable: ldr x1, [x1, :lo12:__phys_end] sub x1, x1, x0 dcache_by_line_op civac, sy, x0, x1, x2, x3 - isb ret -- 2.31.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [kvm-unit-tests PATCH v2 4/6] lib: arm64: Consolidate register definitions to sysreg.h 2021-03-22 15:06 [kvm-unit-tests PATCH v2 0/6] Misc assembly fixes and cleanups Alexandru Elisei ` (2 preceding siblings ...) 2021-03-22 15:06 ` [kvm-unit-tests PATCH v2 3/6] arm/arm64: Remove unnecessary ISB when doing dcache maintenance Alexandru Elisei @ 2021-03-22 15:06 ` Alexandru Elisei 2021-03-22 15:06 ` [kvm-unit-tests PATCH v2 5/6] arm64: Configure SCTLR_EL1 at boot Alexandru Elisei ` (2 subsequent siblings) 6 siblings, 0 replies; 9+ messages in thread From: Alexandru Elisei @ 2021-03-22 15:06 UTC (permalink / raw) To: drjones, kvm, kvmarm; +Cc: andre.przywara Move system register field definitions to sysreg.h, where the opcodes for system register access are defined, to align ourselves with the Linux kernel. EL2 support, needed for EFI and nested virtualization testing, will require additional register and field definions, and having them in the same place as Linux will make maintenance easier. Reviewed-by: Andre Przywara <andre.przywara@arm.com> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> --- lib/arm64/asm/arch_gicv3.h | 6 ------ lib/arm64/asm/processor.h | 10 ---------- lib/arm64/asm/sysreg.h | 17 +++++++++++++++++ arm/cstart64.S | 2 +- 4 files changed, 18 insertions(+), 17 deletions(-) diff --git a/lib/arm64/asm/arch_gicv3.h b/lib/arm64/asm/arch_gicv3.h index a7994ec2fbbe..fdee4de1f1f6 100644 --- a/lib/arm64/asm/arch_gicv3.h +++ b/lib/arm64/asm/arch_gicv3.h @@ -10,12 +10,6 @@ #include <asm/sysreg.h> -#define ICC_PMR_EL1 sys_reg(3, 0, 4, 6, 0) -#define ICC_SGI1R_EL1 sys_reg(3, 0, 12, 11, 5) -#define ICC_IAR1_EL1 sys_reg(3, 0, 12, 12, 0) -#define ICC_EOIR1_EL1 sys_reg(3, 0, 12, 12, 1) -#define ICC_GRPEN1_EL1 sys_reg(3, 0, 12, 12, 7) - #ifndef __ASSEMBLY__ #include <libcflat.h> diff --git a/lib/arm64/asm/processor.h b/lib/arm64/asm/processor.h index cdc2463e1981..4a3d826ab560 100644 --- a/lib/arm64/asm/processor.h +++ b/lib/arm64/asm/processor.h @@ -6,16 +6,6 @@ * This work is licensed under the terms of the GNU LGPL, version 2. */ -/* System Control Register (SCTLR_EL1) bits */ -#define SCTLR_EL1_EE (1 << 25) -#define SCTLR_EL1_WXN (1 << 19) -#define SCTLR_EL1_I (1 << 12) -#define SCTLR_EL1_SA0 (1 << 4) -#define SCTLR_EL1_SA (1 << 3) -#define SCTLR_EL1_C (1 << 2) -#define SCTLR_EL1_A (1 << 1) -#define SCTLR_EL1_M (1 << 0) - #ifndef __ASSEMBLY__ #include <asm/ptrace.h> #include <asm/esr.h> diff --git a/lib/arm64/asm/sysreg.h b/lib/arm64/asm/sysreg.h index 378bf7ebb3b5..9d6b4fc66936 100644 --- a/lib/arm64/asm/sysreg.h +++ b/lib/arm64/asm/sysreg.h @@ -70,4 +70,21 @@ asm( " .endm\n" ); #endif /* __ASSEMBLY__ */ + +#define ICC_PMR_EL1 sys_reg(3, 0, 4, 6, 0) +#define ICC_SGI1R_EL1 sys_reg(3, 0, 12, 11, 5) +#define ICC_IAR1_EL1 sys_reg(3, 0, 12, 12, 0) +#define ICC_EOIR1_EL1 sys_reg(3, 0, 12, 12, 1) +#define ICC_GRPEN1_EL1 sys_reg(3, 0, 12, 12, 7) + +/* System Control Register (SCTLR_EL1) bits */ +#define SCTLR_EL1_EE (1 << 25) +#define SCTLR_EL1_WXN (1 << 19) +#define SCTLR_EL1_I (1 << 12) +#define SCTLR_EL1_SA0 (1 << 4) +#define SCTLR_EL1_SA (1 << 3) +#define SCTLR_EL1_C (1 << 2) +#define SCTLR_EL1_A (1 << 1) +#define SCTLR_EL1_M (1 << 0) + #endif /* _ASMARM64_SYSREG_H_ */ diff --git a/arm/cstart64.S b/arm/cstart64.S index c1deff842f03..f6c5d2ebccf3 100644 --- a/arm/cstart64.S +++ b/arm/cstart64.S @@ -10,9 +10,9 @@ #include <asm/asm-offsets.h> #include <asm/assembler.h> #include <asm/ptrace.h> -#include <asm/processor.h> #include <asm/page.h> #include <asm/pgtable-hwdef.h> +#include <asm/sysreg.h> .section .init -- 2.31.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [kvm-unit-tests PATCH v2 5/6] arm64: Configure SCTLR_EL1 at boot 2021-03-22 15:06 [kvm-unit-tests PATCH v2 0/6] Misc assembly fixes and cleanups Alexandru Elisei ` (3 preceding siblings ...) 2021-03-22 15:06 ` [kvm-unit-tests PATCH v2 4/6] lib: arm64: Consolidate register definitions to sysreg.h Alexandru Elisei @ 2021-03-22 15:06 ` Alexandru Elisei 2021-03-22 15:06 ` [kvm-unit-tests PATCH v2 6/6] arm64: Disable TTBR1_EL1 translation table walks Alexandru Elisei 2021-03-25 15:31 ` [kvm-unit-tests PATCH v2 0/6] Misc assembly fixes and cleanups Andrew Jones 6 siblings, 0 replies; 9+ messages in thread From: Alexandru Elisei @ 2021-03-22 15:06 UTC (permalink / raw) To: drjones, kvm, kvmarm; +Cc: andre.przywara Some fields in SCTLR_EL1 are UNKNOWN at reset and the arm64 boot requirements, as stated by Linux in Documentation/arm64/booting.rst, do not specify a particular value for all the fields. Do not rely on the good will of the hypervisor and userspace to set SCTLR_EL1 to a sane value (by their definition of sane) and set SCTLR_EL1 explicitely before running setup(). This will ensure that all tests are performed with the hardware set up identically, regardless of the KVM or VMM versions. Reviewed-by: Andre Przywara <andre.przywara@arm.com> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> --- lib/arm64/asm/sysreg.h | 7 +++++++ arm/cstart64.S | 5 +++++ 2 files changed, 12 insertions(+) diff --git a/lib/arm64/asm/sysreg.h b/lib/arm64/asm/sysreg.h index 9d6b4fc66936..18c4ed39557a 100644 --- a/lib/arm64/asm/sysreg.h +++ b/lib/arm64/asm/sysreg.h @@ -8,6 +8,8 @@ #ifndef _ASMARM64_SYSREG_H_ #define _ASMARM64_SYSREG_H_ +#include <linux/const.h> + #define sys_reg(op0, op1, crn, crm, op2) \ ((((op0)&3)<<19)|((op1)<<16)|((crn)<<12)|((crm)<<8)|((op2)<<5)) @@ -87,4 +89,9 @@ asm( #define SCTLR_EL1_A (1 << 1) #define SCTLR_EL1_M (1 << 0) +#define SCTLR_EL1_RES1 (_BITUL(7) | _BITUL(8) | _BITUL(11) | _BITUL(20) | \ + _BITUL(22) | _BITUL(23) | _BITUL(28) | _BITUL(29)) +#define INIT_SCTLR_EL1_MMU_OFF \ + SCTLR_EL1_RES1 + #endif /* _ASMARM64_SYSREG_H_ */ diff --git a/arm/cstart64.S b/arm/cstart64.S index f6c5d2ebccf3..42a838ff4c38 100644 --- a/arm/cstart64.S +++ b/arm/cstart64.S @@ -52,6 +52,11 @@ start: b 1b 1: + /* set SCTLR_EL1 to a known value */ + ldr x4, =INIT_SCTLR_EL1_MMU_OFF + msr sctlr_el1, x4 + isb + /* set up stack */ mov x4, #1 msr spsel, x4 -- 2.31.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [kvm-unit-tests PATCH v2 6/6] arm64: Disable TTBR1_EL1 translation table walks 2021-03-22 15:06 [kvm-unit-tests PATCH v2 0/6] Misc assembly fixes and cleanups Alexandru Elisei ` (4 preceding siblings ...) 2021-03-22 15:06 ` [kvm-unit-tests PATCH v2 5/6] arm64: Configure SCTLR_EL1 at boot Alexandru Elisei @ 2021-03-22 15:06 ` Alexandru Elisei 2021-03-25 15:31 ` [kvm-unit-tests PATCH v2 0/6] Misc assembly fixes and cleanups Andrew Jones 6 siblings, 0 replies; 9+ messages in thread From: Alexandru Elisei @ 2021-03-22 15:06 UTC (permalink / raw) To: drjones, kvm, kvmarm; +Cc: andre.przywara, Mark Rutland From an architectural point of view, the PE can speculate instruction fetches and data reads at any time when the MMU is enabled using the translation tables from TTBR0_EL1 and TTBR1_EL1. kvm-unit-tests uses an identity map, and as such it only programs TTBR0_EL1 with a valid table and leaves TTBR1_EL1 unchanged. The reset value for TTBR1_EL1 is UNKNOWN, which means it is possible for the PE to perform reads from memory locations where accesses can cause side effects (like memory-mapped devices) as part of the speculated translation table walk. So far, this hasn't been a problem, because KVM resets TTBR{0,1}_EL1 to zero, and that address is used for emulation for both qemu and kvmtool and it doesn't point to a real device. However, kvm-unit-tests shouldn't rely on a particular combination of hypervisor and userspace for correctness. Another situation where we can't rely on these assumptions being true is when kvm-unit-tests is run as an EFI app. To prevent reads from arbitrary addresses, set the TCR_EL1.EPD1 bit to disable speculative translation table walks using TTBR1_EL1. This is similar to EDK2 commit fafb7e9c110e ("ArmPkg: correct TTBR1_EL1 settings in TCR_EL1"). Also mentioned in that commit is the Cortex-A57 erratum 822227 which impacts the hypervisor, but kvm-unit-tests is protected against it because asm_mmu_enable sets both the TCR_EL1.TG0 and TCR_EL1.TG1 bits when programming the register. Suggested-by: Mark Rutland <mark.rutland@arm.com> Reviewed-by: Andre Przywara <andre.przywara@arm.com> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> --- lib/arm64/asm/pgtable-hwdef.h | 1 + arm/cstart64.S | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/arm64/asm/pgtable-hwdef.h b/lib/arm64/asm/pgtable-hwdef.h index 48a1d1ab1ac2..8c41fe123fb3 100644 --- a/lib/arm64/asm/pgtable-hwdef.h +++ b/lib/arm64/asm/pgtable-hwdef.h @@ -136,6 +136,7 @@ #define TCR_ORGN_WBnWA ((UL(3) << 10) | (UL(3) << 26)) #define TCR_ORGN_MASK ((UL(3) << 10) | (UL(3) << 26)) #define TCR_SHARED ((UL(3) << 12) | (UL(3) << 28)) +#define TCR_EPD1 (UL(1) << 23) #define TCR_TG0_4K (UL(0) << 14) #define TCR_TG0_64K (UL(1) << 14) #define TCR_TG0_16K (UL(2) << 14) diff --git a/arm/cstart64.S b/arm/cstart64.S index 42a838ff4c38..3d359c8387c9 100644 --- a/arm/cstart64.S +++ b/arm/cstart64.S @@ -181,7 +181,8 @@ asm_mmu_enable: ldr x1, =TCR_TxSZ(VA_BITS) | \ TCR_TG_FLAGS | \ TCR_IRGN_WBWA | TCR_ORGN_WBWA | \ - TCR_SHARED + TCR_SHARED | \ + TCR_EPD1 mrs x2, id_aa64mmfr0_el1 bfi x1, x2, #32, #3 msr tcr_el1, x1 -- 2.31.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [kvm-unit-tests PATCH v2 0/6] Misc assembly fixes and cleanups 2021-03-22 15:06 [kvm-unit-tests PATCH v2 0/6] Misc assembly fixes and cleanups Alexandru Elisei ` (5 preceding siblings ...) 2021-03-22 15:06 ` [kvm-unit-tests PATCH v2 6/6] arm64: Disable TTBR1_EL1 translation table walks Alexandru Elisei @ 2021-03-25 15:31 ` Andrew Jones 6 siblings, 0 replies; 9+ messages in thread From: Andrew Jones @ 2021-03-25 15:31 UTC (permalink / raw) To: Alexandru Elisei; +Cc: kvm, kvmarm, andre.przywara On Mon, Mar 22, 2021 at 03:06:35PM +0000, Alexandru Elisei wrote: > This series is mostly fixes and cleanups for things I found when playing > with EFI support. Most of them I hope are fairly self-explanatory. > > What is clearly aimed at running on baremetal is patch #2 ("arm/arm64: > Remove dcache_line_size global variable"), which is needed because the > startup environment is different for EFI apps and we're going to need to do > cache maintenance before setup() is run. > > Patch #4 ("lib: arm64: Consolidate register definitions to sysreg.h") is > there to make importing register definitions and other header files from > Linux (like el2_setup.h) easier by switching to the same layout. And arm > is already using sysreg.h for SCTLR fields. > > Changes in v2: > * Gathered Reviewed-by tags, thank you! > * For patch #2 ("arm/arm64: Remove dcache_line_size global variable"), I've > modified the commit message to mention the change in parameters for > dcache_by_line_op, I've added the proper header guards to > lib/arm/asm/assembler.h and I've changed raw_dcache_line_size to use ubfx > instead of ubfm. > > Alexandru Elisei (6): > arm64: Remove unnecessary ISB when writing to SPSel > arm/arm64: Remove dcache_line_size global variable > arm/arm64: Remove unnecessary ISB when doing dcache maintenance > lib: arm64: Consolidate register definitions to sysreg.h > arm64: Configure SCTLR_EL1 at boot > arm64: Disable TTBR1_EL1 translation table walks > > lib/arm/asm/assembler.h | 53 ++++++++++++++++++++++++++++++++++ > lib/arm/asm/processor.h | 7 ----- > lib/arm64/asm/arch_gicv3.h | 6 ---- > lib/arm64/asm/assembler.h | 54 +++++++++++++++++++++++++++++++++++ > lib/arm64/asm/pgtable-hwdef.h | 1 + > lib/arm64/asm/processor.h | 17 ----------- > lib/arm64/asm/sysreg.h | 24 ++++++++++++++++ > lib/arm/setup.c | 7 ----- > arm/cstart.S | 19 ++---------- > arm/cstart64.S | 28 +++++++----------- > 10 files changed, 145 insertions(+), 71 deletions(-) > create mode 100644 lib/arm/asm/assembler.h > create mode 100644 lib/arm64/asm/assembler.h > > -- > 2.31.0 > Applied to arm/queue https://gitlab.com/rhdrjones/kvm-unit-tests/-/commits/arm/queue Thanks, drew ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-03-25 15:32 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-03-22 15:06 [kvm-unit-tests PATCH v2 0/6] Misc assembly fixes and cleanups Alexandru Elisei 2021-03-22 15:06 ` [kvm-unit-tests PATCH v2 1/6] arm64: Remove unnecessary ISB when writing to SPSel Alexandru Elisei 2021-03-22 15:06 ` [kvm-unit-tests PATCH v2 2/6] arm/arm64: Remove dcache_line_size global variable Alexandru Elisei 2021-03-23 16:32 ` Andre Przywara 2021-03-22 15:06 ` [kvm-unit-tests PATCH v2 3/6] arm/arm64: Remove unnecessary ISB when doing dcache maintenance Alexandru Elisei 2021-03-22 15:06 ` [kvm-unit-tests PATCH v2 4/6] lib: arm64: Consolidate register definitions to sysreg.h Alexandru Elisei 2021-03-22 15:06 ` [kvm-unit-tests PATCH v2 5/6] arm64: Configure SCTLR_EL1 at boot Alexandru Elisei 2021-03-22 15:06 ` [kvm-unit-tests PATCH v2 6/6] arm64: Disable TTBR1_EL1 translation table walks Alexandru Elisei 2021-03-25 15:31 ` [kvm-unit-tests PATCH v2 0/6] Misc assembly fixes and cleanups Andrew Jones
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).