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