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.
> > +
> > #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.
> > +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()?
> > + 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
