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

[Kai Huang <kai.huang@intel.com>]

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.

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.

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


-- 
Thanks,
-Kai