Re: [Qemu-devel] [Qemu-ppc] [PATCH] spapr: Implement bug in spapr-vty device to be compatible with PowerVM

[Thomas Huth]

On 20.11.2017 08:14, David Gibson wrote:
> The spapr-vty device implements the PAPR defined virtual console,
> which is also implemented by IBM's proprietary PowerVM hypervisor.
> 
> PowerVM's implementation has a bug where it inserts an extra \0 after
> every \r going to the guest.  Because of that Linux's guest side
> driver has a workaround which strips \0 characters that appear
> immediately after a \r.

Ouch.

I wonder whether it would make more sense to change the guest side
driver to apply the workaround only if it's really running under
PowerVM...? E.g. what if the PowerVM bug ever gets fixed one day?

> That means that when running under qemu, sending a binary stream from
> host to guest via spapr-vty which happens to include a \r\0 sequence
> will get corrupted by that workaround.
> 
> To deal with that, this patch duplicates PowerVM's bug, inserting an
> extra \0 after each \r.  Ugly, but the best option available.
> 
> Signed-off-by: David Gibson <address@hidden>
> ---
>  hw/char/spapr_vty.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/hw/char/spapr_vty.c b/hw/char/spapr_vty.c
> index 0fa416ca6b..a95e5e91a7 100644
> --- a/hw/char/spapr_vty.c
> +++ b/hw/char/spapr_vty.c
> @@ -58,6 +58,24 @@ static int vty_getchars(VIOsPAPRDevice *sdev, uint8_t 
> *buf, int max)
>  
>      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;
> +            }
> +        }
>      }
>  
>      qemu_chr_fe_accept_input(&dev->chardev);
> 

Code looks fine to me, so:

Reviewed-by: Thomas Huth <address@hidden>