Re: [Qemu-block] [Qemu-devel] [PATCH v6 16/29] libqos: Use explicit QTestState for virtio operations
On 09/05/2017 07:43 AM, Paolo Bonzini wrote:
>> typedef struct QVirtioDevice {
>> const QVirtioBus *bus;
>> +QTestState *qts;
>> /* Device type */
>> uint16_t device_type;
>> } QVirtioDevice;
>> @@ -35,12 +36,14 @@ typedef struct QVirtQueue {
>> uint16_t last_used_idx;
>> bool indirect;
>> bool event;
>> +QTestState *qts;
>> } QVirtQueue;
>>
>> typedef struct QVRingIndirectDesc {
>> uint64_t desc; /* This points to an array fo struct vring_desc */
>> uint16_t index;
>> uint16_t elem;
>> +QTestState *qts;
>> } QVRingIndirectDesc;
>
> Adding QTestState to QVRingIndirectDesc and QVirtQueue sounds somewhat
> ugly to me. I think they should either rather have a pointer to the
> associated QVirtioDevice, or the functions where this is needed
> (qvring_init() for example) should get a "QTestState *" parameter instead.
>
>
> I agree with Thomas and even extend it to QVirtioDevice. If there is a
> has-a hierarchy between libqos objects, only the topmost one (such as the
> virtio mmio bus device, and the PCI host bridge---or possibly even the
> machine object exclusively) should have a QTestState. Everyone else can
> access it implicitly through operations on its bus.
[Paolo, gmail is really messing up with your quoting style ;( ]
Okay, so if I understand correctly, at most a QVRingIndirectDesc can
have a pointer back to the QVirtioDevice that owns it, and in turn the
QVirtioDevice has a pointer back to the QPCIBus that the device is on,
and anywhere that needs a QTestState follows the chain back to the
QPCIBus to get it. I'll play with the idea, as it may still allow for
more compact code than passing an explicit QTestState parameter through
every call.
>
> Test code on the other hand can use global_qtest implicitly when they
> prepare/read buffers that the devices do DMA from/to.
Not when I'm done - my next series gets rid of implicit global_qtest
everywhere.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-block] [Qemu-devel] [PATCH v6 16/29] libqos: Use explicit QTestState for virtio operations
> typedef struct QVirtioDevice {
> const QVirtioBus *bus;
> +QTestState *qts;
> /* Device type */
> uint16_t device_type;
> } QVirtioDevice;
> @@ -35,12 +36,14 @@ typedef struct QVirtQueue {
> uint16_t last_used_idx;
> bool indirect;
> bool event;
> +QTestState *qts;
> } QVirtQueue;
>
> typedef struct QVRingIndirectDesc {
> uint64_t desc; /* This points to an array fo struct vring_desc */
> uint16_t index;
> uint16_t elem;
> +QTestState *qts;
> } QVRingIndirectDesc;
Adding QTestState to QVRingIndirectDesc and QVirtQueue sounds somewhat
ugly to me. I think they should either rather have a pointer to the
associated QVirtioDevice, or the functions where this is needed
(qvring_init() for example) should get a "QTestState *" parameter instead.
I agree with Thomas and even extend it to QVirtioDevice. If there is a
has-a hierarchy between libqos objects, only the topmost one (such as the
virtio mmio bus device, and the PCI host bridge---or possibly even the
machine object exclusively) should have a QTestState. Everyone else can
access it implicitly through operations on its bus.
Test code on the other hand can use global_qtest implicitly when they
prepare/read buffers that the devices do DMA from/to.
Paolo
Just my 0.02 €.
Thomas
Re: [Qemu-block] [Qemu-devel] [PATCH v6 16/29] libqos: Use explicit QTestState for virtio operations
On 01.09.2017 20:03, Eric Blake wrote:
> Drop one more client of global_qtest by teaching all virtio test
> functionality to pass in an explicit QTestState in constructors,
> where it is then reused for later access. Adjust all callers.
> This gets us one step closer to eliminating implicit use of
> global_qtest.
>
> Signed-off-by: Eric Blake
> ---
> tests/libqos/virtio-mmio.h | 3 +-
> tests/libqos/virtio.h | 5 ++-
> tests/libqos/virtio-mmio.c | 54 +---
> tests/libqos/virtio-pci.c | 3 ++
> tests/libqos/virtio.c | 77
> ++
> tests/virtio-blk-test.c| 3 +-
> 6 files changed, 84 insertions(+), 61 deletions(-)
>
> diff --git a/tests/libqos/virtio-mmio.h b/tests/libqos/virtio-mmio.h
> index e3e52b9ce1..bd01386054 100644
> --- a/tests/libqos/virtio-mmio.h
> +++ b/tests/libqos/virtio-mmio.h
> @@ -41,6 +41,7 @@ typedef struct QVirtioMMIODevice {
>
> extern const QVirtioBus qvirtio_mmio;
>
> -QVirtioMMIODevice *qvirtio_mmio_init_device(uint64_t addr, uint32_t
> page_size);
> +QVirtioMMIODevice *qvirtio_mmio_init_device(QTestState *qts, uint64_t addr,
> +uint32_t page_size);
>
> #endif
> diff --git a/tests/libqos/virtio.h b/tests/libqos/virtio.h
> index 8fbcd1869c..d180d54fc4 100644
> --- a/tests/libqos/virtio.h
> +++ b/tests/libqos/virtio.h
> @@ -19,6 +19,7 @@ typedef struct QVirtioBus QVirtioBus;
>
> typedef struct QVirtioDevice {
> const QVirtioBus *bus;
> +QTestState *qts;
> /* Device type */
> uint16_t device_type;
> } QVirtioDevice;
> @@ -35,12 +36,14 @@ typedef struct QVirtQueue {
> uint16_t last_used_idx;
> bool indirect;
> bool event;
> +QTestState *qts;
> } QVirtQueue;
>
> typedef struct QVRingIndirectDesc {
> uint64_t desc; /* This points to an array fo struct vring_desc */
> uint16_t index;
> uint16_t elem;
> +QTestState *qts;
> } QVRingIndirectDesc;
Adding QTestState to QVRingIndirectDesc and QVirtQueue sounds somewhat
ugly to me. I think they should either rather have a pointer to the
associated QVirtioDevice, or the functions where this is needed
(qvring_init() for example) should get a "QTestState *" parameter instead.
Just my 0.02 €.
Thomas
