Re: [PATCH 1/2] KVM: arm64: nv: Fix relative priorities of exceptions generated by ERETAx

[Marc Zyngier <maz@kernel.org>]

Hi Joey,

On Wed, 29 May 2024 15:27:27 +0100,
Joey Gouly <joey.gouly@arm.com> wrote:
> 
> Hi Marc,
> 
> These references are from ARM DDI 0487 J.a.

Ah, you have an upgrade pending! ;-)

> 
> On Tue, May 28, 2024 at 11:06:31AM +0100, Marc Zyngier wrote:
> > ERETAx can fail in multiple ways:
> > 
> > (1) ELR_EL2 points lalaland
> > (2) we get a PAC failure
> > (3) SPSR_EL2 has the wrong mode
> > 
> > (1) is easy, as we just let the CPU do its thing and deliver an
> > Instruction Abort. However, (2) and (3) are interesting, because
> > the PAC failure priority is way below that of the Illegal Execution
> > State exception.
> > 
> > Which means that if we have detected a PAC failure (and that we have
> > FPACCOMBINE), we must be careful to give priority to the Illegal
> > Execution State exception, should one be pending.
> 
> This is IZFGJP Prioritization of Synchronous exceptions taken to
> AArch64 state.
>

Indeed.

> > 
> > Solving this involves hoisting the SPSR calculation earlier and
> > testing for the IL bit before injecting the FPAC exception.
> > 
> > In the extreme case of a ERETAx returning to an invalid mode *and*
> > failing its PAC check, we end up with an Instruction Abort (due
> > to the new PC being mangled by the failed Auth) *and* PSTATE.IL
> > being set. Which matches the requirements of the architecture.
> 
> And this is IGPPXQ, which says "which causes the next instruction to
> generate an Illegal State exception. The exception return
> instruction does not generate the exception."
> 
> Which matches since Instruction Abort has a higher priority than
> Illegal Exception.

Spot on. Although it is a bit surprising that in all cases, the CPU
has to attempt fetching the next instruction before delivering the IL
exception so that it can prioritise the Abort. I guess it simplifies
some HW implementations, but makes SW suffer a bit.

> 
> > 
> > Whilst we're at it, remove a stale comment that states the obvious
> > and only confuses the reader.
> > 
> > Fixes: 213b3d1ea161 ("KVM: arm64: nv: Handle ERETA[AB] instructions")
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> >  arch/arm64/kvm/emulate-nested.c | 21 +++++++++++----------
> >  1 file changed, 11 insertions(+), 10 deletions(-)
> > 
> > diff --git a/arch/arm64/kvm/emulate-nested.c b/arch/arm64/kvm/emulate-nested.c
> > index 72d733c74a38..54090967a335 100644
> > --- a/arch/arm64/kvm/emulate-nested.c
> > +++ b/arch/arm64/kvm/emulate-nested.c
> > @@ -2181,16 +2181,23 @@ void kvm_emulate_nested_eret(struct kvm_vcpu *vcpu)
> >  	if (forward_traps(vcpu, HCR_NV))
> >  		return;
> >  
> > +	spsr = vcpu_read_sys_reg(vcpu, SPSR_EL2);
> > +	spsr = kvm_check_illegal_exception_return(vcpu, spsr);
> > +
> >  	/* Check for an ERETAx */
> >  	esr = kvm_vcpu_get_esr(vcpu);
> >  	if (esr_iss_is_eretax(esr) && !kvm_auth_eretax(vcpu, &elr)) {
> >  		/*
> > -		 * Oh no, ERETAx failed to authenticate.  If we have
> > -		 * FPACCOMBINE, deliver an exception right away.  If we
> > -		 * don't, then let the mangled ELR value trickle down the
> > +		 * Oh no, ERETAx failed to authenticate.
> > +		 *
> > +		 * If we have FPACCOMBINE and we don't have a pending
> > +		 * Illegal Execution State exception (which has priority
> > +		 * over FPAC), deliver an exception right away.
> > +		 *
> > +		 * Otherwise, let the mangled ELR value trickle down the
> >  		 * ERET handling, and the guest will have a little surprise.
> >  		 */
> > -		if (kvm_has_pauth(vcpu->kvm, FPACCOMBINE)) {
> > +		if (kvm_has_pauth(vcpu->kvm, FPACCOMBINE) && !(spsr & PSR_IL_BIT)) {
> >  			esr &= ESR_ELx_ERET_ISS_ERETA;
> >  			esr |= FIELD_PREP(ESR_ELx_EC_MASK, ESR_ELx_EC_FPAC);
> >  			kvm_inject_nested_sync(vcpu, esr);
> > @@ -2201,17 +2208,11 @@ void kvm_emulate_nested_eret(struct kvm_vcpu *vcpu)
> >  	preempt_disable();
> >  	kvm_arch_vcpu_put(vcpu);
> >  
> > -	spsr = __vcpu_sys_reg(vcpu, SPSR_EL2);
> > -	spsr = kvm_check_illegal_exception_return(vcpu, spsr);
> >  	if (!esr_iss_is_eretax(esr))
> >  		elr = __vcpu_sys_reg(vcpu, ELR_EL2);
> >  
> >  	trace_kvm_nested_eret(vcpu, elr, spsr);
> >  
> > -	/*
> > -	 * Note that the current exception level is always the virtual EL2,
> > -	 * since we set HCR_EL2.NV bit only when entering the virtual EL2.
> > -	 */
> >  	*vcpu_pc(vcpu) = elr;
> >  	*vcpu_cpsr(vcpu) = spsr;
> >  
> 
> Those references were just me checking it, not suggesting you edit
> the commit message.

Well, that's excellent detective work!

>
> Reviewed-by: Joey Gouly <joey.gouly@arm.com>

Thanks again,

	M.

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