On Thu, Jul 8, 2021 at 4:57 PM Kuppuswamy, Sathyanarayanan
[off-list ref] wrote:

On 7/8/21 4:36 PM, Dan Williams wrote:

quoted

quoted

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

quoted

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.

Those appear to be for core functionality, not one off drivers. Where
is the code that does the SetupEventNotifyInterrupt, is it a driver?

quoted

quoted

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

If you have a lock would TDX KVM even notice that its parallel
requests are being handled serially? I.e. even if they said "yes,
multiple requests may happen in parallel", until it becomes an actual
latency problem in practice it's not clear that this generous use of
resources is justified.

Scratch that... this driver already has the attestation_lock! So, it's
already the case that only one thread can be attesting at a time. The
per-file buffer is unecessary.

quoted

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.

keyutils supports generating and passing blobs into and out of the
kernel with a handle associated to those blobs. This driver adds a TDX
way to pass blobs into and out of the kernel. If Linux grows other
TDX-like attestation requirements in the future (e.g. PCI SPDM) should
each of those invent their own user ABI for passing blobs around?