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

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

On Mon, Jun 29, 2026 at 11:39:37AM +0000, sashiko-bot@kernel.org wrote:
> 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.
> 
> [ ... ]

Sure, can be changed.

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

It can not.
offset+bits_per_long has to be bigger than split_end. Since split_end is 
the last bit of a level-2 block, it starts splitting again only if the 
address > split_end. 

> > +			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:
> 

Yes, they are used as loop bounds.
No they are not re-declared inside a loop, so it should be fine.

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

Okay, because this end is non-inclusive in kvm_mmu_split_huge_pages, but 
for split_end it will cause to use the next block of address.

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

I need to fix  this in v3.


> 
> > +		}
> >  
> >  		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