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

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

[Philippe Mathieu-Daudé]

Nothing very exciting, cleanups before more serious changes.

Since v1: addressed Paolo's comments
- Also alias QDev properties on the PCI device (patch 6)
- Initialize the 'id' property on the PCI multi-UART device (patch 7)

Patches 1-6 already queued by Paolo.

$ git backport-diff -r v2 -u v1
Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respective=
ly

001/7:[----] [--] 'hw/char/serial: Assert serial_ioport_read/write offset fit=
s 8 bytes'
002/7:[----] [--] 'hw/char/serial: Replace commented DPRINTF() by trace event'
003/7:[----] [--] 'hw/char/serial: Remove old DEBUG_SERIAL commented code'
004/7:[----] [--] 'hw/char/serial: Rename I/O read/write trace events'
005/7:[----] [-C] 'hw/char/serial: Make 'wakeup' property boolean'
006/7:[0003] [FC] 'hw/char/serial: Alias QDEV properties from generic serial =
object'
007/7:[0001] [FC] 'hw/char/serial: Let SerialState have an 'id' field'

Paolo, can you take this #6 (instead of v1) and #7 now?

Thanks,

Phil.

Philippe Mathieu-Daud=C3=A9 (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: 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-pci-multi.c |  1 +
 hw/char/serial-pci.c       |  3 ++-
 hw/char/serial.c           | 25 +++++++------------------
 hw/char/trace-events       |  5 +++--
 6 files changed, 17 insertions(+), 24 deletions(-)

--=20
2.26.2

[PATCH v2 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.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
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

[PATCH v2 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 2442a9f7d5f..17304bef26d 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 v2 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 v2 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.

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 | 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 17304bef26d..609df10fed4 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

[PATCH v2 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.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
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 264f529a7f1..09845721384 100644
--- a/include/hw/char/serial.h
+++ b/include/hw/char/serial.h
@@ -61,7 +61,7 @@ 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 d4aad81a85c..7e1b36c3f67 100644
--- a/hw/char/serial-isa.c
+++ b/hw/char/serial-isa.c
@@ -119,7 +119,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

[PATCH v2 6/7] hw/char/serial: 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 (except on
the PCI multi-UART device).

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

diff --git a/hw/char/serial-isa.c b/hw/char/serial-isa.c
index 7e1b36c3f67..c0607476c22 100644
--- a/hw/char/serial-isa.c
+++ b/hw/char/serial-isa.c
@@ -118,8 +118,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(),
 };
 
@@ -140,6 +138,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 = {
diff --git a/hw/char/serial-pci.c b/hw/char/serial-pci.c
index f68948154e0..81da2783f9e 100644
--- a/hw/char/serial-pci.c
+++ b/hw/char/serial-pci.c
@@ -84,7 +84,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(),
 };
@@ -109,6 +108,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);
 }
 
 static const TypeInfo serial_pci_info = {
-- 
2.26.2

[PATCH v2 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-pci-multi.c | 1 +
 hw/char/serial.c           | 7 ++++---
 hw/char/trace-events       | 6 +++---
 4 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h
index 09845721384..a0aaa319a93 100644
--- a/include/hw/char/serial.h
+++ b/include/hw/char/serial.h
@@ -76,6 +76,7 @@ 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;
 };
diff --git a/hw/char/serial-pci-multi.c b/hw/char/serial-pci-multi.c
index 2cf3e441773..53e0c6e25c0 100644
--- 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;
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 609df10fed4..45b8eaab655 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 v2 0/7] hw/char/serial: Housekeeping

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200912114040.918464-1-f4bug@amsat.org/



Hi,

This series failed build test on FreeBSD host. Please find the details below.






The full log is available at
http://patchew.org/logs/20200912114040.918464-1-f4bug@amsat.org/testing.FreeBSD/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

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

[Ed Maste]

On Sat, 12 Sep 2020 at 07:46, <no-reply@patchew.org> wrote:
>
> Patchew URL: https://patchew.org/QEMU/20200912114040.918464-1-f4bug@amsat.org/
>
> Hi,
>
> This series failed build test on FreeBSD host. Please find the details below.
>
> The full log is available at
> http://patchew.org/logs/20200912114040.918464-1-f4bug@amsat.org/testing.FreeBSD/?type=message.

Looks like a full disk:

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]      patchew/20200912114040.918464-1-f4bug@amsat.org
-> patchew/20200912114040.918464-1-f4bug@amsat.org
error: copy-fd: write returned No space left on device
fatal: failed to copy file to
'/var/tmp/patchew-tester-tmp-b6phottt/src/.git/objects/pack/pack-7bdf4f5a855cab7e92327c9c14876dca81c24a36.pack':
No space left on device
fatal: The remote end hung up unexpectedly

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

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200912114040.918464-1-f4bug@amsat.org/



Hi,

This series seems to have some coding style problems. See output below for
more information:

N/A. Internal error while reading log file



The full log is available at
http://patchew.org/logs/20200912114040.918464-1-f4bug@amsat.org/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com