On Wed, May 18, 2016 at 11:03:39AM +0200, Alexander Gordeev wrote:
> On Fri, Apr 22, 2016 at 05:35:16PM +0200, Andrew Jones wrote:
> > On Mon, Apr 11, 2016 at 01:04:22PM +0200, Alexander Gordeev wrote:
> > > Cc: Thomas Huth <thuth@xxxxxxxxxx>
> > > Cc: Andrew Jones <drjones@xxxxxxxxxx>
> > > Signed-off-by: Alexander Gordeev <agordeev@xxxxxxxxxx>
> > > ---
> > > lib/asm-generic/pci.h | 6 +++++
> > > lib/pci.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > > lib/pci.h | 3 +++
> > > 3 files changed, 76 insertions(+)
> > >
> > > diff --git a/lib/asm-generic/pci.h b/lib/asm-generic/pci.h
> > > index 15f23079f27e..3f2c6913f0d4 100644
> > > --- a/lib/asm-generic/pci.h
> > > +++ b/lib/asm-generic/pci.h
> > > @@ -22,4 +22,10 @@ phys_addr_t pci_xlate_addr(pcidevaddr_t __unused dev, uint64_t addr)
> > > }
> > > #endif
> > >
> > > +#ifndef pci_print_arch
> > > +static inline void pci_print_arch(void)
> > > +{
> > > +}
> > > +#endif
> >
> > What's this function for? I think pci_print() should be enough. Or is
> > this for the host bridge? If so, then there should be a non-arch
> > specific pci_host_bridge_print() call written that does everything it
> > can with arch-neutral code, and just use arch calls as needed.
>
> It is not only for the host bridge. My intention is printing any PCI
> specifics that an architecture may have. So in case of the current ARM
> and PPC it would be the host bridge. In general case there could be few
> host bridges or irregular PCI controller or something like that.
>
> Currently ARM pci_print_arch() will print PCI memory space layout.
> That is quite auxiliary info and frankly, pci_print_arch() is not
> necessary at all. Up to you.
Let's drop it for now.
>
> > > +
> > > #endif
> > > diff --git a/lib/pci.c b/lib/pci.c
> > > index 46aee60e0f90..a3c680670fe0 100644
> > > --- a/lib/pci.c
> > > +++ b/lib/pci.c
> > > @@ -116,3 +116,70 @@ bool pci_bar_is64(pcidevaddr_t dev, int bar_num)
> > > else
> > > return false;
> > > }
> > > +
> > > +void pci_type_desc(int type, char *desc, int len)
> > > +{
> > > + if ((type & PCI_BASE_ADDRESS_SPACE) == PCI_BASE_ADDRESS_SPACE_IO) {
> > > + strcpy(desc, "PIO"); /* strncpy() would be better */
> > > + } else {
> > > + static char *str[] = { "32", "1M", "64" };
> > > + int idx = (type & PCI_BASE_ADDRESS_MEM_TYPE_MASK) >> 1;
> > > + int pfetch = type & PCI_BASE_ADDRESS_MEM_PREFETCH;
> > > + snprintf(desc, len, "MEM%s%s", str[idx], pfetch ? "/p" : "");
> > > + }
> > > +}
> >
> > What else, besides pci_dev_print, would call this function? If only one
> > function needs to output these descs, then we don't need the strcpy,
> > snprintf stuff, we just need printf at the call site.
> >
> > Oh, I see. It's because desc shows up in two printfs below. Well, I'd
> > still just repeat the printfs, or maybe create a macro.
>
> Also, a follow-up patch would use pci_type_desc() to describe host
> bridge's memory regions.
OK, but rather than using snprintf, you could just use an enum/switch,
allowing you to return, e.g. "PIO", "MEM32/p", etc. I think that would
be cleaner.
>
> > > +static void pci_dev_print(pcidevaddr_t dev)
> > > +{
> > > + uint16_t vendor_id = pci_config_readw(dev, PCI_VENDOR_ID);
> > > + uint16_t device_id = pci_config_readw(dev, PCI_DEVICE_ID);
> > > + uint8_t header = pci_config_readb(dev, PCI_HEADER_TYPE);
> > > + uint8_t progif = pci_config_readb(dev, PCI_CLASS_PROG);
> > > + uint8_t subclass = pci_config_readb(dev, PCI_CLASS_DEVICE);
> > > + uint8_t class = pci_config_readb(dev, PCI_CLASS_DEVICE + 1);
> > > + int bar;
> > > +
> > > + printf("dev %2d fn %d vendor_id %04x device_id %04x type %02x "
> > > + "progif %02x class %02x subclass %02x\n",
> > > + dev / 8, dev % 8, vendor_id, device_id, header,
> > > + progif, class, subclass);
> > > +
> > > + if (header != PCI_HEADER_TYPE_NORMAL)
> > > + return;
> > > +
> > > + for (bar = 0; bar < 6; bar++) {
> > > + phys_addr_t start, end;
> > > + char desc[8];
> > > +
> > > + if (!pci_bar_is_valid(dev, bar))
> > > + break;
> > > +
> > > + start = pci_bar_addr(dev, bar);
> > > + end = start + pci_bar_size(dev, bar) - 1;
> > > +
> > > + pci_type_desc(bar, desc, sizeof(desc));
> > > +
> > > + if (pci_bar_is64(dev, bar)) {
> > > + printf("\tBAR#%d,%d [%-7s %" PRIx64 "-%" PRIx64 "]\n",
> > > + bar, bar + 1, desc, start, end);
> > > + bar++;
> > > + } else {
> > > + printf("\tBAR#%d [%-7s %02x-%02x]\n",
> > > + bar, desc, (uint32_t)start, (uint32_t)end);
> > > + }
> > > + }
> > > +}
> > > +
> > > +void pci_print(void)
> > > +{
> > > + pcidevaddr_t dev;
> > > +
> > > + pci_print_arch();
> > > +
> > > + for (dev = 0; dev < 256; ++dev) {
> > > + if (pci_config_readw(dev, PCI_VENDOR_ID) != (uint16_t)~0 &&
> > > + pci_config_readw(dev, PCI_DEVICE_ID) != (uint16_t)~0) {
> >
> > You could create a pci_dev_valid() function that checks vendor/device
> > id.
>
> What about pci_dev_exists() instead of pci_dev_valid()?
Whatever terminology matches the spec.
>
> > > + pci_dev_print(dev);
> > > + }
> >
> > nit: no need for {} with a single line.
> >
> > > + }
> > > +}
> > > diff --git a/lib/pci.h b/lib/pci.h
> > > index 69d2a62f1b32..36dd67e19838 100644
> > > --- a/lib/pci.h
> > > +++ b/lib/pci.h
> > > @@ -22,6 +22,9 @@ bool pci_bar_is64(pcidevaddr_t dev, int bar_num);
> > > bool pci_bar_is_memory(pcidevaddr_t dev, int bar_num);
> > > bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num);
> > >
> > > +void pci_type_desc(int type, char *desc, int len);
> > > +void pci_print(void);
> > > +
> > > /*
> > > * pci-testdev is a driver for the pci-testdev qemu pci device. The
> > > * device enables testing mmio and portio exits, and measuring their
> > > --
> > > 1.8.3.1
> > >
> >
> > Thanks,
> > drew
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
