Re: PATCH: display MSI-X table and pba offsets from "pciconf -c"

Hi Jim,

On Thu, Jan 31, 2013 at 3:13 PM, Jim Harris <jim.harris_at_gmail.com> wrote:
>
>
> On Thu, Jan 31, 2013 at 3:52 PM, Neel Natu <neelnatu_at_gmail.com> wrote:
>>
>> Hi,
>>
>> The following patch teaches pciconf(8) to display the table and pba
>> offsets when it displays the MSI-X capability.
>>
>> The new output format will look like:
>>
>> cap 11[70] = MSI-X supports 10 messages in map 0x1c[0x0][0x2000] enabled
>> OR
>> cap 11[70] = MSI-X supports 10 messages in maps 0x10[0x0] and
>> 0x14[0x1000] enabled
>>
>> Any objections to committing the patch?
>
>
> Functionally I think this is a good addition.  More information from pciconf
> the better.
>
> Other comments below.
>
>>
>> best
>> Neel
>>
>> Index: usr.sbin/pciconf/cap.c
>> ===================================================================
>> --- cap.c       (revision 246087)
>> +++ cap.c       (working copy)
>> _at__at_ -449,22 +449,30 _at__at_
>>  static void
>>  cap_msix(int fd, struct pci_conf *p, uint8_t ptr)
>>  {
>> -       uint32_t val;
>> +       uint32_t val, table_offset, pba_offset;
>
>
> Variables should be in alphabetical order.
>

Ok.

>>
>>         uint16_t ctrl;
>>         int msgnum, table_bar, pba_bar;
>>
>>         ctrl = read_config(fd, &p->pc_sel, ptr + PCIR_MSIX_CTRL, 2);
>>         msgnum = (ctrl & PCIM_MSIXCTRL_TABLE_SIZE) + 1;
>> +
>
>
> Newline added intentionally?
>

Yes.

>>
>>         val = read_config(fd, &p->pc_sel, ptr + PCIR_MSIX_TABLE, 4);
>>         table_bar = PCIR_BAR(val & PCIM_MSIX_BIR_MASK);
>> +       table_offset = val & ~PCIM_MSIX_BIR_MASK;
>> +
>>         val = read_config(fd, &p->pc_sel, ptr + PCIR_MSIX_PBA, 4);
>> -       pba_bar = PCIR_BAR(val & PCIM_MSIX_BIR_MASK);
>> +       pba_bar = PCIR_BAR(val & PCIM_MSIX_BIR_MASK);
>
>
> Looks like these two lines only have whitespace difference.
>

Yes, there was trailing whitespace there previously.

>>
>> +       pba_offset = val & ~PCIM_MSIX_BIR_MASK;
>> +
>>         printf("MSI-X supports %d message%s ", msgnum,
>>             (msgnum == 1) ? "" : "s");
>> -       if (table_bar == pba_bar)
>> -               printf("in map 0x%x", table_bar);
>> -       else
>> -               printf("in maps 0x%x and 0x%x", table_bar, pba_bar);
>> +       if (table_bar == pba_bar) {
>> +               printf("in map 0x%x[0x%x][0x%x]",
>> +                       table_bar, table_offset, pba_offset);
>> +       } else {
>> +               printf("in maps 0x%x[0x%x] and 0x%x[0x%x]",
>> +                       table_bar, table_offset, pba_bar, pba_offset);
>> +       }
>
>
> Seems like at least for the case where the table and pba are behind
> different BARs, this will exceed 80 characters.  So maybe the maps always go
> on a new line?  Similar to what's done at the end of cap_express for
> printing the link speed.
>

Ok, the new format will look like:

cap 11[70] = MSI-X supports 10 messages, enabled
                     Table in map 0x1c[0x0], PBA in map 0x1c[0x2000]

Updated patch at the end of the email.

best
Neel

>>
>>         if (ctrl & PCIM_MSIXCTRL_MSIX_ENABLE)
>>                 printf(" enabled");
>>  }


Index: usr.sbin/pciconf/cap.c
===================================================================
--- usr.sbin/pciconf/cap.c	(revision 246087)
+++ usr.sbin/pciconf/cap.c	(working copy)
_at__at_ -449,24 +449,28 _at__at_
 static void
 cap_msix(int fd, struct pci_conf *p, uint8_t ptr)
 {
-	uint32_t val;
+	uint32_t pba_offset, table_offset, val;
+	int msgnum, pba_bar, table_bar;
 	uint16_t ctrl;
-	int msgnum, table_bar, pba_bar;

 	ctrl = read_config(fd, &p->pc_sel, ptr + PCIR_MSIX_CTRL, 2);
 	msgnum = (ctrl & PCIM_MSIXCTRL_TABLE_SIZE) + 1;
+
 	val = read_config(fd, &p->pc_sel, ptr + PCIR_MSIX_TABLE, 4);
 	table_bar = PCIR_BAR(val & PCIM_MSIX_BIR_MASK);
+	table_offset = val & ~PCIM_MSIX_BIR_MASK;
+
 	val = read_config(fd, &p->pc_sel, ptr + PCIR_MSIX_PBA, 4);
-	pba_bar = PCIR_BAR(val & PCIM_MSIX_BIR_MASK);	
-	printf("MSI-X supports %d message%s ", msgnum,
-	    (msgnum == 1) ? "" : "s");
-	if (table_bar == pba_bar)
-		printf("in map 0x%x", table_bar);
-	else
-		printf("in maps 0x%x and 0x%x", table_bar, pba_bar);
-	if (ctrl & PCIM_MSIXCTRL_MSIX_ENABLE)
-		printf(" enabled");
+	pba_bar = PCIR_BAR(val & PCIM_MSIX_BIR_MASK);
+	pba_offset = val & ~PCIM_MSIX_BIR_MASK;
+
+	printf("MSI-X supports %d message%s%s\n", msgnum,
+	    (msgnum == 1) ? "" : "s",
+	    (ctrl & PCIM_MSIXCTRL_MSIX_ENABLE) ? ", enabled" : "");
+
+	printf("                 ");
+	printf("Table in map 0x%x[0x%x], PBA in map 0x%x[0x%x]",
+	    table_bar, table_offset, pba_bar, pba_offset);
 }

 static void