| Message ID | 3d013f0091ac6babb55f30cadf6ed806c322b7ce.1582576372.git.jag.raman@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Initial support for multi-process qemu | expand |
* 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? > + 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?? > + 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
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. -- 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 >
* 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
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; + 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); + + return val; +} + +void notify_proxy(int efd, uint64_t val) +{ + val = (val == ULLONG_MAX) ? val : (val + 1); + + if (write(efd, &val, sizeof(val)) == -1) { + 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);