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

[Jag Raman]

On 3/3/2020 2:56 PM, Dr. David Alan Gilbert wrote:
> * Jagannathan Raman (address@hidden) 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 <address@hidden>
> > Signed-off-by: Jagannathan Raman <address@hidden>
> > Signed-off-by: Elena Ufimtseva <address@hidden>
> > ---
> >   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.

--
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 / address@hidden / Manchester, UK