Re: [PATCH 04/22] KVM: arm64: nv: Only shadow writable-dirty guest descs as writable

[Oliver Upton <oupton@kernel.org>]

On Mon, Sep 21, 2026 at 05:51:20PM +0100, Leonardo Bras wrote:
> > diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> > index 07bd1e3ae9fb..f35c4ce95473 100644
> > --- a/arch/arm64/kvm/mmu.c
> > +++ b/arch/arm64/kvm/mmu.c
> > @@ -1572,7 +1572,7 @@ static int topup_mmu_memcache(struct kvm_vcpu *vcpu, void *memcache)
> >  static enum kvm_pgtable_prot adjust_nested_fault_perms(struct kvm_s2_trans *nested,
> >  						       enum kvm_pgtable_prot prot)
> >  {
> > -	if (!nested->writable)
> > +	if (!(nested->writable && nested->dirty))
> >  		prot &= ~KVM_PGTABLE_PROT_W;
> 
> So if the page gets prot_w if it's either writable or dirty, or both.
> Humm, since we should _not_ have a dirty page that is not writable, that 
> could be fine. But then, why test the dirty?
> 
> Maybe we should have a KVM_PGTABLE_PROT_D (dirty) as well? 

The reasoning here requires zooming out and looking at the full flow of
a nested stage-2 abort.

Suppose an L2 takes a translation fault for an address that is missing
from the shadow stage-2. L0 KVM walks the L1 page tables and arrives at
a writable-clean descriptor for the fault address. That descriptor gets
shadowed into L0 KVM's shadow stage-2 MMU. Since L0 KVM relies on taking
a permission fault to set the dirty state in the L1 page tables, we can
only install a read-only translation in the shadow stage-2.

Prematurely granting write access would be architecturally incorrect
since the L1 descriptor would remain in a writable-clean state. In the
same vein, setting the page with DBM=1 in the shadow stage-2 would be
incorrect since hardware will relax it to writable-dirty without an
intervening fault that can be used to fix the L1 descriptor.

> >  	if (!nested->readable)
> >  		prot &= ~KVM_PGTABLE_PROT_R;
> > diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
> > index b247bc1d83fa..dcc7d0cc7c95 100644
> > --- a/arch/arm64/kvm/nested.c
> > +++ b/arch/arm64/kvm/nested.c
> > @@ -269,6 +269,8 @@ static void compute_s2_permissions(struct kvm_vcpu *vcpu, struct s2_walk_info *w
> >  
> >  	trans->readable = s2ap & BIT(0);
> >  	trans->writable = s2ap & BIT(1);
> > +
> > +	trans->dirty = ws->desc & BIT(7);
> >  }
> 
> IIUC, both writable and dirty here will always have the same value, as they 
> both are set based in the same bit in ws->desc.
> 
> Maybe this is intended, but then it's a bit confusing they are not 
> both using the s2ap variable.

This is intentional, although this series gives an incomplete picture.

I am separately tracking the dirty state of the descriptor in
anticipation of FEAT_S2PIE support for nested. It just so happens that
in the direct permission model the 'write' and 'dirty' bits are aliased
to the same single bit.

> Another point here is that the actual writable bit should be DBM, at least 
> in some scenarios, but this discussion can be contended in the New PTE 
> patchset. 
> 
> Also, nits: 
> - any reason for the newline between writable and dirty?
> - should not KVM_PTE_LEAF_ATTR_LO_S2_S2AP_W be used instead of BIT[7] in 
> this patch, as it makes more clear what we are checking?

Same thing here: these are both intentional changes in preparation for
supporting direct and indirect permission models at stage-2.

Thanks,
Oliver