Re: [PATCH v13 22/22] Documentation/x86: Add documentation for TDX host support

[Huang, Kai]

Thanks for reviewing the doc!

> > +Making SEAMCALL requires the CPU already being in VMX operation (VMXON
> > +has been done).  For now both tdx_enable() and tdx_cpu_enable() don't
> > +handle VMXON internally, but depends on the caller to guarantee that.
> Isn't this an implementation detail. It's fine mentioning that TDX 
> requires VMX being enabled but whether it's being handled by current 
> code or not is an implementation details. I think this is better left as 
> a comment in the code rather than in the doc, it will likely quickly go 
> stale.

Both above, and ...
> 
> > +
> > +To enable TDX, the caller of TDX should: 1) hold read lock of CPU hotplug
> 
> nit: Hold CPU hotplug lock in read mode. And again, this is more of an 
> implementation details, the important bit is that cpu hotplug must be 
> blocked while enabling is in progress, no?

... this are used to explain ...
> 
> > +lock; 2) do VMXON and tdx_enable_cpu() on all online cpus successfully;
> > +3) call tdx_enable().  For example::
> > +
> > +        cpus_read_lock();
> > +        on_each_cpu(vmxon_and_tdx_cpu_enable());
> > +        ret = tdx_enable();
> > +        cpus_read_unlock();
> > +        if (ret)
> > +                goto no_tdx;
> > +        // TDX is ready to use

... the pseudo code here, with the purpose to give caller example on how to use.

However I also agree we should make the doc concise as nobody wants to read
boring and lengthy materials.  I personally don't know how to draw the line, so
I can remove all those if that better.  But before I commit to do let's wait for
some more time to hear from others.

> > +
> > +And the caller of TDX must guarantee the tdx_cpu_enable() has been
> > +successfully done on any cpu before it wants to run any other SEAMCALL.
> > +A typical usage is do both VMXON and tdx_cpu_enable() in CPU hotplug
> > +online callback, and refuse to online if tdx_cpu_enable() fails.
> > +
> > +User can consult dmesg to see whether the TDX module has been initialized.
> > +
> > +If the TDX module is initialized successfully, dmesg shows something
> > +like below::
> > +
> > +  [..] tdx: TDX module: attributes 0x0, vendor_id 0x8086, major_version 1, minor_version 0, build_date 20211209, build_num 160
> > +  [..] tdx: 262668 KBs allocated for PAMT.
> > +  [..] tdx: module initialized.
> > +
> > +If the TDX module failed to initialize, dmesg also shows it failed to
> > +initialize::
> > +
> > +  [..] tdx: module initialization failed ...
> 
> nit: You give specific strings which tdx is going to use, of course 
> those can change and will go stale here. Instead, perhaps just 
> mentioning that the dmesg is going to be contains a message signifying 
> error or success.
> > +

Somehow I believe this would be useful, as this message isn't just for developer
(like the pseudo code above), but for the user.  Of course w/o explicitly saying
here, the user should also be able to easily find this particular sentence
(because it's too obvious), but I guess providing it here would make it even
easier for the user.  Also, I am not expecting this particular message to be
changed soon or frequently.

Again, I am quite open to all of these, but let's some more time :-)