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

[Sathyanarayanan Kuppuswamy]

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

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