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

  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.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 Documentation/admin-guide/kernel-parameters.txt | 6 ++++++
 drivers/iommu/intel/iommu.c                     | 7 +++++++
 2 files changed, 13 insertions(+)
  

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? all it does is effectively
disable untrusted? Why not a global option? All iommu with ATS will
need this?

Also, why doesn't a "CSP" set their ACPI to make the devices they want
to use ATS with trusted instead of this?

Jason
  

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.

> all it does is effectively
> disable untrusted?

It's irrelevant to untrusted devices.

Untrusted devices, with pci_dev->untrusted set, means device connects to
the system through some kind of untrusted external port, e.g.
thunderbolt ports. For those devices, ATS shouldn't be enabled for those
devices.

Here we are talking about soc-integrated devices vs. discrete PCI
devices (connected to the system through internal PCI slot). The problem
is that the DMAR ACPI table only defines ATS attribute for Soc-
integrated devices, which causes ATS (and its ancillary features) on the
discrete PCI devices not to work.

> Why not a global option? All iommu with ATS will
> need this?
> 
> Also, why doesn't a "CSP" set their ACPI to make the devices they want
> to use ATS with trusted instead of this?

For example, users might purchase general servers and use their own or
third-party PCIe adapters on them. They have no means to customize the
BIOS.

Best regards,
baolu
  

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.

> > all it does is effectively
> > disable untrusted?
> 
> It's irrelevant to untrusted devices.
> 
> Untrusted devices, with pci_dev->untrusted set, means device connects to
> the system through some kind of untrusted external port, e.g.
> thunderbolt ports. For those devices, ATS shouldn't be enabled for those
> devices.

Yes
 
> Here we are talking about soc-integrated devices vs. discrete PCI
> devices (connected to the system through internal PCI slot). The problem
> is that the DMAR ACPI table only defines ATS attribute for Soc-
> integrated devices, which causes ATS (and its ancillary features) on the
> discrete PCI devices not to work.

So, IMHO, it is a bug to set what Linux calls as untrusted for
discrete slots. We also definately don't want internal slots forced to
non-identity mode either, for example.

This should be addressed at the PCI layer. Untrusted should always
mean that the iommu layer should fully secure the device. It means
identity translation should be blocked, it means the HW should reject
translated requests, and ATS thus is not supported.

Every single iommu driver should implement this consistently across
the whole subsystem.

If you don't want devices to be secured then that is a PCI/bios layer
problem to get the correct data into the untrusted flag.

Not an iommu problem to ignore the flag.

Jason
  

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

Thanks,
Robin.

>>> all it does is effectively
>>> disable untrusted?
>>
>> It's irrelevant to untrusted devices.
>>
>> Untrusted devices, with pci_dev->untrusted set, means device connects to
>> the system through some kind of untrusted external port, e.g.
>> thunderbolt ports. For those devices, ATS shouldn't be enabled for those
>> devices.
> 
> Yes
>   
>> Here we are talking about soc-integrated devices vs. discrete PCI
>> devices (connected to the system through internal PCI slot). The problem
>> is that the DMAR ACPI table only defines ATS attribute for Soc-
>> integrated devices, which causes ATS (and its ancillary features) on the
>> discrete PCI devices not to work.
> 
> So, IMHO, it is a bug to set what Linux calls as untrusted for
> discrete slots. We also definately don't want internal slots forced to
> non-identity mode either, for example.
> 
> This should be addressed at the PCI layer. Untrusted should always
> mean that the iommu layer should fully secure the device. It means
> identity translation should be blocked, it means the HW should reject
> translated requests, and ATS thus is not supported.
> 
> Every single iommu driver should implement this consistently across
> the whole subsystem.
> 
> If you don't want devices to be secured then that is a PCI/bios layer
> problem to get the correct data into the untrusted flag.
> 
> Not an iommu problem to ignore the flag.
> 
> Jason
  

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()

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.

Jason
  

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.
  

On 3/1/23 10:04 PM, Jason Gunthorpe wrote:
>> Here we are talking about soc-integrated devices vs. discrete PCI
>> devices (connected to the system through internal PCI slot). The problem
>> is that the DMAR ACPI table only defines ATS attribute for Soc-
>> integrated devices, which causes ATS (and its ancillary features) on the
>> discrete PCI devices not to work.
> So, IMHO, it is a bug to set what Linux calls as untrusted for
> discrete slots. We also definately don't want internal slots forced to
> non-identity mode either, for example.

Linux doesn't set untrusted for PCI discrete slots. It reads port's
ExternalFacingPort property and set pdev->untrusted for all devices
underneath it. For ACPI compatible platforms, this property is only set
for thunderbolt ports as far as I have seen.

drivers/pci/pci-acpi.c:
1373 static void pci_acpi_set_external_facing(struct pci_dev *dev)
1374 {
1375         u8 val;
1376
1377         if (pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT)
1378                 return;
1379         if (device_property_read_u8(&dev->dev, 
"ExternalFacingPort", &val))
1380                 return;
1381
1382         /*
1383          * These root ports expose PCIe (including DMA) outside of the
1384          * system.  Everything downstream from them is external.
1385          */
1386         if (val)
1387                 dev->external_facing = 1;
1388 }

and

drivers/pci/probe.c:
1597 static void set_pcie_untrusted(struct pci_dev *dev)
1598 {
1599         struct pci_dev *parent;
1600
1601         /*
1602          * If the upstream bridge is untrusted we treat this device
1603          * untrusted as well.
1604          */
1605         parent = pci_upstream_bridge(dev);
1606         if (parent && (parent->untrusted || parent->external_facing))
1607                 dev->untrusted = true;
1608 }

> 
> This should be addressed at the PCI layer. Untrusted should always
> mean that the iommu layer should fully secure the device. It means
> identity translation should be blocked, it means the HW should reject
> translated requests, and ATS thus is not supported.
> 
> Every single iommu driver should implement this consistently across
> the whole subsystem.
> 
> If you don't want devices to be secured then that is a PCI/bios layer
> problem to get the correct data into the untrusted flag.
> 
> Not an iommu problem to ignore the flag.

The problem here is that, even for PCI *trusted* devices, the IOMMU
drivers (Intel and SMMUv3) still have a allowed list for ATS. Only
devices in this list are allowed to use ATS. This cause some PCI
discrete devices not able to use ATS even its pdev->untrust is *not*
set.

The purpose of this patch is to allow privileged users to choose to skip
the allowed list according to their wishes. So that, as long as the PCI
layer treats the device as trusted, it can use ATS in the IOMMU layer.

At present, this patch is only for VT-d driver, but I have no objection
to bring it up to the core as a common mechanism.

Best regards,
baolu
  

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
  

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Thursday, March 2, 2023 1:43 AM
> 
> 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.
> 

That is not the purpose of SATC.

The ATS support in VT-d side is reported in two interfaces:

1) "Device-TLB support" in Extended Capability Register;
2) Root port ATS capability in ACPI ATSR structure;

A device gets ATS enabled if 1/2 are true and !pdev->untrusted. Same
as SMMU does.

The main purpose of SATC is to describe which ATS-capable integrated
device meets the requirements of securely using ATS as stated in VT-d
spec 4.4. Kind of a way of building trust on the device which is fully
validated to not allow misusing translated transaction to attack memory
outside of its attached domain.

System software which cares about it could use the information in
a more restrictive ATS model (beyond !pdev->untrusted).

But this is moot in current intel-iommu driver which still makes decision
based on !pdev->untrusted as other IOMMU drivers do. SATC is used
kind of as a quick path to bypass ATSR check given no root port for
integrated devices (plus a small trick on HW managed ATS).

So I agree this patch is not required. We should not have a relax option
to bypass ATSR check anyway.

But Baolu, seems there is a small bug on handling satcu->atc_required.
This indicates that ATS must be enabled as a functional requirement.
Then we should handle the failure of pci_enable_ats() on such device.

Thanks
Kevin
  

On 2023/3/3 16:19, Tian, Kevin wrote:
> But Baolu, seems there is a small bug on handling satcu->atc_required.
> This indicates that ATS must be enabled as a functional requirement.
> Then we should handle the failure of pci_enable_ats() on such device.

Yes. We have people working on this. For example, when pci=noats is opt-
in, pci_enable_ats() definitely will return failure.

Best regards,
baolu
  

On Fri, Mar 03, 2023 at 08:19:29AM +0000, Tian, Kevin wrote:
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > Sent: Thursday, March 2, 2023 1:43 AM
> > 
> > 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.
> > 
> 
> That is not the purpose of SATC.
> 
> The ATS support in VT-d side is reported in two interfaces:
> 
> 1) "Device-TLB support" in Extended Capability Register;
> 2) Root port ATS capability in ACPI ATSR structure;
> 
> A device gets ATS enabled if 1/2 are true and !pdev->untrusted. Same
> as SMMU does.
> 
> The main purpose of SATC is to describe which ATS-capable integrated
> device meets the requirements of securely using ATS as stated in VT-d
> spec 4.4. 

Then it should be mapped to pdev->untrusted and possibly
pdev->untrusted to be enhanced to be more descriptive.

iommu driver and BIOS should have no role in security policy beyond
feeding in data to a common policy engine.

Jason
  

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Friday, March 3, 2023 9:18 PM
> 
> On Fri, Mar 03, 2023 at 08:19:29AM +0000, Tian, Kevin wrote:
> > > From: Jason Gunthorpe <jgg@nvidia.com>
> > > Sent: Thursday, March 2, 2023 1:43 AM
> > >
> > > 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.
> > >
> >
> > That is not the purpose of SATC.
> >
> > The ATS support in VT-d side is reported in two interfaces:
> >
> > 1) "Device-TLB support" in Extended Capability Register;
> > 2) Root port ATS capability in ACPI ATSR structure;
> >
> > A device gets ATS enabled if 1/2 are true and !pdev->untrusted. Same
> > as SMMU does.
> >
> > The main purpose of SATC is to describe which ATS-capable integrated
> > device meets the requirements of securely using ATS as stated in VT-d
> > spec 4.4.
> 
> Then it should be mapped to pdev->untrusted and possibly
> pdev->untrusted to be enhanced to be more descriptive.
> 
> iommu driver and BIOS should have no role in security policy beyond
> feeding in data to a common policy engine.
> 

That makes sense.