lkml.org 
[lkml]   [2023]   [Mar]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH 1/1] iommu/vt-d: Add opt-in for ATS support on discrete devices
From
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

\
 
 \ /
  Last update: 2023-03-27 00:41    [W:0.069 / U:4.344 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site