Messages in this thread | | | Date | Thu, 2 Mar 2023 10:30:25 +0800 | Subject | Re: [PATCH 1/1] iommu/vt-d: Add opt-in for ATS support on discrete devices | From | 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
| |