| Message ID | 6ccdfb27c7aa5a5bb7e153165cf90114cae4687c.1724776335.git.nicolinc@nvidia.com |
|---|---|
| State | New |
| Headers | show |
| Series | iommufd: Add VIOMMU infrastructure (Part-1) | expand |
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
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
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
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
On Wed, Sep 11, 2024 at 08:17:22PM -0700, Nicolin Chen wrote: > 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. I tried exposing the struct iommufd_viommu to drivers, and was able to drop a couple of helpers, except these two: struct device *vdev_to_dev(struct iommufd_vdevice *vdev) { return vdev ? vdev->idev->dev : NULL; } // Without it, we need to expose struct iommufd_device. struct iommu_domain * iommufd_viommu_to_parent_domain(struct iommufd_viommu *viommu) { if (!viommu || !viommu->hwpt) return NULL; return viommu->hwpt->common.domain; } // Without it, we need to expose struct iommufd_hwpt_page. Thanks Nicolin
On Fri, Oct 04, 2024 at 10:19:43PM -0700, Nicolin Chen wrote: > I tried exposing the struct iommufd_viommu to drivers, and was > able to drop a couple of helpers, except these two: > > struct device *vdev_to_dev(struct iommufd_vdevice *vdev) > { > return vdev ? vdev->idev->dev : NULL; > } // Without it, we need to expose struct iommufd_device. > > struct iommu_domain * > iommufd_viommu_to_parent_domain(struct iommufd_viommu *viommu) > { > if (!viommu || !viommu->hwpt) > return NULL; > return viommu->hwpt->common.domain; > } // Without it, we need to expose struct iommufd_hwpt_page. It seems OK, there isn't really locking entanglements or performance path on this stuff? Jason
On Mon, Oct 07, 2024 at 12:38:37PM -0300, Jason Gunthorpe wrote: > On Fri, Oct 04, 2024 at 10:19:43PM -0700, Nicolin Chen wrote: > > I tried exposing the struct iommufd_viommu to drivers, and was > > able to drop a couple of helpers, except these two: > > > > struct device *vdev_to_dev(struct iommufd_vdevice *vdev) > > { > > return vdev ? vdev->idev->dev : NULL; > > } // Without it, we need to expose struct iommufd_device. > > > > struct iommu_domain * > > iommufd_viommu_to_parent_domain(struct iommufd_viommu *viommu) > > { > > if (!viommu || !viommu->hwpt) > > return NULL; > > return viommu->hwpt->common.domain; > > } // Without it, we need to expose struct iommufd_hwpt_page. > > It seems OK, there isn't really locking entanglements or performance > path on this stuff? ----- The typical use case of the first one is like: dev = vdev_to_dev(xa_load(&viommu->vdevs, (unsigned long)vdev_id)); so I am asking for: /* Caller should lock via viommu->vdevs_rwsem with proper permission */ ----- And for the second one: /* * Convert a viommu to the encapsulated nesting parent domain. A caller must be * aware of the life cycle of the viommu pointer: only call this function in a * callback functions of viommu_alloc or a viommu op. */ ----- Thanks Nicolin
On Mon, Oct 07, 2024 at 09:36:18AM -0700, Nicolin Chen wrote: > On Mon, Oct 07, 2024 at 12:38:37PM -0300, Jason Gunthorpe wrote: > > On Fri, Oct 04, 2024 at 10:19:43PM -0700, Nicolin Chen wrote: > > > I tried exposing the struct iommufd_viommu to drivers, and was > > > able to drop a couple of helpers, except these two: > > > > > > struct device *vdev_to_dev(struct iommufd_vdevice *vdev) > > > { > > > return vdev ? vdev->idev->dev : NULL; > > > } // Without it, we need to expose struct iommufd_device. > > > > > > struct iommu_domain * > > > iommufd_viommu_to_parent_domain(struct iommufd_viommu *viommu) > > > { > > > if (!viommu || !viommu->hwpt) > > > return NULL; > > > return viommu->hwpt->common.domain; > > > } // Without it, we need to expose struct iommufd_hwpt_page. > > > > It seems OK, there isn't really locking entanglements or performance > > path on this stuff? > > ----- > The typical use case of the first one is like: > dev = vdev_to_dev(xa_load(&viommu->vdevs, (unsigned long)vdev_id)); > so I am asking for: > /* Caller should lock via viommu->vdevs_rwsem with proper permission */ Why would vdev_to_dev need that locking? The viommu cannot change hwpt during its lifecycle? Jason
On Mon, Oct 07, 2024 at 02:11:19PM -0300, Jason Gunthorpe wrote: > On Mon, Oct 07, 2024 at 09:36:18AM -0700, Nicolin Chen wrote: > > On Mon, Oct 07, 2024 at 12:38:37PM -0300, Jason Gunthorpe wrote: > > > On Fri, Oct 04, 2024 at 10:19:43PM -0700, Nicolin Chen wrote: > > > > I tried exposing the struct iommufd_viommu to drivers, and was > > > > able to drop a couple of helpers, except these two: > > > > > > > > struct device *vdev_to_dev(struct iommufd_vdevice *vdev) > > > > { > > > > return vdev ? vdev->idev->dev : NULL; > > > > } // Without it, we need to expose struct iommufd_device. > > > > > > > > struct iommu_domain * > > > > iommufd_viommu_to_parent_domain(struct iommufd_viommu *viommu) > > > > { > > > > if (!viommu || !viommu->hwpt) > > > > return NULL; > > > > return viommu->hwpt->common.domain; > > > > } // Without it, we need to expose struct iommufd_hwpt_page. > > > > > > It seems OK, there isn't really locking entanglements or performance > > > path on this stuff? > > > > ----- > > The typical use case of the first one is like: > > dev = vdev_to_dev(xa_load(&viommu->vdevs, (unsigned long)vdev_id)); > > so I am asking for: > > /* Caller should lock via viommu->vdevs_rwsem with proper permission */ > > Why would vdev_to_dev need that locking? The viommu cannot change hwpt > during its lifecycle? This is for vdev/dev v.s. hwpt. We need the lock for viommu's vdev xarray. Yet, giving a 2nd thought, I feel the lock would be useless if a driver tries to refer the returned vdev (with this helper) after the vdev object is destroyed.. We could only note something similar that caller must be aware of the life cycle of vdev itself.. Nicolin
On Mon, Oct 07, 2024 at 10:25:01AM -0700, Nicolin Chen wrote: > This is for vdev/dev v.s. hwpt. We need the lock for viommu's > vdev xarray. > > Yet, giving a 2nd thought, I feel the lock would be useless if > a driver tries to refer the returned vdev (with this helper) > after the vdev object is destroyed.. > > We could only note something similar that caller must be aware > of the life cycle of vdev itself.. Yes, I imagined you'd use the xa_lock for this an it solves both problems at once. Jason
On Mon, Oct 07, 2024 at 03:28:16PM -0300, Jason Gunthorpe wrote: > On Mon, Oct 07, 2024 at 10:25:01AM -0700, Nicolin Chen wrote: > > > This is for vdev/dev v.s. hwpt. We need the lock for viommu's > > vdev xarray. > > > > Yet, giving a 2nd thought, I feel the lock would be useless if > > a driver tries to refer the returned vdev (with this helper) > > after the vdev object is destroyed.. > > > > We could only note something similar that caller must be aware > > of the life cycle of vdev itself.. > > Yes, I imagined you'd use the xa_lock for this an it solves both > problems at once. Ah! We don't even need a rwsem then.. Thanks! Nicolin
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
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