Re: [PATCH RESEND v3 2/2] virtio: remove unnecessary host_features in ->get_features()

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Nov 21, 2022 at 10:50:20AM -0500, Stefan Hajnoczi wrote:
> Since at least commit 6b8f1020540c27246277377aa2c3331ad2bfb160 ("virtio:
> move host_features") the ->get_features() function has been called with
> host_features as an argument.
> 
> Some devices manually add host_features in ->get_features() although the
> features argument already contains host_features. Make all devices
> consistent by dropping the unnecessary code.
> 
> Cc: Cornelia Huck <cohuck@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Causes qos-test failures:

ok 65 /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-net-pci/virtio-net/virtio-net-tests/vhost-user/flags-mismatch
**
ERROR:../tests/qtest/vhost-user-test.c:889:wait_for_rings_started: assertion failed (ctpop64(s->rings) == count): (2 == 4)
# child process (/x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-net-pci/virtio-net/virtio-net-tests/vhost-user/multiqueue/subprocess [856811]) exit status: 1 (error)
# child process (/x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-net-pci/virtio-net/virtio-net-tests/vhost-user/multiqueue/subprocess [856811]) stdout: ""
# child process (/x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-net-pci/virtio-net/virtio-net-tests/vhost-user/multiqueue/subprocess [856811]) stderr: "**\nERROR:../tests/qtest/vhost-user-test.c:889:wait_for_rings_started: assertion failed (ctpop64(s->rings) == count): (2 == 4)\n"
**
ERROR:../tests/qtest/qos-test.c:191:subprocess_run_one_test: child process (/x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-net-pci/virtio-net/virtio-net-tests/vhost-user/multiqueue/subprocess [856811]) failed unexpectedly
Bail out! ERROR:../tests/qtest/qos-test.c:191:subprocess_run_one_test: child process (/x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-net-pci/virtio-net/virtio-net-tests/vhost-user/multiqueue/subprocess [856811]) failed unexpectedly
runone.sh: line 10: 856419 Aborted                 (core dumped) QTEST_QEMU_STORAGE_DAEMON_BINARY=./build/storage-daemon/qemu-storage-daemon QTEST_QEMU_BINARY=build/x86_64-softmmu/qemu-system-x86_64 "$@"


> ---
>  hw/block/virtio-blk.c       | 3 ---
>  hw/char/virtio-serial-bus.c | 1 -
>  hw/net/virtio-net.c         | 3 ---
>  hw/scsi/vhost-scsi-common.c | 3 ---
>  hw/scsi/virtio-scsi.c       | 4 ----
>  hw/virtio/virtio-balloon.c  | 2 --
>  6 files changed, 16 deletions(-)
> 
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index f717550fdc..a744012fec 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -983,9 +983,6 @@ static uint64_t virtio_blk_get_features(VirtIODevice *vdev, uint64_t features,
>  {
>      VirtIOBlock *s = VIRTIO_BLK(vdev);
>  
> -    /* Firstly sync all virtio-blk possible supported features */
> -    features |= s->host_features;
> -
>      virtio_add_feature(&features, VIRTIO_BLK_F_SEG_MAX);
>      virtio_add_feature(&features, VIRTIO_BLK_F_GEOMETRY);
>      virtio_add_feature(&features, VIRTIO_BLK_F_TOPOLOGY);
> diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
> index 7d4601cb5d..1414fb85ae 100644
> --- a/hw/char/virtio-serial-bus.c
> +++ b/hw/char/virtio-serial-bus.c
> @@ -557,7 +557,6 @@ static uint64_t get_features(VirtIODevice *vdev, uint64_t features,
>  
>      vser = VIRTIO_SERIAL(vdev);
>  
> -    features |= vser->host_features;
>      if (vser->bus.max_nr_ports > 1) {
>          virtio_add_feature(&features, VIRTIO_CONSOLE_F_MULTIPORT);
>      }
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index aba12759d5..35b45eb6e3 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -777,9 +777,6 @@ static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features,
>      VirtIONet *n = VIRTIO_NET(vdev);
>      NetClientState *nc = qemu_get_queue(n->nic);
>  
> -    /* Firstly sync all virtio-net possible supported features */
> -    features |= n->host_features;
> -
>      virtio_add_feature(&features, VIRTIO_NET_F_MAC);
>  
>      if (!peer_has_vnet_hdr(n)) {
> diff --git a/hw/scsi/vhost-scsi-common.c b/hw/scsi/vhost-scsi-common.c
> index 767f827e55..8b26f90aa1 100644
> --- a/hw/scsi/vhost-scsi-common.c
> +++ b/hw/scsi/vhost-scsi-common.c
> @@ -124,9 +124,6 @@ uint64_t vhost_scsi_common_get_features(VirtIODevice *vdev, uint64_t features,
>  {
>      VHostSCSICommon *vsc = VHOST_SCSI_COMMON(vdev);
>  
> -    /* Turn on predefined features supported by this device */
> -    features |= vsc->host_features;
> -
>      return vhost_get_features(&vsc->dev, vsc->feature_bits, features);
>  }
>  
> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> index 6f6e2e32ba..40e10322fb 100644
> --- a/hw/scsi/virtio-scsi.c
> +++ b/hw/scsi/virtio-scsi.c
> @@ -817,10 +817,6 @@ static uint64_t virtio_scsi_get_features(VirtIODevice *vdev,
>                                           uint64_t requested_features,
>                                           Error **errp)
>  {
> -    VirtIOSCSI *s = VIRTIO_SCSI(vdev);
> -
> -    /* Firstly sync all virtio-scsi possible supported features */
> -    requested_features |= s->host_features;
>      return requested_features;
>  }
>  
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index 73ac5eb675..0e9ca71b15 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -796,8 +796,6 @@ static void virtio_balloon_set_config(VirtIODevice *vdev,
>  static uint64_t virtio_balloon_get_features(VirtIODevice *vdev, uint64_t f,
>                                              Error **errp)
>  {
> -    VirtIOBalloon *dev = VIRTIO_BALLOON(vdev);
> -    f |= dev->host_features;
>      virtio_add_feature(&f, VIRTIO_BALLOON_F_STATS_VQ);
>  
>      return f;
> -- 
> 2.38.1