Re: [PATCH 3/6] hw/arm/xen_arm.c: convert DPRINTF to tracepoints

["Alex Bennée" <alex.bennee@linaro.org>]

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 |  7 +++++++
>  hw/arm/xen_arm.c    | 26 +++++++++++++++-----------
>  2 files changed, 22 insertions(+), 11 deletions(-)
>
> diff --git a/hw/arm/trace-events b/hw/arm/trace-events
> index a6a67d5f16..e3f5d677d7 100644
> --- a/hw/arm/trace-events
> +++ b/hw/arm/trace-events
> @@ -81,3 +81,10 @@ 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"
> +
> +# 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_enable_tpm(uint64_t addr) "Connected tpmdev at address 0x%lx"
> +xen_arm_init(const char *msg) "%s"
> diff --git a/hw/arm/xen_arm.c b/hw/arm/xen_arm.c
> index a5631529d0..a024117d22 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);
>      }
>  }
>  
> @@ -117,15 +119,13 @@ 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]);
>      }

I wonder if a single trace_xen_init_ram(machine->ram_size) at the top
would be better as everything can be inferred from that.

>  }
>  
> @@ -158,7 +158,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");
> +        trace_xen_enable_tpm_not_found();

This smells like it should be an error_report (or maybe warn_report) as
its a misconfiguration the user/tools should know about.

>          return;
>      }
>      dev = qdev_new(TYPE_TPM_TIS_SYSBUS);
> @@ -168,7 +168,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 +179,11 @@ 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");
> +        trace_xen_arm_init("ram_size not specified. "
> +                           "QEMU machine started "
> +                           "without IOREQ "
> +                           "(no emulated devices"
> +                           "including Virtio)");

again at least an warn_report...

>          return;
>      }
>  
> @@ -194,7 +197,8 @@ 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");
> +        trace_xen_arm_init("tpm-base-addr is not provided."
> +                           "TPM will not be enabled");

warn_report.

>      }
>  #endif
>  }

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro