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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.05.2024 12:03, Roger Pau Monné wrote:
> On Tue, May 21, 2024 at 08:21:35AM +0200, Jan Beulich wrote:
>> 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 ...
> 
> Oh, indeed, sorry, the full sentences should have been:
> 
> Please bear with me, I think I'm confused, why would use_ats() return
> different values for the same device?

Right now it can't, yet in principle opt_ats could change value when
wired up accordingly via hypfs.

> And if that's the case, don't we want to update dte->i so that it
> matches the ATS state signaled by use_ats()?

That depends on what adjustment would be done besides changing the
variable value, if that was to become runtime changeable.

>>> 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.
> 
> Given the current logic in use_ats() AFAICT the return value of that
> function should not change for a given device?

Right now it shouldn't, yes. I'm still a little hesitant to have callers
make implications from that.

Jan