Re: [PATCH 1/1] iommu/vt-d: Add opt-in for ATS support on discrete devices

[Robin Murphy]

On 2023-03-01 17:42, Jason Gunthorpe wrote:
> On Wed, Mar 01, 2023 at 05:15:33PM +0000, Robin Murphy wrote:
> > On 2023-03-01 14:04, Jason Gunthorpe wrote:
> > > On Wed, Mar 01, 2023 at 12:22:23PM +0800, Baolu Lu wrote:
> > > > On 2/28/23 8:23 PM, Jason Gunthorpe wrote:
> > > > > On Tue, Feb 28, 2023 at 10:33:41AM +0800, Lu Baolu wrote:
> > > > > > In normal processing of PCIe ATS requests, the IOMMU performs address
> > > > > > translation and returns the device a physical memory address which
> > > > > > will be stored in that device's IOTLB. The device may subsequently
> > > > > > issue Translated DMA request containing physical memory address. The
> > > > > > IOMMU only checks that the device was allowed to issue such requests
> > > > > > and does not attempt to validate the physical address.
> > > > > >
> > > > > > The Intel IOMMU implementation only allows PCIe ATS on several SOC-
> > > > > > integrated devices which are opt-in’ed through the ACPI tables to
> > > > > > prevent any compromised device from accessing arbitrary physical
> > > > > > memory.
> > > > > >
> > > > > > Add a kernel option intel_iommu=relax_ats to allow users to have an
> > > > > > opt-in to allow turning on ATS at as wish, especially for CSP-owned
> > > > > > vertical devices. In any case, risky devices are not allowed to use
> > > > > > ATS.
> > > > > Why is this an intel specific option?
> > > >
> > > > I only see similar situation on ARM SMMUv3 platforms. The device ATS is
> > > > only allowed when the ATS bit is set in RC node of the ACPI/IORT table.
> > >
> > > It should be common, all iommus using ATS need this logic.
> >
> > The IORT flags are not this kind of policy, they are a necessary description
> > of the hardware. The mix-and-match nature of licensable IP means that just
> > because an SMMU supports the ATS-relevant features defined by the SMMU
> > architecture, that doesn't say that whatever PCIe IP the customer has chosen
> > to pair it with also supports ATS. Even when both ends nominally support it,
> > it's still possible to integrate them together in ways where ATS wouldn't be
> > functional.
> >
> > In general, if a feature is marked as unsupported in IORT, the only way to
> > "relax" that would be if you have a silicon fab handy. If any system vendor
> > *was* to misuse IORT to impose arbitrary and unwelcome usage policy on their
> > customers, then those customers should demand a firmware update (or at least
> > use their own patched IORT, which is pretty trivial with the kernel's
> > existing ACPI table override mechanism).
>
> This makes sense.
>
> I think Intel has confused their version of the IORT.
>
> The ACPI tables read by the iommu driver should be strictly about
> IOMMU HW capability, like Robin describes for ARM.
>
> Security policy flows through the ExternalFacingPort ACPI via
> pci_acpi_set_external_facing() and triggers pdev->untrusted.
>
> When an iommu driver sees pdev->untrusted it is supposed to ensure
> that translated TLPs are blocked. Since nothing does this explicitly
> it is presumably happening because ATS being disabled also blocks
> translated TLPs and we check untrusted as part of pci_enable_ats()

At least for SMMU, we seem to be relying on pci_ats_supported() 
including pdev->untrusted in its decision - that will propagate back to 
master->ats_enabled = false inside the driver, which in turn will lead 
to arm_smmu_write_strtab_ent() leaving STE.EATS at the default setting 
which aborts all translation requests and translated transactions.

> If Intel BIOS's have populated the "satcu" to say that ATS is not
> supported by the HW when the HW supports ATS perfectly fine, then get
> the BIOS fixed or patch the ACPI until it is fixed. The BIOS should
> not be saying that the HW does not support ATS when it does, it is a
> simple BIOS bug.
>
> Alternatively if you have some definitive way to know that the HW
> supports ATS then you should route the satcu information to
> pdev->untrusted and ignore it at the iommu driver level.

From a quick look at the VT-d spec, it sounds like the ATSR structure 
is intended to be functionally equivalent to IORT's Root Complex "ATS 
Attribute", while the SATC is a slightly specialised version for RCiEPs. 
The spec even says "Software must enable ATS on endpoint devices behind 
a Root Port only if the Root Port is reported as supporting ATS 
transactions". It also seems to be implied that this should be based on 
what Intel themselves have validated, so an option for the user to say 
"sure, ATS works everywhere, I know better" and simply bypass all the 
existing checks doesn't really seem safe to me :/

I'd be inclined to hold the same opinion as for IORT here - if a user 
ever really does need to engage expert mode to safely work around a bad 
BIOS with known-good information, they should already have the tools to 
override the whole DMAR table as they see fit.

Thanks,
Robin.