| Message ID | 20221121144921.2830330-1-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] include/hw: attempt to document VirtIO feature variables (!DISCUSS!) | expand |
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? > + * > + * 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? > + * 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. > 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? > + */ > 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 >
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. 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 >>
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. > 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
"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
On Mon, 21 Nov 2022 at 14:25, Alex Bennée <alex.bennee@linaro.org> 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. 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
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.bennee@linaro.org> 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. > > 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.
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 + * + * Finally the @protocol_features is the final protocal feature + * 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. 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. + */ uint64_t host_features; + uint64_t guest_features; uint64_t backend_features; + size_t config_len; void *config; uint16_t config_vector;
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(-)