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

[Alex Bennée]

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


-- 
Alex Bennée