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

[Leonardo Bras <leo.bras@arm.com>]

On Wed, Sep 23, 2026 at 10:16:06AM -0700, Oliver Upton wrote:
> 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.

Oh, right, the pseudocode.
Yeah, it looks very much like your code, even the use of DBM+HD to reset 
AP[2]. But that's S1, right?

Stage2 has a different form [*], yet carries the same idea:
writable = S2AP[1] || (DBM && HD && (!HDBSS || HDBSS_NOT_FULL()));

Most interestingly, in that order.
So DBM is not a writable bit, in fact. :(
It's just a bit that, with HD=1, can change the permission of a page 
to writable.

Right, that fits better with the whole HAFDBS feature as well.

Reading the code again, the only difference with the pseudocode is that you 
evaluate DBM & HD before even checking S2AP[1], but that is not relevant as 
the end result is the same.

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

Thanks for your patience!
Leo


[*] https://support.arm.com/documentation/ddi0487/mc/-Part-J-Architectural-Pseudocode/-Chapter-J1-A-profile-Architecture-Pseudocode/-J1-2-Pseudocode-for-AArch64-operation/-J1-2-676-AArch64-S2DirectBasePermissions?lang=en