On Fri, Sep 23, 2016 at 03:25:42PM +0800, Peter Xu wrote:
> Some nit-picks inline...
>
> (btw, this looks more like a test case shared by platforms rather than
> a library, so shall we move it outside of lib/? I don't know.)
We only have lib/ for shared code right now. I don't think we need
to add a new directory for shared testcode vs. libcode yet, as this
is the first case. If it becomes a more common thing to do, though,
then I agree that it may be nicer to have a new directory for it.
Thanks,
drew
>
> On Wed, Aug 17, 2016 at 02:07:13PM +0200, Alexander Gordeev wrote:
>
> [...]
>
> > +static bool pci_testdev_one(struct pci_test_dev_hdr *test,
> > + int test_nr,
> > + struct pci_testdev_ops *ops)
> > +{
> > + u8 width;
> > + u32 count, sig, off;
> > + const int nr_writes = 16;
> > + int i;
> > +
> > + ops->io_writeb(test_nr, &test->test);
> > + count = ops->io_readl(&test->count);
> > + if (count != 0)
> > + return false;
> > +
> > + width = ops->io_readb(&test->width);
> > + if (width != 1 && width != 2 && width != 4)
> > + return false;
>
> IIUC we only have 1?
>
> > +
> > + sig = ops->io_readl(&test->data);
> > + off = ops->io_readl(&test->offset);
> > +
> > + for (i = 0; i < nr_writes; i++) {
> > + switch (width) {
> > + case 1: ops->io_writeb(sig, (void *)test + off); break;
> > + case 2: ops->io_writew(sig, (void *)test + off); break;
> > + case 4: ops->io_writel(sig, (void *)test + off); break;
>
> Here as well. Could I ask why we are handling 2/4?
>
> [...]
>
> > +static int pci_testdev_all(struct pci_test_dev_hdr *test,
> > + struct pci_testdev_ops *ops)
> > +{
> > + int i;
> > +
> > + for (i = 0;; i++) {
> > + if (!pci_testdev_one(test, i, ops))
> > + break;
>
> Since we have defined PCI_TESTDEV_NUM_TESTS, shall we use it here to
> stop the loop rather than depending on a failure code from
> pci_testdev_one()?
>
> [...]
>
> > +int pci_testdev(void)
> > +{
> > + phys_addr_t addr;
> > + void __iomem *mem, *io;
> > + pcidevaddr_t dev;
> > + int nr_tests = 0;
> > + bool ret;
> > +
> > + dev = pci_find_dev(PCI_VENDOR_ID_REDHAT, PCI_DEVICE_ID_REDHAT_TEST);
> > + if (dev == PCIDEVADDR_INVALID) {
> > + printf("'pci-testdev' device is not found, "
> > + "check QEMU '-device pci-testdev' parameter\n");
> > + return -1;
> > + }
> > +
> > + ret = pci_bar_is_valid(dev, 0) && pci_bar_is_valid(dev, 1);
> > + assert(ret);
> > +
> > + addr = pci_bar_get_addr(dev, 0);
> > + mem = ioremap(addr, PAGE_SIZE);
> > +
> > + addr = pci_bar_get_addr(dev, 1);
> > + io = (void *)(unsigned long)addr;
>
> x86/vmexit.c is using pci-testdev as well. Maybe we can generalize the
> init part and share it? (Actually there is patch in my local tree for
> this, but haven't posted :)
>
> Thanks!
>
> -- peterx
> --
> 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
