2022-03-04 20:06:44

by Isaku Yamahata

Subject: [RFC PATCH v5 021/104] KVM: x86: Introduce hooks to free VM callback prezap and vm_free

From: Kai Huang <[email protected]>

Before tearing down private page tables, TDX requires some resources of the
guest TD to be destroyed (i.e. keyID must have been reclaimed, etc). Add
prezap callback before tearing down private page tables for it.

TDX needs to free some resources after other resources (i.e. vcpu related
resources). Add vm_free callback at the end of kvm_arch_destroy_vm().

Signed-off-by: Kai Huang <[email protected]>
Signed-off-by: Isaku Yamahata <[email protected]>
---
arch/x86/include/asm/kvm-x86-ops.h | 2 ++
arch/x86/include/asm/kvm_host.h | 2 ++
arch/x86/kvm/x86.c | 8 ++++++++
3 files changed, 12 insertions(+)

diff --git a/arch/x86/include/asm/kvm-x86-ops.h b/arch/x86/include/asm/kvm-x86-ops.h
index 8125d43d3566..ef48dcc98cfc 100644
--- a/arch/x86/include/asm/kvm-x86-ops.h
+++ b/arch/x86/include/asm/kvm-x86-ops.h
@@ -20,7 +20,9 @@ KVM_X86_OP(has_emulated_msr)
KVM_X86_OP(vcpu_after_set_cpuid)
KVM_X86_OP(is_vm_type_supported)
KVM_X86_OP(vm_init)
+KVM_X86_OP_NULL(mmu_prezap)
KVM_X86_OP_NULL(vm_destroy)
+KVM_X86_OP_NULL(vm_free)
KVM_X86_OP(vcpu_create)
KVM_X86_OP(vcpu_free)
KVM_X86_OP(vcpu_reset)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 8de357a9ad30..5ff7a0fba311 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1326,7 +1326,9 @@ struct kvm_x86_ops {
bool (*is_vm_type_supported)(unsigned long vm_type);
unsigned int vm_size;
int (*vm_init)(struct kvm *kvm);
+ void (*mmu_prezap)(struct kvm *kvm);
void (*vm_destroy)(struct kvm *kvm);
+ void (*vm_free)(struct kvm *kvm);

/* Create, but do not attach this VCPU */
int (*vcpu_create)(struct kvm_vcpu *vcpu);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index f6438750d190..a48f5c69fadb 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -11779,6 +11779,7 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
kvm_page_track_cleanup(kvm);
kvm_xen_destroy_vm(kvm);
kvm_hv_destroy_vm(kvm);
+ static_call_cond(kvm_x86_vm_free)(kvm);
}

static void memslot_rmap_free(struct kvm_memory_slot *slot)
@@ -12036,6 +12037,13 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,

void kvm_arch_flush_shadow_all(struct kvm *kvm)
{
+ /*
+ * kvm_mmu_zap_all() zaps both private and shared page tables. Before
+ * tearing down private page tables, TDX requires some TD resources to
+ * be destroyed (i.e. keyID must have been reclaimed, etc). Invoke
+ * kvm_x86_mmu_prezap() for this.
+ */
+ static_call_cond(kvm_x86_mmu_prezap)(kvm);
kvm_mmu_zap_all(kvm);
}

--
2.25.1

2022-03-31 05:16:33

by Kai Huang

Subject: Re: [RFC PATCH v5 021/104] KVM: x86: Introduce hooks to free VM callback prezap and vm_free

On Fri, 2022-03-04 at 11:48 -0800, [email protected] wrote:
> From: Kai Huang <[email protected]>
>
> Before tearing down private page tables, TDX requires some resources of the
> guest TD to be destroyed (i.e. keyID must have been reclaimed, etc). Add
> prezap callback before tearing down private page tables for it.
>
> TDX needs to free some resources after other resources (i.e. vcpu related
> resources). Add vm_free callback at the end of kvm_arch_destroy_vm().
>
> Signed-off-by: Kai Huang <[email protected]>
> Signed-off-by: Isaku Yamahata <[email protected]>
> ---
> arch/x86/include/asm/kvm-x86-ops.h | 2 ++
> arch/x86/include/asm/kvm_host.h | 2 ++
> arch/x86/kvm/x86.c | 8 ++++++++
> 3 files changed, 12 insertions(+)
>
> diff --git a/arch/x86/include/asm/kvm-x86-ops.h b/arch/x86/include/asm/kvm-x86-ops.h
> index 8125d43d3566..ef48dcc98cfc 100644
> --- a/arch/x86/include/asm/kvm-x86-ops.h
> +++ b/arch/x86/include/asm/kvm-x86-ops.h
> @@ -20,7 +20,9 @@ KVM_X86_OP(has_emulated_msr)
> KVM_X86_OP(vcpu_after_set_cpuid)
> KVM_X86_OP(is_vm_type_supported)
> KVM_X86_OP(vm_init)
> +KVM_X86_OP_NULL(mmu_prezap)
> KVM_X86_OP_NULL(vm_destroy)
> +KVM_X86_OP_NULL(vm_free)
> KVM_X86_OP(vcpu_create)
> KVM_X86_OP(vcpu_free)
> KVM_X86_OP(vcpu_reset)
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 8de357a9ad30..5ff7a0fba311 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1326,7 +1326,9 @@ struct kvm_x86_ops {
> bool (*is_vm_type_supported)(unsigned long vm_type);
> unsigned int vm_size;
> int (*vm_init)(struct kvm *kvm);
> + void (*mmu_prezap)(struct kvm *kvm);
> void (*vm_destroy)(struct kvm *kvm);
> + void (*vm_free)(struct kvm *kvm);
>
> /* Create, but do not attach this VCPU */
> int (*vcpu_create)(struct kvm_vcpu *vcpu);
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index f6438750d190..a48f5c69fadb 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -11779,6 +11779,7 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
> kvm_page_track_cleanup(kvm);
> kvm_xen_destroy_vm(kvm);
> kvm_hv_destroy_vm(kvm);
> + static_call_cond(kvm_x86_vm_free)(kvm);
> }
>
> static void memslot_rmap_free(struct kvm_memory_slot *slot)
> @@ -12036,6 +12037,13 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,
>
> void kvm_arch_flush_shadow_all(struct kvm *kvm)
> {
> + /*
> + * kvm_mmu_zap_all() zaps both private and shared page tables. Before
> + * tearing down private page tables, TDX requires some TD resources to
> + * be destroyed (i.e. keyID must have been reclaimed, etc). Invoke
> + * kvm_x86_mmu_prezap() for this.
> + */
> + static_call_cond(kvm_x86_mmu_prezap)(kvm);
> kvm_mmu_zap_all(kvm);
> }
>

The two callbacks are introduced here but they are actually implemented in 2
patches later (patch 24 KVM: TDX: create/destroy VM structure). Why not just
squash this patch to patch 24? Or at least you can put this patch right before
patch 24.

Please feel free to remove my SoB and From if this bothers you to squash.

2022-04-02 14:21:49

by Isaku Yamahata

Subject: Re: [RFC PATCH v5 021/104] KVM: x86: Introduce hooks to free VM callback prezap and vm_free

On Thu, Mar 31, 2022 at 04:02:24PM +1300,
Kai Huang <[email protected]> wrote:

> On Fri, 2022-03-04 at 11:48 -0800, [email protected] wrote:
> > From: Kai Huang <[email protected]>
> >
> > Before tearing down private page tables, TDX requires some resources of the
> > guest TD to be destroyed (i.e. keyID must have been reclaimed, etc). Add
> > prezap callback before tearing down private page tables for it.
> >
> > TDX needs to free some resources after other resources (i.e. vcpu related
> > resources). Add vm_free callback at the end of kvm_arch_destroy_vm().
> >
> > Signed-off-by: Kai Huang <[email protected]>
> > Signed-off-by: Isaku Yamahata <[email protected]>
> > ---
> > arch/x86/include/asm/kvm-x86-ops.h | 2 ++
> > arch/x86/include/asm/kvm_host.h | 2 ++
> > arch/x86/kvm/x86.c | 8 ++++++++
> > 3 files changed, 12 insertions(+)
> >
> > diff --git a/arch/x86/include/asm/kvm-x86-ops.h b/arch/x86/include/asm/kvm-x86-ops.h
> > index 8125d43d3566..ef48dcc98cfc 100644
> > --- a/arch/x86/include/asm/kvm-x86-ops.h
> > +++ b/arch/x86/include/asm/kvm-x86-ops.h
> > @@ -20,7 +20,9 @@ KVM_X86_OP(has_emulated_msr)
> > KVM_X86_OP(vcpu_after_set_cpuid)
> > KVM_X86_OP(is_vm_type_supported)
> > KVM_X86_OP(vm_init)
> > +KVM_X86_OP_NULL(mmu_prezap)
> > KVM_X86_OP_NULL(vm_destroy)
> > +KVM_X86_OP_NULL(vm_free)
> > KVM_X86_OP(vcpu_create)
> > KVM_X86_OP(vcpu_free)
> > KVM_X86_OP(vcpu_reset)
> > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > index 8de357a9ad30..5ff7a0fba311 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -1326,7 +1326,9 @@ struct kvm_x86_ops {
> > bool (*is_vm_type_supported)(unsigned long vm_type);
> > unsigned int vm_size;
> > int (*vm_init)(struct kvm *kvm);
> > + void (*mmu_prezap)(struct kvm *kvm);
> > void (*vm_destroy)(struct kvm *kvm);
> > + void (*vm_free)(struct kvm *kvm);
> >
> > /* Create, but do not attach this VCPU */
> > int (*vcpu_create)(struct kvm_vcpu *vcpu);
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index f6438750d190..a48f5c69fadb 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -11779,6 +11779,7 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
> > kvm_page_track_cleanup(kvm);
> > kvm_xen_destroy_vm(kvm);
> > kvm_hv_destroy_vm(kvm);
> > + static_call_cond(kvm_x86_vm_free)(kvm);
> > }
> >
> > static void memslot_rmap_free(struct kvm_memory_slot *slot)
> > @@ -12036,6 +12037,13 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,
> >
> > void kvm_arch_flush_shadow_all(struct kvm *kvm)
> > {
> > + /*
> > + * kvm_mmu_zap_all() zaps both private and shared page tables. Before
> > + * tearing down private page tables, TDX requires some TD resources to
> > + * be destroyed (i.e. keyID must have been reclaimed, etc). Invoke
> > + * kvm_x86_mmu_prezap() for this.
> > + */
> > + static_call_cond(kvm_x86_mmu_prezap)(kvm);
> > kvm_mmu_zap_all(kvm);
> > }
> >
>
> The two callbacks are introduced here but they are actually implemented in 2
> patches later (patch 24 KVM: TDX: create/destroy VM structure). Why not just
> squash this patch to patch 24? Or at least you can put this patch right before
> patch 24.

Ok. I'll squash this patch into it.
--
Isaku Yamahata <[email protected]>

2022-04-06 11:55:55

by Paolo Bonzini

Subject: Re: [RFC PATCH v5 021/104] KVM: x86: Introduce hooks to free VM callback prezap and vm_free

On 3/4/22 20:48, [email protected] wrote:
> From: Kai Huang <[email protected]>
>
> Before tearing down private page tables, TDX requires some resources of the
> guest TD to be destroyed (i.e. keyID must have been reclaimed, etc). Add
> prezap callback before tearing down private page tables for it.
>
> TDX needs to free some resources after other resources (i.e. vcpu related
> resources). Add vm_free callback at the end of kvm_arch_destroy_vm().
>
> Signed-off-by: Kai Huang <[email protected]>
> Signed-off-by: Isaku Yamahata <[email protected]>
> ---
> arch/x86/include/asm/kvm-x86-ops.h | 2 ++
> arch/x86/include/asm/kvm_host.h | 2 ++
> arch/x86/kvm/x86.c | 8 ++++++++
> 3 files changed, 12 insertions(+)
>
> diff --git a/arch/x86/include/asm/kvm-x86-ops.h b/arch/x86/include/asm/kvm-x86-ops.h
> index 8125d43d3566..ef48dcc98cfc 100644
> --- a/arch/x86/include/asm/kvm-x86-ops.h
> +++ b/arch/x86/include/asm/kvm-x86-ops.h
> @@ -20,7 +20,9 @@ KVM_X86_OP(has_emulated_msr)
> KVM_X86_OP(vcpu_after_set_cpuid)
> KVM_X86_OP(is_vm_type_supported)
> KVM_X86_OP(vm_init)
> +KVM_X86_OP_NULL(mmu_prezap)
> KVM_X86_OP_NULL(vm_destroy)
> +KVM_X86_OP_NULL(vm_free)
> KVM_X86_OP(vcpu_create)
> KVM_X86_OP(vcpu_free)
> KVM_X86_OP(vcpu_reset)
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 8de357a9ad30..5ff7a0fba311 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1326,7 +1326,9 @@ struct kvm_x86_ops {
> bool (*is_vm_type_supported)(unsigned long vm_type);
> unsigned int vm_size;
> int (*vm_init)(struct kvm *kvm);
> + void (*mmu_prezap)(struct kvm *kvm);
> void (*vm_destroy)(struct kvm *kvm);
> + void (*vm_free)(struct kvm *kvm);
>
> /* Create, but do not attach this VCPU */
> int (*vcpu_create)(struct kvm_vcpu *vcpu);
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index f6438750d190..a48f5c69fadb 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -11779,6 +11779,7 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
> kvm_page_track_cleanup(kvm);
> kvm_xen_destroy_vm(kvm);
> kvm_hv_destroy_vm(kvm);
> + static_call_cond(kvm_x86_vm_free)(kvm);
> }
>
> static void memslot_rmap_free(struct kvm_memory_slot *slot)
> @@ -12036,6 +12037,13 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,
>
> void kvm_arch_flush_shadow_all(struct kvm *kvm)
> {
> + /*
> + * kvm_mmu_zap_all() zaps both private and shared page tables. Before
> + * tearing down private page tables, TDX requires some TD resources to
> + * be destroyed (i.e. keyID must have been reclaimed, etc). Invoke
> + * kvm_x86_mmu_prezap() for this.
> + */
> + static_call_cond(kvm_x86_mmu_prezap)(kvm);
> kvm_mmu_zap_all(kvm);

Please rename the hook to (*flush_shadow_all_private).

Otherwise ok,

Paolo

> }
>