RE: [PATCH v2 4/6] iommufd: Deliver fault messages to user space

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Jason Gunthorpe <jgg@xxxxxxxx>
> Sent: Monday, January 15, 2024 4:47 PM
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@xxxxxxxxxx>
> Cc: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>; Kevin Tian <kevin.tian@xxxxxxxxx>;
> Joerg Roedel <joro@xxxxxxxxxx>; Will Deacon <will@xxxxxxxxxx>; Robin
> Murphy <robin.murphy@xxxxxxx>; Jean-Philippe Brucker <jean-
> philippe@xxxxxxxxxx>; Nicolin Chen <nicolinc@xxxxxxxxxx>; Yi Liu
> <yi.l.liu@xxxxxxxxx>; Jacob Pan <jacob.jun.pan@xxxxxxxxxxxxxxx>;
> iommu@xxxxxxxxxxxxxxx; linux-kselftest@xxxxxxxxxxxxxxx;
> virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v2 4/6] iommufd: Deliver fault messages to user space
> 
> On Fri, Jan 12, 2024 at 05:46:13PM +0000, Shameerali Kolothum Thodi wrote:
> >
> >
> > > -----Original Message-----
> > > From: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
> > > Sent: Thursday, October 26, 2023 3:49 AM
> > > To: Jason Gunthorpe <jgg@xxxxxxxx>; Kevin Tian <kevin.tian@xxxxxxxxx>;
> Joerg
> > > Roedel <joro@xxxxxxxxxx>; Will Deacon <will@xxxxxxxxxx>; Robin
> Murphy
> > > <robin.murphy@xxxxxxx>; Jean-Philippe Brucker <jean-
> philippe@xxxxxxxxxx>;
> > > Nicolin Chen <nicolinc@xxxxxxxxxx>; Yi Liu <yi.l.liu@xxxxxxxxx>; Jacob
> Pan
> > > <jacob.jun.pan@xxxxxxxxxxxxxxx>
> > > Cc: iommu@xxxxxxxxxxxxxxx; linux-kselftest@xxxxxxxxxxxxxxx;
> > > virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> Lu
> > > Baolu <baolu.lu@xxxxxxxxxxxxxxx>
> > > Subject: [PATCH v2 4/6] iommufd: Deliver fault messages to user space
> > >
> > [...]
> >
> > Hi,
> >
> > > +static ssize_t hwpt_fault_fops_write(struct file *filep,
> > > +				     const char __user *buf,
> > > +				     size_t count, loff_t *ppos)
> > > +{
> > > +	size_t response_size = sizeof(struct iommu_hwpt_page_response);
> > > +	struct hw_pgtable_fault *fault = filep->private_data;
> > > +	struct iommu_hwpt_page_response response;
> > > +	struct iommufd_hw_pagetable *hwpt;
> > > +	struct iopf_group *iter, *group;
> > > +	struct iommufd_device *idev;
> > > +	size_t done = 0;
> > > +	int rc = 0;
> > > +
> > > +	if (*ppos || count % response_size)
> > > +		return -ESPIPE;
> > > +
> > > +	mutex_lock(&fault->mutex);
> > > +	while (!list_empty(&fault->response) && count > done) {
> > > +		rc = copy_from_user(&response, buf + done, response_size);
> > > +		if (rc)
> > > +			break;
> > > +
> > > +		/* Get the device that this response targets at. */
> > > +		idev = container_of(iommufd_get_object(fault->ictx,
> > > +						       response.dev_id,
> > > +						       IOMMUFD_OBJ_DEVICE),
> > > +				    struct iommufd_device, obj);
> > > +		if (IS_ERR(idev)) {
> > > +			rc = PTR_ERR(idev);
> > > +			break;
> > > +		}
> > > +
> > > +		/*
> > > +		 * Get the hw page table that this response was generated
> for.
> > > +		 * It must match the one stored in the fault data.
> > > +		 */
> > > +		hwpt = container_of(iommufd_get_object(fault->ictx,
> > > +						       response.hwpt_id,
> > > +
> > > IOMMUFD_OBJ_HW_PAGETABLE),
> > > +				    struct iommufd_hw_pagetable, obj);
> > > +		if (IS_ERR(hwpt)) {
> > > +			iommufd_put_object(&idev->obj);
> > > +			rc = PTR_ERR(hwpt);
> > > +			break;
> > > +		}
> > > +
> > > +		if (hwpt != fault->hwpt) {
> > > +			rc = -EINVAL;
> > > +			goto put_obj;
> > > +		}
> > > +
> > > +		group = NULL;
> > > +		list_for_each_entry(iter, &fault->response, node) {
> > > +			if (response.grpid != iter->last_fault.fault.prm.grpid)
> > > +				continue;
> > > +
> > > +			if (idev->dev != iter->dev)
> > > +				continue;
> > > +
> > > +			if ((iter->last_fault.fault.prm.flags &
> > > +			     IOMMU_FAULT_PAGE_REQUEST_PASID_VALID)
> &&
> > > +			    response.pasid != iter->last_fault.fault.prm.pasid)
> > > +				continue;
> >
> > I am trying to get vSVA working with this series and got hit by the above
> check.
> > On ARM platforms, page responses to stall events(CMD_RESUME) do not
> have
> > an associated pasid.  I think, either we need to check here using
> > IOMMU_FAULT_PAGE_RESPONSE_NEEDS_PASID or remove the check
> > as it will be eventually done in iommu_page_response().
> 
> That doesn't sound right..
> 
> The PASID is the only information we have for userspace to identify
> the domain that is being faulted. It cannot be optional on the request
> side.
> 

Yes, it is valid on the request side. But this is on the response side.

> If it is valid when userspace does read() then it should be valid when
> userspace does write() too.
> 
> It is the only way the kernel can actually match request and response
> here.

The kernel currently checks the pasid only if IOMMU_FAULT_PAGE_RESPONSE_NEEDS_PASID
is set.

https://lore.kernel.org/linux-iommu/20200616144712.748818-1-jean-philippe@xxxxxxxxxx/

> So, I think you have a userspace issue to not provide the right
> pasid??

This is not just ARM stall resume case, but for some PCI devices as well as per
the above commit log. So do we really need to track this in userspace ?

Thanks,
Shameer