Re: [PATCH 06/22] KVM: arm64: nv: Use a helper for stage-2 descriptor updates

[Oliver Upton <oupton@kernel.org>]

On Tue, Sep 22, 2026 at 05:14:19PM +0100, Leonardo Bras wrote:
> > @@ -386,21 +403,9 @@ static int walk_nested_s2_pgd(struct kvm_vcpu *vcpu, struct kvm_walk_access *acc
> >  		return 1;
> >  	}
> >  
> > -	if (wi->ha)
> > -		new_desc |= KVM_PTE_LEAF_ATTR_LO_S2_AF;
> > -
> > -	if (new_desc != ws.desc) {
> > -		ret = swap_guest_s2_desc(vcpu, ws.desc_pa, ws.desc, new_desc, wi);
> > -		if (ret == -EAGAIN)
> > -			return ret;
> > -		if (ret) {
> > -			out->esr = ESR_ELx_FSC_SEA_TTW(ws.level);
> > -			out->desc = ws.desc;
> > -			return 1;
> > -		}
> > -
> > -		ws.desc = new_desc;
> > -	}
> > +	ret = handle_desc_update(vcpu, wi, &ws, out, access);
> > +	if (ret)
> > +		return ret;
> >  
> >  	if (!(ws.desc & KVM_PTE_LEAF_ATTR_LO_S2_AF)) {
> >  		out->esr = compute_fsc(ws.level, ESR_ELx_FSC_ACCESS);
> > -- 
> > 2.47.3
> > 
> 
> IIUC, you pulled stuff around swap_guest_s2_desc() and put it inside it, 
> while changing the function name to handle_desc_update(), so it should be 
> the same logic.
> 
> It looks much better this way, and most things seem to have the same efect.
> 
> The only thing that I notice was "ws->desc = new":
> The previous version would do that only after running __kvm_at_swap_desc() 
> and only when the return value is zero. 
> 
> The new version does that every time "old != new", and before running 
> __kvm_at_swap_desc() and does not take into account it's return value.
> 
> This means that on this new version if __kvm_at_swap_desc() returns 1:
> 	ws->desc = new;
> 	ret = 1;
> 	out->desc = ws->desc; //(i.e. new)
> 
> And in the previous version, it would make out->desc = ws->desc, that being 
> the original value, not the new one.
> 
> Is this ok? 
> (I am commenting because it could be unintentional)

Yes, this is ok.

If __kvm_at_swap_desc() returns -EAGAIN we replay the faulting
instruction. No consumption of the walk result.

If __kvm_at_swap_desc() returns anything else we return an SEA TTW
fault via the walk result struct. Notice that it is a union, and
readers are expected to dereference the right sub-struct based on
s1_walk_result::failed.

Thanks,
Oliver