[RFC PATCH 1/3] KVM: arm64: Use a generalized accessor for SMCCC args

[RFC PATCH 1/3] KVM: arm64: Use a generalized accessor for SMCCC args

[Oliver Upton]

Avoid the need to introduce yet another accessor for every argument
position with a generalized one instead. Align with the SMCCC
specification and count arguments starting from 1.

No functional change intended.

Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/include/asm/kvm_hypercalls.h | 22 ++++++++--------------
 arch/arm64/kvm/hypercalls.c             |  4 ++--
 arch/arm64/kvm/psci.c                   | 14 +++++++-------
 arch/arm64/kvm/pvtime.c                 |  2 +-
 arch/arm64/kvm/trng.c                   |  4 ++--
 5 files changed, 20 insertions(+), 26 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_hypercalls.h b/arch/arm64/include/asm/kvm_hypercalls.h
index dfebe8dd8dcd..c30d6ae76000 100644
--- a/arch/arm64/include/asm/kvm_hypercalls.h
+++ b/arch/arm64/include/asm/kvm_hypercalls.h
@@ -13,20 +13,14 @@ static inline u32 smccc_get_function(struct kvm_vcpu *vcpu)
 	return vcpu_get_reg(vcpu, 0);
 }
 
-static inline unsigned long smccc_get_arg1(struct kvm_vcpu *vcpu)
-{
-	return vcpu_get_reg(vcpu, 1);
-}
-
-static inline unsigned long smccc_get_arg2(struct kvm_vcpu *vcpu)
-{
-	return vcpu_get_reg(vcpu, 2);
-}
-
-static inline unsigned long smccc_get_arg3(struct kvm_vcpu *vcpu)
-{
-	return vcpu_get_reg(vcpu, 3);
-}
+#define smccc_get_arg(vcpu, arg)			\
+({							\
+	u64 __val;					\
+							\
+	BUILD_BUG_ON(arg < 1 || arg > 17);		\
+	__val = vcpu_get_reg(vcpu, arg);		\
+	__val;						\
+})
 
 static inline void smccc_set_retval(struct kvm_vcpu *vcpu,
 				    unsigned long a0,
diff --git a/arch/arm64/kvm/hypercalls.c b/arch/arm64/kvm/hypercalls.c
index 6804075ce57f..62ce45d0d957 100644
--- a/arch/arm64/kvm/hypercalls.c
+++ b/arch/arm64/kvm/hypercalls.c
@@ -40,7 +40,7 @@ static void kvm_ptp_get_time(struct kvm_vcpu *vcpu, u64 *val)
 	 * (virtual or physical) with the first argument of the SMCCC
 	 * call. In case the identifier is not supported, error out.
 	 */
-	feature = smccc_get_arg1(vcpu);
+	feature = smccc_get_arg(vcpu, 1);
 	switch (feature) {
 	case KVM_PTP_VIRT_COUNTER:
 		cycles = systime_snapshot.cycles - vcpu_read_sys_reg(vcpu, CNTVOFF_EL2);
@@ -136,7 +136,7 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
 		val[0] = ARM_SMCCC_VERSION_1_1;
 		break;
 	case ARM_SMCCC_ARCH_FEATURES_FUNC_ID:
-		feature = smccc_get_arg1(vcpu);
+		feature = smccc_get_arg(vcpu, 1);
 		switch (feature) {
 		case ARM_SMCCC_ARCH_WORKAROUND_1:
 			switch (arm64_get_spectre_v2_state()) {
diff --git a/arch/arm64/kvm/psci.c b/arch/arm64/kvm/psci.c
index 809710808b25..15a380858ccb 100644
--- a/arch/arm64/kvm/psci.c
+++ b/arch/arm64/kvm/psci.c
@@ -63,7 +63,7 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
 	struct kvm_vcpu *vcpu = NULL;
 	unsigned long cpu_id;
 
-	cpu_id = smccc_get_arg1(source_vcpu);
+	cpu_id = smccc_get_arg(source_vcpu, 1);
 	if (!kvm_psci_valid_affinity(source_vcpu, cpu_id))
 		return PSCI_RET_INVALID_PARAMS;
 
@@ -84,7 +84,7 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
 
 	reset_state = &vcpu->arch.reset_state;
 
-	reset_state->pc = smccc_get_arg2(source_vcpu);
+	reset_state->pc = smccc_get_arg(source_vcpu, 2);
 
 	/* Propagate caller endianness */
 	reset_state->be = kvm_vcpu_is_be(source_vcpu);
@@ -93,7 +93,7 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
 	 * NOTE: We always update r0 (or x0) because for PSCI v0.1
 	 * the general purpose registers are undefined upon CPU_ON.
 	 */
-	reset_state->r0 = smccc_get_arg3(source_vcpu);
+	reset_state->r0 = smccc_get_arg(source_vcpu, 3);
 
 	WRITE_ONCE(reset_state->reset, true);
 	kvm_make_request(KVM_REQ_VCPU_RESET, vcpu);
@@ -120,8 +120,8 @@ static unsigned long kvm_psci_vcpu_affinity_info(struct kvm_vcpu *vcpu)
 	struct kvm *kvm = vcpu->kvm;
 	struct kvm_vcpu *tmp;
 
-	target_affinity = smccc_get_arg1(vcpu);
-	lowest_affinity_level = smccc_get_arg2(vcpu);
+	target_affinity = smccc_get_arg(vcpu, 1);
+	lowest_affinity_level = smccc_get_arg(vcpu, 2);
 
 	if (!kvm_psci_valid_affinity(vcpu, target_affinity))
 		return PSCI_RET_INVALID_PARAMS;
@@ -317,7 +317,7 @@ static int kvm_psci_1_x_call(struct kvm_vcpu *vcpu, u32 minor)
 		val = minor == 0 ? KVM_ARM_PSCI_1_0 : KVM_ARM_PSCI_1_1;
 		break;
 	case PSCI_1_0_FN_PSCI_FEATURES:
-		arg = smccc_get_arg1(vcpu);
+		arg = smccc_get_arg(vcpu, 1);
 		val = kvm_psci_check_allowed_function(vcpu, arg);
 		if (val)
 			break;
@@ -371,7 +371,7 @@ static int kvm_psci_1_x_call(struct kvm_vcpu *vcpu, u32 minor)
 		fallthrough;
 	case PSCI_1_1_FN64_SYSTEM_RESET2:
 		if (minor >= 1) {
-			arg = smccc_get_arg1(vcpu);
+			arg = smccc_get_arg(vcpu, 1);
 
 			if (arg <= PSCI_1_1_RESET_TYPE_SYSTEM_WARM_RESET ||
 			    arg >= PSCI_1_1_RESET_TYPE_VENDOR_START) {
diff --git a/arch/arm64/kvm/pvtime.c b/arch/arm64/kvm/pvtime.c
index 614b2e70b815..587eadf77f29 100644
--- a/arch/arm64/kvm/pvtime.c
+++ b/arch/arm64/kvm/pvtime.c
@@ -33,7 +33,7 @@ void kvm_update_stolen_time(struct kvm_vcpu *vcpu)
 
 long kvm_hypercall_pv_features(struct kvm_vcpu *vcpu)
 {
-	u32 feature = smccc_get_arg1(vcpu);
+	u32 feature = smccc_get_arg(vcpu, 1);
 	long val = SMCCC_RET_NOT_SUPPORTED;
 
 	switch (feature) {
diff --git a/arch/arm64/kvm/trng.c b/arch/arm64/kvm/trng.c
index b257d4eace50..953cb0d1b6ff 100644
--- a/arch/arm64/kvm/trng.c
+++ b/arch/arm64/kvm/trng.c
@@ -23,7 +23,7 @@ static const uuid_t arm_smc_trng_uuid __aligned(4) = UUID_INIT(
 static int kvm_trng_do_rnd(struct kvm_vcpu *vcpu, int size)
 {
 	DECLARE_BITMAP(bits, TRNG_MAX_BITS64);
-	u32 num_bits = smccc_get_arg1(vcpu);
+	u32 num_bits = smccc_get_arg(vcpu, 1);
 	int i;
 
 	if (num_bits > 3 * size) {
@@ -59,7 +59,7 @@ 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)) {
+		switch (smccc_get_arg(vcpu, 1)) {
 		case ARM_SMCCC_TRNG_VERSION:
 		case ARM_SMCCC_TRNG_FEATURES:
 		case ARM_SMCCC_TRNG_GET_UUID:
-- 
2.38.1.431.g37b22c650d-goog

[RFC PATCH 2/3] KVM: arm64: Allow userspace to trap SMCCC sub-ranges

[Oliver Upton]

As the SMCCC (and related specifications) march towards an
'everything and the kitchen sink' interface for interacting with a
system, it is less likely that KVM will implement every supported
feature.

Add a capability that allows userspace to trap hypercall ranges,
allowing the VMM to mix-and-match between calls handled in userspace vs.
KVM.

Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/include/asm/kvm_host.h |  5 ++++
 arch/arm64/include/uapi/asm/kvm.h | 15 ++++++++++
 arch/arm64/kvm/arm.c              | 10 +++++++
 arch/arm64/kvm/hypercalls.c       | 48 +++++++++++++++++++++++++++++++
 include/uapi/linux/kvm.h          |  1 +
 5 files changed, 79 insertions(+)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index e33ed7c09a28..cc3872f1900c 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -52,6 +52,9 @@
 
 #define KVM_HAVE_MMU_RWLOCK
 
+#define KVM_ARM_USER_HYPERCALL_FLAGS	\
+		GENMASK_ULL(KVM_ARM_USER_HYPERCALL_FLAGS_COUNT - 1, 0)
+
 /*
  * Mode of operation configurable with kvm-arm.mode early param.
  * See Documentation/admin-guide/kernel-parameters.txt for more information.
@@ -104,11 +107,13 @@ struct kvm_arch_memory_slot {
 /**
  * struct kvm_smccc_features: Descriptor of the hypercall services exposed to the guests
  *
+ * @user_trap_bmap: Bitmap of SMCCC function ranges trapped to userspace
  * @std_bmap: Bitmap of standard secure service calls
  * @std_hyp_bmap: Bitmap of standard hypervisor service calls
  * @vendor_hyp_bmap: Bitmap of vendor specific hypervisor service calls
  */
 struct kvm_smccc_features {
+	unsigned long user_trap_bmap;
 	unsigned long std_bmap;
 	unsigned long std_hyp_bmap;
 	unsigned long vendor_hyp_bmap;
diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
index 316917b98707..07fa3f597e61 100644
--- a/arch/arm64/include/uapi/asm/kvm.h
+++ b/arch/arm64/include/uapi/asm/kvm.h
@@ -370,6 +370,21 @@ enum {
 #endif
 };
 
+enum {
+	KVM_ARM_USER_HYPERCALL_OWNER_ARCH		= 0,
+	KVM_ARM_USER_HYPERCALL_OWNER_CPU		= 1,
+	KVM_ARM_USER_HYPERCALL_OWNER_SIP		= 2,
+	KVM_ARM_USER_HYPERCALL_OWNER_OEM		= 3,
+	KVM_ARM_USER_HYPERCALL_OWNER_STANDARD		= 4,
+	KVM_ARM_USER_HYPERCALL_OWNER_STANDARD_HYP	= 5,
+	KVM_ARM_USER_HYPERCALL_OWNER_VENDOR_HYP		= 6,
+	KVM_ARM_USER_HYPERCALL_OWNER_TRUSTED_APP	= 7,
+	KVM_ARM_USER_HYPERCALL_OWNER_TRUSTED_OS		= 8,
+#ifdef __KERNEL__
+	KVM_ARM_USER_HYPERCALL_FLAGS_COUNT,
+#endif
+};
+
 /* Device Control API: ARM VGIC */
 #define KVM_DEV_ARM_VGIC_GRP_ADDR	0
 #define KVM_DEV_ARM_VGIC_GRP_DIST_REGS	1
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index 6f0b56e7f8c7..6e8a222fc295 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -100,6 +100,13 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
 		r = 0;
 		set_bit(KVM_ARCH_FLAG_SYSTEM_SUSPEND_ENABLED, &kvm->arch.flags);
 		break;
+	case KVM_CAP_ARM_USER_HYPERCALLS:
+		if (cap->args[0] & ~KVM_ARM_USER_HYPERCALL_FLAGS)
+			return -EINVAL;
+
+		r = 0;
+		kvm->arch.smccc_feat.user_trap_bmap = cap->args[0];
+		break;
 	default:
 		r = -EINVAL;
 		break;
@@ -285,6 +292,9 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 	case KVM_CAP_ARM_PTRAUTH_GENERIC:
 		r = system_has_full_ptr_auth();
 		break;
+	case KVM_CAP_ARM_USER_HYPERCALLS:
+		r = KVM_ARM_USER_HYPERCALL_FLAGS;
+		break;
 	default:
 		r = 0;
 	}
diff --git a/arch/arm64/kvm/hypercalls.c b/arch/arm64/kvm/hypercalls.c
index 62ce45d0d957..22a23b12201d 100644
--- a/arch/arm64/kvm/hypercalls.c
+++ b/arch/arm64/kvm/hypercalls.c
@@ -92,6 +92,49 @@ static bool kvm_hvc_call_default_allowed(u32 func_id)
 	}
 }
 
+static bool kvm_hvc_call_user_trapped(struct kvm_vcpu *vcpu, u32 func_id)
+{
+	struct kvm *kvm = vcpu->kvm;
+	unsigned long *bmap = &kvm->arch.smccc_feat.user_trap_bmap;
+
+	switch (ARM_SMCCC_OWNER_NUM(func_id)) {
+	case ARM_SMCCC_OWNER_ARCH:
+		return test_bit(KVM_ARM_USER_HYPERCALL_OWNER_ARCH, bmap);
+	case ARM_SMCCC_OWNER_CPU:
+		return test_bit(KVM_ARM_USER_HYPERCALL_OWNER_CPU, bmap);
+	case ARM_SMCCC_OWNER_SIP:
+		return test_bit(KVM_ARM_USER_HYPERCALL_OWNER_SIP, bmap);
+	case ARM_SMCCC_OWNER_OEM:
+		return test_bit(KVM_ARM_USER_HYPERCALL_OWNER_OEM, bmap);
+	case ARM_SMCCC_OWNER_STANDARD:
+		return test_bit(KVM_ARM_USER_HYPERCALL_OWNER_STANDARD, bmap);
+	case ARM_SMCCC_OWNER_STANDARD_HYP:
+		return test_bit(KVM_ARM_USER_HYPERCALL_OWNER_STANDARD_HYP, bmap);
+	case ARM_SMCCC_OWNER_VENDOR_HYP:
+		return test_bit(KVM_ARM_USER_HYPERCALL_OWNER_VENDOR_HYP, bmap);
+	case ARM_SMCCC_OWNER_TRUSTED_APP ... ARM_SMCCC_OWNER_TRUSTED_APP_END:
+		return test_bit(KVM_ARM_USER_HYPERCALL_OWNER_TRUSTED_APP, bmap);
+	case ARM_SMCCC_OWNER_TRUSTED_OS ... ARM_SMCCC_OWNER_TRUSTED_OS_END:
+		return test_bit(KVM_ARM_USER_HYPERCALL_OWNER_TRUSTED_OS, bmap);
+	default:
+		return false;
+	}
+}
+
+static void kvm_hvc_prepare_user_trap(struct kvm_vcpu *vcpu)
+{
+	struct kvm_run *run = vcpu->run;
+
+	run->exit_reason	= KVM_EXIT_HYPERCALL;
+	run->hypercall.nr	= smccc_get_function(vcpu);
+	run->hypercall.args[0]	= smccc_get_arg(vcpu, 1);
+	run->hypercall.args[1]	= smccc_get_arg(vcpu, 2);
+	run->hypercall.args[2]	= smccc_get_arg(vcpu, 3);
+	run->hypercall.args[3]	= smccc_get_arg(vcpu, 4);
+	run->hypercall.args[4]	= smccc_get_arg(vcpu, 5);
+	run->hypercall.args[5]	= smccc_get_arg(vcpu, 6);
+}
+
 static bool kvm_hvc_call_allowed(struct kvm_vcpu *vcpu, u32 func_id)
 {
 	struct kvm_smccc_features *smccc_feat = &vcpu->kvm->arch.smccc_feat;
@@ -128,6 +171,11 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
 	u32 feature;
 	gpa_t gpa;
 
+	if (kvm_hvc_call_user_trapped(vcpu, func_id)) {
+		kvm_hvc_prepare_user_trap(vcpu);
+		return 0;
+	}
+
 	if (!kvm_hvc_call_allowed(vcpu, func_id))
 		goto out;
 
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 0d5d4419139a..ea6c1983a545 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -1178,6 +1178,7 @@ struct kvm_ppc_resize_hpt {
 #define KVM_CAP_S390_ZPCI_OP 221
 #define KVM_CAP_S390_CPU_TOPOLOGY 222
 #define KVM_CAP_DIRTY_LOG_RING_ACQ_REL 223
+#define KVM_CAP_ARM_USER_HYPERCALLS 224
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
-- 
2.38.1.431.g37b22c650d-goog

Re: [RFC PATCH 2/3] KVM: arm64: Allow userspace to trap SMCCC sub-ranges

[Marc Zyngier]

On Thu, 10 Nov 2022 01:53:26 +0000,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> As the SMCCC (and related specifications) march towards an
> 'everything and the kitchen sink' interface for interacting with a
> system, it is less likely that KVM will implement every supported
> feature.
> 
> Add a capability that allows userspace to trap hypercall ranges,
> allowing the VMM to mix-and-match between calls handled in userspace vs.
> KVM.
> 
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
>  arch/arm64/include/asm/kvm_host.h |  5 ++++
>  arch/arm64/include/uapi/asm/kvm.h | 15 ++++++++++
>  arch/arm64/kvm/arm.c              | 10 +++++++
>  arch/arm64/kvm/hypercalls.c       | 48 +++++++++++++++++++++++++++++++
>  include/uapi/linux/kvm.h          |  1 +
>  5 files changed, 79 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index e33ed7c09a28..cc3872f1900c 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -52,6 +52,9 @@
>  
>  #define KVM_HAVE_MMU_RWLOCK
>  
> +#define KVM_ARM_USER_HYPERCALL_FLAGS	\
> +		GENMASK_ULL(KVM_ARM_USER_HYPERCALL_FLAGS_COUNT - 1, 0)
> +
>  /*
>   * Mode of operation configurable with kvm-arm.mode early param.
>   * See Documentation/admin-guide/kernel-parameters.txt for more information.
> @@ -104,11 +107,13 @@ struct kvm_arch_memory_slot {
>  /**
>   * struct kvm_smccc_features: Descriptor of the hypercall services exposed to the guests
>   *
> + * @user_trap_bmap: Bitmap of SMCCC function ranges trapped to userspace
>   * @std_bmap: Bitmap of standard secure service calls
>   * @std_hyp_bmap: Bitmap of standard hypervisor service calls
>   * @vendor_hyp_bmap: Bitmap of vendor specific hypervisor service calls
>   */
>  struct kvm_smccc_features {
> +	unsigned long user_trap_bmap;

nit: I strongly object to the word 'trap'. By definition, this is a
trap. The difference here is that you *forward* something to userspace
instead of implementing it in the kernel.

>  	unsigned long std_bmap;
>  	unsigned long std_hyp_bmap;
>  	unsigned long vendor_hyp_bmap;
> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> index 316917b98707..07fa3f597e61 100644
> --- a/arch/arm64/include/uapi/asm/kvm.h
> +++ b/arch/arm64/include/uapi/asm/kvm.h
> @@ -370,6 +370,21 @@ enum {
>  #endif
>  };
>  
> +enum {
> +	KVM_ARM_USER_HYPERCALL_OWNER_ARCH		= 0,
> +	KVM_ARM_USER_HYPERCALL_OWNER_CPU		= 1,
> +	KVM_ARM_USER_HYPERCALL_OWNER_SIP		= 2,
> +	KVM_ARM_USER_HYPERCALL_OWNER_OEM		= 3,
> +	KVM_ARM_USER_HYPERCALL_OWNER_STANDARD		= 4,
> +	KVM_ARM_USER_HYPERCALL_OWNER_STANDARD_HYP	= 5,
> +	KVM_ARM_USER_HYPERCALL_OWNER_VENDOR_HYP		= 6,
> +	KVM_ARM_USER_HYPERCALL_OWNER_TRUSTED_APP	= 7,
> +	KVM_ARM_USER_HYPERCALL_OWNER_TRUSTED_OS		= 8,
> +#ifdef __KERNEL__
> +	KVM_ARM_USER_HYPERCALL_FLAGS_COUNT,
> +#endif
> +};
> +
>  /* Device Control API: ARM VGIC */
>  #define KVM_DEV_ARM_VGIC_GRP_ADDR	0
>  #define KVM_DEV_ARM_VGIC_GRP_DIST_REGS	1
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 6f0b56e7f8c7..6e8a222fc295 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -100,6 +100,13 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
>  		r = 0;
>  		set_bit(KVM_ARCH_FLAG_SYSTEM_SUSPEND_ENABLED, &kvm->arch.flags);
>  		break;
> +	case KVM_CAP_ARM_USER_HYPERCALLS:
> +		if (cap->args[0] & ~KVM_ARM_USER_HYPERCALL_FLAGS)
> +			return -EINVAL;
> +
> +		r = 0;
> +		kvm->arch.smccc_feat.user_trap_bmap = cap->args[0];
> +		break;
>  	default:
>  		r = -EINVAL;
>  		break;
> @@ -285,6 +292,9 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>  	case KVM_CAP_ARM_PTRAUTH_GENERIC:
>  		r = system_has_full_ptr_auth();
>  		break;
> +	case KVM_CAP_ARM_USER_HYPERCALLS:
> +		r = KVM_ARM_USER_HYPERCALL_FLAGS;
> +		break;
>  	default:
>  		r = 0;
>  	}
> diff --git a/arch/arm64/kvm/hypercalls.c b/arch/arm64/kvm/hypercalls.c
> index 62ce45d0d957..22a23b12201d 100644
> --- a/arch/arm64/kvm/hypercalls.c
> +++ b/arch/arm64/kvm/hypercalls.c
> @@ -92,6 +92,49 @@ static bool kvm_hvc_call_default_allowed(u32 func_id)
>  	}
>  }
>  
> +static bool kvm_hvc_call_user_trapped(struct kvm_vcpu *vcpu, u32 func_id)
> +{
> +	struct kvm *kvm = vcpu->kvm;
> +	unsigned long *bmap = &kvm->arch.smccc_feat.user_trap_bmap;
> +
> +	switch (ARM_SMCCC_OWNER_NUM(func_id)) {
> +	case ARM_SMCCC_OWNER_ARCH:
> +		return test_bit(KVM_ARM_USER_HYPERCALL_OWNER_ARCH, bmap);
> +	case ARM_SMCCC_OWNER_CPU:
> +		return test_bit(KVM_ARM_USER_HYPERCALL_OWNER_CPU, bmap);
> +	case ARM_SMCCC_OWNER_SIP:
> +		return test_bit(KVM_ARM_USER_HYPERCALL_OWNER_SIP, bmap);
> +	case ARM_SMCCC_OWNER_OEM:
> +		return test_bit(KVM_ARM_USER_HYPERCALL_OWNER_OEM, bmap);
> +	case ARM_SMCCC_OWNER_STANDARD:
> +		return test_bit(KVM_ARM_USER_HYPERCALL_OWNER_STANDARD, bmap);
> +	case ARM_SMCCC_OWNER_STANDARD_HYP:
> +		return test_bit(KVM_ARM_USER_HYPERCALL_OWNER_STANDARD_HYP, bmap);
> +	case ARM_SMCCC_OWNER_VENDOR_HYP:
> +		return test_bit(KVM_ARM_USER_HYPERCALL_OWNER_VENDOR_HYP, bmap);
> +	case ARM_SMCCC_OWNER_TRUSTED_APP ... ARM_SMCCC_OWNER_TRUSTED_APP_END:
> +		return test_bit(KVM_ARM_USER_HYPERCALL_OWNER_TRUSTED_APP, bmap);
> +	case ARM_SMCCC_OWNER_TRUSTED_OS ... ARM_SMCCC_OWNER_TRUSTED_OS_END:
> +		return test_bit(KVM_ARM_USER_HYPERCALL_OWNER_TRUSTED_OS, bmap);
> +	default:
> +		return false;
> +	}

You have multiple problems here:

- the granularity is way too coarse. You want to express arbitrary
  ranges, and not necessarily grab a whole owner range.

- you have now an overlap between ranges that are handled in the
  kernel (PSCI, spectre mitigations) and ranges that userspace wants
  to observe. Not good.

If we are going down this road, this can only be done at the
*function* level. And userspace must know that the kernel will refuse
to forward some ranges.

So obviously, this cannot be a simple bitmap. Making it a radix tree
(or an xarray, which is basically the same thing) could work. And the
filtering request from userspace can be similar to what we have for
the PMU filters.

> +}
> +
> +static void kvm_hvc_prepare_user_trap(struct kvm_vcpu *vcpu)
> +{
> +	struct kvm_run *run = vcpu->run;
> +
> +	run->exit_reason	= KVM_EXIT_HYPERCALL;
> +	run->hypercall.nr	= smccc_get_function(vcpu);
> +	run->hypercall.args[0]	= smccc_get_arg(vcpu, 1);
> +	run->hypercall.args[1]	= smccc_get_arg(vcpu, 2);
> +	run->hypercall.args[2]	= smccc_get_arg(vcpu, 3);
> +	run->hypercall.args[3]	= smccc_get_arg(vcpu, 4);
> +	run->hypercall.args[4]	= smccc_get_arg(vcpu, 5);
> +	run->hypercall.args[5]	= smccc_get_arg(vcpu, 6);

All of which is readily available through the ONE_REG interface. I'm
mildly reluctant to expose another interface that disclose the same
information (yes, I understand the performance impact).

Thanks,

	M.

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

Re: [RFC PATCH 2/3] KVM: arm64: Allow userspace to trap SMCCC sub-ranges

[Oliver Upton]

On Thu, Nov 10, 2022 at 12:22:12PM +0000, Marc Zyngier wrote:
> > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > index e33ed7c09a28..cc3872f1900c 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -52,6 +52,9 @@
> >  
> >  #define KVM_HAVE_MMU_RWLOCK
> >  
> > +#define KVM_ARM_USER_HYPERCALL_FLAGS	\
> > +		GENMASK_ULL(KVM_ARM_USER_HYPERCALL_FLAGS_COUNT - 1, 0)
> > +
> >  /*
> >   * Mode of operation configurable with kvm-arm.mode early param.
> >   * See Documentation/admin-guide/kernel-parameters.txt for more information.
> > @@ -104,11 +107,13 @@ struct kvm_arch_memory_slot {
> >  /**
> >   * struct kvm_smccc_features: Descriptor of the hypercall services exposed to the guests
> >   *
> > + * @user_trap_bmap: Bitmap of SMCCC function ranges trapped to userspace
> >   * @std_bmap: Bitmap of standard secure service calls
> >   * @std_hyp_bmap: Bitmap of standard hypervisor service calls
> >   * @vendor_hyp_bmap: Bitmap of vendor specific hypervisor service calls
> >   */
> >  struct kvm_smccc_features {
> > +	unsigned long user_trap_bmap;
> 
> nit: I strongly object to the word 'trap'. By definition, this is a
> trap. The difference here is that you *forward* something to userspace
> instead of implementing it in the kernel.

I think you're being polite calling this a 'nit' :-)

Naming came about lazily to shorten some names, but completely breaks
the notion of what a trap is. Oops.

> > diff --git a/arch/arm64/kvm/hypercalls.c b/arch/arm64/kvm/hypercalls.c
> > index 62ce45d0d957..22a23b12201d 100644
> > --- a/arch/arm64/kvm/hypercalls.c
> > +++ b/arch/arm64/kvm/hypercalls.c
> > @@ -92,6 +92,49 @@ static bool kvm_hvc_call_default_allowed(u32 func_id)
> >  	}
> >  }
> >  
> > +static bool kvm_hvc_call_user_trapped(struct kvm_vcpu *vcpu, u32 func_id)
> > +{
> > +	struct kvm *kvm = vcpu->kvm;
> > +	unsigned long *bmap = &kvm->arch.smccc_feat.user_trap_bmap;
> > +
> > +	switch (ARM_SMCCC_OWNER_NUM(func_id)) {
> > +	case ARM_SMCCC_OWNER_ARCH:
> > +		return test_bit(KVM_ARM_USER_HYPERCALL_OWNER_ARCH, bmap);
> > +	case ARM_SMCCC_OWNER_CPU:
> > +		return test_bit(KVM_ARM_USER_HYPERCALL_OWNER_CPU, bmap);
> > +	case ARM_SMCCC_OWNER_SIP:
> > +		return test_bit(KVM_ARM_USER_HYPERCALL_OWNER_SIP, bmap);
> > +	case ARM_SMCCC_OWNER_OEM:
> > +		return test_bit(KVM_ARM_USER_HYPERCALL_OWNER_OEM, bmap);
> > +	case ARM_SMCCC_OWNER_STANDARD:
> > +		return test_bit(KVM_ARM_USER_HYPERCALL_OWNER_STANDARD, bmap);
> > +	case ARM_SMCCC_OWNER_STANDARD_HYP:
> > +		return test_bit(KVM_ARM_USER_HYPERCALL_OWNER_STANDARD_HYP, bmap);
> > +	case ARM_SMCCC_OWNER_VENDOR_HYP:
> > +		return test_bit(KVM_ARM_USER_HYPERCALL_OWNER_VENDOR_HYP, bmap);
> > +	case ARM_SMCCC_OWNER_TRUSTED_APP ... ARM_SMCCC_OWNER_TRUSTED_APP_END:
> > +		return test_bit(KVM_ARM_USER_HYPERCALL_OWNER_TRUSTED_APP, bmap);
> > +	case ARM_SMCCC_OWNER_TRUSTED_OS ... ARM_SMCCC_OWNER_TRUSTED_OS_END:
> > +		return test_bit(KVM_ARM_USER_HYPERCALL_OWNER_TRUSTED_OS, bmap);
> > +	default:
> > +		return false;
> > +	}
> 
> You have multiple problems here:
> 
> - the granularity is way too coarse. You want to express arbitrary
>   ranges, and not necessarily grab a whole owner range.
> 
> - you have now an overlap between ranges that are handled in the
>   kernel (PSCI, spectre mitigations) and ranges that userspace wants
>   to observe. Not good.

We need to come to agreement on what degree of mix-and-match should be
supported.

Spectre really ought to be in the kernel, and I don't think anyone is
particularly excited about reimplementing PSCI. Right now my interest
in this starts and ends with forwarding the vendor-specific hypercall
range to userspace, allowing something like Hyper-V PV on KVM.

> If we are going down this road, this can only be done at the
> *function* level. And userspace must know that the kernel will refuse
> to forward some ranges.

The goal of what I was trying to get at is that either the kernel or
userspace takes ownership of a range that has an ABI, but not both. i.e.
you really wouldn't want some VMM or cloud provider trapping portions of
KVM's vendor-specific range while still reporting a 'vanilla' ABI at the
time of discovery. Same goes for PSCI, TRNG, etc.

> So obviously, this cannot be a simple bitmap. Making it a radix tree
> (or an xarray, which is basically the same thing) could work. And the
> filtering request from userspace can be similar to what we have for
> the PMU filters.

Right, we'll need a more robust data structure for all this.

My only concern is that communicating the hypercall filter between
user/kernel with a set of ranges or function numbers is that we could be
mutating what KVM *doesn't* already implement into an ABI of sorts.

i.e. suppose that userspace wants to filter function(s) in an
unallocated/unused range of function numbers. Later down the line KVM
adds support for a new shiny thing and the filter becomes a subset of a
now allocated range of calls. We then reject the filter due to the
incongruence.

> > +}
> > +
> > +static void kvm_hvc_prepare_user_trap(struct kvm_vcpu *vcpu)
> > +{
> > +	struct kvm_run *run = vcpu->run;
> > +
> > +	run->exit_reason	= KVM_EXIT_HYPERCALL;
> > +	run->hypercall.nr	= smccc_get_function(vcpu);
> > +	run->hypercall.args[0]	= smccc_get_arg(vcpu, 1);
> > +	run->hypercall.args[1]	= smccc_get_arg(vcpu, 2);
> > +	run->hypercall.args[2]	= smccc_get_arg(vcpu, 3);
> > +	run->hypercall.args[3]	= smccc_get_arg(vcpu, 4);
> > +	run->hypercall.args[4]	= smccc_get_arg(vcpu, 5);
> > +	run->hypercall.args[5]	= smccc_get_arg(vcpu, 6);
> 
> All of which is readily available through the ONE_REG interface. I'm
> mildly reluctant to expose another interface that disclose the same
> information (yes, I understand the performance impact).

I can drop this bit for now, always easy to add it back in and advertize
with a flag if the overhead is too great.

--
Thanks,
Oliver

Re: [RFC PATCH 2/3] KVM: arm64: Allow userspace to trap SMCCC sub-ranges

[Marc Zyngier]

On Thu, 10 Nov 2022 21:13:54 +0000,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> On Thu, Nov 10, 2022 at 12:22:12PM +0000, Marc Zyngier wrote:
> > > +static bool kvm_hvc_call_user_trapped(struct kvm_vcpu *vcpu, u32 func_id)
> > > +{
> > > +	struct kvm *kvm = vcpu->kvm;
> > > +	unsigned long *bmap = &kvm->arch.smccc_feat.user_trap_bmap;
> > > +
> > > +	switch (ARM_SMCCC_OWNER_NUM(func_id)) {
> > > +	case ARM_SMCCC_OWNER_ARCH:
> > > +		return test_bit(KVM_ARM_USER_HYPERCALL_OWNER_ARCH, bmap);
> > > +	case ARM_SMCCC_OWNER_CPU:
> > > +		return test_bit(KVM_ARM_USER_HYPERCALL_OWNER_CPU, bmap);
> > > +	case ARM_SMCCC_OWNER_SIP:
> > > +		return test_bit(KVM_ARM_USER_HYPERCALL_OWNER_SIP, bmap);
> > > +	case ARM_SMCCC_OWNER_OEM:
> > > +		return test_bit(KVM_ARM_USER_HYPERCALL_OWNER_OEM, bmap);
> > > +	case ARM_SMCCC_OWNER_STANDARD:
> > > +		return test_bit(KVM_ARM_USER_HYPERCALL_OWNER_STANDARD, bmap);
> > > +	case ARM_SMCCC_OWNER_STANDARD_HYP:
> > > +		return test_bit(KVM_ARM_USER_HYPERCALL_OWNER_STANDARD_HYP, bmap);
> > > +	case ARM_SMCCC_OWNER_VENDOR_HYP:
> > > +		return test_bit(KVM_ARM_USER_HYPERCALL_OWNER_VENDOR_HYP, bmap);
> > > +	case ARM_SMCCC_OWNER_TRUSTED_APP ... ARM_SMCCC_OWNER_TRUSTED_APP_END:
> > > +		return test_bit(KVM_ARM_USER_HYPERCALL_OWNER_TRUSTED_APP, bmap);
> > > +	case ARM_SMCCC_OWNER_TRUSTED_OS ... ARM_SMCCC_OWNER_TRUSTED_OS_END:
> > > +		return test_bit(KVM_ARM_USER_HYPERCALL_OWNER_TRUSTED_OS, bmap);
> > > +	default:
> > > +		return false;
> > > +	}
> > 
> > You have multiple problems here:
> > 
> > - the granularity is way too coarse. You want to express arbitrary
> >   ranges, and not necessarily grab a whole owner range.
> > 
> > - you have now an overlap between ranges that are handled in the
> >   kernel (PSCI, spectre mitigations) and ranges that userspace wants
> >   to observe. Not good.
> 
> We need to come to agreement on what degree of mix-and-match should be
> supported.
> 
> Spectre really ought to be in the kernel, and I don't think anyone is
> particularly excited about reimplementing PSCI. Right now my interest
> in this starts and ends with forwarding the vendor-specific hypercall
> range to userspace, allowing something like Hyper-V PV on KVM.
> 
> > If we are going down this road, this can only be done at the
> > *function* level. And userspace must know that the kernel will refuse
> > to forward some ranges.
> 
> The goal of what I was trying to get at is that either the kernel or
> userspace takes ownership of a range that has an ABI, but not both. i.e.
> you really wouldn't want some VMM or cloud provider trapping portions of
> KVM's vendor-specific range while still reporting a 'vanilla' ABI at the
> time of discovery. Same goes for PSCI, TRNG, etc.

But I definitely think this is one of the major use cases. For
example, there is value in taking PSCI to userspace in order to
implement a newer version of the spec, or to support sub-features that
KVM doesn't (want to) implement. I don't think this changes the ABI
from the guest perspective.

pKVM also has a use case for this where userspace gets a notification
of the hypercall that a guest has performed to share memory.

Communication with a TEE also is on the cards, as would be a FFA
implementation. All of this could be implemented in KVM, or in
userspace, depending what users of these misfeatures want to do.

> 
> > So obviously, this cannot be a simple bitmap. Making it a radix tree
> > (or an xarray, which is basically the same thing) could work. And the
> > filtering request from userspace can be similar to what we have for
> > the PMU filters.
> 
> Right, we'll need a more robust data structure for all this.
> 
> My only concern is that communicating the hypercall filter between
> user/kernel with a set of ranges or function numbers is that we could be
> mutating what KVM *doesn't* already implement into an ABI of sorts.
> 
> i.e. suppose that userspace wants to filter function(s) in an
> unallocated/unused range of function numbers. Later down the line KVM
> adds support for a new shiny thing and the filter becomes a subset of a
> now allocated range of calls. We then reject the filter due to the
> incongruence.

But isn't the problem to ask for ranges that are unallocated the first
place? What semantic can userspace give to such a thing other than
replying "not implemented", which is what the kernel would do anyway?

The more interesting problem is when you want to emulate another
hypervisor, and that the vendor spaces overlap (a very likely
outcome). Somehow, this means overriding all the KVM-specific
hypercalls, and let userspace deal with it. But again, this can be
done on a per function basis.

Thanks,

	M.

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

Re: [RFC PATCH 2/3] KVM: arm64: Allow userspace to trap SMCCC sub-ranges

[Oliver Upton]

On Fri, Nov 11, 2022 at 08:26:02AM +0000, Marc Zyngier wrote:
> On Thu, 10 Nov 2022 21:13:54 +0000, Oliver Upton <oliver.upton@linux.dev> wrote:
> > The goal of what I was trying to get at is that either the kernel or
> > userspace takes ownership of a range that has an ABI, but not both. i.e.
> > you really wouldn't want some VMM or cloud provider trapping portions of
> > KVM's vendor-specific range while still reporting a 'vanilla' ABI at the
> > time of discovery. Same goes for PSCI, TRNG, etc.
> 
> But I definitely think this is one of the major use cases. For
> example, there is value in taking PSCI to userspace in order to
> implement a newer version of the spec, or to support sub-features that
> KVM doesn't (want to) implement. I don't think this changes the ABI from
> the guest perspective.

I disagree for the implications of partially trapping the 'Vendor
Specific Hypervisor Service'. If the UID for the range still reports KVM
but userspace decided to add some new widget, then from the guest
perspective that widget is now part of KVM's own ABI with the guest.

Trapping the whole range is a bit of a hack to workaround the need for
more complicated verification of a hypercall filter.

But for everything else, I'm fine with arbitrary function filtering.
Userspace is always welcome to shoot itself in the foot.

> pKVM also has a use case for this where userspace gets a notification
> of the hypercall that a guest has performed to share memory.

Is that hypercall in the 'Vendor Specific Hypervisor Service' range?

> Communication with a TEE also is on the cards, as would be a FFA
> implementation. All of this could be implemented in KVM, or in
> userspace, depending what users of these misfeatures want to do.

I'm very hopeful that by forwarding all of this to userspace we can get
out of the business of implementing every darn spec that comes along.

> > > So obviously, this cannot be a simple bitmap. Making it a radix tree
> > > (or an xarray, which is basically the same thing) could work. And the
> > > filtering request from userspace can be similar to what we have for
> > > the PMU filters.
> > 
> > Right, we'll need a more robust data structure for all this.
> > 
> > My only concern is that communicating the hypercall filter between
> > user/kernel with a set of ranges or function numbers is that we could be
> > mutating what KVM *doesn't* already implement into an ABI of sorts.
> > 
> > i.e. suppose that userspace wants to filter function(s) in an
> > unallocated/unused range of function numbers. Later down the line KVM
> > adds support for a new shiny thing and the filter becomes a subset of a
> > now allocated range of calls. We then reject the filter due to the
> > incongruence.
> 
> But isn't the problem to ask for ranges that are unallocated the first
> place? What semantic can userspace give to such a thing other than
> replying "not implemented", which is what the kernel would do anyway?

... assuming we know about all defined services in the kernel. I don't
care about it too much, but there is the case about a new userspace on
an old kernel.

> The more interesting problem is when you want to emulate another
> hypervisor, and that the vendor spaces overlap (a very likely
> outcome). Somehow, this means overriding all the KVM-specific
> hypercalls, and let userspace deal with it. But again, this can be
> done on a per function basis.

Other hypercalls removed, would you be in favor of an all-or-nothing
rule for KVM's vendor range implementation? Of course, that could still
be enforced on whatever UAPI approach we take.

--
Thanks,
Oliver

Re: [RFC PATCH 2/3] KVM: arm64: Allow userspace to trap SMCCC sub-ranges

[Marc Zyngier]

On Fri, 11 Nov 2022 23:39:09 +0000,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> On Fri, Nov 11, 2022 at 08:26:02AM +0000, Marc Zyngier wrote:
> > On Thu, 10 Nov 2022 21:13:54 +0000, Oliver Upton <oliver.upton@linux.dev> wrote:
> > > The goal of what I was trying to get at is that either the kernel or
> > > userspace takes ownership of a range that has an ABI, but not both. i.e.
> > > you really wouldn't want some VMM or cloud provider trapping portions of
> > > KVM's vendor-specific range while still reporting a 'vanilla' ABI at the
> > > time of discovery. Same goes for PSCI, TRNG, etc.
> > 
> > But I definitely think this is one of the major use cases. For
> > example, there is value in taking PSCI to userspace in order to
> > implement a newer version of the spec, or to support sub-features that
> > KVM doesn't (want to) implement. I don't think this changes the ABI from
> > the guest perspective.
> 
> I disagree for the implications of partially trapping the 'Vendor
> Specific Hypervisor Service'. If the UID for the range still reports KVM
> but userspace decided to add some new widget, then from the guest
> perspective that widget is now part of KVM's own ABI with the guest.

But that's what I mean by "I don't think this changes the ABI from the
guest perspective". The guest cannot know who is doing the emulation
anyway, so it is userspace's duty to preserve the illusion. At the
end of the day, this is only a configuration mechanism, and it is no
different from all other configuration bits (i.e. they need to be
identical on both side for migration).

> Trapping the whole range is a bit of a hack to workaround the need for
> more complicated verification of a hypercall filter.

We already need these things for architected hypercalls. Once we have
the infrastructure, it doesn't matter anymore which range this is for.

> 
> But for everything else, I'm fine with arbitrary function filtering.
> Userspace is always welcome to shoot itself in the foot.
> 
> > pKVM also has a use case for this where userspace gets a notification
> > of the hypercall that a guest has performed to share memory.
> 
> Is that hypercall in the 'Vendor Specific Hypervisor Service' range?

Yes. It is get another KVM hypercall.

> 
> > Communication with a TEE also is on the cards, as would be a FFA
> > implementation. All of this could be implemented in KVM, or in
> > userspace, depending what users of these misfeatures want to do.
> 
> I'm very hopeful that by forwarding all of this to userspace we can get
> out of the business of implementing every darn spec that comes along.

Good luck. All the TEEs have private, home grown APIs, and every
vendor will want to implement their own crap (i.e. there is no spec).

	M.

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

Re: [RFC PATCH 2/3] KVM: arm64: Allow userspace to trap SMCCC sub-ranges

[Will Deacon]

Hey Oliver,

On Thu, Nov 10, 2022 at 01:53:26AM +0000, Oliver Upton wrote:
> As the SMCCC (and related specifications) march towards an
> 'everything and the kitchen sink' interface for interacting with a
> system, it is less likely that KVM will implement every supported
> feature.
> 
> Add a capability that allows userspace to trap hypercall ranges,
> allowing the VMM to mix-and-match between calls handled in userspace vs.
> KVM.
> 
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
>  arch/arm64/include/asm/kvm_host.h |  5 ++++
>  arch/arm64/include/uapi/asm/kvm.h | 15 ++++++++++
>  arch/arm64/kvm/arm.c              | 10 +++++++
>  arch/arm64/kvm/hypercalls.c       | 48 +++++++++++++++++++++++++++++++
>  include/uapi/linux/kvm.h          |  1 +
>  5 files changed, 79 insertions(+)

[...]

> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 6f0b56e7f8c7..6e8a222fc295 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -100,6 +100,13 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
>  		r = 0;
>  		set_bit(KVM_ARCH_FLAG_SYSTEM_SUSPEND_ENABLED, &kvm->arch.flags);
>  		break;
> +	case KVM_CAP_ARM_USER_HYPERCALLS:
> +		if (cap->args[0] & ~KVM_ARM_USER_HYPERCALL_FLAGS)
> +			return -EINVAL;

Why not use KVM_CAP_EXIT_HYPERCALL for this? At some point during pKVM
development, we used that to notify the VMM about memory being shared
back from the guest but we eventually dropped it as the notification to
userspace ended up not being needed:

https://android-kvm.googlesource.com/linux/+/dbd2861832dfc4c8a3103214b3c212ee7ace1c44%5E%21/
https://android-kvm.googlesource.com/linux/+/2a3afc6da99c0e0cb62be1687153ee572903aa80%5E%21/

I'm not saying that what we did was necessarily better, but it seems a bit
simpler and I figured it might be useful to point you to it.

Will

Re: [RFC PATCH 2/3] KVM: arm64: Allow userspace to trap SMCCC sub-ranges

[Oliver Upton]

On Fri, Nov 18, 2022 at 02:56:38PM +0000, Will Deacon wrote:
> On Thu, Nov 10, 2022 at 01:53:26AM +0000, Oliver Upton wrote:
> > As the SMCCC (and related specifications) march towards an
> > 'everything and the kitchen sink' interface for interacting with a
> > system, it is less likely that KVM will implement every supported
> > feature.
> > 
> > Add a capability that allows userspace to trap hypercall ranges,
> > allowing the VMM to mix-and-match between calls handled in userspace vs.
> > KVM.
> > 
> > Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> > ---
> >  arch/arm64/include/asm/kvm_host.h |  5 ++++
> >  arch/arm64/include/uapi/asm/kvm.h | 15 ++++++++++
> >  arch/arm64/kvm/arm.c              | 10 +++++++
> >  arch/arm64/kvm/hypercalls.c       | 48 +++++++++++++++++++++++++++++++
> >  include/uapi/linux/kvm.h          |  1 +
> >  5 files changed, 79 insertions(+)
> 
> [...]
> 
> > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> > index 6f0b56e7f8c7..6e8a222fc295 100644
> > --- a/arch/arm64/kvm/arm.c
> > +++ b/arch/arm64/kvm/arm.c
> > @@ -100,6 +100,13 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
> >  		r = 0;
> >  		set_bit(KVM_ARCH_FLAG_SYSTEM_SUSPEND_ENABLED, &kvm->arch.flags);
> >  		break;
> > +	case KVM_CAP_ARM_USER_HYPERCALLS:
> > +		if (cap->args[0] & ~KVM_ARM_USER_HYPERCALL_FLAGS)
> > +			return -EINVAL;
> 
> Why not use KVM_CAP_EXIT_HYPERCALL for this?

Err... I hilariously hijacked its UAPI for the exit but added a new cap
for it :)

I think the direction going forward will be to provide userspace with a
range-based filter such that (to a degree) we can arbitrarily forward
hypercalls to userspace, allowing for a mix-and-match approach.

> At some point during pKVM
> development, we used that to notify the VMM about memory being shared
> back from the guest but we eventually dropped it as the notification to
> userspace ended up not being needed:
> 
> https://android-kvm.googlesource.com/linux/+/dbd2861832dfc4c8a3103214b3c212ee7ace1c44%5E%21/
> https://android-kvm.googlesource.com/linux/+/2a3afc6da99c0e0cb62be1687153ee572903aa80%5E%21/
> 
> I'm not saying that what we did was necessarily better, but it seems a bit
> simpler and I figured it might be useful to point you to it.

Yeah, this is certainly a lot cleaner than what I've proposed here. And
frankly, for my immediate interest (forwarding vendor hypercalls to
userspace), this would fit the bill. OTOH, I was hoping that something
a bit more flexible could move the onus of implementing every darn spec
onto userspace (where possible).

I know you said pKVM has no need for userspace notifications at this
moment, but could user hypercalls be useful again going forward?

--
Thanks,
Oliver

[RFC PATCH 3/3] KVM: selftests: Test user hypercalls

[Oliver Upton]

Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 tools/testing/selftests/kvm/.gitignore        |   1 +
 tools/testing/selftests/kvm/Makefile          |   1 +
 .../selftests/kvm/aarch64/user_hypercalls.c   | 130 ++++++++++++++++++
 3 files changed, 132 insertions(+)
 create mode 100644 tools/testing/selftests/kvm/aarch64/user_hypercalls.c

diff --git a/tools/testing/selftests/kvm/.gitignore b/tools/testing/selftests/kvm/.gitignore
index 2f0d705db9db..4f45e987985f 100644
--- a/tools/testing/selftests/kvm/.gitignore
+++ b/tools/testing/selftests/kvm/.gitignore
@@ -5,6 +5,7 @@
 /aarch64/get-reg-list
 /aarch64/hypercalls
 /aarch64/psci_test
+/aarch64/user_hypercalls
 /aarch64/vcpu_width_config
 /aarch64/vgic_init
 /aarch64/vgic_irq
diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index 0172eb6cb6ee..8ac1988ea669 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -153,6 +153,7 @@ 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/user_hypercalls
 TEST_GEN_PROGS_aarch64 += aarch64/vcpu_width_config
 TEST_GEN_PROGS_aarch64 += aarch64/vgic_init
 TEST_GEN_PROGS_aarch64 += aarch64/vgic_irq
diff --git a/tools/testing/selftests/kvm/aarch64/user_hypercalls.c b/tools/testing/selftests/kvm/aarch64/user_hypercalls.c
new file mode 100644
index 000000000000..94ac821c5474
--- /dev/null
+++ b/tools/testing/selftests/kvm/aarch64/user_hypercalls.c
@@ -0,0 +1,130 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/arm-smccc.h>
+
+#include "kvm_util.h"
+#include "processor.h"
+#include "test_util.h"
+
+struct test_case {
+	uint64_t	cap;
+	uint32_t	function;
+	uint64_t	args[6];
+};
+
+#define TEST_OWNER(name)							\
+{										\
+	.cap		= BIT_ULL(KVM_ARM_USER_HYPERCALL_OWNER_ ## name),	\
+	.function	= ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,		\
+					     ARM_SMCCC_SMC_32,			\
+					     ARM_SMCCC_OWNER_ ## name,		\
+					     0),				\
+	.args		= {							\
+		__LINE__,							\
+		__LINE__ + 1,							\
+		__LINE__ + 2,							\
+		__LINE__ + 3,							\
+		__LINE__ + 4,							\
+		__LINE__ + 5,							\
+	},									\
+},										\
+{										\
+	.cap		= BIT_ULL(KVM_ARM_USER_HYPERCALL_OWNER_ ## name),	\
+	.function	= ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,		\
+					     ARM_SMCCC_SMC_64,			\
+					     ARM_SMCCC_OWNER_ ## name,		\
+					     0),				\
+	.args		= {							\
+		__LINE__,							\
+		__LINE__ + 1,							\
+		__LINE__ + 2,							\
+		__LINE__ + 3,							\
+		__LINE__ + 4,							\
+		__LINE__ + 5,							\
+	},									\
+}										\
+
+static struct test_case test_cases[] = {
+	TEST_OWNER(ARCH),
+	TEST_OWNER(CPU),
+	TEST_OWNER(SIP),
+	TEST_OWNER(OEM),
+	TEST_OWNER(STANDARD),
+	TEST_OWNER(STANDARD_HYP),
+	TEST_OWNER(VENDOR_HYP),
+	TEST_OWNER(TRUSTED_APP),
+	TEST_OWNER(TRUSTED_OS),
+};
+
+static void guest_main(const struct test_case *test)
+{
+	struct arm_smccc_res unused;
+
+	smccc_hvc(test->function, test->args[0], test->args[1], test->args[2],
+		  test->args[3], test->args[4], test->args[5], 0, &unused);
+
+	GUEST_ASSERT(0);
+}
+
+static void handle_hvc(const struct test_case *test, struct kvm_vcpu *vcpu)
+{
+	struct kvm_run *run = vcpu->run;
+
+	TEST_ASSERT(run->hypercall.nr == test->function,
+		    "unexpected function ID: %llx (expected %x)",
+		    run->hypercall.nr, test->function);
+
+	TEST_ASSERT(!memcmp(&run->hypercall.args, &test->args, sizeof(test->args)),
+		    "unexpected hypercall arguments");
+}
+
+static void handle_ucall(struct kvm_vcpu *vcpu)
+{
+	struct ucall uc;
+
+	switch (get_ucall(vcpu, &uc)) {
+	case UCALL_ABORT:
+		REPORT_GUEST_ASSERT(uc);
+		break;
+	default:
+		TEST_FAIL("unhandled ucall: %lu", uc.cmd);
+	}
+}
+
+static void run_test(const struct test_case *test)
+{
+	struct kvm_vcpu *vcpu;
+	struct kvm_vm *vm;
+
+	vm = vm_create_with_one_vcpu(&vcpu, guest_main);
+	vm_enable_cap(vm, KVM_CAP_ARM_USER_HYPERCALLS, test->cap);
+	ucall_init(vm, NULL);
+
+	vcpu_args_set(vcpu, 1, test);
+
+	vcpu_run(vcpu);
+	switch (vcpu->run->exit_reason) {
+	case KVM_EXIT_HYPERCALL:
+		handle_hvc(test, vcpu);
+		break;
+	case KVM_EXIT_MMIO:
+		handle_ucall(vcpu);
+		break;
+	default:
+		TEST_FAIL("unhandled exit reason: %u (%s)", vcpu->run->exit_reason,
+			  exit_reason_str(vcpu->run->exit_reason));
+	}
+
+	ucall_uninit(vm);
+	kvm_vm_free(vm);
+}
+
+int main(void)
+{
+	int i;
+
+	TEST_REQUIRE(kvm_has_cap(KVM_CAP_ARM_USER_HYPERCALLS));
+
+	for (i = 0; i < ARRAY_SIZE(test_cases); i++)
+		run_test(&test_cases[i]);
+}
-- 
2.38.1.431.g37b22c650d-goog