Re: [PATCH] KVM: arm64: nv: Fix PSTATE construction on illegal exception return

[Marc Zyngier <maz@kernel.org>]

On Wed, 17 Jun 2026 18:19:20 +0100,
Fuad Tabba <tabba@google.com> wrote:
> 
> On Wed, 17 Jun 2026 at 17:45, Marc Zyngier <maz@kernel.org> wrote:
> >
> > On Wed, 17 Jun 2026 15:49:07 +0100,
> > Fuad Tabba <tabba@google.com> wrote:
> > >
> > > kvm_check_illegal_exception_return() sourced the flags {N,Z,C,V} and
> > > masks {D,A,I,F} of the resulting PSTATE from the current PSTATE, but
> > > R_VWJHB takes them from the SPSR being returned to and leaves
> > > PSTATE.{EL,SP,nRW} (and EXLOCK when FEAT_GCS) unchanged. PAN, ALLINT
> > > and PM were not applied at all.
> > >
> > > Build the PSTATE by taking those fields from the SPSR while preserving
> > > EL, SP, nRW and EXLOCK from the current PSTATE, then set IL.
> > >
> > > Fixes: 47f3a2fc765a ("KVM: arm64: nv: Support virtual EL2 exceptions")
> > > Suggested-by: Marc Zyngier <maz@kernel.org>
> > > Link: https://lore.kernel.org/all/86wlvxs5r0.wl-maz@kernel.org/
> > > Signed-off-by: Fuad Tabba <tabba@google.com>
> > > ---
> > > This is a modified version of Marc's suggested diff [1]. That diff applied
> > > a single mask to the incoming SPSR, which also takes PSTATE.{EL,SP,nRW}
> > > (and EXLOCK) from the SPSR. The ARM ARM leaves those fields unchanged on an
> > > illegal exception return. This path is reached precisely because SPSR.M is
> > > illegal (EL3, M[1]=1, AArch32, EL1 under TGE), so this version preserves
> > > EL/SP/nRW/EXLOCK from the current PSTATE and takes only the flags, masks
> > > and PAN/ALLINT/PM from the SPSR.
> > >
> > > [1] https://lore.kernel.org/all/86wlvxs5r0.wl-maz@kernel.org/
> > > ---
> > >  arch/arm64/kvm/emulate-nested.c | 33 +++++++++++++++++++++++----------
> > >  1 file changed, 23 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/arch/arm64/kvm/emulate-nested.c b/arch/arm64/kvm/emulate-nested.c
> > > index dba7ced74ca5..ace2b40cf875 100644
> > > --- a/arch/arm64/kvm/emulate-nested.c
> > > +++ b/arch/arm64/kvm/emulate-nested.c
> > > @@ -2738,17 +2738,30 @@ static u64 kvm_check_illegal_exception_return(struct kvm_vcpu *vcpu, u64 spsr)
> > >           (spsr & PSR_MODE32_BIT) ||
> > >           (vcpu_el2_tge_is_set(vcpu) && (mode == PSR_MODE_EL1t ||
> > >                                          mode == PSR_MODE_EL1h))) {
> > > -             /*
> > > -              * The guest is playing with our nerves. Preserve EL, SP,
> > > -              * masks, flags from the existing PSTATE, and set IL.
> > > -              * The HW will then generate an Illegal State Exception
> > > -              * immediately after ERET.
> > > -              */
> > > -             spsr = *vcpu_cpsr(vcpu);
> > > +             u64 cpsr = *vcpu_cpsr(vcpu);
> > > +             u64 mask;
> > >
> > > -             spsr &= (PSR_D_BIT | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT |
> > > -                      PSR_N_BIT | PSR_Z_BIT | PSR_C_BIT | PSR_V_BIT |
> > > -                      PSR_MODE_MASK | PSR_MODE32_BIT);
> > > +             /*
> > > +              * On an illegal exception return, PSTATE.{EL,SP,nRW} and,
> > > +              * if FEAT_GCS, PSTATE.EXLOCK are unchanged, while the flags
> > > +              * and masks are taken from the SPSR (R_VWJHB). Set IL so the
> > > +              * HW generates an Illegal State Exception right after ERET.
> > > +              */
> > > +             mask = PSR_D_BIT | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT |
> > > +                    PSR_N_BIT | PSR_Z_BIT | PSR_C_BIT | PSR_V_BIT;
> > > +
> > > +             if (kvm_has_feat(vcpu->kvm, ID_AA64MMFR1_EL1, PAN, IMP))
> > > +                     mask |= PSR_PAN_BIT;
> > > +             if (kvm_has_feat(vcpu->kvm, ID_AA64PFR1_EL1, NMI, IMP))
> > > +                     mask |= ALLINT_ALLINT;
> > > +             /* FEAT_SPE_EXC and FEAT_TRBE_EXC also gate PSTATE.PM one day... */
> > > +             if (kvm_has_feat(vcpu->kvm, ID_AA64DFR1_EL1, EBEP, IMP))
> > > +                     mask |= BIT_ULL(32);    /* PSTATE.PM */
> > > +
> > > +             spsr &= mask;
> > > +             spsr |= cpsr & (PSR_MODE_MASK | PSR_MODE32_BIT);
> > > +             if (kvm_has_feat(vcpu->kvm, ID_AA64PFR1_EL1, GCS, IMP))
> > > +                     spsr |= cpsr & BIT_ULL(34);     /* PSTATE.EXLOCK */
> > >               spsr |= PSR_IL_BIT;
> > >       }
> >
> > While I'm happy that you caught the bugs I left for you to address,
> > the overall structure is a bit inconsistent. I'd like to have:
> >
> > - a mask of the bits we preserve from SPSR, and apply that to SPSR
> >   itself
> >
> > - a mask of the bits we preserve from PSTATE, and transfer them to
> >   SPSR
> >
> > - the comment at the top to describe this in that particular order.
> >
> > With that, I reworked your patch as follows:
> >
> > diff --git a/arch/arm64/kvm/emulate-nested.c b/arch/arm64/kvm/emulate-nested.c
> > index bb335fa16f7cc..243e5e26f7018 100644
> > --- a/arch/arm64/kvm/emulate-nested.c
> > +++ b/arch/arm64/kvm/emulate-nested.c
> > @@ -2746,14 +2746,14 @@ static u64 kvm_check_illegal_exception_return(struct kvm_vcpu *vcpu, u64 spsr)
> >             (spsr & PSR_MODE32_BIT) ||
> >             (vcpu_el2_tge_is_set(vcpu) && (mode == PSR_MODE_EL1t ||
> >                                            mode == PSR_MODE_EL1h))) {
> > -               u64 cpsr = *vcpu_cpsr(vcpu);
> >                 u64 mask;
> >
> >                 /*
> > -                * On an illegal exception return, PSTATE.{EL,SP,nRW} and,
> > -                * if FEAT_GCS, PSTATE.EXLOCK are unchanged, while the flags
> > -                * and masks are taken from the SPSR (R_VWJHB). Set IL so the
> > -                * HW generates an Illegal State Exception right after ERET.
> > +                * On an illegal exception return, the flags and masks are
> > +                * taken from the SPSR while PSTATE.{EL,SP,nRW} and, if
> > +                * FEAT_GCS, PSTATE.EXLOCK are unchanged (R_VWJHB). Set IL
> > +                * so the HW generates an Illegal State Exception right
> > +                * after ERET.
> >                  */
> >                 mask = PSR_D_BIT | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT |
> >                        PSR_N_BIT | PSR_Z_BIT | PSR_C_BIT | PSR_V_BIT;
> > @@ -2767,9 +2767,12 @@ static u64 kvm_check_illegal_exception_return(struct kvm_vcpu *vcpu, u64 spsr)
> >                         mask |= BIT_ULL(32);    /* PSTATE.PM */
> >
> >                 spsr &= mask;
> > -               spsr |= cpsr & (PSR_MODE_MASK | PSR_MODE32_BIT);
> > +
> > +               mask = PSR_MODE_MASK | PSR_MODE32_BIT;
> >                 if (kvm_has_feat(vcpu->kvm, ID_AA64PFR1_EL1, GCS, IMP))
> > -                       spsr |= cpsr & BIT_ULL(34);     /* PSTATE.EXLOCK */
> > +                       mask |= BIT_ULL(34);    /* PSTATE.EXLOCK */
> > +
> > +               spsr |= *vcpu_cpsr(vcpu) & mask;
> >                 spsr |= PSR_IL_BIT;
> >         }
> >
> > Does that work for you?
> 
> Yes, that's cleaner, thanks. The two-mask form (take from SPSR, then
> preserve from PSTATE) reads better than my interleaved version.
> 
> Would you like me to send a v2 (which would be identical to the above?
> :) ), or would you fold it in?

I've squashed that in.

Thanks,

	M.

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