Re: [PATCH v5 07/22] x86/virt/tdx: Implement SEAMCALL function

[Kai Huang]

On Mon, 2022-06-27 at 13:58 -0700, Dave Hansen wrote:
> On 6/26/22 22:23, Kai Huang wrote:
> > On Fri, 2022-06-24 at 11:38 -0700, Dave Hansen wrote:
> > > On 6/22/22 04:16, Kai Huang wrote:
> > > > SEAMCALL instruction causes #GP when SEAMRR isn't enabled, and #UD when
> > > > CPU is not in VMX operation.  The TDX_MODULE_CALL macro doesn't handle
> > > > SEAMCALL exceptions.  Leave to the caller to guarantee those conditions
> > > > before calling __seamcall().
> > > 
> > > I was trying to make the argument earlier that you don't need *ANY*
> > > detection for TDX, other than the ability to make a SEAMCALL.
> > > Basically, patch 01/22 could go away.
> ...
> > > So what does patch 01/22 buy us?  One EXTABLE entry?
> > 
> > There are below pros if we can detect whether TDX is enabled by BIOS during boot
> > before initializing the TDX Module:
> > 
> > 1) There are requirements from customers to report whether platform supports TDX
> > and the TDX keyID numbers before initializing the TDX module so the userspace
> > cloud software can use this information to do something.  Sorry I cannot find
> > the lore link now.
> 
> <sigh>
> 
> Never listen to customers literally.  It'll just lead you down the wrong
> path.  They told you, "we need $FOO in dmesg" and you ran with it
> without understanding why.  The fact that you even *need* to find the
> lore link is because you didn't bother to realize what they really needed.
> 
> dmesg is not ABI.  It's for humans.  If you need data out of the kernel,
> do it with a *REAL* ABI.  Not dmesg.

Showing in the dmesg is the first step, but later we have plan to expose keyID
info via /sysfs.  Of course, it's always arguable customer's such requirement is
absolutely needed, but to me it's still a good thing to have code to detect TDX
during boot.  The code isn't complicated as you can see.

> 
> > 2) As you can see, it can be used to handle ACPI CPU/memory hotplug and driver
> > managed memory hotplug.  Kexec() support patch also can use it.
> > 
> > Particularly, in concept, ACPI CPU/memory hotplug is only related to whether TDX
> > is enabled by BIOS, but not whether TDX module is loaded, or the result of
> > initializing the TDX module.  So I think we should have some code to detect TDX
> > during boot.
> 
> This is *EXACTLY* why our colleagues at Intel needs to tell us about
> what the OS and firmware should do when TDX is in varying states of decay.

Yes I am working on it to make it public.

> 
> Does the mere presence of the TDX module prevent hotplug?  
> 

For ACPI CPU hotplug, yes.  The TDX module even doesn't need to be loaded. 
Whether SEAMRR is enabled determines.

For ACPI memory hotplug, in practice yes.  For architectural behaviour, I'll
work with others internally to get some public statement.

> Or, if a
> system has the TDX module loaded but no intent to ever use TDX, why
> can't it just use hotplug like a normal system which is not addled with
> the TDX albatross around its neck?

I think if a machine has enabled TDX in the BIOS, the user of the machine very
likely has intention to actually use TDX.

Yes for driver-managed memory hotplug, it makes sense if user doesn't want to
use TDX, it's better to not disable it.  But to me it's also not a disaster if
we just disable driver-managed memory hotplug if TDX is enabled by BIOS.

For ACPI memory hotplug, I think in practice we can treat it as BIOS bug, but
I'll get some public statement around this.

> 
> > Also, it seems adding EXTABLE to TDX_MODULE_CALL doesn't have significantly less
> > code comparing to detecting TDX during boot:
> 
> It depends on a bunch of things.  It might only be a line or two of
> assembly.
> 
> If you actually went and tried it, you might be able to convince me it's
> a bad idea.

The code I showed is basically the patch we need to call SEAMCALL at runtime w/o
detecting TDX at first.  #GP must be handled as it is what SEAMCALL triggers if
TDX is not enabled.  #UD happens when CPU isn't in VMX operation, and we should
distinguish it from #GP if we already want to handle #GP.


-- 
Thanks,
-Kai