Re: [PATCH 1/6] hw/arm/z2: convert DPRINTF to tracepoints

[Alex Bennée]

Manos Pitsidianakis <manos.pitsidianakis@linaro.org> writes:

> Tracing DPRINTFs to stderr might not be desired. A developer that relies
> on tracepoints should be able to opt-in to each tracepoint and rely on
> QEMU's log redirection, instead of stderr by default.
>
> This commit converts DPRINTFs in this file that are used for tracing
> into tracepoints.
>
> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> ---
>  hw/arm/trace-events |  8 ++++++++
>  hw/arm/z2.c         | 26 +++++++++-----------------
>  2 files changed, 17 insertions(+), 17 deletions(-)
>
> diff --git a/hw/arm/trace-events b/hw/arm/trace-events
> index cdc1ea06a8..a262ad2e6a 100644
> --- a/hw/arm/trace-events
> +++ b/hw/arm/trace-events
> @@ -55,3 +55,11 @@ smmuv3_notify_flag_add(const char *iommu) "ADD 
> SMMUNotifier node for iommu mr=%s
>  smmuv3_notify_flag_del(const char *iommu) "DEL SMMUNotifier node for iommu 
> mr=%s"
>  smmuv3_inv_notifiers_iova(const char *name, uint16_t asid, uint16_t vmid, 
> uint64_t iova, uint8_t tg, uint64_t num_pages) "iommu mr=%s asid=%d vmid=%d 
> iova=0x%"PRIx64" tg=%d num_pages=0x%"PRIx64
>  
> +# z2.c
> +z2_lcd_cur_reg_update(uint8_t r) "reg: 0x%x"
> +z2_lcd_enable_disable(uint16_t v) "value: 0x%x"
> +z2_lcd_enable_disable_result(const char * result) "LCD %s"
> +z2_lcd_invalid_command(uint8_t value) "0x%x"
> +z2_aer915_send_too_log(int8_t msg) "message too long (%i bytes)"
> +z2_aer915_send(uint8_t reg, uint8_t value) "reg %d value 0x%02x"
> +z2_aer915_i2c_start_recv(uint16_t len) "I2C_START_RECV: short message with 
> len %d"
> diff --git a/hw/arm/z2.c b/hw/arm/z2.c
> index 83741a4909..6c0889d698 100644
> --- a/hw/arm/z2.c
> +++ b/hw/arm/z2.c
> @@ -28,13 +28,7 @@
>  #include "cpu.h"
>  #include "qom/object.h"
>  #include "qapi/error.h"
> -
> -#ifdef DEBUG_Z2
> -#define DPRINTF(fmt, ...) \
> -        printf(fmt, ## __VA_ARGS__)
> -#else
> -#define DPRINTF(fmt, ...)
> -#endif
> +#include "trace.h"
>  
>  static const struct keymap map[0x100] = {
>      [0 ... 0xff] = { -1, -1 },
> @@ -127,22 +121,22 @@ static uint32_t zipit_lcd_transfer(SSIPeripheral *dev, 
> uint32_t value)
>      if (z->pos == 3) {

Maybe we could just have:

   trace_z2_lcd_reg_update(z->buf[0], z->buf[1], z->buf[2]);

here

>          switch (z->buf[0]) {
>          case 0x74:
> -            DPRINTF("%s: reg: 0x%.2x\n", __func__, z->buf[2]);
> +            trace_z2_lcd_cur_reg_update(z->buf[2]);

drop this

>              z->cur_reg = z->buf[2];
>              break;
>          case 0x76:
>              val = z->buf[1] << 8 | z->buf[2];
> -            DPRINTF("%s: value: 0x%.4x\n", __func__, val);
> +            trace_z2_lcd_enable_disable(val);

and this

>              if (z->cur_reg == 0x22 && val == 0x0000) {
>                  z->enabled = 1;
> -                printf("%s: LCD enabled\n", __func__);
> +                trace_z2_lcd_enable_disable_result("enabled");
>              } else if (z->cur_reg == 0x10 && val == 0x0000) {
>                  z->enabled = 0;
> -                printf("%s: LCD disabled\n", __func__);
> +                trace_z2_lcd_enable_disable_result("disabled");

and just have two trace points, one for enable and one for disable to
save spamming a string into the log.

>              }
>              break;
>          default:
> -            DPRINTF("%s: unknown command!\n", __func__);
> +            trace_z2_lcd_invalid_command(z->buf[0]);

drop this, it can be inferred if we trace the command stream above.

>              break;
>          }
>          z->pos = 0;
> @@ -212,14 +206,12 @@ static int aer915_send(I2CSlave *i2c, uint8_t data)
>  
>      s->buf[s->len] = data;
>      if (s->len++ > 2) {
> -        DPRINTF("%s: message too long (%i bytes)\n",
> -            __func__, s->len);
> +        trace_z2_aer915_send_too_log(s->len);

long

>          return 1;
>      }
>  
>      if (s->len == 2) {
> -        DPRINTF("%s: reg %d value 0x%02x\n", __func__,
> -                s->buf[0], s->buf[1]);
> +        trace_z2_aer915_send(s->buf[0], s->buf[1]);
>      }
>  
>      return 0;
> @@ -235,7 +227,7 @@ static int aer915_event(I2CSlave *i2c, enum i2c_event 
> event)
>          break;
>      case I2C_START_RECV:
>          if (s->len != 1) {
> -            DPRINTF("%s: short message!?\n", __func__);
> +            trace_z2_aer915_i2c_start_recv(s->len);
>          }
>          break;
>      case I2C_FINISH:

maybe better just to have a:

  trace_aer915_event(event, s->len)

before the return?

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro