* [PATCH v9 1/6] vfio: Do not allow !ops->dma_unmap in vfio_pin/unpin_pages()
2023-07-27 7:23 [PATCH v9 0/6] cover-letter: Add IO page table replacement support Nicolin Chen
@ 2023-07-27 7:23 ` Nicolin Chen
2023-07-27 7:23 ` [PATCH v9 2/6] iommufd: Allow passing in iopt_access_list_id to iopt_remove_access() Nicolin Chen
` (4 subsequent siblings)
5 siblings, 0 replies; 11+ messages in thread
From: Nicolin Chen @ 2023-07-27 7:23 UTC (permalink / raw)
To: jgg, kevin.tian
Cc: yi.l.liu, joro, will, robin.murphy, alex.williamson, shuah,
linux-kernel, iommu, kvm, linux-kselftest, mjrosato, farman
A driver that doesn't implement ops->dma_unmap shouldn't be allowed to do
vfio_pin/unpin_pages(), though it can use vfio_dma_rw() to access an iova
range. Deny !ops->dma_unmap cases in vfio_pin/unpin_pages().
Suggested-by: Kevin Tian <kevin.tian@intel.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Reviewed-by: Yi Liu <yi.l.liu@intel.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Reviewed-by: Alex Williamson <alex.williamson@redhat.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
drivers/vfio/vfio_main.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
index 902f06e52c48..0da8ed81a97d 100644
--- a/drivers/vfio/vfio_main.c
+++ b/drivers/vfio/vfio_main.c
@@ -1483,6 +1483,8 @@ int vfio_pin_pages(struct vfio_device *device, dma_addr_t iova,
/* group->container cannot change while a vfio device is open */
if (!pages || !npage || WARN_ON(!vfio_assert_device_open(device)))
return -EINVAL;
+ if (!device->ops->dma_unmap)
+ return -EINVAL;
if (vfio_device_has_container(device))
return vfio_device_container_pin_pages(device, iova,
npage, prot, pages);
@@ -1520,6 +1522,8 @@ void vfio_unpin_pages(struct vfio_device *device, dma_addr_t iova, int npage)
{
if (WARN_ON(!vfio_assert_device_open(device)))
return;
+ if (WARN_ON(!device->ops->dma_unmap))
+ return;
if (vfio_device_has_container(device)) {
vfio_device_container_unpin_pages(device, iova, npage);
--
2.41.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH v9 2/6] iommufd: Allow passing in iopt_access_list_id to iopt_remove_access()
2023-07-27 7:23 [PATCH v9 0/6] cover-letter: Add IO page table replacement support Nicolin Chen
2023-07-27 7:23 ` [PATCH v9 1/6] vfio: Do not allow !ops->dma_unmap in vfio_pin/unpin_pages() Nicolin Chen
@ 2023-07-27 7:23 ` Nicolin Chen
2023-07-27 9:07 ` Nicolin Chen
2023-07-27 14:34 ` Jason Gunthorpe
2023-07-27 7:23 ` [PATCH v9 3/6] iommufd: Add iommufd_access_change_ioas helper Nicolin Chen
` (3 subsequent siblings)
5 siblings, 2 replies; 11+ messages in thread
From: Nicolin Chen @ 2023-07-27 7:23 UTC (permalink / raw)
To: jgg, kevin.tian
Cc: yi.l.liu, joro, will, robin.murphy, alex.williamson, shuah,
linux-kernel, iommu, kvm, linux-kselftest, mjrosato, farman
This is a preparatory change for ioas replacement support for access.
The replacement routine does an iopt_add_access() at a new IOAS first
and then iopt_remove_access() at the old IOAS upon the success of the
first call. However, the first call overrides the iopt_access_list_id
in the access struct, resulting in that id un-erased in the xarray.
Add an iopt_access_list_id in iopt_remove_access, so the replacement
routine can save the id before it gets overwritten, and pass the id
in iopt_remove_access() for a proper cleanup.
The existing callers should just pass in access->iopt_access_list_id.
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
drivers/iommu/iommufd/device.c | 6 ++++--
drivers/iommu/iommufd/io_pagetable.c | 6 +++---
drivers/iommu/iommufd/iommufd_private.h | 3 ++-
3 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index 57c0e81f5073..7a3e8660b902 100644
--- a/drivers/iommu/iommufd/device.c
+++ b/drivers/iommu/iommufd/device.c
@@ -690,7 +690,8 @@ void iommufd_access_destroy_object(struct iommufd_object *obj)
container_of(obj, struct iommufd_access, obj);
if (access->ioas) {
- iopt_remove_access(&access->ioas->iopt, access);
+ iopt_remove_access(&access->ioas->iopt, access,
+ access->iopt_access_list_id);
refcount_dec(&access->ioas->obj.users);
access->ioas = NULL;
}
@@ -776,7 +777,8 @@ void iommufd_access_detach(struct iommufd_access *access)
access->ops->unmap(access->data, 0, ULONG_MAX);
mutex_lock(&access->ioas_lock);
}
- iopt_remove_access(&cur_ioas->iopt, access);
+ iopt_remove_access(&cur_ioas->iopt, access,
+ access->iopt_access_list_id);
refcount_dec(&cur_ioas->obj.users);
out:
access->ioas_unpin = NULL;
diff --git a/drivers/iommu/iommufd/io_pagetable.c b/drivers/iommu/iommufd/io_pagetable.c
index 4d095115c2d0..3a598182b761 100644
--- a/drivers/iommu/iommufd/io_pagetable.c
+++ b/drivers/iommu/iommufd/io_pagetable.c
@@ -1158,12 +1158,12 @@ int iopt_add_access(struct io_pagetable *iopt, struct iommufd_access *access)
}
void iopt_remove_access(struct io_pagetable *iopt,
- struct iommufd_access *access)
+ struct iommufd_access *access,
+ u32 iopt_access_list_id)
{
down_write(&iopt->domains_rwsem);
down_write(&iopt->iova_rwsem);
- WARN_ON(xa_erase(&iopt->access_list, access->iopt_access_list_id) !=
- access);
+ WARN_ON(xa_erase(&iopt->access_list, iopt_access_list_id) != access);
WARN_ON(iopt_calculate_iova_alignment(iopt));
up_write(&iopt->iova_rwsem);
up_write(&iopt->domains_rwsem);
diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
index dba730129b8c..8ba786bc95ff 100644
--- a/drivers/iommu/iommufd/iommufd_private.h
+++ b/drivers/iommu/iommufd/iommufd_private.h
@@ -323,7 +323,8 @@ struct iommufd_access {
int iopt_add_access(struct io_pagetable *iopt, struct iommufd_access *access);
void iopt_remove_access(struct io_pagetable *iopt,
- struct iommufd_access *access);
+ struct iommufd_access *access,
+ u32 iopt_access_list_id);
void iommufd_access_destroy_object(struct iommufd_object *obj);
#ifdef CONFIG_IOMMUFD_TEST
--
2.41.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v9 2/6] iommufd: Allow passing in iopt_access_list_id to iopt_remove_access()
2023-07-27 7:23 ` [PATCH v9 2/6] iommufd: Allow passing in iopt_access_list_id to iopt_remove_access() Nicolin Chen
@ 2023-07-27 9:07 ` Nicolin Chen
2023-07-27 14:34 ` Jason Gunthorpe
1 sibling, 0 replies; 11+ messages in thread
From: Nicolin Chen @ 2023-07-27 9:07 UTC (permalink / raw)
To: jgg, kevin.tian
Cc: yi.l.liu, joro, will, robin.murphy, alex.williamson, shuah,
linux-kernel, iommu, kvm, linux-kselftest, mjrosato, farman
On Thu, Jul 27, 2023 at 12:23:07AM -0700, Nicolin Chen wrote:
> This is a preparatory change for ioas replacement support for access.
> The replacement routine does an iopt_add_access() at a new IOAS first
> and then iopt_remove_access() at the old IOAS upon the success of the
> first call. However, the first call overrides the iopt_access_list_id
> in the access struct, resulting in that id un-erased in the xarray.
>
> Add an iopt_access_list_id in iopt_remove_access, so the replacement
> routine can save the id before it gets overwritten, and pass the id
> in iopt_remove_access() for a proper cleanup.
>
> The existing callers should just pass in access->iopt_access_list_id.
>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
Forgot to add:
Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v9 2/6] iommufd: Allow passing in iopt_access_list_id to iopt_remove_access()
2023-07-27 7:23 ` [PATCH v9 2/6] iommufd: Allow passing in iopt_access_list_id to iopt_remove_access() Nicolin Chen
2023-07-27 9:07 ` Nicolin Chen
@ 2023-07-27 14:34 ` Jason Gunthorpe
1 sibling, 0 replies; 11+ messages in thread
From: Jason Gunthorpe @ 2023-07-27 14:34 UTC (permalink / raw)
To: Nicolin Chen
Cc: kevin.tian, yi.l.liu, joro, will, robin.murphy, alex.williamson,
shuah, linux-kernel, iommu, kvm, linux-kselftest, mjrosato,
farman
On Thu, Jul 27, 2023 at 12:23:07AM -0700, Nicolin Chen wrote:
> This is a preparatory change for ioas replacement support for access.
> The replacement routine does an iopt_add_access() at a new IOAS first
> and then iopt_remove_access() at the old IOAS upon the success of the
> first call. However, the first call overrides the iopt_access_list_id
> in the access struct, resulting in that id un-erased in the xarray.
>
> Add an iopt_access_list_id in iopt_remove_access, so the replacement
> routine can save the id before it gets overwritten, and pass the id
> in iopt_remove_access() for a proper cleanup.
>
> The existing callers should just pass in access->iopt_access_list_id.
>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> ---
> drivers/iommu/iommufd/device.c | 6 ++++--
> drivers/iommu/iommufd/io_pagetable.c | 6 +++---
> drivers/iommu/iommufd/iommufd_private.h | 3 ++-
> 3 files changed, 9 insertions(+), 6 deletions(-)
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Jason
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v9 3/6] iommufd: Add iommufd_access_change_ioas helper
2023-07-27 7:23 [PATCH v9 0/6] cover-letter: Add IO page table replacement support Nicolin Chen
2023-07-27 7:23 ` [PATCH v9 1/6] vfio: Do not allow !ops->dma_unmap in vfio_pin/unpin_pages() Nicolin Chen
2023-07-27 7:23 ` [PATCH v9 2/6] iommufd: Allow passing in iopt_access_list_id to iopt_remove_access() Nicolin Chen
@ 2023-07-27 7:23 ` Nicolin Chen
2023-07-27 14:40 ` Jason Gunthorpe
2023-07-27 7:23 ` [PATCH v9 4/6] iommufd: Add iommufd_access_replace() API Nicolin Chen
` (2 subsequent siblings)
5 siblings, 1 reply; 11+ messages in thread
From: Nicolin Chen @ 2023-07-27 7:23 UTC (permalink / raw)
To: jgg, kevin.tian
Cc: yi.l.liu, joro, will, robin.murphy, alex.williamson, shuah,
linux-kernel, iommu, kvm, linux-kselftest, mjrosato, farman
The complication of the mutex and refcount will be amplified after we
introduce the replace support for access. So, add a preparatory change
of a constitutive helper iommufd_access_change_ioas(), to take care of
the existing iommufd_access_attach() and iommufd_access_detach().
Also, update the unprotect routine in iommufd_access_destroy_object()
to calling the new iommufd_access_change_ioas() helper.
Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
drivers/iommu/iommufd/device.c | 112 ++++++++++++++++++++-------------
1 file changed, 69 insertions(+), 43 deletions(-)
diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index 7a3e8660b902..d9680a247e1c 100644
--- a/drivers/iommu/iommufd/device.c
+++ b/drivers/iommu/iommufd/device.c
@@ -684,17 +684,69 @@ void iommufd_device_detach(struct iommufd_device *idev)
}
EXPORT_SYMBOL_NS_GPL(iommufd_device_detach, IOMMUFD);
+static int iommufd_access_change_ioas(struct iommufd_access *access,
+ struct iommufd_ioas *new_ioas)
+{
+ u32 iopt_access_list_id = access->iopt_access_list_id;
+ struct iommufd_ioas *cur_ioas = access->ioas;
+ int rc;
+
+ lockdep_assert_held(&access->ioas_lock);
+
+ /* We are racing with a concurrent detach, bail */
+ if (cur_ioas != access->ioas_unpin)
+ return -EBUSY;
+
+ if (IS_ERR(new_ioas))
+ return PTR_ERR(new_ioas);
+
+ if (cur_ioas == new_ioas) {
+ /* Do not forget to put since we allow a duplication */
+ iommufd_put_object(&new_ioas->obj);
+ return 0;
+ }
+
+ /*
+ * Set ioas to NULL to block any further iommufd_access_pin_pages().
+ * iommufd_access_unpin_pages() can continue using access->ioas_unpin.
+ */
+ access->ioas = NULL;
+
+ if (new_ioas) {
+ rc = iopt_add_access(&new_ioas->iopt, access);
+ if (rc) {
+ iommufd_put_object(&new_ioas->obj);
+ access->ioas = cur_ioas;
+ return rc;
+ }
+ iommufd_ref_to_users(&new_ioas->obj);
+ }
+
+ if (cur_ioas) {
+ if (access->ops->unmap) {
+ mutex_unlock(&access->ioas_lock);
+ access->ops->unmap(access->data, 0, ULONG_MAX);
+ mutex_lock(&access->ioas_lock);
+ }
+ iopt_remove_access(&cur_ioas->iopt, access, iopt_access_list_id);
+ refcount_dec(&cur_ioas->obj.users);
+ }
+
+ access->ioas = new_ioas;
+ access->ioas_unpin = new_ioas;
+
+ return 0;
+}
+
void iommufd_access_destroy_object(struct iommufd_object *obj)
{
struct iommufd_access *access =
container_of(obj, struct iommufd_access, obj);
- if (access->ioas) {
- iopt_remove_access(&access->ioas->iopt, access,
- access->iopt_access_list_id);
- refcount_dec(&access->ioas->obj.users);
- access->ioas = NULL;
- }
+ mutex_lock(&access->ioas_lock);
+ if (access->ioas)
+ WARN_ON(iommufd_access_change_ioas(access, NULL));
+ mutex_unlock(&access->ioas_lock);
iommufd_ctx_put(access->ictx);
}
@@ -761,60 +813,34 @@ EXPORT_SYMBOL_NS_GPL(iommufd_access_destroy, IOMMUFD);
void iommufd_access_detach(struct iommufd_access *access)
{
- struct iommufd_ioas *cur_ioas = access->ioas;
+ int rc;
mutex_lock(&access->ioas_lock);
- if (WARN_ON(!access->ioas))
- goto out;
- /*
- * Set ioas to NULL to block any further iommufd_access_pin_pages().
- * iommufd_access_unpin_pages() can continue using access->ioas_unpin.
- */
- access->ioas = NULL;
-
- if (access->ops->unmap) {
+ if (WARN_ON(!access->ioas)) {
mutex_unlock(&access->ioas_lock);
- access->ops->unmap(access->data, 0, ULONG_MAX);
- mutex_lock(&access->ioas_lock);
+ return;
}
- iopt_remove_access(&cur_ioas->iopt, access,
- access->iopt_access_list_id);
- refcount_dec(&cur_ioas->obj.users);
-out:
- access->ioas_unpin = NULL;
+ rc = iommufd_access_change_ioas(access, NULL);
+ WARN_ON(rc);
mutex_unlock(&access->ioas_lock);
}
EXPORT_SYMBOL_NS_GPL(iommufd_access_detach, IOMMUFD);
int iommufd_access_attach(struct iommufd_access *access, u32 ioas_id)
{
- struct iommufd_ioas *new_ioas;
- int rc = 0;
+ struct iommufd_ctx *ictx = access->ictx;
+ int rc;
mutex_lock(&access->ioas_lock);
- if (WARN_ON(access->ioas || access->ioas_unpin)) {
+ if (WARN_ON(access->ioas)) {
mutex_unlock(&access->ioas_lock);
return -EINVAL;
}
- new_ioas = iommufd_get_ioas(access->ictx, ioas_id);
- if (IS_ERR(new_ioas)) {
- mutex_unlock(&access->ioas_lock);
- return PTR_ERR(new_ioas);
- }
-
- rc = iopt_add_access(&new_ioas->iopt, access);
- if (rc) {
- mutex_unlock(&access->ioas_lock);
- iommufd_put_object(&new_ioas->obj);
- return rc;
- }
- iommufd_ref_to_users(&new_ioas->obj);
-
- access->ioas = new_ioas;
- access->ioas_unpin = new_ioas;
+ rc = iommufd_access_change_ioas(access,
+ iommufd_get_ioas(ictx, ioas_id));
mutex_unlock(&access->ioas_lock);
- return 0;
+ return rc;
}
EXPORT_SYMBOL_NS_GPL(iommufd_access_attach, IOMMUFD);
--
2.41.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v9 3/6] iommufd: Add iommufd_access_change_ioas helper
2023-07-27 7:23 ` [PATCH v9 3/6] iommufd: Add iommufd_access_change_ioas helper Nicolin Chen
@ 2023-07-27 14:40 ` Jason Gunthorpe
2023-07-27 18:47 ` Nicolin Chen
0 siblings, 1 reply; 11+ messages in thread
From: Jason Gunthorpe @ 2023-07-27 14:40 UTC (permalink / raw)
To: Nicolin Chen
Cc: kevin.tian, yi.l.liu, joro, will, robin.murphy, alex.williamson,
shuah, linux-kernel, iommu, kvm, linux-kselftest, mjrosato,
farman
On Thu, Jul 27, 2023 at 12:23:08AM -0700, Nicolin Chen wrote:
> The complication of the mutex and refcount will be amplified after we
> introduce the replace support for access. So, add a preparatory change
> of a constitutive helper iommufd_access_change_ioas(), to take care of
> the existing iommufd_access_attach() and iommufd_access_detach().
>
> Also, update the unprotect routine in iommufd_access_destroy_object()
> to calling the new iommufd_access_change_ioas() helper.
>
> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> ---
> drivers/iommu/iommufd/device.c | 112 ++++++++++++++++++++-------------
> 1 file changed, 69 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
> index 7a3e8660b902..d9680a247e1c 100644
> --- a/drivers/iommu/iommufd/device.c
> +++ b/drivers/iommu/iommufd/device.c
> @@ -684,17 +684,69 @@ void iommufd_device_detach(struct iommufd_device *idev)
> }
> EXPORT_SYMBOL_NS_GPL(iommufd_device_detach, IOMMUFD);
>
> +static int iommufd_access_change_ioas(struct iommufd_access *access,
> + struct iommufd_ioas *new_ioas)
> +{
> + u32 iopt_access_list_id = access->iopt_access_list_id;
> + struct iommufd_ioas *cur_ioas = access->ioas;
> + int rc;
> +
> + lockdep_assert_held(&access->ioas_lock);
> +
> + /* We are racing with a concurrent detach, bail */
> + if (cur_ioas != access->ioas_unpin)
> + return -EBUSY;
> +
> + if (IS_ERR(new_ioas))
> + return PTR_ERR(new_ioas);
> +
> + if (cur_ioas == new_ioas) {
> + /* Do not forget to put since we allow a duplication */
> + iommufd_put_object(&new_ioas->obj);
> + return 0;
> + }
> +
> + /*
> + * Set ioas to NULL to block any further iommufd_access_pin_pages().
> + * iommufd_access_unpin_pages() can continue using access->ioas_unpin.
> + */
> + access->ioas = NULL;
> +
> + if (new_ioas) {
> + rc = iopt_add_access(&new_ioas->iopt, access);
> + if (rc) {
> + iommufd_put_object(&new_ioas->obj);
> + access->ioas = cur_ioas;
> + return rc;
> + }
> + iommufd_ref_to_users(&new_ioas->obj);
Kevin's suggestion to just open code the refcount_inc here
And have a wrapper func that does:
iommufd_access_change_ioas_id(struct iommufd_access *access, u32 id)
{
struct iommufd_ioas *ioas = iommufd_get_ioas(ictx, ioas_id);
int rc;
if (IS_ERR(ioas))
return PTR_ERR(ioas);
rc = iommufd_access_change_ioas(access, ioas);
iommufd_put_object(&ioas->obj);
return rc;
}
Does looks cleaner
Then we delete iommufd_ref_to_users() as there are no users (once all
the branches are merged).
Logic looks OK otherwise
Jason
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v9 3/6] iommufd: Add iommufd_access_change_ioas helper
2023-07-27 14:40 ` Jason Gunthorpe
@ 2023-07-27 18:47 ` Nicolin Chen
0 siblings, 0 replies; 11+ messages in thread
From: Nicolin Chen @ 2023-07-27 18:47 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: kevin.tian, yi.l.liu, joro, will, robin.murphy, alex.williamson,
shuah, linux-kernel, iommu, kvm, linux-kselftest, mjrosato,
farman
On Thu, Jul 27, 2023 at 11:40:17AM -0300, Jason Gunthorpe wrote:
> On Thu, Jul 27, 2023 at 12:23:08AM -0700, Nicolin Chen wrote:
> > + if (new_ioas) {
> > + rc = iopt_add_access(&new_ioas->iopt, access);
> > + if (rc) {
> > + iommufd_put_object(&new_ioas->obj);
> > + access->ioas = cur_ioas;
> > + return rc;
> > + }
> > + iommufd_ref_to_users(&new_ioas->obj);
>
> Kevin's suggestion to just open code the refcount_inc here
Will replace this iommufd_ref_to_users with a refcount_inc in v10.
> And have a wrapper func that does:
>
> iommufd_access_change_ioas_id(struct iommufd_access *access, u32 id)
> {
> struct iommufd_ioas *ioas = iommufd_get_ioas(ictx, ioas_id);
> int rc;
>
> if (IS_ERR(ioas))
> return PTR_ERR(ioas);
> rc = iommufd_access_change_ioas(access, ioas);
> iommufd_put_object(&ioas->obj);
> return rc;
> }
>
> Does looks cleaner
I see. So we can drop iommufd_put_object(&new_ioas->obj) in
iommufd_access_change_ioas().
> Then we delete iommufd_ref_to_users() as there are no users (once all
> the branches are merged).
Ack.
Nicolin
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v9 4/6] iommufd: Add iommufd_access_replace() API
2023-07-27 7:23 [PATCH v9 0/6] cover-letter: Add IO page table replacement support Nicolin Chen
` (2 preceding siblings ...)
2023-07-27 7:23 ` [PATCH v9 3/6] iommufd: Add iommufd_access_change_ioas helper Nicolin Chen
@ 2023-07-27 7:23 ` Nicolin Chen
2023-07-27 7:23 ` [PATCH v9 5/6] iommufd/selftest: Add IOMMU_TEST_OP_ACCESS_REPLACE_IOAS coverage Nicolin Chen
2023-07-27 7:23 ` [PATCH v9 6/6] vfio: Support IO page table replacement Nicolin Chen
5 siblings, 0 replies; 11+ messages in thread
From: Nicolin Chen @ 2023-07-27 7:23 UTC (permalink / raw)
To: jgg, kevin.tian
Cc: yi.l.liu, joro, will, robin.murphy, alex.williamson, shuah,
linux-kernel, iommu, kvm, linux-kselftest, mjrosato, farman
Taking advantage of the new iommufd_access_change_ioas helper, add a new
iommufd_access_replace() for VFIO emulated pathway to use.
Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
drivers/iommu/iommufd/device.c | 17 +++++++++++++++++
include/linux/iommufd.h | 1 +
2 files changed, 18 insertions(+)
diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index d9680a247e1c..2dfeb65c7090 100644
--- a/drivers/iommu/iommufd/device.c
+++ b/drivers/iommu/iommufd/device.c
@@ -844,6 +844,23 @@ int iommufd_access_attach(struct iommufd_access *access, u32 ioas_id)
}
EXPORT_SYMBOL_NS_GPL(iommufd_access_attach, IOMMUFD);
+int iommufd_access_replace(struct iommufd_access *access, u32 ioas_id)
+{
+ struct iommufd_ctx *ictx = access->ictx;
+ int rc;
+
+ mutex_lock(&access->ioas_lock);
+ if (!access->ioas) {
+ mutex_unlock(&access->ioas_lock);
+ return -ENOENT;
+ }
+ rc = iommufd_access_change_ioas(access,
+ iommufd_get_ioas(ictx, ioas_id));
+ mutex_unlock(&access->ioas_lock);
+ return rc;
+}
+EXPORT_SYMBOL_NS_GPL(iommufd_access_replace, IOMMUFD);
+
/**
* iommufd_access_notify_unmap - Notify users of an iopt to stop using it
* @iopt: iopt to work on
diff --git a/include/linux/iommufd.h b/include/linux/iommufd.h
index 0ac60256b659..ffc3a949f837 100644
--- a/include/linux/iommufd.h
+++ b/include/linux/iommufd.h
@@ -49,6 +49,7 @@ iommufd_access_create(struct iommufd_ctx *ictx,
const struct iommufd_access_ops *ops, void *data, u32 *id);
void iommufd_access_destroy(struct iommufd_access *access);
int iommufd_access_attach(struct iommufd_access *access, u32 ioas_id);
+int iommufd_access_replace(struct iommufd_access *access, u32 ioas_id);
void iommufd_access_detach(struct iommufd_access *access);
void iommufd_ctx_get(struct iommufd_ctx *ictx);
--
2.41.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH v9 5/6] iommufd/selftest: Add IOMMU_TEST_OP_ACCESS_REPLACE_IOAS coverage
2023-07-27 7:23 [PATCH v9 0/6] cover-letter: Add IO page table replacement support Nicolin Chen
` (3 preceding siblings ...)
2023-07-27 7:23 ` [PATCH v9 4/6] iommufd: Add iommufd_access_replace() API Nicolin Chen
@ 2023-07-27 7:23 ` Nicolin Chen
2023-07-27 7:23 ` [PATCH v9 6/6] vfio: Support IO page table replacement Nicolin Chen
5 siblings, 0 replies; 11+ messages in thread
From: Nicolin Chen @ 2023-07-27 7:23 UTC (permalink / raw)
To: jgg, kevin.tian
Cc: yi.l.liu, joro, will, robin.murphy, alex.williamson, shuah,
linux-kernel, iommu, kvm, linux-kselftest, mjrosato, farman
Add a new IOMMU_TEST_OP_ACCESS_REPLACE_IOAS to allow replacing the
access->ioas, corresponding to the iommufd_access_replace() helper.
Then add a replace coverage as a part of user_copy test case, which
basically repeats the copy test after replacing the old ioas with a
new one.
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
drivers/iommu/iommufd/iommufd_test.h | 4 +++
drivers/iommu/iommufd/selftest.c | 19 ++++++++++++
tools/testing/selftests/iommu/iommufd.c | 29 +++++++++++++++++--
tools/testing/selftests/iommu/iommufd_utils.h | 19 ++++++++++++
4 files changed, 69 insertions(+), 2 deletions(-)
diff --git a/drivers/iommu/iommufd/iommufd_test.h b/drivers/iommu/iommufd/iommufd_test.h
index dd9168a20ddf..258de2253b61 100644
--- a/drivers/iommu/iommufd/iommufd_test.h
+++ b/drivers/iommu/iommufd/iommufd_test.h
@@ -18,6 +18,7 @@ enum {
IOMMU_TEST_OP_ACCESS_RW,
IOMMU_TEST_OP_SET_TEMP_MEMORY_LIMIT,
IOMMU_TEST_OP_MOCK_DOMAIN_REPLACE,
+ IOMMU_TEST_OP_ACCESS_REPLACE_IOAS,
};
enum {
@@ -91,6 +92,9 @@ struct iommu_test_cmd {
struct {
__u32 limit;
} memory_limit;
+ struct {
+ __u32 ioas_id;
+ } access_replace_ioas;
};
__u32 last;
};
diff --git a/drivers/iommu/iommufd/selftest.c b/drivers/iommu/iommufd/selftest.c
index 9d43334e4faf..bb2cd54ca7b6 100644
--- a/drivers/iommu/iommufd/selftest.c
+++ b/drivers/iommu/iommufd/selftest.c
@@ -785,6 +785,22 @@ static int iommufd_test_create_access(struct iommufd_ucmd *ucmd,
return rc;
}
+static int iommufd_test_access_replace_ioas(struct iommufd_ucmd *ucmd,
+ unsigned int access_id,
+ unsigned int ioas_id)
+{
+ struct selftest_access *staccess;
+ int rc;
+
+ staccess = iommufd_access_get(access_id);
+ if (IS_ERR(staccess))
+ return PTR_ERR(staccess);
+
+ rc = iommufd_access_replace(staccess->access, ioas_id);
+ fput(staccess->file);
+ return rc;
+}
+
/* Check that the pages in a page array match the pages in the user VA */
static int iommufd_test_check_pages(void __user *uptr, struct page **pages,
size_t npages)
@@ -1000,6 +1016,9 @@ int iommufd_test(struct iommufd_ucmd *ucmd)
case IOMMU_TEST_OP_CREATE_ACCESS:
return iommufd_test_create_access(ucmd, cmd->id,
cmd->create_access.flags);
+ case IOMMU_TEST_OP_ACCESS_REPLACE_IOAS:
+ return iommufd_test_access_replace_ioas(
+ ucmd, cmd->id, cmd->access_replace_ioas.ioas_id);
case IOMMU_TEST_OP_ACCESS_PAGES:
return iommufd_test_access_pages(
ucmd, cmd->id, cmd->access_pages.iova,
diff --git a/tools/testing/selftests/iommu/iommufd.c b/tools/testing/selftests/iommu/iommufd.c
index dc09c1de319f..8acd0af37aa5 100644
--- a/tools/testing/selftests/iommu/iommufd.c
+++ b/tools/testing/selftests/iommu/iommufd.c
@@ -1283,7 +1283,13 @@ TEST_F(iommufd_mock_domain, user_copy)
.dst_iova = MOCK_APERTURE_START,
.length = BUFFER_SIZE,
};
- unsigned int ioas_id;
+ struct iommu_ioas_unmap unmap_cmd = {
+ .size = sizeof(unmap_cmd),
+ .ioas_id = self->ioas_id,
+ .iova = MOCK_APERTURE_START,
+ .length = BUFFER_SIZE,
+ };
+ unsigned int new_ioas_id, ioas_id;
/* Pin the pages in an IOAS with no domains then copy to an IOAS with domains */
test_ioctl_ioas_alloc(&ioas_id);
@@ -1301,11 +1307,30 @@ TEST_F(iommufd_mock_domain, user_copy)
ASSERT_EQ(0, ioctl(self->fd, IOMMU_IOAS_COPY, ©_cmd));
check_mock_iova(buffer, MOCK_APERTURE_START, BUFFER_SIZE);
+ /* Now replace the ioas with a new one */
+ test_ioctl_ioas_alloc(&new_ioas_id);
+ test_ioctl_ioas_map_id(new_ioas_id, buffer, BUFFER_SIZE,
+ ©_cmd.src_iova);
+ test_cmd_access_replace_ioas(access_cmd.id, new_ioas_id);
+
+ /* Destroy the old ioas and cleanup copied mapping */
+ ASSERT_EQ(0, ioctl(self->fd, IOMMU_IOAS_UNMAP, &unmap_cmd));
+ test_ioctl_destroy(ioas_id);
+
+ /* Then run the same test again with the new ioas */
+ access_cmd.access_pages.iova = copy_cmd.src_iova;
+ ASSERT_EQ(0,
+ ioctl(self->fd, _IOMMU_TEST_CMD(IOMMU_TEST_OP_ACCESS_PAGES),
+ &access_cmd));
+ copy_cmd.src_ioas_id = new_ioas_id;
+ ASSERT_EQ(0, ioctl(self->fd, IOMMU_IOAS_COPY, ©_cmd));
+ check_mock_iova(buffer, MOCK_APERTURE_START, BUFFER_SIZE);
+
test_cmd_destroy_access_pages(
access_cmd.id, access_cmd.access_pages.out_access_pages_id);
test_cmd_destroy_access(access_cmd.id);
- test_ioctl_destroy(ioas_id);
+ test_ioctl_destroy(new_ioas_id);
}
TEST_F(iommufd_mock_domain, replace)
diff --git a/tools/testing/selftests/iommu/iommufd_utils.h b/tools/testing/selftests/iommu/iommufd_utils.h
index 53b4d3f2d9fc..70353e68e599 100644
--- a/tools/testing/selftests/iommu/iommufd_utils.h
+++ b/tools/testing/selftests/iommu/iommufd_utils.h
@@ -119,6 +119,25 @@ static int _test_cmd_hwpt_alloc(int fd, __u32 device_id, __u32 pt_id,
#define test_cmd_hwpt_alloc(device_id, pt_id, hwpt_id) \
ASSERT_EQ(0, _test_cmd_hwpt_alloc(self->fd, device_id, pt_id, hwpt_id))
+static int _test_cmd_access_replace_ioas(int fd, __u32 access_id,
+ unsigned int ioas_id)
+{
+ struct iommu_test_cmd cmd = {
+ .size = sizeof(cmd),
+ .op = IOMMU_TEST_OP_ACCESS_REPLACE_IOAS,
+ .id = access_id,
+ .access_replace_ioas = { .ioas_id = ioas_id },
+ };
+ int ret;
+
+ ret = ioctl(fd, IOMMU_TEST_CMD, &cmd);
+ if (ret)
+ return ret;
+ return 0;
+}
+#define test_cmd_access_replace_ioas(access_id, ioas_id) \
+ ASSERT_EQ(0, _test_cmd_access_replace_ioas(self->fd, access_id, ioas_id))
+
static int _test_cmd_create_access(int fd, unsigned int ioas_id,
__u32 *access_id, unsigned int flags)
{
--
2.41.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH v9 6/6] vfio: Support IO page table replacement
2023-07-27 7:23 [PATCH v9 0/6] cover-letter: Add IO page table replacement support Nicolin Chen
` (4 preceding siblings ...)
2023-07-27 7:23 ` [PATCH v9 5/6] iommufd/selftest: Add IOMMU_TEST_OP_ACCESS_REPLACE_IOAS coverage Nicolin Chen
@ 2023-07-27 7:23 ` Nicolin Chen
5 siblings, 0 replies; 11+ messages in thread
From: Nicolin Chen @ 2023-07-27 7:23 UTC (permalink / raw)
To: jgg, kevin.tian
Cc: yi.l.liu, joro, will, robin.murphy, alex.williamson, shuah,
linux-kernel, iommu, kvm, linux-kselftest, mjrosato, farman
Now both the physical path and the emulated path should support an IO page
table replacement. Call iommufd_device_replace/iommufd_access_replace(),
when vdev->iommufd_attached is true.
Also update the VFIO_DEVICE_ATTACH_IOMMUFD_PT kdoc in the uAPI header.
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Reviewed-by: Alex Williamson <alex.williamson@redhat.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
drivers/vfio/iommufd.c | 11 ++++++-----
include/uapi/linux/vfio.h | 6 ++++++
2 files changed, 12 insertions(+), 5 deletions(-)
diff --git a/drivers/vfio/iommufd.c b/drivers/vfio/iommufd.c
index 4d84904fd927..82eba6966fa5 100644
--- a/drivers/vfio/iommufd.c
+++ b/drivers/vfio/iommufd.c
@@ -146,9 +146,9 @@ int vfio_iommufd_physical_attach_ioas(struct vfio_device *vdev, u32 *pt_id)
return -EINVAL;
if (vdev->iommufd_attached)
- return -EBUSY;
-
- rc = iommufd_device_attach(vdev->iommufd_device, pt_id);
+ rc = iommufd_device_replace(vdev->iommufd_device, pt_id);
+ else
+ rc = iommufd_device_attach(vdev->iommufd_device, pt_id);
if (rc)
return rc;
vdev->iommufd_attached = true;
@@ -223,8 +223,9 @@ int vfio_iommufd_emulated_attach_ioas(struct vfio_device *vdev, u32 *pt_id)
lockdep_assert_held(&vdev->dev_set->lock);
if (vdev->iommufd_attached)
- return -EBUSY;
- rc = iommufd_access_attach(vdev->iommufd_access, *pt_id);
+ rc = iommufd_access_replace(vdev->iommufd_access, *pt_id);
+ else
+ rc = iommufd_access_attach(vdev->iommufd_access, *pt_id);
if (rc)
return rc;
vdev->iommufd_attached = true;
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index fa06e3eb4955..537157ff8670 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -939,6 +939,12 @@ struct vfio_device_bind_iommufd {
* Undo by VFIO_DEVICE_DETACH_IOMMUFD_PT or device fd close. This is only
* allowed on cdev fds.
*
+ * If a vfio device is currently attached to a valid hw_pagetable, without doing
+ * a VFIO_DEVICE_DETACH_IOMMUFD_PT, a second VFIO_DEVICE_ATTACH_IOMMUFD_PT ioctl
+ * passing in another hw_pagetable (hwpt) id is allowed. This action, also known
+ * as a hw_pagetable replacement, will replace the device's currently attached
+ * hw_pagetable with a new hw_pagetable corresponding to the given pt_id.
+ *
* Return: 0 on success, -errno on failure.
*/
struct vfio_device_attach_iommufd_pt {
--
2.41.0
^ permalink raw reply related [flat|nested] 11+ messages in thread