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.
[..]
> > @@ -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.
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.
> 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.
