Driver for new "VMD" device

PCI-e segments will continue to use the lower 16 bits as required by ACPI. Special domains may use the full 32-bits.

Signed-off-by Keith Busch <[email protected]>

---
 lib/filter.c |    2 +-
 lib/pci.h    |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

lib/filter.c +1 -1

index d4254a0..075dc2f 100644
@@ -45,7 +45,7 @@ pci_filter_parse_slot_v33(struct pci_filter *f, char *str)
          if (str[0] && strcmp(str, "*"))
            {
              long int x = strtol(str, &e, 16);
-             if ((e && *e) || (x < 0 || x > 0xffff))
+             if ((e && *e) || (x < 0))
                return "Invalid domain number";
              f->domain = x;
            }

lib/pci.h +0 -0

index 10ba831..7e42765 100644

lib/pci.h +0 -0

   u8 bus, dev, func;                   /* Bus inside domain, device and function */
 

   /* These fields are set by pci_fill_info() */

--
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

PCI-e segments will continue to use the lower 16 bits as required by ACPI. Special domains may use the full 32-bits.

Signed-off-by Keith Busch <[email protected]>

---
 lib/filter.c |    2 +-
 lib/pci.h    |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

lib/filter.c +0 -0

index d4254a0..075dc2f 100644

lib/filter.c +0 -0

 		return "Invalid domain number";
 	      f->domain = x;
 	    }

lib/pci.h +1 -1

index 10ba831..7e42765 100644
@@ -119,7 +119,7 @@ struct pci_param *pci_walk_params(struct pci_access *acc, struct pci_param *prev
 
 struct pci_dev {
   struct pci_dev *next;			/* Next device in the chain */
-  u16 domain;				/* PCI domain (host bridge) */
+  int32_t domain;			/* PCI domain (host bridge) */
   u8 bus, dev, func;			/* Bus inside domain, device and function */
 
   /* These fields are set by pci_fill_info() */

Signature · 7 lines

1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

lib/filter.c pci_filter_parse_slot_v33(struct pci_filter *f, char *str) · line 48

Quoted 6 lines

lib/filter.c +1 -1

@@ -45,7 +45,7 @@ pci_filter_parse_slot_v33(struct pci_filter *f, char *str)
 	  if (str[0] && strcmp(str, "*"))
 	    {
 	      long int x = strtol(str, &e, 16);
-	      if ((e && *e) || (x < 0 || x > 0xffff))
+	      if ((e && *e) || (x < 0))

Just out of curiosity (I don't maintain pciutils; Martin would apply this one), is there some part of the PCI or PCI firmware spec that is relevant to this change? Maybe this is connected to parsing things exported by the kernel and not directly tied to PCI at the spec level.

Whatever it is, a pointer to the producer of the information you're consuming here would help us understand and review the patch.

Quoted 12 lines · Bjorn Helgaas

lib/filter.c pci_filter_parse_slot_v33(struct pci_filter *f, char *str) · line 48

Quoted 6 lines

lib/filter.c +1 -1

@@ -45,7 +45,7 @@ pci_filter_parse_slot_v33(struct pci_filter *f, char *str)
 	  if (str[0] && strcmp(str, "*"))
 	    {
 	      long int x = strtol(str, &e, 16);
-	      if ((e && *e) || (x < 0 || x > 0xffff))
+	      if ((e && *e) || (x < 0))

Just out of curiosity (I don't maintain pciutils; Martin would apply this one), is there some part of the PCI or PCI firmware spec that is relevant to this change? Maybe this is connected to parsing things exported by the kernel and not directly tied to PCI at the spec level.

Quoted 2 lines

Whatever it is, a pointer to the producer of the information you're consuming here would help us understand and review the patch.

Hi Bjorn,

This is not tied to anything defined in PCI spec. Domain numbers being a software construct (ACPI6, §6.5.6), we don't need to constrain the representation. ACPI defines 16-bit segments, and domains provided by this new host bridge do not define _SEG, so this series proposes domain numbers outside the ACPI reachable range to avoid potential clashes.

The pciutils patch just synchronizes the essential tooling software with the kernel software's new representation.

PCI-e segments will continue to use the lower 16 bits as required by ACPI. Special domains may use the full 32-bits.

Signed-off-by Keith Busch <[email protected]>

---
 lib/filter.c |    2 +-
 lib/pci.h    |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

lib/filter.c +1 -1

index d4254a0..075dc2f 100644
@@ -45,7 +45,7 @@ pci_filter_parse_slot_v33(struct pci_filter *f, char *str)
 	  if (str[0] && strcmp(str, "*"))
 	    {
 	      long int x = strtol(str, &e, 16);
-	      if ((e && *e) || (x < 0 || x > 0xffff))
+	      if ((e && *e) || (x < 0))
 		return "Invalid domain number";
 	      f->domain = x;
 	    }

lib/pci.h +0 -0

index 10ba831..7e42765 100644

This is definitely not enough. Try grepping the source for "domain" :-)

At least the following places need updating, too:

o struct pci_filter and operations on it

o Format strings for printing domains at various places

o ABI compability ... changing a field in the middle of struct pci_dev (or pci_filter) is going to break ABI, so you either need to change the structures in a backward-compatible way, or to use ABI versioning.

Also, we should decide on what type the domain should have -- currently, some places use "int", others use u16, and your patch introduces int32_t. I would prefer u32 myself, but especially in the filters we should be careful about how to encode "any domain".

Quoted 5 lines

This is definitely not enough. Try grepping the source for "domain" :-)

At least the following places need updating, too:

o struct pci_filter and operations on it

Not sure I follow. struct pci_filter's domain was already a 32-bit int.

Quoted 1 line

o Format strings for printing domains at various places

Are you wanting a %04x for 16 bit domains and %08x for 32 bit ones? The %04x specifier still works with 32-bit values.

Quoted 3 lines

o ABI compability ... changing a field in the middle of struct pci_dev (or pci_filter) is going to break ABI, so you either need to change the structures in a backward-compatible way, or to use ABI versioning.

It looks like there's a 16-bit gap after device_class. Would it be acceptable place the domain's upper 16 bits in there to keep ABI compatibility?

Does not allocate a child bus if the new bus number does not fit in the parent's bus resource window.

Signed-off-by Keith Busch <[email protected]>

---
 drivers/pci/probe.c | 6 ++++++
 1 file changed, 6 insertions(+)

drivers/pci/probe.c +0 -0

index edb1984..6e29f7a 100644

drivers/pci/probe.c +1 -0

+	}
 	/*
 	 * Allocate a new bus, and inherit stuff from the parent..
 	 */

Signature · 6 lines

2.6.2.307.g37023ba

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

drivers/pci/probe.c static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent, · line 711

Quoted 5 lines · Keith Busch

drivers/pci/probe.c +5 -0

+	if (busnr > parent->busn_res.end) {
+		dev_printk(KERN_DEBUG, &parent->dev,
+			  "can not alloc bus:%d under %pR\n", busnr,
+			  &parent->busn_res);
+		return NULL;

Can you take a look at 1820ffdccb9b ("PCI: Make sure bus number resources stay within their parents bounds") and 12d8706963f0 ("Revert "PCI: Make sure bus number resources stay within their parents bounds"")?

This is implemented differently, but it seems like it might expose the same problem we found with 1820ffdccb9b.

If you could take a look and confirm that "no, this does something differently than 1820ffdccb9b did" or "yes, this might expose that problem again," that would help.

Does not allocate a child bus if the new bus number does not fit in the parent's bus resource window.

Signed-off-by Keith Busch <[email protected]>

---
 drivers/pci/probe.c | 6 ++++++
 1 file changed, 6 insertions(+)

drivers/pci/probe.c +6 -0

index edb1984..6e29f7a 100644
@@ -704,6 +704,12 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
 	int i;
 	int ret;
 
+	if (busnr > parent->busn_res.end) {
+		dev_printk(KERN_DEBUG, &parent->dev,
+			  "can not alloc bus:%d under %pR\n", busnr,
+			  &parent->busn_res);
+		return NULL;
+	}
 	/*
 	 * Allocate a new bus, and inherit stuff from the parent..
 	 */

Signature · 6 lines

2.6.2.307.g37023ba

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

The Intel Volume Management Device (VMD) is an integrated endpoint on the platform's PCIe root complex that acts as a host bridge to a secondary PCIe domain. BIOS can reassign one or more root ports to appear within a VMD domain instead of the primary domain. The immediate benefit is that additional PCI-e domains allow more than 256 buses in a system by letting bus number be reused across different domains.

+/*
+ * VMD h/w converts posted config writes to non-posted. The read-back in this
+ * function forces the completion so it returns only after the config space was
+ * written, as expected.

+ */

arch/x86/pci/vmd.c new file +82 -0

+static int vmd_pci_write(struct pci_bus *bus, unsigned int devfn, int reg,
+							int len, u32 value)
+{
+	int ret = 0;
+	unsigned long flags;
+	struct vmd_dev *vmd = vmd_from_bus(bus);
+	char __iomem *addr = vmd->cfgbar + (bus->number << 20) +
+						(devfn << 12) + reg;
+
+	if ((addr - vmd->cfgbar) + len >= resource_size(&vmd->dev->resource[0]))
+		return -EFAULT;
+
+	spin_lock_irqsave(&vmd->cfg_lock, flags);
+	switch (len) {
+	case 1:
+		writeb(value, addr);
+		readb(addr);
+		break;
+	case 2:
+		writew(value, addr);
+		readw(addr);
+		break;
+	case 4:
+		writel(value, addr);
+		readl(addr);
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+	spin_unlock_irqrestore(&vmd->cfg_lock, flags);
+	return ret;
+}
 
+static int vmd_probe(struct pci_dev *dev, const struct pci_device_id *id)
+{
+	struct vmd_dev *vmd;
+	int i, err;
+
+	if (resource_size(&dev->resource[0]) < (1 << 20))
+		return -ENOMEM;
+
+	vmd = devm_kzalloc(&dev->dev, sizeof(*vmd), GFP_KERNEL);
+	if (!vmd)
+		return -ENOMEM;
+
+	vmd->dev = dev;
+	err = pcim_enable_device(dev);
+	if (err < 0)
+		return err;
+
+	vmd->cfgbar = pcim_iomap(dev, 0, 0);
+	if (!vmd->cfgbar)
+		return -ENOMEM;
+
+	pci_set_master(dev);
+	if (dma_set_mask_and_coherent(&dev->dev, DMA_BIT_MASK(64)) &&
+	    dma_set_mask_and_coherent(&dev->dev, DMA_BIT_MASK(32)))
+		return -ENODEV;
+
+	vmd->msix_count = pci_msix_vec_count(dev);
+	if (vmd->msix_count < 0)
+		return -ENODEV;
+
+	vmd->irqs = devm_kcalloc(&dev->dev, vmd->msix_count, sizeof(*vmd->irqs),
+							GFP_KERNEL);
+	if (!vmd->irqs)
+		return -ENOMEM;
+
+	vmd->msix_entries = devm_kcalloc(&dev->dev, vmd->msix_count,
+					sizeof(*vmd->msix_entries), GFP_KERNEL);
+	if(!vmd->msix_entries)
+		return -ENOMEM;
+	for (i = 0; i < vmd->msix_count; i++)
+		vmd->msix_entries[i].entry = i;
+
+	vmd->msix_count = pci_enable_msix_range(vmd->dev, vmd->msix_entries, 1,
+							vmd->msix_count);
+	if (vmd->msix_count < 0)
+		return vmd->msix_count;
+
+	for (i = 0; i < vmd->msix_count; i++) {
+		INIT_LIST_HEAD(&vmd->irqs[i].irq_list);

Quoted 4 lines · Keith Busch

+/*
+ * VMD h/w converts posted config writes to non-posted. The read-back in this
+ * function forces the completion so it returns only after the config space was
+ * written, as expected.

This comment sounds backwards:

posted writes don't wait for completion
non-posted writes do wait for completion

If the hardware converts to non-posted writes, you shouldn't need a read-back. It seems like you would need the read-back if the hardware converted non-posted to posted.

Seems like it might be nice to have something in dmesg that would connect this PCI device to the new PCI domain. It's a new, unusual topology and a hint might help everybody understand what's going on.

New pci device provides additional pci domains that start above what 16 bits can address.

Signed-off-by Keith Busch <[email protected]>

---
 drivers/pci/pcie/aer/aer_inject.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

drivers/pci/pcie/aer/aer_inject.c +0 -0

index 182224a..24b191e 100644

drivers/pci/pcie/aer/aer_inject.c +7 -7

 };
 
 struct aer_error {
 	struct list_head list;
-	u16 domain;
+	int domain;
 	unsigned int bus;
 	unsigned int devfn;
 	int pos_cap_err;

@@ -74,7 +74,7 @@ static LIST_HEAD(pci_bus_ops_list);
 /* Protect einjected and pci_bus_ops_list */
 static DEFINE_SPINLOCK(inject_lock);
 
-static void aer_error_init(struct aer_error *err, u16 domain,
+static void aer_error_init(struct aer_error *err, int domain,
 			   unsigned int bus, unsigned int devfn,
 			   int pos_cap_err)
 {

@@ -86,7 +86,7 @@ static void aer_error_init(struct aer_error *err, u16 domain,
 }
 
 /* inject_lock must be held before calling */
-static struct aer_error *__find_aer_error(u16 domain, unsigned int bus,
+static struct aer_error *__find_aer_error(int domain, unsigned int bus,
 					  unsigned int devfn)
 {
 	struct aer_error *err;

@@ -106,7 +106,7 @@ static struct aer_error *__find_aer_error_by_dev(struct pci_dev *dev)
 	int domain = pci_domain_nr(dev->bus);
 	if (domain < 0)
 		return NULL;
-	return __find_aer_error((u16)domain, dev->bus->number, dev->devfn);
+	return __find_aer_error(domain, dev->bus->number, dev->devfn);
 }
 
 /* inject_lock must be held before calling */

@@ -196,7 +196,7 @@ static int pci_read_aer(struct pci_bus *bus, unsigned int devfn, int where,
 	domain = pci_domain_nr(bus);
 	if (domain < 0)
 		goto out;
-	err = __find_aer_error((u16)domain, bus->number, devfn);
+	err = __find_aer_error(domain, bus->number, devfn);
 	if (!err)
 		goto out;
 

@@ -228,7 +228,7 @@ static int pci_write_aer(struct pci_bus *bus, unsigned int devfn, int where,
 	domain = pci_domain_nr(bus);
 	if (domain < 0)
 		goto out;
-	err = __find_aer_error((u16)domain, bus->number, devfn);
+	err = __find_aer_error(domain, bus->number, devfn);
 	if (!err)
 		goto out;
 

@@ -329,7 +329,7 @@ static int aer_inject(struct aer_error_inj *einj)
 	u32 sever, cor_mask, uncor_mask, cor_mask_orig = 0, uncor_mask_orig = 0;
 	int ret = 0;
 
-	dev = pci_get_domain_bus_and_slot((int)einj->domain, einj->bus, devfn);
+	dev = pci_get_domain_bus_and_slot(einj->domain, einj->bus, devfn);
 	if (!dev)
 		return -ENODEV;
 	rpdev = pcie_find_root_port(dev);

Signature · 6 lines

2.6.2.307.g37023ba

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Review

Quoted 2 lines · Keith Busch

(quoted diff) +1 -1

-	u16 domain;
+	int domain;

If you want 32 bits explicitly, why don't you use u32 here?