On Fri, Jul 16, 2021 at 07:12:16PM +0800, Dongdong Liu wrote:

Hi Bjorn

Many thanks for your review.

On 2021/7/16 1:23, Bjorn Helgaas wrote:

quoted

[+cc Logan]

On Mon, Jun 21, 2021 at 06:27:20PM +0800, Dongdong Liu wrote:

quoted

10-Bit Tag capability, introduced in PCIe-4.0 increases the total Tag
field size from 8 bits to 10 bits.

For platforms where the RC supports 10-Bit Tag Completer capability,
it is highly recommended for platform firmware or operating software

Recommended by whom?  If the spec recommends it, we should provide the
citation.

PCIe spec 5.0 r1.0 section 2.2.6.2 IMPLEMENTATION NOTE says that.
Will fix.

Thanks, that will be helpful.

quoted

quoted

that configures PCIe hierarchies to Set the 10-Bit Tag Requester Enable
bit automatically in Endpoints with 10-Bit Tag Requester capability. This
enables the important class of 10-Bit Tag capable adapters that send
Memory Read Requests only to host memory.

What is the implication for P2PDMA?  What happens if we enable 10-bit
tags for device A, and A generates Mem Read Requests to device B,
which does not support 10-bit tags?

PCIe spec 5.0 r1.0 section 2.2.6.2 says
If an Endpoint supports sending Requests to other Endpoints (as opposed to
host memory), the Endpoint must not send 10-Bit Tag Requests to another
given Endpoint unless an implementation-specific mechanism determines that
the Endpoint supports 10-Bit Tag Completer capability. Not sending 10-Bit
Tag Requests to other Endpoints at all
may be acceptable for some implementations. More sophisticated mechanisms
are outside the scope of this specification.

Not sending 10-Bit Tag Requests to other Endpoints at all seems simple.
Add kernel parameter pci=pcie_bus_peer2peer when boot kernel with P2PDMA,
then do not config 10-BIT Tag.

if (pcie_bus_config != PCIE_BUS_PEER2PEER)
	pci_configure_10bit_tags(dev);

Seems like a reasonable start.  I wish this were more dynamic and we
didn't have to rely on a kernel parameter to make P2PDMA safe, but
that seems to be the current situation.

Does the same consideration apply to enabling Extended Tags (8-bit
tags)?  I would guess so, but sec 2.2.6.2 says "Receivers/Completers
must handle 8-bit Tag values correctly regardless of the setting of
their Extended Tag Field Enable bit" so there's some subtlety there
with regard to what "Extended Tag Field Supported" means.

I don't know why the "Extended Tag Field Supported" bit exists if all
receivers are required to support 8-bit tags.

If we need a similar change to pci_configure_extended_tags() to check
pcie_bus_config, that should be a separate patch because it would be a
bug fix independent of 10-bit tag support.

Bjorn