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

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

On Fri, Sep 18, 2026 at 07:58:43PM +0800, Tian Zheng wrote:
> 
> 
> On 9/2/2026 1:15 AM, Leonardo Bras wrote:
> > @@ -2570,53 +2571,76 @@ int __init kvm_mmu_init(u32 hyp_va_bits)
> >   out_destroy_pgtable:
> >   	kvm_pgtable_hyp_destroy(hyp_pgtable);
> >   out_free_pgtable:
> >   	kfree(hyp_pgtable);
> >   	hyp_pgtable = NULL;
> >   out:
> >   	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;
> > +
> > +	kvm_make_all_cpus_request(kvm, KVM_REQ_RELOAD_STAGE2);
> > +}
> > +
> 
> Hi Leo,
> 
> HD alone is architecturally a no-op. According to the Arm spec, the
> VTCR_EL2.HD field description reads: "When the Effective value of
> VTCR_EL2.HA is 0, this field behaves as 0 for all purposes other than a
> direct read of the value of this bit." So patch 5 as it stands never
> actually enables stage-2 dirty management.

Oh, right, I missed that. :/

For the above to work it would need to have the HA bit:

+	if (set)
+		kvm->arch.mmu.vtcr |= VTCR_EL2_HA | VTCR_EL2_HD;
+	else
+		kvm->arch.mmu.vtcr &= ~(VTCR_EL2_HA |VTCR_EL2_HD);

> 
> In the combined series, I'm planning to replace both kvm_set_hafdbs()
> and our earlier enable/disable hooks with a single derived mode:
> 
> ```
> /*
>  *   logging && HDBSS-capable  ->  HDBSS  (HD|HA|HDBSS)
>  *   logging, no HDBSS         ->  off
>  *   !logging, HAFDBS-capable  ->  HAFDBS (HD|HA)
>  */

Yes, seems correct, if the performance numbers can show patch 5 provides
performance improvements.

> void kvm_arch_update_hw_dirty_mode(struct kvm *kvm)
> {
> 	bool logging = atomic_read(&kvm->nr_memslots_dirty_logging) != 0;
> 	unsigned long target;
> 
> 	if (logging && kvm_supports_hdbss(kvm))
> 		target = VTCR_EL2_HD | VTCR_EL2_HA | VTCR_EL2_HDBSS;
> 	else if (logging || !kvm_supports_hafdbs(kvm))
> 		target = 0;
> 	else
> 		target = VTCR_EL2_HD | VTCR_EL2_HA;
> 	...
> }
> ```

Humm, do we actually need the atomic_read being ran when we are enabling 
dirty_logging? Maybe we can restrain it only to the disabling part.

> 
> HA is always set alongside HD by construction, so the no-op issue goes
> away.

Right.

> 
> I'll fold this into the HDBSS v5 series when I send it out. Let me know
> if you'd rather keep it in your v2 instead.
> 

Feel free to merge your part on top of that, I will work on collecting the 
numbers, though.

Thanks!
Leo