* Jag Raman (jag.ra...@oracle.com) wrote:
>
>
> On 3/3/2020 2:56 PM, Dr. David Alan Gilbert wrote:
> > * Jagannathan Raman (jag.ra...@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.john...@oracle.com>
> > > Signed-off-by: Jagannathan Raman <jag.ra...@oracle.com>
> > > Signed-off-by: Elena Ufimtseva <elena.ufimts...@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 / dgilb...@redhat.com / Manchester, UK
> >
>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
