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

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

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.


> 
> 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.

> 
> 

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer