Re: [Qemu-devel] [PATCH 1/6] tests/libqos: Make generic virtio code independent from global_qtest

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3652 bytes --]

On 8/13/19 4:30 AM, Thomas Huth wrote:
> The libqos library functions should never depend on global_qtest,
> since these functions might be used in tests that track multiple
> test states. Pass around a pointer to the QTestState instead.

Thanks for picking up on my initial RFC along these lines a couple years
ago now; it's taking us a while, but these cleanups are worthwhile.

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

> @@ -193,7 +193,7 @@ void qvring_init(QTestState *qts, const QGuestAllocator *alloc, QVirtQueue *vq,
>                   0);
>  }
>  
> -QVRingIndirectDesc *qvring_indirect_desc_setup(QVirtioDevice *d,
> +QVRingIndirectDesc *qvring_indirect_desc_setup(QTestState *qs, QVirtioDevice *d,
>                                          QGuestAllocator *alloc, uint16_t elem)
>  {

Pre-existing, but indentation is off; maybe you want to fix that while here.


> -void qvirtqueue_kick(QVirtioDevice *d, QVirtQueue *vq, uint32_t free_head)
> +void qvirtqueue_kick(QTestState *qts, QVirtioDevice *d, QVirtQueue *vq,
> +                     uint32_t free_head)
>  {
>      /* vq->avail->idx */
> -    uint16_t idx = readw(vq->avail + 2);
> +    uint16_t idx = qtest_readw(qts, vq->avail + 2);
>      /* vq->used->flags */
>      uint16_t flags;
>      /* vq->used->avail_event */
>      uint16_t avail_event;
>  
>      /* vq->avail->ring[idx % vq->size] */
> -    writew(vq->avail + 4 + (2 * (idx % vq->size)), free_head);
> +    qtest_writew(qts, vq->avail + 4 + (2 * (idx % vq->size)), free_head);
>      /* vq->avail->idx */
> -    writew(vq->avail + 2, idx + 1);
> +    qtest_writew(qts, vq->avail + 2, idx + 1);
>  
>      /* Must read after idx is updated */
> -    flags = readw(vq->avail);
> -    avail_event = readw(vq->used + 4 +
> +    flags = qtest_readw(qts, vq->avail);
> +    avail_event = qtest_readw(qts, vq->used + 4 +
>                                  sizeof(struct vring_used_elem) * vq->size);

Here as well.

> @@ -333,23 +337,25 @@ bool qvirtqueue_get_buf(QVirtQueue *vq, uint32_t *desc_idx, uint32_t *len)
>          sizeof(struct vring_used_elem);
>  
>      if (desc_idx) {
> -        *desc_idx = readl(elem_addr + offsetof(struct vring_used_elem, id));
> +        *desc_idx = qtest_readl(qts, elem_addr +
> +                                     offsetof(struct vring_used_elem, id));

This one works as-is, although it gives me a double-take when the
indentation is not right after a ( in the line above.  I probably would
have written either:

        *desc_idx = qtest_readl(qts,
                                elem_addr + offsetof(struct
vring_used_elem, id));

or

        *desc_idx = qtest_readl(qts, (elem_addr +
                                      offsetof(struct vring_used_elem,
id)));

or even introduce a temp variable to bring down the line length.


> @@ -131,17 +131,20 @@ void qvirtqueue_cleanup(const QVirtioBus *bus, QVirtQueue *vq,
>  
>  void qvring_init(QTestState *qts, const QGuestAllocator *alloc, QVirtQueue *vq,
>                   uint64_t addr);
> -QVRingIndirectDesc *qvring_indirect_desc_setup(QVirtioDevice *d,
> +QVRingIndirectDesc *qvring_indirect_desc_setup(QTestState *qs, QVirtioDevice *d,
>                                          QGuestAllocator *alloc, uint16_t elem);

Another pre-existing odd indentation (here, it looks like line length
was the reason).

Indentation is minor, so:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]