Re: [PATCH] hw/char/pl011: Output characters using best-effort mode

[Gavin Shan <gshan@redhat.com>]

Hi Philippe,

On 2/20/20 7:47 PM, Philippe Mathieu-Daudé wrote:
> Cc'ing the chardev maintainers:
> 
> ./scripts/get_maintainer.pl -f chardev/char-fe.c
> "Marc-André Lureau" <marcandre.lureau@redhat.com> (maintainer:Character device...)
> Paolo Bonzini <pbonzini@redhat.com> (reviewer:Character device...)
> qemu-devel@nongnu.org (open list:All patches CC here)
> 

Thanks for keeping right persons copied :)

> 
> On 2/20/20 7:01 AM, Gavin Shan wrote:
>> Currently, PL011 is used by ARM virt board by default. It's possible to
>> block the system from booting. With below parameters in command line, the
>> backend could run into endless attempts of transmitting packets, which
>> can't succeed because of running out of sending buffer. The socket might
>> be not accepted n server side. It's not correct because disconnected
>> serial port shouldn't stop the system from booting.
>>
>>     -machine virt,gic-version=3 -cpu max -m 4096
>>     -monitor none -serial tcp:127.0.0.1:50900
> 
> Is the behavior similar when using the 'nowait' option?
> 

The issue happens on TCP client side, but 'nowait' is used for TCP
server according to the following document. I got same behavior after
giving a 'nowait' in my case.

https://qemu.weilnetz.de/doc/qemu-doc.html   (search 'nowait')

nowait specifies that QEMU should not block waiting for a client to connect
to a listening socket.

>>
>> The issue can be reproduced by starting a program which listens on TCP
>> port 50900 and then sleep without accepting any incoming connections. On
>> the other hand, a VM is started with above parameters and modified qemu
>> where the PL011 is flooded with 5000K data after it's created. Eventually,
>> the flooding won't proceed and stops after transmitting 2574K data. It's
>> basically to simulate tons of output from EDK-II and demonstrates how the
>> tons of output can block the system from booting.
>>
>> This fixes the issue by using newly added API qemu_chr_fe_try_write_all(),
>> which provides another type of service (best-effort). It's different from
>> qemu_chr_fe_write_all() as the data will be dropped if the backend has
>> been running into so-called broken state or 50 attempts of transmissions.
>> The broken state is cleared if the data is transmitted at once.
>>
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>> ---
>>   chardev/char-fe.c         | 15 +++++++++++++--
>>   chardev/char.c            | 20 ++++++++++++++------
>>   hw/char/pl011.c           |  5 +----
>>   include/chardev/char-fe.h | 14 ++++++++++++++
>>   include/chardev/char.h    |  6 ++++--
>>   5 files changed, 46 insertions(+), 14 deletions(-)
>>
>> diff --git a/chardev/char-fe.c b/chardev/char-fe.c
>> index f3530a90e6..6558fcfb94 100644
>> --- a/chardev/char-fe.c
>> +++ b/chardev/char-fe.c
>> @@ -39,7 +39,7 @@ int qemu_chr_fe_write(CharBackend *be, const uint8_t *buf, int len)
>>           return 0;
>>       }
>> -    return qemu_chr_write(s, buf, len, false);
>> +    return qemu_chr_write(s, buf, len, false, false);
>>   }
>>   int qemu_chr_fe_write_all(CharBackend *be, const uint8_t *buf, int len)
>> @@ -50,7 +50,18 @@ int qemu_chr_fe_write_all(CharBackend *be, const uint8_t *buf, int len)
>>           return 0;
>>       }
>> -    return qemu_chr_write(s, buf, len, true);
>> +    return qemu_chr_write(s, buf, len, true, false);
>> +}
>> +
>> +int qemu_chr_fe_try_write_all(CharBackend *be, const uint8_t *buf, int len)
>> +{
>> +    Chardev *s = be->chr;
>> +
>> +    if (!s) {
>> +        return 0;
>> +    }
>> +
>> +    return qemu_chr_write(s, buf, len, true, true);
>>   }
>>   int qemu_chr_fe_read_all(CharBackend *be, uint8_t *buf, int len)
>> diff --git a/chardev/char.c b/chardev/char.c
>> index 87237568df..cd17fac123 100644
>> --- a/chardev/char.c
>> +++ b/chardev/char.c
>> @@ -106,9 +106,8 @@ static void qemu_chr_write_log(Chardev *s, const uint8_t *buf, size_t len)
>>       }
>>   }
>> -static int qemu_chr_write_buffer(Chardev *s,
>> -                                 const uint8_t *buf, int len,
>> -                                 int *offset, bool write_all)
>> +static int qemu_chr_write_buffer(Chardev *s, const uint8_t *buf, int len,
>> +                                 int *offset, bool write_all, bool best_effort)
>>   {
>>       ChardevClass *cc = CHARDEV_GET_CLASS(s);
>>       int res = 0;
>> @@ -119,7 +118,14 @@ static int qemu_chr_write_buffer(Chardev *s,
>>       retry:
>>           res = cc->chr_write(s, buf + *offset, len - *offset);
>>           if (res < 0 && errno == EAGAIN && write_all) {
>> +            if (best_effort && s->retries > 50) {
>> +                break;
>> +            }
>> +
>>               g_usleep(100);
>> +            if (best_effort) {
>> +                s->retries++;
>> +            }
>>               goto retry;
>>           }
>> @@ -127,6 +133,7 @@ static int qemu_chr_write_buffer(Chardev *s,
>>               break;
>>           }
>> +        s->retries = 0;
>>           *offset += res;
>>           if (!write_all) {
>>               break;
>> @@ -140,7 +147,8 @@ static int qemu_chr_write_buffer(Chardev *s,
>>       return res;
>>   }
>> -int qemu_chr_write(Chardev *s, const uint8_t *buf, int len, bool write_all)
>> +int qemu_chr_write(Chardev *s, const uint8_t *buf, int len,
>> +                   bool write_all, bool best_effort)
>>   {
>>       int offset = 0;
>>       int res;
>> @@ -148,11 +156,11 @@ int qemu_chr_write(Chardev *s, const uint8_t *buf, int len, bool write_all)
>>       if (qemu_chr_replay(s) && replay_mode == REPLAY_MODE_PLAY) {
>>           replay_char_write_event_load(&res, &offset);
>>           assert(offset <= len);
>> -        qemu_chr_write_buffer(s, buf, offset, &offset, true);
>> +        qemu_chr_write_buffer(s, buf, offset, &offset, true, false);
>>           return res;
>>       }
>> -    res = qemu_chr_write_buffer(s, buf, len, &offset, write_all);
>> +    res = qemu_chr_write_buffer(s, buf, len, &offset, write_all, best_effort);
>>       if (qemu_chr_replay(s) && replay_mode == REPLAY_MODE_RECORD) {
>>           replay_char_write_event_save(res, offset);
>> diff --git a/hw/char/pl011.c b/hw/char/pl011.c
>> index 13e784f9d9..348188f49e 100644
>> --- a/hw/char/pl011.c
>> +++ b/hw/char/pl011.c
>> @@ -179,11 +179,8 @@ static void pl011_write(void *opaque, hwaddr offset,
>>       switch (offset >> 2) {
>>       case 0: /* UARTDR */
>> -        /* ??? Check if transmitter is enabled.  */
>>           ch = value;
>> -        /* XXX this blocks entire thread. Rewrite to use
>> -         * qemu_chr_fe_write and background I/O callbacks */
>> -        qemu_chr_fe_write_all(&s->chr, &ch, 1);
>> +        qemu_chr_fe_try_write_all(&s->chr, &ch, 1);
>>           s->int_level |= PL011_INT_TX;
>>           pl011_update(s);
>>           break;
>> diff --git a/include/chardev/char-fe.h b/include/chardev/char-fe.h
>> index a553843364..18281ccfca 100644
>> --- a/include/chardev/char-fe.h
>> +++ b/include/chardev/char-fe.h
>> @@ -220,6 +220,20 @@ int qemu_chr_fe_write(CharBackend *be, const uint8_t *buf, int len);
>>    */
>>   int qemu_chr_fe_write_all(CharBackend *be, const uint8_t *buf, int len);
>> +/**
>> + * qemu_chr_fe_try_write_all:
>> + * @buf: the data
>> + * @len: the number of bytes to send
>> + *
>> + * Write data to a character backend from the front end.  This function will
>> + * send data from the front end to the back end. It provides function as to
>> + * @qemu_chr_fe_write_all, except the data will be dropped after 50 attempts
>> + * of transmissions are done.
>> + *
>> + * Returns: the number of bytes consumed (0 if no associated Chardev)
>> + */
>> +int qemu_chr_fe_try_write_all(CharBackend *be, const uint8_t *buf, int len);
>> +
>>   /**
>>    * qemu_chr_fe_read_all:
>>    * @buf: the data buffer
>> diff --git a/include/chardev/char.h b/include/chardev/char.h
>> index 00589a6025..425a007a0a 100644
>> --- a/include/chardev/char.h
>> +++ b/include/chardev/char.h
>> @@ -65,6 +65,7 @@ struct Chardev {
>>       char *filename;
>>       int logfd;
>>       int be_open;
>> +    int retries;
>>       GSource *gsource;
>>       GMainContext *gcontext;
>>       DECLARE_BITMAP(features, QEMU_CHAR_FEATURE_LAST);
>> @@ -221,8 +222,9 @@ void qemu_chr_set_feature(Chardev *chr,
>>                             ChardevFeature feature);
>>   QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename,
>>                                   bool permit_mux_mon);
>> -int qemu_chr_write(Chardev *s, const uint8_t *buf, int len, bool write_all);
>> -#define qemu_chr_write_all(s, buf, len) qemu_chr_write(s, buf, len, true)
>> +int qemu_chr_write(Chardev *s, const uint8_t *buf, int len,
>> +                   bool write_all, bool best_effort);
>> +#define qemu_chr_write_all(s, buf, len) qemu_chr_write(s, buf, len, true, false)
>>   int qemu_chr_wait_connected(Chardev *chr, Error **errp);
>>   #define TYPE_CHARDEV "chardev"
>>

Thanks,
Gavin