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
