From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id AA790C4450A for ; Thu, 16 Jul 2026 07:16:03 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:Content-Transfer-Encoding: Content-Type:In-Reply-To:From:References:CC:To:Subject:MIME-Version:Date: Message-ID:Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=gm22hrAigOsN+KttpcC84tePunTEx/exI0QjC5Fai5Y=; b=Ke5kb2X5VUPSLk8kY/t4PtfoH+ SB1ByhGqJ/7Wuy9BYyoQdo7RpzS/RtutXjErcnuiQ5+UP2dtXerGOolsD81iQBQqmjJJa+3LmJKvq fS6gUZEuF53BoTf7r2fbt0fhPwvswJK7y/ezm63BMT+v0VfVq1R5iNn4agg4TYkB3kjMrXazoRVJy kp6ltTPLE8TpBvb8+9c9hiFJ9vySiku+MgA5/UYp7rF1n0afjNagnpXeT3BeBoRgDO79KMPdKPMxQ /fe8Okg5FnCklQYV6XDGgSwa0WCxvxZDSCoPIH74f7sNESV+75BOEZ03jKc1e2lHCfZBPuEIaIqIs xat7sUVQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.99.1 #2 (Red Hat Linux)) id 1wkGK4-0000000GdAs-2bjP; Thu, 16 Jul 2026 07:15:56 +0000 Received: from canpmsgout06.his.huawei.com ([113.46.200.221]) by bombadil.infradead.org with esmtps (Exim 4.99.1 #2 (Red Hat Linux)) id 1wkGK1-0000000Gd9v-117Q for linux-arm-kernel@lists.infradead.org; Thu, 16 Jul 2026 07:15:55 +0000 dkim-signature: v=1; a=rsa-sha256; d=huawei.com; s=dkim; c=relaxed/relaxed; q=dns/txt; h=From; bh=gm22hrAigOsN+KttpcC84tePunTEx/exI0QjC5Fai5Y=; b=yBBwLWwhKXoz9vTB9kpDoK2b7JIdgTcVC1/JzUy3AG5oY2ge2ptFVoFcdkafGHdJPX87POmog oD2EnAH422UG4u191J+ptDvMLIavGkO1iFragFPJjNkF9RZPVhs2u7QYmXulIcPlT7zuee2bgpl tpb28QPfIBsvj2LMEBA0e2Q= Received: from mail.maildlp.com (unknown [172.19.163.104]) by canpmsgout06.his.huawei.com (SkyGuard) with ESMTPS id 4h13x83FFpzRhVG; Thu, 16 Jul 2026 15:06:20 +0800 (CST) Received: from kwepemr100010.china.huawei.com (unknown [7.202.195.125]) by mail.maildlp.com (Postfix) with ESMTPS id 23FBB4057F; Thu, 16 Jul 2026 15:15:38 +0800 (CST) Received: from [10.67.120.103] (10.67.120.103) by kwepemr100010.china.huawei.com (7.202.195.125) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.1544.36; Thu, 16 Jul 2026 15:15:37 +0800 Message-ID: Date: Thu, 16 Jul 2026 15:15:38 +0800 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v4 6/6] KVM: arm64: Add auto HDBSS enable/disable on dirty logging change To: Leonardo Bras CC: , , , , , , , , , , , , , , , , , , References: <20260709104026.2612599-1-zhengtian10@huawei.com> <20260709104026.2612599-7-zhengtian10@huawei.com> From: Tian Zheng In-Reply-To: Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.120.103] X-ClientProxiedBy: kwepems100001.china.huawei.com (7.221.188.238) To kwepemr100010.china.huawei.com (7.202.195.125) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.9.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20260716_001553_914806_811E1FA7 X-CRM114-Status: GOOD ( 50.66 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 7/13/2026 10:50 PM, Leonardo Bras wrote: > On Thu, Jul 09, 2026 at 06:40:26PM +0800, Tian Zheng wrote: >> From: eillon >> >> HDBSS buffers store per-page dirty state after the stage-2 page tables >> have been split down to page granularity (chunk_size == PAGE_SIZE). > chunk_size != PAGE_SIZE now, but that should change as well :) > >> When chunk_size == 0 the kernel may lazily skip splitting block mappings, >> leaving the page table coarser than what HDBSS expects. Therefore, >> enabling HDBSS requires disabling lazy split so that all block mappings >> are eagerly broken down before the buffer starts recording. > (See cover letter reply) > >> Add VM-level HDBSS enable/disable support. When dirty logging is >> enabled on any memslot, HDBSS is automatically enabled. When dirty >> logging is disabled on all memslots, HDBSS is automatically disabled. >> >> This includes: >> - kvm_arm_enable_hdbss_global() to enable HDBSS for all vCPUs >> - kvm_arm_disable_hdbss_global() to disable and free HDBSS buffers >> - kvm_arm_hdbss_on_dirty_logging_change() for auto enable/disable >> - kvm_arch_destroy_vm() cleanup path >> - kvm_arch_commit_memory_region() integration >> >> Signed-off-by: Eillon >> Signed-off-by: Tian Zheng >> --- >> arch/arm64/include/asm/kvm_dirty_bit.h | 2 + >> arch/arm64/kvm/arm.c | 8 ++ >> arch/arm64/kvm/dirty_bit.c | 105 +++++++++++++++++++++++++ >> arch/arm64/kvm/mmu.c | 3 + >> 4 files changed, 118 insertions(+) >> >> diff --git a/arch/arm64/include/asm/kvm_dirty_bit.h b/arch/arm64/include/asm/kvm_dirty_bit.h >> index 4b28000e972f..a4cda8cdab24 100644 >> --- a/arch/arm64/include/asm/kvm_dirty_bit.h >> +++ b/arch/arm64/include/asm/kvm_dirty_bit.h >> @@ -23,5 +23,7 @@ int kvm_arm_vcpu_alloc_hdbss(struct kvm_vcpu *vcpu, unsigned int order); >> void kvm_arm_vcpu_free_hdbss(struct kvm_vcpu *vcpu); >> void kvm_flush_hdbss_buffer(struct kvm_vcpu *vcpu); >> int kvm_handle_hdbss_fault(struct kvm_vcpu *vcpu); >> +void kvm_arm_hdbss_on_dirty_logging_change(struct kvm *kvm, int nr_memslots_logging); >> +void kvm_arm_disable_hdbss_global(struct kvm *kvm); >> >> #endif /* __ARM64_KVM_DIRTY_BIT_H__ */ >> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c >> index 566953a4e23a..536d94799ba8 100644 >> --- a/arch/arm64/kvm/arm.c >> +++ b/arch/arm64/kvm/arm.c >> @@ -317,6 +317,14 @@ void kvm_arch_destroy_vm(struct kvm *kvm) >> if (is_protected_kvm_enabled()) >> pkvm_destroy_hyp_vm(kvm); >> >> + /* >> + * Userspace may destroy the VM without disabling dirty logging, >> + * so the auto-disable path is never reached. Force disable HDBSS >> + * here to ensure vCPU buffers are freed and prevent memory leaks. >> + */ >> + if (kvm->arch.enable_hdbss) >> + kvm_arm_disable_hdbss_global(kvm); >> + >> kvm_uninit_stage2_mmu(kvm); >> kvm_destroy_mpidr_data(kvm); >> >> diff --git a/arch/arm64/kvm/dirty_bit.c b/arch/arm64/kvm/dirty_bit.c >> index 002366337637..c5bf866c23ef 100644 >> --- a/arch/arm64/kvm/dirty_bit.c >> +++ b/arch/arm64/kvm/dirty_bit.c >> @@ -112,3 +112,108 @@ int kvm_handle_hdbss_fault(struct kvm_vcpu *vcpu) >> return -EFAULT; >> } >> } >> + >> +static unsigned int hdbss_auto_select_order(struct kvm *kvm) >> +{ >> + unsigned long npages = 0; >> + struct kvm_memory_slot *memslot; >> + int bkt; >> + >> + kvm_for_each_memslot(memslot, bkt, kvm_memslots(kvm)) >> + npages += memslot->npages; >> + >> + if (npages <= 16384) >> + return 0; >> + else if (npages <= 262144) >> + return 3; >> + else if (npages <= 4194304) >> + return 6; >> + else >> + return 9; >> +} > IIUC you are counting the amount of pages the VM has, and based on that > allocating a size for the HDBSS buffer. > > A few notes here: > - It's not really nice to use magic numbers around like this. If you > actually want to use it, then use stuff like SZ_16K, SZ_256K, SZ_4M and > so on. > - You are returning magic numbers as well, why is it 0, 3, 6, or 9 here? > It only makes sense if the person is reading HDBSSBR_EL2 documentation, > which should not be necessary at this point. That's one reason I > recommended to using sizes. If that was really the best way to use it, > I would recommend using the defines that we get from sysreg, and you > actually used before to set the maximum order on a previous patch. > - Also, if you can return only valid values here, why do you check against > the maximum value in that previous patch? > - Also, are you using some undisclosed rule here? On 'order 0' the > meanining is 4KB, which translate to 512 HDBSS entries. Why are you using > it for any value under 16K? Same for 3-32KB-4kEntries you use for under > 256K pages (and so on). If you are assuming a logical rule such as > 'N pages would be ok with N/32 entries' it has to be described here at > least. > - Not sure VM size is the best way of doing that, since it will depend > more on the dirtying rate than the actual size, and most VMs would just > use the biggest size (4M x 4K pages is just 16GB). For instance with > dirty_ring we can use the dirty_ring.size as a better option. > (I know this is a hard one to estimate when using dirty-bitmap, though) > >> + >> +/* >> + * Enable HDBSS for all vCPUs in the VM. >> + * >> + * Called from kvm_arm_hdbss_on_dirty_logging_change() which is invoked >> + * by kvm_arch_commit_memory_region() under kvm->slots_lock. >> + * >> + * If buffer allocation fails, HDBSS remains disabled and dirty tracking >> + * falls back to the traditional software-based approach (PTE write-protect >> + * + software dirty marking). This does not affect correctness; dirty >> + * logging remains functional without HDBSS. >> + */ >> +static int kvm_arm_enable_hdbss_global(struct kvm *kvm) >> +{ >> + int err; >> + unsigned long i; >> + unsigned int order; >> + struct kvm_vcpu *vcpu; >> + >> + if (!system_supports_hdbss()) >> + return 0; >> + >> + if (kvm->dirty_ring_size) /* Don't support HDBSS in dirty ring mode */ >> + return 0; >> + >> + if (kvm->arch.enable_hdbss) /* Already On */ >> + return 0; >> + >> + /* Turn it on */ >> + order = hdbss_auto_select_order(kvm); >> + kvm_for_each_vcpu(i, vcpu, kvm) { >> + err = kvm_arm_vcpu_alloc_hdbss(vcpu, order); >> + if (err) >> + goto error_alloc; >> + } >> + >> + kvm->arch.enable_hdbss = true; >> + kvm->arch.mmu.vtcr |= VTCR_EL2_HD | VTCR_EL2_HDBSS | VTCR_EL2_HA; >> + >> + /* >> + * We should kick vcpus out of guest mode here to load new >> + * vtcr value to vtcr_el2 register when re-enter guest mode. >> + */ >> + kvm_for_each_vcpu(i, vcpu, kvm) >> + kvm_vcpu_kick(vcpu); >> + >> + return 0; >> + >> +error_alloc: >> + kvm_for_each_vcpu(i, vcpu, kvm) >> + if (vcpu->arch.hdbss.base_phys) >> + kvm_arm_vcpu_free_hdbss(vcpu); >> + >> + pr_warn_once("kvm: failed to allocate HDBSS buffers (order=%u), " >> + "falling back to software dirty tracking\n", order); >> + return -ENOMEM; >> +} >> + >> +void kvm_arm_disable_hdbss_global(struct kvm *kvm) >> +{ >> + unsigned long i; >> + struct kvm_vcpu *vcpu; >> + >> + if (!kvm->arch.enable_hdbss) /* Already Off */ >> + return; >> + >> + /* Turn it off */ >> + kvm->arch.mmu.vtcr &= ~(VTCR_EL2_HD | VTCR_EL2_HDBSS | VTCR_EL2_HA); >> + >> + kvm_for_each_vcpu(i, vcpu, kvm) >> + kvm_arm_vcpu_free_hdbss(vcpu); >> + >> + kvm->arch.enable_hdbss = false; >> +} >> + > Okay, say the user requested it to be disabled, you change the global vtcr, > then free the hdbss on every vcpu. > > But the vcpus are still running, and since they will only disable this when > they go out of the guest, then in again, HDBSS will still be running, > right? > > If some page gets dirty in the between, would not the HDBSS try to write to > the already loaded buffer adress, and write to memory that have already > been freed here? > > >> +void kvm_arm_hdbss_on_dirty_logging_change(struct kvm *kvm, int nr_memslots_logging) >> +{ >> + /* >> + * Called from kvm_arch_commit_memory_region() under kvm->slots_lock. >> + * All state transitions are serialized by slots_lock. >> + */ >> + if (nr_memslots_logging > 0 && !kvm->arch.enable_hdbss) >> + kvm_arm_enable_hdbss_global(kvm); >> + else if (nr_memslots_logging == 0 && kvm->arch.enable_hdbss) >> + kvm_arm_disable_hdbss_global(kvm); >> +} >> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c >> index 949fb895add6..484f48dae000 100644 >> --- a/arch/arm64/kvm/mmu.c >> +++ b/arch/arm64/kvm/mmu.c >> @@ -2588,6 +2588,9 @@ void kvm_arch_commit_memory_region(struct kvm *kvm, >> { >> bool log_dirty_pages = new && new->flags & KVM_MEM_LOG_DIRTY_PAGES; >> >> + kvm_arm_hdbss_on_dirty_logging_change(kvm, >> + atomic_read(&kvm->nr_memslots_dirty_logging)); >> + >> /* >> * At this point memslot has been committed and there is an >> * allocated dirty_bitmap[], dirty pages will be tracked while the >> -- >> 2.33.0 >> > Okay, reading the above I remembered something really complicated: > We can't really enable HDBSS partially if we start with DBM set for all > pages. Once we enable HDBSS wit will track changes for all memslots. > > The only way to enable it partially would be to set DBM during the > dirty-bit tracking, which I recall being complicated for some reasons. > > Well, we have to think about the overall strategy before a next version. > > Thanks! > Leo Hi, Leo One thing just clicked for me: we don't actually need per-memslot DBM control. PML on x86 works the same way — D bit set on all writable SPTEs unconditionally, buffer records everything. The filtering happens in software at flush time: vmx_flush_pml_buffer() -> kvm_vcpu_mark_page_dirty() -> mark_page_dirty_in_slot(), where kvm_slot_dirty_track_enabled() discards non-tracked memslots. Our HDBSS does exactly the same: global DBM, buffer records all writes, kvm_flush_hdbss_buffer() -> mark_page_dirty_in_slot() filters at flush time. So I'm questioning the premise — why should HDBSS be held to a different standard than PML? If overhead becomes a real problem, we can revisit per-memslot later. For v5, I think global DBM is fine. So, I think we just need to focus on how to correctly handle DBM for huge pages.