by Greg Kroah-Hartman

[permalink] [raw]

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

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 <[email protected]>");
> >>>> +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

2022-11-03 01:33:54

by Kuppuswamy Sathyanarayanan

[permalink] [raw]

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

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 <[email protected]>");
>>>>>> +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