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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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?

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()?

> > 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?

Thanks, Roger.