On Wed, 2023-04-05 at 13:07 -0700, Isaku Yamahata wrote: > On Wed, Mar 29, 2023 at 06:04:38PM -0700, > Isaku Yamahata <isaku.yamahata@xxxxxxxxx> wrote: > > > On Wed, Mar 29, 2023 at 11:17:31PM +0000, > > "Huang, Kai" <kai.huang@xxxxxxxxx> wrote: > > > > > On Wed, 2023-03-29 at 14:56 -0700, Isaku Yamahata wrote: > > > > On Wed, Mar 29, 2023 at 01:13:45AM +0000, > > > > "Huang, Kai" <kai.huang@xxxxxxxxx> wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > + /* > > > > > > > > + * TDX requires those methods to enable VMXON by > > > > > > > > + * kvm_hardware_enable/disable_all() > > > > > > > > + */ > > > > > > > > + static_call_update(kvm_x86_check_processor_compatibility, > > > > > > > > + ops->runtime_ops->check_processor_compatibility); > > > > > > > > + static_call_update(kvm_x86_hardware_enable, > > > > > > > > + ops->runtime_ops->hardware_enable); > > > > > > > > + static_call_update(kvm_x86_hardware_disable, > > > > > > > > + ops->runtime_ops->hardware_disable); > > > > > > > > r = ops->hardware_setup(); > > > > > > > > if (r != 0) > > > > > > > > goto out_mmu_exit; > > > > > > > > > > > > > > Hmm.. I think this is ugly. Perhaps we should never do any > > > > > > > static_call(kvm_x86_xxx)() in hardware_setup(), because hardware_setup() is > > > > > > > called before kvm_ops_update() and may update vendor's kvm_x86_ops. > > > > > > > > > > > > > > So probably use hardware_enable_all() in hardware_setup() is a bad idea. > > > > > > > > > > > > > > I think we have below options on how to handle: > > > > > > > > > > > > > > 1) Use VMX's kvm_x86_ops directly in tdx_hardware_setup(). For instance, > > > > > > > something like below: > > > > > > > > > > > > > > int __init tdx_hardware_setup(struct kvm_x86_ops *x86_ops) > > > > > > > { > > > > > > > ... > > > > > > > > > > > > > > cpus_read_lock(); > > > > > > > r = on_each_cpu(vt_x86_ops.hardware_enable, ...); > > > > > > > if (!r) > > > > > > > r = tdx_module_setup(); > > > > > > > on_each_cpu(vt_x86_ops.hardware_disable, ...); > > > > > > > cpus_read_unlock(); > > > > > > > > > > > > > > ... > > > > > > > } > > > > > > > > > > > > > > But this doesn't clean up nicely when there's some particular cpus fail to do > > > > > > > hardware_enable(). To clean up nicely, we do need additional things similar to > > > > > > > the hardware_enable_all() code path: a per-cpu variable or a cpumask_t + a > > > > > > > wrapper of vt_x86_ops->hardware_enable() to track which cpus have done > > > > > > > hardware_enable() successfully. > > > > > > > > > > > > > > 2) Move those static_call_update() into tdx_hardware_setup() so they are TDX > > > > > > > code self-contained. But this would require exposing kvm_x86_ops as symbol, > > > > > > > which isn't nice either. > > > > > > > > > > > > > > 3) Introduce another kvm_x86_init_ops->hardware_post_setup(), which is called > > > > > > > after kvm_ops_update(). > > > > > > > > > > > > > > Personally, I think 3) perhaps is the most elegant one, but not sure whether > > > > > > > Sean/Paolo has any opinion. > > > > > > > > > > > > I think we can simply update the ops before calling hardware_enable() and > > > > > > clean up ops on failure. > > > > > > > > > > > > > > > > > > > > > > This doesn't work because hardware_setup() may update vendor's kvm_x86_ops. > > > > > > > > > > If you do kvm_ops_update() before hardware_setup(), you need to manually update > > > > > those updated (in hardware_setup()) callbacks again after. > > > > > > > > We can call kvm_ops_update() twice before and after hardware_setup(). > > > > > > > > > > Personally I think it's too ugly. > > > > So you prefer the option 3 to calling kvm_ops_update() twice. Okay, I'll update > > the patch. > > After playing with hardware_post_setup(), it's inevitable to call > kvm_ops_update() twice. > When VMX initialization succeeded with hardware_setup(), but TDX initialization > with hardware_post_setup() failed, we'd like to support only VMX with warning > message. In such case, we need to revert x86_ops to VMX only. > It doesn't make sense to introduce hardware_post_setup() to avoid calling > kvm_update_ops twice. > OK. Then how about option 1) ? We just need another wrapper around vt_x86_ops.hardware_{enable|disable}() and use a VT's own per-cpu variable to track which has cpu has done the vmx_hardware_enable(). We can even put the per-cpu variable inside the vt_hardware_enable() itself w/o introducing the wrapper. But again it's better if Sean can input here.