| Message ID | 20221128164105.1191058-5-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | fix vhost-user issues with CI | expand |
On Mon, 28 Nov 2022 at 11:42, Alex Bennée <alex.bennee@linaro.org> wrote: > > There was a disconnect here because vdev->host_features was set to > random rubbish. This caused a weird negotiation between the driver and > device that took no account of the features provided by the backend. > To fix this we must set vdev->host_features once we have initialised > the vhost backend. > > [AJB: however this is confusing because AFAICT none of the other > vhost-user devices do this.] > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > --- > hw/virtio/vhost-user-gpio.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c > index 5851cb3bc9..b2496c824c 100644 > --- a/hw/virtio/vhost-user-gpio.c > +++ b/hw/virtio/vhost-user-gpio.c > @@ -228,6 +228,12 @@ static int vu_gpio_connect(DeviceState *dev, Error **errp) > return ret; > } > > + /* > + * Once we have initialised the vhost backend we can finally set > + * the what host features are available for this device. > + */ > + vdev->host_features = vhost_dev->features; vdev->host_feature is already set by virtio_bus_device_plugged -> vu_gpio_get_features. Something is still wrong. My understanding is: ->realize() performs a blocking connect so when it returns and virtio_bus_device_plugged() runs, we'll be able to fetch the backend features from ->get_features(). The assumption is that the backend features don't change across reconnection (I think). vhost-user-gpio seems to follow the same flow as the other vhost-user devices (vhost-user-net is special, so let's ignore it), so I don't understand why it's necessary to manually assign ->host_features here? Stefan
Stefan Hajnoczi <stefanha@gmail.com> writes: > On Mon, 28 Nov 2022 at 11:42, Alex Bennée <alex.bennee@linaro.org> wrote: >> >> There was a disconnect here because vdev->host_features was set to >> random rubbish. This caused a weird negotiation between the driver and >> device that took no account of the features provided by the backend. >> To fix this we must set vdev->host_features once we have initialised >> the vhost backend. >> >> [AJB: however this is confusing because AFAICT none of the other >> vhost-user devices do this.] >> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >> --- >> hw/virtio/vhost-user-gpio.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c >> index 5851cb3bc9..b2496c824c 100644 >> --- a/hw/virtio/vhost-user-gpio.c >> +++ b/hw/virtio/vhost-user-gpio.c >> @@ -228,6 +228,12 @@ static int vu_gpio_connect(DeviceState *dev, Error **errp) >> return ret; >> } >> >> + /* >> + * Once we have initialised the vhost backend we can finally set >> + * the what host features are available for this device. >> + */ >> + vdev->host_features = vhost_dev->features; > > vdev->host_feature is already set by virtio_bus_device_plugged -> > vu_gpio_get_features. > > Something is still wrong. > > My understanding is: ->realize() performs a blocking connect so when > it returns and virtio_bus_device_plugged() runs, we'll be able to > fetch the backend features from ->get_features(). The assumption is > that the backend features don't change across reconnection (I think). > > vhost-user-gpio seems to follow the same flow as the other vhost-user > devices (vhost-user-net is special, so let's ignore it), so I don't > understand why it's necessary to manually assign ->host_features here? I think you are right. I suspect I got confused while chasing down the missing high bits (due to the legacy VirtIO negotiation). Also slightly thrown off by the appearance of virtio-net (I assume because of a machine default?): ➜ env QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon QTEST_QEMU_IMG=./qemu-img QTEST_QEMU_BINARY=./qemu-system-aarch64 G_TEST_DBUS_DAEMON=/home/alex/ lsrc/qemu.git/tests/dbus-vmstate-daemon.sh MALLOC_PERTURB_=177 ./tests/qtest/qos-test --tap -p /aarch64/virt/generic-pcihost/pci-bus-generic/pci-bus/vhost-user-gpio-pci/vhost-user-gpio/vhost-user-gpio-tests/read-guest-mem/memfile # random seed: R02S235ee9d31e87e0159f810979be62563e # starting QEMU: exec ./qemu-system-aarch64 -qtest unix:/tmp/qtest-1276289.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-1276289.qmp,id=char0 -mon chardev=char0,mode=control -display none -machine none -accel qtest # Start of aarch64 tests # Start of virt tests # Start of generic-pcihost tests # Start of pci-bus-generic tests # Start of pci-bus tests # Start of vhost-user-gpio-pci tests # Start of vhost-user-gpio tests # Start of vhost-user-gpio-tests tests # Start of read-guest-mem tests virtio_bus_device_plugged: virtio-net host_features = 0x10179bf8064 vu_gpio_connect: pre host_features = 10039000000 vu_gpio_connect: post host_features = 140000001 virtio_bus_device_plugged: virtio-gpio host_features = 0x140000001 qemu-system-aarch64: Failed to set msg fds. qemu-system-aarch64: vhost VQ 0 ring restore failed: -22: Invalid argument (22) qemu-system-aarch64: Failed to set msg fds. qemu-system-aarch64: vhost VQ 1 ring restore failed: -22: Invalid argument (22) qemu-system-aarch64: Failed to set msg fds. qemu-system-aarch64: vhost_set_vring_call failed: Invalid argument (22) qemu-system-aarch64: Failed to set msg fds. qemu-system-aarch64: vhost_set_vring_call failed: Invalid argument (22) # qos_test running single test in subprocess # set_protocol_features: 0x200 # set_owner: start of session # vhost-user: un-handled message: 14 # vhost-user: un-handled message: 14 # set_vring_num: 0/256 # set_vring_addr: 0x7f55b0000000/0x7f55affff000/0x7f55b0001000 ok 1 /aarch64/virt/generic-pcihost/pci-bus-generic/pci-bus/vhost-user-gpio-pci/vhost-user-gpio/vhost-user-gpio-tests/read-guest-mem/memfile # Start of memfile tests # End of memfile tests # End of read-guest-mem tests # End of vhost-user-gpio-tests tests # End of vhost-user-gpio tests # End of vhost-user-gpio-pci tests # End of pci-bus tests # End of pci-bus-generic tests # End of generic-pcihost tests # End of virt tests # End of aarch64 tests 1..1 and for mmio: env MALLOC_PERTURB_=235 QTEST_QEMU_IMG=./qemu-img QTEST_QEMU_BINARY="./qemu-system-arm" QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon G_TEST_DBUS_DAEMON=/home/alex/lsrc/qemu.git/tests/dbus-vmstate-daemon.sh ./tests/qtest/qos-test --tap -p /arm/virt/virtio-mmio/virtio-bus/vhost-user-gpio-device/vhost-user-gpio/vhost-user-gpio-tests/read-guest-mem/memfile # random seed: R02Sac48bb073367f77c1c1a1db6b5245c9e # starting QEMU: exec ./qemu-system-arm -qtest unix:/tmp/qtest-1276963.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-1276963.qmp,id=char0 -mon chardev=char0,mode=control -display none -machine none -accel qtest # Start of arm tests # Start of virt tests # Start of virtio-mmio tests # Start of virtio-bus tests # Start of vhost-user-gpio-device tests # Start of vhost-user-gpio tests # Start of vhost-user-gpio-tests tests # Start of read-guest-mem tests virtio_bus_device_plugged: virtio-net host_features = 0x10179bf8064 vu_gpio_connect: pre host_features = 10039000000 vu_gpio_connect: post host_features = 140000001 virtio_bus_device_plugged: virtio-gpio host_features = 0x140000001 qemu-system-arm: Failed to set msg fds. qemu-system-arm: vhost_set_vring_call failed: Invalid argument (22) qemu-system-arm: Failed to set msg fds. qemu-system-arm: vhost_set_vring_call failed: Invalid argument (22) # qos_test running single test in subprocess # set_protocol_features: 0x200 # set_owner: start of session # vhost-user: un-handled message: 14 # vhost-user: un-handled message: 14 ok 1 /arm/virt/virtio-mmio/virtio-bus/vhost-user-gpio-device/vhost-user-gpio/vhost-user-gpio-tests/read-guest-mem/memfile # Start of memfile tests # End of memfile tests # End of read-guest-mem tests # End of vhost-user-gpio-tests tests # End of vhost-user-gpio tests # End of vhost-user-gpio-device tests # End of virtio-bus tests # End of virtio-mmio tests # End of virt tests # End of arm tests 1..1 I'll drop this patch for now and re-test. > > Stefan
On Mon, Nov 28, 2022 at 07:39:29PM +0000, Alex Bennée wrote: > > Stefan Hajnoczi <stefanha@gmail.com> writes: > > > On Mon, 28 Nov 2022 at 11:42, Alex Bennée <alex.bennee@linaro.org> wrote: > >> > >> There was a disconnect here because vdev->host_features was set to > >> random rubbish. This caused a weird negotiation between the driver and > >> device that took no account of the features provided by the backend. > >> To fix this we must set vdev->host_features once we have initialised > >> the vhost backend. > >> > >> [AJB: however this is confusing because AFAICT none of the other > >> vhost-user devices do this.] > >> > >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > >> --- > >> hw/virtio/vhost-user-gpio.c | 6 ++++++ > >> 1 file changed, 6 insertions(+) > >> > >> diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c > >> index 5851cb3bc9..b2496c824c 100644 > >> --- a/hw/virtio/vhost-user-gpio.c > >> +++ b/hw/virtio/vhost-user-gpio.c > >> @@ -228,6 +228,12 @@ static int vu_gpio_connect(DeviceState *dev, Error **errp) > >> return ret; > >> } > >> > >> + /* > >> + * Once we have initialised the vhost backend we can finally set > >> + * the what host features are available for this device. > >> + */ > >> + vdev->host_features = vhost_dev->features; > > > > vdev->host_feature is already set by virtio_bus_device_plugged -> > > vu_gpio_get_features. > > > > Something is still wrong. > > > > My understanding is: ->realize() performs a blocking connect so when > > it returns and virtio_bus_device_plugged() runs, we'll be able to > > fetch the backend features from ->get_features(). The assumption is > > that the backend features don't change across reconnection (I think). > > > > vhost-user-gpio seems to follow the same flow as the other vhost-user > > devices (vhost-user-net is special, so let's ignore it), so I don't > > understand why it's necessary to manually assign ->host_features here? > > I think you are right. I suspect I got confused while chasing down the > missing high bits (due to the legacy VirtIO negotiation). Also slightly > thrown off by the appearance of virtio-net (I assume because of a > machine default?): > > ➜ env QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon QTEST_QEMU_IMG=./qemu-img QTEST_QEMU_BINARY=./qemu-system-aarch64 G_TEST_DBUS_DAEMON=/home/alex/ > lsrc/qemu.git/tests/dbus-vmstate-daemon.sh MALLOC_PERTURB_=177 ./tests/qtest/qos-test --tap -p /aarch64/virt/generic-pcihost/pci-bus-generic/pci-bus/vhost-user-gpio-pci/vhost-user-gpio/vhost-user-gpio-tests/read-guest-mem/memfile > # random seed: R02S235ee9d31e87e0159f810979be62563e > # starting QEMU: exec ./qemu-system-aarch64 -qtest unix:/tmp/qtest-1276289.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-1276289.qmp,id=char0 -mon chardev=char0,mode=control -display none -machine none -accel qtest > # Start of aarch64 tests > # Start of virt tests > # Start of generic-pcihost tests > # Start of pci-bus-generic tests > # Start of pci-bus tests > # Start of vhost-user-gpio-pci tests > # Start of vhost-user-gpio tests > # Start of vhost-user-gpio-tests tests > # Start of read-guest-mem tests > virtio_bus_device_plugged: virtio-net host_features = 0x10179bf8064 > vu_gpio_connect: pre host_features = 10039000000 > vu_gpio_connect: post host_features = 140000001 > virtio_bus_device_plugged: virtio-gpio host_features = 0x140000001 > qemu-system-aarch64: Failed to set msg fds. > qemu-system-aarch64: vhost VQ 0 ring restore failed: -22: Invalid argument (22) > qemu-system-aarch64: Failed to set msg fds. > qemu-system-aarch64: vhost VQ 1 ring restore failed: -22: Invalid argument (22) > qemu-system-aarch64: Failed to set msg fds. > qemu-system-aarch64: vhost_set_vring_call failed: Invalid argument (22) > qemu-system-aarch64: Failed to set msg fds. > qemu-system-aarch64: vhost_set_vring_call failed: Invalid argument (22) > # qos_test running single test in subprocess > # set_protocol_features: 0x200 > # set_owner: start of session > # vhost-user: un-handled message: 14 > # vhost-user: un-handled message: 14 > # set_vring_num: 0/256 > # set_vring_addr: 0x7f55b0000000/0x7f55affff000/0x7f55b0001000 > ok 1 /aarch64/virt/generic-pcihost/pci-bus-generic/pci-bus/vhost-user-gpio-pci/vhost-user-gpio/vhost-user-gpio-tests/read-guest-mem/memfile > # Start of memfile tests > # End of memfile tests > # End of read-guest-mem tests > # End of vhost-user-gpio-tests tests > # End of vhost-user-gpio tests > # End of vhost-user-gpio-pci tests > # End of pci-bus tests > # End of pci-bus-generic tests > # End of generic-pcihost tests > # End of virt tests > # End of aarch64 tests > 1..1 > > and for mmio: > > env MALLOC_PERTURB_=235 QTEST_QEMU_IMG=./qemu-img QTEST_QEMU_BINARY="./qemu-system-arm" QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon G_TEST_DBUS_DAEMON=/home/alex/lsrc/qemu.git/tests/dbus-vmstate-daemon.sh ./tests/qtest/qos-test --tap -p /arm/virt/virtio-mmio/virtio-bus/vhost-user-gpio-device/vhost-user-gpio/vhost-user-gpio-tests/read-guest-mem/memfile > # random seed: R02Sac48bb073367f77c1c1a1db6b5245c9e > # starting QEMU: exec ./qemu-system-arm -qtest unix:/tmp/qtest-1276963.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-1276963.qmp,id=char0 -mon chardev=char0,mode=control -display none -machine none -accel qtest > # Start of arm tests > # Start of virt tests > # Start of virtio-mmio tests > # Start of virtio-bus tests > # Start of vhost-user-gpio-device tests > # Start of vhost-user-gpio tests > # Start of vhost-user-gpio-tests tests > # Start of read-guest-mem tests > virtio_bus_device_plugged: virtio-net host_features = 0x10179bf8064 > vu_gpio_connect: pre host_features = 10039000000 > vu_gpio_connect: post host_features = 140000001 > virtio_bus_device_plugged: virtio-gpio host_features = 0x140000001 > qemu-system-arm: Failed to set msg fds. > qemu-system-arm: vhost_set_vring_call failed: Invalid argument (22) > qemu-system-arm: Failed to set msg fds. > qemu-system-arm: vhost_set_vring_call failed: Invalid argument (22) > # qos_test running single test in subprocess > # set_protocol_features: 0x200 > # set_owner: start of session > # vhost-user: un-handled message: 14 > # vhost-user: un-handled message: 14 > ok 1 /arm/virt/virtio-mmio/virtio-bus/vhost-user-gpio-device/vhost-user-gpio/vhost-user-gpio-tests/read-guest-mem/memfile > # Start of memfile tests > # End of memfile tests > # End of read-guest-mem tests > # End of vhost-user-gpio-tests tests > # End of vhost-user-gpio tests > # End of vhost-user-gpio-device tests > # End of virtio-bus tests > # End of virtio-mmio tests > # End of virt tests > # End of arm tests > 1..1 > > I'll drop this patch for now and re-test. So I should expect v4? And I guess we can split out documentation things then? > > > > Stefan > > > -- > Alex Bennée
Hi Alex, I'm waiting for a v4 or a confirmation that you've retested and I can just drop this patch. Thanks! Stefan
On Tue, Nov 29, 2022 at 04:01:25PM -0500, Stefan Hajnoczi wrote: > Hi Alex, > I'm waiting for a v4 or a confirmation that you've retested and I can > just drop this patch. > > Thanks! > > Stefan Note things need to be reordered, patch 2 should come last. So I'd really like to see v4 if possible.
Stefan Hajnoczi <stefanha@gmail.com> writes: > Hi Alex, > I'm waiting for a v4 or a confirmation that you've retested and I can > just drop this patch. I've re-ordered and I'll post the up to date series with the dropped patch tomorrow. I was hoping for r-b's for the other patches. > > Thanks! > > Stefan
diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c index 5851cb3bc9..b2496c824c 100644 --- a/hw/virtio/vhost-user-gpio.c +++ b/hw/virtio/vhost-user-gpio.c @@ -228,6 +228,12 @@ static int vu_gpio_connect(DeviceState *dev, Error **errp) return ret; } + /* + * Once we have initialised the vhost backend we can finally set + * the what host features are available for this device. + */ + vdev->host_features = vhost_dev->features; + /* restore vhost state */ if (virtio_device_started(vdev, vdev->status)) { vu_gpio_start(vdev);
There was a disconnect here because vdev->host_features was set to random rubbish. This caused a weird negotiation between the driver and device that took no account of the features provided by the backend. To fix this we must set vdev->host_features once we have initialised the vhost backend. [AJB: however this is confusing because AFAICT none of the other vhost-user devices do this.] Signed-off-by: Alex Bennée <alex.bennee@linaro.org> --- hw/virtio/vhost-user-gpio.c | 6 ++++++ 1 file changed, 6 insertions(+)