Re: [PATCH v3] tests: qtest: Add virtio-iommu test

[Jean-Philippe Brucker]

Hi Eric,

On Thu, Oct 14, 2021 at 04:34:05AM -0400, Eric Auger wrote:
> Add the framework to test the virtio-iommu-pci device
> and tests exercising the attach/detach, map/unmap API.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Acked-by: Thomas Huth <thuth@redhat.com>
> 
> ---
> 
> This applies on top of jean-Philippe's
> [PATCH v4 00/11] virtio-iommu: Add ACPI support
> branch can be found at:
> https://github.com/eauger/qemu.git
> branch qtest-virtio-iommu-v3
> 
> To run the tests:
> make tests/qtest/qos-test
> cd build
> QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon 
> QTEST_QEMU_BINARY=x86_64-softmmu/qemu-system-x86_64  tests/qtest/qos-test

Looks like some archs cannot run the test:

$ make check    # built with all targets
qemu-system-arm: -device virtio-iommu-device: VIRTIO-IOMMU is not attached to 
any PCI bus!
Broken pipe
ERROR qtest-arm/qos-test - too few tests run (expected 80, got 75)

Also, should the test run on aarch64?


[...]
> diff --git a/tests/qtest/virtio-iommu-test.c b/tests/qtest/virtio-iommu-test.c
> new file mode 100644
> index 0000000000..ac4d38c779
> --- /dev/null
> +++ b/tests/qtest/virtio-iommu-test.c
> @@ -0,0 +1,299 @@
> +/*
> + * QTest testcase for VirtIO IOMMU
> + *
> + * Copyright (c) 2021 Red Hat, Inc.
> + *
> + * Authors:
> + *  Eric Auger <eric.auger@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or (at 
> your
> + * option) any later version.  See the COPYING file in the top-level 
> directory.
> + *
> + */
> +
> +#include "qemu/osdep.h"
> +#include "libqtest-single.h"
> +#include "qemu/module.h"
> +#include "libqos/qgraph.h"
> +#include "libqos/virtio-iommu.h"
> +#include "hw/virtio/virtio-iommu.h"
> +
> +#define PCI_SLOT_HP             0x06
> +#define QVIRTIO_IOMMU_TIMEOUT_US (30 * 1000 * 1000)
> +
> +static QGuestAllocator *alloc;
> +
> +static void pci_config(void *obj, void *data, QGuestAllocator *t_alloc)
> +{
> +    QVirtioIOMMU *v_iommu = obj;
> +    QVirtioDevice *dev = v_iommu->vdev;
> +    uint64_t input_range_start = qvirtio_config_readq(dev, 8);
> +    uint64_t input_range_end = qvirtio_config_readq(dev, 16);
> +    uint32_t domain_range_start = qvirtio_config_readl(dev, 24);
> +    uint32_t domain_range_end = qvirtio_config_readl(dev, 28);
> +
> +    g_assert_cmpint(input_range_start, ==, 0);
> +    g_assert_cmphex(input_range_end, ==, UINT64_MAX);
> +    g_assert_cmpint(domain_range_start, ==, 0);
> +    g_assert_cmpint(domain_range_end, ==, 32);

By the way, this value seems to be left from when the config declared a
number of domain bits. It's now a range so the value could be UINT32_MAX.
Right now the driver can't manage more than 32 endpoints at a time.
I have a patch changing that but planning to send later, it doesn't feel
urgent.

> +}
> +
> +/**
> + * send_attach_detach - Send an attach/detach command to the device
> + * @type: VIRTIO_IOMMU_T_ATTACH/VIRTIO_IOMMU_T_DETACH
> + * @domain: domain the end point is attached to
> + * @ep: end-point

("endpoint"?)

> + */
> +static int send_attach_detach(QTestState *qts, QVirtioIOMMU *v_iommu,
> +                              uint8_t type, uint32_t domain, uint32_t ep)
> +{
> +    QVirtioDevice *dev = v_iommu->vdev;
> +    QVirtQueue *vq = v_iommu->vq;
> +    uint64_t ro_addr, wr_addr;
> +    uint32_t free_head;
> +    struct virtio_iommu_req_attach req; /* same layout as detach */

Should the reserved fields be initialized to zero?

The test fails here with my recent bypass patch, which sanity-checks the
new flags field. But I could change the test in the bypass series, since
the test doesn't fail as is.

On a related note, the spec says that the device MUST reject the request
if field reserved is not zero. I can also send a patch for that.

> +    size_t ro_size = sizeof(req) - sizeof(struct virtio_iommu_req_tail);
> +    size_t wr_size = sizeof(struct virtio_iommu_req_tail);
> +    struct virtio_iommu_req_tail buffer;
> +    int ret;
> +
> +    req.head.type = type;
> +    req.domain = domain;
> +    req.endpoint = ep;

A driver would explicitly write little-endian in there with cpu_to_le*().
I guess that also matters here if the test could run on a big-endian host?

> +
> +    ro_addr = guest_alloc(alloc, ro_size);
> +    wr_addr = guest_alloc(alloc, wr_size);
> +
> +    qtest_memwrite(qts, ro_addr, &req, ro_size);
> +    free_head = qvirtqueue_add(qts, vq, ro_addr, ro_size, false, true);
> +    qvirtqueue_add(qts, vq, wr_addr, wr_size, true, false);
> +    qvirtqueue_kick(qts, dev, vq, free_head);
> +    qvirtio_wait_used_elem(qts, dev, vq, free_head, NULL,
> +                           QVIRTIO_IOMMU_TIMEOUT_US);
> +    qtest_memread(qts, wr_addr, &buffer, wr_size);
> +    ret = buffer.status;

Could check that the rest of the buffer is still 0

> +    guest_free(alloc, ro_addr);
> +    guest_free(alloc, wr_addr);
> +    return ret;
> +}
> +
> +/**
> + * send_map - Send a map command to the device
> + * @domain: domain the new binding is attached to

(what is the new binding?)

> + * @virt_start: iova start
> + * @virt_end: iova end
> + * @phys_start: base physical address
> + * @flags: mapping flags
> + */
> +static int send_map(QTestState *qts, QVirtioIOMMU *v_iommu,
> +                    uint32_t domain, uint64_t virt_start, uint64_t virt_end,
> +                    uint64_t phys_start, uint32_t flags)
> +{
> +    QVirtioDevice *dev = v_iommu->vdev;
> +    QVirtQueue *vq = v_iommu->vq;
> +    uint64_t ro_addr, wr_addr;
> +    uint32_t free_head;
> +    struct virtio_iommu_req_map req;
> +    size_t ro_size = sizeof(req) - sizeof(struct virtio_iommu_req_tail);
> +    size_t wr_size = sizeof(struct virtio_iommu_req_tail);
> +    struct virtio_iommu_req_tail buffer;
> +    int ret;
> +
> +    req.head.type = VIRTIO_IOMMU_T_MAP;
> +    req.domain = domain;
> +    req.virt_start = virt_start;
> +    req.virt_end = virt_end;
> +    req.phys_start = phys_start;
> +    req.flags = flags;
> +
> +    ro_addr = guest_alloc(alloc, ro_size);
> +    wr_addr = guest_alloc(alloc, wr_size);
> +
> +    qtest_memwrite(qts, ro_addr, &req, ro_size);
> +    free_head = qvirtqueue_add(qts, vq, ro_addr, ro_size, false, true);
> +    qvirtqueue_add(qts, vq, wr_addr, wr_size, true, false);
> +    qvirtqueue_kick(qts, dev, vq, free_head);
> +    qvirtio_wait_used_elem(qts, dev, vq, free_head, NULL,
> +                           QVIRTIO_IOMMU_TIMEOUT_US);
> +    qtest_memread(qts, wr_addr, &buffer, wr_size);
> +    ret = buffer.status;
> +    guest_free(alloc, ro_addr);
> +    guest_free(alloc, wr_addr);
> +    return ret;
> +}
> +
> +/**
> + * send_unmap - Send an unmap command to the device
> + * @domain: domain the new binding is attached to
> + * @virt_start: iova start
> + * @virt_end: iova end
> + */
> +static int send_unmap(QTestState *qts, QVirtioIOMMU *v_iommu,
> +                      uint32_t domain, uint64_t virt_start, uint64_t 
> virt_end)
> +{
> +    QVirtioDevice *dev = v_iommu->vdev;
> +    QVirtQueue *vq = v_iommu->vq;
> +    uint64_t ro_addr, wr_addr;
> +    uint32_t free_head;
> +    struct virtio_iommu_req_unmap req;
> +    size_t ro_size = sizeof(req) - sizeof(struct virtio_iommu_req_tail);
> +    size_t wr_size = sizeof(struct virtio_iommu_req_tail);
> +    struct virtio_iommu_req_tail buffer;
> +    int ret;
> +
> +    req.head.type = VIRTIO_IOMMU_T_UNMAP;
> +    req.domain = domain;
> +    req.virt_start = virt_start;
> +    req.virt_end = virt_end;
> +
> +    ro_addr = guest_alloc(alloc, ro_size);
> +    wr_addr = guest_alloc(alloc, wr_size);
> +
> +    qtest_memwrite(qts, ro_addr, &req, ro_size);
> +    free_head = qvirtqueue_add(qts, vq, ro_addr, ro_size, false, true);
> +    qvirtqueue_add(qts, vq, wr_addr, wr_size, true, false);
> +    qvirtqueue_kick(qts, dev, vq, free_head);
> +    qvirtio_wait_used_elem(qts, dev, vq, free_head, NULL,
> +                           QVIRTIO_IOMMU_TIMEOUT_US);
> +    qtest_memread(qts, wr_addr, &buffer, wr_size);
> +    ret = buffer.status;
> +    guest_free(alloc, ro_addr);
> +    guest_free(alloc, wr_addr);
> +    return ret;
> +}
> +
> +/* Test unmap scenari documented in the spec v0.12 */
> +static void test_attach_detach(void *obj, void *data, QGuestAllocator 
> *t_alloc)
> +{
> +    QVirtioIOMMU *v_iommu = obj;
> +    QTestState *qts = global_qtest;
> +    int ret;
> +
> +    alloc = t_alloc;
> +
> +    /* type, domain, ep */
> +
> +    /* attach ep0 to domain 0 */

By the way, what endpoint is this, the host bridge?  I'm still trying to
understand the test framework, how do we know that ep 0 exists and ep 1
doesn't?

> +    ret = send_attach_detach(qts, v_iommu, VIRTIO_IOMMU_T_ATTACH, 0, 0);
> +    g_assert_cmpint(ret, ==, 0);
> +
> +    /* attach a non existing device (1) */

(444)?

> +    ret = send_attach_detach(qts, v_iommu, VIRTIO_IOMMU_T_ATTACH, 0, 444);
> +    g_assert_cmpint(ret, ==, VIRTIO_IOMMU_S_NOENT);
> +
> +    /* detach a non existing device (1) */
> +    ret = send_attach_detach(qts, v_iommu, VIRTIO_IOMMU_T_DETACH, 0, 1);
> +    g_assert_cmpint(ret, ==, VIRTIO_IOMMU_S_NOENT);
> +
> +    /* move ep0 from domain 0 to domain 1 */
> +    ret = send_attach_detach(qts, v_iommu, VIRTIO_IOMMU_T_ATTACH, 1, 0);
> +    g_assert_cmpint(ret, ==, 0);
> +
> +    /* detach ep0 to domain 0 */

from

> +    ret = send_attach_detach(qts, v_iommu, VIRTIO_IOMMU_T_DETACH, 0, 0);
> +    g_assert_cmpint(ret, ==, VIRTIO_IOMMU_S_INVAL);
> +
> +    /* detach ep0 from domain 1 */
> +    ret = send_attach_detach(qts, v_iommu, VIRTIO_IOMMU_T_DETACH, 1, 0);
> +    g_assert_cmpint(ret, ==, 0);
> +
> +    ret = send_attach_detach(qts, v_iommu, VIRTIO_IOMMU_T_ATTACH, 1, 0);
> +    g_assert_cmpint(ret, ==, 0);
> +    ret = send_map(qts, v_iommu, 1, 0x0, 0xFFF, 0xa1000,
> +                   VIRTIO_IOMMU_MAP_F_READ);
> +    g_assert_cmpint(ret, ==, 0);

I was going to say that success here depends on the minimum page size
offered by the device (if the page size if 64k this request would fail),
but there is no check for page alignment at the moment.

It's just a SHOULD in the spec so we don't have to add the check, but it
may help with the VFIO integration - even though VFIO will fail unaligned
MAP ioctls, we report success to the driver. So I've added alignment
checks to my TODO list, but nothing urgent

> +    ret = send_map(qts, v_iommu, 1, 0x2000, 0x2FFF, 0xb1000,
> +                   VIRTIO_IOMMU_MAP_F_READ);
> +    g_assert_cmpint(ret, ==, 0);
> +    ret = send_attach_detach(qts, v_iommu, VIRTIO_IOMMU_T_DETACH, 1, 0);
> +    g_assert_cmpint(ret, ==, 0);
> +}
> +
> +static void test_map_unmap(void *obj, void *data, QGuestAllocator *t_alloc)
> +{
> +    QVirtioIOMMU *v_iommu = obj;
> +    QTestState *qts = global_qtest;
> +    int ret;
> +
> +    alloc = t_alloc;
> +
> +    /* attach ep0 to domain 1 */
> +    ret = send_attach_detach(qts, v_iommu, VIRTIO_IOMMU_T_ATTACH, 1, 0);
> +    g_assert_cmpint(ret, ==, 0);
> +
> +    ret = send_map(qts, v_iommu, 0, 0, 0xFFF, 0xa1000, 
> VIRTIO_IOMMU_MAP_F_READ);
> +    g_assert_cmpint(ret, ==, VIRTIO_IOMMU_S_NOENT);
> +
> +    /* domain, virt start, virt end, phys start, flags */
> +    ret = send_map(qts, v_iommu, 1, 0, 0xFFF, 0xa1000, 
> VIRTIO_IOMMU_MAP_F_READ);
> +    g_assert_cmpint(ret, ==, 0);

Could you also check that resending the map returns INVAL?

> +
> +    ret = send_unmap(qts, v_iommu, 4, 0x10, 0xFFF);
> +    g_assert_cmpint(ret, ==, VIRTIO_IOMMU_S_NOENT);
> +
> +    ret = send_unmap(qts, v_iommu, 1, 0x10, 0xFFF);
> +    g_assert_cmpint(ret, ==, VIRTIO_IOMMU_S_RANGE);
> +
> +    ret = send_unmap(qts, v_iommu, 1, 0, 0x1000);
> +    g_assert_cmpint(ret, ==, 0); /* unmap everything */
> +
> +    /* Spec example sequence */
> +
> +    /* 1 */
> +    ret = send_unmap(qts, v_iommu, 1, 0, 4);
> +    g_assert_cmpint(ret, ==, 0); /* doesn't unmap anything */
> +
> +    /* 2 */
> +    send_map(qts, v_iommu, 1, 0, 9, 0xa1000, VIRTIO_IOMMU_MAP_F_READ);

Same as above, the IOVAs aren't aligned on page size (the example assumes
a 1-byte granule, which arguably isn't the best idea but seemed easier to
read). The device currently accepts this but it would be better to write
the test to comply with page alignment. I can also change that when I get
to the alignment checks.

> +    ret = send_unmap(qts, v_iommu, 1, 0, 9);
> +    g_assert_cmpint(ret, ==, 0); /* unmaps [0,9] */
> +
> +    /* 3 */
> +    send_map(qts, v_iommu, 1, 0, 4, 0xb1000, VIRTIO_IOMMU_MAP_F_READ);
> +    send_map(qts, v_iommu, 1, 5, 9, 0xb2000, VIRTIO_IOMMU_MAP_F_READ);
> +    ret = send_unmap(qts, v_iommu, 1, 0, 9);
> +    g_assert_cmpint(ret, ==, 0); /* unmaps [0,4] and [5,9] */
> +
> +    /* 4 */
> +    send_map(qts, v_iommu, 1, 0, 9, 0xc1000, VIRTIO_IOMMU_MAP_F_READ);

Could check the return values: INVAL here means the previous unmap didn't
actually work

> +    ret = send_unmap(qts, v_iommu, 1, 0, 4);
> +    g_assert_cmpint(ret, ==, VIRTIO_IOMMU_S_RANGE); /* doesn't unmap 
> anything */
> +
> +    ret = send_unmap(qts, v_iommu, 1, 0, 10);
> +    g_assert_cmpint(ret, ==, 0);
> +
> +    /* 5 */
> +    send_map(qts, v_iommu, 1, 0, 4, 0xd1000, VIRTIO_IOMMU_MAP_F_READ);
> +    send_map(qts, v_iommu, 1, 5, 9, 0xd2000, VIRTIO_IOMMU_MAP_F_READ);
> +    ret = send_unmap(qts, v_iommu, 1, 0, 4);
> +    g_assert_cmpint(ret, ==, 0); /* unmaps [0,4] */
> +
> +    ret = send_unmap(qts, v_iommu, 1, 5, 9);
> +    g_assert_cmpint(ret, ==, 0);
> +
> +    /* 6 */
> +    send_map(qts, v_iommu, 1, 0, 4, 0xe2000, VIRTIO_IOMMU_MAP_F_READ);
> +    ret = send_unmap(qts, v_iommu, 1, 0, 9);
> +    g_assert_cmpint(ret, ==, 0); /* unmaps [0,4] */
> +
> +    /* 7 */
> +    send_map(qts, v_iommu, 1, 0, 4, 0xf2000, VIRTIO_IOMMU_MAP_F_READ);
> +    send_map(qts, v_iommu, 1, 10, 14, 0xf3000, VIRTIO_IOMMU_MAP_F_READ);
> +    ret = send_unmap(qts, v_iommu, 1, 0, 14);
> +    g_assert_cmpint(ret, ==, 0); /* unmaps [0,4] and [10,14] */
> +
> +    send_unmap(qts, v_iommu, 1, 0, 100);

Shouldn't be needed?

> +    send_map(qts, v_iommu, 1, 10, 14, 0xf3000, VIRTIO_IOMMU_MAP_F_READ);
> +    send_map(qts, v_iommu, 1, 0, 4, 0xf2000, VIRTIO_IOMMU_MAP_F_READ);
> +    ret = send_unmap(qts, v_iommu, 1, 0, 4);
> +    g_assert_cmpint(ret, ==, 0); /* unmaps [0,4] and [10,14] */

Only unmaps [0,4]

Thanks,
Jean

> +}
> +
> +static void register_virtio_iommu_test(void)
> +{
> +    qos_add_test("config", "virtio-iommu", pci_config, NULL);
> +    qos_add_test("attach_detach", "virtio-iommu", test_attach_detach, NULL);
> +    qos_add_test("map_unmap", "virtio-iommu", test_map_unmap, NULL);
> +}
> +
> +libqos_init(register_virtio_iommu_test);
> -- 
> 2.30.1
>