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

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

Hi Gavin,

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)


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 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"
>