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

[Dan Williams <dan.j.williams@intel.com>]

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.