Re: [PATCH v2 1/2] KVM: arm64: nv: Fix handling of XN[0] when !FEAT_XNX

[Oliver Upton <oupton@kernel.org>]

Hey Wei-Lin,

Thanks for the review.

On Wed, Jun 03, 2026 at 11:57:20PM +0100, Wei-Lin Chang wrote:
> Hi Oliver,
> 
> On Tue, Jun 02, 2026 at 09:59:00AM -0700, Oliver Upton wrote:
> > XN has already been extracted from its bitfield position so using
> > FIELD_PREP() on the mask that clears XN[0] is completely broken, having
> > the effect of unconditionally granting execute permissions...
> > 
> > Fix the obvious mistake by manipulating the right bit.
> > 
> > Cc: stable@vger.kernel.org
> > Fixes: d93febe2ed2e ("KVM: arm64: nv: Forward FEAT_XNX permissions to the shadow stage-2")
> > Reviewed-by: Wei-Lin Chang <weilin.chang@arm.com>
> > Signed-off-by: Oliver Upton <oupton@kernel.org>
> > ---
> >  arch/arm64/include/asm/kvm_nested.h | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/kvm_nested.h b/arch/arm64/include/asm/kvm_nested.h
> > index 091544e6af44..a0eb83319c2e 100644
> > --- a/arch/arm64/include/asm/kvm_nested.h
> > +++ b/arch/arm64/include/asm/kvm_nested.h
> > @@ -131,7 +131,7 @@ static inline bool kvm_s2_trans_exec_el0(struct kvm *kvm, struct kvm_s2_trans *t
> >  	u8 xn = FIELD_GET(KVM_PTE_LEAF_ATTR_HI_S2_XN, trans->desc);
> >  
> >  	if (!kvm_has_xnx(kvm))
> > -		xn &= FIELD_PREP(KVM_PTE_LEAF_ATTR_HI_S2_XN, 0b10);
> > +		xn &= 0b10;
> >  
> >  	switch (xn) {
> >  	case 0b00:
> > @@ -147,7 +147,7 @@ static inline bool kvm_s2_trans_exec_el1(struct kvm *kvm, struct kvm_s2_trans *t
> >  	u8 xn = FIELD_GET(KVM_PTE_LEAF_ATTR_HI_S2_XN, trans->desc);
> >  
> >  	if (!kvm_has_xnx(kvm))
> > -		xn &= FIELD_PREP(KVM_PTE_LEAF_ATTR_HI_S2_XN, 0b10);
> > +		xn &= 0b10;
> >  
> 
> Now that the other patch brings up kvm_pgtable_stage2_pte_prot(), what
> do you think about also using that here? It can save a little bit of
> duplicated decode logic.
> 
> Other than this being in a header and we'll have to move the code
> around for this to work, I'm curious are there any other issues with
> this idea?

No issues with your suggestion but I plan on nuking the kvm_s2_trans*()
accessors soon :)

Ultimately kvm_s2_trans should just contain pre-computed permissions,
which matters more when dealing with descriptor fields that require the
MMU context to make sense of (like DBM). On top of that, getters for
obviously named fields isn't adding a whole lot.

Any concerns with leaving as-is for now?

-- 
Thanks,
Oliver