Re: [PATCH v5 14/15] KVM: arm64: Document HDBSS buffer size ioctl

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] kvm: arm64: missing vCPU kick in kvm_arch_update_hw_dirty_mode causes data corruption
- [High] kvm: arm64: stage2_wrprotect_walker clobbers concurrent hardware PTE updates
- [Medium] kvm: uapi: enabling dirty ring silently resets HDBSS buffer size
- [Low] kvm: uapi: KVM_CHECK_EXTENSION misused to return mutable VM state
--

commit ad3d8df658768bdca959742e1b61e3dd3914fdd8
Author: Tian Zheng <zhengtian10@huawei.com>

KVM: arm64: Document HDBSS buffer size ioctl
    
Document KVM_CAP_ARM_HDBSS_BUFFER_SIZE, which lets userspace configure the
per-vCPU HDBSS buffer size for hardware-assisted dirty tracking during live
migration.

> diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
> --- a/Documentation/virt/kvm/api.rst
> +++ b/Documentation/virt/kvm/api.rst
> @@ -9056,6 +9056,34 @@ enabled, cmma can't be enabled anymore and pfmfi and the storage key
>  interpretation are disabled. If cmma has already been enabled or the
>  hpage_2g module parameter is not set to 1, -EINVAL is returned.
>  
> +7.48 KVM_CAP_ARM_HDBSS_BUFFER_SIZE
> +-----------------------------------
> +
> +:Architectures: arm64
> +:Target: VM
> +:Parameters: args[0] is the per-vCPU HDBSS buffer size in bytes
> +:Returns: 0 on success; -EINVAL if the size is invalid or vCPUs have already
> +          been created; -EBUSY if the buffer size was already configured.
> +
> +This capability configures the per-vCPU HDBSS buffer size used for
> +hardware-assisted dirty tracking during live migration.
> +
> +Userspace sets the size in bytes via KVM_ENABLE_CAP. KVM allocates
> +per-vCPU HDBSS buffers of the requested size.
> +
> +KVM_CHECK_EXTENSION returns the maximum supported size (``SZ_2M``)
> +when queried without a VM, or the configured per-VM size (default
> +``PAGE_SIZE``) when queried with a VM.

[Severity: Low]
KVM_CHECK_EXTENSION is designed to advertise static feature support and
limits, not to serve as a GET interface for mutable, dynamically configured
VM attributes.

Would it be better to use a dedicated interface to read back this dynamic
state instead of KVM_CHECK_EXTENSION?

> +
> +Constraints:
> +
> +- The size must be a power of two in [``PAGE_SIZE``, ``SZ_2M``].
> +- Dirty-bitmap mode only: rejected with -EINVAL once the dirty ring
> +  (``KVM_CAP_DIRTY_LOG_RING``) is enabled, and enabling the ring after
> +  a size was set resets it to the default.

[Severity: Medium]
The KVM guidelines mandate that new memslot and vCPU flags default to
immutable. 

Instead of silently mutating previously configured state (which can lead to
unpredictable state-machine transitions that userspace is not prepared
for), should this incompatible capability configuration fail explicitly by
returning an error like -EINVAL?

> +- Must be set before any vCPU is created; a second setting is rejected
> +  with -EBUSY.
>  
> diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
> --- a/arch/arm64/kvm/hyp/pgtable.c
> +++ b/arch/arm64/kvm/hyp/pgtable.c

[ ... ]

> @@ -1285,12 +1298,47 @@ static int stage2_update_leaf_attrs(struct kvm_pgtable *pgt, u64 addr,
>  	return 0;
>  }
>  
> +static int stage2_wrprotect_walker(const struct kvm_pgtable_visit_ctx *ctx,
> +				   enum kvm_pgtable_walk_flags visit)
> +{
> +	kvm_pte_t new = ctx->old & ~KVM_PTE_LEAF_ATTR_LO_S2_S2AP_W;
> +
> +	/* We remove DBM on blocks so they can fault and get split */
> +	if (ctx->level < KVM_PGTABLE_LAST_LEVEL)
> +		new &= ~KVM_PTE_LEAF_ATTR_HI_S2_DBM;
> +
> +	if (kvm_pte_valid(ctx->old) && ctx->old != new &&
> +	    (ctx->old & KVM_PTE_LEAF_ATTR_LO_S2_S2AP_W) &&
> +	    ctx->mm_ops->mark_page_dirty)
> +		ctx->mm_ops->mark_page_dirty(kvm_pte_to_phys(ctx->old));
> +
> +	/*
> +	 * The plain WRITE_ONCE races with hardware updates; both are
> +	 * benign.
> +	 *
> +	 * AF: the update may be lost, and is set on the next access.
> +	 *
> +	 * Dirty state: we only rewrite entries whose old value had S2AP[1]
> +	 * set, while hardware only promotes entries with S2AP[1] clear, so
> +	 * the two never touch the same entry. The one overlap is DBM removal
> +	 * on writable-clean blocks: a racing promotion is demoted back to
> +	 * read-only, but the write is still recorded in the HDBSS buffer and
> +	 * the folio was marked dirty at fault-in, so nothing is lost.
> +	 */
> +	if (kvm_pte_valid(ctx->old) && ctx->old != new)
> +		WRITE_ONCE(*ctx->ptep, new);

[Severity: High]
The global review guidelines strictly mandate: When KVM updates SPTE bits that
the hardware page-table walker also writes (Dirty/Access on leaf entries), the
update must not clobber a concurrent hardware update.

Since WRITE_ONCE unconditionally overwrites the PTE with the old masked
value, could this non-atomic write in stage2_wrprotect_walker() clobber
concurrent hardware updates to the Access Flag (AF) or Dirty Bit on leaf
PTEs?

> +
> +	return 0;
> +}
> +
> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> --- a/arch/arm64/kvm/mmu.c
> +++ b/arch/arm64/kvm/mmu.c

[ ... ]

> @@ -2581,6 +2629,45 @@ int __init kvm_mmu_init(u32 hyp_va_bits)
>  	return err;
>  }
>  
> +/*
> + * The VM's hardware dirty-management mode is a derived value, a pure
> + * function of the static capabilities and the number of logging
> + * memslots, so recomputing it on every event cannot lose an update
> + * and needs no locking against racing writers:
> + *
> + *   logging && HDBSS-capable  ->  HD|HA|HDBSS (hardware tracking)
> + *   logging, no HDBSS         ->  off (write-protect faults)
> + *   !logging && HAFDBS-cap.   ->  HD|HA (only written pages go dirty)
> + */
> +void kvm_arch_update_hw_dirty_mode(struct kvm *kvm)
> +{
> +	unsigned long cur, target;
> +	bool logging = atomic_read(&kvm->nr_memslots_dirty_logging) != 0;
> +
> +	if (logging && kvm_supports_hdbss(kvm))
> +		target = VTCR_EL2_HD | VTCR_EL2_HA | VTCR_EL2_HDBSS;
> +	else if (logging || !kvm_supports_hafdbs())
> +		target = 0;
> +	else
> +		target = VTCR_EL2_HD | VTCR_EL2_HA;
> +
> +	cur = kvm->arch.mmu.vtcr & (VTCR_EL2_HD | VTCR_EL2_HA | VTCR_EL2_HDBSS);
> +	if (cur == target)
> +		return;
> +
> +	kvm->arch.mmu.vtcr = (kvm->arch.mmu.vtcr &
> +			      ~(VTCR_EL2_HD | VTCR_EL2_HA | VTCR_EL2_HDBSS)) |
> +			     target;
> +
> +	kvm_make_all_cpus_request(kvm, KVM_REQ_RELOAD_STAGE2);
> +
> +	/*
> +	 * The request only reloads VTCR_EL2; cached translations keep
> +	 * the old permissions until invalidated.
> +	 */
> +	kvm_flush_remote_tlbs(kvm);

[Severity: Critical]
Inside kvm_arch_update_hw_dirty_mode(), kvm_make_all_cpus_request() is issued
with KVM_REQ_RELOAD_STAGE2, which is defined as:

#define KVM_REQ_RELOAD_STAGE2 \
	KVM_ARCH_REQ_FLAGS(12, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)

The KVM_REQUEST_NO_WAKEUP flag prevents the request from interrupting running
vCPUs. The subsequent kvm_flush_remote_tlbs() executes a hardware broadcast TLBI
and also does not send IPIs. 

Because running vCPUs are never kicked out of guest mode, won't they
continue executing with their physical VTCR_EL2 register holding the
stale state (HD=1)?

If software dirty logging is active but HDBSS is not correctly synchronized
on the CPU, the hardware page table walker could silently promote
writable-clean PTEs to writable-dirty without trapping. Since KVM never
sees the write, could this cause silent data loss during live migration?

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260929103655.85107-1-zhengtian10@huawei.com?part=14