Re: [PATCH v2 06/13] KVM: arm64: dirty_bit: Add base FEAT_HACDBS cleaning routine

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Missing Context Synchronization Event (ISB) after writes to control-plane system registers.
- [High] TOCTOU race in wfi() polling loop causes a lost wakeup and indefinite CPU stall.
- [High] Missing memory barrier (dsb) before starting the HACDBS hardware engine.
- [High] Missing speculative execution barrier (dsb) after disabling an asynchronous hardware engine.
--

commit 47f5964570817fd652e798e082c349d012b3edda
Author: Leonardo Bras <leo.bras@arm.com>

KVM: arm64: dirty_bit: Add base FEAT_HACDBS cleaning routine

Introduce the basic cleaning routine that is going to be used for both
dirty-bitmap and dirty-ring routines. It sets the required registers with
the input buffer, and wait for HACDBSIRQ to happen, which means either
the task is done, or there was some error during processing.

> diff --git a/arch/arm64/kvm/dirty_bit.c b/arch/arm64/kvm/dirty_bit.c
> index 789da8712b1b4..e4283828b7807 100644
> --- a/arch/arm64/kvm/dirty_bit.c
> +++ b/arch/arm64/kvm/dirty_bit.c

[ ... ]

> @@ -24,6 +25,86 @@ DEFINE_PER_CPU(struct hacdbs, hacdbs_pcp) = {
>  
>  static __ro_after_init int hacdbsirq = -1;
>  
> +static void hacdbs_start(u64 *hw_entries, int size)
> +{
> +	u64 br;
> +	/* Each entry is 8 bytes */
> +	int size_b = size * sizeof(hw_entries[0]);
> +	int size_p2 = max(roundup_pow_of_two(size_b), PAGE_SIZE);
> +
> +	/* If not using the full size of the array, put a stop entry at the end */
> +	if (size_b < size_p2)
> +		hw_entries[size] = HDBSS_ENTRY_VALID | HDBSS_ENTRY_TTWL_RESV;
> +
> +	sysreg_clear_set_s(SYS_HACDBSCONS_EL2,
> +			   HACDBSCONS_EL2_ERR_REASON | HACDBSCONS_EL2_INDEX, 0);
> +

[Severity: High]
Does this code need an immediate isb() after writing the control-plane system
register SYS_HACDBSCONS_EL2? The ARM64 guidelines indicate that an isb() must
be the very next instruction after writing a control-plane system register to
ensure the pipeline doesn't operate with architecturally undefined state.

[Severity: High]
Additionally, should there be a dsb() between the memory store to
hw_entries[size] and the write to SYS_HACDBSBR_EL2 below? Without an explicit
memory barrier, the store buffer might not be globally visible before the
engine begins reading memory, which could cause it to miss the stop marker
and access out-of-bounds memory.

> +	br = (virt_to_phys(hw_entries) & HACDBSBR_EL2_BADDR_MASK) |
> +	     FIELD_PREP(HACDBSBR_EL2_SZ, ilog2(size_p2) - 12) |
> +	     FIELD_PREP(HACDBSBR_EL2_EN, 1);
> +
> +	this_cpu_write(hacdbs_pcp.status, HACDBS_RUNNING);
> +	this_cpu_write(hacdbs_pcp.size, size);
> +	write_sysreg_s(br, SYS_HACDBSBR_EL2);
> +	isb();
> +}

[ ... ]

> +/*
> + * Clears dirty-bits for an array of pages (hw_entries) using HACDBS
> + * Returns the number of items cleaned from the array. If returns value < size,
> + *	there was an error in the processing.
> + */
> +static int dirty_bit_clear(struct kvm *kvm, u64 *hw_entries, int size)
> +{
> +	u64 hcr_el2;
> +	int ret;
> +
> +	preempt_disable();
> +
> +	hcr_el2 = read_sysreg(HCR_EL2);
> +	write_sysreg(hcr_el2 | HCR_EL2_VM, HCR_EL2);
> +	__load_stage2(&kvm->arch.mmu);

[Severity: High]
Is an isb() required immediately after writing HCR_EL2 before proceeding to
__load_stage2()? HCR_EL2 is a control-plane register, and delaying the barrier
can cause the pipeline to operate with architecturally undefined state.

> +
> +	hacdbs_start(hw_entries, size);
> +
> +	do {
> +		wfi();
> +	} while (this_cpu_read(hacdbs_pcp.status) == HACDBS_RUNNING);

[Severity: High]
Could this polling loop cause a lost wakeup and indefinite CPU stall? The loop
unconditionally sleeps on the first iteration and checks the condition
non-atomically with respect to interrupts. 

If the HACDBSIRQ interrupt is delivered and serviced just before the wfi()
instruction executes, the wfi() will wait for a new interrupt that will never
arrive. Should the check be made atomic with the sleep, for example by masking
IRQs before the check?

> +
> +	ret = hacdbs_stop();
> +
> +	write_sysreg(hcr_el2, HCR_EL2);
> +	isb();

[Severity: High]
Should there be a speculative execution barrier (dsb) after stopping the engine
in hacdbs_stop() and before changing the translation context here? 

The ARM64 guidelines mandate a speculative execution barrier (dsb + isb) after
disabling an asynchronous engine to definitively halt out-of-context execution
before updating the translation regime.

> +
> +	/*
> +	 * No DSB is needed here, as kvm_flush_remote_tlbs_memslot() that happens
> +	 * later in generic dirty-cleaning code already performs a DSB before
> +	 * doing the TLBI.
> +	 */
> +
> +	preempt_enable();
> +
> +	return ret;
> +}

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