Re: [PATCH 7/7] hw/char/serial: Let SerialState have an 'id' field

[Paolo Bonzini <pbonzini@redhat.com>]

On 07/09/20 03:55, Philippe Mathieu-Daudé wrote:
> When a SoC has multiple UARTs (some configured differently),
> it is hard to associate events to their UART.
> 
> To be able to distinct trace events between various instances,
> add an 'id' field. Update the trace format accordingly.
> 
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/hw/char/serial.h | 1 +
>  hw/char/serial.c         | 7 ++++---
>  hw/char/trace-events     | 6 +++---
>  3 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h
> index 3d2a5b27e87..3ee2d096a85 100644
> --- a/include/hw/char/serial.h
> +++ b/include/hw/char/serial.h
> @@ -75,6 +75,7 @@ typedef struct SerialState {
>      uint64_t char_transmit_time;    /* time to transmit a char in ticks */
>      int poll_msl;
>  
> +    uint8_t id;
>      QEMUTimer *modem_status_poll;
>      MemoryRegion io;
>  } SerialState;
> diff --git a/hw/char/serial.c b/hw/char/serial.c
> index ade89fadb44..e5a6b939f13 100644
> --- a/hw/char/serial.c
> +++ b/hw/char/serial.c
> @@ -177,7 +177,7 @@ static void serial_update_parameters(SerialState *s)
>      ssp.stop_bits = stop_bits;
>      s->char_transmit_time =  (NANOSECONDS_PER_SECOND / speed) * frame_size;
>      qemu_chr_fe_ioctl(&s->chr, CHR_IOCTL_SERIAL_SET_PARAMS, &ssp);
> -    trace_serial_update_parameters(speed, parity, data_bits, stop_bits);
> +    trace_serial_update_parameters(s->id, speed, parity, data_bits, stop_bits);
>  }
>  
>  static void serial_update_msl(SerialState *s)
> @@ -333,7 +333,7 @@ static void serial_ioport_write(void *opaque, hwaddr addr, uint64_t val,
>      SerialState *s = opaque;
>  
>      assert(size == 1 && addr < 8);
> -    trace_serial_write(addr, val);
> +    trace_serial_write(s->id, addr, val);
>      switch(addr) {
>      default:
>      case 0:
> @@ -550,7 +550,7 @@ static uint64_t serial_ioport_read(void *opaque, hwaddr addr, unsigned size)
>          ret = s->scr;
>          break;
>      }
> -    trace_serial_read(addr, ret);
> +    trace_serial_read(s->id, addr, ret);
>      return ret;
>  }
>  
> @@ -1013,6 +1013,7 @@ static const TypeInfo serial_io_info = {
>  };
>  
>  static Property serial_properties[] = {
> +    DEFINE_PROP_UINT8("id", SerialState, id, 0),
>      DEFINE_PROP_CHR("chardev", SerialState, chr),
>      DEFINE_PROP_UINT32("baudbase", SerialState, baudbase, 115200),
>      DEFINE_PROP_BOOL("wakeup", SerialState, wakeup, false),
> diff --git a/hw/char/trace-events b/hw/char/trace-events
> index cd36b63f39d..40800c9334c 100644
> --- a/hw/char/trace-events
> +++ b/hw/char/trace-events
> @@ -5,9 +5,9 @@ parallel_ioport_read(const char *desc, uint16_t addr, uint8_t value) "read [%s]
>  parallel_ioport_write(const char *desc, uint16_t addr, uint8_t value) "write [%s] addr 0x%02x val 0x%02x"
>  
>  # serial.c
> -serial_read(uint16_t addr, uint8_t value) "read addr 0x%02x val 0x%02x"
> -serial_write(uint16_t addr, uint8_t value) "write addr 0x%02x val 0x%02x"
> -serial_update_parameters(uint64_t baudrate, char parity, int data_bits, int stop_bits) "baudrate=%"PRIu64" parity='%c' data=%d stop=%d"
> +serial_read(uint8_t id, uint8_t addr, uint8_t value) "id#%u read addr 0x%x val 0x%02x"
> +serial_write(uint8_t id, uint8_t addr, uint8_t value) "id#%u write addr 0x%x val 0x%02x"
> +serial_update_parameters(uint8_t id, uint64_t baudrate, char parity, int data_bits, int stop_bits) "id#%u baudrate=%"PRIu64" parity=%c data=%d stop=%d"
>  
>  # virtio-serial-bus.c
>  virtio_serial_send_control_event(unsigned int port, uint16_t event, uint16_t value) "port %u, event %u, value %u"
> 

I'm not sure about making this one a one-off for serial.c.  You could
add the SerialState* too, for example.  I have queued the other six though.

Paolo