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.
