Received-SPF: pass (google.com: domain of linux-kernel+bounces-73128-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Tue, 20 Feb 2024 14:57:52 +0100
From: Joel Granados <j.granados@samsung.com>
To: Lu Baolu <baolu.lu@linux.intel.com>
CC: Jason Gunthorpe <jgg@ziepe.ca>, Kevin Tian <kevin.tian@intel.com>, Joerg
	Roedel <joro@8bytes.org>, Will Deacon <will@kernel.org>, Robin Murphy
	<robin.murphy@arm.com>, Jean-Philippe Brucker <jean-philippe@linaro.org>,
	Nicolin Chen <nicolinc@nvidia.com>, Yi Liu <yi.l.liu@intel.com>, Jacob Pan
	<jacob.jun.pan@linux.intel.com>, <iommu@lists.linux.dev>,
	<virtualization@lists.linux-foundation.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v3 6/8] iommufd: IOPF-capable hw page table
 attach/detach/replace
Message-ID: <20240220135752.vksznb4rdj73ln6c@joelS2.panther.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg="pgp-sha512";
	protocol="application/pgp-signature"; boundary="pf2tgjfb6gzecilc"
Content-Disposition: inline
In-Reply-To: <20240122073903.24406-7-baolu.lu@linux.intel.com>
CMS-TYPE: 201P
References: <20240122073903.24406-1-baolu.lu@linux.intel.com>
	<20240122073903.24406-7-baolu.lu@linux.intel.com>
	<CGME20240220135755eucas1p24e882cb5a735caed4bbfedb22a811442@eucas1p2.samsung.com>

--pf2tgjfb6gzecilc
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Mon, Jan 22, 2024 at 03:39:01PM +0800, Lu Baolu wrote:
> The iopf-capable hw page table attach/detach/replace should use the iommu
> iopf-specific interfaces. The pointer to iommufd_device is stored in the
> private field of the attachment cookie, so that it can be easily retrieved
> in the fault handling paths. The references to iommufd_device and
> iommufd_hw_pagetable objects are held until the cookie is released, which
> happens after the hw_pagetable is detached from the device and all
> outstanding iopf's are responded to. This guarantees that both the device
> and hw_pagetable are valid before domain detachment and outstanding faults
> are handled.
>=20
> The iopf-capable hw page tables can only be attached to devices that
> support the IOMMU_DEV_FEAT_IOPF feature. On the first attachment of an
> iopf-capable hw_pagetable to the device, the IOPF feature is enabled on
> the device. Similarly, after the last iopf-capable hwpt is detached from
> the device, the IOPF feature is disabled on the device.
>=20
> The current implementation allows a replacement between iopf-capable and
> non-iopf-capable hw page tables. This matches the nested translation use
> case, where a parent domain is attached by default and can then be
> replaced with a nested user domain with iopf support.
>=20
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/iommufd/iommufd_private.h |   7 ++
>  drivers/iommu/iommufd/device.c          |  15 ++-
>  drivers/iommu/iommufd/fault.c           | 122 ++++++++++++++++++++++++
>  3 files changed, 141 insertions(+), 3 deletions(-)
>=20
> diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iomm=
ufd/iommufd_private.h
> index 2780bed0c6b1..9844a1289c01 100644
> --- a/drivers/iommu/iommufd/iommufd_private.h
> +++ b/drivers/iommu/iommufd/iommufd_private.h
> @@ -398,6 +398,7 @@ struct iommufd_device {
>  	/* always the physical device */
>  	struct device *dev;
>  	bool enforce_cache_coherency;
> +	bool iopf_enabled;
>  	/* outstanding faults awaiting response indexed by fault group id */
>  	struct xarray faults;
>  };
> @@ -459,6 +460,12 @@ iommufd_get_fault(struct iommufd_ucmd *ucmd, u32 id)
>  int iommufd_fault_alloc(struct iommufd_ucmd *ucmd);
>  void iommufd_fault_destroy(struct iommufd_object *obj);
>  int iommufd_fault_iopf_handler(struct iopf_group *group);
> +int iommufd_fault_domain_attach_dev(struct iommufd_hw_pagetable *hwpt,
> +				    struct iommufd_device *idev);
> +void iommufd_fault_domain_detach_dev(struct iommufd_hw_pagetable *hwpt,
> +				     struct iommufd_device *idev);
> +int iommufd_fault_domain_replace_dev(struct iommufd_hw_pagetable *hwpt,
> +				     struct iommufd_device *idev);
> =20
>  #ifdef CONFIG_IOMMUFD_TEST
>  int iommufd_test(struct iommufd_ucmd *ucmd);
> diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/devic=
e.c
> index d70913ee8fdf..c4737e876ebc 100644
> --- a/drivers/iommu/iommufd/device.c
> +++ b/drivers/iommu/iommufd/device.c
> @@ -377,7 +377,10 @@ int iommufd_hw_pagetable_attach(struct iommufd_hw_pa=
getable *hwpt,
>  	 * attachment.
>  	 */
>  	if (list_empty(&idev->igroup->device_list)) {
> -		rc =3D iommu_attach_group(hwpt->domain, idev->igroup->group);
> +		if (hwpt->fault_capable)
> +			rc =3D iommufd_fault_domain_attach_dev(hwpt, idev);
> +		else
> +			rc =3D iommu_attach_group(hwpt->domain, idev->igroup->group);
>  		if (rc)
>  			goto err_unresv;
>  		idev->igroup->hwpt =3D hwpt;
> @@ -403,7 +406,10 @@ iommufd_hw_pagetable_detach(struct iommufd_device *i=
dev)
>  	mutex_lock(&idev->igroup->lock);
>  	list_del(&idev->group_item);
>  	if (list_empty(&idev->igroup->device_list)) {
> -		iommu_detach_group(hwpt->domain, idev->igroup->group);
> +		if (hwpt->fault_capable)
> +			iommufd_fault_domain_detach_dev(hwpt, idev);
> +		else
> +			iommu_detach_group(hwpt->domain, idev->igroup->group);
>  		idev->igroup->hwpt =3D NULL;
>  	}
>  	if (hwpt_is_paging(hwpt))
> @@ -498,7 +504,10 @@ iommufd_device_do_replace(struct iommufd_device *ide=
v,
>  			goto err_unlock;
>  	}
> =20
> -	rc =3D iommu_group_replace_domain(igroup->group, hwpt->domain);
> +	if (old_hwpt->fault_capable || hwpt->fault_capable)
> +		rc =3D iommufd_fault_domain_replace_dev(hwpt, idev);
> +	else
> +		rc =3D iommu_group_replace_domain(igroup->group, hwpt->domain);
>  	if (rc)
>  		goto err_unresv;
> =20
> diff --git a/drivers/iommu/iommufd/fault.c b/drivers/iommu/iommufd/fault.c
> index e752d1c49dde..a4a49f3cd4c2 100644
> --- a/drivers/iommu/iommufd/fault.c
> +++ b/drivers/iommu/iommufd/fault.c
> @@ -267,3 +267,125 @@ int iommufd_fault_iopf_handler(struct iopf_group *g=
roup)
> =20
>  	return 0;
>  }
> +
> +static void release_attach_cookie(struct iopf_attach_cookie *cookie)
> +{
> +	struct iommufd_hw_pagetable *hwpt =3D cookie->domain->fault_data;
There is a possibility here of cookie->domain being NULL. When you call
release_attach_cookie from iommufd_fault_domain_attach_dev if
idev->iopf_enabled is false. In this case, you have not set the domain
yet.

> +	struct iommufd_device *idev =3D cookie->private;
> +
> +	refcount_dec(&idev->obj.users);
> +	refcount_dec(&hwpt->obj.users);
You should decrease this ref count only if the cookie actually had a
domain.

This function could be something like this:

	static void release_attach_cookie(struct iopf_attach_cookie *cookie)
	{
		struct iommufd_hw_pagetable *hwpt;
		struct iommufd_device *idev =3D cookie->private;

		refcount_dec(&idev->obj.users);
		if (cookie->domain) {
			hwpt =3D cookie->domain->fault_data;
			refcount_dec(&hwpt->obj.users);
		}
		kfree(cookie);
	}


> +	kfree(cookie);
> +}
> +
> +static int iommufd_fault_iopf_enable(struct iommufd_device *idev)
> +{
> +	int ret;
> +
> +	if (idev->iopf_enabled)
> +		return 0;
> +
> +	ret =3D iommu_dev_enable_feature(idev->dev, IOMMU_DEV_FEAT_IOPF);
> +	if (ret)
> +		return ret;
> +
> +	idev->iopf_enabled =3D true;
> +
> +	return 0;
> +}
> +
> +static void iommufd_fault_iopf_disable(struct iommufd_device *idev)
> +{
> +	if (!idev->iopf_enabled)
> +		return;
> +
> +	iommu_dev_disable_feature(idev->dev, IOMMU_DEV_FEAT_IOPF);
> +	idev->iopf_enabled =3D false;
> +}
> +
> +int iommufd_fault_domain_attach_dev(struct iommufd_hw_pagetable *hwpt,
> +				    struct iommufd_device *idev)
> +{
> +	struct iopf_attach_cookie *cookie;
> +	int ret;
> +
> +	cookie =3D kzalloc(sizeof(*cookie), GFP_KERNEL);
> +	if (!cookie)
> +		return -ENOMEM;
> +
> +	refcount_inc(&hwpt->obj.users);
> +	refcount_inc(&idev->obj.users);
> +	cookie->release =3D release_attach_cookie;
> +	cookie->private =3D idev;
> +
> +	if (!idev->iopf_enabled) {
> +		ret =3D iommufd_fault_iopf_enable(idev);
> +		if (ret)
> +			goto out_put_cookie;
You have not set domain here and release_attach_cookie will try to
access a null address.

> +	}
> +
> +	ret =3D iopf_domain_attach(hwpt->domain, idev->dev, IOMMU_NO_PASID, coo=
kie);
> +	if (ret)
> +		goto out_disable_iopf;
> +
> +	return 0;
> +out_disable_iopf:
> +	iommufd_fault_iopf_disable(idev);
> +out_put_cookie:
> +	release_attach_cookie(cookie);
> +
> +	return ret;
> +}
> +
> +void iommufd_fault_domain_detach_dev(struct iommufd_hw_pagetable *hwpt,
> +				     struct iommufd_device *idev)
> +{
> +	iopf_domain_detach(hwpt->domain, idev->dev, IOMMU_NO_PASID);
> +	iommufd_fault_iopf_disable(idev);
> +}
> +
> +int iommufd_fault_domain_replace_dev(struct iommufd_hw_pagetable *hwpt,
> +				     struct iommufd_device *idev)
> +{
> +	bool iopf_enabled_originally =3D idev->iopf_enabled;
> +	struct iopf_attach_cookie *cookie =3D NULL;
> +	int ret;
> +
> +	if (hwpt->fault_capable) {
> +		cookie =3D kzalloc(sizeof(*cookie), GFP_KERNEL);
> +		if (!cookie)
> +			return -ENOMEM;
> +
> +		refcount_inc(&hwpt->obj.users);
> +		refcount_inc(&idev->obj.users);
> +		cookie->release =3D release_attach_cookie;
> +		cookie->private =3D idev;
> +
> +		if (!idev->iopf_enabled) {
> +			ret =3D iommufd_fault_iopf_enable(idev);
> +			if (ret) {
> +				release_attach_cookie(cookie);
> +				return ret;
> +			}
> +		}
> +	}
> +
> +	ret =3D iopf_domain_replace(hwpt->domain, idev->dev, IOMMU_NO_PASID, co=
okie);
> +	if (ret) {
> +		goto out_put_cookie;
> +	}
> +
> +	if (iopf_enabled_originally && !hwpt->fault_capable)
> +		iommufd_fault_iopf_disable(idev);
> +
> +	return 0;
> +out_put_cookie:
> +	if (hwpt->fault_capable)
> +		release_attach_cookie(cookie);
> +	if (iopf_enabled_originally && !idev->iopf_enabled)
> +		iommufd_fault_iopf_enable(idev);
> +	else if (!iopf_enabled_originally && idev->iopf_enabled)
> +		iommufd_fault_iopf_disable(idev);
> +
> +	return ret;
> +}
> --=20
> 2.34.1
>=20

--=20

Joel Granados

--pf2tgjfb6gzecilc
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQGzBAABCgAdFiEErkcJVyXmMSXOyyeQupfNUreWQU8FAmXUr94ACgkQupfNUreW
QU+ZOQv/Wip7KOIrGYyBLCBQ30IEYCwyj195c62gJ76/wxu+HvwnyJowrYzU0ub2
LODfpdTmIcJusUFIiJFWMFmBYThJMgEE1UqjPhItqKXR0bZ4ALPxXZNrj3pLCB7V
E1g0K1Sq0Mvf11nAm97b4pv4CSXIfNoNezBucK0B34cX3qGcTXRz1UNaEFDYs4ZH
b+Ms9REbV3nlmZxssOycD+Q0pG13yGIzg5N3SEMSrCW70VK95kZZDUWSd1eGC7iY
uX6DLWilTLYz97BbrBGaoXX0VQXUoO0bdI31ye9W76UE20Dn8BDQmOLFciCqNfWT
MhyFihjGmMbJckIY1qPQq7dsucQ4D1VF6G5ckTgVGyI9xZjW61qz+0UhW2RIj1Nt
TJrwGxL99zIkeg5VxObdHQxq7uxX5UNAi4LFNroiLc2VWuHvrB4dmghEZS4C0aPk
2vtLYqDaPZzsRscRK5e7mF9VlcQlmmZyjr8vApF/mOkWiDYQCq6BKXW9g84tTsbm
QL+ayV9j
=mvOj
-----END PGP SIGNATURE-----

--pf2tgjfb6gzecilc--