diff mbox series

[V3,04/10] PCI/TPH: Add pci=nostmode to force No ST Mode

Message ID 20240717205511.2541693-5-wei.huang2@amd.com (mailing list archive)
State Superseded
Delegated to: Bjorn Helgaas
Headers show
Series PCIe TPH and cache direct injection support | expand

Commit Message

Wei Huang July 17, 2024, 8:55 p.m. UTC
When "No ST mode" is enabled, endpoint 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                        | 22 +++++++++++++++++++
 include/linux/pci-tph.h                       |  2 ++
 include/linux/pci.h                           |  1 +
 6 files changed, 44 insertions(+), 1 deletion(-)

Comments

Bjorn Helgaas July 23, 2024, 10:44 p.m. UTC | #1
On Wed, Jul 17, 2024 at 03:55:05PM -0500, Wei Huang wrote:
> When "No ST mode" is enabled, endpoint 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.

Capitalize technical terms defined by spec.

> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -4656,6 +4656,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

Needs a little more context here about what this means.  Users won't
know where to even look for "No ST Mode" unless they have a copy of
the spec.

> +++ 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())
> +			pcie_tph_set_nostmode(pci_dev);

Same comment here; can we do this outside the probe() path somehow?

>  		return rc;
>  	}
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 4cbfd5b53be8..8745ce1c4a9a 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);

s/pci/pcie/

Unexport unless it's useful for drivers.

Bjorn
Wei Huang Aug. 2, 2024, 4:29 a.m. UTC | #2
On 7/23/24 17:44, Bjorn Helgaas wrote:
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -4656,6 +4656,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
> 
> Needs a little more context here about what this means.  Users won't
> know where to even look for "No ST Mode" unless they have a copy of
> the spec.
> 

I can certainly add more description to talk about "No ST Mode". Also, 
will "tph_nostmode" be better than "nostmode" in your opinion?
Bjorn Helgaas Aug. 2, 2024, 5:05 p.m. UTC | #3
On Thu, Aug 01, 2024 at 11:29:22PM -0500, Wei Huang wrote:
> On 7/23/24 17:44, Bjorn Helgaas wrote:
> > > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > > @@ -4656,6 +4656,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
> > 
> > Needs a little more context here about what this means.  Users won't
> > know where to even look for "No ST Mode" unless they have a copy of
> > the spec.
> 
> I can certainly add more description to talk about "No ST Mode". Also, will
> "tph_nostmode" be better than "nostmode" in your opinion?

I don't really care about the parameter name.  I think just spelling
these out once, e.g., "PCIe TLP Processing Hints (TPH)" and "No ST
Mode (Steering Tags must be all zeros)" is probably enough context.
diff mbox series

Patch

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 65581ebd9b50..1b761f062969 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -4656,6 +4656,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..abe66541536e 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())
+			pcie_tph_set_nostmode(pci_dev);
 
 		return rc;
 	}
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 4cbfd5b53be8..8745ce1c4a9a 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 */
@@ -6881,6 +6890,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 ad58a892792c..fb8e2f920712 100644
--- a/drivers/pci/pcie/tph.c
+++ b/drivers/pci/pcie/tph.c
@@ -13,6 +13,19 @@ 
 
 #include "../pci.h"
 
+/* Update the ST Mode Select field of TPH Control Register */
+static void set_ctrl_reg_mode_sel(struct pci_dev *pdev, u8 st_mode)
+{
+	u32 reg_val;
+
+	pci_read_config_dword(pdev, pdev->tph_cap + PCI_TPH_CTRL, &reg_val);
+
+	reg_val &= ~PCI_TPH_CTRL_MODE_SEL_MASK;
+	reg_val |= FIELD_PREP(PCI_TPH_CTRL_MODE_SEL_MASK, st_mode);
+
+	pci_write_config_dword(pdev, pdev->tph_cap + PCI_TPH_CTRL, reg_val);
+}
+
 /* Update the TPH Requester Enable field of TPH Control Register */
 static void set_ctrl_reg_req_en(struct pci_dev *pdev, u8 req_type)
 {
@@ -26,6 +39,15 @@  static void set_ctrl_reg_req_en(struct pci_dev *pdev, u8 req_type)
 	pci_write_config_dword(pdev, pdev->tph_cap + PCI_TPH_CTRL, reg_val);
 }
 
+void pcie_tph_set_nostmode(struct pci_dev *pdev)
+{
+	if (!pdev->tph_cap)
+		return;
+
+	set_ctrl_reg_mode_sel(pdev, PCI_TPH_NO_ST_MODE);
+	set_ctrl_reg_req_en(pdev, PCI_TPH_REQ_TPH_ONLY);
+}
+
 void pcie_tph_disable(struct pci_dev *pdev)
 {
 	if (!pdev->tph_cap)
diff --git a/include/linux/pci-tph.h b/include/linux/pci-tph.h
index e0b782bda929..8fce3969277c 100644
--- a/include/linux/pci-tph.h
+++ b/include/linux/pci-tph.h
@@ -11,8 +11,10 @@ 
 
 #ifdef CONFIG_PCIE_TPH
 void pcie_tph_disable(struct pci_dev *dev);
+void pcie_tph_set_nostmode(struct pci_dev *dev);
 #else
 static inline void pcie_tph_disable(struct pci_dev *dev) {}
+static inline void pcie_tph_set_nostmode(struct pci_dev *dev) {}
 #endif
 
 #endif /* LINUX_PCI_TPH_H */
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 05fbbd9ad6b4..ac58f3919993 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1841,6 +1841,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);