#3062 closed defect (invalid)

When a PCI addreess range is empty PCI Library Programs Bridges with errouneous address range

Reported by: Jeffrey Hill Owned by: Daniel Hellstrom
Priority: normal Milestone:
Component: score Version: 4.11
Severity: normal Keywords:
Cc: Blocked By:
Blocking:

Description

It looks like defensive code as below should be added to the top of pci_set_bar in pci_cfg_auto.c so that, when the bridge address range is empty, erroneous address ranges are not programmed into the bridge device.

@@ -703,47 +707,49 @@ static void pci_set_bar(struct pci_dev *dev, int residx)
 	if ((res->flags == 0) || (res->flags & PCI_RES_FAIL))
 		return;
 
+  if ( res->end <= res->start )
+		return;
+

Change History (13)

comment:1 Changed on Jul 5, 2017 at 6:04:06 PM by Jeffrey Hill

Version: 4.124.11

comment:2 Changed on Jul 5, 2017 at 6:29:15 PM by Jeffrey Hill

Component: Generalcpukit
Owner: set to joel.sherrill@…

comment:3 Changed on Jul 5, 2017 at 6:33:28 PM by Joel Sherrill

Owner: changed from joel.sherrill@… to Ben Gras
Status: newassigned

comment:4 Changed on Jul 5, 2017 at 6:55:12 PM by Jeffrey Hill

Considering this further, if it was a plug-and-play PCI system, then one would also need to make certain that the address range was disabled in the bridge if the topology changed in way that caused the address range in a particular bridge to become empty in a 2nd or 3rd scan after power on.

The scenario that I am concerned about would be if someone, for example, removed a card with the power on and then soft rebooted RTEMS. That might happen for example if a PCI Express over cable device was turned off.

This sounds like a very legitimate concern in our situation so I will submit another suggested path. standby...

comment:5 Changed on Jul 5, 2017 at 7:36:45 PM by Jeffrey Hill

I had a closer look and I believe that there is no need to be concerned about plug-and-play. This conclusion is based on identifying the following code.

line 309, the memory and IO spaces are initially disabled when the device or bridge is located.

PCI_CFG_W16(pcidev, PCIR_COMMAND, 0);

Line 342, the bridge's memory and IO space base addresses are cleared to zero when the bridge device is initially located

PCI_CFG_W32(pcidev, 0x28, 0);
PCI_CFG_W32(pcidev, 0x2C, 0);

comment:6 Changed on Jul 5, 2017 at 7:57:35 PM by Jeffrey Hill

This change still appears to be needed

@@ -703,47 +707,49 @@ static void pci_set_bar(struct pci_dev *dev, int residx)
 	if ((res->flags == 0) || (res->flags & PCI_RES_FAIL))
 		return;
 
+  if ( res->end <= res->start )
+		return;
+

comment:7 Changed on Jul 5, 2017 at 11:38:46 PM by Jeffrey Hill

I modified code starting at line 740 as follows, and now DEBUG diagnostics are much easier to follow.

-
-		/* Limit and Base */
-		tmp = ((res->end-1) & 0xfff00000) | (res->start >> 16);
-
-		DBG("PCI[%x:%x:%x]: BRIDGE BAR 0x%x: 0x%08x\n",
-			PCI_DEV_EXPAND(pcidev),
-			0x20 + (res->bar-BRIDGE_RES_MEMIO)*4, tmp);
-		PCI_CFG_W32(pcidev, 0x20+(res->bar-BRIDGE_RES_MEMIO)*4, tmp);
+    /* Limit and Base */
+    tmp = ((res->end-1) & 0xfff00000) | (res->start >> 16);
+#ifdef DEBUG
+    char * pName = "BOGOSITY";
+    if ( res->bar == BRIDGE_RES_MEMIO ) {
+      pName = "MEM IO";
+    }
+    else if ( res->bar == PCI_RES_MEM_PREFETCH ) {
+      pName = "MEM PREFETCH";
+    }
+    DBG("PCI[%x:%x:%x]: BRIDGE %s BAR: 0x%08x-0x%08x\n",
+      PCI_DEV_EXPAND(pcidev),
+      pName,
+      (tmp & 0xfff0 ) << 16u,
+      (tmp & 0xfff00000) | 0x000fffff);
+#endif
+    PCI_CFG_W32(pcidev, 0x20+(res->bar-BRIDGE_RES_MEMIO)*4, tmp);

comment:8 Changed on Jul 6, 2017 at 12:31:23 AM by Jeffrey Hill

One is fairly quickly convinced that there will be an issue when "end <= start" if we look at this line in the source. Refer to the PCI Bridge specification to fully understand this part of the code. The bridge window position and size registers at offset 0x20 and 0x24 are being programmed.

line 741:

    tmp = ((res->end-1) & 0xfff00000) | (res->start >> 16);
Last edited on Jul 6, 2017 at 12:49:53 AM by Jeffrey Hill (previous) (diff)

comment:9 Changed on Jul 7, 2017 at 3:09:26 PM by Gedare Bloom

Owner: changed from Ben Gras to Daniel Hellstrom

comment:10 Changed on Jul 11, 2017 at 8:49:05 PM by Jeffrey Hill

Certainly, in the above snippet a very wide window would be opened if res->end and res->start were both zero, and perhaps I recall correctly that it is normal to use those values for PCI hierarchical address spaces that are discovered to be unused.

comment:11 Changed on Jul 11, 2017 at 10:08:29 PM by Jeffrey Hill

At line 309 the memory and IO spaces are initially disabled when the device or bridge is located.

    PCI_CFG_W16(pcidev, PCIR_COMMAND, 0);

At Line 342 the bridge's 64 bit memory and memory IO space base addresses are cleared to zero when the bridge device is initially located

    PCI_CFG_W32(pcidev, 0x28, 0);
    PCI_CFG_W32(pcidev, 0x2C, 0);

However, I don't see that registers 0x1c, 0x20, and 0x24 are receiving initialization in pci_find_devs, and therefore they will need to be set later

And therefore my suggested change below isn’t going to perform the necessary initialization for the unused address spaces in a bridge.

@@ -703,47 +707,49 @@ static void pci_set_bar(struct pci_dev *dev, int residx)
 	if ((res->flags == 0) || (res->flags & PCI_RES_FAIL))
 		return;
 
+  if ( res->end <= res->start )
+		return;
+

I will need to watch it run again and verify that res->end and res->start are actually being used when there values are both zero, as I fear

comment:12 Changed on Jul 11, 2017 at 10:47:12 PM by Jeffrey Hill

Ok, res->end isnt used when it is zero so I will close out the request

comment:13 Changed on Jul 11, 2017 at 10:50:48 PM by Jeffrey Hill

Resolution: invalid
Status: assignedclosed

Sorry, some confusion here when bringing up a new FPGA PCIe hardware designed led to some incorrect conclusions about issues in the software, and so I must retract this request.

Note: See TracTickets for help on using tickets.