diff mbox series

[RFC,v2,13/26] KVM: arm64: Enable access to sanitized CPU features at EL2

Message ID 20210108121524.656872-14-qperret@google.com
State New
Headers show
Series KVM/arm64: A stage 2 for the host | expand

Commit Message

Quentin Perret Jan. 8, 2021, 12:15 p.m. UTC 
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/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

Comments

Marc Zyngier Jan. 13, 2021, 11:33 a.m. UTC | #1 
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

> <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/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 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/kernel/cpufeature.c 

> b/arch/arm64/kernel/cpufeature.c

> index bc3549663957..c2019aaaadc3 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 <asm/virt.h>

>  #include <asm/kvm_arm.h>

>  #include <asm/kvm_asm.h>

> +#include <asm/kvm_cpufeature.h>

>  #include <asm/kvm_mmu.h>

>  #include <asm/kvm_emulate.h>

>  #include <asm/sections.h>

> @@ -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 <asm/kvm_cpufeature.h>

> +};


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...
Quentin Perret Jan. 13, 2021, 2:23 p.m. UTC | #2 
Hey Marc,

On Wednesday 13 Jan 2021 at 11:33:13 (+0000), 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 <asm/kvm_cpufeature.h>

> > +};

> 

> 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
Quentin Perret Jan. 13, 2021, 2:35 p.m. UTC | #3 
On Wednesday 13 Jan 2021 at 14:23:03 (+0000), 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
Marc Zyngier Jan. 13, 2021, 5:27 p.m. UTC | #4 
On 2021-01-13 14:35, Quentin Perret wrote:
> On Wednesday 13 Jan 2021 at 14:23:03 (+0000), 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...
Quentin Perret Jan. 13, 2021, 6:28 p.m. UTC | #5 
On Wednesday 13 Jan 2021 at 17:27:49 (+0000), Marc Zyngier wrote:
> On 2021-01-13 14:35, Quentin Perret wrote:

> > On Wednesday 13 Jan 2021 at 14:23:03 (+0000), 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
  ffff8000116c17b0 g     O .init.data     0000000000000030 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
diff mbox series

Patch

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 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/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index bc3549663957..c2019aaaadc3 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 <asm/virt.h>
 #include <asm/kvm_arm.h>
 #include <asm/kvm_asm.h>
+#include <asm/kvm_cpufeature.h>
 #include <asm/kvm_mmu.h>
 #include <asm/kvm_emulate.h>
 #include <asm/sections.h>
@@ -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;
+} hyp_ftr_regs[] = {
+	#include <asm/kvm_cpufeature.h>
+};
+
+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;
+
 	/*
 	 * Allocate Hyp PGD and setup Hyp identity mapping
 	 */
diff --git a/arch/arm64/kvm/hyp/nvhe/Makefile b/arch/arm64/kvm/hyp/nvhe/Makefile
index 9e5eacfec6ec..72cfe53f106f 100644
--- a/arch/arm64/kvm/hyp/nvhe/Makefile
+++ b/arch/arm64/kvm/hyp/nvhe/Makefile
@@ -10,7 +10,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>