On Thu, Oct 31, 2024, Kai Huang wrote:
> On Wed, 2024-10-30 at 08:19 -0700, Sean Christopherson wrote:
> > > +void __init tdx_bringup(void)
> > > +{
> > > + enable_tdx = enable_tdx && !__tdx_bringup();
> >
> > Ah. I don't love this approach because it mixes "failure" due to an unsupported
> > configuration, with failure due to unexpected issues. E.g. if enabling virtualization
> > fails, loading KVM-the-module absolutely should fail too, not simply disable TDX.
>
> Thanks for the comments.
>
> I see your point. However for "enabling virtualization failure" kvm_init() will
> also try to do (default behaviour), so if it fails it will result in module
> loading failure eventually. So while I guess it would be slightly better to
> make module loading fail if "enabling virtualization fails" in TDX, it is a nit
> issue to me.
>
> I think "enabling virtualization failure" is the only "unexpected issue" that
> should result in module loading failure. For any other TDX-specific
> initialization failure (e.g., any memory allocation in future patches) it's
> better to only disable TDX.
I disagree. The platform owner wants TDX to be enabled, KVM shouldn't silently
disable TDX because of a transient, unrelated failure.
If TDX _can't_ be supported, e.g. because EPT or MMIO SPTE caching was explicitly
disable, then that's different. And that's the general pattern throughout KVM.
If a requested feature isn't supported, then KVM continues on updates the module
param accordingly. But if something outright fails during setup, KVM aborts the
entire sequence.
> So I can change to "make loading KVM-the-module fail if enabling virtualization
> fails in TDX", but I want to confirm this is what you want?
I would prefer the logic to be: reject loading kvm-intel.ko if an operation that
would normally succeed, fails.
