Re: [PATCH 3/6] KVM: arm64: Consider SCTLR_EL2.M when mapping the L1 VNCR page

On Tue, 04 Aug 2026 12:03:31 +0100,
Joey Gouly <[email protected]> wrote:
> 
> Hi Marc,
> 
> Two small comments/suggestions.
> 
> On Sat, Aug 01, 2026 at 01:48:15PM +0100, Marc Zyngier wrote:
> > We record a VNCR TLB even when SCTLR_EL2.M is 0 in order to make
> > our life easier. But this is not something that the architecture
> > anticipate.
> > 
> > As a consequence, a hypervisor is free to set VNCR_EL2 to
> > some PA when SCTLR_EL2.M==0, use it to run a guest which indirectly
> > accesses the VNCR page, then eventually set SCTLR_EL2.M==1 with
> > the same VA. Yes, this is odd, but apparently legal.
> > 
> > A common trick in HW is to invalidate the TLBs on SCTLR_ELx.M being
> > flipped. But doing this is a not a good idea for us (we'd need to
> > trap SCTLR accesses), and wouldn't scale as we nest deeper.
> > 
> > Instead, use the fact that the S1 MMU being off at the point of
> > translation is cached in our TLB, and simply ignore it if it
> > doesn't match the current MMU state.
> 
> Could you expand/change this a little, something like:
> 	
> 	Instead, use the fact that the S1 MMU being off at the point of
> 	translation is cached in our VNCR TLB, and if it doesn't match
> 	the current MMU state, leave the VNCR unmapped.
>

Yup, that reads better. Thanks.

> > 
> > Fixes: 2a359e072596f ("KVM: arm64: nv: Handle mapping of VNCR_EL2 at EL2")
> > Signed-off-by: Marc Zyngier <[email protected]>
> > Cc: [email protected]
> > ---
> >  arch/arm64/include/asm/kvm_nested.h | 7 +++++++
> >  arch/arm64/kvm/at.c                 | 2 --
> >  arch/arm64/kvm/nested.c             | 3 +++
> >  3 files changed, 10 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/kvm_nested.h b/arch/arm64/include/asm/kvm_nested.h
> > index 012d711034d17..cc48817a5bf7a 100644
> > --- a/arch/arm64/include/asm/kvm_nested.h
> > +++ b/arch/arm64/include/asm/kvm_nested.h
> > @@ -388,6 +388,8 @@ struct s1_walk_result {
> >  	bool	failed;
> >  };
> >  
> > +#define S1_MMU_DISABLED		(-127)
> > +
> >  static inline void fail_s1_walk(struct s1_walk_result *wr, u8 fst, bool s1ptw)
> >  {
> >  	wr->fst		= fst;
> > @@ -396,6 +398,11 @@ static inline void fail_s1_walk(struct s1_walk_result *wr, u8 fst, bool s1ptw)
> >  	wr->failed	= true;
> >  }
> >  
> > +static inline bool s1_mmu_disabled(struct s1_walk_result *wr)
> > +{
> > +	return wr->level == S1_MMU_DISABLED;
> > +}
> > +
> >  int __kvm_translate_va(struct kvm_vcpu *vcpu, struct s1_walk_info *wi,
> >  		       struct s1_walk_result *wr, u64 va);
> >  int __kvm_find_s1_desc_level(struct kvm_vcpu *vcpu, u64 va, u64 ipa,
> > diff --git a/arch/arm64/kvm/at.c b/arch/arm64/kvm/at.c
> > index 640f2dc00a8ba..0926426b87989 100644
> > --- a/arch/arm64/kvm/at.c
> > +++ b/arch/arm64/kvm/at.c
> > @@ -11,8 +11,6 @@
> >  #include <asm/kvm_mmu.h>
> >  #include <asm/lsui.h>
> >  
> > -#define S1_MMU_DISABLED		(-127)
> > -
> >  static int get_ia_size(struct s1_walk_info *wi)
> >  {
> >  	return 64 - wi->txsz;
> > diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
> > index 035cda256e2a5..d7dba02dc84fe 100644
> > --- a/arch/arm64/kvm/nested.c
> > +++ b/arch/arm64/kvm/nested.c
> > @@ -1578,6 +1578,9 @@ static void kvm_map_l1_vncr(struct kvm_vcpu *vcpu)
> >  	if (!vt->valid)
> >  		return;
> >  
> > +	if (!(vcpu_read_sys_reg(vcpu, SCTLR_EL2) & SCTLR_ELx_M) != s1_mmu_disabled(&vt->wr))
> > +		return;
> 
> A comment here would also be useful for future archaeology (also current
> reading since the !( .. != ) code is confusing).
> 
> Or maybe:
> 
> 	bool mmu_disabled_sctlr = vcpu_read_sys_reg(vcpu, SCTLR_EL2) & SCTLR_ELx_M) == 0;
> 	if (mmu_disabled_sctlr != s1_mmu_disabled(&vt->wr)) 

And maybe using positive logic? I'll see what I can do.

Thanks,

	M.

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