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

[Philippe Mathieu-Daudé]

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:

-- >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);
 }
---

And amend to this one:

-- >8 --
--- a/hw/char/serial-pci-multi.c
+++ b/hw/char/serial-pci-multi.c
@@ -105,6 +105,7 @@ static void multi_serial_pci_realize(PCIDevice *dev,
Error **errp)

     for (i = 0; i < nports; i++) {
         s = pci->state + i;
+        qdev_prop_set_uint8(s, "id", i);
         if (!qdev_realize(DEVICE(s), NULL, errp)) {
             multi_serial_pci_exit(dev);
             return;
---

Then we are done.

>  I have queued the other six though.

Thanks!

> 
> Paolo
> 
>