diff mbox series

[v2,10/19] iommufd/viommu: Add vdev_id helpers for IOMMU drivers

Message ID 6ccdfb27c7aa5a5bb7e153165cf90114cae4687c.1724776335.git.nicolinc@nvidia.com
State New
Headers show
Series iommufd: Add VIOMMU infrastructure (Part-1) | expand

Commit Message

Nicolin Chen Aug. 27, 2024, 4:59 p.m. UTC 
Driver can call the iommufd_viommu_find_device() to find a device pointer
using its per-viommu virtual ID. The returned device must be protected by
the pair of iommufd_viommu_lock/unlock_vdev_id() function.

Put these three functions into a new viommu_api file, to build it with the
IOMMUFD_DRIVER config.

Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/iommu/iommufd/Makefile     |  2 +-
 drivers/iommu/iommufd/viommu_api.c | 39 ++++++++++++++++++++++++++++++
 include/linux/iommufd.h            | 16 ++++++++++++
 3 files changed, 56 insertions(+), 1 deletion(-)
 create mode 100644 drivers/iommu/iommufd/viommu_api.c

Comments

Jason Gunthorpe Sept. 5, 2024, 4:14 p.m. UTC | #1 
On Tue, Aug 27, 2024 at 09:59:47AM -0700, Nicolin Chen wrote:
> Driver can call the iommufd_viommu_find_device() to find a device pointer
> using its per-viommu virtual ID. The returned device must be protected by
> the pair of iommufd_viommu_lock/unlock_vdev_id() function.
> 
> Put these three functions into a new viommu_api file, to build it with the
> IOMMUFD_DRIVER config.
> 
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> ---
>  drivers/iommu/iommufd/Makefile     |  2 +-
>  drivers/iommu/iommufd/viommu_api.c | 39 ++++++++++++++++++++++++++++++
>  include/linux/iommufd.h            | 16 ++++++++++++
>  3 files changed, 56 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/iommu/iommufd/viommu_api.c

I still think this is better to just share the struct content with the
driver, eventually we want to do this anyhow as the driver will
want to use container_of() techniques to reach its private data.

> +/*
> + * Find a device attached to an VIOMMU object using a virtual device ID that was
> + * set via an IOMMUFD_CMD_VIOMMU_SET_VDEV_ID. Callers of this function must call
> + * iommufd_viommu_lock_vdev_id() prior and iommufd_viommu_unlock_vdev_id() after
> + *
> + * Return device or NULL.
> + */
> +struct device *iommufd_viommu_find_device(struct iommufd_viommu *viommu, u64 id)
> +{
> +	struct iommufd_vdev_id *vdev_id;
> +
> +	lockdep_assert_held(&viommu->vdev_ids_rwsem);
> +
> +	xa_lock(&viommu->vdev_ids);
> +	vdev_id = xa_load(&viommu->vdev_ids, (unsigned long)id);
> +	xa_unlock(&viommu->vdev_ids);

No need for this lock, xa_load is rcu safe against concurrent writer

Jason
Nicolin Chen Sept. 5, 2024, 5:53 p.m. UTC | #2 
On Thu, Sep 05, 2024 at 01:14:15PM -0300, Jason Gunthorpe wrote:
> On Tue, Aug 27, 2024 at 09:59:47AM -0700, Nicolin Chen wrote:
> > Driver can call the iommufd_viommu_find_device() to find a device pointer
> > using its per-viommu virtual ID. The returned device must be protected by
> > the pair of iommufd_viommu_lock/unlock_vdev_id() function.
> > 
> > Put these three functions into a new viommu_api file, to build it with the
> > IOMMUFD_DRIVER config.
> > 
> > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> > ---
> >  drivers/iommu/iommufd/Makefile     |  2 +-
> >  drivers/iommu/iommufd/viommu_api.c | 39 ++++++++++++++++++++++++++++++
> >  include/linux/iommufd.h            | 16 ++++++++++++
> >  3 files changed, 56 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/iommu/iommufd/viommu_api.c
> 
> I still think this is better to just share the struct content with the
> driver, eventually we want to do this anyhow as the driver will
> want to use container_of() techniques to reach its private data.

In my mind, exposing everything to the driver is something that
we have to (for driver-managed structures) v.s. we want to...
Even in that case, a driver actually only need to know the size
of the core structure, without touching what's inside(?).

I am a bit worried that drivers would abuse the content in the
core-level structure.. Providing a set of API would encourage
them to keep the core structure intact, hopefully..

> > +/*
> > + * Find a device attached to an VIOMMU object using a virtual device ID that was
> > + * set via an IOMMUFD_CMD_VIOMMU_SET_VDEV_ID. Callers of this function must call
> > + * iommufd_viommu_lock_vdev_id() prior and iommufd_viommu_unlock_vdev_id() after
> > + *
> > + * Return device or NULL.
> > + */
> > +struct device *iommufd_viommu_find_device(struct iommufd_viommu *viommu, u64 id)
> > +{
> > +	struct iommufd_vdev_id *vdev_id;
> > +
> > +	lockdep_assert_held(&viommu->vdev_ids_rwsem);
> > +
> > +	xa_lock(&viommu->vdev_ids);
> > +	vdev_id = xa_load(&viommu->vdev_ids, (unsigned long)id);
> > +	xa_unlock(&viommu->vdev_ids);
> 
> No need for this lock, xa_load is rcu safe against concurrent writer

I see iommufd's device.c and main.c grab xa_lock before xa_load?

Thanks
Nicolin
Jason Gunthorpe Sept. 11, 2024, 11:11 p.m. UTC | #3 
On Thu, Sep 05, 2024 at 10:53:31AM -0700, Nicolin Chen wrote:
> On Thu, Sep 05, 2024 at 01:14:15PM -0300, Jason Gunthorpe wrote:
> > On Tue, Aug 27, 2024 at 09:59:47AM -0700, Nicolin Chen wrote:
> > > Driver can call the iommufd_viommu_find_device() to find a device pointer
> > > using its per-viommu virtual ID. The returned device must be protected by
> > > the pair of iommufd_viommu_lock/unlock_vdev_id() function.
> > > 
> > > Put these three functions into a new viommu_api file, to build it with the
> > > IOMMUFD_DRIVER config.
> > > 
> > > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> > > ---
> > >  drivers/iommu/iommufd/Makefile     |  2 +-
> > >  drivers/iommu/iommufd/viommu_api.c | 39 ++++++++++++++++++++++++++++++
> > >  include/linux/iommufd.h            | 16 ++++++++++++
> > >  3 files changed, 56 insertions(+), 1 deletion(-)
> > >  create mode 100644 drivers/iommu/iommufd/viommu_api.c
> > 
> > I still think this is better to just share the struct content with the
> > driver, eventually we want to do this anyhow as the driver will
> > want to use container_of() techniques to reach its private data.
> 
> In my mind, exposing everything to the driver is something that
> we have to (for driver-managed structures) v.s. we want to...
> Even in that case, a driver actually only need to know the size
> of the core structure, without touching what's inside(?).
> 
> I am a bit worried that drivers would abuse the content in the
> core-level structure.. Providing a set of API would encourage
> them to keep the core structure intact, hopefully..

This is always a tension in the kernel. If the core apis can be nice
and tidy then it is a reasonable direction

But here I think we've cross some threshold where the APIs are
complex, want to be inlined and really we just want to expose data not
APIs to drivers.

> > No need for this lock, xa_load is rcu safe against concurrent writer
> 
> I see iommufd's device.c and main.c grab xa_lock before xa_load?

That is not to protect the xa_load, it is to protect the lifetime of
pointer it returns

Jason
Nicolin Chen Sept. 12, 2024, 3:17 a.m. UTC | #4 
On Wed, Sep 11, 2024 at 08:11:03PM -0300, Jason Gunthorpe wrote:
> On Thu, Sep 05, 2024 at 10:53:31AM -0700, Nicolin Chen wrote:
> > On Thu, Sep 05, 2024 at 01:14:15PM -0300, Jason Gunthorpe wrote:
> > > On Tue, Aug 27, 2024 at 09:59:47AM -0700, Nicolin Chen wrote:
> > > > Driver can call the iommufd_viommu_find_device() to find a device pointer
> > > > using its per-viommu virtual ID. The returned device must be protected by
> > > > the pair of iommufd_viommu_lock/unlock_vdev_id() function.
> > > > 
> > > > Put these three functions into a new viommu_api file, to build it with the
> > > > IOMMUFD_DRIVER config.
> > > > 
> > > > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> > > > ---
> > > >  drivers/iommu/iommufd/Makefile     |  2 +-
> > > >  drivers/iommu/iommufd/viommu_api.c | 39 ++++++++++++++++++++++++++++++
> > > >  include/linux/iommufd.h            | 16 ++++++++++++
> > > >  3 files changed, 56 insertions(+), 1 deletion(-)
> > > >  create mode 100644 drivers/iommu/iommufd/viommu_api.c
> > > 
> > > I still think this is better to just share the struct content with the
> > > driver, eventually we want to do this anyhow as the driver will
> > > want to use container_of() techniques to reach its private data.
> > 
> > In my mind, exposing everything to the driver is something that
> > we have to (for driver-managed structures) v.s. we want to...
> > Even in that case, a driver actually only need to know the size
> > of the core structure, without touching what's inside(?).
> > 
> > I am a bit worried that drivers would abuse the content in the
> > core-level structure.. Providing a set of API would encourage
> > them to keep the core structure intact, hopefully..
> 
> This is always a tension in the kernel. If the core apis can be nice
> and tidy then it is a reasonable direction
> 
> But here I think we've cross some threshold where the APIs are
> complex, want to be inlined and really we just want to expose data not
> APIs to drivers.

OK. I'll think of a rework. And might need another justification
for a DEFAULT type of vIOMMU object to fit in.

> > > No need for this lock, xa_load is rcu safe against concurrent writer
> > 
> > I see iommufd's device.c and main.c grab xa_lock before xa_load?
> 
> That is not to protect the xa_load, it is to protect the lifetime of
> pointer it returns

I see. I'd drop it.

Thanks
Nicolin
diff mbox series

Patch

diff --git a/drivers/iommu/iommufd/Makefile b/drivers/iommu/iommufd/Makefile
index df490e836b30..288ef3e895e3 100644
--- a/drivers/iommu/iommufd/Makefile
+++ b/drivers/iommu/iommufd/Makefile
@@ -13,4 +13,4 @@  iommufd-y := \
 iommufd-$(CONFIG_IOMMUFD_TEST) += selftest.o
 
 obj-$(CONFIG_IOMMUFD) += iommufd.o
-obj-$(CONFIG_IOMMUFD_DRIVER) += iova_bitmap.o
+obj-$(CONFIG_IOMMUFD_DRIVER) += iova_bitmap.o viommu_api.o
diff --git a/drivers/iommu/iommufd/viommu_api.c b/drivers/iommu/iommufd/viommu_api.c
new file mode 100644
index 000000000000..e0ee592ce834
--- /dev/null
+++ b/drivers/iommu/iommufd/viommu_api.c
@@ -0,0 +1,39 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright (c) 2024, NVIDIA CORPORATION & AFFILIATES
+ */
+
+#include "iommufd_private.h"
+
+void iommufd_viommu_lock_vdev_id(struct iommufd_viommu *viommu)
+{
+	down_read(&viommu->vdev_ids_rwsem);
+}
+EXPORT_SYMBOL_NS_GPL(iommufd_viommu_lock_vdev_id, IOMMUFD);
+
+void iommufd_viommu_unlock_vdev_id(struct iommufd_viommu *viommu)
+{
+	up_read(&viommu->vdev_ids_rwsem);
+}
+EXPORT_SYMBOL_NS_GPL(iommufd_viommu_unlock_vdev_id, IOMMUFD);
+
+/*
+ * Find a device attached to an VIOMMU object using a virtual device ID that was
+ * set via an IOMMUFD_CMD_VIOMMU_SET_VDEV_ID. Callers of this function must call
+ * iommufd_viommu_lock_vdev_id() prior and iommufd_viommu_unlock_vdev_id() after
+ *
+ * Return device or NULL.
+ */
+struct device *iommufd_viommu_find_device(struct iommufd_viommu *viommu, u64 id)
+{
+	struct iommufd_vdev_id *vdev_id;
+
+	lockdep_assert_held(&viommu->vdev_ids_rwsem);
+
+	xa_lock(&viommu->vdev_ids);
+	vdev_id = xa_load(&viommu->vdev_ids, (unsigned long)id);
+	xa_unlock(&viommu->vdev_ids);
+	if (!vdev_id || vdev_id->id != id)
+		return NULL;
+	return vdev_id->idev->dev;
+}
+EXPORT_SYMBOL_NS_GPL(iommufd_viommu_find_device, IOMMUFD);
diff --git a/include/linux/iommufd.h b/include/linux/iommufd.h
index 85291b346348..364f151d281d 100644
--- a/include/linux/iommufd.h
+++ b/include/linux/iommufd.h
@@ -89,6 +89,9 @@  int iommufd_access_rw(struct iommufd_access *access, unsigned long iova,
 int iommufd_vfio_compat_ioas_get_id(struct iommufd_ctx *ictx, u32 *out_ioas_id);
 int iommufd_vfio_compat_ioas_create(struct iommufd_ctx *ictx);
 int iommufd_vfio_compat_set_no_iommu(struct iommufd_ctx *ictx);
+void iommufd_viommu_lock_vdev_id(struct iommufd_viommu *viommu);
+void iommufd_viommu_unlock_vdev_id(struct iommufd_viommu *viommu);
+struct device *iommufd_viommu_find_device(struct iommufd_viommu *viommu, u64 id);
 #else /* !CONFIG_IOMMUFD */
 static inline struct iommufd_ctx *iommufd_ctx_from_file(struct file *file)
 {
@@ -129,5 +132,18 @@  static inline int iommufd_vfio_compat_set_no_iommu(struct iommufd_ctx *ictx)
 {
 	return -EOPNOTSUPP;
 }
+
+void iommufd_viommu_lock_vdev_id(struct iommufd_viommu *viommu)
+{
+}
+
+void iommufd_viommu_unlock_vdev_id(struct iommufd_viommu *viommu)
+{
+}
+
+struct device *iommufd_viommu_find_device(struct iommufd_viommu *viommu, u64 id)
+{
+	return NULL;
+}
 #endif /* CONFIG_IOMMUFD */
 #endif