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

[Vasant Hegde <vasant.hegde@amd.com>]

Kevin,


On 3/5/2025 8:54 AM, Tian, Kevin wrote:
>> From: Vasant Hegde <vasant.hegde@amd.com>
>> Sent: Monday, March 3, 2025 7:42 PM
>>
>> Currently AMD driver enables device ATS and configures IOMMU DTE (per
>> device
>> setup) during attach_device() path. There was a discussion to move device
>> ATS
>> enablement to probe() path. We will move it to probe() path soon.
> 
> Based on discussion in this thread I'd expect doing it during
> attach is the long-term direction.

AMD does actual enablement of ATS per device (via DTE setting) in attach device
path. So I see this as default requirement. Of course some of policy enforcement
we can implement later.

May be we should document full flow in detail and make sure it takes care of all
HW requirements. Then decide when to do each part.

> 
>>
>>
>>>
>>> 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.
>>
>> Right. Lets see if below flow covers all the requirement
>>   - Define new flags
>>     IOMMU_HWPT_FLAGS_SATS / IOMMU_HWPT_FLAGS_ATS
> 
> SATS policy should be managed by the sys admin, while ATS feature
> itself can be opted by the user.

"user" you mean like iommufd during domain allocation?

> 
>>
>>   - Enhance 'struct iommu_device' to include ATS flags (NO ATS, ATS, SECURE
>> ATS)
>>     iommu_device->ats_flag
> 
> as commented by Jason this needs to differentiate domain types,
> either due to hw lacking of support for certain types or the admin
> imposes different security levels across different types.

Right.

> 
>>
>>   - During probe() driver will update this field (similar to max_pasids)
>>     If (IOMMU needs Secure ATS always)
>> 	iommu_device->ats_flag = IOMMU_HWPT_FLAGS_SATS
>>     else if (device supports ATS and IOMMU supports Secure ATS)
>> 	iommu_device->ats_flag = IOMMU_HWPT_FLAGS_SATS |
>> IOMMU_HWPT_FLAGS_ATS
>>     else if (device supports ATS and IOMMU supports ATS)
>> 	iommu_device->ats_flag = IOMMU_HWPT_FLAGS_ATS
>>     else
>> 	iommu_device->ats_flag = <NO FLAGS> (May be have one more flag
>> IOMMU_HWPT_FLAGS_NO_ATS) ?
> 
> Here the device->ats_flag should be more an ats_policy thing? At
> probe time it's assigned with a default policy according to device
> type, platform requirement, etc. and later exposed via sysfs for
> admin opt.

Right. We can call it as ats_policy (expected/desired mode by HW driver).
So now core knows HW requirement. Also default and global policy enforcement
(via kernel command line).

During domain allocation, we can enforce required domain specific policy.


> 
> Actual ats enabling/enforcement is done at attach time, according
> to ats_policy.

Then during attach_dev() we pass those flags as argument?

> 
>>
>>
>>   - Based on default setting, kernel command line option and probe() data,
>> core
>> layer can adjust ATS capability. i. e.
>>      - If we have kernel command line (say iommu.ats=<x>), use that data for
>> all
>> ATS capable device.
>>      - If not, apply default
>>       untrusted device -> force secure ATS or no ats
>>       If HW needs Secure ATS always , enable Secure ATS
>>       For all other cases use ATS
> 
> this is an interesting one, i.e. whether we still want a global iommu
> option to enable sats for all applicable devices/domains. the pros
> is that it is simple and allows underlying driver to add sats support
> before a more flexible sysfs interface is ready. the downside is that
> it's unclear which device/domain types are affected by sats on
> different platforms w/o digging into the hw detail...

I preferred global option (that's why this patch added amd_iommu=sats) because
as you said its simple, can be enforced at global level. Also w/ sysfs interface
we have to update for each device/group indivisual. Another drawback is, w/
sysfs first we have to unbind the driver (which may not be possible always),
modify ATS setting, then bind driver again.

In Summary, I visualize it three level:
  - Default policy implemented in kernel
  - Global policy enforcement by admin via kernel command line
  - Per group/device level enforcement via sysfs

Do we need something for iommufd/vfio? Where user can tell desired mode while
allocating HWPT domains?

-Vasant