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

[Fenghua Yu]

Hi, Jean and Jacob,

On 3/7/23 08:33, Fenghua Yu wrote:
> 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.

Further comment from Jason confirms that iommu_sva_find() will be 
removed (https://lore.kernel.org/lkml/ZAjSsm4%2FPDRqViwa@xxxxxxxxxx/).

So cannot call iommu_sva_find() any more. Will maintain mm and find mm 
from PASID inside IDXD driver. And will implement accessing the remote 
mm inside IDXD driver although the implementation will have duplicate 
code as access_remote_vm().

Next version will only change IDXD driver code. There won't be IOMMU 
code change.

Thanks.

-Fenghua