Re: [RFC PATCH v2 13/26] KVM: arm64: Enable access to sanitized CPU features at EL2
On Wednesday 13 Jan 2021 at 17:27:49 (+), Marc Zyngier wrote: > On 2021-01-13 14:35, Quentin Perret wrote: > > On Wednesday 13 Jan 2021 at 14:23:03 (+), Quentin Perret wrote: > > > Good point, that would be nice indeed. Can I use that from outside an > > > __init function? > > > > Just gave it a go, and the answer to this appears to be yes, > > surprisingly -- I was expecting a compile-time warning similar to what > > we get when non-__init code calls into __init, but that doesn't seem to > > trigger here. Anyways, I'll add the annotation in v3. > > That's surprising. I'd definitely expect something to explode... > Do you have CONFIG_DEBUG_SECTION_MISMATCH=y? Yes I do, so, that doesn't seem to be it. Now, the plot thickens: I _do_ get a warning if I remove the 'const' qualifier. But interestingly, in both cases hyp_ftr_regs is placed in .init.data: $ objdump -t vmlinux | grep hyp_ftr_regs 8000116c17b0 g O .init.data 0030 hyp_ftr_regs The warning is silenced only if I mark hyp_ftr_regs as const. modpost bug? I'll double check my findings and follow up in a separate series. Thanks, Quentin ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [RFC PATCH v2 13/26] KVM: arm64: Enable access to sanitized CPU features at EL2
On Wednesday 13 Jan 2021 at 14:23:03 (+), Quentin Perret wrote: > Good point, that would be nice indeed. Can I use that from outside an > __init function? Just gave it a go, and the answer to this appears to be yes, surprisingly -- I was expecting a compile-time warning similar to what we get when non-__init code calls into __init, but that doesn't seem to trigger here. Anyways, I'll add the annotation in v3. Thanks, Quentin ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [RFC PATCH v2 13/26] KVM: arm64: Enable access to sanitized CPU features at EL2
Hey Marc,
On Wednesday 13 Jan 2021 at 11:33:13 (+), Marc Zyngier wrote:
> > +#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) },
> > +static const struct __ftr_reg_copy_entry {
> > + u32 sys_id;
> > + struct arm64_ftr_reg*dst;
>
> Why do we need the whole data structure? Can't we just live with sys_val?
I don't have a use-case for anything else than sys_val, so yes I think I
should be able to simplify. I'll try that for v3.
>
> > +} hyp_ftr_regs[] = {
> > + #include
> > +};
>
> Can't this be made __initdata?
Good point, that would be nice indeed. Can I use that from outside an
__init function? If not, I'll need to rework the code a bit more, but
that should be simple enough either way.
> > +
> > +static int copy_cpu_ftr_regs(void)
> > +{
> > + int i, ret;
> > +
> > + for (i = 0; i < ARRAY_SIZE(hyp_ftr_regs); i++) {
> > + ret = copy_ftr_reg(hyp_ftr_regs[i].sys_id, hyp_ftr_regs[i].dst);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > /**
> > * Inits Hyp-mode on all online CPUs
> > */
> > @@ -1705,6 +1729,13 @@ static int init_hyp_mode(void)
> > int cpu;
> > int err = 0;
> >
> > + /*
> > +* Copy the required CPU feature register in their EL2 counterpart
> > +*/
> > + err = copy_cpu_ftr_regs();
> > + if (err)
> > + return err;
> > +
>
> Just to keep things together, please move any sysreg manipulation into
> sys_regs.c, most probably into kvm_sys_reg_table_init().
Will do.
Thanks,
Quentin
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [RFC PATCH v2 13/26] KVM: arm64: Enable access to sanitized CPU features at EL2
On 2021-01-13 14:35, Quentin Perret wrote: On Wednesday 13 Jan 2021 at 14:23:03 (+), Quentin Perret wrote: Good point, that would be nice indeed. Can I use that from outside an __init function? Just gave it a go, and the answer to this appears to be yes, surprisingly -- I was expecting a compile-time warning similar to what we get when non-__init code calls into __init, but that doesn't seem to trigger here. Anyways, I'll add the annotation in v3. That's surprising. I'd definitely expect something to explode... Do you have CONFIG_DEBUG_SECTION_MISMATCH=y? M. -- Jazz is not dead. It just smells funny... ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [RFC PATCH v2 13/26] KVM: arm64: Enable access to sanitized CPU features at EL2
Hi Quentin,
On 2021-01-08 12:15, 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/kernel/cpufeature.c | 12 ++
arch/arm64/kvm/arm.c| 31 +
arch/arm64/kvm/hyp/nvhe/Makefile| 3 ++-
arch/arm64/kvm/hyp/nvhe/cache.S | 13 +++
arch/arm64/kvm/hyp/nvhe/cpufeature.c| 8 +++
7 files changed, 84 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 16063c813dcd..742e9bcc051b 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -600,6 +600,7 @@ void __init setup_cpu_features(void);
void check_local_cpu_capabilities(void);
u64 read_sanitised_ftr_reg(u32 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/kernel/cpufeature.c
b/arch/arm64/kernel/cpufeature.c
index bc3549663957..c2019dc3 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -1113,6 +1113,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: return read_sysreg_s(r)
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index 51b53ca36dc5..9fd769349e9e 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -34,6 +34,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -1697,6 +1698,29 @@ static void teardown_hyp_mode(void)
}
}
+#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) },
+static const struct __ftr_reg_copy_entry {
+ u32 sys_id;
+ struct arm64_ftr_reg*dst;
Why do we need the whole data structure? Can't we just live with
sys_val?
+} hyp_ftr_regs[] = {
+ #include
+};
Can't this be made __initdata?
+
+static int copy_cpu_ftr_regs(void)
+{
+ int i, ret;
+
+ for (i = 0; i < ARRAY_SIZE(hyp_ftr_regs); i++) {
+ ret = copy_ftr_reg(hyp_ftr_regs[i].sys_id, hyp_ftr_regs[i].dst);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
/**
* Inits Hyp-mode on all online CPUs
*/
@@ -1705,6 +1729,13 @@ static int init_hyp_mode(void)
int cpu;
int err = 0;
+ /*
+* Copy the required CPU feature register in their EL2 counterpart
+*/
+ err = copy_cpu_ftr_regs();
+ if (err)
+ return err;
+
Just to keep things together, please move any sysreg manipulation into
sys_regs.c, most probably into kvm_sys_reg_table_init().
Thanks,
M.
--
Jazz is not dead. It just smells funny...
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
