lkml.org 
[lkml]   [2023]   [Jan]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v3 1/1] PCI: Add translated request only flag for pci_enable_pasid()
    On Sun, Jan 29, 2023 at 04:42:32PM +0800, Baolu Lu wrote:
    > On 2023/1/28 1:30, Bjorn Helgaas wrote:
    > > On Sat, Jan 14, 2023 at 03:34:20PM +0800, Lu Baolu wrote:
    > > > The PCIe fabric routes Memory Requests based on the TLP address, ignoring
    > > > the PASID. In order to ensure system integrity, commit 201007ef707a ("PCI:
    > > > Enable PASID only when ACS RR & UF enabled on upstream path") requires
    > > > some ACS features being supported on device's upstream path when enabling
    > > > PCI/PASID.
    > > >
    > > > One alternative is ATS/PRI which lets the device resolve the PASID + addr
    > > > pair before a memory request is made into a routeable TLB address through
    > > > the translation agent.
    > >
    > > This sounds like "ATS/PRI" is a solution to a problem, but we haven't
    > > stated the problem yet.
    > >
    > > > Those resolved addresses are then cached on the
    > > > device instead of in the IOMMU TLB and the device always sets translated
    > > > bit for PASID. One example of those devices are AMD graphic devices that
    > > > always have ACS or ATS/PRI enabled together with PASID.
    > > >
    > > > This adds a flag parameter in the pci_enable_pasid() helper, with which
    > > > the device driver could opt-in the fact that device always sets the
    > > > translated bit for PASID.
    > > >
    > > > It also applies this opt-in for AMD graphic devices. Without this change,
    > > > kernel boots to black screen on a system with below AMD graphic device:
    > > >
    > > > 00:01.0 VGA compatible controller: Advanced Micro Devices, Inc.
    > > > [AMD/ATI] Wani [Radeon R5/R6/R7 Graphics] (rev ca)
    > > > (prog-if 00 [VGA controller])
    > > > DeviceName: ATI EG BROADWAY
    > > > Subsystem: Hewlett-Packard Company Device 8332
    > >
    > > What is the underlying failure here? "Black screen" is useful but we
    > > should say *why* that happens, e.g., transactions went the wrong place
    > > or whatever.

    > > I guess PCI_PASID_XLATED_REQ_ONLY is something only the driver knows,
    > > right? We can't deduce from architected config space that the device
    > > will produce PASID prefixes for every Memory Request, can we?
    >
    > No, we can't. That's the reason why we need a flag here.

    Sorry, I'm still confused. PCI_PASID_XLATED_REQ_ONLY is a
    device-specific property, and you want to opt-in AMD graphics devices.
    Where's the AMD graphics-specific change? The current patch does
    this:

    pdev_pri_ats_enable
    pci_enable_pasid(pdev, 0, PCI_PASID_XLATED_REQ_ONLY)

    which looks like it does it for *all* devices below an AMD IOMMU,
    without any device or driver input.

    PCIe r6.0, sec 6.20.1:

    A Function is not permitted to generate Requests using Translated
    Addresses and a PASID unless both PASID Enable and Translated
    Requests with PASID Enable are Set.

    You want AMD graphics devices to do DMA with translated addresses and
    PASID, right? pci_enable_pasid() sets PASID Enable
    (PCI_PASID_CTRL_ENABLE), but I don't see where "Translated Requests
    with PASID Enable" is set. We don't even have a #define for it.

    I would think we should check "Translated Requests with PASID
    Supported" before setting "Translated Requests with PASID Enable",
    too?

    > PCI: Add translated request only flag for pci_enable_pasid()
    >
    > The PCIe fabric routes Memory Requests based on the TLP address, ignoring
    > the PASID. In order to ensure system integrity, commit 201007ef707a ("PCI:
    > Enable PASID only when ACS RR & UF enabled on upstream path") requires
    > some ACS features being supported on device's upstream path when enabling
    > PCI/PASID.
    >
    > However, above change causes the Linux kernel boots to black screen on a
    > system with below graphic device:

    We need a PCIe concept-level description of the issue first, i.e., in
    terms of DMA, PASID, ACS, etc. Then we can mention the AMD GPU issue
    as an instance.

    > 00:01.0 VGA compatible controller: Advanced Micro Devices, Inc.
    > [AMD/ATI] Wani [Radeon R5/R6/R7 Graphics] (rev ca)
    > (prog-if 00 [VGA controller])
    > DeviceName: ATI EG BROADWAY
    > Subsystem: Hewlett-Packard Company Device 8332
    >
    > The kernel trace looks like below:
    >
    > Call Trace:
    > <TASK>
    > amd_iommu_attach_device+0x2e0/0x300
    > __iommu_attach_device+0x1b/0x90
    > iommu_attach_group+0x65/0xa0
    > amd_iommu_init_device+0x16b/0x250 [iommu_v2]
    > kfd_iommu_resume+0x4c/0x1a0 [amdgpu]
    > kgd2kfd_resume_iommu+0x12/0x30 [amdgpu]
    > kgd2kfd_device_init.cold+0x346/0x49a [amdgpu]
    > amdgpu_amdkfd_device_init+0x142/0x1d0 [amdgpu]
    > amdgpu_device_init.cold+0x19f5/0x1e21 [amdgpu]
    > ? _raw_spin_lock_irqsave+0x23/0x50
    > amdgpu_driver_load_kms+0x15/0x110 [amdgpu]
    > amdgpu_pci_probe+0x161/0x370 [amdgpu]
    > local_pci_probe+0x41/0x80
    > pci_device_probe+0xb3/0x220
    > really_probe+0xde/0x380
    > ? pm_runtime_barrier+0x50/0x90
    > __driver_probe_device+0x78/0x170
    > driver_probe_device+0x1f/0x90
    > __driver_attach+0xce/0x1c0
    > ? __pfx___driver_attach+0x10/0x10
    > bus_for_each_dev+0x73/0xa0
    > bus_add_driver+0x1ae/0x200
    > driver_register+0x89/0xe0
    > ? __pfx_init_module+0x10/0x10 [amdgpu]
    > do_one_initcall+0x59/0x230
    > do_init_module+0x4a/0x200
    > __do_sys_init_module+0x157/0x180
    > do_syscall_64+0x5b/0x80
    > ? handle_mm_fault+0xff/0x2f0
    > ? do_user_addr_fault+0x1ef/0x690
    > ? exc_page_fault+0x70/0x170
    > entry_SYSCALL_64_after_hwframe+0x72/0xdc

    The stack trace doesn't seem like it shows a failure, so I'm not sure
    it's useful this time. If it is, we can at least strip out the
    irrelevant pieces.

    > The AMD iommu driver allocates a new domain (called v2 domain) for the

    "v2 domain" needs to be something greppable -- an identifier,
    filename, etc.

    > amdgpu device and enables its PCI PASID/ATS/PRI before attaching the
    > v2 domain to it. The failure of pci_enable_pasid() due to lack of ACS
    > causes the domain attaching device to fail. The amdgpu device is unable
    > to DMA normally, resulting in a black screen of the system.
    >
    > However, this device is special as it relies on ATS/PRI to resolve the
    > PASID + addr pair before a memory request is made into a routeable TLB
    > address through the translation agent. Those resolved addresses are then
    > cached on the device instead of in the IOMMU TLB and the device always
    > uses translated memory request for PASID.
    >
    > ACS is not necessary for the devices that always use translated memory
    > request for PASID. But this is device specific and only device driver
    > knows this. We can't deduce this from architected config space.
    >
    > Add a flag for pci_enable_pasid(), with which the device drivers could
    > opt-in the fact that device always uses translated memory requests for
    > PASID hence the ACS is not a necessity. Apply this opt-in for above AMD
    > graphic device.
    >
    > At present, it is a common practice to enable/disable PCI PASID in the
    > iommu drivers. Considering that the device driver knows more about the
    > specific device, it's better to move pci_enable_pasid() into the specific
    > device drivers.
    > [-- end --]
    >
    > --
    > Best regards,
    > baolu

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