| Message ID | 20220524154056.2896913-1-alex.bennee@linaro.org |
|---|---|
| Headers | show |
| Series | virtio-gpio and various virtio cleanups | expand |
On 24/05/2022 17.40, Alex Bennée wrote: > When trying to work out what the virtio-net-tests where doing it was > hard because the g_test_trap_subprocess redirects all output to > /dev/null. Lift this restriction by using the appropriate flags so you > can see something similar to what the vhost-user-blk tests show when > running. > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > Message-Id: <20220407150042.2338562-1-alex.bennee@linaro.org> > > --- > v2 > - keep dumping of CLI behind the g_test_verbose flag > --- > tests/qtest/qos-test.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/tests/qtest/qos-test.c b/tests/qtest/qos-test.c > index f97d0a08fd..7e1c8fc579 100644 > --- a/tests/qtest/qos-test.c > +++ b/tests/qtest/qos-test.c > @@ -185,7 +185,8 @@ static void run_one_test(const void *arg) > static void subprocess_run_one_test(const void *arg) > { > const gchar *path = arg; > - g_test_trap_subprocess(path, 0, 0); > + g_test_trap_subprocess(path, 0, > + G_TEST_SUBPROCESS_INHERIT_STDOUT | G_TEST_SUBPROCESS_INHERIT_STDERR); > g_test_trap_assert_passed(); > } > Acked-by: Thomas Huth <thuth@redhat.com>
On 24/05/2022 17.40, Alex Bennée wrote: > Hangs have been observed in the tests and currently we don't timeout > if a subprocess hangs. Rectify that. > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > --- > tests/qtest/qos-test.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tests/qtest/qos-test.c b/tests/qtest/qos-test.c > index 7e1c8fc579..46623da731 100644 > --- a/tests/qtest/qos-test.c > +++ b/tests/qtest/qos-test.c > @@ -185,7 +185,7 @@ static void run_one_test(const void *arg) > static void subprocess_run_one_test(const void *arg) > { > const gchar *path = arg; > - g_test_trap_subprocess(path, 0, > + g_test_trap_subprocess(path, 60 * G_USEC_PER_SEC, 60 seconds is not that much for a slow test running on a slow and overloaded CI host ... maybe rather go for 180 seconds or even more? Thomas
On Tue, May 24, 2022 at 04:40:41PM +0100, Alex Bennée wrote: > Hi, > > This series ostensibly adds virtio-user-gpio stubs to the build for > use with an external vhost-user daemon. We've been testing it with our > rust daemons from: > > https://github.com/rust-vmm/vhost-device > > Getting the test enabled took some doing most likely because the need > for CONFIG support exercised additional paths in the code that were > not used for the simpler virtio-net tests. As a result the series has > a number of cleanup and documentation patches. > > The final thing that needed fixing was the ensuring that > VHOST_USER_F_PROTOCOL_FEATURES didn't get squashed in the negotiation > process. This was the hardest thing to track down as we store the > feature bits in several places variously as: > > in VirtIODevice as: > uint64_t guest_features; > uint64_t host_features; > uint64_t backend_features; None of these know about VHOST_USER_F_PROTOCOL_FEATURES and vhost-user's unfiltered feature bits should never be passed to VirtIODevice. > > in vhost_dev as: > uint64_t features; > uint64_t acked_features; > uint64_t backend_features; I don't think these should know about VHOST_USER_F_PROTOCOL_FEATURES either. AFAIK vhost_dev deals with VIRTIO feature bits, not raw vhost-user GET_FEATURES. Stefan
Stefan Hajnoczi <stefanha@redhat.com> writes: > [[PGP Signed Part:Undecided]] > On Tue, May 24, 2022 at 04:40:41PM +0100, Alex Bennée wrote: >> Hi, >> >> This series ostensibly adds virtio-user-gpio stubs to the build for >> use with an external vhost-user daemon. We've been testing it with our >> rust daemons from: >> >> https://github.com/rust-vmm/vhost-device >> >> Getting the test enabled took some doing most likely because the need >> for CONFIG support exercised additional paths in the code that were >> not used for the simpler virtio-net tests. As a result the series has >> a number of cleanup and documentation patches. >> >> The final thing that needed fixing was the ensuring that >> VHOST_USER_F_PROTOCOL_FEATURES didn't get squashed in the negotiation >> process. This was the hardest thing to track down as we store the >> feature bits in several places variously as: >> >> in VirtIODevice as: >> uint64_t guest_features; >> uint64_t host_features; >> uint64_t backend_features; > > None of these know about VHOST_USER_F_PROTOCOL_FEATURES and vhost-user's > unfiltered feature bits should never be passed to VirtIODevice. > >> >> in vhost_dev as: >> uint64_t features; >> uint64_t acked_features; >> uint64_t backend_features; > > I don't think these should know about VHOST_USER_F_PROTOCOL_FEATURES > either. AFAIK vhost_dev deals with VIRTIO feature bits, not raw > vhost-user GET_FEATURES. So where does VHOST_USER_F_PROTOCOL_FEATURES get set before it's set with the VHOST_USER_SET_FEATURES message? Currently it's fed via: uint64_t features = vhost_dev->acked_features; in vhost_dev_set_features() which does apply a few extra bits (VHOST_F_LOG_ALL/VIRTIO_F_IOMMU_PLATFORM). Maybe it should be adding VHOST_USER_F_PROTOCOL_FEATURES here? How should it be signalled by the vhost-user backend that this should be done? Overload the function? > > Stefan > > [[End of PGP Signed Part]]
On Thu, 7 Jul 2022 at 14:42, Alex Bennée <alex.bennee@linaro.org> wrote: > > > Stefan Hajnoczi <stefanha@redhat.com> writes: > > > [[PGP Signed Part:Undecided]] > > On Tue, May 24, 2022 at 04:40:41PM +0100, Alex Bennée wrote: > >> Hi, > >> > >> This series ostensibly adds virtio-user-gpio stubs to the build for > >> use with an external vhost-user daemon. We've been testing it with our > >> rust daemons from: > >> > >> https://github.com/rust-vmm/vhost-device > >> > >> Getting the test enabled took some doing most likely because the need > >> for CONFIG support exercised additional paths in the code that were > >> not used for the simpler virtio-net tests. As a result the series has > >> a number of cleanup and documentation patches. > >> > >> The final thing that needed fixing was the ensuring that > >> VHOST_USER_F_PROTOCOL_FEATURES didn't get squashed in the negotiation > >> process. This was the hardest thing to track down as we store the > >> feature bits in several places variously as: > >> > >> in VirtIODevice as: > >> uint64_t guest_features; > >> uint64_t host_features; > >> uint64_t backend_features; > > > > None of these know about VHOST_USER_F_PROTOCOL_FEATURES and vhost-user's > > unfiltered feature bits should never be passed to VirtIODevice. > > > >> > >> in vhost_dev as: > >> uint64_t features; > >> uint64_t acked_features; > >> uint64_t backend_features; > > > > I don't think these should know about VHOST_USER_F_PROTOCOL_FEATURES > > either. AFAIK vhost_dev deals with VIRTIO feature bits, not raw > > vhost-user GET_FEATURES. > > So where does VHOST_USER_F_PROTOCOL_FEATURES get set before it's set > with the VHOST_USER_SET_FEATURES message? Currently it's fed via: > > uint64_t features = vhost_dev->acked_features; > > in vhost_dev_set_features() which does apply a few extra bits > (VHOST_F_LOG_ALL/VIRTIO_F_IOMMU_PLATFORM). Maybe it should be adding > VHOST_USER_F_PROTOCOL_FEATURES here? How should it be signalled by the > vhost-user backend that this should be done? Overload the function? A modern vhost-user server replies to VHOST_USER_GET_FEATURES with VHOST_USER_F_PROTOCOL_FEATURES set. That's when the vhost-user client encounters this bit. The vhost-user client should then filter out VHOST_USER_F_PROTOCOL_FEATURES because it belongs to the vhost-user protocol and isn't a real VIRTIO feature bit. The client uses the filtered VIRTIO feature bits and it now knows whether the vhost-user server supports the VHOST_USER_GET_PROTOCOL_FEATURES and VHOST_USER_SET_PROTOCOL_FEATURES messages. I think vhost_user_set_features() should set VHOST_USER_F_PROTOCOL_FEATURES if the server returned it from VHOST_USER_GET_FEATURES. At the moment vhost_user_backend_init() stores VHOST_USER_F_PROTOCOL_FEATURES in struct vhost_dev->backend_features, which only seems to be used by vhost-net code. The other vhost-user devices set acked_features = guest_features and ignore backend_features. As a result I guess they don't set VHOST_USER_F_PROTOCOL_FEATURES in the VHOST_USER_SET_FEATURES message. Most vhost-user servers probably don't care and still respond to VHOST_USER_GET_PROTOCOL_FEATURES and VHOST_USER_SET_PROTOCOL_FEATURES messages (although the vhost-user protocol spec mentions other protocol differences when VHOST_USER_F_PROTOCOL_FEATURES is not negotiated). Does this match what you've found? The code is a maze so I may have gotten something wrong. In general I think hw/virtio/vhost-user.c should be responsible for VHOST_USER_F_PROTOCOL_FEATURES and no other part of the QEMU codebase should ever see the bit since it's a vhost-user protocol detail and not part of VIRTIO or even a common part of vhost. Stefan
Stefan Hajnoczi <stefanha@gmail.com> writes: > On Thu, 7 Jul 2022 at 14:42, Alex Bennée <alex.bennee@linaro.org> wrote: >> >> >> Stefan Hajnoczi <stefanha@redhat.com> writes: >> >> > [[PGP Signed Part:Undecided]] >> > On Tue, May 24, 2022 at 04:40:41PM +0100, Alex Bennée wrote: >> >> Hi, >> >> >> >> This series ostensibly adds virtio-user-gpio stubs to the build for >> >> use with an external vhost-user daemon. We've been testing it with our >> >> rust daemons from: >> >> >> >> https://github.com/rust-vmm/vhost-device >> >> >> >> Getting the test enabled took some doing most likely because the need >> >> for CONFIG support exercised additional paths in the code that were >> >> not used for the simpler virtio-net tests. As a result the series has >> >> a number of cleanup and documentation patches. >> >> >> >> The final thing that needed fixing was the ensuring that >> >> VHOST_USER_F_PROTOCOL_FEATURES didn't get squashed in the negotiation >> >> process. This was the hardest thing to track down as we store the >> >> feature bits in several places variously as: >> >> >> >> in VirtIODevice as: >> >> uint64_t guest_features; >> >> uint64_t host_features; >> >> uint64_t backend_features; >> > >> > None of these know about VHOST_USER_F_PROTOCOL_FEATURES and vhost-user's >> > unfiltered feature bits should never be passed to VirtIODevice. >> > >> >> >> >> in vhost_dev as: >> >> uint64_t features; >> >> uint64_t acked_features; >> >> uint64_t backend_features; >> > >> > I don't think these should know about VHOST_USER_F_PROTOCOL_FEATURES >> > either. AFAIK vhost_dev deals with VIRTIO feature bits, not raw >> > vhost-user GET_FEATURES. >> >> So where does VHOST_USER_F_PROTOCOL_FEATURES get set before it's set >> with the VHOST_USER_SET_FEATURES message? Currently it's fed via: >> >> uint64_t features = vhost_dev->acked_features; >> >> in vhost_dev_set_features() which does apply a few extra bits >> (VHOST_F_LOG_ALL/VIRTIO_F_IOMMU_PLATFORM). Maybe it should be adding >> VHOST_USER_F_PROTOCOL_FEATURES here? How should it be signalled by the >> vhost-user backend that this should be done? Overload the function? > > A modern vhost-user server replies to VHOST_USER_GET_FEATURES with > VHOST_USER_F_PROTOCOL_FEATURES set. That's when the vhost-user client > encounters this bit. > > The vhost-user client should then filter out > VHOST_USER_F_PROTOCOL_FEATURES because it belongs to the vhost-user > protocol and isn't a real VIRTIO feature bit. The client uses the > filtered VIRTIO feature bits and it now knows whether the vhost-user > server supports the VHOST_USER_GET_PROTOCOL_FEATURES and > VHOST_USER_SET_PROTOCOL_FEATURES messages. > > I think vhost_user_set_features() should set > VHOST_USER_F_PROTOCOL_FEATURES if the server returned it from > VHOST_USER_GET_FEATURES. At the moment vhost_user_backend_init() > stores VHOST_USER_F_PROTOCOL_FEATURES in struct > vhost_dev->backend_features, which only seems to be used by vhost-net > code. I can clean-up the documentation for this. I wonder if the VirtIODevice backend_features is a duplication that should be removed? > The other vhost-user devices set acked_features = guest_features and > ignore backend_features. As a result I guess they don't set > VHOST_USER_F_PROTOCOL_FEATURES in the VHOST_USER_SET_FEATURES message. > Most vhost-user servers probably don't care and still respond to > VHOST_USER_GET_PROTOCOL_FEATURES and VHOST_USER_SET_PROTOCOL_FEATURES > messages (although the vhost-user protocol spec mentions other > protocol differences when VHOST_USER_F_PROTOCOL_FEATURES is not > negotiated). > > Does this match what you've found? The code is a maze so I may have > gotten something wrong. I think so. As you say it's a bit of a maze and hopefully we can more clearly document when and where things are and how they should be used. The various virtio devices have grown organically so there are inconsistencies that need ironing out. > In general I think hw/virtio/vhost-user.c > should be responsible for VHOST_USER_F_PROTOCOL_FEATURES and no other > part of the QEMU codebase should ever see the bit since it's a > vhost-user protocol detail and not part of VIRTIO or even a common > part of vhost. OK I'll see what I can cook up. > > Stefan
On Thu, Jul 7, 2022, 17:28 Alex Bennée <alex.bennee@linaro.org> wrote: > > Stefan Hajnoczi <stefanha@gmail.com> writes: > > > On Thu, 7 Jul 2022 at 14:42, Alex Bennée <alex.bennee@linaro.org> wrote: > >> > >> > >> Stefan Hajnoczi <stefanha@redhat.com> writes: > >> > >> > [[PGP Signed Part:Undecided]] > >> > On Tue, May 24, 2022 at 04:40:41PM +0100, Alex Bennée wrote: > >> >> Hi, > >> >> > >> >> This series ostensibly adds virtio-user-gpio stubs to the build for > >> >> use with an external vhost-user daemon. We've been testing it with > our > >> >> rust daemons from: > >> >> > >> >> https://github.com/rust-vmm/vhost-device > >> >> > >> >> Getting the test enabled took some doing most likely because the need > >> >> for CONFIG support exercised additional paths in the code that were > >> >> not used for the simpler virtio-net tests. As a result the series has > >> >> a number of cleanup and documentation patches. > >> >> > >> >> The final thing that needed fixing was the ensuring that > >> >> VHOST_USER_F_PROTOCOL_FEATURES didn't get squashed in the negotiation > >> >> process. This was the hardest thing to track down as we store the > >> >> feature bits in several places variously as: > >> >> > >> >> in VirtIODevice as: > >> >> uint64_t guest_features; > >> >> uint64_t host_features; > >> >> uint64_t backend_features; > >> > > >> > None of these know about VHOST_USER_F_PROTOCOL_FEATURES and > vhost-user's > >> > unfiltered feature bits should never be passed to VirtIODevice. > >> > > >> >> > >> >> in vhost_dev as: > >> >> uint64_t features; > >> >> uint64_t acked_features; > >> >> uint64_t backend_features; > >> > > >> > I don't think these should know about VHOST_USER_F_PROTOCOL_FEATURES > >> > either. AFAIK vhost_dev deals with VIRTIO feature bits, not raw > >> > vhost-user GET_FEATURES. > >> > >> So where does VHOST_USER_F_PROTOCOL_FEATURES get set before it's set > >> with the VHOST_USER_SET_FEATURES message? Currently it's fed via: > >> > >> uint64_t features = vhost_dev->acked_features; > >> > >> in vhost_dev_set_features() which does apply a few extra bits > >> (VHOST_F_LOG_ALL/VIRTIO_F_IOMMU_PLATFORM). Maybe it should be adding > >> VHOST_USER_F_PROTOCOL_FEATURES here? How should it be signalled by the > >> vhost-user backend that this should be done? Overload the function? > > > > A modern vhost-user server replies to VHOST_USER_GET_FEATURES with > > VHOST_USER_F_PROTOCOL_FEATURES set. That's when the vhost-user client > > encounters this bit. > > > > The vhost-user client should then filter out > > VHOST_USER_F_PROTOCOL_FEATURES because it belongs to the vhost-user > > protocol and isn't a real VIRTIO feature bit. The client uses the > > filtered VIRTIO feature bits and it now knows whether the vhost-user > > server supports the VHOST_USER_GET_PROTOCOL_FEATURES and > > VHOST_USER_SET_PROTOCOL_FEATURES messages. > > > > I think vhost_user_set_features() should set > > VHOST_USER_F_PROTOCOL_FEATURES if the server returned it from > > VHOST_USER_GET_FEATURES. At the moment vhost_user_backend_init() > > stores VHOST_USER_F_PROTOCOL_FEATURES in struct > > vhost_dev->backend_features, which only seems to be used by vhost-net > > code. > > I can clean-up the documentation for this. I wonder if the VirtIODevice > backend_features is a duplication that should be removed? > I don't know the code well enough to say, but it's possible that it can be simplified. Stefan
Hi, This series ostensibly adds virtio-user-gpio stubs to the build for use with an external vhost-user daemon. We've been testing it with our rust daemons from: https://github.com/rust-vmm/vhost-device Getting the test enabled took some doing most likely because the need for CONFIG support exercised additional paths in the code that were not used for the simpler virtio-net tests. As a result the series has a number of cleanup and documentation patches. The final thing that needed fixing was the ensuring that VHOST_USER_F_PROTOCOL_FEATURES didn't get squashed in the negotiation process. This was the hardest thing to track down as we store the feature bits in several places variously as: in VirtIODevice as: uint64_t guest_features; uint64_t host_features; uint64_t backend_features; in vhost_dev as: uint64_t features; uint64_t acked_features; uint64_t backend_features; and a number of the other device structures also have various features fields along with get/set function handlers. It wasn't super clear what the flow through this structures was meant to be but I'm fairly sure there is unnecessary duplication in there somewhere. We could certainly do with some docstrings to make the point of each field clear. Going forward I wonder if having a fake vhost-user daemon is sustainable in the long term. Maybe it would be better to spawn real vhost-user daemons in a test mode so we don't end up duplicating the whole protocol? Anyway please review. Alex Bennée (13): contrib/vhost-user-blk: fix 32 bit build and enable include/hw/virtio: more comment for VIRTIO_F_BAD_FEATURE include/hw/virtio: document vhost_get_features include/hw/virtio: document vhost_ack_features tests/qtest: pass stdout/stderr down to subtests tests/qtest: add a timeout for subprocess_run_one_test tests/qtest: use qos_printf instead of g_test_message tests/qtest: catch unhandled vhost-user messages tests/qtest: use g_autofree for test_server_create_chr tests/qtest: plain g_assert for VHOST_USER_F_PROTOCOL_FEATURES tests/qtest: implement stub for VHOST_USER_GET_CONFIG tests/qtest: add a get_features op to vhost-user-test tests/qtest: enable tests for virtio-gpio Viresh Kumar (2): hw/virtio: add boilerplate for vhost-user-gpio device hw/virtio: add vhost-user-gpio-pci boilerplate meson.build | 2 +- include/hw/virtio/vhost-user-gpio.h | 35 +++ include/hw/virtio/vhost.h | 21 ++ include/hw/virtio/virtio.h | 7 +- tests/qtest/libqos/virtio-gpio.h | 35 +++ contrib/vhost-user-blk/vhost-user-blk.c | 6 +- hw/virtio/vhost-user-gpio-pci.c | 69 +++++ hw/virtio/vhost-user-gpio.c | 357 ++++++++++++++++++++++++ tests/qtest/libqos/virtio-gpio.c | 171 ++++++++++++ tests/qtest/libqos/virtio.c | 2 +- tests/qtest/qos-test.c | 8 +- tests/qtest/vhost-user-test.c | 179 ++++++++++-- contrib/vhost-user-blk/meson.build | 3 +- hw/virtio/Kconfig | 5 + hw/virtio/meson.build | 2 + tests/qtest/libqos/meson.build | 1 + 16 files changed, 872 insertions(+), 31 deletions(-) create mode 100644 include/hw/virtio/vhost-user-gpio.h create mode 100644 tests/qtest/libqos/virtio-gpio.h create mode 100644 hw/virtio/vhost-user-gpio-pci.c create mode 100644 hw/virtio/vhost-user-gpio.c create mode 100644 tests/qtest/libqos/virtio-gpio.c