Re: [Qemu-devel] [PATCH] upgrading emulated UART to 16550A

[Anthony Liguori <anthony@codemonkey.ws>]

Stefano Stabellini wrote:
> This is an improved version of the UART 16550A emulation patch.
> The changes compared to previous version are:
>
> - no token bucket anymore;
>
> - fixed a small bug handling IRQs; this was the problem that prevented
> kgdb to work over the serial (thanks to Jason Wessel for the help
> spotting and reproducing this bug).
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>
> ---
>
> diff --git a/hw/serial.c b/hw/serial.c
> index ffd6ac9..5aab00e 100644
> --- a/hw/serial.c
> +++ b/hw/serial.c
> @@ -1,7 +1,7 @@
>  /*
> - * QEMU 16450 UART emulation
> + * QEMU 16550A UART emulation
>   *
> - * Copyright (c) 2003-2004 Fabrice Bellard
> + * Copyright (c) 2003-2008 Fabrice Bellard
>   

I think the idea was to add a new copyright entry, not to modify 
Fabrice's copyright.

>   *
>   * Permission is hereby granted, free of charge, to any person obtaining a copy
>   * of this software and associated documentation files (the "Software"), to deal
> @@ -21,6 +21,10 @@
>   * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>   * THE SOFTWARE.
>   */
> +
> +#include <termios.h>
> +#include <sys/ioctl.h>
>   

This doesn't look Windows friendly.

> @@ -100,55 +131,105 @@ struct SerialState {
>      target_phys_addr_t base;
>      int it_shift;
>      int baudbase;
> -    QEMUTimer *tx_timer;
> -    int tx_burst;
> +    int tsr_retry;
> +
> +    uint64_t last_xmit_ts;              /* Time when the last byte was successfully sent out of the tsr */
> +    SerialFIFO recv_fifo;
> +    SerialFIFO xmit_fifo;
> +
> +    struct QEMUTimer *fifo_timeout_timer;
> +    int timeout_ipending;                   /* timeout interrupt pending state */
> +    struct QEMUTimer *transmit_timer;
> +
> +
> +    uint64_t char_transmit_time;               /* time to transmit a char in ticks*/
> +    int poll_msl;
> +
> +    struct QEMUTimer *modem_status_poll;
>  };
>
> +static void serial_receive1(void *opaque, const uint8_t *buf, int size);
> +
> +static void fifo_clear(SerialState *s, int fifo) {
>   

minor nit, but the '{' should be on a new line.

> +    SerialFIFO *f = ( fifo ) ? &s->recv_fifo : &s->xmit_fifo;
> +    memset(f->data, 0, UART_FIFO_LENGTH);
> +    f->count = 0;
> +    f->head = 0;
> +    f->tail = 0;
> +}
> +
> +static int fifo_put(SerialState *s, int fifo, uint8_t chr) {
> +    SerialFIFO *f = ( fifo ) ? &s->recv_fifo : &s->xmit_fifo;
> +
> +    f->data[f->head++] = chr;
> +
> +    if (f->head == UART_FIFO_LENGTH)
> +        f->head = 0;
> +    f->count++;
> +
> +    return 1;
> +}
> +
> +uint8_t fifo_get(SerialState *s, int fifo) {
>   

Any reason for this not to be static?

> -static void serial_tx_done(void *opaque)
> -{
> -    SerialState *s = opaque;
>
> -    if (s->tx_burst < 0) {
> -        uint16_t divider;
> +    if ( ( s->ier & UART_IER_RLSI ) && (s->lsr & UART_LSR_INT_ANY ) ) {
> +        tmp_iir = UART_IIR_RLSI;
> +    } else if ( s->timeout_ipending ) {
> +        tmp_iir = UART_IIR_CTI;
> +    } else if ( ( s->ier & UART_IER_RDI ) && (s->lsr & UART_LSR_DR ) ) {
> +        if ( !(s->fcr & UART_FCR_FE) ) {
> +           tmp_iir = UART_IIR_RDI;
> +        } else if ( s->recv_fifo.count >= s->recv_fifo.itl ) {
> +           tmp_iir = UART_IIR_RDI;
> +        }
> +    } else if ( (s->ier & UART_IER_THRI) && s->thr_ipending ) {
> +        tmp_iir = UART_IIR_THRI;
> +    } else if ( (s->ier & UART_IER_MSI) && (s->msr & UART_MSR_ANY_DELTA) ) {
> +        tmp_iir = UART_IIR_MSI;
> +    }
>
> -        if (s->divider)
> -          divider = s->divider;
> -        else
> -          divider = 1;
> +    s->iir = tmp_iir | ( s->iir & 0xF0 );
>
> -        /* We assume 10 bits/char, OK for this purpose. */
> -        s->tx_burst = THROTTLE_TX_INTERVAL * 1000 /
> -            (1000000 * 10 / (s->baudbase / divider));
> +    if ( tmp_iir != UART_IIR_NO_INT ) {
> +        qemu_irq_raise(s->irq);
> +    } else {
> +        qemu_irq_lower(s->irq);
>   

More nits, lose the whitespace in the conditions.  For instance:

} else if ( ( s->ier & UART_IER_RDI )  =>  } else if ((s->ier & 
UART_IER_RDI).

The rest of the file uses the later style so it's a little weird to have 
portions of the code be different.

>      }
> -    s->thr_ipending = 1;
> -    s->lsr |= UART_LSR_THRE;
> -    s->lsr |= UART_LSR_TEMT;
> -    serial_update_irq(s);
>  }
>
>  static void serial_update_parameters(SerialState *s)
>  {
> -    int speed, parity, data_bits, stop_bits;
> +    int speed, parity, data_bits, stop_bits, frame_size;
>      QEMUSerialSetParams ssp;
>
> +    if (s->divider == 0)
> +        return;
> +
> +    frame_size = 1;
>      if (s->lcr & 0x08) {
>          if (s->lcr & 0x10)
>              parity = 'E';
> @@ -156,19 +237,21 @@ static void serial_update_parameters(SerialState *s)
>              parity = 'O';
>      } else {
>              parity = 'N';
> +            frame_size = 0;
>      }
>      if (s->lcr & 0x04)
>          stop_bits = 2;
>      else
>          stop_bits = 1;
> +
>      data_bits = (s->lcr & 0x03) + 5;
> -    if (s->divider == 0)
> -        return;
> +    frame_size += data_bits + stop_bits;
>      speed = s->baudbase / s->divider;
>      ssp.speed = speed;
>      ssp.parity = parity;
>      ssp.data_bits = data_bits;
>      ssp.stop_bits = stop_bits;
> +    s->char_transmit_time =  ( ticks_per_sec / speed ) * frame_size;
>      qemu_chr_ioctl(s->chr, CHR_IOCTL_SERIAL_SET_PARAMS, &ssp);
>  #if 0
>      printf("speed=%d parity=%c data=%d stop=%d\n",
> @@ -176,6 +259,88 @@ static void serial_update_parameters(SerialState *s)
>  #endif
>  }
>
> +static void serial_update_msl( SerialState *s )
>   

More bad whitespace.

> +static void serial_xmit(void *opaque) {
> +    SerialState *s = opaque;
> +    uint64_t new_xmit_ts = qemu_get_clock(vm_clock);
> +
> +    if ( s->tsr_retry <= 0 ) {
> +        if (s->fcr & UART_FCR_FE) {
> +            s->tsr = fifo_get(s,XMIT_FIFO);
> +            if ( !s->xmit_fifo.count )
> +                s->lsr |= UART_LSR_THRE;
> +        } else {
> +            s->tsr = s->thr;
> +            s->lsr |= UART_LSR_THRE;
> +        }
> +    }
> +
> +    if ( (s->mcr & UART_MCR_LOOP
> +	  /* in loopback mode, say that we just received a char */
> +	  ? (serial_receive1(s, &s->tsr, 1), 1)
> +	  : qemu_chr_write(s->chr, &s->tsr, 1))
>   

This is just nutty :-)  Please rewrite this if() statement to be a 
little less obscure.

> @@ -524,18 +832,11 @@ SerialState *serial_mm_init (target_phys_addr_t base, int it_shift,
>      s = qemu_mallocz(sizeof(SerialState));
>      if (!s)
>          return NULL;
> -    s->irq = irq;
> +
>      s->base = base;
>      s->it_shift = it_shift;
> -    s->baudbase= baudbase;
> -
> -    s->tx_timer = qemu_new_timer(vm_clock, serial_tx_done, s);
> -    if (!s->tx_timer)
> -        return NULL;
> -
> -    qemu_register_reset(serial_reset, s);
> -    serial_reset(s);
>
> +    serial_init_core(s, irq, baudbase, chr);
>      register_savevm("serial", base, 2, serial_save, serial_load, s);
>   

Isn't it necessary to bump the savevm() version number since you've 
changed the format.

Regards,

Anthony Liguori