Re: [PATCH v4 1/3] x86/tdx: Add TDX Guest attestation interface driver
From: Kai Huang
Date: Wed Apr 27 2022 - 19:40:47 EST
On Wed, 2022-04-27 at 14:45 -0700, Sathyanarayanan Kuppuswamy wrote:
> 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@xxxxxxxxxx/
> > 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@xxxxxxxxxxxxxxx/
> > 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@xxxxxxxxxx/
> > 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
I am just pointing out Andy made such suggestion before, and it's not something
we cannot support.
Anyway will let you decide.