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@xxxxxxxxxx>
> >>
> [...]
> >>--- 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
