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

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

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

IIUC they will have no effect until the base register says enabled. So an 
isb() at the end should be enough.

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

Again, it won't operate until the enable at the end of hacdbs_start()

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

Answered before. Yes, it's unlikely, but maybe we need a WFIT.

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


There is LITERALY a comment below addressing this.

> 
> > +
> > +	/*
> > +	 * 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