On 27/8/24 22:32, Jason Gunthorpe wrote:
On Fri, Aug 23, 2024 at 11:21:21PM +1000, Alexey Kardashevskiy wrote:
The module responsibilities are:
1. detect TEE support in a device and create nodes in the device's sysfs
entry;
2. allow binding a PCI device to a VM for passing it through in a trusted
manner;
Binding devices to VMs and managing their lifecycle is the purvue of
VFIO and iommufd, it should not be exposed via weird sysfs calls like
this. You can't build the right security model without being inside
the VFIO context.
Is "extend the MAP_DMA uAPI to accept {gmemfd, offset}" enough for the
VFIO context, or there is more and I am missing it?
As I said in the other email, it seems like the PSP and the iommu
driver need to coordinate to ensure the two DTEs are consistent, and
solve the other sequencing problems you seem to have.
Correct. That DTE/sDTE hack is rather for showing the bare minimum
needed to get IDE+TDISP going, we will definitely address this better.
I'm not convinced this should be in some side module - it seems like
this is possibly more logically integrated as part of the iommu..
There are two things which the module's sysfs interface tries dealing with:
1) device authentication (by the PSP, contrary to Lukas'es host-based
CMA) and PCIe link encryption (PCIe IDE keys only programmable via the PSP);
2) VFIO + Coco VM.
The first part does not touch VFIO or IOMMU, and the sysfs interface
provides API mostly for 1).
The proposed sysfs interface does not do VFIO or IOMMUFD binding though
as it is weird indeed, even for test/bringup purposes, the only somewhat
useful sysfs bit here is the interface report (a PCI/TDISP thing, comes
from a device).
Besides sysfs, the module provides common "verbs" to be defined by the
platform (which is right now a reduced set of the AMD PSP operations but
the hope is it can be generalized); and the module also does PCIe DOE
bouncing (which is also not uncommon). Part of this exercise is trying
to find some common ground (if it is possible), hence routing everything
via this module.
+static ssize_t tsm_dev_connect_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct tsm_dev *tdev = tsm_dev_get(dev);
+ unsigned long val;
+ ssize_t ret = -EIO;
+
+ if (kstrtoul(buf, 0, &val) < 0)
+ ret = -EINVAL;
+ else if (val && !tdev->connected)
+ ret = tsm_dev_connect(tdev, tsm.private_data, val);
+ else if (!val && tdev->connected)
+ ret = tsm_dev_reclaim(tdev, tsm.private_data);
+
+ if (!ret)
+ ret = count;
+
+ tsm_dev_put(tdev);
+
+ return ret;
+}
+
+static ssize_t tsm_dev_connect_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+ struct tsm_dev *tdev = tsm_dev_get(dev);
+ ssize_t ret = sysfs_emit(buf, "%u\n", tdev->connected);
+
+ tsm_dev_put(tdev);
+ return ret;
+}
+
+static DEVICE_ATTR_RW(tsm_dev_connect);
Please do a much better job explaining the uAPIS you are trying to
build in all the commit messages and how you expect them to be used.
True and sorry about that, will do better...
Picking this stuff out of a 6k loc series is a bit tricky
Thanks for the comments!
Jason
--
Alexey
