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

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

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:
> >> Make the variable a tristate, with (as done elsewhere) a negative value
> >> meaning "default". Since all use sites need looking at, also rename it
> >> to match our usual "opt_*" pattern. While touching it, also move it to
> >> .data.ro_after_init.
> > 
> > I guess I need to look at further patches, as given the feedback on
> > the past version I think we agreed we want to set ATS unconditionally
> > disabled by default, and hence I'm not sure I see the point of the
> > tri-state if enabling ATS will require an explicit opt-in on the
> > command line (ats=1).
> 
> With the present wording in the VT-d spec (which we've now had vague
> indication that it may not be meant that way) there needs to be
> tristate behavior:
> - With "ats=0" ATS won't be used.
> - With "ats=1" ATS will be used for all ATS-capable devices.
> - Without either option ATS will be used for devices where firmware
>   mandates its use.

I'm afraid I don't agree to this behavior.  Regardless of what the
firmware requests ATS must only be enabled on user-request (iow: when
the ats=1 command line option is passed).  Otherwise ATS must remain
disabled for all devices.  It's not fine for firmware to trigger the
enabling of a feature that's not supported on Xen.

> >> @@ -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().

> >> @@ -257,7 +257,7 @@ static int __must_check amd_iommu_setup_
> >>                                           ACPI_IVHD_SYSTEM_MGMT));
> >>  
> >>          if ( use_ats(pdev, iommu, ivrs_dev) )
> >> -            ASSERT(dte->i == ats_enabled);
> >> +            ASSERT(dte->i);
> > 
> > ASSERT(dte->i == use_ats(pdev, iommu, ivrs_dev));
> 
> I'm okay switching here, but better to the precise logical equivalent of
> the earlier code:
> 
> ASSERT(dte->i || !use_ats(pdev, iommu, ivrs_dev));

Hm, I see.  I think we should be more strict with this (see my
previous comment), but we could defer to a later change.

> 
> >> @@ -43,7 +43,7 @@ static inline int pci_ats_enabled(int se
> >>  
> >>  static inline int pci_ats_device(int seg, int bus, int devfn)
> >>  {
> >> -    if ( !ats_enabled )
> >> +    if ( !opt_ats )
> >>          return 0;
> > 
> > Can't you remove that check altogether now, since you are adding an
> > opt_ats check to use_ats()?
> 
> Two reasons why not: For one this isn't AMD-specific code, and hence
> shouldn't be tied to the AMD-specific use_ats(). In principle VT-d
> code should be okay to call here, too. And then
> amd_iommu_disable_domain_device() doesn't use use_ats(), but does call
> here.

Oh, that's confusing, I didn't realize use_ats was AMD specific code.
It should have some kind of prefix to avoid this kind of confusion.

Thanks, Roger.