Stefan Hajnoczi
2021-Jul-05 12:49 UTC
[PATCH v8 10/10] Documentation: Add documentation for VDUSE
On Mon, Jul 05, 2021 at 11:36:15AM +0800, Jason Wang wrote:> > ? 2021/7/4 ??5:49, Yongji Xie ??: > > > > OK, I get you now. Since the VIRTIO specification says "Device > > > > configuration space is generally used for rarely-changing or > > > > initialization-time parameters". I assume the VDUSE_DEV_SET_CONFIG > > > > ioctl should not be called frequently. > > > The spec uses MUST and other terms to define the precise requirements. > > > Here the language (especially the word "generally") is weaker and means > > > there may be exceptions. > > > > > > Another type of access that doesn't work with the VDUSE_DEV_SET_CONFIG > > > approach is reads that have side-effects. For example, imagine a field > > > containing an error code if the device encounters a problem unrelated to > > > a specific virtqueue request. Reading from this field resets the error > > > code to 0, saving the driver an extra configuration space write access > > > and possibly race conditions. It isn't possible to implement those > > > semantics suing VDUSE_DEV_SET_CONFIG. It's another corner case, but it > > > makes me think that the interface does not allow full VIRTIO semantics. > > > Note that though you're correct, my understanding is that config space is > not suitable for this kind of error propagating. And it would be very hard > to implement such kind of semantic in some transports.? Virtqueue should be > much better. As Yong Ji quoted, the config space is used for > "rarely-changing or intialization-time parameters". > > > > Agreed. I will use VDUSE_DEV_GET_CONFIG in the next version. And to > > handle the message failure, I'm going to add a return value to > > virtio_config_ops.get() and virtio_cread_* API so that the error can > > be propagated to the virtio device driver. Then the virtio-blk device > > driver can be modified to handle that. > > > > Jason and Stefan, what do you think of this way?Why does VDUSE_DEV_GET_CONFIG need to support an error return value? The VIRTIO spec provides no way for the device to report errors from config space accesses. The QEMU virtio-pci implementation returns -1 from invalid virtio_config_read*() and silently discards virtio_config_write*() accesses. VDUSE can take the same approach with VDUSE_DEV_GET_CONFIG/VDUSE_DEV_SET_CONFIG.> I'd like to stick to the current assumption thich get_config won't fail. > That is to say, > > 1) maintain a config in the kernel, make sure the config space read can > always succeed > 2) introduce an ioctl for the vduse usersapce to update the config space. > 3) we can synchronize with the vduse userspace during set_config > > Does this work?I noticed that caching is also allowed by the vhost-user protocol messages (QEMU's docs/interop/vhost-user.rst), but the device doesn't know whether or not caching is in effect. The interface you outlined above requires caching. Is there a reason why the host kernel vDPA code needs to cache the configuration space? Here are the vhost-user protocol messages: Virtio device config space ^^^^^^^^^^^^^^^^^^^^^^^^^^ +--------+------+-------+---------+ | offset | size | flags | payload | +--------+------+-------+---------+ :offset: a 32-bit offset of virtio device's configuration space :size: a 32-bit configuration space access size in bytes :flags: a 32-bit value: - 0: Vhost master messages used for writeable fields - 1: Vhost master messages used for live migration :payload: Size bytes array holding the contents of the virtio device's configuration space ... ``VHOST_USER_GET_CONFIG`` :id: 24 :equivalent ioctl: N/A :master payload: virtio device config space :slave payload: virtio device config space When ``VHOST_USER_PROTOCOL_F_CONFIG`` is negotiated, this message is submitted by the vhost-user master to fetch the contents of the virtio device configuration space, vhost-user slave's payload size MUST match master's request, vhost-user slave uses zero length of payload to indicate an error to vhost-user master. The vhost-user master may cache the contents to avoid repeated ``VHOST_USER_GET_CONFIG`` calls. ``VHOST_USER_SET_CONFIG`` :id: 25 :equivalent ioctl: N/A :master payload: virtio device config space :slave payload: N/A When ``VHOST_USER_PROTOCOL_F_CONFIG`` is negotiated, this message is submitted by the vhost-user master when the Guest changes the virtio device configuration space and also can be used for live migration on the destination host. The vhost-user slave must check the flags field, and slaves MUST NOT accept SET_CONFIG for read-only configuration space fields unless the live migration bit is set. Stefan -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 488 bytes Desc: not available URL: <http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20210705/8f85b1a1/attachment.sig>
Jason Wang
2021-Jul-06 02:34 UTC
[PATCH v8 10/10] Documentation: Add documentation for VDUSE
? 2021/7/5 ??8:49, Stefan Hajnoczi ??:> On Mon, Jul 05, 2021 at 11:36:15AM +0800, Jason Wang wrote: >> ? 2021/7/4 ??5:49, Yongji Xie ??: >>>>> OK, I get you now. Since the VIRTIO specification says "Device >>>>> configuration space is generally used for rarely-changing or >>>>> initialization-time parameters". I assume the VDUSE_DEV_SET_CONFIG >>>>> ioctl should not be called frequently. >>>> The spec uses MUST and other terms to define the precise requirements. >>>> Here the language (especially the word "generally") is weaker and means >>>> there may be exceptions. >>>> >>>> Another type of access that doesn't work with the VDUSE_DEV_SET_CONFIG >>>> approach is reads that have side-effects. For example, imagine a field >>>> containing an error code if the device encounters a problem unrelated to >>>> a specific virtqueue request. Reading from this field resets the error >>>> code to 0, saving the driver an extra configuration space write access >>>> and possibly race conditions. It isn't possible to implement those >>>> semantics suing VDUSE_DEV_SET_CONFIG. It's another corner case, but it >>>> makes me think that the interface does not allow full VIRTIO semantics. >> >> Note that though you're correct, my understanding is that config space is >> not suitable for this kind of error propagating. And it would be very hard >> to implement such kind of semantic in some transports.? Virtqueue should be >> much better. As Yong Ji quoted, the config space is used for >> "rarely-changing or intialization-time parameters". >> >> >>> Agreed. I will use VDUSE_DEV_GET_CONFIG in the next version. And to >>> handle the message failure, I'm going to add a return value to >>> virtio_config_ops.get() and virtio_cread_* API so that the error can >>> be propagated to the virtio device driver. Then the virtio-blk device >>> driver can be modified to handle that. >>> >>> Jason and Stefan, what do you think of this way? > Why does VDUSE_DEV_GET_CONFIG need to support an error return value? > > The VIRTIO spec provides no way for the device to report errors from > config space accesses. > > The QEMU virtio-pci implementation returns -1 from invalid > virtio_config_read*() and silently discards virtio_config_write*() > accesses. > > VDUSE can take the same approach with > VDUSE_DEV_GET_CONFIG/VDUSE_DEV_SET_CONFIG. > >> I'd like to stick to the current assumption thich get_config won't fail. >> That is to say, >> >> 1) maintain a config in the kernel, make sure the config space read can >> always succeed >> 2) introduce an ioctl for the vduse usersapce to update the config space. >> 3) we can synchronize with the vduse userspace during set_config >> >> Does this work? > I noticed that caching is also allowed by the vhost-user protocol > messages (QEMU's docs/interop/vhost-user.rst), but the device doesn't > know whether or not caching is in effect. The interface you outlined > above requires caching. > > Is there a reason why the host kernel vDPA code needs to cache the > configuration space?Because: 1) Kernel can not wait forever in get_config(), this is the major difference with vhost-user. 2) Stick to the current assumption that virtio_cread() should always succeed. Thanks> > Here are the vhost-user protocol messages: > > Virtio device config space > ^^^^^^^^^^^^^^^^^^^^^^^^^^ > > +--------+------+-------+---------+ > | offset | size | flags | payload | > +--------+------+-------+---------+ > > :offset: a 32-bit offset of virtio device's configuration space > > :size: a 32-bit configuration space access size in bytes > > :flags: a 32-bit value: > - 0: Vhost master messages used for writeable fields > - 1: Vhost master messages used for live migration > > :payload: Size bytes array holding the contents of the virtio > device's configuration space > > ... > > ``VHOST_USER_GET_CONFIG`` > :id: 24 > :equivalent ioctl: N/A > :master payload: virtio device config space > :slave payload: virtio device config space > > When ``VHOST_USER_PROTOCOL_F_CONFIG`` is negotiated, this message is > submitted by the vhost-user master to fetch the contents of the > virtio device configuration space, vhost-user slave's payload size > MUST match master's request, vhost-user slave uses zero length of > payload to indicate an error to vhost-user master. The vhost-user > master may cache the contents to avoid repeated > ``VHOST_USER_GET_CONFIG`` calls. > > ``VHOST_USER_SET_CONFIG`` > :id: 25 > :equivalent ioctl: N/A > :master payload: virtio device config space > :slave payload: N/A > > When ``VHOST_USER_PROTOCOL_F_CONFIG`` is negotiated, this message is > submitted by the vhost-user master when the Guest changes the virtio > device configuration space and also can be used for live migration > on the destination host. The vhost-user slave must check the flags > field, and slaves MUST NOT accept SET_CONFIG for read-only > configuration space fields unless the live migration bit is set. > > Stefan