Re: [PATCH] Add lspci support for Enhanced Allocation Capability.

[David Daney <ddaney@caviumnetworks.com>]

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