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

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

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:
> >>>> 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.
> 
> Well. On one hand I can see your point. Otoh with the spec still being the
> way it is, on systems mandating ATS use for at least one device we'd then
> simply need to deem Xen unsupported there altogether. The goal of the
> series, though, is to make things work as mandated by the spec on such
> systems, which to me implies we need to consider use of ATS supported in
> such cases (and only for those specific devices, i.e. still without
> considering use of "ats" on the command line supported).

I'm in general hesitant of ATS because I think it undermines the
security of PCI passthrough.  However this would still be acceptable
for dom0 because it's (usually?) part of the trusted base of a Xen
host.

If we want to make use of ATS for devices assigned to dom0 we should
clarify the warning in xen-command-line.pandoc.

We should also consider that dom0 usually does a lot of p2m
manipulations (by having to map grants and foreign pages).  Those will
result in p2m flushes that will lead to IOMMU flushes, and when using
ATS that will require device TLB flushes.  I wonder how much of an
overhead this will add to normal dom0 operations (plus the added risk
of those device TLB flushes stalling the IOMMU queue).

I would be much more comfortable with making the ats= command line
option a tri-state:

ats={0,1,mandatory}

Where the 'mandatory' option or equivalent enables ATS only for
devices that mandate it.  However I still think the default option
should be disabled for all devices.  If devices that require ATS are
found on the system I would use `warning_add()` to notify the user
of the need to consider adding ats=mandatory to the command line.

> If and when the spec was changed to clarify the flag is a performance hint,
> not a functional requirement, then we could do as you suggest. At which
> point, as mentioned before, opt_ats may be possible to become a plain
> boolean variable.

It's a complex situation, and I'm kind of surprised by the
introduction of this mandatory ATS requirement by Intel in a
non-backwards compatible way (as the specification claims the device
won't be functional without ATS enabled if required).

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

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

> Thinking about it: Maybe your comment roots in you meaning to leverage here
> that use_ats() is not supposed to return different values for the same device,
> when invoked multiple times. If so, I'm afraid I'm hesitant to make use of
> such a property when I can easily avoid it.
> 
> >>>> @@ -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.
> 
> Hmm, the function being static in an AMD-only file, I would have thought that
> makes it clear enough that it's AMD-specific.

Yes, sure, I guess the name looked generic enough to be something that
could be shared across vendor implementations.

Thanks, Roger.