Re: [PATCH v2 08/16] iommu: define and export iommu_access_remote_vm()

[Fenghua Yu]

Hi, Jean,

On 3/7/23 00:40, Jean-Philippe Brucker wrote:
> Hi Fenghua,
>
> On Mon, Mar 06, 2023 at 08:31:30AM -0800, Fenghua Yu wrote:
> > Define and export iommu_access_remote_vm() to allow IOMMU related
> > drivers to access user address space by PASID.
> >
> > The IDXD driver would like to use it to write the user's completion
> > record that the hardware device is not able to write to due to user
> > page fault.
> >
> > Without the API, it's complex for IDXD driver to copy completion record
> > to a process' fault address for two reasons:
> > 1. access_remote_vm() is not exported and shouldn't be exported for
> >     drivers because drivers may easily cause mm reference issue.
> > 2. user frees fault address pages to trigger fault by IDXD device.
> >
> > The driver has to call iommu_sva_find(), kthread_use_mm(), re-implement
> > majority of access_remote_vm() etc to access remote vm.
> >
> > This IOMMU specific API hides these details and provides a clean interface
> > for idxd driver and potentially other IOMMU related drivers.
> >
> > Suggested-by: Alistair Popple <apopple@xxxxxxxxxx>
> > Signed-off-by: Fenghua Yu <fenghua.yu@xxxxxxxxx>
> > Cc: Joerg Roedel <joro@xxxxxxxxxx>
> > Cc: Will Deacon <will@xxxxxxxxxx>
> > Cc: Robin Murphy <robin.murphy@xxxxxxx>
> > Cc: Alistair Popple <apopple@xxxxxxxxxx>
> > Cc: Lorenzo Stoakes <lstoakes@xxxxxxxxx>
> > Cc: Christoph Hellwig <hch@xxxxxxxxxxxxx>
> > Cc: iommu@xxxxxxxxxxxxxxx
> > ---
> > v2:
> > - Define and export iommu_access_remote_vm() for IDXD driver to write
> >    completion record to user address space. This change removes
> >    patch 8 and 9 in v1 (Alistair Popple)
> >
> >   drivers/iommu/iommu-sva.c | 35 +++++++++++++++++++++++++++++++++++
> >   include/linux/iommu.h     |  9 +++++++++
> >   2 files changed, 44 insertions(+)
> >
> > diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
> > index 24bf9b2b58aa..1d7a0aee58f7 100644
> > --- a/drivers/iommu/iommu-sva.c
> > +++ b/drivers/iommu/iommu-sva.c
> > @@ -71,6 +71,41 @@ struct mm_struct *iommu_sva_find(ioasid_t pasid)
> >   }
> >   EXPORT_SYMBOL_GPL(iommu_sva_find);
> >   
> > +/**
> > + * iommu_access_remote_vm - access another process' address space by PASID
> > + * @pasid:	Process Address Space ID assigned to the mm
> > + * @addr:	start address to access
> > + * @buf:	source or destination buffer
> > + * @len:	number of bytes to transfer
> > + * @gup_flags:	flags modifying lookup behaviour
> > + *
> > + * Another process' address space is found by PASID. A reference on @mm
> > + * is taken and released inside the function.
> > + *
> > + * Return: number of bytes copied from source to destination.
> > + */
> > +int iommu_access_remote_vm(ioasid_t pasid, unsigned long addr, void *buf,
> > +			   int len, unsigned int gup_flags)
> > +{
> > +	struct mm_struct *mm;
> > +	int copied;
> > +
> > +	mm = iommu_sva_find(pasid);
>
> The ability to find a mm by PASID is being removed, see
> https://lore.kernel.org/linux-iommu/20230301235646.2692846-4-jacob.jun.pan@xxxxxxxxxxxxxxx/

Thank you very much for pointing out this.

I talked to Jacob just now. He will keep iommu_sva_find() function
in his next version because this patch is still using the function. He 
agrees that I can still call iommu_sva_find() in this patch.

-Fenghua