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.
>
>> + *
>> + * 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. 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
