On Thu, Nov 03, 2022 at 07:22:11AM +0000, Tian, Kevin wrote:
From: Jason Gunthorpe jgg@nvidia.com Sent: Wednesday, October 26, 2022 2:12 AM
--- a/MAINTAINERS +++ b/MAINTAINERS @@ -10714,6 +10714,16 @@ F: drivers/iommu/dma-iommu.h F: drivers/iommu/iova.c F: include/linux/iova.h
+IOMMU FD
remove the space, i.e. IOMMUFD
OK
+config IOMMUFD
- tristate "IOMMU Userspace API"
- select INTERVAL_TREE
- select INTERVAL_TREE_SPAN_ITER
- select IOMMU_API
- default n
- help
Provides /dev/iommu the user API to control the IOMMU subsystemas
it relates to managing IO page tables that point at user spacememory.
This would commonly be used in combination with VFIO.remove this line
Sure
+/**
- iommufd_put_object_keep_user() - Release part of the refcount on obj
what does 'part of the refcount' mean?
- @obj - Object to release
- Objects have two protections to ensure that userspace has a consistent
- experience with destruction. Normally objects are locked so that destroy
will
- block while there are concurrent users, and wait for the object to be
- unlocked.
- However, destroy can also be blocked by holding users reference counts
on the
- objects, in that case destroy will immediately return EBUSY and will not
wait
- for reference counts to go to zero.
- This function switches from blocking userspace to returning EBUSY.
Not sure where "switch from... to..." comes from. Also this function alone doesn't deal anything with EBUSY. Probably it is clearer that this interface is used for long-term refcounting which the destroy path should favor to not block as it did for transient refcounting in concurrent ioctl paths.
How about this:
/** * iommufd_ref_to_users() - Switch from destroy_rwsem to users refcount * protection * @obj - Object to release * * Objects have two refcount protections (destroy_rwsem and the refcount_t * users). Holding either of these will prevent the object from being destroyed. * * Depending on the use case, one protection or the other is appropriate. In * most cases references are being protected by the destroy_rwsem. This allows * orderly destruction of the object becuase iommufd_object_destroy_user() will * wait for it to become unlocked. However, as a rwsem, it cannot be held across * a system call return. So cases that have longer term needs must switch * to the weaker users refcount_t. * * With users protection iommufd_object_destroy_user() will return -EBUSY to * userspace and refuse to destroy the object. */
With a function rename.
- It should be used in places where the users will be held beyond a single
- system call.
'users' or 'external drivers'? Do we ever allow userspace to hold the lock of a kernel object for undefined time?
"users" is the name of the variable and the refcount
We don't allow userspace to hold a lock, but we do allow it to hold a refcount. And the refcount is defined so that it must be unheld internally in the kernel when the fd is released. Eg this is part of what the access_ops unmap callback is doing.
+++ b/drivers/iommu/iommufd/main.c @@ -0,0 +1,345 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* Copyright (C) 2021 Intel Corporation
- Copyright (c) 2021-2022, NVIDIA CORPORATION & AFFILIATES
- iommufd provides control over the IOMMU HW objects created by
IOMMU kernel
- drivers. IOMMU HW objects revolve around IO page tables that map
incoming DMA
- addresses (IOVA) to CPU addresses.
"to bus addresses".
CPU address is correct, these are all phys_addr_t's. The IOVA is the "bus address" in kernel terminology.
- The API is divided into a general portion that is intended to work with any
- kernel IOMMU driver, and a device specific portion that is intended to be
- used with a userspace HW driver paired with the specific kernel driver.
This
- mechanism allows all the unique functionalities in individual IOMMUs to
be
- exposed to userspace control.
there is no device specific portion in this series.
Lets drop the paragraph
+/*
- Allow concurrent access to the object. This should only be done once the
- system call that created the object is guaranteed to succeed.
an object is not always created by a system call, e.g. iommufd_access
Yes, but that is actually a special case, "iommufd_access" is a "leaf" object that doesn't have any way for userspace to block it. Something like that can always be destroyed, indeed we require it as part of the drive facing API. So this guidance isn't aimed at that.
How about:
/* * Allow concurrent access to the object. * * Once another thread can see the object pointer it can prevent object * destruction. Expect for special kernel-only objects there is no in-kernel way * to reliably destroy a single object. Thus all APIs that are creating objects * must use iommufd_object_abort() to handle their errors and only call * iommufd_object_finalize() once object creation cannot fail. */
+static int iommufd_destroy(struct iommufd_ucmd *ucmd) +{
- struct iommu_destroy *cmd = ucmd->cmd;
- struct iommufd_object *obj;
- obj = iommufd_get_object(ucmd->ictx, cmd->id,
IOMMUFD_OBJ_ANY);
- if (IS_ERR(obj))
return PTR_ERR(obj);- iommufd_put_object_keep_user(obj);
- if (!iommufd_object_destroy_user(ucmd->ictx, obj))
return -EBUSY;Add a comment that it implies a refcnt hold by external driver in a long time so return error instead of blocking...
Lets do "See iommufd_ref_to_users"
- nr = _IOC_NR(cmd);
- if (nr < IOMMUFD_CMD_BASE ||
(nr - IOMMUFD_CMD_BASE) >= ARRAY_SIZE(iommufd_ioctl_ops))return -ENOIOCTLCMD;According to the description in iommufd.h:
- ENOTTY: The IOCTL number itself is not supported at all
Yes, it is weird, but in ioctl handlers you return ENOTTY by returning ENOIOCTLCMD which is then converted to ENOTTY in vfs_ioctl()
- op = &iommufd_ioctl_ops[nr - IOMMUFD_CMD_BASE];
- if (op->ioctl_num != cmd)
return -ENOIOCTLCMD;- if (ucmd.user_size < op->min_size)
return -EOPNOTSUPP;-EINVAL?
Yes
+/**
- DOC: General ioctl format
- The ioctl mechanims follows a general format to allow for extensibility.
mechanism
It was changed to "interface" in another comment
Thanks, Jason