| Message ID | 20230209175820.1939006-13-maz@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Prefix patches for NV support | expand |
Hi Marc, On Thu, Feb 09, 2023 at 05:58:14PM +0000, Marc Zyngier wrote: > From: Jintack Lim <jintack.lim@linaro.org> > > VMs used to execute hvc #0 for the psci call if EL3 is not implemented. > However, when we come to provide the virtual EL2 mode to the VM, the > host OS inside the VM calls kvm_call_hyp() which is also hvc #0. So, > it's hard to differentiate between them from the host hypervisor's point > of view. > > So, let the VM execute smc instruction for the psci call. On ARMv8.3, > even if EL3 is not implemented, a smc instruction executed at non-secure > EL1 is trapped to EL2 if HCR_EL2.TSC==1, rather than being treated as > UNDEFINED. So, the host hypervisor can handle this psci call without any > confusion. I think this commit message is rather stale to the point of being rather misleading. This lets the vEL2 get at the entire gamut of SMCCC calls we have in KVM, not just PSCI. Of course, no problem with that since it is a requirement, but for posterity the commit message should reflect the current state of KVM. If I may suggest: Non-nested guests have used the hvc instruction to initiate SMCCC calls into KVM. This is quite a poor fit for NV as hvc exceptions are always taken to EL2. In other words, KVM needs to unconditionally forward the hvc exception back into vEL2 to uphold the architecture. Instead, treat the smc instruction from vEL2 as we would a guest hypercall, thereby allowing the vEL2 to interact with KVM's hypercall surface. Note that on NV-capable hardware HCR_EL2.TSC causes smc instructions executed in non-secure EL1 to trap to EL2, even if EL3 is not implemented. > Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com> > Signed-off-by: Jintack Lim <jintack.lim@linaro.org> > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > arch/arm64/kvm/handle_exit.c | 24 ++++++++++++++++++++++-- > 1 file changed, 22 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c > index e75101f2aa6c..b0c343c4e062 100644 > --- a/arch/arm64/kvm/handle_exit.c > +++ b/arch/arm64/kvm/handle_exit.c > @@ -63,6 +63,8 @@ static int handle_hvc(struct kvm_vcpu *vcpu) > > static int handle_smc(struct kvm_vcpu *vcpu) > { > + int ret; > + > /* > * "If an SMC instruction executed at Non-secure EL1 is > * trapped to EL2 because HCR_EL2.TSC is 1, the exception is a > @@ -70,10 +72,28 @@ 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... > + * > + * If imm is non-zero, it's not defined, so just skip it. > + */ > + if (kvm_vcpu_hvc_get_imm(vcpu)) { > + vcpu_set_reg(vcpu, 0, ~0UL); > + kvm_incr_pc(vcpu); > + return 1; > + } > + > + /* > + * If imm is zero, it's a psci call. > + * Note that on ARMv8.3, even if EL3 is not implemented, SMC executed > + * at Non-secure EL1 is trapped to EL2 if HCR_EL2.TSC==1, rather than > + * being treated as UNDEFINED. > */ > - vcpu_set_reg(vcpu, 0, ~0UL); > + ret = kvm_hvc_call_handler(vcpu); > + if (ret < 0) > + vcpu_set_reg(vcpu, 0, ~0UL); > + This also has the subtle effect of allowing smc instructions from a non-nested guest to hit our hypercall surface too. I think we should avoid this and only handle smcs that actually come from a vEL2. What do you think about the following? I can squash in all of the changes I've asked for here. diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c index b0c343c4e062..a798c0b4d717 100644 --- a/arch/arm64/kvm/handle_exit.c +++ b/arch/arm64/kvm/handle_exit.c @@ -73,16 +73,18 @@ 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... * - * If imm is non-zero, it's not defined, so just skip it. + * Only handle SMCs from the virtual EL2 with an immediate of zero and + * skip it otherwise. */ - if (kvm_vcpu_hvc_get_imm(vcpu)) { + if (!vcpu_is_el2(vcpu) || kvm_vcpu_hvc_get_imm(vcpu)) { vcpu_set_reg(vcpu, 0, ~0UL); kvm_incr_pc(vcpu); return 1; } /* - * If imm is zero, it's a psci call. + * If imm is zero then it is likely an SMCCC call. + * * Note that on ARMv8.3, even if EL3 is not implemented, SMC executed * at Non-secure EL1 is trapped to EL2 if HCR_EL2.TSC==1, rather than * being treated as UNDEFINED.
On Sat, 11 Feb 2023 10:07:41 +0000, Oliver Upton <oliver.upton@linux.dev> wrote: > > Hi Marc, > > On Thu, Feb 09, 2023 at 05:58:14PM +0000, Marc Zyngier wrote: > > From: Jintack Lim <jintack.lim@linaro.org> > > > > VMs used to execute hvc #0 for the psci call if EL3 is not implemented. > > However, when we come to provide the virtual EL2 mode to the VM, the > > host OS inside the VM calls kvm_call_hyp() which is also hvc #0. So, > > it's hard to differentiate between them from the host hypervisor's point > > of view. > > > > So, let the VM execute smc instruction for the psci call. On ARMv8.3, > > even if EL3 is not implemented, a smc instruction executed at non-secure > > EL1 is trapped to EL2 if HCR_EL2.TSC==1, rather than being treated as > > UNDEFINED. So, the host hypervisor can handle this psci call without any > > confusion. > > I think this commit message is rather stale to the point of being rather > misleading. This lets the vEL2 get at the entire gamut of SMCCC calls we > have in KVM, not just PSCI. > > Of course, no problem with that since it is a requirement, but for > posterity the commit message should reflect the current state of KVM. > > If I may suggest: > > Non-nested guests have used the hvc instruction to initiate SMCCC > calls into KVM. This is quite a poor fit for NV as hvc exceptions are > always taken to EL2. In other words, KVM needs to unconditionally > forward the hvc exception back into vEL2 to uphold the architecture. > > Instead, treat the smc instruction from vEL2 as we would a guest > hypercall, thereby allowing the vEL2 to interact with KVM's hypercall > surface. Note that on NV-capable hardware HCR_EL2.TSC causes smc > instructions executed in non-secure EL1 to trap to EL2, even if EL3 is > not implemented. Very nice! > > > Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com> > > Signed-off-by: Jintack Lim <jintack.lim@linaro.org> > > Signed-off-by: Marc Zyngier <maz@kernel.org> > > --- > > arch/arm64/kvm/handle_exit.c | 24 ++++++++++++++++++++++-- > > 1 file changed, 22 insertions(+), 2 deletions(-) > > > > diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c > > index e75101f2aa6c..b0c343c4e062 100644 > > --- a/arch/arm64/kvm/handle_exit.c > > +++ b/arch/arm64/kvm/handle_exit.c > > @@ -63,6 +63,8 @@ static int handle_hvc(struct kvm_vcpu *vcpu) > > > > static int handle_smc(struct kvm_vcpu *vcpu) > > { > > + int ret; > > + > > /* > > * "If an SMC instruction executed at Non-secure EL1 is > > * trapped to EL2 because HCR_EL2.TSC is 1, the exception is a > > @@ -70,10 +72,28 @@ 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... > > + * > > + * If imm is non-zero, it's not defined, so just skip it. > > + */ > > + if (kvm_vcpu_hvc_get_imm(vcpu)) { > > + vcpu_set_reg(vcpu, 0, ~0UL); > > + kvm_incr_pc(vcpu); > > + return 1; > > + } > > + > > + /* > > + * If imm is zero, it's a psci call. > > + * Note that on ARMv8.3, even if EL3 is not implemented, SMC executed > > + * at Non-secure EL1 is trapped to EL2 if HCR_EL2.TSC==1, rather than > > + * being treated as UNDEFINED. > > */ > > - vcpu_set_reg(vcpu, 0, ~0UL); > > + ret = kvm_hvc_call_handler(vcpu); > > + if (ret < 0) > > + vcpu_set_reg(vcpu, 0, ~0UL); > > + > > This also has the subtle effect of allowing smc instructions from a > non-nested guest to hit our hypercall surface too. I think we'll have to eventually allow that (see the TRNG spec which we blatantly deviate from by requiring an HVC), but we don't have to cross that bridge just yet. > I think we should avoid this and only handle smcs that actually come > from a vEL2. What do you think about the following? > > I can squash in all of the changes I've asked for here. > > diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c > index b0c343c4e062..a798c0b4d717 100644 > --- a/arch/arm64/kvm/handle_exit.c > +++ b/arch/arm64/kvm/handle_exit.c > @@ -73,16 +73,18 @@ 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... > * > - * If imm is non-zero, it's not defined, so just skip it. > + * Only handle SMCs from the virtual EL2 with an immediate of zero and > + * skip it otherwise. > */ > - if (kvm_vcpu_hvc_get_imm(vcpu)) { > + if (!vcpu_is_el2(vcpu) || kvm_vcpu_hvc_get_imm(vcpu)) { > vcpu_set_reg(vcpu, 0, ~0UL); > kvm_incr_pc(vcpu); > return 1; > } > > /* > - * If imm is zero, it's a psci call. > + * If imm is zero then it is likely an SMCCC call. > + * > * Note that on ARMv8.3, even if EL3 is not implemented, SMC executed > * at Non-secure EL1 is trapped to EL2 if HCR_EL2.TSC==1, rather than > * being treated as UNDEFINED. > Looks good to me. Thanks for the suggestions! M.
On Sat, Feb 11, 2023 at 10:31:59AM +0000, Marc Zyngier wrote: > On Sat, 11 Feb 2023 10:07:41 +0000, Oliver Upton <oliver.upton@linux.dev> wrote: [...] > > This also has the subtle effect of allowing smc instructions from a > > non-nested guest to hit our hypercall surface too. > > I think we'll have to eventually allow that (see the TRNG spec which > we blatantly deviate from by requiring an HVC), but we don't have to > cross that bridge just yet. Perhaps I'll continue to bury my head in the sand and act like you didn't say that :) I seem to recall that the SMCCC suggests either the SMC or HVC instruction could be used if both EL2 and EL3 are implemented. So we've messed that up too. My only worry is if we open up the use of SMCs and userspace does something silly in ACPI/DT and unconditionally picks SMCs over HVCs. The VM won't get far on pre-NV hardware w/o EL3... We could always just hide the presence of EL3 for non-NV guests :)
diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c index e75101f2aa6c..b0c343c4e062 100644 --- a/arch/arm64/kvm/handle_exit.c +++ b/arch/arm64/kvm/handle_exit.c @@ -63,6 +63,8 @@ static int handle_hvc(struct kvm_vcpu *vcpu) static int handle_smc(struct kvm_vcpu *vcpu) { + int ret; + /* * "If an SMC instruction executed at Non-secure EL1 is * trapped to EL2 because HCR_EL2.TSC is 1, the exception is a @@ -70,10 +72,28 @@ 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... + * + * If imm is non-zero, it's not defined, so just skip it. + */ + if (kvm_vcpu_hvc_get_imm(vcpu)) { + vcpu_set_reg(vcpu, 0, ~0UL); + kvm_incr_pc(vcpu); + return 1; + } + + /* + * If imm is zero, it's a psci call. + * Note that on ARMv8.3, even if EL3 is not implemented, SMC executed + * at Non-secure EL1 is trapped to EL2 if HCR_EL2.TSC==1, rather than + * being treated as UNDEFINED. */ - vcpu_set_reg(vcpu, 0, ~0UL); + ret = kvm_hvc_call_handler(vcpu); + if (ret < 0) + vcpu_set_reg(vcpu, 0, ~0UL); + kvm_incr_pc(vcpu); - return 1; + + return ret; } /*