[PATCH v7 00/20] TDX host kernel support

On Tue, 2022-11-22 at 20:14 +0100, Peter Zijlstra wrote:

On Tue, Nov 22, 2022 at 10:57:52AM -0800, Dave Hansen wrote:

quoted

To me, this starts to veer way too far into internal implementation details.

	Issue the TDH.SYS.LP.SHUTDOWN SEAMCALL on all BIOS-enabled CPUs
	to shut down the TDX module.

We really need to let go of the whole 'all BIOS-enabled CPUs' thing.

As replied in another email I'll follow up with TDX module guys to see whether
we can remove/ease such limitation.

Thanks!

On Tue, 2022-11-22 at 10:06 +0100, Peter Zijlstra wrote:

On Mon, Nov 21, 2022 at 01:26:27PM +1300, Kai Huang wrote:

quoted

+/*
+ * Wrapper of __seamcall() to convert SEAMCALL leaf function error code
+ * to kernel error code.  @seamcall_ret and @out contain the SEAMCALL
+ * leaf function return code and the additional output respectively if
+ * not NULL.
+ */
+static int __always_unused seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9,
+				    u64 *seamcall_ret,
+				    struct tdx_module_output *out)
+{

What's the point of a 'static __always_unused' function again? Other
than to test the DCE pass of a linker, that is?

It is used to avoid the compile warning as so far with this patch it doesn't
have any caller.  Without the __always_unused, the compiler will complain.

Originally it was in the patch "Shut down TDX module in case of error" where it
was firstly called.  Dave suggested to move it out:

https://lore.kernel.org/all/6ed2746d-f44c-4511-7373-5706dd7c3f0f@intel.com/

On Tue, Nov 22, 2022 at 04:21:38PM -0800, Dave Hansen wrote:

quoted

+	/*
+	 * All memory regions that can be used by the TDX module must be
+	 * passed to the TDX module during the module initialization.
+	 * Once this is done, all "TDX-usable" memory regions are fixed
+	 * during module's runtime.
+	 *
+	 * The initial support of TDX guests only allocates memory from
+	 * the global page allocator.  To keep things simple, for now
+	 * just make sure all pages in the page allocator are TDX memory.
+	 *
+	 * To achieve this, use all system memory in the core-mm at the
+	 * time of initializing the TDX module as TDX memory, and at the
+	 * meantime, reject any new memory in memory hot-add.
+	 *
+	 * This works as in practice, all boot-time present DIMM is TDX
+	 * convertible memory.  However if any new memory is hot-added
+	 * before initializing the TDX module, the initialization will
+	 * fail due to that memory is not covered by CMR.
+	 *
+	 * This can be enhanced in the future, i.e. by allowing adding or
+	 * onlining non-TDX memory to a separate node, in which case the
+	 * "TDX-capable" nodes and the "non-TDX-capable" nodes can exist
+	 * together -- the userspace/kernel just needs to make sure pages
+	 * for TDX guests must come from those "TDX-capable" nodes.
+	 *
+	 * Build the list of TDX memory regions as mentioned above so
+	 * they can be passed to the TDX module later.
+	 */

This is msotly Documentation/, not a code comment.  Please clean it up.

So personally, I *vastly* prefer code comments over this Documentation/
cesspit. Putting things in Documentation/ is a bit like an
old-folks-home, neatly out of the way to (bit)rot in peace.

And that whole .rst disease is making it unreadable for anybody that
still knows how to use a text editor :-(

On Tue, 2022-11-22 at 19:31 +0000, Sean Christopherson wrote:

On Tue, Nov 22, 2022, Peter Zijlstra wrote:

quoted

On Tue, Nov 22, 2022 at 04:06:25PM +0100, Thomas Gleixner wrote:

quoted

On Tue, Nov 22 2022 at 10:20, Peter Zijlstra wrote:

quoted

On Mon, Nov 21, 2022 at 01:26:28PM +1300, Kai Huang wrote:

quoted

Shutting down the TDX module requires calling TDH.SYS.LP.SHUTDOWN on all
BIOS-enabled CPUs, and the SEMACALL can run concurrently on different
CPUs.  Implement a mechanism to run SEAMCALL concurrently on all online
CPUs and use it to shut down the module.  Later logical-cpu scope module
initialization will use it too.

Uhh, those requirements ^ are not met by this:

  Can run concurrently != Must run concurrently
 
The documentation clearly says "can run concurrently" as quoted above.

The next sentense says: "Implement a mechanism to run SEAMCALL
concurrently" -- it does not.

Anyway, since we're all in agreement there is no such requirement at
all, a schedule_on_each_cpu() might be more appropriate, there is no
reason to use IPIs and spin-waiting for any of this.

Backing up a bit, what's the reason for _any_ of this?  The changelog says

  It's pointless to leave the TDX module in some middle state.

but IMO it's just as pointless to do a shutdown unless the kernel benefits in
some meaningful way.  And IIUC, TDH.SYS.LP.SHUTDOWN does nothing more than change
the SEAM VMCS.HOST_RIP to point to an error trampoline.  E.g. it's not like doing
a shutdown lets the kernel reclaim memory that was gifted to the TDX module.

The metadata memory has been freed back to the kernel in case of error before
shutting down the module.

In other words, this is just a really expensive way of changing a function pointer,
and the only way it would ever benefit the kernel is if there is a kernel bug that
leads to trying to use TDX after a fatal error.  And even then, the only difference
seems to be that subsequent bogus SEAMCALLs would get a more unique error message.

The only issue of leaving module open is like you said bogus SEAMCALLs can still
be made.  There's a slight risk that those SEAMCALLs may actually be able to do
something that may crash the kernel (i.e. if the module is shut down due to
TDH.SYS.INIT.TDMR error, then further bogus TDH.SYS.INIT.TDMR can still corrupt
the metadata).

But, this belongs to "kernel bug" or "kernel is under attack" category.  Neither
of them should be a concern of TDX: host kernel is out of TCB, and preventing
DoS attack is not part of TDX anyway.

Also, in case of kexec(), we cannot shut down the module either (in this
implementation, due to CPU may not be in VMX operation when kexec()).

So I agree with you, it's fine to not shut down the module.

Hi maintainers, does this look good to you?

On Tue, 2022-11-22 at 10:05 -0800, Dave Hansen wrote:

On 11/20/22 16:26, Kai Huang wrote:

quoted

2) It is more flexible to support TDX module runtime updating in the
future (after updating the TDX module, it needs to be initialized
again).

I hate this generic blabber about "more flexible".  There's a *REASON*
it's more flexible, so let's talk about the reasons, please.

It's really something like this, right?

	The TDX module design allows it to be updated while the system
	is running.  The update procedure shares quite a few steps with
	this "on demand" loading mechanism.  The hope is that much of
	this "on demand" mechanism can be shared with a future "update"
	mechanism.  A boot-time TDX module implementation would not be
	able to share much code with the update mechanism.

Yes.  Thanks.

quoted

3) It avoids having to do a "temporary" solution to handle VMXON in the
core (non-KVM) kernel for now.  This is because SEAMCALL requires CPU
being in VMX operation (VMXON is done), but currently only KVM handles
VMXON.  Adding VMXON support to the core kernel isn't trivial.  More
importantly, from long-term a reference-based approach is likely needed
in the core kernel as more kernel components are likely needed to
support TDX as well.  Allow KVM to initialize the TDX module avoids
having to handle VMXON during kernel boot for now.

There are a lot of words in there.

3) Loading the TDX module requires VMX to be enabled.  Currently, only
   the kernel KVM code mucks with VMX enabling.  If the TDX module were
   to be initialized separately from KVM (like at boot), the boot code
   would need to be taught how to muck with VMX enabling and KVM would
   need to be taught how to cope with that.  Making KVM itself
   responsible for TDX initialization lets the rest of the kernel stay
   blissfully unaware of VMX.

Thanks.

quoted

Add a placeholder tdx_enable() to detect and initialize the TDX module
on demand, with a state machine protected by mutex to support concurrent
calls from multiple callers.

As opposed to concurrent calls from one caller? ;)

How about below?

"
Add a placeholder tdx_enable() to initialize the TDX module on demand.  So far
KVM will be the only caller, but other kernel components will need to use it too
in the future.  Just use a mutex protected state machine to make sure the module
initialization can only be done once.
"

quoted

The TDX module will be initialized in multi-steps defined by the TDX
module:

  1) Global initialization;
  2) Logical-CPU scope initialization;
  3) Enumerate the TDX module capabilities and platform configuration;
  4) Configure the TDX module about TDX usable memory ranges and global
     KeyID information;
  5) Package-scope configuration for the global KeyID;
  6) Initialize usable memory ranges based on 4).

This would actually be a nice place to call out the SEAMCALL names and
mention that each of these steps involves a set of SEAMCALLs.

How about below?

"
The TDX module will be initialized in multi-steps defined by the TDX module and
each of those steps involves a specific SEAMCALL:
  1) Global initialization using TDH.SYS.INIT.
  2) Logical-CPU scope initialization using TDH.SYS.LP.INIT.
  3) Enumerate the TDX module capabilities and TDX-capable memory information 
     using TDH.SYS.INFO.
  4) Configure the TDX module with TDX-usable memory regions and the global
     KeyID information using TDH.SYS.CONFIG.
  5) Package-scope configuration for the global KeyID using TDH.SYS.KEY.CONFIG.
  6) Initialize TDX-usable memory regions using TDH.SYS.TDMR.INIT.

Before step 4), the kernel needs to build a set of TDX-usable memory regions,
and construct data structures to cover those regions.
"

quoted

The TDX module can also be shut down at any time during its lifetime.
In case of any error during the initialization process, shut down the
module.  It's pointless to leave the module in any intermediate state
during the initialization.

Both logical CPU scope initialization and shutting down the TDX module
require calling SEAMCALL on all boot-time present CPUs.  For simplicity
just temporarily disable CPU hotplug during the module initialization.

You might want to more precisely define "boot-time present CPUs".  The
boot of *what*?

How about use "BIOS-enabled CPUs" instead? If OK I'll use it consistently across
this series.

quoted

diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
index 8d943bdc8335..28c187b8726f 100644
--- a/arch/x86/virt/vmx/tdx/tdx.c
+++ b/arch/x86/virt/vmx/tdx/tdx.c

@@ -10,15 +10,34 @@
 #include <linux/types.h>
 #include <linux/init.h>
 #include <linux/printk.h>
+#include <linux/mutex.h>
+#include <linux/cpu.h>
+#include <linux/cpumask.h>
 #include <asm/msr-index.h>
 #include <asm/msr.h>
 #include <asm/apic.h>
 #include <asm/tdx.h>
 #include "tdx.h"
 
+/* TDX module status during initialization */
+enum tdx_module_status_t {
+	/* TDX module hasn't been detected and initialized */
+	TDX_MODULE_UNKNOWN,
+	/* TDX module is not loaded */
+	TDX_MODULE_NONE,
+	/* TDX module is initialized */
+	TDX_MODULE_INITIALIZED,
+	/* TDX module is shut down due to initialization error */
+	TDX_MODULE_SHUTDOWN,
+};

Are these part of the ABI or just a purely OS-side construct?

Purely OS-side construct.  I'll explicitly call out in the comment.

quoted

 static u32 tdx_keyid_start __ro_after_init;
 static u32 tdx_keyid_num __ro_after_init;
 
+static enum tdx_module_status_t tdx_module_status;
+/* Prevent concurrent attempts on TDX detection and initialization */
+static DEFINE_MUTEX(tdx_module_lock);
+
 /*
  * Detect TDX private KeyIDs to see whether TDX has been enabled by the
  * BIOS.  Both initializing the TDX module and running TDX guest require

@@ -104,3 +123,134 @@ bool platform_tdx_enabled(void)
 {
 	return !!tdx_keyid_num;
 }
+
+/*
+ * Detect and initialize the TDX module.
+ *
+ * Return -ENODEV when the TDX module is not loaded, 0 when it
+ * is successfully initialized, or other error when it fails to
+ * initialize.
+ */
+static int init_tdx_module(void)
+{
+	/* The TDX module hasn't been detected */
+	return -ENODEV;
+}
+
+static void shutdown_tdx_module(void)
+{
+	/* TODO: Shut down the TDX module */
+}
+
+static int __tdx_enable(void)
+{
+	int ret;
+
+	/*
+	 * Initializing the TDX module requires doing SEAMCALL on all
+	 * boot-time present CPUs.  For simplicity temporarily disable
+	 * CPU hotplug to prevent any CPU from going offline during
+	 * the initialization.
+	 */
+	cpus_read_lock();
+
+	/*
+	 * Check whether all boot-time present CPUs are online and
+	 * return early with a message so the user can be aware.
+	 *
+	 * Note a non-buggy BIOS should never support physical (ACPI)
+	 * CPU hotplug when TDX is enabled, and all boot-time present
+	 * CPU should be enabled in MADT, so there should be no
+	 * disabled_cpus and num_processors won't change at runtime
+	 * either.
+	 */

Again, there are a lot of words in that comment, but I'm not sure why
it's here.  Despite all the whinging about ACPI, doesn't it boil down to:

	The TDX module itself establishes its own concept of how many
	logical CPUs there are in the system when it is loaded.  

This isn't accurate.  TDX MCHECK records the total number of logical CPUs when
the BIOS enables TDX.  This happens before the TDX module is loaded.  In fact
the TDX module only gets this information from a secret location.

The
	module will reject initialization attempts unless the kernel
	runs TDX initialization code on every last CPU.

	Ensure that the kernel is able to run code on all known logical
	CPUs.

How about:

	TDX itself establishes its own concept of how many logical CPUs there 
	are in the system when it gets enabled by the BIOS.  The module will 
	reject initialization attempts unless the kernel runs TDX
initialization 
	code on every last CPU.

	Ensure that the kernel is able to run code on all known logical CPUs.

and these checks are just to see if the kernel has shot itself in the
foot and is *KNOWS* that it is currently unable to run code on some
logical CPU?

Yes.

quoted

+	if (disabled_cpus || num_online_cpus() != num_processors) {
+		pr_err("Unable to initialize the TDX module when there's offline CPU(s).\n");
+		ret = -EINVAL;
+		goto out;
+	}
+
+	ret = init_tdx_module();
+	if (ret == -ENODEV) {

Why check for -ENODEV exclusively?  Is there some other error nonzero
code that indicates success?

The idea is to print out "TDX module not loaded" to separate it from other
errors, so that the user can get a better idea when something goes wrong.

quoted

+		pr_info("TDX module is not loaded.\n");
+		tdx_module_status = TDX_MODULE_NONE;
+		goto out;
+	}
+
+	/*
+	 * Shut down the TDX module in case of any error during the
+	 * initialization process.  It's meaningless to leave the TDX
+	 * module in any middle state of the initialization process.
+	 *
+	 * Shutting down the module also requires doing SEAMCALL on all
+	 * MADT-enabled CPUs.  Do it while CPU hotplug is disabled.
+	 *
+	 * Return all errors during the initialization as -EFAULT as the
+	 * module is always shut down.
+	 */
+	if (ret) {
+		pr_info("Failed to initialize TDX module. Shut it down.\n");

"Shut it down" seems wrong here.  That could be interpreted as "I have
already shut it down".  "Shutting down" seems better.

Will change to "Shutting down" if we still want to keep the shut down patch
(please see my another reply to Sean).

quoted

+		shutdown_tdx_module();
+		tdx_module_status = TDX_MODULE_SHUTDOWN;
+		ret = -EFAULT;
+		goto out;
+	}
+
+	pr_info("TDX module initialized.\n");
+	tdx_module_status = TDX_MODULE_INITIALIZED;
+out:
+	cpus_read_unlock();
+
+	return ret;
+}
+
+/**
+ * tdx_enable - Enable TDX by initializing the TDX module
+ *
+ * Caller to make sure all CPUs are online and in VMX operation before
+ * calling this function.  CPU hotplug is temporarily disabled internally
+ * to prevent any cpu from going offline.

"cpu" or "CPU"?

quoted

+ * This function can be called in parallel by multiple callers.
+ *
+ * Return:
+ *
+ * * 0:		The TDX module has been successfully initialized.
+ * * -ENODEV:	The TDX module is not loaded, or TDX is not supported.
+ * * -EINVAL:	The TDX module cannot be initialized due to certain
+ *		conditions are not met (i.e. when not all MADT-enabled
+ *		CPUs are not online).
+ * * -EFAULT:	Other internal fatal errors, or the TDX module is in
+ *		shutdown mode due to it failed to initialize in previous
+ *		attempts.
+ */

I honestly don't think all these error codes mean anything.  They're
plumbed nowhere and the use of -EFAULT is just plain wrong.

Nobody can *DO* anything with these anyway.

Just give one error code and make sure that you have pr_info()'s around
to make it clear what went wrong.  Then just do -EINVAL universally.
Remove all the nonsense comments.

OK. 

quoted

+int tdx_enable(void)
+{
+	int ret;
+
+	if (!platform_tdx_enabled())
+		return -ENODEV;
+
+	mutex_lock(&tdx_module_lock);
+
+	switch (tdx_module_status) {
+	case TDX_MODULE_UNKNOWN:
+		ret = __tdx_enable();
+		break;
+	case TDX_MODULE_NONE:
+		ret = -ENODEV;
+		break;

TDX_MODULE_NONE should probably be called TDX_MODULE_NOT_LOADED.  A
comment would also be nice:

	/* The BIOS did not load the module.  No way to fix that. */

quoted

+	case TDX_MODULE_INITIALIZED:

		/* Already initialized, great, tell the caller: */

Will do.

quoted

+		ret = 0;
+		break;
+	default:
+		WARN_ON_ONCE(tdx_module_status != TDX_MODULE_SHUTDOWN);
+		ret = -EFAULT;
+		break;
+	}

I don't get what that default: is for or what it has to do with
TDX_MODULE_SHUTDOWN.

I meant we can only have 4 possible status, and the default case must be the
TDX_MODULE_SHUTDOWN state.

I think I can just remove that WARN()?

quoted

+	mutex_unlock(&tdx_module_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(tdx_enable);

On Tue, 2022-11-22 at 10:20 -0800, Dave Hansen wrote:

On 11/20/22 16:26, Kai Huang wrote:

quoted

TDX introduces a new CPU mode: Secure Arbitration Mode (SEAM).  This
mode runs only the TDX module itself or other code to load the TDX
module.

The host kernel communicates with SEAM software via a new SEAMCALL
instruction.  This is conceptually similar to a guest->host hypercall,
except it is made from the host to SEAM software instead.

The TDX module defines a set of SEAMCALL leaf functions to allow the
host to initialize it, and to create and run protected VMs.  SEAMCALL
leaf functions use an ABI different from the x86-64 system-v ABI.
Instead, they share the same ABI with the TDCALL leaf functions.

I may have suggested this along the way, but the mention of the sysv ABI
is just confusing here.  This is enough for a changelog:

	The TDX module establishes a new SEAMCALL ABI which allows the
	host to initialize the module and to and to manage VMs.

Kill the rest.

Thanks will do.

quoted

Implement a function __seamcall() to allow the host to make SEAMCALL
to SEAM software using the TDX_MODULE_CALL macro which is the common
assembly for both SEAMCALL and TDCALL.

In general, I dislike mentioning function names in changelogs.  Keep
this high-level, like:

	Add infrastructure to make SEAMCALLs.  The SEAMCALL ABI is very
	similar to the TDCALL ABI and leverages much TDCALL
	infrastructure.

Will do.

quoted

SEAMCALL instruction causes #GP when SEAMRR isn't enabled, and #UD when
CPU is not in VMX operation.  The current TDX_MODULE_CALL macro doesn't
handle any of them.  There's no way to check whether the CPU is in VMX
operation or not.

What is SEAMRR?

Sorry it is a leftover.  Should be "when TDX isn't enabled".

Why even mention this behavior in the changelog.  Is this a problem?
Does it have a solution?

My intention was to provide some background information why to extend
TDX_MODULE_CALL macro to handle #UD and #GP as mentioned below.

quoted

Initializing the TDX module is done at runtime on demand, and it depends
on the caller to ensure CPU is in VMX operation before making SEAMCALL.
To avoid getting Oops when the caller mistakenly tries to initialize the
TDX module when CPU is not in VMX operation, extend the TDX_MODULE_CALL
macro to handle #UD (and also #GP, which can theoretically still happen
when TDX isn't actually enabled by the BIOS, i.e. due to BIOS bug).

I'm not completely sure this is worth it.  If the BIOS lies, we oops.
There are lots of ways that the BIOS lying can make the kernel oops.
What's one more?

I agree.  But if we want to handle #UD, then #GP won't cause oops any more, so I
just added error code for #GP too.

Or perhaps we can change to below: ?

"... extend the TDX_MODULE_CALL to handle #UD (and opportunistically #GP since
they share the same assembly)."

Or other suggestions?

quoted

Introduce two new TDX error codes for #UD and #GP respectively so the
caller can distinguish.  Also, Opportunistically put the new TDX error
codes and the existing TDX_SEAMCALL_VMFAILINVALID into INTEL_TDX_HOST
Kconfig option as they are only used when it is on.

As __seamcall() can potentially return multiple error codes, besides the
actual SEAMCALL leaf function return code, also introduce a wrapper
function seamcall() to convert the __seamcall() error code to the kernel
error code, so the caller doesn't need to duplicate the code to check
return value of __seamcall() and return kernel error code accordingly.

[...]

quoted

+/*
+ * Wrapper of __seamcall() to convert SEAMCALL leaf function error code
+ * to kernel error code.  @seamcall_ret and @out contain the SEAMCALL
+ * leaf function return code and the additional output respectively if
+ * not NULL.
+ */
+static int __always_unused seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9,
+				    u64 *seamcall_ret,
+				    struct tdx_module_output *out)
+{
+	u64 sret;
+
+	sret = __seamcall(fn, rcx, rdx, r8, r9, out);
+
+	/* Save SEAMCALL return code if caller wants it */
+	if (seamcall_ret)
+		*seamcall_ret = sret;
+
+	/* SEAMCALL was successful */
+	if (!sret)
+		return 0;
+
+	switch (sret) {
+	case TDX_SEAMCALL_GP:
+		/*
+		 * platform_tdx_enabled() is checked to be true
+		 * before making any SEAMCALL.
+		 */

This doesn't make any sense.  "platform_tdx_enabled() is checked"???

Do you mean that it *should* be checked and probably wasn't which is
what caused the error?

I meant tdx_enable() already calls platform_tdx_enabled() to check whether BIOS
has enabled TDX at the very beginning before making any SEAMCALL, so
theoretically #GP should not happen unless there's BIOS bug.  I thought a WARN()
can help to catch.

quoted

+		WARN_ON_ONCE(1);
+		fallthrough;
+	case TDX_SEAMCALL_VMFAILINVALID:
+		/* Return -ENODEV if the TDX module is not loaded. */
+		return -ENODEV;

Pro tip: you don't need to rewrite code in comments.  If the code
literally says, "return -ENODEV", there is very little value in writing
virtually identical bytes "Return -ENODEV" in the comment.

Indeed.  Thanks for the tip!  I'll update those comments.

Kai!

On Wed, Nov 23 2022 at 00:30, Kai Huang wrote:

On Tue, 2022-11-22 at 21:03 +0100, Thomas Gleixner wrote:

quoted

Clearly that's nowhere spelled out in the documentation, but I don't
buy the 'architecturaly required' argument not at all. It's an
implementation detail of the TDX module.

I agree on hardware level there shouldn't be such requirement (not 100% sure
though), but I guess from kernel's perspective, "the implementation detail of
the TDX module" is sort of "architectural requirement"

Sure, but then it needs to be clearly documented so.

-- at least Intel arch guys think so I guess.

Intel "arch" guys? You might look up the meanings of "arch" in a
dictionary. LKML is not twatter.

quoted

Technically there is IMO ZERO requirement to do so.

 1) The TDX module is global

 2) Seam-root and Seam-non-root operation are strictly a LP property.

    The only architectural prerequisite for using Seam on a LP is that
    obviously the encryption/decryption mechanics have been initialized
    on the package to which the LP belongs.

I can see why it might be complicated to add/remove an LP after
initialization fact, but technically it should be possible.

"kernel soft offline" actually isn't an issue.  We can bring down a logical cpu
after it gets initialized and then bring it up again.

That's the whole point where this discussion started: _AFTER_ it gets
initialized.

Which means that, e.g. adding "nosmt" to the kernel command line will
make TDX fail hard because at the point where TDX is initialized the
hyperthreads are not 'soft' online and cannot respond to anything, but
the BIOS already accounted them.

This is just wrong as we all know that "nosmt" is sadly one of the
obvious counter measures for the never ending flood of speculation
issues.

Only add/removal of physical cpu will cause problem: 

You wish. 

TDX MCHECK verifies all boot-time present cpus to make sure they are TDX-
compatible before it enables TDX in hardware.  MCHECK cannot run on hot-added
CPU, so TDX cannot support physical CPU hotplug.

TDX can rightfully impose the limitation that it only executes on CPUs,
which are known at boot/initialization time, and only utilizes "known"
memory. That's it, but that does not enforce to prevent physical hotplug
in general.

We tried to get it clarified in the specification, and below is what TDX/module
arch guys agreed to put to the TDX module spec (just checked it's not in latest
public spec yet, but they said it will be in next release):

"
4.1.3.2.  CPU Configuration

During platform boot, MCHECK verifies all logical CPUs to ensure they
meet TDX’s

That MCHECK falls into the category of security voodoo.

It needs to verify _ALL_ logical CPUs to ensure that Intel did not put
different models and steppings into a package or what?

security and certain functionality requirements, and MCHECK passes the following
CPU configuration information to the NP-SEAMLDR, P-SEAMLDR and the TDX Module:

·         Total number of logical processors in the platform.

You surely need MCHECK for this

·         Total number of installed packages in the platform.

and for this...

·         A table of per-package CPU family, model and stepping etc.
identification, as enumerated by CPUID(1).EAX.
The above information is static and does not change after platform boot and
MCHECK run.

Note:     TDX doesn’t support adding or removing CPUs from TDX security
perimeter, as checked my MCHECK.

More security voodoo. The TDX security perimeter has nothing to do with
adding or removing CPUs on a system. If that'd be true then TDX is a
complete fail.

BIOS should prevent CPUs from being hot-added or hot-removed after
platform boots.

If the BIOS does not prevent it, then TDX and the Seam module will not
even notice unless the OS tries to invoke seamcall() on a newly plugged
CPU.

A newly added CPU and newly added memory should not have any impact on
the TDX integrity of the already running system. If they have then
again, TDX is broken by design.

The TDX module performs additional checks of the CPU’s configuration and
supported features, by reading MSRs and CPUID information as described in the
following sections.

to ensure that the MCHECK generated table is still valid at the point
where TDX is initialized? 

That said, this documentation is at least better than the existing void,
but that does not make it technically correct.

Thanks,

        tglx

On Tue, 2022-11-22 at 15:39 -0800, Dave Hansen wrote:

On 11/20/22 16:26, Kai Huang wrote:

quoted

TDX provides increased levels of memory confidentiality and integrity.
This requires special hardware support for features like memory
encryption and storage of memory integrity checksums.  Not all memory
satisfies these requirements.

As a result, TDX introduced the concept of a "Convertible Memory Region"
(CMR).  During boot, the firmware builds a list of all of the memory
ranges which can provide the TDX security guarantees.  The list of these
ranges, along with TDX module information, is available to the kernel by
querying the TDX module via TDH.SYS.INFO SEAMCALL.

I think the last sentence goes too far.  What does it matter what the
name of the SEAMCALL is?  Who cares at this point?  It's in the patch.
Scroll down two pages if you really care.

I'll remove "via TDH.SYS.INFO SEAMCALL".

quoted

The host kernel can choose whether or not to use all convertible memory
regions as TDX-usable memory.  Before the TDX module is ready to create
any TDX guests, the kernel needs to configure the TDX-usable memory
regions by passing an array of "TD Memory Regions" (TDMRs) to the TDX
module.  Constructing the TDMR array requires information of both the
TDX module (TDSYSINFO_STRUCT) and the Convertible Memory Regions.  Call
TDH.SYS.INFO to get this information as a preparation.

That last sentece is kinda goofy.  I think there's a way to distill this
whole thing down more effecively.

	CMRs tell the kernel which memory is TDX compatible.  The kernel
	takes CMRs and constructs  "TD Memory Regions" (TDMRs).  TDMRs
	let the kernel grante TDX protections to some or all of the CMR
	areas.

Will do.

But it seems we should still mention "Constructing TDMRs requires information of
both the TDX module (TDSYSINFO_STRUCT) and the CMRs"?  The reason is to justify
"use static to avoid having to pass them as function arguments when constructing
TDMRs" below.

quoted

Use static variables for both TDSYSINFO_STRUCT and CMR array to avoid

I find it very useful to be precise when referring to code.  Your code
says 'tdsysinfo_struct', yet this says 'TDSYSINFO_STRUCT'.  Why the
difference?

Here I actually didn't intend to refer to any code.  In the above paragraph
(that is going to be replaced with yours), I mentioned "TDSYSINFO_STRUCT" to
explain what does "information of the TDX module" actually refer to, since
TDSYSINFO_STRUCT is used in the spec. 

What's your preference?

quoted

having to pass them as function arguments when constructing the TDMR
array.  And they are too big to be put to the stack anyway.  Also, KVM
needs to use the TDSYSINFO_STRUCT to create TDX guests.

This is also a great place to mention that the tdsysinfo_struct contains
a *lot* of gunk which will not be used for a bit or that may never get
used.

Perhaps below?

"Note many members in tdsysinfo_struct' are not used by the kernel".

Btw, may I ask why does it matter?

[...]

quoted

+
+/* Check CMRs reported by TDH.SYS.INFO, and trim tail empty CMRs. */
+static int trim_empty_cmrs(struct cmr_info *cmr_array, int *actual_cmr_num)
+{
+	struct cmr_info *cmr;
+	int i, cmr_num;
+
+	/*
+	 * Intel TDX module spec, 20.7.3 CMR_INFO:
+	 *
+	 *   TDH.SYS.INFO leaf function returns a MAX_CMRS (32) entry
+	 *   array of CMR_INFO entries. The CMRs are sorted from the
+	 *   lowest base address to the highest base address, and they
+	 *   are non-overlapping.
+	 *
+	 * This implies that BIOS may generate invalid empty entries
+	 * if total CMRs are less than 32.  Need to skip them manually.
+	 *
+	 * CMR also must be 4K aligned.  TDX doesn't trust BIOS.  TDX
+	 * actually verifies CMRs before it gets enabled, so anything
+	 * doesn't meet above means kernel bug (or TDX is broken).
+	 */

I dislike comments like this that describe all the code below.  Can't
you simply put the comment near the code that implements it?

Will do.

quoted

+	cmr = &cmr_array[0];
+	/* There must be at least one valid CMR */
+	if (WARN_ON_ONCE(is_cmr_empty(cmr) || !is_cmr_ok(cmr)))
+		goto err;
+
+	cmr_num = *actual_cmr_num;
+	for (i = 1; i < cmr_num; i++) {
+		struct cmr_info *cmr = &cmr_array[i];
+		struct cmr_info *prev_cmr = NULL;
+
+		/* Skip further empty CMRs */
+		if (is_cmr_empty(cmr))
+			break;
+
+		/*
+		 * Do sanity check anyway to make sure CMRs:
+		 *  - are 4K aligned
+		 *  - don't overlap
+		 *  - are in address ascending order.
+		 */
+		if (WARN_ON_ONCE(!is_cmr_ok(cmr)))
+			goto err;

Why does cmr_array[0] get a pass on the empty and sanity checks?

TDX MCHECK verifies CMRs before enabling TDX, so there must be at least one
valid CMR.

And cmr_array[0] is checked before this loop.

quoted

+		prev_cmr = &cmr_array[i - 1];
+		if (WARN_ON_ONCE((prev_cmr->base + prev_cmr->size) >
+					cmr->base))
+			goto err;
+	}
+
+	/* Update the actual number of CMRs */
+	*actual_cmr_num = i;

That comment is not helpful.  Yes, this is literally updating the number
of CMRs.  Literally.  That's the "what".  But, the "why" is important.
Why is it doing this?

When building the list of "TDX-usable" memory regions, the kernel verifies those
regions against CMRs to see whether they are truly convertible memory.

How about adding a comment like below:

	/*
	 * When the kernel builds the TDX-usable memory regions, it verifies
	 * they are truly convertible memory by checking them against CMRs.
	 * Update the actual number of CMRs to skip those empty CMRs.
	 */

Also, I think printing CMRs in the dmesg is helpful.  Printing empty (zero) CMRs
will put meaningless log to the dmesg.
 

quoted

+	/* Print kernel checked CMRs */
+	print_cmrs(cmr_array, *actual_cmr_num, "Kernel-checked-CMR");

This is the point where I start to lose patience with these comments.
These are just a waste of space.

Sorry will remove.

Also, I saw the loop above check 'cmr_num' CMRs for is_cmr_ok().  Now,
it'll print an 'actual_cmr_num=1' number of CMRs as being
"kernel-checked".  Why?  That makes zero sense.

The loop quits when it sees an empty CMR.  I think there's no need to check
further CMRs as they must be empty (TDX MCHECK verifies CMRs).

quoted

+	return 0;
+err:
+	pr_info("[TDX broken ?]: Invalid CMRs detected\n");
+	print_cmrs(cmr_array, cmr_num, "BIOS-CMR");
+	return -EINVAL;
+}
+
+static int tdx_get_sysinfo(void)
+{
+	struct tdx_module_output out;
+	int ret;
+
+	BUILD_BUG_ON(sizeof(struct tdsysinfo_struct) != TDSYSINFO_STRUCT_SIZE);
+
+	ret = seamcall(TDH_SYS_INFO, __pa(&tdx_sysinfo), TDSYSINFO_STRUCT_SIZE,
+			__pa(tdx_cmr_array), MAX_CMRS, NULL, &out);
+	if (ret)
+		return ret;
+
+	/* R9 contains the actual entries written the CMR array. */
+	tdx_cmr_num = out.r9;
+
+	pr_info("TDX module: atributes 0x%x, vendor_id 0x%x, major_version %u, minor_version %u, build_date %u, build_num %u",
+		tdx_sysinfo.attributes, tdx_sysinfo.vendor_id,
+		tdx_sysinfo.major_version, tdx_sysinfo.minor_version,
+		tdx_sysinfo.build_date, tdx_sysinfo.build_num);

This is a case where a little bit of vertical alignment will go a long way:

quoted

+		tdx_sysinfo.attributes,    tdx_sysinfo.vendor_id,
+		tdx_sysinfo.major_version, tdx_sysinfo.minor_version,
+		tdx_sysinfo.build_date,    tdx_sysinfo.build_num);

Thanks will do.

quoted

+
+	/*
+	 * trim_empty_cmrs() updates the actual number of CMRs by
+	 * dropping all tail empty CMRs.
+	 */
+	return trim_empty_cmrs(tdx_cmr_array, &tdx_cmr_num);
+}

Why does this both need to respect the "tdx_cmr_num = out.r9" value
*and* trim the empty ones?  Couldn't it just ignore the "tdx_cmr_num =
out.r9" value and just trim the empty ones either way?  It's not like
there is a billion of them.  It would simplify the code for sure.

OK.  Since spec says MAX_CMRs is 32, so I can use 32 instead of reading out from
R9.

[...]

quoted

+struct cpuid_config {
+	u32	leaf;
+	u32	sub_leaf;
+	u32	eax;
+	u32	ebx;
+	u32	ecx;
+	u32	edx;
+} __packed;
+
+#define TDSYSINFO_STRUCT_SIZE		1024
+#define TDSYSINFO_STRUCT_ALIGNMENT	1024
+
+struct tdsysinfo_struct {
+	/* TDX-SEAM Module Info */
+	u32	attributes;
+	u32	vendor_id;
+	u32	build_date;
+	u16	build_num;
+	u16	minor_version;
+	u16	major_version;
+	u8	reserved0[14];
+	/* Memory Info */
+	u16	max_tdmrs;
+	u16	max_reserved_per_tdmr;
+	u16	pamt_entry_size;
+	u8	reserved1[10];
+	/* Control Struct Info */
+	u16	tdcs_base_size;
+	u8	reserved2[2];
+	u16	tdvps_base_size;
+	u8	tdvps_xfam_dependent_size;
+	u8	reserved3[9];
+	/* TD Capabilities */
+	u64	attributes_fixed0;
+	u64	attributes_fixed1;
+	u64	xfam_fixed0;
+	u64	xfam_fixed1;
+	u8	reserved4[32];
+	u32	num_cpuid_config;
+	/*
+	 * The actual number of CPUID_CONFIG depends on above
+	 * 'num_cpuid_config'.  The size of 'struct tdsysinfo_struct'
+	 * is 1024B defined by TDX architecture.  Use a union with
+	 * specific padding to make 'sizeof(struct tdsysinfo_struct)'
+	 * equal to 1024.
+	 */
+	union {
+		struct cpuid_config	cpuid_configs[0];
+		u8			reserved5[892];
+	};

Can you double check what the "right" way to do variable arrays is these
days?  I thought the [0] method was discouraged.

Also, it isn't *really* 892 bytes of reserved space, right?  Anything
that's not cpuid_configs[] is reserved, I presume.  Could you try to be
more precise there?

I'll do some study first here and get back to you.  Thanks.

The intention is to make sure the structure size is 1024B, so that the static
variable will have enough space for the TDX module to write.

On Tue, 2022-11-22 at 11:14 -0800, Dave Hansen wrote:

On 11/20/22 16:26, Kai Huang wrote:

quoted

The first step of initializing the module is to call TDH.SYS.INIT once
on any logical cpu to do module global initialization.  Do the module
global initialization.

It also detects the TDX module, as seamcall() returns -ENODEV when the
module is not loaded.

Part of making a good patch set is telling a bit of a story.  In patch
4, you laid out 6 steps necessary to initialize TDX.  On top of that,
there is infrastructure  It would be great to lay that out in a way that
folks can actually follow along.

For instance, it would be great to tell the reader here that this patch
is an inflection point.  It is transitioning out of the infrastructure
(patches 1->6) and into the actual "multi-steps" of initialization that
the module spec requires.

This patch is *TOTALLY* different from the one before it because it
actually _starts_ to do something useful.

But, you wouldn't know it from the changelog.

I'll try to enhance the changelog to make them more connected.  Right now I
don't have a clear clue on how to write in best way.

quoted

 arch/x86/virt/vmx/tdx/tdx.c | 19 +++++++++++++++++--
 arch/x86/virt/vmx/tdx/tdx.h |  1 +
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
index 5db1a05cb4bd..f292292313bd 100644
--- a/arch/x86/virt/vmx/tdx/tdx.c
+++ b/arch/x86/virt/vmx/tdx/tdx.c

@@ -208,8 +208,23 @@ static void seamcall_on_each_cpu(struct seamcall_ctx *sc)
  */
 static int init_tdx_module(void)
 {
-	/* The TDX module hasn't been detected */
-	return -ENODEV;
+	int ret;
+
+	/*
+	 * Call TDH.SYS.INIT to do the global initialization of
+	 * the TDX module.  It also detects the module.
+	 */
+	ret = seamcall(TDH_SYS_INIT, 0, 0, 0, 0, NULL, NULL);
+	if (ret)
+		goto out;

Please also note that the 0's are all just unused parameters.  They mean
nothing.

Will add to the comment.

quoted

+
+	/*
+	 * Return -EINVAL until all steps of TDX module initialization
+	 * process are done.
+	 */
+	ret = -EINVAL;
+out:
+	return ret;
 }

It might be a bit unconventional, but can you imagine how well it would
tell the story if this comment said:

	/*
	 * TODO:
	 *  - Logical-CPU scope initialization (TDH_SYS_INIT_LP)
	 *  - Enumerate capabilities and platform configuration
	      (TDH_SYS_CONFIG)
	 ...
	 */

and then each of the following patches that *did* those things removed
the TODO line from the list.

That TODO list could have been added in patch 4.

Thanks for suggestion.  Will do.

I think I can do this to "construct TDMRs" related patches too.

On Wed, 2022-11-23 at 12:05 +0100, Thomas Gleixner wrote:

Kai!

On Wed, Nov 23 2022 at 00:30, Kai Huang wrote:

quoted

On Tue, 2022-11-22 at 21:03 +0100, Thomas Gleixner wrote:

quoted

Clearly that's nowhere spelled out in the documentation, but I don't
buy the 'architecturaly required' argument not at all. It's an
implementation detail of the TDX module.

I agree on hardware level there shouldn't be such requirement (not 100% sure
though), but I guess from kernel's perspective, "the implementation detail of
the TDX module" is sort of "architectural requirement"

Sure, but then it needs to be clearly documented so.

quoted

-- at least Intel arch guys think so I guess.

Intel "arch" guys? You might look up the meanings of "arch" in a
dictionary. LKML is not twatter.

quoted

quoted

Technically there is IMO ZERO requirement to do so.

 1) The TDX module is global

 2) Seam-root and Seam-non-root operation are strictly a LP property.

    The only architectural prerequisite for using Seam on a LP is that
    obviously the encryption/decryption mechanics have been initialized
    on the package to which the LP belongs.

I can see why it might be complicated to add/remove an LP after
initialization fact, but technically it should be possible.

"kernel soft offline" actually isn't an issue.  We can bring down a logical cpu
after it gets initialized and then bring it up again.

That's the whole point where this discussion started: _AFTER_ it gets
initialized.

Which means that, e.g. adding "nosmt" to the kernel command line will
make TDX fail hard because at the point where TDX is initialized the
hyperthreads are not 'soft' online and cannot respond to anything, but
the BIOS already accounted them.

This is just wrong as we all know that "nosmt" is sadly one of the
obvious counter measures for the never ending flood of speculation
issues.

Agree.  As said in my other replies, I'll follow up with TDX module guys on
this.

quoted

Only add/removal of physical cpu will cause problem: 

You wish. 

quoted

TDX MCHECK verifies all boot-time present cpus to make sure they are TDX-
compatible before it enables TDX in hardware.  MCHECK cannot run on hot-added
CPU, so TDX cannot support physical CPU hotplug.

TDX can rightfully impose the limitation that it only executes on CPUs,
which are known at boot/initialization time, and only utilizes "known"
memory. That's it, but that does not enforce to prevent physical hotplug
in general.

Adding physical CPUs should have no problem I guess, they just cannot run TDX. 
TDX architecture doesn't expect they can run TDX code anyway.

But would physical removal of boot-time present CPU cause problem? TDX MCHECK
checks/records boot-time present CPUs.  If a CPU is removed and then a new one
is added, then TDX still treats it is TDX-compatible, but it may actually not.

So if this happens, from functionality's point of view, it can break.  I think
TDX still wants to guarantee TDX code can work correctly on "TDX recorded" CPUs.

Also, I am not sure whether there's any security issue if a malicious kernel
tries to run TDX code on such removed-then-added CPU.

This seems a TDX architecture problem rather than kernel policy issue.

quoted

We tried to get it clarified in the specification, and below is what TDX/module
arch guys agreed to put to the TDX module spec (just checked it's not in latest
public spec yet, but they said it will be in next release):

"
4.1.3.2.  CPU Configuration

During platform boot, MCHECK verifies all logical CPUs to ensure they
meet TDX’s

That MCHECK falls into the category of security voodoo.

It needs to verify _ALL_ logical CPUs to ensure that Intel did not put
different models and steppings into a package or what?

I am guessing so.

quoted

security and certain functionality requirements, and MCHECK passes the following
CPU configuration information to the NP-SEAMLDR, P-SEAMLDR and the TDX Module:

·         Total number of logical processors in the platform.

You surely need MCHECK for this

quoted

·         Total number of installed packages in the platform.

and for this...

quoted

·         A table of per-package CPU family, model and stepping etc.
identification, as enumerated by CPUID(1).EAX.
The above information is static and does not change after platform boot and
MCHECK run.

Note:     TDX doesn’t support adding or removing CPUs from TDX security
perimeter, as checked my MCHECK.

More security voodoo. The TDX security perimeter has nothing to do with
adding or removing CPUs on a system. If that'd be true then TDX is a
complete fail.

No argument here.

quoted

BIOS should prevent CPUs from being hot-added or hot-removed after
platform boots.

If the BIOS does not prevent it, then TDX and the Seam module will not
even notice unless the OS tries to invoke seamcall() on a newly plugged
CPU.

A newly added CPU and newly added memory should not have any impact on
the TDX integrity of the already running system. If they have then
again, TDX is broken by design.

No argument here either.

quoted

The TDX module performs additional checks of the CPU’s configuration and
supported features, by reading MSRs and CPUID information as described in the
following sections.

to ensure that the MCHECK generated table is still valid at the point
where TDX is initialized? 

I think it is trying to say:

MCHECK doesn't do all the verifications. Some verfications are deferred to the
TDX module to check when it gets initialized.

That said, this documentation is at least better than the existing void,
but that does not make it technically correct.

Thanks,

        tglx

On Wed, Nov 23, 2022, Huang, Kai wrote:

On Tue, 2022-11-22 at 17:04 -0800, Dave Hansen wrote:

quoted

On 11/22/22 16:58, Huang, Kai wrote:

quoted

On Tue, 2022-11-22 at 11:24 -0800, Dave Hansen wrote:

quoted

quoted

I was expecting TDX to not get initialized until the first TDX using KVM
instance is created. Am I wrong?

I went looking for it in this series to prove you wrong.  I failed.  😄

tdx_enable() is buried in here somewhere:

quoted

https://lore.kernel.org/lkml/CAAhR5DFrwP+5K8MOxz5YK7jYShhaK4A+2h1Pi31U_9+Z+cz-0A@mail.gmail.com/T/

I don't have the patience to dig it out today, so I guess we'll have Kai
tell us.

It will be done when KVM module is loaded, but not when the first TDX guest is
created.

Why is it done that way?

Can it be changed to delay TDX initialization until the first TDX guest
needs to run?

Sean suggested.

Hi Sean, could you commenet?

Waiting until the first TDX guest is created would result in false advertising,
as KVM wouldn't know whether or not TDX is actually supported until that first
VM is created.  If we can guarantee that TDH.SYS.INIT will fail if and only if
there is a kernel bug, then I would be ok deferring the "enabling" until the
first VM is created.

On 11/23/22 08:20, Sean Christopherson wrote:

quoted

quoted

Why is it done that way?

Can it be changed to delay TDX initialization until the first TDX guest
needs to run?

Sean suggested.

Hi Sean, could you commenet?

Waiting until the first TDX guest is created would result in false advertising,
as KVM wouldn't know whether or not TDX is actually supported until that first
VM is created.  If we can guarantee that TDH.SYS.INIT will fail if and only if
there is a kernel bug, then I would be ok deferring the "enabling" until the
first VM is created.

There's no way we can guarantee _that_.  For one, the PAMT* allocations
can always fail.  I guess we could ask sysadmins to fire up a guest to
"prime" things, but that seems a little silly.  Maybe that would work as
the initial implementation that we merge, but I suspect our users will
demand more determinism, maybe a boot or module parameter.

 * Physical Address Metadata Table, a large physically contiguous data
   structure, the rough equivalent of 'struct page' for the TDX module

On 11/23/22 03:40, Huang, Kai wrote:

On Tue, 2022-11-22 at 15:39 -0800, Dave Hansen wrote:

quoted

That last sentece is kinda goofy.  I think there's a way to distill this
whole thing down more effecively.

      CMRs tell the kernel which memory is TDX compatible.  The kernel
      takes CMRs and constructs  "TD Memory Regions" (TDMRs).  TDMRs
      let the kernel grant TDX protections to some or all of the CMR
      areas.

Will do.

But it seems we should still mention "Constructing TDMRs requires information of
both the TDX module (TDSYSINFO_STRUCT) and the CMRs"?  The reason is to justify
"use static to avoid having to pass them as function arguments when constructing
TDMRs" below.

In a changelog, no.  You do *NOT* use super technical language in
changelogs if not super necessary.  Mentioning "TDSYSINFO_STRUCT" here
is useless.  The *MOST* you would do for a good changelog is:

	The kernel takes CMRs (plus a little more metadata) and
	constructs "TD Memory Regions" (TDMRs).

You just need to talk about things at a high level in mostly
non-technical language so that folks know the structure of the code
below.  It's not a replacement for the code, the comments, *OR* the TDX
module specification.

I'm also not quite sure that this justifies the static variables anyway.
 They could be dynamically allocated and passed around, for instance.

quoted

quoted

Use static variables for both TDSYSINFO_STRUCT and CMR array to avoid

I find it very useful to be precise when referring to code.  Your code
says 'tdsysinfo_struct', yet this says 'TDSYSINFO_STRUCT'.  Why the
difference?

Here I actually didn't intend to refer to any code.  In the above paragraph
(that is going to be replaced with yours), I mentioned "TDSYSINFO_STRUCT" to
explain what does "information of the TDX module" actually refer to, since
TDSYSINFO_STRUCT is used in the spec.

What's your preference?

Kill all mentions to TDSYSINFO_STRUCT whatsoever in the changelog.
Write comprehensible English.

quoted

quoted

having to pass them as function arguments when constructing the TDMR
array.  And they are too big to be put to the stack anyway.  Also, KVM
needs to use the TDSYSINFO_STRUCT to create TDX guests.

This is also a great place to mention that the tdsysinfo_struct contains
a *lot* of gunk which will not be used for a bit or that may never get
used.

Perhaps below?

"Note many members in tdsysinfo_struct' are not used by the kernel".

Btw, may I ask why does it matter?

Because you're adding a massive structure with all kinds of fields.
Those fields mostly aren't used.  That could be from an error in this
series, or because they will be used later or because they will *never*
be used.

quoted

quoted

+   cmr = &cmr_array[0];
+   /* There must be at least one valid CMR */
+   if (WARN_ON_ONCE(is_cmr_empty(cmr) || !is_cmr_ok(cmr)))
+           goto err;
+
+   cmr_num = *actual_cmr_num;
+   for (i = 1; i < cmr_num; i++) {
+           struct cmr_info *cmr = &cmr_array[i];
+           struct cmr_info *prev_cmr = NULL;
+
+           /* Skip further empty CMRs */
+           if (is_cmr_empty(cmr))
+                   break;
+
+           /*
+            * Do sanity check anyway to make sure CMRs:
+            *  - are 4K aligned
+            *  - don't overlap
+            *  - are in address ascending order.
+            */
+           if (WARN_ON_ONCE(!is_cmr_ok(cmr)))
+                   goto err;

Why does cmr_array[0] get a pass on the empty and sanity checks?

TDX MCHECK verifies CMRs before enabling TDX, so there must be at least one
valid CMR.

And cmr_array[0] is checked before this loop.

I think you're confusing two separate things.  MCHECK ensures that there
is convertible memory.  The CMRs that this code looks at are software
(TD module) defined and created structures that the OS and the module share.

This cmr_array[] structure is not created by MCHECK.

Go look at your code.  Consider what will happen if cmr_array[0] is
empty or !is_cmr_ok().  Then consider what will happen if cmr_array[1]
has the same happen.

Does that end result really justify having separate code for
cmr_array[0] and cmr_array[>0]?

quoted

quoted

+           prev_cmr = &cmr_array[i - 1];
+           if (WARN_ON_ONCE((prev_cmr->base + prev_cmr->size) >
+                                   cmr->base))
+                   goto err;
+   }
+
+   /* Update the actual number of CMRs */
+   *actual_cmr_num = i;

That comment is not helpful.  Yes, this is literally updating the number
of CMRs.  Literally.  That's the "what".  But, the "why" is important.
Why is it doing this?

When building the list of "TDX-usable" memory regions, the kernel verifies those
regions against CMRs to see whether they are truly convertible memory.

How about adding a comment like below:

        /*
         * When the kernel builds the TDX-usable memory regions, it verifies
         * they are truly convertible memory by checking them against CMRs.
         * Update the actual number of CMRs to skip those empty CMRs.
         */

Also, I think printing CMRs in the dmesg is helpful.  Printing empty (zero) CMRs
will put meaningless log to the dmesg.

So it's just about printing them?

Then put a dang switch to the print function that says "print them all"
or not.

...

quoted

Also, I saw the loop above check 'cmr_num' CMRs for is_cmr_ok().  Now,
it'll print an 'actual_cmr_num=1' number of CMRs as being
"kernel-checked".  Why?  That makes zero sense.

The loop quits when it sees an empty CMR.  I think there's no need to check
further CMRs as they must be empty (TDX MCHECK verifies CMRs).

OK, so you're going to get some more homework here.  Please explain to
me how MCHECK and the CMR array that comes out of the TDX module are
related.  How does the output from MCHECK get turned into the in-memory
cmr_array[], step by step?

At this point, I fear that you're offering up MCHECK like it's a bag of
magic beans rather than really truly thinking about the cmr_array[] data
structure.  How it is generated?  How might it be broken? Who might
break it?   If so, what the kernel should do about it?

quoted

quoted

+
+   /*
+    * trim_empty_cmrs() updates the actual number of CMRs by
+    * dropping all tail empty CMRs.
+    */
+   return trim_empty_cmrs(tdx_cmr_array, &tdx_cmr_num);
+}

Why does this both need to respect the "tdx_cmr_num = out.r9" value
*and* trim the empty ones?  Couldn't it just ignore the "tdx_cmr_num =
out.r9" value and just trim the empty ones either way?  It's not like
there is a billion of them.  It would simplify the code for sure.

OK.  Since spec says MAX_CMRs is 32, so I can use 32 instead of reading out from
R9.

But then you still have the "trimming" code.  Why not just trust "r9"
and then axe all the trimming code?  Heck, and most of the sanity checks.

This code could be a *lot* smaller.

On 11/23/22 02:18, Huang, Kai wrote:

quoted

Again, there are a lot of words in that comment, but I'm not sure why
it's here.  Despite all the whinging about ACPI, doesn't it boil down to:

      The TDX module itself establishes its own concept of how many
      logical CPUs there are in the system when it is loaded.

This isn't accurate.  TDX MCHECK records the total number of logical CPUs when
the BIOS enables TDX.  This happens before the TDX module is loaded.  In fact
the TDX module only gets this information from a secret location.

Kai, this is the point where I lose patience with the conversation
around this series.  I'll you paste you the line of code where the TDX
module literally "establishes its own concept of how many logical CPUs
there are in the system":

    //NUM_LPS
    tdx_global_data_ptr->num_of_lps = sysinfo_table_ptr->mcheck_fields.tot_num_lps;

Yes, this is derived directly from MCHECK.  But, this concept is
separate from MCHECK.  We have seen zero actual facts from you or other
folks at Intel that this is anything other than an arbitrary choice made
for the convenience of the TDX module.  It _might_ not be this way.  I'm
open to hearing those facts and changing my position on this.

But, please bring facts, not random references to "secret locations"
that aren't that secret.

On Wed, Nov 23, 2022, Dave Hansen wrote:

On 11/23/22 08:20, Sean Christopherson wrote:

quoted

quoted

quoted

Why is it done that way?

Can it be changed to delay TDX initialization until the first TDX guest
needs to run?

Sean suggested.

Hi Sean, could you commenet?

Waiting until the first TDX guest is created would result in false advertising,
as KVM wouldn't know whether or not TDX is actually supported until that first
VM is created.  If we can guarantee that TDH.SYS.INIT will fail if and only if
there is a kernel bug, then I would be ok deferring the "enabling" until the
first VM is created.

There's no way we can guarantee _that_.  For one, the PAMT* allocations
can always fail.  I guess we could ask sysadmins to fire up a guest to
"prime" things, but that seems a little silly.  Maybe that would work as
the initial implementation that we merge, but I suspect our users will
demand more determinism, maybe a boot or module parameter.

Oh, you mean all of TDX initialization?  I thought "initialization" here mean just
doing tdx_enable().

Yeah, that's not going to be a viable option.  Aside from lacking determinisim,
it would be all too easy to end up on a system with fragmented memory that can't
allocate the PAMTs post-boot.

On 11/23/22 09:37, Sean Christopherson wrote:

On Wed, Nov 23, 2022, Dave Hansen wrote:

quoted

There's no way we can guarantee _that_.  For one, the PAMT* allocations
can always fail.  I guess we could ask sysadmins to fire up a guest to
"prime" things, but that seems a little silly.  Maybe that would work as
the initial implementation that we merge, but I suspect our users will
demand more determinism, maybe a boot or module parameter.

Oh, you mean all of TDX initialization?  I thought "initialization" here mean just
doing tdx_enable().

Yes, but the first call to tdx_enable() does TDH_SYS_INIT and all the
subsequent work to get the module going.

Yeah, that's not going to be a viable option.  Aside from lacking determinisim,
it would be all too easy to end up on a system with fragmented memory that can't
allocate the PAMTs post-boot.

For now, the post-boot runtime PAMT allocations are the one any only way
that TDX can be initialized.  I pushed for it to be done this way.
Here's why:

Doing tdx_enable() is relatively slow and it eats up a non-zero amount
of physically contiguous RAM for metadata (~1/256th or ~0.4% of RAM).
Systems that support TDX but will never run TDX guests should not pay
that cost.

That means that we either make folks opt-in at boot-time or we try to
make a best effort at runtime to do the metadata allocations.

From my perspective, the best-effort stuff is absolutely needed.  Users
are going to forget the command-line opt in and there's no harm in
_trying_ the big allocations even if they fail.

Second, in reality, the "real" systems that can run TDX guests are
probably not going to sit around fragmenting memory for a month before
they run their first guest.  They're going to run one shortly after they
boot when memory isn't fragmented and the best-effort allocation will
work really well.

Third, if anyone *REALLY* cared to make it reliable *and* wanted to sit
around fragmenting memory for a month, they could just start a TDX guest
and kill it to get TDX initialized.  This isn't ideal.  But, to me, it
beats defining some new, separate ABI (or boot/module option) to do it.

So, let's have those discussions.  Long-term, what *is* the most
reliable way to get the TDX module loaded with 100% determinism?  What
new ABI or interfaces are needed?  Also, is that 100% determinism
required the moment this series is merged?  Or, can we work up to it?

I think it can wait until this particular series is farther along.

On Wed, Nov 23, 2022, Dave Hansen wrote:

On 11/23/22 09:37, Sean Christopherson wrote:

quoted

On Wed, Nov 23, 2022, Dave Hansen wrote:

quoted

There's no way we can guarantee _that_.  For one, the PAMT* allocations
can always fail.  I guess we could ask sysadmins to fire up a guest to
"prime" things, but that seems a little silly.  Maybe that would work as
the initial implementation that we merge, but I suspect our users will
demand more determinism, maybe a boot or module parameter.

Oh, you mean all of TDX initialization?  I thought "initialization" here mean just
doing tdx_enable().

Yes, but the first call to tdx_enable() does TDH_SYS_INIT and all the
subsequent work to get the module going.

Ah, sorry, I misread the diff.  Actually applied the patches this time...

quoted

Yeah, that's not going to be a viable option.  Aside from lacking determinisim,
it would be all too easy to end up on a system with fragmented memory that can't
allocate the PAMTs post-boot.

For now, the post-boot runtime PAMT allocations are the one any only way
that TDX can be initialized.  I pushed for it to be done this way.
Here's why:

Doing tdx_enable() is relatively slow and it eats up a non-zero amount
of physically contiguous RAM for metadata (~1/256th or ~0.4% of RAM).
Systems that support TDX but will never run TDX guests should not pay
that cost.

That means that we either make folks opt-in at boot-time or we try to
make a best effort at runtime to do the metadata allocations.

From my perspective, the best-effort stuff is absolutely needed.  Users
are going to forget the command-line opt in

Eh, any sufficiently robust deployment should be able to ensure that its kernels
boot with "required" command-line options.

and there's no harm in _trying_ the big allocations even if they fail.

No, but there is "harm" if a host can't provide the functionality the control
plane thinks it supports.

Second, in reality, the "real" systems that can run TDX guests are
probably not going to sit around fragmenting memory for a month before
they run their first guest.  They're going to run one shortly after they
boot when memory isn't fragmented and the best-effort allocation will
work really well.

I don't think this will hold true.  Long term, we (Google) want to have a common
pool for non-TDX and TDX VMs.  Forcing TDX VMs to use a dedicated pool of hosts
makes it much more difficult to react to demand, e.g. if we carve out N hosts for
TDX, but only use 10% of those hosts, then that's a lot of wasted capacity/money.
IIRC, people have discussed dynamically reconfiguring hosts so that systems could
be moved in/out of a dedicated pool, but that's still suboptimal, e.g. would
require emptying a host to reboot+reconfigure..

If/when we end up with a common pool, then it's very likely that we could have a
TDX-capable host go weeks/months before launching its first TDX VM.

Third, if anyone *REALLY* cared to make it reliable *and* wanted to sit
around fragmenting memory for a month, they could just start a TDX guest
and kill it to get TDX initialized.  This isn't ideal.  But, to me, it
beats defining some new, separate ABI (or boot/module option) to do it.

That's a hack.  I have no objection to waiting until KVM is _loaded_ to initialize
TDX, but waiting until KVM_CREATE_VM is not acceptable.  Use cases aside, KVM's ABI
would be a mess, e.g. KVM couldn't use KVM_CHECK_EXTENSION or any other /dev/kvm
ioctl() to enumerate TDX support.

So, let's have those discussions.  Long-term, what *is* the most
reliable way to get the TDX module loaded with 100% determinism?  What
new ABI or interfaces are needed?  Also, is that 100% determinism
required the moment this series is merged?  Or, can we work up to it?

I don't think we (Google again) strictly need 100% determinism with respect to
enabling TDX, what's most important is that if a host says it's TDX-capable, then
it really is TDX-capable.  I'm sure we'll treat "failure to load" as an error,
but it doesn't necessarily need to be a fatal error as the host can still run in
a degraded state (no idea if we'll actually do that though).

I think it can wait until this particular series is farther along.

For an opt-in kernel param, agreed.  That can be added later, e.g. if it turns
out that the PAMT allocation failure rate is too high.

On Wed, 2022-11-23 at 08:58 -0800, Dave Hansen wrote:

On 11/23/22 02:18, Huang, Kai wrote:

quoted

quoted

Again, there are a lot of words in that comment, but I'm not sure why
it's here.  Despite all the whinging about ACPI, doesn't it boil down to:

      The TDX module itself establishes its own concept of how many
      logical CPUs there are in the system when it is loaded.

This isn't accurate.  TDX MCHECK records the total number of logical CPUs when
the BIOS enables TDX.  This happens before the TDX module is loaded.  In fact
the TDX module only gets this information from a secret location.

Kai, this is the point where I lose patience with the conversation
around this series.  I'll you paste you the line of code where the TDX
module literally "establishes its own concept of how many logical CPUs
there are in the system":

quoted

    //NUM_LPS
    tdx_global_data_ptr->num_of_lps = sysinfo_table_ptr->mcheck_fields.tot_num_lps;

Yes, this is derived directly from MCHECK.  But, this concept is
separate from MCHECK.  We have seen zero actual facts from you or other
folks at Intel that this is anything other than an arbitrary choice made
for the convenience of the TDX module.  It _might_ not be this way.  I'm
open to hearing those facts and changing my position on this.

But, please bring facts, not random references to "secret locations"
that aren't that secret.

Agreed.  Thanks for providing the comment and will use it.

On 11/20/22 16:26, Kai Huang wrote:

TDX provides increased levels of memory confidentiality and integrity.
This requires special hardware support for features like memory
encryption and storage of memory integrity checksums.  Not all memory
satisfies these requirements.

As a result, the TDX introduced the concept of a "Convertible Memory

s/the TDX introduced/TDX introduces/

Region" (CMR).  During boot, the firmware builds a list of all of the
memory ranges which can provide the TDX security guarantees.  The list
of these ranges is available to the kernel by querying the TDX module.

The TDX architecture needs additional metadata to record things like
which TD guest "owns" a given page of memory.  This metadata essentially
serves as the 'struct page' for the TDX module.  The space for this
metadata is not reserved by the hardware up front and must be allocated
by the kernel and given to the TDX module.

Since this metadata consumes space, the VMM can choose whether or not to
allocate it for a given area of convertible memory.  If it chooses not
to, the memory cannot receive TDX protections and can not be used by TDX
guests as private memory.

For every memory region that the VMM wants to use as TDX memory, it sets
up a "TD Memory Region" (TDMR).  Each TDMR represents a physically
contiguous convertible range and must also have its own physically
contiguous metadata table, referred to as a Physical Address Metadata
Table (PAMT), to track status for each page in the TDMR range.

Unlike a CMR, each TDMR requires 1G granularity and alignment.  To
support physical RAM areas that don't meet those strict requirements,
each TDMR permits a number of internal "reserved areas" which can be
placed over memory holes.  If PAMT metadata is placed within a TDMR it
must be covered by one of these reserved areas.

Let's summarize the concepts:

 CMR - Firmware-enumerated physical ranges that support TDX.  CMRs are
       4K aligned.
TDMR - Physical address range which is chosen by the kernel to support
       TDX.  1G granularity and alignment required.  Each TDMR has
       reserved areas where TDX memory holes and overlapping PAMTs can
       be put into.

s/put into/represented/

PAMT - Physically contiguous TDX metadata.  One table for each page size
       per TDMR.  Roughly 1/256th of TDMR in size.  256G TDMR = ~1G
       PAMT.

As one step of initializing the TDX module, the kernel configures
TDX-usable memory regions by passing an array of TDMRs to the TDX module.

Constructing the array of TDMRs consists below steps:

1) Create TDMRs to cover all memory regions that the TDX module can use;

Slight tweak:

1) Create TDMRs to cover all memory regions that the TDX module will use
   for TD memory

The TDX module "uses" more memory than strictly the TMDR's.

2) Allocate and set up PAMT for each TDMR;
3) Set up reserved areas for each TDMR.

s/Set up/Designate/

Add a placeholder to construct TDMRs to do the above steps after all
TDX memory regions are verified to be truly convertible.  Always free
TDMRs at the end of the initialization (no matter successful or not)
as TDMRs are only used during the initialization.

The changelog here actually looks really good to me so far.

I think we can spare the bytes to add "culate" in the function name so
we don't think these are California TDMRs.

First, I think 'tdx_sysinfo' should probably be a local variable in
init_tdx_module() and have its address passed in here.  Having global
variables always makes it more opaque about who is initializing it.

Second, if this code is making assumptions about
'max_reserved_per_tdmr', then let's actually add assertions or sanity
checks.  For instance:

	if (tdx_sysinfo.max_reserved_per_tdmr > MAX_TDMRS)
		return -1;

or even:

	if (tdmr_sz > PAGE_SIZE)
		return -1;

It does almost no good to just assert what the limits are in a comment.

<sigh> More silly comments.

The place to document this is TDMR_INFO_ALIGNMENT.  If anyone wants to
know what the alignment is, exactly, they can look at the definition.
They don't need to be told *TWICE* what TDMR_INFO_ALIGNMENT #defines to
in one comment.

OK, I think you're just trolling me now.  Two *MORE* mentions of the
512-byte alignment?

This looks massively over complicated.

Get rid of this function entirely.  Then create:

static int tdmr_array_size(void)
{
	return tdmr_size_single() * tdx_sysinfo.max_tdmrs;
}

The *caller* can do:

	tdmr_array = alloc_pages_exact(tdmr_array_size(),
				       GFP_KERNEL | __GFP_ZERO);
	if (!tdmr_array) {
		...

Then the error path is:

	free_pages_exact(tdmr_array, tdmr_array_size());

Then, there are no size pointers going back and forth.  Easy peasy.  I'm
OK with a little arithmetic being repeated.

I tend to write these like:

"tdmr_num" is the number of *a* TDMR.

"nr_tdmrs" is the number of TDMRs.

On 11/20/22 16:26, Kai Huang wrote:

The kernel configures TDX-usable memory regions by passing an array of
"TD Memory Regions" (TDMRs) to the TDX module.  Each TDMR contains the
information of the base/size of a memory region, the base/size of the
associated Physical Address Metadata Table (PAMT) and a list of reserved
areas in the region.

Create a number of TDMRs to cover all TDX memory regions.  To keep it
simple, always try to create one TDMR for each memory region.  As the
first step only set up the base/size for each TDMR.

Each TDMR must be 1G aligned and the size must be in 1G granularity.
This implies that one TDMR could cover multiple memory regions.  If a
memory region spans the 1GB boundary and the former part is already
covered by the previous TDMR, just create a new TDMR for the remaining
part.

TDX only supports a limited number of TDMRs.  Disable TDX if all TDMRs
are consumed but there is more memory region to cover.

Good changelog.  This patch is doing *one* thing.

I'm always skeptical that it's a good idea to take this in code:

	tdmr->base

and make it this:

	tdmr_start(tdmr)

because the helper is *LESS* compact than the open-coded form!  I hope
I'm proven wrong.

FWIW, I think it's probably a bad idea to have 'struct tdmr_info *'
types floating around since:

	tmdr_info_array[0]

works, but:

	tmdr_info_array[1]

will blow up in your face.  It would almost make sense to have

struct tdmr_info_list {
	struct tdmr_info *first_tdmr;
}

and then pass around pointers to the 'struct tdmr_info_list'.  Maybe
that's overkill, but it is kinda silly to call something an array if []
doesn't work on it.

This seems like it might tend to under-utilize TDMRs.  I'm sure this is
done for simplicity, but is it OK?  Why is it OK?  How are you sure this
won't bite us later?

Nit: a little vertical alignment can make this much more readable:

		start = TDMR_ALIGN_DOWN(tmb->start_pfn << PAGE_SHIFT);
		end   = TDMR_ALIGN_UP  (tmb->end_pfn   << PAGE_SHIFT);

Again, we have a comment over a if() block that describes what the
individual steps in the block do.  *Plus* each individual step is
*ALREADY* commented.  What purpose does this comment serve?

Seems like a positive return value could be the number of populated
TDMRs.  That would get rid of the int* argument.

 /*
  * Construct an array of TDMRs to cover all TDX memory ranges.
  * The actual number of TDMRs is kept to @tdmr_num.
  */

OK, so something else allocated the 'tdmr_array' and it's being passed
in here to fill it out.  "construct" and "create" are both near synonyms
for "allocate", which isn't even being done here.

We want something here that will make it clear that this function is
taking an already populated list of TDMRs and filling it out.
"fill_tmdrs()" seems like it might be a better choice.

This is also a place where better words can help.  If the function is
called "construct", then there's *ZERO* value in using the same word in
the comment.  Using a word that is a close synonym but that can contrast
it with something different would be really nice, say:

This is also a place where the calling convention can be used to add
clarity.  If you implicitly use a global variable, you have to explain
that.  But, if you pass *in* a variable, it's a lot more clear.

Take this, for instance:

/*
 * Take the memory referenced in @tdx_memlist and populate the
 * preallocated @tmdr_array, following all the special alignment
 * and size rules for TDMR.
 */
static int fill_out_tdmrs(struct list_head *tdx_memlist,
			  struct tdmr_info *tdmr_array)
{
...

That's 100% crystal clear about what's going on.  You know what the
inputs are and the outputs.  You also know why this is even necessary.
It's implied a bit, but it's because TDMRs have special rules about
size/alignment and tdx_memlists do not.

On Wed, 2022-11-23 at 08:44 -0800, Dave Hansen wrote:

quoted

On 11/23/22 03:40, Huang, Kai wrote:

quoted

quoted

On Tue, 2022-11-22 at 15:39 -0800, Dave Hansen wrote:

quoted

quoted

That last sentece is kinda goofy.  I think there's a way to distill this
whole thing down more effecively.

      CMRs tell the kernel which memory is TDX compatible.  The kernel
      takes CMRs and constructs  "TD Memory Regions" (TDMRs).  TDMRs
      let the kernel grant TDX protections to some or all of the CMR
      areas.

Will do.

But it seems we should still mention "Constructing TDMRs requires information of
both the TDX module (TDSYSINFO_STRUCT) and the CMRs"?  The reason is to justify
"use static to avoid having to pass them as function arguments when constructing
TDMRs" below.

In a changelog, no.  You do *NOT* use super technical language in
changelogs if not super necessary.  Mentioning "TDSYSINFO_STRUCT" here
is useless.  The *MOST* you would do for a good changelog is:

	The kernel takes CMRs (plus a little more metadata) and
	constructs "TD Memory Regions" (TDMRs).

You just need to talk about things at a high level in mostly
non-technical language so that folks know the structure of the code
below.  It's not a replacement for the code, the comments, *OR* the TDX
module specification.

I'm also not quite sure that this justifies the static variables anyway.
 They could be dynamically allocated and passed around, for instance.

I see. Thanks for explaining.

quoted

quoted

quoted

quoted

quoted

quoted

quoted

Use static variables for both TDSYSINFO_STRUCT and CMR array to avoid

I find it very useful to be precise when referring to code.  Your code
says 'tdsysinfo_struct', yet this says 'TDSYSINFO_STRUCT'.  Why the
difference?

Here I actually didn't intend to refer to any code.  In the above paragraph
(that is going to be replaced with yours), I mentioned "TDSYSINFO_STRUCT" to
explain what does "information of the TDX module" actually refer to, since
TDSYSINFO_STRUCT is used in the spec.

What's your preference?

Kill all mentions to TDSYSINFO_STRUCT whatsoever in the changelog.
Write comprehensible English.

OK.

quoted

quoted

quoted

quoted

quoted

quoted

quoted

having to pass them as function arguments when constructing the TDMR
array.  And they are too big to be put to the stack anyway.  Also, KVM
needs to use the TDSYSINFO_STRUCT to create TDX guests.

This is also a great place to mention that the tdsysinfo_struct contains
a *lot* of gunk which will not be used for a bit or that may never get
used.

Perhaps below?

"Note many members in tdsysinfo_struct' are not used by the kernel".

Btw, may I ask why does it matter?

Because you're adding a massive structure with all kinds of fields.
Those fields mostly aren't used.  That could be from an error in this
series, or because they will be used later or because they will *never*
be used.

OK.

quoted

quoted

quoted

quoted

quoted

quoted

quoted

+   cmr = &cmr_array[0];
+   /* There must be at least one valid CMR */
+   if (WARN_ON_ONCE(is_cmr_empty(cmr) || !is_cmr_ok(cmr)))
+           goto err;
+
+   cmr_num = *actual_cmr_num;
+   for (i = 1; i < cmr_num; i++) {
+           struct cmr_info *cmr = &cmr_array[i];
+           struct cmr_info *prev_cmr = NULL;
+
+           /* Skip further empty CMRs */
+           if (is_cmr_empty(cmr))
+                   break;
+
+           /*
+            * Do sanity check anyway to make sure CMRs:
+            *  - are 4K aligned
+            *  - don't overlap
+            *  - are in address ascending order.
+            */
+           if (WARN_ON_ONCE(!is_cmr_ok(cmr)))
+                   goto err;

Why does cmr_array[0] get a pass on the empty and sanity checks?

TDX MCHECK verifies CMRs before enabling TDX, so there must be at least one
valid CMR.

And cmr_array[0] is checked before this loop.

I think you're confusing two separate things.  MCHECK ensures that there
is convertible memory.  The CMRs that this code looks at are software
(TD module) defined and created structures that the OS and the module share.

Not sure whether I completely got your words, but the CMRs are generated by the
BIOS, verified and stored by the MCHECK. Thus the CMR structure is also
meaningful to the BIOS and the MCHECK, but not TDX module defined and created.

There are couple of places in the TDX module spec which says this. One example
is "Table 3.1: Typical Intel TDX Module Platform-Scope Initialization Sequence"
and "13.1.1. Initialization and Configuration Flow". They both mention:

"BIOS configures Convertible Memory Regions (CMRs); MCHECK checks them and
securely stores the information."

Also, "20.8.3 CMR_INFO":

"CMR_INFO is designed to provide information about a Convertible Memory Range
(CMR), as configured by BIOS and checked and stored securely by MCHECK."

quoted

This cmr_array[] structure is not created by MCHECK.

Right.

But TDH.SYS.INFO only "Retrieve Intel TDX module information and convertible
memory (CMR) information." by writing CMRs to the buffer provided by the kernel
(cmr_array[]).

So my understanding is the entries in the cmr_array[] are just the same CMRs
that are verified by the MCHECK.

quoted

Go look at your code.  Consider what will happen if cmr_array[0] is
empty or !is_cmr_ok().  Then consider what will happen if cmr_array[1]
has the same happen.

Does that end result really justify having separate code for
cmr_array[0] and cmr_array[>0]?

One slight difference is cmr_array[0] must be valid, but cmr_array[>1] can be
empty. And for cmr_array[>0] we also have additional check against the previous
one.

quoted

quoted

quoted

quoted

quoted

quoted

quoted

+           prev_cmr = &cmr_array[i - 1];
+           if (WARN_ON_ONCE((prev_cmr->base + prev_cmr->size) >
+                                   cmr->base))
+                   goto err;
+   }
+
+   /* Update the actual number of CMRs */
+   *actual_cmr_num = i;

That comment is not helpful.  Yes, this is literally updating the number
of CMRs.  Literally.  That's the "what".  But, the "why" is important.
Why is it doing this?

When building the list of "TDX-usable" memory regions, the kernel verifies those
regions against CMRs to see whether they are truly convertible memory.

How about adding a comment like below:

        /*
         * When the kernel builds the TDX-usable memory regions, it verifies
         * they are truly convertible memory by checking them against CMRs.
         * Update the actual number of CMRs to skip those empty CMRs.
         */

Also, I think printing CMRs in the dmesg is helpful.  Printing empty (zero) CMRs
will put meaningless log to the dmesg.

So it's just about printing them?

Then put a dang switch to the print function that says "print them all"
or not.

Yes can do. Currently "print them all" is only done when the CMR sanity check
fails. We can unconditionally "print valid CMRs" if we don't need that check.

quoted

...

quoted

quoted

quoted

quoted

Also, I saw the loop above check 'cmr_num' CMRs for is_cmr_ok().  Now,
it'll print an 'actual_cmr_num=1' number of CMRs as being
"kernel-checked".  Why?  That makes zero sense.

The loop quits when it sees an empty CMR.  I think there's no need to check
further CMRs as they must be empty (TDX MCHECK verifies CMRs).

OK, so you're going to get some more homework here.  Please explain to
me how MCHECK and the CMR array that comes out of the TDX module are
related.  How does the output from MCHECK get turned into the in-memory
cmr_array[], step by step?

(Please also see my above reply)

1. BIOS generates the CMRs and pass to the MCHECK
2. MCHECK verifies CMRs and stores the "CMR table in a pre-defined location in
SEAMRR’s SEAMCFG region so it can be read later and trusted by the Intel TDX
module" (13.1.4.1 Intel TDX ISA Background: Convertible Memory Ranges (CMRs)).
3. TDH.SYS.INFO copies the CMRs to the buffer provided by the kernel
(cmr_array[]).

quoted

At this point, I fear that you're offering up MCHECK like it's a bag of
magic beans rather than really truly thinking about the cmr_array[] data
structure.  How it is generated?  How might it be broken? Who might
break it?   If so, what the kernel should do about it?

Only kernel bug can break the cmr_array[] I think. As described in "13.1.4.1
Intel TDX ISA Background: Convertible Memory Ranges (CMRs)", MCHECK should have
guaranteed that:
	- there must be one CMR
	- CMR is page aligned
	- CMRs don't overlap and in address ascending order

The only legal thing is there might be empty CMRs at the tail of the cmr_array[]
following one or more valid CMRs.

quoted

quoted

quoted

quoted

quoted

quoted

quoted

+
+   /*
+    * trim_empty_cmrs() updates the actual number of CMRs by
+    * dropping all tail empty CMRs.
+    */
+   return trim_empty_cmrs(tdx_cmr_array, &tdx_cmr_num);
+}

Why does this both need to respect the "tdx_cmr_num = out.r9" value
*and* trim the empty ones?  Couldn't it just ignore the "tdx_cmr_num =
out.r9" value and just trim the empty ones either way?  It's not like
there is a billion of them.  It would simplify the code for sure.

OK.  Since spec says MAX_CMRs is 32, so I can use 32 instead of reading out from
R9.

But then you still have the "trimming" code.  Why not just trust "r9"
and then axe all the trimming code?  Heck, and most of the sanity checks.

This code could be a *lot* smaller.

As I said the only problem is there might be empty CMRs at the tail of the
cmr_array[] following one or more valid CMRs.  

But we can also do nothing here, but just skip empty CMRs when comparing the
memory region to it (in next patch).

Or, we don't even need to explicitly check memory region against CMRs. If the
memory regions that we provided in the TDMR doesn't fall into CMR, then
TDH.SYS.CONFIG will fail. We can just depend on the SEAMCALL to do that.

On 11/20/22 16:26, Kai Huang wrote:

The TDX module uses additional metadata to record things like which
guest "owns" a given page of memory.  This metadata, referred as
Physical Address Metadata Table (PAMT), essentially serves as the
'struct page' for the TDX module.  PAMTs are not reserved by hardware
up front.  They must be allocated by the kernel and then given to the
TDX module.

... during module initialization.

TDX supports 3 page sizes: 4K, 2M, and 1G.  Each "TD Memory Region"
(TDMR) has 3 PAMTs to track the 3 supported page sizes.  Each PAMT must
be a physically contiguous area from a Convertible Memory Region (CMR).
However, the PAMTs which track pages in one TDMR do not need to reside
within that TDMR but can be anywhere in CMRs.  If one PAMT overlaps with
any TDMR, the overlapping part must be reported as a reserved area in
that particular TDMR.

Use alloc_contig_pages() since PAMT must be a physically contiguous area
and it may be potentially large (~1/256th of the size of the given TDMR).
The downside is alloc_contig_pages() may fail at runtime.  One (bad)
mitigation is to launch a TD guest early during system boot to get those
PAMTs allocated at early time, but the only way to fix is to add a boot
option to allocate or reserve PAMTs during kernel boot.

FWIW, we all agree that this is a bad permanent way to leave things.
You can call me out here as proposing that this wart be left in place
while this series is merged and is a detail we can work on afterword
with new module params, boot options, Kconfig or whatever.

TDX only supports a limited number of reserved areas per TDMR to cover
both PAMTs and memory holes within the given TDMR.  If many PAMTs are
allocated within a single TDMR, the reserved areas may not be sufficient
to cover all of them.

Adopt the following policies when allocating PAMTs for a given TDMR:

  - Allocate three PAMTs of the TDMR in one contiguous chunk to minimize
    the total number of reserved areas consumed for PAMTs.
  - Try to first allocate PAMT from the local node of the TDMR for better
    NUMA locality.

Also dump out how many pages are allocated for PAMTs when the TDX module
is initialized successfully.

... this helps answer the eternal "where did all my memory go?" questions.

Aha, the first use of tmb->nid!  I wondered why that was there.

That's probably better a pr_warn() or something.  A backtrace and all
that jazz seems a bit overly dramatic for this.

+	return 0;
+}

The rest of this actually looks fine.  It's nearing ack'able state.

This needs a comment.

This is another case where it's confusing to be passing around 'struct
tdmr_info *'.  Is this *A* TDMR or an array?


/*
 * Go through tdx_memlist to find holes between memory areas.  If any of
 * those holes fall within @tdmr, set up a TDMR reserved area to cover
 * the hole.
 */
static int tdmr_populate_rsvd_holes(struct list_head *tdx_memlist,
				    struct tdmr_info *tdmr,
				    int *rsvd_idx)

I'm having a hard time following this, especially the mixing of
semantics between 'prev_end' both pointing to tdmr and to tmb addresses.

Here, 'prev_end' logically represents the last address which we know has
been handled.  All of the holes in the addresses below it have been
dealt with.  It is safe to set here to tdmr_start() because this
function call is uniquely tasked with setting up reserved areas in
'tdmr'.  So, it can safely consider any holes before tdmr_start(tdmr) as
being handled.

But, dang, there's a lot of complexity there.

First, the:

	/* Mark holes between memory regions as reserved */

comment is misleading.  It has *ZILCH* to do with the "prev_end =
tdmr_start(tdmr);" assignment.

This at least needs:

   /* Start looking for reserved blocks at the beginning of the TDMR: */
   prev_end = tdmr_start(tdmr);

but I also get the feeling that 'prev_end' is a crummy variable name.  I
don't have any better suggestions at the moment.

More alignment opportunities:

		start = tmb->start_pfn << PAGE_SHIFT;
		end   = tmb->end_pfn   << PAGE_SHIFT;

Can't this just be:

		/*
		 * Skip over memory areas that
		 * have already been dealt with.
		 */

Ugh, and I guess we don't know where the PAMTs will be ordered with
respect to holes, so sorting is the easiest way to do this.

<snip>

On 11/20/22 16:26, Kai Huang wrote:

This doesn't "reserve" squat.

You could argue that it "picks", "chooses", or "designates" the
'tdx_global_keyid', but where is the "reservation"?

On 11/20/22 16:26, Kai Huang wrote:

Aagh.  Return of (a different) 512-byte aligned structure.

+	array_sz = ALIGN(tdmr_num * sizeof(u64), TDMR_INFO_PA_ARRAY_ALIGNMENT);
+	tdmr_pa_array = kzalloc(array_sz, GFP_KERNEL);

Just to be clear, all that chatter about alignment is because the
*START* of the array has to be aligned.  Right?  I see alignment for
'array_sz', but that's not the start of the array.

tdmr_pa_array is the start of the array.  Where is *THAT* aligned?

How does rounding up the size make kzalloc() magically know how to align
the *START* of the allocation?

Because I'm actually questioning my own sanity at this point, I went and
double-checked the docs (Documentation/core-api/memory-allocation.rst):

The address of a chunk allocated with `kmalloc` is aligned to at least
ARCH_KMALLOC_MINALIGN bytes.  For sizes which are a power of two, the
alignment is also guaranteed to be at least the respective size.

Hint #1: ARCH_KMALLOC_MINALIGN is way less than 512.
Hint #2: tdmr_num is not guaranteed to be a power of two.
Hint #3: Comments don't actually affect the allocation

<snip>

On 11/20/22 16:26, Kai Huang wrote:

After the array of TDMRs and the global KeyID are configured to the TDX
module, use TDH.SYS.KEY.CONFIG to configure the key of the global KeyID
on all packages.

TDH.SYS.KEY.CONFIG must be done on one (any) cpu for each package.  And
it cannot run concurrently on different CPUs.  Implement a helper to
run SEAMCALL on one cpu for each package one by one, and use it to
configure the global KeyID on all packages.

This has the same problems as SYS.LP.INIT.  It's basically snake oil in
some TDX configurations.

This really only needs to be done when the TDX module has memory
mappings on a socket for which it needs to use the "global KeyID".  If
there's no PAMT on a socket, there are probably no allocations there to
speak of and no *technical* reason to call TDH.SYS.KEY.CONFIG on that
socket.  At least none I can see.

So, let's check up on this requirement as well.  This could also turn
out to be a real pain if all the CPUs on a socket are offline.

Intel hardware doesn't guarantee cache coherency across different
KeyIDs.  The kernel needs to flush PAMT's dirty cachelines (associated
with KeyID 0) before the TDX module uses the global KeyID to access the
PAMT.  Following the TDX module specification, flush cache before
configuring the global KeyID on all packages.

I think it's probably worth an aside here about why TDX security isn't
dependent on this step.  I *think* it boils down to the memory integrity
protections.  If the caches aren't flushed, a dirty KeyID-0 cacheline
could be written back to RAM.  The TDX module would come along later and
read the cacheline using KeyID-whatever, get an integrity mismatch,
machine check, and then everyone would be sad.

Everyone is sad, but TDX security remains intact because memory
integrity saved us.

Is it memory integrity or the TD bit, actually?

Given the PAMT size can be large (~1/256th of system RAM), just use
WBINVD on all CPUs to flush.

<sigh>

Note if any TDH.SYS.KEY.CONFIG fails, the TDX module may already have
used the global KeyID to write any PAMT.  Therefore, need to use WBINVD
to flush cache before freeing the PAMTs back to the kernel.  Note using
MOVDIR64B (which changes the page's associated KeyID from the old TDX
private KeyID back to KeyID 0, which is used by the kernel) to clear
PMATs isn't needed, as the KeyID 0 doesn't support integrity check.

I hope this is covered in the code well.

I don't think you need to cover this twice.  Just do it in one comment.

						s/initialized/written/

MOVDIR64B is the tiniest of implementation details and also not the only
way to initialize memory integrity metadata.

Just keep this high level:

		* No need to worry about memory integrity checks here.
		* KeyID 0 has integrity checking disabled.

On 11/20/22 16:26, Kai Huang wrote:

Initialize TDMRs via TDH.SYS.TDMR.INIT as the last step to complete the
TDX initialization.

All TDMRs need to be initialized using TDH.SYS.TDMR.INIT SEAMCALL before
the memory pages can be used by the TDX module.  The time to initialize
TDMR is proportional to the size of the TDMR because TDH.SYS.TDMR.INIT
internally initializes the PAMT entries using the global KeyID.

To avoid long latency caused in one SEAMCALL, TDH.SYS.TDMR.INIT only
initializes an (implementation-specific) subset of PAMT entries of one
TDMR in one invocation.  The caller needs to call TDH.SYS.TDMR.INIT
iteratively until all PAMT entries of the given TDMR are initialized.

TDH.SYS.TDMR.INITs can run concurrently on multiple CPUs as long as they
are initializing different TDMRs.  To keep it simple, just initialize
all TDMRs one by one.  On a 2-socket machine with 2.2G CPUs and 64GB
memory, each TDH.SYS.TDMR.INIT roughly takes couple of microseconds on
average, and it takes roughly dozens of milliseconds to complete the
initialization of all TDMRs while system is idle.

Any chance you could say TDH.SYS.TDMR.INIT a few more times in there? ;)

Seriously, please try to trim that down.  If you talk about the
implementation in detail in the code comments, don't cover it in detail
in the changelog too.

Also, this is a momentous patch, right?  It's the last piece.  Maybe
worth calling that out.

Does this comment add value?  Even if it does, it is better than naming
the dang function init_one_tdmr()?

The PAMT discussion here is, IMNHO silly.  If this were about
initializing PAMT's then it should be renamed init_pamts() and the
SEAMCALL should be called PAMT_INIT or soemthing.  It's not, and the ABI
is built around the TDMR and *its* addresses.

Let's chop this comment down:

	/*
	 * Initializing a TDMR can be time consuming.  To avoid long
	 * SEAMCALLs, the TDX module may only initialize a part of the
	 * TDMR in each call.
	 */

Talk about the looping logic in the loop.

That comment is a wee bit superfluous, don't you think?

+		cond_resched();

		/* Keep making SEAMCALLs until the TDMR is done */

Does this comment add value?

Trim down the comment:

	/*
	 * This operation is costly.  It can be parallelized,
	 * but keep it simple for now.
	 */

On Tue, 2022-11-22 at 16:21 -0800, Dave Hansen wrote:

On 11/20/22 16:26, Kai Huang wrote:

quoted

TDX reports a list of "Convertible Memory Region" (CMR) to indicate all
memory regions that can possibly be used by the TDX module, but they are
not automatically usable to the TDX module.  As a step of initializing
the TDX module, the kernel needs to choose a list of memory regions (out
from convertible memory regions) that the TDX module can use and pass
those regions to the TDX module.  Once this is done, those "TDX-usable"
memory regions are fixed during module's lifetime.  No more TDX-usable
memory can be added to the TDX module after that.

The initial support of TDX guests will only allocate TDX guest memory
from the global page allocator.  To keep things simple, this initial
implementation simply guarantees all pages in the page allocator are TDX
memory.  To achieve this, use all system memory in the core-mm at the
time of initializing the TDX module as TDX memory, and at the meantime,
refuse to add any non-TDX-memory in the memory hotplug.

Specifically, walk through all memory regions managed by memblock and
add them to a global list of "TDX-usable" memory regions, which is a
fixed list after the module initialization (or empty if initialization
fails).  To reject non-TDX-memory in memory hotplug, add an additional
check in arch_add_memory() to check whether the new region is covered by
any region in the "TDX-usable" memory region list.

Note this requires all memory regions in memblock are TDX convertible
memory when initializing the TDX module.  This is true in practice if no
new memory has been hot-added before initializing the TDX module, since
in practice all boot-time present DIMM is TDX convertible memory.  If
any new memory has been hot-added, then initializing the TDX module will
fail due to that memory region is not covered by CMR.

This can be enhanced in the future, i.e. by allowing adding non-TDX
memory to a separate NUMA node.  In this case, the "TDX-capable" nodes
and the "non-TDX-capable" nodes can co-exist, but the kernel/userspace
needs to guarantee memory pages for TDX guests are always allocated from
the "TDX-capable" nodes.

Note TDX assumes convertible memory is always physically present during
machine's runtime.  A non-buggy BIOS should never support hot-removal of
any convertible memory.  This implementation doesn't handle ACPI memory
removal but depends on the BIOS to behave correctly.

My eyes glazed over about halfway through that.  Can you try to trim it
down a bit, or at least try to summarize it better up front?

Will do.

quoted

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index dd333b46fafb..b36129183035 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig

@@ -1959,6 +1959,7 @@ config INTEL_TDX_HOST
 	depends on X86_64
 	depends on KVM_INTEL
 	depends on X86_X2APIC
+	select ARCH_KEEP_MEMBLOCK
 	help
 	  Intel Trust Domain Extensions (TDX) protects guest VMs from malicious
 	  host and certain physical attacks.  This option enables necessary TDX

diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
index d688228f3151..71169ecefabf 100644
--- a/arch/x86/include/asm/tdx.h
+++ b/arch/x86/include/asm/tdx.h

@@ -111,9 +111,12 @@ static inline long tdx_kvm_hypercall(unsigned int nr, unsigned long p1,
 #ifdef CONFIG_INTEL_TDX_HOST
 bool platform_tdx_enabled(void);
 int tdx_enable(void);
+bool tdx_cc_memory_compatible(unsigned long start_pfn, unsigned long end_pfn);
 #else	/* !CONFIG_INTEL_TDX_HOST */
 static inline bool platform_tdx_enabled(void) { return false; }
 static inline int tdx_enable(void)  { return -ENODEV; }
+static inline bool tdx_cc_memory_compatible(unsigned long start_pfn,
+		unsigned long end_pfn) { return true; }
 #endif	/* CONFIG_INTEL_TDX_HOST */
 
 #endif /* !__ASSEMBLY__ */

diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
index 3f040c6e5d13..900341333d7e 100644
--- a/arch/x86/mm/init_64.c
+++ b/arch/x86/mm/init_64.c

@@ -55,6 +55,7 @@
 #include <asm/uv/uv.h>
 #include <asm/setup.h>
 #include <asm/ftrace.h>
+#include <asm/tdx.h>
 
 #include "mm_internal.h"
 

@@ -968,6 +969,15 @@ int arch_add_memory(int nid, u64 start, u64 size,
 	unsigned long start_pfn = start >> PAGE_SHIFT;
 	unsigned long nr_pages = size >> PAGE_SHIFT;
 
+	/*
+	 * For now if TDX is enabled, all pages in the page allocator

s/For now//

Will do.

quoted

+	 * must be TDX memory, which is a fixed set of memory regions
+	 * that are passed to the TDX module.  Reject the new region
+	 * if it is not TDX memory to guarantee above is true.
+	 */
+	if (!tdx_cc_memory_compatible(start_pfn, start_pfn + nr_pages))
+		return -EINVAL;

There's a real art to making a right-size comment.  I don't think this
needs to be any more than:

	/*
	 * Not all memory is compatible with TDX.  Reject
	 * the addition of any incomatible memory.
	 */

Thanks.

If you want to write a treatise, do it in Documentation or at the
tdx_cc_memory_compatible() definition.

quoted

 	init_memory_mapping(start, start + size, params->pgprot);
 
 	return add_pages(nid, start_pfn, nr_pages, params);

diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
index 43227af25e44..32af86e31c47 100644
--- a/arch/x86/virt/vmx/tdx/tdx.c
+++ b/arch/x86/virt/vmx/tdx/tdx.c

@@ -16,6 +16,11 @@
 #include <linux/smp.h>
 #include <linux/atomic.h>
 #include <linux/align.h>
+#include <linux/list.h>
+#include <linux/slab.h>
+#include <linux/memblock.h>
+#include <linux/minmax.h>
+#include <linux/sizes.h>
 #include <asm/msr-index.h>
 #include <asm/msr.h>
 #include <asm/apic.h>

@@ -34,6 +39,13 @@ enum tdx_module_status_t {
 	TDX_MODULE_SHUTDOWN,
 };
 
+struct tdx_memblock {
+	struct list_head list;
+	unsigned long start_pfn;
+	unsigned long end_pfn;
+	int nid;
+};

Why does the nid matter?

It is used to find the node for the PAMT allocation for a given TDMR.

quoted

 static u32 tdx_keyid_start __ro_after_init;
 static u32 tdx_keyid_num __ro_after_init;
 

@@ -46,6 +58,9 @@ static struct tdsysinfo_struct tdx_sysinfo;
 static struct cmr_info tdx_cmr_array[MAX_CMRS] __aligned(CMR_INFO_ARRAY_ALIGNMENT);
 static int tdx_cmr_num;
 
+/* All TDX-usable memory regions */
+static LIST_HEAD(tdx_memlist);
+
 /*
  * Detect TDX private KeyIDs to see whether TDX has been enabled by the
  * BIOS.  Both initializing the TDX module and running TDX guest require

@@ -329,6 +344,107 @@ static int tdx_get_sysinfo(void)
 	return trim_empty_cmrs(tdx_cmr_array, &tdx_cmr_num);
 }
 
+/* Check whether the given pfn range is covered by any CMR or not. */
+static bool pfn_range_covered_by_cmr(unsigned long start_pfn,
+				     unsigned long end_pfn)
+{
+	int i;
+
+	for (i = 0; i < tdx_cmr_num; i++) {
+		struct cmr_info *cmr = &tdx_cmr_array[i];
+		unsigned long cmr_start_pfn;
+		unsigned long cmr_end_pfn;
+
+		cmr_start_pfn = cmr->base >> PAGE_SHIFT;
+		cmr_end_pfn = (cmr->base + cmr->size) >> PAGE_SHIFT;
+
+		if (start_pfn >= cmr_start_pfn && end_pfn <= cmr_end_pfn)
+			return true;
+	}

What if the pfn range overlaps two CMRs?  It will never pass any
individual overlap test and will return false.

We can only return true if the two CMRs are contiguous.  

I cannot think out a reason that a reasonable BIOS could generate contiguous
CMRs.  Perhaps one reason is two contiguous NUMA nodes?  For this case, memblock
has made sure no memory region could cross NUMA nodes, so the start_pfn/end_pfn
here should always be within one node.  Perhaps we can add a comment for this
case?

Anyway I am not sure whether it is worth to consider "contiguous CMRs" case.

quoted

+	return false;
+}
+
+/*
+ * Add a memory region on a given node as a TDX memory block.  The caller
+ * to make sure all memory regions are added in address ascending order

s/to/must/

Thanks.

quoted

+ * and don't overlap.
+ */
+static int add_tdx_memblock(unsigned long start_pfn, unsigned long end_pfn,
+			    int nid)
+{
+	struct tdx_memblock *tmb;
+
+	tmb = kmalloc(sizeof(*tmb), GFP_KERNEL);
+	if (!tmb)
+		return -ENOMEM;
+
+	INIT_LIST_HEAD(&tmb->list);
+	tmb->start_pfn = start_pfn;
+	tmb->end_pfn = end_pfn;
+	tmb->nid = nid;
+
+	list_add_tail(&tmb->list, &tdx_memlist);
+	return 0;
+}
+
+static void free_tdx_memory(void)

This is named a bit too generically.  How about free_tdx_memlist() or
something?

Will use free_tdx_memlist().  Do you want to also change build_tdx_memory() to
build_tdx_memlist()?

quoted

+{
+	while (!list_empty(&tdx_memlist)) {
+		struct tdx_memblock *tmb = list_first_entry(&tdx_memlist,
+				struct tdx_memblock, list);
+
+		list_del(&tmb->list);
+		kfree(tmb);
+	}
+}
+
+/*
+ * Add all memblock memory regions to the @tdx_memlist as TDX memory.
+ * Must be called when get_online_mems() is called by the caller.
+ */

Again, this explains the "what", but not the "why".

/*
 * Ensure that all memblock memory regions are convertible to TDX
 * memory.  Once this has been established, stash the memblock
 * ranges off in a secondary structure because $REASONS.
 */

Which makes me wonder: Why do you even need a secondary structure here?
What's wrong with the memblocks themselves?

One reason is the new region has already been added to memblock before calling
arch_add_memory(), so we cannot compare the new region against memblock.

The other  reason is memblock is updated in memory hotplug so it really isn't a
set of *fixed* memory regions, which TDX requires.  Having TDX's own tdx_memlist
can support such case: after module initialization, some memory can be hot-
removed and then hot-added again, because the hot-added range will be covered by
@tdx_memlist.

quoted

+static int build_tdx_memory(void)
+{
+	unsigned long start_pfn, end_pfn;
+	int i, nid, ret;
+
+	for_each_mem_pfn_range(i, MAX_NUMNODES, &start_pfn, &end_pfn, &nid) {
+		/*
+		 * The first 1MB may not be reported as TDX convertible
+		 * memory.  Manually exclude them as TDX memory.

I don't like the "may not" here very much.

quoted

+		 * This is fine as the first 1MB is already reserved in
+		 * reserve_real_mode() and won't end up to ZONE_DMA as
+		 * free page anyway.

			 ^ free pages

quoted

+		 */

This way too wishy washy.  The TDX module may or may not...  Then, it
doesn't matter since reserve_real_mode() does it anyway...

Then it goes and adds code to skip it!

quoted

+		start_pfn = max(start_pfn, (unsigned long)SZ_1M >> PAGE_SHIFT);
+		if (start_pfn >= end_pfn)
+			continue;

Please just put a dang stake in the ground.  If the other code deals
with this, then explain *why* more is needed here.

How about adding below before the 'for_each_mem_pfn_range()' loop:

	/*
	 * Some reserved pages in memblock (i.e. kernel init code/data) are
	 * freed to the page allocator directly.  Use for_each_mem_pfn_range()
	 * instead of for_each_free_mem_range() to make sure all pages in the
	 * page allocator are covered as TDX memory.
	 */

It explains why to use for_each_mem_pfn_range().

And here before skipping first 1MB, we add below:

		/*
		 * The first 1MB is not reported as TDX covertible memory.
		 * Although the first 1MB is always reserved and won't end up
		 * to the page allocator, it is still in memblock's memory
		 * regions.  Skip them manually to exclude them as TDX memory.
		 */

quoted

+		/* Verify memory is truly TDX convertible memory */
+		if (!pfn_range_covered_by_cmr(start_pfn, end_pfn)) {
+			pr_info("Memory region [0x%lx, 0x%lx) is not TDX convertible memorry.\n",
+					start_pfn << PAGE_SHIFT,
+					end_pfn << PAGE_SHIFT);
+			return -EINVAL;

... no 'goto err'?  This leaks all the previous add_tdx_memblock()
structures, right?

Right.  It's a leftover from the old code.  Will fix.

quoted

+		}
+
+		/*
+		 * Add the memory regions as TDX memory.  The regions in
+		 * memblock has already guaranteed they are in address
+		 * ascending order and don't overlap.
+		 */
+		ret = add_tdx_memblock(start_pfn, end_pfn, nid);
+		if (ret)
+			goto err;
+	}
+
+	return 0;
+err:
+	free_tdx_memory();
+	return ret;
+}
+
 /*
  * Detect and initialize the TDX module.
  *

@@ -357,12 +473,56 @@ static int init_tdx_module(void)
 	if (ret)
 		goto out;
 
+	/*
+	 * All memory regions that can be used by the TDX module must be
+	 * passed to the TDX module during the module initialization.
+	 * Once this is done, all "TDX-usable" memory regions are fixed
+	 * during module's runtime.
+	 *
+	 * The initial support of TDX guests only allocates memory from
+	 * the global page allocator.  To keep things simple, for now
+	 * just make sure all pages in the page allocator are TDX memory.
+	 *
+	 * To achieve this, use all system memory in the core-mm at the
+	 * time of initializing the TDX module as TDX memory, and at the
+	 * meantime, reject any new memory in memory hot-add.
+	 *
+	 * This works as in practice, all boot-time present DIMM is TDX
+	 * convertible memory.  However if any new memory is hot-added
+	 * before initializing the TDX module, the initialization will
+	 * fail due to that memory is not covered by CMR.
+	 *
+	 * This can be enhanced in the future, i.e. by allowing adding or
+	 * onlining non-TDX memory to a separate node, in which case the
+	 * "TDX-capable" nodes and the "non-TDX-capable" nodes can exist
+	 * together -- the userspace/kernel just needs to make sure pages
+	 * for TDX guests must come from those "TDX-capable" nodes.
+	 *
+	 * Build the list of TDX memory regions as mentioned above so
+	 * they can be passed to the TDX module later.
+	 */

This is msotly Documentation/, not a code comment.  Please clean it up.

Will try to clean up.

quoted

+	get_online_mems();
+
+	ret = build_tdx_memory();
+	if (ret)
+		goto out;
 	/*
 	 * Return -EINVAL until all steps of TDX module initialization
 	 * process are done.
 	 */
 	ret = -EINVAL;
 out:
+	/*
+	 * Memory hotplug checks the hot-added memory region against the
+	 * @tdx_memlist to see if the region is TDX memory.
+	 *
+	 * Do put_online_mems() here to make sure any modification to
+	 * @tdx_memlist is done while holding the memory hotplug read
+	 * lock, so that the memory hotplug path can just check the
+	 * @tdx_memlist w/o holding the @tdx_module_lock which may cause
+	 * deadlock.
+	 */

I'm honestly not following any of that.

How about:

	/*
	 * Make sure tdx_cc_memory_compatible() either sees a fixed set of
	 * memory regions in @tdx_memlist, or an empty list.
	 */
	

quoted

+	put_online_mems();
 	return ret;
 }
 

@@ -485,3 +645,26 @@ int tdx_enable(void)
 	return ret;
 }
 EXPORT_SYMBOL_GPL(tdx_enable);
+
+/*
+ * Check whether the given range is TDX memory.  Must be called between
+ * mem_hotplug_begin()/mem_hotplug_done().
+ */
+bool tdx_cc_memory_compatible(unsigned long start_pfn, unsigned long end_pfn)
+{
+	struct tdx_memblock *tmb;
+
+	/* Empty list means TDX isn't enabled successfully */
+	if (list_empty(&tdx_memlist))
+		return true;
+
+	list_for_each_entry(tmb, &tdx_memlist, list) {
+		/*
+		 * The new range is TDX memory if it is fully covered
+		 * by any TDX memory block.
+		 */
+		if (start_pfn >= tmb->start_pfn && end_pfn <= tmb->end_pfn)
+			return true;

Same bug.  What if the start/end_pfn range is covered by more than one
tdx_memblock?

We may want to return true if tdx_memblocks are contiguous.

However I don't think this will happen?

tdx_memblock is from memblock, and when two memory regions in memblock are
contiguous, they must have different node, or flags.

My understanding is the hot-added memory region here cannot across NUMA nodes,
nor have different flags,  correct?

quoted

+	}
+	return false;
+}

On 11/23/22 17:04, Huang, Kai wrote:

On Tue, 2022-11-22 at 16:21 -0800, Dave Hansen wrote:

quoted

quoted

+struct tdx_memblock {
+   struct list_head list;
+   unsigned long start_pfn;
+   unsigned long end_pfn;
+   int nid;
+};

Why does the nid matter?

It is used to find the node for the PAMT allocation for a given TDMR.

... which is in this patch?

You can't just plop unused and unmentioned nuggets in the code.  Remove
it until it is needed.

quoted

quoted

+/* Check whether the given pfn range is covered by any CMR or not. */
+static bool pfn_range_covered_by_cmr(unsigned long start_pfn,
+                                unsigned long end_pfn)
+{
+   int i;
+
+   for (i = 0; i < tdx_cmr_num; i++) {
+           struct cmr_info *cmr = &tdx_cmr_array[i];
+           unsigned long cmr_start_pfn;
+           unsigned long cmr_end_pfn;
+
+           cmr_start_pfn = cmr->base >> PAGE_SHIFT;
+           cmr_end_pfn = (cmr->base + cmr->size) >> PAGE_SHIFT;
+
+           if (start_pfn >= cmr_start_pfn && end_pfn <= cmr_end_pfn)
+                   return true;
+   }

What if the pfn range overlaps two CMRs?  It will never pass any
individual overlap test and will return false.

We can only return true if the two CMRs are contiguous.

I cannot think out a reason that a reasonable BIOS could generate contiguous
CMRs.

Because it can?

We don't just try and randomly assign what we think is reasonable or
not.  First and foremost, we need to ask whether the configuration in
question is allowed by the spec.

Would it be a *valid* thing to have two adjacent CMRs?  Does the TDX
module spec disallow it?

Perhaps one reason is two contiguous NUMA nodes?  For this case, memblock
has made sure no memory region could cross NUMA nodes, so the start_pfn/end_pfn
here should always be within one node.  Perhaps we can add a comment for this
case?

<cough> numa=off <cough>

Anyway I am not sure whether it is worth to consider "contiguous CMRs" case.

I am sure.  You need to consider it.

quoted

quoted

+ * and don't overlap.
+ */
+static int add_tdx_memblock(unsigned long start_pfn, unsigned long end_pfn,
+                       int nid)
+{
+   struct tdx_memblock *tmb;
+
+   tmb = kmalloc(sizeof(*tmb), GFP_KERNEL);
+   if (!tmb)
+           return -ENOMEM;
+
+   INIT_LIST_HEAD(&tmb->list);
+   tmb->start_pfn = start_pfn;
+   tmb->end_pfn = end_pfn;
+   tmb->nid = nid;
+
+   list_add_tail(&tmb->list, &tdx_memlist);
+   return 0;
+}
+
+static void free_tdx_memory(void)

This is named a bit too generically.  How about free_tdx_memlist() or
something?

Will use free_tdx_memlist().  Do you want to also change build_tdx_memory() to
build_tdx_memlist()?

Does it build a memlist?

quoted

quoted

+{
+   while (!list_empty(&tdx_memlist)) {
+           struct tdx_memblock *tmb = list_first_entry(&tdx_memlist,
+                           struct tdx_memblock, list);
+
+           list_del(&tmb->list);
+           kfree(tmb);
+   }
+}
+
+/*
+ * Add all memblock memory regions to the @tdx_memlist as TDX memory.
+ * Must be called when get_online_mems() is called by the caller.
+ */

Again, this explains the "what", but not the "why".

/*
 * Ensure that all memblock memory regions are convertible to TDX
 * memory.  Once this has been established, stash the memblock
 * ranges off in a secondary structure because $REASONS.
 */

Which makes me wonder: Why do you even need a secondary structure here?
What's wrong with the memblocks themselves?

One reason is the new region has already been added to memblock before calling
arch_add_memory(), so we cannot compare the new region against memblock.

The other  reason is memblock is updated in memory hotplug so it really isn't a
set of *fixed* memory regions, which TDX requires.  Having TDX's own tdx_memlist
can support such case: after module initialization, some memory can be hot-
removed and then hot-added again, because the hot-added range will be covered by
@tdx_memlist.

OK, that's fair enough.  Memblocks change and we don't lock out changes
to memblocks.  But, the TDX setup is based on memblocks at one point in
time, so we effectively need to know what their state was at that point
in time.

quoted

quoted

+static int build_tdx_memory(void)
+{
+   unsigned long start_pfn, end_pfn;
+   int i, nid, ret;
+
+   for_each_mem_pfn_range(i, MAX_NUMNODES, &start_pfn, &end_pfn, &nid) {
+           /*
+            * The first 1MB may not be reported as TDX convertible
+            * memory.  Manually exclude them as TDX memory.

I don't like the "may not" here very much.

quoted

+            * This is fine as the first 1MB is already reserved in
+            * reserve_real_mode() and won't end up to ZONE_DMA as
+            * free page anyway.

                       ^ free pages

quoted

+            */

This way too wishy washy.  The TDX module may or may not...  Then, it
doesn't matter since reserve_real_mode() does it anyway...

Then it goes and adds code to skip it!

quoted

+           start_pfn = max(start_pfn, (unsigned long)SZ_1M >> PAGE_SHIFT);
+           if (start_pfn >= end_pfn)
+                   continue;

Please just put a dang stake in the ground.  If the other code deals
with this, then explain *why* more is needed here.

How about adding below before the 'for_each_mem_pfn_range()' loop:

        /*
         * Some reserved pages in memblock (i.e. kernel init code/data) are
         * freed to the page allocator directly.  Use for_each_mem_pfn_range()
         * instead of for_each_free_mem_range() to make sure all pages in the
         * page allocator are covered as TDX memory.
         */

It explains why to use for_each_mem_pfn_range().

I actually wasn't asking about the for_each_mem_pfn_range() use.

And here before skipping first 1MB, we add below:

                /*
                 * The first 1MB is not reported as TDX covertible memory.
                 * Although the first 1MB is always reserved and won't end up
                 * to the page allocator, it is still in memblock's memory
                 * regions.  Skip them manually to exclude them as TDX memory.
                 */

That looks OK, with the spelling fixed.

quoted

quoted

+           /* Verify memory is truly TDX convertible memory */
+           if (!pfn_range_covered_by_cmr(start_pfn, end_pfn)) {
+                   pr_info("Memory region [0x%lx, 0x%lx) is not TDX convertible memorry.\n",
+                                   start_pfn << PAGE_SHIFT,
+                                   end_pfn << PAGE_SHIFT);
+                   return -EINVAL;

... no 'goto err'?  This leaks all the previous add_tdx_memblock()
structures, right?

Right.  It's a leftover from the old code.  Will fix.

quoted

quoted

+           }
+
+           /*
+            * Add the memory regions as TDX memory.  The regions in
+            * memblock has already guaranteed they are in address
+            * ascending order and don't overlap.
+            */
+           ret = add_tdx_memblock(start_pfn, end_pfn, nid);
+           if (ret)
+                   goto err;
+   }
+
+   return 0;
+err:
+   free_tdx_memory();
+   return ret;
+}
+
 /*
  * Detect and initialize the TDX module.
  *

@@ -357,12 +473,56 @@ static int init_tdx_module(void)
    if (ret)
            goto out;

+   /*
+    * All memory regions that can be used by the TDX module must be
+    * passed to the TDX module during the module initialization.
+    * Once this is done, all "TDX-usable" memory regions are fixed
+    * during module's runtime.
+    *
+    * The initial support of TDX guests only allocates memory from
+    * the global page allocator.  To keep things simple, for now
+    * just make sure all pages in the page allocator are TDX memory.
+    *
+    * To achieve this, use all system memory in the core-mm at the
+    * time of initializing the TDX module as TDX memory, and at the
+    * meantime, reject any new memory in memory hot-add.
+    *
+    * This works as in practice, all boot-time present DIMM is TDX
+    * convertible memory.  However if any new memory is hot-added
+    * before initializing the TDX module, the initialization will
+    * fail due to that memory is not covered by CMR.
+    *
+    * This can be enhanced in the future, i.e. by allowing adding or
+    * onlining non-TDX memory to a separate node, in which case the
+    * "TDX-capable" nodes and the "non-TDX-capable" nodes can exist
+    * together -- the userspace/kernel just needs to make sure pages
+    * for TDX guests must come from those "TDX-capable" nodes.
+    *
+    * Build the list of TDX memory regions as mentioned above so
+    * they can be passed to the TDX module later.
+    */

This is msotly Documentation/, not a code comment.  Please clean it up.

Will try to clean up.

quoted

quoted

+   get_online_mems();
+
+   ret = build_tdx_memory();
+   if (ret)
+           goto out;
    /*
     * Return -EINVAL until all steps of TDX module initialization
     * process are done.
     */
    ret = -EINVAL;
 out:
+   /*
+    * Memory hotplug checks the hot-added memory region against the
+    * @tdx_memlist to see if the region is TDX memory.
+    *
+    * Do put_online_mems() here to make sure any modification to
+    * @tdx_memlist is done while holding the memory hotplug read
+    * lock, so that the memory hotplug path can just check the
+    * @tdx_memlist w/o holding the @tdx_module_lock which may cause
+    * deadlock.
+    */

I'm honestly not following any of that.

How about:

        /*
         * Make sure tdx_cc_memory_compatible() either sees a fixed set of
         * memory regions in @tdx_memlist, or an empty list.
         */

That's a comment for the lock side, not the unlock side.  It should be:

	/*
	 * @tdx_memlist is written here and read at memory hotplug time.
	 * Lock out memory hotplug code while building it.
	 */

quoted

quoted

+   put_online_mems();
    return ret;
 }

@@ -485,3 +645,26 @@ int tdx_enable(void)
    return ret;
 }
 EXPORT_SYMBOL_GPL(tdx_enable);
+
+/*
+ * Check whether the given range is TDX memory.  Must be called between
+ * mem_hotplug_begin()/mem_hotplug_done().
+ */
+bool tdx_cc_memory_compatible(unsigned long start_pfn, unsigned long end_pfn)
+{
+   struct tdx_memblock *tmb;
+
+   /* Empty list means TDX isn't enabled successfully */
+   if (list_empty(&tdx_memlist))
+           return true;
+
+   list_for_each_entry(tmb, &tdx_memlist, list) {
+           /*
+            * The new range is TDX memory if it is fully covered
+            * by any TDX memory block.
+            */
+           if (start_pfn >= tmb->start_pfn && end_pfn <= tmb->end_pfn)
+                   return true;

Same bug.  What if the start/end_pfn range is covered by more than one
tdx_memblock?

We may want to return true if tdx_memblocks are contiguous.

However I don't think this will happen?

tdx_memblock is from memblock, and when two memory regions in memblock are
contiguous, they must have different node, or flags.

My understanding is the hot-added memory region here cannot across NUMA nodes,
nor have different flags,  correct?

I'm not sure what flags are in this context.

Kai Huang wrote:

arch_add_memory() does not add memory to the page allocator.  For
example, memremap_pages() uses arch_add_memory() and explicitly does not
release the memory to the page allocator. This check belongs in
add_memory_resource() to prevent new memory that violates TDX from being
onlined. Hopefully there is also an option to disable TDX from the
kernel boot command line to recover memory-hotplug without needing to
boot into the BIOS to toggle TDX.

On Wed, 2022-11-23 at 17:22 -0800, Hansen, Dave wrote:

On 11/23/22 17:04, Huang, Kai wrote:

quoted

On Tue, 2022-11-22 at 16:21 -0800, Dave Hansen wrote:

quoted

quoted

+struct tdx_memblock {
+   struct list_head list;
+   unsigned long start_pfn;
+   unsigned long end_pfn;
+   int nid;
+};

Why does the nid matter?

It is used to find the node for the PAMT allocation for a given TDMR.

... which is in this patch?

You can't just plop unused and unmentioned nuggets in the code.  Remove
it until it is needed.

OK. I'll move to the PAMT allocation patch.

quoted

quoted

quoted

+/* Check whether the given pfn range is covered by any CMR or not. */
+static bool pfn_range_covered_by_cmr(unsigned long start_pfn,
+                                unsigned long end_pfn)
+{
+   int i;
+
+   for (i = 0; i < tdx_cmr_num; i++) {
+           struct cmr_info *cmr = &tdx_cmr_array[i];
+           unsigned long cmr_start_pfn;
+           unsigned long cmr_end_pfn;
+
+           cmr_start_pfn = cmr->base >> PAGE_SHIFT;
+           cmr_end_pfn = (cmr->base + cmr->size) >> PAGE_SHIFT;
+
+           if (start_pfn >= cmr_start_pfn && end_pfn <= cmr_end_pfn)
+                   return true;
+   }

What if the pfn range overlaps two CMRs?  It will never pass any
individual overlap test and will return false.

We can only return true if the two CMRs are contiguous.

I cannot think out a reason that a reasonable BIOS could generate contiguous
CMRs.

Because it can?

We don't just try and randomly assign what we think is reasonable or
not.  First and foremost, we need to ask whether the configuration in
question is allowed by the spec.

Would it be a *valid* thing to have two adjacent CMRs?  Does the TDX
module spec disallow it?

No the TDX module doesn't disallow it, IIUC.  The spec only says they don't
overlap.

quoted

Perhaps one reason is two contiguous NUMA nodes?  For this case, memblock
has made sure no memory region could cross NUMA nodes, so the start_pfn/end_pfn
here should always be within one node.  Perhaps we can add a comment for this
case?

<cough> numa=off <cough>

quoted

Anyway I am not sure whether it is worth to consider "contiguous CMRs" case.

I am sure.  You need to consider it.

OK.

Also, as mentioned in another reply to patch "Get information about TDX module
and TDX-capable memory", we can depend on TDH.SYS.CONFIG to return failure but
don't necessarily need to sanity check all memory regions are CMR memory.  This
way we can just removing above sanity check code here.

What do you think?

quoted

quoted

quoted

+ * and don't overlap.
+ */
+static int add_tdx_memblock(unsigned long start_pfn, unsigned long end_pfn,
+                       int nid)
+{
+   struct tdx_memblock *tmb;
+
+   tmb = kmalloc(sizeof(*tmb), GFP_KERNEL);
+   if (!tmb)
+           return -ENOMEM;
+
+   INIT_LIST_HEAD(&tmb->list);
+   tmb->start_pfn = start_pfn;
+   tmb->end_pfn = end_pfn;
+   tmb->nid = nid;
+
+   list_add_tail(&tmb->list, &tdx_memlist);
+   return 0;
+}
+
+static void free_tdx_memory(void)

This is named a bit too generically.  How about free_tdx_memlist() or
something?

Will use free_tdx_memlist().  Do you want to also change build_tdx_memory() to
build_tdx_memlist()?

Does it build a memlist?

Yes.


[...]

I actually wasn't asking about the for_each_mem_pfn_range() use.

quoted

And here before skipping first 1MB, we add below:

                /*
                 * The first 1MB is not reported as TDX covertible memory.
                 * Although the first 1MB is always reserved and won't end up
                 * to the page allocator, it is still in memblock's memory
                 * regions.  Skip them manually to exclude them as TDX memory.
                 */

That looks OK, with the spelling fixed.

Yes "covertible" -> "convertible".


[...]

quoted

quoted

quoted

 out:
+   /*
+    * Memory hotplug checks the hot-added memory region against the
+    * @tdx_memlist to see if the region is TDX memory.
+    *
+    * Do put_online_mems() here to make sure any modification to
+    * @tdx_memlist is done while holding the memory hotplug read
+    * lock, so that the memory hotplug path can just check the
+    * @tdx_memlist w/o holding the @tdx_module_lock which may cause
+    * deadlock.
+    */

I'm honestly not following any of that.

How about:

        /*
         * Make sure tdx_cc_memory_compatible() either sees a fixed set of
         * memory regions in @tdx_memlist, or an empty list.
         */

That's a comment for the lock side, not the unlock side.  It should be:

	/*
	 * @tdx_memlist is written here and read at memory hotplug time.
	 * Lock out memory hotplug code while building it.
	 */

Thanks.

quoted

quoted

quoted

+   put_online_mems();
    return ret;
 }

@@ -485,3 +645,26 @@ int tdx_enable(void)
    return ret;
 }
 EXPORT_SYMBOL_GPL(tdx_enable);
+
+/*
+ * Check whether the given range is TDX memory.  Must be called between
+ * mem_hotplug_begin()/mem_hotplug_done().
+ */
+bool tdx_cc_memory_compatible(unsigned long start_pfn, unsigned long end_pfn)
+{
+   struct tdx_memblock *tmb;
+
+   /* Empty list means TDX isn't enabled successfully */
+   if (list_empty(&tdx_memlist))
+           return true;
+
+   list_for_each_entry(tmb, &tdx_memlist, list) {
+           /*
+            * The new range is TDX memory if it is fully covered
+            * by any TDX memory block.
+            */
+           if (start_pfn >= tmb->start_pfn && end_pfn <= tmb->end_pfn)
+                   return true;

Same bug.  What if the start/end_pfn range is covered by more than one
tdx_memblock?

We may want to return true if tdx_memblocks are contiguous.

However I don't think this will happen?

tdx_memblock is from memblock, and when two memory regions in memblock are
contiguous, they must have different node, or flags.

My understanding is the hot-added memory region here cannot across NUMA nodes,
nor have different flags,  correct?

I'm not sure what flags are in this context.

The flags in 'struct memblock_region':

enum memblock_flags {           
        MEMBLOCK_NONE           = 0x0,  /* No special request */
        MEMBLOCK_HOTPLUG        = 0x1,  /* hotpluggable region */              
        MEMBLOCK_MIRROR         = 0x2,  /* mirrored region */                  
        MEMBLOCK_NOMAP          = 0x4,  /* don't add to kernel direct mapping */
        MEMBLOCK_DRIVER_MANAGED = 0x8,  /* always detected via a driver */     
};      
        
/**                                                                            
 * struct memblock_region - represents a memory region                         
 * @base: base address of the region                                           
 * @size: size of the region                                                   
 * @flags: memory region attributes
 * @nid: NUMA node id
 */             
struct memblock_region {
        phys_addr_t base;
        phys_addr_t size;
        enum memblock_flags flags;
#ifdef CONFIG_NUMA
        int nid;
#endif  
};      

On Wed, 2022-11-23 at 17:50 -0800, Dan Williams wrote:

quoted

@@ -968,6 +969,15 @@ int arch_add_memory(int nid, u64 start, u64 size,

  	unsigned long start_pfn = start >> PAGE_SHIFT;
  	unsigned long nr_pages = size >> PAGE_SHIFT;
  
+	/*
+	 * For now if TDX is enabled, all pages in the page allocator
+	 * must be TDX memory, which is a fixed set of memory regions
+	 * that are passed to the TDX module.  Reject the new region
+	 * if it is not TDX memory to guarantee above is true.
+	 */
+	if (!tdx_cc_memory_compatible(start_pfn, start_pfn + nr_pages))
+		return -EINVAL;

arch_add_memory() does not add memory to the page allocator.  For
example, memremap_pages() uses arch_add_memory() and explicitly does not
release the memory to the page allocator. 

Indeed.  Sorry I missed this.

This check belongs in
add_memory_resource() to prevent new memory that violates TDX from being
onlined. 

This would require adding another 'arch_cc_memory_compatible()' to the common
add_memory_resource() (I actually long time ago had such patch to work with the
memremap_pages() you mentioned above).

How about adding a memory_notifier to the TDX code, and reject online of TDX
incompatible memory (something like below)?  The benefit is this is TDX code
self contained and won't pollute the common mm code:

+static int tdx_memory_notifier(struct notifier_block *nb,
+                              unsigned long action, void *v)
+{
+       struct memory_notify *mn = v;
+
+       if (action != MEM_GOING_ONLINE)
+               return NOTIFY_OK;
+
+       /*
+        * Not all memory is compatible with TDX.  Reject
+        * online of any incompatible memory.
+        */
+       return tdx_cc_memory_compatible(mn->start_pfn,
+                       mn->start_pfn + mn->nr_pages) ? NOTIFY_OK : NOTIFY_BAD;
+}
+
+static struct notifier_block tdx_memory_nb = {
+       .notifier_call = tdx_memory_notifier,
+};

Hopefully there is also an option to disable TDX from the
kernel boot command line to recover memory-hotplug without needing to
boot into the BIOS to toggle TDX.

I am fine.

Hi Dave, is it OK to include such command line to this initial TDX submission?

On Wed, Nov 23, 2022 at 05:50:37PM -0800, Dan Williams wrote:

arch_add_memory() does not add memory to the page allocator.  For
example, memremap_pages() uses arch_add_memory() and explicitly does not
release the memory to the page allocator. This check belongs in
add_memory_resource() to prevent new memory that violates TDX from being
onlined. Hopefully there is also an option to disable TDX from the
kernel boot command line to recover memory-hotplug without needing to
boot into the BIOS to toggle TDX.

So I've been pushing for all this to either require: tdx=force on the
cmdline to boot-time enable, or delay all the memory allocation to the
first KVM/TDX instance being created.

That is, by default, none of this crud should ever trigger and consume
memory if you're not using TDX (most of us really).

(every machine I have loads kvm.ko unconditionally -- even if I never
user KVM, so kvm.ko load time is not a valid point in time to do TDX
enablement).

On Wed, 2022-11-23 at 14:17 -0800, Dave Hansen wrote:

On 11/20/22 16:26, Kai Huang wrote:

quoted

TDX provides increased levels of memory confidentiality and integrity.
This requires special hardware support for features like memory
encryption and storage of memory integrity checksums.  Not all memory
satisfies these requirements.

As a result, the TDX introduced the concept of a "Convertible Memory

s/the TDX introduced/TDX introduces/

quoted

Region" (CMR).  During boot, the firmware builds a list of all of the
memory ranges which can provide the TDX security guarantees.  The list
of these ranges is available to the kernel by querying the TDX module.

The TDX architecture needs additional metadata to record things like
which TD guest "owns" a given page of memory.  This metadata essentially
serves as the 'struct page' for the TDX module.  The space for this
metadata is not reserved by the hardware up front and must be allocated
by the kernel and given to the TDX module.

Since this metadata consumes space, the VMM can choose whether or not to
allocate it for a given area of convertible memory.  If it chooses not
to, the memory cannot receive TDX protections and can not be used by TDX
guests as private memory.

For every memory region that the VMM wants to use as TDX memory, it sets
up a "TD Memory Region" (TDMR).  Each TDMR represents a physically
contiguous convertible range and must also have its own physically
contiguous metadata table, referred to as a Physical Address Metadata
Table (PAMT), to track status for each page in the TDMR range.

Unlike a CMR, each TDMR requires 1G granularity and alignment.  To
support physical RAM areas that don't meet those strict requirements,
each TDMR permits a number of internal "reserved areas" which can be
placed over memory holes.  If PAMT metadata is placed within a TDMR it
must be covered by one of these reserved areas.

Let's summarize the concepts:

 CMR - Firmware-enumerated physical ranges that support TDX.  CMRs are
       4K aligned.
TDMR - Physical address range which is chosen by the kernel to support
       TDX.  1G granularity and alignment required.  Each TDMR has
       reserved areas where TDX memory holes and overlapping PAMTs can
       be put into.

s/put into/represented/

quoted

PAMT - Physically contiguous TDX metadata.  One table for each page size
       per TDMR.  Roughly 1/256th of TDMR in size.  256G TDMR = ~1G
       PAMT.

As one step of initializing the TDX module, the kernel configures
TDX-usable memory regions by passing an array of TDMRs to the TDX module.

Constructing the array of TDMRs consists below steps:

1) Create TDMRs to cover all memory regions that the TDX module can use;

Slight tweak:

1) Create TDMRs to cover all memory regions that the TDX module will use
   for TD memory

The TDX module "uses" more memory than strictly the TMDR's.

quoted

2) Allocate and set up PAMT for each TDMR;
3) Set up reserved areas for each TDMR.

s/Set up/Designate/

Thanks. All above will be addressed.

quoted

Add a placeholder to construct TDMRs to do the above steps after all
TDX memory regions are verified to be truly convertible.  Always free
TDMRs at the end of the initialization (no matter successful or not)
as TDMRs are only used during the initialization.

The changelog here actually looks really good to me so far.

quoted

diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
index 32af86e31c47..26048c6b0170 100644
--- a/arch/x86/virt/vmx/tdx/tdx.c
+++ b/arch/x86/virt/vmx/tdx/tdx.c

@@ -445,6 +445,63 @@ static int build_tdx_memory(void)
 	return ret;
 }
 
+/* Calculate the actual TDMR_INFO size */
+static inline int cal_tdmr_size(void)

I think we can spare the bytes to add "culate" in the function name so
we don't think these are California TDMRs.

Sure will do.

quoted

+{
+	int tdmr_sz;
+
+	/*
+	 * The actual size of TDMR_INFO depends on the maximum number
+	 * of reserved areas.
+	 *
+	 * Note: for TDX1.0 the max_reserved_per_tdmr is 16, and
+	 * TDMR_INFO size is aligned up to 512-byte.  Even it is
+	 * extended in the future, it would be insane if TDMR_INFO
+	 * becomes larger than 4K.  The tdmr_sz here should never
+	 * overflow.
+	 */
+	tdmr_sz = sizeof(struct tdmr_info);
+	tdmr_sz += sizeof(struct tdmr_reserved_area) *
+		   tdx_sysinfo.max_reserved_per_tdmr;

First, I think 'tdx_sysinfo' should probably be a local variable in
init_tdx_module() and have its address passed in here.  Having global
variables always makes it more opaque about who is initializing it.

Second, if this code is making assumptions about
'max_reserved_per_tdmr', then let's actually add assertions or sanity
checks.  For instance:

	if (tdx_sysinfo.max_reserved_per_tdmr > MAX_TDMRS)
		return -1;

or even:

	if (tdmr_sz > PAGE_SIZE)
		return -1;

I can add this.

It does almost no good to just assert what the limits are in a comment.

quoted

+	/*
+	 * TDX requires each TDMR_INFO to be 512-byte aligned.  Always
+	 * round up TDMR_INFO size to the 512-byte boundary.
+	 */

<sigh> More silly comments.

The place to document this is TDMR_INFO_ALIGNMENT.  If anyone wants to
know what the alignment is, exactly, they can look at the definition.
They don't need to be told *TWICE* what TDMR_INFO_ALIGNMENT #defines to
in one comment.

I see.  Then I think we don't even need this comment since the name of
TDMR_INFO_ALIGNMENT already implies?

quoted

+	return ALIGN(tdmr_sz, TDMR_INFO_ALIGNMENT);
+}
+
+static struct tdmr_info *alloc_tdmr_array(int *array_sz)
+{
+	/*
+	 * TDX requires each TDMR_INFO to be 512-byte aligned.
+	 * Use alloc_pages_exact() to allocate all TDMRs at once.
+	 * Each TDMR_INFO will still be 512-byte aligned since
+	 * cal_tdmr_size() always returns 512-byte aligned size.
+	 */

OK, I think you're just trolling me now.  Two *MORE* mentions of the
512-byte alignment?

I'll remove.

quoted

+	*array_sz = cal_tdmr_size() * tdx_sysinfo.max_tdmrs;
+
+	/*
+	 * Zero the buffer so 'struct tdmr_info::size' can be
+	 * used to determine whether a TDMR is valid.
+	 *
+	 * Note: for TDX1.0 the max_tdmrs is 64 and TDMR_INFO size
+	 * is 512-byte.  Even they are extended in the future, it
+	 * would be insane if the total size exceeds 4MB.
+	 */
+	return alloc_pages_exact(*array_sz, GFP_KERNEL | __GFP_ZERO);
+}

This looks massively over complicated.

Get rid of this function entirely.  Then create:

static int tdmr_array_size(void)
{
	return tdmr_size_single() * tdx_sysinfo.max_tdmrs;
}

The *caller* can do:

	tdmr_array = alloc_pages_exact(tdmr_array_size(),
				       GFP_KERNEL | __GFP_ZERO);
	if (!tdmr_array) {
		...

Then the error path is:

	free_pages_exact(tdmr_array, tdmr_array_size());

Then, there are no size pointers going back and forth.  Easy peasy.  I'm
OK with a little arithmetic being repeated.

Yes.  Will do.

quoted

+/*
+ * Construct an array of TDMRs to cover all TDX memory ranges.
+ * The actual number of TDMRs is kept to @tdmr_num.
+ */
+static int construct_tdmrs(struct tdmr_info *tdmr_array, int *tdmr_num)
+{
+	/* Return -EINVAL until constructing TDMRs is done */
+	return -EINVAL;
+}
+
 /*
  * Detect and initialize the TDX module.
  *

@@ -454,6 +511,9 @@ static int build_tdx_memory(void)
  */
 static int init_tdx_module(void)
 {
+	struct tdmr_info *tdmr_array;
+	int tdmr_array_sz;
+	int tdmr_num;

I tend to write these like:

"tdmr_num" is the number of *a* TDMR.

"nr_tdmrs" is the number of TDMRs.

Indeed.  Will do.

On Thu, 2022-11-24 at 10:26 +0100, Peter Zijlstra wrote:

On Wed, Nov 23, 2022 at 05:50:37PM -0800, Dan Williams wrote:

quoted

arch_add_memory() does not add memory to the page allocator.  For
example, memremap_pages() uses arch_add_memory() and explicitly does not
release the memory to the page allocator. This check belongs in
add_memory_resource() to prevent new memory that violates TDX from being
onlined. Hopefully there is also an option to disable TDX from the
kernel boot command line to recover memory-hotplug without needing to
boot into the BIOS to toggle TDX.

So I've been pushing for all this to either require: tdx=force on the
cmdline to boot-time enable, or delay all the memory allocation to the
first KVM/TDX instance being created.

That is, by default, none of this crud should ever trigger and consume
memory if you're not using TDX (most of us really).

(every machine I have loads kvm.ko unconditionally -- even if I never
user KVM, so kvm.ko load time is not a valid point in time to do TDX
enablement).

Thanks for input.  I am fine with 'tdx=force'.

Although, I'd like to point out KVM will have a module parameter 'enable_tdx'.

Hi Dave, Sean, do you have any comments?

quoted

+static inline u64 tdmr_start(struct tdmr_info *tdmr)
+{
+	return tdmr->base;
+}

I'm always skeptical that it's a good idea to take this in code:

	tdmr->base

and make it this:

	tdmr_start(tdmr)

because the helper is *LESS* compact than the open-coded form!  I hope
I'm proven wrong.

IIUC you prefer using tdmr->base directly. Will do.

quoted

+static inline u64 tdmr_end(struct tdmr_info *tdmr)
+{
+	return tdmr->base + tdmr->size;
+}
+
 /* Calculate the actual TDMR_INFO size */
 static inline int cal_tdmr_size(void)
 {

@@ -492,14 +510,98 @@ static struct tdmr_info *alloc_tdmr_array(int *array_sz)
 	return alloc_pages_exact(*array_sz, GFP_KERNEL | __GFP_ZERO);
 }
 
+static struct tdmr_info *tdmr_array_entry(struct tdmr_info *tdmr_array,
+					  int idx)
+{
+	return (struct tdmr_info *)((unsigned long)tdmr_array +
+			cal_tdmr_size() * idx);
+}

FWIW, I think it's probably a bad idea to have 'struct tdmr_info *'
types floating around since:

	tmdr_info_array[0]

works, but:

	tmdr_info_array[1]

will blow up in your face.  It would almost make sense to have

struct tdmr_info_list {
	struct tdmr_info *first_tdmr;
}

and then pass around pointers to the 'struct tdmr_info_list'.  Maybe
that's overkill, but it is kinda silly to call something an array if []
doesn't work on it.