Messages in this thread | | | Subject | Re: [PATCH v2 5/6] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver | From | "Kuppuswamy, Sathyanarayanan" <> | Date | Thu, 8 Jul 2021 16:57:24 -0700 |
| |
On 7/8/21 4:36 PM, Dan Williams wrote: >> +static int tdg_attest_open(struct inode *inode, struct file *file) >> +{ >> + /* >> + * Currently tdg_event_notify_handler is only used in attestation >> + * driver. But, WRITE_ONCE is used as benign data race notice. >> + */ >> + WRITE_ONCE(tdg_event_notify_handler, attestation_callback_handler); > Why is this ioctl not part of the driver that registered the interrupt
We cannot club them because they are not functionally related. Even notification is a separate common feature supported by TDX and configured using SetupEventNotifyInterrupt hypercall. It is not related to TDX attestation. Attestation just uses event notification interface to get the quote completion event.
> handler for this callback in the first instance? I've never seen this > style of cross-driver communication before.
This is similar to x86_platform_ipi_callback() acrn_setup_intr_handler() use cases.
> >> + >> + file->private_data = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO, >> + get_order(QUOTE_SIZE)); > Why does this driver abandon all semblance of type-safety and use > ->private_data directly? This also seems an easy way to consume > memory, just keep opening this device over and over again. > > AFAICS this buffer is only used ephemerally. I see no reason it needs > to be allocated once per open file. Unless you need several threads to > be running the attestation process in parallel just allocate a single > buffer at module init (statically defined or on the heap) and use a > lock to enforce only one user of this buffer at a time. That would > also solve your direct-map fracturing problem.
Theoretically attestation requests can be sent in parallel. I have allocated the memory in open() call mainly for this reason. But current TDX ABI specification does not clearly specify this possibility and I am not sure whether TDX KVM supports it. Let me confirm about it again with TDX KVM owner. If such model is not currently supported, then I will move the memory allocation to init code.
> > All that said, this new user ABI for passing blobs in and out of the > kernel is something that the keyutils API already does. Did you > consider add_key() / request_key() for this case? That would also be > the natural path for the end step of requesting the drive decrypt key. > I.e. a chain of key payloads starting with establishing the > attestation blob.
I am not sure whether we can use keyutil interface for attestation. AFAIK, there are other use cases for attestation other than getting keys for encrypted drives.
-- Sathyanarayanan Kuppuswamy Linux Kernel Developer
| |