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

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

[Sean O. Stalley]

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

We've done a pretty good job of abstracting EA from drivers, but there
are some properties of BAR Equivalent resources that don't really jive
with traditional PCI BARs.  In particular, natural alignment is only
encouraged, not required.

Why does this matter?  There are drivers like vfio-pci that will
happily gobble up the EA abstraction that's been implemented and
expose a device using EA to userspace as if those resources are
traditional BARs.  Pretty cool.  The vfio API is bus agnostic, so it
doesn't care about alignment.  The problem comes with PCI config space
emulation where we don't let userspace manipulate the BAR value, but
we do emulate BAR sizing.  The abstraction kind of falls apart if
userspace gets garbage when they try to size what appears to be a
traditional BAR, but is actually a BAR equivalent.

We could simply round up the size in vfio to make it naturally
aligned, but then we're imposing artificial sizes to the user and we
have the discontinuity that BAR size emulation and vfio region size
reporting don't agree on the size.  I think what we want to do is
expose EA to the user, reporting traditional BARs with BEIs as
zero-sized and providing additional regions for the user to access
each EA region, whether it has a BEI or not.

To facilitate that, a flag indicating whether a PCI resource is a
traditional BAR or BAR equivalent seems much nicer than attempting
to size the BAR ourselves or deducing it through the EA capability.

Thoughts?

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 drivers/pci/pci.c      | 2 +-
 include/linux/ioport.h | 2 ++
 2 files changed, 3 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..5acc194 100644
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h
@@ -105,6 +105,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

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

[Alex Williamson]

On Thu, 2016-01-21 at 14:13 -0800, Sean O. Stalley wrote:
> From: Alex Williamson <alex.williamson@redhat.com>
> 
> We've done a pretty good job of abstracting EA from drivers, but there
> are some properties of BAR Equivalent resources that don't really jive
> with traditional PCI BARs.  In particular, natural alignment is only
> encouraged, not required.
> 
> Why does this matter?  There are drivers like vfio-pci that will
> happily gobble up the EA abstraction that's been implemented and
> expose a device using EA to userspace as if those resources are
> traditional BARs.  Pretty cool.  The vfio API is bus agnostic, so it
> doesn't care about alignment.  The problem comes with PCI config space
> emulation where we don't let userspace manipulate the BAR value, but
> we do emulate BAR sizing.  The abstraction kind of falls apart if
> userspace gets garbage when they try to size what appears to be a
> traditional BAR, but is actually a BAR equivalent.
> 
> We could simply round up the size in vfio to make it naturally
> aligned, but then we're imposing artificial sizes to the user and we
> have the discontinuity that BAR size emulation and vfio region size
> reporting don't agree on the size.  I think what we want to do is
> expose EA to the user, reporting traditional BARs with BEIs as
> zero-sized and providing additional regions for the user to access
> each EA region, whether it has a BEI or not.
> 
> To facilitate that, a flag indicating whether a PCI resource is a
> traditional BAR or BAR equivalent seems much nicer than attempting
> to size the BAR ourselves or deducing it through the EA capability.
> 
> Thoughts?
> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---

Feel free to rewrite this changelog and tailor it to the use case
that's currently driving the change.  Add a sign-off for yourself too
since the patch is passing through your hands.  Thanks,

Alex

>  drivers/pci/pci.c      | 2 +-
>  include/linux/ioport.h | 2 ++
>  2 files changed, 3 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..5acc194 100644
> --- a/include/linux/ioport.h
> +++ b/include/linux/ioport.h
> @@ -105,6 +105,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)			\

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

[Sean O. Stalley]

On Thu, Jan 21, 2016 at 03:28:50PM -0700, Alex Williamson wrote:
> On Thu, 2016-01-21 at 14:13 -0800, Sean O. Stalley wrote:
> > From: Alex Williamson <alex.williamson@redhat.com>
> > 
> > We've done a pretty good job of abstracting EA from drivers, but there
> > are some properties of BAR Equivalent resources that don't really jive
> > with traditional PCI BARs.  In particular, natural alignment is only
> > encouraged, not required.
> > 
> > Why does this matter?  There are drivers like vfio-pci that will
> > happily gobble up the EA abstraction that's been implemented and
> > expose a device using EA to userspace as if those resources are
> > traditional BARs.  Pretty cool.  The vfio API is bus agnostic, so it
> > doesn't care about alignment.  The problem comes with PCI config space
> > emulation where we don't let userspace manipulate the BAR value, but
> > we do emulate BAR sizing.  The abstraction kind of falls apart if
> > userspace gets garbage when they try to size what appears to be a
> > traditional BAR, but is actually a BAR equivalent.
> > 
> > We could simply round up the size in vfio to make it naturally
> > aligned, but then we're imposing artificial sizes to the user and we
> > have the discontinuity that BAR size emulation and vfio region size
> > reporting don't agree on the size.  I think what we want to do is
> > expose EA to the user, reporting traditional BARs with BEIs as
> > zero-sized and providing additional regions for the user to access
> > each EA region, whether it has a BEI or not.
> > 
> > To facilitate that, a flag indicating whether a PCI resource is a
> > traditional BAR or BAR equivalent seems much nicer than attempting
> > to size the BAR ourselves or deducing it through the EA capability.
> > 
> > Thoughts?
> > 
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > ---
> 
> Feel free to rewrite this changelog and tailor it to the use case
> that's currently driving the change.  Add a sign-off for yourself too
> since the patch is passing through your hands.  Thanks,

Will do. I noticed it didn't fit, but didn't want to change it was in your name.
I'll rewrite the commit message to explain the current use-case.

Thanks,
Sean

> Alex
> 
> >  drivers/pci/pci.c      | 2 +-
> >  include/linux/ioport.h | 2 ++
> >  2 files changed, 3 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..5acc194 100644
> > --- a/include/linux/ioport.h
> > +++ b/include/linux/ioport.h
> > @@ -105,6 +105,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)			\
> 

[PATCH 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.


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 | 2 ++
 2 files changed, 3 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 1/2] pci: Identify Enhanced Allocation (EA) BAR Equivalent resources

[Sean O. Stalley]

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

We've done a pretty good job of abstracting EA from drivers,
but there are some cases where flaging resources as EA would be
useful. Specifically, EA is flexabable when it comes to resource
alignment & sizing. This can create issues when existing software
makes assumptions about the PCI resources.

For example, lspci assumes any PCI resource exposed by the kernel
that isn't visible in standard configspace comes from a VF BAR.
vfio makes assumptions about alignment and sizing, and runs into
problems when attempting to emulate a BAR Equivalent EA resource.

To facilitate that, a flag indicating whether a PCI resource is a
traditional BAR or BAR equivalent seems much nicer than attempting
to deduce where the resource came from from the size & address fields
(which is impossible if a EA resource happens naturally aligned).

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 | 2 ++
 2 files changed, 3 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..5acc194 100644
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h
@@ -105,6 +105,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 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 0/2] pci/lspci: Identify Enhanced Allocation (EA) Resources

[Sean O. Stalley]

Sorry, I forgot to increment the patch version.

This is V2. The only difference from V1 is the commit message on 1/2.
No functional changes.

-Sean

On Fri, Jan 22, 2016 at 02:04:52PM -0800, Sean O. Stalley wrote:
> 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.
> 
> 
> 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 | 2 ++
>  2 files changed, 3 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
> 

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

[Bjorn Helgaas]

On Fri, Jan 22, 2016 at 02:04:53PM -0800, Sean O. Stalley wrote:
> From: Alex Williamson <alex.williamson@redhat.com>
> 
> We've done a pretty good job of abstracting EA from drivers,
> but there are some cases where flaging resources as EA would be
> useful. Specifically, EA is flexabable when it comes to resource

Heh, "flexabable" :)

> alignment & sizing. This can create issues when existing software
> makes assumptions about the PCI resources.
> 
> For example, lspci assumes any PCI resource exposed by the kernel
> that isn't visible in standard configspace comes from a VF BAR.
> vfio makes assumptions about alignment and sizing, and runs into
> problems when attempting to emulate a BAR Equivalent EA resource.
> 
> To facilitate that, a flag indicating whether a PCI resource is a
> traditional BAR or BAR equivalent seems much nicer than attempting
> to deduce where the resource came from from the size & address fields
> (which is impossible if a EA resource happens naturally aligned).
> 
> 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 | 2 ++
>  2 files changed, 3 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..5acc194 100644
> --- a/include/linux/ioport.h
> +++ b/include/linux/ioport.h
> @@ -105,6 +105,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)

Hmmm, I didn't realize until I read the lspci patch that the resource
flags are exposed via sysfs (I think in resource_show()) and are thus
part of the sysfs ABI.  It would be nice to have a comment to that
effect here in ioport.h so we don't move bits and inadvertently break
something.

The changelog should also connect the dots a little bit more, e.g.,
mention the sysfs file affected by this patch, and that this currently
only makes lspci print slightly more correct output.

>  /* helpers to define resources */
>  #define DEFINE_RES_NAMED(_start, _size, _name, _flags)			\
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html