On Fri, 19 Jan 2024 at 13:24, Manos Pitsidianakis
<manos.pitsidiana...@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.pitsidiana...@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
