Re: [Qemu-devel] [PATCH v2 5/5] libqos: Add virtio MMIO support

["Marc Marí" <marc.mari.barcelo@gmail.com>]

El Mon, 17 Nov 2014 15:48:09 +0000
Stefan Hajnoczi <stefanha@gmail.com> escribió:
> On Sat, Nov 01, 2014 at 06:02:30PM +0100, Marc Marí wrote:
> 
> > +static void mmio_basic(void)
> > +{
> > +    QVirtioMMIODevice *dev;
> > +    QVirtQueue *vq;
> > +    QGuestAllocator *alloc;
> > +    QVirtioBlkReq req;
> > +    int n_size = 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 = qvirtio_mmio_init_device(MMIO_DEV_BASE_ADDR,
> > MMIO_PAGE_SIZE);
> > +    g_assert(dev != NULL);
> > +    g_assert_cmphex(dev->vdev.device_type, ==,
> > 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 = qvirtio_config_readq(&qvirtio_mmio, &dev->vdev,
> > +
> > QVIRTIO_MMIO_DEVICE_SPECIFIC);
> > +    g_assert_cmpint(capacity, ==, TEST_IMAGE_SIZE / 512);
> > +
> > +    features = qvirtio_get_features(&qvirtio_mmio, &dev->vdev);
> > +    features = features & ~(QVIRTIO_F_RING_INDIRECT_DESC |
> > +                                QVIRTIO_F_RING_EVENT_IDX |
> > QVIRTIO_BLK_F_SCSI);
> > +    qvirtio_set_features(&qvirtio_mmio, &dev->vdev, features);
> > +
> > +    alloc = generic_alloc_init(MMIO_RAM_ADDR, MMIO_RAM_SIZE,
> > MMIO_PAGE_SIZE);
> > +    vq = 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 = qvirtio_config_readq(&qvirtio_mmio, &dev->vdev,
> > +
> > QVIRTIO_MMIO_DEVICE_SPECIFIC);
> > +    g_assert_cmpint(capacity, ==, n_size / 512);
> > +
> > +    /* Write request */
> > +    req.type = QVIRTIO_BLK_T_OUT;
> > +    req.ioprio = 1;
> > +    req.sector = 0;
> > +    req.data = g_malloc0(512);
> > +    strcpy(req.data, "TEST");
> > +
> > +    req_addr = virtio_blk_request(alloc, &req, 512);
> > +
> > +    g_free(req.data);
> > +
> > +    free_head = 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 = readb(req_addr + 528);
> > +    g_assert_cmpint(status, ==, 0);
> > +
> > +    guest_free(alloc, req_addr);
> > +
> > +    /* Read request */
> > +    req.type = QVIRTIO_BLK_T_IN;
> > +    req.ioprio = 1;
> > +    req.sector = 0;
> > +    req.data = g_malloc0(512);
> > +
> > +    req_addr = virtio_blk_request(alloc, &req, 512);
> > +
> > +    g_free(req.data);
> > +
> > +    free_head = 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 = readb(req_addr + 528);
> > +    g_assert_cmpint(status, ==, 0);
> > +
> > +    data = g_malloc0(512);
> > +    memread(req_addr + 16, data, 512);
> > +    g_assert_cmpstr(data, ==, "TEST");
> > +    g_free(data);
> 
> There is a lot of code duplication here.  Can the test logic but
> shared between PCI and MMIO?

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.

By now, I will extract just write - read operation. If you think that
adding abstraction for arch specific offsets is worth the effort, then
I'll add them.

Marc