Re: [PATCH v3 4/7] hw/virtio: ensure a valid host_feature set for virtio-user-gpio

[Michael S. Tsirkin]

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