Re: [Qemu-devel] [RFC 0/3] qemu-char: Add poll timeouts for character backends

[Heinz Graalfs <graalfs@linux.vnet.ibm.com>]

On 24/10/14 11:15, Paolo Bonzini wrote:
> On 10/24/2014 10:13 AM, Heinz Graalfs wrote:
>> On s390 one can observe system hang situations wrt console input when
>> using 'dataplane=on'.
>>
>> dataplane processing causes an inactive main thread and an active
>> dataplane thread.
>>
>> When a character backend descriptor disappears from the main thread's
>> poll() descriptor array (when can_read() returns 0) it happens that it
>> will never reappear in the poll() array due to missing poll() interrupts.
>>
>> The following patches fix observed hangs on s390 and provide a means
>> to avoid potential hangs in other backends/frontends.
> 
> I think all you need is a simple
> 
>     qemu_notify_event();
> 
> call when can_read can go from 0 to 1, for example just before
> get_console_data returns (for hw/char/sclpconsole-lm.c).
> 

definitely, just that simple!

> By the way, for hw/char/sclpconsole-lm.c I'm not sure what happens if
> scon->length == SIZE_CONSOLE_BUFFER.  You cannot read, so you cannot
> generate an event, and you cannot reset scon->length because you cannot
> generate an event.  I think something like this is needed:
> 
> diff --git a/hw/char/sclpconsole-lm.c b/hw/char/sclpconsole-lm.c
> index 80dd0a9..c61b77b 100644
> --- a/hw/char/sclpconsole-lm.c
> +++ b/hw/char/sclpconsole-lm.c
> @@ -61,10 +61,9 @@ static int chr_can_read(void *opaque)
> 
>      if (scon->event.event_pending) {
>          return 0;
> -    } else if (SIZE_CONSOLE_BUFFER - scon->length) {
> +    } else {
>          return 1;
>      }
> -    return 0;
>  }
> 
>  static void chr_read(void *opaque, const uint8_t *buf, int size)
> @@ -78,6 +77,10 @@ static void chr_read(void *opaque, const uint8_t
> *buf, int size)
>          sclp_service_interrupt(0);
>          return;
>      }
> +    if (scon->length == SIZE_CONSOLE_BUFFER) {
> +        /* Eat the character, but still process CR and LF.  */
> +        return;
> +    }

yes, thanks a lot

>      scon->buf[scon->length] = *buf;
>      scon->length += 1;
>      if (scon->echo) {
> 
> Paolo
> 
>