Re: [PATCH v16 2/3] virt: Add TDX guest driver

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Sat, Oct 29, 2022 at 04:17:39PM -0700, Sathyanarayanan Kuppuswamy wrote:
> Hi Greg
> 
> On 10/27/22 11:25 PM, Greg Kroah-Hartman wrote:
> > On Thu, Oct 27, 2022 at 05:28:19PM -0700, Kuppuswamy Sathyanarayanan wrote:
> 
> >> +
> >> +static long tdx_guest_ioctl(struct file *file, unsigned int cmd,
> >> +			    unsigned long arg)
> >> +{
> >> +	switch (cmd) {
> >> +	case TDX_CMD_GET_REPORT:
> >> +		return tdx_get_report((void __user *)arg);
> > 
> > You know the type of this pointer here, why not cast it instead of
> > having to cast it from void * again?
> 
> The only place we use arg pointer is in copy_from_user() function,
> which expects void __user * pointer. So why cast it as struct
> tdx_report_req * here?

Because then your function will show the true type and you don't have to
cast it again.

> >> +MODULE_AUTHOR("Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>");
> >> +MODULE_DESCRIPTION("TDX Guest Driver");
> >> +MODULE_LICENSE("GPL");
> >> diff --git a/include/uapi/linux/tdx-guest.h b/include/uapi/linux/tdx-guest.h
> >> new file mode 100644
> >> index 000000000000..29453e6a7ced
> >> --- /dev/null
> >> +++ b/include/uapi/linux/tdx-guest.h
> >> @@ -0,0 +1,55 @@
> >> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> >> +/*
> >> + * Userspace interface for TDX guest driver
> >> + *
> >> + * Copyright (C) 2022 Intel Corporation
> >> + */
> >> +
> >> +#ifndef _UAPI_LINUX_TDX_GUEST_H_
> >> +#define _UAPI_LINUX_TDX_GUEST_H_
> >> +
> >> +#include <linux/ioctl.h>
> >> +#include <linux/types.h>
> >> +
> >> +/* Length of the REPORTDATA used in TDG.MR.REPORT TDCALL */
> >> +#define TDX_REPORTDATA_LEN              64
> >> +
> >> +/* Length of TDREPORT used in TDG.MR.REPORT TDCALL */
> >> +#define TDX_REPORT_LEN                  1024
> > 
> > As these are fixed values, why do you have to say them again in the
> > structure?
> 
> These length recommendations are provided by the TDX Module, and there is
> a slight possibility that the TDX Module will increase the maximum size
> of the REPORTDATA and TDREPORT in the future.

We do not write kernel code for "slight possibilities sometime in the
future".

> To handle such length
> changes, rather than inventing a new IOCTL for it in the future, making
> the current one flexible to handle such changes seems better.

Please work through the code and see how that would really look, and
what would break if you were to change that in the future (remember
kernel code and userspace code is not upgraded at the same time.)

> One less ABI
> to maintain is always better, right? My initial design did use fixed size
> buffers like you have recommended, but later changed it as per review
> suggestion to make the ABI flexible.

Again, work through and try to determine if the added complexity will
really work here.

What is wrong with just adding a new ioctl if in the future, you really
do need to change something?  That way you are sure that nothing will
break and userspace will be finen with it.  It is not like you are
forbidden to add new ioctls later, you would have to change the kernel
code no matter what anyway.

Keep it simple please.

greg k-h