From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55853) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XssIp-0006wo-Rl for qemu-devel@nongnu.org; Mon, 24 Nov 2014 06:59:42 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XssIe-00072X-Sv for qemu-devel@nongnu.org; Mon, 24 Nov 2014 06:59:35 -0500 Received: from mx1.redhat.com ([209.132.183.28]:59297) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XssIe-00071v-LR for qemu-devel@nongnu.org; Mon, 24 Nov 2014 06:59:24 -0500 Date: Mon, 24 Nov 2014 11:59:19 +0000 From: Stefan Hajnoczi Message-ID: <20141124115919.GE4596@stefanha-thinkpad.redhat.com> References: <1414861350-13099-1-git-send-email-marc.mari.barcelo@gmail.com> <1414861350-13099-6-git-send-email-marc.mari.barcelo@gmail.com> <20141117154809.GI16192@stefanha-thinkpad.redhat.com> <20141123124125.7cbe61ea@crunchbang> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="4Epv4kl9IRBfg3rk" Content-Disposition: inline In-Reply-To: <20141123124125.7cbe61ea@crunchbang> Subject: Re: [Qemu-devel] [PATCH v2 5/5] libqos: Add virtio MMIO support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Marc =?iso-8859-1?Q?Mar=ED?= Cc: Stefan Hajnoczi , qemu-devel@nongnu.org, Andreas =?iso-8859-1?Q?F=E4rber?= --4Epv4kl9IRBfg3rk Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sun, Nov 23, 2014 at 12:41:25PM +0100, Marc Mar=ED wrote: > El Mon, 17 Nov 2014 15:48:09 +0000 > Stefan Hajnoczi escribi=F3: > > On Sat, Nov 01, 2014 at 06:02:30PM +0100, Marc Mar=ED wrote: > >=20 > > > +static void mmio_basic(void) > > > +{ > > > + QVirtioMMIODevice *dev; > > > + QVirtQueue *vq; > > > + QGuestAllocator *alloc; > > > + QVirtioBlkReq req; > > > + int n_size =3D TEST_IMAGE_SIZE / 2; > > > + uint64_t req_addr; > > > + uint64_t capacity; > > > + uint32_t features; > > > + uint32_t free_head; > > > + uint8_t status; > > > + char *data; > > > + > > > + arm_test_start(); > > > + > > > + dev =3D qvirtio_mmio_init_device(MMIO_DEV_BASE_ADDR, > > > MMIO_PAGE_SIZE); > > > + g_assert(dev !=3D NULL); > > > + g_assert_cmphex(dev->vdev.device_type, =3D=3D, > > > QVIRTIO_BLK_DEVICE_ID); + > > > + qvirtio_reset(&qvirtio_mmio, &dev->vdev); > > > + qvirtio_set_acknowledge(&qvirtio_mmio, &dev->vdev); > > > + qvirtio_set_driver(&qvirtio_mmio, &dev->vdev); > > > + > > > + capacity =3D qvirtio_config_readq(&qvirtio_mmio, &dev->vdev, > > > + > > > QVIRTIO_MMIO_DEVICE_SPECIFIC); > > > + g_assert_cmpint(capacity, =3D=3D, TEST_IMAGE_SIZE / 512); > > > + > > > + features =3D qvirtio_get_features(&qvirtio_mmio, &dev->vdev); > > > + features =3D features & ~(QVIRTIO_F_RING_INDIRECT_DESC | > > > + QVIRTIO_F_RING_EVENT_IDX | > > > QVIRTIO_BLK_F_SCSI); > > > + qvirtio_set_features(&qvirtio_mmio, &dev->vdev, features); > > > + > > > + alloc =3D generic_alloc_init(MMIO_RAM_ADDR, MMIO_RAM_SIZE, > > > MMIO_PAGE_SIZE); > > > + vq =3D qvirtqueue_setup(&qvirtio_mmio, &dev->vdev, alloc, 0); > > > + > > > + qvirtio_set_driver_ok(&qvirtio_mmio, &dev->vdev); > > > + > > > + qmp("{ 'execute': 'block_resize', 'arguments': { 'device': > > > 'drive0', " > > > + " 'size': > > > %d } }", n_size); + > > > + qvirtio_wait_queue_isr(&qvirtio_mmio, &dev->vdev, vq, > > > + QVIRTIO_BLK_TIMEOUT_US); > > > + > > > + capacity =3D qvirtio_config_readq(&qvirtio_mmio, &dev->vdev, > > > + > > > QVIRTIO_MMIO_DEVICE_SPECIFIC); > > > + g_assert_cmpint(capacity, =3D=3D, n_size / 512); > > > + > > > + /* Write request */ > > > + req.type =3D QVIRTIO_BLK_T_OUT; > > > + req.ioprio =3D 1; > > > + req.sector =3D 0; > > > + req.data =3D g_malloc0(512); > > > + strcpy(req.data, "TEST"); > > > + > > > + req_addr =3D virtio_blk_request(alloc, &req, 512); > > > + > > > + g_free(req.data); > > > + > > > + free_head =3D qvirtqueue_add(vq, req_addr, 528, false, true); > > > + qvirtqueue_add(vq, req_addr + 528, 1, true, false); > > > + qvirtqueue_kick(&qvirtio_mmio, &dev->vdev, vq, free_head); > > > + > > > + qvirtio_wait_queue_isr(&qvirtio_mmio, &dev->vdev, vq, > > > + QVIRTIO_BLK_TIMEOUT_US); > > > + status =3D readb(req_addr + 528); > > > + g_assert_cmpint(status, =3D=3D, 0); > > > + > > > + guest_free(alloc, req_addr); > > > + > > > + /* Read request */ > > > + req.type =3D QVIRTIO_BLK_T_IN; > > > + req.ioprio =3D 1; > > > + req.sector =3D 0; > > > + req.data =3D g_malloc0(512); > > > + > > > + req_addr =3D virtio_blk_request(alloc, &req, 512); > > > + > > > + g_free(req.data); > > > + > > > + free_head =3D qvirtqueue_add(vq, req_addr, 16, false, true); > > > + qvirtqueue_add(vq, req_addr + 16, 513, true, false); > > > + > > > + qvirtqueue_kick(&qvirtio_mmio, &dev->vdev, vq, free_head); > > > + > > > + qvirtio_wait_queue_isr(&qvirtio_mmio, &dev->vdev, vq, > > > + QVIRTIO_BLK_TIMEOUT_US); > > > + status =3D readb(req_addr + 528); > > > + g_assert_cmpint(status, =3D=3D, 0); > > > + > > > + data =3D g_malloc0(512); > > > + memread(req_addr + 16, data, 512); > > > + g_assert_cmpstr(data, =3D=3D, "TEST"); > > > + g_free(data); > >=20 > > There is a lot of code duplication here. Can the test logic but > > shared between PCI and MMIO? >=20 > The code duplication that can be easily extracted and shared is > performing a simple write - read operation on the block device (which > is used in various test cases). Other places (for example, checking the > image size) use arch specific offsets. This could be abstracted, but I > think is a bit too complicated for a test case. Virtio config space is not transport-specific. The Linux virtio_blk driver does: virtio_cread(vdev, struct virtio_blk_config, capacity, &capacity); Why does this patch use: capacity =3D qvirtio_config_readq(&qvirtio_mmio, &dev->vdev, QVIRTIO_MMIO_D= EVICE_SPECIFIC); ? I don't have the virtio spec open right now, but I guess qvirtio_config_readq() should always start at virtio configuration byte 0 and not rely on transport-specific offsets. Stefan --4Epv4kl9IRBfg3rk Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJUcx2XAAoJEJykq7OBq3PI4rMH/iO/OlmIDPmW7wID8Wln8RgA jMAjT4+fKCo3NTufzpVDQR4YW5KeO00tf4EGtBLY8/MLWUoXWsjl+G/oIUMZyOvn jVyqGtPQbjscylXU2+HfOfR+j4T95ylu7MJndOATNK7jOqD1s1F5p972xmwP32Cd 9yiiG1ltQhn0WDvJExWRUvhlCn1rM1tTVw2ccKaNwUm+3ZB7JZwPguiyeyMWIGxn OV/IJEzt8vA1/D4lGqg4XAxX/WNTF0Pql8nNoHVZto4kCEowNVNk5oH88Td0Uhk0 4hN/wl+GllZCK1UNP4Jvl63otI3zeLRh4yRGfXfcMHeDT6TptMUyMLbITV2OHls= =BU9g -----END PGP SIGNATURE----- --4Epv4kl9IRBfg3rk--