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

[Eric Auger]

Hi Jean,

On 10/14/21 2:11 PM, Jean-Philippe Brucker wrote:
> 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?
So this should be fixed now.
I removed the following pieces that caused the trouble
qos_node_create_driver("virtio-iommu-device", virtio_iommu_device_create);
qos_node_consumes("virtio-iommu-device", "virtio-bus", NULL);
qos_node_produces("virtio-iommu-device", "virtio");
qos_node_produces("virtio-iommu-device", "virtio-iommu");
>
> [...]
>> 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.
OK so effectively the qemu device has not been updated accordingly. So
if you have a patch ready, I will let you send it and update the test
later then.
>
>> +}
>> +
>> +/**
>> + * 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"?)
ok
>
>> + */
>> +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?
yes
>
> 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.
OK this test may be added later.
>
>> +    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?
yes I see such conversions in tests/qtest/virtio-9p-test.c too.
>
>> +
>> +    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
TBD
>
>> +    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?)
meant mapping, fixed
>
>> + * @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?
Yes that's the host bridge
There is graph built based defined relations between components
https://wiki.qemu.org/Features/qtest_driver_framework

that's why at the moment the test is not run on aarch64 because some
relations are missing and the virt machine libqos node does not induce
the instantiation of any PCI bus I think.

>
>> +    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)?
yep
>
>> +    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
OK
>
>> +    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?
added
>
>> +
>> +    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.
this was supposed to map onto virtio-v1.1-cs01.pdf examples
5.13.6.6. Maybe we should also consider to change the spec then?
>
>> +    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
done
>
>> +    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?
removed
>
>> +    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]
OK

Thanks!

Eric
>
> 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
>>