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

[Baolu Lu]

On 3/2/23 2:19 AM, Robin Murphy wrote:
> 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.

Intel VT-d does the same thing.

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

Make sense to me. BIOS upgrading or ACPI table overriding should help in
such cases. I will stop this patch unless there're any other special
reasons.

> Thanks,
> Robin.

Best regards,
baolu