Re: [Qemu-devel] [PATCH v6 08/12] vhost-user: add vhost_user_input_get_config()

[Michael S. Tsirkin]

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 <address@hidden> 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 <address@hidden> 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 <address@hidden>
> > > > > Reviewed-by: Gerd Hoffmann <address@hidden>
> > > >
> > > > 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