| Message ID | 20230404154050.2270077-9-oliver.upton@linux.dev (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,01/13] KVM: x86: Redefine 'longmode' as a flag for KVM_EXIT_HYPERCALL | expand |
On Tue, 04 Apr 2023 16:40:45 +0100, Oliver Upton <oliver.upton@linux.dev> 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). > > Signed-off-by: Oliver Upton <oliver.upton@linux.dev> > --- > Documentation/virt/kvm/api.rst | 18 ++++++++++++++++-- > arch/arm64/include/uapi/asm/kvm.h | 4 ++++ > arch/arm64/kvm/handle_exit.c | 4 +++- > arch/arm64/kvm/hypercalls.c | 16 ++++++++++++++++ > 4 files changed, 39 insertions(+), 3 deletions(-) > > diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst > index 9b01e3d0e757..9497792c4ee5 100644 > --- a/Documentation/virt/kvm/api.rst > +++ b/Documentation/virt/kvm/api.rst > @@ -6221,11 +6221,25 @@ to the byte array. > __u64 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: > +---------- > + > +``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 f9672ef1159a..f86446c5a7e3 100644 > --- a/arch/arm64/include/uapi/asm/kvm.h > +++ b/arch/arm64/include/uapi/asm/kvm.h > @@ -472,12 +472,16 @@ enum { > enum kvm_smccc_filter_action { > KVM_SMCCC_FILTER_HANDLE = 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 68f95dcd41a1..3f43e20c48b6 100644 > --- a/arch/arm64/kvm/handle_exit.c > +++ b/arch/arm64/kvm/handle_exit.c > @@ -71,7 +71,9 @@ static int handle_smc(struct kvm_vcpu *vcpu) > * Trap exception, not a Secure Monitor Call exception [...]" > * > * We need to advance the PC after the trap, as it would > - * otherwise return to the same address... > + * otherwise return to the same address. Furthermore, pre-incrementing > + * the PC before potentially exiting to userspace maintains the same > + * abstraction for both SMCs and HVCs. nit: this comment really needs to find its way in the documentation so that a VMM author can determine the PC of the SMC/HVC. This is specially important for 32bit, which has a 16bit encodings for SMC/HVC. And thinking of it, this outlines a small flaw in this API. If luserspace needs to find out about the address of the HVC/SMC, it needs to know the *size* of the instruction. But we don't propagate the ESR value. I think this still works by construction (userspace can check PSTATE and work out whether we're in ARM or Thumb mode), but this feels fragile. Should we expose the ESR, or at least ESR_EL2.IL as an additional flag? Thanks, M.
On Wed, 05 Apr 2023 08:35:05 +0100, Marc Zyngier <maz@kernel.org> wrote: > > On Tue, 04 Apr 2023 16:40:45 +0100, > Oliver Upton <oliver.upton@linux.dev> 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). > > > > Signed-off-by: Oliver Upton <oliver.upton@linux.dev> > > --- > > Documentation/virt/kvm/api.rst | 18 ++++++++++++++++-- > > arch/arm64/include/uapi/asm/kvm.h | 4 ++++ > > arch/arm64/kvm/handle_exit.c | 4 +++- > > arch/arm64/kvm/hypercalls.c | 16 ++++++++++++++++ > > 4 files changed, 39 insertions(+), 3 deletions(-) > > > > diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst > > index 9b01e3d0e757..9497792c4ee5 100644 > > --- a/Documentation/virt/kvm/api.rst > > +++ b/Documentation/virt/kvm/api.rst > > @@ -6221,11 +6221,25 @@ to the byte array. > > __u64 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: > > +---------- > > + > > +``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 f9672ef1159a..f86446c5a7e3 100644 > > --- a/arch/arm64/include/uapi/asm/kvm.h > > +++ b/arch/arm64/include/uapi/asm/kvm.h > > @@ -472,12 +472,16 @@ enum { > > enum kvm_smccc_filter_action { > > KVM_SMCCC_FILTER_HANDLE = 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 68f95dcd41a1..3f43e20c48b6 100644 > > --- a/arch/arm64/kvm/handle_exit.c > > +++ b/arch/arm64/kvm/handle_exit.c > > @@ -71,7 +71,9 @@ static int handle_smc(struct kvm_vcpu *vcpu) > > * Trap exception, not a Secure Monitor Call exception [...]" > > * > > * We need to advance the PC after the trap, as it would > > - * otherwise return to the same address... > > + * otherwise return to the same address. Furthermore, pre-incrementing > > + * the PC before potentially exiting to userspace maintains the same > > + * abstraction for both SMCs and HVCs. > > nit: this comment really needs to find its way in the documentation so > that a VMM author can determine the PC of the SMC/HVC. This is > specially important for 32bit, which has a 16bit encodings for > SMC/HVC. > > And thinking of it, this outlines a small flaw in this API. If > luserspace needs to find out about the address of the HVC/SMC, it > needs to know the *size* of the instruction. But we don't propagate > the ESR value. I think this still works by construction (userspace can > check PSTATE and work out whether we're in ARM or Thumb mode), but > this feels fragile. > > Should we expose the ESR, or at least ESR_EL2.IL as an additional > flag? Just to make this a quicker round trip, I hacked the following together. If you agree with it, I'll stick it on top and get the ball rolling. Thanks, M. From 9b830e7a3819c2771074bebe66c1d5f20394e3cc Mon Sep 17 00:00:00 2001 From: Marc Zyngier <maz@kernel.org> Date: Wed, 5 Apr 2023 12:48:58 +0100 Subject: [PATCH] KVM: arm64: Expose SMC/HVC width to userspace When returning to userspace to handle a SMCCC call, we consistently set PC to point to the instruction immediately after the HVC/SMC. However, should userspace need to know the exact address of the trapping instruction, it needs to know about the *size* of that instruction. For AArch64, this is pretty easy. For AArch32, this is a bit more funky, as Thumb has 16bit encodings for both HVC and SMC. Expose this to userspace with a new flag that directly derives from ESR_EL2.IL. Also update the documentation to reflect the PC state at the point of exit. Finally, this fixes a small buglet where the hypercall.{args,ret} fields would not be cleared on exit, and could contain some random junk. Signed-off-by: Marc Zyngier <maz@kernel.org> --- Documentation/virt/kvm/api.rst | 8 ++++++++ arch/arm64/include/uapi/asm/kvm.h | 3 ++- arch/arm64/kvm/hypercalls.c | 16 +++++++++++----- 3 files changed, 21 insertions(+), 6 deletions(-) diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst index c8ab2f730945..103f945959ed 100644 --- a/Documentation/virt/kvm/api.rst +++ b/Documentation/virt/kvm/api.rst @@ -6244,6 +6244,14 @@ Definition of ``flags``: conduit to initiate the SMCCC call. If this bit is 0 then the guest used the HVC conduit for the SMCCC call. + - ``KVM_HYPERCALL_EXIT_16BIT``: Indicates that the guest used a 16bit + instruction to initiate the SMCCC call. If this bit is 0 then the + guest used a 32bit instruction. An AArch64 guest always has this + bit set to 0. + +At the point of exit, PC points to the instruction immediately following +the trapping instruction. + :: /* KVM_EXIT_TPR_ACCESS */ diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h index 3dcfa4bfdf83..b1c1edf85480 100644 --- a/arch/arm64/include/uapi/asm/kvm.h +++ b/arch/arm64/include/uapi/asm/kvm.h @@ -491,7 +491,8 @@ struct kvm_smccc_filter { }; /* arm64-specific KVM_EXIT_HYPERCALL flags */ -#define KVM_HYPERCALL_EXIT_SMC (1U << 0) +#define KVM_HYPERCALL_EXIT_SMC (1U << 0) +#define KVM_HYPERCALL_EXIT_16BIT (1U << 1) #endif diff --git a/arch/arm64/kvm/hypercalls.c b/arch/arm64/kvm/hypercalls.c index 9a35d6d18193..3b6523f25afc 100644 --- a/arch/arm64/kvm/hypercalls.c +++ b/arch/arm64/kvm/hypercalls.c @@ -222,13 +222,19 @@ static void kvm_prepare_hypercall_exit(struct kvm_vcpu *vcpu, u32 func_id) { u8 ec = ESR_ELx_EC(kvm_vcpu_get_esr(vcpu)); struct kvm_run *run = vcpu->run; - - run->exit_reason = KVM_EXIT_HYPERCALL; - run->hypercall.nr = func_id; - run->hypercall.flags = 0; + u64 flags = 0; if (ec == ESR_ELx_EC_SMC32 || ec == ESR_ELx_EC_SMC64) - run->hypercall.flags |= KVM_HYPERCALL_EXIT_SMC; + flags |= KVM_HYPERCALL_EXIT_SMC; + + if (!kvm_vcpu_trap_il_is32bit(vcpu)) + flags |= KVM_HYPERCALL_EXIT_16BIT; + + run->exit_reason = KVM_EXIT_HYPERCALL; + run->hypercall = (typeof(run->hypercall)) { + .nr = func_id, + .flags = flags, + }; } int kvm_smccc_call_handler(struct kvm_vcpu *vcpu)
Hey Marc, On Wed, Apr 05, 2023 at 12:59:20PM +0100, Marc Zyngier wrote: [...] > > > diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c > > > index 68f95dcd41a1..3f43e20c48b6 100644 > > > --- a/arch/arm64/kvm/handle_exit.c > > > +++ b/arch/arm64/kvm/handle_exit.c > > > @@ -71,7 +71,9 @@ static int handle_smc(struct kvm_vcpu *vcpu) > > > * Trap exception, not a Secure Monitor Call exception [...]" > > > * > > > * We need to advance the PC after the trap, as it would > > > - * otherwise return to the same address... > > > + * otherwise return to the same address. Furthermore, pre-incrementing > > > + * the PC before potentially exiting to userspace maintains the same > > > + * abstraction for both SMCs and HVCs. > > > > nit: this comment really needs to find its way in the documentation so > > that a VMM author can determine the PC of the SMC/HVC. This is > > specially important for 32bit, which has a 16bit encodings for > > SMC/HVC. > > > > And thinking of it, this outlines a small flaw in this API. If > > luserspace needs to find out about the address of the HVC/SMC, it > > needs to know the *size* of the instruction. But we don't propagate > > the ESR value. I think this still works by construction (userspace can > > check PSTATE and work out whether we're in ARM or Thumb mode), but > > this feels fragile. > > > > Should we expose the ESR, or at least ESR_EL2.IL as an additional > > flag? > > Just to make this a quicker round trip, I hacked the following > together. If you agree with it, I'll stick it on top and get the ball > rolling. Less work for me? How could I say no :) > From 9b830e7a3819c2771074bebe66c1d5f20394e3cc Mon Sep 17 00:00:00 2001 > From: Marc Zyngier <maz@kernel.org> > Date: Wed, 5 Apr 2023 12:48:58 +0100 > Subject: [PATCH] KVM: arm64: Expose SMC/HVC width to userspace > > When returning to userspace to handle a SMCCC call, we consistently > set PC to point to the instruction immediately after the HVC/SMC. > > However, should userspace need to know the exact address of the > trapping instruction, it needs to know about the *size* of that > instruction. For AArch64, this is pretty easy. For AArch32, this > is a bit more funky, as Thumb has 16bit encodings for both HVC > and SMC. > > Expose this to userspace with a new flag that directly derives > from ESR_EL2.IL. Also update the documentation to reflect the PC > state at the point of exit. > > Finally, this fixes a small buglet where the hypercall.{args,ret} > fields would not be cleared on exit, and could contain some > random junk. > > Signed-off-by: Marc Zyngier <maz@kernel.org> Reviewed-by: Oliver Upton <oliver.upton@linux.dev> > --- > Documentation/virt/kvm/api.rst | 8 ++++++++ > arch/arm64/include/uapi/asm/kvm.h | 3 ++- > arch/arm64/kvm/hypercalls.c | 16 +++++++++++----- > 3 files changed, 21 insertions(+), 6 deletions(-) > > diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst > index c8ab2f730945..103f945959ed 100644 > --- a/Documentation/virt/kvm/api.rst > +++ b/Documentation/virt/kvm/api.rst > @@ -6244,6 +6244,14 @@ Definition of ``flags``: > conduit to initiate the SMCCC call. If this bit is 0 then the guest > used the HVC conduit for the SMCCC call. > > + - ``KVM_HYPERCALL_EXIT_16BIT``: Indicates that the guest used a 16bit > + instruction to initiate the SMCCC call. If this bit is 0 then the > + guest used a 32bit instruction. An AArch64 guest always has this > + bit set to 0. > + > +At the point of exit, PC points to the instruction immediately following > +the trapping instruction. > + > :: > > /* KVM_EXIT_TPR_ACCESS */ > diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h > index 3dcfa4bfdf83..b1c1edf85480 100644 > --- a/arch/arm64/include/uapi/asm/kvm.h > +++ b/arch/arm64/include/uapi/asm/kvm.h > @@ -491,7 +491,8 @@ struct kvm_smccc_filter { > }; > > /* arm64-specific KVM_EXIT_HYPERCALL flags */ > -#define KVM_HYPERCALL_EXIT_SMC (1U << 0) > +#define KVM_HYPERCALL_EXIT_SMC (1U << 0) > +#define KVM_HYPERCALL_EXIT_16BIT (1U << 1) > > #endif > > diff --git a/arch/arm64/kvm/hypercalls.c b/arch/arm64/kvm/hypercalls.c > index 9a35d6d18193..3b6523f25afc 100644 > --- a/arch/arm64/kvm/hypercalls.c > +++ b/arch/arm64/kvm/hypercalls.c > @@ -222,13 +222,19 @@ static void kvm_prepare_hypercall_exit(struct kvm_vcpu *vcpu, u32 func_id) > { > u8 ec = ESR_ELx_EC(kvm_vcpu_get_esr(vcpu)); > struct kvm_run *run = vcpu->run; > - > - run->exit_reason = KVM_EXIT_HYPERCALL; > - run->hypercall.nr = func_id; > - run->hypercall.flags = 0; > + u64 flags = 0; > > if (ec == ESR_ELx_EC_SMC32 || ec == ESR_ELx_EC_SMC64) > - run->hypercall.flags |= KVM_HYPERCALL_EXIT_SMC; > + flags |= KVM_HYPERCALL_EXIT_SMC; > + > + if (!kvm_vcpu_trap_il_is32bit(vcpu)) > + flags |= KVM_HYPERCALL_EXIT_16BIT; > + > + run->exit_reason = KVM_EXIT_HYPERCALL; > + run->hypercall = (typeof(run->hypercall)) { > + .nr = func_id, > + .flags = flags, > + }; > } > > int kvm_smccc_call_handler(struct kvm_vcpu *vcpu) > -- > 2.34.1 > > > -- > Without deviation from the norm, progress is not possible.
Hi Marc, Sorry for jumping late. I am updating the Qemu code for the VCPU hotplug to reflect Oliver's SMCCC filtering changes and I have few doubts. Please scroll below. > From: Marc Zyngier <maz@kernel.org> > Sent: Wednesday, April 5, 2023 12:59 PM > To: Oliver Upton <oliver.upton@linux.dev> > Cc: kvmarm@lists.linux.dev; kvm@vger.kernel.org; Paolo Bonzini > <pbonzini@redhat.com>; James Morse <james.morse@arm.com>; Suzuki K Poulose > <suzuki.poulose@arm.com>; yuzenghui <yuzenghui@huawei.com>; Sean > Christopherson <seanjc@google.com>; Salil Mehta <salil.mehta@huawei.com> > Subject: Re: [PATCH v3 08/13] KVM: arm64: Add support for > KVM_EXIT_HYPERCALL > > On Wed, 05 Apr 2023 08:35:05 +0100, > Marc Zyngier <maz@kernel.org> wrote: > > > > On Tue, 04 Apr 2023 16:40:45 +0100, > > Oliver Upton <oliver.upton@linux.dev> 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). > > > > > > Signed-off-by: Oliver Upton <oliver.upton@linux.dev> > > > --- > > > Documentation/virt/kvm/api.rst | 18 ++++++++++++++++-- > > > arch/arm64/include/uapi/asm/kvm.h | 4 ++++ > > > arch/arm64/kvm/handle_exit.c | 4 +++- > > > arch/arm64/kvm/hypercalls.c | 16 ++++++++++++++++ > > > 4 files changed, 39 insertions(+), 3 deletions(-) > > > > > > diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst > > > index 9b01e3d0e757..9497792c4ee5 100644 > > > --- a/Documentation/virt/kvm/api.rst > > > +++ b/Documentation/virt/kvm/api.rst > > > @@ -6221,11 +6221,25 @@ to the byte array. > > > __u64 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: > > > +---------- > > > + > > > +``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 f9672ef1159a..f86446c5a7e3 100644 > > > --- a/arch/arm64/include/uapi/asm/kvm.h > > > +++ b/arch/arm64/include/uapi/asm/kvm.h > > > @@ -472,12 +472,16 @@ enum { > > > enum kvm_smccc_filter_action { > > > KVM_SMCCC_FILTER_HANDLE = 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 68f95dcd41a1..3f43e20c48b6 100644 > > > --- a/arch/arm64/kvm/handle_exit.c > > > +++ b/arch/arm64/kvm/handle_exit.c > > > @@ -71,7 +71,9 @@ static int handle_smc(struct kvm_vcpu *vcpu) > > > * Trap exception, not a Secure Monitor Call exception [...]" > > > * > > > * We need to advance the PC after the trap, as it would > > > - * otherwise return to the same address... > > > + * otherwise return to the same address. Furthermore, pre-incrementing > > > + * the PC before potentially exiting to userspace maintains the same > > > + * abstraction for both SMCs and HVCs. > > > > nit: this comment really needs to find its way in the documentation so > > that a VMM author can determine the PC of the SMC/HVC. This is > > specially important for 32bit, which has a 16bit encodings for > > SMC/HVC. > > > > And thinking of it, this outlines a small flaw in this API. If > > luserspace needs to find out about the address of the HVC/SMC, it > > needs to know the *size* of the instruction. But we don't propagate > > the ESR value. I think this still works by construction (userspace can > > check PSTATE and work out whether we're in ARM or Thumb mode), but > > this feels fragile. > > > > Should we expose the ESR, or at least ESR_EL2.IL as an additional > > flag? I think we would need "Immediate value" of the ESR_EL2 register in the user-space/VMM to be able to construct the syndrome value. I cannot see where it is being sent? Please correct me if I am missing anything here. > Just to make this a quicker round trip, I hacked the following > together. If you agree with it, I'll stick it on top and get the ball > rolling. > > Thanks, > > M. > > From 9b830e7a3819c2771074bebe66c1d5f20394e3cc Mon Sep 17 00:00:00 2001 > From: Marc Zyngier <maz@kernel.org> > Date: Wed, 5 Apr 2023 12:48:58 +0100 > Subject: [PATCH] KVM: arm64: Expose SMC/HVC width to userspace > > When returning to userspace to handle a SMCCC call, we consistently > set PC to point to the instruction immediately after the HVC/SMC. > > However, should userspace need to know the exact address of the > trapping instruction, it needs to know about the *size* of that > instruction. For AArch64, this is pretty easy. For AArch32, this > is a bit more funky, as Thumb has 16bit encodings for both HVC > and SMC. > > Expose this to userspace with a new flag that directly derives > from ESR_EL2.IL. Also update the documentation to reflect the PC > state at the point of exit. > > Finally, this fixes a small buglet where the hypercall.{args,ret} > fields would not be cleared on exit, and could contain some > random junk. > > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > Documentation/virt/kvm/api.rst | 8 ++++++++ > arch/arm64/include/uapi/asm/kvm.h | 3 ++- > arch/arm64/kvm/hypercalls.c | 16 +++++++++++----- > 3 files changed, 21 insertions(+), 6 deletions(-) > > diff --git a/Documentation/virt/kvm/api.rst > b/Documentation/virt/kvm/api.rst > index c8ab2f730945..103f945959ed 100644 > --- a/Documentation/virt/kvm/api.rst > +++ b/Documentation/virt/kvm/api.rst > @@ -6244,6 +6244,14 @@ Definition of ``flags``: > conduit to initiate the SMCCC call. If this bit is 0 then the guest > used the HVC conduit for the SMCCC call. > > + - ``KVM_HYPERCALL_EXIT_16BIT``: Indicates that the guest used a 16bit > + instruction to initiate the SMCCC call. If this bit is 0 then the > + guest used a 32bit instruction. An AArch64 guest always has this > + bit set to 0. > + > +At the point of exit, PC points to the instruction immediately following > +the trapping instruction. > + > :: > > /* KVM_EXIT_TPR_ACCESS */ > diff --git a/arch/arm64/include/uapi/asm/kvm.h > b/arch/arm64/include/uapi/asm/kvm.h > index 3dcfa4bfdf83..b1c1edf85480 100644 > --- a/arch/arm64/include/uapi/asm/kvm.h > +++ b/arch/arm64/include/uapi/asm/kvm.h > @@ -491,7 +491,8 @@ struct kvm_smccc_filter { > }; > > /* arm64-specific KVM_EXIT_HYPERCALL flags */ > -#define KVM_HYPERCALL_EXIT_SMC (1U << 0) > +#define KVM_HYPERCALL_EXIT_SMC (1U << 0) > +#define KVM_HYPERCALL_EXIT_16BIT (1U << 1) > > #endif > > diff --git a/arch/arm64/kvm/hypercalls.c b/arch/arm64/kvm/hypercalls.c > index 9a35d6d18193..3b6523f25afc 100644 > --- a/arch/arm64/kvm/hypercalls.c > +++ b/arch/arm64/kvm/hypercalls.c > @@ -222,13 +222,19 @@ static void kvm_prepare_hypercall_exit(struct > kvm_vcpu *vcpu, u32 func_id) > { > u8 ec = ESR_ELx_EC(kvm_vcpu_get_esr(vcpu)); > struct kvm_run *run = vcpu->run; > - > - run->exit_reason = KVM_EXIT_HYPERCALL; > - run->hypercall.nr = func_id; > - run->hypercall.flags = 0; > + u64 flags = 0; > > if (ec == ESR_ELx_EC_SMC32 || ec == ESR_ELx_EC_SMC64) > - run->hypercall.flags |= KVM_HYPERCALL_EXIT_SMC; > + flags |= KVM_HYPERCALL_EXIT_SMC; > + > + if (!kvm_vcpu_trap_il_is32bit(vcpu)) > + flags |= KVM_HYPERCALL_EXIT_16BIT; > + > + run->exit_reason = KVM_EXIT_HYPERCALL; > + run->hypercall = (typeof(run->hypercall)) { > + .nr = func_id, Earlier this was the place holder for the "immediate value" Best regards Salil
Hi Salil, On Wed, May 17, 2023 at 06:00:18PM +0000, Salil Mehta wrote: [...] > > > Should we expose the ESR, or at least ESR_EL2.IL as an additional > > > flag? > > > I think we would need "Immediate value" of the ESR_EL2 register in the > user-space/VMM to be able to construct the syndrome value. I cannot see > where it is being sent? The immediate value is not exposed to userspace, although by definition the immediate value must be zero. The SMCCC spec requires all compliant calls to use an immediate of zero (DEN0028E 2.9). Is there a legitimate use case for hypercalls with a nonzero immediate? They would no longer be considered SMCCC calls at that point, so they wouldn't work with the new UAPI.
On Wed, 17 May 2023 19:38:14 +0100, Oliver Upton <oliver.upton@linux.dev> wrote: > > Hi Salil, > > On Wed, May 17, 2023 at 06:00:18PM +0000, Salil Mehta wrote: > > [...] > > > > > Should we expose the ESR, or at least ESR_EL2.IL as an additional > > > > flag? > > > > > > I think we would need "Immediate value" of the ESR_EL2 register in the > > user-space/VMM to be able to construct the syndrome value. I cannot see > > where it is being sent? > > The immediate value is not exposed to userspace, although by definition > the immediate value must be zero. The SMCCC spec requires all compliant > calls to use an immediate of zero (DEN0028E 2.9). > > Is there a legitimate use case for hypercalls with a nonzero immediate? > They would no longer be considered SMCCC calls at that point, so they > wouldn't work with the new UAPI. I agree. The use of non-zero immediate has long been deprecated. I guess we should actually reject non-zero immediate for HVC just like we do for SMC. If there is an actual need for a non-zero immediate to be propagated to userspace (want to emulate Xen's infamous 'HVC #0xEA1'?), then this should be an extension to the current API. Thanks, M.
Hi Oliver, > From: Oliver Upton <oliver.upton@linux.dev> > Sent: Wednesday, May 17, 2023 7:38 PM > To: Salil Mehta <salil.mehta@huawei.com> > Cc: Marc Zyngier <maz@kernel.org>; kvmarm@lists.linux.dev; > kvm@vger.kernel.org; Paolo Bonzini <pbonzini@redhat.com>; James Morse > <james.morse@arm.com>; Suzuki K Poulose <suzuki.poulose@arm.com>; yuzenghui > <yuzenghui@huawei.com>; Sean Christopherson <seanjc@google.com> > Subject: Re: [PATCH v3 08/13] KVM: arm64: Add support for > KVM_EXIT_HYPERCALL > > Hi Salil, > > On Wed, May 17, 2023 at 06:00:18PM +0000, Salil Mehta wrote: > > [...] > > > > > Should we expose the ESR, or at least ESR_EL2.IL as an additional > > > > flag? > > > > > > I think we would need "Immediate value" of the ESR_EL2 register in the > > user-space/VMM to be able to construct the syndrome value. I cannot see > > where it is being sent? > > The immediate value is not exposed to userspace, although by definition > the immediate value must be zero. The SMCCC spec requires all compliant > calls to use an immediate of zero (DEN0028E 2.9). Sure. I do understand this. > Is there a legitimate use case for hypercalls with a nonzero immediate? To be frank I was not sure of this either and therefore I thought it would be safe to keep the handling in user-space/Qemu generic as it is now by constructing a syndrome value depending upon immediate value and other accompanying parameters from the KVM. Also, I am not sure what it could break or what platforms it could break. I think we need some Qemu folks to pitch-in and comment on this. > They would no longer be considered SMCCC calls at that point, so they > wouldn't work with the new UAPI. True. So should we do this change now? Thanks Salil
Hi Marc, > From: Marc Zyngier <maz@kernel.org> > Sent: Thursday, May 18, 2023 9:06 AM > To: Oliver Upton <oliver.upton@linux.dev> > Cc: Salil Mehta <salil.mehta@huawei.com>; kvmarm@lists.linux.dev; > kvm@vger.kernel.org; Paolo Bonzini <pbonzini@redhat.com>; James Morse > <james.morse@arm.com>; Suzuki K Poulose <suzuki.poulose@arm.com>; yuzenghui > <yuzenghui@huawei.com>; Sean Christopherson <seanjc@google.com> > Subject: Re: [PATCH v3 08/13] KVM: arm64: Add support for > KVM_EXIT_HYPERCALL > > On Wed, 17 May 2023 19:38:14 +0100, > Oliver Upton <oliver.upton@linux.dev> wrote: > > > > Hi Salil, > > > > On Wed, May 17, 2023 at 06:00:18PM +0000, Salil Mehta wrote: > > > > [...] > > > > > > > Should we expose the ESR, or at least ESR_EL2.IL as an additional > > > > > flag? > > > > > > > > > I think we would need "Immediate value" of the ESR_EL2 register in the > > > user-space/VMM to be able to construct the syndrome value. I cannot see > > > where it is being sent? > > > > The immediate value is not exposed to userspace, although by definition > > the immediate value must be zero. The SMCCC spec requires all compliant > > calls to use an immediate of zero (DEN0028E 2.9). > > > > Is there a legitimate use case for hypercalls with a nonzero immediate? > > They would no longer be considered SMCCC calls at that point, so they > > wouldn't work with the new UAPI. > > I agree. The use of non-zero immediate has long been deprecated. I > guess we should actually reject non-zero immediate for HVC just like > we do for SMC. Ok. Maybe I will hard code Immediate value as 0 to create a syndrome value at the VMM/Qemu and will also put a note stating non-zero immediate for HVC/SVC are not supported/deprecated. > If there is an actual need for a non-zero immediate to be propagated > to userspace (want to emulate Xen's infamous 'HVC #0xEA1'?), then this > should be an extension to the current API. Oh ok, then perhaps this new extension change should be simultaneously committed to avoid breaking Xen? Thanks Salil
On Thu, 18 May 2023 10:08:46 +0100, Salil Mehta <salil.mehta@huawei.com> wrote: > > Hi Marc, > > > From: Marc Zyngier <maz@kernel.org> > > Sent: Thursday, May 18, 2023 9:06 AM > > To: Oliver Upton <oliver.upton@linux.dev> > > Cc: Salil Mehta <salil.mehta@huawei.com>; kvmarm@lists.linux.dev; > > kvm@vger.kernel.org; Paolo Bonzini <pbonzini@redhat.com>; James Morse > > <james.morse@arm.com>; Suzuki K Poulose <suzuki.poulose@arm.com>; yuzenghui > > <yuzenghui@huawei.com>; Sean Christopherson <seanjc@google.com> > > Subject: Re: [PATCH v3 08/13] KVM: arm64: Add support for > > KVM_EXIT_HYPERCALL > > > > On Wed, 17 May 2023 19:38:14 +0100, > > Oliver Upton <oliver.upton@linux.dev> wrote: > > > > > > Hi Salil, > > > > > > On Wed, May 17, 2023 at 06:00:18PM +0000, Salil Mehta wrote: > > > > > > [...] > > > > > > > > > Should we expose the ESR, or at least ESR_EL2.IL as an additional > > > > > > flag? > > > > > > > > > > > > I think we would need "Immediate value" of the ESR_EL2 register in the > > > > user-space/VMM to be able to construct the syndrome value. I cannot see > > > > where it is being sent? > > > > > > The immediate value is not exposed to userspace, although by definition > > > the immediate value must be zero. The SMCCC spec requires all compliant > > > calls to use an immediate of zero (DEN0028E 2.9). > > > > > > Is there a legitimate use case for hypercalls with a nonzero immediate? > > > They would no longer be considered SMCCC calls at that point, so they > > > wouldn't work with the new UAPI. > > > > I agree. The use of non-zero immediate has long been deprecated. I > > guess we should actually reject non-zero immediate for HVC just like > > we do for SMC. > > > Ok. Maybe I will hard code Immediate value as 0 to create a syndrome value > at the VMM/Qemu and will also put a note stating non-zero immediate for > HVC/SVC are not supported/deprecated. Yes, because this should be the only situation where you should see such an exit to userspace. > > If there is an actual need for a non-zero immediate to be propagated > > to userspace (want to emulate Xen's infamous 'HVC #0xEA1'?), then this > > should be an extension to the current API. > > Oh ok, then perhaps this new extension change should be simultaneously > committed to avoid breaking Xen? How would that break Xen? I don't have any plan to emulate Xen in any shape or form, and I don't think anyone want to do that in userspace either. I really want to see an actual use case to expand this stuff. Because so far, we follow the strict SMCCC spec, and nothing else. But if we admit deviations, do we also have to expose SMC and HVC as different instructions? Thanks, M.
Hi Marc, > From: Marc Zyngier <maz@kernel.org> > Sent: Thursday, May 18, 2023 10:43 AM > To: Salil Mehta <salil.mehta@huawei.com> > Cc: Oliver Upton <oliver.upton@linux.dev>; kvmarm@lists.linux.dev; > kvm@vger.kernel.org; Paolo Bonzini <pbonzini@redhat.com>; James Morse > <james.morse@arm.com>; Suzuki K Poulose <suzuki.poulose@arm.com>; yuzenghui > <yuzenghui@huawei.com>; Sean Christopherson <seanjc@google.com> > Subject: Re: [PATCH v3 08/13] KVM: arm64: Add support for > KVM_EXIT_HYPERCALL > [...] > > > > > > > Should we expose the ESR, or at least ESR_EL2.IL as an additional > > > > > > > flag? > > > > > > > > > > > > > > > I think we would need "Immediate value" of the ESR_EL2 register in the > > > > > user-space/VMM to be able to construct the syndrome value. I cannot see > > > > > where it is being sent? > > > > > > > > The immediate value is not exposed to userspace, although by definition > > > > the immediate value must be zero. The SMCCC spec requires all compliant > > > > calls to use an immediate of zero (DEN0028E 2.9). > > > > > > > > Is there a legitimate use case for hypercalls with a nonzero immediate? > > > > They would no longer be considered SMCCC calls at that point, so they > > > > wouldn't work with the new UAPI. > > > > > > I agree. The use of non-zero immediate has long been deprecated. I > > > guess we should actually reject non-zero immediate for HVC just like > > > we do for SMC. > > > > > > Ok. Maybe I will hard code Immediate value as 0 to create a syndrome value > > at the VMM/Qemu and will also put a note stating non-zero immediate for > > HVC/SVC are not supported/deprecated. > > Yes, because this should be the only situation where you should see > such an exit to userspace. > > > > If there is an actual need for a non-zero immediate to be propagated > > > to userspace (want to emulate Xen's infamous 'HVC #0xEA1'?), then this > > > should be an extension to the current API. > > > > Oh ok, then perhaps this new extension change should be simultaneously > > committed to avoid breaking Xen? > > How would that break Xen? I don't have any plan to emulate Xen in any > shape or form, and I don't think anyone want to do that in userspace > either. Ok. I thought you are suggestion an exception which is already in use and the current change might break it but now that does not seems to be the case. > I really want to see an actual use case to expand this stuff. Because > so far, we follow the strict SMCCC spec, and nothing else. But if we > admit deviations, do we also have to expose SMC and HVC as different > instructions? Ok got it. No problem and makes sense what you just said. Maybe in future if it is really required then we can always come back to it. For now, I will go ahead with the agreed change in the qemu. Many thanks Salil.
diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst index 9b01e3d0e757..9497792c4ee5 100644 --- a/Documentation/virt/kvm/api.rst +++ b/Documentation/virt/kvm/api.rst @@ -6221,11 +6221,25 @@ to the byte array. __u64 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: +---------- + +``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 f9672ef1159a..f86446c5a7e3 100644 --- a/arch/arm64/include/uapi/asm/kvm.h +++ b/arch/arm64/include/uapi/asm/kvm.h @@ -472,12 +472,16 @@ enum { enum kvm_smccc_filter_action { KVM_SMCCC_FILTER_HANDLE = 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 68f95dcd41a1..3f43e20c48b6 100644 --- a/arch/arm64/kvm/handle_exit.c +++ b/arch/arm64/kvm/handle_exit.c @@ -71,7 +71,9 @@ static int handle_smc(struct kvm_vcpu *vcpu) * Trap exception, not a Secure Monitor Call exception [...]" * * We need to advance the PC after the trap, as it would - * otherwise return to the same address... + * otherwise return to the same address. Furthermore, pre-incrementing + * the PC before potentially exiting to userspace maintains the same + * abstraction for both SMCs and HVCs. */ kvm_incr_pc(vcpu); diff --git a/arch/arm64/kvm/hypercalls.c b/arch/arm64/kvm/hypercalls.c index ba7cd84c6668..2db53709bec1 100644 --- a/arch/arm64/kvm/hypercalls.c +++ b/arch/arm64/kvm/hypercalls.c @@ -180,6 +180,19 @@ static u8 kvm_smccc_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) +{ + u8 ec = ESR_ELx_EC(kvm_vcpu_get_esr(vcpu)); + struct kvm_run *run = vcpu->run; + + run->exit_reason = KVM_EXIT_HYPERCALL; + run->hypercall.nr = func_id; + run->hypercall.flags = 0; + + if (ec == ESR_ELx_EC_SMC32 || ec == ESR_ELx_EC_SMC64) + run->hypercall.flags |= KVM_HYPERCALL_EXIT_SMC; +} + int kvm_smccc_call_handler(struct kvm_vcpu *vcpu) { struct kvm_smccc_features *smccc_feat = &vcpu->kvm->arch.smccc_feat; @@ -195,6 +208,9 @@ int kvm_smccc_call_handler(struct kvm_vcpu *vcpu) break; case KVM_SMCCC_FILTER_DENY: goto out; + case KVM_SMCCC_FILTER_FWD_TO_USER: + kvm_prepare_hypercall_exit(vcpu, func_id); + return 0; default: WARN_RATELIMIT(1, "Unhandled SMCCC filter action: %d\n", action); goto out;
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). Signed-off-by: Oliver Upton <oliver.upton@linux.dev> --- Documentation/virt/kvm/api.rst | 18 ++++++++++++++++-- arch/arm64/include/uapi/asm/kvm.h | 4 ++++ arch/arm64/kvm/handle_exit.c | 4 +++- arch/arm64/kvm/hypercalls.c | 16 ++++++++++++++++ 4 files changed, 39 insertions(+), 3 deletions(-)