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

Yi Liu
2024-12-10T03:10:19Z

On 2024/12/9 22:57, Jason Gunthorpe wrote:
> 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.

yes. I also had an offline email to confirm with Vasant, and he confirmed
a non-pasid capable device should be able to use pasid-capable domain (V2
page table.

As far as I know, allocating PASID cpable domain depends on whether
the underlying IOMMU hw supports the page table format (AMD IOMMU V2, Intel
VT-d Stage-1, ARM SMMUv3 Stage-1 page table?) or not. So even for a
non-pasid capable device, allocating pasid-capable domain should succeed if
its IOMMU supports the page table format.

>>> 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.

The non-pasid and pasid shares much code, I think it should be doable.

>> 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.

The problem is Intel side, we allow attaching nested domains to pasid. :(
That's why I'm asking for updating the description of ALLOC_PASID and
the enforcement to be only applicable to paging domains, not applicable for
nested domains.

> 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.

I'm ok to do it in iommufd as long as it is only applicable to hwpt_paging.
Otherwise, attaching nested domain to pasid would be failed according to
the aforementioned enforcement.

--
Regards,
Yi Liu