[01/21] KVM: x86/mmu: Implement memslot deletion for TDX

Commit Message

Edgecombe, Rick P Sept. 4, 2024, 3:07 a.m. UTC

Force TDX VMs to use the KVM_X86_QUIRK_SLOT_ZAP_ALL behavior.

TDs cannot use the fast zapping operation to implement memslot deletion for
a couple reasons:
1. KVM cannot fully zap and re-build TDX private PTEs without coordinating
   with the guest. This is due to the TDs needing to "accept" memory. So
   an operation to delete a memslot needs to limit the private zapping to
   the range of the memslot.
2. For reason (1), kvm_mmu_zap_all_fast() is limited to direct (shared)
   roots. This means it will not zap the mirror (private) PTEs. If a
   memslot is deleted with private memory mapped, the private memory would
   remain mapped in the TD. Then if later the gmem fd was whole punched,
   the pages could be freed on the host while still mapped in the TD. This
   is because that operation would no longer have the memslot to map the
   pgoff to the gfn.

To handle the first case, userspace could simply set the
KVM_X86_QUIRK_SLOT_ZAP_ALL quirk for TDs. This would prevent the issue in
(1), but it is not sufficient to resolve (2) because the problems there
extend beyond the userspace's TD, to affecting the rest of the host. So the
zap-leafs-only behavior is required for both

A couple options were considered, including forcing
KVM_X86_QUIRK_SLOT_ZAP_ALL to always be on for TDs, however due to the
currently limited quirks interface (no way to query quirks, or force them
to be disabled), this would require developing additional interfaces. So
instead just do the simple thing and make TDs always do the zap-leafs
behavior like when KVM_X86_QUIRK_SLOT_ZAP_ALL is disabled.

While at it, have the new behavior apply to all non-KVM_X86_DEFAULT_VM VMs,
as the previous behavior was not ideal (see [0]). It is assumed until
proven otherwise that the other VM types will not be exposed to the bug[1]
that derailed that effort.

Memslot deletion needs to zap both the private and shared mappings of a
GFN, so update the attr_filter field in kvm_mmu_zap_memslot_leafs() to
include both.

Co-developed-by: Yan Zhao <[e-mail]>
Signed-off-by: Yan Zhao <[e-mail]>
Signed-off-by: Rick Edgecombe <[e-mail]>
Link: https://lore.kernel.org/kvm/[e-mail]/ [0]
Link: https://patchwork.kernel.org/project/kvm/patch/[e-mail] [1]
---
TDX MMU part 2 v1:
 - Clarify TDX limits on zapping private memory (Sean)

Memslot quirk series:
 - New patch
---
 arch/x86/kvm/mmu/mmu.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Paolo Bonzini Sept. 9, 2024, 1:44 p.m. UTC | #1

On 9/4/24 05:07, Rick Edgecombe wrote:
> Force TDX VMs to use the KVM_X86_QUIRK_SLOT_ZAP_ALL behavior.
> 
> TDs cannot use the fast zapping operation to implement memslot deletion for
> a couple reasons:
> 1. KVM cannot fully zap and re-build TDX private PTEs without coordinating
>     with the guest. This is due to the TDs needing to "accept" memory. So
>     an operation to delete a memslot needs to limit the private zapping to
>     the range of the memslot.
> 2. For reason (1), kvm_mmu_zap_all_fast() is limited to direct (shared)
>     roots. This means it will not zap the mirror (private) PTEs. If a
>     memslot is deleted with private memory mapped, the private memory would
>     remain mapped in the TD. Then if later the gmem fd was whole punched,
>     the pages could be freed on the host while still mapped in the TD. This
>     is because that operation would no longer have the memslot to map the
>     pgoff to the gfn.
> 
> To handle the first case, userspace could simply set the
> KVM_X86_QUIRK_SLOT_ZAP_ALL quirk for TDs. This would prevent the issue in
> (1), but it is not sufficient to resolve (2) because the problems there
> extend beyond the userspace's TD, to affecting the rest of the host. So the
> zap-leafs-only behavior is required for both
> 
> A couple options were considered, including forcing
> KVM_X86_QUIRK_SLOT_ZAP_ALL to always be on for TDs, however due to the
> currently limited quirks interface (no way to query quirks, or force them
> to be disabled), this would require developing additional interfaces. So
> instead just do the simple thing and make TDs always do the zap-leafs
> behavior like when KVM_X86_QUIRK_SLOT_ZAP_ALL is disabled.
> 
> While at it, have the new behavior apply to all non-KVM_X86_DEFAULT_VM VMs,
> as the previous behavior was not ideal (see [0]). It is assumed until
> proven otherwise that the other VM types will not be exposed to the bug[1]
> that derailed that effort.
> 
> Memslot deletion needs to zap both the private and shared mappings of a
> GFN, so update the attr_filter field in kvm_mmu_zap_memslot_leafs() to
> include both.
> 
> Co-developed-by: Yan Zhao <[e-mail]>
> Signed-off-by: Yan Zhao <[e-mail]>
> Signed-off-by: Rick Edgecombe <[e-mail]>
> Link: https://lore.kernel.org/kvm/[e-mail]/ [0]
> Link: https://patchwork.kernel.org/project/kvm/patch/[e-mail] [1]
> ---
> TDX MMU part 2 v1:
>   - Clarify TDX limits on zapping private memory (Sean)
> 
> Memslot quirk series:
>   - New patch
> ---
>   arch/x86/kvm/mmu/mmu.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index a8d91cf11761..7e66d7c426c1 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -7104,6 +7104,7 @@ static void kvm_mmu_zap_memslot_leafs(struct kvm *kvm, struct kvm_memory_slot *s
>   		.start = slot->base_gfn,
>   		.end = slot->base_gfn + slot->npages,
>   		.may_block = true,
> +		.attr_filter = KVM_FILTER_PRIVATE | KVM_FILTER_SHARED,
>   	};
>   	bool flush = false;
>   

Stale commit message, I guess.

Paolo

Edgecombe, Rick P Sept. 9, 2024, 9:06 p.m. UTC | #2

On Mon, 2024-09-09 at 15:44 +0200, Paolo Bonzini wrote:
> 
> Stale commit message, I guess.

Oof, yes. We can lose the KVM_X86_QUIRK_SLOT_ZAP_ALL discussion.

Patch

diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index a8d91cf11761..7e66d7c426c1 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -7104,6 +7104,7 @@  static void kvm_mmu_zap_memslot_leafs(struct kvm *kvm, struct kvm_memory_slot *s
 		.start = slot->base_gfn,
 		.end = slot->base_gfn + slot->npages,
 		.may_block = true,
+		.attr_filter = KVM_FILTER_PRIVATE | KVM_FILTER_SHARED,
 	};
 	bool flush = false;