Re: [RFC PATCH v2 07/22] coco/tsm: Add tsm and tsm-host modules

[Dan Williams <dan.j.williams@intel.com>]

Alexey Kardashevskiy wrote:
> The TSM module is a library to create sysfs nodes common for hypervisors
> and VMs. It also provides helpers to parse interface reports (required
> by VMs, visible to HVs). It registers 3 device classes:
> - tsm: one per platform,
> - tsm-dev: for physical functions, ("TDEV");
> - tdm-tdi: for PCI functions being assigned to VMs ("TDI").
> 
> The library adds a child device of "tsm-dev" or/and "tsm-tdi" class
> for every capable PCI device. Note that the module is made bus-agnostic.

There was some discussion on the merits of "TDEV" and "TDI" objects in
the PCI/TSM thread [1], I will summarize the main objections here:

 * PCI device security is a PCI device property. That security property is
   not strictly limited to the platform TEE Security Manager (TSM) case.
   The PCI device authentication enabling, mentioned as "Lukas's CMA" in
   the cover letter, adds a TSM-independent authentication and device
   measurement collection ABI. The PCI/TSM proposal simply aims to reuse
   that ABI and existing PCI device object lifecycle expectations.

 * PCI device security is a PCI specification [2]. The acronym soup of PCI
   device security (TDISP, IDE, CMA, SPDM, DOE) is deeply entangled with
   PCI specifics. If other buses grow the ability to add devices to a
   confidential VM's TCB that future enabling need not be encumbered by
   premature adherence to the TDEV+TDI object model, the bus can do what
   makes sense for its specific mechanisms. The kernel can abstract common
   attributes and ABI without the burden of a new object model.

[1]: http://lore.kernel.org/67b8e5328fd41_2d2c294e5@dwillia2-xfh.jf.intel.com.notmuch
[2]: http://lore.kernel.org/67c128dcb5c21_1a7729454@dwillia2-xfh.jf.intel.com.notmuch

> New device nodes provide sysfs interface for fetching device certificates
> and measurements and TDI interface reports.
> Nodes with the "_user" suffix provide human-readable information, without
> that suffix it is raw binary data to be copied to a guest.
> 
> The TSM-HOST module adds hypervisor-only functionality on top. At the
> moment it is:
> - "connect" to enable/disable IDE (a PCI link encryption);
> - "TDI bind" to manage a PCI function passed through to a secure VM.
> 
> A platform is expected to register itself in TSM-HOST and provide
> necessary callbacks. No platform is added here, AMD SEV is coming in the
> next patches.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@amd.com>
[..]
> diff --git a/drivers/virt/coco/host/tsm-host.c b/drivers/virt/coco/host/tsm-host.c
> new file mode 100644
> index 000000000000..80f3315fb195
> --- /dev/null
> +++ b/drivers/virt/coco/host/tsm-host.c
[..]
> +static ssize_t tsm_dev_status_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct tsm_dev *tdev = container_of(dev, struct tsm_dev, dev);
> +	struct tsm_dev_status s = { 0 };
> +	int ret = tsm_dev_status(tdev, &s);
> +	ssize_t ret1;

I know this is just an RFC, but...

> +
> +	ret1 = sysfs_emit(buf, "ret=%d\n"

What does "ret" mean to userspace?

> +			  "ctx_state=%x\n"

This violates the one property per file sysfs expectation.

> +			  "tc_mask=%x\n"

Is this the Link IDE traffic class?

> +			  "certs_slot=%x\n"
> +			  "device_id=%x:%x.%d\n"
> +			  "segment_id=%x\n"

These last 2 lines are all redundant information relative to the PCI
device name, right?

> +			  "no_fw_update=%x\n",
> +			  ret,
> +			  s.ctx_state,
> +			  s.tc_mask,
> +			  s.certs_slot,
> +			  (s.device_id >> 8) & 0xff,
> +			  (s.device_id >> 3) & 0x1f,
> +			  s.device_id & 0x07,
> +			  s.segment_id,
> +			  s.no_fw_update);
> +
> +	tsm_dev_put(tdev);

I would not expect sysfs to need to manage device references. If the
device is registered sysfs is live and the reference is already
elevated. If the device is unregistered, sysfs is disabled and attribute
handlers are no longer executing.

[..]
> +static ssize_t tsm_tdi_status_user_show(struct device *dev,
> +					struct device_attribute *attr,
> +					char *buf)
> +{
> +	struct tsm_tdi *tdi = container_of(dev, struct tsm_tdi, dev);
> +	struct tsm_dev *tdev = tdi->tdev;
> +	struct tsm_host_subsys *hsubsys = (struct tsm_host_subsys *) tdev->tsm;
> +	struct tsm_tdi_status ts = { 0 };
> +	char algos[256] = "";
> +	unsigned int n, m;
> +	int ret;
> +
> +	ret = tsm_tdi_status(tdi, hsubsys->private_data, &ts);
> +	if (ret < 0)
> +		return sysfs_emit(buf, "ret=%d\n\n", ret);
> +
> +	if (!ts.valid)
> +		return sysfs_emit(buf, "ret=%d\nstate=%d:%s\n",
> +				  ret, ts.state, tdisp_state_to_str(ts.state));
> +
> +	n = snprintf(buf, PAGE_SIZE,
> +		     "ret=%d\n"
> +		     "state=%d:%s\n"
> +		     "meas_digest_fresh=%x\n"
> +		     "meas_digest_valid=%x\n"
> +		     "all_request_redirect=%x\n"
> +		     "bind_p2p=%x\n"
> +		     "lock_msix=%x\n"
> +		     "no_fw_update=%x\n"
> +		     "cache_line_size=%d\n"
> +		     "algos=%#llx:%s\n"
> +		     "report_counter=%lld\n"
> +		     ,
> +		     ret,
> +		     ts.state, tdisp_state_to_str(ts.state),
> +		     ts.meas_digest_fresh,
> +		     ts.meas_digest_valid,
> +		     ts.all_request_redirect,
> +		     ts.bind_p2p,
> +		     ts.lock_msix,
> +		     ts.no_fw_update,
> +		     ts.cache_line_size,
> +		     ts.spdm_algos, spdm_algos_to_str(ts.spdm_algos, algos, sizeof(algos) - 1),
> +		     ts.intf_report_counter);
> +
> +	n += snprintf(buf + n, PAGE_SIZE - n, "Certs digest: ");
> +	m = hex_dump_to_buffer(ts.certs_digest, sizeof(ts.certs_digest), 32, 1,
> +			       buf + n, PAGE_SIZE - n, false);
> +	n += min(PAGE_SIZE - n, m);
> +	n += snprintf(buf + n, PAGE_SIZE - n, "...\nMeasurements digest: ");
> +	m = hex_dump_to_buffer(ts.meas_digest, sizeof(ts.meas_digest), 32, 1,
> +			       buf + n, PAGE_SIZE - n, false);
> +	n += min(PAGE_SIZE - n, m);
> +	n += snprintf(buf + n, PAGE_SIZE - n, "...\nInterface report digest: ");
> +	m = hex_dump_to_buffer(ts.interface_report_digest, sizeof(ts.interface_report_digest),
> +			       32, 1, buf + n, PAGE_SIZE - n, false);
> +	n += min(PAGE_SIZE - n, m);
> +	n += snprintf(buf + n, PAGE_SIZE - n, "...\n");

More sysfs expectation violations...

Let's start working on what the Plumbers feedback to Lukas on his
attempt to export PCI CMA device evidence through sysfs means for the
TSM side. I do not expect it will all be strictly reusable but the
transport and some record formats should be unified. Specifically I want
to start the discussion about collaboration and differences among
PCI-CMA-netlink and PCI-TSM-netlink. For example, device measurements
are ostensibly common, but interface reports are unique to the TSM flow.