Re: [RFC PATCH 2/5] KVM: arm64: Add KVM_PGTABLE_PROT_DIRTY

[Leonardo Bras]

On Mon, Nov 09, 2026 at 10:27:36AM +0800, Tian Zheng wrote:
> 
> 
> On 9/2/2026 1:15 AM, Leonardo Bras wrote:
> > Second step of changing the encoding for the Stage2 PTE descriptor,
> > introduce the concept of dirty page, so we can have a writable but not
> > dirty (WC) page, and a writable and dirty (WD) page.
> > 
> > In order to do so, evaluate uses in a per-case basis, and figure what
> > concept was important in each case (being dirty, or writable).
> > 
> > Signed-off-by: Leonardo Bras <leo.bras@xxxxxxx>
> > ---
> >   arch/arm64/include/asm/kvm_pgtable.h |  9 ++++++---
> >   arch/arm64/kvm/hyp/pgtable.c         | 23 +++++++++++++++++------
> >   arch/arm64/kvm/mmu.c                 | 27 ++++++++++++++++-----------
> >   arch/arm64/kvm/ptdump.c              |  6 ++++++
> >   4 files changed, 45 insertions(+), 20 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/kvm_pgtable.h b/arch/arm64/include/asm/kvm_pgtable.h
> > index 37baa86d6fd8..379031c74cbc 100644
> > --- a/arch/arm64/include/asm/kvm_pgtable.h
> > +++ b/arch/arm64/include/asm/kvm_pgtable.h
> > @@ -258,45 +258,48 @@ enum kvm_pgtable_stage2_flags {
> >   	KVM_PGTABLE_S2_AS_S1			= BIT(1),
> >   };
> >   /**
> >    * enum kvm_pgtable_prot - Page-table permissions and attributes.
> >    * @KVM_PGTABLE_PROT_UX:	Unprivileged execute permission.
> >    * @KVM_PGTABLE_PROT_PX:	Privileged execute permission.
> >    * @KVM_PGTABLE_PROT_X:		Privileged and unprivileged execute permission.
> >    * @KVM_PGTABLE_PROT_W:		Write permission.
> >    * @KVM_PGTABLE_PROT_R:		Read permission.
> > + * @KVM_PGTABLE_PROT_DIRTY:	Dirty attribute.
> >    * @KVM_PGTABLE_PROT_DEVICE:	Device attributes.
> >    * @KVM_PGTABLE_PROT_NORMAL_NC:	Normal noncacheable attributes.
> >    * @KVM_PGTABLE_PROT_SW0:	Software bit 0.
> >    * @KVM_PGTABLE_PROT_SW1:	Software bit 1.
> >    * @KVM_PGTABLE_PROT_SW2:	Software bit 2.
> >    * @KVM_PGTABLE_PROT_SW3:	Software bit 3.
> >    */
> >   enum kvm_pgtable_prot {
> >   	KVM_PGTABLE_PROT_PX			= BIT(0),
> >   	KVM_PGTABLE_PROT_UX			= BIT(1),
> >   	KVM_PGTABLE_PROT_X			= KVM_PGTABLE_PROT_PX	|
> >   						  KVM_PGTABLE_PROT_UX,
> >   	KVM_PGTABLE_PROT_W			= BIT(2),
> >   	KVM_PGTABLE_PROT_R			= BIT(3),
> > +	KVM_PGTABLE_PROT_DIRTY			= BIT(4),
> > -	KVM_PGTABLE_PROT_DEVICE			= BIT(4),
> > -	KVM_PGTABLE_PROT_NORMAL_NC		= BIT(5),
> > +	KVM_PGTABLE_PROT_DEVICE			= BIT(5),
> > +	KVM_PGTABLE_PROT_NORMAL_NC		= BIT(6),
> >   	KVM_PGTABLE_PROT_SW0			= BIT(55),
> >   	KVM_PGTABLE_PROT_SW1			= BIT(56),
> >   	KVM_PGTABLE_PROT_SW2			= BIT(57),
> >   	KVM_PGTABLE_PROT_SW3			= BIT(58),
> >   };
> > -#define KVM_PGTABLE_PROT_RW	(KVM_PGTABLE_PROT_R | KVM_PGTABLE_PROT_W)
> > +#define KVM_PGTABLE_PROT_RW	(KVM_PGTABLE_PROT_R | KVM_PGTABLE_PROT_W | \
> > +				 KVM_PGTABLE_PROT_DIRTY)
> >   #define KVM_PGTABLE_PROT_RWX	(KVM_PGTABLE_PROT_RW | KVM_PGTABLE_PROT_X)
> >   #define PKVM_HOST_MEM_PROT	KVM_PGTABLE_PROT_RWX
> >   #define PKVM_HOST_MMIO_PROT	KVM_PGTABLE_PROT_RW
> >   #define PAGE_HYP		KVM_PGTABLE_PROT_RW
> >   #define PAGE_HYP_EXEC		(KVM_PGTABLE_PROT_R | KVM_PGTABLE_PROT_X)
> >   #define PAGE_HYP_RO		(KVM_PGTABLE_PROT_R)
> >   #define PAGE_HYP_DEVICE		(PAGE_HYP | KVM_PGTABLE_PROT_DEVICE)
> > diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
> > index ca49f1bd7c34..2ff33d3e371e 100644
> > --- a/arch/arm64/kvm/hyp/pgtable.c
> > +++ b/arch/arm64/kvm/hyp/pgtable.c
> > @@ -724,22 +724,26 @@ static int stage2_set_prot_attr(struct kvm_pgtable *pgt, enum kvm_pgtable_prot p
> >   		attr = KVM_S2_MEMATTR(pgt, NORMAL);
> >   	}
> >   	r = stage2_set_xn_attr(prot, &attr);
> >   	if (r)
> >   		return r;
> >   	if (prot & KVM_PGTABLE_PROT_R)
> >   		attr |= KVM_PTE_LEAF_ATTR_LO_S2_S2AP_R;
> > -	if (prot & KVM_PGTABLE_PROT_W)
> > -		attr |= KVM_PTE_LEAF_ATTR_HI_S2_DBM | KVM_PTE_LEAF_ATTR_LO_S2_S2AP_W;
> > +	if (prot & KVM_PGTABLE_PROT_W) {
> > +		attr |= KVM_PTE_LEAF_ATTR_HI_S2_DBM;
> > +
> > +		if (prot & KVM_PGTABLE_PROT_DIRTY)
> > +			attr |= KVM_PTE_LEAF_ATTR_LO_S2_S2AP_W;
> > +	}
> >   	if (!kvm_lpa2_is_enabled())
> >   		attr |= FIELD_PREP(KVM_PTE_LEAF_ATTR_LO_S2_SH, sh);
> >   	attr |= KVM_PTE_LEAF_ATTR_LO_S2_AF;
> >   	attr |= prot & KVM_PTE_LEAF_ATTR_HI_SW;
> >   	*ptep = attr;
> >   	return 0;
> > @@ -747,23 +751,27 @@ static int stage2_set_prot_attr(struct kvm_pgtable *pgt, enum kvm_pgtable_prot p
> >   enum kvm_pgtable_prot kvm_pgtable_stage2_pte_prot(kvm_pte_t pte)
> >   {
> >   	enum kvm_pgtable_prot prot = pte & KVM_PTE_LEAF_ATTR_HI_SW;
> >   	if (!kvm_pte_valid(pte))
> >   		return prot;
> >   	if (pte & KVM_PTE_LEAF_ATTR_LO_S2_S2AP_R)
> >   		prot |= KVM_PGTABLE_PROT_R;
> > -	if (pte & KVM_PTE_LEAF_ATTR_HI_S2_DBM)
> > +	if (pte & KVM_PTE_LEAF_ATTR_HI_S2_DBM) {
> >   		prot |= KVM_PGTABLE_PROT_W;
> > +		if (pte & KVM_PTE_LEAF_ATTR_LO_S2_S2AP_W)
> > +			prot |= KVM_PGTABLE_PROT_DIRTY;
> > +	}
> > +
> >   	switch (FIELD_GET(KVM_PTE_LEAF_ATTR_HI_S2_XN, pte)) {
> >   	case 0b00:
> >   		prot |= KVM_PGTABLE_PROT_PX | KVM_PGTABLE_PROT_UX;
> >   		break;
> >   	case 0b01:
> >   		prot |= KVM_PGTABLE_PROT_UX;
> >   		break;
> >   	case 0b11:
> >   		prot |= KVM_PGTABLE_PROT_PX;
> >   		break;
> > @@ -1282,21 +1290,20 @@ static int stage2_update_leaf_attrs(struct kvm_pgtable *pgt, u64 addr,
> >   		*orig_pte = data.pte;
> >   	if (level)
> >   		*level = data.level;
> >   	return 0;
> >   }
> >   int kvm_pgtable_stage2_wrprotect(struct kvm_pgtable *pgt, u64 addr, u64 size)
> >   {
> >   	return stage2_update_leaf_attrs(pgt, addr, size, 0,
> > -					KVM_PTE_LEAF_ATTR_HI_S2_DBM |
> >   					KVM_PTE_LEAF_ATTR_LO_S2_S2AP_W,
> >   					NULL, NULL,
> >   					KVM_PGTABLE_WALK_IGNORE_EAGAIN);
> >   }
> >   void kvm_pgtable_stage2_mkyoung(struct kvm_pgtable *pgt, u64 addr,
> >   				enum kvm_pgtable_walk_flags flags)
> >   {
> >   	int ret;
> > @@ -1362,22 +1369,26 @@ int kvm_pgtable_stage2_relax_perms(struct kvm_pgtable *pgt, u64 addr,
> >   	kvm_pte_t xn = 0, set = 0, clr = 0;
> >   	s8 level;
> >   	int ret;
> >   	if (prot & KVM_PTE_LEAF_ATTR_HI_SW)
> >   		return -EINVAL;
> >   	if (prot & KVM_PGTABLE_PROT_R)
> >   		set |= KVM_PTE_LEAF_ATTR_LO_S2_S2AP_R;
> > -	if (prot & KVM_PGTABLE_PROT_W)
> > -		set |= KVM_PTE_LEAF_ATTR_HI_S2_DBM | KVM_PTE_LEAF_ATTR_LO_S2_S2AP_W;
> > +	if (prot & KVM_PGTABLE_PROT_W) {
> > +		set |= KVM_PTE_LEAF_ATTR_HI_S2_DBM;
> > +
> > +		if (prot & KVM_PGTABLE_PROT_DIRTY)
> > +			set |= KVM_PTE_LEAF_ATTR_LO_S2_S2AP_W;
> > +	}
> > 
> >   	if (prot & KVM_PGTABLE_PROT_X) {
> >   		ret = stage2_set_xn_attr(prot, &xn);
> >   		if (ret)
> >   			return ret;
> >   		set |= xn & KVM_PTE_LEAF_ATTR_HI_S2_XN;
> >   		clr |= ~xn & KVM_PTE_LEAF_ATTR_HI_S2_XN;
> >   	}
> > diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> > index 9ba86450fe4a..9d4f70430bbe 100644
> > --- a/arch/arm64/kvm/mmu.c
> > +++ b/arch/arm64/kvm/mmu.c
> > @@ -1205,21 +1205,23 @@ int topup_hyp_memcache(struct kvm_hyp_memcache *mc, unsigned long min_pages)
> >   int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa,
> >   			  phys_addr_t pa, unsigned long size, bool writable)
> >   {
> >   	phys_addr_t addr;
> >   	int ret = 0;
> >   	struct kvm_mmu_memory_cache cache = { .gfp_zero = __GFP_ZERO };
> >   	struct kvm_s2_mmu *mmu = &kvm->arch.mmu;
> >   	struct kvm_pgtable *pgt = mmu->pgt;
> >   	enum kvm_pgtable_prot prot = KVM_PGTABLE_PROT_DEVICE |
> >   				     KVM_PGTABLE_PROT_R |
> > -				     (writable ? KVM_PGTABLE_PROT_W : 0);
> > +				     (writable ?
> > +				      (KVM_PGTABLE_PROT_W | KVM_PGTABLE_PROT_DIRTY) :
> > +				      0);
> >   	if (is_protected_kvm_enabled())
> >   		return -EPERM;
> >   	size += offset_in_page(guest_ipa);
> >   	guest_ipa &= PAGE_MASK;
> >   	for (addr = guest_ipa; addr < guest_ipa + size; addr += PAGE_SIZE) {
> >   		ret = kvm_mmu_topup_memory_cache(&cache,
> >   						 kvm_mmu_cache_min_pages(mmu));
> > @@ -1571,21 +1573,21 @@ static int topup_mmu_memcache(struct kvm_vcpu *vcpu, void *memcache)
> >    *
> >    * Also encode the level of the original translation in the SW bits of the leaf
> >    * entry as a proxy for the span of that translation. This will be retrieved on
> >    * TLB invalidation from the guest and used to limit the invalidation scope if a
> >    * TTL hint or a range isn't provided.
> >    */
> >   static enum kvm_pgtable_prot adjust_nested_fault_perms(struct kvm_s2_trans *nested,
> >   						       enum kvm_pgtable_prot prot)
> >   {
> >   	if (!kvm_s2_trans_writable(nested))
> > -		prot &= ~KVM_PGTABLE_PROT_W;
> > +		prot &= ~(KVM_PGTABLE_PROT_W | KVM_PGTABLE_PROT_DIRTY);
> >   	if (!kvm_s2_trans_readable(nested))
> >   		prot &= ~KVM_PGTABLE_PROT_R;
> >   	return prot | kvm_encode_nested_level(nested);
> >   }
> >   static enum kvm_pgtable_prot adjust_nested_exec_perms(struct kvm *kvm,
> >   						      struct kvm_s2_trans *nested,
> >   						      enum kvm_pgtable_prot prot)
> >   {
> > @@ -1642,21 +1644,21 @@ static int gmem_abort(const struct kvm_s2_fault_desc *s2fd)
> >   	smp_rmb();
> >   	ret = kvm_gmem_get_pfn(kvm, s2fd->memslot, gfn, &pfn, &page, NULL);
> >   	if (ret) {
> >   		kvm_prepare_memory_fault_exit(s2fd->vcpu, s2fd->fault_ipa, PAGE_SIZE,
> >   					      write_fault, exec_fault, false);
> >   		return ret;
> >   	}
> >   	if (!(s2fd->memslot->flags & KVM_MEM_READONLY))
> > -		prot |= KVM_PGTABLE_PROT_W;
> > +		prot |= KVM_PGTABLE_PROT_W | KVM_PGTABLE_PROT_DIRTY;
> >   	if (s2fd->nested)
> >   		prot = adjust_nested_fault_perms(s2fd->nested, prot);
> >   	if (exec_fault || cpus_have_final_cap(ARM64_HAS_CACHE_DIC))
> >   		prot |= KVM_PGTABLE_PROT_X;
> >   	if (s2fd->nested)
> >   		prot = adjust_nested_exec_perms(kvm, s2fd->nested, prot);
> > @@ -1674,24 +1676,24 @@ static int gmem_abort(const struct kvm_s2_fault_desc *s2fd)
> >   		prot &= ~KVM_NV_GUEST_MAP_SZ;
> >   		ret = KVM_PGT_FN(kvm_pgtable_stage2_relax_perms)(pgt, s2fd->fault_ipa,
> >   								 prot, flags);
> >   	} else {
> >   		ret = KVM_PGT_FN(kvm_pgtable_stage2_map)(pgt, s2fd->fault_ipa, PAGE_SIZE,
> >   							 __pfn_to_phys(pfn), prot,
> >   							 memcache, flags);
> >   	}
> >   out_unlock:
> > -	kvm_release_faultin_page(kvm, page, !!ret, prot & KVM_PGTABLE_PROT_W);
> > +	kvm_release_faultin_page(kvm, page, !!ret, prot & KVM_PGTABLE_PROT_DIRTY);
> >   	kvm_fault_unlock(kvm);
> > -	if ((prot & KVM_PGTABLE_PROT_W) && !ret)
> > +	if ((prot & KVM_PGTABLE_PROT_DIRTY) && !ret)
> >   		mark_page_dirty_in_slot(kvm, s2fd->memslot, gfn);
> >   	return ret != -EAGAIN ? ret : 0;
> >   }
> >   struct kvm_s2_fault_vma_info {
> >   	unsigned long	mmu_seq;
> >   	long		vma_pagesize;
> >   	vm_flags_t	vm_flags;
> >   	unsigned long	max_map_size;
> > @@ -1977,25 +1979,28 @@ static int kvm_s2_fault_compute_prot(const struct kvm_s2_fault_desc *s2fd,
> >   	 * and trigger the exception here. Since the memslot is valid, inject
> >   	 * the fault back to the guest.
> >   	 */
> >   	if (esr_fsc_is_excl_atomic_fault(kvm_vcpu_get_esr(s2fd->vcpu))) {
> >   		kvm_inject_dabt_excl_atomic(s2fd->vcpu, kvm_vcpu_get_hfar(s2fd->vcpu));
> >   		return 1;
> >   	}
> >   	*prot = KVM_PGTABLE_PROT_R;
> > -	if (s2vi->map_writable && (s2vi->device ||
> > -				   !memslot_is_logging(s2fd->memslot) ||
> > -				   kvm_is_write_fault(s2fd->vcpu)))
> > +	if (s2vi->map_writable) {
> >   		*prot |= KVM_PGTABLE_PROT_W;
> > +		if (s2vi->device || !memslot_is_logging(s2fd->memslot) ||
> > +		    kvm_is_write_fault(s2fd->vcpu))
> > +			*prot |= KVM_PGTABLE_PROT_DIRTY;
> > +	}
> > +
> 
> 
> Hi Leo,
> 
> After reviewing this patch again, I found an issue with the current
> scheme: for a read fault in a logging memslot, we deliberately install
> the page writable-clean so hardware can promote it later. That breaks
> MAP_PRIVATE (COW) memory, which requires a write fault -> FOLL_WRITE ->
> host COW copy, so a read fault must map RO. Otherwise the guest's later
> write no longer faults and COW is skipped.
> 
> In our sandbox tests, the re-fault storm caused concurrent_resume to
> trigger about 1.57x more faults (784K vs 500K) and about 8x longer wall
> time (~688ms vs 85ms).
> 
> To fix it, we should restore the writable gate:
> 
> ```c
> if (s2vi->map_writable && (s2vi->device ||
>                            !memslot_is_logging(s2fd->memslot) ||
>                            kvm_is_write_fault(s2fd->vcpu))) {
> 	*prot |= KVM_PGTABLE_PROT_W;
> 
> 	if (... keep DIRTY logic ...)
> 		*prot |= KVM_PGTABLE_PROT_DIRTY;
> }
> ```
> 
> Read faults in a logging memslot stay RO; only the first write fault
> installs writable, so COW breaks normally.
> 
> What do you think?

The page has to be !dirty at S2 on a read at dirty_tracking, so we don't 
miss a write. That being said,we can have a read-only (or WC) S2 mapping on 
a writable host page.

So on this scenario of marking a page WC on a read fault while 
dirty-tracking, we can just mark the host page writable.

What do you think?

(Also, keep in mind a lot of this will change with the next versions of the 
PTE encoding patchset, maybe we should worry on refactoring that later)


Thanks!
Leo