On Tue, 2022-07-26 at 17:39 -0700, Isaku Yamahata wrote: > On Tue, Jul 12, 2022 at 01:13:10PM +1200, > Kai Huang <kai.huang@xxxxxxxxx> wrote: > > > > To use TDX functionality, TDX module needs to be loaded and initialized. > > > This patch is to call a function, tdx_init(), when loading kvm_intel.ko. > > > > Could you add explain why we need to init TDX module when loading KVM module? > > Makes sense. Added a paragraph for it. > > > > > Add a hook, kvm_arch_post_hardware_enable_setup, to module initialization > > > while hardware is enabled, i.e. after hardware_enable_all() and before > > > hardware_disable_all(). Because TDX requires all present CPUs to enable > > > VMX (VMXON). > > > > Please explicitly say it is a replacement of the default __weak version, so > > people can know there's already a default one. Otherwise people may wonder why > > this isn't called in this patch (i.e. I skipped patch 03 as it looks not > > directly related to TDX). > > > > That being said, why cannot you send out that patch separately but have to > > include it into TDX series? > > > > Looking at it, the only thing that is related to TDX is an empty > > kvm_arch_post_hardware_enable_setup() with a comment saying TDX needs to do > > something there. This logic has nothing to do with the actual job in that > > patch. > > > > So why cannot we introduce that __weak version in this patch, so that the rest > > of it can be non-TDX related at all and can be upstreamed separately? > > The patch that adds weak kvm_arch_post_hardware_enable_setup() doesn't make > sense without the hook because it only enable_hardware and then disable hardware > immediately. It's not a disaster if you describe the reason to do so in the changelog, but no strong opinion here. But I do think you need a comment to explain why disable hardware is called immediately. Is it because we want to maintain the current behaviour that we want to allow out-of-tree driver, i.e. virtualbox to be loaded when KVM is loaded? > The patch touches multiple kvm arch. and I split out TDX specific part in this > patch. Ideally those two patch should be near. But I move it early to draw > attention for reviewers from multiple kvm arch. Explicitly say this is the replacement of the default __weak version is fine. > > Here is the updated version. > > KVM: TDX: Initialize the TDX module when loading the KVM intel kernel module > > To use TDX, the TDX module needs to be loaded and initialized. This patch > is to call a function to initialize the TDX module when loading KVM intel > kernel module. > > There are several options on when to initialize the TDX module. A.) > kernel boot time as builtin, B.) kernel module loading time, C.) the first > guest TD creation time. B.) was chosen. A.) causes unnecessary overhead > (boot time and memory) even when TDX isn't used. With C.), a user may hit > an error of the TDX initialization when trying to create the first guest > TD. The machine that fails to initialize the TDX module can't boot any > guest TD further. Such failure is undesirable. B.) has a good balance > between them. You don't need to mention A. When this patch is merged, the host series must have been merged already. In another words, this is already a fact, but not an option. > > Add a hook, kvm_arch_post_hardware_enable_setup, to module initialization > while hardware is enabled, i.e. after hardware_enable_all() and before > hardware_disable_all(). > You don't need to say "add a hook ..., i.e. after hardware_enable_all() and before hardware_disable_all()". Where the function is called is already a fact. We have a __weak version already. > Because TDX requires all present CPUs to enable > VMX (VMXON). The x86 specific kvm_arch_post_hardware_enable_setup overrides > the existing weak symbol of kvm_arch_post_hardware_enable_setup which is > called at the KVM module initialization. >
