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

[Baolu Lu <baolu.lu@linux.intel.com>]

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