Messages in this thread | ![/](/images/icornerl.gif) | | Date | Wed, 2 Nov 2022 17:36:18 -0700 | Subject | Re: [PATCH v16 2/3] virt: Add TDX guest driver | From | Sathyanarayanan Kuppuswamy <> |
| |
Hi,
On 11/1/22 11:58 PM, Greg Kroah-Hartman wrote: > 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@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. >> >> >> 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.
Buffer allocation time is very negligible compared to the TDCALL execution time. So we will not gain much by such optimization, and it is not a time critical path either. Using separate buffers for input and output, in my opinion, keeps it cleaner and easier to read. Hope it is fine with you.
> > Simple is good, especially if it works today. >
I am fine with it. If there are no objections, I will go ahead with this approach.
> 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
-- Sathyanarayanan Kuppuswamy Linux Kernel Developer
| ![\](/images/icornerr.gif) |