On Tue, Jul 6, 2021 at 6:22 PM Stefan Hajnoczi <stefanha@xxxxxxxxxx> wrote: > > On Tue, Jul 06, 2021 at 11:04:18AM +0800, Yongji Xie wrote: > > On Mon, Jul 5, 2021 at 8:50 PM Stefan Hajnoczi <stefanha@xxxxxxxxxx> wrote: > > > > > > 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? > > > > > > > We add a timeout and return error in case userspace never replies to > > the message. > > > > > 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 noticed that virtio_config_read*() only returns -1 when we access a > > invalid field. But in the VDUSE case, VDUSE_DEV_GET_CONFIG might fail > > when we access a valid field. Not sure if it's ok to silently ignore > > this kind of error. > > That's a good point but it's a general VIRTIO issue. Any device > implementation (QEMU userspace, hardware vDPA, etc) can fail, so the > VIRTIO specification needs to provide a way for the driver to detect > this. > > If userspace violates the contract then VDUSE needs to mark the device > broken. QEMU's device emulation does something similar with the > vdev->broken flag. > > The VIRTIO Device Status field DEVICE_NEEDS_RESET bit can be set by > vDPA/VDUSE to indicate that the device is not operational and must be > reset. > It might be a solution. But DEVICE_NEEDS_RESET is not implemented currently. So I'm thinking whether it's ok to add a check of DEVICE_NEEDS_RESET status bit in probe function of virtio device driver (e.g. virtio-blk driver). Then VDUSE can make use of it to fail device initailization when configuration space access failed. Thanks, Yongji
