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

On Thursday, January 31, 2013 7:37:48 pm Neel Natu wrote:
> 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.

Looks good to me.

-- 
John Baldwin