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

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

On Tue, Jun 23, 2026 at 11:41:43AM -0700, Oliver Upton wrote:
> KVM will soon add support for hardware dirty state management for nested
> guests. In order to emulate dirty state transitions on the guest
> desciptor, KVM will need to use read-only hardware mappings and re-walk
> the guest page tables upon taking a permission fault.
> 
> Prepare by limiting shadow stage-2 and shadow VNCR translations to
> read-only for writable-clean guest translations.
> 
> Signed-off-by: Oliver Upton <oupton@kernel.org>
> ---
>  arch/arm64/include/asm/kvm_nested.h | 2 ++
>  arch/arm64/kvm/at.c                 | 1 +
>  arch/arm64/kvm/mmu.c                | 2 +-
>  arch/arm64/kvm/nested.c             | 4 +++-
>  4 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_nested.h b/arch/arm64/include/asm/kvm_nested.h
> index 3b36ed7c7608..7fe6fb56c187 100644
> --- a/arch/arm64/include/asm/kvm_nested.h
> +++ b/arch/arm64/include/asm/kvm_nested.h
> @@ -94,6 +94,7 @@ struct kvm_s2_trans {
>  	u32		esr;
>  	bool		writable;
>  	bool		readable;
> +	bool		dirty;
>  	bool		px;
>  	bool		ux;
>  };
> @@ -323,6 +324,7 @@ struct s1_walk_result {
>  			bool	pr;
>  			bool	pw;
>  			bool	px;
> +			bool	dirty;
>  		};
>  		struct {
>  			u8	fst;
> diff --git a/arch/arm64/kvm/at.c b/arch/arm64/kvm/at.c
> index 86b499e7a9a0..7a84495a2e6d 100644
> --- a/arch/arm64/kvm/at.c
> +++ b/arch/arm64/kvm/at.c
> @@ -1317,6 +1317,7 @@ static void compute_s1_permissions(struct kvm_vcpu *vcpu,
>  			  (pan3_enabled(vcpu, wi->regime) && wr->ux));
>  	wr->pw &= !pan;
>  	wr->pr &= !pan;
> +	wr->dirty = !(wr->desc & BIT(7));
>  }
>  
>  static int handle_at_slow(struct kvm_vcpu *vcpu, u32 op, u64 vaddr, u64 *par)
> 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? 

>  	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.

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?


>  
>  /*
> @@ -1603,7 +1605,7 @@ static void kvm_map_l1_vncr(struct kvm_vcpu *vcpu)
>  
>  	vt->cpu = smp_processor_id();
>  
> -	if (vt->hpa_writable && vt->wr.pw && vt->wr.pr)
> +	if (vt->hpa_writable && vt->wr.pw && vt->wr.dirty && vt->wr.pr)
>  		prot = PAGE_KERNEL;
>  	else if (vt->wr.pr)
>  		prot = PAGE_KERNEL_RO;
> -- 
> 2.47.3
> 

Thanks!
Leo