diff mbox

Add lspci support for Enhanced Allocation Capability.

Message ID 1449872845-3596-1-git-send-email-ddaney.cavm@gmail.com
State Not Applicable
Headers show

Commit Message

David Daney Dec. 11, 2015, 10:27 p.m. UTC
From: David Daney <david.daney@cavium.com>

The PCISIG recently added the Enhanced Allocation Capability.  Decode
it in lspci.

---
The specification is currently located here:

https://pcisig.com/sites/default/files/specification_documents/ECN_Enhanced_Allocation_23_Oct_2014_Final.pdf

Bjorn Helgaas recently merged Linux kernel support for EA, and Cavium
ThunderX processors have implemented it.  This patch gives us a pretty
view of the capability structure.

Thanks,
David Daney


 lib/header.h |   1 +
 ls-caps.c    | 142 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 143 insertions(+)

Comments

Sean O. Stalley Dec. 12, 2015, 1:13 a.m. UTC | #1
Thanks for doing this. I tried it out & everything worked well for me.
Only a couple things:

The memory regions described by EA also show up before the EA Capability
and appear as virtual, ex:
	Region 0: [virtual] Memory at 80000000 ...
I don't think we want these regions to be flagged as virtual.
I think this happens because lspci can't read/write to the BAR region,
and so it assumes it's looking at a VF.

The rest of my gripes are minor syntax stuff. see inline.

Thanks again,
Sean

On Fri, Dec 11, 2015 at 02:27:25PM -0800, David Daney wrote:
> From: David Daney <david.daney@cavium.com>
> 
> The PCISIG recently added the Enhanced Allocation Capability.  Decode
> it in lspci.
> 
> ---
> The specification is currently located here:
> 
> https://pcisig.com/sites/default/files/specification_documents/ECN_Enhanced_Allocation_23_Oct_2014_Final.pdf
> 
> Bjorn Helgaas recently merged Linux kernel support for EA, and Cavium
> ThunderX processors have implemented it.  This patch gives us a pretty
> view of the capability structure.
> 
> Thanks,
> David Daney
> 
> 
>  lib/header.h |   1 +
>  ls-caps.c    | 142 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 143 insertions(+)
> 
> diff --git a/lib/header.h b/lib/header.h
> index f7cdee7..8c80e45 100644
> --- a/lib/header.h
> +++ b/lib/header.h
> @@ -203,6 +203,7 @@
>  #define  PCI_CAP_ID_MSIX	0x11	/* MSI-X */
>  #define  PCI_CAP_ID_SATA	0x12	/* Serial-ATA HBA */
>  #define  PCI_CAP_ID_AF		0x13	/* Advanced features of PCI devices integrated in PCIe root cplx */
> +#define  PCI_CAP_ID_EA		0x14	/* Enhanced Allocation */
>  #define PCI_CAP_LIST_NEXT	1	/* Next capability in the list */
>  #define PCI_CAP_FLAGS		2	/* Capability defined flags (16 bits) */
>  #define PCI_CAP_SIZEOF		4
> diff --git a/ls-caps.c b/ls-caps.c
> index c145ed6..dec47f4 100644
> --- a/ls-caps.c
> +++ b/ls-caps.c
> @@ -1254,6 +1254,145 @@ cap_sata_hba(struct device *d, int where, int cap)
>      printf(" BAR??%d\n", bar);
>  }
>  
> +static const char *cap_ea_property(unsigned p)
> +{
> +  switch (p) {
> +  case 0x00:
> +    return "memory space, non-prefetchable";
> +  case 0x01:
> +    return "memory space, prefetchable";
> +  case 0x02:
> +    return "I/O space";
> +  case 0x03:
> +    return "VF memory space, prefetchable";
> +  case 0x04:
> +    return "VF memory space, non-prefetchable";
> +  case 0x05:
> +    return "allocation behind bridge, non-prefetchable memory";
> +  case 0x06:
> +    return "allocation behind bridge, prefetchable memory";
> +  case 0x07:
> +    return "allocation behind bridge, I/O space";
> +  case 0xfd:
> +    return "memory space resource unavailable for use";
> +  case 0xfe:
> +    return "I/O space resource unavailable for use";
> +  case 0xff:
> +    return "entry unavailable for use";
> +  default:
> +    return "reserved";
> +  }
> +}
> +
> +static void cap_ea(struct device *d, int where, int cap)
> +{
> +  unsigned int entry;
> +  int entry_base = where + 4;
> +  unsigned int num_entries = BITS((unsigned int)cap, 0, 6);
> +  u8 htype = get_conf_byte(d, PCI_HEADER_TYPE) & 0x7f;
> +
> +  printf("Enhanced Allocation: Num Entries=%u", num_entries);
CamelCase.
> +  if (htype == PCI_HEADER_TYPE_BRIDGE) {
> +    byte fixed_sub, fixed_sec;
> +
> +    entry_base += 4;
> +    if (!config_fetch(d, where + 4, 2)) {
> +      printf("\n");
> +      return;
> +    }
> +    fixed_sec = get_conf_byte(d, where + 4);
> +    fixed_sub = get_conf_byte(d, where + 4);
> +    printf(", secondary=%d, subordinate=%d", fixed_sec, fixed_sub);
> +  }
> +  printf("\n");
> +  if (verbose < 2)
> +    return;
> +
> +  for (entry = 0; entry < num_entries; entry++) {
> +    int max_offset_high_pos, has_base_high, has_max_offset_high;
> +    u32 entry_header;
> +    u32 base, max_offset;
> +    unsigned int es, bei, pp, sp, e, w;
> +
> +    if (!config_fetch(d, entry_base, 4))
> +      return;
> +    entry_header = get_conf_long(d, entry_base);
> +    es = BITS(entry_header, 0, 3);
> +    bei = BITS(entry_header, 4, 4);
> +    pp = BITS(entry_header, 8, 8);
> +    sp = BITS(entry_header, 16, 8);
> +    w = BITS(entry_header, 30, 1);
> +    e = BITS(entry_header, 31, 1);
> +    if (!config_fetch(d, entry_base + 4, es * 4))
> +      return;
> +    printf("\t\tEntry-%u: Enable%c Writable%c, Entry Size=%u\n", entry,
I would change this line to:
	printf("\t\tEntry %u: Enable%c Writable%c, EntrySize=%u\n", ...);
	Changes:         ^                              ^ 
	"-" after Entry to a " " (to match the "Region %i" used for BARs)
	make EntrySize CamelCase (to match the other fields in lspci)
> +	   e ? '+': '-',
> +	   w ? '+': '-', es);
> +    printf("\t\t\t BAR Equivalent Indicator (BEI): ");
> +    switch (bei) {
> +    case 0:
> +    case 1:
> +    case 2:
> +    case 3:
> +    case 4:
> +    case 5:
> +      printf("BAR%u", bei);
> +      break;
> +    case 6:
> +      printf("Resource behind function");
> +      break;
> +    case 7:
> +      printf("Not Indicated");
> +      break;
> +    case 8:
> +      printf("Expansion ROM");
> +      break;
> +    case 9:
> +    case 10:
> +    case 11:
> +    case 12:
> +    case 13:
> +    case 14:
> +      printf("VF-BAR%u", bei - 9);
> +      break;
> +    default:
> +      printf("Reserved");
> +      break;
> +    }
> +    printf("\n");
> +    printf("\t\t\t Primary Properties (PP): %s\n", cap_ea_property(pp));
> +    printf("\t\t\t Secondary Properties (SP): %s\n", cap_ea_property(sp));
"PP" & "SP" are probably sufficient, I don't think we need to write the whole thing out.

Also, for the case where the Primary property is valid, can we hide the Secondary property if it is 0xFF?
My concern is that people may read the "entry unavailable for use" line as a warning and think something is wrong.
Maybe it would be better to make it say "Secondary Properties (SP): Not Present", or something similar.

> +
> +    base = get_conf_long(d, entry_base + 4);
> +    has_base_high = ((base & 2) != 0);
> +    base &= ~3;
> +
> +    max_offset = get_conf_long(d, entry_base + 8);
> +    has_max_offset_high = ((max_offset & 2) != 0);
> +    max_offset |= 3;
> +    max_offset_high_pos = entry_base + 12;
> +
> +    printf("\t\t\t Base: ");
> +    if (has_base_high) {
> +      u32 base_high = get_conf_long(d, entry_base + 12);
> +
> +      printf("%x", base_high);
> +      max_offset_high_pos += 4;
> +    }
> +    printf("%08x\n", base);
> +
> +    printf("\t\t\t Max Offset: ");
CamelCase.
> +    if (has_max_offset_high) {
> +      u32 max_offset_high = get_conf_long(d, max_offset_high_pos);
> +
> +      printf("%x", max_offset_high);
> +    }
> +    printf("%08x\n", max_offset);
> +
> +    entry_base += 4 + 4 * es;
> +  }
> +}
> +
>  void
>  show_caps(struct device *d, int where)
>  {
> @@ -1348,6 +1487,9 @@ show_caps(struct device *d, int where)
>  	    case PCI_CAP_ID_AF:
>  	      cap_af(d, where);
>  	      break;
> +	    case PCI_CAP_ID_EA:
> +	      cap_ea(d, where, cap);
> +	      break;
>  	    default:
>  	      printf("#%02x [%04x]\n", id, cap);
>  	    }
> -- 
> 1.8.3.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
--
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
David Daney Dec. 12, 2015, 1:18 a.m. UTC | #2
I sent a new v2 of the patch.  This version has a the wrong offset for 
the subordinate bus, and could make better use of the FLAG() macro etc.

David Daney


On 12/11/2015 02:27 PM, David Daney wrote:
> From: David Daney <david.daney@cavium.com>
>
> The PCISIG recently added the Enhanced Allocation Capability.  Decode
> it in lspci.
>
> ---
> The specification is currently located here:
>
> https://pcisig.com/sites/default/files/specification_documents/ECN_Enhanced_Allocation_23_Oct_2014_Final.pdf
>
> Bjorn Helgaas recently merged Linux kernel support for EA, and Cavium
> ThunderX processors have implemented it.  This patch gives us a pretty
> view of the capability structure.
>
> Thanks,
> David Daney
>
>
>   lib/header.h |   1 +
>   ls-caps.c    | 142 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 143 insertions(+)
>
> diff --git a/lib/header.h b/lib/header.h
> index f7cdee7..8c80e45 100644
> --- a/lib/header.h
> +++ b/lib/header.h
> @@ -203,6 +203,7 @@
>   #define  PCI_CAP_ID_MSIX	0x11	/* MSI-X */
>   #define  PCI_CAP_ID_SATA	0x12	/* Serial-ATA HBA */
>   #define  PCI_CAP_ID_AF		0x13	/* Advanced features of PCI devices integrated in PCIe root cplx */
> +#define  PCI_CAP_ID_EA		0x14	/* Enhanced Allocation */
>   #define PCI_CAP_LIST_NEXT	1	/* Next capability in the list */
>   #define PCI_CAP_FLAGS		2	/* Capability defined flags (16 bits) */
>   #define PCI_CAP_SIZEOF		4
> diff --git a/ls-caps.c b/ls-caps.c
> index c145ed6..dec47f4 100644
> --- a/ls-caps.c
> +++ b/ls-caps.c
> @@ -1254,6 +1254,145 @@ cap_sata_hba(struct device *d, int where, int cap)
>       printf(" BAR??%d\n", bar);
>   }
>
> +static const char *cap_ea_property(unsigned p)
> +{
> +  switch (p) {
> +  case 0x00:
> +    return "memory space, non-prefetchable";
> +  case 0x01:
> +    return "memory space, prefetchable";
> +  case 0x02:
> +    return "I/O space";
> +  case 0x03:
> +    return "VF memory space, prefetchable";
> +  case 0x04:
> +    return "VF memory space, non-prefetchable";
> +  case 0x05:
> +    return "allocation behind bridge, non-prefetchable memory";
> +  case 0x06:
> +    return "allocation behind bridge, prefetchable memory";
> +  case 0x07:
> +    return "allocation behind bridge, I/O space";
> +  case 0xfd:
> +    return "memory space resource unavailable for use";
> +  case 0xfe:
> +    return "I/O space resource unavailable for use";
> +  case 0xff:
> +    return "entry unavailable for use";
> +  default:
> +    return "reserved";
> +  }
> +}
> +
> +static void cap_ea(struct device *d, int where, int cap)
> +{
> +  unsigned int entry;
> +  int entry_base = where + 4;
> +  unsigned int num_entries = BITS((unsigned int)cap, 0, 6);
> +  u8 htype = get_conf_byte(d, PCI_HEADER_TYPE) & 0x7f;
> +
> +  printf("Enhanced Allocation: Num Entries=%u", num_entries);
> +  if (htype == PCI_HEADER_TYPE_BRIDGE) {
> +    byte fixed_sub, fixed_sec;
> +
> +    entry_base += 4;
> +    if (!config_fetch(d, where + 4, 2)) {
> +      printf("\n");
> +      return;
> +    }
> +    fixed_sec = get_conf_byte(d, where + 4);
> +    fixed_sub = get_conf_byte(d, where + 4);
> +    printf(", secondary=%d, subordinate=%d", fixed_sec, fixed_sub);
> +  }
> +  printf("\n");
> +  if (verbose < 2)
> +    return;
> +
> +  for (entry = 0; entry < num_entries; entry++) {
> +    int max_offset_high_pos, has_base_high, has_max_offset_high;
> +    u32 entry_header;
> +    u32 base, max_offset;
> +    unsigned int es, bei, pp, sp, e, w;
> +
> +    if (!config_fetch(d, entry_base, 4))
> +      return;
> +    entry_header = get_conf_long(d, entry_base);
> +    es = BITS(entry_header, 0, 3);
> +    bei = BITS(entry_header, 4, 4);
> +    pp = BITS(entry_header, 8, 8);
> +    sp = BITS(entry_header, 16, 8);
> +    w = BITS(entry_header, 30, 1);
> +    e = BITS(entry_header, 31, 1);
> +    if (!config_fetch(d, entry_base + 4, es * 4))
> +      return;
> +    printf("\t\tEntry-%u: Enable%c Writable%c, Entry Size=%u\n", entry,
> +	   e ? '+': '-',
> +	   w ? '+': '-', es);
> +    printf("\t\t\t BAR Equivalent Indicator (BEI): ");
> +    switch (bei) {
> +    case 0:
> +    case 1:
> +    case 2:
> +    case 3:
> +    case 4:
> +    case 5:
> +      printf("BAR%u", bei);
> +      break;
> +    case 6:
> +      printf("Resource behind function");
> +      break;
> +    case 7:
> +      printf("Not Indicated");
> +      break;
> +    case 8:
> +      printf("Expansion ROM");
> +      break;
> +    case 9:
> +    case 10:
> +    case 11:
> +    case 12:
> +    case 13:
> +    case 14:
> +      printf("VF-BAR%u", bei - 9);
> +      break;
> +    default:
> +      printf("Reserved");
> +      break;
> +    }
> +    printf("\n");
> +    printf("\t\t\t Primary Properties (PP): %s\n", cap_ea_property(pp));
> +    printf("\t\t\t Secondary Properties (SP): %s\n", cap_ea_property(sp));
> +
> +    base = get_conf_long(d, entry_base + 4);
> +    has_base_high = ((base & 2) != 0);
> +    base &= ~3;
> +
> +    max_offset = get_conf_long(d, entry_base + 8);
> +    has_max_offset_high = ((max_offset & 2) != 0);
> +    max_offset |= 3;
> +    max_offset_high_pos = entry_base + 12;
> +
> +    printf("\t\t\t Base: ");
> +    if (has_base_high) {
> +      u32 base_high = get_conf_long(d, entry_base + 12);
> +
> +      printf("%x", base_high);
> +      max_offset_high_pos += 4;
> +    }
> +    printf("%08x\n", base);
> +
> +    printf("\t\t\t Max Offset: ");
> +    if (has_max_offset_high) {
> +      u32 max_offset_high = get_conf_long(d, max_offset_high_pos);
> +
> +      printf("%x", max_offset_high);
> +    }
> +    printf("%08x\n", max_offset);
> +
> +    entry_base += 4 + 4 * es;
> +  }
> +}
> +
>   void
>   show_caps(struct device *d, int where)
>   {
> @@ -1348,6 +1487,9 @@ show_caps(struct device *d, int where)
>   	    case PCI_CAP_ID_AF:
>   	      cap_af(d, where);
>   	      break;
> +	    case PCI_CAP_ID_EA:
> +	      cap_ea(d, where, cap);
> +	      break;
>   	    default:
>   	      printf("#%02x [%04x]\n", id, cap);
>   	    }
>

--
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
David Daney Dec. 12, 2015, 1:30 a.m. UTC | #3
On 12/11/2015 05:13 PM, Sean O. Stalley wrote:
> Thanks for doing this. I tried it out & everything worked well for me.
> Only a couple things:
>
> The memory regions described by EA also show up before the EA Capability
> and appear as virtual, ex:
> 	Region 0: [virtual] Memory at 80000000 ...

We are just dumping out the config space.  lspci doesn't really know 
where the kernel gets the reported BAR values.

I don't think we want to suppress the dumping of the BARs, or the 
kernel's interpretation thereof.

> I don't think we want these regions to be flagged as virtual.
> I think this happens because lspci can't read/write to the BAR region,
> and so it assumes it's looking at a VF.
>
> The rest of my gripes are minor syntax stuff. see inline.
>
> Thanks again,
> Sean
>
> On Fri, Dec 11, 2015 at 02:27:25PM -0800, David Daney wrote:
>> From: David Daney <david.daney@cavium.com>
>>
[...]
>> --- a/ls-caps.c
>> +++ b/ls-caps.c
>> @@ -1254,6 +1254,145 @@ cap_sata_hba(struct device *d, int where, int cap)
>>       printf(" BAR??%d\n", bar);
>>   }
>>
[...]
>> +static void cap_ea(struct device *d, int where, int cap)
>> +{
>> +  unsigned int entry;
>> +  int entry_base = where + 4;
>> +  unsigned int num_entries = BITS((unsigned int)cap, 0, 6);
>> +  u8 htype = get_conf_byte(d, PCI_HEADER_TYPE) & 0x7f;
>> +
>> +  printf("Enhanced Allocation: Num Entries=%u", num_entries);
> CamelCase.

Those are the names from the specification, I would like to keep them in 
some form.  As noted below, it may be desirable to squash it to "NumEntries"


>> +  if (htype == PCI_HEADER_TYPE_BRIDGE) {
>> +    byte fixed_sub, fixed_sec;
>> +
>> +    entry_base += 4;
>> +    if (!config_fetch(d, where + 4, 2)) {
>> +      printf("\n");
>> +      return;
>> +    }
>> +    fixed_sec = get_conf_byte(d, where + 4);
>> +    fixed_sub = get_conf_byte(d, where + 4);
>> +    printf(", secondary=%d, subordinate=%d", fixed_sec, fixed_sub);
>> +  }
>> +  printf("\n");
>> +  if (verbose < 2)
>> +    return;
>> +
>> +  for (entry = 0; entry < num_entries; entry++) {
>> +    int max_offset_high_pos, has_base_high, has_max_offset_high;
>> +    u32 entry_header;
>> +    u32 base, max_offset;
>> +    unsigned int es, bei, pp, sp, e, w;
>> +
>> +    if (!config_fetch(d, entry_base, 4))
>> +      return;
>> +    entry_header = get_conf_long(d, entry_base);
>> +    es = BITS(entry_header, 0, 3);
>> +    bei = BITS(entry_header, 4, 4);
>> +    pp = BITS(entry_header, 8, 8);
>> +    sp = BITS(entry_header, 16, 8);
>> +    w = BITS(entry_header, 30, 1);
>> +    e = BITS(entry_header, 31, 1);
>> +    if (!config_fetch(d, entry_base + 4, es * 4))
>> +      return;
>> +    printf("\t\tEntry-%u: Enable%c Writable%c, Entry Size=%u\n", entry,
> I would change this line to:
> 	printf("\t\tEntry %u: Enable%c Writable%c, EntrySize=%u\n", ...);

We could do that.  I would let others decide if squashing "Entry Size" 
to "EntrySize" is desirable.


> 	Changes:         ^                              ^
> 	"-" after Entry to a " " (to match the "Region %i" used for BARs)
> 	make EntrySize CamelCase (to match the other fields in lspci)
[...]
>> +    printf("\n");
>> +    printf("\t\t\t Primary Properties (PP): %s\n", cap_ea_property(pp));
>> +    printf("\t\t\t Secondary Properties (SP): %s\n", cap_ea_property(sp));
> "PP" & "SP" are probably sufficient, I don't think we need to write the whole thing out.
>
> Also, for the case where the Primary property is valid, can we hide the Secondary property if it is 0xFF?
> My concern is that people may read the "entry unavailable for use" line as a warning and think something is wrong.
> Maybe it would be better to make it say "Secondary Properties (SP): Not Present", or something similar.
>

That is a good point.  I will try something like that.

>> +
>> +    base = get_conf_long(d, entry_base + 4);
>> +    has_base_high = ((base & 2) != 0);
>> +    base &= ~3;
>> +
>> +    max_offset = get_conf_long(d, entry_base + 8);
>> +    has_max_offset_high = ((max_offset & 2) != 0);
>> +    max_offset |= 3;
>> +    max_offset_high_pos = entry_base + 12;
>> +
>> +    printf("\t\t\t Base: ");
>> +    if (has_base_high) {
>> +      u32 base_high = get_conf_long(d, entry_base + 12);
>> +
>> +      printf("%x", base_high);
>> +      max_offset_high_pos += 4;
>> +    }
>> +    printf("%08x\n", base);
>> +
>> +    printf("\t\t\t Max Offset: ");
> CamelCase.

Again, I am trying to match the wording of the specification.  Although 
in this case I think I got it wrong.  It should probably be squashed to 
"MaxOffset'

[...]

Thanks for reviewing it.

David Daney

--
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
Sean O. Stalley Dec. 14, 2015, 7:07 p.m. UTC | #4
On Fri, Dec 11, 2015 at 05:30:30PM -0800, David Daney wrote:
> On 12/11/2015 05:13 PM, Sean O. Stalley wrote:
> >Thanks for doing this. I tried it out & everything worked well for me.
> >Only a couple things:
> >
> >The memory regions described by EA also show up before the EA Capability
> >and appear as virtual, ex:
> >	Region 0: [virtual] Memory at 80000000 ...
> 
> We are just dumping out the config space.  lspci doesn't really know
> where the kernel gets the reported BAR values.
> 
> I don't think we want to suppress the dumping of the BARs, or the
> kernel's interpretation thereof.

That's a good point. I guess we should keep dumping the BARs.
I still think we should change the [virtual] flag though.

lspci assumes that if the OS reports a region and configspace doesn't,
that the region came from an SR-IOV entry.

Before EA, this was a safe assumption. Now regions can come from EA entries,
we can't assume that a region with no BAR is always virtual.


Also, I just noticed that show_size() assumes power-of-2 sizes for regions.
It doesn't look like it will display some EA-supported sizes correctly
(Ex: 1110 bytes would get truncated and show up as " [size=1K]").

> >I don't think we want these regions to be flagged as virtual.
> >I think this happens because lspci can't read/write to the BAR region,
> >and so it assumes it's looking at a VF.
> >
> >The rest of my gripes are minor syntax stuff. see inline.
> >
> >Thanks again,
> >Sean
> >
> >On Fri, Dec 11, 2015 at 02:27:25PM -0800, David Daney wrote:
> >>From: David Daney <david.daney@cavium.com>
> >>
> [...]
> >>--- a/ls-caps.c
> >>+++ b/ls-caps.c
> >>@@ -1254,6 +1254,145 @@ cap_sata_hba(struct device *d, int where, int cap)
> >>      printf(" BAR??%d\n", bar);
> >>  }
> >>
> [...]
> >>+static void cap_ea(struct device *d, int where, int cap)
> >>+{
> >>+  unsigned int entry;
> >>+  int entry_base = where + 4;
> >>+  unsigned int num_entries = BITS((unsigned int)cap, 0, 6);
> >>+  u8 htype = get_conf_byte(d, PCI_HEADER_TYPE) & 0x7f;
> >>+
> >>+  printf("Enhanced Allocation: Num Entries=%u", num_entries);
> >CamelCase.
> 
> Those are the names from the specification, I would like to keep
> them in some form.  As noted below, it may be desirable to squash it
> to "NumEntries"

Agreed. see below.

> >>+  if (htype == PCI_HEADER_TYPE_BRIDGE) {
> >>+    byte fixed_sub, fixed_sec;
> >>+
> >>+    entry_base += 4;
> >>+    if (!config_fetch(d, where + 4, 2)) {
> >>+      printf("\n");
> >>+      return;
> >>+    }
> >>+    fixed_sec = get_conf_byte(d, where + 4);
> >>+    fixed_sub = get_conf_byte(d, where + 4);
> >>+    printf(", secondary=%d, subordinate=%d", fixed_sec, fixed_sub);
> >>+  }
> >>+  printf("\n");
> >>+  if (verbose < 2)
> >>+    return;
> >>+
> >>+  for (entry = 0; entry < num_entries; entry++) {
> >>+    int max_offset_high_pos, has_base_high, has_max_offset_high;
> >>+    u32 entry_header;
> >>+    u32 base, max_offset;
> >>+    unsigned int es, bei, pp, sp, e, w;
> >>+
> >>+    if (!config_fetch(d, entry_base, 4))
> >>+      return;
> >>+    entry_header = get_conf_long(d, entry_base);
> >>+    es = BITS(entry_header, 0, 3);
> >>+    bei = BITS(entry_header, 4, 4);
> >>+    pp = BITS(entry_header, 8, 8);
> >>+    sp = BITS(entry_header, 16, 8);
> >>+    w = BITS(entry_header, 30, 1);
> >>+    e = BITS(entry_header, 31, 1);
> >>+    if (!config_fetch(d, entry_base + 4, es * 4))
> >>+      return;
> >>+    printf("\t\tEntry-%u: Enable%c Writable%c, Entry Size=%u\n", entry,
> >I would change this line to:
> >	printf("\t\tEntry %u: Enable%c Writable%c, EntrySize=%u\n", ...);
> 
> We could do that.  I would let others decide if squashing "Entry
> Size" to "EntrySize" is desirable.

Sounds good. Lets see what others want.
I don't care too much either way,
I just thought CamelCase would make EA stuff slightly easier to grep.

> >	Changes:         ^                              ^
> >	"-" after Entry to a " " (to match the "Region %i" used for BARs)
> >	make EntrySize CamelCase (to match the other fields in lspci)
> [...]
> >>+    printf("\n");
> >>+    printf("\t\t\t Primary Properties (PP): %s\n", cap_ea_property(pp));
> >>+    printf("\t\t\t Secondary Properties (SP): %s\n", cap_ea_property(sp));
> >"PP" & "SP" are probably sufficient, I don't think we need to write the whole thing out.
> >
> >Also, for the case where the Primary property is valid, can we hide the Secondary property if it is 0xFF?
> >My concern is that people may read the "entry unavailable for use" line as a warning and think something is wrong.
> >Maybe it would be better to make it say "Secondary Properties (SP): Not Present", or something similar.
> >
> 
> That is a good point.  I will try something like that.
> 
> >>+
> >>+    base = get_conf_long(d, entry_base + 4);
> >>+    has_base_high = ((base & 2) != 0);
> >>+    base &= ~3;
> >>+
> >>+    max_offset = get_conf_long(d, entry_base + 8);
> >>+    has_max_offset_high = ((max_offset & 2) != 0);
> >>+    max_offset |= 3;
> >>+    max_offset_high_pos = entry_base + 12;
> >>+
> >>+    printf("\t\t\t Base: ");
> >>+    if (has_base_high) {
> >>+      u32 base_high = get_conf_long(d, entry_base + 12);
> >>+
> >>+      printf("%x", base_high);
> >>+      max_offset_high_pos += 4;
> >>+    }
> >>+    printf("%08x\n", base);
> >>+
> >>+    printf("\t\t\t Max Offset: ");
> >CamelCase.
> 
> Again, I am trying to match the wording of the specification.
> Although in this case I think I got it wrong.  It should probably be
> squashed to "MaxOffset'
> 
> [...]
> 
> Thanks for reviewing it.
> 
> David Daney
> 
--
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
diff mbox

Patch

diff --git a/lib/header.h b/lib/header.h
index f7cdee7..8c80e45 100644
--- a/lib/header.h
+++ b/lib/header.h
@@ -203,6 +203,7 @@ 
 #define  PCI_CAP_ID_MSIX	0x11	/* MSI-X */
 #define  PCI_CAP_ID_SATA	0x12	/* Serial-ATA HBA */
 #define  PCI_CAP_ID_AF		0x13	/* Advanced features of PCI devices integrated in PCIe root cplx */
+#define  PCI_CAP_ID_EA		0x14	/* Enhanced Allocation */
 #define PCI_CAP_LIST_NEXT	1	/* Next capability in the list */
 #define PCI_CAP_FLAGS		2	/* Capability defined flags (16 bits) */
 #define PCI_CAP_SIZEOF		4
diff --git a/ls-caps.c b/ls-caps.c
index c145ed6..dec47f4 100644
--- a/ls-caps.c
+++ b/ls-caps.c
@@ -1254,6 +1254,145 @@  cap_sata_hba(struct device *d, int where, int cap)
     printf(" BAR??%d\n", bar);
 }
 
+static const char *cap_ea_property(unsigned p)
+{
+  switch (p) {
+  case 0x00:
+    return "memory space, non-prefetchable";
+  case 0x01:
+    return "memory space, prefetchable";
+  case 0x02:
+    return "I/O space";
+  case 0x03:
+    return "VF memory space, prefetchable";
+  case 0x04:
+    return "VF memory space, non-prefetchable";
+  case 0x05:
+    return "allocation behind bridge, non-prefetchable memory";
+  case 0x06:
+    return "allocation behind bridge, prefetchable memory";
+  case 0x07:
+    return "allocation behind bridge, I/O space";
+  case 0xfd:
+    return "memory space resource unavailable for use";
+  case 0xfe:
+    return "I/O space resource unavailable for use";
+  case 0xff:
+    return "entry unavailable for use";
+  default:
+    return "reserved";
+  }
+}
+
+static void cap_ea(struct device *d, int where, int cap)
+{
+  unsigned int entry;
+  int entry_base = where + 4;
+  unsigned int num_entries = BITS((unsigned int)cap, 0, 6);
+  u8 htype = get_conf_byte(d, PCI_HEADER_TYPE) & 0x7f;
+
+  printf("Enhanced Allocation: Num Entries=%u", num_entries);
+  if (htype == PCI_HEADER_TYPE_BRIDGE) {
+    byte fixed_sub, fixed_sec;
+
+    entry_base += 4;
+    if (!config_fetch(d, where + 4, 2)) {
+      printf("\n");
+      return;
+    }
+    fixed_sec = get_conf_byte(d, where + 4);
+    fixed_sub = get_conf_byte(d, where + 4);
+    printf(", secondary=%d, subordinate=%d", fixed_sec, fixed_sub);
+  }
+  printf("\n");
+  if (verbose < 2)
+    return;
+
+  for (entry = 0; entry < num_entries; entry++) {
+    int max_offset_high_pos, has_base_high, has_max_offset_high;
+    u32 entry_header;
+    u32 base, max_offset;
+    unsigned int es, bei, pp, sp, e, w;
+
+    if (!config_fetch(d, entry_base, 4))
+      return;
+    entry_header = get_conf_long(d, entry_base);
+    es = BITS(entry_header, 0, 3);
+    bei = BITS(entry_header, 4, 4);
+    pp = BITS(entry_header, 8, 8);
+    sp = BITS(entry_header, 16, 8);
+    w = BITS(entry_header, 30, 1);
+    e = BITS(entry_header, 31, 1);
+    if (!config_fetch(d, entry_base + 4, es * 4))
+      return;
+    printf("\t\tEntry-%u: Enable%c Writable%c, Entry Size=%u\n", entry,
+	   e ? '+': '-',
+	   w ? '+': '-', es);
+    printf("\t\t\t BAR Equivalent Indicator (BEI): ");
+    switch (bei) {
+    case 0:
+    case 1:
+    case 2:
+    case 3:
+    case 4:
+    case 5:
+      printf("BAR%u", bei);
+      break;
+    case 6:
+      printf("Resource behind function");
+      break;
+    case 7:
+      printf("Not Indicated");
+      break;
+    case 8:
+      printf("Expansion ROM");
+      break;
+    case 9:
+    case 10:
+    case 11:
+    case 12:
+    case 13:
+    case 14:
+      printf("VF-BAR%u", bei - 9);
+      break;
+    default:
+      printf("Reserved");
+      break;
+    }
+    printf("\n");
+    printf("\t\t\t Primary Properties (PP): %s\n", cap_ea_property(pp));
+    printf("\t\t\t Secondary Properties (SP): %s\n", cap_ea_property(sp));
+
+    base = get_conf_long(d, entry_base + 4);
+    has_base_high = ((base & 2) != 0);
+    base &= ~3;
+
+    max_offset = get_conf_long(d, entry_base + 8);
+    has_max_offset_high = ((max_offset & 2) != 0);
+    max_offset |= 3;
+    max_offset_high_pos = entry_base + 12;
+
+    printf("\t\t\t Base: ");
+    if (has_base_high) {
+      u32 base_high = get_conf_long(d, entry_base + 12);
+
+      printf("%x", base_high);
+      max_offset_high_pos += 4;
+    }
+    printf("%08x\n", base);
+
+    printf("\t\t\t Max Offset: ");
+    if (has_max_offset_high) {
+      u32 max_offset_high = get_conf_long(d, max_offset_high_pos);
+
+      printf("%x", max_offset_high);
+    }
+    printf("%08x\n", max_offset);
+
+    entry_base += 4 + 4 * es;
+  }
+}
+
 void
 show_caps(struct device *d, int where)
 {
@@ -1348,6 +1487,9 @@  show_caps(struct device *d, int where)
 	    case PCI_CAP_ID_AF:
 	      cap_af(d, where);
 	      break;
+	    case PCI_CAP_ID_EA:
+	      cap_ea(d, where, cap);
+	      break;
 	    default:
 	      printf("#%02x [%04x]\n", id, cap);
 	    }