Re: [PATCH v4 00/12] Initial support for SMMUv3 nested translation

Baolu Lu
2025-02-14T05:42:54Z

On 2/14/25 02:43, Jason Gunthorpe wrote:
> On Thu, Feb 13, 2025 at 07:57:51PM +0800, Baolu Lu wrote:
>> On 2025/2/5 11:45, Baolu Lu wrote:
>>> On 2025/1/23 3:26, Jason Gunthorpe wrote:
>>>> On Fri, Nov 15, 2024 at 01:55:22PM -0400, Jason Gunthorpe wrote:
>>>>>>> I need your help to remove IOMMU_DEV_FEAT_IOPF from the intel
>>>>>>> driver. I have a patch series that eliminates it from all the other
>>>>>>> drivers, and I wrote a patch to remove FEAT_SVA from intel..
>>>>>> Yes, sure. Let's make this happen in the next cycle.
>>>>>>
>>>>>> FEAT_IOPF could be removed. IOPF manipulation can be handled in the
>>>>>> domain attachment path. A per-device refcount can be implemented. This
>>>>>> count increments with each iopf-capable domain attachment
>>>>>> and decrements
>>>>>> with each detachment. PCI PRI is enabled for the first iopf-capable
>>>>>> domain and disabled when the last one is removed. Probably we can also
>>>>>> solve the PF/VF sharing PRI issue.
>>>>> Here is what I have so far, if you send me a patch for vt-d to move
>>>>> FEAT_IOPF into attach as you describe above (see what I did to arm for
>>>>> example), then I can send it next cycle
>>>>>
>>>>> https://github.com/jgunthorpe/linux/commits/iommu_no_feat/
>>>> Hey Baolu, a reminder on this, lets try for it next cycle?
>>>
>>> Oh, I forgot this. Thanks for the reminding. Sure, let's try to make it
>>> in the next cycle.
>>
>> Hi Jason,
>>
>> I've worked through the entire series. The patches are available here:
>>
>> https://github.com/LuBaolu/intel-iommu/commits/iommu-no-feat-v6.14-rc2
>>
>> Please check if this is the right direction.
>
> Looks great, and you did all the cleanup stuff too!
>
> The vt-d flow is a little more complicated than the ARM logic because
> the driver flow is structed differently.
>
> Do we really want ATS turned on if the only thing attached is an
> IDENTITY domain? That will unnecessarily slow down ATS capable HW.. It
> is functionally OK though.

It depends. The Intel driver uses a simple approach.

When the IOMMU is working in legacy mode, PASID and PRI are not
supported. In this case, ATS will not be enabled if the identity domain
is attached to the device.

When the IOMMU is working in scalable mode, PASID and PRI are supported.
ATS will always be enabled, even if the identity domain is attached to
the device, because the PASID might use PRI, which depends on ATS
functionality. This might not be the best choice, but it is the
simplest and functional.

If any device does not work with the identity domain and ATS enabled,
then we can add a quirk to the driver, as we already have such a
mechanism.

>
> Also, are there enough ATC flushes around any domain type change? I
> didn't check..

The VT-d specification defines guidance for software to manage caches,
both the IOTLB in the IOMMU and the ATC on the device (Table 28,
Guidance to Software for Invalidations). The driver was written
according to that guidance.

> I feel like we should leave "iommu: Move PRI enablement for VF to
> iommu driver" out for now, every driver needs this check? AMD
> supports SVA and PRI so it needs it too.

Yes, agreed.

>
> Do you want to squash those fixup patches and post it?

Yes, sure. Let me post it for further review.

> Thanks,
> Jason

Thanks,
baolu