KVM: arm64: Add support for hypercall services selection

Common hypercall firmware register handing is currently employed by psci.c. Since the upcoming patches add more of these registers, it's better to move the generic handling to hypercall.c for a cleaner presentation.

While we are at it, collect all the firmware registers under fw_reg_ids[] to help implement kvm_arm_get_fw_num_regs() and kvm_arm_copy_fw_reg_indices() in a generic way.

No functional change intended.

Signed-off-by Raghavendra Rao Ananta <[email protected]>

---
 arch/arm64/kvm/guest.c       |   2 +-
 arch/arm64/kvm/hypercalls.c  | 151 +++++++++++++++++++++++++++++++
 arch/arm64/kvm/psci.c        | 167 +++--------------------------------
 include/kvm/arm_hypercalls.h |   7 ++
 include/kvm/arm_psci.h       |   8 +-
 5 files changed, 172 insertions(+), 163 deletions(-)

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

index 5ce26bedf23c..625f97f7b304 100644
@@ -18,7 +18,7 @@
 #include <linux/string.h>
 #include <linux/vmalloc.h>
 #include <linux/fs.h>
-#include <kvm/arm_psci.h>
+#include <kvm/arm_hypercalls.h>
 #include <asm/cputype.h>
 #include <linux/uaccess.h>
 #include <asm/fpsimd.h>

arch/arm64/kvm/hypercalls.c +13 -0

index 30da78f72b3b..d030939c5929 100644
@@ -146,3 +146,154 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
 	smccc_set_retval(vcpu, val[0], val[1], val[2], val[3]);
 	return 1;
 }
+
+static const u64 fw_reg_ids[] = {
+	KVM_REG_ARM_PSCI_VERSION,
+	KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1,
+	KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2,
+};
+
+int kvm_arm_get_fw_num_regs(struct kvm_vcpu *vcpu)
+{
+	return ARRAY_SIZE(fw_reg_ids);
+}
+
+int kvm_arm_copy_fw_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)

arch/arm64/kvm/hypercalls.c +39 -0

+#define KVM_REG_FEATURE_LEVEL_WIDTH	4
+#define KVM_REG_FEATURE_LEVEL_MASK	(BIT(KVM_REG_FEATURE_LEVEL_WIDTH) - 1)
+
+/*
+ * Convert the workaround level into an easy-to-compare number, where higher
+ * values mean better protection.
+ */
+static int get_kernel_wa_level(u64 regid)
+{
+	switch (regid) {
+	case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1:
+		switch (arm64_get_spectre_v2_state()) {
+		case SPECTRE_VULNERABLE:
+			return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_NOT_AVAIL;
+		case SPECTRE_MITIGATED:
+			return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_AVAIL;
+		case SPECTRE_UNAFFECTED:
+			return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_NOT_REQUIRED;
+		}
+		return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_NOT_AVAIL;
+	case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2:
+		switch (arm64_get_spectre_v4_state()) {
+		case SPECTRE_MITIGATED:
+			/*
+			 * As for the hypercall discovery, we pretend we
+			 * don't have any FW mitigation if SSBS is there at
+			 * all times.
+			 */
+			if (cpus_have_final_cap(ARM64_SSBS))
+				return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_AVAIL;
+			fallthrough;
+		case SPECTRE_UNAFFECTED:
+			return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_REQUIRED;
+		case SPECTRE_VULNERABLE:
+			return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_AVAIL;
+		}
+	}
+
+	return -EINVAL;

arch/arm64/kvm/hypercalls.c +78 -0

+		break;
+	case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1:
+	case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2:
+		val = get_kernel_wa_level(reg->id) & KVM_REG_FEATURE_LEVEL_MASK;
+		break;
+	default:
+		return -ENOENT;
+	}
+
+	if (copy_to_user(uaddr, &val, KVM_REG_SIZE(reg->id)))
+		return -EFAULT;
+
+	return 0;
+}
+
+int kvm_arm_set_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
+{
+	void __user *uaddr = (void __user *)(long)reg->addr;
+	u64 val;
+	int wa_level;
+
+	if (copy_from_user(&val, uaddr, KVM_REG_SIZE(reg->id)))
+		return -EFAULT;
+
+	switch (reg->id) {
+	case KVM_REG_ARM_PSCI_VERSION:
+		return kvm_arm_set_psci_fw_reg(vcpu, val);
+
+	case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1:
+		if (val & ~KVM_REG_FEATURE_LEVEL_MASK)
+			return -EINVAL;
+
+		if (get_kernel_wa_level(reg->id) < val)
+			return -EINVAL;
+
+		return 0;
+
+	case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2:
+		if (val & ~(KVM_REG_FEATURE_LEVEL_MASK |
+			    KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_ENABLED))
+			return -EINVAL;
+
+		/* The enabled bit must not be set unless the level is AVAIL. */
+		if ((val & KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_ENABLED) &&
+		    (val & KVM_REG_FEATURE_LEVEL_MASK) != KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_AVAIL)
+			return -EINVAL;
+
+		/*
+		 * Map all the possible incoming states to the only two we
+		 * really want to deal with.
+		 */
+		switch (val & KVM_REG_FEATURE_LEVEL_MASK) {
+		case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_AVAIL:
+		case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNKNOWN:
+			wa_level = KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_AVAIL;
+			break;
+		case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_AVAIL:
+		case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_REQUIRED:
+			wa_level = KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_REQUIRED;
+			break;
+		default:
+			return -EINVAL;
+		}
+
+		/*
+		 * We can deal with NOT_AVAIL on NOT_REQUIRED, but not the
+		 * other way around.
+		 */
+		if (get_kernel_wa_level(reg->id) < wa_level)
+			return -EINVAL;
+
+		return 0;
+	default:
+		return -ENOENT;
+	}
+
+	return -EINVAL;
+}

arch/arm64/kvm/psci.c +12 -155

index 74c47d420253..b9bcbc919b19 100644
@@ -404,168 +404,25 @@ int kvm_psci_call(struct kvm_vcpu *vcpu)
 	};
 }
 
-int kvm_arm_get_fw_num_regs(struct kvm_vcpu *vcpu)
+int kvm_arm_set_psci_fw_reg(struct kvm_vcpu *vcpu, u64 val)
 {
-	return 3;		/* PSCI version and two workaround registers */
-}
-
-int kvm_arm_copy_fw_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
-{
-	if (put_user(KVM_REG_ARM_PSCI_VERSION, uindices++))
-		return -EFAULT;
-
-	if (put_user(KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1, uindices++))
-		return -EFAULT;
-
-	if (put_user(KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2, uindices++))
-		return -EFAULT;
-
-	return 0;
-}
+	bool wants_02;
 
-#define KVM_REG_FEATURE_LEVEL_WIDTH	4
-#define KVM_REG_FEATURE_LEVEL_MASK	(BIT(KVM_REG_FEATURE_LEVEL_WIDTH) - 1)
-
-/*
- * Convert the workaround level into an easy-to-compare number, where higher
- * values mean better protection.
- */
-static int get_kernel_wa_level(u64 regid)
-{
-	switch (regid) {
-	case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1:
-		switch (arm64_get_spectre_v2_state()) {
-		case SPECTRE_VULNERABLE:
-			return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_NOT_AVAIL;
-		case SPECTRE_MITIGATED:
-			return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_AVAIL;
-		case SPECTRE_UNAFFECTED:
-			return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_NOT_REQUIRED;
-		}
-		return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_NOT_AVAIL;
-	case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2:
-		switch (arm64_get_spectre_v4_state()) {
-		case SPECTRE_MITIGATED:
-			/*
-			 * As for the hypercall discovery, we pretend we
-			 * don't have any FW mitigation if SSBS is there at
-			 * all times.
-			 */
-			if (cpus_have_final_cap(ARM64_SSBS))
-				return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_AVAIL;
-			fallthrough;
-		case SPECTRE_UNAFFECTED:
-			return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_REQUIRED;
-		case SPECTRE_VULNERABLE:
-			return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_AVAIL;
-		}
-	}
+	wants_02 = test_bit(KVM_ARM_VCPU_PSCI_0_2, vcpu->arch.features);
 
-	return -EINVAL;
-}
-
-int kvm_arm_get_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
-{
-	void __user *uaddr = (void __user *)(long)reg->addr;
-	u64 val;
-
-	switch (reg->id) {
-	case KVM_REG_ARM_PSCI_VERSION:
-		val = kvm_psci_version(vcpu, vcpu->kvm);
-		break;
-	case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1:
-	case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2:
-		val = get_kernel_wa_level(reg->id) & KVM_REG_FEATURE_LEVEL_MASK;
-		break;
-	default:
-		return -ENOENT;
-	}
-
-	if (copy_to_user(uaddr, &val, KVM_REG_SIZE(reg->id)))
-		return -EFAULT;
-
-	return 0;
-}
-
-int kvm_arm_set_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
-{
-	void __user *uaddr = (void __user *)(long)reg->addr;
-	u64 val;
-	int wa_level;
-
-	if (copy_from_user(&val, uaddr, KVM_REG_SIZE(reg->id)))
-		return -EFAULT;
-
-	switch (reg->id) {
-	case KVM_REG_ARM_PSCI_VERSION:
-	{
-		bool wants_02;
-
-		wants_02 = test_bit(KVM_ARM_VCPU_PSCI_0_2, vcpu->arch.features);
-
-		switch (val) {
-		case KVM_ARM_PSCI_0_1:
-			if (wants_02)
-				return -EINVAL;
-			vcpu->kvm->arch.psci_version = val;
-			return 0;
-		case KVM_ARM_PSCI_0_2:
-		case KVM_ARM_PSCI_1_0:
-			if (!wants_02)
-				return -EINVAL;
-			vcpu->kvm->arch.psci_version = val;
-			return 0;
-		}
-		break;
-	}
-
-	case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1:
-		if (val & ~KVM_REG_FEATURE_LEVEL_MASK)
-			return -EINVAL;
-
-		if (get_kernel_wa_level(reg->id) < val)
+	switch (val) {
+	case KVM_ARM_PSCI_0_1:
+		if (wants_02)
 			return -EINVAL;
-
+		vcpu->kvm->arch.psci_version = val;
 		return 0;
-
-	case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2:
-		if (val & ~(KVM_REG_FEATURE_LEVEL_MASK |
-			    KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_ENABLED))
-			return -EINVAL;
-
-		/* The enabled bit must not be set unless the level is AVAIL. */
-		if ((val & KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_ENABLED) &&
-		    (val & KVM_REG_FEATURE_LEVEL_MASK) != KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_AVAIL)
-			return -EINVAL;
-
-		/*
-		 * Map all the possible incoming states to the only two we
-		 * really want to deal with.
-		 */
-		switch (val & KVM_REG_FEATURE_LEVEL_MASK) {
-		case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_AVAIL:
-		case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNKNOWN:
-			wa_level = KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_AVAIL;
-			break;
-		case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_AVAIL:
-		case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_REQUIRED:
-			wa_level = KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_REQUIRED;
-			break;
-		default:
-			return -EINVAL;
-		}
-
-		/*
-		 * We can deal with NOT_AVAIL on NOT_REQUIRED, but not the
-		 * other way around.
-		 */
-		if (get_kernel_wa_level(reg->id) < wa_level)
+	case KVM_ARM_PSCI_0_2:
+	case KVM_ARM_PSCI_1_0:
+		if (!wants_02)
 			return -EINVAL;
-
+		vcpu->kvm->arch.psci_version = val;
 		return 0;
 	default:
-		return -ENOENT;
+		return -EINVAL;
 	}
-
-	return -EINVAL;
 }

include/kvm/arm_hypercalls.h +7 -0

index 0e2509d27910..5d38628a8d04 100644
@@ -40,4 +40,11 @@ static inline void smccc_set_retval(struct kvm_vcpu *vcpu,
 	vcpu_set_reg(vcpu, 3, a3);
 }
 
+struct kvm_one_reg;
+
+int kvm_arm_get_fw_num_regs(struct kvm_vcpu *vcpu);
+int kvm_arm_copy_fw_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices);
+int kvm_arm_get_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg);
+int kvm_arm_set_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg);
+
 #endif

include/kvm/arm_psci.h +1 -7

index 5b58bd2fe088..eddbd7d805e9 100644
@@ -41,12 +41,6 @@ static inline int kvm_psci_version(struct kvm_vcpu *vcpu, struct kvm *kvm)
 
 
 int kvm_psci_call(struct kvm_vcpu *vcpu);
-
-struct kvm_one_reg;
-
-int kvm_arm_get_fw_num_regs(struct kvm_vcpu *vcpu);
-int kvm_arm_copy_fw_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices);
-int kvm_arm_get_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg);
-int kvm_arm_set_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg);
+int kvm_arm_set_psci_fw_reg(struct kvm_vcpu *vcpu, u64 val);
 
 #endif /* __KVM_ARM_PSCI_H__ */

Signature · 1 line

2.33.1.1089.g2158813163f-goog

Hi Raghu,

Quoted 54 lines · Raghavendra Rao Ananta

Common hypercall firmware register handing is currently employed by psci.c. Since the upcoming patches add more of these registers, it's better to move the generic handling to hypercall.c for a cleaner presentation.

While we are at it, collect all the firmware registers under fw_reg_ids[] to help implement kvm_arm_get_fw_num_regs() and kvm_arm_copy_fw_reg_indices() in a generic way.

No functional change intended.

Signed-off-by Raghavendra Rao Ananta <[email protected]>

---
 arch/arm64/kvm/guest.c       |   2 +-
 arch/arm64/kvm/hypercalls.c  | 151 +++++++++++++++++++++++++++++++
 arch/arm64/kvm/psci.c        | 167 +++--------------------------------
 include/kvm/arm_hypercalls.h |   7 ++
 include/kvm/arm_psci.h       |   8 +-
 5 files changed, 172 insertions(+), 163 deletions(-)

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

index 5ce26bedf23c..625f97f7b304 100644
@@ -18,7 +18,7 @@
 #include <linux/string.h>
 #include <linux/vmalloc.h>
 #include <linux/fs.h>
-#include <kvm/arm_psci.h>
+#include <kvm/arm_hypercalls.h>
 #include <asm/cputype.h>
 #include <linux/uaccess.h>
 #include <asm/fpsimd.h>

arch/arm64/kvm/hypercalls.c +13 -0

index 30da78f72b3b..d030939c5929 100644
@@ -146,3 +146,154 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
      smccc_set_retval(vcpu, val[0], val[1], val[2], val[3]);
      return 1;
 }
+
+static const u64 fw_reg_ids[] = {
+     KVM_REG_ARM_PSCI_VERSION,
+     KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1,
+     KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2,
+};
+
+int kvm_arm_get_fw_num_regs(struct kvm_vcpu *vcpu)
+{
+     return ARRAY_SIZE(fw_reg_ids);
+}
+
+int kvm_arm_copy_fw_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)

arch/arm64/kvm/hypercalls.c int kvm_hvc_call_handler(struct kvm_vcpu *vcpu) · line 171

Quoted 10 lines

arch/arm64/kvm/hypercalls.c +10 -0

+{
+     int i;
+
+     for (i = 0; i < ARRAY_SIZE(fw_reg_ids); i++) {
+             if (put_user(fw_reg_ids[i], uindices))
+                     return -EFAULT;
+     }
+
+     return 0;
+}

It would appear that this patch is separating out the hypercall services to each handle their own FW regs. At the same time, this is consolidating the register enumeration into a single place.

It would be nice to keep the scoping consistent with your accessors below, or simply just handle all regs in hypercalls.c. Abstracting per-service might result in a lot of boilerplate, though.

Quoted 44 lines

arch/arm64/kvm/hypercalls.c +44 -0

+#define KVM_REG_FEATURE_LEVEL_WIDTH  4
+#define KVM_REG_FEATURE_LEVEL_MASK   (BIT(KVM_REG_FEATURE_LEVEL_WIDTH) - 1)
+
+/*
+ * Convert the workaround level into an easy-to-compare number, where higher
+ * values mean better protection.
+ */
+static int get_kernel_wa_level(u64 regid)
+{
+     switch (regid) {
+     case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1:
+             switch (arm64_get_spectre_v2_state()) {
+             case SPECTRE_VULNERABLE:
+                     return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_NOT_AVAIL;
+             case SPECTRE_MITIGATED:
+                     return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_AVAIL;
+             case SPECTRE_UNAFFECTED:
+                     return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_NOT_REQUIRED;
+             }
+             return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_NOT_AVAIL;
+     case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2:
+             switch (arm64_get_spectre_v4_state()) {
+             case SPECTRE_MITIGATED:
+                     /*
+                      * As for the hypercall discovery, we pretend we
+                      * don't have any FW mitigation if SSBS is there at
+                      * all times.
+                      */
+                     if (cpus_have_final_cap(ARM64_SSBS))
+                             return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_AVAIL;
+                     fallthrough;
+             case SPECTRE_UNAFFECTED:
+                     return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_REQUIRED;
+             case SPECTRE_VULNERABLE:
+                     return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_AVAIL;
+             }
+     }
+
+     return -EINVAL;
+}
+
+int kvm_arm_get_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
+{
+     void __user *uaddr = (void __user *)(long)reg->addr;

arch/arm64/kvm/hypercalls.c int kvm_hvc_call_handler(struct kvm_vcpu *vcpu) · line 221

Quoted 5 lines

arch/arm64/kvm/hypercalls.c +5 -0

+     u64 val;
+
+     switch (reg->id) {
+     case KVM_REG_ARM_PSCI_VERSION:
+             val = kvm_psci_version(vcpu, vcpu->kvm);

Should this become kvm_arm_get_fw_reg() to consistently genericize the PSCI FW register accessors?

Quoted 300 lines

arch/arm64/kvm/hypercalls.c +78 -0

+             break;
+     case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1:
+     case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2:
+             val = get_kernel_wa_level(reg->id) & KVM_REG_FEATURE_LEVEL_MASK;
+             break;
+     default:
+             return -ENOENT;
+     }
+
+     if (copy_to_user(uaddr, &val, KVM_REG_SIZE(reg->id)))
+             return -EFAULT;
+
+     return 0;
+}
+
+int kvm_arm_set_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
+{
+     void __user *uaddr = (void __user *)(long)reg->addr;
+     u64 val;
+     int wa_level;
+
+     if (copy_from_user(&val, uaddr, KVM_REG_SIZE(reg->id)))
+             return -EFAULT;
+
+     switch (reg->id) {
+     case KVM_REG_ARM_PSCI_VERSION:
+             return kvm_arm_set_psci_fw_reg(vcpu, val);
+
+     case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1:
+             if (val & ~KVM_REG_FEATURE_LEVEL_MASK)
+                     return -EINVAL;
+
+             if (get_kernel_wa_level(reg->id) < val)
+                     return -EINVAL;
+
+             return 0;
+
+     case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2:
+             if (val & ~(KVM_REG_FEATURE_LEVEL_MASK |
+                         KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_ENABLED))
+                     return -EINVAL;
+
+             /* The enabled bit must not be set unless the level is AVAIL. */
+             if ((val & KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_ENABLED) &&
+                 (val & KVM_REG_FEATURE_LEVEL_MASK) != KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_AVAIL)
+                     return -EINVAL;
+
+             /*
+              * Map all the possible incoming states to the only two we
+              * really want to deal with.
+              */
+             switch (val & KVM_REG_FEATURE_LEVEL_MASK) {
+             case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_AVAIL:
+             case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNKNOWN:
+                     wa_level = KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_AVAIL;
+                     break;
+             case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_AVAIL:
+             case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_REQUIRED:
+                     wa_level = KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_REQUIRED;
+                     break;
+             default:
+                     return -EINVAL;
+             }
+
+             /*
+              * We can deal with NOT_AVAIL on NOT_REQUIRED, but not the
+              * other way around.
+              */
+             if (get_kernel_wa_level(reg->id) < wa_level)
+                     return -EINVAL;
+
+             return 0;
+     default:
+             return -ENOENT;
+     }
+
+     return -EINVAL;
+}

arch/arm64/kvm/psci.c +12 -155

index 74c47d420253..b9bcbc919b19 100644
@@ -404,168 +404,25 @@ int kvm_psci_call(struct kvm_vcpu *vcpu)
      };
 }
 
-int kvm_arm_get_fw_num_regs(struct kvm_vcpu *vcpu)
+int kvm_arm_set_psci_fw_reg(struct kvm_vcpu *vcpu, u64 val)
 {
-     return 3;               /* PSCI version and two workaround registers */
-}
-
-int kvm_arm_copy_fw_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
-{
-     if (put_user(KVM_REG_ARM_PSCI_VERSION, uindices++))
-             return -EFAULT;
-
-     if (put_user(KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1, uindices++))
-             return -EFAULT;
-
-     if (put_user(KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2, uindices++))
-             return -EFAULT;
-
-     return 0;
-}
+     bool wants_02;
 
-#define KVM_REG_FEATURE_LEVEL_WIDTH  4
-#define KVM_REG_FEATURE_LEVEL_MASK   (BIT(KVM_REG_FEATURE_LEVEL_WIDTH) - 1)
-
-/*
- * Convert the workaround level into an easy-to-compare number, where higher
- * values mean better protection.
- */
-static int get_kernel_wa_level(u64 regid)
-{
-     switch (regid) {
-     case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1:
-             switch (arm64_get_spectre_v2_state()) {
-             case SPECTRE_VULNERABLE:
-                     return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_NOT_AVAIL;
-             case SPECTRE_MITIGATED:
-                     return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_AVAIL;
-             case SPECTRE_UNAFFECTED:
-                     return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_NOT_REQUIRED;
-             }
-             return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_NOT_AVAIL;
-     case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2:
-             switch (arm64_get_spectre_v4_state()) {
-             case SPECTRE_MITIGATED:
-                     /*
-                      * As for the hypercall discovery, we pretend we
-                      * don't have any FW mitigation if SSBS is there at
-                      * all times.
-                      */
-                     if (cpus_have_final_cap(ARM64_SSBS))
-                             return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_AVAIL;
-                     fallthrough;
-             case SPECTRE_UNAFFECTED:
-                     return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_REQUIRED;
-             case SPECTRE_VULNERABLE:
-                     return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_AVAIL;
-             }
-     }
+     wants_02 = test_bit(KVM_ARM_VCPU_PSCI_0_2, vcpu->arch.features);
 
-     return -EINVAL;
-}
-
-int kvm_arm_get_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
-{
-     void __user *uaddr = (void __user *)(long)reg->addr;
-     u64 val;
-
-     switch (reg->id) {
-     case KVM_REG_ARM_PSCI_VERSION:
-             val = kvm_psci_version(vcpu, vcpu->kvm);
-             break;
-     case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1:
-     case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2:
-             val = get_kernel_wa_level(reg->id) & KVM_REG_FEATURE_LEVEL_MASK;
-             break;
-     default:
-             return -ENOENT;
-     }
-
-     if (copy_to_user(uaddr, &val, KVM_REG_SIZE(reg->id)))
-             return -EFAULT;
-
-     return 0;
-}
-
-int kvm_arm_set_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
-{
-     void __user *uaddr = (void __user *)(long)reg->addr;
-     u64 val;
-     int wa_level;
-
-     if (copy_from_user(&val, uaddr, KVM_REG_SIZE(reg->id)))
-             return -EFAULT;
-
-     switch (reg->id) {
-     case KVM_REG_ARM_PSCI_VERSION:
-     {
-             bool wants_02;
-
-             wants_02 = test_bit(KVM_ARM_VCPU_PSCI_0_2, vcpu->arch.features);
-
-             switch (val) {
-             case KVM_ARM_PSCI_0_1:
-                     if (wants_02)
-                             return -EINVAL;
-                     vcpu->kvm->arch.psci_version = val;
-                     return 0;
-             case KVM_ARM_PSCI_0_2:
-             case KVM_ARM_PSCI_1_0:
-                     if (!wants_02)
-                             return -EINVAL;
-                     vcpu->kvm->arch.psci_version = val;
-                     return 0;
-             }
-             break;
-     }
-
-     case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1:
-             if (val & ~KVM_REG_FEATURE_LEVEL_MASK)
-                     return -EINVAL;
-
-             if (get_kernel_wa_level(reg->id) < val)
+     switch (val) {
+     case KVM_ARM_PSCI_0_1:
+             if (wants_02)
                      return -EINVAL;
-
+             vcpu->kvm->arch.psci_version = val;
              return 0;
-
-     case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2:
-             if (val & ~(KVM_REG_FEATURE_LEVEL_MASK |
-                         KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_ENABLED))
-                     return -EINVAL;
-
-             /* The enabled bit must not be set unless the level is AVAIL. */
-             if ((val & KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_ENABLED) &&
-                 (val & KVM_REG_FEATURE_LEVEL_MASK) != KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_AVAIL)
-                     return -EINVAL;
-
-             /*
-              * Map all the possible incoming states to the only two we
-              * really want to deal with.
-              */
-             switch (val & KVM_REG_FEATURE_LEVEL_MASK) {
-             case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_AVAIL:
-             case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNKNOWN:
-                     wa_level = KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_AVAIL;
-                     break;
-             case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_AVAIL:
-             case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_REQUIRED:
-                     wa_level = KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_REQUIRED;
-                     break;
-             default:
-                     return -EINVAL;
-             }
-
-             /*
-              * We can deal with NOT_AVAIL on NOT_REQUIRED, but not the
-              * other way around.
-              */
-             if (get_kernel_wa_level(reg->id) < wa_level)
+     case KVM_ARM_PSCI_0_2:
+     case KVM_ARM_PSCI_1_0:
+             if (!wants_02)
                      return -EINVAL;
-
+             vcpu->kvm->arch.psci_version = val;
              return 0;
      default:
-             return -ENOENT;
+             return -EINVAL;
      }
-
-     return -EINVAL;
 }

include/kvm/arm_hypercalls.h +7 -0

index 0e2509d27910..5d38628a8d04 100644
@@ -40,4 +40,11 @@ static inline void smccc_set_retval(struct kvm_vcpu *vcpu,
      vcpu_set_reg(vcpu, 3, a3);
 }
 
+struct kvm_one_reg;
+
+int kvm_arm_get_fw_num_regs(struct kvm_vcpu *vcpu);
+int kvm_arm_copy_fw_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices);
+int kvm_arm_get_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg);
+int kvm_arm_set_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg);
+
 #endif

include/kvm/arm_psci.h +1 -8

index 5b58bd2fe088..eddbd7d805e9 100644
@@ -41,12 +41,6 @@ static inline int kvm_psci_version(struct kvm_vcpu *vcpu, struct kvm *kvm)
 
 
 int kvm_psci_call(struct kvm_vcpu *vcpu);
-
-struct kvm_one_reg;
-
-int kvm_arm_get_fw_num_regs(struct kvm_vcpu *vcpu);
-int kvm_arm_copy_fw_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices);
-int kvm_arm_get_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg);
-int kvm_arm_set_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg);
+int kvm_arm_set_psci_fw_reg(struct kvm_vcpu *vcpu, u64 val);
 
 #endif /* __KVM_ARM_PSCI_H__ */
--

2.33.1.1089.g2158813163f-goog

Hi Oliver,

Quoted 73 lines · Oliver Upton

Hi Raghu,

Quoted 54 lines · Raghavendra Rao Ananta

Common hypercall firmware register handing is currently employed by psci.c. Since the upcoming patches add more of these registers, it's better to move the generic handling to hypercall.c for a cleaner presentation.

While we are at it, collect all the firmware registers under fw_reg_ids[] to help implement kvm_arm_get_fw_num_regs() and kvm_arm_copy_fw_reg_indices() in a generic way.

No functional change intended.

Signed-off-by Raghavendra Rao Ananta <[email protected]>

---
 arch/arm64/kvm/guest.c       |   2 +-
 arch/arm64/kvm/hypercalls.c  | 151 +++++++++++++++++++++++++++++++
 arch/arm64/kvm/psci.c        | 167 +++--------------------------------
 include/kvm/arm_hypercalls.h |   7 ++
 include/kvm/arm_psci.h       |   8 +-
 5 files changed, 172 insertions(+), 163 deletions(-)

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

index 5ce26bedf23c..625f97f7b304 100644
@@ -18,7 +18,7 @@
 #include <linux/string.h>
 #include <linux/vmalloc.h>
 #include <linux/fs.h>
-#include <kvm/arm_psci.h>
+#include <kvm/arm_hypercalls.h>
 #include <asm/cputype.h>
 #include <linux/uaccess.h>
 #include <asm/fpsimd.h>

arch/arm64/kvm/hypercalls.c +13 -0

index 30da78f72b3b..d030939c5929 100644
@@ -146,3 +146,154 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
      smccc_set_retval(vcpu, val[0], val[1], val[2], val[3]);
      return 1;
 }
+
+static const u64 fw_reg_ids[] = {
+     KVM_REG_ARM_PSCI_VERSION,
+     KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1,
+     KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2,
+};
+
+int kvm_arm_get_fw_num_regs(struct kvm_vcpu *vcpu)
+{
+     return ARRAY_SIZE(fw_reg_ids);
+}
+
+int kvm_arm_copy_fw_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)

arch/arm64/kvm/hypercalls.c int kvm_hvc_call_handler(struct kvm_vcpu *vcpu) · line 171

Quoted 10 lines

arch/arm64/kvm/hypercalls.c +10 -0

+{
+     int i;
+
+     for (i = 0; i < ARRAY_SIZE(fw_reg_ids); i++) {
+             if (put_user(fw_reg_ids[i], uindices))
+                     return -EFAULT;
+     }
+
+     return 0;
+}

It would appear that this patch is separating out the hypercall services to each handle their own FW regs. At the same time, this is consolidating the register enumeration into a single place.

Quoted 4 lines

It would be nice to keep the scoping consistent with your accessors below, or simply just handle all regs in hypercalls.c. Abstracting per-service might result in a lot of boilerplate, though.

It's neither here nor there, unfortunately, because of how the fw registers exists. We have a dedicated fw register for psci and a file of its own (psci.c). Some of the other services, such as TRNG, have their own file, but because of the bitmap design, they won't have their own fw register. And the ARCH_WORKAROUND have their dedicated registers, but no file of their own. So, at best I was aiming to push all the things relevant to a service in its own file (psci for example), just to have a better file-context, while leaving others (and generic handling stuff) in hypercall.c.

Just to maintain consistency, I can create a dedicated file for the ARCH_WORKAROUND registers, if you feel that's better.

Quoted 44 lines

Quoted 44 lines

arch/arm64/kvm/hypercalls.c +44 -0

+#define KVM_REG_FEATURE_LEVEL_WIDTH  4
+#define KVM_REG_FEATURE_LEVEL_MASK   (BIT(KVM_REG_FEATURE_LEVEL_WIDTH) - 1)
+
+/*
+ * Convert the workaround level into an easy-to-compare number, where higher
+ * values mean better protection.
+ */
+static int get_kernel_wa_level(u64 regid)
+{
+     switch (regid) {
+     case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1:
+             switch (arm64_get_spectre_v2_state()) {
+             case SPECTRE_VULNERABLE:
+                     return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_NOT_AVAIL;
+             case SPECTRE_MITIGATED:
+                     return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_AVAIL;
+             case SPECTRE_UNAFFECTED:
+                     return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_NOT_REQUIRED;
+             }
+             return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_NOT_AVAIL;
+     case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2:
+             switch (arm64_get_spectre_v4_state()) {
+             case SPECTRE_MITIGATED:
+                     /*
+                      * As for the hypercall discovery, we pretend we
+                      * don't have any FW mitigation if SSBS is there at
+                      * all times.
+                      */
+                     if (cpus_have_final_cap(ARM64_SSBS))
+                             return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_AVAIL;
+                     fallthrough;
+             case SPECTRE_UNAFFECTED:
+                     return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_REQUIRED;
+             case SPECTRE_VULNERABLE:
+                     return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_AVAIL;
+             }
+     }
+
+     return -EINVAL;
+}
+
+int kvm_arm_get_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
+{
+     void __user *uaddr = (void __user *)(long)reg->addr;

Quoted 9 lines

arch/arm64/kvm/hypercalls.c int kvm_hvc_call_handler(struct kvm_vcpu *vcpu) · line 221

Quoted 5 lines

arch/arm64/kvm/hypercalls.c +5 -0

+     u64 val;
+
+     switch (reg->id) {
+     case KVM_REG_ARM_PSCI_VERSION:
+             val = kvm_psci_version(vcpu, vcpu->kvm);

Should this become kvm_arm_get_fw_reg() to consistently genericize the PSCI FW register accessors?

Sorry, I didn't follow. Did you mean, "kvm_arm_get_psci_fw_reg()"?

The hypercall firmware registers may hold versioning information for a particular hypercall service. Before a VM starts, these registers are read/write to the user-space. That is, it can freely modify the fields as it sees fit for the guest. However, this shouldn't be allowed once the VM is started since it may confuse the guest as it may have read an older value. As a result, introduce a helper interface to convert the registers to read-only once any vCPU starts running.

Extend this interface to also clear off all the feature bitmaps of the firmware registers upon first write. Since KVM exposes an upper limit of the feature-set to user-space via these registers, this action will ensure that no new features get enabled by accident if the user-space isn't aware of a newly added register.

Since the upcoming changes introduces more firmware registers, rename the documentation to PSCI (psci.rst) to a more generic hypercall.rst.

Signed-off-by Raghavendra Rao Ananta <[email protected]>

---
 .../virt/kvm/arm/{psci.rst => hypercalls.rst} | 24 +++----
 Documentation/virt/kvm/arm/index.rst          |  2 +-
 arch/arm64/include/asm/kvm_host.h             |  8 +++
 arch/arm64/kvm/arm.c                          |  7 +++
 arch/arm64/kvm/hypercalls.c                   | 62 +++++++++++++++++++
 5 files changed, 90 insertions(+), 13 deletions(-)
 rename Documentation/virt/kvm/arm/{psci.rst => hypercalls.rst} (81%)

Documentation/virt/kvm/arm/hypercalls.rst renamed from Documentation/virt/kvm/arm/psci.rst +1 -3

similarity index 81%
rename from Documentation/virt/kvm/arm/psci.rst
rename to Documentation/virt/kvm/arm/hypercalls.rst
index d52c2e83b5b8..85dfd682d811 100644
@@ -1,22 +1,19 @@
 .. SPDX-License-Identifier: GPL-2.0
 
-=========================================
-Power State Coordination Interface (PSCI)
-=========================================
+=======================

Documentation/virt/kvm/arm/hypercalls.rst renamed from Documentation/virt/kvm/arm/psci.rst +5 -0

+different (PSCI) version to be exposed out of the blue to an unsuspecting
+guest.
 
 In order to remedy this situation, KVM exposes a set of "firmware
 pseudo-registers" that can be manipulated using the GET/SET_ONE_REG

@@ -26,6 +23,9 @@ to a convenient value if required.
 The following register is defined:
 
 * KVM_REG_ARM_PSCI_VERSION:
+    KVM implements the PSCI (Power State Coordination Interface)
+    specification in order to provide services such as CPU on/off, reset
+    and power-off to the guest.
 
   - Only valid if the vcpu has the KVM_ARM_VCPU_PSCI_0_2 feature set
     (and thus has already been initialized)

Documentation/virt/kvm/arm/index.rst +1 -1

index 78a9b670aafe..e84848432158 100644
@@ -8,6 +8,6 @@ ARM
    :maxdepth: 2
 
    hyp-abi
-   psci
+   hypercalls
    pvtime
    ptp_kvm

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

index d0221fb69a60..0b2502494a17 100644
@@ -102,6 +102,11 @@ struct kvm_s2_mmu {
 struct kvm_arch_memory_slot {
 };
 
+struct hvc_reg_desc {
+	bool write_disabled;
+	bool write_attempted;
+};
+
 struct kvm_arch {
 	struct kvm_s2_mmu mmu;
 

@@ -137,6 +142,9 @@ struct kvm_arch {
 
 	/* Memory Tagging Extension enabled for the guest */
 	bool mte_enabled;
+
+	/* Hypercall firmware registers' information */
+	struct hvc_reg_desc hvc_desc;
 };
 
 struct kvm_vcpu_fault_info {

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

index 24a1e86d7128..f9a25e439e99 100644

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

 	return ret;
 }
 

arch/arm64/kvm/hypercalls.c +13 -0

index d030939c5929..7e873206a05b 100644
@@ -58,6 +58,12 @@ static void kvm_ptp_get_time(struct kvm_vcpu *vcpu, u64 *val)
 	val[3] = lower_32_bits(cycles);
 }
 
+static u64 *kvm_fw_reg_to_bmap(struct kvm *kvm, u64 fw_reg)
+{
+	/* No firmware registers supporting hvc bitmaps exits yet */
+	return NULL;
+}
+
 int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
 {
 	u32 func_id = smccc_get_function(vcpu);

@@ -234,15 +240,71 @@ int kvm_arm_get_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 	return 0;
 }
 
+static void kvm_fw_regs_sanitize(struct kvm *kvm, struct hvc_reg_desc *hvc_desc)
+{
+	unsigned int i;
+	u64 *hc_bmap = NULL;
+
+	mutex_lock(&kvm->lock);
+

arch/arm64/kvm/hypercalls.c +38 -0

+out:
+	mutex_unlock(&kvm->lock);
+}
+
+static bool
+kvm_fw_regs_block_write(struct kvm *kvm, struct hvc_reg_desc *hvc_desc, u64 val)
+{
+	bool ret = false;
+	unsigned int i;
+	u64 *hc_bmap = NULL;
+
+	mutex_lock(&kvm->lock);
+
+	for (i = 0; i < ARRAY_SIZE(fw_reg_ids); i++) {
+		hc_bmap = kvm_fw_reg_to_bmap(kvm, fw_reg_ids[i]);
+		if (hc_bmap)
+			break;
+	}
+
+	if (!hc_bmap)
+		goto out;
+
+	/* Do not allow any updates if the VM has already started */
+	if (hvc_desc->write_disabled && val != *hc_bmap)
+		ret = true;
+
+out:
+	mutex_unlock(&kvm->lock);
+	return ret;
+}
+
 int kvm_arm_set_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 {
 	void __user *uaddr = (void __user *)(long)reg->addr;
+	struct kvm *kvm = vcpu->kvm;
+	struct hvc_reg_desc *hvc_desc = &kvm->arch.hvc_desc;
 	u64 val;
 	int wa_level;
 
 	if (copy_from_user(&val, uaddr, KVM_REG_SIZE(reg->id)))
 		return -EFAULT;
 
+	if (kvm_fw_regs_block_write(kvm, hvc_desc, val))
+		return -EBUSY;
+
+	kvm_fw_regs_sanitize(kvm, hvc_desc);
+
 	switch (reg->id) {
 	case KVM_REG_ARM_PSCI_VERSION:
 		return kvm_arm_set_psci_fw_reg(vcpu, val);

Signature · 1 line

2.33.1.1089.g2158813163f-goog

Quoted 20 lines · Raghavendra Rao Ananta

The hypercall firmware registers may hold versioning information for a particular hypercall service. Before a VM starts, these registers are read/write to the user-space. That is, it can freely modify the fields as it sees fit for the guest. However, this shouldn't be allowed once the VM is started since it may confuse the guest as it may have read an older value. As a result, introduce a helper interface to convert the registers to read-only once any vCPU starts running.

Extend this interface to also clear off all the feature bitmaps of the firmware registers upon first write. Since KVM exposes an upper limit of the feature-set to user-space via these registers, this action will ensure that no new features get enabled by accident if the user-space isn't aware of a newly added register.

Since the upcoming changes introduces more firmware registers, rename the documentation to PSCI (psci.rst) to a more generic hypercall.rst.

Signed-off-by Raghavendra Rao Ananta <[email protected]>

Quoted 8 lines

---
 .../virt/kvm/arm/{psci.rst => hypercalls.rst} | 24 +++----
 Documentation/virt/kvm/arm/index.rst          |  2 +-
 arch/arm64/include/asm/kvm_host.h             |  8 +++
 arch/arm64/kvm/arm.c                          |  7 +++
 arch/arm64/kvm/hypercalls.c                   | 62 +++++++++++++++++++
 5 files changed, 90 insertions(+), 13 deletions(-)
 rename Documentation/virt/kvm/arm/{psci.rst => hypercalls.rst} (81%)

nit: consider doing the rename in a separate patch.

Quoted 14 lines

Documentation/virt/kvm/arm/hypercalls.rst renamed from Documentation/virt/kvm/arm/psci.rst +1 -3

similarity index 81%
rename from Documentation/virt/kvm/arm/psci.rst
rename to Documentation/virt/kvm/arm/hypercalls.rst
index d52c2e83b5b8..85dfd682d811 100644
@@ -1,22 +1,19 @@
 .. SPDX-License-Identifier: GPL-2.0
 
-=========================================
-Power State Coordination Interface (PSCI)
-=========================================
+=======================

Documentation/virt/kvm/arm/hypercalls.rst line 7

Quoted 10 lines

Documentation/virt/kvm/arm/hypercalls.rst renamed from Documentation/virt/kvm/arm/psci.rst +3 -6

+ARM Hypercall Interface
+=======================
 
-KVM implements the PSCI (Power State Coordination Interface)
-specification in order to provide services such as CPU on/off, reset
-and power-off to the guest.
-
-The PSCI specification is regularly updated to provide new features,
-and KVM implements these updates if they make sense from a virtualization
+New hypercalls are regularly added by ARM specifications (or KVM), and

nit: maybe we should use the abstraction of "hypercall service" to refer to the functional groups of hypercalls. i.e. PSCI or TRNG are hypercall services.

Quoted 3 lines

Documentation/virt/kvm/arm/hypercalls.rst renamed from Documentation/virt/kvm/arm/psci.rst +1 -0

+are made available to the guests if they make sense from a virtualization
 point of view.
 

Documentation/virt/kvm/arm/hypercalls.rst line 14

Quoted 7 lines

Documentation/virt/kvm/arm/hypercalls.rst renamed from Documentation/virt/kvm/arm/psci.rst +2 -3

 This means that a guest booted on two different versions of KVM can
 observe two different "firmware" revisions. This could cause issues if
-a given guest is tied to a particular PSCI revision (unlikely), or if
-a migration causes a different PSCI version to be exposed out of the
-blue to an unsuspecting guest.
+a given guest is tied to a particular version of a specific hypercall
+(PSCI revision for instance (unlikely)), or if a migration causes a

a particular version of a hypercall service

Quoted 58 lines

Documentation/virt/kvm/arm/hypercalls.rst renamed from Documentation/virt/kvm/arm/psci.rst +5 -0

+different (PSCI) version to be exposed out of the blue to an unsuspecting
+guest.
 
 In order to remedy this situation, KVM exposes a set of "firmware
 pseudo-registers" that can be manipulated using the GET/SET_ONE_REG

@@ -26,6 +23,9 @@ to a convenient value if required.
 The following register is defined:
 
 * KVM_REG_ARM_PSCI_VERSION:
+    KVM implements the PSCI (Power State Coordination Interface)
+    specification in order to provide services such as CPU on/off, reset
+    and power-off to the guest.
 
   - Only valid if the vcpu has the KVM_ARM_VCPU_PSCI_0_2 feature set
     (and thus has already been initialized)

Documentation/virt/kvm/arm/index.rst +1 -1

index 78a9b670aafe..e84848432158 100644
@@ -8,6 +8,6 @@ ARM
    :maxdepth: 2
 
    hyp-abi
-   psci
+   hypercalls
    pvtime
    ptp_kvm

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

index d0221fb69a60..0b2502494a17 100644
@@ -102,6 +102,11 @@ struct kvm_s2_mmu {
 struct kvm_arch_memory_slot {
 };
 
+struct hvc_reg_desc {
+     bool write_disabled;
+     bool write_attempted;
+};
+
 struct kvm_arch {
      struct kvm_s2_mmu mmu;
 

@@ -137,6 +142,9 @@ struct kvm_arch {
 
      /* Memory Tagging Extension enabled for the guest */
      bool mte_enabled;
+
+     /* Hypercall firmware registers' information */
+     struct hvc_reg_desc hvc_desc;
 };
 
 struct kvm_vcpu_fault_info {

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

index 24a1e86d7128..f9a25e439e99 100644

arch/arm64/kvm/arm.c static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu) · line 639

Quoted 10 lines

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

@@ -630,6 +630,13 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu)
      if (kvm_vm_is_protected(kvm))
              kvm_call_hyp_nvhe(__pkvm_vcpu_init_traps, vcpu);
 
+     /* Mark the hypercall firmware registers as read-only since
+      * at least once vCPU is about to start running.
+      */
+     mutex_lock(&kvm->lock);
+     kvm->arch.hvc_desc.write_disabled = true;
+     mutex_unlock(&kvm->lock);
+

This really is just an alias for if any vCPU in the VM has started yet. While the ARM KVM code does some bookkeeping around which vCPUs have been started, it is in no way specific to ARM.

It might be nice to hoist vcpu->arch.has_run_once into the generic KVM code, then build some nice abstractions there to easily determine if any vCPU in the VM has been started yet.

Quoted 31 lines

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

      return ret;
 }
 

arch/arm64/kvm/hypercalls.c +13 -0

index d030939c5929..7e873206a05b 100644
@@ -58,6 +58,12 @@ static void kvm_ptp_get_time(struct kvm_vcpu *vcpu, u64 *val)
      val[3] = lower_32_bits(cycles);
 }
 
+static u64 *kvm_fw_reg_to_bmap(struct kvm *kvm, u64 fw_reg)
+{
+     /* No firmware registers supporting hvc bitmaps exits yet */
+     return NULL;
+}
+
 int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
 {
      u32 func_id = smccc_get_function(vcpu);

@@ -234,15 +240,71 @@ int kvm_arm_get_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
      return 0;
 }
 
+static void kvm_fw_regs_sanitize(struct kvm *kvm, struct hvc_reg_desc *hvc_desc)
+{
+     unsigned int i;
+     u64 *hc_bmap = NULL;
+
+     mutex_lock(&kvm->lock);
+

arch/arm64/kvm/hypercalls.c int kvm_arm_get_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) · line 259

Quoted 10 lines

arch/arm64/kvm/hypercalls.c +10 -0

+     if (hvc_desc->write_attempted)
+             goto out;
+
+     hvc_desc->write_attempted = true;
+
+     for (i = 0; i < ARRAY_SIZE(fw_reg_ids); i++) {
+             hc_bmap = kvm_fw_reg_to_bmap(kvm, fw_reg_ids[i]);
+             if (hc_bmap)
+                     *hc_bmap = 0;
+     }

Maybe instead of checking for feature bitmap registers in the full range of FW registers, you could separately track a list of feature bitmap regs and just iterate over that.

You could then just stash an array/substructure of feature bitmap reg values in struct kvm_arch, along with a bitmap of which regs were touched by the VMM.

For the first vCPU in KVM_RUN, zero out the FW feature regs that were never written to. You could then punt the clobber operation and do it exactly once for a VM.

Quoted 42 lines

arch/arm64/kvm/hypercalls.c +33 -0

+out:
+     mutex_unlock(&kvm->lock);
+}
+
+static bool
+kvm_fw_regs_block_write(struct kvm *kvm, struct hvc_reg_desc *hvc_desc, u64 val)
+{
+     bool ret = false;
+     unsigned int i;
+     u64 *hc_bmap = NULL;
+
+     mutex_lock(&kvm->lock);
+
+     for (i = 0; i < ARRAY_SIZE(fw_reg_ids); i++) {
+             hc_bmap = kvm_fw_reg_to_bmap(kvm, fw_reg_ids[i]);
+             if (hc_bmap)
+                     break;
+     }
+
+     if (!hc_bmap)
+             goto out;
+
+     /* Do not allow any updates if the VM has already started */
+     if (hvc_desc->write_disabled && val != *hc_bmap)
+             ret = true;
+
+out:
+     mutex_unlock(&kvm->lock);
+     return ret;
+}
+
 int kvm_arm_set_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 {
      void __user *uaddr = (void __user *)(long)reg->addr;
+     struct kvm *kvm = vcpu->kvm;
+     struct hvc_reg_desc *hvc_desc = &kvm->arch.hvc_desc;
      u64 val;
      int wa_level;
 
      if (copy_from_user(&val, uaddr, KVM_REG_SIZE(reg->id)))
              return -EFAULT;
 

Quoted 11 lines

arch/arm64/kvm/hypercalls.c +5 -1

+     if (kvm_fw_regs_block_write(kvm, hvc_desc, val))
+             return -EBUSY;
+
+     kvm_fw_regs_sanitize(kvm, hvc_desc);
+
      switch (reg->id) {
      case KVM_REG_ARM_PSCI_VERSION:
              return kvm_arm_set_psci_fw_reg(vcpu, val);
--

2.33.1.1089.g2158813163f-goog

Introduce a firmware register that encapsulates standard secure service calls (owner value 4) as a bitmap. Depending on how the user-space configures the register, the features will be enabled or disabled for the guest. Currently, this includes support only for ARM True Random Number Generator (TRNG) service, with bit-0 of the register representing mandatory features of v1.0.

Signed-off-by Raghavendra Rao Ananta <[email protected]>

---
 Documentation/virt/kvm/arm/hypercalls.rst | 17 +++++
 arch/arm64/include/asm/kvm_host.h         |  2 +
 arch/arm64/include/uapi/asm/kvm.h         |  6 ++
 arch/arm64/kvm/arm.c                      |  8 +++
 arch/arm64/kvm/hypercalls.c               | 75 ++++++++++++++++++++++-
 arch/arm64/kvm/trng.c                     |  9 +--
 include/kvm/arm_hypercalls.h              |  5 ++
 7 files changed, 113 insertions(+), 9 deletions(-)

Documentation/virt/kvm/arm/hypercalls.rst +0 -0

index 85dfd682d811..1601919f256d 100644

Documentation/virt/kvm/arm/hypercalls.rst +7 -0

+in the form of a feature bitmap. Upon VM creation, by default, KVM exposes
+all the features to the guest, which can be learnt using GET_ONE_REG
+interface. Conversely, the features can be enabled or disabled via the
+SET_ONE_REG interface. These registers allow the user-space modification
+only until the VM has started running, after which they turn to read-only
+registers. SET_ONE_REG in this scenario will return -EBUSY.
+
 The following register is defined:
 
 * KVM_REG_ARM_PSCI_VERSION:

Documentation/virt/kvm/arm/hypercalls.rst +3 -0

+        The bit represents the services offered under v1.0 of ARM True Random Number Generator
+        (TRNG) specification (ARM DEN 0098).
+
 .. [1] https://developer.arm.com/-/media/developer/pdf/ARM_DEN_0070A_Firmware_interfaces_for_mitigating_CVE-2017-5715.pdf

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

index 0b2502494a17..176d6be7b4da 100644
@@ -105,6 +105,8 @@ struct kvm_arch_memory_slot {
 struct hvc_reg_desc {
 	bool write_disabled;
 	bool write_attempted;
+
+	u64 kvm_std_bmap;
 };
 
 struct kvm_arch {

arch/arm64/include/uapi/asm/kvm.h +0 -0

index b3edde68bc3e..6387dea5396d 100644

arch/arm64/include/uapi/asm/kvm.h +0 -0

 /* SVE registers */
 #define KVM_REG_ARM64_SVE		(0x15 << KVM_REG_ARM_COPROC_SHIFT)
 

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

index f9a25e439e99..1cf58aa49222 100644
@@ -130,6 +130,13 @@ static void set_default_spectre(struct kvm *kvm)
 		kvm->arch.pfr0_csv3 = 1;
 }
 
+static void set_default_hypercalls(struct kvm *kvm)
+{
+	struct hvc_reg_desc *hvc_desc = &kvm->arch.hvc_desc;
+
+	hvc_desc->kvm_std_bmap = ARM_SMCCC_STD_FEATURES;
+}
+
 /**
  * kvm_arch_init_vm - initializes a VM data structure
  * @kvm:	pointer to the KVM struct

@@ -156,6 +163,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
 	kvm->arch.max_vcpus = kvm_arm_default_max_vcpus();
 
 	set_default_spectre(kvm);
+	set_default_hypercalls(kvm);
 
 	return ret;
 out_free_stage2_pgd:

arch/arm64/kvm/hypercalls.c +45 -2

index 7e873206a05b..0b3006353bf6 100644
@@ -60,8 +60,64 @@ static void kvm_ptp_get_time(struct kvm_vcpu *vcpu, u64 *val)
 
 static u64 *kvm_fw_reg_to_bmap(struct kvm *kvm, u64 fw_reg)
 {
-	/* No firmware registers supporting hvc bitmaps exits yet */
-	return NULL;
+	struct hvc_reg_desc *hvc_desc = &kvm->arch.hvc_desc;
+
+	switch (fw_reg) {
+	case KVM_REG_ARM_STD:
+		return &hvc_desc->kvm_std_bmap;
+	default:
+		return NULL;
+	}
+}
+
+struct kvm_hvc_func_map {
+	u32 func_id;
+	u64 bmap_bit;
+};
+
+#define HVC_FUNC_MAP_DESC(func, bit)	\
+	{					\
+		.func_id = func,		\
+		.bmap_bit = bit,		\
+	}
+
+static const struct kvm_hvc_func_map hvc_std_map[] = {
+	HVC_FUNC_MAP_DESC(ARM_SMCCC_TRNG_GET_UUID, KVM_REG_ARM_STD_TRNG_V1_0),
+	HVC_FUNC_MAP_DESC(ARM_SMCCC_TRNG_RND32, KVM_REG_ARM_STD_TRNG_V1_0),
+	HVC_FUNC_MAP_DESC(ARM_SMCCC_TRNG_RND64, KVM_REG_ARM_STD_TRNG_V1_0),
+};
+
+bool kvm_hvc_call_supported(struct kvm_vcpu *vcpu, u32 func_id)
+{
+	struct kvm *kvm = vcpu->kvm;
+	u8 hvc_owner = ARM_SMCCC_OWNER_NUM(func_id);
+	const struct kvm_hvc_func_map *hvc_func_map = NULL;
+
+	u64 fw_reg, *hc_bmap;
+	unsigned int map_sz, i;
+
+	switch (hvc_owner) {
+	case ARM_SMCCC_OWNER_STANDARD:
+		fw_reg = KVM_REG_ARM_STD;
+		hvc_func_map = hvc_std_map;
+		map_sz = ARRAY_SIZE(hvc_std_map);
+		break;
+	default:
+		/* Allow all the owners that aren't mapped */
+		return true;

arch/arm64/kvm/hypercalls.c +7 -0

+
+	/* Allow all the functions of an owner that aren't mapped */
+	return true;
 }
 
 int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)

@@ -71,6 +127,9 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
 	u32 feature;
 	gpa_t gpa;
 
+	if (!kvm_hvc_call_supported(vcpu, func_id))
+		goto out;
+
 	switch (func_id) {
 	case ARM_SMCCC_VERSION_FUNC_ID:
 		val[0] = ARM_SMCCC_VERSION_1_1;

@@ -149,6 +208,7 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
 		return kvm_psci_call(vcpu);
 	}
 
+out:
 	smccc_set_retval(vcpu, val[0], val[1], val[2], val[3]);
 	return 1;
 }

arch/arm64/kvm/hypercalls.c +10 -0

 };
 
 int kvm_arm_get_fw_num_regs(struct kvm_vcpu *vcpu)

@@ -219,6 +280,7 @@ static int get_kernel_wa_level(u64 regid)
 
 int kvm_arm_get_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 {
+	struct hvc_reg_desc *hvc_desc = &vcpu->kvm->arch.hvc_desc;
 	void __user *uaddr = (void __user *)(long)reg->addr;
 	u64 val;
 

@@ -230,6 +292,9 @@ int kvm_arm_get_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 	case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2:
 		val = get_kernel_wa_level(reg->id) & KVM_REG_FEATURE_LEVEL_MASK;
 		break;
+	case KVM_REG_ARM_STD:
+		val = hvc_desc->kvm_std_bmap;
+		break;
 	default:
 		return -ENOENT;
 	}

@@ -352,6 +417,12 @@ int kvm_arm_set_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 		if (get_kernel_wa_level(reg->id) < wa_level)
 			return -EINVAL;
 
+		return 0;
+	case KVM_REG_ARM_STD:
+		if (val & ~ARM_SMCCC_STD_FEATURES)
+			return -EINVAL;
+
+		hvc_desc->kvm_std_bmap = val;
 		return 0;
 	default:
 		return -ENOENT;

arch/arm64/kvm/trng.c +2 -7

index 99bdd7103c9c..6dff765f5b9b 100644
@@ -60,14 +60,9 @@ int kvm_trng_call(struct kvm_vcpu *vcpu)
 		val = ARM_SMCCC_TRNG_VERSION_1_0;
 		break;
 	case ARM_SMCCC_TRNG_FEATURES:
-		switch (smccc_get_arg1(vcpu)) {
-		case ARM_SMCCC_TRNG_VERSION:
-		case ARM_SMCCC_TRNG_FEATURES:
-		case ARM_SMCCC_TRNG_GET_UUID:
-		case ARM_SMCCC_TRNG_RND32:
-		case ARM_SMCCC_TRNG_RND64:
+		if (kvm_hvc_call_supported(vcpu, smccc_get_arg1(vcpu)))
 			val = TRNG_SUCCESS;
-		}
+
 		break;
 	case ARM_SMCCC_TRNG_GET_UUID:
 		smccc_set_retval(vcpu, le32_to_cpu(u[0]), le32_to_cpu(u[1]),

include/kvm/arm_hypercalls.h +5 -0

index 5d38628a8d04..5f01bb139312 100644
@@ -6,6 +6,9 @@
 
 #include <asm/kvm_emulate.h>
 
+#define ARM_SMCCC_STD_FEATURES \
+	GENMASK_ULL(KVM_REG_ARM_STD_BMAP_MAX - 1, 0)
+
 int kvm_hvc_call_handler(struct kvm_vcpu *vcpu);
 
 static inline u32 smccc_get_function(struct kvm_vcpu *vcpu)

@@ -47,4 +50,6 @@ int kvm_arm_copy_fw_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices);
 int kvm_arm_get_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg);
 int kvm_arm_set_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg);
 
+bool kvm_hvc_call_supported(struct kvm_vcpu *vcpu, u32 func_id);
+
 #endif

Signature · 1 line

2.33.1.1089.g2158813163f-goog

Quoted 23 lines · Raghavendra Rao Ananta

Introduce a firmware register that encapsulates standard secure service calls (owner value 4) as a bitmap. Depending on how the user-space configures the register, the features will be enabled or disabled for the guest. Currently, this includes support only for ARM True Random Number Generator (TRNG) service, with bit-0 of the register representing mandatory features of v1.0.

Signed-off-by Raghavendra Rao Ananta <[email protected]>

---
 Documentation/virt/kvm/arm/hypercalls.rst | 17 +++++
 arch/arm64/include/asm/kvm_host.h         |  2 +
 arch/arm64/include/uapi/asm/kvm.h         |  6 ++
 arch/arm64/kvm/arm.c                      |  8 +++
 arch/arm64/kvm/hypercalls.c               | 75 ++++++++++++++++++++++-
 arch/arm64/kvm/trng.c                     |  9 +--
 include/kvm/arm_hypercalls.h              |  5 ++
 7 files changed, 113 insertions(+), 9 deletions(-)

Documentation/virt/kvm/arm/hypercalls.rst +0 -0

index 85dfd682d811..1601919f256d 100644

Documentation/virt/kvm/arm/hypercalls.rst pseudo-registers" that can be manipulated using the GET/SET_ONE_REG · line 23

Quoted 4 lines

Documentation/virt/kvm/arm/hypercalls.rst +1 -0

@@ -20,6 +20,14 @@ pseudo-registers" that can be manipulated using the GET/SET_ONE_REG
 interface. These registers can be saved/restored by userspace, and set
 to a convenient value if required.
 
+The firmware register KVM_REG_ARM_STD exposes the hypercall services

nit: try to cram BITMAP in the name. IMO, this will help disambiguate with version-based FW regs, like KVM_REG_ARM_PSCI_VERSION.

Quoted 14 lines

Documentation/virt/kvm/arm/hypercalls.rst +7 -0

+in the form of a feature bitmap. Upon VM creation, by default, KVM exposes
+all the features to the guest, which can be learnt using GET_ONE_REG
+interface. Conversely, the features can be enabled or disabled via the
+SET_ONE_REG interface. These registers allow the user-space modification
+only until the VM has started running, after which they turn to read-only
+registers. SET_ONE_REG in this scenario will return -EBUSY.
+
 The following register is defined:
 
 * KVM_REG_ARM_PSCI_VERSION:

@@ -74,4 +82,13 @@ The following register is defined:
     KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_REQUIRED:
       The workaround is always active on this vCPU or it is not needed.
 

Documentation/virt/kvm/arm/hypercalls.rst The following register is defined: · line 90

Quoted 6 lines

Documentation/virt/kvm/arm/hypercalls.rst +6 -0

+* KVM_REG_ARM_STD:
+    Controls the bitmap of the ARM Standard Secure Service Calls.
+
+    The following bits are accepted:
+
+      KVM_REG_ARM_STD_TRNG_V1_0:

state the bit position as well

Quoted 22 lines

Documentation/virt/kvm/arm/hypercalls.rst +3 -0

+        The bit represents the services offered under v1.0 of ARM True Random Number Generator
+        (TRNG) specification (ARM DEN 0098).
+
 .. [1] https://developer.arm.com/-/media/developer/pdf/ARM_DEN_0070A_Firmware_interfaces_for_mitigating_CVE-2017-5715.pdf

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

index 0b2502494a17..176d6be7b4da 100644
@@ -105,6 +105,8 @@ struct kvm_arch_memory_slot {
 struct hvc_reg_desc {
      bool write_disabled;
      bool write_attempted;
+
+     u64 kvm_std_bmap;
 };
 
 struct kvm_arch {

arch/arm64/include/uapi/asm/kvm.h +0 -0

index b3edde68bc3e..6387dea5396d 100644

arch/arm64/include/uapi/asm/kvm.h struct kvm_arm_copy_mte_tags { · line 289

Quoted 9 lines

arch/arm64/include/uapi/asm/kvm.h +6 -0

@@ -281,6 +281,12 @@ struct kvm_arm_copy_mte_tags {
 #define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_REQUIRED     3
 #define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_ENABLED          (1U << 4)
 
+#define KVM_REG_ARM_STD                      KVM_REG_ARM_FW_REG(3)
+enum kvm_reg_arm_std_bmap {
+     KVM_REG_ARM_STD_TRNG_V1_0,
+     KVM_REG_ARM_STD_BMAP_MAX,
+};
+

I would recommend just defining the bit values explicitly rather than using an enumeration:

  #define KVM_REG_ARM_STD_TRNG_V1_0     (1ULL << 0)

You do lose the convenience of having KVM_REG_ARM_STD_BMAP_MAX.

Quoted 84 lines

arch/arm64/include/uapi/asm/kvm.h +0 -0

 /* SVE registers */
 #define KVM_REG_ARM64_SVE            (0x15 << KVM_REG_ARM_COPROC_SHIFT)
 

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

index f9a25e439e99..1cf58aa49222 100644
@@ -130,6 +130,13 @@ static void set_default_spectre(struct kvm *kvm)
              kvm->arch.pfr0_csv3 = 1;
 }
 
+static void set_default_hypercalls(struct kvm *kvm)
+{
+     struct hvc_reg_desc *hvc_desc = &kvm->arch.hvc_desc;
+
+     hvc_desc->kvm_std_bmap = ARM_SMCCC_STD_FEATURES;
+}
+
 /**
  * kvm_arch_init_vm - initializes a VM data structure
  * @kvm:     pointer to the KVM struct

@@ -156,6 +163,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
      kvm->arch.max_vcpus = kvm_arm_default_max_vcpus();
 
      set_default_spectre(kvm);
+     set_default_hypercalls(kvm);
 
      return ret;
 out_free_stage2_pgd:

arch/arm64/kvm/hypercalls.c +45 -2

index 7e873206a05b..0b3006353bf6 100644
@@ -60,8 +60,64 @@ static void kvm_ptp_get_time(struct kvm_vcpu *vcpu, u64 *val)
 
 static u64 *kvm_fw_reg_to_bmap(struct kvm *kvm, u64 fw_reg)
 {
-     /* No firmware registers supporting hvc bitmaps exits yet */
-     return NULL;
+     struct hvc_reg_desc *hvc_desc = &kvm->arch.hvc_desc;
+
+     switch (fw_reg) {
+     case KVM_REG_ARM_STD:
+             return &hvc_desc->kvm_std_bmap;
+     default:
+             return NULL;
+     }
+}
+
+struct kvm_hvc_func_map {
+     u32 func_id;
+     u64 bmap_bit;
+};
+
+#define HVC_FUNC_MAP_DESC(func, bit) \
+     {                                       \
+             .func_id = func,                \
+             .bmap_bit = bit,                \
+     }
+
+static const struct kvm_hvc_func_map hvc_std_map[] = {
+     HVC_FUNC_MAP_DESC(ARM_SMCCC_TRNG_GET_UUID, KVM_REG_ARM_STD_TRNG_V1_0),
+     HVC_FUNC_MAP_DESC(ARM_SMCCC_TRNG_RND32, KVM_REG_ARM_STD_TRNG_V1_0),
+     HVC_FUNC_MAP_DESC(ARM_SMCCC_TRNG_RND64, KVM_REG_ARM_STD_TRNG_V1_0),
+};
+
+bool kvm_hvc_call_supported(struct kvm_vcpu *vcpu, u32 func_id)
+{
+     struct kvm *kvm = vcpu->kvm;
+     u8 hvc_owner = ARM_SMCCC_OWNER_NUM(func_id);
+     const struct kvm_hvc_func_map *hvc_func_map = NULL;
+
+     u64 fw_reg, *hc_bmap;
+     unsigned int map_sz, i;
+
+     switch (hvc_owner) {
+     case ARM_SMCCC_OWNER_STANDARD:
+             fw_reg = KVM_REG_ARM_STD;
+             hvc_func_map = hvc_std_map;
+             map_sz = ARRAY_SIZE(hvc_std_map);
+             break;
+     default:
+             /* Allow all the owners that aren't mapped */
+             return true;

arch/arm64/kvm/hypercalls.c static void kvm_ptp_get_time(struct kvm_vcpu *vcpu, u64 *val) · line 117

Quoted 10 lines

arch/arm64/kvm/hypercalls.c +10 -0

+     }
+
+     hc_bmap = kvm_fw_reg_to_bmap(kvm, fw_reg);
+     if (!hc_bmap)
+             return true;
+
+     for (i = 0; i < map_sz; i++) {
+             if (func_id == hvc_func_map[i].func_id)
+                     return *hc_bmap & BIT(hvc_func_map[i].bmap_bit);
+     }

Hrm...

Could you instead define a helper function for each service and use a switch statement to ensure each function tests the correct bit? This would avoid the need to loop over a map.

Quoted 20 lines

arch/arm64/kvm/hypercalls.c +6 -0

+
+     /* Allow all the functions of an owner that aren't mapped */
+     return true;
 }
 
 int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)

@@ -71,6 +127,9 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
      u32 feature;
      gpa_t gpa;
 
+     if (!kvm_hvc_call_supported(vcpu, func_id))
+             goto out;
+
      switch (func_id) {
      case ARM_SMCCC_VERSION_FUNC_ID:
              val[0] = ARM_SMCCC_VERSION_1_1;

@@ -149,6 +208,7 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
              return kvm_psci_call(vcpu);
      }
 

arch/arm64/kvm/hypercalls.c static const u64 fw_reg_ids[] = { · line 220

Quoted 9 lines

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

+out:
      smccc_set_retval(vcpu, val[0], val[1], val[2], val[3]);
      return 1;
 }

@@ -157,6 +217,7 @@ static const u64 fw_reg_ids[] = {
      KVM_REG_ARM_PSCI_VERSION,
      KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1,
      KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2,
+     KVM_REG_ARM_STD,

This (and all other FW regs you add) need to be added to the get-reg-list selftest. Marc/Andrew have reminded me enough times to do this myself, so I'll share suggestion :-P

Quoted 79 lines

arch/arm64/kvm/hypercalls.c +10 -0

 };
 
 int kvm_arm_get_fw_num_regs(struct kvm_vcpu *vcpu)

@@ -219,6 +280,7 @@ static int get_kernel_wa_level(u64 regid)
 
 int kvm_arm_get_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 {
+     struct hvc_reg_desc *hvc_desc = &vcpu->kvm->arch.hvc_desc;
      void __user *uaddr = (void __user *)(long)reg->addr;
      u64 val;
 

@@ -230,6 +292,9 @@ int kvm_arm_get_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
      case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2:
              val = get_kernel_wa_level(reg->id) & KVM_REG_FEATURE_LEVEL_MASK;
              break;
+     case KVM_REG_ARM_STD:
+             val = hvc_desc->kvm_std_bmap;
+             break;
      default:
              return -ENOENT;
      }

@@ -352,6 +417,12 @@ int kvm_arm_set_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
              if (get_kernel_wa_level(reg->id) < wa_level)
                      return -EINVAL;
 
+             return 0;
+     case KVM_REG_ARM_STD:
+             if (val & ~ARM_SMCCC_STD_FEATURES)
+                     return -EINVAL;
+
+             hvc_desc->kvm_std_bmap = val;
              return 0;
      default:
              return -ENOENT;

arch/arm64/kvm/trng.c +2 -7

index 99bdd7103c9c..6dff765f5b9b 100644
@@ -60,14 +60,9 @@ int kvm_trng_call(struct kvm_vcpu *vcpu)
              val = ARM_SMCCC_TRNG_VERSION_1_0;
              break;
      case ARM_SMCCC_TRNG_FEATURES:
-             switch (smccc_get_arg1(vcpu)) {
-             case ARM_SMCCC_TRNG_VERSION:
-             case ARM_SMCCC_TRNG_FEATURES:
-             case ARM_SMCCC_TRNG_GET_UUID:
-             case ARM_SMCCC_TRNG_RND32:
-             case ARM_SMCCC_TRNG_RND64:
+             if (kvm_hvc_call_supported(vcpu, smccc_get_arg1(vcpu)))
                      val = TRNG_SUCCESS;
-             }
+
              break;
      case ARM_SMCCC_TRNG_GET_UUID:
              smccc_set_retval(vcpu, le32_to_cpu(u[0]), le32_to_cpu(u[1]),

include/kvm/arm_hypercalls.h +5 -1

index 5d38628a8d04..5f01bb139312 100644
@@ -6,6 +6,9 @@
 
 #include <asm/kvm_emulate.h>
 
+#define ARM_SMCCC_STD_FEATURES \
+     GENMASK_ULL(KVM_REG_ARM_STD_BMAP_MAX - 1, 0)
+
 int kvm_hvc_call_handler(struct kvm_vcpu *vcpu);
 
 static inline u32 smccc_get_function(struct kvm_vcpu *vcpu)

@@ -47,4 +50,6 @@ int kvm_arm_copy_fw_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices);
 int kvm_arm_get_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg);
 int kvm_arm_set_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg);
 
+bool kvm_hvc_call_supported(struct kvm_vcpu *vcpu, u32 func_id);
+
 #endif
--

2.33.1.1089.g2158813163f-goog

Import the firmware definitions for the firmware registers, KVM_REG_ARM_STD, KVM_REG_ARM_STD_HYP, and KVM_REG_ARM_VENDOR_HYP.

Signed-off-by Raghavendra Rao Ananta <[email protected]>

---
 tools/arch/arm64/include/uapi/asm/kvm.h | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

tools/arch/arm64/include/uapi/asm/kvm.h +18 -0

index b3edde68bc3e..a1d0e8e69eed 100644
@@ -281,6 +281,24 @@ struct kvm_arm_copy_mte_tags {
 #define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_REQUIRED	3
 #define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_ENABLED     	(1U << 4)
 
+#define KVM_REG_ARM_STD			KVM_REG_ARM_FW_REG(3)
+enum kvm_reg_arm_std_bmap {
+	KVM_REG_ARM_STD_TRNG_V1_0,
+	KVM_REG_ARM_STD_BMAP_MAX,
+};
+
+#define KVM_REG_ARM_STD_HYP		KVM_REG_ARM_FW_REG(4)
+enum kvm_reg_arm_std_hyp_bmap {
+	KVM_REG_ARM_STD_HYP_PV_TIME_ST,
+	KVM_REG_ARM_STD_HYP_BMAP_MAX,
+};
+
+#define KVM_REG_ARM_VENDOR_HYP		KVM_REG_ARM_FW_REG(5)
+enum kvm_reg_arm_vendor_hyp_bmap {
+	KVM_REG_ARM_VENDOR_HYP_PTP,
+	KVM_REG_ARM_VENDOR_HYP_BMAP_MAX,
+};
+
 /* SVE registers */
 #define KVM_REG_ARM64_SVE		(0x15 << KVM_REG_ARM_COPROC_SHIFT)
 

Signature · 1 line

2.33.1.1089.g2158813163f-goog

Quoted 3 lines · Raghavendra Rao Ananta

Import the firmware definitions for the firmware registers, KVM_REG_ARM_STD, KVM_REG_ARM_STD_HYP, and KVM_REG_ARM_VENDOR_HYP.

Quoted 5 lines

Signed-off-by Raghavendra Rao Ananta <[email protected]>

---
 tools/arch/arm64/include/uapi/asm/kvm.h | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

Won't we have the latest UAPI headers available in usr/include/ at build time?

--
Oliver

Quoted 32 lines

tools/arch/arm64/include/uapi/asm/kvm.h +18 -1

index b3edde68bc3e..a1d0e8e69eed 100644
@@ -281,6 +281,24 @@ struct kvm_arm_copy_mte_tags {
 #define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_REQUIRED     3
 #define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_ENABLED          (1U << 4)
 
+#define KVM_REG_ARM_STD                      KVM_REG_ARM_FW_REG(3)
+enum kvm_reg_arm_std_bmap {
+     KVM_REG_ARM_STD_TRNG_V1_0,
+     KVM_REG_ARM_STD_BMAP_MAX,
+};
+
+#define KVM_REG_ARM_STD_HYP          KVM_REG_ARM_FW_REG(4)
+enum kvm_reg_arm_std_hyp_bmap {
+     KVM_REG_ARM_STD_HYP_PV_TIME_ST,
+     KVM_REG_ARM_STD_HYP_BMAP_MAX,
+};
+
+#define KVM_REG_ARM_VENDOR_HYP               KVM_REG_ARM_FW_REG(5)
+enum kvm_reg_arm_vendor_hyp_bmap {
+     KVM_REG_ARM_VENDOR_HYP_PTP,
+     KVM_REG_ARM_VENDOR_HYP_BMAP_MAX,
+};
+
 /* SVE registers */
 #define KVM_REG_ARM64_SVE            (0x15 << KVM_REG_ARM_COPROC_SHIFT)
 
--

2.33.1.1089.g2158813163f-goog