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

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

Xu Yilun <yilun.xu@linux.intel.com> writes:

> On Wed, Feb 26, 2025 at 05:40:02PM +0530, 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.
>
> Could you elaborate why vdev create & LOCK can't be done at the same
> time, when guest requests "lock"? Intel TDX also requires firmware calls
> like tdi_create(alloc metadata) & tdi_bind(do LOCK), but I don't see
> there is need to break them out in different phases.
>

Yes, that is possible and might be what I will end up doing. Right now
I have kept the interface flexible enough as I am writing these changes.
Device can possibly be presented in locked state to the guest.

>
>> 
>> 
>> >> 
>> >> 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
>
> What does "connect" do in guest?
>

Nothing much for now. I added that to keep it consistent with host
workflow. That is device transition from PCI_TSM_INIT -> PCI_TSM_CONNECT
-> PCI_TSM_BOUND -> PCI_TSM_LOCK -> PCI_TSM_RUN.

Relevant part of the TSM backend in guest

static int cca_tsm_connect(struct pci_dev *pdev, int new_state)
{
	unsigned long ret;
	int connect_state;
	struct pci_tsm *pci_tsm = pdev->tsm;

	connect_state = pci_tsm->state;
	switch (new_state) {
	case PCI_TSM_CONNECT:
		pci_tsm->state = PCI_TSM_CONNECT;
		break;
	case PCI_TSM_LOCKED:
		if (connect_state != PCI_TSM_CONNECT)
			return -EINVAL;

		ret = rsi_device_lock(pdev);
		if (ret) {
			pci_err(pdev, "failed to lock the device (%lu)\n", ret);
			return -EIO;
		}
		pci_tsm->state = PCI_TSM_LOCKED;
		break;


-aneesh