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

[Thomas Huth <thuth@redhat.com>]

On 14.04.2016 10:34, Alexander Gordeev wrote:
> On Thu, Apr 14, 2016 at 09:43:00AM +0200, Thomas Huth wrote:
>> On 11.04.2016 13:04, 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
>>> +
>>>  #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 */
>>
>> Maybe that's a good point in time now to introduce strncpy to
>> kvm-unit-tests? (or maybe even a better variant like strlcpy?)
> 
> Andrew?
> 
>>> +	} else {
>>> +		static char *str[] = { "32", "1M", "64" };
>>
>> Since you're depending on external values as array index below, I'd play
>> safe here and add a fourth entry like "??" or "RES" or so.
> 
> As I got from previous Andrew's explanations we would rather crash
> in case of incorrect access.

Well, but then please rather "crash" with an assert statement or
something similar. Otherwise, it depends on a random value that resides
there in memory. And at least on powerpc, we do not use the MMU (yet),
so this would likely end up in a very random, unreadable string, instead
of an immediate crash.

>>> +		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" : "");
>>> +	}
>>> +}
>>> +
>>> +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);
>>
>> I think you should mask away the uppermost bit of the header type byte -
>> otherwise your code does not work with multi-function devices.
> 
> See below.
> 
>>> +	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;
> 
> 	if ((header & 0x7f) != PCI_HEADER_TYPE_NORMAL)	/* mask multi-function bit */
> 		return;
> 
> I would rather masked it here.
> ?

That's fine for me, too.

 Thomas