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

[sashiko-bot@kernel.org]

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 <leo.bras@arm.com>
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/20260729104548.3439958-1-leo.bras@arm.com?part=1