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

[Yi Liu <yi.l.liu@intel.com>]

On 2024/12/7 01:58, Jason Gunthorpe wrote:
> On Fri, Dec 06, 2024 at 03:57:39PM +0800, Yi Liu wrote:
>> Hi Jason, Vasant,
>>
>> When cooking new version, I got three opens on enforcing using
>> pasid-compatible domains to pasid-capable device in iommufd as suggested
>> in [1].
>>
>> - Concept problem when considering nested domain
>>    IIUC. pasid-compatible domain means the domain can be attached to PASID.
>>    e.g. AMD requires using V2 page table hence it can be configed to GCR3.
>>    However, the nested domain uses both V1 and V2 page table, I don't think
>>    it can be attached to PASID on AMD. So nested domain can not be
>>    considered as pasid-compatible.
> 
> Yes.
> 
>>    Based on this, this enforcement only
>>    applies to paging-domains. If so, do we still need to enforce it in
>>    iommufd? Will it simpler to let the AMD iommu driver to deal it?
> 
> I think driver should deal with it, Intel doesn't have that
> limitation. I sent patches to fix that detection for AMD and ARM
> already.

ok.

> 
>> - PASID-capable device v.s. PASID-enabled device
>>    We keep saying PASID-capable, but system may not enable it. Would it
>>    better enforce the pasid-compatible domain for PASID-enabled device?
>>    Seems all iommu vendor will enable PASID if it's supported. But
>>    conceptly, it is be more accurate if only do it when PASID is
>>    enabled.
> 
> If we want to do more here we should put the core code in charge of
> deciding of a device will be PASID enabled and the IOMMU driver only
> indicates if it can be PASID supported.
> 
>>    For PCI devices, we can check if the pasid cap is enabled from device
>>    config space. But for non-PCI PASID support (e.g. some ARM platform), I
>>    don't know if there is any way to check the PASID enabled or not. Or, to
>>    cover both, we need an iommu API to check PASID enabled or not?
> 
> Yes, some iommu API, I suggest a flag in the common iommu_device. We
> already have max_pasids there, it may already be nearly enough.

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.

[1] https://lore.kernel.org/kvm/20241108121742.18889-6-yi.l.liu@intel.com/

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

> 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. But just as the beginning of this email, the nested domains
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?

Regards,
Yi Liu