[PATCH 0/3] pci: virtual channel save/restore support

[PATCH 0/3] pci: virtual channel save/restore support

[Alex Williamson]

Turns out that even though we don't do much with virtual channel, we
still need to save/restore it around reset.  The case where this comes
into play currently is a card with a switch and multiple devices, all
supporting VC (Nvidia GRID).  On some platforms the system firmware
will leave all the virtual channel capabilities in their default
state, where the VC0 TC/VC map is 0xff (TC0-7 enabled over VC0).
Other systems decide to restrict the available traffic classes and
configure 0x01 (only TC0 over VC0).  At handoff, everything works, but
as soon as we do a device reset, especially if done via bus reset, the
endpoint can be returned to the spec defined default rather than the
platform default.  If such a device is then assigned to a guest, the
guest driver sees multiple TCs enabled, attempts to use them, and it
fails.  The failure mode depends on the platform, ranging from the
driver in the guest failing to work to host panic from an unsupported
transaction.

This series attempts to implement full save/restore support for all
types of virtual channel (VC, MFVC, and VC9).  The buffer sizing code
will execute for any device with these capabilities, but of couse the
actual save/restore is only done if a reset is requested for the
device, which typically means assignment to a VM.  Thanks,

Alex

---

Alex Williamson (3):
      pci: Generalize wait loop
      pci: Add support for save/restore of extended capabilities
      pci: Add Virtual Channel to save/restore support


 drivers/pci/pci.c   |  488 ++++++++++++++++++++++++++++++++++++++++++++++++---
 include/linux/pci.h |    3 
 2 files changed, 459 insertions(+), 32 deletions(-)

[PATCH 1/3] pci: Generalize wait loop

[Alex Williamson]

We currently have two instance of this loop which waits for a pending
bit to clear in a status dword.  Generalize the function for future
users.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 drivers/pci/pci.c |   54 ++++++++++++++++++++++++++++++-----------------------
 1 file changed, 31 insertions(+), 23 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 33120d1..05868e4 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -431,6 +431,32 @@ pci_find_parent_resource(const struct pci_dev *dev, struct resource *res)
 }
 
 /**
+ * pci_wait_for_pending - wait for @mask bit(s) to clear in status word @pos
+ * @dev: the PCI device to operate on
+ * @pos: config space offset of status word
+ * @mask: mask of bit(s) to care about in status word
+ *
+ * Return 1 when mask bit(s) in status word clear, 0 otherwise.
+ */
+static int pci_wait_for_pending(struct pci_dev *dev, int pos, u16 mask)
+{
+	int i;
+
+	/* Wait for Transaction Pending bit clean */
+	for (i = 0; i < 4; i++) {
+		u16 status;
+		if (i)
+			msleep((1 << (i - 1)) * 100);
+
+		pci_read_config_word(dev, pos, &status);
+		if (!(status & mask))
+			return 1;
+	}
+
+	return 0;
+}
+
+/**
  * pci_restore_bars - restore a devices BAR values (e.g. after wake-up)
  * @dev: PCI device to have its BARs restored
  *
@@ -3204,20 +3230,10 @@ EXPORT_SYMBOL(pci_set_dma_seg_boundary);
  */
 int pci_wait_for_pending_transaction(struct pci_dev *dev)
 {
-	int i;
-	u16 status;
-
-	/* Wait for Transaction Pending bit clean */
-	for (i = 0; i < 4; i++) {
-		if (i)
-			msleep((1 << (i - 1)) * 100);
-
-		pcie_capability_read_word(dev, PCI_EXP_DEVSTA, &status);
-		if (!(status & PCI_EXP_DEVSTA_TRPND))
-			return 1;
-	}
+	if (!pci_is_pcie(dev))
+		return 1;
 
-	return 0;
+	return pci_wait_for_pending(dev, PCI_EXP_DEVSTA, PCI_EXP_DEVSTA_TRPND);
 }
 EXPORT_SYMBOL(pci_wait_for_pending_transaction);
 
@@ -3244,10 +3260,8 @@ static int pcie_flr(struct pci_dev *dev, int probe)
 
 static int pci_af_flr(struct pci_dev *dev, int probe)
 {
-	int i;
 	int pos;
 	u8 cap;
-	u8 status;
 
 	pos = pci_find_capability(dev, PCI_CAP_ID_AF);
 	if (!pos)
@@ -3261,14 +3275,8 @@ static int pci_af_flr(struct pci_dev *dev, int probe)
 		return 0;
 
 	/* Wait for Transaction Pending bit clean */
-	for (i = 0; i < 4; i++) {
-		if (i)
-			msleep((1 << (i - 1)) * 100);
-
-		pci_read_config_byte(dev, pos + PCI_AF_STATUS, &status);
-		if (!(status & PCI_AF_STATUS_TP))
-			goto clear;
-	}
+	if (pci_wait_for_pending(dev, PCI_AF_STATUS, PCI_AF_STATUS_TP))
+		goto clear;
 
 	dev_err(&dev->dev, "transaction is not cleared; "
 			"proceeding with reset anyway\n");

[PATCH 2/3] pci: Add support for save/restore of extended capabilities

[Alex Williamson]

Current save/restore is specific to standard capabilities.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 drivers/pci/pci.c   |   47 +++++++++++++++++++++++++++++++++++++++--------
 include/linux/pci.h |    3 ++-
 2 files changed, 41 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 05868e4..a898e4e 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -861,18 +861,30 @@ EXPORT_SYMBOL(pci_choose_state);
 #define PCI_EXP_SAVE_REGS	7
 
 
-static struct pci_cap_saved_state *pci_find_saved_cap(
-	struct pci_dev *pci_dev, char cap)
+static struct pci_cap_saved_state *_pci_find_saved_cap(struct pci_dev *pci_dev,
+						       u16 cap, bool extended)
 {
 	struct pci_cap_saved_state *tmp;
 
 	hlist_for_each_entry(tmp, &pci_dev->saved_cap_space, next) {
-		if (tmp->cap.cap_nr == cap)
+		if (tmp->cap.cap_extended == extended && tmp->cap.cap_nr == cap)
 			return tmp;
 	}
 	return NULL;
 }
 
+static struct pci_cap_saved_state *pci_find_saved_cap(struct pci_dev *dev,
+						      char cap)
+{
+	return _pci_find_saved_cap(dev, cap, false);
+}
+
+static struct pci_cap_saved_state *pci_find_saved_ext_cap(struct pci_dev *dev,
+							  u16 cap)
+{
+	return _pci_find_saved_cap(dev, cap, true);
+}
+
 static int pci_save_pcie_state(struct pci_dev *dev)
 {
 	int i = 0;
@@ -1113,7 +1125,7 @@ int pci_load_saved_state(struct pci_dev *dev, struct pci_saved_state *state)
 	while (cap->size) {
 		struct pci_cap_saved_state *tmp;
 
-		tmp = pci_find_saved_cap(dev, cap->cap_nr);
+		tmp = _pci_find_saved_cap(dev, cap->cap_nr, cap->cap_extended);
 		if (!tmp || tmp->cap.size != cap->size)
 			return -EINVAL;
 
@@ -2047,18 +2059,24 @@ static void pci_add_saved_cap(struct pci_dev *pci_dev,
 }
 
 /**
- * pci_add_cap_save_buffer - allocate buffer for saving given capability registers
+ * _pci_add_cap_save_buffer - allocate buffer for saving given
+ *                            capability registers
  * @dev: the PCI device
  * @cap: the capability to allocate the buffer for
+ * @extended: Standard or Extended capability ID
  * @size: requested size of the buffer
  */
-static int pci_add_cap_save_buffer(
-	struct pci_dev *dev, char cap, unsigned int size)
+static int _pci_add_cap_save_buffer(struct pci_dev *dev, u16 cap,
+				    bool extended, unsigned int size)
 {
 	int pos;
 	struct pci_cap_saved_state *save_state;
 
-	pos = pci_find_capability(dev, cap);
+	if (extended)
+		pos = pci_find_ext_capability(dev, cap);
+	else
+		pos = pci_find_capability(dev, cap);
+
 	if (pos <= 0)
 		return 0;
 
@@ -2067,12 +2085,25 @@ static int pci_add_cap_save_buffer(
 		return -ENOMEM;
 
 	save_state->cap.cap_nr = cap;
+	save_state->cap.cap_extended = extended;
 	save_state->cap.size = size;
 	pci_add_saved_cap(dev, save_state);
 
 	return 0;
 }
 
+static int pci_add_cap_save_buffer(struct pci_dev *dev,
+				   char cap, unsigned int size)
+{
+	return _pci_add_cap_save_buffer(dev, cap, false, size);
+}
+
+static int pci_add_ext_cap_save_buffer(struct pci_dev *dev,
+				       u16 cap, unsigned int size)
+{
+	return _pci_add_cap_save_buffer(dev, cap, true, size);
+}
+
 /**
  * pci_allocate_cap_save_buffers - allocate buffers for saving capabilities
  * @dev: the PCI device
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 1084a15..adcc948 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -224,7 +224,8 @@ enum pci_bus_speed {
 };
 
 struct pci_cap_saved_data {
-	char cap_nr;
+	u16 cap_nr;
+	bool cap_extended;
 	unsigned int size;
 	u32 data[0];
 };

[PATCH 3/3] pci: Add Virtual Channel to save/restore support

[Alex Williamson]

While we don't really have any infrastructure for making use of VC
support, the system BIOS can configure the topology to non-default
VC values prior to boot.  This may be due to silicon bugs, desire to
reserve traffic classes, or perhaps just BIOS bugs.  When we reset
devices, the VC configuration may return to default values, which can
be incompatible with devices upstream.  For instance, Nvidia GRID
cards provide a PCIe switch and some number of GPUs, all supporting
VC.  The power-on default for VC is to support TC0-7 across VC0,
however some platforms will only enable TC0/VC0 mapping across the
topology.  When we do a secondary bus reset on the downstream switch
port, the GPU is reset to a TC0-7/VC0 mapping while the opposite end
of the link only enables TC0/VC0.  If the GPU attempts to use TC1-7,
it fails.

This patch attempts to provide complete support for VC save/restore,
even beyond the minimally required use case above.  This includes
save/restore and reload of the arbitration table, save/restore and
reload of the port arbitration tables, and re-enabling of the
channels for VC, VC9, and MFVC capabilities.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 drivers/pci/pci.c |  387 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 387 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index a898e4e..3a1d060 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -969,6 +969,367 @@ static void pci_restore_pcix_state(struct pci_dev *dev)
 	pci_write_config_word(dev, pos + PCI_X_CMD, cap[i++]);
 }
 
+/**
+ * pci_vc_save_restore_dwords - Save or restore a series of dwords
+ * @dev: device
+ * @pos: starting config space position
+ * @buf: buffer to save to or restore from
+ * @dwords: number of dwords to save/restore
+ * @save: whether to save or restore
+ */
+static void pci_vc_save_restore_dwords(struct pci_dev *dev, int pos,
+				       u32 *buf, int dwords, bool save)
+{
+	int i;
+
+	for (i = 0; i < dwords; i++, buf++) {
+		if (save)
+			pci_read_config_dword(dev, pos + (i * 4), buf);
+		else
+			pci_write_config_dword(dev, pos + (i * 4), *buf);
+	}
+}
+
+/**
+ * pci_vc_load_port_arb_table - load and wait for VC arbitration table
+ * @dev: device
+ * @pos: starting position of VC capability (VC/VC9/MFVC)
+ *
+ * Signal a VC arbitration table to load and wait for completion.
+ */
+static void pci_vc_load_arb_table(struct pci_dev *dev, int pos)
+{
+	u32 ctrl;
+
+	pci_read_config_dword(dev, pos + PCI_VC_PORT_CTRL, &ctrl);
+	pci_write_config_dword(dev, pos + PCI_VC_PORT_CTRL, ctrl | 1);
+	if (pci_wait_for_pending(dev, pos + PCI_VC_PORT_STATUS, 1))
+		return;
+
+	dev_err(&dev->dev, "VC arbitration table failed to load\n");
+}
+
+/**
+ * pci_vc_load_port_arb_table - Load and wait for VC port arbitration table
+ * @dev: device
+ * @pos: starting position of VC capability (VC/VC9/MFVC)
+ * @res: VC res number, ie. VCn (0-7)
+ *
+ * Signal a VC port arbitration table to load and wait for completion.
+ */
+static void pci_vc_load_port_arb_table(struct pci_dev *dev, int pos, int res)
+{
+	int ctrl_pos, status_pos;
+	u32 ctrl;
+
+	ctrl_pos = pos + PCI_VC_RES_CTRL + (res * PCI_CAP_VC_PER_VC_SIZEOF);
+	status_pos = pos + PCI_VC_RES_STATUS + (res * PCI_CAP_VC_PER_VC_SIZEOF);
+
+	pci_read_config_dword(dev, ctrl_pos, &ctrl);
+	pci_write_config_dword(dev, ctrl_pos, ctrl | (1 << 16));
+
+	if (pci_wait_for_pending(dev, status_pos, 1))
+		return;
+
+	dev_err(&dev->dev, "VC%d port arbitration table failed to load\n", res);
+}
+
+/**
+ * pci_vc_enable - Enable virtual channel
+ * @dev: device
+ * @pos: starting position of VC capability (VC/VC9/MFVC)
+ * @res: VC res number, ie. VCn (0-7)
+ *
+ * A VC is enabled by setting the enable bit in matching resource control
+ * registers on both sides of a link.  We therefore need to find the opposite
+ * end of the link.  To keep this simple we enable from the downstream device.
+ * RC devices do not have an upstream device, nor does it seem that VC9 do
+ * (spec is unclear).  Once we find the upstream device, match the VC ID to
+ * get the correct resource, disable and enable on both ends.
+ */
+static void pci_vc_enable(struct pci_dev *dev, int pos, int res)
+{
+	int ctrl_pos, status_pos, id, pos2, evcc, i, ctrl_pos2, status_pos2;
+	u32 ctrl, header, reg1, ctrl2;
+	struct pci_dev *link = NULL;
+
+	/* Enable VCs from the downstream device */
+	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
+	    pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM)
+		return;
+
+	ctrl_pos = pos + PCI_VC_RES_CTRL + (res * PCI_CAP_VC_PER_VC_SIZEOF);
+	status_pos = pos + PCI_VC_RES_STATUS + (res * PCI_CAP_VC_PER_VC_SIZEOF);
+
+	pci_read_config_dword(dev, ctrl_pos, &ctrl);
+	id = (ctrl >> 24) & 0x7;
+
+	pci_read_config_dword(dev, pos, &header);
+
+	/* If there is no opposite end of the link, skip to enable */
+	if (PCI_EXT_CAP_ID(header) == PCI_EXT_CAP_ID_VC9 ||
+	    pci_is_root_bus(dev->bus))
+		goto enable;
+
+	pos2 = pci_find_ext_capability(dev->bus->self, PCI_EXT_CAP_ID_VC);
+	if (pos2 <= 0)
+		goto enable;
+
+	pci_read_config_dword(dev->bus->self, pos2 + PCI_VC_PORT_REG1, &reg1);
+	evcc = reg1 & PCI_VC_REG1_EVCC;
+
+	/* VC0 is hardwired enabled, so we can start with 1 */
+	for (i = 1; i < evcc + 1; i++) {
+		ctrl_pos2 = pos2 + PCI_VC_RES_CTRL +
+				(i * PCI_CAP_VC_PER_VC_SIZEOF);
+		status_pos2 = pos2 + PCI_VC_RES_STATUS +
+				(i * PCI_CAP_VC_PER_VC_SIZEOF);
+		pci_read_config_dword(dev->bus->self, ctrl_pos2, &ctrl2);
+		if (((ctrl2 >> 24) & 0x7) == id) {
+			link = dev->bus->self;
+			break;
+		}
+	}
+
+	if (!link)
+		goto enable;
+
+	/* Disable if enabled */
+	if (ctrl2 & (1U << 31)) {
+		ctrl2 &= ~(1U << 31);
+		pci_write_config_dword(link, ctrl_pos2, ctrl2);
+	}
+
+	/* Enable on both ends */
+	ctrl2 |= 1U << 31;
+	pci_write_config_dword(link, ctrl_pos2, ctrl2);
+enable:
+	ctrl |= 1U << 31;
+	pci_write_config_dword(dev, ctrl_pos, ctrl);
+
+	if (!pci_wait_for_pending(dev, status_pos, 0x2))
+		dev_err(&dev->dev, "VC%d negotiation stuck pending\n", id);
+
+	if (link && !pci_wait_for_pending(link, status_pos2, 0x2))
+		dev_err(&link->dev, "VC%d negotiation stuck pending\n", id);
+}
+
+/**
+ * pci_vc_do_save_buffer - Size, save, or restore VC state
+ * @dev: device
+ * @pos: starting position of VC capability (VC/VC9/MFVC)
+ * @save_state: buffer for save/restore
+ * @name: for error message
+ * @save: if provided a buffer, this indicates what to do with it
+ *
+ * Walking Virtual Channel config space to size, save, or restore it
+ * is complicated, so we do it all from one function to reduce code and
+ * guarantee ordering matches in the buffer.  When called with NULL
+ * @save_state, return the size of the necessary save buffer.  When called
+ * with a non-NULL @save_state, @save determines whether we save to the
+ * buffer or restore from it.
+ */
+static int pci_vc_do_save_buffer(struct pci_dev *dev, int pos,
+				 struct pci_cap_saved_state *save_state,
+				 bool save)
+{
+	u32 reg1;
+	char evcc, lpevcc, parb_size;
+	int i, len = 0;
+	u32 *buf = save_state ? save_state->cap.data : NULL;
+
+	/* Sanity check buffer size for save/restore */
+	if (buf && save_state->cap.size !=
+	    pci_vc_do_save_buffer(dev, pos, NULL, save)) {
+		dev_err(&dev->dev,
+			"VC save buffer size does not match @0x%x\n", pos);
+		return -ENOMEM;
+	}
+
+	pci_read_config_dword(dev, pos + PCI_VC_PORT_REG1, &reg1);
+	/* Extended VC Count (not counting VC0) */
+	evcc = reg1 & PCI_VC_REG1_EVCC;
+	/* Low Priority Extended VC Count (not counting VC0) */
+	lpevcc = (reg1 >> 4) & PCI_VC_REG1_EVCC;
+	/* Port Arbitration Table Entry Size (bits) */
+	parb_size = 1 << ((reg1 >> 10) & 0x3);
+
+	/*
+	 * Port VC Control Register contains VC Arbitration Select, which
+	 * cannot be modified when more than one LPVC is in operation.  We
+	 * therefore save/restore it first, as only VC0 should be enabled
+	 * after device reset.
+	 */
+	if (buf) {
+		pci_vc_save_restore_dwords(dev, pos + PCI_VC_PORT_CTRL,
+					   buf, 1, save);
+		buf++;
+	}
+	len += 4;
+
+	/*
+	 * If we have any Low Priority VCs and a VC Arbitration Table Offset
+	 * in Port VC Capability Register 2 then save/restore it next.
+	 */
+	if (lpevcc) {
+		u32 reg2;
+		int vcarb_offset;
+
+		pci_read_config_dword(dev, pos + PCI_VC_PORT_REG2, &reg2);
+		vcarb_offset = (reg2 >> 24) * 16;
+
+		if (vcarb_offset) {
+			int size, vcarb_phases = 0;
+
+			if (reg2 & PCI_VC_REG2_128_PHASE)
+				vcarb_phases = 128;
+			else if (reg2 & PCI_VC_REG2_64_PHASE)
+				vcarb_phases = 64;
+			else if (reg2 & PCI_VC_REG2_32_PHASE)
+				vcarb_phases = 32;
+
+			/* Fixed 4 bits per phase per lpevcc (plus VC0) */
+			size = ((lpevcc + 1) * vcarb_phases * 4) / 8;
+
+			if (size && buf) {
+				pci_vc_save_restore_dwords(dev,
+							   pos + vcarb_offset,
+							   buf, size / 4, save);
+				/*
+				 * On restore, we need to signal hardware to
+				 * re-load the VC Arbitration Table.
+				 */
+				if (!save)
+					pci_vc_load_arb_table(dev, pos);
+
+				buf += size / 4;
+			}
+			len += size;
+		}
+	}
+
+	/*
+	 * In addition to each VC Resource Control Register, we may have a
+	 * Port Arbitration Table attached to each VC.  The Port Arbitration
+	 * Table Offset in each VC Resource Capability Register tells us if
+	 * it exists.  The entry size is global from the Port VC Capability
+	 * Register1 above.  The number of phases is determined per VC.
+	 */
+	for (i = 0; i < evcc + 1; i++) {
+		u32 cap;
+		int parb_offset;
+
+		pci_read_config_dword(dev, pos + PCI_VC_RES_CAP +
+				      (i * PCI_CAP_VC_PER_VC_SIZEOF), &cap);
+		parb_offset = (cap >> 24) * 16;
+		if (parb_offset) {
+			int size, parb_phases = 0;
+
+			if (cap & 0x20)
+				parb_phases = 256;
+			else if (cap & 0x18)
+				parb_phases = 128;
+			else if (cap & 0x4)
+				parb_phases = 64;
+			else if (cap & 0x2)
+				parb_phases = 32;
+
+			size = (parb_size * parb_phases) / 8;
+
+			if (size && buf) {
+				pci_vc_save_restore_dwords(dev,
+							   pos + parb_offset,
+							   buf, size / 4, save);
+				buf += size / 4;
+			}
+			len += size;
+		}
+
+		/* VC Resource Control Register */
+		if (buf) {
+			int ctrl_pos = pos + PCI_VC_RES_CTRL +
+						(i * PCI_CAP_VC_PER_VC_SIZEOF);
+			if (save)
+				pci_read_config_dword(dev, ctrl_pos, buf);
+			else {
+				u32 tmp, ctrl = *(u32 *)buf;
+				/*
+				 * For an FLR case, the VC config may remain.
+				 * Preserve enable bit, restore the rest.
+				 */
+				pci_read_config_dword(dev, ctrl_pos, &tmp);
+				tmp &= 1U << 31;
+				tmp |= ctrl & ~(1U << 31);
+				pci_write_config_dword(dev, ctrl_pos, tmp);
+				/* Load port arbitration table if used */
+				if (ctrl & (7 << 17))
+					pci_vc_load_port_arb_table(dev, pos, i);
+				/* Re-enable if needed */
+				if ((ctrl ^ tmp) & (1U << 31))
+					pci_vc_enable(dev, pos, i);
+			}
+			buf++;
+		}
+		len += 4;
+	}
+
+	return buf ? 0 : len;
+}
+
+static struct {
+	u16 id;
+	const char *name;
+} vc_caps[] = { { PCI_EXT_CAP_ID_MFVC, "MFVC" },
+		{ PCI_EXT_CAP_ID_VC, "VC" },
+		{ PCI_EXT_CAP_ID_VC9, "VC9" } };
+
+static int pci_save_vc_state(struct pci_dev *dev)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(vc_caps); i++) {
+		int pos, ret;
+		struct pci_cap_saved_state *save_state;
+
+		pos = pci_find_ext_capability(dev, vc_caps[i].id);
+		if (pos <= 0)
+			continue;
+
+		save_state = pci_find_saved_ext_cap(dev, vc_caps[i].id);
+		if (!save_state) {
+			dev_err(&dev->dev, "%s buffer not found in %s\n",
+				vc_caps[i].name, __func__);
+			return -ENOMEM;
+		}
+
+		ret = pci_vc_do_save_buffer(dev, pos, save_state, true);
+		if (ret) {
+			dev_err(&dev->dev, "%s save unsuccessful %s\n",
+				vc_caps[i].name, __func__);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static void pci_restore_vc_state(struct pci_dev *dev)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(vc_caps); i++) {
+		int pos;
+		struct pci_cap_saved_state *save_state;
+
+		pos = pci_find_ext_capability(dev, vc_caps[i].id);
+		save_state = pci_find_saved_ext_cap(dev, vc_caps[i].id);
+		if (!save_state || pos <= 0)
+			continue;
+
+		pci_vc_do_save_buffer(dev, pos, save_state, false);
+	}
+}
+
 
 /**
  * pci_save_state - save the PCI configuration space of a device before suspending
@@ -986,6 +1347,8 @@ pci_save_state(struct pci_dev *dev)
 		return i;
 	if ((i = pci_save_pcix_state(dev)) != 0)
 		return i;
+	if ((i = pci_save_vc_state(dev)) != 0)
+		return i;
 	return 0;
 }
 
@@ -1048,6 +1411,7 @@ void pci_restore_state(struct pci_dev *dev)
 	/* PCI Express register must be restored first */
 	pci_restore_pcie_state(dev);
 	pci_restore_ats_state(dev);
+	pci_restore_vc_state(dev);
 
 	pci_restore_config_space(dev);
 
@@ -2104,6 +2468,27 @@ static int pci_add_ext_cap_save_buffer(struct pci_dev *dev,
 	return _pci_add_cap_save_buffer(dev, cap, true, size);
 }
 
+static void pci_allocate_vc_save_buffers(struct pci_dev *dev)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(vc_caps); i++) {
+		int error, pos = pci_find_ext_capability(dev, vc_caps[i].id);
+		int len;
+
+		if (pos <= 0)
+			continue;
+
+		len = pci_vc_do_save_buffer(dev, pos, NULL, false);
+		error = pci_add_ext_cap_save_buffer(dev,
+						    vc_caps[i].id, len);
+		if (error)
+			dev_err(&dev->dev,
+				"unable to preallocate %s save buffer\n",
+				vc_caps[i].name);
+	}
+}
+
 /**
  * pci_allocate_cap_save_buffers - allocate buffers for saving capabilities
  * @dev: the PCI device
@@ -2122,6 +2507,8 @@ void pci_allocate_cap_save_buffers(struct pci_dev *dev)
 	if (error)
 		dev_err(&dev->dev,
 			"unable to preallocate PCI-X save buffer\n");
+
+	pci_allocate_vc_save_buffers(dev);
 }
 
 void pci_free_cap_save_buffers(struct pci_dev *dev)

Re: [PATCH 3/3] pci: Add Virtual Channel to save/restore support

[Bjorn Helgaas]

On Tue, Dec 10, 2013 at 11:48:45AM -0700, Alex Williamson wrote:
> While we don't really have any infrastructure for making use of VC
> support, the system BIOS can configure the topology to non-default
> VC values prior to boot.  This may be due to silicon bugs, desire to
> reserve traffic classes, or perhaps just BIOS bugs.  When we reset
> devices, the VC configuration may return to default values, which can
> be incompatible with devices upstream.  For instance, Nvidia GRID
> cards provide a PCIe switch and some number of GPUs, all supporting
> VC.  The power-on default for VC is to support TC0-7 across VC0,
> however some platforms will only enable TC0/VC0 mapping across the
> topology.  When we do a secondary bus reset on the downstream switch
> port, the GPU is reset to a TC0-7/VC0 mapping while the opposite end
> of the link only enables TC0/VC0.  If the GPU attempts to use TC1-7,
> it fails.
> 
> This patch attempts to provide complete support for VC save/restore,
> even beyond the minimally required use case above.  This includes
> save/restore and reload of the arbitration table, save/restore and
> reload of the port arbitration tables, and re-enabling of the
> channels for VC, VC9, and MFVC capabilities.
> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
>  drivers/pci/pci.c |  387 +++++++++++++++++++++++++++++++++++++++++++++++++++++

Wow, that's a lot of code just to support resetting a device.  I wish I had
a better idea, but I don't.  I know we have similar save/restore code in
pci.c already, but would it make sense to put this in a vc.c to keep pci.c
from growing without bound?

>  1 file changed, 387 insertions(+)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index a898e4e..3a1d060 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -969,6 +969,367 @@ static void pci_restore_pcix_state(struct pci_dev *dev)
>  	pci_write_config_word(dev, pos + PCI_X_CMD, cap[i++]);
>  }
>  
> +/**
> + * pci_vc_save_restore_dwords - Save or restore a series of dwords
> + * @dev: device
> + * @pos: starting config space position
> + * @buf: buffer to save to or restore from
> + * @dwords: number of dwords to save/restore
> + * @save: whether to save or restore
> + */
> +static void pci_vc_save_restore_dwords(struct pci_dev *dev, int pos,
> +				       u32 *buf, int dwords, bool save)

Nothing VC-specific here, so maybe remove "_vc_" from the name.

> +{
> +	int i;
> +
> +	for (i = 0; i < dwords; i++, buf++) {
> +		if (save)
> +			pci_read_config_dword(dev, pos + (i * 4), buf);
> +		else
> +			pci_write_config_dword(dev, pos + (i * 4), *buf);
> +	}
> +}
> +
> +/**
> + * pci_vc_load_port_arb_table - load and wait for VC arbitration table
> + * @dev: device
> + * @pos: starting position of VC capability (VC/VC9/MFVC)
> + *
> + * Signal a VC arbitration table to load and wait for completion.
> + */
> +static void pci_vc_load_arb_table(struct pci_dev *dev, int pos)
> +{
> +	u32 ctrl;
> +
> +	pci_read_config_dword(dev, pos + PCI_VC_PORT_CTRL, &ctrl);
> +	pci_write_config_dword(dev, pos + PCI_VC_PORT_CTRL, ctrl | 1);
> +	if (pci_wait_for_pending(dev, pos + PCI_VC_PORT_STATUS, 1))

Comment doesn't match function name.  The spec "Request hardware to apply
new values" language would be a useful clue that this doesn't actually
*write* the table; before reading the spec, I initially looked for a
buffer containing the arbitration table.  It'd be nice to have #defines
for the bits here, i.e., PCI_VC_PORT_CTRL_LOAD_ARB or similar.  The
spec says these are 16-bit registers; shouldn't these be "word" accesses?

> +		return;
> +
> +	dev_err(&dev->dev, "VC arbitration table failed to load\n");
> +}
> +
> +/**
> + * pci_vc_load_port_arb_table - Load and wait for VC port arbitration table
> + * @dev: device
> + * @pos: starting position of VC capability (VC/VC9/MFVC)
> + * @res: VC res number, ie. VCn (0-7)
> + *
> + * Signal a VC port arbitration table to load and wait for completion.
> + */
> +static void pci_vc_load_port_arb_table(struct pci_dev *dev, int pos, int res)
> +{
> +	int ctrl_pos, status_pos;
> +	u32 ctrl;
> +
> +	ctrl_pos = pos + PCI_VC_RES_CTRL + (res * PCI_CAP_VC_PER_VC_SIZEOF);
> +	status_pos = pos + PCI_VC_RES_STATUS + (res * PCI_CAP_VC_PER_VC_SIZEOF);
> +
> +	pci_read_config_dword(dev, ctrl_pos, &ctrl);
> +	pci_write_config_dword(dev, ctrl_pos, ctrl | (1 << 16));
> +
> +	if (pci_wait_for_pending(dev, status_pos, 1))

#defines, please, to help grep users later.

> +		return;
> +
> +	dev_err(&dev->dev, "VC%d port arbitration table failed to load\n", res);
> +}
> +
> +/**
> + * pci_vc_enable - Enable virtual channel
> + * @dev: device
> + * @pos: starting position of VC capability (VC/VC9/MFVC)
> + * @res: VC res number, ie. VCn (0-7)
> + *
> + * A VC is enabled by setting the enable bit in matching resource control
> + * registers on both sides of a link.  We therefore need to find the opposite
> + * end of the link.  To keep this simple we enable from the downstream device.
> + * RC devices do not have an upstream device, nor does it seem that VC9 do
> + * (spec is unclear).  Once we find the upstream device, match the VC ID to
> + * get the correct resource, disable and enable on both ends.
> + */
> +static void pci_vc_enable(struct pci_dev *dev, int pos, int res)
> +{
> +	int ctrl_pos, status_pos, id, pos2, evcc, i, ctrl_pos2, status_pos2;
> +	u32 ctrl, header, reg1, ctrl2;
> +	struct pci_dev *link = NULL;
> +
> +	/* Enable VCs from the downstream device */
> +	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
> +	    pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM)
> +		return;
> +
> +	ctrl_pos = pos + PCI_VC_RES_CTRL + (res * PCI_CAP_VC_PER_VC_SIZEOF);
> +	status_pos = pos + PCI_VC_RES_STATUS + (res * PCI_CAP_VC_PER_VC_SIZEOF);
> +
> +	pci_read_config_dword(dev, ctrl_pos, &ctrl);
> +	id = (ctrl >> 24) & 0x7;

#define PCI_VC_RES_CTRL_ID 0x07000000.

It gets a little cumbersome to have #defines for *both* the mask and the
shift; the compromise I like is to have a #define for the mask (which shows
the position in the register) and use bare numbers when we need to shift.
Then it's easy to find uses of the field with grep or cscope, and the mask
definition helps manual decoding.  There are several other opportunities
for mask #defines below.

In this case, you only compare ID values, so there's actually no need to
shift it.

> +
> +	pci_read_config_dword(dev, pos, &header);
> +
> +	/* If there is no opposite end of the link, skip to enable */
> +	if (PCI_EXT_CAP_ID(header) == PCI_EXT_CAP_ID_VC9 ||
> +	    pci_is_root_bus(dev->bus))
> +		goto enable;
> +
> +	pos2 = pci_find_ext_capability(dev->bus->self, PCI_EXT_CAP_ID_VC);
> +	if (pos2 <= 0)
> +		goto enable;

I don't think < 0 is possible (several other occurrences below, too).

> +
> +	pci_read_config_dword(dev->bus->self, pos2 + PCI_VC_PORT_REG1, &reg1);
> +	evcc = reg1 & PCI_VC_REG1_EVCC;
> +
> +	/* VC0 is hardwired enabled, so we can start with 1 */
> +	for (i = 1; i < evcc + 1; i++) {
> +		ctrl_pos2 = pos2 + PCI_VC_RES_CTRL +
> +				(i * PCI_CAP_VC_PER_VC_SIZEOF);
> +		status_pos2 = pos2 + PCI_VC_RES_STATUS +
> +				(i * PCI_CAP_VC_PER_VC_SIZEOF);
> +		pci_read_config_dword(dev->bus->self, ctrl_pos2, &ctrl2);
> +		if (((ctrl2 >> 24) & 0x7) == id) {
> +			link = dev->bus->self;
> +			break;
> +		}
> +	}
> +
> +	if (!link)
> +		goto enable;
> +
> +	/* Disable if enabled */
> +	if (ctrl2 & (1U << 31)) {
> +		ctrl2 &= ~(1U << 31);
> +		pci_write_config_dword(link, ctrl_pos2, ctrl2);
> +	}
> +
> +	/* Enable on both ends */
> +	ctrl2 |= 1U << 31;
> +	pci_write_config_dword(link, ctrl_pos2, ctrl2);
> +enable:
> +	ctrl |= 1U << 31;
> +	pci_write_config_dword(dev, ctrl_pos, ctrl);
> +
> +	if (!pci_wait_for_pending(dev, status_pos, 0x2))
> +		dev_err(&dev->dev, "VC%d negotiation stuck pending\n", id);
> +
> +	if (link && !pci_wait_for_pending(link, status_pos2, 0x2))
> +		dev_err(&link->dev, "VC%d negotiation stuck pending\n", id);
> +}
> +
> +/**
> + * pci_vc_do_save_buffer - Size, save, or restore VC state
> + * @dev: device
> + * @pos: starting position of VC capability (VC/VC9/MFVC)
> + * @save_state: buffer for save/restore
> + * @name: for error message
> + * @save: if provided a buffer, this indicates what to do with it
> + *
> + * Walking Virtual Channel config space to size, save, or restore it
> + * is complicated, so we do it all from one function to reduce code and
> + * guarantee ordering matches in the buffer.  When called with NULL
> + * @save_state, return the size of the necessary save buffer.  When called
> + * with a non-NULL @save_state, @save determines whether we save to the
> + * buffer or restore from it.
> + */
> +static int pci_vc_do_save_buffer(struct pci_dev *dev, int pos,
> +				 struct pci_cap_saved_state *save_state,
> +				 bool save)
> +{
> +	u32 reg1;
> +	char evcc, lpevcc, parb_size;
> +	int i, len = 0;
> +	u32 *buf = save_state ? save_state->cap.data : NULL;
> +
> +	/* Sanity check buffer size for save/restore */
> +	if (buf && save_state->cap.size !=
> +	    pci_vc_do_save_buffer(dev, pos, NULL, save)) {
> +		dev_err(&dev->dev,
> +			"VC save buffer size does not match @0x%x\n", pos);
> +		return -ENOMEM;
> +	}
> +
> +	pci_read_config_dword(dev, pos + PCI_VC_PORT_REG1, &reg1);
> +	/* Extended VC Count (not counting VC0) */
> +	evcc = reg1 & PCI_VC_REG1_EVCC;
> +	/* Low Priority Extended VC Count (not counting VC0) */
> +	lpevcc = (reg1 >> 4) & PCI_VC_REG1_EVCC;

Please make a new #define, since LPEVCC is a separate field.

> +	/* Port Arbitration Table Entry Size (bits) */
> +	parb_size = 1 << ((reg1 >> 10) & 0x3);
> +
> +	/*
> +	 * Port VC Control Register contains VC Arbitration Select, which
> +	 * cannot be modified when more than one LPVC is in operation.  We
> +	 * therefore save/restore it first, as only VC0 should be enabled
> +	 * after device reset.
> +	 */
> +	if (buf) {
> +		pci_vc_save_restore_dwords(dev, pos + PCI_VC_PORT_CTRL,
> +					   buf, 1, save);
> +		buf++;
> +	}
> +	len += 4;
> +
> +	/*
> +	 * If we have any Low Priority VCs and a VC Arbitration Table Offset
> +	 * in Port VC Capability Register 2 then save/restore it next.
> +	 */
> +	if (lpevcc) {
> +		u32 reg2;
> +		int vcarb_offset;
> +
> +		pci_read_config_dword(dev, pos + PCI_VC_PORT_REG2, &reg2);
> +		vcarb_offset = (reg2 >> 24) * 16;
> +
> +		if (vcarb_offset) {
> +			int size, vcarb_phases = 0;
> +
> +			if (reg2 & PCI_VC_REG2_128_PHASE)
> +				vcarb_phases = 128;
> +			else if (reg2 & PCI_VC_REG2_64_PHASE)
> +				vcarb_phases = 64;
> +			else if (reg2 & PCI_VC_REG2_32_PHASE)
> +				vcarb_phases = 32;
> +
> +			/* Fixed 4 bits per phase per lpevcc (plus VC0) */
> +			size = ((lpevcc + 1) * vcarb_phases * 4) / 8;
> +
> +			if (size && buf) {
> +				pci_vc_save_restore_dwords(dev,
> +							   pos + vcarb_offset,
> +							   buf, size / 4, save);
> +				/*
> +				 * On restore, we need to signal hardware to
> +				 * re-load the VC Arbitration Table.
> +				 */
> +				if (!save)
> +					pci_vc_load_arb_table(dev, pos);
> +
> +				buf += size / 4;
> +			}
> +			len += size;
> +		}
> +	}
> +
> +	/*
> +	 * In addition to each VC Resource Control Register, we may have a
> +	 * Port Arbitration Table attached to each VC.  The Port Arbitration
> +	 * Table Offset in each VC Resource Capability Register tells us if
> +	 * it exists.  The entry size is global from the Port VC Capability
> +	 * Register1 above.  The number of phases is determined per VC.
> +	 */
> +	for (i = 0; i < evcc + 1; i++) {
> +		u32 cap;
> +		int parb_offset;
> +
> +		pci_read_config_dword(dev, pos + PCI_VC_RES_CAP +
> +				      (i * PCI_CAP_VC_PER_VC_SIZEOF), &cap);
> +		parb_offset = (cap >> 24) * 16;
> +		if (parb_offset) {
> +			int size, parb_phases = 0;
> +
> +			if (cap & 0x20)
> +				parb_phases = 256;
> +			else if (cap & 0x18)
> +				parb_phases = 128;
> +			else if (cap & 0x4)
> +				parb_phases = 64;
> +			else if (cap & 0x2)
> +				parb_phases = 32;
> +
> +			size = (parb_size * parb_phases) / 8;
> +
> +			if (size && buf) {
> +				pci_vc_save_restore_dwords(dev,
> +							   pos + parb_offset,
> +							   buf, size / 4, save);
> +				buf += size / 4;
> +			}
> +			len += size;
> +		}
> +
> +		/* VC Resource Control Register */
> +		if (buf) {
> +			int ctrl_pos = pos + PCI_VC_RES_CTRL +
> +						(i * PCI_CAP_VC_PER_VC_SIZEOF);
> +			if (save)
> +				pci_read_config_dword(dev, ctrl_pos, buf);
> +			else {
> +				u32 tmp, ctrl = *(u32 *)buf;
> +				/*
> +				 * For an FLR case, the VC config may remain.
> +				 * Preserve enable bit, restore the rest.
> +				 */
> +				pci_read_config_dword(dev, ctrl_pos, &tmp);
> +				tmp &= 1U << 31;
> +				tmp |= ctrl & ~(1U << 31);
> +				pci_write_config_dword(dev, ctrl_pos, tmp);
> +				/* Load port arbitration table if used */
> +				if (ctrl & (7 << 17))
> +					pci_vc_load_port_arb_table(dev, pos, i);
> +				/* Re-enable if needed */
> +				if ((ctrl ^ tmp) & (1U << 31))
> +					pci_vc_enable(dev, pos, i);
> +			}
> +			buf++;
> +		}
> +		len += 4;
> +	}
> +
> +	return buf ? 0 : len;
> +}
> +
> +static struct {
> +	u16 id;
> +	const char *name;
> +} vc_caps[] = { { PCI_EXT_CAP_ID_MFVC, "MFVC" },
> +		{ PCI_EXT_CAP_ID_VC, "VC" },
> +		{ PCI_EXT_CAP_ID_VC9, "VC9" } };
> +
> +static int pci_save_vc_state(struct pci_dev *dev)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(vc_caps); i++) {
> +		int pos, ret;
> +		struct pci_cap_saved_state *save_state;
> +
> +		pos = pci_find_ext_capability(dev, vc_caps[i].id);
> +		if (pos <= 0)
> +			continue;
> +
> +		save_state = pci_find_saved_ext_cap(dev, vc_caps[i].id);
> +		if (!save_state) {
> +			dev_err(&dev->dev, "%s buffer not found in %s\n",
> +				vc_caps[i].name, __func__);
> +			return -ENOMEM;
> +		}
> +
> +		ret = pci_vc_do_save_buffer(dev, pos, save_state, true);
> +		if (ret) {
> +			dev_err(&dev->dev, "%s save unsuccessful %s\n",
> +				vc_caps[i].name, __func__);
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static void pci_restore_vc_state(struct pci_dev *dev)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(vc_caps); i++) {
> +		int pos;
> +		struct pci_cap_saved_state *save_state;
> +
> +		pos = pci_find_ext_capability(dev, vc_caps[i].id);
> +		save_state = pci_find_saved_ext_cap(dev, vc_caps[i].id);
> +		if (!save_state || pos <= 0)
> +			continue;
> +
> +		pci_vc_do_save_buffer(dev, pos, save_state, false);
> +	}
> +}
> +
>  
>  /**
>   * pci_save_state - save the PCI configuration space of a device before suspending
> @@ -986,6 +1347,8 @@ pci_save_state(struct pci_dev *dev)
>  		return i;
>  	if ((i = pci_save_pcix_state(dev)) != 0)
>  		return i;
> +	if ((i = pci_save_vc_state(dev)) != 0)
> +		return i;
>  	return 0;
>  }
>  
> @@ -1048,6 +1411,7 @@ void pci_restore_state(struct pci_dev *dev)
>  	/* PCI Express register must be restored first */
>  	pci_restore_pcie_state(dev);
>  	pci_restore_ats_state(dev);
> +	pci_restore_vc_state(dev);
>  
>  	pci_restore_config_space(dev);
>  
> @@ -2104,6 +2468,27 @@ static int pci_add_ext_cap_save_buffer(struct pci_dev *dev,
>  	return _pci_add_cap_save_buffer(dev, cap, true, size);
>  }
>  
> +static void pci_allocate_vc_save_buffers(struct pci_dev *dev)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(vc_caps); i++) {
> +		int error, pos = pci_find_ext_capability(dev, vc_caps[i].id);
> +		int len;
> +
> +		if (pos <= 0)
> +			continue;
> +
> +		len = pci_vc_do_save_buffer(dev, pos, NULL, false);
> +		error = pci_add_ext_cap_save_buffer(dev,
> +						    vc_caps[i].id, len);
> +		if (error)
> +			dev_err(&dev->dev,
> +				"unable to preallocate %s save buffer\n",
> +				vc_caps[i].name);
> +	}
> +}
> +
>  /**
>   * pci_allocate_cap_save_buffers - allocate buffers for saving capabilities
>   * @dev: the PCI device
> @@ -2122,6 +2507,8 @@ void pci_allocate_cap_save_buffers(struct pci_dev *dev)
>  	if (error)
>  		dev_err(&dev->dev,
>  			"unable to preallocate PCI-X save buffer\n");
> +
> +	pci_allocate_vc_save_buffers(dev);
>  }
>  
>  void pci_free_cap_save_buffers(struct pci_dev *dev)
> 

Re: [PATCH 3/3] pci: Add Virtual Channel to save/restore support

[Bjorn Helgaas]

On Tue, Dec 10, 2013 at 11:48:45AM -0700, Alex Williamson wrote:
>...
> +	pci_read_config_dword(dev->bus->self, pos2 + PCI_VC_PORT_REG1, &reg1);
> +	evcc = reg1 & PCI_VC_REG1_EVCC;

I think PCI_VC_PORT_REG1 and PCI_VC_PORT_REG2 are mis-named and should be
changed to CAP1 and CAP2 or similar.  Almost everything here is a
"register."

Bjorn

Re: [PATCH 3/3] pci: Add Virtual Channel to save/restore support

[Alex Williamson]

On Tue, 2013-12-17 at 11:03 -0700, Bjorn Helgaas wrote:
> On Tue, Dec 10, 2013 at 11:48:45AM -0700, Alex Williamson wrote:
> >...
> > +	pci_read_config_dword(dev->bus->self, pos2 + PCI_VC_PORT_REG1, &reg1);
> > +	evcc = reg1 & PCI_VC_REG1_EVCC;
> 
> I think PCI_VC_PORT_REG1 and PCI_VC_PORT_REG2 are mis-named and should be
> changed to CAP1 and CAP2 or similar.  Almost everything here is a
> "register."

That's true, but it matches the name in the spec: "Port VC Capability
Register 1/2".  Thanks,

Alex

Re: [PATCH 3/3] pci: Add Virtual Channel to save/restore support

[Bjorn Helgaas]

On Tue, Dec 17, 2013 at 1:28 PM, Alex Williamson
<alex.williamson@redhat.com> wrote:
> On Tue, 2013-12-17 at 11:03 -0700, Bjorn Helgaas wrote:
>> On Tue, Dec 10, 2013 at 11:48:45AM -0700, Alex Williamson wrote:
>> >...
>> > +   pci_read_config_dword(dev->bus->self, pos2 + PCI_VC_PORT_REG1, &reg1);
>> > +   evcc = reg1 & PCI_VC_REG1_EVCC;
>>
>> I think PCI_VC_PORT_REG1 and PCI_VC_PORT_REG2 are mis-named and should be
>> changed to CAP1 and CAP2 or similar.  Almost everything here is a
>> "register."
>
> That's true, but it matches the name in the spec: "Port VC Capability
> Register 1/2".  Thanks,

Sure, but we use PCI_EXP_DEVCAP for the "Device Capabilities
Register," PCI_EXP_DEVCTL for the "Device Control Register," etc.  All
I'm saying is that "REG" doesn't convey as much information as "CAP."

Bjorn

Re: [PATCH 3/3] pci: Add Virtual Channel to save/restore support

[Alex Williamson]

On Mon, 2013-12-16 at 17:48 -0700, Bjorn Helgaas wrote:
> On Tue, Dec 10, 2013 at 11:48:45AM -0700, Alex Williamson wrote:
> > While we don't really have any infrastructure for making use of VC
> > support, the system BIOS can configure the topology to non-default
> > VC values prior to boot.  This may be due to silicon bugs, desire to
> > reserve traffic classes, or perhaps just BIOS bugs.  When we reset
> > devices, the VC configuration may return to default values, which can
> > be incompatible with devices upstream.  For instance, Nvidia GRID
> > cards provide a PCIe switch and some number of GPUs, all supporting
> > VC.  The power-on default for VC is to support TC0-7 across VC0,
> > however some platforms will only enable TC0/VC0 mapping across the
> > topology.  When we do a secondary bus reset on the downstream switch
> > port, the GPU is reset to a TC0-7/VC0 mapping while the opposite end
> > of the link only enables TC0/VC0.  If the GPU attempts to use TC1-7,
> > it fails.
> > 
> > This patch attempts to provide complete support for VC save/restore,
> > even beyond the minimally required use case above.  This includes
> > save/restore and reload of the arbitration table, save/restore and
> > reload of the port arbitration tables, and re-enabling of the
> > channels for VC, VC9, and MFVC capabilities.
> > 
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > ---
> >  drivers/pci/pci.c |  387 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> 
> Wow, that's a lot of code just to support resetting a device.  I wish I had
> a better idea, but I don't.

I know, me too.  I tried to keep the code as compact as I could.  If you
look at it on a per capability basis, since this supports VC/VC9/MFVC,
it's not so bad ;)

>   I know we have similar save/restore code in
> pci.c already, but would it make sense to put this in a vc.c to keep pci.c
> from growing without bound?

Yep, I can do that.

> >  1 file changed, 387 insertions(+)
> > 
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index a898e4e..3a1d060 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -969,6 +969,367 @@ static void pci_restore_pcix_state(struct pci_dev *dev)
> >  	pci_write_config_word(dev, pos + PCI_X_CMD, cap[i++]);
> >  }
> >  
> > +/**
> > + * pci_vc_save_restore_dwords - Save or restore a series of dwords
> > + * @dev: device
> > + * @pos: starting config space position
> > + * @buf: buffer to save to or restore from
> > + * @dwords: number of dwords to save/restore
> > + * @save: whether to save or restore
> > + */
> > +static void pci_vc_save_restore_dwords(struct pci_dev *dev, int pos,
> > +				       u32 *buf, int dwords, bool save)
> 
> Nothing VC-specific here, so maybe remove "_vc_" from the name.

Sure, but if I make a vc.c there's no reason not to move this function
there so retaining _vc_ avoids polluting the common pci namespace.

> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < dwords; i++, buf++) {
> > +		if (save)
> > +			pci_read_config_dword(dev, pos + (i * 4), buf);
> > +		else
> > +			pci_write_config_dword(dev, pos + (i * 4), *buf);
> > +	}
> > +}
> > +
> > +/**
> > + * pci_vc_load_port_arb_table - load and wait for VC arbitration table
> > + * @dev: device
> > + * @pos: starting position of VC capability (VC/VC9/MFVC)
> > + *
> > + * Signal a VC arbitration table to load and wait for completion.
> > + */
> > +static void pci_vc_load_arb_table(struct pci_dev *dev, int pos)
> > +{
> > +	u32 ctrl;
> > +
> > +	pci_read_config_dword(dev, pos + PCI_VC_PORT_CTRL, &ctrl);
> > +	pci_write_config_dword(dev, pos + PCI_VC_PORT_CTRL, ctrl | 1);
> > +	if (pci_wait_for_pending(dev, pos + PCI_VC_PORT_STATUS, 1))
> 
> Comment doesn't match function name.  The spec "Request hardware to apply
> new values" language would be a useful clue that this doesn't actually
> *write* the table; before reading the spec, I initially looked for a
> buffer containing the arbitration table.  It'd be nice to have #defines
> for the bits here, i.e., PCI_VC_PORT_CTRL_LOAD_ARB or similar.  The

Ok

> spec says these are 16-bit registers; shouldn't these be "word" accesses?

The control registers are 32-bit, the status registers are 16-bit.
pci_wait_for_pending uses word access.

> > +		return;
> > +
> > +	dev_err(&dev->dev, "VC arbitration table failed to load\n");
> > +}
> > +
> > +/**
> > + * pci_vc_load_port_arb_table - Load and wait for VC port arbitration table
> > + * @dev: device
> > + * @pos: starting position of VC capability (VC/VC9/MFVC)
> > + * @res: VC res number, ie. VCn (0-7)
> > + *
> > + * Signal a VC port arbitration table to load and wait for completion.
> > + */
> > +static void pci_vc_load_port_arb_table(struct pci_dev *dev, int pos, int res)
> > +{
> > +	int ctrl_pos, status_pos;
> > +	u32 ctrl;
> > +
> > +	ctrl_pos = pos + PCI_VC_RES_CTRL + (res * PCI_CAP_VC_PER_VC_SIZEOF);
> > +	status_pos = pos + PCI_VC_RES_STATUS + (res * PCI_CAP_VC_PER_VC_SIZEOF);
> > +
> > +	pci_read_config_dword(dev, ctrl_pos, &ctrl);
> > +	pci_write_config_dword(dev, ctrl_pos, ctrl | (1 << 16));
> > +
> > +	if (pci_wait_for_pending(dev, status_pos, 1))
> 
> #defines, please, to help grep users later.

Yep

> > +		return;
> > +
> > +	dev_err(&dev->dev, "VC%d port arbitration table failed to load\n", res);
> > +}
> > +
> > +/**
> > + * pci_vc_enable - Enable virtual channel
> > + * @dev: device
> > + * @pos: starting position of VC capability (VC/VC9/MFVC)
> > + * @res: VC res number, ie. VCn (0-7)
> > + *
> > + * A VC is enabled by setting the enable bit in matching resource control
> > + * registers on both sides of a link.  We therefore need to find the opposite
> > + * end of the link.  To keep this simple we enable from the downstream device.
> > + * RC devices do not have an upstream device, nor does it seem that VC9 do
> > + * (spec is unclear).  Once we find the upstream device, match the VC ID to
> > + * get the correct resource, disable and enable on both ends.
> > + */
> > +static void pci_vc_enable(struct pci_dev *dev, int pos, int res)
> > +{
> > +	int ctrl_pos, status_pos, id, pos2, evcc, i, ctrl_pos2, status_pos2;
> > +	u32 ctrl, header, reg1, ctrl2;
> > +	struct pci_dev *link = NULL;
> > +
> > +	/* Enable VCs from the downstream device */
> > +	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
> > +	    pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM)
> > +		return;
> > +
> > +	ctrl_pos = pos + PCI_VC_RES_CTRL + (res * PCI_CAP_VC_PER_VC_SIZEOF);
> > +	status_pos = pos + PCI_VC_RES_STATUS + (res * PCI_CAP_VC_PER_VC_SIZEOF);
> > +
> > +	pci_read_config_dword(dev, ctrl_pos, &ctrl);
> > +	id = (ctrl >> 24) & 0x7;
> 
> #define PCI_VC_RES_CTRL_ID 0x07000000.
> 
> It gets a little cumbersome to have #defines for *both* the mask and the
> shift; the compromise I like is to have a #define for the mask (which shows
> the position in the register) and use bare numbers when we need to shift.
> Then it's easy to find uses of the field with grep or cscope, and the mask
> definition helps manual decoding.  There are several other opportunities
> for mask #defines below.

Ok, figuring out the define for mask vs shift did play a part in doing
neither.  I'll follow your standard.

> In this case, you only compare ID values, so there's actually no need to
> shift it.

Good point.

> > +
> > +	pci_read_config_dword(dev, pos, &header);
> > +
> > +	/* If there is no opposite end of the link, skip to enable */
> > +	if (PCI_EXT_CAP_ID(header) == PCI_EXT_CAP_ID_VC9 ||
> > +	    pci_is_root_bus(dev->bus))
> > +		goto enable;
> > +
> > +	pos2 = pci_find_ext_capability(dev->bus->self, PCI_EXT_CAP_ID_VC);
> > +	if (pos2 <= 0)
> > +		goto enable;
> 
> I don't think < 0 is possible (several other occurrences below, too).

Copied from elsewhere, but no need to propagate poor form.

> > +
> > +	pci_read_config_dword(dev->bus->self, pos2 + PCI_VC_PORT_REG1, &reg1);
> > +	evcc = reg1 & PCI_VC_REG1_EVCC;
> > +
> > +	/* VC0 is hardwired enabled, so we can start with 1 */
> > +	for (i = 1; i < evcc + 1; i++) {
> > +		ctrl_pos2 = pos2 + PCI_VC_RES_CTRL +
> > +				(i * PCI_CAP_VC_PER_VC_SIZEOF);
> > +		status_pos2 = pos2 + PCI_VC_RES_STATUS +
> > +				(i * PCI_CAP_VC_PER_VC_SIZEOF);
> > +		pci_read_config_dword(dev->bus->self, ctrl_pos2, &ctrl2);
> > +		if (((ctrl2 >> 24) & 0x7) == id) {
> > +			link = dev->bus->self;
> > +			break;
> > +		}
> > +	}
> > +
> > +	if (!link)
> > +		goto enable;
> > +
> > +	/* Disable if enabled */
> > +	if (ctrl2 & (1U << 31)) {
> > +		ctrl2 &= ~(1U << 31);
> > +		pci_write_config_dword(link, ctrl_pos2, ctrl2);
> > +	}
> > +
> > +	/* Enable on both ends */
> > +	ctrl2 |= 1U << 31;
> > +	pci_write_config_dword(link, ctrl_pos2, ctrl2);
> > +enable:
> > +	ctrl |= 1U << 31;
> > +	pci_write_config_dword(dev, ctrl_pos, ctrl);
> > +
> > +	if (!pci_wait_for_pending(dev, status_pos, 0x2))
> > +		dev_err(&dev->dev, "VC%d negotiation stuck pending\n", id);
> > +
> > +	if (link && !pci_wait_for_pending(link, status_pos2, 0x2))
> > +		dev_err(&link->dev, "VC%d negotiation stuck pending\n", id);
> > +}
> > +
> > +/**
> > + * pci_vc_do_save_buffer - Size, save, or restore VC state
> > + * @dev: device
> > + * @pos: starting position of VC capability (VC/VC9/MFVC)
> > + * @save_state: buffer for save/restore
> > + * @name: for error message
> > + * @save: if provided a buffer, this indicates what to do with it
> > + *
> > + * Walking Virtual Channel config space to size, save, or restore it
> > + * is complicated, so we do it all from one function to reduce code and
> > + * guarantee ordering matches in the buffer.  When called with NULL
> > + * @save_state, return the size of the necessary save buffer.  When called
> > + * with a non-NULL @save_state, @save determines whether we save to the
> > + * buffer or restore from it.
> > + */
> > +static int pci_vc_do_save_buffer(struct pci_dev *dev, int pos,
> > +				 struct pci_cap_saved_state *save_state,
> > +				 bool save)
> > +{
> > +	u32 reg1;
> > +	char evcc, lpevcc, parb_size;
> > +	int i, len = 0;
> > +	u32 *buf = save_state ? save_state->cap.data : NULL;
> > +
> > +	/* Sanity check buffer size for save/restore */
> > +	if (buf && save_state->cap.size !=
> > +	    pci_vc_do_save_buffer(dev, pos, NULL, save)) {
> > +		dev_err(&dev->dev,
> > +			"VC save buffer size does not match @0x%x\n", pos);
> > +		return -ENOMEM;
> > +	}
> > +
> > +	pci_read_config_dword(dev, pos + PCI_VC_PORT_REG1, &reg1);
> > +	/* Extended VC Count (not counting VC0) */
> > +	evcc = reg1 & PCI_VC_REG1_EVCC;
> > +	/* Low Priority Extended VC Count (not counting VC0) */
> > +	lpevcc = (reg1 >> 4) & PCI_VC_REG1_EVCC;
> 
> Please make a new #define, since LPEVCC is a separate field.

Ok

Thanks for the review,

Alex

> +	/* Port Arbitration Table Entry Size (bits) */
> > +	parb_size = 1 << ((reg1 >> 10) & 0x3);
> > +
> > +	/*
> > +	 * Port VC Control Register contains VC Arbitration Select, which
> > +	 * cannot be modified when more than one LPVC is in operation.  We
> > +	 * therefore save/restore it first, as only VC0 should be enabled
> > +	 * after device reset.
> > +	 */
> > +	if (buf) {
> > +		pci_vc_save_restore_dwords(dev, pos + PCI_VC_PORT_CTRL,
> > +					   buf, 1, save);
> > +		buf++;
> > +	}
> > +	len += 4;
> > +
> > +	/*
> > +	 * If we have any Low Priority VCs and a VC Arbitration Table Offset
> > +	 * in Port VC Capability Register 2 then save/restore it next.
> > +	 */
> > +	if (lpevcc) {
> > +		u32 reg2;
> > +		int vcarb_offset;
> > +
> > +		pci_read_config_dword(dev, pos + PCI_VC_PORT_REG2, &reg2);
> > +		vcarb_offset = (reg2 >> 24) * 16;
> > +
> > +		if (vcarb_offset) {
> > +			int size, vcarb_phases = 0;
> > +
> > +			if (reg2 & PCI_VC_REG2_128_PHASE)
> > +				vcarb_phases = 128;
> > +			else if (reg2 & PCI_VC_REG2_64_PHASE)
> > +				vcarb_phases = 64;
> > +			else if (reg2 & PCI_VC_REG2_32_PHASE)
> > +				vcarb_phases = 32;
> > +
> > +			/* Fixed 4 bits per phase per lpevcc (plus VC0) */
> > +			size = ((lpevcc + 1) * vcarb_phases * 4) / 8;
> > +
> > +			if (size && buf) {
> > +				pci_vc_save_restore_dwords(dev,
> > +							   pos + vcarb_offset,
> > +							   buf, size / 4, save);
> > +				/*
> > +				 * On restore, we need to signal hardware to
> > +				 * re-load the VC Arbitration Table.
> > +				 */
> > +				if (!save)
> > +					pci_vc_load_arb_table(dev, pos);
> > +
> > +				buf += size / 4;
> > +			}
> > +			len += size;
> > +		}
> > +	}
> > +
> > +	/*
> > +	 * In addition to each VC Resource Control Register, we may have a
> > +	 * Port Arbitration Table attached to each VC.  The Port Arbitration
> > +	 * Table Offset in each VC Resource Capability Register tells us if
> > +	 * it exists.  The entry size is global from the Port VC Capability
> > +	 * Register1 above.  The number of phases is determined per VC.
> > +	 */
> > +	for (i = 0; i < evcc + 1; i++) {
> > +		u32 cap;
> > +		int parb_offset;
> > +
> > +		pci_read_config_dword(dev, pos + PCI_VC_RES_CAP +
> > +				      (i * PCI_CAP_VC_PER_VC_SIZEOF), &cap);
> > +		parb_offset = (cap >> 24) * 16;
> > +		if (parb_offset) {
> > +			int size, parb_phases = 0;
> > +
> > +			if (cap & 0x20)
> > +				parb_phases = 256;
> > +			else if (cap & 0x18)
> > +				parb_phases = 128;
> > +			else if (cap & 0x4)
> > +				parb_phases = 64;
> > +			else if (cap & 0x2)
> > +				parb_phases = 32;
> > +
> > +			size = (parb_size * parb_phases) / 8;
> > +
> > +			if (size && buf) {
> > +				pci_vc_save_restore_dwords(dev,
> > +							   pos + parb_offset,
> > +							   buf, size / 4, save);
> > +				buf += size / 4;
> > +			}
> > +			len += size;
> > +		}
> > +
> > +		/* VC Resource Control Register */
> > +		if (buf) {
> > +			int ctrl_pos = pos + PCI_VC_RES_CTRL +
> > +						(i * PCI_CAP_VC_PER_VC_SIZEOF);
> > +			if (save)
> > +				pci_read_config_dword(dev, ctrl_pos, buf);
> > +			else {
> > +				u32 tmp, ctrl = *(u32 *)buf;
> > +				/*
> > +				 * For an FLR case, the VC config may remain.
> > +				 * Preserve enable bit, restore the rest.
> > +				 */
> > +				pci_read_config_dword(dev, ctrl_pos, &tmp);
> > +				tmp &= 1U << 31;
> > +				tmp |= ctrl & ~(1U << 31);
> > +				pci_write_config_dword(dev, ctrl_pos, tmp);
> > +				/* Load port arbitration table if used */
> > +				if (ctrl & (7 << 17))
> > +					pci_vc_load_port_arb_table(dev, pos, i);
> > +				/* Re-enable if needed */
> > +				if ((ctrl ^ tmp) & (1U << 31))
> > +					pci_vc_enable(dev, pos, i);
> > +			}
> > +			buf++;
> > +		}
> > +		len += 4;
> > +	}
> > +
> > +	return buf ? 0 : len;
> > +}
> > +
> > +static struct {
> > +	u16 id;
> > +	const char *name;
> > +} vc_caps[] = { { PCI_EXT_CAP_ID_MFVC, "MFVC" },
> > +		{ PCI_EXT_CAP_ID_VC, "VC" },
> > +		{ PCI_EXT_CAP_ID_VC9, "VC9" } };
> > +
> > +static int pci_save_vc_state(struct pci_dev *dev)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(vc_caps); i++) {
> > +		int pos, ret;
> > +		struct pci_cap_saved_state *save_state;
> > +
> > +		pos = pci_find_ext_capability(dev, vc_caps[i].id);
> > +		if (pos <= 0)
> > +			continue;
> > +
> > +		save_state = pci_find_saved_ext_cap(dev, vc_caps[i].id);
> > +		if (!save_state) {
> > +			dev_err(&dev->dev, "%s buffer not found in %s\n",
> > +				vc_caps[i].name, __func__);
> > +			return -ENOMEM;
> > +		}
> > +
> > +		ret = pci_vc_do_save_buffer(dev, pos, save_state, true);
> > +		if (ret) {
> > +			dev_err(&dev->dev, "%s save unsuccessful %s\n",
> > +				vc_caps[i].name, __func__);
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static void pci_restore_vc_state(struct pci_dev *dev)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(vc_caps); i++) {
> > +		int pos;
> > +		struct pci_cap_saved_state *save_state;
> > +
> > +		pos = pci_find_ext_capability(dev, vc_caps[i].id);
> > +		save_state = pci_find_saved_ext_cap(dev, vc_caps[i].id);
> > +		if (!save_state || pos <= 0)
> > +			continue;
> > +
> > +		pci_vc_do_save_buffer(dev, pos, save_state, false);
> > +	}
> > +}
> > +
> >  
> >  /**
> >   * pci_save_state - save the PCI configuration space of a device before suspending
> > @@ -986,6 +1347,8 @@ pci_save_state(struct pci_dev *dev)
> >  		return i;
> >  	if ((i = pci_save_pcix_state(dev)) != 0)
> >  		return i;
> > +	if ((i = pci_save_vc_state(dev)) != 0)
> > +		return i;
> >  	return 0;
> >  }
> >  
> > @@ -1048,6 +1411,7 @@ void pci_restore_state(struct pci_dev *dev)
> >  	/* PCI Express register must be restored first */
> >  	pci_restore_pcie_state(dev);
> >  	pci_restore_ats_state(dev);
> > +	pci_restore_vc_state(dev);
> >  
> >  	pci_restore_config_space(dev);
> >  
> > @@ -2104,6 +2468,27 @@ static int pci_add_ext_cap_save_buffer(struct pci_dev *dev,
> >  	return _pci_add_cap_save_buffer(dev, cap, true, size);
> >  }
> >  
> > +static void pci_allocate_vc_save_buffers(struct pci_dev *dev)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(vc_caps); i++) {
> > +		int error, pos = pci_find_ext_capability(dev, vc_caps[i].id);
> > +		int len;
> > +
> > +		if (pos <= 0)
> > +			continue;
> > +
> > +		len = pci_vc_do_save_buffer(dev, pos, NULL, false);
> > +		error = pci_add_ext_cap_save_buffer(dev,
> > +						    vc_caps[i].id, len);
> > +		if (error)
> > +			dev_err(&dev->dev,
> > +				"unable to preallocate %s save buffer\n",
> > +				vc_caps[i].name);
> > +	}
> > +}
> > +
> >  /**
> >   * pci_allocate_cap_save_buffers - allocate buffers for saving capabilities
> >   * @dev: the PCI device
> > @@ -2122,6 +2507,8 @@ void pci_allocate_cap_save_buffers(struct pci_dev *dev)
> >  	if (error)
> >  		dev_err(&dev->dev,
> >  			"unable to preallocate PCI-X save buffer\n");
> > +
> > +	pci_allocate_vc_save_buffers(dev);
> >  }
> >  
> >  void pci_free_cap_save_buffers(struct pci_dev *dev)
> > 

Re: [PATCH 3/3] pci: Add Virtual Channel to save/restore support

[Bjorn Helgaas]

On Tue, Dec 17, 2013 at 3:12 PM, Alex Williamson
<alex.williamson@redhat.com> wrote:
> On Mon, 2013-12-16 at 17:48 -0700, Bjorn Helgaas wrote:
>> On Tue, Dec 10, 2013 at 11:48:45AM -0700, Alex Williamson wrote:

>> > +static void pci_vc_load_arb_table(struct pci_dev *dev, int pos)
>> > +{
>> > +   u32 ctrl;
>> > +
>> > +   pci_read_config_dword(dev, pos + PCI_VC_PORT_CTRL, &ctrl);
>> > +   pci_write_config_dword(dev, pos + PCI_VC_PORT_CTRL, ctrl | 1);
>> > +   if (pci_wait_for_pending(dev, pos + PCI_VC_PORT_STATUS, 1))

>> spec says these are 16-bit registers; shouldn't these be "word" accesses?
>
> The control registers are 32-bit, the status registers are 16-bit.
> pci_wait_for_pending uses word access.

The "VC Resource Control Registers" at offset 14h + (n * 0Ch) are 32
bits, but I was referring to the PCI_VC_PORT_CTRL accesses, sorry I
didn't make that clear.  I'm looking at 7.11.4 "Port VC Control
Register" at offset 12 (0Ch), and that one looks like 16 bits to me.

Bjorn

Re: [PATCH 3/3] pci: Add Virtual Channel to save/restore support

[Bjorn Helgaas]

On Tue, Dec 17, 2013 at 3:12 PM, Alex Williamson
<alex.williamson@redhat.com> wrote:
> On Mon, 2013-12-16 at 17:48 -0700, Bjorn Helgaas wrote:
>> On Tue, Dec 10, 2013 at 11:48:45AM -0700, Alex Williamson wrote:

>> > +static void pci_vc_load_arb_table(struct pci_dev *dev, int pos)
>> > +{
>> > +   u32 ctrl;
>> > +
>> > +   pci_read_config_dword(dev, pos + PCI_VC_PORT_CTRL, &ctrl);
>> > +   pci_write_config_dword(dev, pos + PCI_VC_PORT_CTRL, ctrl | 1);
>> > +   if (pci_wait_for_pending(dev, pos + PCI_VC_PORT_STATUS, 1))

>> spec says these are 16-bit registers; shouldn't these be "word" accesses?
>
> The control registers are 32-bit, the status registers are 16-bit.
> pci_wait_for_pending uses word access.

The "VC Resource Control Registers" at offset 14h + (n * 0Ch) are 32
bits, but I was referring to the PCI_VC_PORT_CTRL accesses (sorry I
didn't make that clear).  I'm looking at 7.11.4 "Port VC Control
Register" at offset 12 (0Ch), and that one looks like 16 bits to me.

Bjorn

Re: [PATCH 3/3] pci: Add Virtual Channel to save/restore support

[Alex Williamson]

On Tue, 2013-12-17 at 15:24 -0700, Bjorn Helgaas wrote:
> On Tue, Dec 17, 2013 at 3:12 PM, Alex Williamson
> <alex.williamson@redhat.com> wrote:
> > On Mon, 2013-12-16 at 17:48 -0700, Bjorn Helgaas wrote:
> >> On Tue, Dec 10, 2013 at 11:48:45AM -0700, Alex Williamson wrote:
> 
> >> > +static void pci_vc_load_arb_table(struct pci_dev *dev, int pos)
> >> > +{
> >> > +   u32 ctrl;
> >> > +
> >> > +   pci_read_config_dword(dev, pos + PCI_VC_PORT_CTRL, &ctrl);
> >> > +   pci_write_config_dword(dev, pos + PCI_VC_PORT_CTRL, ctrl | 1);
> >> > +   if (pci_wait_for_pending(dev, pos + PCI_VC_PORT_STATUS, 1))
> 
> >> spec says these are 16-bit registers; shouldn't these be "word" accesses?
> >
> > The control registers are 32-bit, the status registers are 16-bit.
> > pci_wait_for_pending uses word access.
> 
> The "VC Resource Control Registers" at offset 14h + (n * 0Ch) are 32
> bits, but I was referring to the PCI_VC_PORT_CTRL accesses, sorry I
> didn't make that clear.  I'm looking at 7.11.4 "Port VC Control
> Register" at offset 12 (0Ch), and that one looks like 16 bits to me.

Indeed it is.  Will fix.  Thanks,

Alex