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

[Jag Raman <jag.raman@oracle.com>]

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
>