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

[Sathyanarayanan Kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com>]

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