Re: [PATCH v4 13/34] KVM: arm64: Enable access to sanitized CPU features at EL2
On Fri, Mar 12, 2021 at 06:34:09AM +, Quentin Perret wrote:
> On Thursday 11 Mar 2021 at 19:36:39 (+), Will Deacon wrote:
> > On Wed, Mar 10, 2021 at 05:57:30PM +, 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
> > > #include
> > > #include
> > > +#include
> > > #include
> > > #include
> > > #include
> > > @@ -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 *)_nvhe_sym(name) },
> > > +struct __ftr_reg_copy_entry {
> > > + u32 sys_id;
> > > + struct arm64_ftr_reg*dst;
> > > +} hyp_ftr_regs[] __initdata = {
> > > + #include
> > > +};
> >
> > 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 *)_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
Re: [PATCH v4 13/34] KVM: arm64: Enable access to sanitized CPU features at EL2
On Thursday 11 Mar 2021 at 19:36:39 (+), Will Deacon wrote:
> On Wed, Mar 10, 2021 at 05:57:30PM +, 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 ..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
> > +#include
> > +#include
> > +
> > +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
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -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 *)_nvhe_sym(name) },
> > +struct __ftr_reg_copy_entry {
> > + u32 sys_id;
> > + struct arm64_ftr_reg*dst;
> > +} hyp_ftr_regs[] __initdata = {
> > + #include
> > +};
>
> 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 *)_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
Re: [PATCH v4 13/34] KVM: arm64: Enable access to sanitized CPU features at EL2
On Wed, Mar 10, 2021 at 05:57:30PM +, 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
> 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
> ---
> 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 ..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
> + */
> +
> +#include
> +
> +#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 ..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
> +#include
> +#include
> +
> +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
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -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
[PATCH v4 13/34] KVM: arm64: Enable access to sanitized CPU features at EL2
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
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
---
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 ..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
+ */
+
+#include
+
+#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 ..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
+#include
+#include
+
