[RFC PATCH v2 0/6] KVM: arm64: Userspace SMCCC call filtering

[RFC PATCH v2 0/6] KVM: arm64: Userspace SMCCC call filtering

[Oliver Upton]

The Arm SMCCC is rather prescriptive in regards to the allocation of
SMCCC function ID ranges. Many of the hypercall ranges have an
associated specification from Arm (FF-A, PSCI, SDEI, etc.) with some
room for vendor-specific implementations.

The ever-expanding SMCCC surface leaves a lot of work within KVM for
providing new features. Furthermore, KVM implements its own
vendor-specific ABI, with little room for other implementations (like
Hyper-V, for example).

Not only that, it would appear that vCPU hotplug [1] has a legitimate
use case for something like this, sending PSCI calls to userspace (where
they should have gone in the first place).

=> We have these new hypercall bitmap registers, why not use that?

The hypercall bitmap registers aren't necessarily aimed at the same
problem. The bitmap registers allow a VMM to preserve the ABI the guest
gets from KVM by default when migrating between hosts. By default KVM
exposes the entire feature set to the guest, whereas user SMCCC calls
need explicit opt-in from userspace.

Applies to 6.2-rc3.

TODO:
 - Reject the ranges of hypercalls we don't want userspace to handle.
   Spectre crud mainly, any others?

   I plan on using the invariant of the maple tree to reject filters
   that intersect with a reserved range.

 - Should exits for SMC calls have the PC pre-incremented to align with
   HVC? Go read the comment in handle_smc() if you aren't following.

   I think the answer is 'yes', but opinions welcome as always :)

 - This series unifies the SMCCC space for HVCs and SMCs but this
   requires a lot more thought. Otherwise, we can add support for two
   separate namespaces.

 - Testing! I only got as far as compiling this on my machine. At
   minimum a decent selftest is requried considering the UAPI here is
   rather involved.

RFC v1 -> v2:
 - Use a range-based interface instead of filtering entire services
 - Stop using the braindead term of 'trapping' in relation to userspace.

Oliver Upton (6):
  KVM: arm64: Add a helper to check if a VM has ran once
  KVM: arm64: Add vm fd device attribute accessors
  KVM: arm64: Refactor hvc filtering to support different actions
  KVM: arm64: Use a maple tree to represent the SMCCC filter
  KVM: arm64: Add support for KVM_EXIT_HYPERCALL
  KVM: arm64: Indroduce support for userspace SMCCC filtering

 Documentation/virt/kvm/api.rst        |  24 +++-
 Documentation/virt/kvm/devices/vm.rst |  67 ++++++++++
 arch/arm64/include/asm/kvm_host.h     |   8 +-
 arch/arm64/include/uapi/asm/kvm.h     |  31 +++++
 arch/arm64/kvm/arm.c                  |  35 +++++
 arch/arm64/kvm/handle_exit.c          |  12 +-
 arch/arm64/kvm/hypercalls.c           | 176 +++++++++++++++++++++++++-
 arch/arm64/kvm/pmu-emul.c             |   4 +-
 include/kvm/arm_hypercalls.h          |   5 +
 include/uapi/linux/kvm.h              |   2 +-
 10 files changed, 350 insertions(+), 14 deletions(-)


base-commit: b7bfaa761d760e72a969d116517eaa12e404c262
-- 
2.39.1.581.gbfd45094c4-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC PATCH v2 1/6] KVM: arm64: Add a helper to check if a VM has ran once

[Oliver Upton]

The test_bit(...) pattern is quite a lot of keystrokes. Replace
existing callsites with a helper.

No functional change intended.

Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/include/asm/kvm_host.h | 3 +++
 arch/arm64/kvm/pmu-emul.c         | 4 ++--
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 35a159d131b5..012e94bc9e4a 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -1019,6 +1019,9 @@ bool kvm_arm_vcpu_is_finalized(struct kvm_vcpu *vcpu);
 	(system_supports_32bit_el0() &&				\
 	 !static_branch_unlikely(&arm64_mismatched_32bit_el0))
 
+#define kvm_vm_has_ran_once(kvm)					\
+	(test_bit(KVM_ARCH_FLAG_HAS_RAN_ONCE, &(kvm)->arch.flags))
+
 int kvm_trng_call(struct kvm_vcpu *vcpu);
 #ifdef CONFIG_KVM
 extern phys_addr_t hyp_mem_base;
diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index 24908400e190..a0fc569fdbca 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -880,7 +880,7 @@ static int kvm_arm_pmu_v3_set_pmu(struct kvm_vcpu *vcpu, int pmu_id)
 	list_for_each_entry(entry, &arm_pmus, entry) {
 		arm_pmu = entry->arm_pmu;
 		if (arm_pmu->pmu.type == pmu_id) {
-			if (test_bit(KVM_ARCH_FLAG_HAS_RAN_ONCE, &kvm->arch.flags) ||
+			if (kvm_vm_has_ran_once(kvm) ||
 			    (kvm->arch.pmu_filter && kvm->arch.arm_pmu != arm_pmu)) {
 				ret = -EBUSY;
 				break;
@@ -963,7 +963,7 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
 
 		mutex_lock(&kvm->lock);
 
-		if (test_bit(KVM_ARCH_FLAG_HAS_RAN_ONCE, &kvm->arch.flags)) {
+		if (kvm_vm_has_ran_once(kvm)) {
 			mutex_unlock(&kvm->lock);
 			return -EBUSY;
 		}
-- 
2.39.1.581.gbfd45094c4-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH v2 1/6] KVM: arm64: Add a helper to check if a VM has ran once

[Sean Christopherson]

On Sat, Feb 11, 2023, Oliver Upton wrote:
> The test_bit(...) pattern is quite a lot of keystrokes. Replace
> existing callsites with a helper.
> 
> No functional change intended.
> 
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
>  arch/arm64/include/asm/kvm_host.h | 3 +++
>  arch/arm64/kvm/pmu-emul.c         | 4 ++--
>  2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 35a159d131b5..012e94bc9e4a 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -1019,6 +1019,9 @@ bool kvm_arm_vcpu_is_finalized(struct kvm_vcpu *vcpu);
>  	(system_supports_32bit_el0() &&				\
>  	 !static_branch_unlikely(&arm64_mismatched_32bit_el0))
>  
> +#define kvm_vm_has_ran_once(kvm)					\

From the peanut gallery...

The ONCE part of the flag+API is unnecessary and flawed from a pendatic point of
view, e.g. if a VM has ran twice...

What about kvm_vm_has_run() to align with a similar proposed x86 API for individual
vCPUs[*], if either one ever gets moved to common code?

[*] https://lore.kernel.org/all/20230210003148.2646712-3-seanjc@google.com

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH v2 1/6] KVM: arm64: Add a helper to check if a VM has ran once

[Marc Zyngier]

On Mon, 13 Feb 2023 15:36:52 +0000,
Sean Christopherson <seanjc@google.com> wrote:
> 
> On Sat, Feb 11, 2023, Oliver Upton wrote:
> > The test_bit(...) pattern is quite a lot of keystrokes. Replace
> > existing callsites with a helper.
> > 
> > No functional change intended.
> > 
> > Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> > ---
> >  arch/arm64/include/asm/kvm_host.h | 3 +++
> >  arch/arm64/kvm/pmu-emul.c         | 4 ++--
> >  2 files changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > index 35a159d131b5..012e94bc9e4a 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -1019,6 +1019,9 @@ bool kvm_arm_vcpu_is_finalized(struct kvm_vcpu *vcpu);
> >  	(system_supports_32bit_el0() &&				\
> >  	 !static_branch_unlikely(&arm64_mismatched_32bit_el0))
> >  
> > +#define kvm_vm_has_ran_once(kvm)					\
> 
> From the peanut gallery...
> 
> The ONCE part of the flag+API is unnecessary and flawed from a pendatic point of
> view, e.g. if a VM has ran twice...

Well, what I really wanted was:

kvm_vm_has_run_at_least_once_on_this_side_of_the_multiverse()

> What about kvm_vm_has_run() to align with a similar proposed x86 API for individual
> vCPUs[*], if either one ever gets moved to common code?

I think the original wording is understood by the very people who mess
with this code, most of whom are not even native English speakers. It
may not be pretty to your eyes, but hey, I'm not pretty either.

	M.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC PATCH v2 2/6] KVM: arm64: Add vm fd device attribute accessors

[Oliver Upton]

A subsequent change will allow userspace to convey a filter for
hypercalls through a vm device attribute. Add the requisite boilerplate
for vm attribute accessors.

Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/kvm/arm.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index 9c5573bc4614..09efa893e03a 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -1449,11 +1449,28 @@ static int kvm_vm_ioctl_set_device_addr(struct kvm *kvm,
 	}
 }
 
+static int kvm_vm_has_attr(struct kvm *kvm, struct kvm_device_attr *attr)
+{
+	switch (attr->group) {
+	default:
+		return -ENXIO;
+	}
+}
+
+static int kvm_vm_set_attr(struct kvm *kvm, struct kvm_device_attr *attr)
+{
+	switch (attr->group) {
+	default:
+		return -ENXIO;
+	}
+}
+
 long kvm_arch_vm_ioctl(struct file *filp,
 		       unsigned int ioctl, unsigned long arg)
 {
 	struct kvm *kvm = filp->private_data;
 	void __user *argp = (void __user *)arg;
+	struct kvm_device_attr attr;
 
 	switch (ioctl) {
 	case KVM_CREATE_IRQCHIP: {
@@ -1489,6 +1506,18 @@ long kvm_arch_vm_ioctl(struct file *filp,
 			return -EFAULT;
 		return kvm_vm_ioctl_mte_copy_tags(kvm, &copy_tags);
 	}
+	case KVM_HAS_DEVICE_ATTR: {
+		if (copy_from_user(&attr, argp, sizeof(attr)))
+			return -EFAULT;
+
+		return kvm_vm_has_attr(kvm, &attr);
+	}
+	case KVM_SET_DEVICE_ATTR: {
+		if (copy_from_user(&attr, argp, sizeof(attr)))
+			return -EFAULT;
+
+		return kvm_vm_set_attr(kvm, &attr);
+	}
 	default:
 		return -EINVAL;
 	}
-- 
2.39.1.581.gbfd45094c4-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC PATCH v2 3/6] KVM: arm64: Refactor hvc filtering to support different actions

[Oliver Upton]

KVM presently allows userspace to filter guest hypercalls with bitmaps
expressed via pseudo-firmware registers. These bitmaps have a narrow
scope and, of course, can only allow/deny a particular call. A
subsequent change to KVM will introduce a generalized UAPI for filtering
hypercalls, allowing functions to be forwarded to userspace.

Refactor the existing hypercall filtering logic to make room for more
than two actions. While at it, generalize the function names around
SMCCC as it is the basis for the upcoming UAPI.

No functional change intended.

Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/include/uapi/asm/kvm.h |  9 +++++++++
 arch/arm64/kvm/hypercalls.c       | 19 +++++++++++++++----
 2 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
index a7a857f1784d..e298574a45ea 100644
--- a/arch/arm64/include/uapi/asm/kvm.h
+++ b/arch/arm64/include/uapi/asm/kvm.h
@@ -468,6 +468,15 @@ enum {
 /* run->fail_entry.hardware_entry_failure_reason codes. */
 #define KVM_EXIT_FAIL_ENTRY_CPU_UNSUPPORTED	(1ULL << 0)
 
+enum kvm_smccc_filter_action {
+	KVM_SMCCC_FILTER_ALLOW = 0,
+	KVM_SMCCC_FILTER_DENY,
+
+#ifdef __KERNEL__
+	NR_SMCCC_FILTER_ACTIONS
+#endif
+};
+
 #endif
 
 #endif /* __ARM_KVM_H__ */
diff --git a/arch/arm64/kvm/hypercalls.c b/arch/arm64/kvm/hypercalls.c
index c9f401fa01a9..980546b295b3 100644
--- a/arch/arm64/kvm/hypercalls.c
+++ b/arch/arm64/kvm/hypercalls.c
@@ -65,7 +65,7 @@ static void kvm_ptp_get_time(struct kvm_vcpu *vcpu, u64 *val)
 	val[3] = lower_32_bits(cycles);
 }
 
-static bool kvm_hvc_call_default_allowed(u32 func_id)
+static bool kvm_smccc_default_call(u32 func_id)
 {
 	switch (func_id) {
 	/*
@@ -93,7 +93,7 @@ static bool kvm_hvc_call_default_allowed(u32 func_id)
 	}
 }
 
-static bool kvm_hvc_call_allowed(struct kvm_vcpu *vcpu, u32 func_id)
+static bool kvm_smccc_test_fw_bmap(struct kvm_vcpu *vcpu, u32 func_id)
 {
 	struct kvm_smccc_features *smccc_feat = &vcpu->kvm->arch.smccc_feat;
 
@@ -117,19 +117,30 @@ static bool kvm_hvc_call_allowed(struct kvm_vcpu *vcpu, u32 func_id)
 		return test_bit(KVM_REG_ARM_VENDOR_HYP_BIT_PTP,
 				&smccc_feat->vendor_hyp_bmap);
 	default:
-		return kvm_hvc_call_default_allowed(func_id);
+		return false;
 	}
 }
 
+static u8 kvm_hvc_get_action(struct kvm_vcpu *vcpu, u32 func_id)
+{
+	if (kvm_smccc_default_call(func_id) ||
+	    kvm_smccc_test_fw_bmap(vcpu, func_id))
+		return KVM_SMCCC_FILTER_ALLOW;
+
+	return KVM_SMCCC_FILTER_DENY;
+}
+
 int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
 {
 	struct kvm_smccc_features *smccc_feat = &vcpu->kvm->arch.smccc_feat;
 	u32 func_id = smccc_get_function(vcpu);
 	u64 val[4] = {SMCCC_RET_NOT_SUPPORTED};
 	u32 feature;
+	u8 action;
 	gpa_t gpa;
 
-	if (!kvm_hvc_call_allowed(vcpu, func_id))
+	action = kvm_hvc_get_action(vcpu, func_id);
+	if (action == KVM_SMCCC_FILTER_DENY)
 		goto out;
 
 	switch (func_id) {
-- 
2.39.1.581.gbfd45094c4-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC PATCH v2 4/6] KVM: arm64: Use a maple tree to represent the SMCCC filter

[Oliver Upton]

Maple tree is an efficient B-tree implementation that is intended for
storing non-overlapping intervals. Such a data structure is a good fit
for the SMCCC filter as it is desirable to sparsely allocate the 32 bit
function ID space.

To that end, add a maple tree to kvm_arch and correctly init/teardown
along with the VM. Wire in a test against the hypercall filter for HVCs
which does nothing until the controls are exposed to userspace.

Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/include/asm/kvm_host.h |  5 +++-
 arch/arm64/kvm/arm.c              |  2 ++
 arch/arm64/kvm/hypercalls.c       | 38 +++++++++++++++++++++++++++++++
 include/kvm/arm_hypercalls.h      |  1 +
 4 files changed, 45 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 012e94bc9e4a..a20875dde1f5 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -16,6 +16,7 @@
 #include <linux/types.h>
 #include <linux/jump_label.h>
 #include <linux/kvm_types.h>
+#include <linux/maple_tree.h>
 #include <linux/percpu.h>
 #include <linux/psci.h>
 #include <asm/arch_gicv3.h>
@@ -213,7 +214,8 @@ struct kvm_arch {
 #define KVM_ARCH_FLAG_EL1_32BIT				4
 	/* PSCI SYSTEM_SUSPEND enabled for the guest */
 #define KVM_ARCH_FLAG_SYSTEM_SUSPEND_ENABLED		5
-
+	/* SMCCC filter initialized for the VM */
+#define KVM_ARCH_FLAG_SMCCC_FILTER_CONFIGURED		6
 	unsigned long flags;
 
 	/*
@@ -234,6 +236,7 @@ struct kvm_arch {
 
 	/* Hypercall features firmware registers' descriptor */
 	struct kvm_smccc_features smccc_feat;
+	struct maple_tree smccc_filter;
 
 	/*
 	 * For an untrusted host VM, 'pkvm.handle' is used to lookup
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index 09efa893e03a..e04690caad77 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -202,6 +202,8 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
 	kvm_destroy_vcpus(kvm);
 
 	kvm_unshare_hyp(kvm, kvm + 1);
+
+	kvm_arm_teardown_hypercalls(kvm);
 }
 
 int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
diff --git a/arch/arm64/kvm/hypercalls.c b/arch/arm64/kvm/hypercalls.c
index 980546b295b3..45b8371816fd 100644
--- a/arch/arm64/kvm/hypercalls.c
+++ b/arch/arm64/kvm/hypercalls.c
@@ -121,8 +121,39 @@ static bool kvm_smccc_test_fw_bmap(struct kvm_vcpu *vcpu, u32 func_id)
 	}
 }
 
+static bool kvm_smccc_filter_configured(struct kvm *kvm)
+{
+	return test_bit(KVM_ARCH_FLAG_SMCCC_FILTER_CONFIGURED, &kvm->arch.flags);
+}
+
+static u8 kvm_smccc_filter_get_action(struct kvm *kvm, u32 func_id)
+{
+	unsigned long index = func_id;
+	void *val;
+
+	if (!kvm_smccc_filter_configured(kvm))
+		return KVM_SMCCC_FILTER_ALLOW;
+
+	/*
+	 * But where's the error handling, you say?
+	 *
+	 * mt_find() returns NULL if no entry was found, which just so happens
+	 * to match KVM_SMCCC_FILTER_ALLOW.
+	 */
+	val = mt_find(&kvm->arch.smccc_filter, &index, index);
+	return xa_to_value(val);
+}
+
 static u8 kvm_hvc_get_action(struct kvm_vcpu *vcpu, u32 func_id)
 {
+	/*
+	 * Intervening actions in the SMCCC filter take precedence over the
+	 * pseudo-firmware register bitmaps.
+	 */
+	u8 action = kvm_smccc_filter_get_action(vcpu->kvm, func_id);
+	if (action != KVM_SMCCC_FILTER_ALLOW)
+		return action;
+
 	if (kvm_smccc_default_call(func_id) ||
 	    kvm_smccc_test_fw_bmap(vcpu, func_id))
 		return KVM_SMCCC_FILTER_ALLOW;
@@ -256,6 +287,13 @@ void kvm_arm_init_hypercalls(struct kvm *kvm)
 	smccc_feat->std_bmap = KVM_ARM_SMCCC_STD_FEATURES;
 	smccc_feat->std_hyp_bmap = KVM_ARM_SMCCC_STD_HYP_FEATURES;
 	smccc_feat->vendor_hyp_bmap = KVM_ARM_SMCCC_VENDOR_HYP_FEATURES;
+
+	mt_init(&kvm->arch.smccc_filter);
+}
+
+void kvm_arm_teardown_hypercalls(struct kvm *kvm)
+{
+	mtree_destroy(&kvm->arch.smccc_filter);
 }
 
 int kvm_arm_get_fw_num_regs(struct kvm_vcpu *vcpu)
diff --git a/include/kvm/arm_hypercalls.h b/include/kvm/arm_hypercalls.h
index 1188f116cf4e..ae1ada68cdc2 100644
--- a/include/kvm/arm_hypercalls.h
+++ b/include/kvm/arm_hypercalls.h
@@ -43,6 +43,7 @@ static inline void smccc_set_retval(struct kvm_vcpu *vcpu,
 struct kvm_one_reg;
 
 void kvm_arm_init_hypercalls(struct kvm *kvm);
+void kvm_arm_teardown_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);
-- 
2.39.1.581.gbfd45094c4-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC PATCH v2 5/6] KVM: arm64: Add support for KVM_EXIT_HYPERCALL

[Oliver Upton]

In anticipation of user hypercall filters, add the necessary plumbing to
get SMCCC calls out to userspace. Even though the exit structure has
space for KVM to pass register arguments, let's just avoid it altogether
and let userspace poke at the registers via KVM_GET_ONE_REG.

This deliberately stretches the definition of a 'hypercall' to cover
SMCs from EL1 in addition to the HVCs we know and love. KVM doesn't
support EL1 calls into secure services, but now we can paint that as a
userspace problem and be done with it.

Finally, we need a flag to let userspace know what conduit instruction
was used (i.e. SMC vs. HVC). Redefine the remaining padding in
kvm_run::hypercall to accomplish this. Let's all take a moment
to admire the flowers and see how 'longmode' tied up a full u32 in the
UAPI. Weep.

Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 Documentation/virt/kvm/api.rst    | 24 +++++++++++++++++++++---
 arch/arm64/include/uapi/asm/kvm.h |  4 ++++
 arch/arm64/kvm/handle_exit.c      | 12 +++++++++---
 arch/arm64/kvm/hypercalls.c       | 31 +++++++++++++++++++++++++++++++
 include/kvm/arm_hypercalls.h      |  1 +
 include/uapi/linux/kvm.h          |  2 +-
 6 files changed, 67 insertions(+), 7 deletions(-)

diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
index deb494f759ed..c82da1a84590 100644
--- a/Documentation/virt/kvm/api.rst
+++ b/Documentation/virt/kvm/api.rst
@@ -6119,14 +6119,32 @@ to the byte array.
 			__u64 args[6];
 			__u64 ret;
 			__u32 longmode;
-			__u32 pad;
+			__u32 flags;
 		} hypercall;
 
-Unused.  This was once used for 'hypercall to userspace'.  To implement
-such functionality, use KVM_EXIT_IO (x86) or KVM_EXIT_MMIO (all except s390).
+
+It is strongly recommended that userspace use ``KVM_EXIT_IO`` (x86) or
+``KVM_EXIT_MMIO`` (all except s390) to implement functionality that
+requires a guest to interact with host userpace.
 
 .. note:: KVM_EXIT_IO is significantly faster than KVM_EXIT_MMIO.
 
+For arm64:
+----------
+
+SMCCC exits can be enabled depending on the configuration of the SMCCC
+filter. See the Documentation/virt/kvm/devices/vm.rst
+``KVM_ARM_SMCCC_FILTER`` for more details.
+
+``nr`` contains the function ID of the guest's SMCCC call. Userspace is
+expected to use the ``KVM_GET_ONE_REG`` ioctl to retrieve the call
+parameters from the vCPU's GPRs.
+
+Definition of ``flags``:
+ - ``KVM_HYPERCALL_EXIT_SMC``: Indicates that the guest used the SMC
+   conduit to initiate the SMCCC call. If this bit is 0 then the guest
+   used the HVC conduit for the SMCCC call.
+
 ::
 
 		/* KVM_EXIT_TPR_ACCESS */
diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
index e298574a45ea..5c98e7c39ba1 100644
--- a/arch/arm64/include/uapi/asm/kvm.h
+++ b/arch/arm64/include/uapi/asm/kvm.h
@@ -471,12 +471,16 @@ enum {
 enum kvm_smccc_filter_action {
 	KVM_SMCCC_FILTER_ALLOW = 0,
 	KVM_SMCCC_FILTER_DENY,
+	KVM_SMCCC_FILTER_FWD_TO_USER,
 
 #ifdef __KERNEL__
 	NR_SMCCC_FILTER_ACTIONS
 #endif
 };
 
+/* arm64-specific KVM_EXIT_HYPERCALL flags */
+#define KVM_HYPERCALL_EXIT_SMC	(1U << 0)
+
 #endif
 
 #endif /* __ARM_KVM_H__ */
diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
index e778eefcf214..d15ff6c795e1 100644
--- a/arch/arm64/kvm/handle_exit.c
+++ b/arch/arm64/kvm/handle_exit.c
@@ -52,6 +52,8 @@ static int handle_hvc(struct kvm_vcpu *vcpu)
 
 static int handle_smc(struct kvm_vcpu *vcpu)
 {
+	int ret = kvm_smc_call_handler(vcpu);
+
 	/*
 	 * "If an SMC instruction executed at Non-secure EL1 is
 	 * trapped to EL2 because HCR_EL2.TSC is 1, the exception is a
@@ -60,9 +62,13 @@ static int handle_smc(struct kvm_vcpu *vcpu)
 	 * We need to advance the PC after the trap, as it would
 	 * otherwise return to the same address...
 	 */
-	vcpu_set_reg(vcpu, 0, ~0UL);
-	kvm_incr_pc(vcpu);
-	return 1;
+	if (ret < 0) {
+		vcpu_set_reg(vcpu, 0, ~0UL);
+		kvm_incr_pc(vcpu);
+		return 1;
+	}
+
+	return ret;
 }
 
 /*
diff --git a/arch/arm64/kvm/hypercalls.c b/arch/arm64/kvm/hypercalls.c
index 45b8371816fd..f095c048730a 100644
--- a/arch/arm64/kvm/hypercalls.c
+++ b/arch/arm64/kvm/hypercalls.c
@@ -161,6 +161,17 @@ static u8 kvm_hvc_get_action(struct kvm_vcpu *vcpu, u32 func_id)
 	return KVM_SMCCC_FILTER_DENY;
 }
 
+static void kvm_prepare_hypercall_exit(struct kvm_vcpu *vcpu, u32 func_id, bool smc)
+{
+	struct kvm_run *run = vcpu->run;
+
+	run->exit_reason = KVM_EXIT_HYPERCALL;
+	run->hypercall.nr = func_id;
+
+	if (smc)
+		run->hypercall.flags = KVM_HYPERCALL_EXIT_SMC;
+}
+
 int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
 {
 	struct kvm_smccc_features *smccc_feat = &vcpu->kvm->arch.smccc_feat;
@@ -173,6 +184,10 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
 	action = kvm_hvc_get_action(vcpu, func_id);
 	if (action == KVM_SMCCC_FILTER_DENY)
 		goto out;
+	if (action == KVM_SMCCC_FILTER_FWD_TO_USER) {
+		kvm_prepare_hypercall_exit(vcpu, func_id, false);
+		return 0;
+	}
 
 	switch (func_id) {
 	case ARM_SMCCC_VERSION_FUNC_ID:
@@ -270,6 +285,22 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
 	return 1;
 }
 
+int kvm_smc_call_handler(struct kvm_vcpu *vcpu)
+{
+	u32 func_id = smccc_get_function(vcpu);
+	u8 action = kvm_smccc_filter_get_action(vcpu->kvm, func_id);
+
+	/*
+	 * KVM doesn't support SMCs from EL1, so reject the call if userspace
+	 * doesn't want it.
+	 */
+	if (action != KVM_SMCCC_FILTER_FWD_TO_USER)
+		return -ENOSYS;
+
+	kvm_prepare_hypercall_exit(vcpu, func_id, true);
+	return 0;
+}
+
 static const u64 kvm_arm_fw_reg_ids[] = {
 	KVM_REG_ARM_PSCI_VERSION,
 	KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1,
diff --git a/include/kvm/arm_hypercalls.h b/include/kvm/arm_hypercalls.h
index ae1ada68cdc2..4707aa206c4e 100644
--- a/include/kvm/arm_hypercalls.h
+++ b/include/kvm/arm_hypercalls.h
@@ -7,6 +7,7 @@
 #include <asm/kvm_emulate.h>
 
 int kvm_hvc_call_handler(struct kvm_vcpu *vcpu);
+int kvm_smc_call_handler(struct kvm_vcpu *vcpu);
 
 static inline u32 smccc_get_function(struct kvm_vcpu *vcpu)
 {
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 55155e262646..0ae7cf8ca2db 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -342,7 +342,7 @@ struct kvm_run {
 			__u64 args[6];
 			__u64 ret;
 			__u32 longmode;
-			__u32 pad;
+			__u32 flags;
 		} hypercall;
 		/* KVM_EXIT_TPR_ACCESS */
 		struct {
-- 
2.39.1.581.gbfd45094c4-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH v2 5/6] KVM: arm64: Add support for KVM_EXIT_HYPERCALL

[Sean Christopherson]

On Sat, Feb 11, 2023, Oliver Upton wrote:
> Finally, we need a flag to let userspace know what conduit instruction
> was used (i.e. SMC vs. HVC). Redefine the remaining padding in
> kvm_run::hypercall to accomplish this. Let's all take a moment
> to admire the flowers and see how 'longmode' tied up a full u32 in the
> UAPI. Weep.

I don't think it has to, at least not for other architectures.  No other arch
currently supports KVM_EXIT_HYPERCALL, so breakage is extremely unlikely.  E.g.
we can likely get away with this, and then just have x86 add new flags at bit 32.

diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 55155e262646..7d3ad820d55c 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -341,8 +341,14 @@ struct kvm_run {
                        __u64 nr;
                        __u64 args[6];
                        __u64 ret;
-                       __u32 longmode;
-                       __u32 pad;
+                       union {
+                               /*
+                                * Long Mode, a.k.a. 64-bit mode, takes up the
+                                * first 32 flags on x86 (historical sludge).
+                                */
+                               __u32 longmode;
+                               __u64 flags;
+                       };
                } hypercall;
                /* KVM_EXIT_TPR_ACCESS */
                struct {
 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH v2 5/6] KVM: arm64: Add support for KVM_EXIT_HYPERCALL

[Oliver Upton]

On Mon, Feb 13, 2023 at 04:01:29PM +0000, Sean Christopherson wrote:
> On Sat, Feb 11, 2023, Oliver Upton wrote:
> > Finally, we need a flag to let userspace know what conduit instruction
> > was used (i.e. SMC vs. HVC). Redefine the remaining padding in
> > kvm_run::hypercall to accomplish this. Let's all take a moment
> > to admire the flowers and see how 'longmode' tied up a full u32 in the
> > UAPI. Weep.
> 
> I don't think it has to, at least not for other architectures.

And surrender the opportunity to write a smartass commit message? I
think not.


> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 55155e262646..7d3ad820d55c 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -341,8 +341,14 @@ struct kvm_run {
>                         __u64 nr;
>                         __u64 args[6];
>                         __u64 ret;
> -                       __u32 longmode;
> -                       __u32 pad;
> +                       union {
> +                               /*
> +                                * Long Mode, a.k.a. 64-bit mode, takes up the
> +                                * first 32 flags on x86 (historical sludge).
> +                                */
> +                               __u32 longmode;
> +                               __u64 flags;
> +                       };

You could pull further shenanigans with the x86 residue by guarding
longmode with #ifndef __KERNEL__ and using a macro to raise the
'longmode' flag.

Then we can all pretend it never existed :)

-- 
Thanks,
Oliver

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH v2 5/6] KVM: arm64: Add support for KVM_EXIT_HYPERCALL

[James Morse]

Hi Oliver,

On 11/02/2023 01:37, Oliver Upton wrote:
> In anticipation of user hypercall filters, add the necessary plumbing to
> get SMCCC calls out to userspace. Even though the exit structure has
> space for KVM to pass register arguments, let's just avoid it altogether
> and let userspace poke at the registers via KVM_GET_ONE_REG.
> 
> This deliberately stretches the definition of a 'hypercall' to cover
> SMCs from EL1 in addition to the HVCs we know and love. KVM doesn't
> support EL1 calls into secure services, but now we can paint that as a
> userspace problem and be done with it.
> 
> Finally, we need a flag to let userspace know what conduit instruction
> was used (i.e. SMC vs. HVC). Redefine the remaining padding in
> kvm_run::hypercall to accomplish this. Let's all take a moment
> to admire the flowers and see how 'longmode' tied up a full u32 in the
> UAPI. Weep.

> diff --git a/arch/arm64/kvm/hypercalls.c b/arch/arm64/kvm/hypercalls.c
> index 45b8371816fd..f095c048730a 100644
> --- a/arch/arm64/kvm/hypercalls.c
> +++ b/arch/arm64/kvm/hypercalls.c
> @@ -161,6 +161,17 @@ static u8 kvm_hvc_get_action(struct kvm_vcpu *vcpu, u32 func_id)
>  	return KVM_SMCCC_FILTER_DENY;
>  }
>  
> +static void kvm_prepare_hypercall_exit(struct kvm_vcpu *vcpu, u32 func_id, bool smc)
> +{
> +	struct kvm_run *run = vcpu->run;
> +
> +	run->exit_reason = KVM_EXIT_HYPERCALL;

> +	run->hypercall.nr = func_id;

This is a bit weird. The func_id is the x0 value, so it would more naturally live in
run->hypercall.args[0].

User-space also needs the SMC/HVC immediate value, as that is only available in the ESR.
It makes more sense to put the immediate value here.


> +	if (smc)
> +		run->hypercall.flags = KVM_HYPERCALL_EXIT_SMC;
> +}
> +
>  int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
>  {
>  	struct kvm_smccc_features *smccc_feat = &vcpu->kvm->arch.smccc_feat;


Thanks,

James

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH v2 5/6] KVM: arm64: Add support for KVM_EXIT_HYPERCALL

[Oliver Upton]

Hi James,

On Fri, Feb 24, 2023 at 03:12:33PM +0000, James Morse wrote:
> On 11/02/2023 01:37, Oliver Upton wrote:
> > +static void kvm_prepare_hypercall_exit(struct kvm_vcpu *vcpu, u32 func_id, bool smc)
> > +{
> > +	struct kvm_run *run = vcpu->run;
> > +
> > +	run->exit_reason = KVM_EXIT_HYPERCALL;
> 
> > +	run->hypercall.nr = func_id;
> 
> This is a bit weird. The func_id is the x0 value, so it would more naturally live in
> run->hypercall.args[0].
> 
> User-space also needs the SMC/HVC immediate value, as that is only available in the ESR.
> It makes more sense to put the immediate value here.

Completely buy that userspace has no way of getting at the immediate.
But why do we need to expose it in the first place?

The UAPI here has been constructed around SMCCC, so the immediate should
be zero across the board. Sure, SMCCC says that nonzero values can be
used by the hypervisor, but in that case the register interface needn't
follow SMCCC (i.e. what if the function ID is in x7 for some silly
reason).

Curious if there's a use case you had in mind I haven't thought of.

> > +	if (smc)
> > +		run->hypercall.flags = KVM_HYPERCALL_EXIT_SMC;

Drive by self-review (be warned!): flags needs explicit zeroing,
otherwise this flag will remain up for a subsequent HVC.

-- 
Thanks,
Oliver

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC PATCH v2 6/6] KVM: arm64: Indroduce support for userspace SMCCC filtering

[Oliver Upton]

As the SMCCC (and related specifications) march towards an 'everything
and the kitchen sink' interface for interacting with a system it becomes
less likely that KVM will support every related feature. We could do
better by letting userspace have a crack at it instead.

Allow userspace to define an 'SMCCC filter' that applies to both HVCs
and SMCs initiated by the guest. Supporting both conduits with this
interface is important for a couple of reasons. Guest SMC usage is table
stakes for a nested guest, as HVCs are always taken to the virtual EL2.
Additionally, guests may want to interact with a service on the secure
side which can now be proxied by userspace.

Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 Documentation/virt/kvm/devices/vm.rst | 67 ++++++++++++++++++++
 arch/arm64/include/uapi/asm/kvm.h     | 18 ++++++
 arch/arm64/kvm/arm.c                  |  4 ++
 arch/arm64/kvm/hypercalls.c           | 88 +++++++++++++++++++++++++++
 include/kvm/arm_hypercalls.h          |  3 +
 5 files changed, 180 insertions(+)

diff --git a/Documentation/virt/kvm/devices/vm.rst b/Documentation/virt/kvm/devices/vm.rst
index 60acc39e0e93..564ecc7cff0d 100644
--- a/Documentation/virt/kvm/devices/vm.rst
+++ b/Documentation/virt/kvm/devices/vm.rst
@@ -317,3 +317,70 @@ Allows userspace to query the status of migration mode.
 	     if it is enabled
 :Returns:   -EFAULT if the given address is not accessible from kernel space;
 	    0 in case of success.
+
+6. GROUP: KVM_ARM_VM_SMCCC_CTRL
+===============================
+
+:Architectures: arm64
+
+6.1. ATTRIBUTE: KVM_ARM_VM_SMCCC_FILTER (w/o)
+---------------------------------------------
+
+:Parameters: Pointer to a ``struct kvm_smccc_filter``
+
+:Returns:
+
+        =======  ===========================================
+        -EEXIST  SMCCC filter already configured for the VM
+        -EBUSY   A vCPU in the VM has already run
+        -EINVAL  Invalid filter configuration
+        -ENOMEM  Failed to allocate memory for the in-kernel
+                 representation of the SMCCC filter
+        =======  ===========================================
+
+Request the installation of an SMCCC call filter described as follows::
+
+    enum kvm_smccc_filter_action {
+            KVM_SMCCC_FILTER_ALLOW = 0,
+            KVM_SMCCC_FILTER_DENY,
+            KVM_SMCCC_FILTER_FWD_TO_USER,
+    };
+
+    struct kvm_smccc_filter_range {
+            __u32 base;
+            __u32 nr_functions;
+            __u8 action;
+            __u8 pad[7];
+    };
+
+    struct kvm_smccc_filter {
+            __u32 nr_entries;
+            __u32 pad;
+
+            struct kvm_smccc_filter_range entries[];
+    };
+
+The filter is defined as a set of non-overlapping ranges. Each
+range defines an action to be applied to SMCCC calls within the range.
+The default configuration of KVM is such that all implemented SMCCC
+calls are allowed by default. Thus, the SMCCC filter can be defined
+sparsely by userspace, only describing ranges that modify the default
+behavior.
+
+The range expressed by ``struct kvm_smccc_filter_range`` is
+[@base, @base + @nr_functions).
+
+The SMCCC filter applies to both SMC and HVC calls initiated by the
+guest.
+
+Actions:
+
+ - ``KVM_SMCCC_FILTER_ALLOW``: Allows the guest SMCCC call to be
+   handled in-kernel. It is strongly recommended that userspace *not*
+   explicitly describe the allowed SMCCC call ranges.
+
+ - ``KVM_SMCCC_FILTER_DENY``: Rejects the guest SMCCC call in-kernel
+   and returns to the guest.
+
+ - ``KVM_SMCCC_FILTER_FWD_TO_USER``: The guest SMCCC call is forwarded
+   to userspace with an exit reason of ``KVM_EXIT_HYPERCALL``.
diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
index 5c98e7c39ba1..5973031c959d 100644
--- a/arch/arm64/include/uapi/asm/kvm.h
+++ b/arch/arm64/include/uapi/asm/kvm.h
@@ -371,6 +371,10 @@ enum {
 #endif
 };
 
+/* Device Control API on vm fd */
+#define KVM_ARM_VM_SMCCC_CTRL		0
+#define   KVM_ARM_VM_SMCCC_FILTER	0
+
 /* Device Control API: ARM VGIC */
 #define KVM_DEV_ARM_VGIC_GRP_ADDR	0
 #define KVM_DEV_ARM_VGIC_GRP_DIST_REGS	1
@@ -478,6 +482,20 @@ enum kvm_smccc_filter_action {
 #endif
 };
 
+struct kvm_smccc_filter_range {
+	__u32 base;
+	__u32 nr_functions;
+	__u8 action;
+	__u8 pad[7];
+};
+
+struct kvm_smccc_filter {
+	__u32 nr_entries;
+	__u32 pad;
+
+	struct kvm_smccc_filter_range entries[];
+};
+
 /* arm64-specific KVM_EXIT_HYPERCALL flags */
 #define KVM_HYPERCALL_EXIT_SMC	(1U << 0)
 
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index e04690caad77..3d64f025ccba 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -1454,6 +1454,8 @@ static int kvm_vm_ioctl_set_device_addr(struct kvm *kvm,
 static int kvm_vm_has_attr(struct kvm *kvm, struct kvm_device_attr *attr)
 {
 	switch (attr->group) {
+	case KVM_ARM_VM_SMCCC_CTRL:
+		return kvm_vm_smccc_has_attr(kvm, attr);
 	default:
 		return -ENXIO;
 	}
@@ -1462,6 +1464,8 @@ static int kvm_vm_has_attr(struct kvm *kvm, struct kvm_device_attr *attr)
 static int kvm_vm_set_attr(struct kvm *kvm, struct kvm_device_attr *attr)
 {
 	switch (attr->group) {
+	case KVM_ARM_VM_SMCCC_CTRL:
+		return kvm_vm_smccc_set_attr(kvm, attr);
 	default:
 		return -ENXIO;
 	}
diff --git a/arch/arm64/kvm/hypercalls.c b/arch/arm64/kvm/hypercalls.c
index f095c048730a..1984df78a307 100644
--- a/arch/arm64/kvm/hypercalls.c
+++ b/arch/arm64/kvm/hypercalls.c
@@ -126,6 +126,72 @@ static bool kvm_smccc_filter_configured(struct kvm *kvm)
 	return test_bit(KVM_ARCH_FLAG_SMCCC_FILTER_CONFIGURED, &kvm->arch.flags);
 }
 
+static int kvm_smccc_filter_insert_range(struct kvm *kvm,
+					 struct kvm_smccc_filter_range *range)
+{
+	struct maple_tree *mt = &kvm->arch.smccc_filter;
+	unsigned long start = range->base;
+	unsigned long end = start + range->nr_functions - 1;
+	int r;
+
+	if (!range->nr_functions || range->action >= NR_SMCCC_FILTER_ACTIONS)
+		return -EINVAL;
+
+	r = mtree_insert_range(mt, start, end, xa_mk_value(range->action), GFP_KERNEL_ACCOUNT);
+	if (r == -EEXIST)
+		return -EINVAL;
+
+	return r;
+}
+
+static int kvm_smccc_set_filter(struct kvm *kvm, struct kvm_smccc_filter __user *uaddr)
+{
+	struct kvm_smccc_filter_range *entries;
+	struct kvm_smccc_filter filter;
+	int i, r;
+
+	if (copy_from_user(&filter, uaddr, sizeof(filter)))
+		return -EFAULT;
+
+	entries = memdup_user(&uaddr->entries, sizeof(*entries) * filter.nr_entries);
+	if (IS_ERR(entries))
+		return PTR_ERR(entries);
+
+	mutex_lock(&kvm->lock);
+
+	if (kvm_vm_has_ran_once(kvm)) {
+		r = -EBUSY;
+		goto out;
+	}
+
+	if (kvm_smccc_filter_configured(kvm)) {
+		r = -EEXIST;
+		goto out;
+	}
+
+	for (i = 0; i < filter.nr_entries; i++) {
+		struct kvm_smccc_filter_range *range = &entries[i];
+
+		r = kvm_smccc_filter_insert_range(kvm, range);
+		if (r)
+			goto out_reset_mt;
+	}
+
+	set_bit(KVM_ARCH_FLAG_SMCCC_FILTER_CONFIGURED, &kvm->arch.flags);
+
+	mutex_unlock(&kvm->lock);
+	kfree(entries);
+	return 0;
+
+out_reset_mt:
+	mtree_destroy(&kvm->arch.smccc_filter);
+	mt_init(&kvm->arch.smccc_filter);
+out:
+	kfree(entries);
+	mutex_unlock(&kvm->lock);
+	return r;
+}
+
 static u8 kvm_smccc_filter_get_action(struct kvm *kvm, u32 func_id)
 {
 	unsigned long index = func_id;
@@ -559,3 +625,25 @@ int kvm_arm_set_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 
 	return -EINVAL;
 }
+
+int kvm_vm_smccc_has_attr(struct kvm *kvm, struct kvm_device_attr *attr)
+{
+	switch (attr->attr) {
+	case KVM_ARM_VM_SMCCC_FILTER:
+		return 0;
+	default:
+		return -ENXIO;
+	}
+}
+
+int kvm_vm_smccc_set_attr(struct kvm *kvm, struct kvm_device_attr *attr)
+{
+	void __user *uaddr = (void __user *)attr->addr;
+
+	switch (attr->attr) {
+	case KVM_ARM_VM_SMCCC_FILTER:
+		return kvm_smccc_set_filter(kvm, uaddr);
+	default:
+		return -ENXIO;
+	}
+}
diff --git a/include/kvm/arm_hypercalls.h b/include/kvm/arm_hypercalls.h
index 4707aa206c4e..d01cef2983ee 100644
--- a/include/kvm/arm_hypercalls.h
+++ b/include/kvm/arm_hypercalls.h
@@ -50,4 +50,7 @@ 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_vm_smccc_has_attr(struct kvm *kvm, struct kvm_device_attr *attr);
+int kvm_vm_smccc_set_attr(struct kvm *kvm, struct kvm_device_attr *attr);
+
 #endif
-- 
2.39.1.581.gbfd45094c4-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH v2 6/6] KVM: arm64: Indroduce support for userspace SMCCC filtering

[Oliver Upton]

On Sat, Feb 11, 2023 at 01:37:59AM +0000, Oliver Upton wrote:

[...]

> +    struct kvm_smccc_filter_range {
> +            __u32 base;
> +            __u32 nr_functions;
> +            __u8 action;
> +            __u8 pad[7];
> +    };
> +
> +    struct kvm_smccc_filter {
> +            __u32 nr_entries;
> +            __u32 pad;
> +
> +            struct kvm_smccc_filter_range entries[];
> +    };

After dealing with this when writing the selftest, I feel like this is a
bit over complicated for both userspace and kernel.

Next revision will take a similar approach to the PMU event filter,
allowing userspace to insert new ranges into the filter with successive
calls to the interface.

-- 
Thanks,
Oliver

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH v2 0/6] KVM: arm64: Userspace SMCCC call filtering

[James Morse]

Hi Oliver,

On 11/02/2023 01:37, Oliver Upton wrote:
> The Arm SMCCC is rather prescriptive in regards to the allocation of
> SMCCC function ID ranges. Many of the hypercall ranges have an
> associated specification from Arm (FF-A, PSCI, SDEI, etc.) with some
> room for vendor-specific implementations.
> 
> The ever-expanding SMCCC surface leaves a lot of work within KVM for
> providing new features. Furthermore, KVM implements its own
> vendor-specific ABI, with little room for other implementations (like
> Hyper-V, for example).
> 
> Not only that, it would appear that vCPU hotplug [1] has a legitimate
> use case for something like this, sending PSCI calls to userspace (where
> they should have gone in the first place).
> 
> => We have these new hypercall bitmap registers, why not use that?
> 
> The hypercall bitmap registers aren't necessarily aimed at the same
> problem. The bitmap registers allow a VMM to preserve the ABI the guest
> gets from KVM by default when migrating between hosts. By default KVM
> exposes the entire feature set to the guest, whereas user SMCCC calls
> need explicit opt-in from userspace.
> 
> Applies to 6.2-rc3.


> TODO:
>  - Reject the ranges of hypercalls we don't want userspace to handle.
>    Spectre crud mainly, any others?

We can predict what future 'ARCH_WORKAROUND_foo' values will be in the future, as they
have to be generated by a single instruction. I think its worth preventing user-space from
using any of those.

I don't see how user-space could possibly implement stolen time correctly ... but I don'
think we should prevent it trying.

The 'features' calls are going to be a headache, especially when the features call in one
range gives results about calls in a different range. (e.g. you query PSCI_FEATURES to
find if SMCCC_VERSION is supported). I'm working on a reference implementation for kvmtool
to show we don't regress any of the existing SMC-CC supoprt.


>    I plan on using the invariant of the maple tree to reject filters
>    that intersect with a reserved range.
> 
>  - Should exits for SMC calls have the PC pre-incremented to align with
>    HVC? Go read the comment in handle_smc() if you aren't following.
> 
>    I think the answer is 'yes', but opinions welcome as always :)

I don't think there is a compelling argument either way. But please document whether
user-space must increment the PC, or must not!


>  - This series unifies the SMCCC space for HVCs and SMCs but this
>    requires a lot more thought. Otherwise, we can add support for two
>    separate namespaces.

I checked with ATG, they think the function IDs are one space, and have no intention of
having different APIs for the same function-id behind HVC/SMC.

They pointed to 'SMCCC issue E, appendix D' which says hypervisors are expected to trap
SMC, both conduits go to the same 'managing EL'.


>  - Testing! I only got as far as compiling this on my machine. At
>    minimum a decent selftest is requried considering the UAPI here is
>    rather involved.

I've got PSCI support in kvmtool, (including cpu-suspend), I intend to try and test as
much of SMC-CC as I can.

I'll rebase the virtual-cpu hotplug stuff onto this, Salil should be able to give some
feedback from the Qemu side.


Thanks,

James

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH v2 0/6] KVM: arm64: Userspace SMCCC call filtering

[Oliver Upton]

Hi James,

On Fri, Feb 24, 2023 at 03:12:08PM +0000, James Morse wrote:
> Hi Oliver,
> 
> On 11/02/2023 01:37, Oliver Upton wrote:
> > The Arm SMCCC is rather prescriptive in regards to the allocation of
> > SMCCC function ID ranges. Many of the hypercall ranges have an
> > associated specification from Arm (FF-A, PSCI, SDEI, etc.) with some
> > room for vendor-specific implementations.
> > 
> > The ever-expanding SMCCC surface leaves a lot of work within KVM for
> > providing new features. Furthermore, KVM implements its own
> > vendor-specific ABI, with little room for other implementations (like
> > Hyper-V, for example).
> > 
> > Not only that, it would appear that vCPU hotplug [1] has a legitimate
> > use case for something like this, sending PSCI calls to userspace (where
> > they should have gone in the first place).
> > 
> > => We have these new hypercall bitmap registers, why not use that?
> > 
> > The hypercall bitmap registers aren't necessarily aimed at the same
> > problem. The bitmap registers allow a VMM to preserve the ABI the guest
> > gets from KVM by default when migrating between hosts. By default KVM
> > exposes the entire feature set to the guest, whereas user SMCCC calls
> > need explicit opt-in from userspace.
> > 
> > Applies to 6.2-rc3.
> 
> 
> > TODO:
> >  - Reject the ranges of hypercalls we don't want userspace to handle.
> >    Spectre crud mainly, any others?
> 
> We can predict what future 'ARCH_WORKAROUND_foo' values will be in the future, as they
> have to be generated by a single instruction. I think its worth preventing user-space from
> using any of those.

Agreed.

> I don't see how user-space could possibly implement stolen time correctly ... but I don'
> think we should prevent it trying.

Yeah, unless there's a real hazard to getting userspace involved (like
above) then I see no issue in letting the VMM have at it.

> The 'features' calls are going to be a headache, especially when the features call in one
> range gives results about calls in a different range. (e.g. you query PSCI_FEATURES to
> find if SMCCC_VERSION is supported). I'm working on a reference implementation for kvmtool
> to show we don't regress any of the existing SMC-CC supoprt.

Goodness, thanks for taking a stab at the userspace angle of this.

I've been mulling on this issue for a while. I originally wanted the
UAPI to consume the whole set of subranges described in the filter at
once to impose some degree of validation, but I worry that'll become
unnecessarily complex (so I will definitely get it wrong).

Short of a better solution for the problem I'm fine saying it is
entirely a userspace problem to present a sensible feature set. No
matter what we do the problem of incorrectly advertising to the guest
will exist with user hypercalls, possibly in a range KVM doesn't
know/care about.

> 
> >    I plan on using the invariant of the maple tree to reject filters
> >    that intersect with a reserved range.
> > 
> >  - Should exits for SMC calls have the PC pre-incremented to align with
> >    HVC? Go read the comment in handle_smc() if you aren't following.
> > 
> >    I think the answer is 'yes', but opinions welcome as always :)
> 
> I don't think there is a compelling argument either way. But please document whether
> user-space must increment the PC, or must not!

I'm going forward with pre-increment, both for consistency with HVCs and
avoiding additional ioctls to manually increment PC from userspace.
Documentation to boot!

> >  - This series unifies the SMCCC space for HVCs and SMCs but this
> >    requires a lot more thought. Otherwise, we can add support for two
> >    separate namespaces.
> 
> I checked with ATG, they think the function IDs are one space, and have no intention of
> having different APIs for the same function-id behind HVC/SMC.
> 
> They pointed to 'SMCCC issue E, appendix D' which says hypervisors are expected to trap
> SMC, both conduits go to the same 'managing EL'.

Excellent, thank you for getting clarification on that. What I'm also
reading here is that KVM is indeed wrong in its unwillingness to handle
SMCCC calls from EL1 :)

> 
> >  - Testing! I only got as far as compiling this on my machine. At
> >    minimum a decent selftest is requried considering the UAPI here is
> >    rather involved.
> 
> I've got PSCI support in kvmtool, (including cpu-suspend), I intend to try and test as
> much of SMC-CC as I can.

Thanks, that is tremendously helpful. Bonus points if userspace PSCI
handling is enabled unconditionally and not just for hotplug :)

-- 
Thanks,
Oliver

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel