KVM: arm64: Userspace SMCCC call filtering
Subsystems (4)
v2 cover KVM: arm64: Userspace SMCCC call filtering
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
Signature · 7 lines
2.39.1.581.gbfd45094c4-goog _______________________________________________ linux-arm-kernel mailing list [email protected] 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
The test_bit(...) pattern is quite a lot of keystrokes. Replace existing callsites with a helper.
No functional change intended.
--- arch/arm64/include/asm/kvm_host.h | 3 +++ arch/arm64/kvm/pmu-emul.c | 4 ++-- 2 files changed, 5 insertions(+), 2 deletions(-)
arch/arm64/include/asm/kvm_host.h +3 -0
@@ -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;
arch/arm64/kvm/pmu-emul.c +2 -2
@@ -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 ; }
Signature · 7 lines
2.39.1.581.gbfd45094c4-goog _______________________________________________ linux-arm-kernel mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Sean Christopherson
reviewerRe: [RFC PATCH v2 1/6] KVM: arm64: Add a helper to check if a VM has ran once
Quoted 16 lines · 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 <[email protected]>--- arch/arm64/include/asm/kvm_host.h | 3 +++ arch/arm64/kvm/pmu-emul.c | 4 ++-- 2 files changed, 5 insertions(+), 2 deletions(-)arch/arm64/include/asm/kvm_host.h +0
-0
arch/arm64/include/asm/kvm_host.h bool kvm_arm_vcpu_is_finalized(struct kvm_vcpu *vcpu); · line 1022
Quoted 4 lines
arch/arm64/include/asm/kvm_host.h +1
-0@@ -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?
Marc Zyngier
maintainerRe: [RFC PATCH v2 1/6] KVM: arm64: Add a helper to check if a VM has ran once
Quoted 21 lines · Sean Christopherson
Quoted 19 lines · 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 <[email protected]>--- arch/arm64/include/asm/kvm_host.h | 3 +++ arch/arm64/kvm/pmu-emul.c | 4 ++-- 2 files changed, 5 insertions(+), 2 deletions(-)arch/arm64/include/asm/kvm_host.h +0
-0@@ -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))
Quoted 6 lines
arch/arm64/include/asm/kvm_host.h bool kvm_arm_vcpu_is_finalized(struct kvm_vcpu *vcpu); · line 1022
Quoted 1 line
arch/arm64/include/asm/kvm_host.h +1
-0+#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()
Quoted 2 lines
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.
Signature · 1 line
Without deviation from the norm, progress is not possible.
RFC PATCH v2 2/6 KVM: arm64: Add vm fd device attribute accessors
In reply to Oliver UptonA subsequent change will allow userspace to convey a filter for hypercalls through a vm device attribute. Add the requisite boilerplate for vm attribute accessors.
--- arch/arm64/kvm/arm.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)
arch/arm64/kvm/arm.c +29 -0
@@ -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, ©_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 ; }
Signature · 7 lines
2.39.1.581.gbfd45094c4-goog _______________________________________________ linux-arm-kernel mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
RFC PATCH v2 3/6 KVM: arm64: Refactor hvc filtering to support different actions
In reply to Oliver UptonKVM 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.
--- arch/arm64/include/uapi/asm/kvm.h | 9 +++++++++ arch/arm64/kvm/hypercalls.c | 19 +++++++++++++++---- 2 files changed, 24 insertions(+), 4 deletions(-)
arch/arm64/include/uapi/asm/kvm.h +9 -0
@@ -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__ */
arch/arm64/kvm/hypercalls.c +15 -4
@@ -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) {
Signature · 7 lines
2.39.1.581.gbfd45094c4-goog _______________________________________________ linux-arm-kernel mailing list [email protected] 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
In reply to Oliver UptonMaple 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.
--- 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(-)
arch/arm64/include/asm/kvm_host.h +4 -1
@@ -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 6unsigned 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
arch/arm64/kvm/arm.c +2 -0
@@ -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)
arch/arm64/kvm/hypercalls.c +38 -0
@@ -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)
include/kvm/arm_hypercalls.h +1 -0
@@ -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);
Signature · 7 lines
2.39.1.581.gbfd45094c4-goog _______________________________________________ linux-arm-kernel mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
RFC PATCH v2 5/6 KVM: arm64: Add support for KVM_EXIT_HYPERCALL
In reply to Oliver UptonIn 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.
--- 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(-)
Documentation/virt/kvm/api.rst +21 -3
@@ -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 */
arch/arm64/include/uapi/asm/kvm.h +4 -0
@@ -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__ */
arch/arm64/kvm/handle_exit.c +9 -3
@@ -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; }/*
arch/arm64/kvm/hypercalls.c +31 -0
@@ -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 ,
include/kvm/arm_hypercalls.h +1 -0
@@ -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) {
include/uapi/linux/kvm.h +1 -1
@@ -342,7 +342,7 @@ struct kvm_run {__u64 args[6 ];__u64 ret;__u32 longmode; -__u32 pad; +__u32 flags; } hypercall;/* KVM_EXIT_TPR_ACCESS */ struct {
Signature · 7 lines
2.39.1.581.gbfd45094c4-goog _______________________________________________ linux-arm-kernel mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Sean Christopherson
reviewerRe: [RFC PATCH v2 5/6] KVM: arm64: Add support for KVM_EXIT_HYPERCALL
Quoted 5 lines · Oliver Upton
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::hypercallto 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.
include/uapi/linux/kvm.h +8 -2
@@ -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 {
Re: [RFC PATCH v2 5/6] KVM: arm64: Add support for KVM_EXIT_HYPERCALL
Quoted 8 lines · Sean Christopherson
Quoted 5 lines · Oliver Upton
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::hypercallto 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.
Quoted 8 lines
include/uapi/linux/kvm.h +0
-0@@ -341,8 +341,14 @@ struct kvm_run {__u64 nr;__u64 args[6 ];__u64 ret;
include/uapi/linux/kvm.h struct kvm_run { · line 351
Quoted 10 lines
include/uapi/linux/kvm.h +8
-2-__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 :)
Signature · 7 lines
Thanks, Oliver _______________________________________________ linux-arm-kernel mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Re: [RFC PATCH v2 5/6] KVM: arm64: Add support for KVM_EXIT_HYPERCALL
In reply to Oliver UptonHi Oliver,
Quoted 21 lines · 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::hypercallto 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.arch/arm64/kvm/hypercalls.c +0
-0
arch/arm64/kvm/hypercalls.c static u8 kvm_hvc_get_action(struct kvm_vcpu *vcpu, u32 func_id) · line 170
Quoted 10 lines
arch/arm64/kvm/hypercalls.c +6
-0@@ -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.
arch/arm64/kvm/hypercalls.c static u8 kvm_hvc_get_action(struct kvm_vcpu *vcpu, u32 func_id) · line 177
Quoted 7 lines
arch/arm64/kvm/hypercalls.c +4
-0+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
Re: [RFC PATCH v2 5/6] KVM: arm64: Add support for KVM_EXIT_HYPERCALL
Hi James,
Quoted 7 lines · James Morse
Quoted 5 lines · Oliver Upton
arch/arm64/kvm/hypercalls.c +5
-0+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 ;
Quoted 7 lines
arch/arm64/kvm/hypercalls.c static u8 kvm_hvc_get_action(struct kvm_vcpu *vcpu, u32 func_id) · line 169
Quoted 1 line
arch/arm64/kvm/hypercalls.c +1
-0+ run->hypercall.nr = func_id;This is a bit weird. The
func_idis 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.
Quoted 2 lines
arch/arm64/kvm/hypercalls.c static u8 kvm_hvc_get_action(struct kvm_vcpu *vcpu, u32 func_id) · line 172
Quoted 2 lines
arch/arm64/kvm/hypercalls.c +2
-0+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.
Signature · 2 lines
Thanks, Oliver
RFC PATCH v2 6/6 KVM: arm64: Indroduce support for userspace SMCCC filtering
In reply to Oliver UptonAs 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.
--- 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(+)
Documentation/virt/kvm/devices/vm.rst +67 -0
@@ -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``.
arch/arm64/include/uapi/asm/kvm.h +18 -0
@@ -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)
arch/arm64/kvm/arm.c +4 -0
@@ -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 ; }
arch/arm64/kvm/hypercalls.c +88 -0
97 lines not on this page · raw message
include/kvm/arm_hypercalls.h +3 -0
@@ -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
Signature · 7 lines
2.39.1.581.gbfd45094c4-goog _______________________________________________ linux-arm-kernel mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Re: [RFC PATCH v2 6/6] KVM: arm64: Indroduce support for userspace SMCCC filtering
On Sat, Feb 11, 2023 at 01:37:59AM +0000, Oliver Upton wrote:
[...]
Documentation/virt/kvm/devices/vm.rst Allows userspace to query the status of migration mode. · line 361
Quoted 13 lines
Documentation/virt/kvm/devices/vm.rst +13
-0+ 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.
Signature · 2 lines
Thanks, Oliver
Hi Oliver,
Quoted 22 lines · 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.
Quoted 6 lines
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.
Quoted 7 lines
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!
Quoted 3 lines
- 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'.
Quoted 3 lines
- 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
Hi James,
Quoted 28 lines · James Morse
Hi Oliver,
Quoted 23 lines · 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.
Quoted 7 lines
Quoted 3 lines
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.
Quoted 2 lines
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.
Quoted 4 lines
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_FEATURESto find ifSMCCC_VERSIONis 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.
Quoted 11 lines
Quoted 8 lines
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!
Quoted 9 lines
Quoted 3 lines
- 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 :)
Quoted 7 lines
Quoted 4 lines
- 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 :)
Signature · 2 lines
Thanks, Oliver