spapr: Implement bug in spapr-vty device to be compatible with PowerVM

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

Posted by David Gibson 6 years, 4 months ago

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.

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 <david@gibson.dropbear.id.au>
---
 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);
-- 
2.14.3

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

Posted by no-reply@patchew.org 6 years, 4 months ago

Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [Qemu-devel] [PATCH] spapr: Implement bug in spapr-vty device to be compatible with PowerVM
Type: series
Message-id: 20171120071423.11693-1-david@gibson.dropbear.id.au

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]               patchew/20171120071423.11693-1-david@gibson.dropbear.id.au -> patchew/20171120071423.11693-1-david@gibson.dropbear.id.au
Switched to a new branch 'test'
08e9d8fe82 spapr: Implement bug in spapr-vty device to be compatible with PowerVM

=== OUTPUT BEGIN ===
Checking PATCH 1/1: spapr: Implement bug in spapr-vty device to be compatible with PowerVM...
ERROR: spaces required around that '-' (ctx:VxV)
#40: FILE: hw/char/spapr_vty.c:68:
+        if (buf[n-1] == '\r') {
                  ^

total: 1 errors, 0 warnings, 24 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

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

Posted by David Gibson 6 years, 4 months ago

On Sun, Nov 19, 2017 at 11:18:55PM -0800, no-reply@patchew.org wrote:
> Hi,
> 
> This series seems to have some coding style problems. See output below for
> more information:
> 
> Subject: [Qemu-devel] [PATCH] spapr: Implement bug in spapr-vty device to be compatible with PowerVM
> Type: series
> Message-id: 20171120071423.11693-1-david@gibson.dropbear.id.au
> 
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> 
> BASE=base
> n=1
> total=$(git log --oneline $BASE.. | wc -l)
> failed=0
> 
> git config --local diff.renamelimit 0
> git config --local diff.renames True
> 
> commits="$(git log --format=%H --reverse $BASE..)"
> for c in $commits; do
>     echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
>     if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
>         failed=1
>         echo
>     fi
>     n=$((n+1))
> done
> 
> exit $failed
> === TEST SCRIPT END ===
> 
> Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
> From https://github.com/patchew-project/qemu
>  * [new tag]               patchew/20171120071423.11693-1-david@gibson.dropbear.id.au -> patchew/20171120071423.11693-1-david@gibson.dropbear.id.au
> Switched to a new branch 'test'
> 08e9d8fe82 spapr: Implement bug in spapr-vty device to be compatible with PowerVM
> 
> === OUTPUT BEGIN ===
> Checking PATCH 1/1: spapr: Implement bug in spapr-vty device to be compatible with PowerVM...
> ERROR: spaces required around that '-' (ctx:VxV)
> #40: FILE: hw/char/spapr_vty.c:68:
> +        if (buf[n-1] == '\r') {

I've corrected this in my tree.

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

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

Posted by Thomas Huth 6 years, 4 months ago

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 <david@gibson.dropbear.id.au>
> ---
>  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 <thuth@redhat.com>

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

Posted by Greg Kurz 6 years, 4 months ago

On Tue, 21 Nov 2017 08:27:51 +0100
Thomas Huth <thuth@redhat.com> wrote:

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

The bug has been around forever (already there in initial linux git
commit 1da177e4c3f4), so I'm not sure PowerVM will ever fix it. It
is legacy API now :) 

Also I'm not sure it is worth the pain to introduce extra complexity
in the existing linux driver... I guess we'd rather encourage people
to switch to virtio serial instead.

Anyway,

Reviewed-by: Greg Kurz <groug@kaod.org>

> > 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 <david@gibson.dropbear.id.au>
> > ---
> >  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 <thuth@redhat.com>
>