Re: [PATCH v3 05/15] iommufd: File descriptor, context, kconfig and makefiles

From: Jason Gunthorpe <jgg@nvidia.com>
To: "Tian, Kevin" <kevin.tian@intel.com>
Cc: Lu Baolu <baolu.lu@linux.intel.com>,
	"bpf@vger.kernel.org" <bpf@vger.kernel.org>,
	Jonathan Corbet <corbet@lwn.net>,
	David Woodhouse <dwmw2@infradead.org>,
	"iommu@lists.linux.dev" <iommu@lists.linux.dev>,
	Joerg Roedel <joro@8bytes.org>,
	"linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>,
	"linux-kselftest@vger.kernel.org"
	<linux-kselftest@vger.kernel.org>,
	"llvm@lists.linux.dev" <llvm@lists.linux.dev>,
	Nathan Chancellor <nathan@kernel.org>,
	Nick Desaulniers <ndesaulniers@google.com>,
	Miguel Ojeda <ojeda@kernel.org>,
	Robin Murphy <robin.murphy@arm.com>,
	Shuah Khan <shuah@kernel.org>,
	Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>,
	Tom Rix <trix@redhat.com>, Will Deacon <will@kernel.org>,
	Alex Williamson <alex.williamson@redhat.com>,
	Chaitanya Kulkarni <chaitanyak@nvidia.com>,
	Cornelia Huck <cohuck@redhat.com>,
	Daniel Jordan <daniel.m.jordan@oracle.com>,
	David Gibson <david@gibson.dropbear.id.au>,
	Eric Auger <eric.auger@redhat.com>,
	Eric Farman <farman@linux.ibm.com>,
	Jason Wang <jasowang@redhat.com>,
	Jean-Philippe Brucker <jean-philippe@linaro.org>,
	"Martins, Joao" <joao.m.martins@oracle.com>,
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	Matthew Rosato <mjrosato@linux.ibm.com>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	Nicolin Chen <nicolinc@nvidia.com>,
	Niklas Schnelle <schnelle@linux.ibm.com>,
	Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>,
	"Liu, Yi L" <yi.l.liu@intel.com>,
	Keqian Zhu <zhukeqian1@huawei.com>
Subject: Re: [PATCH v3 05/15] iommufd: File descriptor, context, kconfig and makefiles
Date: Mon, 7 Nov 2022 13:00:52 -0400	[thread overview]
Message-ID: <Y2k5xIp3TksL94MV@nvidia.com> (raw)
In-Reply-To: <BN9PR11MB527695C6CE5BEA4AFACF3DBB8C389@BN9PR11MB5276.namprd11.prod.outlook.com>

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 subsystem
> > as
> > +	  it relates to managing IO page tables that point at user space
> > memory.
> > +
> > +	  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 