Hi
On Tue, Mar 12, 2019 at 4:49 PM Michael S. Tsirkin <m...@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.lur...@redhat.com>
> > Reviewed-by: Gerd Hoffmann <kra...@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!
--
Marc-André Lureau
