Hi Joey,
On Wed, 29 May 2024 15:27:27 +0100,
Joey Gouly <joey.gouly@xxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxx>
Thanks again,
M.
--
Without deviation from the norm, progress is not possible.
