Fixes and cleanups of PSCI relay for kvmarm/next

PATCH Applied David Brazdil 3 participants 11 messages Dec 8, 2020 – Dec 22, 2020

Subsystems (2)

arm64 KVM arm64

lore

Share & tools

lore

David Brazdil

author

2020-12-08 14:25 UTC #0 lore raw

cover Fixes and cleanups of PSCI relay for kvmarm/next

Small batch of improvements for the 'Opt-in always-on nVHE hypervisor' series, now merged in kvmarm/next.

Patch #1 fixes potential use of invalid v0.1 functions IDs reported by Mark Rutland, patch #2 fixes a warning reported by Qian Cai. Patch #3 avoids a code path not used in VHE, can be dropped if any concerns arise. The remaining patches are minor cleanups from review.

-David

David Brazdil (6):
kvm: arm64: Prevent use of invalid PSCI v0.1 function IDs
kvm: arm64: Use lm_alias in nVHE-only VA conversion
kvm: arm64: Skip computing hyp VA layout for VHE
kvm: arm64: Minor cleanup of hyp variables used in host
kvm: arm64: Remove unused includes in psci-relay.c
kvm: arm64: Move skip_host_instruction to adjust_pc.h

 arch/arm64/include/asm/kvm_host.h          | 26 ++++++++++
 arch/arm64/kernel/smp.c                    |  2 +-
 arch/arm64/kvm/arm.c                       | 18 ++++---
 arch/arm64/kvm/hyp/include/hyp/adjust_pc.h |  9 ++++
 arch/arm64/kvm/hyp/nvhe/hyp-main.c         | 12 +----
 arch/arm64/kvm/hyp/nvhe/hyp-smp.c          |  6 +--
 arch/arm64/kvm/hyp/nvhe/psci-relay.c       | 56 +++++++++++++++-------
 arch/arm64/kvm/va_layout.c                 |  7 ++-
 8 files changed, 95 insertions(+), 41 deletions(-)

--
2.29.2.576.ga3fc446d84-goog
_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

David Brazdil

author

2020-12-08 14:25 UTC #1 lore raw

PATCH 1/6 kvm: arm64: Prevent use of invalid PSCI v0.1 function IDs

PSCI driver exposes a struct containing the PSCI v0.1 function IDs configured in the DT. However, the struct does not convey the information whether these were set from DT or contain the default value zero. This could be a problem for PSCI proxy in KVM protected mode.

Extend config passed to KVM with a bit mask with individual bits set depending on whether the corresponding function pointer in psci_ops is set, eg. set bit for PSCI_CPU_SUSPEND if psci_ops.cpu_suspend != NULL.

Previously config was split into multiple global variables. Put everything into a single struct for convenience.

Reported-by Mark Rutland <[email protected]>

Signed-off-by David Brazdil <[email protected]>

---
 arch/arm64/include/asm/kvm_host.h    | 20 +++++++++++
 arch/arm64/kvm/arm.c                 | 14 +++++---
 arch/arm64/kvm/hyp/nvhe/psci-relay.c | 53 +++++++++++++++++++++-------
 3 files changed, 70 insertions(+), 17 deletions(-)

arch/arm64/include/asm/kvm_host.h +20 -0

index 11beda85ee7e..828d50d40dc2 100644
@@ -17,6 +17,7 @@
 #include <linux/jump_label.h>
 #include <linux/kvm_types.h>
 #include <linux/percpu.h>
+#include <linux/psci.h>
 #include <asm/arch_gicv3.h>
 #include <asm/barrier.h>
 #include <asm/cpufeature.h>

@@ -240,6 +241,25 @@ struct kvm_host_data {
 	struct kvm_pmu_events pmu_events;
 };
 
+#define KVM_HOST_PSCI_0_1_CPU_SUSPEND	BIT(0)
+#define KVM_HOST_PSCI_0_1_CPU_ON	BIT(1)
+#define KVM_HOST_PSCI_0_1_CPU_OFF	BIT(2)
+#define KVM_HOST_PSCI_0_1_MIGRATE	BIT(3)
+
+struct kvm_host_psci_config {
+	/* PSCI version used by host. */
+	u32 version;
+
+	/* Function IDs used by host if version is v0.1. */
+	struct psci_0_1_function_ids function_ids_0_1;
+
+	/* Bitmask of functions enabled for v0.1, bits KVM_HOST_PSCI_0_1_*. */
+	unsigned int enabled_functions_0_1;
+};
+
+extern struct kvm_host_psci_config kvm_nvhe_sym(kvm_host_psci_config);
+#define kvm_host_psci_config CHOOSE_NVHE_SYM(kvm_host_psci_config)
+
 struct vcpu_reset_state {
 	unsigned long	pc;
 	unsigned long	r0;

arch/arm64/kvm/arm.c +10 -4

index 6e637d2b4cfb..6a2f4e01b04f 100644
@@ -66,8 +66,6 @@ static DEFINE_PER_CPU(unsigned char, kvm_arm_hardware_enabled);
 DEFINE_STATIC_KEY_FALSE(userspace_irqchip_in_use);
 
 extern u64 kvm_nvhe_sym(__cpu_logical_map)[NR_CPUS];
-extern u32 kvm_nvhe_sym(kvm_host_psci_version);
-extern struct psci_0_1_function_ids kvm_nvhe_sym(kvm_host_psci_0_1_function_ids);
 
 int kvm_arch_vcpu_should_kick(struct kvm_vcpu *vcpu)
 {

@@ -1618,8 +1616,16 @@ static bool init_psci_relay(void)
 		return false;
 	}
 
-	kvm_nvhe_sym(kvm_host_psci_version) = psci_ops.get_version();
-	kvm_nvhe_sym(kvm_host_psci_0_1_function_ids) = get_psci_0_1_function_ids();
+	kvm_host_psci_config.version = psci_ops.get_version();
+
+	if (kvm_host_psci_config.version == PSCI_VERSION(0, 1)) {
+		kvm_host_psci_config.function_ids_0_1 = get_psci_0_1_function_ids();
+		kvm_host_psci_config.enabled_functions_0_1 =
+			(psci_ops.cpu_suspend ? KVM_HOST_PSCI_0_1_CPU_SUSPEND : 0) |
+			(psci_ops.cpu_off ? KVM_HOST_PSCI_0_1_CPU_OFF : 0) |
+			(psci_ops.cpu_on ? KVM_HOST_PSCI_0_1_CPU_ON : 0) |
+			(psci_ops.migrate ? KVM_HOST_PSCI_0_1_MIGRATE : 0);
+	}
 	return true;
 }

arch/arm64/kvm/hyp/nvhe/psci-relay.c +40 ~~-13~~

87 lines not on this page · raw message

Signature · 6 lines

2.29.2.576.ga3fc446d84-goog

_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Marc Zyngier

maintainer

2020-12-08 15:56 UTC #2 lore raw

Re: [PATCH 1/6] kvm: arm64: Prevent use of invalid PSCI v0.1 function IDs

Quoted 42 lines · David Brazdil

PSCI driver exposes a struct containing the PSCI v0.1 function IDs configured in the DT. However, the struct does not convey the information whether these were set from DT or contain the default value zero. This could be a problem for PSCI proxy in KVM protected mode.
Extend config passed to KVM with a bit mask with individual bits set depending on whether the corresponding function pointer in psci_ops is set, eg. set bit for PSCI_CPU_SUSPEND if psci_ops.cpu_suspend != NULL.
Previously config was split into multiple global variables. Put everything into a single struct for convenience.
Reported-by Mark Rutland <[email protected]>
Signed-off-by David Brazdil <[email protected]>
---
 arch/arm64/include/asm/kvm_host.h    | 20 +++++++++++
 arch/arm64/kvm/arm.c                 | 14 +++++---
 arch/arm64/kvm/hyp/nvhe/psci-relay.c | 53 +++++++++++++++++++++-------
 3 files changed, 70 insertions(+), 17 deletions(-)
arch/arm64/include/asm/kvm_host.h +6 -0
index 11beda85ee7e..828d50d40dc2 100644
@@ -17,6 +17,7 @@
 #include <linux/jump_label.h>
 #include <linux/kvm_types.h>
 #include <linux/percpu.h>
+#include <linux/psci.h>
 #include <asm/arch_gicv3.h>
 #include <asm/barrier.h>
 #include <asm/cpufeature.h>
@@ -240,6 +241,25 @@ struct kvm_host_data {
 	struct kvm_pmu_events pmu_events;
 };
 
+#define KVM_HOST_PSCI_0_1_CPU_SUSPEND	BIT(0)
+#define KVM_HOST_PSCI_0_1_CPU_ON	BIT(1)
+#define KVM_HOST_PSCI_0_1_CPU_OFF	BIT(2)
+#define KVM_HOST_PSCI_0_1_MIGRATE	BIT(3)
+

arch/arm64/include/asm/kvm_host.h struct kvm_host_data { · line 258

Quoted 10 lines

arch/arm64/include/asm/kvm_host.h +9 -0

+struct kvm_host_psci_config {
+	/* PSCI version used by host. */
+	u32 version;
+
+	/* Function IDs used by host if version is v0.1. */
+	struct psci_0_1_function_ids function_ids_0_1;
+
+	/* Bitmask of functions enabled for v0.1, bits KVM_HOST_PSCI_0_1_*. 
 */
+	unsigned int enabled_functions_0_1;

Nit: the conventional type for bitmaps is 'unsigned long'. Also, "enabled" seems odd. Isn't it actually "available"?

Quoted 49 lines

arch/arm64/include/asm/kvm_host.h +5 -0

+};
+
+extern struct kvm_host_psci_config kvm_nvhe_sym(kvm_host_psci_config);
+#define kvm_host_psci_config CHOOSE_NVHE_SYM(kvm_host_psci_config)
+
 struct vcpu_reset_state {
 	unsigned long	pc;

arch/arm64/include/asm/kvm_host.h +0 -0

 	unsigned long	r0;

arch/arm64/kvm/arm.c +0 -2

index 6e637d2b4cfb..6a2f4e01b04f 100644
@@ -66,8 +66,6 @@ static DEFINE_PER_CPU(unsigned char,
 kvm_arm_hardware_enabled);
 DEFINE_STATIC_KEY_FALSE(userspace_irqchip_in_use);
 
 extern u64 kvm_nvhe_sym(__cpu_logical_map)[NR_CPUS];
-extern u32 kvm_nvhe_sym(kvm_host_psci_version);
-extern struct psci_0_1_function_ids
 kvm_nvhe_sym(kvm_host_psci_0_1_function_ids);

int kvm_arch_vcpu_should_kick(struct kvm_vcpu *vcpu) {

arch/arm64/kvm/arm.c +10 -2

@@ -1618,8 +1616,16 @@ static bool init_psci_relay(void)
 		return false;
 	}
 
-	kvm_nvhe_sym(kvm_host_psci_version) = psci_ops.get_version();
-	kvm_nvhe_sym(kvm_host_psci_0_1_function_ids) = 
 get_psci_0_1_function_ids();
+	kvm_host_psci_config.version = psci_ops.get_version();
+
+	if (kvm_host_psci_config.version == PSCI_VERSION(0, 1)) {
+		kvm_host_psci_config.function_ids_0_1 = get_psci_0_1_function_ids();
+		kvm_host_psci_config.enabled_functions_0_1 =
+			(psci_ops.cpu_suspend ? KVM_HOST_PSCI_0_1_CPU_SUSPEND : 0) |
+			(psci_ops.cpu_off ? KVM_HOST_PSCI_0_1_CPU_OFF : 0) |
+			(psci_ops.cpu_on ? KVM_HOST_PSCI_0_1_CPU_ON : 0) |
+			(psci_ops.migrate ? KVM_HOST_PSCI_0_1_MIGRATE : 0);
+	}
 	return true;
 }

arch/arm64/kvm/hyp/nvhe/psci-relay.c +0 -0

index 08dc9de69314..0d6f4aa39621 100644

arch/arm64/kvm/hyp/nvhe/psci-relay.c void kvm_hyp_cpu_resume(unsigned long r0); · line 27

Quoted 9 lines

arch/arm64/kvm/hyp/nvhe/psci-relay.c +2 -3

@@ -22,9 +22,8 @@ void kvm_hyp_cpu_resume(unsigned long r0);
 void __noreturn __host_enter(struct kvm_cpu_context *host_ctxt);
 
 /* Config options set by the host. */
-__ro_after_init u32 kvm_host_psci_version;
-__ro_after_init struct psci_0_1_function_ids 
 kvm_host_psci_0_1_function_ids;
-__ro_after_init s64 hyp_physvirt_offset;
+struct kvm_host_psci_config __ro_after_init kvm_host_psci_config;
+s64 __ro_after_init hyp_physvirt_offset;

Unrelated change?

arch/arm64/kvm/hyp/nvhe/psci-relay.c static u64 get_psci_func_id(struct kvm_cpu_context · line 57

Quoted 9 lines

arch/arm64/kvm/hyp/nvhe/psci-relay.c +1 -0

 #define __hyp_pa(x) ((phys_addr_t)((x)) + hyp_physvirt_offset)

@@ -54,12 +53,41 @@ static u64 get_psci_func_id(struct kvm_cpu_context
 *host_ctxt)
 	return func_id;
 }
 
+static inline bool is_psci_0_1_function_enabled(unsigned int fn_bit)

Don't bother with "inline" outside of an include file. It really doesn't mean much (the compiler is free to ignore it), and it is likely that the compiler will optimise better without guidance (not to mention this is hardly a fast path anyway).

Quoted 78 lines

arch/arm64/kvm/hyp/nvhe/psci-relay.c +32 -4

+{
+	return kvm_host_psci_config.enabled_functions_0_1 & fn_bit;
+}
+
+static inline bool is_psci_0_1_cpu_suspend(u64 func_id)
+{
+	return is_psci_0_1_function_enabled(KVM_HOST_PSCI_0_1_CPU_SUSPEND) &&
+	       (func_id == 
 kvm_host_psci_config.function_ids_0_1.cpu_suspend);
+}
+
+static inline bool is_psci_0_1_cpu_on(u64 func_id)
+{
+	return is_psci_0_1_function_enabled(KVM_HOST_PSCI_0_1_CPU_ON) &&
+	       (func_id == kvm_host_psci_config.function_ids_0_1.cpu_on);
+}
+
+static inline bool is_psci_0_1_cpu_off(u64 func_id)
+{
+	return is_psci_0_1_function_enabled(KVM_HOST_PSCI_0_1_CPU_OFF) &&
+	       (func_id == kvm_host_psci_config.function_ids_0_1.cpu_off);
+}
+
+static inline bool is_psci_0_1_migrate(u64 func_id)
+{
+	return is_psci_0_1_function_enabled(KVM_HOST_PSCI_0_1_MIGRATE) &&
+	       (func_id == kvm_host_psci_config.function_ids_0_1.migrate);
+}
+
 static bool is_psci_0_1_call(u64 func_id)
 {
-	return (func_id == kvm_host_psci_0_1_function_ids.cpu_suspend) ||
-	       (func_id == kvm_host_psci_0_1_function_ids.cpu_on) ||
-	       (func_id == kvm_host_psci_0_1_function_ids.cpu_off) ||
-	       (func_id == kvm_host_psci_0_1_function_ids.migrate);
+	return is_psci_0_1_cpu_suspend(func_id) ||
+	       is_psci_0_1_cpu_on(func_id) ||
+	       is_psci_0_1_cpu_off(func_id) ||
+	       is_psci_0_1_migrate(func_id);
 }

static bool is_psci_0_2_call(u64 func_id)

arch/arm64/kvm/hyp/nvhe/psci-relay.c +4 -5

@@ -71,7 +99,7 @@ static bool is_psci_0_2_call(u64 func_id)
 
 static bool is_psci_call(u64 func_id)
 {
-	switch (kvm_host_psci_version) {
+	switch (kvm_host_psci_config.version) {
 	case PSCI_VERSION(0, 1):
 		return is_psci_0_1_call(func_id);
 	default:

@@ -248,12 +276,11 @@ asmlinkage void __noreturn
 kvm_host_psci_cpu_entry(bool is_cpu_on)
 
 static unsigned long psci_0_1_handler(u64 func_id, struct
 kvm_cpu_context *host_ctxt)
 {
-	if ((func_id == kvm_host_psci_0_1_function_ids.cpu_off) ||
-	    (func_id == kvm_host_psci_0_1_function_ids.migrate))
+	if (is_psci_0_1_cpu_off(func_id) || is_psci_0_1_migrate(func_id))
 		return psci_forward(host_ctxt);
-	else if (func_id == kvm_host_psci_0_1_function_ids.cpu_on)
+	else if (is_psci_0_1_cpu_on(func_id))
 		return psci_cpu_on(func_id, host_ctxt);
-	else if (func_id == kvm_host_psci_0_1_function_ids.cpu_suspend)
+	else if (is_psci_0_1_cpu_suspend(func_id))
 		return psci_cpu_suspend(func_id, host_ctxt);

arch/arm64/kvm/hyp/nvhe/psci-relay.c +1 -1

 	else
 		return PSCI_RET_NOT_SUPPORTED;

@@ -304,7 +331,7 @@ bool kvm_host_psci_handler(struct kvm_cpu_context
 *host_ctxt)
 	if (!is_psci_call(func_id))
 		return false;
 
-	switch (kvm_host_psci_version) {
+	switch (kvm_host_psci_config.version) {
 	case PSCI_VERSION(0, 1):
 		ret = psci_0_1_handler(func_id, host_ctxt);

Quoted 1 line

break;

Otherwise looks OK. Don't bother respinning the series for my comments, I can tidy things up as I apply it if there are no other issues.

Thanks,

M.

Signature · 5 lines

Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Mark Rutland

reviewer

2020-12-08 17:26 UTC #3 lore raw

Re: [PATCH 1/6] kvm: arm64: Prevent use of invalid PSCI v0.1 function IDs

Quoted 47 lines · Marc Zyngier

Quoted 46 lines · David Brazdil
PSCI driver exposes a struct containing the PSCI v0.1 function IDs configured in the DT. However, the struct does not convey the information whether these were set from DT or contain the default value zero. This could be a problem for PSCI proxy in KVM protected mode.
Extend config passed to KVM with a bit mask with individual bits set depending on whether the corresponding function pointer in psci_ops is set, eg. set bit for PSCI_CPU_SUSPEND if psci_ops.cpu_suspend != NULL.
Previously config was split into multiple global variables. Put everything into a single struct for convenience.
Reported-by Mark Rutland <[email protected]>
Signed-off-by David Brazdil <[email protected]>
---
 arch/arm64/include/asm/kvm_host.h    | 20 +++++++++++
 arch/arm64/kvm/arm.c                 | 14 +++++---
 arch/arm64/kvm/hyp/nvhe/psci-relay.c | 53 +++++++++++++++++++++-------
 3 files changed, 70 insertions(+), 17 deletions(-)
arch/arm64/include/asm/kvm_host.h +10 -0
index 11beda85ee7e..828d50d40dc2 100644
@@ -17,6 +17,7 @@
 #include <linux/jump_label.h>
 #include <linux/kvm_types.h>
 #include <linux/percpu.h>
+#include <linux/psci.h>
 #include <asm/arch_gicv3.h>
 #include <asm/barrier.h>
 #include <asm/cpufeature.h>
@@ -240,6 +241,25 @@ struct kvm_host_data {
 	struct kvm_pmu_events pmu_events;
 };
 
+#define KVM_HOST_PSCI_0_1_CPU_SUSPEND	BIT(0)
+#define KVM_HOST_PSCI_0_1_CPU_ON	BIT(1)
+#define KVM_HOST_PSCI_0_1_CPU_OFF	BIT(2)
+#define KVM_HOST_PSCI_0_1_MIGRATE	BIT(3)
+
+struct kvm_host_psci_config {
+	/* PSCI version used by host. */
+	u32 version;
+

Quoted 8 lines

arch/arm64/include/asm/kvm_host.h struct kvm_host_data { · line 257
Quoted 5 lines
arch/arm64/include/asm/kvm_host.h +5 -0
+	/* Function IDs used by host if version is v0.1. */
+	struct psci_0_1_function_ids function_ids_0_1;
+
+	/* Bitmask of functions enabled for v0.1, bits KVM_HOST_PSCI_0_1_*. */
+	unsigned int enabled_functions_0_1;
Nit: the conventional type for bitmaps is 'unsigned long'. Also, "enabled" seems odd. Isn't it actually "available"?

Sure, that or "implemented" works here.

Since there are only 4 functions here, it might make sense to use independent bools rather than a bitmap, which might make this a bit simpler...

Quoted 10 lines

arch/arm64/include/asm/kvm_host.h struct kvm_host_data {

Quoted 10 lines

arch/arm64/include/asm/kvm_host.h +9 -0

 get_psci_0_1_function_ids();
+	kvm_host_psci_config.version = psci_ops.get_version();
+
+	if (kvm_host_psci_config.version == PSCI_VERSION(0, 1)) {
+		kvm_host_psci_config.function_ids_0_1 = get_psci_0_1_function_ids();
+		kvm_host_psci_config.enabled_functions_0_1 =
+			(psci_ops.cpu_suspend ? KVM_HOST_PSCI_0_1_CPU_SUSPEND : 0) |
+			(psci_ops.cpu_off ? KVM_HOST_PSCI_0_1_CPU_OFF : 0) |
+			(psci_ops.cpu_on ? KVM_HOST_PSCI_0_1_CPU_ON : 0) |
+			(psci_ops.migrate ? KVM_HOST_PSCI_0_1_MIGRATE : 0);

... since e.g. this could be roughly:

	kvm_host_psci_config.cpu_suspend_implemented = psci_ops.cpu_suspend;
	kvm_host_psci_config.cpu_off_implemented = psci_ops.cpu_off;
	kvm_host_psci_config.cpu_on_implemented = psci_ops.cpu_on;
	kvm_host_psci_config.migrate_implemented = psci_ops.migrate;

Quoted 5 lines

arch/arm64/include/asm/kvm_host.h

Quoted 5 lines

arch/arm64/include/asm/kvm_host.h +5 -0

+static inline bool is_psci_0_1_cpu_suspend(u64 func_id)
+{
+	return is_psci_0_1_function_enabled(KVM_HOST_PSCI_0_1_CPU_SUSPEND) &&
+	       (func_id == kvm_host_psci_config.function_ids_0_1.cpu_suspend);
+}

...and similarly:

	return  kvm_host_psci_config.cpu_suspend_implemented &&
		func_id == kvm_host_psci_config.function_ids_0_1.cpu_suspend)

Quoted 3 lines

Otherwise looks OK. Don't bother respinning the series for my comments, I can tidy things up as I apply it if there are no other issues.

FWIW, I'm happy with whatever choose to do here, so don't feel like you have to follow my suggestions above.

Thanks,
Mark.
_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Marc Zyngier

maintainer

2020-12-22 16:05 UTC #4 lore raw

Re: [PATCH 1/6] kvm: arm64: Prevent use of invalid PSCI v0.1 function IDs

Quoted 93 lines · Mark Rutland

Quoted 47 lines · Marc Zyngier
Quoted 46 lines · David Brazdil
PSCI driver exposes a struct containing the PSCI v0.1 function IDs configured in the DT. However, the struct does not convey the information whether these were set from DT or contain the default value zero. This could be a problem for PSCI proxy in KVM protected mode.
Extend config passed to KVM with a bit mask with individual bits set depending on whether the corresponding function pointer in psci_ops is set, eg. set bit for PSCI_CPU_SUSPEND if psci_ops.cpu_suspend != NULL.
Previously config was split into multiple global variables. Put everything into a single struct for convenience.
Reported-by Mark Rutland <[email protected]>
Signed-off-by David Brazdil <[email protected]>
---
 arch/arm64/include/asm/kvm_host.h    | 20 +++++++++++
 arch/arm64/kvm/arm.c                 | 14 +++++---
 arch/arm64/kvm/hyp/nvhe/psci-relay.c | 53 +++++++++++++++++++++-------
 3 files changed, 70 insertions(+), 17 deletions(-)
arch/arm64/include/asm/kvm_host.h +10 -0
index 11beda85ee7e..828d50d40dc2 100644
@@ -17,6 +17,7 @@
 #include <linux/jump_label.h>
 #include <linux/kvm_types.h>
 #include <linux/percpu.h>
+#include <linux/psci.h>
 #include <asm/arch_gicv3.h>
 #include <asm/barrier.h>
 #include <asm/cpufeature.h>
@@ -240,6 +241,25 @@ struct kvm_host_data {
 	struct kvm_pmu_events pmu_events;
 };
 
+#define KVM_HOST_PSCI_0_1_CPU_SUSPEND	BIT(0)
+#define KVM_HOST_PSCI_0_1_CPU_ON	BIT(1)
+#define KVM_HOST_PSCI_0_1_CPU_OFF	BIT(2)
+#define KVM_HOST_PSCI_0_1_MIGRATE	BIT(3)
+
+struct kvm_host_psci_config {
+	/* PSCI version used by host. */
+	u32 version;
+
Quoted 8 lines
arch/arm64/include/asm/kvm_host.h struct kvm_host_data { · line 257
Quoted 5 lines
arch/arm64/include/asm/kvm_host.h +5 -0
+	/* Function IDs used by host if version is v0.1. */
+	struct psci_0_1_function_ids function_ids_0_1;
+
+	/* Bitmask of functions enabled for v0.1, bits KVM_HOST_PSCI_0_1_*. */
+	unsigned int enabled_functions_0_1;
Nit: the conventional type for bitmaps is 'unsigned long'. Also, "enabled" seems odd. Isn't it actually "available"?
Sure, that or "implemented" works here.
Since there are only 4 functions here, it might make sense to use independent bools rather than a bitmap, which might make this a bit simpler...
Quoted 10 lines
arch/arm64/include/asm/kvm_host.h struct kvm_host_data {
Quoted 10 lines
arch/arm64/include/asm/kvm_host.h +9 -0
 get_psci_0_1_function_ids();
+	kvm_host_psci_config.version = psci_ops.get_version();
+
+	if (kvm_host_psci_config.version == PSCI_VERSION(0, 1)) {
+		kvm_host_psci_config.function_ids_0_1 = get_psci_0_1_function_ids();
+		kvm_host_psci_config.enabled_functions_0_1 =
+			(psci_ops.cpu_suspend ? KVM_HOST_PSCI_0_1_CPU_SUSPEND : 0) |
+			(psci_ops.cpu_off ? KVM_HOST_PSCI_0_1_CPU_OFF : 0) |
+			(psci_ops.cpu_on ? KVM_HOST_PSCI_0_1_CPU_ON : 0) |
+			(psci_ops.migrate ? KVM_HOST_PSCI_0_1_MIGRATE : 0);
... since e.g. this could be roughly:
	kvm_host_psci_config.cpu_suspend_implemented = psci_ops.cpu_suspend;
	kvm_host_psci_config.cpu_off_implemented = psci_ops.cpu_off;
	kvm_host_psci_config.cpu_on_implemented = psci_ops.cpu_on;
	kvm_host_psci_config.migrate_implemented = psci_ops.migrate;
Quoted 5 lines
arch/arm64/include/asm/kvm_host.h
Quoted 5 lines
arch/arm64/include/asm/kvm_host.h +5 -0
+static inline bool is_psci_0_1_cpu_suspend(u64 func_id)
+{
+	return is_psci_0_1_function_enabled(KVM_HOST_PSCI_0_1_CPU_SUSPEND) &&
+	       (func_id == kvm_host_psci_config.function_ids_0_1.cpu_suspend);
+}
...and similarly:
	return  kvm_host_psci_config.cpu_suspend_implemented &&
		func_id == kvm_host_psci_config.function_ids_0_1.cpu_suspend)

Quoted 6 lines

Quoted 3 lines
Otherwise looks OK. Don't bother respinning the series for my comments, I can tidy things up as I apply it if there are no other issues.
FWIW, I'm happy with whatever choose to do here, so don't feel like you have to follow my suggestions above.

FWIW, I've queued the following patch on top of the series, using the above suggestions and some creative use of macros, allowing some cleanup.

M.

From 767c973f2e4a9264a4f159c9fad5ca8acdb9915e Mon Sep 17 00:00:00 2001
From: Marc Zyngier <[email protected]>
Date: Tue, 22 Dec 2020 12:46:41 +0000
Subject: [PATCH] KVM: arm64: Declutter host PSCI 0.1 handling

Although there is nothing wrong with the current host PSCI relay implementation, we can clean it up and remove some of the helpers that do not improve the overall readability of the legacy PSCI 0.1 handling.

Opportunity is taken to turn the bitmap into a set of booleans, and creative use of preprocessor macros make init and check more concise/readable.

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm64/include/asm/kvm_host.h    | 11 ++--
 arch/arm64/kvm/arm.c                 | 12 +++--
 arch/arm64/kvm/hyp/nvhe/psci-relay.c | 77 +++++++---------------------
 3 files changed, 30 insertions(+), 70 deletions(-)

arch/arm64/include/asm/kvm_host.h +4 -7

index bce2452b305c..8fcfab0c2567 100644
@@ -241,11 +241,6 @@ struct kvm_host_data {
 	struct kvm_pmu_events pmu_events;
 };
 
-#define KVM_HOST_PSCI_0_1_CPU_SUSPEND	BIT(0)
-#define KVM_HOST_PSCI_0_1_CPU_ON	BIT(1)
-#define KVM_HOST_PSCI_0_1_CPU_OFF	BIT(2)
-#define KVM_HOST_PSCI_0_1_MIGRATE	BIT(3)
-
 struct kvm_host_psci_config {
 	/* PSCI version used by host. */
 	u32 version;

@@ -253,8 +248,10 @@ struct kvm_host_psci_config {
 	/* Function IDs used by host if version is v0.1. */
 	struct psci_0_1_function_ids function_ids_0_1;
 
-	/* Bitmask of functions enabled for v0.1, bits KVM_HOST_PSCI_0_1_*. */
-	unsigned int enabled_functions_0_1;
+	bool psci_0_1_cpu_suspend_implemented;
+	bool psci_0_1_cpu_on_implemented;
+	bool psci_0_1_cpu_off_implemented;
+	bool psci_0_1_migrate_implemented;
 };
 
 extern struct kvm_host_psci_config kvm_nvhe_sym(kvm_host_psci_config);

arch/arm64/kvm/arm.c +7 -5

index 836ca763b91d..e207e4541f55 100644
@@ -1603,6 +1603,9 @@ static void init_cpu_logical_map(void)
 		hyp_cpu_logical_map[cpu] = cpu_logical_map(cpu);
 }
 
+#define init_psci_0_1_impl_state(config, what)	\
+	config.psci_0_1_ ## what ## _implemented = psci_ops.what
+
 static bool init_psci_relay(void)
 {
 	/*

@@ -1618,11 +1621,10 @@ static bool init_psci_relay(void)
 
 	if (kvm_host_psci_config.version == PSCI_VERSION(0, 1)) {
 		kvm_host_psci_config.function_ids_0_1 = get_psci_0_1_function_ids();
-		kvm_host_psci_config.enabled_functions_0_1 =
-			(psci_ops.cpu_suspend ? KVM_HOST_PSCI_0_1_CPU_SUSPEND : 0) |
-			(psci_ops.cpu_off ? KVM_HOST_PSCI_0_1_CPU_OFF : 0) |
-			(psci_ops.cpu_on ? KVM_HOST_PSCI_0_1_CPU_ON : 0) |
-			(psci_ops.migrate ? KVM_HOST_PSCI_0_1_MIGRATE : 0);
+		init_psci_0_1_impl_state(kvm_host_psci_config, cpu_suspend);
+		init_psci_0_1_impl_state(kvm_host_psci_config, cpu_on);
+		init_psci_0_1_impl_state(kvm_host_psci_config, cpu_off);
+		init_psci_0_1_impl_state(kvm_host_psci_config, migrate);
 	}
 	return true;
 }

arch/arm64/kvm/hyp/nvhe/psci-relay.c +19 ~~-58~~

117 lines not on this page · raw message

Signature · 9 lines

2.29.2


-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

David Brazdil

author

2020-12-08 14:25 UTC #5 lore raw

PATCH 2/6 kvm: arm64: Use lm_alias in nVHE-only VA conversion

In reply to David Brazdil

init_hyp_physvirt_offset() computes PA from a kernel VA. Conversion to kernel linear-map is required first but the code used kvm_ksym_ref() for this purpose. Under VHE that is a NOP and resulted in a runtime warning. Replace kvm_ksym_ref with lm_alias.

Reported-by Qian Cai <[email protected]>

Signed-off-by David Brazdil <[email protected]>

---
 arch/arm64/kvm/va_layout.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/arm64/kvm/va_layout.c +1 -1

index d8cc51bd60bf..914732b88c69 100644
@@ -42,7 +42,7 @@ static void init_hyp_physvirt_offset(void)
 	u64 kern_va, hyp_va;
 
 	/* Compute the offset from the hyp VA and PA of a random symbol. */
-	kern_va = (u64)kvm_ksym_ref(__hyp_text_start);
+	kern_va = (u64)lm_alias(__hyp_text_start);
 	hyp_va = __early_kern_hyp_va(kern_va);
 	CHOOSE_NVHE_SYM(hyp_physvirt_offset) = (s64)__pa(kern_va) - (s64)hyp_va;
 }

Signature · 6 lines

2.29.2.576.ga3fc446d84-goog

_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

David Brazdil

author

2020-12-08 14:25 UTC #6 lore raw

PATCH 3/6 kvm: arm64: Skip computing hyp VA layout for VHE

In reply to David Brazdil

Computing the hyp VA layout is redundant when the kernel runs in EL2 and hyp shares its VA mappings. Make calling kvm_compute_layout() conditional on not just CONFIG_KVM but also !is_kernel_in_hyp_mode().

Signed-off-by David Brazdil <[email protected]>

---
 arch/arm64/kernel/smp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/arm64/kernel/smp.c +1 -1

index 18e9727d3f64..4e585cc892e8 100644
@@ -434,7 +434,7 @@ static void __init hyp_mode_check(void)
 			   "CPU: CPUs started in inconsistent modes");
 	else
 		pr_info("CPU: All CPU(s) started at EL1\n");
-	if (IS_ENABLED(CONFIG_KVM))
+	if (IS_ENABLED(CONFIG_KVM) && !is_kernel_in_hyp_mode())
 		kvm_compute_layout();
 }

Signature · 6 lines

2.29.2.576.ga3fc446d84-goog

_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

David Brazdil

author

2020-12-08 14:25 UTC #7 lore raw

PATCH 4/6 kvm: arm64: Minor cleanup of hyp variables used in host

In reply to David Brazdil

Small cleanup moving declarations of hyp-exported variables to kvm_host.h and using macros to avoid having to refer to them with kvm_nvhe_sym() in host.

No functional change intended.

Signed-off-by David Brazdil <[email protected]>

---
 arch/arm64/include/asm/kvm_host.h | 6 ++++++
 arch/arm64/kvm/arm.c              | 4 +---
 arch/arm64/kvm/hyp/nvhe/hyp-smp.c | 6 +++---
 arch/arm64/kvm/va_layout.c        | 5 ++---
 4 files changed, 12 insertions(+), 9 deletions(-)

arch/arm64/include/asm/kvm_host.h +6 -0

index 828d50d40dc2..bce2452b305c 100644
@@ -260,6 +260,12 @@ struct kvm_host_psci_config {
 extern struct kvm_host_psci_config kvm_nvhe_sym(kvm_host_psci_config);
 #define kvm_host_psci_config CHOOSE_NVHE_SYM(kvm_host_psci_config)
 
+extern s64 kvm_nvhe_sym(hyp_physvirt_offset);
+#define hyp_physvirt_offset CHOOSE_NVHE_SYM(hyp_physvirt_offset)
+
+extern u64 kvm_nvhe_sym(hyp_cpu_logical_map)[NR_CPUS];
+#define hyp_cpu_logical_map CHOOSE_NVHE_SYM(hyp_cpu_logical_map)
+
 struct vcpu_reset_state {
 	unsigned long	pc;
 	unsigned long	r0;

arch/arm64/kvm/arm.c +1 -3

index 6a2f4e01b04f..836ca763b91d 100644
@@ -65,8 +65,6 @@ static bool vgic_present;
 static DEFINE_PER_CPU(unsigned char, kvm_arm_hardware_enabled);
 DEFINE_STATIC_KEY_FALSE(userspace_irqchip_in_use);
 
-extern u64 kvm_nvhe_sym(__cpu_logical_map)[NR_CPUS];
-
 int kvm_arch_vcpu_should_kick(struct kvm_vcpu *vcpu)
 {
 	return kvm_vcpu_exiting_guest_mode(vcpu) == IN_GUEST_MODE;

@@ -1602,7 +1600,7 @@ static void init_cpu_logical_map(void)
 	 * allow any other CPUs from the `possible` set to boot.
 	 */
 	for_each_online_cpu(cpu)
-		kvm_nvhe_sym(__cpu_logical_map)[cpu] = cpu_logical_map(cpu);
+		hyp_cpu_logical_map[cpu] = cpu_logical_map(cpu);
 }
 
 static bool init_psci_relay(void)

arch/arm64/kvm/hyp/nvhe/hyp-smp.c +3 -3

index cbab0c6246e2..2997aa156d8e 100644
@@ -14,14 +14,14 @@
  * Other CPUs should not be allowed to boot because their features were
  * not checked against the finalized system capabilities.
  */
-u64 __ro_after_init __cpu_logical_map[NR_CPUS] = { [0 ... NR_CPUS-1] = INVALID_HWID };
+u64 __ro_after_init hyp_cpu_logical_map[NR_CPUS] = { [0 ... NR_CPUS-1] = INVALID_HWID };
 
 u64 cpu_logical_map(unsigned int cpu)
 {
-	if (cpu >= ARRAY_SIZE(__cpu_logical_map))
+	if (cpu >= ARRAY_SIZE(hyp_cpu_logical_map))
 		hyp_panic();
 
-	return __cpu_logical_map[cpu];
+	return hyp_cpu_logical_map[cpu];
 }
 
 unsigned long __hyp_per_cpu_offset(unsigned int cpu)

arch/arm64/kvm/va_layout.c +2 -3

index 914732b88c69..70fcd6a12fe1 100644
@@ -34,17 +34,16 @@ static u64 __early_kern_hyp_va(u64 addr)
 }
 
 /*
- * Store a hyp VA <-> PA offset into a hyp-owned variable.
+ * Store a hyp VA <-> PA offset into a EL2-owned variable.
  */
 static void init_hyp_physvirt_offset(void)
 {
-	extern s64 kvm_nvhe_sym(hyp_physvirt_offset);
 	u64 kern_va, hyp_va;
 
 	/* Compute the offset from the hyp VA and PA of a random symbol. */
 	kern_va = (u64)lm_alias(__hyp_text_start);
 	hyp_va = __early_kern_hyp_va(kern_va);
-	CHOOSE_NVHE_SYM(hyp_physvirt_offset) = (s64)__pa(kern_va) - (s64)hyp_va;
+	hyp_physvirt_offset = (s64)__pa(kern_va) - (s64)hyp_va;
 }
 
 /*

Signature · 6 lines

2.29.2.576.ga3fc446d84-goog

_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

David Brazdil

author

2020-12-08 14:25 UTC #8 lore raw

PATCH 5/6 kvm: arm64: Remove unused includes in psci-relay.c

In reply to David Brazdil

Minor cleanup removing unused includes.

Signed-off-by David Brazdil <[email protected]>

---
 arch/arm64/kvm/hyp/nvhe/psci-relay.c | 3 ---
 1 file changed, 3 deletions(-)

arch/arm64/kvm/hyp/nvhe/psci-relay.c +0 -3

index 0d6f4aa39621..1f7237e45148 100644
@@ -7,11 +7,8 @@
 #include <asm/kvm_asm.h>
 #include <asm/kvm_hyp.h>
 #include <asm/kvm_mmu.h>
-#include <kvm/arm_hypercalls.h>
 #include <linux/arm-smccc.h>
 #include <linux/kvm_host.h>
-#include <linux/psci.h>
-#include <kvm/arm_psci.h>
 #include <uapi/linux/psci.h>
 
 #include <nvhe/trap_handler.h>

Signature · 6 lines

2.29.2.576.ga3fc446d84-goog

_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

David Brazdil

author

2020-12-08 14:25 UTC #9 lore raw

PATCH 6/6 kvm: arm64: Move skip_host_instruction to adjust_pc.h

In reply to David Brazdil

Move function for skipping host instruction in the host trap handler to a header file containing analogical helpers for guests.

Signed-off-by David Brazdil <[email protected]>

---
 arch/arm64/kvm/hyp/include/hyp/adjust_pc.h |  9 +++++++++
 arch/arm64/kvm/hyp/nvhe/hyp-main.c         | 12 ++----------
 2 files changed, 11 insertions(+), 10 deletions(-)

arch/arm64/kvm/hyp/include/hyp/adjust_pc.h +9 -0

index b1f60923a8fe..61716359035d 100644
@@ -59,4 +59,13 @@ static inline void __adjust_pc(struct kvm_vcpu *vcpu)
 	}
 }
 
+/*
+ * Skip an instruction while host sysregs are live.
+ * Assumes host is always 64-bit.
+ */
+static inline void kvm_skip_host_instr(void)
+{
+	write_sysreg_el2(read_sysreg_el2(SYS_ELR) + 4, SYS_ELR);
+}
+
 #endif

arch/arm64/kvm/hyp/nvhe/hyp-main.c +2 ~~-10~~

index bde658d51404..a906f9e2ff34 100644
@@ -157,11 +157,6 @@ static void default_host_smc_handler(struct kvm_cpu_context *host_ctxt)
 	__kvm_hyp_host_forward_smc(host_ctxt);
 }
 
-static void skip_host_instruction(void)
-{
-	write_sysreg_el2(read_sysreg_el2(SYS_ELR) + 4, SYS_ELR);
-}
-
 static void handle_host_smc(struct kvm_cpu_context *host_ctxt)
 {
 	bool handled;

@@ -170,11 +165,8 @@ static void handle_host_smc(struct kvm_cpu_context *host_ctxt)
 	if (!handled)
 		default_host_smc_handler(host_ctxt);
 
-	/*
-	 * Unlike HVC, the return address of an SMC is the instruction's PC.
-	 * Move the return address past the instruction.
-	 */
-	skip_host_instruction();
+	/* SMC was trapped, move ELR past the current PC. */
+	kvm_skip_host_instr();
 }
 
 void handle_trap(struct kvm_cpu_context *host_ctxt)

Signature · 6 lines

2.29.2.576.ga3fc446d84-goog

_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Marc Zyngier

maintainer Applied

2020-12-22 16:06 UTC #10 lore raw

In reply to David Brazdil

Quoted 9 lines · David Brazdil

Small batch of improvements for the 'Opt-in always-on nVHE hypervisor' series, now merged in kvmarm/next.
Patch #1 fixes potential use of invalid v0.1 functions IDs reported by Mark Rutland, patch #2 fixes a warning reported by Qian Cai. Patch #3 avoids a code path not used in VHE, can be dropped if any concerns arise. The remaining patches are minor cleanups from review.
[...]

Applied to next, thanks!

[1/6] kvm: arm64: Prevent use of invalid PSCI v0.1 function IDs

      commit: ff367fe473a9857160c17827931375a899076394
[2/6] kvm: arm64: Use lm_alias in nVHE-only VA conversion
      commit: 7a96a0687b80a1870c689418d7b72012c8bdd53d
[3/6] kvm: arm64: Skip computing hyp VA layout for VHE
      commit: c3e181aec96f6ada84df1cb72a72be8970f8b284
[4/6] kvm: arm64: Minor cleanup of hyp variables used in host
      commit: 61fe0c37af57ac35472a870581a7d0bb5ac2f63a
[5/6] kvm: arm64: Remove unused includes in psci-relay.c
      commit: e6829e0384a49efe68537298132230bebd8bd1b3
[6/6] kvm: arm64: Move skip_host_instruction to adjust_pc.h
      commit: 860a4c3d1e04a3c3e62bacbbba64417bf49768e2

Cheers,

M.

Signature · 7 lines

Without deviation from the norm, progress is not possible.


_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm