Xu Yilun <yilun.xu@xxxxxxxxxxxxxxx> writes: > On Wed, Feb 26, 2025 at 05:40:02PM +0530, Aneesh Kumar K.V wrote: >> Xu Yilun <yilun.xu@xxxxxxxxxxxxxxx> writes: >> >> > On Fri, Feb 21, 2025 at 01:43:28PM +0530, Aneesh Kumar K.V wrote: >> >> Alexey Kardashevskiy <aik@xxxxxxx> 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)¶m, >> >> + }; >> >> + >> >> + 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
