Re: [PATCH v2 09/13] KVM: arm64: Dirty-bitmap: avoid splitting previously split blocks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Incorrect PMD boundary calculation skips huge page splitting for adjacent PMDs and causes redundant splitting for the last chunk of each PMD.
- [Low] Variable `split_end` is declared as a Guest Physical Address (`gpa_t`) but assigned and compared as a Guest Frame Number (`gfn_t`).
- [Low] The local variables `start` and `end` shadow the outer loop bounds.
--

commit 8863fe6c6c67ca7006255c8805499ac99f887014
Author: Leonardo Bras <leo.bras@arm.com>

KVM: arm64: Dirty-bitmap: avoid splitting previously split blocks

If previous dirty-clean already split a block, then avoid calling the
split helper on that block again.

> diff --git a/arch/arm64/kvm/dirty_bit.c b/arch/arm64/kvm/dirty_bit.c
> --- a/arch/arm64/kvm/dirty_bit.c
> +++ b/arch/arm64/kvm/dirty_bit.c
> @@ -160,6 +160,7 @@ int __kvm_arch_dirty_log_clear(struct kvm *kvm,
>  	u64 ttwl;
>  	u64 start, end;
>  	gfn_t base_gfn;
> +	gpa_t split_end = 0;

[Severity: Low]
This isn't a bug, but should split_end be declared as a gfn_t?

It is declared as a Guest Physical Address (gpa_t) here, but later receives
a Guest Frame Number (gfn_t) from gpa_to_gfn(). While both compile as u64,
this mixes address types.

[ ... ]

> @@ -204,10 +205,14 @@ int __kvm_arch_dirty_log_clear(struct kvm *kvm,
>  
>  		offset = base_gfn + i * BITS_PER_LONG;
>  
> -		if (kvm_dirty_log_manual_protect_and_init_set(kvm))
> -			kvm_mmu_split_huge_pages(kvm,
> -						 gfn_to_gpa(offset + __ffs(mask)),
> -						 gfn_to_gpa(offset + __fls(mask) + 1));
> +		if (kvm_dirty_log_manual_protect_and_init_set(kvm) &&
> +		    (offset + BITS_PER_LONG > split_end)) {

[Severity: High]
Does this comparison check an exclusive GFN against an inclusive GFN?

The expression offset + BITS_PER_LONG calculates an exclusive boundary,
while split_end is calculated below as an inclusive boundary:

    split_end = gpa_to_gfn(ALIGN_DOWN(end, PMD_SIZE) + PMD_SIZE - 1);

Comparing these might cause a redundant split for the last 64-page batch
of an already-split PMD.

> +			gpa_t start = gfn_to_gpa(offset + __ffs(mask));
> +			gpa_t end = gfn_to_gpa(offset + __fls(mask) + 1);

[Severity: Low]
This isn't a bug, but do these variables shadow the outer loop bounds?

The variables start and end declared here share names with the loop bounds:

    u64 start, end;
    ...
    for (unsigned long i = start; i < end; i++) {

While scoping rules use the correct variables, shadowing loop control
variables might make the code prone to subtle bugs during future refactoring.

> +
> +			kvm_mmu_split_huge_pages(kvm, start, end);
> +			split_end = gpa_to_gfn(ALIGN_DOWN(end, PMD_SIZE) + PMD_SIZE - 1);

[Severity: High]
Can this alignment skip splitting for the next PMD?

The variable end is an exclusive physical address:

    gpa_t end = gfn_to_gpa(offset + __fls(mask) + 1);

If a block of dirty pages ends exactly at a PMD boundary, ALIGN_DOWN() on
the exclusive end address aligns it to that boundary. Adding PMD_SIZE - 1
then advances split_end to the end of the next PMD.

This would skip huge page splitting for the adjacent PMD, breaking dirty
logging and potentially causing data corruption during live migration as
dirty pages would not be tracked at the PTE level.

> +		}
>  
>  		do {
>  			idx = mask_to_hdbss(&mask, hw_entries, offset, ttwl, idx, entries_sz);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260629111820.1873540-1-leo.bras@arm.com?part=9