From mboxrd@z Thu Jan  1 00:00:00 1970
From: Peter Xu <peterx@redhat.com>
Subject: Re: [kvm-unit-tests PATCH v7 12/13] pci: Add pci-testdev PCI bus
 test device
Date: Fri, 23 Sep 2016 15:25:42 +0800
Message-ID: <20160923072542.GC15411@pxdev.xzpeter.org>
References: <cover.1471434672.git.agordeev@redhat.com>
 <15b9e7ffcf6f978c58e44e45502331949d61d19f.1471434672.git.agordeev@redhat.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8
Cc: kvm@vger.kernel.org, Thomas Huth <thuth@redhat.com>,
        Andrew Jones <drjones@redhat.com>
To: Alexander Gordeev <agordeev@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:50638 "EHLO mx1.redhat.com"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S965087AbcIWHZq (ORCPT <rfc822;kvm@vger.kernel.org>);
        Fri, 23 Sep 2016 03:25:46 -0400
Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22])
        (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
        (No client certificate requested)
        by mx1.redhat.com (Postfix) with ESMTPS id 836E03F1F1
        for <kvm@vger.kernel.org>; Fri, 23 Sep 2016 07:25:45 +0000 (UTC)
Content-Disposition: inline
In-Reply-To: <15b9e7ffcf6f978c58e44e45502331949d61d19f.1471434672.git.agordeev@redhat.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

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

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