Re: [PATCH v1 2/3] virt: tdx-guest: Add Quote generation support

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

Hi Greg,

Thanks for the review comments.

On 3/25/23 11:34 PM, Greg KH wrote:
> On Sat, Mar 25, 2023 at 11:20:38PM -0700, Kuppuswamy Sathyanarayanan wrote:
>> Since GetQuote support requires usage of DMA APIs, convert TDX guest
>> driver to a platform driver.
> 
> Sorry, but that's not a valid reason to use a platform device for fake
> things like this:
> 
>> +static struct platform_device *tdx_dev;
> 
> Especially a single static one.
> 
>> +static int tdx_guest_probe(struct platform_device *pdev)
>> +{
>> +	if (tdx_register_event_irq_cb(attestation_callback_handler, pdev))
>> +		return -EIO;
>> +
>> +	return misc_register(&tdx_misc_dev);
>> +}
>> +
>> +static int tdx_guest_remove(struct platform_device *pdev)
>> +{
>> +	tdx_unregister_event_irq_cb(attestation_callback_handler, pdev);
>> +	misc_deregister(&tdx_misc_dev);
>> +	return 0;
>> +}
>> +
>> +static struct platform_driver tdx_guest_driver = {
>> +	.probe = tdx_guest_probe,
>> +	.remove = tdx_guest_remove,
>> +	.driver.name = KBUILD_MODNAME,
>> +};
>> +
>>  static const struct x86_cpu_id tdx_guest_ids[] = {
>>  	X86_MATCH_FEATURE(X86_FEATURE_TDX_GUEST, NULL),
>>  	{}
>> @@ -84,16 +310,35 @@ MODULE_DEVICE_TABLE(x86cpu, tdx_guest_ids);
>>  
>>  static int __init tdx_guest_init(void)
>>  {
>> +	int ret;
>> +
>>  	if (!x86_match_cpu(tdx_guest_ids))
>>  		return -ENODEV;
>>  
>> -	return misc_register(&tdx_misc_dev);
>> +	ret = platform_driver_register(&tdx_guest_driver);
>> +	if (ret) {
>> +		pr_err("failed to register driver, err=%d\n", ret);
>> +		return ret;
>> +	}
> 
> No, please do not create a fake platform driver.
> 
>> +	tdx_dev = platform_device_register_simple(KBUILD_MODNAME,
>> +						  PLATFORM_DEVID_NONE,
>> +						  NULL, 0);
> 
> And please do not create a fake platform device.
> 
> As always, do not create fake platform devices for things that are NOT
> platform devices.
> 
> If this device needs DMA (but why?) then make it a real device and tie

Since the Quote generation process requires the guest and VMM to share data,
we need to use shared buffers. But in TDX guest, VMM cannot directly access
the guest private memory. Any memory that is required for communication with
the VMM must be shared explicitly. One way is to allocate the memory on
demand and share it explicitly using set_memory_decrypted() call. Although
this approach is clean, since it breaks the direct mapping, it is not
efficient. An alternative way is to reserve a pool of shared buffers during
boot time and re-use them for guest/VMM communication. Since in TDX, DMA API
is configured to tap into the SWIOTLB memory framework (which is shared by
default), we try to use dma_alloc_* APIs to allocate the shared buffers from
the shared pool without breaking the direct map.

If usage of DMA APIs / platform device is not acceptable for this use case,
an alternative approach is to allocate a fixed number of shared buffers during
the TDX guest driver probe and use it for GetQuote requests. Although it would
limit the amount of memory we can use for GetQuote requests at a time and also
reserve a chunk of memory during the init() time, I think it is an acceptable
tradeoff when compared to alternative choices. The AMD SEV guest driver also
adopts the similar approach. Please let me know if this approach is acceptable.

> it to the bus it belongs to (that it is obviously doing DMA on.)


> 
> thanks,
> 
> greg k-h

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer