Re: [PATCH] RISC-V: KVM: Improve dirty log clearing by skipping zero bits in mask

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] This problem wasn't introduced by this patch, but the non-atomic read-modify-write in `kvm_riscv_gstage_op_pte()` for `GSTAGE_OP_WP` overwrites and loses concurrent hardware A/D (Accessed/Dirty) bit updates. When write-protecting a page, it reads the PTE, clears `_PAGE_WRITE`, and writes it back using `set_pte()`. If the hardware page-table walker concurrently sets the Dirty or Accessed bit during this window, the update is lost. Atomic single-instruction operations (e.g., atomic AND or `cmpxchg`) must be used for updating hardware-shared PTEs.
--

--- Patch [1]: [PATCH] RISC-V: KVM: Improve dirty log clearing by skipping zero bits in mask ---
commit 69dff78a641691bca22a783a5f60188157d6a975
Author: Wang Yechao <wang.yechao255@zte.com.cn>

RISC-V: KVM: Improve dirty log clearing by skipping zero bits in mask

This commit optimizes the KVM dirty log clearing process on RISC-V by
iterating only over the set bits in the dirty mask instead of scanning
the entire range. It uses a new function to process leaf PTEs and entire
huge pages, reducing unnecessary page table walks for sparse masks.

> diff --git a/arch/riscv/kvm/gstage.c b/arch/riscv/kvm/gstage.c
> index c4c3b79567f10..9af86b1c5e3c3 100644
> --- a/arch/riscv/kvm/gstage.c
> +++ b/arch/riscv/kvm/gstage.c

[ ... ]

> @@ -472,6 +472,65 @@ bool kvm_riscv_gstage_wp_range(struct kvm_gstage *gstage, gpa_t start, gpa_t end

[ ... ]

> +bool kvm_riscv_gstage_wp_pt_masked(struct kvm_gstage *gstage, gfn_t base_gfn,
> +				   unsigned long mask)
> +{

[ ... ]

> +		if (found_leaf) {
> +			if (ptep_level) {
> +				addr = ALIGN_DOWN(addr, page_size);
> +				clear_huge_mask(&mask, page_size, base_gfn, addr);
> +			}
> +
> +			flush |= kvm_riscv_gstage_op_pte(gstage, addr, ptep,
> +							 ptep_level, GSTAGE_OP_WP);

[Severity: Critical]
This is a pre-existing issue, but does the invocation of
kvm_riscv_gstage_op_pte() with GSTAGE_OP_WP race with hardware A/D bit
updates?

Looking at kvm_riscv_gstage_op_pte():

	else if (op == GSTAGE_OP_WP)
		set_pte(ptep, __pte(pte_val(ptep_get(ptep)) & ~_PAGE_WRITE));

This performs a non-atomic read-modify-write on the PTE. If the hardware
page-table walker concurrently sets the Dirty or Accessed bit during this
window, would the update be overwritten and lost?

When write-protecting a page for dirty log clearing, losing the
hardware-updated dirty bit could cause KVM to believe a dirtied page is
clean. During VM live migration, could this lead to the page not being
re-transferred, resulting in data corruption in the migrated VM?

Should hardware-shared SPTE updates use atomic single-instruction
operations (like atomic AND or cmpxchg) instead to preserve concurrent
hardware writes?

> +
> +			if (ptep_level)
> +				continue;
> +		}
> +
> +		/* clear the first set bit*/
> +		mask &= mask - 1;
> +	}
> +
> +	return flush;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717080654.877151-1-wang.yechao255@zte.com.cn?part=1