Re: [PATCH] iommu/amd: Add Secure ATS support

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

On 2025/3/4 18:15, Vasant Hegde wrote:
> Jason,
> 
> 
> On 3/4/2025 12:08 AM, Jason Gunthorpe wrote:
>> On Sun, Mar 02, 2025 at 04:10:46PM +0800, Yi Liu wrote:
>>> I'm also thinking about the impact of such a knob. How should we define
>>> this knob. Should we allow it to disable/enable ATS? Especially in runtime.
>>> e.g. if the device is identified to be ATS untrusted by the admin, while
>>> the hw does not support SATS, it seems reasonable to disable ATS for
>>> such device. This might change the driver behavior on ATS enabling. Intel
>>> iommu driver enables ATS as long as it's available in the probe_device()
>>> op. What about AMD and ARM?
>>
>> ARM enables ATS when a PAGING translation is set on the RID or any
>> PASID is used. Otherwise it is off, eg for RID = IDENTITY
>>
>> This seems like a reasonable thing to do to me..
>>
>>> On the other hand, we may just define the knob as sats required or not.
>>> This is just a knob to let kernel know if the iommu driver needs to enable
>>> sats or not. While leave the ATS enabling policy unchanged. This means
>>> we need to probe the sats capability of iommu hw before creating the knob
>>> in sysfs.
>>
>> If we add some ATS control knob then maybe it should be able to
>> disable/enable normal ATS as well?
> 
> Yes. If we are dong knob then we should support normal ATS as well.
> 
>>
>>   never
>>   always
>>   paging-only
>>   secure-always
>>   secure-paging-only
> 
> Why not just expose supported modes (No ATS, ATS, Secure ATS) per group and
> allow admin to select within supported modes?  We may have to add more logic in
> sysfs store/show code path. But user will know what is supported and what they
> can pick.
> 
> Also sysfs knob needs new domain ops to communicate changes to indivisual
> driver. Like in store path
>    - validate the input
>    - make sure driver is unbond so that its safe to modify the
>    - New ops to communicate to indivisual driver
> 	 something like ops->set_ats_mode(dev, mode)

Perhaps marking it in the core level is enough. Any domain that is going to
be used for this device should respect it. e.g. if admin change it to be
requiring SATS, then the default_domain should be reallocated with a flag
to indicate it.

-- 
Regards,
Yi Liu