Date:   Tue, 6 Jul 2021 10:34:33 +0800
From:   Jason Wang <jasowang@...hat.com>
To:     Stefan Hajnoczi <stefanha@...hat.com>
Cc:     Yongji Xie <xieyongji@...edance.com>,
        "Michael S. Tsirkin" <mst@...hat.com>,
        Stefano Garzarella <sgarzare@...hat.com>,
        Parav Pandit <parav@...dia.com>,
        Christoph Hellwig <hch@...radead.org>,
        Christian Brauner <christian.brauner@...onical.com>,
        Randy Dunlap <rdunlap@...radead.org>,
        Matthew Wilcox <willy@...radead.org>,
        Al Viro <viro@...iv.linux.org.uk>,
        Jens Axboe <axboe@...nel.dk>, bcrl@...ck.org,
        Jonathan Corbet <corbet@....net>,
        Mika Penttilä <mika.penttila@...tfour.com>,
        Dan Carpenter <dan.carpenter@...cle.com>, joro@...tes.org,
        Greg KH <gregkh@...uxfoundation.org>, songmuchun@...edance.com,
        virtualization <virtualization@...ts.linux-foundation.org>,
        netdev@...r.kernel.org, kvm <kvm@...r.kernel.org>,
        linux-fsdevel@...r.kernel.org, iommu@...ts.linux-foundation.org,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [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

Powered by blists - more mailing lists