Re: [Qemu-devel] [PATCH] hw/char/serial: Only retry if qemu_chr_fe_write returns 0

[Igor Mammedov <imammedo@redhat.com>]

On Fri, 13 Jul 2018 16:55:15 +0200
Marc-André Lureau <marcandre.lureau@gmail.com> wrote:

> Hi
> 
> On Tue, Jul 10, 2018 at 3:48 PM, Igor Mammedov <imammedo@redhat.com> wrote:
> > On Tue, 5 Jun 2018 11:18:35 +0200
> > Paolo Bonzini <pbonzini@redhat.com> wrote:
> >  
> >> On 05/06/2018 09:54, Sergio Lopez wrote:  
> >> > Only retry on serial_xmit if qemu_chr_fe_write returns 0, as this is the
> >> > only recoverable error.
> >> >
> >> > Retrying with any other scenario, in addition to being a waste of CPU
> >> > cycles, can compromise the Guest stability if by the vCPU issuing the
> >> > write and the main loop thread are, by chance or explicit pinning,
> >> > running on the same pCPU.
> >> >
> >> > Previous discussion:
> >> >
> >> > https://lists.nongnu.org/archive/html/qemu-devel/2018-05/msg06998.html
> >> >
> >> > Signed-off-by: Sergio Lopez <slp@redhat.com>
> >> > ---
> >> >  hw/char/serial.c | 2 +-
> >> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >> >
> >> > diff --git a/hw/char/serial.c b/hw/char/serial.c
> >> > index 605b0d0..6de6c29 100644
> >> > --- a/hw/char/serial.c
> >> > +++ b/hw/char/serial.c
> >> > @@ -260,7 +260,7 @@ static void serial_xmit(SerialState *s)
> >> >          if (s->mcr & UART_MCR_LOOP) {
> >> >              /* in loopback mode, say that we just received a char */
> >> >              serial_receive1(s, &s->tsr, 1);
> >> > -        } else if (qemu_chr_fe_write(&s->chr, &s->tsr, 1) != 1 &&
> >> > +        } else if (qemu_chr_fe_write(&s->chr, &s->tsr, 1) == 0 &&
> >> >                     s->tsr_retry < MAX_XMIT_RETRY) {
> >> >              assert(s->watch_tag == 0);
> >> >              s->watch_tag =
> >> >  
> > Hi Sergio, Paolo,
> >
> > it looks like commit
> > commit 019288bf137183bf3407c9824655b753bfafc99f
> > Author: Sergio Lopez <slp@redhat.com>
> > Date:   Tue Jun 5 03:54:55 2018 -0400
> >
> >     hw/char/serial: Only retry if qemu_chr_fe_write returns 0
> >
> > introduced regression wrt 2.12 and broke windows guest remote kernel debug over
> > serial, where windbg can't connect to target and target hangs when windbg tries
> > to connect to it.
> >
> > Reverting the commit fixes issue
> >  
> 
> is this a possible solution?
it fixes windbg over serial (with and without EINTR)


> diff --git a/hw/char/serial.c b/hw/char/serial.c
> index cd7d747c68..046c4684ff 100644
> --- a/hw/char/serial.c
> +++ b/hw/char/serial.c
> @@ -261,15 +261,20 @@ static void serial_xmit(SerialState *s)
>          if (s->mcr & UART_MCR_LOOP) {
>              /* in loopback mode, say that we just received a char */
>              serial_receive1(s, &s->tsr, 1);
> -        } else if (qemu_chr_fe_write(&s->chr, &s->tsr, 1) == 0 &&
> -                   s->tsr_retry < MAX_XMIT_RETRY) {
> -            assert(s->watch_tag == 0);
> -            s->watch_tag =
> -                qemu_chr_fe_add_watch(&s->chr, G_IO_OUT | G_IO_HUP,
> -                                      serial_watch_cb, s);
> -            if (s->watch_tag > 0) {
> -                s->tsr_retry++;
> -                return;
> +        } else {
> +            int rc = qemu_chr_fe_write(&s->chr, &s->tsr, 1);
> +
> +            if ((rc == 0 ||
> +                 (rc == -1 && (errno == EAGAIN || errno == EINTR))) &&
> +                s->tsr_retry < MAX_XMIT_RETRY) {
> +                assert(s->watch_tag == 0);
> +                s->watch_tag =
> +                    qemu_chr_fe_add_watch(&s->chr, G_IO_OUT | G_IO_HUP,
> +                                          serial_watch_cb, s);
> +                if (s->watch_tag > 0) {
> +                    s->tsr_retry++;
> +                    return;
> +                }
>              }
>          }
>          s->tsr_retry = 0;
> 
>