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.
