Re: [PATCH 1/7] PCI/TSM: Add pci_tsm_{bind,unbind}() methods for instantiating TDIs

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

Aneesh Kumar K.V wrote:
> 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)
> > +{
> > +	const struct pci_tsm_ops *ops;
> > +	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;
> > +
> > +	ops = pdev->tsm->ops;
> > +
> > +	if (!is_link_tsm(ops->owner))
> > +		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 = ops->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);
> > +
> 
> Are we missing assigning pdev and kvm in the above function? 
> 
> modified   drivers/pci/tsm.c
> @@ -356,6 +356,8 @@ int pci_tsm_bind(struct pci_dev *pdev, struct kvm *kvm, u32 tdi_id)
>  	if (IS_ERR(tdi))
>  		return PTR_ERR(tdi);
>  
> +	tdi->pdev = pdev;
> +	tdi->kvm = kvm;

Indeed, but I think I will keep this as the same "constructor" pattern
so it is the TSM driver's job to make sure 'struct pci_tdi' is fully
initialized when ->bind() returns.

This is one of the oversights of not having a ->bind() flow yet in
samples/devsec/.

-- 8< --
diff --git a/drivers/pci/tsm.c b/drivers/pci/tsm.c
index 4688ddbc0b33..125c682d2e08 100644
--- a/drivers/pci/tsm.c
+++ b/drivers/pci/tsm.c
@@ -519,6 +519,20 @@ static struct pci_dev *find_dsm_dev(struct pci_dev *pdev)
 	return NULL;
 }
 
+/**
+ * pci_tsm_tdi_constructor() - base 'struct pci_tdi' initialization for link TSMs
+ * @pdev: The PCI device
+ * @tsm: context to initialize
+ * @ops: PCI link operations provided by the TSM
+ */
+void pci_tsm_tdi_constructor(struct pci_dev *pdev, struct pci_tdi *tdi,
+			     struct kvm *kvm)
+{
+	tdi->pdev = pdev;
+	tdi->kvm = kvm;
+}
+EXPORT_SYMBOL_GPL(pci_tsm_tdi_constructor);
+
 /**
  * pci_tsm_link_constructor() - base 'struct pci_tsm' initialization for link TSMs
  * @pdev: The PCI device
diff --git a/include/linux/pci-tsm.h b/include/linux/pci-tsm.h
index 61c947ff8735..7eae8a1a2853 100644
--- a/include/linux/pci-tsm.h
+++ b/include/linux/pci-tsm.h
@@ -161,6 +161,8 @@ int pci_tsm_doe_transfer(struct pci_dev *pdev, u8 type, const void *req,
 			 size_t req_sz, void *resp, size_t resp_sz);
 int pci_tsm_bind(struct pci_dev *pdev, struct kvm *kvm, u32 tdi_id);
 void pci_tsm_unbind(struct pci_dev *pdev);
+void pci_tsm_tdi_constructor(struct pci_dev *pdev, struct pci_tdi *tdi,
+			     struct kvm *kvm);
 #else
 static inline int pci_tsm_register(struct tsm_dev *tsm_dev)
 {