Re: [RFC PATCH v3 26/59] KVM: x86: Introduce vm_teardown() hook in kvm_arch_vm_destroy()

From: Sean Christopherson <seanjc@google.com>
To: Thomas Gleixner <tglx@linutronix.de>
Cc: Paolo Bonzini <pbonzini@redhat.com>,
	isaku.yamahata@intel.com, Ingo Molnar <mingo@redhat.com>,
	Borislav Petkov <bp@alien8.de>, "H . Peter Anvin" <hpa@zytor.com>,
	Vitaly Kuznetsov <vkuznets@redhat.com>,
	Wanpeng Li <wanpengli@tencent.com>,
	Jim Mattson <jmattson@google.com>, Joerg Roedel <joro@8bytes.org>,
	erdemaktas@google.com, Connor Kuehl <ckuehl@redhat.com>,
	linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
	isaku.yamahata@gmail.com,
	Sean Christopherson <sean.j.christopherson@intel.com>
Subject: Re: [RFC PATCH v3 26/59] KVM: x86: Introduce vm_teardown() hook in kvm_arch_vm_destroy()
Date: Mon, 29 Nov 2021 18:16:30 +0000	[thread overview]
Message-ID: <YaUY/vlFX22DFhsE@google.com> (raw)
In-Reply-To: <8735nkhre8.ffs@tglx>

On Thu, Nov 25, 2021, Thomas Gleixner wrote:
> On Thu, Nov 25 2021 at 21:54, Paolo Bonzini wrote:
> > On 11/25/21 20:46, Thomas Gleixner wrote:
> >> On Wed, Nov 24 2021 at 16:20, isaku yamahata wrote:
> >>> Add a second kvm_x86_ops hook in kvm_arch_vm_destroy() to support TDX's
> >>> destruction path, which needs to first put the VM into a teardown state,
> >>> then free per-vCPU resource, and finally free per-VM resources.
> >>>
> >>> Note, this knowingly creates a discrepancy in nomenclature for SVM as
> >>> svm_vm_teardown() invokes avic_vm_destroy() and sev_vm_destroy().
> >>> Moving the now-misnamed functions or renaming them is left to a future
> >>> patch so as not to introduce a functional change for SVM.
> >> That's just the wrong way around. Fixup SVM first and then add the TDX
> >> muck on top. Stop this 'left to a future patch' nonsense. I know for
> >> sure that those future patches never materialize.
> >
> > Or just keep vm_destroy for the "early" destruction, and give a new name 
> > to the new hook.  It is used to give back the TDCS memory, so perhaps 
> > you can call it vm_free?
> 
> Up to you, but the current approach is bogus. I rather go for a fully
> symmetric interface and let the various incarnations opt in at the right
> place. Similar to what cpu hotplug states are implementing.

Naming aside, that's what is being done, TDX simply needs two hooks instead of one
due to the way KVM handles VM and vCPU destruction.  The alternative would be to
shove and duplicate what is currently common x86 code into VMX/SVM, which IMO is
far worse.

Regarding the naming, I 100% agree SVM should be refactored prior to adding TDX
stuff if we choose to go with vm_teardown() and vm_destroy() instead of Paolo's
suggestion of vm_destroy() and vm_free().  When this patch/code was originally
written, letting SVM become stale was a deliberate choice to reduce conflicts with
upstream as we knew the code would live out of tree for quite some time.  But that
was purely meant to be development "hack", not upstream behavior.