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

[Greg Kroah-Hartman]

On Tue, Nov 01, 2022 at 11:18:29PM -0700, Sathyanarayanan Kuppuswamy wrote:
> Hi Greg,
> 
> On 10/29/22 11:53 PM, Greg Kroah-Hartman wrote:
> > 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@xxxxxxxxxxxxxxx>");
> >>>> +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.
> 
> 
> The following are potential solutions to the possible kernel/userspace
> mix/match issue that may arise in the future if the acceptable reportdata
> length, tdreport length, or subtype values change.
> 
> I've attempted to do a sample implementation as you have suggested to
> check the pros and cons for both solutions. Please let me know what you
> think. Personally I prefer solution 2, as it handles the issue you have
> raised and also keeps the ABI flexible.
> 
> Solution 1:
> ------------
> 
> This is based on your suggestion. I have dropped the IOCTL req members for
> reportdata length (rpd_len), tdreport length (tdr_len) and subtype. I have
> also used fixed size buffers to handle the current requirements.
> 
> Pros: Implementation is simple and clean.
> 
> Cons: May need to add new IOCTL for any future requirement updates.
> 
> Following are the ABI and IOCTL handler implementation details (Note: it
> is not the complete code, only included required details to show how the
> implementation looks):

Naturally, I like this one :)

And you can even make it go faster, with only one allocation, no need
for 2 as your implementation did.

I don't know if speed matters on this, as I don't know how fast the
actual hardware call takes, but making only 1 allocation and removing
all need/worries about length checking and getting that correct is
always a good thing.

Simple is good, especially if it works today.

If you have a new message size/type in the future, great, write a new
ioctl and all is good!

Test your implementations out and see what you feel good about, but
seriously consider keeping this simple if at all possible.

thanks,

greg k-h