Re: [PATCH v3 12/13] PCI/TSM: support TDI related operations for host TSM driver

[Xu Yilun <yilun.xu@linux.intel.com>]

On Tue, May 20, 2025 at 01:12:12PM -0700, Dan Williams wrote:
> Alexey Kardashevskiy wrote:
> [..]
> > > +int pci_tsm_bind(struct pci_dev *pdev, struct kvm *kvm, u64 tdi_id)
> > > +{
> > > +	struct pci_tdi *tdi;
> > > +
> > > +	if (!kvm)
> > > +		return -EINVAL;
> > 
> > Does not belong here, if the caller failed to get the kvm pointer from
> > an fd, then that caller should handle it.
> 
> Sure.
> 
> [..]
> > > +static int __pci_tsm_unbind(struct pci_dev *pdev)
> > > +{
> > > +	struct pci_tdi *tdi;
> > > +
> > > +	lockdep_assert_held(&pci_tsm_rwsem);
> > > +
> > > +	if (!pdev->tsm)
> > > +		return -EINVAL;
> > 
> > Nothing checks for these errors.
> 
> True this function signature should probably drop the error code
> altogether and become a void helper. I.e. it should be impossible for a
> bound device to not have a reference, or not be in the right state.
> 
> > 
> > > +
> > > +	struct pci_dev *pf0_dev __free(pci_dev_put) = tsm_pf0_get(pdev);
> > > +	if (!pf0_dev)
> > > +		return -EINVAL;
> > > +
> > > +	struct mutex *lock __free(tdi_ops_unlock) = tdi_ops_lock(pf0_dev);
> > > +	if (IS_ERR(lock))
> > > +		return PTR_ERR(lock);
> > > +
> > > +	tdi = pdev->tsm->tdi;
> > > +	if (!tdi)
> > > +		return 0;
> > > +
> > > +	tsm_ops->unbind(tdi);
> > > +	pdev->tsm->tdi = NULL;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +int pci_tsm_unbind(struct pci_dev *pdev)
> > > +{
> > > +	struct rw_semaphore *lock __free(tsm_read_unlock) = tsm_read_lock();
> > > +	if (!lock)
> > > +		return -EINTR;
> > > +
> > > +	return __pci_tsm_unbind(pdev);
> > > +}
> > > +EXPORT_SYMBOL_GPL(pci_tsm_unbind);
> > > +
> > > +/**
> > > + * pci_tsm_guest_req - VFIO/IOMMUFD helper to handle guest requests
> > > + * @pdev: @pdev representing a bound tdi
> > > + * @info: envelope for the request
> > > + *
> > > + * Expected flow is guest low-level TSM driver initiates a guest request
> > > + * like "transition TDISP state to RUN", "fetch report" via a
> > > + * technology specific guest-host-interface and KVM exit reason. KVM
> > > + * posts to userspace (e.g. QEMU) that holds the host-to-guest RID
> > > + * mapping.
> > > + */
> > > +int pci_tsm_guest_req(struct pci_dev *pdev, struct pci_tsm_guest_req_info *info)
> > > +{
> > > +	struct pci_tdi *tdi;
> > > +	int rc;
> > > +
> > > +	lockdep_assert_held_read(&pci_tsm_rwsem);
> > > +
> > > +	if (!pdev->tsm)
> > > +		return -ENODEV;
> > > +
> > > +	struct pci_dev *pf0_dev __free(pci_dev_put) = tsm_pf0_get(pdev);
> > > +	if (!pf0_dev)
> > > +		return -EINVAL;
> > > +
> > > +	struct mutex *lock __free(tdi_ops_unlock) = tdi_ops_lock(pf0_dev);
> > > +	if (IS_ERR(lock))
> > > +		return -ENODEV;
> > > +
> > > +	tdi = pdev->tsm->tdi;
> > > +	if (!tdi)
> > > +		return -ENODEV;
> > > +
> > > +	rc = tsm_ops->guest_req(pdev, info);
> > > +	if (rc)
> > > +		return -EIO;
> > 
> > return rc.
> 
> Agree.
> 
> [..]
> > > @@ -86,12 +101,40 @@ static inline bool is_pci_tsm_pf0(struct pci_dev *pdev)
> > >   	return PCI_FUNC(pdev->devfn) == 0;
> > >   }
> > >   
> > > +enum pci_tsm_guest_req_type {
> > > +	PCI_TSM_GUEST_REQ_TDXC,
> > 
> > Will Intel ever need more types here?
> 
> I doubt it as this is routing to a TDX vs TIO vs ... blob handler. It is
> unfortunate that we need this indirection (i.e. missing
> standardization), but it is in the same line-of-thought as
> configfs-tsm-report providing a transport along with a
> technology-specific "provider" identifier for parsing the blob.
> 
> > > + * struct pci_tsm_guest_req_info - parameter for pci_tsm_ops.guest_req()
> > > + * @type: identify the format of the following blobs
> > > + * @type_info: extra input/output info, e.g. firmware error code
> > 
> > Call it "fw_ret"?
> 
> Sure.

This field is intended for out-of-blob values, like fw_ret. But fw_ret
is specified in GHCB and is vendor specific. Other vendors may also
have different values of this kind.

So I intend to gather these out-of-blob values in type_info, like:

enum pci_tsm_guest_req_type {
  PCI_TSM_GUEST_REQ_TDXC,
  PCI_TSM_GUEST_REQ_SEV_SNP,
};

/* SEV SNP guest request type info */
struct pci_tsm_guest_req_sev_snp {
	s32 fw_err;
};

Since IOMMUFD has the userspace entry, maybe these definitions should be
moved to include/uapi/linux/iommufd.h.

In pci-tsm.h, just define:

struct pci_tsm_guest_req_info {
	u32 type;
	void __user *type_info;
	size_t type_info_len;
	void __user *req;
	size_t req_len;
	void __user *resp;
	size_t resp_len;
};

BTW: TDX Connect has no out-of-blob value, so should set type_info_len = 0

> 
> [..]
> > > @@ -102,6 +145,11 @@ struct pci_tsm_ops {
> > >   	void (*remove)(struct pci_tsm *tsm);
> > >   	int (*connect)(struct pci_dev *pdev);
> > >   	void (*disconnect)(struct pci_dev *pdev);
> > > +	struct pci_tdi *(*bind)(struct pci_dev *pdev, struct pci_dev *pf0_dev,
> > > +				struct kvm *kvm, u64 tdi_id);
> > 
> > p0_dev is not needed here, we should be able to calculate it from pdev.
> 
> The pci_tsm core needs to hold the lock before calling this routine. At
> that point might as well pass the already looked up device rather than
> require the low-level TSM driver to repeat that work.
> 
> > tdi_id is 32bit.
> 
> @Yilun, I saw that you had it as 64-bit in one location, was that
> unintentional.

Intel is also switching to gBDF as tdi_id, so yes 32bit is good.

> 
> Note that INTERFACE_ID is Reserved to be 12-bytes, but today FUNCTION_ID
> is indeed only 4-bytes. I will fix this up unless some arch speaks up
> and says they need to pass a larger id around.

Yes.

Thanks,
Yilun

> 
> > Should return an error code. Thanks,
> 
> Lets make it an ERR_PTR() because the low-level provider likely needs to
> allocate more than just a 'struct pci_tdi' on bind.