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

[Aneesh Kumar K.V <aneesh.kumar@kernel.org>]

Dan Williams <dan.j.williams@intel.com> writes:

> 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?
>

Can you share the link or reference to the previous discussion? I wasn’t
aware of it.

I chose to implement vdev_create and TDISP lock as two separate steps to
better align with the RMM spec[1]. Additionally, there is a possibility
that the guest might need to perform certain operations that either
cannot be executed in the TDISP locked state or would cause the device
to transition to an unlocked state.

In such cases, wouldn’t we need a guest interface to move the device
back to the locked state? I understand that this process might trigger a
device reset, which could even require a full restart of the device
assignment workflow

[1] https://developer.arm.com/-/cdn-downloads/permalink/Architectures/Armv9/DEN0137_1.1-alp12.zip

>
>> >> 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?
>

One possible use case I was considering is a guest needing to perform
certain operations before the device transitions to the locked state.

>> "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.

I haven't looked at samples/devsec yet because it has an x86 PCI
dependency, and it was easier to get the ARM RME backend working.
However, I will try to update devsec to work with ARM RME as well

-aneesh