linux-kernel - Re: [PATCH v3 1/1] PCI: Add translated request only flag for pci_enable

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230127173035.GA994835@bhelgaas>
Date:   Fri, 27 Jan 2023 11:30:35 -0600
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Lu Baolu <baolu.lu@...ux.intel.com>
Cc:     Bjorn Helgaas <bhelgaas@...gle.com>,
        Joerg Roedel <jroedel@...e.de>,
        Matt Fagnani <matt.fagnani@...l.net>,
        Christian König <christian.koenig@....com>,
        Jason Gunthorpe <jgg@...dia.com>,
        Kevin Tian <kevin.tian@...el.com>,
        Vasant Hegde <vasant.hegde@....com>,
        Tony Zhu <tony.zhu@...el.com>, linux-pci@...r.kernel.org,
        iommu@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 1/1] PCI: Add translated request only flag for
 pci_enable_pasid()

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.

Nit: "Add a flag ..." and "Apply this opt-in ..." (below).

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

> 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, we will follow up by moving pci_enable_pasid() into
> the specific device drivers.

> @@ -353,12 +353,15 @@ void pci_pasid_init(struct pci_dev *pdev)
>   * pci_enable_pasid - Enable the PASID capability
>   * @pdev: PCI device structure
>   * @features: Features to enable
> + * @flags: device-specific flags
> + *   - PCI_PASID_XLATED_REQ_ONLY: The PCI device always use translated type
> + *                                for all PASID memory requests.

s/use/uses/

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?

Bjorn