On Mon, Nov 21, 2022 at 04:51:40PM -0500, Stefan Hajnoczi wrote:
> On Mon, 21 Nov 2022 at 14:25, Alex Bennée <alex.ben...@linaro.org> wrote:
> > Stefan Hajnoczi <stefa...@gmail.com> writes:
> >
> > > On Mon, 21 Nov 2022 at 09:49, Alex Bennée <alex.ben...@linaro.org> wrote:
> > >>
> > >> We have a bunch of variables associated with the device and the vhost
> > >> backend which are used inconsistently throughout the code base. Lets
> > >> start trying to bring some order by agreeing what each variable is
> > >> for. Some cases to address (vho/vio renames to avoid ambiguous results
> > >> while grepping):
> > >>
> > >> virtio->guest_features is mostly the live status of the features field
> > >> and read and written as such by the guest. It does get manipulated by
> > >> the various load state via virtio_set_features_nocheck(vdev, val) for
> > >> migration.
> > >>
> > >> virtio->host_features is the result of vcd->get_features() most of the
> > >> time and for vhost-user devices eventually ends up down at the vhost
> > >> get features message:
> > >>
> > >> ./hw/virtio/virtio-bus.c:66: vdev->host_features =
> > >> vdc->get_features(vdev, vdev->host_features,
> > >>
> > >> However virtio-net does a lot of direct modification of it:
> > >>
> > >> ./hw/net/virtio-net.c:3517: n->host_features |= (1ULL <<
> > >> VIRTIO_NET_F_MTU);
> > >> ./hw/net/virtio-net.c:3529: n->host_features |= (1ULL <<
> > >> VIRTIO_NET_F_SPEED_DUPLEX);
> > >> ./hw/net/virtio-net.c:3539: n->host_features |= (1ULL <<
> > >> VIRTIO_NET_F_SPEED_DUPLEX);
> > >> ./hw/net/virtio-net.c:3548: n->host_features |= (1ULL <<
> > >> VIRTIO_NET_F_STANDBY);
> > >> ./hw/virtio/virtio.c:3438: bool bad = (val &
> > >> ~(vdev->host_features)) != 0;
> > >>
> > >> And we have this case which propagates the global QMP values for the
> > >> device to the host features. This causes the resent regression of
> > >> vhost-user-sock due to 69e1c14aa2 (virtio: core: vq reset feature
> > >> negotation support) because the reset feature was rejected by the
> > >> vhost-user backend causing it to freeze:
> > >>
> > >> ./hw/virtio/virtio.c:4667: status->host_features =
> > >> qmp_decode_features(vdev->device_id,
> > >>
> > >> virtio->backend_features is only used by virtio-net to stash the
> > >> vhost_net_get_features features for checking later:
> > >>
> > >> features = vhost_net_get_features(get_vhost_net(nc->peer), features);
> > >> vdev->vio_backend_features = features;
> > >>
> > >> and:
> > >>
> > >> if (n->mtu_bypass_backend &&
> > >> !virtio_has_feature(vdev->vio_backend_features,
> > >> VIRTIO_NET_F_MTU)) {
> > >> features &= ~(1ULL << VIRTIO_NET_F_MTU);
> > >> }
> > >>
> > >> vhost_dev->acked_features seems to mostly reflect
> > >> virtio->guest_features (but where in the negotiation cycle?). Except
> > >> for vhost_net where is becomes vhost_dev->backend_features
> > >>
> > >> ./backends/vhost-user.c:87: b->dev.vho_acked_features =
> > >> b->vdev->guest_features;
> > >> ./hw/block/vhost-user-blk.c:149: s->dev.vho_acked_features =
> > >> vdev->guest_features;
> > >> ./hw/net/vhost_net.c:132: net->dev.vho_acked_features =
> > >> net->dev.vho_backend_features;
> > >> ./hw/scsi/vhost-scsi-common.c:53: vsc->dev.vho_acked_features =
> > >> vdev->guest_features;
> > >> ./hw/virtio/vhost-user-fs.c:77: fs->vhost_dev.vho_acked_features =
> > >> vdev->guest_features;
> > >> ./hw/virtio/vhost-user-i2c.c:46: i2c->vhost_dev.vho_acked_features
> > >> = vdev->guest_features;
> > >> ./hw/virtio/vhost-user-rng.c:44: rng->vhost_dev.vho_acked_features
> > >> = vdev->guest_features;
> > >> ./hw/virtio/vhost-vsock-common.c:71:
> > >> vvc->vhost_dev.vho_acked_features = vdev->guest_features;
> > >> ./hw/virtio/vhost.c:1631: hdev->vho_acked_features |=
> > >> bit_mask;
> > >>
> > >> vhost_dev->backend_features has become overloaded with two use cases:
> > >>
> > >> ./hw/block/vhost-user-blk.c:336: s->dev.vho_backend_features = 0;
> > >> ./hw/net/vhost_net.c:180: net->dev.vho_backend_features =
> > >> qemu_has_vnet_hdr(options->net_backend)
> > >> ./hw/net/vhost_net.c:185: net->dev.vho_backend_features = 0;
> > >> ./hw/scsi/vhost-scsi.c:220: vsc->dev.vho_backend_features = 0;
> > >> ./hw/scsi/vhost-user-scsi.c:121: vsc->dev.vho_backend_features = 0;
> > >> ./hw/virtio/vhost-user.c:2083: dev->vho_backend_features |=
> > >> 1ULL << VHOST_USER_F_PROTOCOL_FEATURES;
> > >> One use for saving the availability of a vhost-net feature and another
> > >> for ensuring we add the protocol feature negotiation bit when querying
> > >> a vhost backend. Maybe the places where this is queried should really
> > >> be bools that can be queried in the appropriate places?
> > >>
> > >> Signed-off-by: Alex Bennée <alex.ben...@linaro.org>
> > >> Cc: Stefano Garzarella <sgarz...@redhat.com>
> > >> Cc: "Michael S. Tsirkin" <m...@redhat.com>
> > >> Cc: Stefan Hajnoczi <stefa...@gmail.com>
> > >> ---
> > >> include/hw/virtio/vhost.h | 18 +++++++++++++++---
> > >> include/hw/virtio/virtio.h | 20 +++++++++++++++++++-
> > >> 2 files changed, 34 insertions(+), 4 deletions(-)
> > >>
> > >> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> > >> index 353252ac3e..502aa5677a 100644
> > >> --- a/include/hw/virtio/vhost.h
> > >> +++ b/include/hw/virtio/vhost.h
> > >> @@ -88,13 +88,25 @@ struct vhost_dev {
> > >> int vq_index_end;
> > >> /* if non-zero, minimum required value for max_queues */
> > >> int num_queues;
> > >> + /**
> > >> + * vhost feature handling requires matching the feature set
> > >> + * offered by a backend which may be a subset of the total
> > >> + * features eventually offered to the guest.
> > >> + *
> > >> + * @features: available features provided by the backend
> > >> + * @acked_features: final set of negotiated features with the
> > >> + * front-end driver
> > >> + * @backend_features: additional feature bits applied during
> > >> negotiation
> > >
> > > What does this mean?
> >
> > Well practically it is currently either applying
> > VHOST_USER_F_PROTOCOL_FEATURES to the vhost_user_set_features() or
> > storing VHOST_NET_F_VIRTIO_NET_HDR which I think eventually gets applied
> > to:
> >
> > net->dev.acked_features = net->dev.backend_features;
> >
> > I suspect both could be dropped and handled as flags and applied at the
> > destination.
>
> I noticed there is also a VirtIODevice::backend_features field that is
> barely used. virtio_net stashes vhost_net backend feature bits in
> there, but I think the code would be clearer if it was rewritten
> without the VirtIODevice::backend_features field.
>
> Having both VirtIODevice and struct vhost_dev fields with the same
> name is confusing. :/
>
> >
> > >
> > >> + *
> > >> + * Finally the @protocol_features is the final protocal feature
> > >
> > > s/protocal/protocol/
> > >
> > > All the other fields are VIRTIO feature bits and this field holds the
> > > VHOST_USER_SET_FEATURES feature bits?
> >
> > No these are the protocol features so a totally separate set of feature
> > bits for the vhost user protocol. I don't think these apply to kernel
> > vhost stuff?
>
> Oh, I see. Only vhost-user has this concept.
>
> How about "the vhost-user VHOST_USER_SET_PROTOCOL_FEATURES bits"?
>
> >
> > >
> > >> + * set negotiated between QEMU and the backend (after
> > >> + * VHOST_USER_F_PROTOCOL_FEATURES is negotiated)
> > >> + */
> > >> uint64_t features;
> > >> - /** @acked_features: final set of negotiated features */
> > >> uint64_t acked_features;
> > >> - /** @backend_features: backend specific feature bits */
> > >> uint64_t backend_features;
> > >> - /** @protocol_features: final negotiated protocol features */
> > >> uint64_t protocol_features;
> > >> +
> > >> uint64_t max_queues;
> > >> uint64_t backend_cap;
> > >> /* @started: is the vhost device started? */
> > >> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> > >> index a973811cbf..9939a0a632 100644
> > >> --- a/include/hw/virtio/virtio.h
> > >> +++ b/include/hw/virtio/virtio.h
> > >> @@ -93,6 +93,12 @@ enum virtio_device_endian {
> > >> VIRTIO_DEVICE_ENDIAN_BIG,
> > >> };
> > >>
> > >> +/**
> > >> + * struct VirtIODevice - common VirtIO structure
> > >> + * @name: name of the device
> > >> + * @status: VirtIO Device Status field
> > >> + *
> > >> + */
> > >> struct VirtIODevice
> > >> {
> > >> DeviceState parent_obj;
> > >> @@ -100,9 +106,21 @@ struct VirtIODevice
> > >> uint8_t status;
> > >> uint8_t isr;
> > >> uint16_t queue_sel;
> > >> - uint64_t guest_features;
> > >> + /**
> > >> + * These fields represent a set of VirtIO features at various
> > >> + * levels of the stack. @host_features indicates the complete
> > >> + * feature set the VirtIO device can offer to the driver.
> > >> + * @guest_features indicates which features the VirtIO driver can
> > >> + * support.
> > >
> > > The device never knows the features that the driver can support, so
> > > this sentence is ambiguous/incorrect. The device only knows the
> > > features that the driver writes during negotiation, which the spec
> > > says is a subset of host_features.
> > >
> > > Maybe "indicates the features that driver wrote"?
> > >
> > > I noticed that this field is assigned even when the guest writes
> > > invalid feature bits.
> >
> > Should we fix that? The negotiation sequence should be guest read, mask
> > and write back so the value should be validated against host_features?
>
> I think the negotiation/state machine is okay. Invalid features will
> not be accepted (negotiation fails).
>
> It's just weird that an invalid input from the guest can be stored in
> guest_features. Hopefully nothing trusts the guest_features value
> without first checking that negotiation succeeded (I haven't checked
> the code).
>
> Stefan
We can add more validation in virtio_set_features but I don't think
ultimately it matters much.
--
MST
