RE: [PATCH] virt: tdx-guest: Deprecate legacy IOCTL-based interface for quote generation

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

Nikolay Borisov wrote:
> IOCTL based interface was the natural choice for interacting with the
> quote generation machine at a time when there wasn't anything better.

That's not quite how I would phrase it. Perhaps just quote configfs-tsm
changelog? Something like:

---
"The commit that added configfs-tsm noted that the concept of
attestation reports is '...common across TSMs, but the implementations
are unfortunately vendor specific. While the industry grapples with a
common definition of this attestation format [1], Linux need not make
this problem worse by defining a new ABI per TSM that wants to perform a
similar operation. The current momentum has been to invent new ioctl-ABI
per TSM per function which at best is an abdication of the kernel's
responsibility to make common infrastructure concepts share common
ABI.'

While the SEV-SNP implementation exposed remotely verifiable reports
from day1 the TDX implementation started with a locally verifiable
report retrieved over ioctl, but adopted configs-tsm for conveying its
remotely verifiable report"
---

Then there needs to be a discussion here to describe who is still using
the local attestation reports, what their migration path is (if one is
needed), and some indication how does a distribution vendor know when
those users have migrated or dropped their interest.

> Fortunately, now we have a vendor-agnostic, configfs-based one which
> obviates the need to have the IOCTL-based interface.
> 
> Gate the relevant code behind a Kconfig option, clearly marking it as
> deprecated as well as introduce a runtime warning.
> 
> Signed-off-by: Nikolay Borisov <nik.borisov@suse.com>
> ---
>  drivers/virt/coco/tdx-guest/Kconfig     |  9 +++++++++
>  drivers/virt/coco/tdx-guest/tdx-guest.c | 13 +++++++++++++
>  2 files changed, 22 insertions(+)
> 
> diff --git a/drivers/virt/coco/tdx-guest/Kconfig b/drivers/virt/coco/tdx-guest/Kconfig
> index 22dd59e19431..0f1cfdfbbd28 100644
> --- a/drivers/virt/coco/tdx-guest/Kconfig
> +++ b/drivers/virt/coco/tdx-guest/Kconfig
> @@ -9,3 +9,12 @@ config TDX_GUEST_DRIVER
>  
>  	  To compile this driver as module, choose M here. The module will
>  	  be called tdx-guest.
> +
> +config TDX_GUEST_DRIVER_LEGACY_IOCTL
> +	def_bool y
> +	prompt "Enable legacy ioctl interface (DEPRECATED)"
> +	depends on TDX_GUEST_DRIVER
> +	help
> +	  Enable the legacy IOCTL-based interface to get the TDX report. It's
> +	  deprecated in favor of the configfs based one and will be removed
> +	  in a future release.

This too should document how a user would know if they need this or not.

> diff --git a/drivers/virt/coco/tdx-guest/tdx-guest.c b/drivers/virt/coco/tdx-guest/tdx-guest.c
> index 1253bf76b570..b8cea9486daf 100644
> --- a/drivers/virt/coco/tdx-guest/tdx-guest.c
> +++ b/drivers/virt/coco/tdx-guest/tdx-guest.c
> @@ -66,6 +66,7 @@ static DEFINE_MUTEX(quote_lock);
>   */
>  static u32 getquote_timeout = 30;
>  
> +#ifdef CONFIG_TDX_GUEST_DRIVER_LEGACY_IOCTL
>  static long tdx_get_report0(struct tdx_report_req __user *req)
>  {
>  	u8 *reportdata, *tdreport;
> @@ -100,6 +101,7 @@ static long tdx_get_report0(struct tdx_report_req __user *req)
>  
>  	return ret;
>  }
> +#endif

Please no sprinkling of ifdef throughout the file.  Create a new file
like drivers/virt/coco/tdx-guest/miscdev.c, and optionally compile it.
With a tdx_misc_register() that stubs out the setup, something like:

#ifdef CONFIG_TDX_GUEST_REPORT_IOCTL
int tdx_misc_register(void);
#else
static inline int tdx_misc_register(void) { return 0; }
#endif

>  
>  static void free_quote_buf(void *buf)
>  {
> @@ -249,6 +251,9 @@ static int tdx_report_new(struct tsm_report *report, void *data)
>  	return ret;
>  }
>  
> +
> +
> +#ifdef CONFIG_TDX_GUEST_DRIVER_LEGACY_IOCTL
>  static long tdx_guest_ioctl(struct file *file, unsigned int cmd,
>  			    unsigned long arg)
>  {
> @@ -271,6 +276,7 @@ static struct miscdevice tdx_misc_dev = {
>  	.minor = MISC_DYNAMIC_MINOR,
>  	.fops = &tdx_guest_fops,
>  };
> +#endif
>  
>  static const struct x86_cpu_id tdx_guest_ids[] = {
>  	X86_MATCH_FEATURE(X86_FEATURE_TDX_GUEST, NULL),
> @@ -290,9 +296,12 @@ static int __init tdx_guest_init(void)
>  	if (!x86_match_cpu(tdx_guest_ids))
>  		return -ENODEV;
>  
> +#ifdef CONFIG_TDX_GUEST_DRIVER_LEGACY_IOCTL
> +	pr_info("Legacy IOCTL-based interface is deprecated and will be removed in a future release\n");

No, don't spam the log on driver load, maybe add a pr_info_once() in the
ioctl path itself. Then you get some rough telemmetry of actual usage if
people post kernel logs to the kernel mailing list(s).

>  	ret = misc_register(&tdx_misc_dev);
>  	if (ret)
>  		return ret;
> +#endif
>  
>  	quote_data = alloc_quote_buf();
>  	if (!quote_data) {
> @@ -310,7 +319,9 @@ static int __init tdx_guest_init(void)
>  free_quote:
>  	free_quote_buf(quote_data);
>  free_misc:
> +#ifdef CONFIG_TDX_GUEST_DRIVER_LEGACY_IOCTL
>  	misc_deregister(&tdx_misc_dev);
> +#endif
>  
>  	return ret;
>  }
> @@ -320,7 +331,9 @@ static void __exit tdx_guest_exit(void)
>  {
>  	tsm_unregister(&tdx_tsm_ops);
>  	free_quote_buf(quote_data);
> +#ifdef CONFIG_TDX_GUEST_DRIVER_LEGACY_IOCTL
>  	misc_deregister(&tdx_misc_dev);
> +#endif
>  }
>  module_exit(tdx_guest_exit);
>  
> -- 
> 2.34.1
>