On Wed, Oct 09, 2024 at 09:38:02AM -0700, Nicolin Chen wrote:

@@ -217,12 +217,12 @@ iommufd_object_put_and_try_destroy(struct iommufd_ctx *ictx, iommufd_object_remove(ictx, obj, obj->id, 0); } -struct iommufd_object *_iommufd_object_alloc(struct iommufd_ctx *ictx,

• 			     size_t size,
  

• 			     enum iommufd_object_type type);
  

+struct iommufd_object *iommufd_object_alloc_elm(struct iommufd_ctx *ictx,

• 				size_t size,
  

• 				enum iommufd_object_type type);
  

Maybe call it raw instead of elm? elm suggests it is an item in an array or likewise

Naming aside

Reviewed-by: Jason Gunthorpe jgg@nvidia.com

Jason

On Thu, Oct 17, 2024 at 12:37:49PM -0300, Jason Gunthorpe wrote:

On Thu, Oct 17, 2024 at 11:14:16AM -0300, Jason Gunthorpe wrote:

...

On Wed, Oct 09, 2024 at 09:38:02AM -0700, Nicolin Chen wrote:

...

@@ -217,12 +217,12 @@ iommufd_object_put_and_try_destroy(struct iommufd_ctx *ictx, iommufd_object_remove(ictx, obj, obj->id, 0); } -struct iommufd_object *_iommufd_object_alloc(struct iommufd_ctx *ictx,

• 			     size_t size,
  

• 			     enum iommufd_object_type type);
  

+struct iommufd_object *iommufd_object_alloc_elm(struct iommufd_ctx *ictx,

• 				size_t size,
  

• 				enum iommufd_object_type type);
  

Maybe call it raw instead of elm? elm suggests it is an item in an array or likewise

Or keep this as the __ and rename

You mean "_" v.s. "__"?

#define __iommufd_object_alloc(ictx, ptr, type, obj) \

That one to _elm like this:

#define iommufd_object_alloc_elm(ictx, ptr, type, elm) \ container_of(_iommufd_object_alloc( \ ictx, \ sizeof(*(ptr)) + BUILD_BUG_ON_ZERO( \ offsetof(typeof(*(ptr)), \ obj) != 0), \ type), \ typeof(*(ptr)), elm)

#define iommufd_object_alloc(ictx, ptr, type) \ iommufd_object_alloc_elm(ictx, ptr, type, obj)

Then you can keep the pattern of _ being the allocation function of the macro

If I get it correctly, the change would be [From] level-0: iommufd_object_alloc() level-1: __iommufd_object_alloc() level-2: _iommufd_object_alloc() [To] level-0: iommufd_object_alloc() level-1: iommufd_object_alloc_elm() level-2: _iommufd_object_alloc()

i.e. change the level-1 only.

Thanks Nicolin

On 18/10/24 02:37, Jason Gunthorpe wrote:

On Thu, Oct 17, 2024 at 11:14:16AM -0300, Jason Gunthorpe wrote:

...

On Wed, Oct 09, 2024 at 09:38:02AM -0700, Nicolin Chen wrote:

...

@@ -217,12 +217,12 @@ iommufd_object_put_and_try_destroy(struct iommufd_ctx *ictx, iommufd_object_remove(ictx, obj, obj->id, 0); } -struct iommufd_object *_iommufd_object_alloc(struct iommufd_ctx *ictx,

• 			     size_t size,
  

• 			     enum iommufd_object_type type);
  

+struct iommufd_object *iommufd_object_alloc_elm(struct iommufd_ctx *ictx,

• 				size_t size,
  

• 				enum iommufd_object_type type);
  

Maybe call it raw instead of elm? elm suggests it is an item in an array or likewise

Or keep this as the __ and rename

#define __iommufd_object_alloc(ictx, ptr, type, obj) \

That one to _elm like this:

#define iommufd_object_alloc_elm(ictx, ptr, type, elm) \ container_of(_iommufd_object_alloc( \ ictx, \ sizeof(*(ptr)) + BUILD_BUG_ON_ZERO( \ offsetof(typeof(*(ptr)), \ obj) != 0), \ type), \ typeof(*(ptr)), elm)

#define iommufd_object_alloc(ictx, ptr, type) \ iommufd_object_alloc_elm(ictx, ptr, type, obj)

Bikeshedding, yay :)

After starring at it for 10min - honestly - ditch iommufd_object_alloc_elm() and just pass "obj" (or "common.obj" in that single other occasion) to iommufd_object_alloc().

__iommufd_object_alloc() - a function - will the actual alloc, iommufd_object_alloc() - a macro - will do the types + call the __ variant, simple and no naming issues.

And it would be real nice if it was "iobj" not this "obj" which is way too generic. Thanks,

Then you can keep the pattern of _ being the allocation function of the macro

Jason

On Sat, Oct 12, 2024 at 11:23:07AM +0800, Zhangfei Gao wrote:

...

diff --git a/drivers/iommu/iommufd/viommu_api.c b/drivers/iommu/iommufd/viommu_api.c new file mode 100644 index 000000000000..c1731f080d6b --- /dev/null +++ b/drivers/iommu/iommufd/viommu_api.c @@ -0,0 +1,57 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* Copyright (c) 2024, NVIDIA CORPORATION & AFFILIATES

• */

+#include "iommufd_private.h"

+struct iommufd_object *iommufd_object_alloc_elm(struct iommufd_ctx *ictx,

•                                           size_t size,
  

•                                           enum iommufd_object_type type)
  

+{

•   struct iommufd_object *obj;
  

•   int rc;
  

•   obj = kzalloc(size, GFP_KERNEL_ACCOUNT);
  

•   if (!obj)
  

•           return ERR_PTR(-ENOMEM);
  

•   obj->type = type;
  

•   /* Starts out bias'd by 1 until it is removed from the xarray */
  

•   refcount_set(&obj->shortterm_users, 1);
  

•   refcount_set(&obj->users, 1);
  

here set refcont 1

iommufd_device_bind -> iommufd_object_alloc(ictx, idev, IOMMUFD_OBJ_DEVICE): refcont -> 1 refcount_inc(&idev->obj.users); refcount -> 2 will cause iommufd_device_unbind fail.

May remove refcount_inc(&idev->obj.users) in iommufd_device_bind

Hmm, why would it fail? Or is it failing on your system?

This patch doesn't change the function but only moved it..

Thanks Nicolin

Hi, Nico

On Sat, 12 Oct 2024 at 18:18, Zhangfei Gao zhangfei.gao@linaro.org wrote:

On Sat, 12 Oct 2024 at 12:49, Nicolin Chen nicolinc@nvidia.com wrote:

...

On Sat, Oct 12, 2024 at 11:23:07AM +0800, Zhangfei Gao wrote:

...

...

diff --git a/drivers/iommu/iommufd/viommu_api.c b/drivers/iommu/iommufd/viommu_api.c new file mode 100644 index 000000000000..c1731f080d6b --- /dev/null +++ b/drivers/iommu/iommufd/viommu_api.c @@ -0,0 +1,57 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* Copyright (c) 2024, NVIDIA CORPORATION & AFFILIATES

• */

+#include "iommufd_private.h"

+struct iommufd_object *iommufd_object_alloc_elm(struct iommufd_ctx *ictx,

•                                           size_t size,
  

•                                           enum iommufd_object_type type)
  

+{

•   struct iommufd_object *obj;
  

•   int rc;
  

•   obj = kzalloc(size, GFP_KERNEL_ACCOUNT);
  

•   if (!obj)
  

•           return ERR_PTR(-ENOMEM);
  

•   obj->type = type;
  

•   /* Starts out bias'd by 1 until it is removed from the xarray */
  

•   refcount_set(&obj->shortterm_users, 1);
  

•   refcount_set(&obj->users, 1);
  

here set refcont 1

iommufd_device_bind -> iommufd_object_alloc(ictx, idev, IOMMUFD_OBJ_DEVICE): refcont -> 1 refcount_inc(&idev->obj.users); refcount -> 2 will cause iommufd_device_unbind fail.

May remove refcount_inc(&idev->obj.users) in iommufd_device_bind

Hmm, why would it fail? Or is it failing on your system?

Not sure, still in check, it may only be on my platform.

it hit iommufd_object_remove: if (WARN_ON(obj != to_destroy))

iommufd_device_bind refcount=2 iommufd_device_attach refcount=3 //still not sure which operation inc the count? iommufd_device_detach refcount=4

Have a question, when should iommufd_vdevice_destroy be called, before or after iommufd_device_unbind.

Now iommufd_vdevice_destroy (ref--) is after unbind, hits the if (!refcount_dec_if_one(&obj->users)) check.

iommufd_device_bind iommufd_device_attach iommufd_vdevice_alloc_ioctl

iommufd_device_detach iommufd_device_unbind // refcount check fail iommufd_vdevice_destroy ref--

Thanks

On Mon, Oct 14, 2024 at 07:01:40PM -0700, Nicolin Chen wrote:

On Tue, Oct 15, 2024 at 09:15:01AM +0800, Zhangfei Gao wrote:

...

...

...

iommufd_device_bind iommufd_device_attach iommufd_vdevice_alloc_ioctl

iommufd_device_detach iommufd_device_unbind // refcount check fail iommufd_vdevice_destroy ref--

Things should be symmetric. As you suspected, vdevice should be destroyed before iommufd_device_detach.

I am trying based on your for_iommufd_viommu_p2-v3 branch, do you have this issue? In checking whether close fd before unbind?

Oops, my bad. I will provide a fix.

This should fix the problem:

--------------------------------------------------------------------- diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c index 5fd3dd420290..13100cfea29d 100644 --- a/drivers/iommu/iommufd/device.c +++ b/drivers/iommu/iommufd/device.c @@ -277,6 +277,11 @@ EXPORT_SYMBOL_NS_GPL(iommufd_ctx_has_group, IOMMUFD); */ void iommufd_device_unbind(struct iommufd_device *idev) { + mutex_lock(&idev->igroup->lock); + /* idev->vdev object should be destroyed prior, yet just in case.. */ + if (idev->vdev) + iommufd_object_remove(idev->ictx, NULL, idev->vdev->obj.id, 0); + mutex_unlock(&idev->igroup->lock); iommufd_object_destroy_user(idev->ictx, &idev->obj); } EXPORT_SYMBOL_NS_GPL(iommufd_device_unbind, IOMMUFD); ---------------------------------------------------------------------

Thanks Nicolin

On Wed, Oct 16, 2024 at 09:56:51AM +0800, Zhangfei Gao wrote:

On Wed, 16 Oct 2024 at 02:44, Nicolin Chen nicolinc@nvidia.com wrote:

...

On Mon, Oct 14, 2024 at 07:01:40PM -0700, Nicolin Chen wrote:

...

On Tue, Oct 15, 2024 at 09:15:01AM +0800, Zhangfei Gao wrote:

...

...

...

iommufd_device_bind iommufd_device_attach iommufd_vdevice_alloc_ioctl

iommufd_device_detach iommufd_device_unbind // refcount check fail iommufd_vdevice_destroy ref--

Things should be symmetric. As you suspected, vdevice should be destroyed before iommufd_device_detach.

I am trying based on your for_iommufd_viommu_p2-v3 branch, do you have this issue? In checking whether close fd before unbind?

Oops, my bad. I will provide a fix.

This should fix the problem:

---

diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c index 5fd3dd420290..13100cfea29d 100644 --- a/drivers/iommu/iommufd/device.c +++ b/drivers/iommu/iommufd/device.c @@ -277,6 +277,11 @@ EXPORT_SYMBOL_NS_GPL(iommufd_ctx_has_group, IOMMUFD); */ void iommufd_device_unbind(struct iommufd_device *idev) {

•   mutex_lock(&idev->igroup->lock);
  

•   /* idev->vdev object should be destroyed prior, yet just in case.. */
  

•   if (idev->vdev)
  

•           iommufd_object_remove(idev->ictx, NULL, idev->vdev->obj.id, 0);
  

•   mutex_unlock(&idev->igroup->lock);
    iommufd_object_destroy_user(idev->ictx, &idev->obj);
  

} EXPORT_SYMBOL_NS_GPL(iommufd_device_unbind, IOMMUFD);

---

Not yet [ 574.162112] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000004 [ 574.261102] pc : iommufd_object_remove+0x7c/0x278 [ 574.265795] lr : iommufd_device_unbind+0x44/0x98 in check

Hmm, it's kinda odd it crashes inside iommufd_object_remove(). Did you happen to change something there?

The added iommufd_object_remove() is equivalent to userspace calling the destroy ioctl on the vDEVICE object.

Nicolin

On 10/10/24 03:38, Nicolin Chen wrote:

Add a new ioctl for user space to do a vIOMMU allocation. It must be based on a nesting parent HWPT, so take its refcount.

As an initial version, define an IOMMU_VIOMMU_TYPE_DEFAULT type. Using it, the object will be allocated by the iommufd core.

If an IOMMU driver supports a driver-managed vIOMMU object, it must define its own IOMMU_VIOMMU_TYPE_ in the uAPI header and implement a viommu_alloc op in its iommu_ops.

Signed-off-by: Nicolin Chen nicolinc@nvidia.com

drivers/iommu/iommufd/Makefile | 3 +- drivers/iommu/iommufd/iommufd_private.h | 3 + include/uapi/linux/iommufd.h | 40 +++++++++++ drivers/iommu/iommufd/main.c | 6 ++ drivers/iommu/iommufd/viommu.c | 91 +++++++++++++++++++++++++ 5 files changed, 142 insertions(+), 1 deletion(-) create mode 100644 drivers/iommu/iommufd/viommu.c

diff --git a/drivers/iommu/iommufd/Makefile b/drivers/iommu/iommufd/Makefile index 93daedd7e5c8..288ef3e895e3 100644 --- a/drivers/iommu/iommufd/Makefile +++ b/drivers/iommu/iommufd/Makefile @@ -7,7 +7,8 @@ iommufd-y := \ ioas.o \ main.o \ pages.o \

• vfio_compat.o

• vfio_compat.o \
• viommu.o

iommufd-$(CONFIG_IOMMUFD_TEST) += selftest.o diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h index 6a364073f699..4aefac6af23f 100644 --- a/drivers/iommu/iommufd/iommufd_private.h +++ b/drivers/iommu/iommufd/iommufd_private.h @@ -521,6 +521,9 @@ static inline int iommufd_hwpt_replace_device(struct iommufd_device *idev, return iommu_group_replace_domain(idev->igroup->group, hwpt->domain); } +int iommufd_viommu_alloc_ioctl(struct iommufd_ucmd *ucmd); +void iommufd_viommu_destroy(struct iommufd_object *obj);

• #ifdef CONFIG_IOMMUFD_TEST int iommufd_test(struct iommufd_ucmd *ucmd); void iommufd_selftest_destroy(struct iommufd_object *obj);

diff --git a/include/uapi/linux/iommufd.h b/include/uapi/linux/iommufd.h index cd4920886ad0..db9008a4eeef 100644 --- a/include/uapi/linux/iommufd.h +++ b/include/uapi/linux/iommufd.h @@ -51,6 +51,7 @@ enum { IOMMUFD_CMD_HWPT_GET_DIRTY_BITMAP = 0x8c, IOMMUFD_CMD_HWPT_INVALIDATE = 0x8d, IOMMUFD_CMD_FAULT_QUEUE_ALLOC = 0x8e,

• IOMMUFD_CMD_VIOMMU_ALLOC = 0x8f, };

/** @@ -852,4 +853,43 @@ struct iommu_fault_alloc { __u32 out_fault_fd; }; #define IOMMU_FAULT_QUEUE_ALLOC _IO(IOMMUFD_TYPE, IOMMUFD_CMD_FAULT_QUEUE_ALLOC)

+/**

• • enum iommu_viommu_type - Virtual IOMMU Type
• • @IOMMU_VIOMMU_TYPE_DEFAULT: Core-managed virtual IOMMU type
• */

+enum iommu_viommu_type {

• IOMMU_VIOMMU_TYPE_DEFAULT = 0,

+};

+/**

• • struct iommu_viommu_alloc - ioctl(IOMMU_VIOMMU_ALLOC)
• • @size: sizeof(struct iommu_viommu_alloc)
• • @flags: Must be 0
• • @type: Type of the virtual IOMMU. Must be defined in enum iommu_viommu_type
• • @dev_id: The device's physical IOMMU will be used to back the virtual IOMMU
• • @hwpt_id: ID of a nesting parent HWPT to associate to
• • @out_viommu_id: Output virtual IOMMU ID for the allocated object
• • Allocate a virtual IOMMU object that represents the underlying physical
• • IOMMU's virtualization support. The vIOMMU object is a security-isolated
• • slice of the physical IOMMU HW that is unique to a specific VM. Operations
• • global to the IOMMU are connected to the vIOMMU, such as:
• • • Security namespace for guest owned ID, e.g. guest-controlled cache tags
• • • Access to a sharable nesting parent pagetable across physical IOMMUs
• • • Virtualization of various platforms IDs, e.g. RIDs and others
• • • Delivery of paravirtualized invalidation
• • • Direct assigned invalidation queues
• • • Direct assigned interrupts
• • • Non-affiliated event reporting
• */

+struct iommu_viommu_alloc {

• __u32 size;
• __u32 flags;
• __u32 type;
• __u32 dev_id;
• __u32 hwpt_id;
• __u32 out_viommu_id;

+}; +#define IOMMU_VIOMMU_ALLOC _IO(IOMMUFD_TYPE, IOMMUFD_CMD_VIOMMU_ALLOC) #endif diff --git a/drivers/iommu/iommufd/main.c b/drivers/iommu/iommufd/main.c index 92bd075108e5..cbd0a80b2d67 100644 --- a/drivers/iommu/iommufd/main.c +++ b/drivers/iommu/iommufd/main.c @@ -301,6 +301,7 @@ union ucmd_buffer { struct iommu_ioas_unmap unmap; struct iommu_option option; struct iommu_vfio_ioas vfio_ioas;

• struct iommu_viommu_alloc viommu; #ifdef CONFIG_IOMMUFD_TEST struct iommu_test_cmd test; #endif

@@ -352,6 +353,8 @@ static const struct iommufd_ioctl_op iommufd_ioctl_ops[] = { val64), IOCTL_OP(IOMMU_VFIO_IOAS, iommufd_vfio_ioas, struct iommu_vfio_ioas, __reserved),

• IOCTL_OP(IOMMU_VIOMMU_ALLOC, iommufd_viommu_alloc_ioctl,

•  struct iommu_viommu_alloc, out_viommu_id),
  

  #ifdef CONFIG_IOMMUFD_TEST IOCTL_OP(IOMMU_TEST_CMD, iommufd_test, struct iommu_test_cmd, last), #endif

@@ -487,6 +490,9 @@ static const struct iommufd_object_ops iommufd_object_ops[] = { [IOMMUFD_OBJ_FAULT] = { .destroy = iommufd_fault_destroy, },

• [IOMMUFD_OBJ_VIOMMU] = {

• .destroy = iommufd_viommu_destroy,
  

• }, #ifdef CONFIG_IOMMUFD_TEST [IOMMUFD_OBJ_SELFTEST] = { .destroy = iommufd_selftest_destroy,

diff --git a/drivers/iommu/iommufd/viommu.c b/drivers/iommu/iommufd/viommu.c new file mode 100644 index 000000000000..3a903baeee6a --- /dev/null +++ b/drivers/iommu/iommufd/viommu.c @@ -0,0 +1,91 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* Copyright (c) 2024, NVIDIA CORPORATION & AFFILIATES

• */

+#include "iommufd_private.h"

+void iommufd_viommu_destroy(struct iommufd_object *obj) +{

• struct iommufd_viommu *viommu =

• container_of(obj, struct iommufd_viommu, obj);
  

• if (viommu->ops && viommu->ops->free)

• viommu->ops->free(viommu);
  

• refcount_dec(&viommu->hwpt->common.obj.users);

+}

+int iommufd_viommu_alloc_ioctl(struct iommufd_ucmd *ucmd) +{

• struct iommu_viommu_alloc *cmd = ucmd->cmd;
• struct iommufd_hwpt_paging *hwpt_paging;
• struct iommufd_viommu *viommu;
• struct iommufd_device *idev;
• const struct iommu_ops *ops;
• int rc;
• if (cmd->flags)

• return -EOPNOTSUPP;
  

• idev = iommufd_get_device(ucmd, cmd->dev_id);
• if (IS_ERR(idev))

• return PTR_ERR(idev);
  

• ops = dev_iommu_ops(idev->dev);
• hwpt_paging = iommufd_get_hwpt_paging(ucmd, cmd->hwpt_id);
• if (IS_ERR(hwpt_paging)) {

• rc = PTR_ERR(hwpt_paging);
  

iommufd_get_hwpt_paging() is container_of() which does not test for the value and just does a simple math so the actual error value from iommufd_get_object() is ... lost?

Oh well, not really lost, as "obj" and "common.obj" seem to be forced to be at the beginning of iommufd object structs so container_of() is no-op or is not it?

• goto out_put_idev;
  

• }
• if (!hwpt_paging->nest_parent) {

• rc = -EINVAL;
  

• goto out_put_hwpt;
  

• }
• if (cmd->type == IOMMU_VIOMMU_TYPE_DEFAULT) {

• viommu = __iommufd_viommu_alloc(ucmd->ictx, sizeof(*viommu),
  

• 				NULL);
  

• } else {

• if (!ops->viommu_alloc) {
  

• 	rc = -EOPNOTSUPP;
  

• 	goto out_put_hwpt;
  

• }
  

• viommu = ops->viommu_alloc(idev->dev->iommu->iommu_dev,
  

• 			   hwpt_paging->common.domain,
  

• 			   ucmd->ictx, cmd->type);
  

• }
• if (IS_ERR(viommu)) {

• rc = PTR_ERR(viommu);
  

• goto out_put_hwpt;
  

• }
• viommu->type = cmd->type;
• viommu->ictx = ucmd->ictx;
• viommu->hwpt = hwpt_paging;
• /*

• * A real physical IOMMU instance would unlikely get unplugged, so the
  

• * life cycle of this iommu_dev is guaranteed to stay alive, mostly. A
  

• * pluggable IOMMU instance (if exists) is responsible for refcounting
  

• * on its own.
  

"Assume IOMMUs are unpluggable (the most likely case)" would do imho, all these "unlikely" and "mostly" and "if exists" confuse.

If something needs to be guaranteed to stay alive, may be just call device_get(viommu->dev), with the comment above? Or it is some different refcounting which is missing? Thanks,

• */
  

• viommu->iommu_dev = idev->dev->iommu->iommu_dev;
• refcount_inc(&viommu->hwpt->common.obj.users);
• cmd->out_viommu_id = viommu->obj.id;
• rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd));
• if (rc)

• goto out_abort;
  

• iommufd_object_finalize(ucmd->ictx, &viommu->obj);
• goto out_put_hwpt;

+out_abort:

• iommufd_object_abort_and_destroy(ucmd->ictx, &viommu->obj);

+out_put_hwpt:

• iommufd_put_object(ucmd->ictx, &hwpt_paging->common.obj);

+out_put_idev:

• iommufd_put_object(ucmd->ictx, &idev->obj);
• return rc;

+}

On Mon, Oct 21, 2024 at 07:11:47PM +1100, Alexey Kardashevskiy wrote:

...

• /*
  

•  * A real physical IOMMU instance would unlikely get unplugged, so the
  

•  * life cycle of this iommu_dev is guaranteed to stay alive, mostly. A
  

•  * pluggable IOMMU instance (if exists) is responsible for refcounting
  

•  * on its own.
  

"Assume IOMMUs are unpluggable (the most likely case)" would do imho, all these "unlikely" and "mostly" and "if exists" confuse.

Done.

----------------------------------------------------------------- @@ -63,13 +63,7 @@ int iommufd_viommu_alloc_ioctl(struct iommufd_ucmd *ucmd) viommu->type = cmd->type; viommu->ictx = ucmd->ictx; viommu->hwpt = hwpt_paging; - - /* - * A real physical IOMMU instance would unlikely get unplugged, so the - * life cycle of this iommu_dev is guaranteed to stay alive, mostly. A - * pluggable IOMMU instance (if exists) is responsible for refcounting - * on its own. - */ + /* Assume physical IOMMUs are unpluggable (the most likely case) */ viommu->iommu_dev = __iommu_get_iommu_dev(idev->dev);

refcount_inc(&viommu->hwpt->common.obj.users); -----------------------------------------------------------------

Thanks Nicolin

On Wed, Oct 09, 2024 at 09:38:05AM -0700, Nicolin Chen wrote:

With a viommu object wrapping a potentially shareable S2 domain, a nested domain should be allocated by associating to a viommu instead.

For drivers without a viommu support, keep the parent domain input, which should be just viommu->hwpt->common.domain otherwise.

Signed-off-by: Nicolin Chen nicolinc@nvidia.com

include/linux/iommu.h | 1 + drivers/iommu/amd/iommu.c | 1 + drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 1 + drivers/iommu/intel/iommu.c | 1 + drivers/iommu/iommufd/hw_pagetable.c | 5 +++-- drivers/iommu/iommufd/selftest.c | 1 + 6 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h index 3a50f57b0861..9105478bdbcd 100644 --- a/include/linux/iommu.h +++ b/include/linux/iommu.h @@ -573,6 +573,7 @@ struct iommu_ops { struct iommu_domain *(*domain_alloc)(unsigned iommu_domain_type); struct iommu_domain *(*domain_alloc_user)( struct device *dev, u32 flags, struct iommu_domain *parent,

• struct iommufd_viommu *viommu,
  

  const struct iommu_user_data *user_data);

This re-enforces my feeling we should have made a domain_alloc_nested()..

struct iommu_domain *(*domain_alloc_paging)(struct device *dev); struct iommu_domain *(*domain_alloc_sva)(struct device *dev, diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c index 8364cd6fa47d..3100ddcaf62e 100644 --- a/drivers/iommu/amd/iommu.c +++ b/drivers/iommu/amd/iommu.c @@ -2394,6 +2394,7 @@ static struct iommu_domain *amd_iommu_domain_alloc(unsigned int type) static struct iommu_domain * amd_iommu_domain_alloc_user(struct device *dev, u32 flags, struct iommu_domain *parent,

• 	    struct iommufd_viommu *viommu,
      const struct iommu_user_data *user_data)
  

{

Do these all need to check for NULL viommu now too? Something is needed at least, only valid viommus should be accepted here. Like you can't pass an ARM viommu to AMD or something silly.

Should drivers accept the default viommu?

Jason

On Thu, Oct 17, 2024 at 10:21:31AM -0700, Nicolin Chen wrote:

On Thu, Oct 17, 2024 at 01:51:51PM -0300, Jason Gunthorpe wrote:

...

On Wed, Oct 09, 2024 at 09:38:05AM -0700, Nicolin Chen wrote:

...

With a viommu object wrapping a potentially shareable S2 domain, a nested domain should be allocated by associating to a viommu instead.

For drivers without a viommu support, keep the parent domain input, which should be just viommu->hwpt->common.domain otherwise.

Signed-off-by: Nicolin Chen nicolinc@nvidia.com

include/linux/iommu.h | 1 + drivers/iommu/amd/iommu.c | 1 + drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 1 + drivers/iommu/intel/iommu.c | 1 + drivers/iommu/iommufd/hw_pagetable.c | 5 +++-- drivers/iommu/iommufd/selftest.c | 1 + 6 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h index 3a50f57b0861..9105478bdbcd 100644 --- a/include/linux/iommu.h +++ b/include/linux/iommu.h @@ -573,6 +573,7 @@ struct iommu_ops { struct iommu_domain *(*domain_alloc)(unsigned iommu_domain_type); struct iommu_domain *(*domain_alloc_user)( struct device *dev, u32 flags, struct iommu_domain *parent,

• struct iommufd_viommu *viommu,
  

  const struct iommu_user_data *user_data);

This re-enforces my feeling we should have made a domain_alloc_nested()..

That could make these changes slightly cleaner. Maybe adding a small series prior to your initial nesting, and get it merged quickly?

Maybe we should put an op on the viommu_ops to allocate a nested domain?

It make some sense and resolves my worries about checking for driver ownership.

Jason

On 10/10/24 03:38, Nicolin Chen wrote:

Add a new iommufd_viommu FIXTURE and setup it up with a vIOMMU object.

Any new vIOMMU feature will be added as a TEST_F under that.

Signed-off-by: Nicolin Chen nicolinc@nvidia.com

tools/testing/selftests/iommu/iommufd_utils.h | 28 +++++++ tools/testing/selftests/iommu/iommufd.c | 84 +++++++++++++++++++ 2 files changed, 112 insertions(+)

diff --git a/tools/testing/selftests/iommu/iommufd_utils.h b/tools/testing/selftests/iommu/iommufd_utils.h index 40f6f14ce136..307d097db9dd 100644 --- a/tools/testing/selftests/iommu/iommufd_utils.h +++ b/tools/testing/selftests/iommu/iommufd_utils.h @@ -762,3 +762,31 @@ static int _test_cmd_trigger_iopf(int fd, __u32 device_id, __u32 fault_fd) #define test_cmd_trigger_iopf(device_id, fault_fd) \ ASSERT_EQ(0, _test_cmd_trigger_iopf(self->fd, device_id, fault_fd))

+static int _test_cmd_viommu_alloc(int fd, __u32 device_id, __u32 hwpt_id,

• 		  __u32 type, __u32 flags, __u32 *viommu_id)
  

+{

• struct iommu_viommu_alloc cmd = {

• .size = sizeof(cmd),
  

• .flags = flags,
  

• .type = type,
  

• .dev_id = device_id,
  

• .hwpt_id = hwpt_id,
  

• };
• int ret;
• ret = ioctl(fd, IOMMU_VIOMMU_ALLOC, &cmd);
• if (ret)

• return ret;
  

• if (viommu_id)

"if" can be dropped as viommu_id is always non-null in this test. Thanks,

• *viommu_id = cmd.out_viommu_id;
  

• return 0;

+}

+#define test_cmd_viommu_alloc(device_id, hwpt_id, type, viommu_id) \

• ASSERT_EQ(0, _test_cmd_viommu_alloc(self->fd, device_id, hwpt_id, \

• 			    type, 0, viommu_id))
  

+#define test_err_viommu_alloc(_errno, device_id, hwpt_id, type, viommu_id) \

• EXPECT_ERRNO(_errno, _test_cmd_viommu_alloc(self->fd, device_id, \

• 				    hwpt_id, type, 0,      \
  

• 				    viommu_id))
  

diff --git a/tools/testing/selftests/iommu/iommufd.c b/tools/testing/selftests/iommu/iommufd.c index 4927b9add5ad..c03705825576 100644 --- a/tools/testing/selftests/iommu/iommufd.c +++ b/tools/testing/selftests/iommu/iommufd.c @@ -128,6 +128,7 @@ TEST_F(iommufd, cmd_length) TEST_LENGTH(iommu_ioas_unmap, IOMMU_IOAS_UNMAP, length); TEST_LENGTH(iommu_option, IOMMU_OPTION, val64); TEST_LENGTH(iommu_vfio_ioas, IOMMU_VFIO_IOAS, __reserved);

• TEST_LENGTH(iommu_viommu_alloc, IOMMU_VIOMMU_ALLOC, out_viommu_id); #undef TEST_LENGTH }

@@ -2386,4 +2387,87 @@ TEST_F(vfio_compat_mock_domain, huge_map) } } +FIXTURE(iommufd_viommu) +{

• int fd;
• uint32_t ioas_id;
• uint32_t stdev_id;
• uint32_t hwpt_id;
• uint32_t device_id;
• uint32_t viommu_id;

+};

+FIXTURE_VARIANT(iommufd_viommu) +{

• unsigned int viommu;
• unsigned int viommu_type;

+};

+FIXTURE_SETUP(iommufd_viommu) +{

• self->fd = open("/dev/iommu", O_RDWR);
• ASSERT_NE(-1, self->fd);
• test_ioctl_ioas_alloc(&self->ioas_id);
• test_ioctl_set_default_memory_limit();
• if (variant->viommu) {

• test_cmd_mock_domain(self->ioas_id, &self->stdev_id, NULL,
  

• 		     &self->device_id);
  

• /* Negative test -- invalid hwpt */
  

• test_err_viommu_alloc(ENOENT, self->device_id, self->hwpt_id,
  

• 		      variant->viommu_type, &self->viommu_id);
  

• /* Negative test -- not a nesting parent hwpt */
  

• test_cmd_hwpt_alloc(self->device_id, self->ioas_id, 0,
  

• 		    &self->hwpt_id);
  

• test_err_viommu_alloc(EINVAL, self->device_id, self->hwpt_id,
  

• 		      variant->viommu_type, &self->viommu_id);
  

• test_ioctl_destroy(self->hwpt_id);
  

• /* Allocate a nesting parent HWP */
  

• test_cmd_hwpt_alloc(self->device_id, self->ioas_id,
  

• 		    IOMMU_HWPT_ALLOC_NEST_PARENT,
  

• 		    &self->hwpt_id);
  

• /* Negative test -- unsupported viommu type */
  

• test_err_viommu_alloc(EOPNOTSUPP, self->device_id,
  

• 		      self->hwpt_id, 0xdead, &self->viommu_id);
  

• /* Allocate a default type of viommu */
  

• test_cmd_viommu_alloc(self->device_id, self->hwpt_id,
  

• 		      variant->viommu_type, &self->viommu_id);
  

• } else {

• test_err_viommu_alloc(ENOENT, self->device_id, self->hwpt_id,
  

• 		      variant->viommu_type, &self->viommu_id);
  

• }

+}

+FIXTURE_TEARDOWN(iommufd_viommu) +{

• if (variant->viommu) {

• test_ioctl_destroy(self->viommu_id);
  

• test_ioctl_destroy(self->hwpt_id);
  

• }
• teardown_iommufd(self->fd, _metadata);

+}

+FIXTURE_VARIANT_ADD(iommufd_viommu, no_viommu) +{ +};

+FIXTURE_VARIANT_ADD(iommufd_viommu, viommu_default) +{

• .viommu = 1,
• .viommu_type = IOMMU_VIOMMU_TYPE_DEFAULT,

+};

+FIXTURE_VARIANT_ADD(iommufd_viommu, mock_viommu) +{

• .viommu = 1,
• .viommu_type = IOMMU_VIOMMU_TYPE_SELFTEST,

+};

+TEST_F(iommufd_viommu, viommu_auto_destroy) +{ +}

• TEST_HARNESS_MAIN

Introduce a new IOMMUFD_OBJ_VDEVICE to represent a physical device, i.e. iommufd_device (idev) object, against an iommufd_viommu (vIOMMU) object in the VM. This vDEVICE object (and its structure) holds all the information and attributes in a VM, regarding the device related to the vIOMMU.

As an initial patch, add a per-vIOMMU virtual ID. This can be: - Virtual StreamID on a nested ARM SMMUv3, an index to a Stream Table - Virtual DeviceID on a nested AMD IOMMU, an index to a Device Table - Virtual ID on a nested Intel VT-D IOMMU, an index to a Context Table Potentially, this vDEVICE structure can hold some vData for Confidential Compute Architecture (CCA).

Add a pair of vdevice_alloc and vdevice_free in struct iommufd_viommu_ops to allow driver-level vDEVICE structure allocations.

Similar to iommufd_vdevice_alloc, add an iommufd_vdevice_alloc helper so IOMMU drivers can allocate core-embedded style structures.

Signed-off-by: Nicolin Chen nicolinc@nvidia.com --- drivers/iommu/iommufd/iommufd_private.h | 1 + include/linux/iommufd.h | 31 +++++++++++++++++++++++++ drivers/iommu/iommufd/viommu_api.c | 14 +++++++++++ 3 files changed, 46 insertions(+)

diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h index c80d880f8b6a..0c56a467e440 100644 --- a/drivers/iommu/iommufd/iommufd_private.h +++ b/drivers/iommu/iommufd/iommufd_private.h @@ -132,6 +132,7 @@ enum iommufd_object_type { IOMMUFD_OBJ_ACCESS, IOMMUFD_OBJ_FAULT, IOMMUFD_OBJ_VIOMMU, + IOMMUFD_OBJ_VDEVICE, #ifdef CONFIG_IOMMUFD_TEST IOMMUFD_OBJ_SELFTEST, #endif diff --git a/include/linux/iommufd.h b/include/linux/iommufd.h index 069a38999cdd..510fc961a9ad 100644 --- a/include/linux/iommufd.h +++ b/include/linux/iommufd.h @@ -75,13 +75,30 @@ struct iommufd_viommu { unsigned int type; };

+struct iommufd_vdevice { + struct iommufd_object obj; + struct iommufd_ctx *ictx; + struct iommufd_device *idev; + struct iommufd_viommu *viommu; + u64 id; /* per-vIOMMU virtual ID */ +}; + /** * struct iommufd_viommu_ops - vIOMMU specific operations * @free: Free all driver-specific parts of an iommufd_viommu. The memory of the * vIOMMU will be free-ed by iommufd core after calling this free op. + * @vdevice_alloc: Allocate a driver-managed iommufd_vdevice to init some driver + * specific structure or HW procedure. Note that the core-level + * structure is filled by the iommufd core after calling this op. + * @vdevice_free: Free a driver-managed iommufd_vdevice to de-init its structure + * or HW procedure. The memory of the vdevice will be free-ed by + * iommufd core. */ struct iommufd_viommu_ops { void (*free)(struct iommufd_viommu *viommu); + struct iommufd_vdevice *(*vdevice_alloc)(struct iommufd_viommu *viommu, + struct device *dev, u64 id); + void (*vdevice_free)(struct iommufd_vdevice *vdev); };

#if IS_ENABLED(CONFIG_IOMMUFD) @@ -103,6 +120,8 @@ int iommufd_vfio_compat_set_no_iommu(struct iommufd_ctx *ictx); struct iommufd_viommu * __iommufd_viommu_alloc(struct iommufd_ctx *ictx, size_t size, const struct iommufd_viommu_ops *ops); +struct iommufd_vdevice * +__iommufd_vdevice_alloc(struct iommufd_ctx *ictx, size_t size); #else /* !CONFIG_IOMMUFD */ static inline struct iommufd_ctx *iommufd_ctx_from_file(struct file *file) { @@ -150,6 +169,12 @@ __iommufd_viommu_alloc(struct iommufd_ctx *ictx, size_t size, { return ERR_PTR(-EOPNOTSUPP); } + +static inline struct iommufd_vdevice * +__iommufd_vdevice_alloc(struct iommufd_ctx *ictx, size_t size) +{ + return ERR_PTR(-EOPNOTSUPP); +} #endif /* CONFIG_IOMMUFD */

/* @@ -163,4 +188,10 @@ __iommufd_viommu_alloc(struct iommufd_ctx *ictx, size_t size, struct drv_struct, member)), \ ops), \ struct drv_struct, member) +#define iommufd_vdevice_alloc(ictx, drv_struct, member) \ + container_of(__iommufd_vdevice_alloc(ictx, \ + sizeof(struct drv_struct) + \ + BUILD_BUG_ON_ZERO(offsetof( \ + struct drv_struct, member))), \ + struct drv_struct, member) #endif diff --git a/drivers/iommu/iommufd/viommu_api.c b/drivers/iommu/iommufd/viommu_api.c index c1731f080d6b..8419df3b658c 100644 --- a/drivers/iommu/iommufd/viommu_api.c +++ b/drivers/iommu/iommufd/viommu_api.c @@ -55,3 +55,17 @@ __iommufd_viommu_alloc(struct iommufd_ctx *ictx, size_t size, return viommu; } EXPORT_SYMBOL_NS_GPL(__iommufd_viommu_alloc, IOMMUFD); + +struct iommufd_vdevice * +__iommufd_vdevice_alloc(struct iommufd_ctx *ictx, size_t size) +{ + struct iommufd_object *obj; + + if (WARN_ON(size < sizeof(struct iommufd_vdevice))) + return ERR_PTR(-EINVAL); + obj = iommufd_object_alloc_elm(ictx, size, IOMMUFD_OBJ_VDEVICE); + if (IS_ERR(obj)) + return ERR_CAST(obj); + return container_of(obj, struct iommufd_vdevice, obj); +} +EXPORT_SYMBOL_NS_GPL(__iommufd_vdevice_alloc, IOMMUFD);

On Thu, Oct 17, 2024 at 03:45:56PM -0300, Jason Gunthorpe wrote:

...

+struct iommufd_vdevice * +__iommufd_vdevice_alloc(struct iommufd_ctx *ictx, size_t size) +{

• struct iommufd_object *obj;
• if (WARN_ON(size < sizeof(struct iommufd_vdevice)))

• return ERR_PTR(-EINVAL);
  

• obj = iommufd_object_alloc_elm(ictx, size, IOMMUFD_OBJ_VDEVICE);
• if (IS_ERR(obj))

• return ERR_CAST(obj);
  

• return container_of(obj, struct iommufd_vdevice, obj);

+}

Like for the viommu this can all just be folded into the #define

It seems that we have to keep two small functions as followings, unless we want to expose the enum iommufd_object_type.

--------------------------------------------------------------- diff --git a/drivers/iommu/iommufd/driver.c b/drivers/iommu/iommufd/driver.c index 4495f1aaccca..fdd203487bac 100644 --- a/drivers/iommu/iommufd/driver.c +++ b/drivers/iommu/iommufd/driver.c @@ -38,10 +38,19 @@ _iommufd_object_alloc_member(struct iommufd_ctx *ictx, size_t size, EXPORT_SYMBOL_NS_GPL(_iommufd_object_alloc_member, IOMMUFD);

struct iommufd_viommu *_iommufd_viommu_alloc(struct iommufd_ctx *ictx, size_t size) { return container_of(_iommufd_object_alloc_member(ictx, size, IOMMUFD_OBJ_VIOMMU), struct iommufd_viommu, obj); } EXPORT_SYMBOL_NS_GPL(_iommufd_viommu_alloc, IOMMUFD); + +struct iommufd_vdevice *_iommufd_vdevice_alloc(struct iommufd_ctx *ictx, + size_t size) +{ + return container_of(_iommufd_object_alloc_member(ictx, size, + IOMMUFD_OBJ_VDEVICE), + struct iommufd_vdevice, obj); +} +EXPORT_SYMBOL_NS_GPL(_iommufd_vdevice_alloc, IOMMUFD); ---------------------------------------------------------------

Thanks Nicolin

Add a vdevice_alloc TEST_F to cover the new IOMMU_VDEVICE_ALLOC ioctls.

Also add a vdevice_alloc op to the viommu mock_viommu_ops for a coverage of IOMMU_VIOMMU_TYPE_SELFTEST. The DEFAULT coverage is done via a core- allocated vDEVICE object.

Signed-off-by: Nicolin Chen nicolinc@nvidia.com --- tools/testing/selftests/iommu/iommufd_utils.h | 27 +++++++++++++++++++ drivers/iommu/iommufd/selftest.c | 19 +++++++++++++ tools/testing/selftests/iommu/iommufd.c | 20 ++++++++++++++ 3 files changed, 66 insertions(+)

diff --git a/tools/testing/selftests/iommu/iommufd_utils.h b/tools/testing/selftests/iommu/iommufd_utils.h index 307d097db9dd..ccd1f65df0b0 100644 --- a/tools/testing/selftests/iommu/iommufd_utils.h +++ b/tools/testing/selftests/iommu/iommufd_utils.h @@ -790,3 +790,30 @@ static int _test_cmd_viommu_alloc(int fd, __u32 device_id, __u32 hwpt_id, EXPECT_ERRNO(_errno, _test_cmd_viommu_alloc(self->fd, device_id, \ hwpt_id, type, 0, \ viommu_id)) + +static int _test_cmd_vdevice_alloc(int fd, __u32 viommu_id, __u32 idev_id, + __u64 virt_id, __u32 *vdev_id) +{ + struct iommu_vdevice_alloc cmd = { + .size = sizeof(cmd), + .dev_id = idev_id, + .viommu_id = viommu_id, + .virt_id = virt_id, + }; + int ret; + + ret = ioctl(fd, IOMMU_VDEVICE_ALLOC, &cmd); + if (ret) + return ret; + if (vdev_id) + *vdev_id = cmd.out_vdevice_id; + return 0; +} + +#define test_cmd_vdevice_alloc(viommu_id, idev_id, virt_id, vdev_id) \ + ASSERT_EQ(0, _test_cmd_vdevice_alloc(self->fd, viommu_id, idev_id, \ + virt_id, vdev_id)) +#define test_err_vdevice_alloc(_errno, viommu_id, idev_id, virt_id, vdev_id) \ + EXPECT_ERRNO(_errno, \ + _test_cmd_vdevice_alloc(self->fd, viommu_id, idev_id, \ + virt_id, vdev_id)) diff --git a/drivers/iommu/iommufd/selftest.c b/drivers/iommu/iommufd/selftest.c index 4fcf475facb1..87bc45b86f9e 100644 --- a/drivers/iommu/iommufd/selftest.c +++ b/drivers/iommu/iommufd/selftest.c @@ -136,6 +136,11 @@ struct mock_viommu { struct iommufd_viommu core; };

+struct mock_vdevice { + struct iommufd_vdevice core; + u64 rid; +}; + enum selftest_obj_type { TYPE_IDEV, }; @@ -560,8 +565,22 @@ static void mock_viommu_free(struct iommufd_viommu *viommu) /* iommufd core frees mock_viommu and viommu */ }

+static struct iommufd_vdevice *mock_vdevice_alloc(struct iommufd_viommu *viommu, + struct device *dev, u64 id) +{ + struct mock_vdevice *mock_vdev; + + mock_vdev = iommufd_vdevice_alloc(viommu->ictx, mock_vdevice, core); + if (IS_ERR(mock_vdev)) + return ERR_CAST(mock_vdev); + + mock_vdev->rid = id; + return &mock_vdev->core; +} + static struct iommufd_viommu_ops mock_viommu_ops = { .free = mock_viommu_free, + .vdevice_alloc = mock_vdevice_alloc, };

static struct iommufd_viommu * diff --git a/tools/testing/selftests/iommu/iommufd.c b/tools/testing/selftests/iommu/iommufd.c index c03705825576..af00b082656e 100644 --- a/tools/testing/selftests/iommu/iommufd.c +++ b/tools/testing/selftests/iommu/iommufd.c @@ -129,6 +129,7 @@ TEST_F(iommufd, cmd_length) TEST_LENGTH(iommu_option, IOMMU_OPTION, val64); TEST_LENGTH(iommu_vfio_ioas, IOMMU_VFIO_IOAS, __reserved); TEST_LENGTH(iommu_viommu_alloc, IOMMU_VIOMMU_ALLOC, out_viommu_id); + TEST_LENGTH(iommu_vdevice_alloc, IOMMU_VDEVICE_ALLOC, __reserved2); #undef TEST_LENGTH }

@@ -2470,4 +2471,23 @@ TEST_F(iommufd_viommu, viommu_auto_destroy) { }

+TEST_F(iommufd_viommu, vdevice_alloc) +{ + uint32_t viommu_id = self->viommu_id; + uint32_t dev_id = self->device_id; + uint32_t vdev_id = 0; + + if (dev_id) { + /* Set vdev_id to 0x99, unset it, and set to 0x88 */ + test_cmd_vdevice_alloc(viommu_id, dev_id, 0x99, &vdev_id); + test_err_vdevice_alloc(EEXIST, + viommu_id, dev_id, 0x99, &vdev_id); + test_ioctl_destroy(vdev_id); + test_cmd_vdevice_alloc(viommu_id, dev_id, 0x88, &vdev_id); + test_ioctl_destroy(vdev_id); + } else { + test_err_vdevice_alloc(ENOENT, viommu_id, dev_id, 0x99, NULL); + } +} + TEST_HARNESS_MAIN

Now cache entries for hwpt_nested can be invalidated via the vIOMMU's cache_invalidate op alternatively. Allow iommufd_hwpt_nested_alloc to support such a case.

Signed-off-by: Nicolin Chen nicolinc@nvidia.com --- drivers/iommu/iommufd/hw_pagetable.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/iommufd/hw_pagetable.c b/drivers/iommu/iommufd/hw_pagetable.c index b88a638d07da..ccaaf801955c 100644 --- a/drivers/iommu/iommufd/hw_pagetable.c +++ b/drivers/iommu/iommufd/hw_pagetable.c @@ -202,6 +202,17 @@ iommufd_hwpt_paging_alloc(struct iommufd_ctx *ictx, struct iommufd_ioas *ioas, return ERR_PTR(rc); }

+static inline bool +iommufd_hwpt_nested_has_invalidate_op(struct iommufd_hwpt_nested *hwpt_nested) +{ + struct iommufd_viommu *viommu = hwpt_nested->viommu; + + if (viommu) + return viommu->ops && viommu->ops->cache_invalidate; + else + return hwpt_nested->common.domain->ops->cache_invalidate_user; +} + /** * iommufd_hwpt_nested_alloc() - Get a NESTED iommu_domain for a device * @ictx: iommufd context @@ -257,7 +268,7 @@ iommufd_hwpt_nested_alloc(struct iommufd_ctx *ictx, hwpt->domain->owner = ops;

if (WARN_ON_ONCE(hwpt->domain->type != IOMMU_DOMAIN_NESTED || - !hwpt->domain->ops->cache_invalidate_user)) { + !iommufd_hwpt_nested_has_invalidate_op(hwpt_nested))) { rc = -EINVAL; goto out_abort; }

With a vIOMMU object, use space can flush any IOMMU related cache that can be directed using the vIOMMU object. It is similar to IOMMU_HWPT_INVALIDATE uAPI, but can cover a wider range than IOTLB, e.g. device/desciprtor cache.

Allow hwpt_id of the iommu_hwpt_invalidate structure to carry a viommu_id, and reuse the IOMMU_HWPT_INVALIDATE uAPI for vIOMMU invalidations. Drivers can define different structures for vIOMMU invalidations v.s. HWPT ones.

Update the uAPI, kdoc, and selftest case accordingly.

Reviewed-by: Jason Gunthorpe jgg@nvidia.com Signed-off-by: Nicolin Chen nicolinc@nvidia.com --- include/uapi/linux/iommufd.h | 9 ++++--- drivers/iommu/iommufd/hw_pagetable.c | 32 +++++++++++++++++++------ tools/testing/selftests/iommu/iommufd.c | 4 ++-- 3 files changed, 33 insertions(+), 12 deletions(-)

diff --git a/include/uapi/linux/iommufd.h b/include/uapi/linux/iommufd.h index 3039519d71b7..cd9e135ef563 100644 --- a/include/uapi/linux/iommufd.h +++ b/include/uapi/linux/iommufd.h @@ -728,7 +728,7 @@ struct iommu_hwpt_vtd_s1_invalidate { /** * struct iommu_hwpt_invalidate - ioctl(IOMMU_HWPT_INVALIDATE) * @size: sizeof(struct iommu_hwpt_invalidate) - * @hwpt_id: ID of a nested HWPT for cache invalidation + * @hwpt_id: ID of a nested HWPT or a vIOMMU, for cache invalidation * @data_uptr: User pointer to an array of driver-specific cache invalidation * data. * @data_type: One of enum iommu_hwpt_invalidate_data_type, defining the data @@ -739,8 +739,11 @@ struct iommu_hwpt_vtd_s1_invalidate { * Output the number of requests successfully handled by kernel. * @__reserved: Must be 0. * - * Invalidate the iommu cache for user-managed page table. Modifications on a - * user-managed page table should be followed by this operation to sync cache. + * Invalidate iommu cache for user-managed page table or vIOMMU. Modifications + * on a user-managed page table should be followed by this operation, if a HWPT + * is passed in via @hwpt_id. Other caches, such as device cache or descriptor + * cache can be flushed if a vIOMMU is passed in via the @hwpt_id field. + * * Each ioctl can support one or more cache invalidation requests in the array * that has a total size of @entry_len * @entry_num. * diff --git a/drivers/iommu/iommufd/hw_pagetable.c b/drivers/iommu/iommufd/hw_pagetable.c index ccaaf801955c..211b3f8b4366 100644 --- a/drivers/iommu/iommufd/hw_pagetable.c +++ b/drivers/iommu/iommufd/hw_pagetable.c @@ -444,7 +444,7 @@ int iommufd_hwpt_invalidate(struct iommufd_ucmd *ucmd) .entry_len = cmd->entry_len, .entry_num = cmd->entry_num, }; - struct iommufd_hw_pagetable *hwpt; + struct iommufd_object *pt_obj; u32 done_num = 0; int rc;

@@ -458,17 +458,35 @@ int iommufd_hwpt_invalidate(struct iommufd_ucmd *ucmd) goto out; }

- hwpt = iommufd_get_hwpt_nested(ucmd, cmd->hwpt_id); - if (IS_ERR(hwpt)) { - rc = PTR_ERR(hwpt); + pt_obj = iommufd_get_object(ucmd->ictx, cmd->hwpt_id, IOMMUFD_OBJ_ANY); + if (IS_ERR(pt_obj)) { + rc = PTR_ERR(pt_obj); goto out; } + if (pt_obj->type == IOMMUFD_OBJ_HWPT_NESTED) { + struct iommufd_hw_pagetable *hwpt = + container_of(pt_obj, struct iommufd_hw_pagetable, obj); + + rc = hwpt->domain->ops->cache_invalidate_user(hwpt->domain, + &data_array); + } else if (pt_obj->type == IOMMUFD_OBJ_VIOMMU) { + struct iommufd_viommu *viommu = + container_of(pt_obj, struct iommufd_viommu, obj); + + if (!viommu->ops || !viommu->ops->cache_invalidate) { + rc = -EOPNOTSUPP; + goto out_put_pt; + } + rc = viommu->ops->cache_invalidate(viommu, &data_array); + } else { + rc = -EINVAL; + goto out_put_pt; + }

- rc = hwpt->domain->ops->cache_invalidate_user(hwpt->domain, - &data_array); done_num = data_array.entry_num;

- iommufd_put_object(ucmd->ictx, &hwpt->obj); +out_put_pt: + iommufd_put_object(ucmd->ictx, pt_obj); out: cmd->entry_num = done_num; if (iommufd_ucmd_respond(ucmd, sizeof(*cmd))) diff --git a/tools/testing/selftests/iommu/iommufd.c b/tools/testing/selftests/iommu/iommufd.c index af00b082656e..2651e2b58423 100644 --- a/tools/testing/selftests/iommu/iommufd.c +++ b/tools/testing/selftests/iommu/iommufd.c @@ -362,9 +362,9 @@ TEST_F(iommufd_ioas, alloc_hwpt_nested) EXPECT_ERRNO(EBUSY, _test_ioctl_destroy(self->fd, parent_hwpt_id));

- /* hwpt_invalidate only supports a user-managed hwpt (nested) */ + /* hwpt_invalidate does not support a parent hwpt */ num_inv = 1; - test_err_hwpt_invalidate(ENOENT, parent_hwpt_id, inv_reqs, + test_err_hwpt_invalidate(EINVAL, parent_hwpt_id, inv_reqs, IOMMU_HWPT_INVALIDATE_DATA_SELFTEST, sizeof(*inv_reqs), &num_inv); assert(!num_inv);

From: Jason Gunthorpe jgg@nvidia.com

The iommu_copy_struct_from_user_array helper can be used to copy a single entry from a user array which might not be efficient if the array is big.

Add a new iommu_copy_struct_from_full_user_array to copy the entire user array at once. Update the existing iommu_copy_struct_from_user_array kdoc accordingly.

Signed-off-by: Jason Gunthorpe jgg@nvidia.com Signed-off-by: Nicolin Chen nicolinc@nvidia.com --- include/linux/iommu.h | 49 ++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 48 insertions(+), 1 deletion(-)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h index 1de2aebc4d92..c980fd0e2174 100644 --- a/include/linux/iommu.h +++ b/include/linux/iommu.h @@ -494,7 +494,9 @@ static inline int __iommu_copy_struct_from_user_array( * @index: Index to the location in the array to copy user data from * @min_last: The last member of the data structure @kdst points in the * initial version. - * Return 0 for success, otherwise -error. + * + * Copy a single entry from a user array. Return 0 for success, otherwise + * -error. */ #define iommu_copy_struct_from_user_array(kdst, user_array, data_type, index, \ min_last) \ @@ -502,6 +504,51 @@ static inline int __iommu_copy_struct_from_user_array( kdst, user_array, data_type, index, sizeof(*(kdst)), \ offsetofend(typeof(*(kdst)), min_last))

+ +/** + * iommu_copy_struct_from_full_user_array - Copy iommu driver specific user + * space data from an iommu_user_data_array + * @kdst: Pointer to an iommu driver specific user data that is defined in + * include/uapi/linux/iommufd.h + * @kdst_entry_size: sizeof(*kdst) + * @user_array: Pointer to a struct iommu_user_data_array for a user space + * array + * @data_type: The data type of the @kdst. Must match with @user_array->type + * + * Copy the entire user array. kdst must have room for kdst_entry_size * + * user_array->entry_num bytes. Return 0 for success, otherwise -error. + */ +static inline int +iommu_copy_struct_from_full_user_array(void *kdst, size_t kdst_entry_size, + struct iommu_user_data_array *user_array, + unsigned int data_type) +{ + unsigned int i; + int ret; + + if (user_array->type != data_type) + return -EINVAL; + if (!user_array->entry_num) + return -EINVAL; + if (likely(user_array->entry_len == kdst_entry_size)) { + if (copy_from_user(kdst, user_array->uptr, + user_array->entry_num * + user_array->entry_len)) + return -EFAULT; + } + + /* Copy item by item */ + for (i = 0; i != user_array->entry_num; i++) { + ret = copy_struct_from_user( + kdst + kdst_entry_size * i, kdst_entry_size, + user_array->uptr + user_array->entry_len * i, + user_array->entry_len); + if (ret) + return ret; + } + return 0; +} + /** * struct iommu_ops - iommu ops and capabilities * @capable: check capability

Similar to IOMMU_TEST_OP_MD_CHECK_IOTLB verifying a mock_domain's iotlb, IOMMU_TEST_OP_DEV_CHECK_CACHE will be used to verify a mock_dev's cache.

Signed-off-by: Nicolin Chen nicolinc@nvidia.com --- drivers/iommu/iommufd/iommufd_test.h | 5 ++++ tools/testing/selftests/iommu/iommufd_utils.h | 24 +++++++++++++++++++ drivers/iommu/iommufd/selftest.c | 24 +++++++++++++++++++ tools/testing/selftests/iommu/iommufd.c | 7 +++++- 4 files changed, 59 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/iommufd/iommufd_test.h b/drivers/iommu/iommufd/iommufd_test.h index 05f57a968d25..b226636aa07a 100644 --- a/drivers/iommu/iommufd/iommufd_test.h +++ b/drivers/iommu/iommufd/iommufd_test.h @@ -23,6 +23,7 @@ enum { IOMMU_TEST_OP_DIRTY, IOMMU_TEST_OP_MD_CHECK_IOTLB, IOMMU_TEST_OP_TRIGGER_IOPF, + IOMMU_TEST_OP_DEV_CHECK_CACHE, };

enum { @@ -140,6 +141,10 @@ struct iommu_test_cmd { __u32 perm; __u64 addr; } trigger_iopf; + struct { + __u32 id; + __u32 cache; + } check_dev_cache; }; __u32 last; }; diff --git a/tools/testing/selftests/iommu/iommufd_utils.h b/tools/testing/selftests/iommu/iommufd_utils.h index ccd1f65df0b0..b3da8c96a828 100644 --- a/tools/testing/selftests/iommu/iommufd_utils.h +++ b/tools/testing/selftests/iommu/iommufd_utils.h @@ -234,6 +234,30 @@ static int _test_cmd_hwpt_alloc(int fd, __u32 device_id, __u32 pt_id, __u32 ft_i test_cmd_hwpt_check_iotlb(hwpt_id, i, expected); \ })

+#define test_cmd_dev_check_cache(device_id, cache_id, expected) \ + ({ \ + struct iommu_test_cmd test_cmd = { \ + .size = sizeof(test_cmd), \ + .op = IOMMU_TEST_OP_DEV_CHECK_CACHE, \ + .id = device_id, \ + .check_dev_cache = { \ + .id = cache_id, \ + .cache = expected, \ + }, \ + }; \ + ASSERT_EQ(0, \ + ioctl(self->fd, \ + _IOMMU_TEST_CMD(IOMMU_TEST_OP_DEV_CHECK_CACHE),\ + &test_cmd)); \ + }) + +#define test_cmd_dev_check_cache_all(device_id, expected) \ + ({ \ + int c; \ + for (c = 0; c < MOCK_DEV_CACHE_NUM; c++) \ + test_cmd_dev_check_cache(device_id, c, expected); \ + }) + static int _test_cmd_hwpt_invalidate(int fd, __u32 hwpt_id, void *reqs, uint32_t data_type, uint32_t lreq, uint32_t *nreqs) diff --git a/drivers/iommu/iommufd/selftest.c b/drivers/iommu/iommufd/selftest.c index 8a1aef857922..ac3836c1dbcd 100644 --- a/drivers/iommu/iommufd/selftest.c +++ b/drivers/iommu/iommufd/selftest.c @@ -1106,6 +1106,26 @@ static int iommufd_test_md_check_iotlb(struct iommufd_ucmd *ucmd, return rc; }

+static int iommufd_test_dev_check_cache(struct iommufd_ucmd *ucmd, + u32 idev_id, unsigned int cache_id, + u32 cache) +{ + struct iommufd_device *idev; + struct mock_dev *mdev; + int rc = 0; + + idev = iommufd_get_device(ucmd, idev_id); + if (IS_ERR(idev)) + return PTR_ERR(idev); + mdev = container_of(idev->dev, struct mock_dev, dev); + + if (cache_id > MOCK_DEV_CACHE_ID_MAX || + mdev->cache[cache_id] != cache) + rc = -EINVAL; + iommufd_put_object(ucmd->ictx, &idev->obj); + return rc; +} + struct selftest_access { struct iommufd_access *access; struct file *file; @@ -1615,6 +1635,10 @@ int iommufd_test(struct iommufd_ucmd *ucmd) return iommufd_test_md_check_iotlb(ucmd, cmd->id, cmd->check_iotlb.id, cmd->check_iotlb.iotlb); + case IOMMU_TEST_OP_DEV_CHECK_CACHE: + return iommufd_test_dev_check_cache(ucmd, cmd->id, + cmd->check_dev_cache.id, + cmd->check_dev_cache.cache); case IOMMU_TEST_OP_CREATE_ACCESS: return iommufd_test_create_access(ucmd, cmd->id, cmd->create_access.flags); diff --git a/tools/testing/selftests/iommu/iommufd.c b/tools/testing/selftests/iommu/iommufd.c index 2651e2b58423..d5c5e3389182 100644 --- a/tools/testing/selftests/iommu/iommufd.c +++ b/tools/testing/selftests/iommu/iommufd.c @@ -222,6 +222,8 @@ FIXTURE_SETUP(iommufd_ioas) for (i = 0; i != variant->mock_domains; i++) { test_cmd_mock_domain(self->ioas_id, &self->stdev_id, &self->hwpt_id, &self->device_id); + test_cmd_dev_check_cache_all(self->device_id, + IOMMU_TEST_DEV_CACHE_DEFAULT); self->base_iova = MOCK_APERTURE_START; } } @@ -1386,9 +1388,12 @@ FIXTURE_SETUP(iommufd_mock_domain)

ASSERT_GE(ARRAY_SIZE(self->hwpt_ids), variant->mock_domains);

- for (i = 0; i != variant->mock_domains; i++) + for (i = 0; i != variant->mock_domains; i++) { test_cmd_mock_domain(self->ioas_id, &self->stdev_ids[i], &self->hwpt_ids[i], &self->idev_ids[i]); + test_cmd_dev_check_cache_all(self->idev_ids[0], + IOMMU_TEST_DEV_CACHE_DEFAULT); + } self->hwpt_id = self->hwpt_ids[0];

self->mmap_flags = MAP_SHARED | MAP_ANONYMOUS;

With the introduction of the new object and its infrastructure, update the doc and the vIOMMU graph to reflect that.

Signed-off-by: Nicolin Chen nicolinc@nvidia.com --- Documentation/userspace-api/iommufd.rst | 58 ++++++++++++++++++------- 1 file changed, 42 insertions(+), 16 deletions(-)

diff --git a/Documentation/userspace-api/iommufd.rst b/Documentation/userspace-api/iommufd.rst index 37eb1adda57b..3c27cc92c2cb 100644 --- a/Documentation/userspace-api/iommufd.rst +++ b/Documentation/userspace-api/iommufd.rst @@ -94,6 +94,19 @@ Following IOMMUFD objects are exposed to userspace: backed by corresponding vIOMMU objects, in which case a guest OS would do the "dispatch" naturally instead of VMM trappings.

+ - IOMMUFD_OBJ_VDEVICE, representing a virtual device for an IOMMUFD_OBJ_DEVICE + against an IOMMUFD_OBJ_VIOMMU. This virtual device holds the device's virtual + information or attributes (related to the vIOMMU) in a VM. An immediate vDATA + example can be the virtual ID of the device on a vIOMMU, which is a unique ID + that VMM assigns to the device for a translation channel/port of the vIOMMU, + e.g. vSID of ARM SMMUv3, vDeviceID of AMD IOMMU, and vID of Intel VT-d to a + Context Table. Potential use cases of some advanced security information can + be forwarded via this object too, such as security level or realm information + in a Confidential Compute Architecture. A VMM should create a vDEVICE object + to forward all the device information in a VM, when it connects a device to a + vIOMMU, which is a separate ioctl call from attaching the same device to an + HWPT_PAGING that the vIOMMU holds. + All user-visible objects are destroyed via the IOMMU_DESTROY uAPI.

The diagrams below show relationships between user-visible objects and kernel @@ -133,23 +146,26 @@ creating the objects and links:: |____________| |____________| |______|

_______________________________________________________________________ - | iommufd (with vIOMMU) | + | iommufd (with vIOMMU/vDEVICE) | | | - | [5] | - | _____________ | - | | | | - | [1] | vIOMMU | [4] [2] | - | ________________ | | _____________ ________ | - | | | | [3] | | | | | | - | | IOAS |<---|(HWPT_PAGING)|<---| HWPT_NESTED |<--| DEVICE | | - | |________________| |_____________| |_____________| |________| | - | | | | | | - |_________|____________________|__________________|_______________|_____| - | | | | - | ______v_____ ______v_____ ___v__ - | PFN storage | (paging) | | (nested) | |struct| - |------------>|iommu_domain|<----|iommu_domain|<----|device| - |____________| |____________| |______| + | [5] [6] | + | _____________ _____________ | + | | | | | | + | |----------------| vIOMMU |<---| vDEVICE |<----| | + | | | | |_____________| | | + | | | | | | + | | [1] | | [4] | [2] | + | | ______ | | _____________ _|______ | + | | | | | [3] | | | | | | + | | | IOAS |<---|(HWPT_PAGING)|<---| HWPT_NESTED |<--| DEVICE | | + | | |______| |_____________| |_____________| |________| | + | | | | | | | + |______|________|______________|__________________|_______________|_____| + | | | | | + ______v_____ | ______v_____ ______v_____ ___v__ + | struct | | PFN | (paging) | | (nested) | |struct| + |iommu_device| |------>|iommu_domain|<----|iommu_domain|<----|device| + |____________| storage|____________| |____________| |______|

1. IOMMUFD_OBJ_IOAS is created via the IOMMU_IOAS_ALLOC uAPI. An iommufd can hold multiple IOAS objects. IOAS is the most generic object and does not @@ -212,6 +228,15 @@ creating the objects and links:: the vIOMMU object and the HWPT_PAGING, then this vIOMMU object can be used as a nesting parent object to allocate an HWPT_NESTED object described above.

+6. IOMMUFD_OBJ_VDEVICE can be only manually created via the IOMMU_VDEVICE_ALLOC + uAPI, provided a viommu_id for an iommufd_viommu object and a dev_id for an + iommufd_device object. The vDEVICE object will be the binding between these + two parent objects. Another @virt_id will be also set via the uAPI providing + the iommufd core an index to store the vDEVICE object to a vDEVICE array per + vIOMMU. If necessary, the IOMMU driver may choose to implement a vdevce_alloc + op to init its HW for virtualization feature related to a vDEVICE. Successful + completion of this operation sets up the linkages between vIOMMU and device. + A device can only bind to an iommufd due to DMA ownership claim and attach to at most one IOAS object (no support of PASID yet).

@@ -225,6 +250,7 @@ User visible objects are backed by following datastructures: - iommufd_hwpt_paging for IOMMUFD_OBJ_HWPT_PAGING. - iommufd_hwpt_nested for IOMMUFD_OBJ_HWPT_NESTED. - iommufd_viommu for IOMMUFD_OBJ_VIOMMU. +- iommufd_vdevice for IOMMUFD_OBJ_VDEVICE

Several terminologies when looking at these datastructures:

Implement the vIOMMU's cache_invalidate op for user space to invalidate the IOTLB entries, Device ATS and CD entries that are still cached by hardware.

Add struct iommu_viommu_arm_smmuv3_invalidate defining invalidation entries that are simply in the native format of a 128-bit TLBI command. Scan those commands against the permitted command list and fix their VMID/SID fields.

Co-developed-by: Eric Auger eric.auger@redhat.com Signed-off-by: Eric Auger eric.auger@redhat.com Co-developed-by: Jason Gunthorpe jgg@nvidia.com Signed-off-by: Jason Gunthorpe jgg@nvidia.com Signed-off-by: Nicolin Chen nicolinc@nvidia.com --- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 5 + include/uapi/linux/iommufd.h | 24 ++++ .../arm/arm-smmu-v3/arm-smmu-v3-iommufd.c | 131 +++++++++++++++++- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 6 +- 4 files changed, 162 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h index 844d1dfdea55..000af931a30c 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h @@ -529,6 +529,7 @@ struct arm_smmu_cmdq_ent { #define CMDQ_OP_TLBI_NH_ALL 0x10 #define CMDQ_OP_TLBI_NH_ASID 0x11 #define CMDQ_OP_TLBI_NH_VA 0x12 + #define CMDQ_OP_TLBI_NH_VAA 0x13 #define CMDQ_OP_TLBI_EL2_ALL 0x20 #define CMDQ_OP_TLBI_EL2_ASID 0x21 #define CMDQ_OP_TLBI_EL2_VA 0x22 @@ -949,6 +950,10 @@ void arm_smmu_attach_commit(struct arm_smmu_attach_state *state); void arm_smmu_install_ste_for_dev(struct arm_smmu_master *master, const struct arm_smmu_ste *target);

+int arm_smmu_cmdq_issue_cmdlist(struct arm_smmu_device *smmu, + struct arm_smmu_cmdq *cmdq, u64 *cmds, int n, + bool sync); + #ifdef CONFIG_ARM_SMMU_V3_SVA bool arm_smmu_sva_supported(struct arm_smmu_device *smmu); bool arm_smmu_master_sva_supported(struct arm_smmu_master *master); diff --git a/include/uapi/linux/iommufd.h b/include/uapi/linux/iommufd.h index cd9e135ef563..d9e510ce67cf 100644 --- a/include/uapi/linux/iommufd.h +++ b/include/uapi/linux/iommufd.h @@ -684,9 +684,11 @@ struct iommu_hwpt_get_dirty_bitmap { * enum iommu_hwpt_invalidate_data_type - IOMMU HWPT Cache Invalidation * Data Type * @IOMMU_HWPT_INVALIDATE_DATA_VTD_S1: Invalidation data for VTD_S1 + * @IOMMU_VIOMMU_INVALIDATE_DATA_ARM_SMMUV3: Invalidation data for ARM SMMUv3 */ enum iommu_hwpt_invalidate_data_type { IOMMU_HWPT_INVALIDATE_DATA_VTD_S1 = 0, + IOMMU_VIOMMU_INVALIDATE_DATA_ARM_SMMUV3 = 1, };

/** @@ -725,6 +727,28 @@ struct iommu_hwpt_vtd_s1_invalidate { __u32 __reserved; };

+/** + * struct iommu_viommu_arm_smmuv3_invalidate - ARM SMMUv3 cahce invalidation + * (IOMMU_VIOMMU_INVALIDATE_DATA_ARM_SMMUV3) + * @cmd: 128-bit cache invalidation command that runs in SMMU CMDQ. + * Must be little-endian. + * + * Supported command list only when passing in a vIOMMU via @hwpt_id: + * CMDQ_OP_TLBI_NSNH_ALL + * CMDQ_OP_TLBI_NH_VA + * CMDQ_OP_TLBI_NH_VAA + * CMDQ_OP_TLBI_NH_ALL + * CMDQ_OP_TLBI_NH_ASID + * CMDQ_OP_ATC_INV + * CMDQ_OP_CFGI_CD + * CMDQ_OP_CFGI_CD_ALL + * + * -EIO will be returned if the command is not supported. + */ +struct iommu_viommu_arm_smmuv3_invalidate { + __aligned_le64 cmd[2]; +}; + /** * struct iommu_hwpt_invalidate - ioctl(IOMMU_HWPT_INVALIDATE) * @size: sizeof(struct iommu_hwpt_invalidate) diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c index 5e235fca8f13..1b82579eb252 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c @@ -191,6 +191,14 @@ arm_smmu_domain_alloc_nesting(struct device *dev, u32 flags, if (smmu_parent->smmu != master->smmu) return ERR_PTR(-EINVAL);

+ /* + * FORCE_SYNC is not set with FEAT_NESTING. Some study of the exact HW + * defect is needed to determine if arm_vsmmu_cache_invalidate() needs + * any change to remove this. + */ + if (WARN_ON(master->smmu->options & ARM_SMMU_OPT_CMDQ_FORCE_SYNC)) + return ERR_PTR(-EOPNOTSUPP); + ret = iommu_copy_struct_from_user(&arg, user_data, IOMMU_HWPT_DATA_ARM_SMMUV3, ste); if (ret) @@ -213,6 +221,127 @@ arm_smmu_domain_alloc_nesting(struct device *dev, u32 flags, return &nested_domain->domain; }

+static int arm_vsmmu_vsid_to_sid(struct arm_vsmmu *vsmmu, u32 vsid, u32 *sid) +{ + XA_STATE(xas, &vsmmu->core.vdevs, (unsigned long)vsid); + struct arm_smmu_master *master; + struct device *dev; + int ret = 0; + + xa_lock(&vsmmu->core.vdevs); + dev = vdev_to_dev(xas_load(&xas)); + if (!dev) { + ret = -EIO; + goto unlock; + } + master = dev_iommu_priv_get(dev); + + /* At this moment, iommufd only supports PCI device that has one SID */ + if (sid) + *sid = master->streams[0].id; +unlock: + xa_unlock(&vsmmu->core.vdevs); + return ret; +} + +/* + * Convert, in place, the raw invalidation command into an internal format that + * can be passed to arm_smmu_cmdq_issue_cmdlist(). Internally commands are + * stored in CPU endian. + * + * Enforce the VMID or SID on the command. + */ +static int +arm_vsmmu_convert_user_cmd(struct arm_vsmmu *vsmmu, + struct iommu_viommu_arm_smmuv3_invalidate *cmd) +{ + cmd->cmd[0] = le64_to_cpu(cmd->cmd[0]); + cmd->cmd[1] = le64_to_cpu(cmd->cmd[1]); + + switch (cmd->cmd[0] & CMDQ_0_OP) { + case CMDQ_OP_TLBI_NSNH_ALL: + /* Convert to NH_ALL */ + cmd->cmd[0] = CMDQ_OP_TLBI_NH_ALL | + FIELD_PREP(CMDQ_TLBI_0_VMID, vsmmu->vmid); + cmd->cmd[1] = 0; + break; + case CMDQ_OP_TLBI_NH_VA: + case CMDQ_OP_TLBI_NH_VAA: + case CMDQ_OP_TLBI_NH_ALL: + case CMDQ_OP_TLBI_NH_ASID: + cmd->cmd[0] &= ~CMDQ_TLBI_0_VMID; + cmd->cmd[0] |= FIELD_PREP(CMDQ_TLBI_0_VMID, vsmmu->vmid); + break; + case CMDQ_OP_ATC_INV: + case CMDQ_OP_CFGI_CD: + case CMDQ_OP_CFGI_CD_ALL: + u32 sid, vsid = FIELD_GET(CMDQ_CFGI_0_SID, cmd->cmd[0]); + + if (arm_vsmmu_vsid_to_sid(vsmmu, vsid, &sid)) + return -EIO; + cmd->cmd[0] &= ~CMDQ_CFGI_0_SID; + cmd->cmd[0] |= FIELD_PREP(CMDQ_CFGI_0_SID, sid); + break; + default: + return -EIO; + } + return 0; +} + +static int arm_vsmmu_cache_invalidate(struct iommufd_viommu *viommu, + struct iommu_user_data_array *array) +{ + struct arm_vsmmu *vsmmu = container_of(viommu, struct arm_vsmmu, core); + struct iommu_viommu_arm_smmuv3_invalidate *last; + struct iommu_viommu_arm_smmuv3_invalidate *cmds; + struct iommu_viommu_arm_smmuv3_invalidate *cur; + struct iommu_viommu_arm_smmuv3_invalidate *end; + struct arm_smmu_device *smmu = vsmmu->smmu; + int ret; + + cmds = kcalloc(array->entry_num, sizeof(*cmds), GFP_KERNEL); + if (!cmds) + return -ENOMEM; + cur = cmds; + end = cmds + array->entry_num; + + static_assert(sizeof(*cmds) == 2 * sizeof(u64)); + ret = iommu_copy_struct_from_full_user_array( + cmds, sizeof(*cmds), array, + IOMMU_VIOMMU_INVALIDATE_DATA_ARM_SMMUV3); + if (ret) + goto out; + + last = cmds; + while (cur != end) { + ret = arm_vsmmu_convert_user_cmd(vsmmu, cur); + if (ret) + goto out; + + /* FIXME work in blocks of CMDQ_BATCH_ENTRIES and copy each block? */ + cur++; + if (cur != end && (cur - last) != CMDQ_BATCH_ENTRIES - 1) + continue; + + /* FIXME always uses the main cmdq rather than trying to group by type */ + ret = arm_smmu_cmdq_issue_cmdlist(smmu, &smmu->cmdq, last->cmd, + cur - last, true); + if (ret) { + cur--; + goto out; + } + last = cur; + } +out: + array->entry_num = cur - cmds; + kfree(cmds); + return ret; +} + +const struct iommufd_viommu_ops arm_vsmmu_ops = { + .cache_invalidate = arm_vsmmu_cache_invalidate, +}; + struct iommufd_viommu * arm_vsmmu_alloc(struct iommu_device *iommu_dev, struct iommu_domain *parent, struct iommufd_ctx *ictx, unsigned int viommu_type) @@ -225,7 +354,7 @@ arm_vsmmu_alloc(struct iommu_device *iommu_dev, struct iommu_domain *parent, if (viommu_type != IOMMU_VIOMMU_TYPE_ARM_SMMUV3) return ERR_PTR(-EOPNOTSUPP);

- vsmmu = iommufd_viommu_alloc(ictx, arm_vsmmu, core, NULL); + vsmmu = iommufd_viommu_alloc(ictx, arm_vsmmu, core, &arm_vsmmu_ops); if (IS_ERR(vsmmu)) return ERR_CAST(vsmmu);

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c index 6a23e6dcd5cf..a2bbd140e232 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -766,9 +766,9 @@ static void arm_smmu_cmdq_write_entries(struct arm_smmu_cmdq *cmdq, u64 *cmds, * insert their own list of commands then all of the commands from one * CPU will appear before any of the commands from the other CPU. */ -static int arm_smmu_cmdq_issue_cmdlist(struct arm_smmu_device *smmu, - struct arm_smmu_cmdq *cmdq, - u64 *cmds, int n, bool sync) +int arm_smmu_cmdq_issue_cmdlist(struct arm_smmu_device *smmu, + struct arm_smmu_cmdq *cmdq, u64 *cmds, int n, + bool sync) { u64 cmd_sync[CMDQ_ENT_DWORDS]; u32 prod;

From: Jason Gunthorpe jgg@nvidia.com

The SMMUv3 spec has a note that BYPASS and ATS don't work together under the STE EATS field definition. However there is another section "13.6.4 Full ATS skipping stage 1" that explains under certain conditions BYPASS and ATS do work together if the STE is using S1DSS to select BYPASS and the CD table has the possibility for a substream.

When these comments were written the understanding was that all forms of BYPASS just didn't work and this was to be a future problem to solve.

It turns out that ATS and IDENTITY will always work just fine:

- If STE.Config = BYPASS then the PCI ATS is disabled

- If a PASID domain is attached then S1DSS = BYPASS and ATS will be enabled. This meets the requirements of 13.6.4 to automatically generate 1:1 ATS replies on the RID.

Update the comments to reflect this.

Signed-off-by: Jason Gunthorpe jgg@nvidia.com Signed-off-by: Nicolin Chen nicolinc@nvidia.com --- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c index 1cb4afe7a90a..236f930f9a97 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -2745,9 +2745,14 @@ int arm_smmu_attach_prepare(struct arm_smmu_attach_state *state, * Translation Requests and Translated transactions are denied * as though ATS is disabled for the stream (STE.EATS == 0b00), * causing F_BAD_ATS_TREQ and F_TRANSL_FORBIDDEN events - * (IHI0070Ea 5.2 Stream Table Entry). Thus ATS can only be - * enabled if we have arm_smmu_domain, those always have page - * tables. + * (IHI0070Ea 5.2 Stream Table Entry). + * + * However, if we have installed a CD table and are using S1DSS + * then ATS will work in S1DSS bypass. See "13.6.4 Full ATS + * skipping stage 1". + * + * Disable ATS if we are going to create a normal 0b100 bypass + * STE. */ state->ats_enabled = !state->disable_ats && arm_smmu_ats_supported(master); @@ -3072,8 +3077,10 @@ static void arm_smmu_attach_dev_ste(struct iommu_domain *domain, if (arm_smmu_ssids_in_use(&master->cd_table)) { /* * If a CD table has to be present then we need to run with ATS - * on even though the RID will fail ATS queries with UR. This is - * because we have no idea what the PASID's need. + * on because we have to assume a PASID is using ATS. For + * IDENTITY this will setup things so that S1DSS=bypass which + * follows the explanation in "13.6.4 Full ATS skipping stage 1" + * and allows for ATS on the RID to work. */ state.cd_needs_ats = true; arm_smmu_attach_prepare(&state, domain);