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.hender...@linaro.org> >> Signed-off-by: Philippe Mathieu-Daudé <f4...@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 > >