Re: [RFC PATCH 13/14] hw/char/pl011: add a clock input

[Philippe Mathieu-Daudé]

On 9/25/20 12:17 PM, Luc Michel wrote:
> Add a clock input to the PL011 UART so we can compute the current baud
> rate and trace it. This is intended for developers who wish to use QEMU
> to e.g. debug their firmware or to figure out the baud rate configured
> by an unknown/closed source binary.
> 
> Signed-off-by: Luc Michel <luc@lmichel.fr>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  include/hw/char/pl011.h |  1 +
>  hw/char/pl011.c         | 45 +++++++++++++++++++++++++++++++++++++++++
>  hw/char/trace-events    |  1 +
>  3 files changed, 47 insertions(+)
> 
> diff --git a/include/hw/char/pl011.h b/include/hw/char/pl011.h
> index a91ea50e11..33e5e5317b 100644
> --- a/include/hw/char/pl011.h
> +++ b/include/hw/char/pl011.h
> @@ -47,10 +47,11 @@ struct PL011State {
>      int read_pos;
>      int read_count;
>      int read_trigger;
>      CharBackend chr;
>      qemu_irq irq[6];
> +    Clock *clk;
>      const unsigned char *id;
>  };
>  
>  static inline DeviceState *pl011_create(hwaddr addr,
>                                          qemu_irq irq,
> diff --git a/hw/char/pl011.c b/hw/char/pl011.c
> index 13e784f9d9..ede16c781c 100644
> --- a/hw/char/pl011.c
> +++ b/hw/char/pl011.c
> @@ -20,10 +20,11 @@
>  
>  #include "qemu/osdep.h"
>  #include "hw/char/pl011.h"
>  #include "hw/irq.h"
>  #include "hw/sysbus.h"
> +#include "hw/qdev-clock.h"
>  #include "migration/vmstate.h"
>  #include "chardev/char-fe.h"
>  #include "qemu/log.h"
>  #include "qemu/module.h"
>  #include "trace.h"
> @@ -167,10 +168,29 @@ static void pl011_set_read_trigger(PL011State *s)
>      else
>  #endif
>          s->read_trigger = 1;
>  }
>  
> +static unsigned int pl011_get_baudrate(const PL011State *s)
> +{
> +    uint64_t clk;
> +
> +    if (s->fbrd == 0) {
> +        return 0;
> +    }
> +
> +    clk = clock_get_hz(s->clk);
> +    return (clk / ((s->ibrd << 6) + s->fbrd)) << 2;
> +}
> +
> +static void pl011_trace_baudrate_change(const PL011State *s)
> +{
> +    trace_pl011_baudrate_change(pl011_get_baudrate(s),
> +                                clock_get_hz(s->clk),
> +                                s->ibrd, s->fbrd);
> +}
> +
>  static void pl011_write(void *opaque, hwaddr offset,
>                          uint64_t value, unsigned size)
>  {
>      PL011State *s = (PL011State *)opaque;
>      unsigned char ch;
> @@ -196,13 +216,15 @@ static void pl011_write(void *opaque, hwaddr offset,
>      case 8: /* UARTUARTILPR */
>          s->ilpr = value;
>          break;
>      case 9: /* UARTIBRD */
>          s->ibrd = value;
> +        pl011_trace_baudrate_change(s);
>          break;
>      case 10: /* UARTFBRD */
>          s->fbrd = value;
> +        pl011_trace_baudrate_change(s);
>          break;
>      case 11: /* UARTLCR_H */
>          /* Reset the FIFO state on FIFO enable or disable */
>          if ((s->lcr ^ value) & 0x10) {
>              s->read_count = 0;
> @@ -284,16 +306,33 @@ static void pl011_event(void *opaque, QEMUChrEvent 
> event)
>  {
>      if (event == CHR_EVENT_BREAK)
>          pl011_put_fifo(opaque, 0x400);
>  }
>  
> +static void pl011_clock_update(void *opaque)
> +{
> +    PL011State *s = PL011(opaque);
> +
> +    pl011_trace_baudrate_change(s);
> +}
> +
>  static const MemoryRegionOps pl011_ops = {
>      .read = pl011_read,
>      .write = pl011_write,
>      .endianness = DEVICE_NATIVE_ENDIAN,
>  };
>  
> +static const VMStateDescription vmstate_pl011_clock = {
> +    .name = "pl011/clock",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_CLOCK(clk, PL011State),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  static const VMStateDescription vmstate_pl011 = {
>      .name = "pl011",
>      .version_id = 2,
>      .minimum_version_id = 2,
>      .fields = (VMStateField[]) {
> @@ -312,10 +351,14 @@ static const VMStateDescription vmstate_pl011 = {
>          VMSTATE_UINT32(ifl, PL011State),
>          VMSTATE_INT32(read_pos, PL011State),
>          VMSTATE_INT32(read_count, PL011State),
>          VMSTATE_INT32(read_trigger, PL011State),
>          VMSTATE_END_OF_LIST()
> +    },
> +    .subsections = (const VMStateDescription * []) {
> +        &vmstate_pl011_clock,
> +        NULL
>      }
>  };
>  
>  static Property pl011_properties[] = {
>      DEFINE_PROP_CHR("chardev", PL011State, chr),
> @@ -332,10 +375,12 @@ static void pl011_init(Object *obj)
>      sysbus_init_mmio(sbd, &s->iomem);
>      for (i = 0; i < ARRAY_SIZE(s->irq); i++) {
>          sysbus_init_irq(sbd, &s->irq[i]);
>      }
>  
> +    s->clk = qdev_init_clock_in(DEVICE(obj), "clk", pl011_clock_update, s);
> +
>      s->read_trigger = 1;
>      s->ifl = 0x12;
>      s->cr = 0x300;
>      s->flags = 0x90;
>  
> diff --git a/hw/char/trace-events b/hw/char/trace-events
> index 2442a9f7d5..cc1f0065ac 100644
> --- a/hw/char/trace-events
> +++ b/hw/char/trace-events
> @@ -62,10 +62,11 @@ pl011_read(uint32_t addr, uint32_t value) "addr 0x%08x 
> value 0x%08x"
>  pl011_read_fifo(int read_count) "FIFO read, read_count now %d"
>  pl011_write(uint32_t addr, uint32_t value) "addr 0x%08x value 0x%08x"
>  pl011_can_receive(uint32_t lcr, int read_count, int r) "LCR 0x%08x 
> read_count %d returning %d"
>  pl011_put_fifo(uint32_t c, int read_count) "new char 0x%x read_count now %d"
>  pl011_put_fifo_full(void) "FIFO now full, RXFF set"
> +pl011_baudrate_change(unsigned int baudrate, uint64_t clock, uint32_t ibrd, 
> uint32_t fbrd) "new baudrate %u (clk: %" PRIu64 "hz, ibrd: %" PRIu32 ", fbrd: 
> %" PRIu32 ")"
>  
>  # cmsdk-apb-uart.c
>  cmsdk_apb_uart_read(uint64_t offset, uint64_t data, unsigned size) "CMSDK 
> APB UART read: offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u"
>  cmsdk_apb_uart_write(uint64_t offset, uint64_t data, unsigned size) "CMSDK 
> APB UART write: offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u"
>  cmsdk_apb_uart_reset(void) "CMSDK APB UART: reset"
>