On 6/7/24 5:17 PM, Tian, Kevin wrote:
From: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
Sent: Monday, May 27, 2024 12:05 PM
+static ssize_t iommufd_fault_fops_read(struct file *filep, char __user *buf,
+ size_t count, loff_t *ppos)
+{
+ size_t fault_size = sizeof(struct iommu_hwpt_pgfault);
+ struct iommufd_fault *fault = filep->private_data;
+ struct iommu_hwpt_pgfault data;
+ struct iommufd_device *idev;
+ struct iopf_group *group;
+ struct iopf_fault *iopf;
+ size_t done = 0;
+ int rc = 0;
+
+ if (*ppos || count % fault_size)
+ return -ESPIPE;
the man page says:
"If count is zero, read() returns zero and has no other results."
My understanding is that reading zero bytes is likely to check if a file
descriptor is valid and ready for reading without actually taking any
data from it.
In this code, it just returns 0 and it's compatible with the man page.
Or, I overlooked anything?
+
+ mutex_lock(&fault->mutex);
+ while (!list_empty(&fault->deliver) && count > done) {
+ group = list_first_entry(&fault->deliver,
+ struct iopf_group, node);
+
+ if (group->fault_count * fault_size > count - done)
+ break;
+
+ rc = xa_alloc(&fault->response, &group->cookie, group,
+ xa_limit_32b, GFP_KERNEL);
+ if (rc)
+ break;
+
+ idev = 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 = copy_to_user(buf + done, &data, fault_size);
+ if (rc) {
'rc' should be converted to -EFAULT.
Yes. I will make it like this:
if (copy_to_user(buf + done, &data, fault_size)) {
xa_erase(&fault->response, group->cookie);
rc = -EFAULT;
break;
}
+ xa_erase(&fault->response, group->cookie);
+ break;
+ }
+ done += fault_size;
+ }
+
+ list_del(&group->node);
+ }
+ mutex_unlock(&fault->mutex);
+
+ return done == 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.
I don't quite follow here. The code is doing the following:
- If done == 0, it means nothing has been copied to user space. This
could be due to two reasons:
1) the user read with a count set to 0, or
2) a failure case.
The code returns 0 for the first case and an error number for the
second.
- If done != 0, some data has been copied to user space. In this case,
the code returns the number of data copied regardless of the value of
rc.
+
+static int iommufd_fault_fops_release(struct inode *inode, struct file *filep)
+{
+ struct iommufd_fault *fault = 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...
These two refcounts were requested when the fault object was installed
to the fault FD.
filep = anon_inode_getfile("[iommufd-pgfault]", &iommufd_fault_fops,
fault, O_RDWR);
if (IS_ERR(filep)) {
rc = PTR_ERR(filep);
goto out_abort;
}
refcount_inc(&fault->obj.users);
iommufd_ctx_get(fault->ictx);
fault->filep = filep;
These refcounts must then be released when the FD is released.
+
+ filep = anon_inode_getfile("[iommufd-pgfault]",
&iommufd_fault_fops,
+ fault, O_RDWR);
+ if (IS_ERR(filep)) {
+ rc = PTR_ERR(filep);
+ goto out_abort;
+ }
+
+ refcount_inc(&fault->obj.users);
+ iommufd_ctx_get(fault->ictx);
+ fault->filep = 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.
The filep is allocated and initialized together.
+
+ fdno = get_unused_fd_flags(O_CLOEXEC);
+ if (fdno < 0) {
+ rc = 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
@@ -381,6 +382,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_FAULT_QUEUE_ALLOC, iommufd_fault_alloc,
struct iommu_fault_alloc,
+ out_fault_fd),
ditto
Yes, sure. I wasn't aware of the order.
Best regards,
baolu
