Re: [PATCH 02/22] KVM: arm64: nv: Consolidate computation of stage-2 permissions

[Oliver Upton <oupton@kernel.org>]

Hi Leo,

On Mon, Sep 21, 2026 at 02:46:17PM +0100, Leonardo Bras wrote:
> > +static void compute_s2_permissions(struct kvm_vcpu *vcpu, struct s2_walk_info *wi,
> > +				   struct s2_walk_step *ws, struct kvm_s2_trans *trans)
> > +{
> > +	u8 s2ap = FIELD_GET(KVM_PTE_LEAF_ATTR_LO_S2_S2AP, ws->desc);
> > +	u8 xn = FIELD_GET(KVM_PTE_LEAF_ATTR_HI_S2_XN, ws->desc);
> > +
> > +	if (!kvm_has_xnx(vcpu->kvm))
> > +		xn &= 0b10;
> > +
> > +	switch (xn) {
> > +	case 0b00:
> > +		trans->px = trans->ux = true;
> > +		break;
> > +	case 0b01:
> > +		trans->px = false;
> > +		trans->ux = true;
> > +		break;
> > +	case 0b10:
> > +		trans->px = trans->ux = false;
> > +		break;
> > +	case 0b11:
> > +		trans->px = true;
> > +		trans->ux = false;
> > +		break;
> > +	}
> > +
> 
> Nit: couldn't the above be summarized as below?
> 
> 	trans->ux = !(xn & BIT(1));
> 
> 	if (kvm_has_xnx(vcpu->kvm) && xn & BIT(0)) 
> 		trans->px = !trans->ux;
> 	else
> 		trans->px = trans->ux;
> 					
> 
> or maybe even skip the FIELD_GET and do:
> 
> 	trans->ux = !(ws->desc & KVM_PTE_LEAF_ATTR_HI_S1_UXN);
> 
> 	if (kvm_has_xnx(vcpu->kvm) &&
> 	    ws->desc & KVM_PTE_LEAF_ATTR_HI_S1_PXN) 
> 		trans->px = !trans->ux;
> 	else
> 		trans->px = trans->ux;
> 
> 
> Maybe it's just me, but that looks easier to understand what each bit does 
> based on the feature.

I had opted for a switch statement because I think it's a direct
translation of the truth table in R_STCTY.

> FWIW:
> Reviewed-by: Leonardo Bras <leo.bras@arm.com>

Thanks!

Best,
Oliver