Re: [PATCH v5 08/50] multi-process: add functions to synchronize proxy and remote endpoints

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Jag Raman (jag.raman@oracle.com) wrote:
> 
> 
> On 3/3/2020 2:56 PM, Dr. David Alan Gilbert wrote:
> > * Jagannathan Raman (jag.raman@oracle.com) wrote:
> > > In some cases, for example MMIO read, QEMU has to wait for the remote to
> > > complete a command before proceeding. An eventfd based mechanism is
> > > added to synchronize QEMU & remote process.
> > > 
> > > Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
> > > Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
> > > Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> > > ---
> > >   include/io/mpqemu-link.h |  7 +++++++
> > >   io/mpqemu-link.c         | 41 +++++++++++++++++++++++++++++++++++++++++
> > >   2 files changed, 48 insertions(+)
> > > 
> > > diff --git a/include/io/mpqemu-link.h b/include/io/mpqemu-link.h
> > > index 2f2dd83..ae04fca 100644
> > > --- a/include/io/mpqemu-link.h
> > > +++ b/include/io/mpqemu-link.h
> > > @@ -135,4 +135,11 @@ void mpqemu_link_set_callback(MPQemuLinkState *s,
> > >                                 mpqemu_link_callback callback);
> > >   void mpqemu_start_coms(MPQemuLinkState *s);
> > > +#define GET_REMOTE_WAIT eventfd(0, EFD_CLOEXEC)
> > > +#define PUT_REMOTE_WAIT(wait) close(wait)
> > > +#define PROXY_LINK_WAIT_DONE 1
> > > +
> > > +uint64_t wait_for_remote(int efd);
> > > +void notify_proxy(int fd, uint64_t val);
> > > +
> > >   #endif
> > > diff --git a/io/mpqemu-link.c b/io/mpqemu-link.c
> > > index bac120b..73b7032 100644
> > > --- a/io/mpqemu-link.c
> > > +++ b/io/mpqemu-link.c
> > > @@ -10,6 +10,7 @@
> > >   #include "qemu/osdep.h"
> > >   #include "qemu-common.h"
> > > +#include <poll.h>
> > >   #include "qemu/module.h"
> > >   #include "io/mpqemu-link.h"
> > > @@ -216,6 +217,46 @@ int mpqemu_msg_recv(MPQemuMsg *msg, MPQemuChannel *chan)
> > >       return rc;
> > >   }
> > > +uint64_t wait_for_remote(int efd)
> > > +{
> > > +    struct pollfd pfd = { .fd = efd, .events = POLLIN };
> > > +    uint64_t val;
> > > +    int ret;
> > > +
> > > +    ret = poll(&pfd, 1, 1000);
> > > +
> > > +    switch (ret) {
> > > +    case 0:
> > > +        qemu_log_mask(LOG_REMOTE_DEBUG, "Error wait_for_remote: Timed out\n");
> > > +        /* TODO: Kick-off error recovery */
> > > +        return ULLONG_MAX;
> > 
> > Shouldn't these be UINT64_MAX?
> 
> Sure, we'll change these to UINT64_MAX.
> 
> > 
> > > +    case -1:
> > > +        qemu_log_mask(LOG_REMOTE_DEBUG, "Poll error wait_for_remote: %s\n",
> > > +                      strerror(errno));
> > > +        return ULLONG_MAX;
> > > +    default:
> > > +        if (read(efd, &val, sizeof(val)) == -1) {
> > > +            qemu_log_mask(LOG_REMOTE_DEBUG, "Error wait_for_remote: %s\n",
> > > +                          strerror(errno));
> > > +            return ULLONG_MAX;
> > > +        }
> > > +    }
> > > +
> > > +    val = (val == ULLONG_MAX) ? val : (val - 1);
> > 
> > Can you explain what's going on there??
> 
> This wait_for_remote() function serves two purposes. This first one is
> to synchronize QEMU and the remote process. Secondly, it allows the
> remote process to return a 64-bit value to QEMU. When the remote process
> has completed the task, it uses the notify_proxy() function to return to
> QEMU along with a return value.
> 
> We have implemented this synchronization mechanism using eventfd, as
> it's easy to setup. So the remote process could write a non-zero value
> to the eventfd to wake QEMU up. However, the drawback of using eventfd
> for this purpose is that a return value of zero wouldn't wake QEMU up.
> Therefore, we offset the return value by one at the remote process and
> correct it in the QEMU end.

OK, that needs a big hairy comment to explain what's going on.

Dave

> --
> Jag
> 
> > 
> > > +    return val;
> > > +}
> > > +
> > > +void notify_proxy(int efd, uint64_t val)
> > > +{
> > > +    val = (val == ULLONG_MAX) ? val : (val + 1);
> > > +
> > > +    if (write(efd, &val, sizeof(val)) == -1) {
> > 
> > I'd actually check the write/read's are returning sizeof(val) - they
> > can on a bad day return 0 or send only a few bytes; in theory?
> > 
> > Dave
> > 
> > > +        qemu_log_mask(LOG_REMOTE_DEBUG, "Error notify_proxy: %s\n",
> > > +                      strerror(errno));
> > > +    }
> > > +}
> > > +
> > >   static gboolean mpqemu_link_handler_prepare(GSource *gsrc, gint *timeout)
> > >   {
> > >       g_assert(timeout);
> > > -- 
> > > 1.8.3.1
> > > 
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> > 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK