[PATCH v3 0/2] pci/lspci: Identify Enhanced Allocation (EA) Resources

[PATCH v3 0/2] pci/lspci: Identify Enhanced Allocation (EA) Resources

[Sean O. Stalley]

Identify BAR-equivalent resources that are described by EA entries
with the IORESOURCE_PCI_EA_BEI flag.

lspci cannot distinguish between resources from VF BARs and resources from EA.
This results in EA Resources being incorrectly identified as [virtual].
Adding this flag allows EA resources to be marked more accurately as [enhanced].

Although this patchset only add support for this flag to lspci,
there are other use cases (such as vfio) where knowing a resource
comes from EA would be useful.

[PATCH 1/2] is for the kernel, [PATCH 2/2] is for lspci.

Changes from V1: 
	-Rewrote commit message for linux changes

Changes from V2: 
	- Rewrote commit message for linux, fixing spelling :)
	- Added warning about resource flags being exposed in sysfs

Alex Williamson (1):
  pci: Identify Enhanced Allocation (EA) BAR Equivalent resources

Sean O. Stalley (1):
  Add support for enhanced allocation regions

linux changes:
 drivers/pci/pci.c      | 2 +-
 include/linux/ioport.h | 7 +++++++
 2 files changed, 8 insertions(+), 1 deletion(-)

lspci changes:
 lib/header.h |  4 ++++
 lib/pci.h    |  2 ++
 lib/sysfs.c  |  5 ++++-
 lspci.c      | 17 ++++++++++++++---
 4 files changed, 24 insertions(+), 4 deletions(-)


-- 
1.9.1

[PATCH v3 1/2] pci: Identify Enhanced Allocation (EA) BAR Equivalent resources

[Sean O. Stalley]

From: Alex Williamson <alex.williamson@redhat.com>

Resource flags are exposed to userspace via the sysfs "resource" file.
lspci reads the sysfs file to determine resource properties.
Adding this flag allows lspci to distinguish between [virtual]
and [enhanced] resources.

If the resource is not aligned, userspace could deduce where
the resource is EA based on the size & address fields.
However, a flag indicating whether a PCI resource is a traditional BAR
or BAR equivalent seems like a much simpler solution and works if the
EA resource is aligned.

Although this patchset only improves lspci, other uses for this
flag have been identified. For example, vfio makes assumptions
about alignment and sizing, and runs into problems when attempting
to emulate a BAR Equivalent EA resource.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
Signed-off-by: Sean O. Stalley <sean.stalley@intel.com>
---
 drivers/pci/pci.c      | 2 +-
 include/linux/ioport.h | 7 +++++++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index d1a7105..8ff678c 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2229,7 +2229,7 @@ void pci_pm_init(struct pci_dev *dev)
 
 static unsigned long pci_ea_flags(struct pci_dev *dev, u8 prop)
 {
-	unsigned long flags = IORESOURCE_PCI_FIXED;
+	unsigned long flags = IORESOURCE_PCI_FIXED | IORESOURCE_PCI_EA_BEI;
 
 	switch (prop) {
 	case PCI_EA_P_MEM:
diff --git a/include/linux/ioport.h b/include/linux/ioport.h
index 24bea08..88816f9 100644
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h
@@ -25,7 +25,11 @@ struct resource {
 
 /*
  * IO resources have these defined flags.
+ *
+ * PCI devices expose these flags to userspace in the "resource" sysfs file,
+ * Moving them around could break stuff, so don't do it.
  */
+
 #define IORESOURCE_BITS		0x000000ff	/* Bus-specific bits */
 
 #define IORESOURCE_TYPE_BITS	0x00001f00	/* Resource type */
@@ -97,6 +101,7 @@ struct resource {
 #define IORESOURCE_IO_SPARSE		(1<<2)
 
 /* PCI ROM control bits (IORESOURCE_BITS) */
+
 #define IORESOURCE_ROM_ENABLE		(1<<0)	/* ROM is enabled, same as PCI_ROM_ADDRESS_ENABLE */
 #define IORESOURCE_ROM_SHADOW		(1<<1)	/* ROM is copy at C000:0 */
 #define IORESOURCE_ROM_COPY		(1<<2)	/* ROM is alloc'd copy, resource field overlaid */
@@ -105,6 +110,8 @@ struct resource {
 /* PCI control bits.  Shares IORESOURCE_BITS with above PCI ROM.  */
 #define IORESOURCE_PCI_FIXED		(1<<4)	/* Do not move resource */
 
+/* PCI Enhanced Allocation defined BAR equivalent resource */
+#define IORESOURCE_PCI_EA_BEI		(1<<5)
 
 /* helpers to define resources */
 #define DEFINE_RES_NAMED(_start, _size, _name, _flags)			\
-- 
1.9.1

[PATCH v3 2/2] Add support for enhanced allocation regions

[Sean O. Stalley]

Append [enhanced] to Regions that contain the BEI flag in sysfs.
Don't tuncate least significant bits of the region size.
ex: a 2000 byte region should display [size=2000] instead of [size=1K]

Signed-off-by: Sean O. Stalley <sean.stalley@intel.com>
---
 lib/header.h |  4 ++++
 lib/pci.h    |  2 ++
 lib/sysfs.c  |  5 ++++-
 lspci.c      | 17 ++++++++++++++---
 4 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/lib/header.h b/lib/header.h
index b8f7dc1..7b9a803 100644
--- a/lib/header.h
+++ b/lib/header.h
@@ -1235,3 +1235,7 @@
 
 #define PCI_VENDOR_ID_INTEL		0x8086
 #define PCI_VENDOR_ID_COMPAQ		0x0e11
+
+/* taken from <include/linux/ioport.h> */
+
+#define IORESOURCE_PCI_EA_BEI          (1<<5)
diff --git a/lib/pci.h b/lib/pci.h
index 9c1e281..a88e156 100644
--- a/lib/pci.h
+++ b/lib/pci.h
@@ -129,8 +129,10 @@ struct pci_dev {
   int irq;				/* IRQ number */
   pciaddr_t base_addr[6];		/* Base addresses including flags in lower bits */
   pciaddr_t size[6];			/* Region sizes */
+  pciaddr_t flags[6];			/* Region flags */
   pciaddr_t rom_base_addr;		/* Expansion ROM base address */
   pciaddr_t rom_size;			/* Expansion ROM size */
+  pciaddr_t rom_flags;			/* Expansion ROM flags */
   struct pci_cap *first_cap;		/* List of capabilities */
   char *phy_slot;			/* Physical slot */
   char *module_alias;			/* Linux kernel module alias */
diff --git a/lib/sysfs.c b/lib/sysfs.c
index 986ecc9..e900e3a 100644
--- a/lib/sysfs.c
+++ b/lib/sysfs.c
@@ -153,14 +153,17 @@ sysfs_get_resources(struct pci_dev *d)
 	size = end - start + 1;
       else
 	size = 0;
-      flags &= PCI_ADDR_FLAG_MASK;
       if (i < 6)
 	{
+	  d->flags[i] = flags;
+	  flags &= PCI_ADDR_FLAG_MASK;
 	  d->base_addr[i] = start | flags;
 	  d->size[i] = size;
 	}
       else
 	{
+	  d->rom_flags = flags;
+	  flags &= PCI_ADDR_FLAG_MASK;
 	  d->rom_base_addr = start | flags;
 	  d->rom_size = size;
 	}
diff --git a/lspci.c b/lspci.c
index fe7b7fe..358bb34 100644
--- a/lspci.c
+++ b/lspci.c
@@ -336,7 +336,7 @@ show_size(pciaddr_t x)
   if (!x)
     return;
   for (i = 0; i < (sizeof(suffix) / sizeof(*suffix) - 1); i++) {
-    if (x < 1024)
+    if (x % 1024)
       break;
     x /= 1024;
   }
@@ -364,7 +364,12 @@ show_bases(struct device *d, int cnt)
 	printf("\tRegion %d: ", i);
       else
 	putchar('\t');
-      if (pos && !flg)			/* Reported by the OS, but not by the device */
+
+      if (p->flags[i] & IORESOURCE_PCI_EA_BEI)
+	{
+	  printf("[enhanced] ");
+	}
+      else if (pos && !flg)	/* Reported by the OS, but not by the device */
 	{
 	  printf("[virtual] ");
 	  flg = pos;
@@ -437,7 +442,13 @@ show_rom(struct device *d, int reg)
   if (!rom && !flg && !len)
     return;
   putchar('\t');
-  if ((rom & PCI_ROM_ADDRESS_MASK) && !(flg & PCI_ROM_ADDRESS_MASK))
+
+
+  if (p->rom_flags & IORESOURCE_PCI_EA_BEI)
+    {
+      printf("[enhanced] ");
+    }
+  else if ((rom & PCI_ROM_ADDRESS_MASK) && !(flg & PCI_ROM_ADDRESS_MASK))
     {
       printf("[virtual] ");
       flg = rom;
-- 
1.9.1

Re: [PATCH v3 2/2] Add support for enhanced allocation regions

[Martin Mares]

Hello!

> diff --git a/lib/header.h b/lib/header.h
> index b8f7dc1..7b9a803 100644
> --- a/lib/header.h
> +++ b/lib/header.h
> @@ -1235,3 +1235,7 @@
>  
>  #define PCI_VENDOR_ID_INTEL		0x8086
>  #define PCI_VENDOR_ID_COMPAQ		0x0e11
> +
> +/* taken from <include/linux/ioport.h> */
> +
> +#define IORESOURCE_PCI_EA_BEI          (1<<5)
> diff --git a/lib/pci.h b/lib/pci.h
> index 9c1e281..a88e156 100644
> --- a/lib/pci.h
> +++ b/lib/pci.h
> @@ -129,8 +129,10 @@ struct pci_dev {
>    int irq;				/* IRQ number */
>    pciaddr_t base_addr[6];		/* Base addresses including flags in lower bits */
>    pciaddr_t size[6];			/* Region sizes */
> +  pciaddr_t flags[6];			/* Region flags */
>    pciaddr_t rom_base_addr;		/* Expansion ROM base address */
>    pciaddr_t rom_size;			/* Expansion ROM size */
> +  pciaddr_t rom_flags;			/* Expansion ROM flags */

First, please explain in comments how does this thing work.

Second, please try not to break binary compatibility. New fields should
be added at the end of the public part of the structure. You also have to
define a new version of pci_fill_info(), so that applications using the
new layout of the structure will require a new version of libpci.

> +      if (p->flags[i] & IORESOURCE_PCI_EA_BEI)
> +	{
> +	  printf("[enhanced] ");
> +	}

Please follow the style of the rest of the code. One-statement conditions
do not have braces.

				Martin

Re: [PATCH v3 2/2] Add support for enhanced allocation regions

[Sean O. Stalley]

Thanks for reviewing Martin. My response is inline.

-Sean

On Tue, Feb 09, 2016 at 10:54:53AM +0100, Martin Mares wrote:
> Hello!
> 
> > diff --git a/lib/header.h b/lib/header.h
> > index b8f7dc1..7b9a803 100644
> > --- a/lib/header.h
> > +++ b/lib/header.h
> > @@ -1235,3 +1235,7 @@
> >  
> >  #define PCI_VENDOR_ID_INTEL		0x8086
> >  #define PCI_VENDOR_ID_COMPAQ		0x0e11
> > +
> > +/* taken from <include/linux/ioport.h> */
> > +
> > +#define IORESOURCE_PCI_EA_BEI          (1<<5)
> > diff --git a/lib/pci.h b/lib/pci.h
> > index 9c1e281..a88e156 100644
> > --- a/lib/pci.h
> > +++ b/lib/pci.h
> > @@ -129,8 +129,10 @@ struct pci_dev {
> >    int irq;				/* IRQ number */
> >    pciaddr_t base_addr[6];		/* Base addresses including flags in lower bits */
> >    pciaddr_t size[6];			/* Region sizes */
> > +  pciaddr_t flags[6];			/* Region flags */
> >    pciaddr_t rom_base_addr;		/* Expansion ROM base address */
> >    pciaddr_t rom_size;			/* Expansion ROM size */
> > +  pciaddr_t rom_flags;			/* Expansion ROM flags */
> 
> First, please explain in comments how does this thing work.
> 

Sure thing. Would you like multiple lines,
or just a few words about where the flags come from?

> Second, please try not to break binary compatibility. New fields should
> be added at the end of the public part of the structure. You also have to
> define a new version of pci_fill_info(), so that applications using the
> new layout of the structure will require a new version of libpci.
> 

I moved the new fields to the end of the public data in the struct.
I'll increment the API version number and add the appropriate checks.
I was hoping to send out a new version of the patchset today,
but adding the versioning added more time than I expected. 

> > +      if (p->flags[i] & IORESOURCE_PCI_EA_BEI)
> > +	{
> > +	  printf("[enhanced] ");
> > +	}
> 
> Please follow the style of the rest of the code. One-statement conditions
> do not have braces.

Ok, will do.