[RFC PATCH 0/8] KVM: arm64: Add support for hypercall services selection

[RFC PATCH 0/8] KVM: arm64: Add support for hypercall services selection

[Raghavendra Rao Ananta]

Hello,

Continuing the discussion from [1], the series tries to add support
for the user-space to elect the hypercall services that it wishes
to expose to the guest, rather than the guest discovering them
unconditionally. The idea employed by the series was taken from
[1] as suggested by Marc Z.

In a broad sense, the idea is similar to the current implementation
of PSCI interface- create a 'psuedo-firmware register' to handle the
firmware revisions. The series extends this idea to all the other
hypercalls such as TRNG (True Random Number Generator), PV_TIME
(Paravirtualized Time), and PTP (Precision Time protocol).

For better categorization and future scaling, firmware registers
are introduced based on the SMCCC service call owner (standard secure
service, standard hypervisor service, and vendor specific hypervisor
service). Each of these registers exposes the features employed in
the form of a bitmap and are enveloped into a generic interface (for
future expansion).

Upon VM creation, all the features supported by each owner type are
enabled. User-space/VMM can learn about the services currently enabled
via GET_ONE_REG and can manipulate them via SET_ONE_REG interfaces.
These 'writes' directly effect the bitmap, which is further checked
when the guest tries to issue the hypercall and a decision is taken
weather or not the hypercall is accessable to the guest. The interface
works well across live-migrations where the VMM can simply save/restore
these firmware registers using the existing IOCTL interfaces.

Upon VM start (at least one vCPU runs), the registers become read-only
and cannot be manupulated by the VMM. This is just to avoid providing
conflicting views of the services to the guests.

One of the problems that the series need to address is the enablement
of the features carried by a firmware register, whose existance is
not known to the VMM yet. A couple of ideas were discussed to handle this:

    1) Upon the first SET_ONE_REG, clear all the firmware registers
    implicitly. It's the responsibility of the VMM to make sure that it
    configures all the registers that's known to it.

    2) Contrary to #1, which implicitly clears all the registers, introduce
    a new capability to handle this explicitly. That is, the after learning
    about the services supported by the host, the VMM writes to the
    capability to explictly clear the registers.

The series currently employs #1 just for the sake of completion, but is
open for further discussion.

The patches are based off of kvmarm-next 5.15-rc4, with the selftest
patches from [2] applied.

Patch-1 factors out the non-PSCI related interface from psci.c to
hypercalls.c, as the series would extend the list in the upcoming
patches.

Patch-2 sets up a base environment to handle the 'writes' of firmware
register- clear all the registers upon first 'write' and block 'writes'
to the registers upon VM start.

Patch-3 introduces the firmware register, KVM_REG_ARM_STD, which holds
the standard secure services (such as TRNG).

Patch-4 introduces the firmware register, KVM_REG_ARM_STD_HYP, which holds
the standard hypervisor services (such as PV_TIME).

Patch-5 introduces the firmware register, KVM_REG_ARM_VENDOR_HYP, which holds
the vendor specific hypercall services.

Patch-6 imports the firmware registers' UAPI definitions into tools/ for
further use in selftests.

Patch-7 imports the SMCCC definitions from linux/arm-smccc.h into tools/
for further use in selftests.

Patch-8 adds the selftest to test the guest (using 'hvc') and VMM
interfaces (SET/GET_ONE_REG).

[1]: https://lore.kernel.org/kvmarm/874kbcpmlq.wl-maz@kernel.org/T/
[2]: https://lore.kernel.org/kvmarm/YUzgdbYk8BeCnHyW@google.com/

Raghavendra Rao Ananta (8):
  KVM: arm64: Factor out firmware register handling from psci.c
  KVM: arm64: Setup base for hypercall firmware registers
  KVM: arm64: Add standard secure service calls firmware register
  KVM: arm64: Add standard hypervisor service calls firmware register
  KVM: arm64: Add vendor hypervisor service calls firmware register
  tools: Import the firmware registers
  tools: Import ARM SMCCC definitions
  selftests: KVM: aarch64: Introduce hypercall ABI test

 .../virt/kvm/arm/{psci.rst => hypercalls.rst} |  59 ++-
 Documentation/virt/kvm/arm/index.rst          |   2 +-
 arch/arm64/include/asm/kvm_host.h             |  12 +
 arch/arm64/include/uapi/asm/kvm.h             |  18 +
 arch/arm64/kvm/arm.c                          |  17 +
 arch/arm64/kvm/guest.c                        |   2 +-
 arch/arm64/kvm/hypercalls.c                   | 339 ++++++++++++++++-
 arch/arm64/kvm/psci.c                         | 167 +--------
 arch/arm64/kvm/pvtime.c                       |   3 +
 arch/arm64/kvm/trng.c                         |   9 +-
 include/kvm/arm_hypercalls.h                  |  18 +
 include/kvm/arm_psci.h                        |   8 +-
 tools/arch/arm64/include/uapi/asm/kvm.h       |  18 +
 tools/include/linux/arm-smccc.h               | 188 ++++++++++
 tools/testing/selftests/kvm/.gitignore        |   1 +
 tools/testing/selftests/kvm/Makefile          |   1 +
 .../selftests/kvm/aarch64/hypercalls.c        | 340 ++++++++++++++++++
 17 files changed, 1018 insertions(+), 184 deletions(-)
 rename Documentation/virt/kvm/arm/{psci.rst => hypercalls.rst} (57%)
 create mode 100644 tools/include/linux/arm-smccc.h
 create mode 100644 tools/testing/selftests/kvm/aarch64/hypercalls.c

-- 
2.33.1.1089.g2158813163f-goog

[RFC PATCH 1/8] KVM: arm64: Factor out firmware register handling from psci.c

[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 <rananta@google.com>
---
 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(-)

diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
index 5ce26bedf23c..625f97f7b304 100644
--- a/arch/arm64/kvm/guest.c
+++ b/arch/arm64/kvm/guest.c
@@ -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>
diff --git a/arch/arm64/kvm/hypercalls.c b/arch/arm64/kvm/hypercalls.c
index 30da78f72b3b..d030939c5929 100644
--- a/arch/arm64/kvm/hypercalls.c
+++ b/arch/arm64/kvm/hypercalls.c
@@ -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)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(fw_reg_ids); i++) {
+		if (put_user(fw_reg_ids[i], uindices))
+			return -EFAULT;
+	}
+
+	return 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;
+	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:
+		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;
+}
diff --git a/arch/arm64/kvm/psci.c b/arch/arm64/kvm/psci.c
index 74c47d420253..b9bcbc919b19 100644
--- a/arch/arm64/kvm/psci.c
+++ b/arch/arm64/kvm/psci.c
@@ -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;
 }
diff --git a/include/kvm/arm_hypercalls.h b/include/kvm/arm_hypercalls.h
index 0e2509d27910..5d38628a8d04 100644
--- a/include/kvm/arm_hypercalls.h
+++ b/include/kvm/arm_hypercalls.h
@@ -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
diff --git a/include/kvm/arm_psci.h b/include/kvm/arm_psci.h
index 5b58bd2fe088..eddbd7d805e9 100644
--- a/include/kvm/arm_psci.h
+++ b/include/kvm/arm_psci.h
@@ -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

Re: [RFC PATCH 1/8] KVM: arm64: Factor out firmware register handling from psci.c

[Oliver Upton]

Hi Raghu,

On Tue, Nov 02, 2021 at 12:21:56AM +0000, Raghavendra Rao Ananta wrote:
> 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 <rananta@google.com>
> ---
>  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(-)
> 
> diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> index 5ce26bedf23c..625f97f7b304 100644
> --- a/arch/arm64/kvm/guest.c
> +++ b/arch/arm64/kvm/guest.c
> @@ -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>
> diff --git a/arch/arm64/kvm/hypercalls.c b/arch/arm64/kvm/hypercalls.c
> index 30da78f72b3b..d030939c5929 100644
> --- a/arch/arm64/kvm/hypercalls.c
> +++ b/arch/arm64/kvm/hypercalls.c
> @@ -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)
> +{
> +	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.

> +#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;
> +	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?

> +		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;
> +}
> diff --git a/arch/arm64/kvm/psci.c b/arch/arm64/kvm/psci.c
> index 74c47d420253..b9bcbc919b19 100644
> --- a/arch/arm64/kvm/psci.c
> +++ b/arch/arm64/kvm/psci.c
> @@ -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;
>  }
> diff --git a/include/kvm/arm_hypercalls.h b/include/kvm/arm_hypercalls.h
> index 0e2509d27910..5d38628a8d04 100644
> --- a/include/kvm/arm_hypercalls.h
> +++ b/include/kvm/arm_hypercalls.h
> @@ -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
> diff --git a/include/kvm/arm_psci.h b/include/kvm/arm_psci.h
> index 5b58bd2fe088..eddbd7d805e9 100644
> --- a/include/kvm/arm_psci.h
> +++ b/include/kvm/arm_psci.h
> @@ -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
> 

Re: [RFC PATCH 1/8] KVM: arm64: Factor out firmware register handling from psci.c

[Raghavendra Rao Ananta]

Hi Oliver,

On Wed, Nov 3, 2021 at 2:43 PM Oliver Upton <oupton@google.com> wrote:
>
> Hi Raghu,
>
> On Tue, Nov 02, 2021 at 12:21:56AM +0000, Raghavendra Rao Ananta wrote:
> > 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 <rananta@google.com>
> > ---
> >  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(-)
> >
> > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> > index 5ce26bedf23c..625f97f7b304 100644
> > --- a/arch/arm64/kvm/guest.c
> > +++ b/arch/arm64/kvm/guest.c
> > @@ -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>
> > diff --git a/arch/arm64/kvm/hypercalls.c b/arch/arm64/kvm/hypercalls.c
> > index 30da78f72b3b..d030939c5929 100644
> > --- a/arch/arm64/kvm/hypercalls.c
> > +++ b/arch/arm64/kvm/hypercalls.c
> > @@ -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)
> > +{
> > +     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.
>
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.

> > +#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;
> > +     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()"?

> > +             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;
> > +}
> > diff --git a/arch/arm64/kvm/psci.c b/arch/arm64/kvm/psci.c
> > index 74c47d420253..b9bcbc919b19 100644
> > --- a/arch/arm64/kvm/psci.c
> > +++ b/arch/arm64/kvm/psci.c
> > @@ -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;
> >  }
> > diff --git a/include/kvm/arm_hypercalls.h b/include/kvm/arm_hypercalls.h
> > index 0e2509d27910..5d38628a8d04 100644
> > --- a/include/kvm/arm_hypercalls.h
> > +++ b/include/kvm/arm_hypercalls.h
> > @@ -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
> > diff --git a/include/kvm/arm_psci.h b/include/kvm/arm_psci.h
> > index 5b58bd2fe088..eddbd7d805e9 100644
> > --- a/include/kvm/arm_psci.h
> > +++ b/include/kvm/arm_psci.h
> > @@ -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
> >

Re: [RFC PATCH 1/8] KVM: arm64: Factor out firmware register handling from psci.c

[Oliver Upton]

On Thu, Nov 04, 2021 at 10:16:21AM -0700, Raghavendra Rao Ananta wrote:
> Hi Oliver,
> 
> On Wed, Nov 3, 2021 at 2:43 PM Oliver Upton <oupton@google.com> wrote:
> >
> > Hi Raghu,
> >
> > On Tue, Nov 02, 2021 at 12:21:56AM +0000, Raghavendra Rao Ananta wrote:
> > > 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 <rananta@google.com>
> > > ---
> > >  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(-)
> > >
> > > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> > > index 5ce26bedf23c..625f97f7b304 100644
> > > --- a/arch/arm64/kvm/guest.c
> > > +++ b/arch/arm64/kvm/guest.c
> > > @@ -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>
> > > diff --git a/arch/arm64/kvm/hypercalls.c b/arch/arm64/kvm/hypercalls.c
> > > index 30da78f72b3b..d030939c5929 100644
> > > --- a/arch/arm64/kvm/hypercalls.c
> > > +++ b/arch/arm64/kvm/hypercalls.c
> > > @@ -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)
> > > +{
> > > +     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.
> >
> 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.
>

Perhaps the easiest thing to do would be to keep all firmware ID
registers in one place, much like we do for the ARM feature ID regs in
sys_regs.c.

> > > +#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;
> > > +     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()"?

Right :) Of course, this could become irrelevant depending on how you
address scoping of the FW regs.

--
Thanks,
Oliver

[RFC PATCH 2/8] KVM: arm64: Setup base for hypercall firmware registers

[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 <rananta@google.com>
---
 .../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%)

diff --git a/Documentation/virt/kvm/arm/psci.rst b/Documentation/virt/kvm/arm/hypercalls.rst
similarity index 81%
rename from Documentation/virt/kvm/arm/psci.rst
rename to Documentation/virt/kvm/arm/hypercalls.rst
index d52c2e83b5b8..85dfd682d811 100644
--- a/Documentation/virt/kvm/arm/psci.rst
+++ b/Documentation/virt/kvm/arm/hypercalls.rst
@@ -1,22 +1,19 @@
 .. SPDX-License-Identifier: GPL-2.0
 
-=========================================
-Power State Coordination Interface (PSCI)
-=========================================
+=======================
+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
+are made available to the guests if they make sense from a virtualization
 point of view.
 
 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
+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)
diff --git a/Documentation/virt/kvm/arm/index.rst b/Documentation/virt/kvm/arm/index.rst
index 78a9b670aafe..e84848432158 100644
--- a/Documentation/virt/kvm/arm/index.rst
+++ b/Documentation/virt/kvm/arm/index.rst
@@ -8,6 +8,6 @@ ARM
    :maxdepth: 2
 
    hyp-abi
-   psci
+   hypercalls
    pvtime
    ptp_kvm
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index d0221fb69a60..0b2502494a17 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -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 {
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index 24a1e86d7128..f9a25e439e99 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -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);
+
 	return ret;
 }
 
diff --git a/arch/arm64/kvm/hypercalls.c b/arch/arm64/kvm/hypercalls.c
index d030939c5929..7e873206a05b 100644
--- a/arch/arm64/kvm/hypercalls.c
+++ b/arch/arm64/kvm/hypercalls.c
@@ -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);
+
+	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;
+	}
+
+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);
-- 
2.33.1.1089.g2158813163f-goog

Re: [RFC PATCH 2/8] KVM: arm64: Setup base for hypercall firmware registers

[Oliver Upton]

On Tue, Nov 02, 2021 at 12:21:57AM +0000, Raghavendra Rao Ananta wrote:
> 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 <rananta@google.com>
> ---
>  .../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.

> diff --git a/Documentation/virt/kvm/arm/psci.rst b/Documentation/virt/kvm/arm/hypercalls.rst
> similarity index 81%
> rename from Documentation/virt/kvm/arm/psci.rst
> rename to Documentation/virt/kvm/arm/hypercalls.rst
> index d52c2e83b5b8..85dfd682d811 100644
> --- a/Documentation/virt/kvm/arm/psci.rst
> +++ b/Documentation/virt/kvm/arm/hypercalls.rst
> @@ -1,22 +1,19 @@
>  .. SPDX-License-Identifier: GPL-2.0
>  
> -=========================================
> -Power State Coordination Interface (PSCI)
> -=========================================
> +=======================
> +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.

> +are made available to the guests if they make sense from a virtualization
>  point of view.
>  
>  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

> +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)
> diff --git a/Documentation/virt/kvm/arm/index.rst b/Documentation/virt/kvm/arm/index.rst
> index 78a9b670aafe..e84848432158 100644
> --- a/Documentation/virt/kvm/arm/index.rst
> +++ b/Documentation/virt/kvm/arm/index.rst
> @@ -8,6 +8,6 @@ ARM
>     :maxdepth: 2
>  
>     hyp-abi
> -   psci
> +   hypercalls
>     pvtime
>     ptp_kvm
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index d0221fb69a60..0b2502494a17 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -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 {
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 24a1e86d7128..f9a25e439e99 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -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.

>  	return ret;
>  }
>  
> diff --git a/arch/arm64/kvm/hypercalls.c b/arch/arm64/kvm/hypercalls.c
> index d030939c5929..7e873206a05b 100644
> --- a/arch/arm64/kvm/hypercalls.c
> +++ b/arch/arm64/kvm/hypercalls.c
> @@ -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);
> +
> +	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.

> +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);
> -- 
> 2.33.1.1089.g2158813163f-goog
> 

Re: [RFC PATCH 2/8] KVM: arm64: Setup base for hypercall firmware registers

[Raghavendra Rao Ananta]

On Wed, Nov 3, 2021 at 3:18 PM Oliver Upton <oupton@google.com> wrote:
>
> On Tue, Nov 02, 2021 at 12:21:57AM +0000, Raghavendra Rao Ananta wrote:
> > 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 <rananta@google.com>
> > ---
> >  .../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.
>
> > diff --git a/Documentation/virt/kvm/arm/psci.rst b/Documentation/virt/kvm/arm/hypercalls.rst
> > similarity index 81%
> > rename from Documentation/virt/kvm/arm/psci.rst
> > rename to Documentation/virt/kvm/arm/hypercalls.rst
> > index d52c2e83b5b8..85dfd682d811 100644
> > --- a/Documentation/virt/kvm/arm/psci.rst
> > +++ b/Documentation/virt/kvm/arm/hypercalls.rst
> > @@ -1,22 +1,19 @@
> >  .. SPDX-License-Identifier: GPL-2.0
> >
> > -=========================================
> > -Power State Coordination Interface (PSCI)
> > -=========================================
> > +=======================
> > +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.
>
> > +are made available to the guests if they make sense from a virtualization
> >  point of view.
> >
> >  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
>
Sure, I can address your comments on this file. Thanks!
> > +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)
> > diff --git a/Documentation/virt/kvm/arm/index.rst b/Documentation/virt/kvm/arm/index.rst
> > index 78a9b670aafe..e84848432158 100644
> > --- a/Documentation/virt/kvm/arm/index.rst
> > +++ b/Documentation/virt/kvm/arm/index.rst
> > @@ -8,6 +8,6 @@ ARM
> >     :maxdepth: 2
> >
> >     hyp-abi
> > -   psci
> > +   hypercalls
> >     pvtime
> >     ptp_kvm
> > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > index d0221fb69a60..0b2502494a17 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -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 {
> > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> > index 24a1e86d7128..f9a25e439e99 100644
> > --- a/arch/arm64/kvm/arm.c
> > +++ b/arch/arm64/kvm/arm.c
> > @@ -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.
>
Sure, let me look into it..
> >       return ret;
> >  }
> >
> > diff --git a/arch/arm64/kvm/hypercalls.c b/arch/arm64/kvm/hypercalls.c
> > index d030939c5929..7e873206a05b 100644
> > --- a/arch/arm64/kvm/hypercalls.c
> > +++ b/arch/arm64/kvm/hypercalls.c
> > @@ -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);
> > +
> > +     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.
>
Sure, I guess there are some cases that I missed checking. Will try to
address them in the next patchset.

Regards,
Raghavendra
> > +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);
> > --
> > 2.33.1.1089.g2158813163f-goog
> >

[RFC PATCH 3/8] KVM: arm64: Add standard secure service calls firmware register

[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 <rananta@google.com>
---
 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(-)

diff --git a/Documentation/virt/kvm/arm/hypercalls.rst b/Documentation/virt/kvm/arm/hypercalls.rst
index 85dfd682d811..1601919f256d 100644
--- a/Documentation/virt/kvm/arm/hypercalls.rst
+++ b/Documentation/virt/kvm/arm/hypercalls.rst
@@ -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
+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.
 
+* 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:
+        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
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 0b2502494a17..176d6be7b4da 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -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 {
diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
index b3edde68bc3e..6387dea5396d 100644
--- a/arch/arm64/include/uapi/asm/kvm.h
+++ b/arch/arm64/include/uapi/asm/kvm.h
@@ -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,
+};
+
 /* SVE registers */
 #define KVM_REG_ARM64_SVE		(0x15 << KVM_REG_ARM_COPROC_SHIFT)
 
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index f9a25e439e99..1cf58aa49222 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -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:
diff --git a/arch/arm64/kvm/hypercalls.c b/arch/arm64/kvm/hypercalls.c
index 7e873206a05b..0b3006353bf6 100644
--- a/arch/arm64/kvm/hypercalls.c
+++ b/arch/arm64/kvm/hypercalls.c
@@ -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;
+	}
+
+	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);
+	}
+
+	/* 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;
 }
@@ -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,
 };
 
 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;
diff --git a/arch/arm64/kvm/trng.c b/arch/arm64/kvm/trng.c
index 99bdd7103c9c..6dff765f5b9b 100644
--- a/arch/arm64/kvm/trng.c
+++ b/arch/arm64/kvm/trng.c
@@ -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]),
diff --git a/include/kvm/arm_hypercalls.h b/include/kvm/arm_hypercalls.h
index 5d38628a8d04..5f01bb139312 100644
--- a/include/kvm/arm_hypercalls.h
+++ b/include/kvm/arm_hypercalls.h
@@ -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

Re: [RFC PATCH 3/8] KVM: arm64: Add standard secure service calls firmware register

[Oliver Upton]

On Tue, Nov 02, 2021 at 12:21:58AM +0000, Raghavendra Rao Ananta wrote:
> 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 <rananta@google.com>
> ---
>  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(-)
> 
> diff --git a/Documentation/virt/kvm/arm/hypercalls.rst b/Documentation/virt/kvm/arm/hypercalls.rst
> index 85dfd682d811..1601919f256d 100644
> --- a/Documentation/virt/kvm/arm/hypercalls.rst
> +++ b/Documentation/virt/kvm/arm/hypercalls.rst
> @@ -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.

> +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.
>  
> +* 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

> +        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
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 0b2502494a17..176d6be7b4da 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -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 {
> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> index b3edde68bc3e..6387dea5396d 100644
> --- a/arch/arm64/include/uapi/asm/kvm.h
> +++ b/arch/arm64/include/uapi/asm/kvm.h
> @@ -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.

>  /* SVE registers */
>  #define KVM_REG_ARM64_SVE		(0x15 << KVM_REG_ARM_COPROC_SHIFT)
>  
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index f9a25e439e99..1cf58aa49222 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -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:
> diff --git a/arch/arm64/kvm/hypercalls.c b/arch/arm64/kvm/hypercalls.c
> index 7e873206a05b..0b3006353bf6 100644
> --- a/arch/arm64/kvm/hypercalls.c
> +++ b/arch/arm64/kvm/hypercalls.c
> @@ -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;
> +	}
> +
> +	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.

> +
> +	/* 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;
>  }
> @@ -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

>  };
>  
>  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;
> diff --git a/arch/arm64/kvm/trng.c b/arch/arm64/kvm/trng.c
> index 99bdd7103c9c..6dff765f5b9b 100644
> --- a/arch/arm64/kvm/trng.c
> +++ b/arch/arm64/kvm/trng.c
> @@ -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]),
> diff --git a/include/kvm/arm_hypercalls.h b/include/kvm/arm_hypercalls.h
> index 5d38628a8d04..5f01bb139312 100644
> --- a/include/kvm/arm_hypercalls.h
> +++ b/include/kvm/arm_hypercalls.h
> @@ -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
> 

Re: [RFC PATCH 3/8] KVM: arm64: Add standard secure service calls firmware register

[Raghavendra Rao Ananta]

On Wed, Nov 3, 2021 at 5:15 PM Oliver Upton <oupton@google.com> wrote:
>
> On Tue, Nov 02, 2021 at 12:21:58AM +0000, Raghavendra Rao Ananta wrote:
> > 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 <rananta@google.com>
> > ---
> >  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(-)
> >
> > diff --git a/Documentation/virt/kvm/arm/hypercalls.rst b/Documentation/virt/kvm/arm/hypercalls.rst
> > index 85dfd682d811..1601919f256d 100644
> > --- a/Documentation/virt/kvm/arm/hypercalls.rst
> > +++ b/Documentation/virt/kvm/arm/hypercalls.rst
> > @@ -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.
>
> > +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.
> >
> > +* 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
>
I was afraid of the name getting too long. But let me see.
> > +        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
> > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > index 0b2502494a17..176d6be7b4da 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -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 {
> > diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> > index b3edde68bc3e..6387dea5396d 100644
> > --- a/arch/arm64/include/uapi/asm/kvm.h
> > +++ b/arch/arm64/include/uapi/asm/kvm.h
> > @@ -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.
>
Just curious, any particular reason for this? IMO, going an enum route
could avoid human errors. Anything I'm missing?
> >  /* SVE registers */
> >  #define KVM_REG_ARM64_SVE            (0x15 << KVM_REG_ARM_COPROC_SHIFT)
> >
> > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> > index f9a25e439e99..1cf58aa49222 100644
> > --- a/arch/arm64/kvm/arm.c
> > +++ b/arch/arm64/kvm/arm.c
> > @@ -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:
> > diff --git a/arch/arm64/kvm/hypercalls.c b/arch/arm64/kvm/hypercalls.c
> > index 7e873206a05b..0b3006353bf6 100644
> > --- a/arch/arm64/kvm/hypercalls.c
> > +++ b/arch/arm64/kvm/hypercalls.c
> > @@ -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;
> > +     }
> > +
> > +     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.
>
I think so.. I guess I was trying to avoid making too many changes if
we want to support a new func_id.
> > +
> > +     /* 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;
> >  }
> > @@ -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
>
Yes, of course. It's on my todo list. I'll try to include that in the
next patchset.
> >  };
> >
> >  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;
> > diff --git a/arch/arm64/kvm/trng.c b/arch/arm64/kvm/trng.c
> > index 99bdd7103c9c..6dff765f5b9b 100644
> > --- a/arch/arm64/kvm/trng.c
> > +++ b/arch/arm64/kvm/trng.c
> > @@ -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]),
> > diff --git a/include/kvm/arm_hypercalls.h b/include/kvm/arm_hypercalls.h
> > index 5d38628a8d04..5f01bb139312 100644
> > --- a/include/kvm/arm_hypercalls.h
> > +++ b/include/kvm/arm_hypercalls.h
> > @@ -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
> >

[RFC PATCH 4/8] KVM: arm64: Add standard hypervisor service calls firmware register

[Raghavendra Rao Ananta]

Introduce the firmware register to hold the standard hypervisor
service calls (owner value 5) as a bitmap. The bitmap represents
the features that'll be enabled for the guest, as configured by
the user-space. Currently, this includes support only for
Paravirtualized time, represented by bit-0.

Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
---
 Documentation/virt/kvm/arm/hypercalls.rst | 23 ++++++++++++++++-------
 arch/arm64/include/asm/kvm_host.h         |  1 +
 arch/arm64/include/uapi/asm/kvm.h         |  6 ++++++
 arch/arm64/kvm/arm.c                      |  1 +
 arch/arm64/kvm/hypercalls.c               | 22 ++++++++++++++++++++++
 arch/arm64/kvm/pvtime.c                   |  3 +++
 include/kvm/arm_hypercalls.h              |  3 +++
 7 files changed, 52 insertions(+), 7 deletions(-)

diff --git a/Documentation/virt/kvm/arm/hypercalls.rst b/Documentation/virt/kvm/arm/hypercalls.rst
index 1601919f256d..2cb82c694868 100644
--- a/Documentation/virt/kvm/arm/hypercalls.rst
+++ b/Documentation/virt/kvm/arm/hypercalls.rst
@@ -20,13 +20,13 @@ 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
-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 firmware registers, KVM_REG_ARM_STD and KVM_REG_ARM_STD_HYP exposes
+the hypercall services 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:
 
@@ -91,4 +91,13 @@ The following register is defined:
         The bit represents the services offered under v1.0 of ARM True Random Number Generator
         (TRNG) specification (ARM DEN 0098).
 
+* KVM_REG_ARM_STD_HYP
+    Controls the bitmap of the ARM Standard Hypervisor Service Calls.
+
+    The following bits are accepted:
+
+      KVM_REG_ARM_STD_HYP_PV_TIME_ST:
+        The bit represents the Paravirtualized Time service (also known as stolen time) as
+        represented by ARM DEN0057A.
+
 .. [1] https://developer.arm.com/-/media/developer/pdf/ARM_DEN_0070A_Firmware_interfaces_for_mitigating_CVE-2017-5715.pdf
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 176d6be7b4da..cee4f4b8a756 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -107,6 +107,7 @@ struct hvc_reg_desc {
 	bool write_attempted;
 
 	u64 kvm_std_bmap;
+	u64 kvm_std_hyp_bmap;
 };
 
 struct kvm_arch {
diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
index 6387dea5396d..46701da1a27d 100644
--- a/arch/arm64/include/uapi/asm/kvm.h
+++ b/arch/arm64/include/uapi/asm/kvm.h
@@ -287,6 +287,12 @@ enum kvm_reg_arm_std_bmap {
 	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,
+};
+
 /* SVE registers */
 #define KVM_REG_ARM64_SVE		(0x15 << KVM_REG_ARM_COPROC_SHIFT)
 
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index 1cf58aa49222..1c69d2a71b86 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -135,6 +135,7 @@ 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;
+	hvc_desc->kvm_std_hyp_bmap = ARM_SMCCC_STD_HYP_FEATURES;
 }
 
 /**
diff --git a/arch/arm64/kvm/hypercalls.c b/arch/arm64/kvm/hypercalls.c
index 0b3006353bf6..46064c515058 100644
--- a/arch/arm64/kvm/hypercalls.c
+++ b/arch/arm64/kvm/hypercalls.c
@@ -65,6 +65,8 @@ static u64 *kvm_fw_reg_to_bmap(struct kvm *kvm, u64 fw_reg)
 	switch (fw_reg) {
 	case KVM_REG_ARM_STD:
 		return &hvc_desc->kvm_std_bmap;
+	case KVM_REG_ARM_STD_HYP:
+		return &hvc_desc->kvm_std_hyp_bmap;
 	default:
 		return NULL;
 	}
@@ -87,6 +89,10 @@ static const struct kvm_hvc_func_map hvc_std_map[] = {
 	HVC_FUNC_MAP_DESC(ARM_SMCCC_TRNG_RND64, KVM_REG_ARM_STD_TRNG_V1_0),
 };
 
+static const struct kvm_hvc_func_map hvc_std_hyp_map[] = {
+	HVC_FUNC_MAP_DESC(ARM_SMCCC_HV_PV_TIME_ST, KVM_REG_ARM_STD_HYP_PV_TIME_ST),
+};
+
 bool kvm_hvc_call_supported(struct kvm_vcpu *vcpu, u32 func_id)
 {
 	struct kvm *kvm = vcpu->kvm;
@@ -102,6 +108,11 @@ bool kvm_hvc_call_supported(struct kvm_vcpu *vcpu, u32 func_id)
 		hvc_func_map = hvc_std_map;
 		map_sz = ARRAY_SIZE(hvc_std_map);
 		break;
+	case ARM_SMCCC_OWNER_STANDARD_HYP:
+		fw_reg = KVM_REG_ARM_STD_HYP;
+		hvc_func_map = hvc_std_hyp_map;
+		map_sz = ARRAY_SIZE(hvc_std_hyp_map);
+		break;
 	default:
 		/* Allow all the owners that aren't mapped */
 		return true;
@@ -218,6 +229,7 @@ static const u64 fw_reg_ids[] = {
 	KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1,
 	KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2,
 	KVM_REG_ARM_STD,
+	KVM_REG_ARM_STD_HYP,
 };
 
 int kvm_arm_get_fw_num_regs(struct kvm_vcpu *vcpu)
@@ -295,6 +307,9 @@ int kvm_arm_get_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 	case KVM_REG_ARM_STD:
 		val = hvc_desc->kvm_std_bmap;
 		break;
+	case KVM_REG_ARM_STD_HYP:
+		val = hvc_desc->kvm_std_hyp_bmap;
+		break;
 	default:
 		return -ENOENT;
 	}
@@ -424,6 +439,13 @@ int kvm_arm_set_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 
 		hvc_desc->kvm_std_bmap = val;
 		return 0;
+
+	case KVM_REG_ARM_STD_HYP:
+		if (val & ~ARM_SMCCC_STD_HYP_FEATURES)
+			return -EINVAL;
+
+		hvc_desc->kvm_std_hyp_bmap = val;
+		return 0;
 	default:
 		return -ENOENT;
 	}
diff --git a/arch/arm64/kvm/pvtime.c b/arch/arm64/kvm/pvtime.c
index 78a09f7a6637..4fa436dbd0b7 100644
--- a/arch/arm64/kvm/pvtime.c
+++ b/arch/arm64/kvm/pvtime.c
@@ -37,6 +37,9 @@ long kvm_hypercall_pv_features(struct kvm_vcpu *vcpu)
 	u32 feature = smccc_get_arg1(vcpu);
 	long val = SMCCC_RET_NOT_SUPPORTED;
 
+	if (!kvm_hvc_call_supported(vcpu, feature))
+		return val;
+
 	switch (feature) {
 	case ARM_SMCCC_HV_PV_TIME_FEATURES:
 	case ARM_SMCCC_HV_PV_TIME_ST:
diff --git a/include/kvm/arm_hypercalls.h b/include/kvm/arm_hypercalls.h
index 5f01bb139312..bbb3b12b10e3 100644
--- a/include/kvm/arm_hypercalls.h
+++ b/include/kvm/arm_hypercalls.h
@@ -9,6 +9,9 @@
 #define ARM_SMCCC_STD_FEATURES \
 	GENMASK_ULL(KVM_REG_ARM_STD_BMAP_MAX - 1, 0)
 
+#define ARM_SMCCC_STD_HYP_FEATURES \
+	GENMASK_ULL(KVM_REG_ARM_STD_HYP_BMAP_MAX - 1, 0)
+
 int kvm_hvc_call_handler(struct kvm_vcpu *vcpu);
 
 static inline u32 smccc_get_function(struct kvm_vcpu *vcpu)
-- 
2.33.1.1089.g2158813163f-goog

[RFC PATCH 5/8] KVM: arm64: Add vendor hypervisor service calls firmware register

[Raghavendra Rao Ananta]

Introduce the firmware register to hold the vendor specific
hypervisor service calls (owner value 6) as a bitmap. The
bitmap represents the features that'll be enabled for the
guest, as configured by the user-space. Currently, this
includes support only for Precision Time Protocol (PTP),
represented by bit-0.

Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
---
 Documentation/virt/kvm/arm/hypercalls.rst | 23 +++++++++++-----
 arch/arm64/include/asm/kvm_host.h         |  1 +
 arch/arm64/include/uapi/asm/kvm.h         |  6 +++++
 arch/arm64/kvm/arm.c                      |  1 +
 arch/arm64/kvm/hypercalls.c               | 33 ++++++++++++++++++++++-
 include/kvm/arm_hypercalls.h              |  3 +++
 6 files changed, 59 insertions(+), 8 deletions(-)

diff --git a/Documentation/virt/kvm/arm/hypercalls.rst b/Documentation/virt/kvm/arm/hypercalls.rst
index 2cb82c694868..61d95f4f1ddf 100644
--- a/Documentation/virt/kvm/arm/hypercalls.rst
+++ b/Documentation/virt/kvm/arm/hypercalls.rst
@@ -20,13 +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 registers, KVM_REG_ARM_STD and KVM_REG_ARM_STD_HYP exposes
-the hypercall services 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 firmware registers, KVM_REG_ARM_STD, KVM_REG_ARM_STD_HYP and
+KVM_REG_ARM_VENDOR_HYP exposes the hypercall services 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:
 
@@ -100,4 +101,12 @@ The following register is defined:
         The bit represents the Paravirtualized Time service (also known as stolen time) as
         represented by ARM DEN0057A.
 
+* KVM_REG_ARM_VENDOR_HYP
+    Controls the bitmap of the Vendor specific Hypervisor Service Calls.
+
+    The following bits are accepted:
+
+      KVM_REG_ARM_STD_HYP_PTP:
+        The bit represents the Precision Time Protocol KVM service.
+
 .. [1] https://developer.arm.com/-/media/developer/pdf/ARM_DEN_0070A_Firmware_interfaces_for_mitigating_CVE-2017-5715.pdf
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index cee4f4b8a756..27861b3bd25f 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -108,6 +108,7 @@ struct hvc_reg_desc {
 
 	u64 kvm_std_bmap;
 	u64 kvm_std_hyp_bmap;
+	u64 kvm_vendor_hyp_bmap;
 };
 
 struct kvm_arch {
diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
index 46701da1a27d..a1d0e8e69eed 100644
--- a/arch/arm64/include/uapi/asm/kvm.h
+++ b/arch/arm64/include/uapi/asm/kvm.h
@@ -293,6 +293,12 @@ enum kvm_reg_arm_std_hyp_bmap {
 	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)
 
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index 1c69d2a71b86..5c89db8336eb 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -136,6 +136,7 @@ static void set_default_hypercalls(struct kvm *kvm)
 
 	hvc_desc->kvm_std_bmap = ARM_SMCCC_STD_FEATURES;
 	hvc_desc->kvm_std_hyp_bmap = ARM_SMCCC_STD_HYP_FEATURES;
+	hvc_desc->kvm_vendor_hyp_bmap = ARM_SMCCC_VENDOR_HYP_FEATURES;
 }
 
 /**
diff --git a/arch/arm64/kvm/hypercalls.c b/arch/arm64/kvm/hypercalls.c
index 46064c515058..74ebe5355dc0 100644
--- a/arch/arm64/kvm/hypercalls.c
+++ b/arch/arm64/kvm/hypercalls.c
@@ -67,6 +67,8 @@ static u64 *kvm_fw_reg_to_bmap(struct kvm *kvm, u64 fw_reg)
 		return &hvc_desc->kvm_std_bmap;
 	case KVM_REG_ARM_STD_HYP:
 		return &hvc_desc->kvm_std_hyp_bmap;
+	case KVM_REG_ARM_VENDOR_HYP:
+		return &hvc_desc->kvm_vendor_hyp_bmap;
 	default:
 		return NULL;
 	}
@@ -93,6 +95,10 @@ static const struct kvm_hvc_func_map hvc_std_hyp_map[] = {
 	HVC_FUNC_MAP_DESC(ARM_SMCCC_HV_PV_TIME_ST, KVM_REG_ARM_STD_HYP_PV_TIME_ST),
 };
 
+static const struct kvm_hvc_func_map hvc_vendor_hyp_map[] = {
+	HVC_FUNC_MAP_DESC(ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID, KVM_REG_ARM_VENDOR_HYP_PTP),
+};
+
 bool kvm_hvc_call_supported(struct kvm_vcpu *vcpu, u32 func_id)
 {
 	struct kvm *kvm = vcpu->kvm;
@@ -113,6 +119,11 @@ bool kvm_hvc_call_supported(struct kvm_vcpu *vcpu, u32 func_id)
 		hvc_func_map = hvc_std_hyp_map;
 		map_sz = ARRAY_SIZE(hvc_std_hyp_map);
 		break;
+	case ARM_SMCCC_OWNER_VENDOR_HYP:
+		fw_reg = KVM_REG_ARM_VENDOR_HYP;
+		hvc_func_map = hvc_vendor_hyp_map;
+		map_sz = ARRAY_SIZE(hvc_vendor_hyp_map);
+		break;
 	default:
 		/* Allow all the owners that aren't mapped */
 		return true;
@@ -133,6 +144,7 @@ bool kvm_hvc_call_supported(struct kvm_vcpu *vcpu, u32 func_id)
 
 int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
 {
+	struct hvc_reg_desc *hvc_desc = &vcpu->kvm->arch.hvc_desc;
 	u32 func_id = smccc_get_function(vcpu);
 	u64 val[4] = {SMCCC_RET_NOT_SUPPORTED};
 	u32 feature;
@@ -204,7 +216,14 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
 		break;
 	case ARM_SMCCC_VENDOR_HYP_KVM_FEATURES_FUNC_ID:
 		val[0] = BIT(ARM_SMCCC_KVM_FUNC_FEATURES);
-		val[0] |= BIT(ARM_SMCCC_KVM_FUNC_PTP);
+
+		/*
+		 * The feature bits exposed to user-space doesn't include
+		 * ARM_SMCCC_KVM_FUNC_FEATURES. However, we expose this to
+		 * the guest as bit-0. Hence, left-shift the user-space
+		 * exposed bitmap by 1 to accommodate this.
+		 */
+		val[0] |= (hvc_desc->kvm_vendor_hyp_bmap << 1);
 		break;
 	case ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID:
 		kvm_ptp_get_time(vcpu, val);
@@ -230,6 +249,7 @@ static const u64 fw_reg_ids[] = {
 	KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2,
 	KVM_REG_ARM_STD,
 	KVM_REG_ARM_STD_HYP,
+	KVM_REG_ARM_VENDOR_HYP,
 };
 
 int kvm_arm_get_fw_num_regs(struct kvm_vcpu *vcpu)
@@ -310,6 +330,9 @@ int kvm_arm_get_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 	case KVM_REG_ARM_STD_HYP:
 		val = hvc_desc->kvm_std_hyp_bmap;
 		break;
+	case KVM_REG_ARM_VENDOR_HYP:
+		val = hvc_desc->kvm_vendor_hyp_bmap;
+		break;
 	default:
 		return -ENOENT;
 	}
@@ -446,6 +469,14 @@ int kvm_arm_set_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 
 		hvc_desc->kvm_std_hyp_bmap = val;
 		return 0;
+
+	case KVM_REG_ARM_VENDOR_HYP:
+		if (val & ~ARM_SMCCC_VENDOR_HYP_FEATURES)
+			return -EINVAL;
+
+		hvc_desc->kvm_vendor_hyp_bmap = val;
+		return 0;
+
 	default:
 		return -ENOENT;
 	}
diff --git a/include/kvm/arm_hypercalls.h b/include/kvm/arm_hypercalls.h
index bbb3b12b10e3..d8c17d161ee5 100644
--- a/include/kvm/arm_hypercalls.h
+++ b/include/kvm/arm_hypercalls.h
@@ -12,6 +12,9 @@
 #define ARM_SMCCC_STD_HYP_FEATURES \
 	GENMASK_ULL(KVM_REG_ARM_STD_HYP_BMAP_MAX - 1, 0)
 
+#define ARM_SMCCC_VENDOR_HYP_FEATURES \
+	GENMASK_ULL(KVM_REG_ARM_VENDOR_HYP_BMAP_MAX - 1, 0)
+
 int kvm_hvc_call_handler(struct kvm_vcpu *vcpu);
 
 static inline u32 smccc_get_function(struct kvm_vcpu *vcpu)
-- 
2.33.1.1089.g2158813163f-goog

[RFC PATCH 6/8] tools: Import the firmware registers

[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.

Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>

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

diff --git a/tools/arch/arm64/include/uapi/asm/kvm.h b/tools/arch/arm64/include/uapi/asm/kvm.h
index b3edde68bc3e..a1d0e8e69eed 100644
--- a/tools/arch/arm64/include/uapi/asm/kvm.h
+++ b/tools/arch/arm64/include/uapi/asm/kvm.h
@@ -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

Re: [RFC PATCH 6/8] tools: Import the firmware registers

[Oliver Upton]

On Tue, Nov 02, 2021 at 12:22:01AM +0000, Raghavendra Rao Ananta wrote:
> 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 <rananta@google.com>
> 
> ---
>  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

> diff --git a/tools/arch/arm64/include/uapi/asm/kvm.h b/tools/arch/arm64/include/uapi/asm/kvm.h
> index b3edde68bc3e..a1d0e8e69eed 100644
> --- a/tools/arch/arm64/include/uapi/asm/kvm.h
> +++ b/tools/arch/arm64/include/uapi/asm/kvm.h
> @@ -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
> 

Re: [RFC PATCH 6/8] tools: Import the firmware registers

[Raghavendra Rao Ananta]

On Wed, Nov 3, 2021 at 5:23 PM Oliver Upton <oupton@google.com> wrote:
>
> On Tue, Nov 02, 2021 at 12:22:01AM +0000, Raghavendra Rao Ananta wrote:
> > 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 <rananta@google.com>
> >
> > ---
> >  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?
>
I think we do. Wasn't aware of this. I'll delete the patch.

Regards,
Raghavendra
> --
> Oliver
>
> > diff --git a/tools/arch/arm64/include/uapi/asm/kvm.h b/tools/arch/arm64/include/uapi/asm/kvm.h
> > index b3edde68bc3e..a1d0e8e69eed 100644
> > --- a/tools/arch/arm64/include/uapi/asm/kvm.h
> > +++ b/tools/arch/arm64/include/uapi/asm/kvm.h
> > @@ -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
> >

[RFC PATCH 7/8] tools: Import ARM SMCCC definitions

[Raghavendra Rao Ananta]

Import the standard SMCCC definitions from include/linux/arm-smccc.h.

Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
---
 tools/include/linux/arm-smccc.h | 188 ++++++++++++++++++++++++++++++++
 1 file changed, 188 insertions(+)
 create mode 100644 tools/include/linux/arm-smccc.h

diff --git a/tools/include/linux/arm-smccc.h b/tools/include/linux/arm-smccc.h
new file mode 100644
index 000000000000..e3a24ae6863a
--- /dev/null
+++ b/tools/include/linux/arm-smccc.h
@@ -0,0 +1,188 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2015, Linaro Limited
+ */
+#ifndef __LINUX_ARM_SMCCC_H
+#define __LINUX_ARM_SMCCC_H
+
+#include <linux/const.h>
+
+/*
+ * This file provides common defines for ARM SMC Calling Convention as
+ * specified in
+ * https://developer.arm.com/docs/den0028/latest
+ *
+ * This code is up-to-date with version DEN 0028 C
+ */
+
+#define ARM_SMCCC_STD_CALL	        _AC(0,U)
+#define ARM_SMCCC_FAST_CALL	        _AC(1,U)
+#define ARM_SMCCC_TYPE_SHIFT		31
+
+#define ARM_SMCCC_SMC_32		0
+#define ARM_SMCCC_SMC_64		1
+#define ARM_SMCCC_CALL_CONV_SHIFT	30
+
+#define ARM_SMCCC_OWNER_MASK		0x3F
+#define ARM_SMCCC_OWNER_SHIFT		24
+
+#define ARM_SMCCC_FUNC_MASK		0xFFFF
+
+#define ARM_SMCCC_IS_FAST_CALL(smc_val)	\
+	((smc_val) & (ARM_SMCCC_FAST_CALL << ARM_SMCCC_TYPE_SHIFT))
+#define ARM_SMCCC_IS_64(smc_val) \
+	((smc_val) & (ARM_SMCCC_SMC_64 << ARM_SMCCC_CALL_CONV_SHIFT))
+#define ARM_SMCCC_FUNC_NUM(smc_val)	((smc_val) & ARM_SMCCC_FUNC_MASK)
+#define ARM_SMCCC_OWNER_NUM(smc_val) \
+	(((smc_val) >> ARM_SMCCC_OWNER_SHIFT) & ARM_SMCCC_OWNER_MASK)
+
+#define ARM_SMCCC_CALL_VAL(type, calling_convention, owner, func_num) \
+	(((type) << ARM_SMCCC_TYPE_SHIFT) | \
+	((calling_convention) << ARM_SMCCC_CALL_CONV_SHIFT) | \
+	(((owner) & ARM_SMCCC_OWNER_MASK) << ARM_SMCCC_OWNER_SHIFT) | \
+	((func_num) & ARM_SMCCC_FUNC_MASK))
+
+#define ARM_SMCCC_OWNER_ARCH		0
+#define ARM_SMCCC_OWNER_CPU		1
+#define ARM_SMCCC_OWNER_SIP		2
+#define ARM_SMCCC_OWNER_OEM		3
+#define ARM_SMCCC_OWNER_STANDARD	4
+#define ARM_SMCCC_OWNER_STANDARD_HYP	5
+#define ARM_SMCCC_OWNER_VENDOR_HYP	6
+#define ARM_SMCCC_OWNER_TRUSTED_APP	48
+#define ARM_SMCCC_OWNER_TRUSTED_APP_END	49
+#define ARM_SMCCC_OWNER_TRUSTED_OS	50
+#define ARM_SMCCC_OWNER_TRUSTED_OS_END	63
+
+#define ARM_SMCCC_FUNC_QUERY_CALL_UID  0xff01
+
+#define ARM_SMCCC_QUIRK_NONE		0
+#define ARM_SMCCC_QUIRK_QCOM_A6		1 /* Save/restore register a6 */
+
+#define ARM_SMCCC_VERSION_1_0		0x10000
+#define ARM_SMCCC_VERSION_1_1		0x10001
+#define ARM_SMCCC_VERSION_1_2		0x10002
+#define ARM_SMCCC_VERSION_1_3		0x10003
+
+#define ARM_SMCCC_1_3_SVE_HINT		0x10000
+
+#define ARM_SMCCC_VERSION_FUNC_ID					\
+	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,				\
+			   ARM_SMCCC_SMC_32,				\
+			   0, 0)
+
+#define ARM_SMCCC_ARCH_FEATURES_FUNC_ID					\
+	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,				\
+			   ARM_SMCCC_SMC_32,				\
+			   0, 1)
+
+#define ARM_SMCCC_ARCH_SOC_ID						\
+	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,				\
+			   ARM_SMCCC_SMC_32,				\
+			   0, 2)
+
+#define ARM_SMCCC_ARCH_WORKAROUND_1					\
+	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,				\
+			   ARM_SMCCC_SMC_32,				\
+			   0, 0x8000)
+
+#define ARM_SMCCC_ARCH_WORKAROUND_2					\
+	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,				\
+			   ARM_SMCCC_SMC_32,				\
+			   0, 0x7fff)
+
+#define ARM_SMCCC_VENDOR_HYP_CALL_UID_FUNC_ID				\
+	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,				\
+			   ARM_SMCCC_SMC_32,				\
+			   ARM_SMCCC_OWNER_VENDOR_HYP,			\
+			   ARM_SMCCC_FUNC_QUERY_CALL_UID)
+
+/* KVM UID value: 28b46fb6-2ec5-11e9-a9ca-4b564d003a74 */
+#define ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_0	0xb66fb428U
+#define ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_1	0xe911c52eU
+#define ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_2	0x564bcaa9U
+#define ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_3	0x743a004dU
+
+/* KVM "vendor specific" services */
+#define ARM_SMCCC_KVM_FUNC_FEATURES		0
+#define ARM_SMCCC_KVM_FUNC_PTP			1
+#define ARM_SMCCC_KVM_FUNC_FEATURES_2		127
+#define ARM_SMCCC_KVM_NUM_FUNCS			128
+
+#define ARM_SMCCC_VENDOR_HYP_KVM_FEATURES_FUNC_ID			\
+	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,				\
+			   ARM_SMCCC_SMC_32,				\
+			   ARM_SMCCC_OWNER_VENDOR_HYP,			\
+			   ARM_SMCCC_KVM_FUNC_FEATURES)
+
+#define SMCCC_ARCH_WORKAROUND_RET_UNAFFECTED	1
+
+/*
+ * ptp_kvm is a feature used for time sync between vm and host.
+ * ptp_kvm module in guest kernel will get service from host using
+ * this hypercall ID.
+ */
+#define ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID				\
+	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,				\
+			   ARM_SMCCC_SMC_32,				\
+			   ARM_SMCCC_OWNER_VENDOR_HYP,			\
+			   ARM_SMCCC_KVM_FUNC_PTP)
+
+/* ptp_kvm counter type ID */
+#define KVM_PTP_VIRT_COUNTER			0
+#define KVM_PTP_PHYS_COUNTER			1
+
+/* Paravirtualised time calls (defined by ARM DEN0057A) */
+#define ARM_SMCCC_HV_PV_TIME_FEATURES				\
+	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,			\
+			   ARM_SMCCC_SMC_64,			\
+			   ARM_SMCCC_OWNER_STANDARD_HYP,	\
+			   0x20)
+
+#define ARM_SMCCC_HV_PV_TIME_ST					\
+	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,			\
+			   ARM_SMCCC_SMC_64,			\
+			   ARM_SMCCC_OWNER_STANDARD_HYP,	\
+			   0x21)
+
+/* TRNG entropy source calls (defined by ARM DEN0098) */
+#define ARM_SMCCC_TRNG_VERSION					\
+	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,			\
+			   ARM_SMCCC_SMC_32,			\
+			   ARM_SMCCC_OWNER_STANDARD,		\
+			   0x50)
+
+#define ARM_SMCCC_TRNG_FEATURES					\
+	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,			\
+			   ARM_SMCCC_SMC_32,			\
+			   ARM_SMCCC_OWNER_STANDARD,		\
+			   0x51)
+
+#define ARM_SMCCC_TRNG_GET_UUID					\
+	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,			\
+			   ARM_SMCCC_SMC_32,			\
+			   ARM_SMCCC_OWNER_STANDARD,		\
+			   0x52)
+
+#define ARM_SMCCC_TRNG_RND32					\
+	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,			\
+			   ARM_SMCCC_SMC_32,			\
+			   ARM_SMCCC_OWNER_STANDARD,		\
+			   0x53)
+
+#define ARM_SMCCC_TRNG_RND64					\
+	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,			\
+			   ARM_SMCCC_SMC_64,			\
+			   ARM_SMCCC_OWNER_STANDARD,		\
+			   0x53)
+
+/*
+ * Return codes defined in ARM DEN 0070A
+ * ARM DEN 0070A is now merged/consolidated into ARM DEN 0028 C
+ */
+#define SMCCC_RET_SUCCESS			0
+#define SMCCC_RET_NOT_SUPPORTED			-1
+#define SMCCC_RET_NOT_REQUIRED			-2
+#define SMCCC_RET_INVALID_PARAMETER		-3
+
+#endif /*__LINUX_ARM_SMCCC_H*/
\ No newline at end of file
-- 
2.33.1.1089.g2158813163f-goog

[RFC PATCH 8/8] selftests: KVM: aarch64: Introduce hypercall ABI test

[Raghavendra Rao Ananta]

Introduce a KVM selftest to check the hypercall interface
for arm64 platforms. The test validates the user-space's
IOCTL interface to read/write the psuedo-firmware registers
as well as its effects on the guest upon certain configurations.

Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
---
 tools/testing/selftests/kvm/.gitignore        |   1 +
 tools/testing/selftests/kvm/Makefile          |   1 +
 .../selftests/kvm/aarch64/hypercalls.c        | 340 ++++++++++++++++++
 3 files changed, 342 insertions(+)
 create mode 100644 tools/testing/selftests/kvm/aarch64/hypercalls.c

diff --git a/tools/testing/selftests/kvm/.gitignore b/tools/testing/selftests/kvm/.gitignore
index 100d9a3a37f6..b56532fc3678 100644
--- a/tools/testing/selftests/kvm/.gitignore
+++ b/tools/testing/selftests/kvm/.gitignore
@@ -2,6 +2,7 @@
 /aarch64/arch_timer
 /aarch64/debug-exceptions
 /aarch64/get-reg-list
+/aarch64/hypercalls
 /aarch64/psci_test
 /aarch64/vgic_init
 /s390x/memop
diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index 8bcc8d1f226e..4b4aa75663bd 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -89,6 +89,7 @@ TEST_GEN_PROGS_x86_64 += kvm_binary_stats_test
 TEST_GEN_PROGS_aarch64 += aarch64/arch_timer
 TEST_GEN_PROGS_aarch64 += aarch64/debug-exceptions
 TEST_GEN_PROGS_aarch64 += aarch64/get-reg-list
+TEST_GEN_PROGS_aarch64 += aarch64/hypercalls
 TEST_GEN_PROGS_aarch64 += aarch64/psci_test
 TEST_GEN_PROGS_aarch64 += aarch64/vgic_init
 TEST_GEN_PROGS_aarch64 += demand_paging_test
diff --git a/tools/testing/selftests/kvm/aarch64/hypercalls.c b/tools/testing/selftests/kvm/aarch64/hypercalls.c
new file mode 100644
index 000000000000..fa422769fcda
--- /dev/null
+++ b/tools/testing/selftests/kvm/aarch64/hypercalls.c
@@ -0,0 +1,340 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <errno.h>
+#include <linux/arm-smccc.h>
+#include <asm/kvm.h>
+#include <kvm_util.h>
+
+#include "processor.h"
+
+#define FW_REG_ULIMIT_VAL(max_feat_bit) (GENMASK_ULL(max_feat_bit, 0))
+
+struct kvm_fw_reg_info {
+	uint64_t reg;		/* Register definition */
+	uint64_t max_feat_bit;	/* Bit that represents the upper limit of the feature-map */
+};
+
+#define FW_REG_INFO(r, bit_max)			\
+	{					\
+		.reg = r,			\
+		.max_feat_bit = bit_max,	\
+	}
+
+static const struct kvm_fw_reg_info fw_reg_info[] = {
+	FW_REG_INFO(KVM_REG_ARM_STD, KVM_REG_ARM_STD_BMAP_MAX - 1),
+	FW_REG_INFO(KVM_REG_ARM_STD_HYP, KVM_REG_ARM_STD_HYP_BMAP_MAX - 1),
+	FW_REG_INFO(KVM_REG_ARM_VENDOR_HYP, KVM_REG_ARM_VENDOR_HYP_BMAP_MAX - 1),
+};
+
+enum test_stage {
+	TEST_STAGE_REG_IFACE,
+	TEST_STAGE_HVC_IFACE_FEAT_DISABLED,
+	TEST_STAGE_HVC_IFACE_FEAT_ENABLED,
+	TEST_STAGE_END,
+};
+
+static int stage;
+
+struct test_hvc_info {
+	uint32_t func_id;
+	int64_t arg0;
+
+	void (*test_hvc_disabled)(const struct test_hvc_info *hc_info,
+					struct arm_smccc_res *res);
+	void (*test_hvc_enabled)(const struct test_hvc_info *hc_info,
+					struct arm_smccc_res *res);
+};
+
+#define TEST_HVC_INFO(f, a0, test_disabled, test_enabled)	\
+	{							\
+		.func_id = f,					\
+		.arg0 = a0,					\
+		.test_hvc_disabled = test_disabled,		\
+		.test_hvc_enabled = test_enabled,		\
+	}
+
+static void
+test_ptp_feat_hvc_disabled(const struct test_hvc_info *hc_info, struct arm_smccc_res *res)
+{
+	GUEST_ASSERT_3((res->a0 & BIT(ARM_SMCCC_KVM_FUNC_PTP)) == 0,
+			res->a0, hc_info->func_id, hc_info->arg0);
+}
+
+static void
+test_ptp_feat_hvc_enabled(const struct test_hvc_info *hc_info, struct arm_smccc_res *res)
+{
+	GUEST_ASSERT_3((res->a0 & BIT(ARM_SMCCC_KVM_FUNC_PTP)) != 0,
+			res->a0, hc_info->func_id, hc_info->arg0);
+}
+
+static const struct test_hvc_info hvc_info[] = {
+	/* KVM_REG_ARM_STD: KVM_REG_ARM_STD_TRNG_V1_0 */
+	TEST_HVC_INFO(ARM_SMCCC_TRNG_FEATURES, ARM_SMCCC_TRNG_RND64, NULL, NULL),
+	TEST_HVC_INFO(ARM_SMCCC_TRNG_GET_UUID, 0, NULL, NULL),
+	TEST_HVC_INFO(ARM_SMCCC_TRNG_RND32, 0, NULL, NULL),
+	TEST_HVC_INFO(ARM_SMCCC_TRNG_RND64, 0, NULL, NULL),
+
+	/* KVM_REG_ARM_STD_HYP: PV_TIME */
+	TEST_HVC_INFO(ARM_SMCCC_HV_PV_TIME_FEATURES, ARM_SMCCC_HV_PV_TIME_ST, NULL, NULL),
+	TEST_HVC_INFO(ARM_SMCCC_HV_PV_TIME_ST, 0, NULL, NULL),
+
+	/* KVM_REG_ARM_VENDOR_HYP: PTP */
+	TEST_HVC_INFO(ARM_SMCCC_VENDOR_HYP_KVM_FEATURES_FUNC_ID,
+			ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID,
+			test_ptp_feat_hvc_disabled, test_ptp_feat_hvc_enabled),
+	TEST_HVC_INFO(ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID,
+			KVM_PTP_VIRT_COUNTER, NULL, NULL),
+};
+
+static void guest_test_hvc(int stage)
+{
+	unsigned int i;
+	struct arm_smccc_res res;
+
+	for (i = 0; i < ARRAY_SIZE(hvc_info); i++) {
+		const struct test_hvc_info *hc_info  = &hvc_info[i];
+
+		memset(&res, 0, sizeof(res));
+		smccc_hvc(hc_info->func_id, hc_info->arg0, 0, 0, 0, 0, 0, 0, &res);
+
+		switch (stage) {
+		case TEST_STAGE_HVC_IFACE_FEAT_DISABLED:
+			if (hc_info->test_hvc_disabled)
+				hc_info->test_hvc_disabled(hc_info, &res);
+			else
+				GUEST_ASSERT_3(res.a0 == SMCCC_RET_NOT_SUPPORTED,
+					res.a0, hc_info->func_id, hc_info->arg0);
+			break;
+		case TEST_STAGE_HVC_IFACE_FEAT_ENABLED:
+			if (hc_info->test_hvc_enabled)
+				hc_info->test_hvc_enabled(hc_info, &res);
+			else
+				GUEST_ASSERT_3(res.a0 != SMCCC_RET_NOT_SUPPORTED,
+					res.a0, hc_info->func_id, hc_info->arg0);
+			break;
+		default:
+			GUEST_ASSERT_1(0, stage);
+		}
+	}
+}
+
+static void guest_code(void)
+{
+	while (stage != TEST_STAGE_END) {
+		switch (stage) {
+		case TEST_STAGE_REG_IFACE:
+			break;
+		case TEST_STAGE_HVC_IFACE_FEAT_DISABLED:
+		case TEST_STAGE_HVC_IFACE_FEAT_ENABLED:
+			guest_test_hvc(stage);
+			break;
+		default:
+			GUEST_ASSERT_1(0, stage);
+		}
+
+		GUEST_SYNC(stage);
+	}
+
+	GUEST_DONE();
+}
+
+static int set_fw_reg(struct kvm_vm *vm, uint64_t id, uint64_t val)
+{
+	struct kvm_one_reg reg = {
+		.id = KVM_REG_ARM_FW_REG(id),
+		.addr = (uint64_t)&val,
+	};
+
+	return _vcpu_ioctl(vm, 0, KVM_SET_ONE_REG, &reg);
+}
+
+static void get_fw_reg(struct kvm_vm *vm, uint64_t id, uint64_t *addr)
+{
+	struct kvm_one_reg reg = {
+		.id = KVM_REG_ARM_FW_REG(id),
+		.addr = (uint64_t)addr,
+	};
+
+	return vcpu_ioctl(vm, 0, KVM_GET_ONE_REG, &reg);
+}
+
+struct st_time {
+	uint32_t rev;
+	uint32_t attr;
+	uint64_t st_time;
+};
+
+#define STEAL_TIME_SIZE		((sizeof(struct st_time) + 63) & ~63)
+#define ST_GPA_BASE		(1 << 30)
+
+static void steal_time_init(struct kvm_vm *vm)
+{
+	uint64_t st_ipa = (ulong)ST_GPA_BASE;
+	unsigned int gpages;
+	struct kvm_device_attr dev = {
+		.group = KVM_ARM_VCPU_PVTIME_CTRL,
+		.attr = KVM_ARM_VCPU_PVTIME_IPA,
+		.addr = (uint64_t)&st_ipa,
+	};
+
+	gpages = vm_calc_num_guest_pages(VM_MODE_DEFAULT, STEAL_TIME_SIZE);
+	vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS, ST_GPA_BASE, 1, gpages, 0);
+
+	vcpu_ioctl(vm, 0, KVM_SET_DEVICE_ATTR, &dev);
+}
+
+static void test_fw_regs_before_vm_start(struct kvm_vm *vm)
+{
+	uint64_t val;
+	unsigned int i;
+	int ret;
+
+	for (i = 0; i < ARRAY_SIZE(fw_reg_info); i++) {
+		const struct kvm_fw_reg_info *reg_info = &fw_reg_info[i];
+
+		/* First read should be an upper limit of the features supported */
+		get_fw_reg(vm, reg_info->reg, &val);
+		TEST_ASSERT(val == FW_REG_ULIMIT_VAL(reg_info->max_feat_bit),
+			"Expected all the features to be set for reg: 0x%lx; expected: 0x%llx; read: 0x%lx\n",
+			reg_info->reg, GENMASK_ULL(reg_info->max_feat_bit, 0), val);
+	}
+
+	for (i = 0; i < ARRAY_SIZE(fw_reg_info); i++) {
+		const struct kvm_fw_reg_info *reg_info = &fw_reg_info[i];
+
+		/* Test disabling all the features of the register map */
+		ret = set_fw_reg(vm, reg_info->reg, 0);
+		TEST_ASSERT(ret == 0,
+			"Failed to clear all the features of reg: 0x%lx; ret: %d\n",
+			reg_info->reg, errno);
+
+		get_fw_reg(vm, reg_info->reg, &val);
+		TEST_ASSERT(val == 0,
+			"Expected all the features to be cleared for reg: 0x%lx\n", reg_info->reg);
+
+		/* Test enabling a feature that's not supported. Avoid this
+		 * check if all the bits are occupied.
+		 */
+		if (reg_info->max_feat_bit < 63) {
+			ret = set_fw_reg(vm, reg_info->reg, BIT(reg_info->max_feat_bit + 1));
+			TEST_ASSERT(ret != 0 && errno == EINVAL,
+			"Unexpected behavior or return value (%d) while setting an unsupported feature for reg: 0x%lx\n",
+			errno, reg_info->reg);
+		}
+	}
+}
+
+static void test_fw_regs_after_vm_start(struct kvm_vm *vm)
+{
+	uint64_t val;
+	unsigned int i;
+	int ret;
+
+	for (i = 0; i < ARRAY_SIZE(fw_reg_info); i++) {
+		const struct kvm_fw_reg_info *reg_info = &fw_reg_info[i];
+
+		/* Before starting the VM, the test clears all the bits.
+		 * Check if that's still the case.
+		 */
+		get_fw_reg(vm, reg_info->reg, &val);
+		TEST_ASSERT(val == 0,
+			"Expected all the features to be cleared for reg: 0x%lx\n",
+			reg_info->reg);
+
+		/* Test setting the last known value. KVM should allow this
+		 * even if VM has started running.
+		 */
+		ret = set_fw_reg(vm, reg_info->reg, 0);
+		TEST_ASSERT(ret == 0,
+			"Failed to clear all the features of reg: 0x%lx; ret: %d\n",
+			reg_info->reg, errno);
+
+		/* Set all the features for this register again. KVM shouldn't
+		 * allow this as the VM is running.
+		 */
+		ret = set_fw_reg(vm, reg_info->reg, FW_REG_ULIMIT_VAL(reg_info->max_feat_bit));
+		TEST_ASSERT(ret != 0 && errno == EBUSY,
+		"Unexpected behavior or return value (%d) while setting a feature while VM is running for reg: 0x%lx\n",
+		errno, reg_info->reg);
+	}
+}
+
+static struct  kvm_vm *test_vm_create(void)
+{
+	struct kvm_vm *vm;
+
+	vm = vm_create_default(0, 0, guest_code);
+
+	ucall_init(vm, NULL);
+	steal_time_init(vm);
+
+	return vm;
+}
+
+static struct kvm_vm *test_guest_stage(struct kvm_vm *vm)
+{
+	struct kvm_vm *ret_vm = vm;
+
+	pr_debug("Stage: %d\n", stage);
+
+	switch (stage) {
+	case TEST_STAGE_REG_IFACE:
+		test_fw_regs_after_vm_start(vm);
+		break;
+	case TEST_STAGE_HVC_IFACE_FEAT_DISABLED:
+		/* Start a new VM so that all the features are now enabled by default */
+		kvm_vm_free(vm);
+		ret_vm = test_vm_create();
+		break;
+	case TEST_STAGE_HVC_IFACE_FEAT_ENABLED:
+		break;
+	default:
+		TEST_FAIL("Unknown test stage: %d\n", stage);
+	}
+
+	stage++;
+	sync_global_to_guest(vm, stage);
+
+	return ret_vm;
+}
+
+static void test_run(void)
+{
+	struct kvm_vm *vm;
+	struct ucall uc;
+	bool guest_done = false;
+
+	vm = test_vm_create();
+
+	test_fw_regs_before_vm_start(vm);
+
+	while (!guest_done) {
+		vcpu_run(vm, 0);
+
+		switch (get_ucall(vm, 0, &uc)) {
+		case UCALL_SYNC:
+			vm = test_guest_stage(vm);
+			break;
+		case UCALL_DONE:
+			guest_done = true;
+			break;
+		case UCALL_ABORT:
+			TEST_FAIL("%s at %s:%ld\n\tvalues: 0x%lx, %lu; %lu, stage: %u",
+			(const char *)uc.args[0], __FILE__, uc.args[1],
+			uc.args[2], uc.args[3], uc.args[4], stage);
+			break;
+		default:
+			TEST_FAIL("Unexpected guest exit\n");
+		}
+	}
+
+	kvm_vm_free(vm);
+}
+
+int main(void)
+{
+	setbuf(stdout, NULL);
+
+	test_run();
+	return 0;
+}
-- 
2.33.1.1089.g2158813163f-goog