Re: [Qemu-devel] [PATCH 23/38] ivshmem: Receive shared memory synchronously in realize()

[Markus Armbruster]

Marc-André Lureau <address@hidden> writes:

> Hi
>
> On Mon, Feb 29, 2016 at 7:40 PM, Markus Armbruster <address@hidden> wrote:
>> When configured for interrupts (property "chardev" given), we receive
>> the shared memory from an ivshmem server.  We do so asynchronously
>> after realize() completes, by setting up callbacks with
>> qemu_chr_add_handlers().
>>
>> Keeping server I/O out of realize() that way avoids delays due to a
>> slow server.  This is probably relevant only for hot plug.
>>
>> However, this funny "no shared memory, yet" state of the device also
>> causes a raft of issues that are hard or impossible to work around:
>>
>> * The guest is exposed to this state: when we enter and leave it its
>>   shared memory contents is apruptly replaced, and device register
>>   IVPosition changes.
>>
>>   This is a known issue.  We document that guests should not access
>>   the shared memory after device initialization until the IVPosition
>>   register becomes non-negative.
>>
>>   For cold plug, the funny state is unlikely to be visible in
>>   practice, because we normally receive the shared memory long before
>>   the guest gets around to mess with the device.
>>
>>   For hot plug, the timing is tighter, but the relative slowness of
>>   PCI device configuration has a good chance to hide the funny state.
>>
>>   In either case, guests complying with the documented procedure are
>>   safe.
>>
>> * Migration becomes racy.
>>
>>   If migration completes before the shared memory setup completes on
>>   the source, shared memory contents is silently lost.  Fortunately,
>>   migration is rather unlikely to win this race.
>>
>>   If the shared memory's ramblock arrives at the destination before
>>   shared memory setup completes, migration fails.
>>
>>   There is no known way for a management application to wait for
>>   shared memory setup to complete.
>>
>>   All you can do is retry failed migration.  You can improve your
>>   chances by leaving more time between running the destination QEMU
>>   and the migrate command.
>>
>>   To mitigate silent memory loss, you need to ensure the server
>>   initializes shared memory exactly the same on source and
>>   destination.
>>
>>   These issues are entirely undocumented so far.
>>
>> I'd expect the server to be almost always fast enough to hide these
>> issues.  But then rare catastrophic races are in a way the worst kind.
>>
>> This is way more trouble than I'm willing to take from any device.
>> Kill the funny state by receiving shared memory synchronously in
>> realize().  If your hot plug hangs, go kill your ivshmem server.
>>
>> For easier review, this commit only makes the receive synchronous, it
>> doesn't add the necessary error propagation.  Without that, the funny
>> state persists.  The next commit will do that, and kill it off for
>> real.
>>
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>  hw/misc/ivshmem.c    | 70 
>> +++++++++++++++++++++++++++++++++++++---------------
>>  tests/ivshmem-test.c | 26 ++++++-------------
>>  2 files changed, 57 insertions(+), 39 deletions(-)
>>
>> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
>> index c366087..352937f 100644
>> --- a/hw/misc/ivshmem.c
>> +++ b/hw/misc/ivshmem.c
>> @@ -676,27 +676,47 @@ static void ivshmem_read(void *opaque, const uint8_t 
>> *buf, int size)
>>      process_msg(s, incoming_posn, incoming_fd);
>>  }
>>
>> -static void ivshmem_check_version(void *opaque, const uint8_t * buf, int 
>> size)
>> +static int64_t ivshmem_recv_msg(IVShmemState *s, int *pfd)
>>  {
>> -    IVShmemState *s = opaque;
>> -    int tmp;
>> -    int64_t version;
>> +    int64_t msg;
>> +    int n, ret;
>>
>> -    if (!fifo_update_and_get_i64(s, buf, size, &version)) {
>> -        return;
>> -    }
>> +    n = 0;
>> +    do {
>> +        ret = qemu_chr_fe_read_all(s->server_chr, (uint8_t *)&msg + n,
>> +                                 sizeof(msg) - n);
>> +        if (ret < 0 && ret != -EINTR) {
>> +            /* TODO error handling */
>> +            return INT64_MIN;
>> +        }
>> +        n += ret;
>> +    } while (n < sizeof(msg));
>>
>> -    tmp = qemu_chr_fe_get_msgfd(s->server_chr);
>> -    if (tmp != -1 || version != IVSHMEM_PROTOCOL_VERSION) {
>> +    *pfd = qemu_chr_fe_get_msgfd(s->server_chr);
>> +    return msg;
>> +}
>> +
>> +static void ivshmem_recv_setup(IVShmemState *s)
>> +{
>> +    int64_t msg;
>> +    int fd;
>> +
>> +    msg = ivshmem_recv_msg(s, &fd);
>> +    if (fd != -1 || msg != IVSHMEM_PROTOCOL_VERSION) {
>>          fprintf(stderr, "incompatible version, you are connecting to a 
>> ivshmem-"
>>                  "server using a different protocol please check your 
>> setup\n");
>> -        qemu_chr_add_handlers(s->server_chr, NULL, NULL, NULL, s);
>>          return;
>>      }
>>
>> -    IVSHMEM_DPRINTF("version check ok, switch to real chardev handler\n");
>> -    qemu_chr_add_handlers(s->server_chr, ivshmem_can_receive, ivshmem_read,
>> -                          NULL, s);
>> +    /*
>> +     * Receive more messages until we got shared memory.
>> +     */
>> +    do {
>> +        msg = ivshmem_recv_msg(s, &fd);
>> +        process_msg(s, msg, fd);
>> +    } while (msg != -1);
>> +
>> +    assert(memory_region_is_mapped(&s->ivshmem));
>
> It is easy to trigger at runtime, I suggest to report an error instead.

On successful return, the shared memory must be mapped.  To ensure that,
the code receives messages until it is.  The assertion double-checks the
code got it right.

However, you're right in that in this half-finished state, the assertion
is problematic: since we don't yet propagate errors, we take the success
path even when process_msg_shmem() failed.

I guess I'll have to turn the assertion into a comment until the next
patch.

> looks good otherwise

Thanks!