Re: [PATCH v4 4/6] KVM: arm64: Add HDBSS per-vCPU buffer management

[Tian Zheng <zhengtian10@huawei.com>]

On 7/9/2026 7:15 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
> - [High] Broken error handling in `kvm_arch_vcpu_create` leads to success masking and hypervisor memory leaks.
> - [High] The hardware producer index `SYS_HDBSSPROD_EL2` is lost during vCPU context switch.
> - [High] `__load_hdbss()` risks a fatal UNDEFINED exception on hardware lacking HDBSS.
> - [High] Misinterpretation of the `order` parameter causes memory waste or buffer undersizing on non-4KB page granules.
> - [Medium] Missing context synchronization event (ISB) after control-plane system register write.
> - [Low] Memory leak when an allocation resolves to physical address 0.
> --
> 
> --- Patch [4]: [PATCH v4 4/6] KVM: arm64: Add HDBSS per-vCPU buffer management ---
> commit 1967caa637e339cacc49eecff93c9e0e02ecad9e
> Author: eillon <yezhenyu2@huawei.com>
> 
> KVM: arm64: Add HDBSS per-vCPU buffer management
> 
> This commit introduces the infrastructure for HDBSS (Hardware Dirty Bit State
> Structure) per-vCPU buffer management, including allocation, freeing, and
> loading of HDBSS registers during vCPU load.
> 
>> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
>> --- a/arch/arm64/kvm/arm.c
>> +++ b/arch/arm64/kvm/arm.c
>> @@ -565,6 +566,12 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
>>   	err = kvm_share_hyp(vcpu, vcpu + 1);
>>   	if (err)
>>   		kvm_vgic_vcpu_destroy(vcpu);
>>   
>> +	if (vcpu->kvm->arch.enable_hdbss) {
>> +		err = kvm_arm_vcpu_alloc_hdbss(vcpu, vcpu->kvm->arch.hdbss_order);
> 
> [Severity: High]
> Does this code mask kvm_share_hyp() failures? If kvm_share_hyp() fails, the
> code falls through. If HDBSS is enabled and allocation succeeds, err is
> overwritten to 0.
> 
> Additionally, if kvm_share_hyp() succeeds but kvm_arm_vcpu_alloc_hdbss() fails,
> is the hypervisor mapping leaked since kvm_unshare_hyp() isn't called before
> returning?
> 
>> +		if (err)
>> +			kvm_vgic_vcpu_destroy(vcpu);
>> +	}
>> +
>>   	return err;
>>   }
> 
>> diff --git a/arch/arm64/kvm/dirty_bit.c b/arch/arm64/kvm/dirty_bit.c
>> --- /dev/null
>> +++ b/arch/arm64/kvm/dirty_bit.c
> [ ... ]
>> +int kvm_arm_vcpu_alloc_hdbss(struct kvm_vcpu *vcpu, unsigned int order)
>> +{
>> +	struct page *hdbss_pg = NULL;
>> +
>> +	if (vcpu->arch.hdbss.hdbssbr_el2 || !system_supports_hdbss())
>> +		return 0;
>> +
>> +	if (order > HDBSS_MAX_ORDER)
>> +		return -EINVAL;
>> +
>> +	hdbss_pg = alloc_pages(GFP_KERNEL_ACCOUNT, order);
>> +	if (!hdbss_pg)
>> +		return -ENOMEM;
>> +
>> +	vcpu->arch.hdbss = (struct vcpu_hdbss_state) {
>> +		.base_phys = page_to_phys(hdbss_pg),
>> +		.hdbssbr_el2 = HDBSSBR_EL2(page_to_phys(hdbss_pg), order),
> 

Addressed in v5, The error path now returns immediately on
kvm_share_hyp() failure, and HDBSS allocation failure calls
kvm_unshare_hyp():

```
err = kvm_share_hyp(vcpu, vcpu + 1);
if (err) {
         kvm_vgic_vcpu_destroy(vcpu);
         return err;
}

err = kvm_arm_vcpu_alloc_hdbss(vcpu);
if (err) {
         kvm_unshare_hyp(vcpu, vcpu + 1);
         kvm_vgic_vcpu_destroy(vcpu);
         return err;
}
```

> [Severity: High]
> Is the order parameter interpreted differently depending on the page granule
> size? The order parameter is used both for alloc_pages() (which scales by
> PAGE_SIZE) and HDBSSBR_EL2() (which expects an absolute hardware size).
> 
> On 64KB pages, supplying order=9 for a 2MB buffer would allocate 32MB from the
> buddy allocator, while order=5 would configure the hardware for 128KB.
> 
>> +		.hdbssprod_el2 = 0,
>> +	};
>> +
>> +	return 0;
>> +}
>> +
>> +void kvm_arm_vcpu_free_hdbss(struct kvm_vcpu *vcpu)
>> +{
>> +	struct page *hdbss_pg;
>> +
>> +	if (!vcpu->arch.hdbss.hdbssbr_el2) {
>> +		return;
>> +	}
> 

Fixed in v5. The order parameter is removed. Buffer size is tracked in
bytes, with separate conversions for the buddy allocator (get_order) and
hardware SZ encoding (ilog2(size) - 12).

> [Severity: Low]
> Could this leak the buffer if the buddy allocator returns physical address 0
> and order is 0? The resulting HDBSSBR_EL2 value would be 0, causing this
> early return to intercept a valid allocation.
> 
>> diff --git a/arch/arm64/kvm/hyp/vhe/switch.c b/arch/arm64/kvm/hyp/vhe/switch.c
>> --- a/arch/arm64/kvm/hyp/vhe/switch.c
>> +++ b/arch/arm64/kvm/hyp/vhe/switch.c
> [ ... ]
>> @@ -213,6 +214,19 @@ static void __vcpu_put_deactivate_traps(struct kvm_vcpu *vcpu)
>>   	local_irq_restore(flags);
>>   }
>>   
>> +static void __load_hdbss(struct kvm_vcpu *vcpu)
>> +{
>> +	struct kvm *kvm = vcpu->kvm;
>> +
>> +	if (!kvm->arch.enable_hdbss)
>> +		return;
>> +
>> +	write_sysreg_s(vcpu->arch.hdbss.hdbssbr_el2, SYS_HDBSSBR_EL2);
> 

Addressed in v5. vcpu_hdbss_state now stores a struct page * instead of
a physical address, and the allocation check uses the page pointer. This
eliminates the PA == 0 special case entirely.

> [Severity: High]
> Will __load_hdbss() trigger a fatal UNDEFINED exception on hardware without
> HDBSS? The function relies solely on kvm->arch.enable_hdbss. If it is enabled
> but the hardware lacks support, accessing SYS_HDBSSBR_EL2 could panic the
> hypervisor.
> 
>> +	write_sysreg_s(vcpu->arch.hdbss.hdbssprod_el2, SYS_HDBSSPROD_EL2);
> 

v5 already handles this. vcpu_hdbss_enabled() checks hw_mmu->vtcr &
VTCR_EL2_HDBSS, and that bit only gets set through
kvm_arm_enable_hdbss_global(), which is guarded by
system_supports_hdbss(). So __load_hdbss() doesn't need to check again.
Hardware support is already guaranteed at that point.

> [Severity: Medium]
> Is an isb() required immediately after writing to SYS_HDBSSBR_EL2? ARM64
> subsystem rules state that every write to a control-plane system register must
> be followed by an isb() as the very next instruction.
> 
>> +
>> +	isb();
>> +}
>> +
>>   void kvm_vcpu_load_vhe(struct kvm_vcpu *vcpu)
>>   {
>>   	host_data_ptr(host_ctxt)->__hyp_running_vcpu = vcpu;
>> @@ -220,6 +234,7 @@ void kvm_vcpu_load_vhe(struct kvm_vcpu *vcpu)
>>   	__vcpu_load_switch_sysregs(vcpu);
>>   	__vcpu_load_activate_traps(vcpu);
>>   	__load_stage2(vcpu->arch.hw_mmu);
>> +	__load_hdbss(vcpu);
>>   }
>>   
>>   void kvm_vcpu_put_vhe(struct kvm_vcpu *vcpu)
>>   {
>>   	__vcpu_put_deactivate_traps(vcpu);
>>   	__vcpu_put_switch_sysregs(vcpu);
>>   
>>   	host_data_ptr(host_ctxt)->__hyp_running_vcpu = NULL;
>>   }
> 

Not required. The architecture only requires an ISB to make the effects
visible, and it's fine to batch multiple system register writes with one
trailing ISB. __load_hdbss() follows the same pattern as
__load_stage2(), which writes VTCR_EL2 and VTTBR_EL2 back-to-back with a
single ISB. So the existing isb() in __load_hdbss() is sufficient.

> [Severity: High]
> Are the hardware producer index updates lost during context switch?
> 
> The producer index is loaded in kvm_vcpu_load_vhe(), but it doesn't appear KVM
> reads and saves it back to vcpu->arch.hdbss.hdbssprod_el2 during
> kvm_vcpu_put_vhe(). Would this rewind the hardware index on the next load?
> 

Addressed in v5, kvm_vcpu_put_vhe() now saves HDBSSPROD_EL2 into vcpu-
 >arch.hdbss.hdbssprod_el2, and __load_hdbss() restores it on the next
load, so the producer index survives context switches and cross-CPU
migration.

Thanks!
Tian