Re: [PATCH v6 09/64] KVM: arm64: nv: Support virtual EL2 exceptions

[Alexandru Elisei <alexandru.elisei@arm.com>]

Hi Marc,

On Thu, Feb 03, 2022 at 05:43:36PM +0000, Marc Zyngier wrote:
> On Wed, 02 Feb 2022 15:23:20 +0000,
> Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> > 
> > Hi,
> > 
> > On Fri, Jan 28, 2022 at 12:18:17PM +0000, Marc Zyngier wrote:
> > > diff --git a/arch/arm64/kvm/emulate-nested.c b/arch/arm64/kvm/emulate-nested.c
> > > new file mode 100644
> > > index 000000000000..f52cd4458947
> > > --- /dev/null
> > > +++ b/arch/arm64/kvm/emulate-nested.c
> > > @@ -0,0 +1,197 @@
> > 
> > Looks like this line:
> > 
> > // SPDX-License-Identifier: GPL-2.0-only
> > 
> > is missing.
> 
> Indeed. I should check all the new files, as they are a bit... off.
> 
> > 
> > > +/*
> > > + * Copyright (C) 2016 - Linaro and Columbia University
> > > + * Author: Jintack Lim <jintack.lim@linaro.org>
> > > + */
> > > +
> > > +#include <linux/kvm.h>
> > > +#include <linux/kvm_host.h>
> > > +
> > > +#include <asm/kvm_emulate.h>
> > > +#include <asm/kvm_nested.h>
> > > +
> > > +#include "hyp/include/hyp/adjust_pc.h"
> > > +
> > > +#include "trace.h"
> > > +
> > > +static u64 kvm_check_illegal_exception_return(struct kvm_vcpu *vcpu, u64 spsr)
> > > +{
> > > +	u64 mode = spsr & PSR_MODE_MASK;
> > > +
> > > +	/*
> > > +	 * Possible causes for an Illegal Exception Return from EL2:
> > > +	 * - trying to return to EL3
> > > +	 * - trying to return to a 32bit EL
> > > +	 * - trying to return to EL1 with HCR_EL2.TGE set
> > > +	 */
> > > +	if (mode == PSR_MODE_EL3t || mode == PSR_MODE_EL3h ||
> > > +	    spsr & PSR_MODE32_BIT ||
> > 
> > I take it KVM will not allow a L1 hypervisor to run EL1 or EL0 in 32
> > bit mode?
> 
> No, that'd really be a distraction at this stage. I don't expect any
> HW supporting NV NV to support AArch32 at EL1, and if someone really
> needs EL0 support (the HW support actually exists), they'll have to
> revisit this.
> 
> >
> > > +	    (vcpu_el2_tge_is_set(vcpu) && (mode == PSR_MODE_EL1t ||
> > > +					   mode == PSR_MODE_EL1h))) {
> > 
> > I think these checks should also be added:
> > 
> > "A return where the value of the saved process state M[4] bit is 0,
> > indicating a return to AArch64 state, and one of the following is
> > true:
> > 
> > - The M[1] bit is 1.
> > - The M[3:0] bits are 0b0001.
> 
> Definitely should add these two, probably in the form of a switch
> enumerating all the possible exception levels rather than checking for
> discrete bits that are hard to reason about.
> 
> > - The Exception level being returned to is using AArch32 state, as
> >   programmed by the SCR_EL3.RW or HCR_EL2.RW bits, or as configured
> >   from reset."
> 
> That's already caught with the SPSR check above.

Hmm... I don't think so. The illegal condition, according to the snippet, should
be:

!(mode & PSR_MODE32_BIT) && !(__vcpu_sys_reg(vcpu, HCR_EL2) & HCR_RW)

Or, perhaps better, KVM could add an HCR_EL2 accessor that treated the
HCR_EL2.RW bit as RA0/WI as per the architecture when the EL1 is not capable of
AArch32.  That would make the above situation impossible.

Thanks,
Alex

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