On Tue, May 9, 2023 at 2:53 PM Marc-André Lureau <marcandre.lur...@gmail.com>
wrote:
> Hi
>
> On Tue, May 9, 2023 at 3:17 PM Albert Esteve <aest...@redhat.com> wrote:
>
>>
>> Hi!
>>
>> On Tue, May 9, 2023 at 12:12 PM Marc-André Lureau <
>> marcandre.lur...@gmail.com> wrote:
>>
>>> Hi
>>>
>>> On Wed, May 3, 2023 at 12:21 PM Albert Esteve <aest...@redhat.com>
>>> wrote:
>>>
>>>> Add vu_write_msg_cb type as a member of the VuDev
>>>> struct.
>>>>
>>>> In order to interact with the virtio-dmabuf
>>>> API, vhost-user backends have available a special
>>>> message type that can be sent to the frontend
>>>> in Qemu, in order to add, lookup, or remove
>>>> entries.
>>>>
>>>> To send these messages and avoid code replication,
>>>> backends will need the write_msg method to be exposed
>>>> to them, similarly to how the read_msg is for
>>>> receiving messages.
>>>>
>>>
>>> I think read_msg was introduced to blend libvhost-user IO to qemu
>>> mainloop & coroutine. Is that what you have in mind for write_msg?
>>>
>>
>> Uhm, after grep'ing, it seems that read_msg is only used within
>> libvhost-user source, so I guess it is mainly used to
>> allow backends to provide custom methods? Maybe I am misunderstanding.
>>
>> But my idea for adding `write_msg` is exposing the write method (i.e.,
>> vu_message_write) to the backends,
>> without having the function signature in the header. This way, vhost-user
>> backends that want to write a message,
>> can just use `dev->write_msg(args...)`. Which would be equivalent to
>> `vu_message_write(args...)` if this
>> was visible for others.
>>
>
>
> Imho it's better to introduce a normal function in that case, that is
> simply export vu_message_write_default().
>
>
>> Another option could be to have a specific public method sending the
>> requests to the frontend, that
>> internally, would use `vu_message_write`. But since we introduce three
>> new message types that
>> backends can send, I thought adding different methods would be a bit too
>> verbose.
>>
>
> Actually, exposing higher-level methods to send specific messages is more
> correct imho.
>
Then I will do that, thanks!
>
>
>>>
>>>> Signed-off-by: Albert Esteve <aest...@redhat.com>
>>>> ---
>>>> subprojects/libvhost-user/libvhost-user.c | 1 +
>>>> subprojects/libvhost-user/libvhost-user.h | 16 ++++++++++++++++
>>>> 2 files changed, 17 insertions(+)
>>>>
>>>> diff --git a/subprojects/libvhost-user/libvhost-user.c
>>>> b/subprojects/libvhost-user/libvhost-user.c
>>>> index 6b4b721225..c50b353915 100644
>>>> --- a/subprojects/libvhost-user/libvhost-user.c
>>>> +++ b/subprojects/libvhost-user/libvhost-user.c
>>>> @@ -2115,6 +2115,7 @@ vu_init(VuDev *dev,
>>>> dev->sock = socket;
>>>> dev->panic = panic;
>>>> dev->read_msg = read_msg ? read_msg : vu_message_read_default;
>>>> + dev->write_msg = vu_message_write;
>>>>
>>>
>>> You are not making it customizable? And the callback is not used.
>>>
>>
>> Making it customizable would require changing the signature of `vu_init`,
>> and I did not see
>> the need for this usecase. I just want to expose the static method to the
>> backends.
>>
>>
> ok
>
>
>> The callback is not used because there is still no virtio device to use
>> it. But this whole
>> infrastructure will be nice to have for the next device that would
>> require it (e.g., virtio-video).
>>
>> In that regard, this commit could be skipped from the PATCH and just
>> change it once there
>> is a virtio device that needs to send a `VHOST_USER_BACKEND_SHARED_OBJECT`
>> message. Basically, I needed it for testing (just had a dummy vhost-user
>> backend that I used for
>> sending messages), and then decided to keep it. But maybe having a
>> simpler patch is better.
>>
>>
>>>
>>>
>>>> dev->set_watch = set_watch;
>>>> dev->remove_watch = remove_watch;
>>>> dev->iface = iface;
>>>> diff --git a/subprojects/libvhost-user/libvhost-user.h
>>>> b/subprojects/libvhost-user/libvhost-user.h
>>>> index 784db65f7c..f5d7162886 100644
>>>> --- a/subprojects/libvhost-user/libvhost-user.h
>>>> +++ b/subprojects/libvhost-user/libvhost-user.h
>>>> @@ -242,6 +242,7 @@ typedef void (*vu_set_features_cb) (VuDev *dev,
>>>> uint64_t features);
>>>> typedef int (*vu_process_msg_cb) (VuDev *dev, VhostUserMsg *vmsg,
>>>> int *do_reply);
>>>> typedef bool (*vu_read_msg_cb) (VuDev *dev, int sock, VhostUserMsg
>>>> *vmsg);
>>>> +typedef bool (*vu_write_msg_cb) (VuDev *dev, int sock, VhostUserMsg
>>>> *vmsg);
>>>> typedef void (*vu_queue_set_started_cb) (VuDev *dev, int qidx, bool
>>>> started);
>>>> typedef bool (*vu_queue_is_processed_in_order_cb) (VuDev *dev, int
>>>> qidx);
>>>> typedef int (*vu_get_config_cb) (VuDev *dev, uint8_t *config, uint32_t
>>>> len);
>>>> @@ -429,6 +430,21 @@ struct VuDev {
>>>> */
>>>> vu_read_msg_cb read_msg;
>>>>
>>>> + /*
>>>> + * @write_msg: custom method to write vhost-user message
>>>> + *
>>>> + * Write data to vhost_user socket fd from the passed
>>>> + * VhostUserMsg *vmsg struct.
>>>> + *
>>>> + * For the details, please refer to vu_message_write in
>>>> libvhost-user.c
>>>> + * which will be used by default when calling vu_unit.
>>>> + * No custom method is allowed.
>>>>
>>>
>>> "No custom method is allowed"?
>>>
>>
>> I meant that I am not making it customizable (from your previous point),
>> as opposed to the `read_msg` method.
>>
>>
>>>
>>>
>>>> + *
>>>> + * Returns: true if vhost-user message successfully sent, false
>>>> otherwise.
>>>> + *
>>>> + */
>>>> + vu_write_msg_cb write_msg;
>>>> +
>>>>
>>>
>>>
>>> --
>>> Marc-André Lureau
>>>
>>
>
> --
> Marc-André Lureau
>
