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

[Philippe Mathieu-Daudé]

On 9/12/20 1:28 PM, Philippe Mathieu-Daudé wrote:
> On 9/12/20 11:14 AM, Paolo Bonzini wrote:
>> 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.
> 
> hw/char/serial-pci-multi.c:45
> 
> Ah indeed, not sure why I only used qdev_alias_all_properties()
> on the ISA one. Probably because I don't use the other ones.
> 
> I'll send a new patch for the PCI-single device:

Bah this can simply be squashed into the previous patch.

> 
> -- >8 --
> --- a/hw/char/serial-pci.c
> +++ b/hw/char/serial-pci.c
> @@ -81,7 +81,6 @@ static const VMStateDescription vmstate_pci_serial = {
>  };
> 
>  static Property serial_pci_properties[] = {
> -    DEFINE_PROP_CHR("chardev",  PCISerialState, state.chr),
>      DEFINE_PROP_UINT8("prog_if",  PCISerialState, prog_if, 0x02),
>      DEFINE_PROP_END_OF_LIST(),
>  };
> @@ -106,6 +105,8 @@ static void serial_pci_init(Object *o)
>      PCISerialState *ps = PCI_SERIAL(o);
> 
>      object_initialize_child(o, "serial", &ps->state, TYPE_SERIAL);
> +
> +    qdev_alias_all_properties(DEVICE(&ps->state), o);
>  }
> ---