Re: [PATCH 08/22] KVM: arm64: nv: Treat DBM as writable at stage-2

[Oliver Upton <oupton@kernel.org>]

On Wed, Sep 23, 2026 at 03:38:36PM +0100, Leonardo Bras wrote:
> On Tue, Jun 23, 2026 at 11:41:47AM -0700, Oliver Upton wrote:
> > When using direct permissions, the DBM bit has the effect of granting
> > the write permission. And, depending on the access, it could grant write
> > permission without a dirty state update.
> > 
> > Signed-off-by: Oliver Upton <oupton@kernel.org>
> > ---
> >  arch/arm64/include/asm/kvm_pgtable.h |  1 +
> >  arch/arm64/kvm/nested.c              | 11 +++++++++++
> >  2 files changed, 12 insertions(+)
> > 
> > diff --git a/arch/arm64/include/asm/kvm_pgtable.h b/arch/arm64/include/asm/kvm_pgtable.h
> > index 22aeb2ed18d1..6ae36973686c 100644
> > --- a/arch/arm64/include/asm/kvm_pgtable.h
> > +++ b/arch/arm64/include/asm/kvm_pgtable.h
> > @@ -94,6 +94,7 @@ typedef u64 kvm_pte_t;
> >  #define KVM_PTE_LEAF_ATTR_HI_S1_PXN	BIT(53)
> >  
> >  #define KVM_PTE_LEAF_ATTR_HI_S2_XN	GENMASK(54, 53)
> > +#define KVM_PTE_LEAF_ATTR_HI_S2_DBM	BIT(51)
> >  
> >  #define KVM_PTE_LEAF_ATTR_HI_S1_GP	BIT(50)
> >  
> > diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
> > index e5a407fc0880..4f13f37e560b 100644
> > --- a/arch/arm64/kvm/nested.c
> > +++ b/arch/arm64/kvm/nested.c
> > @@ -305,6 +305,17 @@ static void compute_s2_permissions(struct kvm_vcpu *vcpu, struct s2_walk_info *w
> >  		break;
> >  	}
> >  
> > +	/*
> > +	 * Descriptors with the DBM bit set while hardware dirty state are
> > +	 * considered writable, even though certain accesses (like AT instructions)
> > +	 * don't actually update the dirty state.
> > +	 *
> > +	 * Assume that walk_nestd_s2_pgd() made the necessary descriptor updates
> > +	 * for the access and just treat DBM as writable here.
> > +	 */
> > +	if (wi->hd && ws->desc & KVM_PTE_LEAF_ATTR_HI_S2_DBM)
> > +		s2ap |= BIT(1);
> > +
> >  	trans->readable = s2ap & BIT(0);
> >  	trans->writable = s2ap & BIT(1);
> >  
> > -- 
> > 2.47.3
> > 
> 
> 
> Okay, now I see why were you doing that on the previous patch.
> 
> Here you test DBM and set writable to 1, and change nothing else, as s2ap 
> is a local variable that is only used to set writable if DBM is set.
> 
> This feels weird, though. That way it looks like DBM is changing s2ap, 
> which is not.
> 
> I suggest doing something like:
> 
> +     if (wi->hd && ws->desc & KVM_PTE_LEAF_ATTR_HI_S2_DBM)
> +             trans->writable = true;
> +
>        trans->readable =  s2ap & BIT(0);
> -      trans->writable = s2ap & BIT(1);
> +      trans->writable |= s2ap & BIT(1);
> 
> Although, this is odd, as S2AP[1] is supposed to be the dirty-bit.
> Maybe it's fine, as in our case, dirty implies writable.
> 
> IHMO, the best approach to this would be to make the movement to 
> DBM = writable, then have something like:
> 
> +	trans->readable =  s2ap & BIT(0);
> +	trans->dirty    =  s2ap & BIT(1);
> +	trans->writable =  ws->desc & KVM_PTE_LEAF_ATTR_HI_S2_DBM;
> 
> +	/* Dirty implies on writable, was used alone on older systems */
> +	trans->writable |= trans->dirty;
> 
> (And we assume DBM always means writable, and not only when VTCR_EL2.HD=1.

The Arm ARM is clear as mud in this area but this doesn't pass the smell
test. Look at R_HBFHL and R_FQNDX, which suggests that S2AP is the
authoritative field for write permissions through a descriptor.

> What do you think?

So the current shape of the code is a byproduct of looking at the Arm
pseudocode. Granted, AArch64.S1DirectBasePermissions() is the one
changing the effective value of AP based on the dirty state of the
descriptor.

Thanks,
Oliver