| Message ID | 20190308140454.32437-9-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | vhost-user-backend & vhost-user-input | expand |
On Fri, Mar 08, 2019 at 03:04:50PM +0100, Marc-André Lureau wrote: > Ask vhost user input backend the list of virtio_input_config. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > Reviewed-by: Gerd Hoffmann <kraxel@redhat.com> I was pushing this and I am puzzled now. > --- > contrib/libvhost-user/libvhost-user.h | 1 + > include/hw/virtio/vhost-backend.h | 4 ++ > hw/virtio/vhost-user.c | 60 +++++++++++++++++++++++++++ > docs/interop/vhost-user.txt | 8 ++++ > 4 files changed, 73 insertions(+) > > diff --git a/contrib/libvhost-user/libvhost-user.h b/contrib/libvhost-user/libvhost-user.h > index c0133b7f3f..b0c798fa1a 100644 > --- a/contrib/libvhost-user/libvhost-user.h > +++ b/contrib/libvhost-user/libvhost-user.h > @@ -91,6 +91,7 @@ typedef enum VhostUserRequest { > VHOST_USER_POSTCOPY_ADVISE = 28, > VHOST_USER_POSTCOPY_LISTEN = 29, > VHOST_USER_POSTCOPY_END = 30, > + VHOST_USER_INPUT_GET_CONFIG = 31, > VHOST_USER_MAX > } VhostUserRequest; > > diff --git a/include/hw/virtio/vhost-backend.h b/include/hw/virtio/vhost-backend.h > index 81283ec50f..1fca321d8a 100644 > --- a/include/hw/virtio/vhost-backend.h > +++ b/include/hw/virtio/vhost-backend.h > @@ -12,6 +12,7 @@ > #define VHOST_BACKEND_H > > #include "exec/memory.h" > +#include "standard-headers/linux/virtio_input.h" > > typedef enum VhostBackendType { > VHOST_BACKEND_TYPE_NONE = 0, > @@ -160,4 +161,7 @@ int vhost_backend_invalidate_device_iotlb(struct vhost_dev *dev, > int vhost_backend_handle_iotlb_msg(struct vhost_dev *dev, > struct vhost_iotlb_msg *imsg); > > +int vhost_user_input_get_config(struct vhost_dev *dev, > + struct virtio_input_config **config); > + > #endif /* VHOST_BACKEND_H */ > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c > index 5df73405bc..cf3fd39035 100644 > --- a/hw/virtio/vhost-user.c > +++ b/hw/virtio/vhost-user.c > @@ -93,6 +93,7 @@ typedef enum VhostUserRequest { > VHOST_USER_POSTCOPY_ADVISE = 28, > VHOST_USER_POSTCOPY_LISTEN = 29, > VHOST_USER_POSTCOPY_END = 30, > + VHOST_USER_INPUT_GET_CONFIG = 31, > VHOST_USER_MAX > } VhostUserRequest; > > @@ -342,6 +343,65 @@ static int vhost_user_write(struct vhost_dev *dev, VhostUserMsg *msg, > return 0; > } > > +static void *vhost_user_read_size(struct vhost_dev *dev, uint32_t size) > +{ > + struct vhost_user *u = dev->opaque; > + CharBackend *chr = u->user->chr; > + int r; > + uint8_t *p = g_malloc(size); > + > + r = qemu_chr_fe_read_all(chr, p, size); > + if (r != size) { > + error_report("Failed to read msg payload." > + " Read %d instead of %u.", r, size); > + g_free(p); > + return NULL; > + } > + > + return p; > +} > + > +int vhost_user_input_get_config(struct vhost_dev *dev, > + struct virtio_input_config **config) > +{ > + void *p = NULL; > + VhostUserMsg msg = { > + .hdr.request = VHOST_USER_INPUT_GET_CONFIG, > + .hdr.flags = VHOST_USER_VERSION, > + }; > + > + if (vhost_user_write(dev, &msg, NULL, 0) < 0) { > + goto err; > + } > + > + if (vhost_user_read_header(dev, &msg) < 0) { > + goto err; > + } > + > + if (msg.hdr.request != VHOST_USER_INPUT_GET_CONFIG) { > + error_report("Received unexpected msg type. Expected %d received %d", > + VHOST_USER_INPUT_GET_CONFIG, msg.hdr.request); > + goto err; > + } > + > + if (msg.hdr.size % sizeof(struct virtio_input_config)) { > + error_report("Invalid msg size"); > + goto err; > + } > + > + p = vhost_user_read_size(dev, msg.hdr.size); > + if (!p) { > + goto err; > + } > + > + *config = p; > + return msg.hdr.size / sizeof(struct virtio_input_config); > + > +err: > + g_free(p); > + return -1; > +} > + > static int vhost_user_set_log_base(struct vhost_dev *dev, uint64_t base, > struct vhost_log *log) > { > diff --git a/docs/interop/vhost-user.txt b/docs/interop/vhost-user.txt > index 9ee2a60cfb..e145b3ec55 100644 > --- a/docs/interop/vhost-user.txt > +++ b/docs/interop/vhost-user.txt > @@ -766,6 +766,14 @@ Master message types > was previously sent. > The value returned is an error indication; 0 is success. > > + * VHOST_USER_INPUT_GET_CONFIG > + Id: 31 > + Master payload: N/A > + Slave payload: (struct virtio_input_config)* > + > + Ask vhost user input backend the list of virtio_input_config, in > + host endianness. > + > Slave message types > ------------------- > Why do we need this? What is wrong with VHOST_USER_GET_CONFIG? And why host endianness? Everything else is little endian ... > -- > 2.21.0.4.g36eb1cb9cf
Hi On Tue, Mar 12, 2019 at 4:49 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Fri, Mar 08, 2019 at 03:04:50PM +0100, Marc-André Lureau wrote: > > Ask vhost user input backend the list of virtio_input_config. > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > Reviewed-by: Gerd Hoffmann <kraxel@redhat.com> > > I was pushing this and I am puzzled now. > > > > --- > > contrib/libvhost-user/libvhost-user.h | 1 + > > include/hw/virtio/vhost-backend.h | 4 ++ > > hw/virtio/vhost-user.c | 60 +++++++++++++++++++++++++++ > > docs/interop/vhost-user.txt | 8 ++++ > > 4 files changed, 73 insertions(+) > > > > diff --git a/contrib/libvhost-user/libvhost-user.h b/contrib/libvhost-user/libvhost-user.h > > index c0133b7f3f..b0c798fa1a 100644 > > --- a/contrib/libvhost-user/libvhost-user.h > > +++ b/contrib/libvhost-user/libvhost-user.h > > @@ -91,6 +91,7 @@ typedef enum VhostUserRequest { > > VHOST_USER_POSTCOPY_ADVISE = 28, > > VHOST_USER_POSTCOPY_LISTEN = 29, > > VHOST_USER_POSTCOPY_END = 30, > > + VHOST_USER_INPUT_GET_CONFIG = 31, > > VHOST_USER_MAX > > } VhostUserRequest; > > > > diff --git a/include/hw/virtio/vhost-backend.h b/include/hw/virtio/vhost-backend.h > > index 81283ec50f..1fca321d8a 100644 > > --- a/include/hw/virtio/vhost-backend.h > > +++ b/include/hw/virtio/vhost-backend.h > > @@ -12,6 +12,7 @@ > > #define VHOST_BACKEND_H > > > > #include "exec/memory.h" > > +#include "standard-headers/linux/virtio_input.h" > > > > typedef enum VhostBackendType { > > VHOST_BACKEND_TYPE_NONE = 0, > > @@ -160,4 +161,7 @@ int vhost_backend_invalidate_device_iotlb(struct vhost_dev *dev, > > int vhost_backend_handle_iotlb_msg(struct vhost_dev *dev, > > struct vhost_iotlb_msg *imsg); > > > > +int vhost_user_input_get_config(struct vhost_dev *dev, > > + struct virtio_input_config **config); > > + > > #endif /* VHOST_BACKEND_H */ > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c > > index 5df73405bc..cf3fd39035 100644 > > --- a/hw/virtio/vhost-user.c > > +++ b/hw/virtio/vhost-user.c > > @@ -93,6 +93,7 @@ typedef enum VhostUserRequest { > > VHOST_USER_POSTCOPY_ADVISE = 28, > > VHOST_USER_POSTCOPY_LISTEN = 29, > > VHOST_USER_POSTCOPY_END = 30, > > + VHOST_USER_INPUT_GET_CONFIG = 31, > > VHOST_USER_MAX > > } VhostUserRequest; > > > > @@ -342,6 +343,65 @@ static int vhost_user_write(struct vhost_dev *dev, VhostUserMsg *msg, > > return 0; > > } > > > > +static void *vhost_user_read_size(struct vhost_dev *dev, uint32_t size) > > +{ > > + struct vhost_user *u = dev->opaque; > > + CharBackend *chr = u->user->chr; > > + int r; > > + uint8_t *p = g_malloc(size); > > + > > + r = qemu_chr_fe_read_all(chr, p, size); > > + if (r != size) { > > + error_report("Failed to read msg payload." > > + " Read %d instead of %u.", r, size); > > + g_free(p); > > + return NULL; > > + } > > + > > + return p; > > +} > > + > > +int vhost_user_input_get_config(struct vhost_dev *dev, > > + struct virtio_input_config **config) > > +{ > > + void *p = NULL; > > + VhostUserMsg msg = { > > + .hdr.request = VHOST_USER_INPUT_GET_CONFIG, > > + .hdr.flags = VHOST_USER_VERSION, > > + }; > > + > > + if (vhost_user_write(dev, &msg, NULL, 0) < 0) { > > + goto err; > > + } > > + > > + if (vhost_user_read_header(dev, &msg) < 0) { > > + goto err; > > + } > > + > > + if (msg.hdr.request != VHOST_USER_INPUT_GET_CONFIG) { > > + error_report("Received unexpected msg type. Expected %d received %d", > > + VHOST_USER_INPUT_GET_CONFIG, msg.hdr.request); > > + goto err; > > + } > > + > > + if (msg.hdr.size % sizeof(struct virtio_input_config)) { > > + error_report("Invalid msg size"); > > + goto err; > > + } > > + > > + p = vhost_user_read_size(dev, msg.hdr.size); > > + if (!p) { > > + goto err; > > + } > > + > > + *config = p; > > + return msg.hdr.size / sizeof(struct virtio_input_config); > > + > > +err: > > + g_free(p); > > + return -1; > > +} > > + > > static int vhost_user_set_log_base(struct vhost_dev *dev, uint64_t base, > > struct vhost_log *log) > > { > > diff --git a/docs/interop/vhost-user.txt b/docs/interop/vhost-user.txt > > index 9ee2a60cfb..e145b3ec55 100644 > > --- a/docs/interop/vhost-user.txt > > +++ b/docs/interop/vhost-user.txt > > @@ -766,6 +766,14 @@ Master message types > > was previously sent. > > The value returned is an error indication; 0 is success. > > > > + * VHOST_USER_INPUT_GET_CONFIG > > + Id: 31 > > + Master payload: N/A > > + Slave payload: (struct virtio_input_config)* > > + > > + Ask vhost user input backend the list of virtio_input_config, in > > + host endianness. > > + > > Slave message types > > ------------------- > > > > Why do we need this? What is wrong with VHOST_USER_GET_CONFIG? We would need more messages to lookup the selected config, see virtio_input_get_config(). But it looks like we could reuse VHOST_USER_SET_CONFIG/VHOST_USER_GET_CONFIG. This will be lower-level, so the backend will have to do a bit more work. At the same time, the backend should probably be ready to handle those messages if qemu start using them. All in all, I think both are compatible, VHOST_USER_INPUT_GET_CONFIG is slightly easier for the backend. > And why host endianness? Everything else is little endian ... Hmm, I am not sure we correctly handle endianness in virtio_input_get_config(). virtio_input_add_config() do not use LE. Gerd, have you checked cross-endian scenarios? Otoh, hw/virtio/vhost-user.c, the protocol, seem to use host endianess anyway. Given that the solution works fine on same-endian and has been pending for a while, can we still get it merged and address the remaining issues during the freeze? Thanks!
> > And why host endianness? Everything else is little endian ... > > Hmm, I am not sure we correctly handle endianness in > virtio_input_get_config(). virtio_input_add_config() do not use LE. > Gerd, have you checked cross-endian scenarios? Sure. BE ppc tcg guest on LE x86 host works fine. All 16 and 32 bit fields in config space and protocol messages are defined to be little endian. It's a pure virtio-1.0 device after all ... Note that all the fields virtio_input_get_config() and virtio_input_add_config() are looking at are 8 bit. cheers, Gerd
Hi On Wed, Mar 13, 2019 at 7:26 AM Gerd Hoffmann <kraxel@redhat.com> wrote: > > > > And why host endianness? Everything else is little endian ... > > > > Hmm, I am not sure we correctly handle endianness in > > virtio_input_get_config(). virtio_input_add_config() do not use LE. > > Gerd, have you checked cross-endian scenarios? > > Sure. BE ppc tcg guest on LE x86 host works fine. > > All 16 and 32 bit fields in config space and protocol > messages are defined to be little endian. It's a pure > virtio-1.0 device after all ... > > Note that all the fields virtio_input_get_config() and > virtio_input_add_config() are looking at are 8 bit. > Right, thanks for clarifying. So we can simply drop "in host endianness" from VHOST_USER_INPUT_GET_CONFIG documentation.
On Tue, Mar 12, 2019 at 09:19:01PM +0100, Marc-André Lureau wrote: > Hi > > On Tue, Mar 12, 2019 at 4:49 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Fri, Mar 08, 2019 at 03:04:50PM +0100, Marc-André Lureau wrote: > > > Ask vhost user input backend the list of virtio_input_config. > > > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > > Reviewed-by: Gerd Hoffmann <kraxel@redhat.com> > > > > I was pushing this and I am puzzled now. > > > > > > > --- > > > contrib/libvhost-user/libvhost-user.h | 1 + > > > include/hw/virtio/vhost-backend.h | 4 ++ > > > hw/virtio/vhost-user.c | 60 +++++++++++++++++++++++++++ > > > docs/interop/vhost-user.txt | 8 ++++ > > > 4 files changed, 73 insertions(+) > > > > > > diff --git a/contrib/libvhost-user/libvhost-user.h b/contrib/libvhost-user/libvhost-user.h > > > index c0133b7f3f..b0c798fa1a 100644 > > > --- a/contrib/libvhost-user/libvhost-user.h > > > +++ b/contrib/libvhost-user/libvhost-user.h > > > @@ -91,6 +91,7 @@ typedef enum VhostUserRequest { > > > VHOST_USER_POSTCOPY_ADVISE = 28, > > > VHOST_USER_POSTCOPY_LISTEN = 29, > > > VHOST_USER_POSTCOPY_END = 30, > > > + VHOST_USER_INPUT_GET_CONFIG = 31, > > > VHOST_USER_MAX > > > } VhostUserRequest; > > > > > > diff --git a/include/hw/virtio/vhost-backend.h b/include/hw/virtio/vhost-backend.h > > > index 81283ec50f..1fca321d8a 100644 > > > --- a/include/hw/virtio/vhost-backend.h > > > +++ b/include/hw/virtio/vhost-backend.h > > > @@ -12,6 +12,7 @@ > > > #define VHOST_BACKEND_H > > > > > > #include "exec/memory.h" > > > +#include "standard-headers/linux/virtio_input.h" > > > > > > typedef enum VhostBackendType { > > > VHOST_BACKEND_TYPE_NONE = 0, > > > @@ -160,4 +161,7 @@ int vhost_backend_invalidate_device_iotlb(struct vhost_dev *dev, > > > int vhost_backend_handle_iotlb_msg(struct vhost_dev *dev, > > > struct vhost_iotlb_msg *imsg); > > > > > > +int vhost_user_input_get_config(struct vhost_dev *dev, > > > + struct virtio_input_config **config); > > > + > > > #endif /* VHOST_BACKEND_H */ > > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c > > > index 5df73405bc..cf3fd39035 100644 > > > --- a/hw/virtio/vhost-user.c > > > +++ b/hw/virtio/vhost-user.c > > > @@ -93,6 +93,7 @@ typedef enum VhostUserRequest { > > > VHOST_USER_POSTCOPY_ADVISE = 28, > > > VHOST_USER_POSTCOPY_LISTEN = 29, > > > VHOST_USER_POSTCOPY_END = 30, > > > + VHOST_USER_INPUT_GET_CONFIG = 31, > > > VHOST_USER_MAX > > > } VhostUserRequest; > > > > > > @@ -342,6 +343,65 @@ static int vhost_user_write(struct vhost_dev *dev, VhostUserMsg *msg, > > > return 0; > > > } > > > > > > +static void *vhost_user_read_size(struct vhost_dev *dev, uint32_t size) > > > +{ > > > + struct vhost_user *u = dev->opaque; > > > + CharBackend *chr = u->user->chr; > > > + int r; > > > + uint8_t *p = g_malloc(size); > > > + > > > + r = qemu_chr_fe_read_all(chr, p, size); > > > + if (r != size) { > > > + error_report("Failed to read msg payload." > > > + " Read %d instead of %u.", r, size); > > > + g_free(p); > > > + return NULL; > > > + } > > > + > > > + return p; > > > +} > > > + > > > +int vhost_user_input_get_config(struct vhost_dev *dev, > > > + struct virtio_input_config **config) > > > +{ > > > + void *p = NULL; > > > + VhostUserMsg msg = { > > > + .hdr.request = VHOST_USER_INPUT_GET_CONFIG, > > > + .hdr.flags = VHOST_USER_VERSION, > > > + }; > > > + > > > + if (vhost_user_write(dev, &msg, NULL, 0) < 0) { > > > + goto err; > > > + } > > > + > > > + if (vhost_user_read_header(dev, &msg) < 0) { > > > + goto err; > > > + } > > > + > > > + if (msg.hdr.request != VHOST_USER_INPUT_GET_CONFIG) { > > > + error_report("Received unexpected msg type. Expected %d received %d", > > > + VHOST_USER_INPUT_GET_CONFIG, msg.hdr.request); > > > + goto err; > > > + } > > > + > > > + if (msg.hdr.size % sizeof(struct virtio_input_config)) { > > > + error_report("Invalid msg size"); > > > + goto err; > > > + } > > > + > > > + p = vhost_user_read_size(dev, msg.hdr.size); > > > + if (!p) { > > > + goto err; > > > + } > > > + > > > + *config = p; > > > + return msg.hdr.size / sizeof(struct virtio_input_config); > > > + > > > +err: > > > + g_free(p); > > > + return -1; > > > +} > > > + > > > static int vhost_user_set_log_base(struct vhost_dev *dev, uint64_t base, > > > struct vhost_log *log) > > > { > > > diff --git a/docs/interop/vhost-user.txt b/docs/interop/vhost-user.txt > > > index 9ee2a60cfb..e145b3ec55 100644 > > > --- a/docs/interop/vhost-user.txt > > > +++ b/docs/interop/vhost-user.txt > > > @@ -766,6 +766,14 @@ Master message types > > > was previously sent. > > > The value returned is an error indication; 0 is success. > > > > > > + * VHOST_USER_INPUT_GET_CONFIG > > > + Id: 31 > > > + Master payload: N/A > > > + Slave payload: (struct virtio_input_config)* > > > + > > > + Ask vhost user input backend the list of virtio_input_config, in > > > + host endianness. > > > + > > > Slave message types > > > ------------------- > > > > > > > Why do we need this? What is wrong with VHOST_USER_GET_CONFIG? > > We would need more messages to lookup the selected config, see > virtio_input_get_config(). > > But it looks like we could reuse > VHOST_USER_SET_CONFIG/VHOST_USER_GET_CONFIG. This will be lower-level, > so the backend will have to do a bit more work. At the same time, the > backend should probably be ready to handle those messages if qemu > start using them. All in all, I think both are compatible, > VHOST_USER_INPUT_GET_CONFIG is slightly easier for the backend. I'm not sure what is meant by that. > > And why host endianness? Everything else is little endian ... > > Hmm, I am not sure we correctly handle endianness in > virtio_input_get_config(). virtio_input_add_config() do not use LE. > Gerd, have you checked cross-endian scenarios? > > Otoh, hw/virtio/vhost-user.c, the protocol, seem to use host endianess anyway. > > Given that the solution works fine on same-endian and has been pending > for a while, can we still get it merged and address the remaining > issues during the freeze? > > Thanks! It's mostly just an example device, isn't it? I don't see what the rush is frankly, and if we make mistakes in the protocol we are stuck maintaining it. > > > > -- > Marc-André Lureau
Hi On Wed, Mar 13, 2019 at 3:16 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Tue, Mar 12, 2019 at 09:19:01PM +0100, Marc-André Lureau wrote: > > Hi > > > > On Tue, Mar 12, 2019 at 4:49 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > On Fri, Mar 08, 2019 at 03:04:50PM +0100, Marc-André Lureau wrote: > > > > Ask vhost user input backend the list of virtio_input_config. > > > > > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > > > Reviewed-by: Gerd Hoffmann <kraxel@redhat.com> > > > > > > I was pushing this and I am puzzled now. > > > > > > > > > > --- > > > > contrib/libvhost-user/libvhost-user.h | 1 + > > > > include/hw/virtio/vhost-backend.h | 4 ++ > > > > hw/virtio/vhost-user.c | 60 +++++++++++++++++++++++++++ > > > > docs/interop/vhost-user.txt | 8 ++++ > > > > 4 files changed, 73 insertions(+) > > > > > > > > diff --git a/contrib/libvhost-user/libvhost-user.h b/contrib/libvhost-user/libvhost-user.h > > > > index c0133b7f3f..b0c798fa1a 100644 > > > > --- a/contrib/libvhost-user/libvhost-user.h > > > > +++ b/contrib/libvhost-user/libvhost-user.h > > > > @@ -91,6 +91,7 @@ typedef enum VhostUserRequest { > > > > VHOST_USER_POSTCOPY_ADVISE = 28, > > > > VHOST_USER_POSTCOPY_LISTEN = 29, > > > > VHOST_USER_POSTCOPY_END = 30, > > > > + VHOST_USER_INPUT_GET_CONFIG = 31, > > > > VHOST_USER_MAX > > > > } VhostUserRequest; > > > > > > > > diff --git a/include/hw/virtio/vhost-backend.h b/include/hw/virtio/vhost-backend.h > > > > index 81283ec50f..1fca321d8a 100644 > > > > --- a/include/hw/virtio/vhost-backend.h > > > > +++ b/include/hw/virtio/vhost-backend.h > > > > @@ -12,6 +12,7 @@ > > > > #define VHOST_BACKEND_H > > > > > > > > #include "exec/memory.h" > > > > +#include "standard-headers/linux/virtio_input.h" > > > > > > > > typedef enum VhostBackendType { > > > > VHOST_BACKEND_TYPE_NONE = 0, > > > > @@ -160,4 +161,7 @@ int vhost_backend_invalidate_device_iotlb(struct vhost_dev *dev, > > > > int vhost_backend_handle_iotlb_msg(struct vhost_dev *dev, > > > > struct vhost_iotlb_msg *imsg); > > > > > > > > +int vhost_user_input_get_config(struct vhost_dev *dev, > > > > + struct virtio_input_config **config); > > > > + > > > > #endif /* VHOST_BACKEND_H */ > > > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c > > > > index 5df73405bc..cf3fd39035 100644 > > > > --- a/hw/virtio/vhost-user.c > > > > +++ b/hw/virtio/vhost-user.c > > > > @@ -93,6 +93,7 @@ typedef enum VhostUserRequest { > > > > VHOST_USER_POSTCOPY_ADVISE = 28, > > > > VHOST_USER_POSTCOPY_LISTEN = 29, > > > > VHOST_USER_POSTCOPY_END = 30, > > > > + VHOST_USER_INPUT_GET_CONFIG = 31, > > > > VHOST_USER_MAX > > > > } VhostUserRequest; > > > > > > > > @@ -342,6 +343,65 @@ static int vhost_user_write(struct vhost_dev *dev, VhostUserMsg *msg, > > > > return 0; > > > > } > > > > > > > > +static void *vhost_user_read_size(struct vhost_dev *dev, uint32_t size) > > > > +{ > > > > + struct vhost_user *u = dev->opaque; > > > > + CharBackend *chr = u->user->chr; > > > > + int r; > > > > + uint8_t *p = g_malloc(size); > > > > + > > > > + r = qemu_chr_fe_read_all(chr, p, size); > > > > + if (r != size) { > > > > + error_report("Failed to read msg payload." > > > > + " Read %d instead of %u.", r, size); > > > > + g_free(p); > > > > + return NULL; > > > > + } > > > > + > > > > + return p; > > > > +} > > > > + > > > > +int vhost_user_input_get_config(struct vhost_dev *dev, > > > > + struct virtio_input_config **config) > > > > +{ > > > > + void *p = NULL; > > > > + VhostUserMsg msg = { > > > > + .hdr.request = VHOST_USER_INPUT_GET_CONFIG, > > > > + .hdr.flags = VHOST_USER_VERSION, > > > > + }; > > > > + > > > > + if (vhost_user_write(dev, &msg, NULL, 0) < 0) { > > > > + goto err; > > > > + } > > > > + > > > > + if (vhost_user_read_header(dev, &msg) < 0) { > > > > + goto err; > > > > + } > > > > + > > > > + if (msg.hdr.request != VHOST_USER_INPUT_GET_CONFIG) { > > > > + error_report("Received unexpected msg type. Expected %d received %d", > > > > + VHOST_USER_INPUT_GET_CONFIG, msg.hdr.request); > > > > + goto err; > > > > + } > > > > + > > > > + if (msg.hdr.size % sizeof(struct virtio_input_config)) { > > > > + error_report("Invalid msg size"); > > > > + goto err; > > > > + } > > > > + > > > > + p = vhost_user_read_size(dev, msg.hdr.size); > > > > + if (!p) { > > > > + goto err; > > > > + } > > > > + > > > > + *config = p; > > > > + return msg.hdr.size / sizeof(struct virtio_input_config); > > > > + > > > > +err: > > > > + g_free(p); > > > > + return -1; > > > > +} > > > > + > > > > static int vhost_user_set_log_base(struct vhost_dev *dev, uint64_t base, > > > > struct vhost_log *log) > > > > { > > > > diff --git a/docs/interop/vhost-user.txt b/docs/interop/vhost-user.txt > > > > index 9ee2a60cfb..e145b3ec55 100644 > > > > --- a/docs/interop/vhost-user.txt > > > > +++ b/docs/interop/vhost-user.txt > > > > @@ -766,6 +766,14 @@ Master message types > > > > was previously sent. > > > > The value returned is an error indication; 0 is success. > > > > > > > > + * VHOST_USER_INPUT_GET_CONFIG > > > > + Id: 31 > > > > + Master payload: N/A > > > > + Slave payload: (struct virtio_input_config)* > > > > + > > > > + Ask vhost user input backend the list of virtio_input_config, in > > > > + host endianness. > > > > + > > > > Slave message types > > > > ------------------- > > > > > > > > > > Why do we need this? What is wrong with VHOST_USER_GET_CONFIG? > > > > We would need more messages to lookup the selected config, see > > virtio_input_get_config(). > > > > But it looks like we could reuse > > VHOST_USER_SET_CONFIG/VHOST_USER_GET_CONFIG. This will be lower-level, > > so the backend will have to do a bit more work. At the same time, the > > backend should probably be ready to handle those messages if qemu > > start using them. All in all, I think both are compatible, > > VHOST_USER_INPUT_GET_CONFIG is slightly easier for the backend. > > I'm not sure what is meant by that. > > > > And why host endianness? Everything else is little endian ... > > > > Hmm, I am not sure we correctly handle endianness in > > virtio_input_get_config(). virtio_input_add_config() do not use LE. > > Gerd, have you checked cross-endian scenarios? > > > > Otoh, hw/virtio/vhost-user.c, the protocol, seem to use host endianess anyway. > > > > Given that the solution works fine on same-endian and has been pending > > for a while, can we still get it merged and address the remaining > > issues during the freeze? > > > > Thanks! > > It's mostly just an example device, isn't it? > I don't see what the rush is frankly, and if we make > mistakes in the protocol we are stuck maintaining it. This particular device isn't really important, no. However, "waiting" for 2y (mostly for review & feedback time) to get the vhost-user-gpu bits merged is a bit disappointing. Iirc, I was asked to generalize vhost-user first when proposing vhost-user-gpu, and vhost-user-input was the simplest. Ok, let's post-pone to 4.1, but please try to give that kind of review earlier, and not at the last minute before a freeze, as it delays the rest of the work for months again. thanks! > > > > > > > > > -- > > Marc-André Lureau
On Wed, Mar 13, 2019 at 03:27:27PM +0100, Marc-André Lureau wrote: > Hi > > On Wed, Mar 13, 2019 at 3:16 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Tue, Mar 12, 2019 at 09:19:01PM +0100, Marc-André Lureau wrote: > > > Hi > > > > > > On Tue, Mar 12, 2019 at 4:49 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > > > On Fri, Mar 08, 2019 at 03:04:50PM +0100, Marc-André Lureau wrote: > > > > > Ask vhost user input backend the list of virtio_input_config. > > > > > > > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > > > > Reviewed-by: Gerd Hoffmann <kraxel@redhat.com> > > > > > > > > I was pushing this and I am puzzled now. > > > > > > > > > > > > > --- > > > > > contrib/libvhost-user/libvhost-user.h | 1 + > > > > > include/hw/virtio/vhost-backend.h | 4 ++ > > > > > hw/virtio/vhost-user.c | 60 +++++++++++++++++++++++++++ > > > > > docs/interop/vhost-user.txt | 8 ++++ > > > > > 4 files changed, 73 insertions(+) > > > > > > > > > > diff --git a/contrib/libvhost-user/libvhost-user.h b/contrib/libvhost-user/libvhost-user.h > > > > > index c0133b7f3f..b0c798fa1a 100644 > > > > > --- a/contrib/libvhost-user/libvhost-user.h > > > > > +++ b/contrib/libvhost-user/libvhost-user.h > > > > > @@ -91,6 +91,7 @@ typedef enum VhostUserRequest { > > > > > VHOST_USER_POSTCOPY_ADVISE = 28, > > > > > VHOST_USER_POSTCOPY_LISTEN = 29, > > > > > VHOST_USER_POSTCOPY_END = 30, > > > > > + VHOST_USER_INPUT_GET_CONFIG = 31, > > > > > VHOST_USER_MAX > > > > > } VhostUserRequest; > > > > > > > > > > diff --git a/include/hw/virtio/vhost-backend.h b/include/hw/virtio/vhost-backend.h > > > > > index 81283ec50f..1fca321d8a 100644 > > > > > --- a/include/hw/virtio/vhost-backend.h > > > > > +++ b/include/hw/virtio/vhost-backend.h > > > > > @@ -12,6 +12,7 @@ > > > > > #define VHOST_BACKEND_H > > > > > > > > > > #include "exec/memory.h" > > > > > +#include "standard-headers/linux/virtio_input.h" > > > > > > > > > > typedef enum VhostBackendType { > > > > > VHOST_BACKEND_TYPE_NONE = 0, > > > > > @@ -160,4 +161,7 @@ int vhost_backend_invalidate_device_iotlb(struct vhost_dev *dev, > > > > > int vhost_backend_handle_iotlb_msg(struct vhost_dev *dev, > > > > > struct vhost_iotlb_msg *imsg); > > > > > > > > > > +int vhost_user_input_get_config(struct vhost_dev *dev, > > > > > + struct virtio_input_config **config); > > > > > + > > > > > #endif /* VHOST_BACKEND_H */ > > > > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c > > > > > index 5df73405bc..cf3fd39035 100644 > > > > > --- a/hw/virtio/vhost-user.c > > > > > +++ b/hw/virtio/vhost-user.c > > > > > @@ -93,6 +93,7 @@ typedef enum VhostUserRequest { > > > > > VHOST_USER_POSTCOPY_ADVISE = 28, > > > > > VHOST_USER_POSTCOPY_LISTEN = 29, > > > > > VHOST_USER_POSTCOPY_END = 30, > > > > > + VHOST_USER_INPUT_GET_CONFIG = 31, > > > > > VHOST_USER_MAX > > > > > } VhostUserRequest; > > > > > > > > > > @@ -342,6 +343,65 @@ static int vhost_user_write(struct vhost_dev *dev, VhostUserMsg *msg, > > > > > return 0; > > > > > } > > > > > > > > > > +static void *vhost_user_read_size(struct vhost_dev *dev, uint32_t size) > > > > > +{ > > > > > + struct vhost_user *u = dev->opaque; > > > > > + CharBackend *chr = u->user->chr; > > > > > + int r; > > > > > + uint8_t *p = g_malloc(size); > > > > > + > > > > > + r = qemu_chr_fe_read_all(chr, p, size); > > > > > + if (r != size) { > > > > > + error_report("Failed to read msg payload." > > > > > + " Read %d instead of %u.", r, size); > > > > > + g_free(p); > > > > > + return NULL; > > > > > + } > > > > > + > > > > > + return p; > > > > > +} > > > > > + > > > > > +int vhost_user_input_get_config(struct vhost_dev *dev, > > > > > + struct virtio_input_config **config) > > > > > +{ > > > > > + void *p = NULL; > > > > > + VhostUserMsg msg = { > > > > > + .hdr.request = VHOST_USER_INPUT_GET_CONFIG, > > > > > + .hdr.flags = VHOST_USER_VERSION, > > > > > + }; > > > > > + > > > > > + if (vhost_user_write(dev, &msg, NULL, 0) < 0) { > > > > > + goto err; > > > > > + } > > > > > + > > > > > + if (vhost_user_read_header(dev, &msg) < 0) { > > > > > + goto err; > > > > > + } > > > > > + > > > > > + if (msg.hdr.request != VHOST_USER_INPUT_GET_CONFIG) { > > > > > + error_report("Received unexpected msg type. Expected %d received %d", > > > > > + VHOST_USER_INPUT_GET_CONFIG, msg.hdr.request); > > > > > + goto err; > > > > > + } > > > > > + > > > > > + if (msg.hdr.size % sizeof(struct virtio_input_config)) { > > > > > + error_report("Invalid msg size"); > > > > > + goto err; > > > > > + } > > > > > + > > > > > + p = vhost_user_read_size(dev, msg.hdr.size); > > > > > + if (!p) { > > > > > + goto err; > > > > > + } > > > > > + > > > > > + *config = p; > > > > > + return msg.hdr.size / sizeof(struct virtio_input_config); > > > > > + > > > > > +err: > > > > > + g_free(p); > > > > > + return -1; > > > > > +} > > > > > + > > > > > static int vhost_user_set_log_base(struct vhost_dev *dev, uint64_t base, > > > > > struct vhost_log *log) > > > > > { > > > > > diff --git a/docs/interop/vhost-user.txt b/docs/interop/vhost-user.txt > > > > > index 9ee2a60cfb..e145b3ec55 100644 > > > > > --- a/docs/interop/vhost-user.txt > > > > > +++ b/docs/interop/vhost-user.txt > > > > > @@ -766,6 +766,14 @@ Master message types > > > > > was previously sent. > > > > > The value returned is an error indication; 0 is success. > > > > > > > > > > + * VHOST_USER_INPUT_GET_CONFIG > > > > > + Id: 31 > > > > > + Master payload: N/A > > > > > + Slave payload: (struct virtio_input_config)* > > > > > + > > > > > + Ask vhost user input backend the list of virtio_input_config, in > > > > > + host endianness. > > > > > + > > > > > Slave message types > > > > > ------------------- > > > > > > > > > > > > > Why do we need this? What is wrong with VHOST_USER_GET_CONFIG? > > > > > > We would need more messages to lookup the selected config, see > > > virtio_input_get_config(). > > > > > > But it looks like we could reuse > > > VHOST_USER_SET_CONFIG/VHOST_USER_GET_CONFIG. This will be lower-level, > > > so the backend will have to do a bit more work. At the same time, the > > > backend should probably be ready to handle those messages if qemu > > > start using them. All in all, I think both are compatible, > > > VHOST_USER_INPUT_GET_CONFIG is slightly easier for the backend. > > > > I'm not sure what is meant by that. > > > > > > And why host endianness? Everything else is little endian ... > > > > > > Hmm, I am not sure we correctly handle endianness in > > > virtio_input_get_config(). virtio_input_add_config() do not use LE. > > > Gerd, have you checked cross-endian scenarios? > > > > > > Otoh, hw/virtio/vhost-user.c, the protocol, seem to use host endianess anyway. > > > > > > Given that the solution works fine on same-endian and has been pending > > > for a while, can we still get it merged and address the remaining > > > issues during the freeze? > > > > > > Thanks! > > > > It's mostly just an example device, isn't it? > > I don't see what the rush is frankly, and if we make > > mistakes in the protocol we are stuck maintaining it. > > This particular device isn't really important, no. However, "waiting" > for 2y (mostly for review & feedback time) to get the vhost-user-gpu > bits merged is a bit disappointing. Iirc, I was asked to generalize > vhost-user first when proposing vhost-user-gpu, and vhost-user-input > was the simplest. Ok, let's post-pone to 4.1, but please try to give > that kind of review earlier, and not at the last minute before a > freeze, as it delays the rest of the work for months again. > > thanks! Sorry. I'll try harder but yes, it's always an issue with patch-sets that involves multiple maintainers. In this case I don't know enough about input/gpu and Gerd about vhost-user. Note that most of the patchset was merged, what's left besides input-specific bits is the backend object. That patch just got unlucky. There was upheaval around kconfig and attempts to add a new object just run into conflicts each time :( So I think after 4.1 you just need to get that one patch in, and then your work on gpu won't be blocked anymore. > > > > > > > > > > > > > > -- > > > Marc-André Lureau > > > > -- > Marc-André Lureau
diff --git a/contrib/libvhost-user/libvhost-user.h b/contrib/libvhost-user/libvhost-user.h index c0133b7f3f..b0c798fa1a 100644 --- a/contrib/libvhost-user/libvhost-user.h +++ b/contrib/libvhost-user/libvhost-user.h @@ -91,6 +91,7 @@ typedef enum VhostUserRequest { VHOST_USER_POSTCOPY_ADVISE = 28, VHOST_USER_POSTCOPY_LISTEN = 29, VHOST_USER_POSTCOPY_END = 30, + VHOST_USER_INPUT_GET_CONFIG = 31, VHOST_USER_MAX } VhostUserRequest; diff --git a/include/hw/virtio/vhost-backend.h b/include/hw/virtio/vhost-backend.h index 81283ec50f..1fca321d8a 100644 --- a/include/hw/virtio/vhost-backend.h +++ b/include/hw/virtio/vhost-backend.h @@ -12,6 +12,7 @@ #define VHOST_BACKEND_H #include "exec/memory.h" +#include "standard-headers/linux/virtio_input.h" typedef enum VhostBackendType { VHOST_BACKEND_TYPE_NONE = 0, @@ -160,4 +161,7 @@ int vhost_backend_invalidate_device_iotlb(struct vhost_dev *dev, int vhost_backend_handle_iotlb_msg(struct vhost_dev *dev, struct vhost_iotlb_msg *imsg); +int vhost_user_input_get_config(struct vhost_dev *dev, + struct virtio_input_config **config); + #endif /* VHOST_BACKEND_H */ diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c index 5df73405bc..cf3fd39035 100644 --- a/hw/virtio/vhost-user.c +++ b/hw/virtio/vhost-user.c @@ -93,6 +93,7 @@ typedef enum VhostUserRequest { VHOST_USER_POSTCOPY_ADVISE = 28, VHOST_USER_POSTCOPY_LISTEN = 29, VHOST_USER_POSTCOPY_END = 30, + VHOST_USER_INPUT_GET_CONFIG = 31, VHOST_USER_MAX } VhostUserRequest; @@ -342,6 +343,65 @@ static int vhost_user_write(struct vhost_dev *dev, VhostUserMsg *msg, return 0; } +static void *vhost_user_read_size(struct vhost_dev *dev, uint32_t size) +{ + struct vhost_user *u = dev->opaque; + CharBackend *chr = u->user->chr; + int r; + uint8_t *p = g_malloc(size); + + r = qemu_chr_fe_read_all(chr, p, size); + if (r != size) { + error_report("Failed to read msg payload." + " Read %d instead of %u.", r, size); + g_free(p); + return NULL; + } + + return p; +} + +int vhost_user_input_get_config(struct vhost_dev *dev, + struct virtio_input_config **config) +{ + void *p = NULL; + VhostUserMsg msg = { + .hdr.request = VHOST_USER_INPUT_GET_CONFIG, + .hdr.flags = VHOST_USER_VERSION, + }; + + if (vhost_user_write(dev, &msg, NULL, 0) < 0) { + goto err; + } + + if (vhost_user_read_header(dev, &msg) < 0) { + goto err; + } + + if (msg.hdr.request != VHOST_USER_INPUT_GET_CONFIG) { + error_report("Received unexpected msg type. Expected %d received %d", + VHOST_USER_INPUT_GET_CONFIG, msg.hdr.request); + goto err; + } + + if (msg.hdr.size % sizeof(struct virtio_input_config)) { + error_report("Invalid msg size"); + goto err; + } + + p = vhost_user_read_size(dev, msg.hdr.size); + if (!p) { + goto err; + } + + *config = p; + return msg.hdr.size / sizeof(struct virtio_input_config); + +err: + g_free(p); + return -1; +} + static int vhost_user_set_log_base(struct vhost_dev *dev, uint64_t base, struct vhost_log *log) { diff --git a/docs/interop/vhost-user.txt b/docs/interop/vhost-user.txt index 9ee2a60cfb..e145b3ec55 100644 --- a/docs/interop/vhost-user.txt +++ b/docs/interop/vhost-user.txt @@ -766,6 +766,14 @@ Master message types was previously sent. The value returned is an error indication; 0 is success. + * VHOST_USER_INPUT_GET_CONFIG + Id: 31 + Master payload: N/A + Slave payload: (struct virtio_input_config)* + + Ask vhost user input backend the list of virtio_input_config, in + host endianness. + Slave message types -------------------