Re: [PATCH v5 08/12] iommufd: Enforce pasid compatible domain for PASID-capable device

[Jason Gunthorpe <jgg@nvidia.com>]

On Sat, Dec 07, 2024 at 06:49:04PM +0800, Yi Liu wrote:
> yeah, the dev->iommu->max_pasids indicates if a device can enable pasid
> or not. It already counts in the iommu support. Since all known iommu
> drivers will enable it once it is supported, can we say
> dev->iommu->max_pasids also means enabled? If so, in the HW_INFO path[1],
> we only need check it instead of checking pci config space.

That would be nice, and it is better than checking config space

> 
> > > - Nest parent domain should never be pasid-compatible?
> > 
> > Up to the driver.
> > 
> > >    I think the AMD iommu uses the V1 page table format for the parent
> > >    domain. Hence parent domain should not be allocated with the
> > >    IOMMU_HWPT_ALLOC_PASID flag. Otherwise, it does not work. Should this
> > >    be enforced in iommufd?
> > 
> > Enforced in the driver.
> 
> ok. BTW. Should we update the below description to be "the rule is only
> applied to the domains that will be attached to pasid-capable device"?
> Otherwise, a 'poor' userspace might consider any domains allocated for
> pasid-capable device must use this flag.
> 
>  * @IOMMU_HWPT_ALLOC_PASID: Requests a domain that can be used with PASID. The
>  *                          domain can be attached to any PASID on the device.
>  *                          Any domain attached to the non-PASID part of the
>  *                          device must also be flaged, otherwise attaching a
>  *                          PASID will blocked.
>  *                          If IOMMU does not support PASID it will return
>  *                          error (-EOPNOTSUPP).

I'm not sure, I think we should not make it dependent on the device
capability. There may be multiple devices in the iommufd and some of
them may be PASID capable. The PASID capable domains should interwork
with all of the devices. Thus I'd also expect to be able to allocate a
PASID capable domain on a non-pasid capable device. Even though that
would be pointless on its own.

> > iommufd should enforce that the domain was created with
> > IOMMU_HWPT_ALLOC_PASID before passing the HWPT to any pasid
> > attach/replace function.
> 
> This seems much simpler enforcement than I did in this patch. I even
> enforced the domains used in the non-pasid path to be flagged with
> _ALLOC_PASID.

That seems good too, if it isn't too hard to do.

> on AMD is not able to be used by pasid, a sane userspace won't do it. So
> such a domain won't appear in the pasid path on AMD platform. But it can be
> on Intel as we support attaching nested domain to pasid. So we would need
> the nested domain be flagged in order to pass this check. Looks like we
> cannot do this enforcement in iommufd. Put it in the iommu drivers would be
> better. Is it?

Why can't it be in iommufd? A PASID domain should be a hwpt_paging
with the ALLOC_PASID flag, just put a bit in the hwpt_paging struct
and be done with it. That automatically rejects nested domains from
pasid.

I would like to keep this detail out of the drivers as I think drivers
will have a hard time getting it right. Drivers should implement their
HW restrictions and if they are more permissive than the iommufd model
that is OK.

We want some reasonable compromise to encourage applications to use
IOMMU_HWPT_ALLOC_PASID properly, but not build too much complexity to
reject driver-specific behavior.

Jason