Messages in this thread | | | Date | Wed, 27 Apr 2022 17:40:25 -0700 | Subject | Re: [PATCH v4 1/3] x86/tdx: Add TDX Guest attestation interface driver | From | 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@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
| |