Re: [PATCH v2 05/12] IOMMU: rename and re-type ats_enabled

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.05.2024 12:29, Roger Pau Monné wrote:
> On Wed, May 15, 2024 at 12:07:50PM +0200, Jan Beulich wrote:
>> On 06.05.2024 15:53, Roger Pau Monné wrote:
>>> On Mon, May 06, 2024 at 03:20:38PM +0200, Jan Beulich wrote:
>>>> On 06.05.2024 14:42, Roger Pau Monné wrote:
>>>>> On Thu, Feb 15, 2024 at 11:15:39AM +0100, Jan Beulich wrote:
>>>>>> @@ -196,7 +196,7 @@ static int __must_check amd_iommu_setup_
>>>>>>          dte->sys_mgt = MASK_EXTR(ivrs_dev->device_flags, 
>>>>>> ACPI_IVHD_SYSTEM_MGMT);
>>>>>>  
>>>>>>          if ( use_ats(pdev, iommu, ivrs_dev) )
>>>>>> -            dte->i = ats_enabled;
>>>>>> +            dte->i = true;
>>>>>
>>>>> Might be easier to just use:
>>>>>
>>>>> dte->i = use_ats(pdev, iommu, ivrs_dev);
>>>>
>>>> I'm hesitant here, as in principle we might be overwriting a "true" by
>>>> "false" then.
>>>
>>> Hm, but that would be fine, what's the point in enabling the IOMMU to
>>> reply to ATS requests if ATS is not enabled on the device?
>>>
>>> IOW: overwriting a "true" with a "false" seem like the correct
>>> behavior if it's based on the output of use_ats().
>>
>> I don't think so, unless there were flow guarantees excluding the possibility
>> of taking this path twice without intermediately disabling the device again.
>> Down from here the enabling of ATS is gated on use_ats(). Hence if, in an
>> earlier invocation, we enabled ATS (and set dte->i), we wouldn't turn off ATS
>> below (there's only code to turn it on), yet with what you suggest we'd clear
>> dte->i.
> 
> Please bear with me, I think I'm confused, why would use_ats(), and if
> that's the case, don't we want to update dte->i so that it matches the
> ATS state?

I'm afraid I can't parse this. Maybe a result of incomplete editing? The
topic is complex enough that I don't want to even try to guess what you
may have meant to ask ...

> Otherwise we would fail to disable IOMMU device address translation
> support if ATS was disabled?

I think the answer here is "no", but with the above I'm not really sure
here, either.

Jan