All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/5] hw/{arm,xen} convert printfs to trace/reports
@ 2024-01-19 13:24 Manos Pitsidianakis
  2024-01-19 13:24 ` [PATCH v2 1/5] hw/arm/z2: convert DPRINTF to trace events and guest errors Manos Pitsidianakis
                   ` (4 more replies)
  0 siblings, 5 replies; 8+ messages in thread
From: Manos Pitsidianakis @ 2024-01-19 13:24 UTC (permalink / raw)
  To: qemu-devel
  Cc: Philippe Mathieu-Daudé, qemu-arm, Paolo Bonzini, Alex Bennée

This series changes some printfs to use the trace event framework. 
Additionally, it converts some error/warning reporting fprintfs to 
error_report/warn_report.

Differences from v1 
<cover.1705662313.git.manos.pitsidianakis@linaro.org>:
    - addressed Alex's review

Manos Pitsidianakis (5):
  hw/arm/z2: convert DPRINTF to trace events and guest errors
  hw/arm/xen_arm.c: convert DPRINTF to trace events and error/warn
    reports
  hw/xen/xen-mapcache.c: convert DPRINTF to tracepoints
  hw/xen/xen-hvm-common.c: convert DPRINTF to tracepoints
  hw/xen: convert stderr prints to error/warn reports

 hw/arm/strongarm.c      | 55 +++++++++++++++++++-------------------
 hw/arm/trace-events     | 15 +++++++++++
 hw/arm/xen_arm.c        | 23 ++++++++--------
 hw/arm/z2.c             | 27 ++++++-------------
 hw/xen/trace-events     | 21 ++++++++++++++-
 hw/xen/xen-hvm-common.c | 47 ++++++++++++++++----------------
 hw/xen/xen-mapcache.c   | 59 ++++++++++++++++++-----------------------
 7 files changed, 131 insertions(+), 116 deletions(-)

Range-diff against v1:
1:  021405f5ef < -:  ---------- hw/arm/z2: convert DPRINTF to tracepoints
2:  3c6fbd73a1 ! 1:  339219e962 hw/arm/strongarm.c: convert DPRINTF to tracepoints
    @@ Metadata
     Author: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
     
      ## Commit message ##
    -    hw/arm/strongarm.c: convert DPRINTF to tracepoints
    +    hw/arm/z2: convert DPRINTF to trace events and guest errors
     
         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
    +    on trace events should be able to opt-in to each trace event 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.
    +    into trace events. DPRINTFs that report guest errors are logged with
    +    LOG_GUEST_ERROR.
     
         Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
     
    @@ hw/arm/strongarm.c: static uint64_t strongarm_pic_mem_read(void *opaque, hwaddr
          default:
     -        printf("%s: Bad register offset 0x" HWADDR_FMT_plx "\n",
     -                        __func__, offset);
    -+        trace_strongarm_pic_mem_read(offset);
    ++        qemu_log_mask(LOG_GUEST_ERROR,
    ++                      "Bad pic mem register read offset 0x%zu",
    ++                      offset);
              return 0;
          }
      }
    @@ hw/arm/strongarm.c: static void strongarm_pic_mem_write(void *opaque, hwaddr off
          default:
     -        printf("%s: Bad register offset 0x" HWADDR_FMT_plx "\n",
     -                        __func__, offset);
    -+        trace_strongarm_pic_mem_write(offset);
    ++        qemu_log_mask(LOG_GUEST_ERROR,
    ++                     "Bad pic mem register write offset 0x%zu",
    ++                      offset);
              break;
          }
          strongarm_pic_update(s);
    @@ hw/arm/strongarm.c: static uint64_t strongarm_rtc_read(void *opaque, hwaddr addr
                      (1000 * ((s->rttr & 0xffff) + 1));
          default:
     -        printf("%s: Bad register 0x" HWADDR_FMT_plx "\n", __func__, addr);
    -+        trace_strongarm_rtc_read(addr);
    ++        qemu_log_mask(LOG_GUEST_ERROR, "Bad rtc register read 0x%zu", addr);
              return 0;
          }
      }
    @@ hw/arm/strongarm.c: static void strongarm_rtc_write(void *opaque, hwaddr addr,
      
          default:
     -        printf("%s: Bad register 0x" HWADDR_FMT_plx "\n", __func__, addr);
    -+        trace_strongarm_rtc_write(addr);
    ++        qemu_log_mask(LOG_GUEST_ERROR, "Bad rtc register write 0x%zu", addr);
          }
      }
      
    @@ hw/arm/strongarm.c: static uint64_t strongarm_gpio_read(void *opaque, hwaddr off
      
          default:
     -        printf("%s: Bad offset 0x" HWADDR_FMT_plx "\n", __func__, offset);
    -+        trace_strongarm_gpio_read(offset);
    ++        qemu_log_mask(LOG_GUEST_ERROR, "Bad gpio read offset 0x%zu", offset);
          }
      
          return 0;
    @@ hw/arm/strongarm.c: static void strongarm_gpio_write(void *opaque, hwaddr offset
      
          default:
     -        printf("%s: Bad offset 0x" HWADDR_FMT_plx "\n", __func__, offset);
    -+        trace_strongarm_gpio_write(offset);
    ++        qemu_log_mask(LOG_GUEST_ERROR, "Bad gpio write offset 0x%zu", offset);
          }
      }
      
    @@ hw/arm/strongarm.c: static uint64_t strongarm_ppc_read(void *opaque, hwaddr offs
      
          default:
     -        printf("%s: Bad offset 0x" HWADDR_FMT_plx "\n", __func__, offset);
    -+        trace_strongarm_ppc_read(offset);
    ++        qemu_log_mask(LOG_GUEST_ERROR, "Bad ppc read offset 0x%zu", offset);
          }
      
          return 0;
    @@ hw/arm/strongarm.c: static void strongarm_ppc_write(void *opaque, hwaddr offset,
      
          default:
     -        printf("%s: Bad offset 0x" HWADDR_FMT_plx "\n", __func__, offset);
    -+        trace_strongarm_ppc_write(offset);
    ++        qemu_log_mask(LOG_GUEST_ERROR, "Bad ppc write offset 0x%zu", offset);
          }
      }
      
    @@ hw/arm/strongarm.c: static uint64_t strongarm_uart_read(void *opaque, hwaddr add
      
          default:
     -        printf("%s: Bad register 0x" HWADDR_FMT_plx "\n", __func__, addr);
    -+        trace_strongarm_uart_read_bad_register(addr);
    ++        qemu_log_mask(LOG_GUEST_ERROR, "Bad uart register read 0x%zu", addr);
              return 0;
          }
      }
    @@ hw/arm/strongarm.c: static void strongarm_uart_write(void *opaque, hwaddr addr,
      
          default:
     -        printf("%s: Bad register 0x" HWADDR_FMT_plx "\n", __func__, addr);
    -+        trace_strongarm_uart_write_bad_register(addr);
    ++        qemu_log_mask(LOG_GUEST_ERROR, "Bad uart register write 0x%zu", addr);
          }
      }
      
    @@ hw/arm/strongarm.c: static uint64_t strongarm_ssp_read(void *opaque, hwaddr addr
              return retval;
          default:
     -        printf("%s: Bad register 0x" HWADDR_FMT_plx "\n", __func__, addr);
    -+        trace_strongarm_ssp_read(addr);
    ++        qemu_log_mask(LOG_GUEST_ERROR, "Bad ssp register read 0x%zu", addr);
              break;
          }
          return 0;
    @@ hw/arm/strongarm.c: static void strongarm_ssp_write(void *opaque, hwaddr addr,
              if ((s->sscr[0] & SSCR0_SSE) && SSCR0_DSS(value) < 4) {
     -            printf("%s: Wrong data size: %i bits\n", __func__,
     -                   (int)SSCR0_DSS(value));
    -+            trace_strongarm_ssp_write_wrong_data_size((int)SSCR0_DSS(value));
    ++            qemu_log_mask(LOG_GUEST_ERROR,
    ++                          "Wrong data size: %i bits",
    ++                          (int)SSCR0_DSS(value));
              }
              if (!(value & SSCR0_SSE)) {
                  s->sssr = 0;
    @@ hw/arm/strongarm.c: static void strongarm_ssp_write(void *opaque, hwaddr addr,
              s->sscr[1] = value & 0x2f;
              if (value & SSCR1_LBM) {
     -            printf("%s: Attempt to use SSP LBM mode\n", __func__);
    -+            trace_strongarm_ssp_write_wrong_data_size_invalid();
    ++            qemu_log_mask(LOG_GUEST_ERROR, "Attempt to use SSP LBM mode");
              }
              strongarm_ssp_fifo_update(s);
              break;
    @@ hw/arm/strongarm.c: static void strongarm_ssp_write(void *opaque, hwaddr addr,
      
          default:
     -        printf("%s: Bad register 0x" HWADDR_FMT_plx "\n", __func__, addr);
    -+        trace_strongarm_ssp_write_bad_register(addr);
    ++        qemu_log_mask(LOG_GUEST_ERROR, "Bad ssp register write 0x%zu", addr);
              break;
          }
      }
     
      ## hw/arm/trace-events ##
    -@@ hw/arm/trace-events: 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"
    +@@ hw/arm/trace-events: 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_reg_update(uint8_t cur, uint8_t _0, uint8_t _1, uint8_t _2, uint32_t value) "cur_reg = 0x%xd, buf = [0x%xd, 0x%xd, 0x%xd], value = 0x%x"
    ++z2_lcd_enable_disable_result(const char * result) "LCD %s"
    ++z2_aer915_send_too_long(int8_t msg) "message too long (%i bytes)"
    ++z2_aer915_send(uint8_t reg, uint8_t value) "reg %d value 0x%02x"
    ++z2_aer915_event(int8_t event, int8_t len) "i2c event =0x%x len=%d bytes"
     +
     +# strongarm.c
     +strongarm_uart_update_parameters(const char *label, int speed, char parity, int data_bits, int stop_bits) "%s speed=%d parity=%c data=%d stop=%d"
    -+strongarm_uart_read_bad_register(uint64_t addr) "Bad uart register 0x%zu"
    -+strongarm_uart_write_bad_register(uint64_t addr) "Bad uart register 0x%zu"
    -+strongarm_pic_mem_read(uint64_t offset) "Bad pic mem register read offset 0x%zu"
    -+strongarm_pic_mem_write(uint64_t offset) "Bad pic mem register write offset 0x%zu"
    -+strongarm_rtc_read(uint64_t addr) "Bad rtc register read 0x%zu"
    -+strongarm_rtc_write(uint64_t addr) "Bad rtc register write 0x%zu"
    -+strongarm_gpio_read(uint64_t offset) "Bad gpio read offset 0x%zu"
    -+strongarm_gpio_write(uint64_t offset) "Bad gpio write offset 0x%zu"
    -+strongarm_ppc_write(uint64_t offset) "Bad ppc write offset 0x%zu"
    -+strongarm_ppc_read(uint64_t offset) "Bad ppc write offset 0x%zu"
    -+strongarm_ssp_read_underrun(void) "SSP Rx Underrun"
    -+strongarm_ssp_read(uint64_t addr) "Bad register 0x%zu"
    -+strongarm_ssp_write_wrong_data_size(int v) "Wrong data size: %i bits"
    -+strongarm_ssp_write_wrong_data_size_invalid(void) "Attempt to use SSP LBM mode"
    -+strongarm_ssp_write_bad_register(uint64_t addr) "Bad register 0x%zu"
    ++strongarm_ssp_read_underrun(void) "SSP rx underrun"
    +
    + ## hw/arm/z2.c ##
    +@@
    + #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 },
    +@@ hw/arm/z2.c: static uint32_t zipit_lcd_transfer(SSIPeripheral *dev, uint32_t value)
    + {
    +     ZipitLCD *z = ZIPIT_LCD(dev);
    +     uint16_t val;
    ++
    ++    trace_z2_lcd_reg_update(z->cur_reg, z->buf[0], z->buf[1], z->buf[2], value);
    +     if (z->selected) {
    +         z->buf[z->pos] = value & 0xff;
    +         z->pos++;
    +@@ hw/arm/z2.c: static uint32_t zipit_lcd_transfer(SSIPeripheral *dev, uint32_t value)
    +     if (z->pos == 3) {
    +         switch (z->buf[0]) {
    +         case 0x74:
    +-            DPRINTF("%s: reg: 0x%.2x\n", __func__, z->buf[2]);
    +             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);
    +             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");
    +             }
    +             break;
    +         default:
    +-            DPRINTF("%s: unknown command!\n", __func__);
    +             break;
    +         }
    +         z->pos = 0;
    +@@ hw/arm/z2.c: 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_long(s->len);
    +         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;
    +@@ hw/arm/z2.c: static int aer915_event(I2CSlave *i2c, enum i2c_event event)
    + {
    +     AER915State *s = AER915(i2c);
    + 
    ++    trace_z2_aer915_event(s->len, event);
    +     switch (event) {
    +     case I2C_START_SEND:
    +         s->len = 0;
    +         break;
    +     case I2C_START_RECV:
    +-        if (s->len != 1) {
    +-            DPRINTF("%s: short message!?\n", __func__);
    +-        }
    +         break;
    +     case I2C_FINISH:
    +         break;
3:  49905a0d22 ! 2:  c7eb8943f0 hw/arm/xen_arm.c: replace DPRINTF with traces
    @@ Metadata
     Author: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
     
      ## Commit message ##
    -    hw/arm/xen_arm.c: replace DPRINTF with traces
    +    hw/arm/xen_arm.c: convert DPRINTF to trace events and error/warn reports
    +
    +    Tracing DPRINTFs to stderr might not be desired. A developer that relies
    +    on trace events should be able to opt-in to each trace event 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 trace events. Errors or warnings are converted to error_report and
    +    warn_report calls.
     
         Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
     
      ## hw/arm/trace-events ##
    -@@ hw/arm/trace-events: strongarm_ssp_read(uint64_t addr) "Bad register 0x%zu"
    - strongarm_ssp_write_wrong_data_size(int v) "Wrong data size: %i bits"
    - strongarm_ssp_write_wrong_data_size_invalid(void) "Attempt to use SSP LBM mode"
    - strongarm_ssp_write_bad_register(uint64_t addr) "Bad register 0x%zu"
    +@@ hw/arm/trace-events: z2_aer915_event(int8_t event, int8_t len) "i2c event =0x%x len=%d bytes"
    + # strongarm.c
    + strongarm_uart_update_parameters(const char *label, int speed, char parity, int data_bits, int stop_bits) "%s speed=%d parity=%c data=%d stop=%d"
    + strongarm_ssp_read_underrun(void) "SSP rx underrun"
     +
     +# xen_arm.c
     +xen_create_virtio_mmio_devices(int i, int irq, uint64_t base) "Created virtio-mmio device %d: irq %d base 0x%lx"
    -+xen_init_ram(const char *hi_xor_low, uint64_t base, uint64_t size) "Initialized region xen.ram.%s: base 0x%lx size 0x%lx"
    -+xen_enable_tpm_not_found(void) "Couldn't find tmp0 backend"
    ++xen_init_ram(uint64_t machine_ram_size) "Initialized xen ram with size 0x%lx"
     +xen_enable_tpm(uint64_t addr) "Connected tpmdev at address 0x%lx"
    -+xen_arm_init(const char *msg) "%s"
     
      ## hw/arm/xen_arm.c ##
     @@
    @@ hw/arm/xen_arm.c: static void xen_create_virtio_mmio_devices(XenArmState *xam)
      }
      
     @@ hw/arm/xen_arm.c: static void xen_init_ram(MachineState *machine)
    +     MemoryRegion *sysmem = get_system_memory();
    +     ram_addr_t block_len, ram_size[GUEST_RAM_BANKS];
    + 
    ++    trace_xen_init_ram(machine->ram_size);
    +     if (machine->ram_size <= GUEST_RAM0_SIZE) {
    +         ram_size[0] = machine->ram_size;
    +         ram_size[1] = 0;
    +@@ hw/arm/xen_arm.c: static void xen_init_ram(MachineState *machine)
          memory_region_init_alias(&ram_lo, NULL, "xen.ram.lo", &ram_memory,
                                   GUEST_RAM0_BASE, ram_size[0]);
          memory_region_add_subregion(sysmem, GUEST_RAM0_BASE, &ram_lo);
     -    DPRINTF("Initialized region xen.ram.lo: base 0x%llx size 0x%lx\n",
     -            GUEST_RAM0_BASE, ram_size[0]);
    -+    trace_xen_init_ram("lo", GUEST_RAM0_BASE, ram_size[0]);
    - 
    +-
          if (ram_size[1] > 0) {
              memory_region_init_alias(&ram_hi, NULL, "xen.ram.hi", &ram_memory,
                                       GUEST_RAM1_BASE, ram_size[1]);
              memory_region_add_subregion(sysmem, GUEST_RAM1_BASE, &ram_hi);
     -        DPRINTF("Initialized region xen.ram.hi: base 0x%llx size 0x%lx\n",
     -                GUEST_RAM1_BASE, ram_size[1]);
    -+        trace_xen_init_ram("hi", GUEST_RAM1_BASE, ram_size[1]);
          }
      }
      
    @@ hw/arm/xen_arm.c: static void xen_enable_tpm(XenArmState *xam)
          TPMBackend *be = qemu_find_tpm_be("tpm0");
          if (be == NULL) {
     -        DPRINTF("Couldn't fine the backend for tpm0\n");
    -+        trace_xen_enable_tpm_not_found();
    ++        error_report("Couldn't find tmp0 backend");
              return;
          }
          dev = qdev_new(TYPE_TPM_TIS_SYSBUS);
    @@ hw/arm/xen_arm.c: static void xen_arm_init(MachineState *machine)
          if (machine->ram_size == 0) {
     -        DPRINTF("ram_size not specified. QEMU machine started without IOREQ"
     -                "(no emulated devices including Virtio)\n");
    -+        trace_xen_arm_init("ram_size not specified. "
    -+                           "QEMU machine started "
    -+                           "without IOREQ "
    -+                           "(no emulated devices"
    -+                           "including Virtio)");
    ++        warn_report("%s non-zero ram size not specified. QEMU machine started"
    ++                    " without IOREQ (no emulated devices including virtio)",
    ++                    MACHINE_CLASS(object_get_class(OBJECT(machine)))->desc);
              return;
          }
      
    @@ hw/arm/xen_arm.c: static void xen_arm_init(MachineState *machine)
              xen_enable_tpm(xam);
          } else {
     -        DPRINTF("tpm-base-addr is not provided. TPM will not be enabled\n");
    -+        trace_xen_arm_init("tpm-base-addr is not provided."
    -+                           "TPM will not be enabled");
    ++        warn_report("tpm-base-addr is not provided. TPM will not be enabled");
          }
      #endif
      }
4:  c5c74aadad = 3:  d5493e864f hw/xen/xen-mapcache.c: convert DPRINTF to tracepoints
5:  d1f96a077b = 4:  651ad62da1 hw/xen/xen-hvm-common.c: convert DPRINTF to tracepoints
6:  a741c52584 = 5:  bf55043690 hw/xen: convert stderr prints to error/warn reports

base-commit: d0f4aa7d50d485b1fb5ec8ab6f934e5df852ab07
-- 
γαῖα πυρί μιχθήτω



^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2 1/5] hw/arm/z2: convert DPRINTF to trace events and guest errors
  2024-01-19 13:24 [PATCH v2 0/5] hw/{arm,xen} convert printfs to trace/reports Manos Pitsidianakis
@ 2024-01-19 13:24 ` Manos Pitsidianakis
  2024-01-22 18:20   ` Peter Maydell
  2024-01-19 13:24 ` [PATCH v2 2/5] hw/arm/xen_arm.c: convert DPRINTF to trace events and error/warn reports Manos Pitsidianakis
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 8+ messages in thread
From: Manos Pitsidianakis @ 2024-01-19 13:24 UTC (permalink / raw)
  To: qemu-devel
  Cc: Philippe Mathieu-Daudé,
	qemu-arm, Paolo Bonzini, Alex Bennée, Peter Maydell

Tracing DPRINTFs to stderr might not be desired. A developer that relies
on trace events should be able to opt-in to each trace event 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 trace events. DPRINTFs that report guest errors are logged with
LOG_GUEST_ERROR.

Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
---
 hw/arm/strongarm.c  | 55 ++++++++++++++++++++++-----------------------
 hw/arm/trace-events | 10 +++++++++
 hw/arm/z2.c         | 27 +++++++---------------
 3 files changed, 45 insertions(+), 47 deletions(-)

diff --git a/hw/arm/strongarm.c b/hw/arm/strongarm.c
index fef3638aca..9fca0b302c 100644
--- a/hw/arm/strongarm.c
+++ b/hw/arm/strongarm.c
@@ -46,8 +46,7 @@
 #include "qemu/cutils.h"
 #include "qemu/log.h"
 #include "qom/object.h"
-
-//#define DEBUG
+#include "trace.h"
 
 /*
  TODO
@@ -66,12 +65,6 @@
  - Enhance UART with modem signals
  */
 
-#ifdef DEBUG
-# define DPRINTF(format, ...) printf(format , ## __VA_ARGS__)
-#else
-# define DPRINTF(format, ...) do { } while (0)
-#endif
-
 static struct {
     hwaddr io_base;
     int irq;
@@ -151,8 +144,9 @@ static uint64_t strongarm_pic_mem_read(void *opaque, hwaddr offset,
     case ICPR:
         return s->pending;
     default:
-        printf("%s: Bad register offset 0x" HWADDR_FMT_plx "\n",
-                        __func__, offset);
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "Bad pic mem register read offset 0x%zu",
+                      offset);
         return 0;
     }
 }
@@ -173,8 +167,9 @@ static void strongarm_pic_mem_write(void *opaque, hwaddr offset,
         s->int_idle = (value & 1) ? 0 : ~0;
         break;
     default:
-        printf("%s: Bad register offset 0x" HWADDR_FMT_plx "\n",
-                        __func__, offset);
+        qemu_log_mask(LOG_GUEST_ERROR,
+                     "Bad pic mem register write offset 0x%zu",
+                      offset);
         break;
     }
     strongarm_pic_update(s);
@@ -333,7 +328,7 @@ static uint64_t strongarm_rtc_read(void *opaque, hwaddr addr,
                 ((qemu_clock_get_ms(rtc_clock) - s->last_hz) << 15) /
                 (1000 * ((s->rttr & 0xffff) + 1));
     default:
-        printf("%s: Bad register 0x" HWADDR_FMT_plx "\n", __func__, addr);
+        qemu_log_mask(LOG_GUEST_ERROR, "Bad rtc register read 0x%zu", addr);
         return 0;
     }
 }
@@ -375,7 +370,7 @@ static void strongarm_rtc_write(void *opaque, hwaddr addr,
         break;
 
     default:
-        printf("%s: Bad register 0x" HWADDR_FMT_plx "\n", __func__, addr);
+        qemu_log_mask(LOG_GUEST_ERROR, "Bad rtc register write 0x%zu", addr);
     }
 }
 
@@ -581,7 +576,7 @@ static uint64_t strongarm_gpio_read(void *opaque, hwaddr offset,
         return s->status;
 
     default:
-        printf("%s: Bad offset 0x" HWADDR_FMT_plx "\n", __func__, offset);
+        qemu_log_mask(LOG_GUEST_ERROR, "Bad gpio read offset 0x%zu", offset);
     }
 
     return 0;
@@ -626,7 +621,7 @@ static void strongarm_gpio_write(void *opaque, hwaddr offset,
         break;
 
     default:
-        printf("%s: Bad offset 0x" HWADDR_FMT_plx "\n", __func__, offset);
+        qemu_log_mask(LOG_GUEST_ERROR, "Bad gpio write offset 0x%zu", offset);
     }
 }
 
@@ -782,7 +777,7 @@ static uint64_t strongarm_ppc_read(void *opaque, hwaddr offset,
         return s->ppfr | ~0x7f001;
 
     default:
-        printf("%s: Bad offset 0x" HWADDR_FMT_plx "\n", __func__, offset);
+        qemu_log_mask(LOG_GUEST_ERROR, "Bad ppc read offset 0x%zu", offset);
     }
 
     return 0;
@@ -817,7 +812,7 @@ static void strongarm_ppc_write(void *opaque, hwaddr offset,
         break;
 
     default:
-        printf("%s: Bad offset 0x" HWADDR_FMT_plx "\n", __func__, offset);
+        qemu_log_mask(LOG_GUEST_ERROR, "Bad ppc write offset 0x%zu", offset);
     }
 }
 
@@ -1029,8 +1024,11 @@ static void strongarm_uart_update_parameters(StrongARMUARTState *s)
     s->char_transmit_time =  (NANOSECONDS_PER_SECOND / speed) * frame_size;
     qemu_chr_fe_ioctl(&s->chr, CHR_IOCTL_SERIAL_SET_PARAMS, &ssp);
 
-    DPRINTF(stderr, "%s speed=%d parity=%c data=%d stop=%d\n", s->chr->label,
-            speed, parity, data_bits, stop_bits);
+    trace_strongarm_uart_update_parameters(s->chr.chr->label?:"NULL",
+                                           speed,
+                                           parity,
+                                           data_bits,
+                                           stop_bits);
 }
 
 static void strongarm_uart_rx_to(void *opaque)
@@ -1164,7 +1162,7 @@ static uint64_t strongarm_uart_read(void *opaque, hwaddr addr,
         return s->utsr1;
 
     default:
-        printf("%s: Bad register 0x" HWADDR_FMT_plx "\n", __func__, addr);
+        qemu_log_mask(LOG_GUEST_ERROR, "Bad uart register read 0x%zu", addr);
         return 0;
     }
 }
@@ -1221,7 +1219,7 @@ static void strongarm_uart_write(void *opaque, hwaddr addr,
         break;
 
     default:
-        printf("%s: Bad register 0x" HWADDR_FMT_plx "\n", __func__, addr);
+        qemu_log_mask(LOG_GUEST_ERROR, "Bad uart register write 0x%zu", addr);
     }
 }
 
@@ -1434,7 +1432,7 @@ static uint64_t strongarm_ssp_read(void *opaque, hwaddr addr,
             return 0xffffffff;
         }
         if (s->rx_level < 1) {
-            printf("%s: SSP Rx Underrun\n", __func__);
+            trace_strongarm_ssp_read_underrun();
             return 0xffffffff;
         }
         s->rx_level--;
@@ -1443,7 +1441,7 @@ static uint64_t strongarm_ssp_read(void *opaque, hwaddr addr,
         strongarm_ssp_fifo_update(s);
         return retval;
     default:
-        printf("%s: Bad register 0x" HWADDR_FMT_plx "\n", __func__, addr);
+        qemu_log_mask(LOG_GUEST_ERROR, "Bad ssp register read 0x%zu", addr);
         break;
     }
     return 0;
@@ -1458,8 +1456,9 @@ static void strongarm_ssp_write(void *opaque, hwaddr addr,
     case SSCR0:
         s->sscr[0] = value & 0xffbf;
         if ((s->sscr[0] & SSCR0_SSE) && SSCR0_DSS(value) < 4) {
-            printf("%s: Wrong data size: %i bits\n", __func__,
-                   (int)SSCR0_DSS(value));
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "Wrong data size: %i bits",
+                          (int)SSCR0_DSS(value));
         }
         if (!(value & SSCR0_SSE)) {
             s->sssr = 0;
@@ -1471,7 +1470,7 @@ static void strongarm_ssp_write(void *opaque, hwaddr addr,
     case SSCR1:
         s->sscr[1] = value & 0x2f;
         if (value & SSCR1_LBM) {
-            printf("%s: Attempt to use SSP LBM mode\n", __func__);
+            qemu_log_mask(LOG_GUEST_ERROR, "Attempt to use SSP LBM mode");
         }
         strongarm_ssp_fifo_update(s);
         break;
@@ -1509,7 +1508,7 @@ static void strongarm_ssp_write(void *opaque, hwaddr addr,
         break;
 
     default:
-        printf("%s: Bad register 0x" HWADDR_FMT_plx "\n", __func__, addr);
+        qemu_log_mask(LOG_GUEST_ERROR, "Bad ssp register write 0x%zu", addr);
         break;
     }
 }
diff --git a/hw/arm/trace-events b/hw/arm/trace-events
index cdc1ea06a8..b0a56fcdc6 100644
--- a/hw/arm/trace-events
+++ b/hw/arm/trace-events
@@ -55,3 +55,13 @@ 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_reg_update(uint8_t cur, uint8_t _0, uint8_t _1, uint8_t _2, uint32_t value) "cur_reg = 0x%xd, buf = [0x%xd, 0x%xd, 0x%xd], value = 0x%x"
+z2_lcd_enable_disable_result(const char * result) "LCD %s"
+z2_aer915_send_too_long(int8_t msg) "message too long (%i bytes)"
+z2_aer915_send(uint8_t reg, uint8_t value) "reg %d value 0x%02x"
+z2_aer915_event(int8_t event, int8_t len) "i2c event =0x%x len=%d bytes"
+
+# strongarm.c
+strongarm_uart_update_parameters(const char *label, int speed, char parity, int data_bits, int stop_bits) "%s speed=%d parity=%c data=%d stop=%d"
+strongarm_ssp_read_underrun(void) "SSP rx underrun"
diff --git a/hw/arm/z2.c b/hw/arm/z2.c
index 83741a4909..147a5f9857 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 },
@@ -120,6 +114,8 @@ static uint32_t zipit_lcd_transfer(SSIPeripheral *dev, uint32_t value)
 {
     ZipitLCD *z = ZIPIT_LCD(dev);
     uint16_t val;
+
+    trace_z2_lcd_reg_update(z->cur_reg, z->buf[0], z->buf[1], z->buf[2], value);
     if (z->selected) {
         z->buf[z->pos] = value & 0xff;
         z->pos++;
@@ -127,22 +123,19 @@ static uint32_t zipit_lcd_transfer(SSIPeripheral *dev, uint32_t value)
     if (z->pos == 3) {
         switch (z->buf[0]) {
         case 0x74:
-            DPRINTF("%s: reg: 0x%.2x\n", __func__, z->buf[2]);
             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);
             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");
             }
             break;
         default:
-            DPRINTF("%s: unknown command!\n", __func__);
             break;
         }
         z->pos = 0;
@@ -212,14 +205,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_long(s->len);
         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;
@@ -229,14 +220,12 @@ static int aer915_event(I2CSlave *i2c, enum i2c_event event)
 {
     AER915State *s = AER915(i2c);
 
+    trace_z2_aer915_event(s->len, event);
     switch (event) {
     case I2C_START_SEND:
         s->len = 0;
         break;
     case I2C_START_RECV:
-        if (s->len != 1) {
-            DPRINTF("%s: short message!?\n", __func__);
-        }
         break;
     case I2C_FINISH:
         break;
-- 
γαῖα πυρί μιχθήτω



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 2/5] hw/arm/xen_arm.c: convert DPRINTF to trace events and error/warn reports
  2024-01-19 13:24 [PATCH v2 0/5] hw/{arm,xen} convert printfs to trace/reports Manos Pitsidianakis
  2024-01-19 13:24 ` [PATCH v2 1/5] hw/arm/z2: convert DPRINTF to trace events and guest errors Manos Pitsidianakis
@ 2024-01-19 13:24 ` Manos Pitsidianakis
  2024-01-19 13:24 ` [PATCH v2 3/5] hw/xen/xen-mapcache.c: convert DPRINTF to tracepoints Manos Pitsidianakis
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 8+ messages in thread
From: Manos Pitsidianakis @ 2024-01-19 13:24 UTC (permalink / raw)
  To: qemu-devel
  Cc: Philippe Mathieu-Daudé,
	qemu-arm, Paolo Bonzini, Alex Bennée, Peter Maydell

Tracing DPRINTFs to stderr might not be desired. A developer that relies
on trace events should be able to opt-in to each trace event 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 trace events. Errors or warnings are converted to error_report and
warn_report calls.

Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
---
 hw/arm/trace-events |  5 +++++
 hw/arm/xen_arm.c    | 23 +++++++++++------------
 2 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/hw/arm/trace-events b/hw/arm/trace-events
index b0a56fcdc6..623eb003bb 100644
--- a/hw/arm/trace-events
+++ b/hw/arm/trace-events
@@ -65,3 +65,8 @@ z2_aer915_event(int8_t event, int8_t len) "i2c event =0x%x len=%d bytes"
 # strongarm.c
 strongarm_uart_update_parameters(const char *label, int speed, char parity, int data_bits, int stop_bits) "%s speed=%d parity=%c data=%d stop=%d"
 strongarm_ssp_read_underrun(void) "SSP rx underrun"
+
+# xen_arm.c
+xen_create_virtio_mmio_devices(int i, int irq, uint64_t base) "Created virtio-mmio device %d: irq %d base 0x%lx"
+xen_init_ram(uint64_t machine_ram_size) "Initialized xen ram with size 0x%lx"
+xen_enable_tpm(uint64_t addr) "Connected tpmdev at address 0x%lx"
diff --git a/hw/arm/xen_arm.c b/hw/arm/xen_arm.c
index a5631529d0..32776d94df 100644
--- a/hw/arm/xen_arm.c
+++ b/hw/arm/xen_arm.c
@@ -34,6 +34,7 @@
 #include "hw/xen/xen-hvm-common.h"
 #include "sysemu/tpm.h"
 #include "hw/xen/arch_hvm.h"
+#include "trace.h"
 
 #define TYPE_XEN_ARM  MACHINE_TYPE_NAME("xenpvh")
 OBJECT_DECLARE_SIMPLE_TYPE(XenArmState, XEN_ARM)
@@ -91,8 +92,9 @@ static void xen_create_virtio_mmio_devices(XenArmState *xam)
 
         sysbus_create_simple("virtio-mmio", base, irq);
 
-        DPRINTF("Created virtio-mmio device %d: irq %d base 0x%lx\n",
-                i, GUEST_VIRTIO_MMIO_SPI_FIRST + i, base);
+        trace_xen_create_virtio_mmio_devices(i,
+                                             GUEST_VIRTIO_MMIO_SPI_FIRST + i,
+                                             base);
     }
 }
 
@@ -101,6 +103,7 @@ static void xen_init_ram(MachineState *machine)
     MemoryRegion *sysmem = get_system_memory();
     ram_addr_t block_len, ram_size[GUEST_RAM_BANKS];
 
+    trace_xen_init_ram(machine->ram_size);
     if (machine->ram_size <= GUEST_RAM0_SIZE) {
         ram_size[0] = machine->ram_size;
         ram_size[1] = 0;
@@ -117,15 +120,10 @@ static void xen_init_ram(MachineState *machine)
     memory_region_init_alias(&ram_lo, NULL, "xen.ram.lo", &ram_memory,
                              GUEST_RAM0_BASE, ram_size[0]);
     memory_region_add_subregion(sysmem, GUEST_RAM0_BASE, &ram_lo);
-    DPRINTF("Initialized region xen.ram.lo: base 0x%llx size 0x%lx\n",
-            GUEST_RAM0_BASE, ram_size[0]);
-
     if (ram_size[1] > 0) {
         memory_region_init_alias(&ram_hi, NULL, "xen.ram.hi", &ram_memory,
                                  GUEST_RAM1_BASE, ram_size[1]);
         memory_region_add_subregion(sysmem, GUEST_RAM1_BASE, &ram_hi);
-        DPRINTF("Initialized region xen.ram.hi: base 0x%llx size 0x%lx\n",
-                GUEST_RAM1_BASE, ram_size[1]);
     }
 }
 
@@ -158,7 +156,7 @@ static void xen_enable_tpm(XenArmState *xam)
 
     TPMBackend *be = qemu_find_tpm_be("tpm0");
     if (be == NULL) {
-        DPRINTF("Couldn't fine the backend for tpm0\n");
+        error_report("Couldn't find tmp0 backend");
         return;
     }
     dev = qdev_new(TYPE_TPM_TIS_SYSBUS);
@@ -168,7 +166,7 @@ static void xen_enable_tpm(XenArmState *xam)
     sysbus_realize_and_unref(busdev, &error_fatal);
     sysbus_mmio_map(busdev, 0, xam->cfg.tpm_base_addr);
 
-    DPRINTF("Connected tpmdev at address 0x%lx\n", xam->cfg.tpm_base_addr);
+    trace_xen_enable_tpm(xam->cfg.tpm_base_addr);
 }
 #endif
 
@@ -179,8 +177,9 @@ static void xen_arm_init(MachineState *machine)
     xam->state =  g_new0(XenIOState, 1);
 
     if (machine->ram_size == 0) {
-        DPRINTF("ram_size not specified. QEMU machine started without IOREQ"
-                "(no emulated devices including Virtio)\n");
+        warn_report("%s non-zero ram size not specified. QEMU machine started"
+                    " without IOREQ (no emulated devices including virtio)",
+                    MACHINE_CLASS(object_get_class(OBJECT(machine)))->desc);
         return;
     }
 
@@ -194,7 +193,7 @@ static void xen_arm_init(MachineState *machine)
     if (xam->cfg.tpm_base_addr) {
         xen_enable_tpm(xam);
     } else {
-        DPRINTF("tpm-base-addr is not provided. TPM will not be enabled\n");
+        warn_report("tpm-base-addr is not provided. TPM will not be enabled");
     }
 #endif
 }
-- 
γαῖα πυρί μιχθήτω



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 3/5] hw/xen/xen-mapcache.c: convert DPRINTF to tracepoints
  2024-01-19 13:24 [PATCH v2 0/5] hw/{arm,xen} convert printfs to trace/reports Manos Pitsidianakis
  2024-01-19 13:24 ` [PATCH v2 1/5] hw/arm/z2: convert DPRINTF to trace events and guest errors Manos Pitsidianakis
  2024-01-19 13:24 ` [PATCH v2 2/5] hw/arm/xen_arm.c: convert DPRINTF to trace events and error/warn reports Manos Pitsidianakis
@ 2024-01-19 13:24 ` Manos Pitsidianakis
  2024-01-19 13:24 ` [PATCH v2 4/5] hw/xen/xen-hvm-common.c: " Manos Pitsidianakis
  2024-01-19 13:24 ` [PATCH v2 5/5] hw/xen: convert stderr prints to error/warn reports Manos Pitsidianakis
  4 siblings, 0 replies; 8+ messages in thread
From: Manos Pitsidianakis @ 2024-01-19 13:24 UTC (permalink / raw)
  To: qemu-devel
  Cc: Philippe Mathieu-Daudé,
	qemu-arm, Paolo Bonzini, Alex Bennée, Stefano Stabellini,
	Anthony Perard, Paul Durrant, xen-devel

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/xen/trace-events   | 11 +++++++++
 hw/xen/xen-mapcache.c | 54 +++++++++++++++++++------------------------
 2 files changed, 35 insertions(+), 30 deletions(-)

diff --git a/hw/xen/trace-events b/hw/xen/trace-events
index 67a6c41926..1b748dba09 100644
--- a/hw/xen/trace-events
+++ b/hw/xen/trace-events
@@ -60,3 +60,14 @@ cpu_ioreq_config_write(void *req, uint32_t sbdf, uint32_t reg, uint32_t size, ui
 xen_map_cache(uint64_t phys_addr) "want 0x%"PRIx64
 xen_remap_bucket(uint64_t index) "index 0x%"PRIx64
 xen_map_cache_return(void* ptr) "%p"
+xen_map_cache_init(uint64_t nr_buckets, uint64_t size) "nr_buckets = 0x%lx size %lu"
+xen_replace_cache_entry_dummy(uint64_t old_phys_addr, uint64_t new_phys_addr) "Replacing a dummy mapcache entry for 0x%"PRIx64" with 0x%"PRIx64
+xen_invalidate_map_cache_entry_unlocked_not_found(void *p) "could not find %p"
+xen_invalidate_map_cache_entry_unlocked_found(uint64_t addr, void *p) "   0x%"PRIx64" -> %p is present"
+xen_invalidate_map_cache_entry_unlocked_miss(void *buffer) "Trying to unmap address %p that is not in the mapcache"
+xen_replace_cache_entry_unlocked_could_not_update_entry(uint64_t old_phys_addr) "Unable to update a mapcache entry for 0x%"PRIx64
+xen_ram_addr_from_mapcache_not_found(void *p) "could not find %p"
+xen_ram_addr_from_mapcache_found(uint64_t addr, void *p) "   0x%"PRIx64" -> %p is present"
+xen_ram_addr_from_mapcache_not_in_cache(void *p) "Trying to find address %p that is not in the mapcache"
+xen_replace_cache_entry_unlocked(uint64_t old_phys_addr) "Trying to update an entry for 0x%"PRIx64" that is not in the mapcache"
+xen_invalidate_map_cache(uint64_t paddr_index, void *vaddr_req) "Locked DMA mapping while invalidating mapcache 0x%"PRIx64" -> %p is present"
diff --git a/hw/xen/xen-mapcache.c b/hw/xen/xen-mapcache.c
index f7d974677d..336c212376 100644
--- a/hw/xen/xen-mapcache.c
+++ b/hw/xen/xen-mapcache.c
@@ -22,16 +22,6 @@
 #include "trace.h"
 
 
-//#define MAPCACHE_DEBUG
-
-#ifdef MAPCACHE_DEBUG
-#  define DPRINTF(fmt, ...) do { \
-    fprintf(stderr, "xen_mapcache: " fmt, ## __VA_ARGS__); \
-} while (0)
-#else
-#  define DPRINTF(fmt, ...) do { } while (0)
-#endif
-
 #if HOST_LONG_BITS == 32
 #  define MCACHE_BUCKET_SHIFT 16
 #  define MCACHE_MAX_SIZE     (1UL<<31) /* 2GB Cap */
@@ -145,8 +135,7 @@ void xen_map_cache_init(phys_offset_to_gaddr_t f, void *opaque)
 
     size = mapcache->nr_buckets * sizeof (MapCacheEntry);
     size = (size + XC_PAGE_SIZE - 1) & ~(XC_PAGE_SIZE - 1);
-    DPRINTF("%s, nr_buckets = %lx size %lu\n", __func__,
-            mapcache->nr_buckets, size);
+    trace_xen_map_cache_init(mapcache->nr_buckets, size);
     mapcache->entry = g_malloc0(size);
 }
 
@@ -286,7 +275,9 @@ tryagain:
         test_bits(address_offset >> XC_PAGE_SHIFT,
                   test_bit_size >> XC_PAGE_SHIFT,
                   mapcache->last_entry->valid_mapping)) {
-        trace_xen_map_cache_return(mapcache->last_entry->vaddr_base + address_offset);
+        trace_xen_map_cache_return(
+            mapcache->last_entry->vaddr_base + address_offset
+        );
         return mapcache->last_entry->vaddr_base + address_offset;
     }
 
@@ -368,7 +359,9 @@ tryagain:
         QTAILQ_INSERT_HEAD(&mapcache->locked_entries, reventry, next);
     }
 
-    trace_xen_map_cache_return(mapcache->last_entry->vaddr_base + address_offset);
+    trace_xen_map_cache_return(
+        mapcache->last_entry->vaddr_base + address_offset
+    );
     return mapcache->last_entry->vaddr_base + address_offset;
 }
 
@@ -402,10 +395,10 @@ ram_addr_t xen_ram_addr_from_mapcache(void *ptr)
         }
     }
     if (!found) {
-        fprintf(stderr, "%s, could not find %p\n", __func__, ptr);
+        trace_xen_ram_addr_from_mapcache_not_found(ptr);
         QTAILQ_FOREACH(reventry, &mapcache->locked_entries, next) {
-            DPRINTF("   "HWADDR_FMT_plx" -> %p is present\n", reventry->paddr_index,
-                    reventry->vaddr_req);
+            trace_xen_ram_addr_from_mapcache_found(reventry->paddr_index,
+                                                   reventry->vaddr_req);
         }
         abort();
         return 0;
@@ -416,7 +409,7 @@ ram_addr_t xen_ram_addr_from_mapcache(void *ptr)
         entry = entry->next;
     }
     if (!entry) {
-        DPRINTF("Trying to find address %p that is not in the mapcache!\n", ptr);
+        trace_xen_ram_addr_from_mapcache_not_in_cache(ptr);
         raddr = 0;
     } else {
         raddr = (reventry->paddr_index << MCACHE_BUCKET_SHIFT) +
@@ -443,9 +436,12 @@ static void xen_invalidate_map_cache_entry_unlocked(uint8_t *buffer)
         }
     }
     if (!found) {
-        DPRINTF("%s, could not find %p\n", __func__, buffer);
+        trace_xen_invalidate_map_cache_entry_unlocked_not_found(buffer);
         QTAILQ_FOREACH(reventry, &mapcache->locked_entries, next) {
-            DPRINTF("   "HWADDR_FMT_plx" -> %p is present\n", reventry->paddr_index, reventry->vaddr_req);
+            trace_xen_invalidate_map_cache_entry_unlocked_found(
+                reventry->paddr_index,
+                reventry->vaddr_req
+            );
         }
         return;
     }
@@ -463,7 +459,7 @@ static void xen_invalidate_map_cache_entry_unlocked(uint8_t *buffer)
         entry = entry->next;
     }
     if (!entry) {
-        DPRINTF("Trying to unmap address %p that is not in the mapcache!\n", buffer);
+        trace_xen_invalidate_map_cache_entry_unlocked_miss(buffer);
         return;
     }
     entry->lock--;
@@ -502,9 +498,8 @@ void xen_invalidate_map_cache(void)
         if (!reventry->dma) {
             continue;
         }
-        fprintf(stderr, "Locked DMA mapping while invalidating mapcache!"
-                " "HWADDR_FMT_plx" -> %p is present\n",
-                reventry->paddr_index, reventry->vaddr_req);
+        trace_xen_invalidate_map_cache(reventry->paddr_index,
+                                       reventry->vaddr_req);
     }
 
     for (i = 0; i < mapcache->nr_buckets; i++) {
@@ -562,24 +557,23 @@ static uint8_t *xen_replace_cache_entry_unlocked(hwaddr old_phys_addr,
         entry = entry->next;
     }
     if (!entry) {
-        DPRINTF("Trying to update an entry for "HWADDR_FMT_plx \
-                "that is not in the mapcache!\n", old_phys_addr);
+        trace_xen_replace_cache_entry_unlocked(old_phys_addr);
         return NULL;
     }
 
     address_index  = new_phys_addr >> MCACHE_BUCKET_SHIFT;
     address_offset = new_phys_addr & (MCACHE_BUCKET_SIZE - 1);
 
-    fprintf(stderr, "Replacing a dummy mapcache entry for "HWADDR_FMT_plx \
-            " with "HWADDR_FMT_plx"\n", old_phys_addr, new_phys_addr);
+    trace_xen_replace_cache_entry_dummy(old_phys_addr, new_phys_addr);
 
     xen_remap_bucket(entry, entry->vaddr_base,
                      cache_size, address_index, false);
     if (!test_bits(address_offset >> XC_PAGE_SHIFT,
                 test_bit_size >> XC_PAGE_SHIFT,
                 entry->valid_mapping)) {
-        DPRINTF("Unable to update a mapcache entry for "HWADDR_FMT_plx"!\n",
-                old_phys_addr);
+        trace_xen_replace_cache_entry_unlocked_could_not_update_entry(
+            old_phys_addr
+        );
         return NULL;
     }
 
-- 
γαῖα πυρί μιχθήτω



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 4/5] hw/xen/xen-hvm-common.c: convert DPRINTF to tracepoints
  2024-01-19 13:24 [PATCH v2 0/5] hw/{arm,xen} convert printfs to trace/reports Manos Pitsidianakis
                   ` (2 preceding siblings ...)
  2024-01-19 13:24 ` [PATCH v2 3/5] hw/xen/xen-mapcache.c: convert DPRINTF to tracepoints Manos Pitsidianakis
@ 2024-01-19 13:24 ` Manos Pitsidianakis
  2024-01-19 13:24 ` [PATCH v2 5/5] hw/xen: convert stderr prints to error/warn reports Manos Pitsidianakis
  4 siblings, 0 replies; 8+ messages in thread
From: Manos Pitsidianakis @ 2024-01-19 13:24 UTC (permalink / raw)
  To: qemu-devel
  Cc: Philippe Mathieu-Daudé,
	qemu-arm, Paolo Bonzini, Alex Bennée, Stefano Stabellini,
	Anthony Perard, Paul Durrant, xen-devel

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/xen/trace-events     | 10 +++++++++-
 hw/xen/xen-hvm-common.c | 35 ++++++++++++++++++-----------------
 2 files changed, 27 insertions(+), 18 deletions(-)

diff --git a/hw/xen/trace-events b/hw/xen/trace-events
index 1b748dba09..dd5b5a7f35 100644
--- a/hw/xen/trace-events
+++ b/hw/xen/trace-events
@@ -42,7 +42,7 @@ xs_node_vscanf(char *path, char *value) "%s %s"
 xs_node_watch(char *path) "%s"
 xs_node_unwatch(char *path) "%s"
 
-# xen-hvm.c
+# xen-hvm-common.c
 xen_ram_alloc(unsigned long ram_addr, unsigned long size) "requested: 0x%lx, size 0x%lx"
 xen_client_set_memory(uint64_t start_addr, unsigned long size, bool log_dirty) "0x%"PRIx64" size 0x%lx, log_dirty %i"
 handle_ioreq(void *req, uint32_t type, uint32_t dir, uint32_t df, uint32_t data_is_ptr, uint64_t addr, uint64_t data, uint32_t count, uint32_t size) "I/O=%p type=%d dir=%d df=%d ptr=%d port=0x%"PRIx64" data=0x%"PRIx64" count=%d size=%d"
@@ -55,6 +55,14 @@ cpu_ioreq_move(void *req, uint32_t dir, uint32_t df, uint32_t data_is_ptr, uint6
 xen_map_resource_ioreq(uint32_t id, void *addr) "id: %u addr: %p"
 cpu_ioreq_config_read(void *req, uint32_t sbdf, uint32_t reg, uint32_t size, uint32_t data) "I/O=%p sbdf=0x%x reg=%u size=%u data=0x%x"
 cpu_ioreq_config_write(void *req, uint32_t sbdf, uint32_t reg, uint32_t size, uint32_t data) "I/O=%p sbdf=0x%x reg=%u size=%u data=0x%x"
+cpu_get_ioreq_from_shared_memory_req_not_ready(int state, int data_is_ptr, uint64_t addr, uint64_t data, uint32_t count, uint32_t size) "I/O request not ready: 0x%x, ptr: 0x%x, port: 0x%"PRIx64", data: 0x%"PRIx64", count: %u, size: %u"
+xen_main_loop_prepare_init_cpu(int id, void *cpu) "cpu_by_vcpu_id[%d]=%p"
+xen_map_ioreq_server_shared_page(long unsigned int ioreq_pfn) "shared page at pfn 0x%lx"
+xen_map_ioreq_server_buffered_io_page(long unsigned int ioreq_pfn) "buffered io page at pfn 0x%lx"
+xen_map_ioreq_server_buffered_io_evtchn(int bufioreq_evtchn) "buffered io evtchn is 0x%x"
+destroy_hvm_domain_cannot_acquire_handle(void) "Cannot acquire xenctrl handle"
+destroy_hvm_domain_failed_action(const char *action, int sts, char *errno_s) "xc_domain_shutdown failed to issue %s, sts %d, %s"
+destroy_hvm_domain_action(int xen_domid, const char *action) "Issued domain %d %s"
 
 # xen-mapcache.c
 xen_map_cache(uint64_t phys_addr) "want 0x%"PRIx64
diff --git a/hw/xen/xen-hvm-common.c b/hw/xen/xen-hvm-common.c
index 47e6cb1db3..05a29c6f11 100644
--- a/hw/xen/xen-hvm-common.c
+++ b/hw/xen/xen-hvm-common.c
@@ -169,11 +169,12 @@ static ioreq_t *cpu_get_ioreq_from_shared_memory(XenIOState *state, int vcpu)
     ioreq_t *req = xen_vcpu_ioreq(state->shared_page, vcpu);
 
     if (req->state != STATE_IOREQ_READY) {
-        DPRINTF("I/O request not ready: "
-                "%x, ptr: %x, port: %"PRIx64", "
-                "data: %"PRIx64", count: %u, size: %u\n",
-                req->state, req->data_is_ptr, req->addr,
-                req->data, req->count, req->size);
+        trace_cpu_get_ioreq_from_shared_memory_req_not_ready(req->state,
+                                                             req->data_is_ptr,
+                                                             req->addr,
+                                                             req->data,
+                                                             req->count,
+                                                             req->size);
         return NULL;
     }
 
@@ -601,10 +602,9 @@ static void xen_main_loop_prepare(XenIOState *state)
     if (evtchn_fd != -1) {
         CPUState *cpu_state;
 
-        DPRINTF("%s: Init cpu_by_vcpu_id\n", __func__);
         CPU_FOREACH(cpu_state) {
-            DPRINTF("%s: cpu_by_vcpu_id[%d]=%p\n",
-                    __func__, cpu_state->cpu_index, cpu_state);
+            trace_xen_main_loop_prepare_init_cpu(cpu_state->cpu_index,
+                                                 cpu_state);
             state->cpu_by_vcpu_id[cpu_state->cpu_index] = cpu_state;
         }
         qemu_set_fd_handler(evtchn_fd, cpu_handle_ioreq, NULL, state);
@@ -681,7 +681,7 @@ static int xen_map_ioreq_server(XenIOState *state)
     }
 
     if (state->shared_page == NULL) {
-        DPRINTF("shared page at pfn %lx\n", ioreq_pfn);
+        trace_xen_map_ioreq_server_shared_page(ioreq_pfn);
 
         state->shared_page = xenforeignmemory_map(xen_fmem, xen_domid,
                                                   PROT_READ | PROT_WRITE,
@@ -693,7 +693,7 @@ static int xen_map_ioreq_server(XenIOState *state)
     }
 
     if (state->buffered_io_page == NULL) {
-        DPRINTF("buffered io page at pfn %lx\n", bufioreq_pfn);
+        trace_xen_map_ioreq_server_buffered_io_page(bufioreq_pfn);
 
         state->buffered_io_page = xenforeignmemory_map(xen_fmem, xen_domid,
                                                        PROT_READ | PROT_WRITE,
@@ -709,7 +709,7 @@ static int xen_map_ioreq_server(XenIOState *state)
         return -1;
     }
 
-    DPRINTF("buffered io evtchn is %x\n", bufioreq_evtchn);
+    trace_xen_map_ioreq_server_buffered_io_evtchn(bufioreq_evtchn);
 
     state->bufioreq_remote_port = bufioreq_evtchn;
 
@@ -737,16 +737,17 @@ void destroy_hvm_domain(bool reboot)
 
     xc_handle = xc_interface_open(0, 0, 0);
     if (xc_handle == NULL) {
-        fprintf(stderr, "Cannot acquire xenctrl handle\n");
+        trace_destroy_hvm_domain_cannot_acquire_handle();
     } else {
         sts = xc_domain_shutdown(xc_handle, xen_domid, reason);
         if (sts != 0) {
-            fprintf(stderr, "xc_domain_shutdown failed to issue %s, "
-                    "sts %d, %s\n", reboot ? "reboot" : "poweroff",
-                    sts, strerror(errno));
+            trace_destroy_hvm_domain_failed_action(
+                reboot ? "reboot" : "poweroff", sts, strerror(errno)
+            );
         } else {
-            fprintf(stderr, "Issued domain %d %s\n", xen_domid,
-                    reboot ? "reboot" : "poweroff");
+            trace_destroy_hvm_domain_action(
+                xen_domid, reboot ? "reboot" : "poweroff"
+            );
         }
         xc_interface_close(xc_handle);
     }
-- 
γαῖα πυρί μιχθήτω



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 5/5] hw/xen: convert stderr prints to error/warn reports
  2024-01-19 13:24 [PATCH v2 0/5] hw/{arm,xen} convert printfs to trace/reports Manos Pitsidianakis
                   ` (3 preceding siblings ...)
  2024-01-19 13:24 ` [PATCH v2 4/5] hw/xen/xen-hvm-common.c: " Manos Pitsidianakis
@ 2024-01-19 13:24 ` Manos Pitsidianakis
  2024-01-19 17:04   ` Philippe Mathieu-Daudé
  4 siblings, 1 reply; 8+ messages in thread
From: Manos Pitsidianakis @ 2024-01-19 13:24 UTC (permalink / raw)
  To: qemu-devel
  Cc: Philippe Mathieu-Daudé,
	qemu-arm, Paolo Bonzini, Alex Bennée, Stefano Stabellini,
	Anthony Perard, Paul Durrant, xen-devel

According to the QEMU Coding Style document:

> Do not use printf(), fprintf() or monitor_printf(). Instead, use
> error_report() or error_vreport() from error-report.h. This ensures the
> error is reported in the right place (current monitor or stderr), and in
> a uniform format.
> Use error_printf() & friends to print additional information.

This commit changes fprintfs that report warnings and errors to the
appropriate report functions.

Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
---
 hw/xen/xen-hvm-common.c | 12 ++++++------
 hw/xen/xen-mapcache.c   |  5 ++---
 2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/hw/xen/xen-hvm-common.c b/hw/xen/xen-hvm-common.c
index 05a29c6f11..baa1adb9f2 100644
--- a/hw/xen/xen-hvm-common.c
+++ b/hw/xen/xen-hvm-common.c
@@ -20,8 +20,8 @@ void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size, MemoryRegion *mr,
 
     if (runstate_check(RUN_STATE_INMIGRATE)) {
         /* RAM already populated in Xen */
-        fprintf(stderr, "%s: do not alloc "RAM_ADDR_FMT
-                " bytes of ram at "RAM_ADDR_FMT" when runstate is INMIGRATE\n",
+        warn_report("%s: do not alloc "RAM_ADDR_FMT
+                " bytes of ram at "RAM_ADDR_FMT" when runstate is INMIGRATE",
                 __func__, size, ram_addr);
         return;
     }
@@ -552,9 +552,9 @@ static void cpu_handle_ioreq(void *opaque)
         req->data = copy.data;
 
         if (req->state != STATE_IOREQ_INPROCESS) {
-            fprintf(stderr, "Badness in I/O request ... not in service?!: "
+            warn_report("Badness in I/O request ... not in service?!: "
                     "%x, ptr: %x, port: %"PRIx64", "
-                    "data: %"PRIx64", count: %u, size: %u, type: %u\n",
+                    "data: %"PRIx64", count: %u, size: %u, type: %u",
                     req->state, req->data_is_ptr, req->addr,
                     req->data, req->count, req->size, req->type);
             destroy_hvm_domain(false);
@@ -758,9 +758,9 @@ void xen_shutdown_fatal_error(const char *fmt, ...)
     va_list ap;
 
     va_start(ap, fmt);
-    vfprintf(stderr, fmt, ap);
+    error_vreport(fmt, ap);
     va_end(ap);
-    fprintf(stderr, "Will destroy the domain.\n");
+    error_report("Will destroy the domain.");
     /* destroy the domain */
     qemu_system_shutdown_request(SHUTDOWN_CAUSE_HOST_ERROR);
 }
diff --git a/hw/xen/xen-mapcache.c b/hw/xen/xen-mapcache.c
index 336c212376..4f956d048e 100644
--- a/hw/xen/xen-mapcache.c
+++ b/hw/xen/xen-mapcache.c
@@ -347,9 +347,8 @@ tryagain:
         MapCacheRev *reventry = g_new0(MapCacheRev, 1);
         entry->lock++;
         if (entry->lock == 0) {
-            fprintf(stderr,
-                    "mapcache entry lock overflow: "HWADDR_FMT_plx" -> %p\n",
-                    entry->paddr_index, entry->vaddr_base);
+            error_report("mapcache entry lock overflow: "HWADDR_FMT_plx" -> %p",
+                         entry->paddr_index, entry->vaddr_base);
             abort();
         }
         reventry->dma = dma;
-- 
γαῖα πυρί μιχθήτω



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 5/5] hw/xen: convert stderr prints to error/warn reports
  2024-01-19 13:24 ` [PATCH v2 5/5] hw/xen: convert stderr prints to error/warn reports Manos Pitsidianakis
@ 2024-01-19 17:04   ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 8+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-01-19 17:04 UTC (permalink / raw)
  To: Manos Pitsidianakis, qemu-devel
  Cc: qemu-arm, Paolo Bonzini, Alex Bennée, Stefano Stabellini,
	Anthony Perard, Paul Durrant, xen-devel

On 19/1/24 14:24, Manos Pitsidianakis wrote:
> According to the QEMU Coding Style document:
> 
>> Do not use printf(), fprintf() or monitor_printf(). Instead, use
>> error_report() or error_vreport() from error-report.h. This ensures the
>> error is reported in the right place (current monitor or stderr), and in
>> a uniform format.
>> Use error_printf() & friends to print additional information.
> 
> This commit changes fprintfs that report warnings and errors to the
> appropriate report functions.
> 
> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> ---
>   hw/xen/xen-hvm-common.c | 12 ++++++------
>   hw/xen/xen-mapcache.c   |  5 ++---
>   2 files changed, 8 insertions(+), 9 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/5] hw/arm/z2: convert DPRINTF to trace events and guest errors
  2024-01-19 13:24 ` [PATCH v2 1/5] hw/arm/z2: convert DPRINTF to trace events and guest errors Manos Pitsidianakis
@ 2024-01-22 18:20   ` Peter Maydell
  0 siblings, 0 replies; 8+ messages in thread
From: Peter Maydell @ 2024-01-22 18:20 UTC (permalink / raw)
  To: Manos Pitsidianakis
  Cc: qemu-devel, Philippe Mathieu-Daudé,
	qemu-arm, Paolo Bonzini, Alex Bennée

On Fri, 19 Jan 2024 at 13:24, Manos Pitsidianakis
<manos.pitsidianakis@linaro.org> wrote:
>
> Tracing DPRINTFs to stderr might not be desired. A developer that relies
> on trace events should be able to opt-in to each trace event 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 trace events. DPRINTFs that report guest errors are logged with
> LOG_GUEST_ERROR.
>
> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> ---
>  hw/arm/strongarm.c  | 55 ++++++++++++++++++++++-----------------------
>  hw/arm/trace-events | 10 +++++++++
>  hw/arm/z2.c         | 27 +++++++---------------

Hi; thanks for sending these patches.

The strongarm.c and z2.c files aren't related (z2 uses
the pxa2xx CPU, not strongarm), so I think these are better
as two separate patches.

>  3 files changed, 45 insertions(+), 47 deletions(-)
>
> diff --git a/hw/arm/strongarm.c b/hw/arm/strongarm.c
> index fef3638aca..9fca0b302c 100644
> --- a/hw/arm/strongarm.c
> +++ b/hw/arm/strongarm.c
> @@ -46,8 +46,7 @@
>  #include "qemu/cutils.h"
>  #include "qemu/log.h"
>  #include "qom/object.h"
> -
> -//#define DEBUG
> +#include "trace.h"
>
>  /*
>   TODO
> @@ -66,12 +65,6 @@
>   - Enhance UART with modem signals
>   */
>
> -#ifdef DEBUG
> -# define DPRINTF(format, ...) printf(format , ## __VA_ARGS__)
> -#else
> -# define DPRINTF(format, ...) do { } while (0)
> -#endif
> -
>  static struct {
>      hwaddr io_base;
>      int irq;
> @@ -151,8 +144,9 @@ static uint64_t strongarm_pic_mem_read(void *opaque, hwaddr offset,
>      case ICPR:
>          return s->pending;
>      default:
> -        printf("%s: Bad register offset 0x" HWADDR_FMT_plx "\n",
> -                        __func__, offset);
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "Bad pic mem register read offset 0x%zu",
> +                      offset);

Message strings for qemu_log_mask() need a trailing "\n"
(unlike trace-event strings).

'offset' is type 'hwaddr', so the correct format string is
HWADDR_FMT_plx as the printf already has, not %zu. (Watch out
that the HWADDR_FMT_* macros include the "%", unlike the
POSIX style PRIx64 etc macros.) Getting the format string
wrong can cause compilation failures on some hosts (eg
32-bit hosts where the size_t etc types are different sizes.)

It's nice with these error messages to be a bit more
precise about the device that's produced them than
just "pic mem". In this case you could say "strongarm_pic".
(The 'mem' isn't really part of the device name.) Or
we are often lazy and use __func__.

Similarly with the other qemu_log_mask() calls below.

>          return 0;
>      }
>  }

> @@ -1029,8 +1024,11 @@ static void strongarm_uart_update_parameters(StrongARMUARTState *s)
>      s->char_transmit_time =  (NANOSECONDS_PER_SECOND / speed) * frame_size;
>      qemu_chr_fe_ioctl(&s->chr, CHR_IOCTL_SERIAL_SET_PARAMS, &ssp);
>
> -    DPRINTF(stderr, "%s speed=%d parity=%c data=%d stop=%d\n", s->chr->label,
> -            speed, parity, data_bits, stop_bits);
> +    trace_strongarm_uart_update_parameters(s->chr.chr->label?:"NULL",

Something's gone wrong here. The old code had 's->chr->label'
and the new code has got an extra '.chr' in it from somewhere.

(Did this really compile ? Check your configure options were
such that the file is being recompiled and the trace events
are being emitted.)

The code needs to handle s->chr being NULL (this will happen
if you start a strongarm machine with the command line argument
"-serial none").

> +                                           speed,
> +                                           parity,
> +                                           data_bits,
> +                                           stop_bits);
>  }
>

> @@ -1458,8 +1456,9 @@ static void strongarm_ssp_write(void *opaque, hwaddr addr,
>      case SSCR0:
>          s->sscr[0] = value & 0xffbf;
>          if ((s->sscr[0] & SSCR0_SSE) && SSCR0_DSS(value) < 4) {
> -            printf("%s: Wrong data size: %i bits\n", __func__,
> -                   (int)SSCR0_DSS(value));
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "Wrong data size: %i bits",
> +                          (int)SSCR0_DSS(value));

This message has no indication at all of what device is producing it.

>          }
>          if (!(value & SSCR0_SSE)) {
>              s->sssr = 0;

> diff --git a/hw/arm/trace-events b/hw/arm/trace-events
> index cdc1ea06a8..b0a56fcdc6 100644
> --- a/hw/arm/trace-events
> +++ b/hw/arm/trace-events
> @@ -55,3 +55,13 @@ 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_reg_update(uint8_t cur, uint8_t _0, uint8_t _1, uint8_t _2, uint32_t value) "cur_reg = 0x%xd, buf = [0x%xd, 0x%xd, 0x%xd], value = 0x%x"

Don't use argument names starting with underscores; those are
reserved for the compiler and the system. Here I would
suggest buf0, buf1, buf2.

"%xd" will print the number in hex followed by a literal 'd' character.
For hex numbers, just "0x%x" is sufficient. (Look at other
trace lines in this file for examples.)

> +z2_lcd_enable_disable_result(const char * result) "LCD %s"

No space after the '*' in the argument type.

> +z2_aer915_send_too_long(int8_t msg) "message too long (%i bytes)"
> +z2_aer915_send(uint8_t reg, uint8_t value) "reg %d value 0x%02x"
> +z2_aer915_event(int8_t event, int8_t len) "i2c event =0x%x len=%d bytes"
> +
> +# strongarm.c
> +strongarm_uart_update_parameters(const char *label, int speed, char parity, int data_bits, int stop_bits) "%s speed=%d parity=%c data=%d stop=%d"
> +strongarm_ssp_read_underrun(void) "SSP rx underrun"

thanks
-- PMM


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2024-01-22 18:21 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-01-19 13:24 [PATCH v2 0/5] hw/{arm,xen} convert printfs to trace/reports Manos Pitsidianakis
2024-01-19 13:24 ` [PATCH v2 1/5] hw/arm/z2: convert DPRINTF to trace events and guest errors Manos Pitsidianakis
2024-01-22 18:20   ` Peter Maydell
2024-01-19 13:24 ` [PATCH v2 2/5] hw/arm/xen_arm.c: convert DPRINTF to trace events and error/warn reports Manos Pitsidianakis
2024-01-19 13:24 ` [PATCH v2 3/5] hw/xen/xen-mapcache.c: convert DPRINTF to tracepoints Manos Pitsidianakis
2024-01-19 13:24 ` [PATCH v2 4/5] hw/xen/xen-hvm-common.c: " Manos Pitsidianakis
2024-01-19 13:24 ` [PATCH v2 5/5] hw/xen: convert stderr prints to error/warn reports Manos Pitsidianakis
2024-01-19 17:04   ` Philippe Mathieu-Daudé

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.