On Tue, May 20, 2025, Kai Huang wrote:
> On Tue, 2025-05-20 at 16:11 -0700, Sean Christopherson wrote:
> > On Tue, May 20, 2025, Kai Huang wrote:
> > > On Mon, 2025-05-19 at 08:39 -0700, Sean Christopherson wrote:
> > > > +static int tdx_sept_remove_private_spte(struct kvm *kvm, gfn_t gfn,
> > > > + enum pg_level level, kvm_pfn_t pfn)
> > > > {
> > > > struct page *page = pfn_to_page(pfn);
> > > > int ret;
> > > > @@ -3507,10 +3507,14 @@ int __init tdx_bringup(void)
> > > > r = __tdx_bringup();
> > > > if (r) {
> > > > /*
> > > > - * Disable TDX only but don't fail to load module if
> > > > - * the TDX module could not be loaded. No need to print
> > > > - * message saying "module is not loaded" because it was
> > > > - * printed when the first SEAMCALL failed.
> > > > + * Disable TDX only but don't fail to load module if the TDX
> > > > + * module could not be loaded. No need to print message saying
> > > > + * "module is not loaded" because it was printed when the first
> > > > + * SEAMCALL failed. Don't bother unwinding the S-EPT hooks or
> > > > + * vm_size, as kvm_x86_ops have already been finalized (and are
> > > > + * intentionally not exported). The S-EPT code is unreachable,
> > > > + * and allocating a few more bytes per VM in a should-be-rare
> > > > + * failure scenario is a non-issue.
> > > > */
> > > > if (r == -ENODEV)
> > > > goto success_disable_tdx;
> > > > @@ -3524,3 +3528,19 @@ int __init tdx_bringup(void)
> > > > enable_tdx = 0;
> > > > return 0;
> > > > }
> > > > +
> > > > +
> > > > +void __init tdx_hardware_setup(void)
> > > > +{
> > > > + /*
> > > > + * Note, if the TDX module can't be loaded, KVM TDX support will be
> > > > + * disabled but KVM will continue loading (see tdx_bringup()).
> > > > + */
> > >
> > > This comment seems a little bit weird to me. I think what you meant here is the
> > > @vm_size and those S-EPT ops are not unwound while TDX cannot be brought up but
> > > KVM is still loaded.
> >
> > This comment is weird? Or the one in tdx_bringup() is weird?
> >
>
> I definitely agree tdx_bringup() is weird :-)
>
> > The sole intent of _this_ comment is to clarify that KVM could still end up
> > running load with TDX disabled.
> >
>
> But this behaviour itself doesn't mean anything,
I disagree. The overwhelming majority of code in KVM expects that either the
associated feature will be fully enabled, or KVM will abort the overall flow,
e.g. refuse to load, fail vCPU/VM creation, etc.
Continuing on is very exceptional IMO, and warrants a comment.
> e.g., if we export kvm_x86_ops, we could unwind them.
Maaaybe. I mean, yes, we could fully unwind kvm_x86_ops, but doing so would make
the overall code far more brittle. E.g. simply updating kvm_x86_ops won't suffice,
as the static_calls also need to be patched, and we would have to be very careful
not to touch anything in kvm_x86_ops that might have been consumed between here
and the call to tdx_bringup().
> So without mentioning "those are not unwound", it doesn't seem useful to me.
>
> But it does have "(see tdx_bringup())" at the end, so OK to me. I guess I just
> wish it could be more verbose.
Yeah, redirecting to another comment isn't a great experience for readers, but I
don't want to duplicate the explanation and details because that risks creating
stale and/or contradicting comments in the future, and in general increases the
maintenance cost (small though it should be in this case).
