Re: [PATCH 05/11] PCI/TSM: Authenticate devices via platform TSM

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

Aneesh Kumar K.V wrote:
> Xu Yilun <yilun.xu@linux.intel.com> writes:
> 
> > On Fri, Feb 21, 2025 at 01:43:28PM +0530, Aneesh Kumar K.V wrote:
> >> Alexey Kardashevskiy <aik@amd.com> writes:
> >> 
> >> ....
> >> 
> >> >
> >> > I am trying to wrap my head around your tsm. here is what I got in my tree:
> >> > https://github.com/aik/linux/blob/tsm/include/linux/tsm.h
> >> >
> >> > Shortly:
> >> >
> >> > drivers/virt/coco/tsm.ko does sysfs (including "connect" and "bind" to 
> >> > control and "certs"/"report" to attest) and implements tsm_dev/tsm_tdi, 
> >> > it does not know pci_dev;
> >> >
> >> > drivers/pci/tsm-pci.ko creates/destroys tsm_dev/tsm_dev using tsm.ko;
> >> >
> >> > drivers/crypto/ccp/ccp.ko (the PSP guy) registers:
> >> > - tsm_subsys in tsm.ko (which does "connect" and "bind" and
> >> > - tsm_bus_subsys in tsm-pci.ko (which does "spdm_forward")
> >> > ccp.ko knows about pci_dev and whatever else comes in the future, and 
> >> > ccp.ko's "connect" implementation calls the IDE library (I am adopting 
> >> > yours now, with some tweaks).
> >> >
> >> > tsm-dev and tsm-tdi embed struct dev each and are added as children to 
> >> > PCI devices: no hide/show attrs, no additional TSM pointer in struct 
> >> > device or pci_dev, looks like:
> >> >
> >> > aik@sc ~> ls  /sys/bus/pci/devices/0000:e1:04.0/tsm-tdi/tdi:0000:e1:04.0/
> >> > device  power  subsystem  tsm_report  tsm_report_user  tsm_tdi_bind 
> >> > tsm_tdi_status  tsm_tdi_status_user  uevent
> >> >
> >> > aik@sc ~> ls  /sys/bus/pci/devices/0000:e1:04.0/tsm_dev/
> >> > device  power  subsystem  tsm_certs  tsm_cert_slot  tsm_certs_user 
> >> > tsm_dev_connect  tsm_dev_status  tsm_meas  tsm_meas_user  uevent
> >> >
> >> > aik@sc ~> ls /sys/class/tsm/tsm0/
> >> > device  power  stream0:0000:e1:00.0  subsystem  uevent
> >> >
> >> > aik@sc ~> ls /sys/class/tsm-dev/
> >> > tdev:0000:c0:01.1  tdev:0000:e0:01.1  tdev:0000:e1:00.0
> >> >
> >> > aik@sc ~> ls /sys/class/tsm-tdi/
> >> > tdi:0000:c0:01.1  tdi:0000:e0:01.1  tdi:0000:e1:00.0  tdi:0000:e1:04.0 
> >> > tdi:0000:e1:04.1  tdi:0000:e1:04.2  tdi:0000:e1:04.3
> >> >
> >> >
> >> > SPDM forwarding seems a bus-agnostic concept, "connect" is a PCI thing 
> >> > but pci_dev is only needed for DOE/IDE.
> >> >
> >> > Or is separating struct pci_dev from struct device not worth it and most 
> >> > of it should go to tsm-pci.ko? Then what is left for tsm.ko? Thanks,
> >> >
> >> 
> >> For the Arm CCA DA, I have structured the flow as follows. I am
> >> currently refining my changes to prepare them for posting. I am using
> >> tsm-core in both the host and guest. There is no bind interface at the
> >> sysfs level; instead, it is managed via the KVM ioctl
> >> 
> >> Host:
> >> step 1.
> >> echo ${DEVICE} > /sys/bus/pci/devices/${DEVICE}/driver/unbind
> >> echo vfio-pci > /sys/bus/pci/devices/${DEVICE}/driver_override
> >> echo ${DEVICE} > /sys/bus/pci/drivers_probe
> >> 
> >> step 2.
> >> echo 1 > /sys/bus/pci/devices/$DEVICE/tsm/connect
> >> 
> >> step 3.
> >> using VMM to make the new KVM_SET_DEVICE_ATTR ioctl
> >> 
> >> +		dev_num = vfio_devices[i].dev_hdr.dev_num;
> >> +		/* kvmtool only do 0 domain, 0 bus and 0 function devices. */
> >> +		guest_bdf = (0ULL << 32) | (0 << 16) | dev_num << 11 | (0 << 8);
> >> +
> >> +		struct kvm_vfio_tsm_bind param = {
> >> +			.guest_rid = guest_bdf,
> >> +			.devfd = vfio_devices[i].fd,
> >> +		};
> >> +		struct kvm_device_attr attr = {
> >> +			.group = KVM_DEV_VFIO_DEVICE,
> >> +			.attr = KVM_DEV_VFIO_DEVICE_TDI_BIND,
> >> +			.addr = (__u64)&param,
> >> +		};
> >> +
> >> +		if (ioctl(kvm_vfio_device, KVM_SET_DEVICE_ATTR, &attr)) {
> >> +			pr_err("Failed KVM_SET_DEVICE_ATTR for KVM_DEV_VFIO_DEVICE");
> >> +			return -ENODEV;
> >> +		}
> >> +
> >
> > I think bind (which brings device to a LOCKED state, no MMIO, no DMA)
> > cannot be a driver agnostic behavior. So I think it should be a VFIO
> > ioctl.
> >
> 
> For the current CCA implementation bind is equivalent to VDEV_CREATE
> which doesn't mark the device LOCKED. Marking the device LOCKED is
> driven by the guest as shown in the steps below.

I plan to switch focus to the bind flow after we achieve consensus on
the base TSM framework pieces, but my initial reaction is that
separating "bind" from "lock" is a finer grained state transition than
has been discussed previously. There are end use cases that justify
exposing LOCKED vs RUN in the ABI, but could point to the use case for
separating the BOUND vs LOCKED states?

> >> Now in the guest we follow the below steps
> >> 
> >> step 1:
> >> echo ${DEVICE} > /sys/bus/pci/devices/${DEVICE}/driver/unbind
> >> 
> >> step 2: Move the device to TDISP LOCK state
> >> echo 1 > /sys/bus/pci/devices/0000:00:00.0/tsm/connect
> >> echo 3 > /sys/bus/pci/devices/0000:00:00.0/tsm/connect
> >
> > Reuse the 'connect' interface? I think it conceptually brings chaos. Is
> > it better we create a new interface?
> >
> 
> I was looking at converting these numbers to strings.
> "1" -> connect
> "2" -> lock

I have been modeling Host-side "connect" as IDE establishment on the PF
while Guest-side "connect" arranges for "bind+lock" on an assigned
function / TDI. Do we really need to expose "lock" as an explicit state
vs interpret what "connect" means in the different contexts?

> "3" -> run
> 
> >> step 3: Moves the device to TDISP RUN state
> >> echo 4 > /sys/bus/pci/devices/0000:00:00.0/tsm/connect
> >
> > Could you elaborate what '1'/'3'/'4' stand for?
> >
> 
> As mentioned above, them move the device to different TDISP state.
> 
> I will reply to this patch with my early RFC chnages for tsm framework.
> I am not yet ready to share the CCA backend changes. But I assume having
> the tsm framework changes alone can be useful?

Yes. There are so many moving pieces and multiple vendors that the only
way to make progress here is to wrestle the common pieces into a form
that all vendors can agree. Feel free to extend the samples/devsec/
implementation to demonstrate flows that CCA needs. The idea is that
sample implementation serves as both a reference implementation and a
simple smoke test for all the common core pieces.