Re: [RFC PATCH 5/5] KVM: arm64: Enable HAFDBS for guests not on migration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Data race and TOCTOU on `kvm->arch.mmu.vtcr` between vCPU setup and memslot updates, breaking dirty logging.
- [High] Missing context-wide TLB invalidation after modifying `VTCR_EL2.HD` results in hardware bypassing write-protection.
- [High] Disabling HAFDBS globally for dirty logging causes massive performance degradation on non-logging memslots due to unnecessary permission faults.
- [Low] Unreachable `KVM_MR_DELETE` check inside `log_dirty_pages` block.
--

commit 54d7950cd735bc1edc3a9503d03ebffb8e6ca046
Author: Leonardo Bras <leo.bras@arm.com>

KVM: arm64: Enable HAFDBS for guests not on migration

This patch enables hardware management of access and dirty bits (HAFDBS)
for KVM guests on arm64 when dirty logging is disabled. It modifies
vCPU setup and memslot commit paths to dynamically toggle VTCR_EL2.HD
based on the presence of dirty-logged memslots.

> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 1e528d53d0931..76aebaa95cdab 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c

[ ... ]

> @@ -1683,6 +1683,13 @@ static int kvm_setup_vcpu(struct kvm_vcpu *vcpu)
>  	if (!ret && vcpu_has_nv(vcpu))
>  		ret = kvm_vcpu_init_nested(vcpu);
>  
> +	/* Enable HAFDBS by default if VHE && !nested  */
> +	if (kvm_supports_hafdbs(kvm) &&
> +	    atomic_read(&kvm->nr_memslots_dirty_logging) == 0)
> +		kvm->arch.mmu.vtcr |= VTCR_EL2_HD;
> +	else
> +		kvm->arch.mmu.vtcr &= ~VTCR_EL2_HD;
> +

[Severity: Critical]
Does this read-modify-write of kvm->arch.mmu.vtcr (holding config_lock)
create a data race against kvm->slots_lock?

In kvm_set_hafdbs() below, this same variable is modified while holding
kvm->slots_lock. If userspace concurrently initializes vCPUs and enables
dirty logging, VTCR_EL2_HD could be incorrectly left at 1. This would cause
the hardware to silently update dirty bits without trapping, bypassing
software dirty logging and resulting in lost dirty pages during migration.

>  	return ret;
>  }

[ ... ]

> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> index 9d4f70430bbea..94094ab56d90f 100644
> --- a/arch/arm64/kvm/mmu.c
> +++ b/arch/arm64/kvm/mmu.c
> @@ -1989,7 +1989,8 @@ static int kvm_s2_fault_compute_prot(const struct kvm_s2_fault_desc *s2fd,
>  	if (s2vi->map_writable) {
>  		*prot |= KVM_PGTABLE_PROT_W;
>  
> -		if (s2vi->device || !memslot_is_logging(s2fd->memslot) ||
> +		if (s2vi->device ||
> +		    !(memslot_is_logging(s2fd->memslot) || kvm_supports_hafdbs(kvm)) ||
>  		    kvm_is_write_fault(s2fd->vcpu))
>  			*prot |= KVM_PGTABLE_PROT_DIRTY;

[Severity: High]
Can checking the static capability kvm_supports_hafdbs() here instead of
the dynamic vtcr state starve non-logging slots of the software fallback?

When dirty logging is enabled on one memslot, kvm_set_hafdbs() disables
VTCR_EL2.HD globally. However, if a guest writes to an unrelated
non-logging slot that was mapped writable-clean, won't the hardware now
generate a stage-2 permission fault?

This could cause performance degradation across the entire VM whenever any
single memslot (like a VGA framebuffer) enables dirty logging.

[ ... ]

> @@ -2577,6 +2578,20 @@ int __init kvm_mmu_init(u32 hyp_va_bits)
>  	return err;
>  }
>  
> +static void kvm_set_hafdbs(struct kvm *kvm, bool set)
> +{
> +	/* Check if no action required */
> +	if (!!(kvm->arch.mmu.vtcr & VTCR_EL2_HD) == set)
> +		return;
> +
> +	if (set)
> +		kvm->arch.mmu.vtcr |= VTCR_EL2_HD;
> +	else
> +		kvm->arch.mmu.vtcr &= ~VTCR_EL2_HD;

[Severity: Critical]
Does modifying kvm->arch.mmu.vtcr here under kvm->slots_lock race against
the unprotected updates in kvm_setup_vcpu() as noted above?

> +
> +	kvm_make_all_cpus_request(kvm, KVM_REQ_RELOAD_STAGE2);

[Severity: High]
Is KVM_REQ_RELOAD_STAGE2 sufficient here to ensure the hardware observes
the changed VTCR_EL2 state?

The ARM architecture permits translation regime registers like VTCR_EL2 to
be cached in the TLB (or a context cache). Modifying this register
typically requires a full VMID TLB invalidation (like calling
kvm_flush_remote_tlbs(kvm)) to guarantee global visibility. 

Without it, the CPU might continue using cached translations with HD=1 in
the write-protected memslot, silently bypassing write-protection.

> +}
> +
>  void kvm_arch_commit_memory_region(struct kvm *kvm,
>  				   struct kvm_memory_slot *old,
>  				   const struct kvm_memory_slot *new,
> @@ -2594,6 +2609,10 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,
>  		if (change == KVM_MR_DELETE)
>  			return;

[Severity: Low]
Is this KVM_MR_DELETE condition reachable?

The log_dirty_pages variable checked just before this is initialized as
'new && new->flags & KVM_MEM_LOG_DIRTY_PAGES'. During a KVM_MR_DELETE
change, the 'new' parameter passed into kvm_arch_commit_memory_region() is
strictly NULL. Therefore, log_dirty_pages will always be false for
deletes, making this nested check dead code.

[ ... ]

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