Re: [PATCH v3 01/11] KVM: arm64: HDBSS bits

On Wed, Jul 29, 2026 at 10:55:40AM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
> - [High] The patch is explicitly marked by the author as a test hack that should not be merged upstream.
> - [High] Hardware Dirty Bit Management (DBM) is not cleared when write-protecting pages, allowing guests to bypass software write-protection (such as userfaultfd-wp or software dirty logging).
> - [Medium] Left shifting a negative signed integer is undefined behavior in C.
> - [Medium] The hardcoded split_page_chunk_size is excessively large, causing massive memory allocations under the mmu_lock.
> - [Low] HDBSS entry macros are defined in a `.c` file instead of a header file, making them inaccessible to other subsystems.
> - [Low] Typo in the parameter name of `has_vhe_hdbss`.
> --
> 
> --- Patch [1]: [PATCH v3 01/11] KVM: arm64: HDBSS bits ---
> commit 125f04d66539bf05e0255567036409a407e48004
> Author: Leonardo Bras <[email protected]>
> Subject: KVM: arm64: HDBSS bits
> 
> A brief summary:
> This commit introduces preliminary support for the Hardware Dirty state
> tracking structure (HDBSS) feature on ARM64. It adds CPU feature detection,
> sets the DBM stage-2 page table flag when write protection is relaxed, and
> configures eager page splitting chunk sizes for testing purposes.
> 
> > I added them here in order to fulfill the dependencies and be able to
> > easily build and test this patchset, but this particular patch should *not*
> > be merged upstream.
> 
> [Severity: High]
> Since this is explicitly marked as a test hack meant only for local building
> and testing, should this patch be dropped from the series before it gets
> merged upstream?
> 
> [ ... ]
> 
> > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> > --- a/arch/arm64/kernel/cpufeature.c
> > +++ b/arch/arm64/kernel/cpufeature.c
> > @@ -2131,6 +2131,11 @@ static bool hvhe_possible(const struct arm64_cpu_capabilities *entry,
> >  	return arm64_test_sw_feature_override(ARM64_SW_FEATURE_OVERRIDE_HVHE);
> >  }
> >  
> > +static bool has_vhe_hdbss(const struct arm64_cpu_capabilities *entry, int cope)
>                                                                              ^^^^
> [Severity: Low]
> Is this intended to be named scope? The standard convention for capability
> matching functions in cpufeature.c uses scope for this parameter.
> 
> [ ... ]
> 
> > diff --git a/arch/arm64/kvm/dirty_bit.c b/arch/arm64/kvm/dirty_bit.c
> > new file mode 100644
> > --- /dev/null
> > +++ b/arch/arm64/kvm/dirty_bit.c
> > @@ -0,0 +1,16 @@
> [ ... ]
> > +#include <asm/kvm_dirty_bit.h>
> > +
> > +/* HDBSS entry field definitions */
> > +#define HDBSS_ENTRY_VALID BIT(0)
> > +#define HDBSS_ENTRY_TTWL_SHIFT (1)
> > +#define HDBSS_ENTRY_TTWL_MASK (GENMASK(3, 1))
> > +#define HDBSS_ENTRY_TTWL(x) \
> > +	(((x) << HDBSS_ENTRY_TTWL_SHIFT) & HDBSS_ENTRY_TTWL_MASK)
> > +#define HDBSS_ENTRY_TTWL_RESV HDBSS_ENTRY_TTWL(-4)
> 
> [Severity: Low]
> Because these HDBSS entry macros are defined directly in the .c file rather
> than in the newly created header arch/arm64/include/asm/kvm_dirty_bit.h, they
> are inaccessible to other subsystems. Should they be moved to the header file
> so dependent patches can use them?
> 
> [Severity: Medium]
> Will HDBSS_ENTRY_TTWL_RESV trigger undefined behavior here? The macro expands
> to ((-4) << 1), and left shifting a negative signed integer is undefined
> behavior in C (prior to C23), which can lead to shift-out-of-bounds splats
> at runtime.
> 
> > +#define HDBSS_ENTRY_IPA GENMASK_ULL(55, 12)
> 
> [ ... ]
> 
> > diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
> > --- a/arch/arm64/kvm/hyp/pgtable.c
> > +++ b/arch/arm64/kvm/hyp/pgtable.c
> > @@ -1367,9 +1371,13 @@ int kvm_pgtable_stage2_relax_perms(struct kvm_pgtable *pgt, u64 addr,
> >  	if (prot & KVM_PGTABLE_PROT_R)
> >  		set |= KVM_PTE_LEAF_ATTR_LO_S2_S2AP_R;
> >  
> > -	if (prot & KVM_PGTABLE_PROT_W)
> > +	if (prot & KVM_PGTABLE_PROT_W) {
> >  		set |= KVM_PTE_LEAF_ATTR_LO_S2_S2AP_W;
> >  
> > +		if (pgt->flags & KVM_PGTABLE_S2_DBM)
> > +			set |= KVM_PTE_LEAF_ATTR_HI_S2_DBM;
> > +	}
> > +
> 
> [Severity: High]
> Does kvm_pgtable_stage2_wrprotect() also need to be updated to clear the DBM
> bit? 
> 
> Currently, kvm_pgtable_stage2_wrprotect() clears the 
> KVM_PTE_LEAF_ATTR_LO_S2_S2AP_W bit to trap writes, but if 
> KVM_PTE_LEAF_ATTR_HI_S2_DBM is left set, the hardware will autonomously update 
> the PTE to allow writes without generating a permission fault. Could this 
> allow guests to bypass software write-protection mechanisms like 
> userfaultfd-wp or KVM software dirty page logging?
> 
> >  	if (prot & KVM_PGTABLE_PROT_X) {
> >  		ret = stage2_set_xn_attr(prot, &xn);
> >  		if (ret)
> 
> [ ... ]
> 
> > diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> > --- a/arch/arm64/kvm/mmu.c
> > +++ b/arch/arm64/kvm/mmu.c
> > @@ -1027,8 +1027,12 @@ int kvm_init_stage2_mmu(struct kvm *kvm, struct kvm_s2_mmu *mmu, unsigned long t
> >  	for_each_possible_cpu(cpu)
> >  		*per_cpu_ptr(mmu->last_vcpu_ran, cpu) = -1;
> >  
> > -	 /* The eager page splitting is disabled by default */
> > -	mmu->split_page_chunk_size = KVM_ARM_EAGER_SPLIT_CHUNK_SIZE_DEFAULT;
> > +	 /* The eager page splitting is disabled by default if system has no HDBSS */
> > +	if (system_supports_hdbss())
> > +		mmu->split_page_chunk_size = 256 * 1024 * PAGE_SIZE;
> 
> [Severity: Medium]
> Is this hardcoded split_page_chunk_size excessively large? This evaluates to
> 1GB on 4KB page systems and 16GB on 64KB page systems. When splitting this
> chunk, kvm_mmu_topup_memory_cache might attempt to allocate a massive amount
> of page tables under the mmu_lock. Could this cause severe latency spikes or
> allocation failures under memory pressure?
> 
> > +	else
> > +		mmu->split_page_chunk_size = KVM_ARM_EAGER_SPLIT_CHUNK_SIZE_DEFAULT;
> > +
> >  	mmu->split_page_cache.gfp_zero = __GFP_ZERO;
> >  
> >  	mmu->pgd_phys = __pa(pgt->pgd);
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1

As previously mentioned, this first patch is just for placing basic 
infrastructure for testing the speedup caused by HACDBS. Of course there 
are a lot of incomplete stuff around, because this does not replace the 
actual patchset.

Thanks
Leo