Re: [PATCH v2 6/6] KVM: arm64: Honor UX/PX attributes for EL2 S1 mappings

[Marc Zyngier <maz@kernel.org>]

On Thu, 11 Dec 2025 15:18:51 +0000,
Joey Gouly <joey.gouly@arm.com> wrote:
> 
> Question,
> 
> On Wed, Dec 10, 2025 at 05:30:24PM +0000, Marc Zyngier wrote:
> > Now that we potentially have two bits to deal with when setting
> > execution permissions, make sure we correctly handle them when both
> > when building the page tables and when reading back from them.
> > 
> > Reported-by: Alexandru Elisei <alexandru.elisei@arm.com>
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> >  arch/arm64/include/asm/kvm_pgtable.h | 12 +++---------
> >  arch/arm64/kvm/hyp/pgtable.c         | 24 +++++++++++++++++++++---
> >  2 files changed, 24 insertions(+), 12 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/kvm_pgtable.h b/arch/arm64/include/asm/kvm_pgtable.h
> > index be68b89692065..095e6b73740a6 100644
> > --- a/arch/arm64/include/asm/kvm_pgtable.h
> > +++ b/arch/arm64/include/asm/kvm_pgtable.h
> > @@ -87,15 +87,9 @@ typedef u64 kvm_pte_t;
> >  
> >  #define KVM_PTE_LEAF_ATTR_HI_SW		GENMASK(58, 55)
> >  
> > -#define __KVM_PTE_LEAF_ATTR_HI_S1_XN	BIT(54)
> > -#define __KVM_PTE_LEAF_ATTR_HI_S1_UXN	BIT(54)
> > -#define __KVM_PTE_LEAF_ATTR_HI_S1_PXN	BIT(53)
> > -
> > -#define KVM_PTE_LEAF_ATTR_HI_S1_XN					\
> > -	({ cpus_have_final_cap(ARM64_KVM_HVHE) ?			\
> > -			(__KVM_PTE_LEAF_ATTR_HI_S1_UXN |		\
> > -			 __KVM_PTE_LEAF_ATTR_HI_S1_PXN) :		\
> > -			__KVM_PTE_LEAF_ATTR_HI_S1_XN; })
> > +#define KVM_PTE_LEAF_ATTR_HI_S1_XN	BIT(54)
> > +#define KVM_PTE_LEAF_ATTR_HI_S1_UXN	BIT(54)
> > +#define KVM_PTE_LEAF_ATTR_HI_S1_PXN	BIT(53)
> >  
> >  #define KVM_PTE_LEAF_ATTR_HI_S2_XN	GENMASK(54, 53)
> >  
> > diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
> > index e0bd6a0172729..97c0835d25590 100644
> > --- a/arch/arm64/kvm/hyp/pgtable.c
> > +++ b/arch/arm64/kvm/hyp/pgtable.c
> > @@ -342,6 +342,9 @@ static int hyp_set_prot_attr(enum kvm_pgtable_prot prot, kvm_pte_t *ptep)
> >  	if (!(prot & KVM_PGTABLE_PROT_R))
> >  		return -EINVAL;
> >  
> > +	if (!cpus_have_final_cap(ARM64_KVM_HVHE))
> > +		prot &= ~KVM_PGTABLE_PROT_UX;
> 
> Trying to understand this part. We don't consider KVM_PGTABLE_PROT_UX below
> when !HVHE, and we don't set it in kvm_pgtable_hyp_pte_prot() when !HVHE
> either, so can it ever actually be set?

Because KVM_PGTABLE_PROT_X, which is directly passed by the high-level
code, is defined as such:

	KVM_PGTABLE_PROT_X			= KVM_PGTABLE_PROT_PX	|
						  KVM_PGTABLE_PROT_UX,

We *could* make that value dependent on HVHE, but since that's in an
enum, it is pretty ugly to do (not impossible though).

But it is in the following code that this becomes useful...

>
> Otherwise LGTM!
> 
> Thanks,
> Joey
> 
> > +
> >  	if (prot & KVM_PGTABLE_PROT_X) {
> >  		if (prot & KVM_PGTABLE_PROT_W)
> >  			return -EINVAL;

... here. If you were passed UX (and only that), and that you're
!HVHE, you won't have execution at all, and can allow writes.

Does that make sense?

	M.

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