[PATCH V2 0/9] PCIe TPH and cache direct injection support

[PATCH V2 0/9] PCIe TPH and cache direct injection support

[Wei Huang]

Hi All,

TPH (TLP Processing Hints) is a PCIe feature that allows endpoint devices
to provide optimization hints for requests that target memory space. These
hints, in a format called steering tag (ST), are provided in the requester's
TLP headers and allow the system hardware, including the Root Complex, to
optimize the utilization of platform resources for the requests.

Upcoming AMD hardware implement a new Cache Injection feature that leverages
TPH. Cache Injection allows PCIe endpoints to inject I/O Coherent DMA writes
directly into an L2 within the CCX (core complex) closest to the CPU core
that will consume it. This technology is aimed at applications requiring high
performance and low latency, such as networking and storage applications.

This series introduces generic TPH support in Linux, allowing STs to be
retrieved from ACPI _DSM (as defined by ACPI) and used by PCIe endpoint
drivers as needed. As a demonstration, it includes an example usage in the
Broadcom BNXT driver. When running on Broadcom NICs with the appropriate
firmware, Cache Injection shows substantial memory bandwidth savings in
real-world benchmarks. This solution is vendor-neutral, as both TPH and ACPI
_DSM are industry standards.

V1->V2:
 * Rebase on top of pci.git/for-linus (6.10-rc1)
 * Address mismatched data types reported by Sparse (Sparse checking passed)
 * Add a new API, pcie_tph_intr_vec_supported(), for checking IRQ mode support
 * Skip bnxt affinity notifier registration if pcie_tph_intr_vec_supported()=false
 * Minor fixes in bnxt driver (i.e. warning messages)

Manoj Panicker (1):
  bnxt_en: Add TPH support in BNXT driver

Michael Chan (1):
  bnxt_en: Pass NQ ID to the FW when allocating RX/RX AGG rings

Wei Huang (8):
  PCI: Introduce PCIe TPH support framework
  PCI: Add TPH related register definition
  PCI/TPH: Implement a command line option to disable TPH
  PCI/TPH: Implement a command line option to force No ST Mode
  PCI/TPH: Introduce API functions to manage steering tags
  PCI/TPH: Retrieve steering tag from ACPI _DSM
  PCI/TPH: Add TPH documentation

 Documentation/PCI/index.rst                   |   1 +
 Documentation/PCI/tph.rst                     |  57 ++
 .../admin-guide/kernel-parameters.txt         |   2 +
 Documentation/driver-api/pci/pci.rst          |   3 +
 drivers/net/ethernet/broadcom/bnxt/bnxt.c     |  62 +-
 drivers/net/ethernet/broadcom/bnxt/bnxt.h     |   4 +
 drivers/pci/pci-driver.c                      |  12 +-
 drivers/pci/pci.c                             |  24 +
 drivers/pci/pci.h                             |   6 +
 drivers/pci/pcie/Kconfig                      |  10 +
 drivers/pci/pcie/Makefile                     |   1 +
 drivers/pci/pcie/tph.c                        | 582 ++++++++++++++++++
 drivers/pci/probe.c                           |   1 +
 drivers/vfio/pci/vfio_pci_config.c            |   7 +-
 include/linux/pci-tph.h                       |  78 +++
 include/linux/pci.h                           |   6 +
 include/uapi/linux/pci_regs.h                 |  35 +-
 17 files changed, 881 insertions(+), 10 deletions(-)
 create mode 100644 Documentation/PCI/tph.rst
 create mode 100644 drivers/pci/pcie/tph.c
 create mode 100644 include/linux/pci-tph.h

-- 
2.44.0

[PATCH V2 1/9] PCI: Introduce PCIe TPH support framework

[Wei Huang]

This patch implements the framework for PCIe TPH support. It introduces
tph.c source file, along with CONFIG_PCIE_TPH, to Linux PCIe subsystem.
A new member, named tph_cap, is also introduced in pci_dev to cache TPH
capability offset.

Co-developed-by: Eric Van Tassell <Eric.VanTassell@amd.com>
Signed-off-by: Eric Van Tassell <Eric.VanTassell@amd.com>
Signed-off-by: Wei Huang <wei.huang2@amd.com>
Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com> 
Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
---
 drivers/pci/pci.h         |  6 ++++++
 drivers/pci/pcie/Kconfig  | 10 ++++++++++
 drivers/pci/pcie/Makefile |  1 +
 drivers/pci/pcie/tph.c    | 28 ++++++++++++++++++++++++++++
 drivers/pci/probe.c       |  1 +
 include/linux/pci.h       |  4 ++++
 6 files changed, 50 insertions(+)
 create mode 100644 drivers/pci/pcie/tph.c

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index fd44565c4756..b371b5b45f86 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -506,6 +506,12 @@ static inline int pci_iov_bus_range(struct pci_bus *bus)
 
 #endif /* CONFIG_PCI_IOV */
 
+#ifdef CONFIG_PCIE_TPH
+void pcie_tph_init(struct pci_dev *dev);
+#else
+static inline void pcie_tph_init(struct pci_dev *dev) {}
+#endif
+
 #ifdef CONFIG_PCIE_PTM
 void pci_ptm_init(struct pci_dev *dev);
 void pci_save_ptm_state(struct pci_dev *dev);
diff --git a/drivers/pci/pcie/Kconfig b/drivers/pci/pcie/Kconfig
index 17919b99fa66..d22857325b3e 100644
--- a/drivers/pci/pcie/Kconfig
+++ b/drivers/pci/pcie/Kconfig
@@ -155,3 +155,13 @@ config PCIE_EDR
 	  the PCI Firmware Specification r3.2.  Enable this if you want to
 	  support hybrid DPC model which uses both firmware and OS to
 	  implement DPC.
+
+config PCIE_TPH
+	bool "TLP Processing Hints"
+	default n
+	help
+	  This option adds support for PCIE TLP Processing Hints (TPH).
+	  TPH allows endpoint devices to provide optimization hints, such as
+	  desired caching behavior, for requests that target memory space.
+	  These hints, called steering tags, can empower the system hardware
+	  to optimize the utilization of platform resources.
diff --git a/drivers/pci/pcie/Makefile b/drivers/pci/pcie/Makefile
index 6461aa93fe76..3542b42ea0b9 100644
--- a/drivers/pci/pcie/Makefile
+++ b/drivers/pci/pcie/Makefile
@@ -13,3 +13,4 @@ obj-$(CONFIG_PCIE_PME)		+= pme.o
 obj-$(CONFIG_PCIE_DPC)		+= dpc.o
 obj-$(CONFIG_PCIE_PTM)		+= ptm.o
 obj-$(CONFIG_PCIE_EDR)		+= edr.o
+obj-$(CONFIG_PCIE_TPH)		+= tph.o
diff --git a/drivers/pci/pcie/tph.c b/drivers/pci/pcie/tph.c
new file mode 100644
index 000000000000..5f0cc06b74bb
--- /dev/null
+++ b/drivers/pci/pcie/tph.c
@@ -0,0 +1,28 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * TPH (TLP Processing Hints) support
+ *
+ * Copyright (C) 2024 Advanced Micro Devices, Inc.
+ *     Eric Van Tassell <Eric.VanTassell@amd.com>
+ *     Wei Huang <wei.huang2@amd.com>
+ */
+
+#define pr_fmt(fmt) "TPH: " fmt
+#define dev_fmt pr_fmt
+
+#include <linux/acpi.h>
+#include <uapi/linux/pci_regs.h>
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/msi.h>
+#include <linux/pci.h>
+#include <linux/msi.h>
+#include <linux/pci-acpi.h>
+
+#include "../pci.h"
+
+void pcie_tph_init(struct pci_dev *dev)
+{
+	dev->tph_cap = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_TPH);
+}
+
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 15168881ec94..1f1ae55a5f83 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2484,6 +2484,7 @@ static void pci_init_capabilities(struct pci_dev *dev)
 	pci_dpc_init(dev);		/* Downstream Port Containment */
 	pci_rcec_init(dev);		/* Root Complex Event Collector */
 	pci_doe_init(dev);		/* Data Object Exchange */
+	pcie_tph_init(dev);             /* TLP Processing Hints */
 
 	pcie_report_downtraining(dev);
 	pci_init_reset_methods(dev);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 5bece7fd11f8..d75a88ec5136 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -530,6 +530,10 @@ struct pci_dev {
 
 	/* These methods index pci_reset_fn_methods[] */
 	u8 reset_methods[PCI_NUM_RESET_METHODS]; /* In priority order */
+
+#ifdef CONFIG_PCIE_TPH
+	u16 tph_cap; /* TPH capability offset */
+#endif
 };
 
 static inline struct pci_dev *pci_physfn(struct pci_dev *dev)
-- 
2.44.0

Re: [PATCH V2 1/9] PCI: Introduce PCIe TPH support framework

[Jonathan Cameron]

On Fri, 31 May 2024 16:38:33 -0500
Wei Huang <wei.huang2@amd.com> wrote:

> This patch implements the framework for PCIe TPH support. It introduces
> tph.c source file, along with CONFIG_PCIE_TPH, to Linux PCIe subsystem.
> A new member, named tph_cap, is also introduced in pci_dev to cache TPH
> capability offset.
> 
> Co-developed-by: Eric Van Tassell <Eric.VanTassell@amd.com>
> Signed-off-by: Eric Van Tassell <Eric.VanTassell@amd.com>
> Signed-off-by: Wei Huang <wei.huang2@amd.com>
> Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com> 
> Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>


One trivial comment inline.
With that fixed.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> diff --git a/drivers/pci/pcie/tph.c b/drivers/pci/pcie/tph.c
> new file mode 100644
> index 000000000000..5f0cc06b74bb
> --- /dev/null
> +++ b/drivers/pci/pcie/tph.c
> @@ -0,0 +1,28 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * TPH (TLP Processing Hints) support
> + *
> + * Copyright (C) 2024 Advanced Micro Devices, Inc.
> + *     Eric Van Tassell <Eric.VanTassell@amd.com>
> + *     Wei Huang <wei.huang2@amd.com>
> + */
> +
> +#define pr_fmt(fmt) "TPH: " fmt
> +#define dev_fmt pr_fmt
> +
> +#include <linux/acpi.h>
> +#include <uapi/linux/pci_regs.h>
> +#include <linux/kernel.h>
> +#include <linux/errno.h>
> +#include <linux/msi.h>
> +#include <linux/pci.h>
> +#include <linux/msi.h>
> +#include <linux/pci-acpi.h>

Introduce headers as you first use them.  That way we can more
easily see if there are unused ones in this list.


> +
> +#include "../pci.h"
> +
> +void pcie_tph_init(struct pci_dev *dev)
> +{
> +	dev->tph_cap = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_TPH);
> +}
> +
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 15168881ec94..1f1ae55a5f83 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2484,6 +2484,7 @@ static void pci_init_capabilities(struct pci_dev *dev)
>  	pci_dpc_init(dev);		/* Downstream Port Containment */
>  	pci_rcec_init(dev);		/* Root Complex Event Collector */
>  	pci_doe_init(dev);		/* Data Object Exchange */
> +	pcie_tph_init(dev);             /* TLP Processing Hints */
>  
>  	pcie_report_downtraining(dev);
>  	pci_init_reset_methods(dev);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 5bece7fd11f8..d75a88ec5136 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -530,6 +530,10 @@ struct pci_dev {
>  
>  	/* These methods index pci_reset_fn_methods[] */
>  	u8 reset_methods[PCI_NUM_RESET_METHODS]; /* In priority order */
> +
> +#ifdef CONFIG_PCIE_TPH
> +	u16 tph_cap; /* TPH capability offset */
> +#endif
>  };
>  
>  static inline struct pci_dev *pci_physfn(struct pci_dev *dev)

Re: [PATCH V2 1/9] PCI: Introduce PCIe TPH support framework

[Bjorn Helgaas]

On Fri, May 31, 2024 at 04:38:33PM -0500, Wei Huang wrote:
> This patch implements the framework for PCIe TPH support. It introduces
> tph.c source file, along with CONFIG_PCIE_TPH, to Linux PCIe subsystem.
> A new member, named tph_cap, is also introduced in pci_dev to cache TPH
> capability offset.

s/This patch implements/Implement/
s/It introduces/Introduce/
s/is also introduced/Add tph_cap .../

https://chris.beams.io/posts/git-commit/
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=v6.9#n94

> +	  This option adds support for PCIE TLP Processing Hints (TPH).
> +	  TPH allows endpoint devices to provide optimization hints, such as
> +	  desired caching behavior, for requests that target memory space.
> +	  These hints, called steering tags, can empower the system hardware
> +	  to optimize the utilization of platform resources.

s/PCIE TLP/PCIe TLP/ to match context.

> +++ b/drivers/pci/pcie/tph.c

> +#define pr_fmt(fmt) "TPH: " fmt
> +#define dev_fmt pr_fmt

Add when used.

> +void pcie_tph_init(struct pci_dev *dev)
> +{
> +	dev->tph_cap = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_TPH);
> +}
> +

  $ git am m/v2_20240531_wei_huang2_pcie_tph_and_cache_direct_injection_support.mbx
  Applying: PCI: Introduce PCIe TPH support framework
  .git/rebase-apply/patch:88: new blank line at EOF.
  +
  warning: 1 line adds whitespace errors.

[PATCH V2 2/9] PCI: Add TPH related register definition

[Wei Huang]

Linux has some basic, but incomplete, definition for the TPH Requester
capability registers. Also the control registers of TPH Requester and
the TPH Completer are missing. This patch adds all required definitions
to support TPH enablement.

Co-developed-by: Eric Van Tassell <Eric.VanTassell@amd.com>
Signed-off-by: Eric Van Tassell <Eric.VanTassell@amd.com>
Signed-off-by: Wei Huang <wei.huang2@amd.com>
Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com> 
Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
---
 drivers/vfio/pci/vfio_pci_config.c |  7 +++---
 include/uapi/linux/pci_regs.h      | 35 ++++++++++++++++++++++++++----
 2 files changed, 35 insertions(+), 7 deletions(-)

diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c
index 97422aafaa7b..de622cdfc2a4 100644
--- a/drivers/vfio/pci/vfio_pci_config.c
+++ b/drivers/vfio/pci/vfio_pci_config.c
@@ -1434,14 +1434,15 @@ static int vfio_ext_cap_len(struct vfio_pci_core_device *vdev, u16 ecap, u16 epo
 		if (ret)
 			return pcibios_err_to_errno(ret);
 
-		if ((dword & PCI_TPH_CAP_LOC_MASK) == PCI_TPH_LOC_CAP) {
+		if (((dword & PCI_TPH_CAP_LOC_MASK) >> PCI_TPH_CAP_LOC_SHIFT)
+			== PCI_TPH_LOC_CAP) {
 			int sts;
 
 			sts = dword & PCI_TPH_CAP_ST_MASK;
 			sts >>= PCI_TPH_CAP_ST_SHIFT;
-			return PCI_TPH_BASE_SIZEOF + (sts * 2) + 2;
+			return PCI_TPH_ST_TABLE + (sts * 2) + 2;
 		}
-		return PCI_TPH_BASE_SIZEOF;
+		return PCI_TPH_ST_TABLE;
 	case PCI_EXT_CAP_ID_DVSEC:
 		ret = pci_read_config_dword(pdev, epos + PCI_DVSEC_HEADER1, &dword);
 		if (ret)
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index 94c00996e633..ae1cf048b04a 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -657,6 +657,7 @@
 #define  PCI_EXP_DEVCAP2_ATOMIC_COMP64	0x00000100 /* 64b AtomicOp completion */
 #define  PCI_EXP_DEVCAP2_ATOMIC_COMP128	0x00000200 /* 128b AtomicOp completion */
 #define  PCI_EXP_DEVCAP2_LTR		0x00000800 /* Latency tolerance reporting */
+#define  PCI_EXP_DEVCAP2_TPH_COMP	0x00003000 /* TPH completer support */
 #define  PCI_EXP_DEVCAP2_OBFF_MASK	0x000c0000 /* OBFF support mechanism */
 #define  PCI_EXP_DEVCAP2_OBFF_MSG	0x00040000 /* New message signaling */
 #define  PCI_EXP_DEVCAP2_OBFF_WAKE	0x00080000 /* Re-use WAKE# for OBFF */
@@ -1020,15 +1021,41 @@
 #define  PCI_DPA_CAP_SUBSTATE_MASK	0x1F	/* # substates - 1 */
 #define PCI_DPA_BASE_SIZEOF	16	/* size with 0 substates */
 
+/* TPH Completer Support */
+#define PCI_EXP_DEVCAP2_TPH_COMP_SHIFT		12
+#define PCI_EXP_DEVCAP2_TPH_COMP_NONE		0x0 /* None */
+#define PCI_EXP_DEVCAP2_TPH_COMP_TPH_ONLY	0x1 /* TPH only */
+#define PCI_EXP_DEVCAP2_TPH_COMP_TPH_AND_EXT	0x3 /* TPH and Extended TPH */
+
 /* TPH Requester */
 #define PCI_TPH_CAP		4	/* capability register */
+#define  PCI_TPH_CAP_NO_ST	0x1	/* no ST mode supported */
+#define  PCI_TPH_CAP_NO_ST_SHIFT	0x0	/* no ST mode supported shift */
+#define  PCI_TPH_CAP_INT_VEC	0x2	/* interrupt vector mode supported */
+#define  PCI_TPH_CAP_INT_VEC_SHIFT	0x1	/* interrupt vector mode supported shift */
+#define  PCI_TPH_CAP_DS		0x4	/* device specific mode supported */
+#define  PCI_TPH_CAP_DS_SHIFT	0x4	/* device specific mode supported shift */
 #define  PCI_TPH_CAP_LOC_MASK	0x600	/* location mask */
-#define   PCI_TPH_LOC_NONE	0x000	/* no location */
-#define   PCI_TPH_LOC_CAP	0x200	/* in capability */
-#define   PCI_TPH_LOC_MSIX	0x400	/* in MSI-X */
+#define  PCI_TPH_CAP_LOC_SHIFT	9	/* location shift */
+#define   PCI_TPH_LOC_NONE	0x0	/*  no ST Table */
+#define   PCI_TPH_LOC_CAP	0x1	/*  ST Table in extended capability */
+#define   PCI_TPH_LOC_MSIX	0x2	/*  ST table in MSI-X table */
 #define PCI_TPH_CAP_ST_MASK	0x07FF0000	/* ST table mask */
 #define PCI_TPH_CAP_ST_SHIFT	16	/* ST table shift */
-#define PCI_TPH_BASE_SIZEOF	0xc	/* size with no ST table */
+
+#define PCI_TPH_CTRL		0x8	/* control register */
+#define  PCI_TPH_CTRL_MODE_SEL_MASK	0x7	/* ST Model Select mask */
+#define  PCI_TPH_CTRL_MODE_SEL_SHIFT	0x0	/* ST Model Select shift */
+#define   PCI_TPH_NO_ST_MODE		0x0	/*  No ST Mode */
+#define   PCI_TPH_INT_VEC_MODE		0x1	/*  Interrupt Vector Mode */
+#define   PCI_TPH_DEV_SPEC_MODE		0x2	/*  Device Specific Mode */
+#define  PCI_TPH_CTRL_REQ_EN_MASK	0x300	/* TPH Requester mask */
+#define  PCI_TPH_CTRL_REQ_EN_SHIFT	8	/* TPH Requester shift */
+#define   PCI_TPH_REQ_DISABLE		0x0	/*  No TPH request allowed */
+#define   PCI_TPH_REQ_TPH_ONLY		0x1	/*  8-bit TPH tags allowed */
+#define   PCI_TPH_REQ_EXT_TPH		0x3	/*  16-bit TPH tags allowed */
+
+#define PCI_TPH_ST_TABLE	0xc	/* base of ST table */
 
 /* Downstream Port Containment */
 #define PCI_EXP_DPC_CAP			0x04	/* DPC Capability */
-- 
2.44.0

Re: [PATCH V2 2/9] PCI: Add TPH related register definition

[Jonathan Cameron]

On Fri, 31 May 2024 16:38:34 -0500
Wei Huang <wei.huang2@amd.com> wrote:

> Linux has some basic, but incomplete, definition for the TPH Requester
> capability registers. Also the control registers of TPH Requester and
> the TPH Completer are missing. This patch adds all required definitions
> to support TPH enablement.
> 
> Co-developed-by: Eric Van Tassell <Eric.VanTassell@amd.com>
> Signed-off-by: Eric Van Tassell <Eric.VanTassell@amd.com>
> Signed-off-by: Wei Huang <wei.huang2@amd.com>
> Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com> 
> Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>

As below, you can't modify uapi headers because we have no idea what
userspace code is already using them.
Also, (annoyingly) the field contents in this header tend (or maybe always are)
in the shifted form.

> ---
>  drivers/vfio/pci/vfio_pci_config.c |  7 +++---
>  include/uapi/linux/pci_regs.h      | 35 ++++++++++++++++++++++++++----
>  2 files changed, 35 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c
> index 97422aafaa7b..de622cdfc2a4 100644
> --- a/drivers/vfio/pci/vfio_pci_config.c
> +++ b/drivers/vfio/pci/vfio_pci_config.c
> @@ -1434,14 +1434,15 @@ static int vfio_ext_cap_len(struct vfio_pci_core_device *vdev, u16 ecap, u16 epo
>  		if (ret)
>  			return pcibios_err_to_errno(ret);
>  
> -		if ((dword & PCI_TPH_CAP_LOC_MASK) == PCI_TPH_LOC_CAP) {
> +		if (((dword & PCI_TPH_CAP_LOC_MASK) >> PCI_TPH_CAP_LOC_SHIFT)
> +			== PCI_TPH_LOC_CAP) {
>  			int sts;
>  
>  			sts = dword & PCI_TPH_CAP_ST_MASK;
>  			sts >>= PCI_TPH_CAP_ST_SHIFT;
> -			return PCI_TPH_BASE_SIZEOF + (sts * 2) + 2;
> +			return PCI_TPH_ST_TABLE + (sts * 2) + 2;
>  		}
> -		return PCI_TPH_BASE_SIZEOF;
> +		return PCI_TPH_ST_TABLE;
>  	case PCI_EXT_CAP_ID_DVSEC:
>  		ret = pci_read_config_dword(pdev, epos + PCI_DVSEC_HEADER1, &dword);
>  		if (ret)
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index 94c00996e633..ae1cf048b04a 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -657,6 +657,7 @@
>  #define  PCI_EXP_DEVCAP2_ATOMIC_COMP64	0x00000100 /* 64b AtomicOp completion */
>  #define  PCI_EXP_DEVCAP2_ATOMIC_COMP128	0x00000200 /* 128b AtomicOp completion */
>  #define  PCI_EXP_DEVCAP2_LTR		0x00000800 /* Latency tolerance reporting */
> +#define  PCI_EXP_DEVCAP2_TPH_COMP	0x00003000 /* TPH completer support */
>  #define  PCI_EXP_DEVCAP2_OBFF_MASK	0x000c0000 /* OBFF support mechanism */
>  #define  PCI_EXP_DEVCAP2_OBFF_MSG	0x00040000 /* New message signaling */
>  #define  PCI_EXP_DEVCAP2_OBFF_WAKE	0x00080000 /* Re-use WAKE# for OBFF */
> @@ -1020,15 +1021,41 @@
>  #define  PCI_DPA_CAP_SUBSTATE_MASK	0x1F	/* # substates - 1 */
>  #define PCI_DPA_BASE_SIZEOF	16	/* size with 0 substates */
>  
> +/* TPH Completer Support */
> +#define PCI_EXP_DEVCAP2_TPH_COMP_SHIFT		12
> +#define PCI_EXP_DEVCAP2_TPH_COMP_NONE		0x0 /* None */
> +#define PCI_EXP_DEVCAP2_TPH_COMP_TPH_ONLY	0x1 /* TPH only */
> +#define PCI_EXP_DEVCAP2_TPH_COMP_TPH_AND_EXT	0x3 /* TPH and Extended TPH */
> +
>  /* TPH Requester */
>  #define PCI_TPH_CAP		4	/* capability register */
> +#define  PCI_TPH_CAP_NO_ST	0x1	/* no ST mode supported */
> +#define  PCI_TPH_CAP_NO_ST_SHIFT	0x0	/* no ST mode supported shift */
> +#define  PCI_TPH_CAP_INT_VEC	0x2	/* interrupt vector mode supported */
> +#define  PCI_TPH_CAP_INT_VEC_SHIFT	0x1	/* interrupt vector mode supported shift */
> +#define  PCI_TPH_CAP_DS		0x4	/* device specific mode supported */
> +#define  PCI_TPH_CAP_DS_SHIFT	0x4	/* device specific mode supported shift */
>  #define  PCI_TPH_CAP_LOC_MASK	0x600	/* location mask */
> -#define   PCI_TPH_LOC_NONE	0x000	/* no location */
> -#define   PCI_TPH_LOC_CAP	0x200	/* in capability */
> -#define   PCI_TPH_LOC_MSIX	0x400	/* in MSI-X */

It's a userspace header, relatively unlikely to be safe to change it...
This would also be inconsistent with how some other registers are defined in here.

I'd love it if we could tidy this up, but we are stuck by this being
in uapi.

> +#define  PCI_TPH_CAP_LOC_SHIFT	9	/* location shift */
> +#define   PCI_TPH_LOC_NONE	0x0	/*  no ST Table */
> +#define   PCI_TPH_LOC_CAP	0x1	/*  ST Table in extended capability */
> +#define   PCI_TPH_LOC_MSIX	0x2	/*  ST table in MSI-X table */
>  #define PCI_TPH_CAP_ST_MASK	0x07FF0000	/* ST table mask */
>  #define PCI_TPH_CAP_ST_SHIFT	16	/* ST table shift */
> -#define PCI_TPH_BASE_SIZEOF	0xc	/* size with no ST table */
> +
> +#define PCI_TPH_CTRL		0x8	/* control register */
> +#define  PCI_TPH_CTRL_MODE_SEL_MASK	0x7	/* ST Model Select mask */
> +#define  PCI_TPH_CTRL_MODE_SEL_SHIFT	0x0	/* ST Model Select shift */
> +#define   PCI_TPH_NO_ST_MODE		0x0	/*  No ST Mode */
> +#define   PCI_TPH_INT_VEC_MODE		0x1	/*  Interrupt Vector Mode */
> +#define   PCI_TPH_DEV_SPEC_MODE		0x2	/*  Device Specific Mode */
> +#define  PCI_TPH_CTRL_REQ_EN_MASK	0x300	/* TPH Requester mask */
> +#define  PCI_TPH_CTRL_REQ_EN_SHIFT	8	/* TPH Requester shift */
> +#define   PCI_TPH_REQ_DISABLE		0x0	/*  No TPH request allowed */
> +#define   PCI_TPH_REQ_TPH_ONLY		0x1	/*  8-bit TPH tags allowed */
> +#define   PCI_TPH_REQ_EXT_TPH		0x3	/*  16-bit TPH tags allowed */
> +
> +#define PCI_TPH_ST_TABLE	0xc	/* base of ST table */
>  
>  /* Downstream Port Containment */
>  #define PCI_EXP_DPC_CAP			0x04	/* DPC Capability */

Re: [PATCH V2 2/9] PCI: Add TPH related register definition

[Wei Huang]

On 6/7/24 11:17, Jonathan Cameron wrote:
> On Fri, 31 May 2024 16:38:34 -0500
> Wei Huang <wei.huang2@amd.com> wrote:
>>  /* TPH Requester */
>>  #define PCI_TPH_CAP		4	/* capability register */
>> +#define  PCI_TPH_CAP_NO_ST	0x1	/* no ST mode supported */
>> +#define  PCI_TPH_CAP_NO_ST_SHIFT	0x0	/* no ST mode supported shift */
>> +#define  PCI_TPH_CAP_INT_VEC	0x2	/* interrupt vector mode supported */
>> +#define  PCI_TPH_CAP_INT_VEC_SHIFT	0x1	/* interrupt vector mode supported shift */
>> +#define  PCI_TPH_CAP_DS		0x4	/* device specific mode supported */
>> +#define  PCI_TPH_CAP_DS_SHIFT	0x4	/* device specific mode supported shift */
>>  #define  PCI_TPH_CAP_LOC_MASK	0x600	/* location mask */
>> -#define   PCI_TPH_LOC_NONE	0x000	/* no location */
>> -#define   PCI_TPH_LOC_CAP	0x200	/* in capability */
>> -#define   PCI_TPH_LOC_MSIX	0x400	/* in MSI-X */
> 
> It's a userspace header, relatively unlikely to be safe to change it...
> This would also be inconsistent with how some other registers are defined in here.
> 
> I'd love it if we could tidy this up, but we are stuck by this being
> in uapi.

Alex Williamson had a similar comment in another email. In V3, I will
only add (necessary) new definitions and refrain from touching the
existing ones.

Re: [PATCH V2 2/9] PCI: Add TPH related register definition

[Bjorn Helgaas]

On Fri, May 31, 2024 at 04:38:34PM -0500, Wei Huang wrote:
> Linux has some basic, but incomplete, definition for the TPH Requester
> capability registers. Also the control registers of TPH Requester and
> the TPH Completer are missing. This patch adds all required definitions
> to support TPH enablement.

s/This patch adds/Add/

> +#define PCI_EXP_DEVCAP2_TPH_COMP_SHIFT		12
> +#define PCI_EXP_DEVCAP2_TPH_COMP_NONE		0x0 /* None */
> +#define PCI_EXP_DEVCAP2_TPH_COMP_TPH_ONLY	0x1 /* TPH only */
> +#define PCI_EXP_DEVCAP2_TPH_COMP_TPH_AND_EXT	0x3 /* TPH and Extended TPH */

Drop the _SHIFT definitions and use FIELD_GET() and FIELD_PREP()
instead.

>  /* TPH Requester */
>  #define PCI_TPH_CAP		4	/* capability register */
> +#define  PCI_TPH_CAP_NO_ST	0x1	/* no ST mode supported */
> +#define  PCI_TPH_CAP_NO_ST_SHIFT	0x0	/* no ST mode supported shift */

Drop _SHIFT and show full register width for PCI_TPH_CAP_NO_ST, e.g.,

  #define  PCI_TPH_CAP_NO_ST 0x00000001

The existing PCI_TPH_CAP_* definitions don't follow that convention,
but the rest of the file does, and this should match.

Re: [PATCH V2 2/9] PCI: Add TPH related register definition

[Wei Huang]

On 6/7/24 11:42, Bjorn Helgaas wrote:
> On Fri, May 31, 2024 at 04:38:34PM -0500, Wei Huang wrote:
>> Linux has some basic, but incomplete, definition for the TPH Requester
>> capability registers. Also the control registers of TPH Requester and
>> the TPH Completer are missing. This patch adds all required definitions
>> to support TPH enablement.
> 
> s/This patch adds/Add/
> 
>> +#define PCI_EXP_DEVCAP2_TPH_COMP_SHIFT		12
>> +#define PCI_EXP_DEVCAP2_TPH_COMP_NONE		0x0 /* None */
>> +#define PCI_EXP_DEVCAP2_TPH_COMP_TPH_ONLY	0x1 /* TPH only */
>> +#define PCI_EXP_DEVCAP2_TPH_COMP_TPH_AND_EXT	0x3 /* TPH and Extended TPH */
> 
> Drop the _SHIFT definitions and use FIELD_GET() and FIELD_PREP()
> instead.
> 
>>  /* TPH Requester */
>>  #define PCI_TPH_CAP		4	/* capability register */
>> +#define  PCI_TPH_CAP_NO_ST	0x1	/* no ST mode supported */
>> +#define  PCI_TPH_CAP_NO_ST_SHIFT	0x0	/* no ST mode supported shift */
> 
> Drop _SHIFT and show full register width for PCI_TPH_CAP_NO_ST, e.g.,
> 
>   #define  PCI_TPH_CAP_NO_ST 0x00000001
> 
> The existing PCI_TPH_CAP_* definitions don't follow that convention,
> but the rest of the file does, and this should match.

Will fix all of the above in V3.

[PATCH V2 3/9] PCI/TPH: Implement a command line option to disable TPH

[Wei Huang]

Provide a kernel option, with related helper functions, to completely
disable TPH so that no TPH headers are generated.

Co-developed-by: Eric Van Tassell <Eric.VanTassell@amd.com>
Signed-off-by: Eric Van Tassell <Eric.VanTassell@amd.com>
Signed-off-by: Wei Huang <wei.huang2@amd.com>
Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com> 
Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
---
 .../admin-guide/kernel-parameters.txt         |  1 +
 drivers/pci/pci-driver.c                      |  7 ++++-
 drivers/pci/pci.c                             | 12 ++++++++
 drivers/pci/pcie/tph.c                        | 30 +++++++++++++++++++
 include/linux/pci-tph.h                       | 19 ++++++++++++
 include/linux/pci.h                           |  1 +
 6 files changed, 69 insertions(+), 1 deletion(-)
 create mode 100644 include/linux/pci-tph.h

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 500cfa776225..fedcc69e35c1 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -4623,6 +4623,7 @@
 		nomio		[S390] Do not use MIO instructions.
 		norid		[S390] ignore the RID field and force use of
 				one PCI domain per PCI function
+		notph		[PCIE] Do not use PCIe TPH
 
 	pcie_aspm=	[PCIE] Forcibly enable or ignore PCIe Active State Power
 			Management.
diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index af2996d0d17f..9722d070c0ca 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -21,6 +21,7 @@
 #include <linux/acpi.h>
 #include <linux/dma-map-ops.h>
 #include <linux/iommu.h>
+#include <linux/pci-tph.h>
 #include "pci.h"
 #include "pcie/portdrv.h"
 
@@ -322,8 +323,12 @@ static long local_pci_probe(void *_ddi)
 	pm_runtime_get_sync(dev);
 	pci_dev->driver = pci_drv;
 	rc = pci_drv->probe(pci_dev, ddi->id);
-	if (!rc)
+	if (!rc) {
+		if (pci_tph_disabled())
+			pcie_tph_disable(pci_dev);
+
 		return rc;
+	}
 	if (rc < 0) {
 		pci_dev->driver = NULL;
 		pm_runtime_put_sync(dev);
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 59e0949fb079..31c443504ce9 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -157,6 +157,9 @@ static bool pcie_ari_disabled;
 /* If set, the PCIe ATS capability will not be used. */
 static bool pcie_ats_disabled;
 
+/* If set, the PCIe TPH capability will not be used. */
+static bool pcie_tph_disabled;
+
 /* If set, the PCI config space of each device is printed during boot. */
 bool pci_early_dump;
 
@@ -166,6 +169,12 @@ bool pci_ats_disabled(void)
 }
 EXPORT_SYMBOL_GPL(pci_ats_disabled);
 
+bool pci_tph_disabled(void)
+{
+	return pcie_tph_disabled;
+}
+EXPORT_SYMBOL_GPL(pci_tph_disabled);
+
 /* Disable bridge_d3 for all PCIe ports */
 static bool pci_bridge_d3_disable;
 /* Force bridge_d3 for all PCIe ports */
@@ -6806,6 +6815,9 @@ static int __init pci_setup(char *str)
 				pci_no_domains();
 			} else if (!strncmp(str, "noari", 5)) {
 				pcie_ari_disabled = true;
+			} else if (!strcmp(str, "notph")) {
+				pr_info("PCIe: TPH is disabled\n");
+				pcie_tph_disabled = true;
 			} else if (!strncmp(str, "cbiosize=", 9)) {
 				pci_cardbus_io_size = memparse(str + 9, &str);
 			} else if (!strncmp(str, "cbmemsize=", 10)) {
diff --git a/drivers/pci/pcie/tph.c b/drivers/pci/pcie/tph.c
index 5f0cc06b74bb..5dc533b89a33 100644
--- a/drivers/pci/pcie/tph.c
+++ b/drivers/pci/pcie/tph.c
@@ -16,11 +16,41 @@
 #include <linux/errno.h>
 #include <linux/msi.h>
 #include <linux/pci.h>
+#include <linux/pci-tph.h>
 #include <linux/msi.h>
 #include <linux/pci-acpi.h>
 
 #include "../pci.h"
 
+static int tph_set_reg_field_u32(struct pci_dev *dev, u8 offset, u32 mask,
+				 u8 shift, u32 field)
+{
+	u32 reg_val;
+	int ret;
+
+	if (!dev->tph_cap)
+		return -EINVAL;
+
+	ret = pci_read_config_dword(dev, dev->tph_cap + offset, &reg_val);
+	if (ret)
+		return ret;
+
+	reg_val &= ~mask;
+	reg_val |= (field << shift) & mask;
+
+	ret = pci_write_config_dword(dev, dev->tph_cap + offset, reg_val);
+
+	return ret;
+}
+
+int pcie_tph_disable(struct pci_dev *dev)
+{
+	return  tph_set_reg_field_u32(dev, PCI_TPH_CTRL,
+				      PCI_TPH_CTRL_REQ_EN_MASK,
+				      PCI_TPH_CTRL_REQ_EN_SHIFT,
+				      PCI_TPH_REQ_DISABLE);
+}
+
 void pcie_tph_init(struct pci_dev *dev)
 {
 	dev->tph_cap = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_TPH);
diff --git a/include/linux/pci-tph.h b/include/linux/pci-tph.h
new file mode 100644
index 000000000000..e187d7e89e8c
--- /dev/null
+++ b/include/linux/pci-tph.h
@@ -0,0 +1,19 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * TPH (TLP Processing Hints)
+ *
+ * Copyright (C) 2024 Advanced Micro Devices, Inc.
+ *     Eric Van Tassell <Eric.VanTassell@amd.com>
+ *     Wei Huang <wei.huang2@amd.com>
+ */
+#ifndef LINUX_PCI_TPH_H
+#define LINUX_PCI_TPH_H
+
+#ifdef CONFIG_PCIE_TPH
+int pcie_tph_disable(struct pci_dev *dev);
+#else
+static inline int pcie_tph_disable(struct pci_dev *dev)
+{ return -EOPNOTSUPP; }
+#endif
+
+#endif /* LINUX_PCI_TPH_H */
diff --git a/include/linux/pci.h b/include/linux/pci.h
index d75a88ec5136..d88ebe87815a 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1841,6 +1841,7 @@ static inline bool pci_aer_available(void) { return false; }
 #endif
 
 bool pci_ats_disabled(void);
+bool pci_tph_disabled(void);
 
 #ifdef CONFIG_PCIE_PTM
 int pci_enable_ptm(struct pci_dev *dev, u8 *granularity);
-- 
2.44.0

Re: [PATCH V2 3/9] PCI/TPH: Implement a command line option to disable TPH

[Jonathan Cameron]

On Fri, 31 May 2024 16:38:35 -0500
Wei Huang <wei.huang2@amd.com> wrote:

> Provide a kernel option, with related helper functions, to completely
> disable TPH so that no TPH headers are generated.

Why would someone use this option?

> 
> Co-developed-by: Eric Van Tassell <Eric.VanTassell@amd.com>
> Signed-off-by: Eric Van Tassell <Eric.VanTassell@amd.com>
> Signed-off-by: Wei Huang <wei.huang2@amd.com>
> Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com> 
> Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
> ---
>  .../admin-guide/kernel-parameters.txt         |  1 +
>  drivers/pci/pci-driver.c                      |  7 ++++-
>  drivers/pci/pci.c                             | 12 ++++++++
>  drivers/pci/pcie/tph.c                        | 30 +++++++++++++++++++
>  include/linux/pci-tph.h                       | 19 ++++++++++++
>  include/linux/pci.h                           |  1 +
>  6 files changed, 69 insertions(+), 1 deletion(-)
>  create mode 100644 include/linux/pci-tph.h
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 500cfa776225..fedcc69e35c1 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -4623,6 +4623,7 @@
>  		nomio		[S390] Do not use MIO instructions.
>  		norid		[S390] ignore the RID field and force use of
>  				one PCI domain per PCI function
> +		notph		[PCIE] Do not use PCIe TPH
>  
>  	pcie_aspm=	[PCIE] Forcibly enable or ignore PCIe Active State Power
>  			Management.
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index af2996d0d17f..9722d070c0ca 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -21,6 +21,7 @@
>  #include <linux/acpi.h>
>  #include <linux/dma-map-ops.h>
>  #include <linux/iommu.h>
> +#include <linux/pci-tph.h>
>  #include "pci.h"
>  #include "pcie/portdrv.h"
>  
> @@ -322,8 +323,12 @@ static long local_pci_probe(void *_ddi)
>  	pm_runtime_get_sync(dev);
>  	pci_dev->driver = pci_drv;
>  	rc = pci_drv->probe(pci_dev, ddi->id);
> -	if (!rc)
> +	if (!rc) {
> +		if (pci_tph_disabled())
> +			pcie_tph_disable(pci_dev);
> +
>  		return rc;
> +	}
>  	if (rc < 0) {
>  		pci_dev->driver = NULL;
>  		pm_runtime_put_sync(dev);
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 59e0949fb079..31c443504ce9 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -157,6 +157,9 @@ static bool pcie_ari_disabled;
>  /* If set, the PCIe ATS capability will not be used. */
>  static bool pcie_ats_disabled;
>  
> +/* If set, the PCIe TPH capability will not be used. */
> +static bool pcie_tph_disabled;
> +
>  /* If set, the PCI config space of each device is printed during boot. */
>  bool pci_early_dump;
>  
> @@ -166,6 +169,12 @@ bool pci_ats_disabled(void)
>  }
>  EXPORT_SYMBOL_GPL(pci_ats_disabled);
>  
> +bool pci_tph_disabled(void)
> +{
> +	return pcie_tph_disabled;
> +}
> +EXPORT_SYMBOL_GPL(pci_tph_disabled);
> +
>  /* Disable bridge_d3 for all PCIe ports */
>  static bool pci_bridge_d3_disable;
>  /* Force bridge_d3 for all PCIe ports */
> @@ -6806,6 +6815,9 @@ static int __init pci_setup(char *str)
>  				pci_no_domains();
>  			} else if (!strncmp(str, "noari", 5)) {
>  				pcie_ari_disabled = true;
> +			} else if (!strcmp(str, "notph")) {
> +				pr_info("PCIe: TPH is disabled\n");
> +				pcie_tph_disabled = true;
>  			} else if (!strncmp(str, "cbiosize=", 9)) {
>  				pci_cardbus_io_size = memparse(str + 9, &str);
>  			} else if (!strncmp(str, "cbmemsize=", 10)) {
> diff --git a/drivers/pci/pcie/tph.c b/drivers/pci/pcie/tph.c
> index 5f0cc06b74bb..5dc533b89a33 100644
> --- a/drivers/pci/pcie/tph.c
> +++ b/drivers/pci/pcie/tph.c
> @@ -16,11 +16,41 @@
>  #include <linux/errno.h>
>  #include <linux/msi.h>
>  #include <linux/pci.h>
> +#include <linux/pci-tph.h>
>  #include <linux/msi.h>
>  #include <linux/pci-acpi.h>
>  
>  #include "../pci.h"
>  
> +static int tph_set_reg_field_u32(struct pci_dev *dev, u8 offset, u32 mask,
> +				 u8 shift, u32 field)

I'm unconvinced this helper makes sense.  Do we do similar for other
PCI capabilities?

If it does make sense to have a field update, then provide
one alongside pci_read_config_dword() etc.

> +{
> +	u32 reg_val;
> +	int ret;
> +
> +	if (!dev->tph_cap)
> +		return -EINVAL;
> +
> +	ret = pci_read_config_dword(dev, dev->tph_cap + offset, &reg_val);
> +	if (ret)
> +		return ret;
> +
> +	reg_val &= ~mask;
> +	reg_val |= (field << shift) & mask;
> +
> +	ret = pci_write_config_dword(dev, dev->tph_cap + offset, reg_val);
> +
> +	return ret;

	return pci_write_config_dword();

> +}
> +
> +int pcie_tph_disable(struct pci_dev *dev)
> +{
> +	return  tph_set_reg_field_u32(dev, PCI_TPH_CTRL,

extra space after return

> +				      PCI_TPH_CTRL_REQ_EN_MASK,
> +				      PCI_TPH_CTRL_REQ_EN_SHIFT,
> +				      PCI_TPH_REQ_DISABLE);
> +}
> +
>  void pcie_tph_init(struct pci_dev *dev)
>  {
>  	dev->tph_cap = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_TPH);

Re: [PATCH V2 3/9] PCI/TPH: Implement a command line option to disable TPH

[Bjorn Helgaas]

On Fri, May 31, 2024 at 04:38:35PM -0500, Wei Huang wrote:
> Provide a kernel option, with related helper functions, to completely
> disable TPH so that no TPH headers are generated.

If we need this option, say what the option is, why we need it, and
include an example of how to use it in the commit log.

Include the option in the subject line, too, e.g., "Add pci=notph
option to disable TPH".  Or maybe "prevent use of TPH"?  "Disable TPH"
hints that BIOS might have enabled it, and this option would turn that
off.  I haven't looked hard enough to know exactly what you intend or
whether there's a difference.

[PATCH V2 4/9] PCI/TPH: Implement a command line option to force No ST Mode

[Wei Huang]

When "No ST mode" is enabled, end-point devices can generate TPH headers
but with all steering tags treated as zero. A steering tag of zero is
interpreted as "using the default policy" by the root complex. This is
essential to quantify the benefit of steering tags for some given
workloads.

Co-developed-by: Eric Van Tassell <Eric.VanTassell@amd.com>
Signed-off-by: Eric Van Tassell <Eric.VanTassell@amd.com>
Signed-off-by: Wei Huang <wei.huang2@amd.com>
Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com> 
Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
---
 .../admin-guide/kernel-parameters.txt         |  1 +
 drivers/pci/pci-driver.c                      |  7 ++++++-
 drivers/pci/pci.c                             | 12 +++++++++++
 drivers/pci/pcie/tph.c                        | 21 +++++++++++++++++++
 include/linux/pci-tph.h                       |  3 +++
 include/linux/pci.h                           |  1 +
 6 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index fedcc69e35c1..e97a4a239563 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -4624,6 +4624,7 @@
 		norid		[S390] ignore the RID field and force use of
 				one PCI domain per PCI function
 		notph		[PCIE] Do not use PCIe TPH
+		nostmode	[PCIE] Force TPH to use No ST Mode
 
 	pcie_aspm=	[PCIE] Forcibly enable or ignore PCIe Active State Power
 			Management.
diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 9722d070c0ca..aa98843d9884 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -324,8 +324,13 @@ static long local_pci_probe(void *_ddi)
 	pci_dev->driver = pci_drv;
 	rc = pci_drv->probe(pci_dev, ddi->id);
 	if (!rc) {
-		if (pci_tph_disabled())
+		if (pci_tph_disabled()) {
 			pcie_tph_disable(pci_dev);
+			return rc;
+		}
+
+		if (pci_tph_nostmode())
+			tph_set_dev_nostmode(pci_dev);
 
 		return rc;
 	}
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 31c443504ce9..f3558a551bf2 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -160,6 +160,9 @@ static bool pcie_ats_disabled;
 /* If set, the PCIe TPH capability will not be used. */
 static bool pcie_tph_disabled;
 
+/* If TPH is enabled, "No ST Mode" will be enforced. */
+static bool pcie_tph_nostmode;
+
 /* If set, the PCI config space of each device is printed during boot. */
 bool pci_early_dump;
 
@@ -175,6 +178,12 @@ bool pci_tph_disabled(void)
 }
 EXPORT_SYMBOL_GPL(pci_tph_disabled);
 
+bool pci_tph_nostmode(void)
+{
+	return pcie_tph_nostmode;
+}
+EXPORT_SYMBOL_GPL(pci_tph_nostmode);
+
 /* Disable bridge_d3 for all PCIe ports */
 static bool pci_bridge_d3_disable;
 /* Force bridge_d3 for all PCIe ports */
@@ -6818,6 +6827,9 @@ static int __init pci_setup(char *str)
 			} else if (!strcmp(str, "notph")) {
 				pr_info("PCIe: TPH is disabled\n");
 				pcie_tph_disabled = true;
+			} else if (!strcmp(str, "nostmode")) {
+				pr_info("PCIe: TPH No ST Mode is enabled\n");
+				pcie_tph_nostmode = true;
 			} else if (!strncmp(str, "cbiosize=", 9)) {
 				pci_cardbus_io_size = memparse(str + 9, &str);
 			} else if (!strncmp(str, "cbmemsize=", 10)) {
diff --git a/drivers/pci/pcie/tph.c b/drivers/pci/pcie/tph.c
index 5dc533b89a33..d5f7309fdf52 100644
--- a/drivers/pci/pcie/tph.c
+++ b/drivers/pci/pcie/tph.c
@@ -43,6 +43,27 @@ static int tph_set_reg_field_u32(struct pci_dev *dev, u8 offset, u32 mask,
 	return ret;
 }
 
+int tph_set_dev_nostmode(struct pci_dev *dev)
+{
+	int ret;
+
+	/* set ST Mode Select to "No ST Mode" */
+	ret = tph_set_reg_field_u32(dev, PCI_TPH_CTRL,
+				    PCI_TPH_CTRL_MODE_SEL_MASK,
+				    PCI_TPH_CTRL_MODE_SEL_SHIFT,
+				    PCI_TPH_NO_ST_MODE);
+	if (ret)
+		return ret;
+
+	/* set "TPH Requester Enable" to "TPH only" */
+	ret = tph_set_reg_field_u32(dev, PCI_TPH_CTRL,
+				    PCI_TPH_CTRL_REQ_EN_MASK,
+				    PCI_TPH_CTRL_REQ_EN_SHIFT,
+				    PCI_TPH_REQ_TPH_ONLY);
+
+	return ret;
+}
+
 int pcie_tph_disable(struct pci_dev *dev)
 {
 	return  tph_set_reg_field_u32(dev, PCI_TPH_CTRL,
diff --git a/include/linux/pci-tph.h b/include/linux/pci-tph.h
index e187d7e89e8c..95269afc8b7d 100644
--- a/include/linux/pci-tph.h
+++ b/include/linux/pci-tph.h
@@ -11,9 +11,12 @@
 
 #ifdef CONFIG_PCIE_TPH
 int pcie_tph_disable(struct pci_dev *dev);
+int tph_set_dev_nostmode(struct pci_dev *dev);
 #else
 static inline int pcie_tph_disable(struct pci_dev *dev)
 { return -EOPNOTSUPP; }
+static inline int tph_set_dev_nostmode(struct pci_dev *dev)
+{ return -EOPNOTSUPP; }
 #endif
 
 #endif /* LINUX_PCI_TPH_H */
diff --git a/include/linux/pci.h b/include/linux/pci.h
index d88ebe87815a..5f520624d133 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1842,6 +1842,7 @@ static inline bool pci_aer_available(void) { return false; }
 
 bool pci_ats_disabled(void);
 bool pci_tph_disabled(void);
+bool pci_tph_nostmode(void);
 
 #ifdef CONFIG_PCIE_PTM
 int pci_enable_ptm(struct pci_dev *dev, u8 *granularity);
-- 
2.44.0

Re: [PATCH V2 4/9] PCI/TPH: Implement a command line option to force No ST Mode

[Jonathan Cameron]

On Fri, 31 May 2024 16:38:36 -0500
Wei Huang <wei.huang2@amd.com> wrote:

> When "No ST mode" is enabled, end-point devices can generate TPH headers
> but with all steering tags treated as zero. A steering tag of zero is
> interpreted as "using the default policy" by the root complex. This is
> essential to quantify the benefit of steering tags for some given
> workloads.

This is a good explanation. Need similar in the previous patch to
justify the disable TPH entirely.

> diff --git a/drivers/pci/pcie/tph.c b/drivers/pci/pcie/tph.c
> index 5dc533b89a33..d5f7309fdf52 100644
> --- a/drivers/pci/pcie/tph.c
> +++ b/drivers/pci/pcie/tph.c
> @@ -43,6 +43,27 @@ static int tph_set_reg_field_u32(struct pci_dev *dev, u8 offset, u32 mask,
>  	return ret;
>  }
>  
> +int tph_set_dev_nostmode(struct pci_dev *dev)
> +{
> +	int ret;
> +
> +	/* set ST Mode Select to "No ST Mode" */
> +	ret = tph_set_reg_field_u32(dev, PCI_TPH_CTRL,
> +				    PCI_TPH_CTRL_MODE_SEL_MASK,
> +				    PCI_TPH_CTRL_MODE_SEL_SHIFT,
> +				    PCI_TPH_NO_ST_MODE);
> +	if (ret)
> +		return ret;
> +
> +	/* set "TPH Requester Enable" to "TPH only" */
> +	ret = tph_set_reg_field_u32(dev, PCI_TPH_CTRL,
> +				    PCI_TPH_CTRL_REQ_EN_MASK,
> +				    PCI_TPH_CTRL_REQ_EN_SHIFT,
> +				    PCI_TPH_REQ_TPH_ONLY);

Unless these have to be two RMW operations. (if they do add a spec reference)
then this is a good example of why a field update function may not be
the right option.  We probably want to RMW once.

return tph_set_reg_field_u32()

> +
> +	return ret;
> +}
> +
>  int pcie_tph_disable(struct pci_dev *dev)
>  {
>  	return  tph_set_reg_field_u32(dev, PCI_TPH_CTRL,

Re: [PATCH V2 4/9] PCI/TPH: Implement a command line option to force No ST Mode

[Bjorn Helgaas]

On Fri, May 31, 2024 at 04:38:36PM -0500, Wei Huang wrote:
> When "No ST mode" is enabled, end-point devices can generate TPH headers
> but with all steering tags treated as zero. A steering tag of zero is
> interpreted as "using the default policy" by the root complex. This is
> essential to quantify the benefit of steering tags for some given
> workloads.

>  #ifdef CONFIG_PCIE_TPH
>  int pcie_tph_disable(struct pci_dev *dev);
> +int tph_set_dev_nostmode(struct pci_dev *dev);

Exported functions need a "pci" or "pcie_" prefix.  We haven't been
completely consistent on this; we have "pci_acs_*", which is obviously
PCIe-specific, but we do also have pcie_find_root_port() etc.

[PATCH V2 5/9] PCI/TPH: Introduce API functions to manage steering tags

[Wei Huang]

This patch introduces three API functions, pcie_tph_intr_vec_supported(),
pcie_tph_get_st() and pcie_tph_set_st(), for a driver to query, retrieve
or configure device's steering tags. There are two possible locations for
steering tag table and the code automatically figure out the right
location to set the tags if pcie_tph_set_st() is called. Note the tag
value is always zero currently and will be extended in the follow-up
patches.

Co-developed-by: Eric Van Tassell <Eric.VanTassell@amd.com>
Signed-off-by: Eric Van Tassell <Eric.VanTassell@amd.com>
Signed-off-by: Wei Huang <wei.huang2@amd.com>
Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com> 
Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
---
 drivers/pci/pcie/tph.c  | 402 ++++++++++++++++++++++++++++++++++++++++
 include/linux/pci-tph.h |  22 +++
 2 files changed, 424 insertions(+)

diff --git a/drivers/pci/pcie/tph.c b/drivers/pci/pcie/tph.c
index d5f7309fdf52..320b99c60365 100644
--- a/drivers/pci/pcie/tph.c
+++ b/drivers/pci/pcie/tph.c
@@ -43,6 +43,336 @@ static int tph_set_reg_field_u32(struct pci_dev *dev, u8 offset, u32 mask,
 	return ret;
 }
 
+static int tph_get_reg_field_u32(struct pci_dev *dev, u8 offset, u32 mask,
+				 u8 shift, u32 *field)
+{
+	u32 reg_val;
+	int ret;
+
+	if (!dev->tph_cap)
+		return -EINVAL;
+
+	ret = pci_read_config_dword(dev, dev->tph_cap + offset, &reg_val);
+	if (ret)
+		return ret;
+
+	*field = (reg_val & mask) >> shift;
+
+	return 0;
+}
+
+static int tph_get_table_size(struct pci_dev *dev, u16 *size_out)
+{
+	int ret;
+	u32 tmp;
+
+	ret = tph_get_reg_field_u32(dev, PCI_TPH_CAP,
+				    PCI_TPH_CAP_ST_MASK,
+				    PCI_TPH_CAP_ST_SHIFT, &tmp);
+
+	if (ret)
+		return ret;
+
+	*size_out = (u16)tmp;
+
+	return 0;
+}
+
+/*
+ * For a given device, return a pointer to the MSI table entry at msi_index.
+ */
+static void __iomem *tph_msix_table_entry(struct pci_dev *dev,
+					  u16 msi_index)
+{
+	void __iomem *entry;
+	u16 tbl_sz;
+	int ret;
+
+	ret = tph_get_table_size(dev, &tbl_sz);
+	if (ret || msi_index > tbl_sz)
+		return NULL;
+
+	entry = dev->msix_base + msi_index * PCI_MSIX_ENTRY_SIZE;
+
+	return entry;
+}
+
+/*
+ * For a given device, return a pointer to the vector control register at
+ * offset 0xc of MSI table entry at msi_index.
+ */
+static void __iomem *tph_msix_vector_control(struct pci_dev *dev,
+					     u16 msi_index)
+{
+	void __iomem *vec_ctrl_addr = tph_msix_table_entry(dev, msi_index);
+
+	if (vec_ctrl_addr)
+		vec_ctrl_addr += PCI_MSIX_ENTRY_VECTOR_CTRL;
+
+	return vec_ctrl_addr;
+}
+
+/*
+ * Translate from MSI-X interrupt index to struct msi_desc *
+ */
+static struct msi_desc *tph_msix_index_to_desc(struct pci_dev *dev, int index)
+{
+	struct msi_desc *entry;
+
+	msi_lock_descs(&dev->dev);
+	msi_for_each_desc(entry, &dev->dev, MSI_DESC_ASSOCIATED) {
+		if (entry->msi_index == index)
+			return entry;
+	}
+	msi_unlock_descs(&dev->dev);
+
+	return NULL;
+}
+
+static bool tph_int_vec_mode_supported(struct pci_dev *dev)
+{
+	u32 mode = 0;
+	int ret;
+
+	ret = tph_get_reg_field_u32(dev, PCI_TPH_CAP,
+				    PCI_TPH_CAP_INT_VEC,
+				    PCI_TPH_CAP_INT_VEC_SHIFT, &mode);
+	if (ret)
+		return false;
+
+	return !!mode;
+}
+
+static int tph_get_table_location(struct pci_dev *dev, u8 *loc_out)
+{
+	u32 loc;
+	int ret;
+
+	ret = tph_get_reg_field_u32(dev, PCI_TPH_CAP, PCI_TPH_CAP_LOC_MASK,
+				    PCI_TPH_CAP_LOC_SHIFT, &loc);
+	if (ret)
+		return ret;
+
+	*loc_out = (u8)loc;
+
+	return 0;
+}
+
+static bool msix_nr_in_bounds(struct pci_dev *dev, int msix_nr)
+{
+	u16 tbl_sz;
+
+	if (tph_get_table_size(dev, &tbl_sz))
+		return false;
+
+	return msix_nr <= tbl_sz;
+}
+
+/* Return root port capability - 0 means none */
+static int get_root_port_completer_cap(struct pci_dev *dev)
+{
+	struct pci_dev *rp;
+	int ret;
+	int val;
+
+	rp = pcie_find_root_port(dev);
+	if (!rp) {
+		pr_err("cannot find root port of %s\n", dev_name(&dev->dev));
+		return 0;
+	}
+
+	ret = pcie_capability_read_dword(rp, PCI_EXP_DEVCAP2, &val);
+	if (ret) {
+		pr_err("cannot read device capabilities 2 of %s\n",
+		       dev_name(&dev->dev));
+		return 0;
+	}
+
+	val &= PCI_EXP_DEVCAP2_TPH_COMP;
+
+	return val >> PCI_EXP_DEVCAP2_TPH_COMP_SHIFT;
+}
+
+/*
+ * TPH device needs to be below a rootport with the TPH Completer and
+ * the completer must offer a compatible level of completer support to that
+ * requested by the device driver.
+ */
+static bool completer_support_ok(struct pci_dev *dev, u8 req)
+{
+	int rp_cap;
+
+	rp_cap = get_root_port_completer_cap(dev);
+
+	if (req > rp_cap) {
+		pr_err("root port lacks proper TPH completer capability\n");
+		return false;
+	}
+
+	return true;
+}
+
+/*
+ * The PCI Specification version 5.0 requires the "No ST Mode" mode
+ * be supported by any compatible device.
+ */
+static bool no_st_mode_supported(struct pci_dev *dev)
+{
+	bool no_st;
+	int ret;
+	u32 tmp;
+
+	ret = tph_get_reg_field_u32(dev, PCI_TPH_CAP, PCI_TPH_CAP_NO_ST,
+				    PCI_TPH_CAP_NO_ST_SHIFT, &tmp);
+	if (ret)
+		return false;
+
+	no_st = !!tmp;
+
+	if (!no_st) {
+		pr_err("TPH devices must support no ST mode\n");
+		return false;
+	}
+
+	return true;
+}
+
+static int tph_write_ctrl_reg(struct pci_dev *dev, u32 value)
+{
+	int ret;
+
+	ret = tph_set_reg_field_u32(dev, PCI_TPH_CTRL, ~0L, 0, value);
+
+	if (ret)
+		goto err_out;
+
+	return 0;
+
+err_out:
+	/* minimizing possible harm by disabling TPH */
+	pcie_tph_disable(dev);
+	return ret;
+}
+
+/* Update the ST Mode Select field of the TPH Control Register */
+static int tph_set_ctrl_reg_mode_sel(struct pci_dev *dev, u8 st_mode)
+{
+	int ret;
+	u32 ctrl_reg;
+
+	ret = tph_get_reg_field_u32(dev, PCI_TPH_CTRL, ~0L, 0, &ctrl_reg);
+	if (ret)
+		return ret;
+
+	/* clear the mode select and enable fields */
+	ctrl_reg &= ~(PCI_TPH_CTRL_MODE_SEL_MASK);
+	ctrl_reg |= ((u32)(st_mode << PCI_TPH_CTRL_MODE_SEL_SHIFT) &
+		     PCI_TPH_CTRL_MODE_SEL_MASK);
+
+	ret = tph_write_ctrl_reg(dev, ctrl_reg);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+/* Write the steering tag to MSI-X vector control register */
+static void tph_write_tag_to_msix(struct pci_dev *dev, int msix_nr, u16 tag)
+{
+	u32 val;
+	void __iomem *vec_ctrl;
+	struct msi_desc *msi_desc;
+
+	msi_desc = tph_msix_index_to_desc(dev, msix_nr);
+	if (!msi_desc) {
+		pr_err("MSI-X descriptor for #%d not found\n", msix_nr);
+		return;
+	}
+
+	vec_ctrl = tph_msix_vector_control(dev, msi_desc->msi_index);
+
+	val = readl(vec_ctrl);
+	val &= 0xffff;
+	val |= (tag << 16);
+	writel(val, vec_ctrl);
+
+	/* read back to flush the update */
+	val = readl(vec_ctrl);
+	msi_unlock_descs(&dev->dev);
+}
+
+/* Update the TPH Requester Enable field of the TPH Control Register */
+static int tph_set_ctrl_reg_en(struct pci_dev *dev, u8 req_type)
+{
+	int ret;
+	u32 ctrl_reg;
+
+	ret = tph_get_reg_field_u32(dev, PCI_TPH_CTRL, ~0L, 0,
+				    &ctrl_reg);
+	if (ret)
+		return ret;
+
+	/* clear the mode select and enable fields and set new values*/
+	ctrl_reg &= ~(PCI_TPH_CTRL_REQ_EN_MASK);
+	ctrl_reg |= (((u32)req_type << PCI_TPH_CTRL_REQ_EN_SHIFT) &
+			PCI_TPH_CTRL_REQ_EN_MASK);
+
+	ret = tph_write_ctrl_reg(dev, ctrl_reg);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static bool pcie_tph_write_st(struct pci_dev *dev, unsigned int msix_nr,
+			      u8 req_type, u16 tag)
+{
+	int offset;
+	u8  loc;
+	int ret;
+
+	/* setting ST isn't needed - not an error, just return true */
+	if (!dev->tph_cap || pci_tph_disabled() || pci_tph_nostmode() ||
+	    !dev->msix_enabled || !tph_int_vec_mode_supported(dev))
+		return true;
+
+	/* setting ST is incorrect in the following cases - return error */
+	if (!no_st_mode_supported(dev) || !msix_nr_in_bounds(dev, msix_nr) ||
+	    !completer_support_ok(dev, req_type))
+		return false;
+
+	/*
+	 * disable TPH before updating the tag to avoid potential instability
+	 * as cautioned about in the "ST Table Programming" of PCI-E spec
+	 */
+	pcie_tph_disable(dev);
+
+	ret = tph_get_table_location(dev, &loc);
+	if (ret)
+		return false;
+
+	switch (loc) {
+	case PCI_TPH_LOC_MSIX:
+		tph_write_tag_to_msix(dev, msix_nr, tag);
+		break;
+	case PCI_TPH_LOC_CAP:
+		offset = dev->tph_cap + PCI_TPH_ST_TABLE
+			  + msix_nr * sizeof(u16);
+		pci_write_config_word(dev, offset, tag);
+		break;
+	default:
+		pr_err("unable to write steering tag for device %s\n",
+		       dev_name(&dev->dev));
+		return false;
+	}
+
+	/* select interrupt vector mode */
+	tph_set_ctrl_reg_mode_sel(dev, PCI_TPH_INT_VEC_MODE);
+	tph_set_ctrl_reg_en(dev, req_type);
+
+	return true;
+}
+
 int tph_set_dev_nostmode(struct pci_dev *dev)
 {
 	int ret;
@@ -77,3 +407,75 @@ void pcie_tph_init(struct pci_dev *dev)
 	dev->tph_cap = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_TPH);
 }
 
+/**
+ * pcie_tph_intr_vec_supported() - Check if interrupt vector mode supported for dev
+ * @dev: pci device
+ *
+ * Return:
+ *        true : intr vector mode supported
+ *        false: intr vector mode not supported
+ */
+bool pcie_tph_intr_vec_supported(struct pci_dev *dev)
+{
+	if (!dev->tph_cap || pci_tph_disabled() || !dev->msix_enabled ||
+	    !tph_int_vec_mode_supported(dev))
+		return false;
+
+	return true;
+}
+EXPORT_SYMBOL(pcie_tph_intr_vec_supported);
+
+/**
+ * pcie_tph_get_st() - Retrieve steering tag for a specific CPU
+ * @dev: pci device
+ * @cpu: the acpi cpu_uid.
+ * @mem_type: memory type (vram, nvram)
+ * @req_type: request type (disable, tph, extended tph)
+ * @tag: steering tag return value
+ *
+ * Return:
+ *        true : success
+ *        false: failed
+ */
+bool pcie_tph_get_st(struct pci_dev *dev, unsigned int cpu,
+		    enum tph_mem_type mem_type, u8 req_type,
+		    u16 *tag)
+{
+	*tag = 0;
+
+	return true;
+}
+EXPORT_SYMBOL(pcie_tph_get_st);
+
+/**
+ * pcie_tph_set_st() - Set steering tag in ST table entry
+ * @dev: pci device
+ * @msix_nr: ordinal number of msix interrupt.
+ * @cpu: the acpi cpu_uid.
+ * @mem_type: memory type (vram, nvram)
+ * @req_type: request type (disable, tph, extended tph)
+ *
+ * Return:
+ *        true : success
+ *        false: failed
+ */
+bool pcie_tph_set_st(struct pci_dev *dev, unsigned int msix_nr,
+		     unsigned int cpu, enum tph_mem_type mem_type,
+		     u8 req_type)
+{
+	u16 tag;
+	bool ret = true;
+
+	ret = pcie_tph_get_st(dev, cpu, mem_type, req_type, &tag);
+
+	if (!ret)
+		return false;
+
+	pr_debug("%s: writing tag %d for msi-x intr %d (cpu: %d)\n",
+		 __func__, tag, msix_nr, cpu);
+
+	ret = pcie_tph_write_st(dev, msix_nr, req_type, tag);
+
+	return ret;
+}
+EXPORT_SYMBOL(pcie_tph_set_st);
diff --git a/include/linux/pci-tph.h b/include/linux/pci-tph.h
index 95269afc8b7d..4fbd1e2fd98c 100644
--- a/include/linux/pci-tph.h
+++ b/include/linux/pci-tph.h
@@ -9,14 +9,36 @@
 #ifndef LINUX_PCI_TPH_H
 #define LINUX_PCI_TPH_H
 
+enum tph_mem_type {
+	TPH_MEM_TYPE_VM,	/* volatile memory type */
+	TPH_MEM_TYPE_PM		/* persistent memory type */
+};
+
 #ifdef CONFIG_PCIE_TPH
 int pcie_tph_disable(struct pci_dev *dev);
 int tph_set_dev_nostmode(struct pci_dev *dev);
+bool pcie_tph_intr_vec_supported(struct pci_dev *dev);
+bool pcie_tph_get_st(struct pci_dev *dev, unsigned int cpu,
+		     enum tph_mem_type tag_type, u8 req_enable,
+		     u16 *tag);
+bool pcie_tph_set_st(struct pci_dev *dev, unsigned int msix_nr,
+		     unsigned int cpu, enum tph_mem_type tag_type,
+		     u8 req_enable);
 #else
 static inline int pcie_tph_disable(struct pci_dev *dev)
 { return -EOPNOTSUPP; }
 static inline int tph_set_dev_nostmode(struct pci_dev *dev)
 { return -EOPNOTSUPP; }
+static inline bool pcie_tph_intr_vec_supported(struct pci_dev *dev)
+{ return false; }
+static inline bool pcie_tph_get_st(struct pci_dev *dev, unsigned int cpu,
+				   enum tph_mem_type tag_type, u8 req_enable,
+				   u16 *tag)
+{ return false; }
+static inline bool pcie_tph_set_st(struct pci_dev *dev, unsigned int msix_nr,
+				   unsigned int cpu, enum tph_mem_type tag_type,
+				   u8 req_enable)
+{ return true; }
 #endif
 
 #endif /* LINUX_PCI_TPH_H */
-- 
2.44.0

Re: [PATCH V2 5/9] PCI/TPH: Introduce API functions to manage steering tags

[kernel test robot]

Hi Wei,

kernel test robot noticed the following build errors:

[auto build test ERROR on pci/next]
[also build test ERROR on pci/for-linus awilliam-vfio/next linus/master awilliam-vfio/for-linus v6.10-rc2 next-20240606]
[cannot apply to horms-ipvs/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Wei-Huang/PCI-Introduce-PCIe-TPH-support-framework/20240601-054423
base:   https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next
patch link:    https://lore.kernel.org/r/20240531213841.3246055-6-wei.huang2%40amd.com
patch subject: [PATCH V2 5/9] PCI/TPH: Introduce API functions to manage steering tags
config: arm-randconfig-r122-20240607 (https://download.01.org/0day-ci/archive/20240607/202406070602.DyLemS9q-lkp@intel.com/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project d7d2d4f53fc79b4b58e8d8d08151b577c3699d4a)
reproduce: (https://download.01.org/0day-ci/archive/20240607/202406070602.DyLemS9q-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202406070602.DyLemS9q-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from drivers/pci/pcie/tph.c:18:
   In file included from include/linux/pci.h:1653:
   In file included from include/linux/dmapool.h:14:
   In file included from include/linux/scatterlist.h:8:
   In file included from include/linux/mm.h:2253:
   include/linux/vmstat.h:514:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
     514 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
         |                               ~~~~~~~~~~~ ^ ~~~
>> drivers/pci/pcie/tph.c:95:15: error: no member named 'msix_base' in 'struct pci_dev'; did you mean 'msix_cap'?
      95 |         entry = dev->msix_base + msi_index * PCI_MSIX_ENTRY_SIZE;
         |                      ^~~~~~~~~
         |                      msix_cap
   include/linux/pci.h:350:6: note: 'msix_cap' declared here
     350 |         u8              msix_cap;       /* MSI-X capability offset */
         |                         ^
   1 warning and 1 error generated.


vim +95 drivers/pci/pcie/tph.c

    80	
    81	/*
    82	 * For a given device, return a pointer to the MSI table entry at msi_index.
    83	 */
    84	static void __iomem *tph_msix_table_entry(struct pci_dev *dev,
    85						  u16 msi_index)
    86	{
    87		void __iomem *entry;
    88		u16 tbl_sz;
    89		int ret;
    90	
    91		ret = tph_get_table_size(dev, &tbl_sz);
    92		if (ret || msi_index > tbl_sz)
    93			return NULL;
    94	
  > 95		entry = dev->msix_base + msi_index * PCI_MSIX_ENTRY_SIZE;
    96	
    97		return entry;
    98	}
    99	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH V2 5/9] PCI/TPH: Introduce API functions to manage steering tags

[Jonathan Cameron]

On Fri, 31 May 2024 16:38:37 -0500
Wei Huang <wei.huang2@amd.com> wrote:

> This patch introduces three API functions, pcie_tph_intr_vec_supported(),
> pcie_tph_get_st() and pcie_tph_set_st(), for a driver to query, retrieve
> or configure device's steering tags. There are two possible locations for
> steering tag table and the code automatically figure out the right
> location to set the tags if pcie_tph_set_st() is called. Note the tag
> value is always zero currently and will be extended in the follow-up
> patches.
> 
> Co-developed-by: Eric Van Tassell <Eric.VanTassell@amd.com>
> Signed-off-by: Eric Van Tassell <Eric.VanTassell@amd.com>
> Signed-off-by: Wei Huang <wei.huang2@amd.com>
> Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com> 
> Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
Hi. 

There are a lot of small functions in here.  I'd look at just calling
the things they are wrapping more directly as with the register
and field names inline it's pretty obvious what is going on.
Wrapping that in a helper function doesn't necessarily help readablity
and perhaps doubles the length of the code.

Various other comments inline.

Jonathan


> ---
>  drivers/pci/pcie/tph.c  | 402 ++++++++++++++++++++++++++++++++++++++++
>  include/linux/pci-tph.h |  22 +++
>  2 files changed, 424 insertions(+)
> 
> diff --git a/drivers/pci/pcie/tph.c b/drivers/pci/pcie/tph.c
> index d5f7309fdf52..320b99c60365 100644
> --- a/drivers/pci/pcie/tph.c
> +++ b/drivers/pci/pcie/tph.c
> @@ -43,6 +43,336 @@ static int tph_set_reg_field_u32(struct pci_dev *dev, u8 offset, u32 mask,
>  	return ret;
>  }
>  
> +static int tph_get_reg_field_u32(struct pci_dev *dev, u8 offset, u32 mask,
> +				 u8 shift, u32 *field)
> +{
> +	u32 reg_val;
> +	int ret;
> +
> +	if (!dev->tph_cap)
> +		return -EINVAL;
> +
> +	ret = pci_read_config_dword(dev, dev->tph_cap + offset, &reg_val);
> +	if (ret)
> +		return ret;
> +
> +	*field = (reg_val & mask) >> shift;
> +
> +	return 0;
> +}

Similar to earlier, I'm not seeing as strong reason for this.
Just do the tph_cap check at the external interface points rather than
in register read paths.

Mind you, if you do keep this a lot of the other helpers
could be flattened as they are simply passing particular parameters
to this and that could be done inline where the results are needed.


> +
> +static int tph_get_table_size(struct pci_dev *dev, u16 *size_out)
> +{
> +	int ret;
> +	u32 tmp;
> +
> +	ret = tph_get_reg_field_u32(dev, PCI_TPH_CAP,
> +				    PCI_TPH_CAP_ST_MASK,
> +				    PCI_TPH_CAP_ST_SHIFT, &tmp);
> +
> +	if (ret)
> +		return ret;
> +
> +	*size_out = (u16)tmp;
> +
> +	return 0;
> +}
> +
> +/*
> + * For a given device, return a pointer to the MSI table entry at msi_index.
> + */
> +static void __iomem *tph_msix_table_entry(struct pci_dev *dev,
> +					  u16 msi_index)
> +{
> +	void __iomem *entry;
> +	u16 tbl_sz;
> +	int ret;
> +
> +	ret = tph_get_table_size(dev, &tbl_sz);
> +	if (ret || msi_index > tbl_sz)
> +		return NULL;
Nice to return the error code via ERR_PTR() etc so ultimate caller gets
some information.
> +
> +	entry = dev->msix_base + msi_index * PCI_MSIX_ENTRY_SIZE;

return dev->msix_base + msi_index * PCI_MSIX_ENTRY_SIZE;

> +
> +	return entry;
> +}
> +
> +/*
> + * For a given device, return a pointer to the vector control register at
> + * offset 0xc of MSI table entry at msi_index.
> + */
> +static void __iomem *tph_msix_vector_control(struct pci_dev *dev,
> +					     u16 msi_index)
> +{
> +	void __iomem *vec_ctrl_addr = tph_msix_table_entry(dev, msi_index);
> +
> +	if (vec_ctrl_addr)
> +		vec_ctrl_addr += PCI_MSIX_ENTRY_VECTOR_CTRL;

I'd do this addition at the caller.  Then don't need this function.

> +
> +	return vec_ctrl_addr;
> +}
> +
> +/*
> + * Translate from MSI-X interrupt index to struct msi_desc *
> + */
> +static struct msi_desc *tph_msix_index_to_desc(struct pci_dev *dev, int index)
> +{
> +	struct msi_desc *entry;
> +
> +	msi_lock_descs(&dev->dev);

I'd take the lock at the caller as not obvious this is going to keep holding it
from the name.

If you call it tph_msix_get_desc_from_index() that would help.

> +	msi_for_each_desc(entry, &dev->dev, MSI_DESC_ASSOCIATED) {
> +		if (entry->msi_index == index)
> +			return entry;
> +	}
> +	msi_unlock_descs(&dev->dev);
> +
> +	return NULL;
> +}

> +
> +static bool msix_nr_in_bounds(struct pci_dev *dev, int msix_nr)
> +{
> +	u16 tbl_sz;
> +
> +	if (tph_get_table_size(dev, &tbl_sz))
> +		return false;
> +
> +	return msix_nr <= tbl_sz;

Use the table entry request and just check reutrn for NULL.

So instead of calling this function,

if (!tph_msix_table_entry(dev, msix_nr));

and drop this function.

> +}
> +
> +/* Return root port capability - 0 means none */
> +static int get_root_port_completer_cap(struct pci_dev *dev)

I'd expect anything ending in _cap in PCI code to be giving
me the offset of a capability. 

static int root_port_tph_support() maybe?

> +{
> +	struct pci_dev *rp;
> +	int ret;
> +	int val;
> +
> +	rp = pcie_find_root_port(dev);
> +	if (!rp) {
> +		pr_err("cannot find root port of %s\n", dev_name(&dev->dev));
> +		return 0;
> +	}
> +
> +	ret = pcie_capability_read_dword(rp, PCI_EXP_DEVCAP2, &val);
> +	if (ret) {
> +		pr_err("cannot read device capabilities 2 of %s\n",
> +		       dev_name(&dev->dev));
> +		return 0;
> +	}
> +
> +	val &= PCI_EXP_DEVCAP2_TPH_COMP;
> +
> +	return val >> PCI_EXP_DEVCAP2_TPH_COMP_SHIFT;
> +}
> +
> +/*
> + * TPH device needs to be below a rootport with the TPH Completer and
> + * the completer must offer a compatible level of completer support to that
> + * requested by the device driver.
> + */
> +static bool completer_support_ok(struct pci_dev *dev, u8 req)
> +{
> +	int rp_cap;
> +
> +	rp_cap = get_root_port_completer_cap(dev);
> +
> +	if (req > rp_cap) {
> +		pr_err("root port lacks proper TPH completer capability\n");

Assumption is that any driver getting to here should really have checked
if this was fine before enabling TPH?  a pr_err() seems overly noisy
otherwise.

> +		return false;
> +	}
> +
> +	return true;
> +}
> +
> +/*
> + * The PCI Specification version 5.0 requires the "No ST Mode" mode
> + * be supported by any compatible device.

Why do we need to check something that 'must' be set to 1?
Are there known buggy devices?  If not, just don't check it
as we can assume this is true and save 20 lines of code.

> + */
> +static bool no_st_mode_supported(struct pci_dev *dev)
> +{
> +	bool no_st;
> +	int ret;
> +	u32 tmp;
> +
> +	ret = tph_get_reg_field_u32(dev, PCI_TPH_CAP, PCI_TPH_CAP_NO_ST,
> +				    PCI_TPH_CAP_NO_ST_SHIFT, &tmp);
> +	if (ret)
> +		return false;
> +
> +	no_st = !!tmp;
> +
> +	if (!no_st) {
> +		pr_err("TPH devices must support no ST mode\n");
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
> +static int tph_write_ctrl_reg(struct pci_dev *dev, u32 value)
> +{
> +	int ret;
> +
> +	ret = tph_set_reg_field_u32(dev, PCI_TPH_CTRL, ~0L, 0, value);
> +
> +	if (ret)
> +		goto err_out;
> +
> +	return 0;
> +
> +err_out:
> +	/* minimizing possible harm by disabling TPH */
> +	pcie_tph_disable(dev);

If the write failed, something is horribly wrong. Do we need
to defend against that case?  It complicates the code a fair bit so
it needs a strong reasoning.  Preferably why can this fail other
than in bug cases or surprise removal or similar.

> +	return ret;
> +}
> +
> +/* Update the ST Mode Select field of the TPH Control Register */
> +static int tph_set_ctrl_reg_mode_sel(struct pci_dev *dev, u8 st_mode)
> +{
> +	int ret;
> +	u32 ctrl_reg;
> +
> +	ret = tph_get_reg_field_u32(dev, PCI_TPH_CTRL, ~0L, 0, &ctrl_reg);
> +	if (ret)
> +		return ret;
> +
> +	/* clear the mode select and enable fields */
> +	ctrl_reg &= ~(PCI_TPH_CTRL_MODE_SEL_MASK);
> +	ctrl_reg |= ((u32)(st_mode << PCI_TPH_CTRL_MODE_SEL_SHIFT) &
> +		     PCI_TPH_CTRL_MODE_SEL_MASK);
> +
> +	ret = tph_write_ctrl_reg(dev, ctrl_reg);

return tph_write_ctrl_reg()

> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +/* Write the steering tag to MSI-X vector control register */
> +static void tph_write_tag_to_msix(struct pci_dev *dev, int msix_nr, u16 tag)
> +{
> +	u32 val;
> +	void __iomem *vec_ctrl;
> +	struct msi_desc *msi_desc;
> +
> +	msi_desc = tph_msix_index_to_desc(dev, msix_nr);
> +	if (!msi_desc) {
> +		pr_err("MSI-X descriptor for #%d not found\n", msix_nr);
> +		return;
return an error so we can handle it at caller.

> +	}
> +
> +	vec_ctrl = tph_msix_vector_control(dev, msi_desc->msi_index);

Whilst this would have already failed, I'd still check for vec_ctrl == NULL
as easier to read.

> +
> +	val = readl(vec_ctrl);
> +	val &= 0xffff;
> +	val |= (tag << 16);
> +	writel(val, vec_ctrl);
> +
> +	/* read back to flush the update */
> +	val = readl(vec_ctrl);
> +	msi_unlock_descs(&dev->dev);
Who took the lock? Not obvious from function naming - see above

> +}
> +

> +static bool pcie_tph_write_st(struct pci_dev *dev, unsigned int msix_nr,
> +			      u8 req_type, u16 tag)

Why bool for error or not?  Better to return an error code.

> +{
> +	int offset;
> +	u8  loc;

Unusual to align the local variables like this. I'd just have a single space before
loc..

> +	int ret;
> +
> +	/* setting ST isn't needed - not an error, just return true */
> +	if (!dev->tph_cap || pci_tph_disabled() || pci_tph_nostmode() ||
> +	    !dev->msix_enabled || !tph_int_vec_mode_supported(dev))
> +		return true;
> +
> +	/* setting ST is incorrect in the following cases - return error */
> +	if (!no_st_mode_supported(dev) || !msix_nr_in_bounds(dev, msix_nr) ||
> +	    !completer_support_ok(dev, req_type))
> +		return false;
> +
> +	/*
> +	 * disable TPH before updating the tag to avoid potential instability
> +	 * as cautioned about in the "ST Table Programming" of PCI-E spec
> +	 */
> +	pcie_tph_disable(dev);
> +
> +	ret = tph_get_table_location(dev, &loc);
> +	if (ret)
> +		return false;
> +
> +	switch (loc) {
> +	case PCI_TPH_LOC_MSIX:
> +		tph_write_tag_to_msix(dev, msix_nr, tag);
handle errors.
> +		break;
> +	case PCI_TPH_LOC_CAP:
> +		offset = dev->tph_cap + PCI_TPH_ST_TABLE
> +			  + msix_nr * sizeof(u16);
> +		pci_write_config_word(dev, offset, tag);
handle errors.
> +		break;
> +	default:
> +		pr_err("unable to write steering tag for device %s\n",
> +		       dev_name(&dev->dev));
> +		return false;
> +	}
> +
> +	/* select interrupt vector mode */
> +	tph_set_ctrl_reg_mode_sel(dev, PCI_TPH_INT_VEC_MODE);
handle errors
> +	tph_set_ctrl_reg_en(dev, req_type);
etc.

> +
> +	return true;
> +}
> +

> +
> +/**
> + * pcie_tph_get_st() - Retrieve steering tag for a specific CPU
> + * @dev: pci device
> + * @cpu: the acpi cpu_uid.
> + * @mem_type: memory type (vram, nvram)
> + * @req_type: request type (disable, tph, extended tph)
> + * @tag: steering tag return value
> + *
> + * Return:
> + *        true : success
> + *        false: failed
> + */
> +bool pcie_tph_get_st(struct pci_dev *dev, unsigned int cpu,

Rename so that it's obvious this isn't just reading back the st
previously set to the device (i.e. it isn't the opposite of
pci_tph_set_st())

pci_tph_get_st_to_use() or something like that.

int so we can have some error information once implemented.
I'm not keen on introducing a stub like this though that doesn't
yet do anything. Bring it in when useful.




> +		    enum tph_mem_type mem_type, u8 req_type,
> +		    u16 *tag)
> +{
> +	*tag = 0;
> +
> +	return true;
> +}
> +EXPORT_SYMBOL(pcie_tph_get_st);
> +
> +/**
> + * pcie_tph_set_st() - Set steering tag in ST table entry
> + * @dev: pci device
> + * @msix_nr: ordinal number of msix interrupt.
> + * @cpu: the acpi cpu_uid.

Given most linux CPU numbers are not necessarily the ACPI CPU ID
I'd call it that.  e.g. cpu_acpi_uid.


> + * @mem_type: memory type (vram, nvram)
> + * @req_type: request type (disable, tph, extended tph)
> + *
> + * Return:
> + *        true : success
> + *        false: failed
> + */
> +bool pcie_tph_set_st(struct pci_dev *dev, unsigned int msix_nr,
> +		     unsigned int cpu, enum tph_mem_type mem_type,
> +		     u8 req_type)
> +{
> +	u16 tag;
> +	bool ret = true;
> +
> +	ret = pcie_tph_get_st(dev, cpu, mem_type, req_type, &tag);
> +
> +	if (!ret)
> +		return false;
> +
> +	pr_debug("%s: writing tag %d for msi-x intr %d (cpu: %d)\n",
> +		 __func__, tag, msix_nr, cpu);
> +
> +	ret = pcie_tph_write_st(dev, msix_nr, req_type, tag);
> +
> +	return ret;
return pcie_tph_write_st() but make it return an integer so caller
gets some information on what when wrong.

> +}
> +EXPORT_SYMBOL(pcie_tph_set_st);

Re: [PATCH V2 5/9] PCI/TPH: Introduce API functions to manage steering tags

[Bjorn Helgaas]

On Fri, May 31, 2024 at 04:38:37PM -0500, Wei Huang wrote:
> This patch introduces three API functions, pcie_tph_intr_vec_supported(),
> pcie_tph_get_st() and pcie_tph_set_st(), for a driver to query, retrieve
> or configure device's steering tags. There are two possible locations for
> steering tag table and the code automatically figure out the right
> location to set the tags if pcie_tph_set_st() is called. Note the tag
> value is always zero currently and will be extended in the follow-up
> patches.

> +static int tph_get_reg_field_u32(struct pci_dev *dev, u8 offset, u32 mask,
> +				 u8 shift, u32 *field)
> +{
> +	u32 reg_val;
> +	int ret;
> +
> +	if (!dev->tph_cap)
> +		return -EINVAL;
> +
> +	ret = pci_read_config_dword(dev, dev->tph_cap + offset, &reg_val);
> +	if (ret)
> +		return ret;
> +
> +	*field = (reg_val & mask) >> shift;
> +
> +	return 0;
> +}
> +
> +static int tph_get_table_size(struct pci_dev *dev, u16 *size_out)
> +{
> +	int ret;
> +	u32 tmp;
> +
> +	ret = tph_get_reg_field_u32(dev, PCI_TPH_CAP,
> +				    PCI_TPH_CAP_ST_MASK,
> +				    PCI_TPH_CAP_ST_SHIFT, &tmp);

Just use FIELD_GET() instead.

> +	if (ret)
> +		return ret;
> +
> +	*size_out = (u16)tmp;
> +
> +	return 0;
> +}
> +
> +/*
> + * For a given device, return a pointer to the MSI table entry at msi_index.

s/MSI/MSI-X/ to avoid any possible confusion.

> +static void __iomem *tph_msix_table_entry(struct pci_dev *dev,
> +					  u16 msi_index)

> +	ret = pcie_capability_read_dword(rp, PCI_EXP_DEVCAP2, &val);
> +	if (ret) {
> +		pr_err("cannot read device capabilities 2 of %s\n",
> +		       dev_name(&dev->dev));

Never use pr_err() when you can use pci_err() instead.  Obviously no
dev_name() needed with pci_err().  Other instances below.

> +	val &= PCI_EXP_DEVCAP2_TPH_COMP;
> +
> +	return val >> PCI_EXP_DEVCAP2_TPH_COMP_SHIFT;

FIELD_GET()

> + * The PCI Specification version 5.0 requires the "No ST Mode" mode
> + * be supported by any compatible device.

Cite r6.0 or newer and include section number.

> +	/* clear the mode select and enable fields and set new values*/

Space before closing */

> +	ctrl_reg &= ~(PCI_TPH_CTRL_REQ_EN_MASK);
> +	ctrl_reg |= (((u32)req_type << PCI_TPH_CTRL_REQ_EN_SHIFT) &
> +			PCI_TPH_CTRL_REQ_EN_MASK);

FIELD_GET()/FIELD_PREP()

> +static bool pcie_tph_write_st(struct pci_dev *dev, unsigned int msix_nr,
> +			      u8 req_type, u16 tag)

This function is not a predicate and testing for true/false gives no
indication of the sense.

For typical functions that do read/write/etc, returning 0 means
success and -errno means failure.  This is the opposite.

> +	/*
> +	 * disable TPH before updating the tag to avoid potential instability
> +	 * as cautioned about in the "ST Table Programming" of PCI-E spec

s/disable/Disable/

"PCIe r6.0, sec ..."

> +bool pcie_tph_set_st(struct pci_dev *dev, unsigned int msix_nr,
> +		     unsigned int cpu, enum tph_mem_type mem_type,
> +		     u8 req_type)

Should return 0 or -errno.

[PATCH V2 6/9] PCI/TPH: Retrieve steering tag from ACPI _DSM

[Wei Huang]

According to PCI SIG ECN, calling the _DSM firmware method for a given
CPU_UID returns the steering tags for different types of memory
(volatile, non-volatile). These tags are supposed to be used in ST
table entry for optimal results.

Co-developed-by: Eric Van Tassell <Eric.VanTassell@amd.com>
Signed-off-by: Eric Van Tassell <Eric.VanTassell@amd.com>
Signed-off-by: Wei Huang <wei.huang2@amd.com>
Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com> 
Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
---
 drivers/pci/pcie/tph.c  | 103 +++++++++++++++++++++++++++++++++++++++-
 include/linux/pci-tph.h |  34 +++++++++++++
 2 files changed, 136 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pcie/tph.c b/drivers/pci/pcie/tph.c
index 320b99c60365..425935a14b62 100644
--- a/drivers/pci/pcie/tph.c
+++ b/drivers/pci/pcie/tph.c
@@ -158,6 +158,98 @@ static int tph_get_table_location(struct pci_dev *dev, u8 *loc_out)
 	return 0;
 }
 
+static u16 tph_extract_tag(enum tph_mem_type mem_type, u8 req_type,
+			   union st_info *st_tag)
+{
+	switch (req_type) {
+	case PCI_TPH_REQ_TPH_ONLY: /* 8 bit tags */
+		switch (mem_type) {
+		case TPH_MEM_TYPE_VM:
+			if (st_tag->vm_st_valid)
+				return st_tag->vm_st;
+			break;
+		case TPH_MEM_TYPE_PM:
+			if (st_tag->pm_st_valid)
+				return st_tag->pm_st;
+			break;
+		}
+		break;
+	case PCI_TPH_REQ_EXT_TPH: /* 16 bit tags */
+		switch (mem_type) {
+		case TPH_MEM_TYPE_VM:
+			if (st_tag->vm_xst_valid)
+				return st_tag->vm_xst;
+			break;
+		case TPH_MEM_TYPE_PM:
+			if (st_tag->pm_xst_valid)
+				return st_tag->pm_xst;
+			break;
+		}
+		break;
+	default:
+		pr_err("invalid steering tag in ACPI _DSM\n");
+		return 0;
+	}
+
+	return 0;
+}
+
+#define MIN_ST_DSM_REV		7
+#define ST_DSM_FUNC_INDEX	0xf
+static bool invoke_dsm(acpi_handle handle, u32 cpu_uid, u8 ph,
+		       u8 target_type, bool cache_ref_valid,
+		       u64 cache_ref, union st_info *st_out)
+{
+	union acpi_object in_obj, in_buf[3], *out_obj;
+
+	in_buf[0].integer.type = ACPI_TYPE_INTEGER;
+	in_buf[0].integer.value = 0; /* 0 => processor cache steering tags */
+
+	in_buf[1].integer.type = ACPI_TYPE_INTEGER;
+	in_buf[1].integer.value = cpu_uid;
+
+	in_buf[2].integer.type = ACPI_TYPE_INTEGER;
+	in_buf[2].integer.value = ph & 3;
+	in_buf[2].integer.value |= (target_type & 1) << 2;
+	in_buf[2].integer.value |= (cache_ref_valid & 1) << 3;
+	in_buf[2].integer.value |= (cache_ref << 32);
+
+	in_obj.type = ACPI_TYPE_PACKAGE;
+	in_obj.package.count = ARRAY_SIZE(in_buf);
+	in_obj.package.elements = in_buf;
+
+	out_obj = acpi_evaluate_dsm(handle, &pci_acpi_dsm_guid, MIN_ST_DSM_REV,
+				    ST_DSM_FUNC_INDEX, &in_obj);
+
+	if (!out_obj)
+		return false;
+
+	if (out_obj->type != ACPI_TYPE_BUFFER) {
+		pr_err("invalid return type %d from TPH _DSM\n",
+		       out_obj->type);
+		ACPI_FREE(out_obj);
+		return false;
+	}
+
+	st_out->value = *((u64 *)(out_obj->buffer.pointer));
+
+	ACPI_FREE(out_obj);
+
+	return true;
+}
+
+static acpi_handle root_complex_acpi_handle(struct pci_dev *dev)
+{
+	struct pci_dev *root_port;
+
+	root_port = pcie_find_root_port(dev);
+
+	if (!root_port || !root_port->bus || !root_port->bus->bridge)
+		return NULL;
+
+	return ACPI_HANDLE(root_port->bus->bridge);
+}
+
 static bool msix_nr_in_bounds(struct pci_dev *dev, int msix_nr)
 {
 	u16 tbl_sz;
@@ -441,7 +533,16 @@ bool pcie_tph_get_st(struct pci_dev *dev, unsigned int cpu,
 		    enum tph_mem_type mem_type, u8 req_type,
 		    u16 *tag)
 {
-	*tag = 0;
+	union st_info info;
+
+	if (!invoke_dsm(root_complex_acpi_handle(dev), cpu, 0, 0, false, 0,
+			&info)) {
+		*tag = 0;
+		return false;
+	}
+
+	*tag = tph_extract_tag(mem_type, req_type, &info);
+	pr_debug("%s: cpu=%d tag=%d\n", __func__, cpu, *tag);
 
 	return true;
 }
diff --git a/include/linux/pci-tph.h b/include/linux/pci-tph.h
index 4fbd1e2fd98c..79533c6254c2 100644
--- a/include/linux/pci-tph.h
+++ b/include/linux/pci-tph.h
@@ -14,6 +14,40 @@ enum tph_mem_type {
 	TPH_MEM_TYPE_PM		/* persistent memory type */
 };
 
+/*
+ * The st_info struct defines the steering tag returned by the firmware _DSM
+ * method defined in PCI SIG ECN. The specification is available at:
+ * https://members.pcisig.com/wg/PCI-SIG/document/15470.
+
+ * @vm_st_valid:  8 bit tag for volatile memory is valid
+ * @vm_xst_valid: 16 bit tag for volatile memory is valid
+ * @vm_ignore:    1 => was and will be ignored, 0 => ph should be supplied
+ * @vm_st:        8 bit steering tag for volatile mem
+ * @vm_xst:       16 bit steering tag for volatile mem
+ * @pm_st_valid:  8 bit tag for persistent memory is valid
+ * @pm_xst_valid: 16 bit tag for persistent memory is valid
+ * @pm_ignore:    1 => was and will be ignore, 0 => ph should be supplied
+ * @pm_st:        8 bit steering tag for persistent mem
+ * @pm_xst:       16 bit steering tag for persistent mem
+ */
+union st_info {
+	struct {
+		u64 vm_st_valid:1,
+		vm_xst_valid:1,
+		vm_ph_ignore:1,
+		rsvd1:5,
+		vm_st:8,
+		vm_xst:16,
+		pm_st_valid:1,
+		pm_xst_valid:1,
+		pm_ph_ignore:1,
+		rsvd2:5,
+		pm_st:8,
+		pm_xst:16;
+	};
+	u64 value;
+};
+
 #ifdef CONFIG_PCIE_TPH
 int pcie_tph_disable(struct pci_dev *dev);
 int tph_set_dev_nostmode(struct pci_dev *dev);
-- 
2.44.0

Re: [PATCH V2 6/9] PCI/TPH: Retrieve steering tag from ACPI _DSM

[Simon Horman]

On Fri, May 31, 2024 at 04:38:38PM -0500, Wei Huang wrote:
> According to PCI SIG ECN, calling the _DSM firmware method for a given
> CPU_UID returns the steering tags for different types of memory
> (volatile, non-volatile). These tags are supposed to be used in ST
> table entry for optimal results.
> 
> Co-developed-by: Eric Van Tassell <Eric.VanTassell@amd.com>
> Signed-off-by: Eric Van Tassell <Eric.VanTassell@amd.com>
> Signed-off-by: Wei Huang <wei.huang2@amd.com>
> Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com> 
> Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>

...

> diff --git a/drivers/pci/pcie/tph.c b/drivers/pci/pcie/tph.c

...

> +static bool invoke_dsm(acpi_handle handle, u32 cpu_uid, u8 ph,
> +		       u8 target_type, bool cache_ref_valid,
> +		       u64 cache_ref, union st_info *st_out)
> +{
> +	union acpi_object in_obj, in_buf[3], *out_obj;
> +
> +	in_buf[0].integer.type = ACPI_TYPE_INTEGER;
> +	in_buf[0].integer.value = 0; /* 0 => processor cache steering tags */
> +
> +	in_buf[1].integer.type = ACPI_TYPE_INTEGER;
> +	in_buf[1].integer.value = cpu_uid;
> +
> +	in_buf[2].integer.type = ACPI_TYPE_INTEGER;
> +	in_buf[2].integer.value = ph & 3;
> +	in_buf[2].integer.value |= (target_type & 1) << 2;
> +	in_buf[2].integer.value |= (cache_ref_valid & 1) << 3;
> +	in_buf[2].integer.value |= (cache_ref << 32);
> +
> +	in_obj.type = ACPI_TYPE_PACKAGE;
> +	in_obj.package.count = ARRAY_SIZE(in_buf);
> +	in_obj.package.elements = in_buf;
> +
> +	out_obj = acpi_evaluate_dsm(handle, &pci_acpi_dsm_guid, MIN_ST_DSM_REV,
> +				    ST_DSM_FUNC_INDEX, &in_obj);

Hi Wei Huang, Eric, all,

This seems to break builds on ARM (32bit) with multi_v7_defconfig.

  .../tph.c:221:39: error: use of undeclared identifier 'pci_acpi_dsm_guid'
  221 |         out_obj = acpi_evaluate_dsm(handle, &pci_acpi_dsm_guid, MIN_ST_DSM_REV,
      |

I suspect a dependency on ACPI in Kconfig is appropriate.

> +
> +	if (!out_obj)
> +		return false;
> +
> +	if (out_obj->type != ACPI_TYPE_BUFFER) {
> +		pr_err("invalid return type %d from TPH _DSM\n",
> +		       out_obj->type);
> +		ACPI_FREE(out_obj);
> +		return false;
> +	}

...

Re: [PATCH V2 6/9] PCI/TPH: Retrieve steering tag from ACPI _DSM

[Wei Huang]

On 6/4/24 10:30, Simon Horman wrote:
> On Fri, May 31, 2024 at 04:38:38PM -0500, Wei Huang wrote:
>> According to PCI SIG ECN, calling the _DSM firmware method for a given
>> CPU_UID returns the steering tags for different types of memory
>> (volatile, non-volatile). These tags are supposed to be used in ST
>> table entry for optimal results.
>>
>> Co-developed-by: Eric Van Tassell <Eric.VanTassell@amd.com>
>> Signed-off-by: Eric Van Tassell <Eric.VanTassell@amd.com>
>> Signed-off-by: Wei Huang <wei.huang2@amd.com>
>> Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
>> Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com> 
>> Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
> 
> ...
> 
>> diff --git a/drivers/pci/pcie/tph.c b/drivers/pci/pcie/tph.c
> 
> ...
> 
>> +static bool invoke_dsm(acpi_handle handle, u32 cpu_uid, u8 ph,
>> +		       u8 target_type, bool cache_ref_valid,
>> +		       u64 cache_ref, union st_info *st_out)
>> +{
>> +	union acpi_object in_obj, in_buf[3], *out_obj;
>> +
>> +	in_buf[0].integer.type = ACPI_TYPE_INTEGER;
>> +	in_buf[0].integer.value = 0; /* 0 => processor cache steering tags */
>> +
>> +	in_buf[1].integer.type = ACPI_TYPE_INTEGER;
>> +	in_buf[1].integer.value = cpu_uid;
>> +
>> +	in_buf[2].integer.type = ACPI_TYPE_INTEGER;
>> +	in_buf[2].integer.value = ph & 3;
>> +	in_buf[2].integer.value |= (target_type & 1) << 2;
>> +	in_buf[2].integer.value |= (cache_ref_valid & 1) << 3;
>> +	in_buf[2].integer.value |= (cache_ref << 32);
>> +
>> +	in_obj.type = ACPI_TYPE_PACKAGE;
>> +	in_obj.package.count = ARRAY_SIZE(in_buf);
>> +	in_obj.package.elements = in_buf;
>> +
>> +	out_obj = acpi_evaluate_dsm(handle, &pci_acpi_dsm_guid, MIN_ST_DSM_REV,
>> +				    ST_DSM_FUNC_INDEX, &in_obj);
> 
> Hi Wei Huang, Eric, all,
> 
> This seems to break builds on ARM (32bit) with multi_v7_defconfig.
> 
>   .../tph.c:221:39: error: use of undeclared identifier 'pci_acpi_dsm_guid'
>   221 |         out_obj = acpi_evaluate_dsm(handle, &pci_acpi_dsm_guid, MIN_ST_DSM_REV,
>       |

Thanks for pointing it out. I will add "depends on ACPI" in Kconfig
which solves the problem:

$ make ARCH=arm CROSS_COMPILE=arm-linux-gnu- multi_v7_defconfig zImage
modules
  CALL    scripts/checksyscalls.sh
  Kernel: arch/arm/boot/Image is ready
  Kernel: arch/arm/boot/zImage is ready

> 
> I suspect a dependency on ACPI in Kconfig is appropriate.
> 
>> +
>> +	if (!out_obj)
>> +		return false;
>> +
>> +	if (out_obj->type != ACPI_TYPE_BUFFER) {
>> +		pr_err("invalid return type %d from TPH _DSM\n",
>> +		       out_obj->type);
>> +		ACPI_FREE(out_obj);
>> +		return false;
>> +	}
> 
> ...

Re: [PATCH V2 6/9] PCI/TPH: Retrieve steering tag from ACPI _DSM

[Jonathan Cameron]

On Fri, 31 May 2024 16:38:38 -0500
Wei Huang <wei.huang2@amd.com> wrote:

> According to PCI SIG ECN, calling the _DSM firmware method for a given
> CPU_UID returns the steering tags for different types of memory
> (volatile, non-volatile). These tags are supposed to be used in ST
> table entry for optimal results.
> 
> Co-developed-by: Eric Van Tassell <Eric.VanTassell@amd.com>
> Signed-off-by: Eric Van Tassell <Eric.VanTassell@amd.com>
> Signed-off-by: Wei Huang <wei.huang2@amd.com>
> Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com> 
> Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
Took a very quick look at this only due to lack of time..

> ---
>  drivers/pci/pcie/tph.c  | 103 +++++++++++++++++++++++++++++++++++++++-
>  include/linux/pci-tph.h |  34 +++++++++++++
>  2 files changed, 136 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pcie/tph.c b/drivers/pci/pcie/tph.c
> index 320b99c60365..425935a14b62 100644
> --- a/drivers/pci/pcie/tph.c
> +++ b/drivers/pci/pcie/tph.c
> @@ -158,6 +158,98 @@ static int tph_get_table_location(struct pci_dev *dev, u8 *loc_out)
>  	return 0;
>  }
>  
> +static u16 tph_extract_tag(enum tph_mem_type mem_type, u8 req_type,
> +			   union st_info *st_tag)
> +{
> +	switch (req_type) {
> +	case PCI_TPH_REQ_TPH_ONLY: /* 8 bit tags */
> +		switch (mem_type) {
> +		case TPH_MEM_TYPE_VM:
> +			if (st_tag->vm_st_valid)
> +				return st_tag->vm_st;
> +			break;
> +		case TPH_MEM_TYPE_PM:
> +			if (st_tag->pm_st_valid)
> +				return st_tag->pm_st;
> +			break;
> +		}
> +		break;
> +	case PCI_TPH_REQ_EXT_TPH: /* 16 bit tags */
> +		switch (mem_type) {
> +		case TPH_MEM_TYPE_VM:
> +			if (st_tag->vm_xst_valid)
> +				return st_tag->vm_xst;
> +			break;
> +		case TPH_MEM_TYPE_PM:
> +			if (st_tag->pm_xst_valid)
> +				return st_tag->pm_xst;
> +			break;
> +		}
> +		break;
> +	default:
> +		pr_err("invalid steering tag in ACPI _DSM\n");
> +		return 0;
Not an error code?  If so need to explain why 0 is the right thing to
return.
> +	}
> +
> +	return 0;
> +}
> +
> +#define MIN_ST_DSM_REV		7
> +#define ST_DSM_FUNC_INDEX	0xf
> +static bool invoke_dsm(acpi_handle handle, u32 cpu_uid, u8 ph,

give that a pci / tph prefix of some type as it's a very generic
name so potential future name clashes likely.

> +		       u8 target_type, bool cache_ref_valid,
> +		       u64 cache_ref, union st_info *st_out)
> +{
> +	union acpi_object in_obj, in_buf[3], *out_obj;

I'm out of time for the day, so not checked this. Will look more
closely in v3.

> +
> +	in_buf[0].integer.type = ACPI_TYPE_INTEGER;
> +	in_buf[0].integer.value = 0; /* 0 => processor cache steering tags */
> +
> +	in_buf[1].integer.type = ACPI_TYPE_INTEGER;
> +	in_buf[1].integer.value = cpu_uid;
> +
> +	in_buf[2].integer.type = ACPI_TYPE_INTEGER;
> +	in_buf[2].integer.value = ph & 3;
> +	in_buf[2].integer.value |= (target_type & 1) << 2;
> +	in_buf[2].integer.value |= (cache_ref_valid & 1) << 3;
> +	in_buf[2].integer.value |= (cache_ref << 32);
> +
> +	in_obj.type = ACPI_TYPE_PACKAGE;
> +	in_obj.package.count = ARRAY_SIZE(in_buf);
> +	in_obj.package.elements = in_buf;
> +
> +	out_obj = acpi_evaluate_dsm(handle, &pci_acpi_dsm_guid, MIN_ST_DSM_REV,
> +				    ST_DSM_FUNC_INDEX, &in_obj);
> +
> +	if (!out_obj)
> +		return false;
> +
> +	if (out_obj->type != ACPI_TYPE_BUFFER) {
> +		pr_err("invalid return type %d from TPH _DSM\n",
> +		       out_obj->type);
> +		ACPI_FREE(out_obj);
> +		return false;
> +	}
> +
> +	st_out->value = *((u64 *)(out_obj->buffer.pointer));
> +
> +	ACPI_FREE(out_obj);
> +
> +	return true;
> +}
> +

>  static bool msix_nr_in_bounds(struct pci_dev *dev, int msix_nr)
>  {
>  	u16 tbl_sz;
> @@ -441,7 +533,16 @@ bool pcie_tph_get_st(struct pci_dev *dev, unsigned int cpu,
>  		    enum tph_mem_type mem_type, u8 req_type,
>  		    u16 *tag)
Add this function in this patch as it's not used before here and all
the logic is about the _DSM
Note name needs to change though.

>  {
> -	*tag = 0;
> +	union st_info info;
> +
> +	if (!invoke_dsm(root_complex_acpi_handle(dev), cpu, 0, 0, false, 0,
> +			&info)) {
> +		*tag = 0;
> +		return false;
> +	}
> +
> +	*tag = tph_extract_tag(mem_type, req_type, &info);
> +	pr_debug("%s: cpu=%d tag=%d\n", __func__, cpu, *tag);
>  
>  	return true;
>  }
> diff --git a/include/linux/pci-tph.h b/include/linux/pci-tph.h
> index 4fbd1e2fd98c..79533c6254c2 100644
> --- a/include/linux/pci-tph.h
> +++ b/include/linux/pci-tph.h
> @@ -14,6 +14,40 @@ enum tph_mem_type {
>  	TPH_MEM_TYPE_PM		/* persistent memory type */
>  };
>  
> +/*
> + * The st_info struct defines the steering tag returned by the firmware _DSM
> + * method defined in PCI SIG ECN. The specification is available at:
> + * https://members.pcisig.com/wg/PCI-SIG/document/15470.
> +
> + * @vm_st_valid:  8 bit tag for volatile memory is valid
> + * @vm_xst_valid: 16 bit tag for volatile memory is valid
> + * @vm_ignore:    1 => was and will be ignored, 0 => ph should be supplied
> + * @vm_st:        8 bit steering tag for volatile mem
> + * @vm_xst:       16 bit steering tag for volatile mem
> + * @pm_st_valid:  8 bit tag for persistent memory is valid
> + * @pm_xst_valid: 16 bit tag for persistent memory is valid
> + * @pm_ignore:    1 => was and will be ignore, 0 => ph should be supplied
pm_ph_ignore

> + * @pm_st:        8 bit steering tag for persistent mem
> + * @pm_xst:       16 bit steering tag for persistent mem
> + */
> +union st_info {
> +	struct {
> +		u64 vm_st_valid:1,
> +		vm_xst_valid:1,
> +		vm_ph_ignore:1,
> +		rsvd1:5,
> +		vm_st:8,
> +		vm_xst:16,
> +		pm_st_valid:1,
> +		pm_xst_valid:1,
> +		pm_ph_ignore:1,
> +		rsvd2:5,
> +		pm_st:8,
> +		pm_xst:16;
> +	};
> +	u64 value;
> +};
Firstly why in a header? If it did want to be then pci-acpi.h might be reasonable.

> +
>  #ifdef CONFIG_PCIE_TPH
>  int pcie_tph_disable(struct pci_dev *dev);
>  int tph_set_dev_nostmode(struct pci_dev *dev);

Re: [PATCH V2 6/9] PCI/TPH: Retrieve steering tag from ACPI _DSM

[Bjorn Helgaas]

On Fri, May 31, 2024 at 04:38:38PM -0500, Wei Huang wrote:
> According to PCI SIG ECN, calling the _DSM firmware method for a given
> CPU_UID returns the steering tags for different types of memory
> (volatile, non-volatile). These tags are supposed to be used in ST
> table entry for optimal results.

Cite PCI Firmware spec if possible.  If it hasn't been incorporated
yet, at least include the exact name of the ECN and the date it was
approved.

Say what the patch does in the commit log (in addition to the subject
line).

> +#define MIN_ST_DSM_REV		7

No useful value in this #define.  If the value ever changes, code
changes will be required too.

> +#define ST_DSM_FUNC_INDEX	0xf

Move to the list in pci-acpi.h with name similar to others.

> +static bool invoke_dsm(acpi_handle handle, u32 cpu_uid, u8 ph,
> +		       u8 target_type, bool cache_ref_valid,
> +		       u64 cache_ref, union st_info *st_out)
> +{

Return 0 or -errno.  "invoke_dsm" is not a predicate with an obvious
true/false meaning.

> +	union acpi_object in_obj, in_buf[3], *out_obj;
> +
> +	in_buf[0].integer.type = ACPI_TYPE_INTEGER;
> +	in_buf[0].integer.value = 0; /* 0 => processor cache steering tags */
> +
> +	in_buf[1].integer.type = ACPI_TYPE_INTEGER;
> +	in_buf[1].integer.value = cpu_uid;
> +
> +	in_buf[2].integer.type = ACPI_TYPE_INTEGER;
> +	in_buf[2].integer.value = ph & 3;
> +	in_buf[2].integer.value |= (target_type & 1) << 2;
> +	in_buf[2].integer.value |= (cache_ref_valid & 1) << 3;
> +	in_buf[2].integer.value |= (cache_ref << 32);
> +
> +	in_obj.type = ACPI_TYPE_PACKAGE;
> +	in_obj.package.count = ARRAY_SIZE(in_buf);
> +	in_obj.package.elements = in_buf;

Must check whether this _DSM function is implemented first, e.g., see
acpi_enable_dpc().

> +	out_obj = acpi_evaluate_dsm(handle, &pci_acpi_dsm_guid, MIN_ST_DSM_REV,
> +				    ST_DSM_FUNC_INDEX, &in_obj);
> +
> +	if (!out_obj)
> +		return false;
> +
> +	if (out_obj->type != ACPI_TYPE_BUFFER) {
> +		pr_err("invalid return type %d from TPH _DSM\n",
> +		       out_obj->type);
> +		ACPI_FREE(out_obj);
> +		return false;
> +	}
> +
> +	st_out->value = *((u64 *)(out_obj->buffer.pointer));
> +
> +	ACPI_FREE(out_obj);
> +
> +	return true;
> +}

[PATCH V2 7/9] PCI/TPH: Add TPH documentation

[Wei Huang]

Provide a document for TPH feature, including the description of
kernel options and driver API interface.

Co-developed-by: Eric Van Tassell <Eric.VanTassell@amd.com>
Signed-off-by: Eric Van Tassell <Eric.VanTassell@amd.com>
Signed-off-by: Wei Huang <wei.huang2@amd.com>
Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com> 
Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
---
 Documentation/PCI/index.rst          |  1 +
 Documentation/PCI/tph.rst            | 57 ++++++++++++++++++++++++++++
 Documentation/driver-api/pci/pci.rst |  3 ++
 3 files changed, 61 insertions(+)
 create mode 100644 Documentation/PCI/tph.rst

diff --git a/Documentation/PCI/index.rst b/Documentation/PCI/index.rst
index e73f84aebde3..5e7c4e6e726b 100644
--- a/Documentation/PCI/index.rst
+++ b/Documentation/PCI/index.rst
@@ -18,3 +18,4 @@ PCI Bus Subsystem
    pcieaer-howto
    endpoint/index
    boot-interrupts
+   tph
diff --git a/Documentation/PCI/tph.rst b/Documentation/PCI/tph.rst
new file mode 100644
index 000000000000..ea9c8313f3e4
--- /dev/null
+++ b/Documentation/PCI/tph.rst
@@ -0,0 +1,57 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+===========
+TPH Support
+===========
+
+
+:Copyright: 2024 Advanced Micro Devices, Inc.
+:Authors: - Eric van Tassell <eric.vantassell@amd.com>
+          - Wei Huang <wei.huang2@amd.com>
+
+Overview
+========
+TPH (TLP Processing Hints) is a PCIe feature that allows endpoint devices
+to provide optimization hints, such as desired caching behavior, for
+requests that target memory space. These hints, in a format called steering
+tags, are provided in the requester's TLP headers and can empower the system
+hardware, including the Root Complex, to optimize the utilization of platform
+resources for the requests.
+
+User Guide
+==========
+
+Kernel Options
+--------------
+There are two kernel command line options available to control TPH feature
+
+   * "notph": TPH will be disabled for all endpoint devices.
+   * "nostmode": TPH will be enabled but the ST Mode will be forced to "No ST Mode".
+
+Device Driver API
+-----------------
+In brief, an endpoint device driver using the TPH interface to configure
+Interrupt Vector Mode will call pcie_tph_set_st() when setting up MSI-X
+interrupts as shown below:
+
+.. code-block:: c
+
+    for (i = 0, j = 0; i < nr_rings; i++) {
+        ...
+        rc = request_irq(irq->vector, irq->handler, flags, irq->name, NULL);
+        ...
+        if (!pcie_tph_set_st(pdev, i, cpumask_first(irq->cpu_mask),
+                             TPH_MEM_TYPE_VM, PCI_TPH_REQ_TPH_ONLY))
+               pr_err("Error in configuring steering tag\n");
+        ...
+    }
+
+If a device only supports TPH vendor specific mode, its driver can call
+pcie_tph_get_st() to retrieve the steering tag for a specific CPU and uses
+the tag to control TPH behavior.
+
+.. kernel-doc:: drivers/pci/pcie/tph.c
+   :export:
+
+.. kernel-doc:: drivers/pci/pcie/tph.c
+   :identifiers: pcie_tph_set_st
diff --git a/Documentation/driver-api/pci/pci.rst b/Documentation/driver-api/pci/pci.rst
index aa40b1cc243b..3d896b2cf16e 100644
--- a/Documentation/driver-api/pci/pci.rst
+++ b/Documentation/driver-api/pci/pci.rst
@@ -46,6 +46,9 @@ PCI Support Library
 .. kernel-doc:: drivers/pci/pci-sysfs.c
    :internal:
 
+.. kernel-doc:: drivers/pci/pcie/tph.c
+   :export:
+
 PCI Hotplug Support Library
 ---------------------------
 
-- 
2.44.0

Re: [PATCH V2 7/9] PCI/TPH: Add TPH documentation

[Jonathan Cameron]

On Fri, 31 May 2024 16:38:39 -0500
Wei Huang <wei.huang2@amd.com> wrote:

> Provide a document for TPH feature, including the description of
> kernel options and driver API interface.
> 
> Co-developed-by: Eric Van Tassell <Eric.VanTassell@amd.com>
> Signed-off-by: Eric Van Tassell <Eric.VanTassell@amd.com>
> Signed-off-by: Wei Huang <wei.huang2@amd.com>
> Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com> 
> Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
Sort and sweet.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  Documentation/PCI/index.rst          |  1 +
>  Documentation/PCI/tph.rst            | 57 ++++++++++++++++++++++++++++
>  Documentation/driver-api/pci/pci.rst |  3 ++
>  3 files changed, 61 insertions(+)
>  create mode 100644 Documentation/PCI/tph.rst
> 
> diff --git a/Documentation/PCI/index.rst b/Documentation/PCI/index.rst
> index e73f84aebde3..5e7c4e6e726b 100644
> --- a/Documentation/PCI/index.rst
> +++ b/Documentation/PCI/index.rst
> @@ -18,3 +18,4 @@ PCI Bus Subsystem
>     pcieaer-howto
>     endpoint/index
>     boot-interrupts
> +   tph
> diff --git a/Documentation/PCI/tph.rst b/Documentation/PCI/tph.rst
> new file mode 100644
> index 000000000000..ea9c8313f3e4
> --- /dev/null
> +++ b/Documentation/PCI/tph.rst
> @@ -0,0 +1,57 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +===========
> +TPH Support
> +===========
> +
> +
> +:Copyright: 2024 Advanced Micro Devices, Inc.
> +:Authors: - Eric van Tassell <eric.vantassell@amd.com>
> +          - Wei Huang <wei.huang2@amd.com>
> +
> +Overview
> +========
> +TPH (TLP Processing Hints) is a PCIe feature that allows endpoint devices
> +to provide optimization hints, such as desired caching behavior, for
> +requests that target memory space. These hints, in a format called steering
> +tags, are provided in the requester's TLP headers and can empower the system
> +hardware, including the Root Complex, to optimize the utilization of platform
> +resources for the requests.
> +
> +User Guide
> +==========
> +
> +Kernel Options
> +--------------
> +There are two kernel command line options available to control TPH feature
> +
> +   * "notph": TPH will be disabled for all endpoint devices.
> +   * "nostmode": TPH will be enabled but the ST Mode will be forced to "No ST Mode".
> +
> +Device Driver API
> +-----------------
> +In brief, an endpoint device driver using the TPH interface to configure
> +Interrupt Vector Mode will call pcie_tph_set_st() when setting up MSI-X
> +interrupts as shown below:
> +
> +.. code-block:: c
> +
> +    for (i = 0, j = 0; i < nr_rings; i++) {
> +        ...
> +        rc = request_irq(irq->vector, irq->handler, flags, irq->name, NULL);
> +        ...
> +        if (!pcie_tph_set_st(pdev, i, cpumask_first(irq->cpu_mask),
> +                             TPH_MEM_TYPE_VM, PCI_TPH_REQ_TPH_ONLY))
> +               pr_err("Error in configuring steering tag\n");
> +        ...
> +    }
> +
> +If a device only supports TPH vendor specific mode, its driver can call
> +pcie_tph_get_st() to retrieve the steering tag for a specific CPU and uses
> +the tag to control TPH behavior.
> +
> +.. kernel-doc:: drivers/pci/pcie/tph.c
> +   :export:
> +
> +.. kernel-doc:: drivers/pci/pcie/tph.c
> +   :identifiers: pcie_tph_set_st
> diff --git a/Documentation/driver-api/pci/pci.rst b/Documentation/driver-api/pci/pci.rst
> index aa40b1cc243b..3d896b2cf16e 100644
> --- a/Documentation/driver-api/pci/pci.rst
> +++ b/Documentation/driver-api/pci/pci.rst
> @@ -46,6 +46,9 @@ PCI Support Library
>  .. kernel-doc:: drivers/pci/pci-sysfs.c
>     :internal:
>  
> +.. kernel-doc:: drivers/pci/pcie/tph.c
> +   :export:
> +
>  PCI Hotplug Support Library
>  ---------------------------
>  

[PATCH V2 8/9] bnxt_en: Add TPH support in BNXT driver

[Wei Huang]

From: Manoj Panicker <manoj.panicker2@amd.com>

As a usage example, this patch implements TPH support in Broadcom BNXT
device driver by invoking pcie_tph_set_st() function when interrupt
affinity is changed.

Signed-off-by: Manoj Panicker <manoj.panicker2@amd.com>
Reviewed-by: Wei Huang <wei.huang2@amd.com>
Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com> 
Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 54 +++++++++++++++++++++++
 drivers/net/ethernet/broadcom/bnxt/bnxt.h |  4 ++
 2 files changed, 58 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index c437ca1c0fd3..2207dac8ce18 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -55,6 +55,7 @@
 #include <net/page_pool/helpers.h>
 #include <linux/align.h>
 #include <net/netdev_queues.h>
+#include <linux/pci-tph.h>
 
 #include "bnxt_hsi.h"
 #include "bnxt.h"
@@ -10683,6 +10684,8 @@ static void bnxt_free_irq(struct bnxt *bp)
 				free_cpumask_var(irq->cpu_mask);
 				irq->have_cpumask = 0;
 			}
+			if (pcie_tph_intr_vec_supported(bp->pdev))
+				irq_set_affinity_notifier(irq->vector, NULL);
 			free_irq(irq->vector, bp->bnapi[i]);
 		}
 
@@ -10690,6 +10693,45 @@ static void bnxt_free_irq(struct bnxt *bp)
 	}
 }
 
+static void bnxt_rtnl_lock_sp(struct bnxt *bp);
+static void bnxt_rtnl_unlock_sp(struct bnxt *bp);
+static void __bnxt_irq_affinity_notify(struct irq_affinity_notify *notify,
+				       const cpumask_t *mask)
+{
+	struct bnxt_irq *irq;
+
+	irq = container_of(notify, struct bnxt_irq, affinity_notify);
+	cpumask_copy(irq->cpu_mask, mask);
+
+	if (!pcie_tph_set_st(irq->bp->pdev, irq->msix_nr,
+			     cpumask_first(irq->cpu_mask),
+			     TPH_MEM_TYPE_VM, PCI_TPH_REQ_TPH_ONLY))
+		netdev_dbg(irq->bp->dev, "error in setting steering tag\n");
+
+	if (netif_running(irq->bp->dev)) {
+		rtnl_lock();
+		bnxt_close_nic(irq->bp, false, false);
+		bnxt_open_nic(irq->bp, false, false);
+		rtnl_unlock();
+	}
+}
+
+static void __bnxt_irq_affinity_release(struct kref __always_unused *ref)
+{
+}
+
+static inline void bnxt_register_affinity_notifier(struct bnxt_irq *irq)
+{
+	struct irq_affinity_notify *notify;
+
+	notify = &irq->affinity_notify;
+	notify->irq = irq->vector;
+	notify->notify = __bnxt_irq_affinity_notify;
+	notify->release = __bnxt_irq_affinity_release;
+
+	irq_set_affinity_notifier(irq->vector, notify);
+}
+
 static int bnxt_request_irq(struct bnxt *bp)
 {
 	int i, j, rc = 0;
@@ -10735,6 +10777,7 @@ static int bnxt_request_irq(struct bnxt *bp)
 			int numa_node = dev_to_node(&bp->pdev->dev);
 
 			irq->have_cpumask = 1;
+			irq->msix_nr = map_idx;
 			cpumask_set_cpu(cpumask_local_spread(i, numa_node),
 					irq->cpu_mask);
 			rc = irq_set_affinity_hint(irq->vector, irq->cpu_mask);
@@ -10744,6 +10787,17 @@ static int bnxt_request_irq(struct bnxt *bp)
 					    irq->vector);
 				break;
 			}
+
+			if (pcie_tph_intr_vec_supported(bp->pdev)) {
+				irq->bp = bp;
+				bnxt_register_affinity_notifier(irq);
+
+				/* first setup */
+				if (!pcie_tph_set_st(bp->pdev, i,
+						     cpumask_first(irq->cpu_mask),
+						     TPH_MEM_TYPE_VM, PCI_TPH_REQ_TPH_ONLY))
+					netdev_dbg(bp->dev, "error in setting steering tag\n");
+			}
 		}
 	}
 	return rc;
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
index 656ab81c0272..4a841e8ccfb7 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
@@ -1195,6 +1195,10 @@ struct bnxt_irq {
 	u8		have_cpumask:1;
 	char		name[IFNAMSIZ + 2];
 	cpumask_var_t	cpu_mask;
+
+	int		msix_nr;
+	struct bnxt	*bp;
+	struct irq_affinity_notify affinity_notify;
 };
 
 #define HWRM_RING_ALLOC_TX	0x1
-- 
2.44.0

[PATCH V2 9/9] bnxt_en: Pass NQ ID to the FW when allocating RX/RX AGG rings

[Wei Huang]

From: Michael Chan <michael.chan@broadcom.com>

Newer firmware can use the NQ ring ID associated with each RX/RX AGG
ring to enable PCIe steering tag.  Older firmware will just ignore the
information.

Signed-off-by: Michael Chan <michael.chan@broadcom.com>
Signed-off-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
Reviewed-by: Hongguang Gao <hongguang.gao@broadcom.com>
Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 2207dac8ce18..308b4747d041 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -6699,10 +6699,12 @@ static int hwrm_ring_alloc_send_msg(struct bnxt *bp,
 
 			/* Association of rx ring with stats context */
 			grp_info = &bp->grp_info[ring->grp_idx];
+			req->nq_ring_id = cpu_to_le16(grp_info->cp_fw_ring_id);
 			req->rx_buf_size = cpu_to_le16(bp->rx_buf_use_size);
 			req->stat_ctx_id = cpu_to_le32(grp_info->fw_stats_ctx);
 			req->enables |= cpu_to_le32(
-				RING_ALLOC_REQ_ENABLES_RX_BUF_SIZE_VALID);
+				RING_ALLOC_REQ_ENABLES_RX_BUF_SIZE_VALID |
+				RING_ALLOC_REQ_ENABLES_NQ_RING_ID_VALID);
 			if (NET_IP_ALIGN == 2)
 				flags = RING_ALLOC_REQ_FLAGS_RX_SOP_PAD;
 			req->flags = cpu_to_le16(flags);
@@ -6714,11 +6716,13 @@ static int hwrm_ring_alloc_send_msg(struct bnxt *bp,
 			/* Association of agg ring with rx ring */
 			grp_info = &bp->grp_info[ring->grp_idx];
 			req->rx_ring_id = cpu_to_le16(grp_info->rx_fw_ring_id);
+			req->nq_ring_id = cpu_to_le16(grp_info->cp_fw_ring_id);
 			req->rx_buf_size = cpu_to_le16(BNXT_RX_PAGE_SIZE);
 			req->stat_ctx_id = cpu_to_le32(grp_info->fw_stats_ctx);
 			req->enables |= cpu_to_le32(
 				RING_ALLOC_REQ_ENABLES_RX_RING_ID_VALID |
-				RING_ALLOC_REQ_ENABLES_RX_BUF_SIZE_VALID);
+				RING_ALLOC_REQ_ENABLES_RX_BUF_SIZE_VALID |
+				RING_ALLOC_REQ_ENABLES_NQ_RING_ID_VALID);
 		} else {
 			req->ring_type = RING_ALLOC_REQ_RING_TYPE_RX;
 		}
-- 
2.44.0