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

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

On 2025/3/4 22:24, Jason Gunthorpe wrote:
> On Tue, Mar 04, 2025 at 03:45:54PM +0530, Vasant Hegde wrote:
>>>   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.
> 
> A sysfs type thing is what I'm thinking
> 
> Just trying to understand what options we should be providing to
> userspace..
> 
> I am sensitive that we'd like to disble ATS for identity modes in many
> common cases and we get that implicitly today.
> 
>> 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)
> 
> Yeah this needs to be considered.. I wonder if a flags input to attach
> is the right way to go.

Do we expect this flag to setup the SATS in the underlying iommu driver?
If yes, I doubt if it is proper. My reason as below:

he reason is more for the #4 case you summarized in earlier mails of this
thread. Such HWs need to map the PA in the DPT/HPT table when a given PA is
mapped to IOVA. We may just hook the DPT/HPT map/unmap to the map/unmap of
a paging domain. Hence it's better to determine if the DPT/HPT is desired
per allocation. Otherwise it may requires upper layer to replay the
mapping. e.g. iommufd replay the mappings stored in IOAS->iopt

" 4) TA is an full physical address and the IOMMU validates the full
     physical using some kind of permission structure (ARM calls this
     Device Permission Table)
"

> 
> That would be convenient for how SMMUv3 is structued now as it can just
> switch modes hitlessly with its existing logic. I think all drivers
> should have a similar design.

I think this is more like we have done for the set_dev_pasid op. Now it
supports replacing domain. But as I replied in above, add a flag in the
attach op might not be a good idea.

-- 
Regards,
Yi Liu