Re: [PATCH v2 1/5] hw/arm/z2: convert DPRINTF to trace events and guest errors

[Peter Maydell <peter.maydell@linaro.org>]

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