Re: [PATCH v3 11/11] KVM: arm64: Enable KVM_HW_DIRTY_BIT

On Wed, Jul 29, 2026 at 11:29:18AM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> - [Critical] Direct accesses to EL2 system registers from EL1 cause host panics on nVHE and pKVM configurations.
> - [High] Writes to control-plane system registers are missing immediate `isb()` synchronization instructions.
> - [High] Unbounded hardware polling loop with preemption disabled.
> - [Medium] Preempt notifier is registered within an atomic section where preemption is inherently disabled.
> --
> 
> --- Patch [11]: [PATCH v3 11/11] KVM: arm64: Enable KVM_HW_DIRTY_BIT ---
> commit 9dec1efff0823ef68d8ee166e3fa545263373c0f
> Author: Leonardo Bras <[email protected]>
> 
> KVM: arm64: Enable KVM_HW_DIRTY_BIT
>     
> Set the corresponding bit to enable hardware accelerated dirty-bitmap and
> dirty-ring cleaning for arm64. Actually using acceleration depends on the
> cpus enabling FEAT_HACDBS as well as the pre-requisite features for it,
> such as FEAT_HDBSS and FEAT_HAFDBS.
> 
> > diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig
> > index 449154f9a4852..3264d8ca093e3 100644
> > --- a/arch/arm64/kvm/Kconfig
> > +++ b/arch/arm64/kvm/Kconfig
> > @@ -34,6 +34,7 @@ menuconfig KVM
> >  	select HAVE_KVM_IRQ_BYPASS
> >  	select HAVE_KVM_READONLY_MEM
> >  	select HAVE_KVM_VCPU_RUN_PID_CHANGE
> > +	select HAVE_KVM_HW_DIRTY_BIT
> >  	select SCHED_INFO
> >  	select GUEST_PERF_EVENTS if PERF_EVENTS
> >  	select KVM_GUEST_MEMFD
> > diff --git a/arch/arm64/kvm/dirty_bit.c b/arch/arm64/kvm/dirty_bit.c
> > new file mode 100644
> > index 0000000000000..ca9cfefe0893b
> > --- /dev/null
> > +++ b/arch/arm64/kvm/dirty_bit.c
> > @@ -0,0 +1,438 @@
> [ ... ]
> > +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: Critical]
> Can this code cause a host panic on nVHE and pKVM configurations?

It's VHE only

> 
> The code directly attempts to read and write EL2 registers, which triggers
> a synchronous undefined exception at EL1. Since kvm_arch_dirty_clear_enabled()
> only checks for system_supports_hacdbs() and does not enforce has_vhe(),
> could an unprivileged userspace application reliably crash the host by
> triggering this on non-VHE hardware?
> 
> [Severity: High]
> Does this code allow the CPU pipeline to observe architecturally undefined
> state? 
> 
> The ARM64 subsystem rules require an isb() as the very next instruction
> following a control-plane sysreg write. Here, SYS_HACDBSCONS_EL2 is modified
> but several instructions and variable assignments occur before an isb() is
> reached.

Already commented on a early patch.

> 
> > +
> > +	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);
> > +
> > +	/* All writes to the array have to complete before starting */
> > +	dsb(ishst);
> > +
> > +	write_sysreg_s(br, SYS_HACDBSBR_EL2);
> > +	isb();
> > +}
> [ ... ]
> > +static void hacdbs_sched_in(struct preempt_notifier *notifier, int cpu)
> > +{
> > +	struct hacdbs_sched_save *save = container_of(notifier,
> > +						      struct hacdbs_sched_save,
> > +						      notifier);
> > +
> > +	write_sysreg_s(save->save.vtcr_el2, SYS_VTCR_EL2);
> > +	write_sysreg_s(save->save.vttbr_el2, SYS_VTTBR_EL2);
> > +	isb();
> 
> [Severity: High]
> Can this missing synchronization lead to unpredictable execution?
> 
> The ARM64 subsystem rules require an isb() as the very next instruction
> following a control-plane sysreg write, but SYS_VTCR_EL2 is written followed
> by SYS_VTTBR_EL2 before the isb() is issued.
> 

iden

> > +
> > +	if (save->save.hcr_vm_set) {
> > +		sysreg_clear_set_hcr(0, HCR_EL2_VM);
> > +		isb();
> > +	}
> > +
> > +	write_sysreg_s(save->save.hacdbscons_el2, SYS_HACDBSCONS_EL2);
> > +	write_sysreg_s(save->save.hacdbsbr_el2, SYS_HACDBSBR_EL2);
> > +	isb();
> 
> [Severity: High]
> Is another isb() required here?
> 
> SYS_HACDBSCONS_EL2 is written followed by SYS_HACDBSBR_EL2 without
> an intervening isb(). Does this sequence allow the CPU pipeline to observe
> stale configurations?
> 

iden

> > +}
> [ ... ]
> > +static int dirty_bit_clear(struct kvm *kvm, u64 *hw_entries, int size)
> > +{
> > +	int ret;
> > +	u64 cons;
> > +	struct hacdbs_sched_save save;
> > +
> > +	preempt_notifier_init(&save.notifier, &hacdbs_preempt_ops);
> > +	preempt_disable();
> > +	preempt_notifier_register(&save.notifier);
> > +	preempt_enable();
> 
> [Severity: Medium]
> Are these preempt notifiers effectively dead code on standard kernels?
> 
> The calling functions (__kvm_arch_dirty_log_clear and
> __kvm_arch_dirty_ring_clear) acquire write_lock(&kvm->mmu_lock) before
> this code runs.
> 
> Since the mmu_lock rwlock already disables preemption, this region cannot be
> preempted. If the hardware cleaning takes a long time, won't the thread
> stall rather than cleanly context switching?
> 

iden

> > +
> > +	__load_stage2(&kvm->arch.mmu);
> > +	sysreg_clear_set_hcr(0, HCR_EL2_VM);
> > +	isb();
> 
> [Severity: Critical]
> Does this code also trigger a synchronous undefined exception at EL1?
> 
> Directly attempting to access EL2 registers from EL1 causes traps on non-VHE
> hardware.
> 

iden

> > +
> > +	hacdbs_start(hw_entries, size);
> > +
> > +	do {
> > +		cons = read_sysreg_s(SYS_HACDBSCONS_EL2);
> > +		if (FIELD_GET(HACDBSCONS_EL2_ERR_REASON, cons))
> > +			break;
> > +
> > +		if (FIELD_GET(HACDBSCONS_EL2_INDEX, cons) >= size)
> > +			break;
> > +
> > +		udelay(1);
> > +	} while (true);
> 
> [Severity: High]
> Could this unbounded polling loop cause a hard lockup?
> 
> Since this runs under write_lock(&kvm->mmu_lock) with preemption disabled,
> if the hardware accelerator hangs, takes unusually long, or fails to set an
> error reason, the CPU will stall indefinitely. Should there be a timeout or
> fallback mechanism here?

iden

Leo