Re: [PATCH RFC 10/15] pci: Add pci_print() and pci_type_desc()

[Andrew Jones <drjones@redhat.com>]

On Mon, Apr 11, 2016 at 01:04:22PM +0200, Alexander Gordeev wrote:
> Cc: Thomas Huth <thuth@redhat.com>
> Cc: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> ---
>  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.

> +
>  #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.

> +
> +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.

> +			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