KVM: arm64: Add support for hypercall services selection

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

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

No functional change intended.

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

Reviewed-by Oliver Upton <[email protected]>

Reviewed-by Gavin Shan <[email protected]>

---
 arch/arm64/kvm/guest.c       |   2 +-
 arch/arm64/kvm/hypercalls.c  | 185 +++++++++++++++++++++++++++++++++++
 arch/arm64/kvm/psci.c        | 183 ----------------------------------
 include/kvm/arm_hypercalls.h |   7 ++
 include/kvm/arm_psci.h       |   7 --
 5 files changed, 193 insertions(+), 191 deletions(-)

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

index 7e15b03fbdf8..0d5cca56cbda 100644
@@ -18,7 +18,7 @@
 #include <linux/string.h>
 #include <linux/vmalloc.h>
 #include <linux/fs.h>
-#include <kvm/arm_psci.h>
+#include <kvm/arm_hypercalls.h>
 #include <asm/cputype.h>
 #include <linux/uaccess.h>
 #include <asm/fpsimd.h>

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

index 202b8c455724..fa6d9378d8e7 100644
@@ -158,3 +158,188 @@ 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 kvm_arm_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_SMCCC_ARCH_WORKAROUND_3,
+};
+
+int kvm_arm_get_fw_num_regs(struct kvm_vcpu *vcpu)
+{
+	return ARRAY_SIZE(kvm_arm_fw_reg_ids);
+}
+
+int kvm_arm_copy_fw_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
+{
+	int i;
+

KVM regularly introduces new hypercall services to the guests without any consent from the userspace. This means, the guests can observe hypercall services in and out as they migrate across various host kernel versions. This could be a major problem if the guest discovered a hypercall, started using it, and after getting migrated to an older kernel realizes that it's no longer available. Depending on how the guest handles the change, there's a potential chance that the guest would just panic.

As a result, there's a need for the userspace to elect the services that it wishes the guest to discover. It can elect these services based on the kernels spread across its (migration) fleet. To remedy this, extend the existing firmware pseudo-registers, such as KVM_REG_ARM_PSCI_VERSION, but by creating a new COPROC register space for all the hypercall services available.

These firmware registers are categorized based on the service call owners, but unlike the existing firmware pseudo-registers, they hold the features supported in the form of a bitmap.

During the VM initialization, the registers are set to upper-limit of the features supported by the corresponding registers. It's expected that the VMMs discover the features provided by each register via GET_ONE_REG, and write back the desired values using SET_ONE_REG. KVM allows this modification only until the VM has started.

Some of the standard features are not mapped to any bits of the registers. But since they can recreate the original problem of making it available without userspace's consent, they need to be explicitly added to the case-list in kvm_hvc_call_default_allowed(). Any function-id that's not enabled via the bitmap, or not listed in kvm_hvc_call_default_allowed, will be returned as SMCCC_RET_NOT_SUPPORTED to the guest.

Older userspace code can simply ignore the feature and the hypercall services will be exposed unconditionally to the guests, thus ensuring backward compatibility.

In this patch, the framework adds the register only for ARM's standard secure services (owner value 4). 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. Other services are momentarily added in the upcoming patches.

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

Reviewed-by Gavin Shan <[email protected]>

---
 arch/arm64/include/asm/kvm_host.h |  12 ++++
 arch/arm64/include/uapi/asm/kvm.h |   9 +++
 arch/arm64/kvm/arm.c              |   1 +
 arch/arm64/kvm/guest.c            |   8 ++-
 arch/arm64/kvm/hypercalls.c       | 106 ++++++++++++++++++++++++++++++
 arch/arm64/kvm/psci.c             |  13 ++++
 include/kvm/arm_hypercalls.h      |   6 ++
 include/kvm/arm_psci.h            |   2 +-
 8 files changed, 154 insertions(+), 3 deletions(-)

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

index 94a27a7520f4..fef597af0beb 100644
@@ -101,6 +101,15 @@ struct kvm_s2_mmu {
 struct kvm_arch_memory_slot {
 };
 
+/**
+ * struct kvm_smccc_features: Descriptor of the hypercall services exposed to the guests
+ *
+ * @std_bmap: Bitmap of standard secure service calls
+ */
+struct kvm_smccc_features {
+	unsigned long std_bmap;
+};
+
 struct kvm_arch {
 	struct kvm_s2_mmu mmu;
 

@@ -150,6 +159,9 @@ struct kvm_arch {
 
 	u8 pfr0_csv2;
 	u8 pfr0_csv3;
+
+	/* Hypercall features firmware registers' descriptor */
+	struct kvm_smccc_features smccc_feat;
 };
 
 struct kvm_vcpu_fault_info {

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

index c1b6ddc02d2f..0b79d2dc6ffd 100644
@@ -332,6 +332,15 @@ struct kvm_arm_copy_mte_tags {
 #define KVM_ARM64_SVE_VLS_WORDS	\
 	((KVM_ARM64_SVE_VQ_MAX - KVM_ARM64_SVE_VQ_MIN) / 64 + 1)
 
+/* Bitmap feature firmware registers */
+#define KVM_REG_ARM_FW_FEAT_BMAP		(0x0016 << KVM_REG_ARM_COPROC_SHIFT)
+#define KVM_REG_ARM_FW_FEAT_BMAP_REG(r)		(KVM_REG_ARM64 | KVM_REG_SIZE_U64 | \
+						KVM_REG_ARM_FW_FEAT_BMAP |	\
+						((r) & 0xffff))
+
+#define KVM_REG_ARM_STD_BMAP			KVM_REG_ARM_FW_FEAT_BMAP_REG(0)
+#define KVM_REG_ARM_STD_BIT_TRNG_V1_0		0
+
 /* Device Control API: ARM VGIC */
 #define KVM_DEV_ARM_VGIC_GRP_ADDR	0
 #define KVM_DEV_ARM_VGIC_GRP_DIST_REGS	1

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

index 523bc934fe2f..a37fadbd617e 100644
@@ -156,6 +156,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);
+	kvm_arm_init_hypercalls(kvm);
 
 	return ret;
 out_free_stage2_pgd:

arch/arm64/kvm/guest.c +6 -2

index 0d5cca56cbda..8c607199cad1 100644
@@ -756,7 +756,9 @@ int kvm_arm_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 
 	switch (reg->id & KVM_REG_ARM_COPROC_MASK) {
 	case KVM_REG_ARM_CORE:	return get_core_reg(vcpu, reg);
-	case KVM_REG_ARM_FW:	return kvm_arm_get_fw_reg(vcpu, reg);
+	case KVM_REG_ARM_FW:
+	case KVM_REG_ARM_FW_FEAT_BMAP:
+		return kvm_arm_get_fw_reg(vcpu, reg);
 	case KVM_REG_ARM64_SVE:	return get_sve_reg(vcpu, reg);
 	}
 

@@ -774,7 +776,9 @@ int kvm_arm_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 
 	switch (reg->id & KVM_REG_ARM_COPROC_MASK) {
 	case KVM_REG_ARM_CORE:	return set_core_reg(vcpu, reg);
-	case KVM_REG_ARM_FW:	return kvm_arm_set_fw_reg(vcpu, reg);
+	case KVM_REG_ARM_FW:
+	case KVM_REG_ARM_FW_FEAT_BMAP:
+		return kvm_arm_set_fw_reg(vcpu, reg);
 	case KVM_REG_ARM64_SVE:	return set_sve_reg(vcpu, reg);
 	}
 

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

index fa6d9378d8e7..5c53fd1f5690 100644

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

+
+static bool kvm_hvc_call_default_allowed(u32 func_id)
+{
+	switch (func_id) {
+	/*
+	 * List of function-ids that are not gated with the bitmapped feature
+	 * firmware registers, and are to be allowed for servicing the call by default.
+	 */
+	case ARM_SMCCC_VERSION_FUNC_ID:
+	case ARM_SMCCC_ARCH_FEATURES_FUNC_ID:
+	case ARM_SMCCC_HV_PV_TIME_FEATURES:
+	case ARM_SMCCC_HV_PV_TIME_ST:
+	case ARM_SMCCC_VENDOR_HYP_CALL_UID_FUNC_ID:
+	case ARM_SMCCC_VENDOR_HYP_KVM_FEATURES_FUNC_ID:
+	case ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID:
+		return true;
+	default:
+		return kvm_psci_func_id_is_valid(func_id);
+	}
+}
+
+static bool kvm_hvc_call_allowed(struct kvm_vcpu *vcpu, u32 func_id)
+{
+	struct kvm_smccc_features *smccc_feat = &vcpu->kvm->arch.smccc_feat;
+
+	switch (func_id) {
+	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:
+		return kvm_arm_fw_reg_feat_enabled(&smccc_feat->std_bmap,
+						KVM_REG_ARM_STD_BIT_TRNG_V1_0);
+	default:
+		return kvm_hvc_call_default_allowed(func_id);
+	}
+}
+
 int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
 {
 	u32 func_id = smccc_get_function(vcpu);

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

@@ -155,6 +200,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;
 }

@@ -164,8 +210,16 @@ static const u64 kvm_arm_fw_reg_ids[] = {
 	KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1,
 	KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2,
 	KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_3,
+	KVM_REG_ARM_STD_BMAP,
 };
 
+void kvm_arm_init_hypercalls(struct kvm *kvm)
+{
+	struct kvm_smccc_features *smccc_feat = &kvm->arch.smccc_feat;
+
+	smccc_feat->std_bmap = KVM_ARM_SMCCC_STD_FEATURES;
+}
+
 int kvm_arm_get_fw_num_regs(struct kvm_vcpu *vcpu)
 {
 	return ARRAY_SIZE(kvm_arm_fw_reg_ids);

@@ -237,6 +291,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 kvm_smccc_features *smccc_feat = &vcpu->kvm->arch.smccc_feat;
 	void __user *uaddr = (void __user *)(long)reg->addr;
 	u64 val;
 

@@ -249,6 +304,9 @@ int kvm_arm_get_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 	case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_3:
 		val = get_kernel_wa_level(reg->id) & KVM_REG_FEATURE_LEVEL_MASK;
 		break;
+	case KVM_REG_ARM_STD_BMAP:
+		val = READ_ONCE(smccc_feat->std_bmap);
+		break;
 	default:
 		return -ENOENT;
 	}

@@ -259,6 +317,52 @@ int kvm_arm_get_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 	return 0;
 }
 
+static int kvm_arm_set_fw_reg_bmap(struct kvm_vcpu *vcpu, u64 reg_id, u64 val)
+{
+	int ret = 0;
+	struct kvm *kvm = vcpu->kvm;
+	struct kvm_smccc_features *smccc_feat = &kvm->arch.smccc_feat;
+	unsigned long *fw_reg_bmap, fw_reg_features;
+
+	switch (reg_id) {
+	case KVM_REG_ARM_STD_BMAP:
+		fw_reg_bmap = &smccc_feat->std_bmap;
+		fw_reg_features = KVM_ARM_SMCCC_STD_FEATURES;
+		break;
+	default:
+		return -ENOENT;
+	}
+
+	/* Check for unsupported bit */
+	if (val & ~fw_reg_features)
+		return -EINVAL;
+
+	/*
+	 * Return -EBUSY if the VM (any vCPU) has already started running.

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

+
+	mutex_lock(&kvm->lock);
+
+	if (test_bit(KVM_ARCH_FLAG_HAS_RAN_ONCE, &kvm->arch.flags)) {
+		ret = -EBUSY;
+		goto out;
+	}
+
+	WRITE_ONCE(*fw_reg_bmap, val);
+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;

@@ -337,6 +441,8 @@ int kvm_arm_set_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 			return -EINVAL;
 
 		return 0;
+	case KVM_REG_ARM_STD_BMAP:
+		return kvm_arm_set_fw_reg_bmap(vcpu, reg->id, val);
 	default:
 		return -ENOENT;
 	}

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

index 4bb76eb09248..94111f02674f 100644

include/kvm/arm_hypercalls.h +4 -0

+
+#define KVM_ARM_SMCCC_STD_FEATURES		GENMASK(KVM_REG_ARM_STD_BMAP_BIT_MAX, 0)
+
 int kvm_hvc_call_handler(struct kvm_vcpu *vcpu);
 
 static inline u32 smccc_get_function(struct kvm_vcpu *vcpu)

@@ -42,6 +47,7 @@ static inline void smccc_set_retval(struct kvm_vcpu *vcpu,
 
 struct kvm_one_reg;
 
+void kvm_arm_init_hypercalls(struct kvm *kvm);
 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);

include/kvm/arm_psci.h +0 -0

index 6e55b9283789..1f9ec5f2e513 100644

include/kvm/arm_psci.h +0 -0

 #endif /* __KVM_ARM_PSCI_H__ */

Signature · 1 line

2.36.0.464.gb9c8b46e94-goog

Add the documentation for the bitmap firmware registers in hypercalls.rst and api.rst. This includes the details for KVM_REG_ARM_STD_BMAP, KVM_REG_ARM_STD_HYP_BMAP, and KVM_REG_ARM_VENDOR_HYP_BMAP registers.

Since the document is growing to carry other hypercall related information, make necessary adjustments to present the document in a generic sense, rather than being PSCI focused.

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

Reviewed-by Gavin Shan <[email protected]>

---
 Documentation/virt/kvm/api.rst            | 16 ++++
 Documentation/virt/kvm/arm/hypercalls.rst | 94 ++++++++++++++++++-----
 2 files changed, 92 insertions(+), 18 deletions(-)

Documentation/virt/kvm/api.rst +4 -0

index 4a900cdbc62e..8ae638be79fd 100644
@@ -2542,6 +2542,22 @@ arm64 firmware pseudo-registers have the following bit pattern::
 
   0x6030 0000 0014 <regno:16>
 
+arm64 bitmap feature firmware pseudo-registers have the following bit pattern::
+
+  0x6030 0000 0016 <regno:16>
+

Documentation/virt/kvm/api.rst +2 -0

+(See Documentation/virt/kvm/arm/hypercalls.rst for more details.)
+
 arm64 SVE registers have the following bit patterns::
 
   0x6080 0000 0015 00 <n:5> <slice:5>   Zn bits[2048*slice + 2047 : 2048*slice]

Documentation/virt/kvm/arm/hypercalls.rst +25 -18

index d52c2e83b5b8..383ca766cf36 100644
@@ -1,32 +1,32 @@
 .. 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.
+KVM handles the hypercall services as requested by the guests. New hypercall
+services are regularly made available by the ARM specification or by KVM (as
+vendor services) if they make sense from a virtualization point of view.
 
-The PSCI specification is regularly updated to provide new features,
-and KVM implements these updates 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.
+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 version of a hypercall service, or if a migration
+causes a different 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
 interface. These registers can be saved/restored by userspace, and set
-to a convenient value if required.
+to a convenient value as required.
 
-The following register is defined:
+The following registers are 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)
   - Returns the current PSCI version on GET_ONE_REG (defaulting to the

@@ -74,4 +74,62 @@ 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.
 
-.. [1] https://developer.arm.com/-/media/developer/pdf/ARM_DEN_0070A_Firmware_interfaces_for_mitigating_CVE-2017-5715.pdf
+
+Bitmap Feature Firmware Registers
+---------------------------------
+
+Contrary to the above registers, the following registers exposes the hypercall
+services in the form of a feature-bitmap to the userspace. This bitmap is
+translated to the services that are available to the guest. There is a register
+defined per service call owner and can be accessed via GET/SET_ONE_REG interface.

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

+
+* KVM_REG_ARM_STD_BMAP:
+    Controls the bitmap of the ARM Standard Secure Service Calls.
+
+  The following bits are accepted:
+
+    Bit-0: KVM_REG_ARM_STD_BIT_TRNG_V1_0:
+      The bit represents the services offered under v1.0 of ARM True Random
+      Number Generator (TRNG) specification, ARM DEN0098.
+
+* KVM_REG_ARM_STD_HYP_BMAP:
+    Controls the bitmap of the ARM Standard Hypervisor Service Calls.
+
+  The following bits are accepted:
+
+    Bit-0: KVM_REG_ARM_STD_HYP_BIT_PV_TIME:
+      The bit represents the Paravirtualized Time service as represented by
+      ARM DEN0057A.
+
+* KVM_REG_ARM_VENDOR_HYP_BMAP:
+    Controls the bitmap of the Vendor specific Hypervisor Service Calls.
+
+  The following bits are accepted:
+
+    Bit-0: KVM_REG_ARM_VENDOR_HYP_BIT_FUNC_FEAT
+      The bit represents the ARM_SMCCC_VENDOR_HYP_KVM_FEATURES_FUNC_ID
+      and ARM_SMCCC_VENDOR_HYP_CALL_UID_FUNC_ID function-ids.
+
+    Bit-1: KVM_REG_ARM_VENDOR_HYP_BIT_PTP:
+      The bit represents the Precision Time Protocol KVM service.
+
+Errors:
+
+    =======  =============================================================
+    -ENOENT   Unknown register accessed.
+    -EBUSY    Attempt a 'write' to the register after the VM has started.
+    -EINVAL   Invalid bitmap written to the register.
+    =======  =============================================================
+
+.. [1] https://developer.arm.com/-/media/developer/pdf/ARM_DEN_0070A_Firmware_interfaces_for_mitigating_CVE-2017-5715.pdf
\ No newline at end of file

Signature · 1 line

2.36.0.464.gb9c8b46e94-goog

Hello,

Continuing the discussion from [1], the series tries to add support for the userspace 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.

Quoted 8 lines · Raghavendra Rao Ananta

Hello,

Continuing the discussion from [1], the series tries to add support for the userspace 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.

As it took some time to get there, and that there was still a bunch of things to address, I've taken the liberty to apply my own fixes to the series.

Please have a look at [1], and let me know if you're OK with the result. If you are, I'll merge the series for 5.19.

Thanks,

        M.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=kvm-arm64/hcall-selection

--
Without deviation from the norm, progress is not possible.

Hi Marc,

Quoted 19 lines · Marc Zyngier

Quoted 8 lines · Raghavendra Rao Ananta

Hello,

Continuing the discussion from [1], the series tries to add support for the userspace 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.

As it took some time to get there, and that there was still a bunch of things to address, I've taken the liberty to apply my own fixes to the series.

Please have a look at [1], and let me know if you're OK with the result. If you are, I'll merge the series for 5.19.

Quoted 4 lines

Thanks,

        M.

Thank you for speeding up the process; appreciate it. However, the series's selftest patches have a dependency on Oliver's PSCI_SYSTEM_SUSPEND's selftest patches [1][2]. Can we pull them in too?

aarch64/hypercalls.c: In function ‘guest_test_hvc’:
aarch64/hypercalls.c:95:30: error: storage size of ‘res’ isn’t known
95 | struct arm_smccc_res res;
| ^~~
aarch64/hypercalls.c:103:17: warning: implicit declaration of function ‘smccc_hvc’ [-Wimplicit-function-declaration]
103 | smccc_hvc(hc_info->func_id, hc_info->arg1, 0,

0, 0, 0, 0, 0, &res);
      |                 ^~~~~~~~~

Also, just a couple of readability nits in the fixed version:

1. Patch-2/9, hypercall.c:kvm_hvc_call_default_allowed(), in the 'default' case, do you think we should probably add a small comment that mentions we are checking for func_id in the PSCI range?

2. Patch-2/9, arm_hypercall.h, clear all the macros in this patch itself instead of doing it in increments (unless there's some reason that I'm missing)?

Quoted 23 lines · Raghavendra Rao Ananta

Hi Marc,

Quoted 19 lines · Marc Zyngier

Quoted 8 lines · Raghavendra Rao Ananta

Hello,

Continuing the discussion from [1], the series tries to add support for the userspace 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.

As it took some time to get there, and that there was still a bunch of things to address, I've taken the liberty to apply my own fixes to the series.

Please have a look at [1], and let me know if you're OK with the result. If you are, I'll merge the series for 5.19.

Quoted 8 lines

Quoted 4 lines

Thanks,

        M.

Thank you for speeding up the process; appreciate it. However, the series's selftest patches have a dependency on Oliver's PSCI_SYSTEM_SUSPEND's selftest patches [1][2]. Can we pull them in too?

Urgh... I guess this is the time to set some ground rules:

- Please don't introduce dependencies between series, that's unmanageable. I really need to see each series independently, and if there is a merge conflict, that's my job to fix (and I don't really mind).

- If there is a dependency between series, please post a version of the required patches as a prefix to your series, assuming this prefix is itself standalone. If it isn't, then something really is wrong, and the series should be resplit.

- You also should be basing your series on an *official* tag from Linus' tree (preferably -rc1, -rc2 or -rc3), and not something random like any odd commit from the KVM tree (I had conflicts while applying this on -rc3, probably due to the non-advertised dependency on Oliver's series).

Quoted 11 lines

aarch64/hypercalls.c: In function ‘guest_test_hvc’:
aarch64/hypercalls.c:95:30: error: storage size of ‘res’ isn’t known
95 | struct arm_smccc_res res;
| ^~~
aarch64/hypercalls.c:103:17: warning: implicit declaration of function ‘smccc_hvc’ [-Wimplicit-function-declaration]
103 | smccc_hvc(hc_info->func_id, hc_info->arg1, 0,

0, 0, 0, 0, 0, &res);
      |                 ^~~~~~~~~

I've picked the two patches, which means they will most likely appear twice in the history. In the future, please reach out so that we can organise this better.

Quoted 5 lines

Also, just a couple of readability nits in the fixed version:

1. Patch-2/9, hypercall.c:kvm_hvc_call_default_allowed(), in the 'default' case, do you think we should probably add a small comment that mentions we are checking for func_id in the PSCI range?

Dumped a one-liner there.

Quoted 3 lines

2. Patch-2/9, arm_hypercall.h, clear all the macros in this patch itself instead of doing it in increments (unless there's some reason that I'm missing)?

Ah, rebasing leftovers, now gone.

I've pushed an updated branch again, please have a look.

        M.

--
Without deviation from the norm, progress is not possible.

Quoted 47 lines · Marc Zyngier

Quoted 23 lines · Raghavendra Rao Ananta

Hi Marc,

Quoted 19 lines · Marc Zyngier

Quoted 8 lines · Raghavendra Rao Ananta

Hello,

Continuing the discussion from [1], the series tries to add support for the userspace 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.

As it took some time to get there, and that there was still a bunch of things to address, I've taken the liberty to apply my own fixes to the series.

Please have a look at [1], and let me know if you're OK with the result. If you are, I'll merge the series for 5.19.

Quoted 8 lines

Quoted 4 lines

Thanks,

        M.

Thank you for speeding up the process; appreciate it. However, the series's selftest patches have a dependency on Oliver's PSCI_SYSTEM_SUSPEND's selftest patches [1][2]. Can we pull them in too?

Urgh... I guess this is the time to set some ground rules:

- Please don't introduce dependencies between series, that's unmanageable. I really need to see each series independently, and if there is a merge conflict, that's my job to fix (and I don't really mind).

- If there is a dependency between series, please post a version of the required patches as a prefix to your series, assuming this prefix is itself standalone. If it isn't, then something really is wrong, and the series should be resplit.

Quoted 6 lines

- You also should be basing your series on an *official* tag from Linus' tree (preferably -rc1, -rc2 or -rc3), and not something random like any odd commit from the KVM tree (I had conflicts while applying this on -rc3, probably due to the non-advertised dependency on Oliver's series).

Thanks for picking the dependency patches. I'll keep these mind the next time I push changes.

Quoted 28 lines

Quoted 11 lines

aarch64/hypercalls.c: In function ‘guest_test_hvc’:
aarch64/hypercalls.c:95:30: error: storage size of ‘res’ isn’t known
95 | struct arm_smccc_res res;
| ^~~
aarch64/hypercalls.c:103:17: warning: implicit declaration of function ‘smccc_hvc’ [-Wimplicit-function-declaration]
103 | smccc_hvc(hc_info->func_id, hc_info->arg1, 0,

0, 0, 0, 0, 0, &res);
      |                 ^~~~~~~~~

I've picked the two patches, which means they will most likely appear twice in the history. In the future, please reach out so that we can organise this better.

Quoted 5 lines

Also, just a couple of readability nits in the fixed version:

1. Patch-2/9, hypercall.c:kvm_hvc_call_default_allowed(), in the 'default' case, do you think we should probably add a small comment that mentions we are checking for func_id in the PSCI range?

Dumped a one-liner there.

Quoted 3 lines

2. Patch-2/9, arm_hypercall.h, clear all the macros in this patch itself instead of doing it in increments (unless there's some reason that I'm missing)?

Quoted 4 lines

Ah, rebasing leftovers, now gone.

I've pushed an updated branch again, please have a look.

Thanks for addressing these. The series looks good now.

Quoted 86 lines · Raghavendra Rao Ananta

Quoted 47 lines · Marc Zyngier

Quoted 23 lines · Raghavendra Rao Ananta

Hi Marc,

Quoted 19 lines · Marc Zyngier

Quoted 8 lines · Raghavendra Rao Ananta

Hello,

Continuing the discussion from [1], the series tries to add support for the userspace 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.

As it took some time to get there, and that there was still a bunch of things to address, I've taken the liberty to apply my own fixes to the series.

Please have a look at [1], and let me know if you're OK with the result. If you are, I'll merge the series for 5.19.

Quoted 8 lines

Quoted 4 lines

Thanks,

        M.

Thank you for speeding up the process; appreciate it. However, the series's selftest patches have a dependency on Oliver's PSCI_SYSTEM_SUSPEND's selftest patches [1][2]. Can we pull them in too?

Urgh... I guess this is the time to set some ground rules:

- Please don't introduce dependencies between series, that's unmanageable. I really need to see each series independently, and if there is a merge conflict, that's my job to fix (and I don't really mind).

- If there is a dependency between series, please post a version of the required patches as a prefix to your series, assuming this prefix is itself standalone. If it isn't, then something really is wrong, and the series should be resplit.

Quoted 6 lines

- You also should be basing your series on an *official* tag from Linus' tree (preferably -rc1, -rc2 or -rc3), and not something random like any odd commit from the KVM tree (I had conflicts while applying this on -rc3, probably due to the non-advertised dependency on Oliver's series).

Thanks for picking the dependency patches. I'll keep these mind the next time I push changes.

Quoted 28 lines

Quoted 11 lines

aarch64/hypercalls.c: In function ‘guest_test_hvc’:
aarch64/hypercalls.c:95:30: error: storage size of ‘res’ isn’t known
95 | struct arm_smccc_res res;
| ^~~
aarch64/hypercalls.c:103:17: warning: implicit declaration of function ‘smccc_hvc’ [-Wimplicit-function-declaration]
103 | smccc_hvc(hc_info->func_id, hc_info->arg1, 0,

0, 0, 0, 0, 0, &res);
      |                 ^~~~~~~~~

I've picked the two patches, which means they will most likely appear twice in the history. In the future, please reach out so that we can organise this better.

Quoted 5 lines

Also, just a couple of readability nits in the fixed version:

1. Patch-2/9, hypercall.c:kvm_hvc_call_default_allowed(), in the 'default' case, do you think we should probably add a small comment that mentions we are checking for func_id in the PSCI range?

Dumped a one-liner there.

Quoted 3 lines

2. Patch-2/9, arm_hypercall.h, clear all the macros in this patch itself instead of doing it in increments (unless there's some reason that I'm missing)?

Quoted 5 lines

Quoted 4 lines

Ah, rebasing leftovers, now gone.

I've pushed an updated branch again, please have a look.

Thanks for addressing these. The series looks good now.

Except it doesn't.

I introduced a bug by overly simplifying kvm_arm_set_fw_reg_bmap(), as we have to allow userspace writing the *same* value. As it turns out, QEMU restores all the registers on each reboot. Which as the vcpus have all run. This in turns triggers another issue in QEMU, which instead of taking the hint ans stopping there, sends all the vcpus into the guest in one go with random states... Crap happens.

I'll wear a brown paper bag for the rest of the day and add the following patch to the branch.

Thanks,

        M.

From 528ada2811ba0bb2b2db5bf0f829b48c50f3c13c Mon Sep 17 00:00:00 2001
From: Marc Zyngier <[email protected]>
Date: Mon, 16 May 2022 17:32:54 +0100
Subject: [PATCH] KVM: arm64: Fix hypercall bitmap writeback when vcpus have
already run

We generally want to disallow hypercall bitmaps being changed once vcpus have already run. But we must allow the write if the written value is unchanged so that userspace can rewrite the register file on reboot, for example.

Without this, a QEMU-based VM will fail to reboot correctly.

The original code was correct, and it is me that introduced the regression.

Fixes 05714cab7d63 ("KVM: arm64: Setup a framework for hypercall bitmap firmware registers")

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

---
 arch/arm64/kvm/hypercalls.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

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

index ccbd3cefb91a..c9f401fa01a9 100644

--
2.34.1

--
Without deviation from the norm, progress is not possible.

Quoted 23 lines · Raghavendra Rao Ananta

Hi Marc,

Quoted 19 lines · Marc Zyngier

Quoted 8 lines · Raghavendra Rao Ananta

Hello,

Continuing the discussion from [1], the series tries to add support for the userspace 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.

As it took some time to get there, and that there was still a bunch of things to address, I've taken the liberty to apply my own fixes to the series.

Please have a look at [1], and let me know if you're OK with the result. If you are, I'll merge the series for 5.19.

Quoted 8 lines

Quoted 4 lines

Thanks,

        M.

Thank you for speeding up the process; appreciate it. However, the series's selftest patches have a dependency on Oliver's PSCI_SYSTEM_SUSPEND's selftest patches [1][2]. Can we pull them in too?

Quoted 3 lines

2. Patch-2/9, arm_hypercall.h, clear all the macros in this patch itself instead of doing it in increments (unless there's some reason that I'm missing)?

Ah, rebasing leftovers, now gone.

I've pushed an updated branch again, please have a look.

Continuing the discussion from [1], the series tries to add support for the userspace 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 concept is similar to the current implementation of PSCI interface- create a 'firmware psuedo-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).

[...]