Re: [PATCH v3 26/26] coco/tdx-host: Set and document TDX Module update expectations

[<dan.j.williams@intel.com>]

Chao Gao wrote:
[..]
> >So, remove "compat_capable" ABI. Amend the "error" ABI documentation
> >with the details for avoiding failures and the risk of running updates
> >on configurations that support update but not collision avoidance.
> 
> Got it. I will modify this patch as follows:

Overall, looks good to me. You can add:

Reviewed-by: Dan Williams <dan.j.williams@intel.com>

...after a few additional fixups below:

> diff --git a/Documentation/ABI/testing/sysfs-devices-faux-tdx-host b/Documentation/ABI/testing/sysfs-devices-faux-tdx-host
> index a3f155977016..0a68e68375fa 100644
> --- a/Documentation/ABI/testing/sysfs-devices-faux-tdx-host
> +++ b/Documentation/ABI/testing/sysfs-devices-faux-tdx-host
> @@ -29,3 +29,57 @@ Description:	(RO) Report the number of remaining updates that can be performed.
> 		4.2 "SEAMLDR.INSTALL" for more information. The documentation is
> 		available at:
> 		https://cdrdv2-public.intel.com/739045/intel-tdx-seamldr-interface-specification.pdf
> +
> +What:		/sys/devices/faux/tdx_host/firmware/seamldr_upload
> +Contact:	linux-coco@lists.linux.dev
> +Description:	(Directory) The seamldr_upload directory implements the
> +		fw_upload sysfs ABI, see
> +		Documentation/ABI/testing/sysfs-class-firmware for the general
> +		description of the attributes @data, @cancel, @error, @loading,
> +		@remaining_size, and @status. This ABI facilitates "Compatible
> +		TDX Module Updates". A compatible update is one that meets the
> +		following criteria:
> +
> +		   Does not interrupt or interfere with any current TDX
> +		   operation or TD VM.
> +
> +		   Does not invalidate any previously consumed Module metadata
> +		   values outside of the TEE_TCB_SVN_2 field (updated Security
> +		   Version Number) in TD Quotes.
> +
> +		   Does not require validation of new Module metadata fields. By
> +		   implication, new Module features and capabilities are only
> +		   available by installing the Module at reboot (BIOS or EFI
> +		   helper loaded).
> +
> +		See tdx_host/firmware/seamldr_upload/error for more details.
> +
> +What:		/sys/devices/faux/tdx_host/firmware/seamldr_upload/error
> +Contact:	linux-coco@lists.linux.dev
> +Description:	(RO) See Documentation/ABI/testing/sysfs-class-firmware for
> +		baseline expectations for this file. The <ERROR> part in the
> +		<STATUS>:<ERROR> format can be:
> +
> +		   "device-busy": Compatibility checks failed or not all CPUs
> +		                  are online
> +		   "flash-wearout": the number of updates reached the limit.
> +		   "read-write-error": Memory allocation failed.
> +		   "hw-error": Cannot communicate with P-SEAMLDR or TDX Module
> +		   "firmware-invalid": The TDX Module to be installed is invalid
> +		                       or other unexpected errors occurred.
> +
> +		"hw-error" or "firmware-invalid" may be fatal, causing all TDs
> +		and the TDX Module to be lost and preventing further TDX
> +		operations. This occurs when /sys/devices/faux/tdx_host/version
> +		becomes unreadable after update failures.

I would specify the exact unambiguous errno value that gets returned on
read when the version become indeterminate, like ENXIO.

> +		and the (previous) TDX Module stay running.
> +
> +		On certain earlier TDX Module versions, incompatible updates may
> +		not trigger "device-busy" errors but instead cause TD
> +		attestation failures.

I would just leave this out. It bitrots quickly and does not provide
any actionable information. This is not the kernel's responsibility...

> +
> +		See version_select_and_load.py [1] documentation for how to
> +		detect compatible updates and whether the current platform
> +		components catch errors or let them leak and cause potential TD
> +		attestation failures.
> +		[1]: https://github.com/intel/confidential-computing.tdx.tdx-module.binaries/blob/main/version_select_and_load.py

...that detail about what happens when compat detection is missing
belongs in the tooling documentation. That documentation does not exist
yet, so this link needs to be replaced with a pointer to documentation
before this goes upstream. I am assuming that we want to create an
actual package that distributions can pick up as project? It might be
worth going through the exercise of packaging the binaries and the tool
as an rpm or deb to get that work bootstrapped.
"version_select_and_load" probably wants a better name like "tdxctl" or
similar.

Note that a tdxctl project would also attract features related to TDX
Connect to wrap common flows around the tdx_host device sysfs ABIs.