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

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

On 2025/2/28 16:47, Vasant Hegde wrote:
> Hi Yi,
> 
> 
> On 2/28/2025 2:17 PM, Yi Liu wrote:
>> On 2025/2/28 16:30, Vasant Hegde wrote:
>>> Hi Yi,
>>>
>>>
>>> On 2/28/2025 1:13 PM, Yi Liu wrote:
>>>> On 2025/2/28 14:32, Tian, Kevin wrote:
>>>>>> From: Vasant Hegde <vasant.hegde@amd.com>
>>>>>> Sent: Thursday, February 27, 2025 11:28 PM
>>>>>>
>>>>>> Hi Kevin,
>>>>>>
>>>>>>
>>>>>> On 2/26/2025 12:35 PM, Tian, Kevin wrote:
>>>>>>>> From: Tian, Kevin
>>>>>>>> Sent: Wednesday, February 26, 2025 10:51 AM
>>>>>>>>
>>>>>>>>> From: Jason Gunthorpe <jgg@ziepe.ca>
>>>>>>>>> Sent: Wednesday, February 26, 2025 9:18 AM
>>>>>>>>>
>>>>>>>>> On Wed, Feb 26, 2025 at 01:12:38AM +0000, Tian, Kevin wrote:
>>>>>>>>>> If above understanding is true, my preference is to support a sats flag
>>>>>>>>>> in domain alloc (nested_parent only and PCI/CXL.io only, as the start).
>>>>>> For
>>>>>>>>>> AMD it turns on the sats bit in the DTE. for Intel/ARM the permission
>>>>>>>>>> structure is created and updated according to the map/unmap calls
>>>>>>>>>> on the parent S2.
>>>>>>>>>
>>>>>>>>> They are functionally different things, and have different
>>>>>>>>> requirements on the PCI topologies supported (eg CXL.cache vs no
>>>>>>>>> CXL.cache)
>>>>>>>>>
>>>>>>>>> I think they need to be different flags
>>>>>>>>>
>>>>>>>>
>>>>>>>> Not exactly. They are functionally different but serving the same purpose
>>>>>>>> to the user. From user p.o.v. it's sufficient to have a general flag for
>>>>>>>> sats
>>>>>>>> when allocating a domain. The underlying driver decides whether such
>>>>>>>> flag is supported based on the domain type and the associated device,
>>>>>>>> just like checks on other existing flags.
>>>>>>>>
>>>>>>>
>>>>>>> Chatted with Yi offline. Having untrusted user control a security
>>>>>>> feature doesn't make much sense. Probably what we really require
>>>>>>> is:
>>>>>>>
>>>>>>> - IOMMU core exposes a separate sats knob per probed device,
>>>>>>>      allowing the administrator to manage the sats policy which could
>>>>>>>      be no ats, unsecure ats and secure ats (might be further set per
>>>>>>>      domain type in case the hw doesn't support sats for all types or
>>>>>>>      the driver doesn't support all hw-supported types in one batch).
>>>>>>
>>>>>> When you say "sats knob", you mean sysfs interface?
>>>>>
>>>>> yes
>>>>
>>>> in case this is the direction. This might be a per iommu_group knob. do we
>>>
>>>
>>>> want a default value per some info that can be probed by kernel?
>>>
>>> I prefer kernel probe/setting default values so that we can enforce HW
>>> restrictions (like SNP, untrusted device etc) and then....
>>
>> I got untrusted device. How is SNP probed? Is there any bit for it?
> 
> Yes. There is a IOMMU feature bit. see iommu_snp_enable().

sure.

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?

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.

Any ideas?

-- 
Regards,
Yi Liu