diff mbox series

[v5] PCI: Add save and restore capability for TPH config space

Message ID 20220708100516.2442-1-paul.e.luse@intel.com (mailing list archive)
State Superseded
Headers show
Series [v5] PCI: Add save and restore capability for TPH config space | expand

Commit Message

Luse, Paul E July 8, 2022, 10:05 a.m. UTC 
From: paul luse <paul.e.luse@intel.com>

The PCI subsystem does not currently save and restore the configuration
space for the TPH (Transaction Layer Packet Processing Hints) capability,
leading to the possibility of configuration loss in some scenarios. For
more information please refer to PCIe r6.0 sec 6.17.

This was discovered working on the SPDK Project (Storage Performance
Development Kit, see https://spdk.io/) where we typically unbind devices
from their native kernel driver and bind to VFIO for use with our own
user space drivers. The process of binding to VFIO results in the loss
of TPH settings due to the resulting PCI reset.

Signed-off-by: Jing Liu <jing2.liu@intel.com>
Signed-off-by: paul luse <paul.e.luse@intel.com>
---
 drivers/pci/pci.c             |  2 +
 drivers/pci/pci.h             |  4 ++
 drivers/pci/pcie/Makefile     |  1 +
 drivers/pci/pcie/tph.c        | 93 +++++++++++++++++++++++++++++++++++
 drivers/pci/probe.c           |  1 +
 include/uapi/linux/pci_regs.h |  2 +
 6 files changed, 103 insertions(+)
 create mode 100644 drivers/pci/pcie/tph.c

Comments

Kuppuswamy Sathyanarayanan July 8, 2022, 2:16 p.m. UTC | #1 
Hi Paul,

On 7/8/22 3:05 AM, Paul Luse wrote:
> From: paul luse <paul.e.luse@intel.com>
> 
> The PCI subsystem does not currently save and restore the configuration
> space for the TPH (Transaction Layer Packet Processing Hints) capability,

May be Transaction Layer Packet Processing Hints (TPH) would be better?

> leading to the possibility of configuration loss in some scenarios. For
> more information please refer to PCIe r6.0 sec 6.17.
> 
> This was discovered working on the SPDK Project (Storage Performance
> Development Kit, see https://spdk.io/) where we typically unbind devices
> from their native kernel driver and bind to VFIO for use with our own
> user space drivers. The process of binding to VFIO results in the loss
> of TPH settings due to the resulting PCI reset.
> 
> Signed-off-by: Jing Liu <jing2.liu@intel.com>

Why about signed-off from Jing? Is this co-developed by Jing? If yes,
I think you should use Co-developed-by:

> Signed-off-by: paul luse <paul.e.luse@intel.com>
> ---

I think your version number is incorrect. Is this supposed to be v3?

Please include version log.

>  drivers/pci/pci.c             |  2 +
>  drivers/pci/pci.h             |  4 ++
>  drivers/pci/pcie/Makefile     |  1 +
>  drivers/pci/pcie/tph.c        | 93 +++++++++++++++++++++++++++++++++++
>  drivers/pci/probe.c           |  1 +
>  include/uapi/linux/pci_regs.h |  2 +
>  6 files changed, 103 insertions(+)
>  create mode 100644 drivers/pci/pcie/tph.c
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index cfaf40a540a8..158712bf3069 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1670,6 +1670,7 @@ int pci_save_state(struct pci_dev *dev)
>  	pci_save_dpc_state(dev);
>  	pci_save_aer_state(dev);
>  	pci_save_ptm_state(dev);
> +	pci_save_tph_state(dev);
>  	return pci_save_vc_state(dev);
>  }
>  EXPORT_SYMBOL(pci_save_state);
> @@ -1782,6 +1783,7 @@ void pci_restore_state(struct pci_dev *dev)
>  	pci_restore_rebar_state(dev);
>  	pci_restore_dpc_state(dev);
>  	pci_restore_ptm_state(dev);
> +	pci_restore_tph_state(dev);
>  
>  	pci_aer_clear_status(dev);
>  	pci_restore_aer_state(dev);
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index e10cdec6c56e..e6214c8e8cb7 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -514,6 +514,10 @@ static inline void pci_restore_ptm_state(struct pci_dev *dev) { }
>  static inline void pci_disable_ptm(struct pci_dev *dev) { }
>  #endif
>  
> +void pci_save_tph_state(struct pci_dev *dev);
> +void pci_restore_tph_state(struct pci_dev *dev);
> +void pci_tph_init(struct pci_dev *dev);

If you follow my following suggestion about moving tph.c contents to
pci.c, you don't need to declare above functions.
 

> +
>  unsigned long pci_cardbus_resource_alignment(struct resource *);
>  
>  static inline resource_size_t pci_resource_alignment(struct pci_dev *dev,
> diff --git a/drivers/pci/pcie/Makefile b/drivers/pci/pcie/Makefile
> index 5783a2f79e6a..1287ec65fb30 100644
> --- a/drivers/pci/pcie/Makefile
> +++ b/drivers/pci/pcie/Makefile
> @@ -3,6 +3,7 @@
>  # Makefile for PCI Express features and port driver
>  
>  pcieportdrv-y			:= portdrv_core.o portdrv_pci.o rcec.o
> +obj-y                           += tph.o
>  
>  obj-$(CONFIG_PCIEPORTBUS)	+= pcieportdrv.o
>  
> diff --git a/drivers/pci/pcie/tph.c b/drivers/pci/pcie/tph.c
> new file mode 100644
> index 000000000000..c0d2f20b7d53
> --- /dev/null
> +++ b/drivers/pci/pcie/tph.c

IMO, you don't need a separate file for this. This could be moved to
pci.c. Check for PCI_EXT_CAP_ID_LTR cap implementation.

> @@ -0,0 +1,93 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * PCI Express Transaction Layer Packet Processing Hints
> + * Copyright (c) 2022, Intel Corporation.
> + */
> +
> +#include "../pci.h"
> +
> +static unsigned int pci_get_tph_st_num_entries(struct pci_dev *dev, u16 tph)
> +{
> +	int num_entries = 0;
> +	u32 cap;
> +
> +	pci_read_config_dword(dev, tph + PCI_TPH_CAP, &cap);
> +	if ((cap & PCI_TPH_CAP_LOC_MASK) == PCI_TPH_LOC_CAP) {
> +		/* per PCI spec, table entries is encoded as N-1 */

Per PCIe spec r6.0, sec titled "TPH Requester Capability Register"

> +		num_entries = ((cap & PCI_TPH_CAP_ST_MASK) >> PCI_TPH_CAP_ST_SHIFT) + 1;
> +	}
> +
> +	return num_entries;
> +}
> +
> +void pci_save_tph_state(struct pci_dev *dev)
> +{
> +	struct pci_cap_saved_state *save_state;
> +	int num_entries, i, offset;
> +	u16 *st_entry, tph;
> +	u32 *cap;
> +
> +	tph = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_TPH);
> +	if (!tph)
> +		return;

I think the above check is redundant. I believe following function
will return NULL if capability is not there.

> +
> +	save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_TPH);
> +	if (!save_state)
> +		return;
> +
> +	/* Save control register as well as all ST entries */
> +	cap = &save_state->cap.data[0];
> +	pci_read_config_dword(dev, tph + PCI_TPH_CTL, cap++);
> +	st_entry = (u16 *)cap;
> +	offset = PCI_TPH_ST_TBL;
> +	num_entries = pci_get_tph_st_num_entries(dev, tph);
> +	for (i = 0; i < num_entries; i++) {
> +		pci_read_config_word(dev, tph + offset, st_entry++);

pci_read_config_word(dev, tph + PCI_TPH_ST_TBL + 2*i, cap++) ?

> +		offset += sizeof(u16);
> +	}
> +}
> +
> +void pci_restore_tph_state(struct pci_dev *dev)
> +{
> +	struct pci_cap_saved_state *save_state;
> +	int num_entries, i, offset;
> +	u16 *st_entry, tph;
> +	u32 *cap;
> +
> +	tph = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_TPH);
> +	if (!tph)
> +		return;

Same as above

> +
> +	save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_TPH);
> +	if (!save_state)
> +		return;
> +
> +	/* Restore control register as well as all ST entries */
> +	cap = &save_state->cap.data[0];
> +	pci_write_config_dword(dev, tph + PCI_TPH_CTL, *cap++);
> +	st_entry = (u16 *)cap;
> +	offset = PCI_TPH_ST_TBL;
> +	num_entries = pci_get_tph_st_num_entries(dev, tph);
> +	for (i = 0; i < num_entries; i++) {
> +		pci_write_config_word(dev, tph + offset, *st_entry++);
> +		offset += sizeof(u16);

Same as above

> +	}
> +}
> +
> +void pci_tph_init(struct pci_dev *dev)
> +{
> +	int num_entries;
> +	u32 save_size;
> +	u16 tph;
> +
> +	if (!pci_is_pcie(dev))
> +		return;
> +
> +	tph = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_TPH);
> +	if (!tph)
> +		return;
> +
> +	num_entries = pci_get_tph_st_num_entries(dev, tph);
> +	save_size = sizeof(int) + num_entries * sizeof(u16);
> +	pci_add_ext_cap_save_buffer(dev, PCI_EXT_CAP_ID_TPH, save_size);
> +}

I think you can move pci_tph_init() to pci_allocate_cap_save_buffers().

> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 17a969942d37..1f5da3dbf128 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2464,6 +2464,7 @@ static void pci_init_capabilities(struct pci_dev *dev)
>  	pci_aer_init(dev);		/* Advanced Error Reporting */
>  	pci_dpc_init(dev);		/* Downstream Port Containment */
>  	pci_rcec_init(dev);		/* Root Complex Event Collector */
> +	pci_tph_init(dev);              /* Transaction Layer Packet Processing Hints */
>  
>  	pcie_report_downtraining(dev);
>  	pci_init_reset_methods(dev);
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index 108f8523fa04..2d8b719adbab 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -1020,6 +1020,8 @@
>  #define   PCI_TPH_LOC_MSIX	0x400	/* in MSI-X */
>  #define PCI_TPH_CAP_ST_MASK	0x07FF0000	/* ST table mask */
>  #define PCI_TPH_CAP_ST_SHIFT	16	/* ST table shift */
> +#define PCI_TPH_CTL             8       /* control offset */

Follow same hex format as below? 0x08

> +#define PCI_TPH_ST_TBL          0xc     /* ST table offset */
>  #define PCI_TPH_BASE_SIZEOF	0xc	/* size with no ST table */
>  
>  /* Downstream Port Containment */
diff mbox series

Patch

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index cfaf40a540a8..158712bf3069 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1670,6 +1670,7 @@  int pci_save_state(struct pci_dev *dev)
 	pci_save_dpc_state(dev);
 	pci_save_aer_state(dev);
 	pci_save_ptm_state(dev);
+	pci_save_tph_state(dev);
 	return pci_save_vc_state(dev);
 }
 EXPORT_SYMBOL(pci_save_state);
@@ -1782,6 +1783,7 @@  void pci_restore_state(struct pci_dev *dev)
 	pci_restore_rebar_state(dev);
 	pci_restore_dpc_state(dev);
 	pci_restore_ptm_state(dev);
+	pci_restore_tph_state(dev);
 
 	pci_aer_clear_status(dev);
 	pci_restore_aer_state(dev);
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index e10cdec6c56e..e6214c8e8cb7 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -514,6 +514,10 @@  static inline void pci_restore_ptm_state(struct pci_dev *dev) { }
 static inline void pci_disable_ptm(struct pci_dev *dev) { }
 #endif
 
+void pci_save_tph_state(struct pci_dev *dev);
+void pci_restore_tph_state(struct pci_dev *dev);
+void pci_tph_init(struct pci_dev *dev);
+
 unsigned long pci_cardbus_resource_alignment(struct resource *);
 
 static inline resource_size_t pci_resource_alignment(struct pci_dev *dev,
diff --git a/drivers/pci/pcie/Makefile b/drivers/pci/pcie/Makefile
index 5783a2f79e6a..1287ec65fb30 100644
--- a/drivers/pci/pcie/Makefile
+++ b/drivers/pci/pcie/Makefile
@@ -3,6 +3,7 @@ 
 # Makefile for PCI Express features and port driver
 
 pcieportdrv-y			:= portdrv_core.o portdrv_pci.o rcec.o
+obj-y                           += tph.o
 
 obj-$(CONFIG_PCIEPORTBUS)	+= pcieportdrv.o
 
diff --git a/drivers/pci/pcie/tph.c b/drivers/pci/pcie/tph.c
new file mode 100644
index 000000000000..c0d2f20b7d53
--- /dev/null
+++ b/drivers/pci/pcie/tph.c
@@ -0,0 +1,93 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * PCI Express Transaction Layer Packet Processing Hints
+ * Copyright (c) 2022, Intel Corporation.
+ */
+
+#include "../pci.h"
+
+static unsigned int pci_get_tph_st_num_entries(struct pci_dev *dev, u16 tph)
+{
+	int num_entries = 0;
+	u32 cap;
+
+	pci_read_config_dword(dev, tph + PCI_TPH_CAP, &cap);
+	if ((cap & PCI_TPH_CAP_LOC_MASK) == PCI_TPH_LOC_CAP) {
+		/* per PCI spec, table entries is encoded as N-1 */
+		num_entries = ((cap & PCI_TPH_CAP_ST_MASK) >> PCI_TPH_CAP_ST_SHIFT) + 1;
+	}
+
+	return num_entries;
+}
+
+void pci_save_tph_state(struct pci_dev *dev)
+{
+	struct pci_cap_saved_state *save_state;
+	int num_entries, i, offset;
+	u16 *st_entry, tph;
+	u32 *cap;
+
+	tph = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_TPH);
+	if (!tph)
+		return;
+
+	save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_TPH);
+	if (!save_state)
+		return;
+
+	/* Save control register as well as all ST entries */
+	cap = &save_state->cap.data[0];
+	pci_read_config_dword(dev, tph + PCI_TPH_CTL, cap++);
+	st_entry = (u16 *)cap;
+	offset = PCI_TPH_ST_TBL;
+	num_entries = pci_get_tph_st_num_entries(dev, tph);
+	for (i = 0; i < num_entries; i++) {
+		pci_read_config_word(dev, tph + offset, st_entry++);
+		offset += sizeof(u16);
+	}
+}
+
+void pci_restore_tph_state(struct pci_dev *dev)
+{
+	struct pci_cap_saved_state *save_state;
+	int num_entries, i, offset;
+	u16 *st_entry, tph;
+	u32 *cap;
+
+	tph = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_TPH);
+	if (!tph)
+		return;
+
+	save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_TPH);
+	if (!save_state)
+		return;
+
+	/* Restore control register as well as all ST entries */
+	cap = &save_state->cap.data[0];
+	pci_write_config_dword(dev, tph + PCI_TPH_CTL, *cap++);
+	st_entry = (u16 *)cap;
+	offset = PCI_TPH_ST_TBL;
+	num_entries = pci_get_tph_st_num_entries(dev, tph);
+	for (i = 0; i < num_entries; i++) {
+		pci_write_config_word(dev, tph + offset, *st_entry++);
+		offset += sizeof(u16);
+	}
+}
+
+void pci_tph_init(struct pci_dev *dev)
+{
+	int num_entries;
+	u32 save_size;
+	u16 tph;
+
+	if (!pci_is_pcie(dev))
+		return;
+
+	tph = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_TPH);
+	if (!tph)
+		return;
+
+	num_entries = pci_get_tph_st_num_entries(dev, tph);
+	save_size = sizeof(int) + num_entries * sizeof(u16);
+	pci_add_ext_cap_save_buffer(dev, PCI_EXT_CAP_ID_TPH, save_size);
+}
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 17a969942d37..1f5da3dbf128 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2464,6 +2464,7 @@  static void pci_init_capabilities(struct pci_dev *dev)
 	pci_aer_init(dev);		/* Advanced Error Reporting */
 	pci_dpc_init(dev);		/* Downstream Port Containment */
 	pci_rcec_init(dev);		/* Root Complex Event Collector */
+	pci_tph_init(dev);              /* Transaction Layer Packet Processing Hints */
 
 	pcie_report_downtraining(dev);
 	pci_init_reset_methods(dev);
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index 108f8523fa04..2d8b719adbab 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -1020,6 +1020,8 @@ 
 #define   PCI_TPH_LOC_MSIX	0x400	/* in MSI-X */
 #define PCI_TPH_CAP_ST_MASK	0x07FF0000	/* ST table mask */
 #define PCI_TPH_CAP_ST_SHIFT	16	/* ST table shift */
+#define PCI_TPH_CTL             8       /* control offset */
+#define PCI_TPH_ST_TBL          0xc     /* ST table offset */
 #define PCI_TPH_BASE_SIZEOF	0xc	/* size with no ST table */
 
 /* Downstream Port Containment */