Re: [PATCH v4 10/17] iommufd: Support IOMMU_HWPT_ALLOC allocation with user data

[Jason Gunthorpe <jgg@nvidia.com>]

On Mon, Oct 16, 2023 at 03:03:04PM +0800, Yi Liu wrote:
> On 2023/10/14 08:51, Nicolin Chen wrote:
> > On Fri, Oct 13, 2023 at 09:07:09PM -0300, Jason Gunthorpe wrote:
> > > On Fri, Oct 13, 2023 at 01:58:59PM -0700, Nicolin Chen wrote:
> > > > On Fri, Oct 13, 2023 at 12:19:23PM -0300, Jason Gunthorpe wrote:
> > > > > On Thu, Sep 21, 2023 at 12:51:31AM -0700, Yi Liu wrote:
> > > > > > IOMMU_HWPT_ALLOC already supports iommu_domain allocation for usersapce.
> > > > > > But it can only allocate a hw_pagetable that associates to a given IOAS,
> > > > > > i.e. only a kernel-managed hw_pagetable of IOMMU_HWPT_TYPE_DEFAULT type.
> > > > > > 
> > > > > > IOMMU drivers can now support user-managed hw_pagetables, for two-stage
> > > > > > translation use cases, that require user data input from the user space.
> > > > > > 
> > > > > > Extend the IOMMU_HWPT_ALLOC ioctl to accept non-default hwpt_type with a
> > > > > > type specified user data. Also, update the @pt_id to accept hwpt_id too
> > > > > > besides an ioas_id. Then, pass them to the downstream alloc_fn().
> > > > > > 
> > > > > > Co-developed-by: Nicolin Chen <nicolinc@nvidia.com>
> > > > > > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> > > > > > Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> > > > > > ---
> > > > > >   drivers/iommu/iommufd/hw_pagetable.c | 19 ++++++++++++++++++-
> > > > > >   include/uapi/linux/iommufd.h         | 23 +++++++++++++++++++++--
> > > > > >   2 files changed, 39 insertions(+), 3 deletions(-)
> > > > > 
> > > > > Can we also come with a small vt-d patch that does implement an op for
> > > > > this? Or is it too big?
> > > > > 
> > > > > It would be nice if we could wrap IOMMU_HWPT_ALLOC into one
> > > > > self-contained series and another series for invalidate.
> > > > 
> > > > We now only use IOMMU_HWPT_ALLOC for nested domain allocations,
> > > > which won't be supported until the cache_invalidate_user ops is
> > > > preset?
> > > > 
> > > > /* e.g. the following piece is in iommufd_user_managed_hwpt_alloc */
> > > > 
> > > > +       /* Driver is buggy by missing cache_invalidate_user in domain_ops */
> > > > +       if (WARN_ON_ONCE(!hwpt->domain->ops->cache_invalidate_user)) {
> > > > +               rc = -EINVAL;
> > > > +               goto out_abort;
> > > > +       }
> > > > 
> > > 
> > > Hm. That hunk could migrate to the invalidate series.
> > > 
> > > I'm just leeary of doing the invalidation too considering how
> > > complicated it is
> > 
> > OK. Let's see how Yi/Kevin/Baolu reply about the feasibility
> > with the VT-d driver. Then Yi and I can accordingly separate
> > the allocation part into a smaller series.
> 
> Current nesting series actually extends HWPT_ALLOC ioctl to accept user
> data for allocating domain with vendor specific data. Nested translation
> happens to be the usage of it. But nesting requires invalidation. If we
> want to do further split, then this new series would be just "extending
> HWPT_ALLOC to accept vendor specific data from userspace". But it will
> lack of a user if nesting is separated. Is this acceptable? @Jason

I'd still like to include the nesting allocation and attach parts
though, even if they are not usable without invalidation ..

> BTW. Do we still have unsolved issue on the invalidation path?

I'm not sure, there were so many different versions of it we need to
go back over it and check the dirver implementations again

Jason