On Thu, Apr 14, 2016 at 10:41:20AM +0200, Thomas Huth wrote:
> 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@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
> >>> +
> >>> #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?
Fine by me, if needed. Typically we've been pretty safe using strcpy
in kvm-unit-tests, particularly when combined with some input sanity
test asserting. What's the concern about desc here? Would it make more
sense to check that concern with an assert?
> >
> >>> + } 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.
Yes. I prefer asserting for any unexpected lib api use, i.e. we should
sanity check with asserts all external inputs, after which we don't
need any error paths for those cases. But using asserts is necessary,
we can't just assume going off in the weeds will lead to an
understandable "crash".
Thanks,
drew
>
> >>> + 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
>
> --
> 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
