[PATCH 0/7] hw/char/serial: Housekeeping

[PATCH 0/7] hw/char/serial: Housekeeping

[Philippe Mathieu-Daudé]

Nothing very exciting, cleanups before more serious changes.

Philippe Mathieu-Daudé (7):
  hw/char/serial: Assert serial_ioport_read/write offset fits 8 bytes
  hw/char/serial: Replace commented DPRINTF() by trace event
  hw/char/serial: Remove old DEBUG_SERIAL commented code
  hw/char/serial: Rename I/O read/write trace events
  hw/char/serial: Make 'wakeup' property boolean
  hw/char/serial-isa: Alias QDEV properties from generic serial object
  hw/char/serial: Let SerialState have an 'id' field

 include/hw/char/serial.h |  3 ++-
 hw/char/serial-isa.c     |  4 ++--
 hw/char/serial.c         | 25 +++++++------------------
 hw/char/trace-events     |  5 +++--
 4 files changed, 14 insertions(+), 23 deletions(-)

-- 
2.26.2

[PATCH 1/7] hw/char/serial: Assert serial_ioport_read/write offset fits 8 bytes

[Philippe Mathieu-Daudé]

The serial device has 8 registers, each 8-bit. The MemoryRegionOps
'serial_io_ops' is initialized with max_access_size=1, and all
memory_region_init_io() callers correctly set the region size to
8 bytes:
- serial_io_realize
- serial_isa_realizefn
- serial_pci_realize
- multi_serial_pci_realize

It is safe to assert the offset argument of serial_ioport_read()
and serial_ioport_write() is always less than 8.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/char/serial.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/char/serial.c b/hw/char/serial.c
index 23864794929..a855ef66ea2 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -344,7 +344,7 @@ static void serial_ioport_write(void *opaque, hwaddr addr, uint64_t val,
 {
     SerialState *s = opaque;
 
-    addr &= 7;
+    assert(size == 1 && addr < 8);
     trace_serial_ioport_write(addr, val);
     switch(addr) {
     default:
@@ -485,7 +485,7 @@ static uint64_t serial_ioport_read(void *opaque, hwaddr addr, unsigned size)
     SerialState *s = opaque;
     uint32_t ret;
 
-    addr &= 7;
+    assert(size == 1 && addr < 8);
     switch(addr) {
     default:
     case 0:
-- 
2.26.2

Re: [PATCH 1/7] hw/char/serial: Assert serial_ioport_read/write offset fits 8 bytes

[Richard Henderson]

On 9/6/20 6:55 PM, Philippe Mathieu-Daudé wrote:
> The serial device has 8 registers, each 8-bit. The MemoryRegionOps
> 'serial_io_ops' is initialized with max_access_size=1, and all
> memory_region_init_io() callers correctly set the region size to
> 8 bytes:
> - serial_io_realize
> - serial_isa_realizefn
> - serial_pci_realize
> - multi_serial_pci_realize
> 
> It is safe to assert the offset argument of serial_ioport_read()
> and serial_ioport_write() is always less than 8.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/char/serial.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[PATCH 2/7] hw/char/serial: Replace commented DPRINTF() by trace event

[Philippe Mathieu-Daudé]

Convert the old debug PRINTF() call to display the UART
baudrate to a trace event.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/char/serial.c     | 4 +---
 hw/char/trace-events | 1 +
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/char/serial.c b/hw/char/serial.c
index a855ef66ea2..fb41337b661 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -187,9 +187,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);
-
-    DPRINTF("speed=%.2f parity=%c data=%d stop=%d\n",
-           speed, parity, data_bits, stop_bits);
+    trace_serial_update_parameters(speed, parity, data_bits, stop_bits);
 }
 
 static void serial_update_msl(SerialState *s)
diff --git a/hw/char/trace-events b/hw/char/trace-events
index d20eafd56f8..85e39d9d62b 100644
--- a/hw/char/trace-events
+++ b/hw/char/trace-events
@@ -7,6 +7,7 @@ parallel_ioport_write(const char *desc, uint16_t addr, uint8_t value) "write [%s
 # serial.c
 serial_ioport_read(uint16_t addr, uint8_t value) "read addr 0x%02x val 0x%02x"
 serial_ioport_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"
 
 # 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

[PATCH 3/7] hw/char/serial: Remove old DEBUG_SERIAL commented code

[Philippe Mathieu-Daudé]

All useful DPRINTF() calls have been converted to trace
events.  Remove a pointless one in the IOEventHandler,
and drop the DEBUG_SERIAL ifdef'ry.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/char/serial.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/hw/char/serial.c b/hw/char/serial.c
index fb41337b661..1e70294f28a 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -36,8 +36,6 @@
 #include "trace.h"
 #include "hw/qdev-properties.h"
 
-//#define DEBUG_SERIAL
-
 #define UART_LCR_DLAB	0x80	/* Divisor latch access bit */
 
 #define UART_IER_MSI	0x08	/* Enable Modem status interrupt */
@@ -102,14 +100,6 @@
 
 #define MAX_XMIT_RETRY      4
 
-#ifdef DEBUG_SERIAL
-#define DPRINTF(fmt, ...) \
-do { fprintf(stderr, "serial: " fmt , ## __VA_ARGS__); } while (0)
-#else
-#define DPRINTF(fmt, ...) \
-do {} while (0)
-#endif
-
 static void serial_receive1(void *opaque, const uint8_t *buf, int size);
 static void serial_xmit(SerialState *s);
 
@@ -636,7 +626,6 @@ static void serial_receive1(void *opaque, const uint8_t *buf, int size)
 static void serial_event(void *opaque, QEMUChrEvent event)
 {
     SerialState *s = opaque;
-    DPRINTF("event %x\n", event);
     if (event == CHR_EVENT_BREAK)
         serial_receive_break(s);
 }
-- 
2.26.2

[PATCH 4/7] hw/char/serial: Rename I/O read/write trace events

[Philippe Mathieu-Daudé]

The serial_mm_read/write() handlers from the TYPE_SERIAL_MM device
call the serial_ioport_read/write() handlers with shifted offset.

When looking at the trace events from this MMIO device, it is
confusing to read the accesses as I/O. Simplify using generic
trace event names which make sense the various uses.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/char/serial.c     | 4 ++--
 hw/char/trace-events | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/char/serial.c b/hw/char/serial.c
index 1e70294f28a..ade4adfd526 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -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_ioport_write(addr, val);
+    trace_serial_write(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_ioport_read(addr, ret);
+    trace_serial_read(addr, ret);
     return ret;
 }
 
diff --git a/hw/char/trace-events b/hw/char/trace-events
index 85e39d9d62b..cd36b63f39d 100644
--- a/hw/char/trace-events
+++ b/hw/char/trace-events
@@ -5,8 +5,8 @@ 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_ioport_read(uint16_t addr, uint8_t value) "read addr 0x%02x val 0x%02x"
-serial_ioport_write(uint16_t addr, uint8_t value) "write addr 0x%02x val 0x%02x"
+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"
 
 # virtio-serial-bus.c
-- 
2.26.2

Re: [PATCH 4/7] hw/char/serial: Rename I/O read/write trace events

[Richard Henderson]

On 9/6/20 6:55 PM, Philippe Mathieu-Daudé wrote:
> The serial_mm_read/write() handlers from the TYPE_SERIAL_MM device
> call the serial_ioport_read/write() handlers with shifted offset.
> 
> When looking at the trace events from this MMIO device, it is
> confusing to read the accesses as I/O. Simplify using generic
> trace event names which make sense the various uses.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/char/serial.c     | 4 ++--
>  hw/char/trace-events | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[PATCH 5/7] hw/char/serial: Make 'wakeup' property boolean

[Philippe Mathieu-Daudé]

Make the "wakeup" property introduced in commit 9826fd597df
("suspend: make serial ports wakeup the guest") a boolean.

As we want to reuse the generic serial properties in the
ISA model (next commit), expose this property.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/char/serial.h | 2 +-
 hw/char/serial-isa.c     | 2 +-
 hw/char/serial.c         | 1 +
 3 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h
index 535fa23a2b8..3d2a5b27e87 100644
--- a/include/hw/char/serial.h
+++ b/include/hw/char/serial.h
@@ -60,7 +60,7 @@ typedef struct SerialState {
     uint32_t baudbase;
     uint32_t tsr_retry;
     guint watch_tag;
-    uint32_t wakeup;
+    bool wakeup;
 
     /* Time when the last byte was successfully sent out of the tsr */
     uint64_t last_xmit_ts;
diff --git a/hw/char/serial-isa.c b/hw/char/serial-isa.c
index b4c65949cd8..a0c338796d5 100644
--- a/hw/char/serial-isa.c
+++ b/hw/char/serial-isa.c
@@ -116,7 +116,7 @@ static Property serial_isa_properties[] = {
     DEFINE_PROP_UINT32("iobase",  ISASerialState, iobase,  -1),
     DEFINE_PROP_UINT32("irq",    ISASerialState, isairq,  -1),
     DEFINE_PROP_CHR("chardev",   ISASerialState, state.chr),
-    DEFINE_PROP_UINT32("wakeup", ISASerialState, state.wakeup, 0),
+    DEFINE_PROP_BOOL("wakeup",   ISASerialState, state.wakeup, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/char/serial.c b/hw/char/serial.c
index ade4adfd526..ade89fadb44 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -1015,6 +1015,7 @@ static const TypeInfo serial_io_info = {
 static Property serial_properties[] = {
     DEFINE_PROP_CHR("chardev", SerialState, chr),
     DEFINE_PROP_UINT32("baudbase", SerialState, baudbase, 115200),
+    DEFINE_PROP_BOOL("wakeup", SerialState, wakeup, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.26.2

Re: [PATCH 5/7] hw/char/serial: Make 'wakeup' property boolean

[Richard Henderson]

On 9/6/20 6:55 PM, Philippe Mathieu-Daudé wrote:
> Make the "wakeup" property introduced in commit 9826fd597df
> ("suspend: make serial ports wakeup the guest") a boolean.
> 
> As we want to reuse the generic serial properties in the
> ISA model (next commit), expose this property.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/hw/char/serial.h | 2 +-
>  hw/char/serial-isa.c     | 2 +-
>  hw/char/serial.c         | 1 +
>  3 files changed, 3 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[PATCH 6/7] hw/char/serial-isa: Alias QDEV properties from generic serial object

[Philippe Mathieu-Daudé]

Instead of overwritting the properties of the generic 'state'
object, alias them.
Note we can now propagate the "baudbase" property.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/char/serial-isa.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/char/serial-isa.c b/hw/char/serial-isa.c
index a0c338796d5..0626edda8d1 100644
--- a/hw/char/serial-isa.c
+++ b/hw/char/serial-isa.c
@@ -115,8 +115,6 @@ static Property serial_isa_properties[] = {
     DEFINE_PROP_UINT32("index",  ISASerialState, index,   -1),
     DEFINE_PROP_UINT32("iobase",  ISASerialState, iobase,  -1),
     DEFINE_PROP_UINT32("irq",    ISASerialState, isairq,  -1),
-    DEFINE_PROP_CHR("chardev",   ISASerialState, state.chr),
-    DEFINE_PROP_BOOL("wakeup",   ISASerialState, state.wakeup, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -137,6 +135,8 @@ static void serial_isa_initfn(Object *o)
     ISASerialState *self = ISA_SERIAL(o);
 
     object_initialize_child(o, "serial", &self->state, TYPE_SERIAL);
+
+    qdev_alias_all_properties(DEVICE(&self->state), o);
 }
 
 static const TypeInfo serial_isa_info = {
-- 
2.26.2

Re: [PATCH 6/7] hw/char/serial-isa: Alias QDEV properties from generic serial object

[Richard Henderson]

On 9/6/20 6:55 PM, Philippe Mathieu-Daudé wrote:
> Instead of overwritting the properties of the generic 'state'
> object, alias them.
> Note we can now propagate the "baudbase" property.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/char/serial-isa.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

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

[Philippe Mathieu-Daudé]

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

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

[Paolo Bonzini]

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

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
> 
> 

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

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

[Paolo Bonzini]

On 12/09/20 13:33, Philippe Mathieu-Daudé wrote:
>> I'll send a new patch for the PCI-single device:
> Bah this can simply be squashed into the previous patch.
> 

Yup, done.

Paolo