Re: [PATCH 4/6] PCI/TSM: Add pci_tsm_bind() helper for instantiating TDIs

[Aneesh Kumar K.V <aneesh.kumar@kernel.org>]

Dan Williams <dan.j.williams@intel.com> writes:

...

> +/**
> + * pci_tsm_bind() - Bind @pdev as a TDI for @kvm
> + * @pdev: PCI device function to bind
> + * @kvm: Private memory attach context
> + * @tdi_id: Identifier (virtual BDF) for the TDI as referenced by the TSM and DSM
> + *
> + * Returns 0 on success, or a negative error code on failure.
> + *
> + * Context: Caller is responsible for constraining the bind lifetime to the
> + * registered state of the device. For example, pci_tsm_bind() /
> + * pci_tsm_unbind() limited to the VFIO driver bound state of the device.
> + */
> +int pci_tsm_bind(struct pci_dev *pdev, struct kvm *kvm, u32 tdi_id)
> +{
> +	struct pci_tsm_pf0 *tsm_pf0;
> +	struct pci_tdi *tdi;
> +
> +	if (!kvm)
> +		return -EINVAL;
> +
> +	guard(rwsem_read)(&pci_tsm_rwsem);
> +
> +	if (!pdev->tsm)
> +		return -EINVAL;
> +
> +	if (!is_link_tsm(pdev->tsm->tsm_dev))
> +		return -ENXIO;
> +
> +	tsm_pf0 = to_pci_tsm_pf0(pdev->tsm);
> +	guard(mutex)(&tsm_pf0->lock);
> +
> +	/* Resolve races to bind a TDI */
> +	if (pdev->tsm->tdi) {
> +		if (pdev->tsm->tdi->kvm == kvm)
> +			return 0;
> +		else
> +			return -EBUSY;
> +	}
> +
> +	tdi = to_pci_tsm_ops(pdev->tsm)->bind(pdev, kvm, tdi_id);
> +	if (IS_ERR(tdi))
> +		return PTR_ERR(tdi);
> +
> +	pdev->tsm->tdi = tdi;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(pci_tsm_bind);

Can we set tdi in the constructor? I use it later as part of a bind()
callback. I’d prefer not to pass tdi as an argument to those functions,
since functions like do_dev_communicate() are reused in other contexts
where tdi isn't needed.

cca_tsm_bind -> cca_vdev_create -> vdev state transition to VDEV_UNLOCKED.

	/* setup host_tdi before call to device communicate */
	host_tdi = to_cca_host_tdi(pdev);
	host_tdi->rmm_vdev = rmm_vdev;

	ret = submit_vdev_state_transition_work(pdev, RMI_VDEV_UNLOCKED);
	/* failure is treated as rmi_vdev_create failure */
	if (ret)
		goto err_vdev_comm;


static inline struct cca_host_tdi *to_cca_host_tdi(struct pci_dev *pdev)
{
	struct pci_tsm *tsm = pdev->tsm;

	if (!tsm || !tsm->tdi)
		return NULL;

	return container_of(tsm->tdi, struct cca_host_tdi, tdi);
}


modified   drivers/pci/tsm.c
@@ -391,8 +391,6 @@ int pci_tsm_bind(struct pci_dev *pdev, struct kvm *kvm, u32 tdi_id)
 	if (IS_ERR(tdi))
 		return PTR_ERR(tdi);
 
-	pdev->tsm->tdi = tdi;
-
 	return 0;
 }
 EXPORT_SYMBOL_GPL(pci_tsm_bind);
@@ -998,6 +996,7 @@ static struct pci_dev *find_dsm_dev(struct pci_dev *pdev)
 void pci_tsm_tdi_constructor(struct pci_dev *pdev, struct pci_tdi *tdi,
 			     struct kvm *kvm, u32 tdi_id)
 {
+	pdev->tsm->tdi = tdi;
 	tdi->pdev = pdev;
 	tdi->kvm = kvm;
 	tdi->tdi_id = tdi_id;

We could do a pci_tsm_tdi_destructor() to reset pdev->tsm->tdi = NULL?  

-aneesh