| Message ID | 20230503081911.119168-5-aesteve@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Virtio shared dma-buf | expand |
Hi On Wed, May 3, 2023 at 12:21 PM Albert Esteve <aesteve@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? > Signed-off-by: Albert Esteve <aesteve@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. > 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"? > + * > + * Returns: true if vhost-user message successfully sent, false > otherwise. > + * > + */ > + vu_write_msg_cb write_msg; > + >
Hi! On Tue, May 9, 2023 at 12:12 PM Marc-André Lureau < marcandre.lureau@gmail.com> wrote: > Hi > > On Wed, May 3, 2023 at 12:21 PM Albert Esteve <aesteve@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. 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. > >> Signed-off-by: Albert Esteve <aesteve@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. 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 >
Hi On Tue, May 9, 2023 at 3:17 PM Albert Esteve <aesteve@redhat.com> wrote: > > Hi! > > On Tue, May 9, 2023 at 12:12 PM Marc-André Lureau < > marcandre.lureau@gmail.com> wrote: > >> Hi >> >> On Wed, May 3, 2023 at 12:21 PM Albert Esteve <aesteve@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. >> >>> Signed-off-by: Albert Esteve <aesteve@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 >> >
On Tue, May 9, 2023 at 2:53 PM Marc-André Lureau <marcandre.lureau@gmail.com> wrote: > Hi > > On Tue, May 9, 2023 at 3:17 PM Albert Esteve <aesteve@redhat.com> wrote: > >> >> Hi! >> >> On Tue, May 9, 2023 at 12:12 PM Marc-André Lureau < >> marcandre.lureau@gmail.com> wrote: >> >>> Hi >>> >>> On Wed, May 3, 2023 at 12:21 PM Albert Esteve <aesteve@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 <aesteve@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 >
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; 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. + * + * Returns: true if vhost-user message successfully sent, false otherwise. + * + */ + vu_write_msg_cb write_msg; + /* * @set_watch: add or update the given fd to the watch set, * call cb when condition is met.
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. Signed-off-by: Albert Esteve <aesteve@redhat.com> --- subprojects/libvhost-user/libvhost-user.c | 1 + subprojects/libvhost-user/libvhost-user.h | 16 ++++++++++++++++ 2 files changed, 17 insertions(+)