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