Re: [PATCH 12/18] KVM: arm64: nv: Handle PSCI call via smc from the guest

[Marc Zyngier <maz@kernel.org>]

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.

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