[PATCH 0/6] libqos, riscv: libqos fixes, add riscv machine

[PATCH 0/6] libqos, riscv: libqos fixes, add riscv machine

[Daniel Henrique Barboza]

Hi,

This series adds a RISC-V 'virt' libqos machine. It's copied from the
existing ARM machine in arm-virt-machine.c.

Due to the increase of test coverage I found out 2 bugs in the virtio
layer of libqos. Patch 1 is a bug related to unintialized guest pages
from guest_alloc() being used, patch 2 is an error in an offset of a
struct. My educated guess on why other boards don't fail these tests
is that either their guest pages happens to be zeroed or happens to have
a certain value that doesn't affect the tests. 

Patches 3, 4 and 5 are changes in the RISC-V 'virt' machine required to
make all tests pass.

Patch 6 adds the RISC-V virt machine nodes. As I said, it's a declared
copy of arm-virt-machine.c with extra steps. One of them is to edit the
attributes of the generic-pcihost liqos driver to fit RISC-V needs,
since some of its attributes (ecam, pio_base and so on) are hardcoded to
the values that the ARM 'virt' machine uses. If desired, we can change
generic-pcihost to be more flexible and not hardcode any machine/arch
specific value.
 

Alex: I'm CCing you because I noticed that you reviewed bug fixes in
libqos/virtio in the past, figured you have some expertise to share
here.


Patches based on master.


Daniel Henrique Barboza (6):
  libqos/virtio.c: init all elems in qvring_indirect_desc_setup()
  libqos/virtio.c: fix 'avail_event' offset in qvring_init()
  hw/riscv/virt.c: create '/soc/pci@...' fdt node earlier
  hw/riscv/virt.c: add virtio-iommu-pci hotplug support
  hw/riscv/virt.c: make aclint compatible with 'qtest' accel
  tests/libqos: add riscv/virt machine nodes

 hw/riscv/virt.c                         |  97 ++++++++++++-----
 tests/qtest/libqos/meson.build          |   1 +
 tests/qtest/libqos/riscv-virt-machine.c | 137 ++++++++++++++++++++++++
 tests/qtest/libqos/virtio.c             |  27 +++--
 4 files changed, 228 insertions(+), 34 deletions(-)
 create mode 100644 tests/qtest/libqos/riscv-virt-machine.c

-- 
2.43.0

[PATCH 1/6] libqos/virtio.c: init all elems in qvring_indirect_desc_setup()

[Daniel Henrique Barboza]

The loop isn't setting the values for the last element. Every other
element is being initialized with addr = 0, flags = VRING_DESC_F_NEXT
and next = i + 1. The last elem is never touched.

This became a problem when enabling a RISC-V 'virt' libqos machine in
the 'indirect' test of virti-blk-test.c. The 'flags' for the last
element will end up being an odd number (since we didn't touch it).
Being an odd number it will be mistaken by VRING_DESC_F_NEXT, which
happens to be 1.

Deep into hw/virt/virtio.c, in virtqueue_split_pop(), into
virtqueue_split_read_next_desc(), a check for VRING_DESC_F_NEXT will be
made to see if we're supposed to chain. The code will keep up chaining
in the last element because the unintialized value happens to be odd.
We'll error out right after that because desc->next (which is also
uninitialized) will be >= max. A VIRTQUEUE_READ_DESC_ERROR will be
returned, with an error message like this in the stderr:

qemu-system-riscv64: Desc next is 49391

Since we never returned, w'll end up timing out at qvirtio_wait_used_elem():

ERROR:../tests/qtest/libqos/virtio.c:236:qvirtio_wait_used_elem:
    assertion failed: (g_get_monotonic_time() - start_time <= timeout_us)

The root cause is using unintialized values from guest_alloc() in
qvring_indirect_desc_setup(). There's no guarantee that the memory pages
retrieved will be zeroed, so we can't make assumptions. In fact, commit
5b4f72f5e8 ("tests/qtest: properly initialise the vring used idx") fixed a
similar problem stating "It is probably not wise to assume guest memory
is zeroed anyway". I concur.

Initialize all elems in qvring_indirect_desc_setup().

Fixes: f294b029aa ("libqos: Added indirect descriptor support to virtio implementation")
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 tests/qtest/libqos/virtio.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/tests/qtest/libqos/virtio.c b/tests/qtest/libqos/virtio.c
index 410513225f..4f39124eba 100644
--- a/tests/qtest/libqos/virtio.c
+++ b/tests/qtest/libqos/virtio.c
@@ -280,14 +280,27 @@ QVRingIndirectDesc *qvring_indirect_desc_setup(QTestState *qs, QVirtioDevice *d,
     indirect->elem = elem;
     indirect->desc = guest_alloc(alloc, sizeof(struct vring_desc) * elem);
 
-    for (i = 0; i < elem - 1; ++i) {
+    for (i = 0; i < elem; ++i) {
         /* indirect->desc[i].addr */
         qvirtio_writeq(d, qs, indirect->desc + (16 * i), 0);
-        /* indirect->desc[i].flags */
-        qvirtio_writew(d, qs, indirect->desc + (16 * i) + 12,
-                       VRING_DESC_F_NEXT);
-        /* indirect->desc[i].next */
-        qvirtio_writew(d, qs, indirect->desc + (16 * i) + 14, i + 1);
+
+        /*
+         * If it's not the last element of the ring, set
+         * the chain (VRING_DESC_F_NEXT) flag and
+         * desc->next. Clear the last element - there's
+         * no guarantee that guest_alloc() will do it.
+         */
+        if (i != elem - 1) {
+            /* indirect->desc[i].flags */
+            qvirtio_writew(d, qs, indirect->desc + (16 * i) + 12,
+                           VRING_DESC_F_NEXT);
+
+            /* indirect->desc[i].next */
+            qvirtio_writew(d, qs, indirect->desc + (16 * i) + 14, i + 1);
+        } else {
+            qvirtio_writew(d, qs, indirect->desc + (16 * i) + 12, 0);
+            qvirtio_writew(d, qs, indirect->desc + (16 * i) + 14, 0);
+        }
     }
 
     return indirect;
-- 
2.43.0

Re: [PATCH 1/6] libqos/virtio.c: init all elems in qvring_indirect_desc_setup()

[Alistair Francis]

On Wed, Feb 14, 2024 at 5:18 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> The loop isn't setting the values for the last element. Every other
> element is being initialized with addr = 0, flags = VRING_DESC_F_NEXT
> and next = i + 1. The last elem is never touched.
>
> This became a problem when enabling a RISC-V 'virt' libqos machine in
> the 'indirect' test of virti-blk-test.c. The 'flags' for the last
> element will end up being an odd number (since we didn't touch it).
> Being an odd number it will be mistaken by VRING_DESC_F_NEXT, which
> happens to be 1.
>
> Deep into hw/virt/virtio.c, in virtqueue_split_pop(), into
> virtqueue_split_read_next_desc(), a check for VRING_DESC_F_NEXT will be
> made to see if we're supposed to chain. The code will keep up chaining
> in the last element because the unintialized value happens to be odd.
> We'll error out right after that because desc->next (which is also
> uninitialized) will be >= max. A VIRTQUEUE_READ_DESC_ERROR will be
> returned, with an error message like this in the stderr:
>
> qemu-system-riscv64: Desc next is 49391
>
> Since we never returned, w'll end up timing out at qvirtio_wait_used_elem():
>
> ERROR:../tests/qtest/libqos/virtio.c:236:qvirtio_wait_used_elem:
>     assertion failed: (g_get_monotonic_time() - start_time <= timeout_us)
>
> The root cause is using unintialized values from guest_alloc() in
> qvring_indirect_desc_setup(). There's no guarantee that the memory pages
> retrieved will be zeroed, so we can't make assumptions. In fact, commit
> 5b4f72f5e8 ("tests/qtest: properly initialise the vring used idx") fixed a
> similar problem stating "It is probably not wise to assume guest memory
> is zeroed anyway". I concur.
>
> Initialize all elems in qvring_indirect_desc_setup().
>
> Fixes: f294b029aa ("libqos: Added indirect descriptor support to virtio implementation")
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  tests/qtest/libqos/virtio.c | 25 +++++++++++++++++++------
>  1 file changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/tests/qtest/libqos/virtio.c b/tests/qtest/libqos/virtio.c
> index 410513225f..4f39124eba 100644
> --- a/tests/qtest/libqos/virtio.c
> +++ b/tests/qtest/libqos/virtio.c
> @@ -280,14 +280,27 @@ QVRingIndirectDesc *qvring_indirect_desc_setup(QTestState *qs, QVirtioDevice *d,
>      indirect->elem = elem;
>      indirect->desc = guest_alloc(alloc, sizeof(struct vring_desc) * elem);
>
> -    for (i = 0; i < elem - 1; ++i) {
> +    for (i = 0; i < elem; ++i) {
>          /* indirect->desc[i].addr */
>          qvirtio_writeq(d, qs, indirect->desc + (16 * i), 0);
> -        /* indirect->desc[i].flags */
> -        qvirtio_writew(d, qs, indirect->desc + (16 * i) + 12,
> -                       VRING_DESC_F_NEXT);
> -        /* indirect->desc[i].next */
> -        qvirtio_writew(d, qs, indirect->desc + (16 * i) + 14, i + 1);
> +
> +        /*
> +         * If it's not the last element of the ring, set
> +         * the chain (VRING_DESC_F_NEXT) flag and
> +         * desc->next. Clear the last element - there's
> +         * no guarantee that guest_alloc() will do it.
> +         */
> +        if (i != elem - 1) {
> +            /* indirect->desc[i].flags */
> +            qvirtio_writew(d, qs, indirect->desc + (16 * i) + 12,
> +                           VRING_DESC_F_NEXT);
> +
> +            /* indirect->desc[i].next */
> +            qvirtio_writew(d, qs, indirect->desc + (16 * i) + 14, i + 1);
> +        } else {
> +            qvirtio_writew(d, qs, indirect->desc + (16 * i) + 12, 0);
> +            qvirtio_writew(d, qs, indirect->desc + (16 * i) + 14, 0);
> +        }
>      }
>
>      return indirect;
> --
> 2.43.0
>
>

Re: [PATCH 1/6] libqos/virtio.c: init all elems in qvring_indirect_desc_setup()

[Thomas Huth]

On 13/02/2024 20.17, Daniel Henrique Barboza wrote:
> The loop isn't setting the values for the last element. Every other
> element is being initialized with addr = 0, flags = VRING_DESC_F_NEXT
> and next = i + 1. The last elem is never touched.
> 
> This became a problem when enabling a RISC-V 'virt' libqos machine in
> the 'indirect' test of virti-blk-test.c. The 'flags' for the last
> element will end up being an odd number (since we didn't touch it).
> Being an odd number it will be mistaken by VRING_DESC_F_NEXT, which
> happens to be 1.
> 
> Deep into hw/virt/virtio.c, in virtqueue_split_pop(), into
> virtqueue_split_read_next_desc(), a check for VRING_DESC_F_NEXT will be
> made to see if we're supposed to chain. The code will keep up chaining
> in the last element because the unintialized value happens to be odd.

s/unintialized/uninitialized/

> We'll error out right after that because desc->next (which is also
> uninitialized) will be >= max. A VIRTQUEUE_READ_DESC_ERROR will be
> returned, with an error message like this in the stderr:
> 
> qemu-system-riscv64: Desc next is 49391
> 
> Since we never returned, w'll end up timing out at qvirtio_wait_used_elem():

s/w'll/we'll/

> ERROR:../tests/qtest/libqos/virtio.c:236:qvirtio_wait_used_elem:
>      assertion failed: (g_get_monotonic_time() - start_time <= timeout_us)
> 
> The root cause is using unintialized values from guest_alloc() in

s/unintialized/uninitialized/

With the typos fixed:
Reviewed-by: Thomas Huth <thuth@redhat.com>

[PATCH 2/6] libqos/virtio.c: fix 'avail_event' offset in qvring_init()

[Daniel Henrique Barboza]

In qvring_init() we're writing vq->used->avail_event at "vq->used + 2 +
array_size".  The struct pointed by vq->used is, from virtio_ring.h
Linux header):

 *	// A ring of used descriptor heads with free-running index.
 *	__virtio16 used_flags;
 *	__virtio16 used_idx;
 *	struct vring_used_elem used[num];
 *	__virtio16 avail_event_idx;

So 'flags' is the word right at vq->used. 'idx' is vq->used + 2. We need
to skip 'used_idx' by adding + 2 bytes, and then sum the vector size, to
reach avail_event_idx. An example on how to properly access this field
can be found in qvirtqueue_kick():

avail_event = qvirtio_readw(d, qts, vq->used + 4 +
                            sizeof(struct vring_used_elem) * vq->size);

This error was detected when enabling the RISC-V 'virt' libqos machine.
The 'idx' test from vhost-user-blk-test.c errors out with a timeout in
qvirtio_wait_used_elem(). The timeout happens because when processing
the first element, 'avail_event' is read in qvirtqueue_kick() as non-zero
because we didn't initialize it properly (and the memory at that point
happened to be non-zero). 'idx' is 0.

All of this makes this condition fail because "idx - avail_event" will
overflow and be non-zero:

/* < 1 because we add elements to avail queue one by one */
if ((flags & VRING_USED_F_NO_NOTIFY) == 0 &&
                        (!vq->event || (uint16_t)(idx-avail_event) < 1)) {
    d->bus->virtqueue_kick(d, vq);
}

As a result the virtqueue is never kicked and we'll timeout waiting for it.

Fixes: 1053587c3f ("libqos: Added EVENT_IDX support")
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 tests/qtest/libqos/virtio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qtest/libqos/virtio.c b/tests/qtest/libqos/virtio.c
index 4f39124eba..82a6e122bf 100644
--- a/tests/qtest/libqos/virtio.c
+++ b/tests/qtest/libqos/virtio.c
@@ -265,7 +265,7 @@ void qvring_init(QTestState *qts, const QGuestAllocator *alloc, QVirtQueue *vq,
     /* vq->used->idx */
     qvirtio_writew(vq->vdev, qts, vq->used + 2, 0);
     /* vq->used->avail_event */
-    qvirtio_writew(vq->vdev, qts, vq->used + 2 +
+    qvirtio_writew(vq->vdev, qts, vq->used + 4 +
                    sizeof(struct vring_used_elem) * vq->size, 0);
 }
 
-- 
2.43.0

Re: [PATCH 2/6] libqos/virtio.c: fix 'avail_event' offset in qvring_init()

[Alistair Francis]

On Wed, Feb 14, 2024 at 5:18 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> In qvring_init() we're writing vq->used->avail_event at "vq->used + 2 +
> array_size".  The struct pointed by vq->used is, from virtio_ring.h
> Linux header):
>
>  *      // A ring of used descriptor heads with free-running index.
>  *      __virtio16 used_flags;
>  *      __virtio16 used_idx;
>  *      struct vring_used_elem used[num];
>  *      __virtio16 avail_event_idx;
>
> So 'flags' is the word right at vq->used. 'idx' is vq->used + 2. We need
> to skip 'used_idx' by adding + 2 bytes, and then sum the vector size, to
> reach avail_event_idx. An example on how to properly access this field
> can be found in qvirtqueue_kick():
>
> avail_event = qvirtio_readw(d, qts, vq->used + 4 +
>                             sizeof(struct vring_used_elem) * vq->size);
>
> This error was detected when enabling the RISC-V 'virt' libqos machine.
> The 'idx' test from vhost-user-blk-test.c errors out with a timeout in
> qvirtio_wait_used_elem(). The timeout happens because when processing
> the first element, 'avail_event' is read in qvirtqueue_kick() as non-zero
> because we didn't initialize it properly (and the memory at that point
> happened to be non-zero). 'idx' is 0.
>
> All of this makes this condition fail because "idx - avail_event" will
> overflow and be non-zero:
>
> /* < 1 because we add elements to avail queue one by one */
> if ((flags & VRING_USED_F_NO_NOTIFY) == 0 &&
>                         (!vq->event || (uint16_t)(idx-avail_event) < 1)) {
>     d->bus->virtqueue_kick(d, vq);
> }
>
> As a result the virtqueue is never kicked and we'll timeout waiting for it.
>
> Fixes: 1053587c3f ("libqos: Added EVENT_IDX support")
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  tests/qtest/libqos/virtio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/qtest/libqos/virtio.c b/tests/qtest/libqos/virtio.c
> index 4f39124eba..82a6e122bf 100644
> --- a/tests/qtest/libqos/virtio.c
> +++ b/tests/qtest/libqos/virtio.c
> @@ -265,7 +265,7 @@ void qvring_init(QTestState *qts, const QGuestAllocator *alloc, QVirtQueue *vq,
>      /* vq->used->idx */
>      qvirtio_writew(vq->vdev, qts, vq->used + 2, 0);
>      /* vq->used->avail_event */
> -    qvirtio_writew(vq->vdev, qts, vq->used + 2 +
> +    qvirtio_writew(vq->vdev, qts, vq->used + 4 +
>                     sizeof(struct vring_used_elem) * vq->size, 0);
>  }
>
> --
> 2.43.0
>
>

Re: [PATCH 2/6] libqos/virtio.c: fix 'avail_event' offset in qvring_init()

[Thomas Huth]

On 13/02/2024 20.17, Daniel Henrique Barboza wrote:
> In qvring_init() we're writing vq->used->avail_event at "vq->used + 2 +
> array_size".  The struct pointed by vq->used is, from virtio_ring.h
> Linux header):
> 
>   *	// A ring of used descriptor heads with free-running index.
>   *	__virtio16 used_flags;
>   *	__virtio16 used_idx;
>   *	struct vring_used_elem used[num];
>   *	__virtio16 avail_event_idx;
> 
> So 'flags' is the word right at vq->used. 'idx' is vq->used + 2. We need
> to skip 'used_idx' by adding + 2 bytes, and then sum the vector size, to
> reach avail_event_idx. An example on how to properly access this field
> can be found in qvirtqueue_kick():
> 
> avail_event = qvirtio_readw(d, qts, vq->used + 4 +
>                              sizeof(struct vring_used_elem) * vq->size);
> 
> This error was detected when enabling the RISC-V 'virt' libqos machine.
> The 'idx' test from vhost-user-blk-test.c errors out with a timeout in
> qvirtio_wait_used_elem(). The timeout happens because when processing
> the first element, 'avail_event' is read in qvirtqueue_kick() as non-zero
> because we didn't initialize it properly (and the memory at that point
> happened to be non-zero). 'idx' is 0.
> 
> All of this makes this condition fail because "idx - avail_event" will
> overflow and be non-zero:
> 
> /* < 1 because we add elements to avail queue one by one */
> if ((flags & VRING_USED_F_NO_NOTIFY) == 0 &&
>                          (!vq->event || (uint16_t)(idx-avail_event) < 1)) {
>      d->bus->virtqueue_kick(d, vq);
> }
> 
> As a result the virtqueue is never kicked and we'll timeout waiting for it.
> 
> Fixes: 1053587c3f ("libqos: Added EVENT_IDX support")
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>   tests/qtest/libqos/virtio.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/qtest/libqos/virtio.c b/tests/qtest/libqos/virtio.c
> index 4f39124eba..82a6e122bf 100644
> --- a/tests/qtest/libqos/virtio.c
> +++ b/tests/qtest/libqos/virtio.c
> @@ -265,7 +265,7 @@ void qvring_init(QTestState *qts, const QGuestAllocator *alloc, QVirtQueue *vq,
>       /* vq->used->idx */
>       qvirtio_writew(vq->vdev, qts, vq->used + 2, 0);
>       /* vq->used->avail_event */
> -    qvirtio_writew(vq->vdev, qts, vq->used + 2 +
> +    qvirtio_writew(vq->vdev, qts, vq->used + 4 +
>                      sizeof(struct vring_used_elem) * vq->size, 0);
>   }
>   

Reviewed-by: Thomas Huth <thuth@redhat.com>

[PATCH 3/6] hw/riscv/virt.c: create '/soc/pci@...' fdt node earlier

[Daniel Henrique Barboza]

Hotplugged FDT nodes will attempt to write this node that, at this
moment, is being created only in create_fdt_pcie() during
finalize_fdt().

Create it earlier.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 hw/riscv/virt.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index fd35c74781..b540f4d3da 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -826,7 +826,6 @@ static void create_fdt_pcie(RISCVVirtState *s, const MemMapEntry *memmap,
 
     name = g_strdup_printf("/soc/pci@%lx",
         (long) memmap[VIRT_PCIE_ECAM].base);
-    qemu_fdt_add_subnode(ms->fdt, name);
     qemu_fdt_setprop_cell(ms->fdt, name, "#address-cells",
         FDT_PCI_ADDR_CELLS);
     qemu_fdt_setprop_cell(ms->fdt, name, "#interrupt-cells",
@@ -996,6 +995,7 @@ static void create_fdt(RISCVVirtState *s, const MemMapEntry *memmap)
 {
     MachineState *ms = MACHINE(s);
     uint8_t rng_seed[32];
+    g_autofree char *name = NULL;
 
     ms->fdt = create_device_tree(&s->fdt_size);
     if (!ms->fdt) {
@@ -1014,6 +1014,13 @@ static void create_fdt(RISCVVirtState *s, const MemMapEntry *memmap)
     qemu_fdt_setprop_cell(ms->fdt, "/soc", "#size-cells", 0x2);
     qemu_fdt_setprop_cell(ms->fdt, "/soc", "#address-cells", 0x2);
 
+    /*
+     * The "/soc/pci@..." node is needed for PCIE hotplugs
+     * that might happen before finalize_fdt().
+     */
+    name = g_strdup_printf("/soc/pci@%lx", (long) memmap[VIRT_PCIE_ECAM].base);
+    qemu_fdt_add_subnode(ms->fdt, name);
+
     qemu_fdt_add_subnode(ms->fdt, "/chosen");
 
     /* Pass seed to RNG */
-- 
2.43.0

Re: [PATCH 3/6] hw/riscv/virt.c: create '/soc/pci@...' fdt node earlier

[Alistair Francis]

On Wed, Feb 14, 2024 at 5:19 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Hotplugged FDT nodes will attempt to write this node that, at this
> moment, is being created only in create_fdt_pcie() during
> finalize_fdt().
>
> Create it earlier.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/riscv/virt.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index fd35c74781..b540f4d3da 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -826,7 +826,6 @@ static void create_fdt_pcie(RISCVVirtState *s, const MemMapEntry *memmap,
>
>      name = g_strdup_printf("/soc/pci@%lx",
>          (long) memmap[VIRT_PCIE_ECAM].base);
> -    qemu_fdt_add_subnode(ms->fdt, name);
>      qemu_fdt_setprop_cell(ms->fdt, name, "#address-cells",
>          FDT_PCI_ADDR_CELLS);
>      qemu_fdt_setprop_cell(ms->fdt, name, "#interrupt-cells",
> @@ -996,6 +995,7 @@ static void create_fdt(RISCVVirtState *s, const MemMapEntry *memmap)
>  {
>      MachineState *ms = MACHINE(s);
>      uint8_t rng_seed[32];
> +    g_autofree char *name = NULL;
>
>      ms->fdt = create_device_tree(&s->fdt_size);
>      if (!ms->fdt) {
> @@ -1014,6 +1014,13 @@ static void create_fdt(RISCVVirtState *s, const MemMapEntry *memmap)
>      qemu_fdt_setprop_cell(ms->fdt, "/soc", "#size-cells", 0x2);
>      qemu_fdt_setprop_cell(ms->fdt, "/soc", "#address-cells", 0x2);
>
> +    /*
> +     * The "/soc/pci@..." node is needed for PCIE hotplugs
> +     * that might happen before finalize_fdt().
> +     */
> +    name = g_strdup_printf("/soc/pci@%lx", (long) memmap[VIRT_PCIE_ECAM].base);
> +    qemu_fdt_add_subnode(ms->fdt, name);
> +
>      qemu_fdt_add_subnode(ms->fdt, "/chosen");
>
>      /* Pass seed to RNG */
> --
> 2.43.0
>
>

[PATCH 4/6] hw/riscv/virt.c: add virtio-iommu-pci hotplug support

[Daniel Henrique Barboza]

We want to add a RISC-V 'virt' libqos machine to increase our test
coverage. Some of the tests will try to plug a virtio-iommu-pci
device into the board and do some tests with it.

Enable virtio-iommu-pci in the 'virt' machine.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 hw/riscv/virt.c | 36 +++++++++++++++++++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index b540f4d3da..54ad809b44 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -53,6 +53,7 @@
 #include "hw/display/ramfb.h"
 #include "hw/acpi/aml-build.h"
 #include "qapi/qapi-visit-common.h"
+#include "hw/virtio/virtio-iommu.h"
 
 /* KVM AIA only supports APLIC MSI. APLIC Wired is always emulated by QEMU. */
 static bool virt_use_kvm_aia(RISCVVirtState *s)
@@ -971,6 +972,34 @@ static void create_fdt_fw_cfg(RISCVVirtState *s, const MemMapEntry *memmap)
     qemu_fdt_setprop(ms->fdt, nodename, "dma-coherent", NULL, 0);
 }
 
+static void create_fdt_virtio_iommu(RISCVVirtState *s, uint16_t bdf)
+{
+    const char compat[] = "virtio,pci-iommu\0pci1af4,1057";
+    void *fdt = MACHINE(s)->fdt;
+    uint32_t iommu_phandle;
+    g_autofree char *iommu_node = NULL;
+    g_autofree char *pci_node = NULL;
+
+    pci_node = g_strdup_printf("/soc/pci@%lx",
+                               (long) virt_memmap[VIRT_PCIE_ECAM].base);
+    iommu_node = g_strdup_printf("%s/virtio_iommu@%x,%x", pci_node,
+                                 PCI_SLOT(bdf), PCI_FUNC(bdf));
+    iommu_phandle = qemu_fdt_alloc_phandle(fdt);
+
+    qemu_fdt_add_subnode(fdt, iommu_node);
+
+    qemu_fdt_setprop(fdt, iommu_node, "compatible", compat, sizeof(compat));
+    qemu_fdt_setprop_sized_cells(fdt, iommu_node, "reg",
+                                 1, bdf << 8, 1, 0, 1, 0,
+                                 1, 0, 1, 0);
+    qemu_fdt_setprop_cell(fdt, iommu_node, "#iommu-cells", 1);
+    qemu_fdt_setprop_cell(fdt, iommu_node, "phandle", iommu_phandle);
+
+    qemu_fdt_setprop_cells(fdt, pci_node, "iommu-map",
+                           0, iommu_phandle, 0, bdf,
+                           bdf + 1, iommu_phandle, bdf + 1, 0xffff - bdf);
+}
+
 static void finalize_fdt(RISCVVirtState *s)
 {
     uint32_t phandle = 1, irq_mmio_phandle = 1, msi_pcie_phandle = 1;
@@ -1680,7 +1709,8 @@ static HotplugHandler *virt_machine_get_hotplug_handler(MachineState *machine,
 {
     MachineClass *mc = MACHINE_GET_CLASS(machine);
 
-    if (device_is_dynamic_sysbus(mc, dev)) {
+    if (device_is_dynamic_sysbus(mc, dev) ||
+        object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI)) {
         return HOTPLUG_HANDLER(machine);
     }
     return NULL;
@@ -1699,6 +1729,10 @@ static void virt_machine_device_plug_cb(HotplugHandler *hotplug_dev,
                                      SYS_BUS_DEVICE(dev));
         }
     }
+
+    if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI)) {
+        create_fdt_virtio_iommu(s, pci_get_bdf(PCI_DEVICE(dev)));
+    }
 }
 
 static void virt_machine_class_init(ObjectClass *oc, void *data)
-- 
2.43.0

Re: [PATCH 4/6] hw/riscv/virt.c: add virtio-iommu-pci hotplug support

[Alistair Francis]

On Wed, Feb 14, 2024 at 5:18 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> We want to add a RISC-V 'virt' libqos machine to increase our test
> coverage. Some of the tests will try to plug a virtio-iommu-pci
> device into the board and do some tests with it.
>
> Enable virtio-iommu-pci in the 'virt' machine.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

Acked-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/riscv/virt.c | 36 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 35 insertions(+), 1 deletion(-)
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index b540f4d3da..54ad809b44 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -53,6 +53,7 @@
>  #include "hw/display/ramfb.h"
>  #include "hw/acpi/aml-build.h"
>  #include "qapi/qapi-visit-common.h"
> +#include "hw/virtio/virtio-iommu.h"
>
>  /* KVM AIA only supports APLIC MSI. APLIC Wired is always emulated by QEMU. */
>  static bool virt_use_kvm_aia(RISCVVirtState *s)
> @@ -971,6 +972,34 @@ static void create_fdt_fw_cfg(RISCVVirtState *s, const MemMapEntry *memmap)
>      qemu_fdt_setprop(ms->fdt, nodename, "dma-coherent", NULL, 0);
>  }
>
> +static void create_fdt_virtio_iommu(RISCVVirtState *s, uint16_t bdf)
> +{
> +    const char compat[] = "virtio,pci-iommu\0pci1af4,1057";
> +    void *fdt = MACHINE(s)->fdt;
> +    uint32_t iommu_phandle;
> +    g_autofree char *iommu_node = NULL;
> +    g_autofree char *pci_node = NULL;
> +
> +    pci_node = g_strdup_printf("/soc/pci@%lx",
> +                               (long) virt_memmap[VIRT_PCIE_ECAM].base);
> +    iommu_node = g_strdup_printf("%s/virtio_iommu@%x,%x", pci_node,
> +                                 PCI_SLOT(bdf), PCI_FUNC(bdf));
> +    iommu_phandle = qemu_fdt_alloc_phandle(fdt);
> +
> +    qemu_fdt_add_subnode(fdt, iommu_node);
> +
> +    qemu_fdt_setprop(fdt, iommu_node, "compatible", compat, sizeof(compat));
> +    qemu_fdt_setprop_sized_cells(fdt, iommu_node, "reg",
> +                                 1, bdf << 8, 1, 0, 1, 0,
> +                                 1, 0, 1, 0);
> +    qemu_fdt_setprop_cell(fdt, iommu_node, "#iommu-cells", 1);
> +    qemu_fdt_setprop_cell(fdt, iommu_node, "phandle", iommu_phandle);
> +
> +    qemu_fdt_setprop_cells(fdt, pci_node, "iommu-map",
> +                           0, iommu_phandle, 0, bdf,
> +                           bdf + 1, iommu_phandle, bdf + 1, 0xffff - bdf);
> +}
> +
>  static void finalize_fdt(RISCVVirtState *s)
>  {
>      uint32_t phandle = 1, irq_mmio_phandle = 1, msi_pcie_phandle = 1;
> @@ -1680,7 +1709,8 @@ static HotplugHandler *virt_machine_get_hotplug_handler(MachineState *machine,
>  {
>      MachineClass *mc = MACHINE_GET_CLASS(machine);
>
> -    if (device_is_dynamic_sysbus(mc, dev)) {
> +    if (device_is_dynamic_sysbus(mc, dev) ||
> +        object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI)) {
>          return HOTPLUG_HANDLER(machine);
>      }
>      return NULL;
> @@ -1699,6 +1729,10 @@ static void virt_machine_device_plug_cb(HotplugHandler *hotplug_dev,
>                                       SYS_BUS_DEVICE(dev));
>          }
>      }
> +
> +    if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI)) {
> +        create_fdt_virtio_iommu(s, pci_get_bdf(PCI_DEVICE(dev)));
> +    }
>  }
>
>  static void virt_machine_class_init(ObjectClass *oc, void *data)
> --
> 2.43.0
>
>

[PATCH 5/6] hw/riscv/virt.c: make aclint compatible with 'qtest' accel

[Daniel Henrique Barboza]

The 'virt' machine makes assumptions on the Advanced Core-Local
Interruptor, or aclint, based on 'tcg_enabled()' conditionals.  This
will impact MSI related tests support when adding a RISC-V 'virt' libqos
machine. The accelerator used in that case, 'qtest', isn't being
accounted for and we'll error out if we try to enable aclint.

Create a new virt_aclint_allowed() helper to gate the aclint code
considering both TCG and 'qtest' accelerators. The error message is
left untouched, mentioning TCG only, because we don't expect the
regular user to be aware of 'qtest'.

We want to add 'qtest' support for aclint only, leaving the TCG specific
bits out of it. This is done by changing the current format we use
today:

if (tcg_enabled()) {
   if (s->have_aclint) { - aclint logic - }
   else { - non-aclint, TCG logic - }
}

into:

if (virt_aclint_allowed() && s->have_aclint) {
 - aclint logic -
} else if (tcg_enabled()) {
 - non-aclint, TCG logic -
}

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 hw/riscv/virt.c | 52 +++++++++++++++++++++++++------------------------
 1 file changed, 27 insertions(+), 25 deletions(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 54ad809b44..a094af97c3 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -48,6 +48,7 @@
 #include "sysemu/tcg.h"
 #include "sysemu/kvm.h"
 #include "sysemu/tpm.h"
+#include "sysemu/qtest.h"
 #include "hw/pci/pci.h"
 #include "hw/pci-host/gpex.h"
 #include "hw/display/ramfb.h"
@@ -61,6 +62,11 @@ static bool virt_use_kvm_aia(RISCVVirtState *s)
     return kvm_irqchip_in_kernel() && s->aia_type == VIRT_AIA_TYPE_APLIC_IMSIC;
 }
 
+static bool virt_aclint_allowed(void)
+{
+    return tcg_enabled() || qtest_enabled();
+}
+
 static const MemMapEntry virt_memmap[] = {
     [VIRT_DEBUG] =        {        0x0,         0x100 },
     [VIRT_MROM] =         {     0x1000,        0xf000 },
@@ -725,14 +731,12 @@ static void create_fdt_sockets(RISCVVirtState *s, const MemMapEntry *memmap,
 
         create_fdt_socket_memory(s, memmap, socket);
 
-        if (tcg_enabled()) {
-            if (s->have_aclint) {
-                create_fdt_socket_aclint(s, memmap, socket,
-                    &intc_phandles[phandle_pos]);
-            } else {
-                create_fdt_socket_clint(s, memmap, socket,
-                    &intc_phandles[phandle_pos]);
-            }
+        if (virt_aclint_allowed() && s->have_aclint) {
+            create_fdt_socket_aclint(s, memmap, socket,
+                                     &intc_phandles[phandle_pos]);
+        } else if (tcg_enabled()) {
+            create_fdt_socket_clint(s, memmap, socket,
+                                    &intc_phandles[phandle_pos]);
         }
     }
 
@@ -1409,7 +1413,7 @@ static void virt_machine_init(MachineState *machine)
         exit(1);
     }
 
-    if (!tcg_enabled() && s->have_aclint) {
+    if (!virt_aclint_allowed() && s->have_aclint) {
         error_report("'aclint' is only available with TCG acceleration");
         exit(1);
     }
@@ -1446,23 +1450,22 @@ static void virt_machine_init(MachineState *machine)
                                 hart_count, &error_abort);
         sysbus_realize(SYS_BUS_DEVICE(&s->soc[i]), &error_fatal);
 
-        if (tcg_enabled()) {
-            if (s->have_aclint) {
-                if (s->aia_type == VIRT_AIA_TYPE_APLIC_IMSIC) {
-                    /* Per-socket ACLINT MTIMER */
-                    riscv_aclint_mtimer_create(memmap[VIRT_CLINT].base +
+        if (virt_aclint_allowed() && s->have_aclint) {
+            if (s->aia_type == VIRT_AIA_TYPE_APLIC_IMSIC) {
+                /* Per-socket ACLINT MTIMER */
+                riscv_aclint_mtimer_create(memmap[VIRT_CLINT].base +
                             i * RISCV_ACLINT_DEFAULT_MTIMER_SIZE,
                         RISCV_ACLINT_DEFAULT_MTIMER_SIZE,
                         base_hartid, hart_count,
                         RISCV_ACLINT_DEFAULT_MTIMECMP,
                         RISCV_ACLINT_DEFAULT_MTIME,
                         RISCV_ACLINT_DEFAULT_TIMEBASE_FREQ, true);
-                } else {
-                    /* Per-socket ACLINT MSWI, MTIMER, and SSWI */
-                    riscv_aclint_swi_create(memmap[VIRT_CLINT].base +
+            } else {
+                /* Per-socket ACLINT MSWI, MTIMER, and SSWI */
+                riscv_aclint_swi_create(memmap[VIRT_CLINT].base +
                             i * memmap[VIRT_CLINT].size,
                         base_hartid, hart_count, false);
-                    riscv_aclint_mtimer_create(memmap[VIRT_CLINT].base +
+                riscv_aclint_mtimer_create(memmap[VIRT_CLINT].base +
                             i * memmap[VIRT_CLINT].size +
                             RISCV_ACLINT_SWI_SIZE,
                         RISCV_ACLINT_DEFAULT_MTIMER_SIZE,
@@ -1470,21 +1473,20 @@ static void virt_machine_init(MachineState *machine)
                         RISCV_ACLINT_DEFAULT_MTIMECMP,
                         RISCV_ACLINT_DEFAULT_MTIME,
                         RISCV_ACLINT_DEFAULT_TIMEBASE_FREQ, true);
-                    riscv_aclint_swi_create(memmap[VIRT_ACLINT_SSWI].base +
+                riscv_aclint_swi_create(memmap[VIRT_ACLINT_SSWI].base +
                             i * memmap[VIRT_ACLINT_SSWI].size,
                         base_hartid, hart_count, true);
-                }
-            } else {
-                /* Per-socket SiFive CLINT */
-                riscv_aclint_swi_create(
+            }
+        } else if (tcg_enabled()) {
+            /* Per-socket SiFive CLINT */
+            riscv_aclint_swi_create(
                     memmap[VIRT_CLINT].base + i * memmap[VIRT_CLINT].size,
                     base_hartid, hart_count, false);
-                riscv_aclint_mtimer_create(memmap[VIRT_CLINT].base +
+            riscv_aclint_mtimer_create(memmap[VIRT_CLINT].base +
                         i * memmap[VIRT_CLINT].size + RISCV_ACLINT_SWI_SIZE,
                     RISCV_ACLINT_DEFAULT_MTIMER_SIZE, base_hartid, hart_count,
                     RISCV_ACLINT_DEFAULT_MTIMECMP, RISCV_ACLINT_DEFAULT_MTIME,
                     RISCV_ACLINT_DEFAULT_TIMEBASE_FREQ, true);
-            }
         }
 
         /* Per-socket interrupt controller */
-- 
2.43.0

Re: [PATCH 5/6] hw/riscv/virt.c: make aclint compatible with 'qtest' accel

[Alistair Francis]

On Wed, Feb 14, 2024 at 5:18 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> The 'virt' machine makes assumptions on the Advanced Core-Local
> Interruptor, or aclint, based on 'tcg_enabled()' conditionals.  This
> will impact MSI related tests support when adding a RISC-V 'virt' libqos
> machine. The accelerator used in that case, 'qtest', isn't being
> accounted for and we'll error out if we try to enable aclint.
>
> Create a new virt_aclint_allowed() helper to gate the aclint code
> considering both TCG and 'qtest' accelerators. The error message is
> left untouched, mentioning TCG only, because we don't expect the
> regular user to be aware of 'qtest'.
>
> We want to add 'qtest' support for aclint only, leaving the TCG specific
> bits out of it. This is done by changing the current format we use
> today:
>
> if (tcg_enabled()) {
>    if (s->have_aclint) { - aclint logic - }
>    else { - non-aclint, TCG logic - }
> }
>
> into:
>
> if (virt_aclint_allowed() && s->have_aclint) {
>  - aclint logic -
> } else if (tcg_enabled()) {
>  - non-aclint, TCG logic -
> }
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/riscv/virt.c | 52 +++++++++++++++++++++++++------------------------
>  1 file changed, 27 insertions(+), 25 deletions(-)
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 54ad809b44..a094af97c3 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -48,6 +48,7 @@
>  #include "sysemu/tcg.h"
>  #include "sysemu/kvm.h"
>  #include "sysemu/tpm.h"
> +#include "sysemu/qtest.h"
>  #include "hw/pci/pci.h"
>  #include "hw/pci-host/gpex.h"
>  #include "hw/display/ramfb.h"
> @@ -61,6 +62,11 @@ static bool virt_use_kvm_aia(RISCVVirtState *s)
>      return kvm_irqchip_in_kernel() && s->aia_type == VIRT_AIA_TYPE_APLIC_IMSIC;
>  }
>
> +static bool virt_aclint_allowed(void)
> +{
> +    return tcg_enabled() || qtest_enabled();
> +}
> +
>  static const MemMapEntry virt_memmap[] = {
>      [VIRT_DEBUG] =        {        0x0,         0x100 },
>      [VIRT_MROM] =         {     0x1000,        0xf000 },
> @@ -725,14 +731,12 @@ static void create_fdt_sockets(RISCVVirtState *s, const MemMapEntry *memmap,
>
>          create_fdt_socket_memory(s, memmap, socket);
>
> -        if (tcg_enabled()) {
> -            if (s->have_aclint) {
> -                create_fdt_socket_aclint(s, memmap, socket,
> -                    &intc_phandles[phandle_pos]);
> -            } else {
> -                create_fdt_socket_clint(s, memmap, socket,
> -                    &intc_phandles[phandle_pos]);
> -            }
> +        if (virt_aclint_allowed() && s->have_aclint) {
> +            create_fdt_socket_aclint(s, memmap, socket,
> +                                     &intc_phandles[phandle_pos]);
> +        } else if (tcg_enabled()) {
> +            create_fdt_socket_clint(s, memmap, socket,
> +                                    &intc_phandles[phandle_pos]);
>          }
>      }
>
> @@ -1409,7 +1413,7 @@ static void virt_machine_init(MachineState *machine)
>          exit(1);
>      }
>
> -    if (!tcg_enabled() && s->have_aclint) {
> +    if (!virt_aclint_allowed() && s->have_aclint) {
>          error_report("'aclint' is only available with TCG acceleration");
>          exit(1);
>      }
> @@ -1446,23 +1450,22 @@ static void virt_machine_init(MachineState *machine)
>                                  hart_count, &error_abort);
>          sysbus_realize(SYS_BUS_DEVICE(&s->soc[i]), &error_fatal);
>
> -        if (tcg_enabled()) {
> -            if (s->have_aclint) {
> -                if (s->aia_type == VIRT_AIA_TYPE_APLIC_IMSIC) {
> -                    /* Per-socket ACLINT MTIMER */
> -                    riscv_aclint_mtimer_create(memmap[VIRT_CLINT].base +
> +        if (virt_aclint_allowed() && s->have_aclint) {
> +            if (s->aia_type == VIRT_AIA_TYPE_APLIC_IMSIC) {
> +                /* Per-socket ACLINT MTIMER */
> +                riscv_aclint_mtimer_create(memmap[VIRT_CLINT].base +
>                              i * RISCV_ACLINT_DEFAULT_MTIMER_SIZE,
>                          RISCV_ACLINT_DEFAULT_MTIMER_SIZE,
>                          base_hartid, hart_count,
>                          RISCV_ACLINT_DEFAULT_MTIMECMP,
>                          RISCV_ACLINT_DEFAULT_MTIME,
>                          RISCV_ACLINT_DEFAULT_TIMEBASE_FREQ, true);
> -                } else {
> -                    /* Per-socket ACLINT MSWI, MTIMER, and SSWI */
> -                    riscv_aclint_swi_create(memmap[VIRT_CLINT].base +
> +            } else {
> +                /* Per-socket ACLINT MSWI, MTIMER, and SSWI */
> +                riscv_aclint_swi_create(memmap[VIRT_CLINT].base +
>                              i * memmap[VIRT_CLINT].size,
>                          base_hartid, hart_count, false);
> -                    riscv_aclint_mtimer_create(memmap[VIRT_CLINT].base +
> +                riscv_aclint_mtimer_create(memmap[VIRT_CLINT].base +
>                              i * memmap[VIRT_CLINT].size +
>                              RISCV_ACLINT_SWI_SIZE,
>                          RISCV_ACLINT_DEFAULT_MTIMER_SIZE,
> @@ -1470,21 +1473,20 @@ static void virt_machine_init(MachineState *machine)
>                          RISCV_ACLINT_DEFAULT_MTIMECMP,
>                          RISCV_ACLINT_DEFAULT_MTIME,
>                          RISCV_ACLINT_DEFAULT_TIMEBASE_FREQ, true);
> -                    riscv_aclint_swi_create(memmap[VIRT_ACLINT_SSWI].base +
> +                riscv_aclint_swi_create(memmap[VIRT_ACLINT_SSWI].base +
>                              i * memmap[VIRT_ACLINT_SSWI].size,
>                          base_hartid, hart_count, true);
> -                }
> -            } else {
> -                /* Per-socket SiFive CLINT */
> -                riscv_aclint_swi_create(
> +            }
> +        } else if (tcg_enabled()) {
> +            /* Per-socket SiFive CLINT */
> +            riscv_aclint_swi_create(
>                      memmap[VIRT_CLINT].base + i * memmap[VIRT_CLINT].size,
>                      base_hartid, hart_count, false);
> -                riscv_aclint_mtimer_create(memmap[VIRT_CLINT].base +
> +            riscv_aclint_mtimer_create(memmap[VIRT_CLINT].base +
>                          i * memmap[VIRT_CLINT].size + RISCV_ACLINT_SWI_SIZE,
>                      RISCV_ACLINT_DEFAULT_MTIMER_SIZE, base_hartid, hart_count,
>                      RISCV_ACLINT_DEFAULT_MTIMECMP, RISCV_ACLINT_DEFAULT_MTIME,
>                      RISCV_ACLINT_DEFAULT_TIMEBASE_FREQ, true);
> -            }
>          }
>
>          /* Per-socket interrupt controller */
> --
> 2.43.0
>
>

[PATCH 6/6] tests/libqos: add riscv/virt machine nodes

[Daniel Henrique Barboza]

Add a RISC-V 'virt' machine to the graph. This implementation is a
modified copy of the existing arm machine in arm-virt-machine.c

It contains a virtio-mmio and a generic-pcihost controller. The
generic-pcihost controller hardcodes assumptions from the ARM 'virt'
machine, like ecam and pio_base addresses, so we'll add an extra step to
set its parameters after creating it.

Our command line is incremented with 'aclint' parameters to allow the
machine to run MSI tests.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 tests/qtest/libqos/meson.build          |   1 +
 tests/qtest/libqos/riscv-virt-machine.c | 137 ++++++++++++++++++++++++
 2 files changed, 138 insertions(+)
 create mode 100644 tests/qtest/libqos/riscv-virt-machine.c

diff --git a/tests/qtest/libqos/meson.build b/tests/qtest/libqos/meson.build
index 90aae42a22..3aed6efcb8 100644
--- a/tests/qtest/libqos/meson.build
+++ b/tests/qtest/libqos/meson.build
@@ -60,6 +60,7 @@ libqos_srcs = files(
         'arm-xilinx-zynq-a9-machine.c',
         'ppc64_pseries-machine.c',
         'x86_64_pc-machine.c',
+        'riscv-virt-machine.c',
 )
 
 if have_virtfs
diff --git a/tests/qtest/libqos/riscv-virt-machine.c b/tests/qtest/libqos/riscv-virt-machine.c
new file mode 100644
index 0000000000..c4364c9c5d
--- /dev/null
+++ b/tests/qtest/libqos/riscv-virt-machine.c
@@ -0,0 +1,137 @@
+/*
+ * libqos driver framework for risc-v
+ *
+ * Initial version based on arm-virt-machine.c
+ *
+ * Copyright (c) 2024 Ventana Micro
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License version 2.1 as published by the Free Software Foundation.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>
+ */
+
+#include "qemu/osdep.h"
+#include "../libqtest.h"
+#include "qemu/module.h"
+#include "libqos-malloc.h"
+#include "qgraph.h"
+#include "virtio-mmio.h"
+#include "generic-pcihost.h"
+#include "hw/pci/pci_regs.h"
+
+#define RISCV_PAGE_SIZE            4096
+
+/* VIRT_DRAM */
+#define RISCV_VIRT_RAM_ADDR        0x80000000
+#define RISCV_VIRT_RAM_SIZE        0x20000000
+
+/*
+ * VIRT_VIRTIO. BASE_ADDR  points to the last
+ * virtio_mmio device.
+ */
+#define VIRTIO_MMIO_BASE_ADDR      0x10008000
+#define VIRTIO_MMIO_SIZE           0x00001000
+
+/* VIRT_PCIE_PIO  */
+#define RISCV_GPEX_PIO_BASE        0x3000000
+#define RISCV_BUS_PIO_LIMIT        0x10000
+
+/* VIRT_PCIE_MMIO */
+#define RISCV_BUS_MMIO_ALLOC_PTR   0x40000000
+#define RISCV_BUS_MMIO_LIMIT       0x80000000
+
+/* VIRT_PCIE_ECAM */
+#define RISCV_ECAM_ALLOC_PTR   0x30000000
+
+typedef struct QVirtMachine QVirtMachine;
+
+struct QVirtMachine {
+    QOSGraphObject obj;
+    QGuestAllocator alloc;
+    QVirtioMMIODevice virtio_mmio;
+    QGenericPCIHost bridge;
+};
+
+static void virt_destructor(QOSGraphObject *obj)
+{
+    QVirtMachine *machine = (QVirtMachine *) obj;
+    alloc_destroy(&machine->alloc);
+}
+
+static void *virt_get_driver(void *object, const char *interface)
+{
+    QVirtMachine *machine = object;
+    if (!g_strcmp0(interface, "memory")) {
+        return &machine->alloc;
+    }
+
+    fprintf(stderr, "%s not present in riscv/virtio\n", interface);
+    g_assert_not_reached();
+}
+
+static QOSGraphObject *virt_get_device(void *obj, const char *device)
+{
+    QVirtMachine *machine = obj;
+    if (!g_strcmp0(device, "generic-pcihost")) {
+        return &machine->bridge.obj;
+    } else if (!g_strcmp0(device, "virtio-mmio")) {
+        return &machine->virtio_mmio.obj;
+    }
+
+    fprintf(stderr, "%s not present in riscv/virt\n", device);
+    g_assert_not_reached();
+}
+
+static void riscv_config_qpci_bus(QGenericPCIBus *qpci)
+{
+    qpci->gpex_pio_base = RISCV_GPEX_PIO_BASE;
+    qpci->bus.pio_limit = RISCV_BUS_PIO_LIMIT;
+
+    qpci->bus.mmio_alloc_ptr = RISCV_BUS_MMIO_ALLOC_PTR;
+    qpci->bus.mmio_limit = RISCV_BUS_MMIO_LIMIT;
+
+    qpci->ecam_alloc_ptr = RISCV_ECAM_ALLOC_PTR;
+}
+
+static void *qos_create_machine_riscv_virt(QTestState *qts)
+{
+    QVirtMachine *machine = g_new0(QVirtMachine, 1);
+
+    alloc_init(&machine->alloc, 0,
+               RISCV_VIRT_RAM_ADDR,
+               RISCV_VIRT_RAM_ADDR + RISCV_VIRT_RAM_SIZE,
+               RISCV_PAGE_SIZE);
+    qvirtio_mmio_init_device(&machine->virtio_mmio, qts, VIRTIO_MMIO_BASE_ADDR,
+                              VIRTIO_MMIO_SIZE);
+
+    qos_create_generic_pcihost(&machine->bridge, qts, &machine->alloc);
+    riscv_config_qpci_bus(&machine->bridge.pci);
+
+    machine->obj.get_device = virt_get_device;
+    machine->obj.get_driver = virt_get_driver;
+    machine->obj.destructor = virt_destructor;
+    return machine;
+}
+
+static void virt_machine_register_nodes(void)
+{
+    qos_node_create_machine_args("riscv32/virt", qos_create_machine_riscv_virt,
+                                 "aclint=on,aia=aplic-imsic");
+    qos_node_contains("riscv32/virt", "virtio-mmio", NULL);
+    qos_node_contains("riscv32/virt", "generic-pcihost", NULL);
+
+    qos_node_create_machine_args("riscv64/virt", qos_create_machine_riscv_virt,
+                                 "aclint=on,aia=aplic-imsic");
+    qos_node_contains("riscv64/virt", "virtio-mmio", NULL);
+    qos_node_contains("riscv64/virt", "generic-pcihost", NULL);
+}
+
+libqos_init(virt_machine_register_nodes);
-- 
2.43.0

Re: [PATCH 6/6] tests/libqos: add riscv/virt machine nodes

[Alistair Francis]

On Wed, Feb 14, 2024 at 5:18 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Add a RISC-V 'virt' machine to the graph. This implementation is a
> modified copy of the existing arm machine in arm-virt-machine.c
>
> It contains a virtio-mmio and a generic-pcihost controller. The
> generic-pcihost controller hardcodes assumptions from the ARM 'virt'
> machine, like ecam and pio_base addresses, so we'll add an extra step to
> set its parameters after creating it.
>
> Our command line is incremented with 'aclint' parameters to allow the
> machine to run MSI tests.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

Acked-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  tests/qtest/libqos/meson.build          |   1 +
>  tests/qtest/libqos/riscv-virt-machine.c | 137 ++++++++++++++++++++++++
>  2 files changed, 138 insertions(+)
>  create mode 100644 tests/qtest/libqos/riscv-virt-machine.c
>
> diff --git a/tests/qtest/libqos/meson.build b/tests/qtest/libqos/meson.build
> index 90aae42a22..3aed6efcb8 100644
> --- a/tests/qtest/libqos/meson.build
> +++ b/tests/qtest/libqos/meson.build
> @@ -60,6 +60,7 @@ libqos_srcs = files(
>          'arm-xilinx-zynq-a9-machine.c',
>          'ppc64_pseries-machine.c',
>          'x86_64_pc-machine.c',
> +        'riscv-virt-machine.c',
>  )
>
>  if have_virtfs
> diff --git a/tests/qtest/libqos/riscv-virt-machine.c b/tests/qtest/libqos/riscv-virt-machine.c
> new file mode 100644
> index 0000000000..c4364c9c5d
> --- /dev/null
> +++ b/tests/qtest/libqos/riscv-virt-machine.c
> @@ -0,0 +1,137 @@
> +/*
> + * libqos driver framework for risc-v
> + *
> + * Initial version based on arm-virt-machine.c
> + *
> + * Copyright (c) 2024 Ventana Micro
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License version 2.1 as published by the Free Software Foundation.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see <http://www.gnu.org/licenses/>
> + */
> +
> +#include "qemu/osdep.h"
> +#include "../libqtest.h"
> +#include "qemu/module.h"
> +#include "libqos-malloc.h"
> +#include "qgraph.h"
> +#include "virtio-mmio.h"
> +#include "generic-pcihost.h"
> +#include "hw/pci/pci_regs.h"
> +
> +#define RISCV_PAGE_SIZE            4096
> +
> +/* VIRT_DRAM */
> +#define RISCV_VIRT_RAM_ADDR        0x80000000
> +#define RISCV_VIRT_RAM_SIZE        0x20000000
> +
> +/*
> + * VIRT_VIRTIO. BASE_ADDR  points to the last
> + * virtio_mmio device.
> + */
> +#define VIRTIO_MMIO_BASE_ADDR      0x10008000
> +#define VIRTIO_MMIO_SIZE           0x00001000
> +
> +/* VIRT_PCIE_PIO  */
> +#define RISCV_GPEX_PIO_BASE        0x3000000
> +#define RISCV_BUS_PIO_LIMIT        0x10000
> +
> +/* VIRT_PCIE_MMIO */
> +#define RISCV_BUS_MMIO_ALLOC_PTR   0x40000000
> +#define RISCV_BUS_MMIO_LIMIT       0x80000000
> +
> +/* VIRT_PCIE_ECAM */
> +#define RISCV_ECAM_ALLOC_PTR   0x30000000
> +
> +typedef struct QVirtMachine QVirtMachine;
> +
> +struct QVirtMachine {
> +    QOSGraphObject obj;
> +    QGuestAllocator alloc;
> +    QVirtioMMIODevice virtio_mmio;
> +    QGenericPCIHost bridge;
> +};
> +
> +static void virt_destructor(QOSGraphObject *obj)
> +{
> +    QVirtMachine *machine = (QVirtMachine *) obj;
> +    alloc_destroy(&machine->alloc);
> +}
> +
> +static void *virt_get_driver(void *object, const char *interface)
> +{
> +    QVirtMachine *machine = object;
> +    if (!g_strcmp0(interface, "memory")) {
> +        return &machine->alloc;
> +    }
> +
> +    fprintf(stderr, "%s not present in riscv/virtio\n", interface);
> +    g_assert_not_reached();
> +}
> +
> +static QOSGraphObject *virt_get_device(void *obj, const char *device)
> +{
> +    QVirtMachine *machine = obj;
> +    if (!g_strcmp0(device, "generic-pcihost")) {
> +        return &machine->bridge.obj;
> +    } else if (!g_strcmp0(device, "virtio-mmio")) {
> +        return &machine->virtio_mmio.obj;
> +    }
> +
> +    fprintf(stderr, "%s not present in riscv/virt\n", device);
> +    g_assert_not_reached();
> +}
> +
> +static void riscv_config_qpci_bus(QGenericPCIBus *qpci)
> +{
> +    qpci->gpex_pio_base = RISCV_GPEX_PIO_BASE;
> +    qpci->bus.pio_limit = RISCV_BUS_PIO_LIMIT;
> +
> +    qpci->bus.mmio_alloc_ptr = RISCV_BUS_MMIO_ALLOC_PTR;
> +    qpci->bus.mmio_limit = RISCV_BUS_MMIO_LIMIT;
> +
> +    qpci->ecam_alloc_ptr = RISCV_ECAM_ALLOC_PTR;
> +}
> +
> +static void *qos_create_machine_riscv_virt(QTestState *qts)
> +{
> +    QVirtMachine *machine = g_new0(QVirtMachine, 1);
> +
> +    alloc_init(&machine->alloc, 0,
> +               RISCV_VIRT_RAM_ADDR,
> +               RISCV_VIRT_RAM_ADDR + RISCV_VIRT_RAM_SIZE,
> +               RISCV_PAGE_SIZE);
> +    qvirtio_mmio_init_device(&machine->virtio_mmio, qts, VIRTIO_MMIO_BASE_ADDR,
> +                              VIRTIO_MMIO_SIZE);
> +
> +    qos_create_generic_pcihost(&machine->bridge, qts, &machine->alloc);
> +    riscv_config_qpci_bus(&machine->bridge.pci);
> +
> +    machine->obj.get_device = virt_get_device;
> +    machine->obj.get_driver = virt_get_driver;
> +    machine->obj.destructor = virt_destructor;
> +    return machine;
> +}
> +
> +static void virt_machine_register_nodes(void)
> +{
> +    qos_node_create_machine_args("riscv32/virt", qos_create_machine_riscv_virt,
> +                                 "aclint=on,aia=aplic-imsic");
> +    qos_node_contains("riscv32/virt", "virtio-mmio", NULL);
> +    qos_node_contains("riscv32/virt", "generic-pcihost", NULL);
> +
> +    qos_node_create_machine_args("riscv64/virt", qos_create_machine_riscv_virt,
> +                                 "aclint=on,aia=aplic-imsic");
> +    qos_node_contains("riscv64/virt", "virtio-mmio", NULL);
> +    qos_node_contains("riscv64/virt", "generic-pcihost", NULL);
> +}
> +
> +libqos_init(virt_machine_register_nodes);
> --
> 2.43.0
>
>

Re: [PATCH 6/6] tests/libqos: add riscv/virt machine nodes

[Thomas Huth]

On 13/02/2024 20.17, Daniel Henrique Barboza wrote:
> Add a RISC-V 'virt' machine to the graph. This implementation is a
> modified copy of the existing arm machine in arm-virt-machine.c
> 
> It contains a virtio-mmio and a generic-pcihost controller. The
> generic-pcihost controller hardcodes assumptions from the ARM 'virt'
> machine, like ecam and pio_base addresses, so we'll add an extra step to
> set its parameters after creating it.
> 
> Our command line is incremented with 'aclint' parameters to allow the
> machine to run MSI tests.
> 
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>   tests/qtest/libqos/meson.build          |   1 +
>   tests/qtest/libqos/riscv-virt-machine.c | 137 ++++++++++++++++++++++++
>   2 files changed, 138 insertions(+)
>   create mode 100644 tests/qtest/libqos/riscv-virt-machine.c

Acked-by: Thomas Huth <thuth@redhat.com>