Re: [RFC PATCH 07/21] pci/tdisp: Introduce tsm module

Alexey Kardashevskiy
2024-09-04T11:13:36Z

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@amd.com>
>> ---
>> 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@amd.com"
>> +#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