On 4/9/24 09:51, Dan Williams wrote:
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;
3. store measurements/certificates/reports and provide access to those for
the userspace via sysfs.
This relies on the platform to register a set of callbacks,
for both host and guest.
And tdi_enabled in the device struct.
I had been holding out hope that when I got this patch the changelog
would give some justification for what folks had been whispering to me
in recent days: "hey Dan, looks like Alexey is completely ignoring the
PCI/TSM approach?".
Bjorn acked that approach here:
http://lore.kernel.org/20240419220729.GA307280@bhelgaas
It is in need of a refresh, preview here:
https://git.kernel.org/pub/scm/linux/kernel/git/djbw/linux.git/commit/?id=5807465b92ac
At best, I am disappointed that this RFC ignored it. More comments
below, but please do clarify if we are working together on a Bjorn-acked
direction, or not.
Together.
My problem with that patchset is that it only does connect/disconnect
and no TDISP business (and I need both for my exercise) and I was hoping
to see some TDISP-aware git tree but this has not happened yet so I
postponed rebasing onto it, due to the lack of time and also apparent
difference between yours and mine TSMs (and I had mine working before I
saw yours and focused on making things work for the starter). Sorry, I
should have spoken louder. Or listen better to that whispering. Or
rebase earlier.
Signed-off-by: Alexey Kardashevskiy <aik@xxxxxxx>
---
drivers/virt/coco/Makefile | 1 +
include/linux/device.h | 5 +
include/linux/tsm.h | 263 ++++
drivers/virt/coco/tsm.c | 1336 ++++++++++++++++++++
Documentation/virt/coco/tsm.rst | 62 +
drivers/virt/coco/Kconfig | 11 +
6 files changed, 1678 insertions(+)
diff --git a/drivers/virt/coco/Makefile b/drivers/virt/coco/Makefile
index 75defec514f8..5d1aefb62714 100644
--- a/drivers/virt/coco/Makefile
+++ b/drivers/virt/coco/Makefile
@@ -3,6 +3,7 @@
# Confidential computing related collateral
#
obj-$(CONFIG_TSM_REPORTS) += tsm-report.o
+obj-$(CONFIG_TSM) += tsm.o
The expectation is that something like drivers/virt/coco/tsm.c would be
the class driver for cross-vendor generic TSM uAPI. The PCI specific
bits go in drivers/pci/tsm.c.
obj-$(CONFIG_EFI_SECRET) += efi_secret/
obj-$(CONFIG_SEV_GUEST) += sev-guest/
obj-$(CONFIG_INTEL_TDX_GUEST) += tdx-guest/
diff --git a/include/linux/device.h b/include/linux/device.h
index 34eb20f5966f..bb58ed1fb8da 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -45,6 +45,7 @@ struct fwnode_handle;
struct iommu_group;
struct dev_pin_info;
struct dev_iommu;
+struct tsm_tdi;
struct msi_device_data;
/**
@@ -801,6 +802,7 @@ struct device {
void (*release)(struct device *dev);
struct iommu_group *iommu_group;
struct dev_iommu *iommu;
+ struct tsm_tdi *tdi;
No. The only known device model for TDIs is PCI devices, i.e. TDISP is a
PCI protocol. Even SPDM which is cross device-type generic did not touch
'struct device'.
TDISP is PCI but DMA is not. This is for:
[RFC PATCH 19/21] sev-guest: Stop changing encrypted page state for
TDISP devices
DMA layer deals with struct device and tries hard to avoid indirect _ops
calls so I was looking for a place for "tdi_enabled" (a bad name,
perhaps, may be call it "dma_encrypted", a few lines below). So I keep
the flag and the pointer together for the RFC. I am hoping for a better
solution for 19/21, then I am absolutely moving tdi* to pci_dev (well,
drop these and just use yours).
struct device_physical_location *physical_location;
@@ -822,6 +824,9 @@ struct device {
#ifdef CONFIG_DMA_NEED_SYNC
bool dma_skip_sync:1;
#endif
+#if defined(CONFIG_TSM) || defined(CONFIG_TSM_MODULE)
+ bool tdi_enabled:1;
+#endif
};
/**
diff --git a/include/linux/tsm.h b/include/linux/tsm.h
new file mode 100644
index 000000000000..d48eceaf5bc0
--- /dev/null
+++ b/include/linux/tsm.h
@@ -0,0 +1,263 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef LINUX_TSM_H
+#define LINUX_TSM_H
+
+#include <linux/cdev.h>
+
+/* SPDM control structure for DOE */
+struct tsm_spdm {
+ unsigned long req_len;
+ void *req;
+ unsigned long rsp_len;
+ void *rsp;
+
+ struct pci_doe_mb *doe_mb;
+ struct pci_doe_mb *doe_mb_secured;
+};
+
+/* Data object for measurements/certificates/attestationreport */
+struct tsm_blob {
+ void *data;
+ size_t len;
+ struct kref kref;
+ void (*release)(struct tsm_blob *b);
Hard to judge the suitability of these without documentation. Skeptical
that these TDISP evidence blobs need to have a lifetime distinct from
the device's TSM context.
You are right.
+};
+
+struct tsm_blob *tsm_blob_new(void *data, size_t len, void (*release)(struct tsm_blob *b));
+struct tsm_blob *tsm_blob_get(struct tsm_blob *b);
+void tsm_blob_put(struct tsm_blob *b);
+
+/**
+ * struct tdisp_interface_id - TDISP INTERFACE_ID Definition
+ *
+ * @function_id: Identifies the function of the device hosting the TDI
+ * 15:0: @rid: Requester ID
+ * 23:16: @rseg: Requester Segment (Reserved if Requester Segment Valid is Clear)
+ * 24: @rseg_valid: Requester Segment Valid
+ * 31:25 – Reserved
+ * 8B - Reserved
+ */
+struct tdisp_interface_id {
+ union {
+ struct {
+ u32 function_id;
+ u8 reserved[8];
+ };
+ struct {
+ u16 rid;
+ u8 rseg;
+ u8 rseg_valid:1;
Linux typically avoids C-bitfields in hardware interfaces in favor of
bitfield.h macros.
>> + };
+ };
+} __packed;
Does this need to be "packed"? Looks naturally aligned to pahole.
"__packed" is also a way to say it is a binary interface, I want to be
precise about this.
+
+/*
+ * Measurement block as defined in SPDM DSP0274.
+ */
+struct spdm_measurement_block_header {
+ u8 index;
+ u8 spec; /* MeasurementSpecification */
+ u16 size;
+} __packed;
+
+struct dmtf_measurement_block_header {
+ u8 type; /* DMTFSpecMeasurementValueType */
+ u16 size; /* DMTFSpecMeasurementValueSize */
+} __packed;
This one might need to be packed.
+
+struct dmtf_measurement_block_device_mode {
+ u32 opmode_cap; /* OperationalModeCapabilties */
+ u32 opmode_sta; /* OperationalModeState */
+ u32 devmode_cap; /* DeviceModeCapabilties */
+ u32 devmode_sta; /* DeviceModeState */
+} __packed;
+
+struct spdm_certchain_block_header {
+ u16 length;
+ u16 reserved;
+} __packed;
These last 2 struct do not seem to need it.
+
+/*
+ * TDI Report Structure as defined in TDISP.
+ */
+struct tdi_report_header {
+ union {
+ u16 interface_info;
+ struct {
+ u16 no_fw_update:1; /* fw updates not permitted in CONFIG_LOCKED or RUN */
+ u16 dma_no_pasid:1; /* TDI generates DMA requests without PASID */
+ u16 dma_pasid:1; /* TDI generates DMA requests with PASID */
+ u16 ats:1; /* ATS supported and enabled for the TDI */
+ u16 prs:1; /* PRS supported and enabled for the TDI */
+ u16 reserved1:11;
+ };
Same C-bitfield comment, as before, and what about big endian hosts?
Right, I'll get rid of c-bitfields in the common parts.
Although I am curious what big-endian platform is going to actually
support this.
+ };
+ u16 reserved2;
+ u16 msi_x_message_control;
+ u16 lnr_control;
+ u32 tph_control;
+ u32 mmio_range_count;
+} __packed;
+
+/*
+ * Each MMIO Range of the TDI is reported with the MMIO reporting offset added.
+ * Base and size in units of 4K pages
+ */
+struct tdi_report_mmio_range {
+ u64 first_page; /* First 4K page with offset added */
+ u32 num; /* Number of 4K pages in this range */
+ union {
+ u32 range_attributes;
+ struct {
+ u32 msix_table:1;
+ u32 msix_pba:1;
+ u32 is_non_tee_mem:1;
+ u32 is_mem_attr_updatable:1;
+ u32 reserved:12;
+ u32 range_id:16;
+ };
+ };
+} __packed;
+
+struct tdi_report_footer {
+ u32 device_specific_info_len;
+ u8 device_specific_info[];
+} __packed;
+
+#define TDI_REPORT_HDR(rep) ((struct tdi_report_header *) ((rep)->data))
+#define TDI_REPORT_MR_NUM(rep) (TDI_REPORT_HDR(rep)->mmio_range_count)
+#define TDI_REPORT_MR_OFF(rep) ((struct tdi_report_mmio_range *) (TDI_REPORT_HDR(rep) + 1))
+#define TDI_REPORT_MR(rep, rangeid) TDI_REPORT_MR_OFF(rep)[rangeid]
+#define TDI_REPORT_FTR(rep) ((struct tdi_report_footer *) &TDI_REPORT_MR((rep), \
+ TDI_REPORT_MR_NUM(rep)))
+
+/* Physical device descriptor responsible for IDE/TDISP setup */
+struct tsm_dev {
+ struct kref kref;
Another kref that begs the question why would a tsm_dev need its own
lifetime? This also goes back to the organization in the PCI/TSM
proposal that all TSM objects are at max bound to the lifetime of
whatever is shorter, the registration of the low-level TSM driver or the
PCI device itself.
That proposal deals with PFs for now and skips TDIs. Since TDI needs its
place in pci_dev too, and I wanted to add the bare minimum to struct
device or pci_dev, I only add TDIs and each of them references a DEV.
Enough to get me going.
+ const struct attribute_group *ag;
PCI device attribute groups are already conveyed in a well known
(lifetime and user visibility) manner. What is motivating this
"re-imagining"?
+ struct pci_dev *pdev; /* Physical PCI function #0 */
+ struct tsm_spdm spdm;
+ struct mutex spdm_mutex;
Is an spdm lock sufficient? I expect the device needs to serialize all
TSM communications, not just spdm? Documentation of the locking would
help.
What other communication do you mean here?
+
+ u8 tc_mask;
+ u8 cert_slot;
+ u8 connected;
+ struct {
+ u8 enabled:1;
+ u8 enable:1;
+ u8 def:1;
+ u8 dev_ide_cfg:1;
+ u8 dev_tee_limited:1;
+ u8 rootport_ide_cfg:1;
+ u8 rootport_tee_limited:1;
+ u8 id;
+ } selective_ide[256];
+ bool ide_pre;
+
+ struct tsm_blob *meas;
+ struct tsm_blob *certs;
Compare these to the blobs that Lukas that maintains for CMA. To my
knoweldge no new kref lifetime rules independent of the authenticated
lifetime.
+
+ void *data; /* Platform specific data */
+};
+
+/* PCI function for passing through, can be the same as tsm_dev::pdev */
+struct tsm_tdi {
+ const struct attribute_group *ag;
+ struct pci_dev *pdev;
+ struct tsm_dev *tdev;
+
+ u8 rseg;
+ u8 rseg_valid;
+ bool validated;
+
+ struct tsm_blob *report;
+
+ void *data; /* Platform specific data */
+
+ u64 vmid;
+ u32 asid;
+ u16 guest_rid; /* BDFn of PCI Fn in the VM */
+};
+
+struct tsm_dev_status {
+ u8 ctx_state;
+ u8 tc_mask;
+ u8 certs_slot;
+ u16 device_id;
+ u16 segment_id;
+ u8 no_fw_update;
+ u16 ide_stream_id[8];
+};
+
+enum tsm_spdm_algos {
+ TSM_TDI_SPDM_ALGOS_DHE_SECP256R1,
+ TSM_TDI_SPDM_ALGOS_DHE_SECP384R1,
+ TSM_TDI_SPDM_ALGOS_AEAD_AES_128_GCM,
+ TSM_TDI_SPDM_ALGOS_AEAD_AES_256_GCM,
+ TSM_TDI_SPDM_ALGOS_ASYM_TPM_ALG_RSASSA_3072,
+ TSM_TDI_SPDM_ALGOS_ASYM_TPM_ALG_ECDSA_ECC_NIST_P256,
+ TSM_TDI_SPDM_ALGOS_ASYM_TPM_ALG_ECDSA_ECC_NIST_P384,
+ TSM_TDI_SPDM_ALGOS_HASH_TPM_ALG_SHA_256,
+ TSM_TDI_SPDM_ALGOS_HASH_TPM_ALG_SHA_384,
+ TSM_TDI_SPDM_ALGOS_KEY_SCHED_SPDM_KEY_SCHEDULE,
+};
+
+enum tsm_tdisp_state {
+ TDISP_STATE_UNAVAIL,
+ TDISP_STATE_CONFIG_UNLOCKED,
+ TDISP_STATE_CONFIG_LOCKED,
+ TDISP_STATE_RUN,
+ TDISP_STATE_ERROR,
+};
+
+struct tsm_tdi_status {
+ bool valid;
+ u8 meas_digest_fresh:1;
+ u8 meas_digest_valid:1;
+ u8 all_request_redirect:1;
+ u8 bind_p2p:1;
+ u8 lock_msix:1;
+ u8 no_fw_update:1;
+ u16 cache_line_size;
+ u64 spdm_algos; /* Bitmask of tsm_spdm_algos */
+ u8 certs_digest[48];
+ u8 meas_digest[48];
+ u8 interface_report_digest[48];
+
+ /* HV only */
+ struct tdisp_interface_id id;
+ u8 guest_report_id[16];
+ enum tsm_tdisp_state state;
+};
+
+struct tsm_ops {
The lack of documentation for these ops makes review difficult.
+ /* HV hooks */
+ int (*dev_connect)(struct tsm_dev *tdev, void *private_data);
Lets not abandon type-safety this early. Is it really the case that all
of these helpers need anonymous globs passed to them?
>> + int (*dev_reclaim)(struct tsm_dev *tdev, void *private_data);
+ int (*dev_status)(struct tsm_dev *tdev, void *private_data, struct tsm_dev_status *s);
+ int (*ide_refresh)(struct tsm_dev *tdev, void *private_data);
IDE Key Refresh seems an enhancement worth breaking out of the base
enabling.
+ int (*tdi_bind)(struct tsm_tdi *tdi, u32 bdfn, u64 vmid, u32 asid, void *private_data);
+ int (*tdi_reclaim)(struct tsm_tdi *tdi, void *private_data);
+
+ int (*guest_request)(struct tsm_tdi *tdi, u32 guest_rid, u64 vmid, void *req_data,
+ enum tsm_tdisp_state *state, void *private_data);
+
+ /* VM hooks */
+ int (*tdi_validate)(struct tsm_tdi *tdi, bool invalidate, void *private_data);
+
+ /* HV and VM hooks */
+ int (*tdi_status)(struct tsm_tdi *tdi, void *private_data, struct tsm_tdi_status *ts);
Lets not mix HV and VM hooks in the same ops without good reason.
+};
+
+void tsm_set_ops(struct tsm_ops *ops, void *private_data);
+struct tsm_tdi *tsm_tdi_get(struct device *dev);
+int tsm_tdi_bind(struct tsm_tdi *tdi, u32 guest_rid, u64 vmid, u32 asid);
+void tsm_tdi_unbind(struct tsm_tdi *tdi);
+int tsm_guest_request(struct tsm_tdi *tdi, enum tsm_tdisp_state *state, void *req_data);
+struct tsm_tdi *tsm_tdi_find(u32 guest_rid, u64 vmid);
+
+int pci_dev_tdi_validate(struct pci_dev *pdev);
+ssize_t tsm_report_gen(struct tsm_blob *report, char *b, size_t len);
+
+#endif /* LINUX_TSM_H */
diff --git a/drivers/virt/coco/tsm.c b/drivers/virt/coco/tsm.c
new file mode 100644
index 000000000000..e90455a0267f
--- /dev/null
+++ b/drivers/virt/coco/tsm.c
@@ -0,0 +1,1336 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/pci-doe.h>
+#include <linux/pci-ide.h>
+#include <linux/file.h>
+#include <linux/fdtable.h>
+#include <linux/tsm.h>
+#include <linux/kvm_host.h>
+
+#define DRIVER_VERSION "0.1"
+#define DRIVER_AUTHOR "aik@xxxxxxx"
+#define DRIVER_DESC "TSM TDISP driver"
+
+static struct {
+ struct tsm_ops *ops;
+ void *private_data;
+
+ uint tc_mask;
+ uint cert_slot;
+ bool physfn;
+} tsm;
+
+module_param_named(tc_mask, tsm.tc_mask, uint, 0644);
+MODULE_PARM_DESC(tc_mask, "Mask of traffic classes enabled in the device");
+
+module_param_named(cert_slot, tsm.cert_slot, uint, 0644);
+MODULE_PARM_DESC(cert_slot, "Slot number of the certificate requested for constructing the SPDM session");
+
+module_param_named(physfn, tsm.physfn, bool, 0644);
+MODULE_PARM_DESC(physfn, "Allow TDI on SR IOV of a physical function");
No. Lets build proper uAPI for these. These TSM global parameters are
what I envisioned hanging off of the global TSM class device.
[..]
+/*
+ * Enables IDE between the RC and the device.
+ * TEE Limited, IDE Cfg space and other bits are hardcoded
+ * as this is a sketch.
It would help to know how in depth to review the pieces if there were
more pointers of "this is serious proposal", and "this is a sketch".
Largely the latter, remember to keep appreciating the "release early"
aspect of it :)
It is a sketch which has been tested on the hardware with both KVM and
SNP VM which (I thought) has some value if posted before the LPC. I
should have made it clearer though.
+ */
+static int tsm_set_sel_ide(struct tsm_dev *tdev)
I find the "sel" abbreviation too short to be useful. Perhaps lets just
call "Selective IDE" "ide" and "Link IDE" "link_ide". Since "Selective"
is the common case.
+{
+ struct pci_dev *rootport;
+ bool printed = false;
+ unsigned int i;
+ int ret = 0;
+
+ rootport = tdev->pdev->bus->self;
Does this assume no intervening IDE switches?
+ for (i = 0; i < ARRAY_SIZE(tdev->selective_ide); ++i) {
+ if (!tdev->selective_ide[i].enable)
+ continue;
+
+ if (!printed) {
+ pci_info(rootport, "Configuring IDE with %s\n",
+ pci_name(tdev->pdev));
+ printed = true;
Why so chatty? Just make if pci_dbg() and be done.
+ }
+ WARN_ON_ONCE(tdev->selective_ide[i].enabled);
Crash the kernel if IDE is already enabled??
+
+ ret = pci_ide_set_sel_rid_assoc(tdev->pdev, i, true, 0, 0, 0xFFFF);
+ if (ret)
+ pci_warn(tdev->pdev,
+ "Failed configuring SelectiveIDE#%d rid1 with %d\n",
+ i, ret);
+ ret = pci_ide_set_sel_addr_assoc(tdev->pdev, i, 0/* RID# */, true,
+ 0, 0xFFFFFFFFFFF00000ULL);
+ if (ret)
+ pci_warn(tdev->pdev,
+ "Failed configuring SelectiveIDE#%d RID#0 with %d\n",
+ i, ret);
+
+ ret = pci_ide_set_sel(tdev->pdev, i,
+ tdev->selective_ide[i].id,
+ tdev->selective_ide[i].enable,
+ tdev->selective_ide[i].def,
+ tdev->selective_ide[i].dev_tee_limited,
+ tdev->selective_ide[i].dev_ide_cfg);
This feels kludgy. IDE is a fundamental mechanism of a PCI device why
would a PCI core helper not know how to extract the settings from a
pdev?
Something like:
pci_ide_setup_stream(pdev, i)
It is unclear to me how we go about what stream(s) need(s) enabling and
what flags to set. Who decides - a driver? a daemon/user?
+ if (ret) {
+ pci_warn(tdev->pdev,
+ "Failed configuring SelectiveIDE#%d with %d\n",
+ i, ret);
+ break;
+ }
+
+ ret = pci_ide_set_sel_rid_assoc(rootport, i, true, 0, 0, 0xFFFF);
+ if (ret)
+ pci_warn(rootport,
+ "Failed configuring SelectiveIDE#%d rid1 with %d\n",
+ i, ret);
+
+ ret = pci_ide_set_sel(rootport, i,
Perhaps:
pci_ide_host_setup_stream(pdev, i)
...I expect the helper should be able to figure out the rootport and RID
association.
Where will the helper get the properties from?
+ tdev->selective_ide[i].id,
+ tdev->selective_ide[i].enable,
+ tdev->selective_ide[i].def,
+ tdev->selective_ide[i].rootport_tee_limited,
+ tdev->selective_ide[i].rootport_ide_cfg);
+ if (ret)
+ pci_warn(rootport,
+ "Failed configuring SelectiveIDE#%d with %d\n",
+ i, ret);
+
+ tdev->selective_ide[i].enabled = 1;
+ }
+
+ return ret;
+}
+
+static void tsm_unset_sel_ide(struct tsm_dev *tdev)
+{
+ struct pci_dev *rootport = tdev->pdev->bus->self;
+ bool printed = false;
+
+ for (unsigned int i = 0; i < ARRAY_SIZE(tdev->selective_ide); ++i) {
+ if (!tdev->selective_ide[i].enabled)
+ continue;
+
+ if (!printed) {
+ pci_info(rootport, "Deconfiguring IDE with %s\n", pci_name(tdev->pdev));
+ printed = true;
+ }
+
+ pci_ide_set_sel(rootport, i, 0, 0, 0, false, false);
+ pci_ide_set_sel(tdev->pdev, i, 0, 0, 0, false, false);
These calls are unreadable, how about:
pci_ide_host_destroy_stream(pdev, i)
pci_ide_destroy_stream(pdev, i)
+static int tsm_dev_connect(struct tsm_dev *tdev, void *private_data, unsigned int val)
+{
+ int ret;
+
+ if (WARN_ON(!tsm.ops->dev_connect))
+ return -EPERM;
How does a device get this far into the flow with a TSM that does not
define the "connect" verb?
+
+ tdev->ide_pre = val == 2;
+ if (tdev->ide_pre)
+ tsm_set_sel_ide(tdev);
+
+ mutex_lock(&tdev->spdm_mutex);
+ while (1) {
+ ret = tsm.ops->dev_connect(tdev, tsm.private_data);
+ if (ret <= 0)
+ break;
+
+ ret = spdm_forward(&tdev->spdm, ret);
+ if (ret < 0)
+ break;
+ }
+ mutex_unlock(&tdev->spdm_mutex);
+
+ if (!tdev->ide_pre)
+ ret = tsm_set_sel_ide(tdev);
+
+ tdev->connected = (ret == 0);
+
+ return ret;
+}
+
+static int tsm_dev_reclaim(struct tsm_dev *tdev, void *private_data)
+{
+ struct pci_dev *pdev = NULL;
+ int ret;
+
+ if (WARN_ON(!tsm.ops->dev_reclaim))
+ return -EPERM;
Similar comment about how this could happen and why crashing the kernel
is ok.
In this exercise, connect/reclaim are triggered via sysfs so this can
happen in my practice.
And it is WARN_ON, not BUG_ON, is it still called "crashing" (vs.
"panic", I never closely thought about it)?
+
+ /* Do not disconnect with active TDIs */
+ for_each_pci_dev(pdev) {
+ struct tsm_tdi *tdi = tsm_tdi_get(&pdev->dev);
+
+ if (tdi && tdi->tdev == tdev && tdi->data)
+ return -EBUSY;
I would expect that removing things out of order causes violence, not
blocking it.
For example you can remove disk drivers while filesystems are still
mounted. What is the administrator's recourse if they *do* want to
shutdown the TSM layer all at once?
"rmmod tsm"
+ }
+
+ if (!tdev->ide_pre)
+ tsm_unset_sel_ide(tdev);
+
+ mutex_lock(&tdev->spdm_mutex);
+ while (1) {
+ ret = tsm.ops->dev_reclaim(tdev, private_data);
+ if (ret <= 0)
+ break;
What is the "reclaim" verb? Is this just a destructor? Does "disconnect"
not sufficiently clean up the device context?
>
+
+ ret = spdm_forward(&tdev->spdm, ret);
+ if (ret < 0)
+ break;
+ }
+ mutex_unlock(&tdev->spdm_mutex);
+
+ if (tdev->ide_pre)
+ tsm_unset_sel_ide(tdev);
+
+ if (!ret)
+ tdev->connected = false;
+
+ return ret;
+}
+
+static int tsm_dev_status(struct tsm_dev *tdev, void *private_data, struct tsm_dev_status *s)
+{
+ if (WARN_ON(!tsm.ops->dev_status))
+ return -EPERM;
+
+ return tsm.ops->dev_status(tdev, private_data, s);
This is asking for better defined semantics.
+}
+
+static int tsm_ide_refresh(struct tsm_dev *tdev, void *private_data)
+{
+ int ret;
+
+ if (!tsm.ops->ide_refresh)
+ return -EPERM;
+
+ mutex_lock(&tdev->spdm_mutex);
+ while (1) {
+ ret = tsm.ops->ide_refresh(tdev, private_data);
+ if (ret <= 0)
+ break;
Why is refresh not "connect"? I.e. connecting an already connected
device refreshes the connection.
Really not sure about that. Either way I am ditching it for now.
+
+ ret = spdm_forward(&tdev->spdm, ret);
+ if (ret < 0)
+ break;
+ }
+ mutex_unlock(&tdev->spdm_mutex);
+
+ return ret;
+}
+
+static void tsm_tdi_reclaim(struct tsm_tdi *tdi, void *private_data)
+{
+ int ret;
+
+ if (WARN_ON(!tsm.ops->tdi_reclaim))
+ return;
+
+ mutex_lock(&tdi->tdev->spdm_mutex);
+ while (1) {
+ ret = tsm.ops->tdi_reclaim(tdi, private_data);
+ if (ret <= 0)
+ break;
What is involved in tdi "reclaim" separately from "unbind"?
"dev_reclaim" and "tdi_reclaim" seem less precise than "disconnect" and
"unbind".
The firmware operates at the finer granularity so there are
create+connect+disconnect+reclaim (for DEV and TDI). My verbs dictionary
evolved from having all of them in the tsm_ops to this subset which
tells the state the verb leaves the device at. This needs correction, yes.
+
+ ret = spdm_forward(&tdi->tdev->spdm, ret);
+ if (ret < 0)
+ break;
+ }
+ mutex_unlock(&tdi->tdev->spdm_mutex);
+}
+
+static int tsm_tdi_validate(struct tsm_tdi *tdi, bool invalidate, void *private_data)
+{
+ int ret;
+
+ if (!tdi || !tsm.ops->tdi_validate)
+ return -EPERM;
+
+ ret = tsm.ops->tdi_validate(tdi, invalidate, private_data);
+ if (ret) {
+ pci_err(tdi->pdev, "Validation failed, ret=%d", ret);
+ tdi->pdev->dev.tdi_enabled = false;
+ }
+
+ return ret;
+}
+
+/* In case BUS_NOTIFY_PCI_BUS_MASTER is no good, a driver can call pci_dev_tdi_validate() */
No. TDISP is a fundamental re-imagining of the PCI device security
model. It deserves first class support in the PCI core, not bolted on
support via bus notifiers.
This one is about sequencing. For example, writing a zero to BME breaks
a TDI after it moved to CONFIG_LOCKED. So, we either:
1) prevent zeroing BME or
2) delay this "validation" step (which also needs a better name).
If 1), then I can call "validate" from the PCI core before the driver's
probe.
If 2), it is either a driver modification to call "validate" explicitly
or have a notifier like this. Or guest's sysfs - as a VM might want to
boot with a "shared" device, get to the userspace where some daemon
inspects the certificates/etc and "validates" the device only if it is
happy with the result. There may be even some vendor-specific device
configuration happening before the validation step.
[..]
I hesitate to keep commenting because this is so far off of the lifetime
and code organization expectations I thought we were negotiating with
the PCI/TSM series. So I will stop here for now.
Good call, sorry for the mess. Thanks for the review!
ps: I'll just fix the things I did not comment on but I'm not ignoring them.
--
Alexey
