Re: [Qemu-devel] [PATCH] spapr: Implement better workaround in spapr-vty device

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3780 bytes --]

On Wed, Jul 31, 2019 at 02:36:54PM +1000, Paul Mackerras wrote:
> Linux guest kernels have code which scans the string of characters
> returned from the H_GET_TERM_CHAR hypercall and removes any \0
> character which comes immediately after a \r character.  This is to
> work around a bug which was present in some ancient versions of
> PowerVM.  In order to avoid the corruption of the console byte stream
> that this introduced, commit 6c3bc244d3cb ("spapr: Implement bug in
> spapr-vty device to be compatible with PowerVM") added a workaround
> which adds a \0 character after every \r character.  Unfortunately,
> this corrupts the console byte stream for those operating systems,
> such as AIX, which don't remove the null bytes.
> 
> We can avoid triggering the Linux kernel workaround if we avoid
> returning a buffer which contains a \0 after a \r.  We can do that by
> breaking out of the loop in vty_getchars() if we are about to insert a
> \0 and the previous character in the buffer is a \r.  That means we
> return the characters up to the \r for the current H_GET_TERM_CHAR,
> and the characters starting with the \0 for the next one.
> 
> With this workaround, we don't insert any spurious characters and we
> avoid triggering the Linux kernel workaround, so the guest will
> receive an uncorrupted stream whether or not they have the workaround.
> 
> Fixes: 6c3bc244d3cb ("spapr: Implement bug in spapr-vty device to be compatible with PowerVM")
> Signed-off-by: Paul Mackerras <paulus@ozlabs.org>

Applied to ppc-for-4.2, thanks.

> ---
>  hw/char/spapr_vty.c | 30 ++++++++++++------------------
>  1 file changed, 12 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/char/spapr_vty.c b/hw/char/spapr_vty.c
> index 617303dbaf..65a7c2ffbd 100644
> --- a/hw/char/spapr_vty.c
> +++ b/hw/char/spapr_vty.c
> @@ -57,25 +57,19 @@ static int vty_getchars(SpaprVioDevice *sdev, uint8_t *buf, int max)
>      int n = 0;
>  
>      while ((n < max) && (dev->out != dev->in)) {
> -        buf[n++] = dev->buf[dev->out++ % VTERM_BUFSIZE];
> -
> -        /* PowerVM's vty implementation has a bug where it inserts a
> -         * \0 after every \r going to the guest.  Existing guests have
> -         * a workaround for this which removes every \0 immediately
> -         * following a \r, so here we make ourselves bug-for-bug
> -         * compatible, so that the guest won't drop a real \0-after-\r
> -         * that happens to occur in a binary stream. */
> -        if (buf[n - 1] == '\r') {
> -            if (n < max) {
> -                buf[n++] = '\0';
> -            } else {
> -                /* No room for the extra \0, roll back and try again
> -                 * next time */
> -                dev->out--;
> -                n--;
> -                break;
> -            }
> +        /*
> +         * Long ago, PowerVM's vty implementation had a bug where it
> +         * inserted a \0 after every \r going to the guest.  Existing
> +         * guests have a workaround for this which removes every \0
> +         * immediately following a \r.  To avoid triggering this
> +         * workaround, we stop before inserting a \0 if the preceding
> +         * character in the output buffer is a \r.
> +         */
> +        if (n > 0 && (buf[n - 1] == '\r') &&
> +                (dev->buf[dev->out % VTERM_BUFSIZE] == '\0')) {
> +            break;
>          }
> +        buf[n++] = dev->buf[dev->out++ % VTERM_BUFSIZE];
>      }
>  
>      qemu_chr_fe_accept_input(&dev->chardev);

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]