Received-SPF: pass (google.com: domain of linux-kernel+bounces-205703-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
From: "Tian, Kevin" <kevin.tian@intel.com>
To: Lu Baolu <baolu.lu@linux.intel.com>, Jason Gunthorpe <jgg@ziepe.ca>,
	"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>, "Liu, Yi L" <yi.l.liu@intel.com>, "Jacob
 Pan" <jacob.jun.pan@linux.intel.com>, Joel Granados <j.granados@samsung.com>
CC: "iommu@lists.linux.dev" <iommu@lists.linux.dev>,
	"virtualization@lists.linux-foundation.org"
	<virtualization@lists.linux-foundation.org>, "linux-kernel@vger.kernel.org"
	<linux-kernel@vger.kernel.org>
Subject: RE: [PATCH v6 06/10] iommufd: Add iommufd fault object
Thread-Topic: [PATCH v6 06/10] iommufd: Add iommufd fault object
Thread-Index: AQHar+txwPSPp6CyskanS/lKKehznrG8Efnw
Date: Fri, 7 Jun 2024 09:17:28 +0000
Message-ID: <BN9PR11MB527615EC664698C340A0CA878CFB2@BN9PR11MB5276.namprd11.prod.outlook.com>
References: <20240527040517.38561-1-baolu.lu@linux.intel.com>
 <20240527040517.38561-7-baolu.lu@linux.intel.com>
In-Reply-To: <20240527040517.38561-7-baolu.lu@linux.intel.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?Ywm39sQrVzAp6znFvosSi6Hrmy7qr52ogI8K2TTzXnf00TvhFjw4xuOHxIwC?=
 =?us-ascii?Q?oX2e9nkj4qaV0ITbX/WKvxY70esmnLKNPhyNLWIWa3wAQpqHPlQHVqUYuu08?=
 =?us-ascii?Q?SZaHdheco/caiR9mMR7gkqSoHYoOeslglfn5ec0EGBlgm6Z0I0wIxT4F6nOY?=
 =?us-ascii?Q?qu05FGUFuJQdmhPjIviUNiiH698SgvCUswBn/H/8c895mkaFlm2h+yFUltf2?=
 =?us-ascii?Q?JZRELHb3A0Po9l1oOrKrN8x1nO5BJBWdn1H31FJe6xx13SF3Jyy/CmCpxXk1?=
 =?us-ascii?Q?UvVaZR97CbT+j97lOJaOBGekKYcL+4XSTe02cVn4561ecZatpNTAY+XD+HW/?=
 =?us-ascii?Q?kLgQr+/nPUgF2jgerl0QTWzL4OgHg+p4A4lGirkL5u9F2aNaHQyeplpeyYG0?=
 =?us-ascii?Q?SAwinnxnds3WIRe/ey5Yk1tctvEywPAjIIpoJ+Vi+OAqX+lTBcsV1GbL30Dt?=
 =?us-ascii?Q?kexxtwtaalkkauqunSZMqPAF4+HfNwsOSjYPoqV4OQel4HNbQATGRcdptJZl?=
 =?us-ascii?Q?D5iXX0Jf7FXXgHc8L+cDqNpBPTHL9pbr5zCTIxeGv9neoPHLFbH/ylc7y4ab?=
 =?us-ascii?Q?P3GhINzBbpmAcmGPZ9/7/GozNUMNTwgmPqd+cTOU/ON6v6L13MNglxQxWimy?=
 =?us-ascii?Q?8ywkv8yQTlBjEf4HW01Ff9bnkx+qSvde9qx6iCAWc0zXeRZHp3j/ZKXEhJMn?=
 =?us-ascii?Q?QlpAZs/HY1Ma7cGDv7wILXoEaomCQY2p14VZpn92ASvqcC25kAh+4L/TQLpN?=
 =?us-ascii?Q?Q+FqUoO3wjdPg6sZgWZUPiaL/Mvbw62mZb4Y7ppEt0Cx5LdQy5ro+6w3IhE3?=
 =?us-ascii?Q?bl94gHJesfTqF9qtxlGGSZJ6Wm2x2YPYoQfNg/NRW6w9BOyqwEpTseQp17lk?=
 =?us-ascii?Q?sAgZsJwTA1u/qkfABKRGNxZvXCeSolKyHHHUt/vHBqU8TlrYSJgvMiT6HGKz?=
 =?us-ascii?Q?qcnjUCzjDgKuBjdEAT3yo342GddhrssUGtEKGiU7vibZKFb09PBeSNkZnSKb?=
 =?us-ascii?Q?NzO0OTRWXEO5TjP4TcjUgguRN3+Wckf/7kTu+oGJ0Df4wn1S2CkqrkKUaNW7?=
 =?us-ascii?Q?SFw01UTV60wDOrwaRvfDD3cJn6sTpDSIBlj5bxp3qRlpRH6qiNBn0Rroxt7a?=
 =?us-ascii?Q?NckHGIysz7HESuV0VsyZPRqtapwBWoihemSg24D7okVmUolOGl6ozdQF0FGB?=
 =?us-ascii?Q?kttSvnnSz8Dg4YCKuefJOSMDyn3+73rGilVUeddNK/5hYBLx1be7eFxFYb9G?=
 =?us-ascii?Q?1kooold7Yh86aOmxF758rFZ81jAia/eXTMM+As/TVszCUgx2uCp7S4OmDZia?=
 =?us-ascii?Q?5fj6tmVTMOvEWUJXAbItXwt+kuYsERb9oKk6FLRUn844FI+xOVIFRe0YtqKq?=
 =?us-ascii?Q?wXG4KJm3qew94Lqw5bh4khoiFZ9+L//SbZsKeCZj2jzdnW3X3bamtak13iNU?=
 =?us-ascii?Q?4Qztzg7jK0LDXzNbpI3omFI6FV2Fq0h4oe6kFDR9MqAJa/K43l+cTve+KuBU?=
 =?us-ascii?Q?9x1ASjFsWoJ+Y/ZLvgZFAge/M88w1Vwr+ZO73Ih2Hk0gfuiiw+ZVOoTVhot8?=
 =?us-ascii?Q?OBVQFkkLj40WWpKPpZxaUyJvWH5VqxAXZB9/t9Yi?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
X-Mailing-List: linux-kernel@vger.kernel.org
List-Id: <linux-kernel.vger.kernel.org>
List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org>
List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org>
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: BN9PR11MB5276.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 1cbf72c1-2017-4177-f418-08dc86d2a6e8
X-MS-Exchange-CrossTenant-originalarrivaltime: 07 Jun 2024 09:17:28.1213
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: v8G0MjXx9FCwf2uBKCca4iqhC5veRpkX5hJhS4WHx8ZpvgecxgwlFaOG9LVCJi+annqXJRsZqZa+JjUc/AtGkQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW3PR11MB4649
X-OriginatorOrg: intel.com

> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Monday, May 27, 2024 12:05 PM
>=20
> +static ssize_t iommufd_fault_fops_read(struct file *filep, char __user *=
buf,
> +				       size_t count, loff_t *ppos)
> +{
> +	size_t fault_size =3D sizeof(struct iommu_hwpt_pgfault);
> +	struct iommufd_fault *fault =3D filep->private_data;
> +	struct iommu_hwpt_pgfault data;
> +	struct iommufd_device *idev;
> +	struct iopf_group *group;
> +	struct iopf_fault *iopf;
> +	size_t done =3D 0;
> +	int rc =3D 0;
> +
> +	if (*ppos || count % fault_size)
> +		return -ESPIPE;

the man page says:

"If count is zero, read() returns zero and has no  other  results."

> +
> +	mutex_lock(&fault->mutex);
> +	while (!list_empty(&fault->deliver) && count > done) {
> +		group =3D list_first_entry(&fault->deliver,
> +					 struct iopf_group, node);
> +
> +		if (group->fault_count * fault_size > count - done)
> +			break;
> +
> +		rc =3D xa_alloc(&fault->response, &group->cookie, group,
> +			      xa_limit_32b, GFP_KERNEL);
> +		if (rc)
> +			break;
> +
> +		idev =3D to_iommufd_handle(group->attach_handle)->idev;
> +		list_for_each_entry(iopf, &group->faults, list) {
> +			iommufd_compose_fault_message(&iopf->fault,
> +						      &data, idev,
> +						      group->cookie);
> +			rc =3D copy_to_user(buf + done, &data, fault_size);
> +			if (rc) {

'rc' should be converted to -EFAULT.

> +				xa_erase(&fault->response, group->cookie);
> +				break;
> +			}
> +			done +=3D fault_size;
> +		}
> +
> +		list_del(&group->node);
> +	}
> +	mutex_unlock(&fault->mutex);
> +
> +	return done =3D=3D 0 ? rc : done;

again this doesn't match the manual:

"On error, -1 is returned, and errno is set appropriately. "

it doesn't matter whether 'done' is 0.

> +
> +static int iommufd_fault_fops_release(struct inode *inode, struct file *=
filep)
> +{
> +	struct iommufd_fault *fault =3D filep->private_data;
> +
> +	iommufd_ctx_put(fault->ictx);
> +	refcount_dec(&fault->obj.users);
> +	return 0;
> +}

hmm this doesn't sound correct. the context and refcount are
acquired in iommufd_fault_alloc() but here they are reverted when
the fd is closed...

> +
> +	filep =3D anon_inode_getfile("[iommufd-pgfault]",
> &iommufd_fault_fops,
> +				   fault, O_RDWR);
> +	if (IS_ERR(filep)) {
> +		rc =3D PTR_ERR(filep);
> +		goto out_abort;
> +	}
> +
> +	refcount_inc(&fault->obj.users);
> +	iommufd_ctx_get(fault->ictx);
> +	fault->filep =3D filep;

those 3 lines can be moved after below fdno get. It's reads slightly
clearer to put file related work together before getting to the last piece
of intiailzation.=20

> +
> +	fdno =3D get_unused_fd_flags(O_CLOEXEC);
> +	if (fdno < 0) {
> +		rc =3D fdno;
> +		goto out_fput;
> +	}
> +
> @@ -332,6 +332,7 @@ union ucmd_buffer {
>  	struct iommu_ioas_unmap unmap;
>  	struct iommu_option option;
>  	struct iommu_vfio_ioas vfio_ioas;
> +	struct iommu_fault_alloc fault;

alphabetic=20

> @@ -381,6 +382,8 @@ static const struct iommufd_ioctl_op
> iommufd_ioctl_ops[] =3D {
>  		 val64),
>  	IOCTL_OP(IOMMU_VFIO_IOAS, iommufd_vfio_ioas, struct
> iommu_vfio_ioas,
>  		 __reserved),
> +	IOCTL_OP(IOMMU_FAULT_QUEUE_ALLOC, iommufd_fault_alloc,
> struct iommu_fault_alloc,
> +		 out_fault_fd),

ditto