| Message ID | 20210310175751.3320106-14-qperret@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: A stage 2 for the host | expand |
On Wed, Mar 10, 2021 at 05:57:30PM +0000, Quentin Perret wrote: > Introduce the infrastructure in KVM enabling to copy CPU feature > registers into EL2-owned data-structures, to allow reading sanitised > values directly at EL2 in nVHE. > > Given that only a subset of these features are being read by the > hypervisor, the ones that need to be copied are to be listed under > <asm/kvm_cpufeature.h> together with the name of the nVHE variable that > will hold the copy. > > While at it, introduce the first user of this infrastructure by > implementing __flush_dcache_area at EL2, which needs > arm64_ftr_reg_ctrel0. > > Signed-off-by: Quentin Perret <qperret@google.com> > --- > arch/arm64/include/asm/cpufeature.h | 1 + > arch/arm64/include/asm/kvm_cpufeature.h | 17 +++++++++++++++++ > arch/arm64/include/asm/kvm_host.h | 4 ++++ > arch/arm64/kernel/cpufeature.c | 13 +++++++++++++ > arch/arm64/kvm/hyp/nvhe/Makefile | 3 ++- > arch/arm64/kvm/hyp/nvhe/cache.S | 13 +++++++++++++ > arch/arm64/kvm/hyp/nvhe/cpufeature.c | 8 ++++++++ > arch/arm64/kvm/sys_regs.c | 21 +++++++++++++++++++++ > 8 files changed, 79 insertions(+), 1 deletion(-) > create mode 100644 arch/arm64/include/asm/kvm_cpufeature.h > create mode 100644 arch/arm64/kvm/hyp/nvhe/cache.S > create mode 100644 arch/arm64/kvm/hyp/nvhe/cpufeature.c > > diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h > index 61177bac49fa..a85cea2cac57 100644 > --- a/arch/arm64/include/asm/cpufeature.h > +++ b/arch/arm64/include/asm/cpufeature.h > @@ -607,6 +607,7 @@ void check_local_cpu_capabilities(void); > > u64 read_sanitised_ftr_reg(u32 id); > u64 __read_sysreg_by_encoding(u32 sys_id); > +int copy_ftr_reg(u32 id, struct arm64_ftr_reg *dst); > > static inline bool cpu_supports_mixed_endian_el0(void) > { > diff --git a/arch/arm64/include/asm/kvm_cpufeature.h b/arch/arm64/include/asm/kvm_cpufeature.h > new file mode 100644 > index 000000000000..d34f85cba358 > --- /dev/null > +++ b/arch/arm64/include/asm/kvm_cpufeature.h > @@ -0,0 +1,17 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * Copyright (C) 2020 - Google LLC > + * Author: Quentin Perret <qperret@google.com> > + */ > + > +#include <asm/cpufeature.h> > + > +#ifndef KVM_HYP_CPU_FTR_REG > +#if defined(__KVM_NVHE_HYPERVISOR__) > +#define KVM_HYP_CPU_FTR_REG(id, name) extern struct arm64_ftr_reg name; > +#else > +#define KVM_HYP_CPU_FTR_REG(id, name) DECLARE_KVM_NVHE_SYM(name); > +#endif > +#endif > + > +KVM_HYP_CPU_FTR_REG(SYS_CTR_EL0, arm64_ftr_reg_ctrel0) > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > index 06ca4828005f..459ee557f87c 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -751,9 +751,13 @@ void kvm_clr_pmu_events(u32 clr); > > void kvm_vcpu_pmu_restore_guest(struct kvm_vcpu *vcpu); > void kvm_vcpu_pmu_restore_host(struct kvm_vcpu *vcpu); > + > +void setup_kvm_el2_caps(void); > #else > static inline void kvm_set_pmu_events(u32 set, struct perf_event_attr *attr) {} > static inline void kvm_clr_pmu_events(u32 clr) {} > + > +static inline void setup_kvm_el2_caps(void) {} > #endif > > void kvm_vcpu_load_sysregs_vhe(struct kvm_vcpu *vcpu); > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c > index 066030717a4c..f2d8b479ff74 100644 > --- a/arch/arm64/kernel/cpufeature.c > +++ b/arch/arm64/kernel/cpufeature.c > @@ -1154,6 +1154,18 @@ u64 read_sanitised_ftr_reg(u32 id) > } > EXPORT_SYMBOL_GPL(read_sanitised_ftr_reg); > > +int copy_ftr_reg(u32 id, struct arm64_ftr_reg *dst) > +{ > + struct arm64_ftr_reg *regp = get_arm64_ftr_reg(id); > + > + if (!regp) > + return -EINVAL; > + > + memcpy(dst, regp, sizeof(*regp)); Can you not just use struct assignment here? > diff --git a/arch/arm64/kvm/hyp/nvhe/cache.S b/arch/arm64/kvm/hyp/nvhe/cache.S > new file mode 100644 > index 000000000000..36cef6915428 > --- /dev/null > +++ b/arch/arm64/kvm/hyp/nvhe/cache.S > @@ -0,0 +1,13 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * Code copied from arch/arm64/mm/cache.S. > + */ > + > +#include <linux/linkage.h> > +#include <asm/assembler.h> > +#include <asm/alternative.h> > + > +SYM_FUNC_START_PI(__flush_dcache_area) > + dcache_by_line_op civac, sy, x0, x1, x2, x3 > + ret > +SYM_FUNC_END_PI(__flush_dcache_area) Separate patch for this? (or fold it into that one really near the start that introduces some other PI helpers). > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index 4f2f1e3145de..84be93df52fa 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -21,6 +21,7 @@ > #include <asm/debug-monitors.h> > #include <asm/esr.h> > #include <asm/kvm_arm.h> > +#include <asm/kvm_cpufeature.h> > #include <asm/kvm_emulate.h> > #include <asm/kvm_hyp.h> > #include <asm/kvm_mmu.h> > @@ -2775,3 +2776,23 @@ void kvm_sys_reg_table_init(void) > /* Clear all higher bits. */ > cache_levels &= (1 << (i*3))-1; > } > + > +#undef KVM_HYP_CPU_FTR_REG > +#define KVM_HYP_CPU_FTR_REG(id, name) \ > + { .sys_id = id, .dst = (struct arm64_ftr_reg *)&kvm_nvhe_sym(name) }, > +struct __ftr_reg_copy_entry { > + u32 sys_id; > + struct arm64_ftr_reg *dst; > +} hyp_ftr_regs[] __initdata = { > + #include <asm/kvm_cpufeature.h> > +}; This looks a bit elaborate to me. Why can't you just spell things out here like: #define KVM_HYP_CPU_FTR_REG(id, name) \ { .sys_id = id, .dst = (struct arm64_ftr_reg *)&kvm_nvhe_sym(name) } struct __ftr_reg_copy_entry { u32 sys_id; struct arm64_ftr_reg *dst; } hyp_ftr_regs[] __initdata = { KVM_HYP_CPU_FTR_REG(SYS_CTR_EL0, arm64_ftr_reg_ctrel0), ... }; and then have the header file be a normal, guarded header which just declares these things? The id parameter to the macro isn't used there. Will
On Thursday 11 Mar 2021 at 19:36:39 (+0000), Will Deacon wrote: > On Wed, Mar 10, 2021 at 05:57:30PM +0000, Quentin Perret wrote: > > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c > > index 066030717a4c..f2d8b479ff74 100644 > > --- a/arch/arm64/kernel/cpufeature.c > > +++ b/arch/arm64/kernel/cpufeature.c > > @@ -1154,6 +1154,18 @@ u64 read_sanitised_ftr_reg(u32 id) > > } > > EXPORT_SYMBOL_GPL(read_sanitised_ftr_reg); > > > > +int copy_ftr_reg(u32 id, struct arm64_ftr_reg *dst) > > +{ > > + struct arm64_ftr_reg *regp = get_arm64_ftr_reg(id); > > + > > + if (!regp) > > + return -EINVAL; > > + > > + memcpy(dst, regp, sizeof(*regp)); > > Can you not just use struct assignment here? Sure. > > diff --git a/arch/arm64/kvm/hyp/nvhe/cache.S b/arch/arm64/kvm/hyp/nvhe/cache.S > > new file mode 100644 > > index 000000000000..36cef6915428 > > --- /dev/null > > +++ b/arch/arm64/kvm/hyp/nvhe/cache.S > > @@ -0,0 +1,13 @@ > > +/* SPDX-License-Identifier: GPL-2.0-only */ > > +/* > > + * Code copied from arch/arm64/mm/cache.S. > > + */ > > + > > +#include <linux/linkage.h> > > +#include <asm/assembler.h> > > +#include <asm/alternative.h> > > + > > +SYM_FUNC_START_PI(__flush_dcache_area) > > + dcache_by_line_op civac, sy, x0, x1, x2, x3 > > + ret > > +SYM_FUNC_END_PI(__flush_dcache_area) > > Separate patch for this? (or fold it into that one really near the start > that introduces some other PI helpers). Right, I guess that'll make reverts and such easier so why not. > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > > index 4f2f1e3145de..84be93df52fa 100644 > > --- a/arch/arm64/kvm/sys_regs.c > > +++ b/arch/arm64/kvm/sys_regs.c > > @@ -21,6 +21,7 @@ > > #include <asm/debug-monitors.h> > > #include <asm/esr.h> > > #include <asm/kvm_arm.h> > > +#include <asm/kvm_cpufeature.h> > > #include <asm/kvm_emulate.h> > > #include <asm/kvm_hyp.h> > > #include <asm/kvm_mmu.h> > > @@ -2775,3 +2776,23 @@ void kvm_sys_reg_table_init(void) > > /* Clear all higher bits. */ > > cache_levels &= (1 << (i*3))-1; > > } > > + > > +#undef KVM_HYP_CPU_FTR_REG > > +#define KVM_HYP_CPU_FTR_REG(id, name) \ > > + { .sys_id = id, .dst = (struct arm64_ftr_reg *)&kvm_nvhe_sym(name) }, > > +struct __ftr_reg_copy_entry { > > + u32 sys_id; > > + struct arm64_ftr_reg *dst; > > +} hyp_ftr_regs[] __initdata = { > > + #include <asm/kvm_cpufeature.h> > > +}; > > This looks a bit elaborate to me. Why can't you just spell things out here > like: > > #define KVM_HYP_CPU_FTR_REG(id, name) \ > { .sys_id = id, .dst = (struct arm64_ftr_reg *)&kvm_nvhe_sym(name) } > > struct __ftr_reg_copy_entry { > u32 sys_id; > struct arm64_ftr_reg *dst; > } hyp_ftr_regs[] __initdata = { > KVM_HYP_CPU_FTR_REG(SYS_CTR_EL0, arm64_ftr_reg_ctrel0), > ... > }; > > and then have the header file be a normal, guarded header which just > declares these things? The id parameter to the macro isn't used there. I just tried to reduce the boilerplate as much as possible -- in the current form you only need to add additional features to the header it'll 'just work'. Thanks, Quentin
On Fri, Mar 12, 2021 at 06:34:09AM +0000, Quentin Perret wrote: > On Thursday 11 Mar 2021 at 19:36:39 (+0000), Will Deacon wrote: > > On Wed, Mar 10, 2021 at 05:57:30PM +0000, Quentin Perret wrote: > > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > > > index 4f2f1e3145de..84be93df52fa 100644 > > > --- a/arch/arm64/kvm/sys_regs.c > > > +++ b/arch/arm64/kvm/sys_regs.c > > > @@ -21,6 +21,7 @@ > > > #include <asm/debug-monitors.h> > > > #include <asm/esr.h> > > > #include <asm/kvm_arm.h> > > > +#include <asm/kvm_cpufeature.h> > > > #include <asm/kvm_emulate.h> > > > #include <asm/kvm_hyp.h> > > > #include <asm/kvm_mmu.h> > > > @@ -2775,3 +2776,23 @@ void kvm_sys_reg_table_init(void) > > > /* Clear all higher bits. */ > > > cache_levels &= (1 << (i*3))-1; > > > } > > > + > > > +#undef KVM_HYP_CPU_FTR_REG > > > +#define KVM_HYP_CPU_FTR_REG(id, name) \ > > > + { .sys_id = id, .dst = (struct arm64_ftr_reg *)&kvm_nvhe_sym(name) }, > > > +struct __ftr_reg_copy_entry { > > > + u32 sys_id; > > > + struct arm64_ftr_reg *dst; > > > +} hyp_ftr_regs[] __initdata = { > > > + #include <asm/kvm_cpufeature.h> > > > +}; > > > > This looks a bit elaborate to me. Why can't you just spell things out here > > like: > > > > #define KVM_HYP_CPU_FTR_REG(id, name) \ > > { .sys_id = id, .dst = (struct arm64_ftr_reg *)&kvm_nvhe_sym(name) } > > > > struct __ftr_reg_copy_entry { > > u32 sys_id; > > struct arm64_ftr_reg *dst; > > } hyp_ftr_regs[] __initdata = { > > KVM_HYP_CPU_FTR_REG(SYS_CTR_EL0, arm64_ftr_reg_ctrel0), > > ... > > }; > > > > and then have the header file be a normal, guarded header which just > > declares these things? The id parameter to the macro isn't used there. > > I just tried to reduce the boilerplate as much as possible -- in the > current form you only need to add additional features to the header > it'll 'just work'. I don't really think it's worth it -- people are used to having to add declarations for things, so keeping it simple should be fine. Will
diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h index 61177bac49fa..a85cea2cac57 100644 --- a/arch/arm64/include/asm/cpufeature.h +++ b/arch/arm64/include/asm/cpufeature.h @@ -607,6 +607,7 @@ void check_local_cpu_capabilities(void); u64 read_sanitised_ftr_reg(u32 id); u64 __read_sysreg_by_encoding(u32 sys_id); +int copy_ftr_reg(u32 id, struct arm64_ftr_reg *dst); static inline bool cpu_supports_mixed_endian_el0(void) { diff --git a/arch/arm64/include/asm/kvm_cpufeature.h b/arch/arm64/include/asm/kvm_cpufeature.h new file mode 100644 index 000000000000..d34f85cba358 --- /dev/null +++ b/arch/arm64/include/asm/kvm_cpufeature.h @@ -0,0 +1,17 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * Copyright (C) 2020 - Google LLC + * Author: Quentin Perret <qperret@google.com> + */ + +#include <asm/cpufeature.h> + +#ifndef KVM_HYP_CPU_FTR_REG +#if defined(__KVM_NVHE_HYPERVISOR__) +#define KVM_HYP_CPU_FTR_REG(id, name) extern struct arm64_ftr_reg name; +#else +#define KVM_HYP_CPU_FTR_REG(id, name) DECLARE_KVM_NVHE_SYM(name); +#endif +#endif + +KVM_HYP_CPU_FTR_REG(SYS_CTR_EL0, arm64_ftr_reg_ctrel0) diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h index 06ca4828005f..459ee557f87c 100644 --- a/arch/arm64/include/asm/kvm_host.h +++ b/arch/arm64/include/asm/kvm_host.h @@ -751,9 +751,13 @@ void kvm_clr_pmu_events(u32 clr); void kvm_vcpu_pmu_restore_guest(struct kvm_vcpu *vcpu); void kvm_vcpu_pmu_restore_host(struct kvm_vcpu *vcpu); + +void setup_kvm_el2_caps(void); #else static inline void kvm_set_pmu_events(u32 set, struct perf_event_attr *attr) {} static inline void kvm_clr_pmu_events(u32 clr) {} + +static inline void setup_kvm_el2_caps(void) {} #endif void kvm_vcpu_load_sysregs_vhe(struct kvm_vcpu *vcpu); diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c index 066030717a4c..f2d8b479ff74 100644 --- a/arch/arm64/kernel/cpufeature.c +++ b/arch/arm64/kernel/cpufeature.c @@ -1154,6 +1154,18 @@ u64 read_sanitised_ftr_reg(u32 id) } EXPORT_SYMBOL_GPL(read_sanitised_ftr_reg); +int copy_ftr_reg(u32 id, struct arm64_ftr_reg *dst) +{ + struct arm64_ftr_reg *regp = get_arm64_ftr_reg(id); + + if (!regp) + return -EINVAL; + + memcpy(dst, regp, sizeof(*regp)); + + return 0; +} + #define read_sysreg_case(r) \ case r: val = read_sysreg_s(r); break; @@ -2773,6 +2785,7 @@ void __init setup_cpu_features(void) setup_system_capabilities(); setup_elf_hwcaps(arm64_elf_hwcaps); + setup_kvm_el2_caps(); if (system_supports_32bit_el0()) setup_elf_hwcaps(compat_elf_hwcaps); diff --git a/arch/arm64/kvm/hyp/nvhe/Makefile b/arch/arm64/kvm/hyp/nvhe/Makefile index 6894a917f290..0033591553fc 100644 --- a/arch/arm64/kvm/hyp/nvhe/Makefile +++ b/arch/arm64/kvm/hyp/nvhe/Makefile @@ -13,7 +13,8 @@ lib-objs := clear_page.o copy_page.o memcpy.o memset.o lib-objs := $(addprefix ../../../lib/, $(lib-objs)) obj-y := timer-sr.o sysreg-sr.o debug-sr.o switch.o tlb.o hyp-init.o host.o \ - hyp-main.o hyp-smp.o psci-relay.o early_alloc.o stub.o page_alloc.o + hyp-main.o hyp-smp.o psci-relay.o early_alloc.o stub.o page_alloc.o \ + cache.o cpufeature.o obj-y += ../vgic-v3-sr.o ../aarch32.o ../vgic-v2-cpuif-proxy.o ../entry.o \ ../fpsimd.o ../hyp-entry.o ../exception.o obj-y += $(lib-objs) diff --git a/arch/arm64/kvm/hyp/nvhe/cache.S b/arch/arm64/kvm/hyp/nvhe/cache.S new file mode 100644 index 000000000000..36cef6915428 --- /dev/null +++ b/arch/arm64/kvm/hyp/nvhe/cache.S @@ -0,0 +1,13 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * Code copied from arch/arm64/mm/cache.S. + */ + +#include <linux/linkage.h> +#include <asm/assembler.h> +#include <asm/alternative.h> + +SYM_FUNC_START_PI(__flush_dcache_area) + dcache_by_line_op civac, sy, x0, x1, x2, x3 + ret +SYM_FUNC_END_PI(__flush_dcache_area) diff --git a/arch/arm64/kvm/hyp/nvhe/cpufeature.c b/arch/arm64/kvm/hyp/nvhe/cpufeature.c new file mode 100644 index 000000000000..a887508f996f --- /dev/null +++ b/arch/arm64/kvm/hyp/nvhe/cpufeature.c @@ -0,0 +1,8 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (C) 2020 - Google LLC + * Author: Quentin Perret <qperret@google.com> + */ + +#define KVM_HYP_CPU_FTR_REG(id, name) struct arm64_ftr_reg name; +#include <asm/kvm_cpufeature.h> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index 4f2f1e3145de..84be93df52fa 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -21,6 +21,7 @@ #include <asm/debug-monitors.h> #include <asm/esr.h> #include <asm/kvm_arm.h> +#include <asm/kvm_cpufeature.h> #include <asm/kvm_emulate.h> #include <asm/kvm_hyp.h> #include <asm/kvm_mmu.h> @@ -2775,3 +2776,23 @@ void kvm_sys_reg_table_init(void) /* Clear all higher bits. */ cache_levels &= (1 << (i*3))-1; } + +#undef KVM_HYP_CPU_FTR_REG +#define KVM_HYP_CPU_FTR_REG(id, name) \ + { .sys_id = id, .dst = (struct arm64_ftr_reg *)&kvm_nvhe_sym(name) }, +struct __ftr_reg_copy_entry { + u32 sys_id; + struct arm64_ftr_reg *dst; +} hyp_ftr_regs[] __initdata = { + #include <asm/kvm_cpufeature.h> +}; + +void __init setup_kvm_el2_caps(void) +{ + int i; + + for (i = 0; i < ARRAY_SIZE(hyp_ftr_regs); i++) { + WARN(copy_ftr_reg(hyp_ftr_regs[i].sys_id, hyp_ftr_regs[i].dst), + "%u feature register not found\n", hyp_ftr_regs[i].sys_id); + } +}
Introduce the infrastructure in KVM enabling to copy CPU feature registers into EL2-owned data-structures, to allow reading sanitised values directly at EL2 in nVHE. Given that only a subset of these features are being read by the hypervisor, the ones that need to be copied are to be listed under <asm/kvm_cpufeature.h> together with the name of the nVHE variable that will hold the copy. While at it, introduce the first user of this infrastructure by implementing __flush_dcache_area at EL2, which needs arm64_ftr_reg_ctrel0. Signed-off-by: Quentin Perret <qperret@google.com> --- arch/arm64/include/asm/cpufeature.h | 1 + arch/arm64/include/asm/kvm_cpufeature.h | 17 +++++++++++++++++ arch/arm64/include/asm/kvm_host.h | 4 ++++ arch/arm64/kernel/cpufeature.c | 13 +++++++++++++ arch/arm64/kvm/hyp/nvhe/Makefile | 3 ++- arch/arm64/kvm/hyp/nvhe/cache.S | 13 +++++++++++++ arch/arm64/kvm/hyp/nvhe/cpufeature.c | 8 ++++++++ arch/arm64/kvm/sys_regs.c | 21 +++++++++++++++++++++ 8 files changed, 79 insertions(+), 1 deletion(-) create mode 100644 arch/arm64/include/asm/kvm_cpufeature.h create mode 100644 arch/arm64/kvm/hyp/nvhe/cache.S create mode 100644 arch/arm64/kvm/hyp/nvhe/cpufeature.c