Re: [RFC PATCH] include/hw: attempt to document VirtIO feature variables (!DISCUSS!)

[Alex Bennée]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Mon, Nov 21, 2022 at 07:15:30PM +0000, Alex Bennée wrote:
>> 
>> Stefan Hajnoczi <stefanha@gmail.com> writes:
>> 
>> > On Mon, 21 Nov 2022 at 09:49, Alex Bennée <alex.bennee@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.bennee@linaro.org>
>> >> Cc: Stefano Garzarella <sgarzare@redhat.com>
>> >> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>> >> Cc: Stefan Hajnoczi <stefanha@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.
>> 
>> >
>> >> +     *
>> >> +     * 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?
>> 
>> >
>> >> +     * 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?
>> 
>> >
>> >> Finally @backend_features represents everything
>> >> +     * supported by the backend. This set might be split between stuff
>> >> +     * done by QEMU itself and stuff handled by an external backend
>> >> +     * (e.g. vhost). As a result some feature bits may be added or
>> >> +     * masked from the backend.
>> >
>> > I'm not 100% sure what this is referring to. Transport features that
>> > are handled by QEMU and not the backend?
>> 
>> Well there is the rub. While looking at the reset stuff it was
>> postulated a device could support reset even if vhost part couldn't.
>
> reset here referring to per-ring reset?  It's possible with enough work
> - you would save ring state for each ring, reset all of vhost, then
> restore all but the one ring that needs to be reset.

Does anything work like this at the moment or is it a fair assumption
that the feature set of a vhost/vhost-user backend will be the maximum
set of features the guest can select from?

>
>> If
>> that is not true maybe we should drop this because host_features should
>> have everything we need?
>> 
>> >
>> >> +     */
>> >>      uint64_t host_features;
>> >> +    uint64_t guest_features;
>> >>      uint64_t backend_features;
>> >> +
>> >>      size_t config_len;
>> >>      void *config;
>> >>      uint16_t config_vector;
>> >> --
>> >> 2.34.1
>> >>
>> 
>> 
>> -- 
>> Alex Bennée


-- 
Alex Bennée