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"
--
2.26.2
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
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
>
>
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);
> }
> ---
© 2016 - 2025 Red Hat, Inc.