Re: [PATCH v4 1/3] x86/tdx: Add TDX Guest attestation interface driver

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

Hi Kai,

On 4/27/22 4:40 PM, Kai Huang wrote:
> On Wed, 2022-04-27 at 14:45 -0700, Sathyanarayanan Kuppuswamy wrote:
>> Hi,
>>
>> On 4/26/22 10:15 PM, Kai Huang wrote:
>>> On Tue, 2022-04-26 at 12:07 -0700, Sathyanarayanan Kuppuswamy wrote:
>>>>> Is there any particular reason to use platform driver and support it as a
>>>>> module?
>>>>>
>>>>> SGX driver uses misc_register() to register /dev/sgx_enclave during boot.
>>>>> Looks
>>>>> it would be simpler.
>>>>
>>>> Main reason is to use a proper device in dma_alloc* APIs.
>>>>
>>>> My initial version only used misc device as you have mentioned. But
>>>> Hans raised a concern about using proper struct device in dma_alloc*
>>>> APIs and suggested modifying the driver to use platform device
>>>> model. You can find relevant discussion here.
>>>>
>>>> https://lore.kernel.org/all/47d06f45-c1b5-2c8f-d937-3abacbf10321@redhat.com/
>>>
>>> Thanks for the info.  Sorry I didn't dig review comments for previous version.
>>>
>>> However, after digging more, I am wondering why do you need to use DMA API at
>>> the first place.
>>>
>>> Firstly, for this basic driver to report TDREPORT to userspace, there's no need
>>> to use any DMA API, nor we need to use shared memory, as we just get the report
>>> into some buffer (doesn't need to be shared) and copy the report back to
>>> userspace.  So it doesn't make a lot sense to use platform device here.
>>
>> Yes. For this patch itself, since  we don't need to use DMA API,
>> platform driver model is not required. But I have made this patch use
>> platform driver format in consideration of its need in the next patch.
>> Making it misc driver in this patch and changing it to platform driver
>> in next patch does not make sense. Since they are all in the same patch
>> set we can add some changes in consideration of the next patch.
>>
>>>
>>> Secondly, in terms of GetQuote support, it seems Dave/Andi suggested you can use
>>> vmalloc()/vmap() and just use set_memory_decrypted() to convert it to shared:
>>>
>>> https://lore.kernel.org/all/ce0feeec-a949-35f8-3010-b0d69acbbc2e@linux.intel.com/
>>>
>>> I don't see why it cannot work?  Then wouldn't this approach be simpler than
>>> using DMA API?  Any reason to choose platform device?
>>
>> Yes, it will work. But I am not sure whether it is simpler than adding
>> platform driver specific buffer code. I have used DMA APIs because it
>> will handle allocation and decryption setting internally. I thought is
>> simpler than we handling it ourselves.
>>
>> But if platform device driver model is not preferred, I can change it.
> 
> I don't think ignoring Dave/Andi's comments w/o providing feedback is good.

It is not intentional. I think it is missed during my vacation due to
some mail access issues. Sorry about it.

> 
> Also I personally don't see how using DMA API is better than using
> vmalloc()/vmap().  In order to use DMA API, you have to add more code to use
> platform_device, which isn't necessary.
> 
> I'll leave this to Dave/Andi.

As I said, I am fine with the change (if it is preferred).

> 
>>
>>
>>>
>>> Btw, a side topic:
>>>
>>> Andy suggested we don't do memory allocation and private-shared conversion at
>>> IOCTL time as the conversion is expensive:
>>>
>>> https://lore.kernel.org/all/06c85c19-e16c-3121-ed47-075cfa779b67@kernel.org/
>>>
>>> This is reasonable (and sorry I didn't see this when I commented in v3).
>>>
>>> To avoid IOCTL time private-shared conversion, and yet support dynamic Quote
>>> length, can we do following:
>>>
>>> - Allocate a *default* size buffer at driver loading time (i.e. 4 pages), and
>>> convert to shared.  This default size should cover 99% cases as Intel QGS
>>> currently generates Quote smaller than 8K, and Intel attestation agent hard-code
>>> a 4 pages buffer for Quote.
>>>
>>> - In GetQuote IOCTL, when the len is larger than default size, we discard the
>>> original one and allocate a larger buffer.
>>>
>>> How does this sound?
>>
>> It sounds fine. Your suggestion can indeed decrease the IOCTL time.
>>
>> But, IMO, since attestation will not be used that frequently,
>> we don't need to consider optimization at this point of time. Also, I
>> think the memory allocation time is negligible compared to time it takes
>> for the TDQUOTE generation.
>>
>> Even if we have to do it, we can add it in future as a separate
>> patch. We don't need to add it part of this basic driver support
>> patch.
>>
>>
> 
> I am just pointing out Andy made such suggestion before, and it's not something
> we cannot support.
> 
> Anyway will let you decide.

Ok.

> 
> 

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer