From 67adf4b39806df42b4c96377b37004de0df3a1fd Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Mon, 13 Mar 2017 14:55:18 -0500 Subject: [PATCH 1/3] trace: Fix backwards mirror_yield parameters block/trace-events lists the parameters for mirror_yield consistently with other mirror events (cnt just after s, like in mirror_before_sleep; in_flight last, like in mirror_yield_in_flight). But the callers were passing parameters in the wrong order, leading to poor trace messages, including type truncation when there are more than 4G dirty sectors involved. Broken since its introduction in commit bd48bde. While touching this, ensure that all callers use the same type (uint64_t) for cnt, as a later patch will enable the compiler to do stricter type-checking. Signed-off-by: Eric Blake Signed-off-by: Stefan Hajnoczi --- block/mirror.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/block/mirror.c b/block/mirror.c index ca4baa510a..9e2fecc15e 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -634,7 +634,8 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s) } if (s->in_flight >= MAX_IN_FLIGHT) { - trace_mirror_yield(s, s->in_flight, s->buf_free_count, -1); + trace_mirror_yield(s, UINT64_MAX, s->buf_free_count, + s->in_flight); mirror_wait_for_io(s); continue; } @@ -809,7 +810,7 @@ static void coroutine_fn mirror_run(void *opaque) s->common.iostatus == BLOCK_DEVICE_IO_STATUS_OK) { if (s->in_flight >= MAX_IN_FLIGHT || s->buf_free_count == 0 || (cnt == 0 && s->in_flight > 0)) { - trace_mirror_yield(s, s->in_flight, s->buf_free_count, cnt); + trace_mirror_yield(s, cnt, s->buf_free_count, s->in_flight); mirror_wait_for_io(s); continue; } else if (cnt != 0) { From d17e744848cac9a96c827be1f244ef9266710d8f Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Mon, 13 Mar 2017 14:55:19 -0500 Subject: [PATCH 2/3] trace: Fix incorrect megasas trace parameters hw/scsi/trace-events lists cmd as the first parameter for both megasas_iovec_overflow and megasas_iovec_underflow, but the caller was mistakenly passing cmd->iov_size twice instead of the command index. Also, trace_megasas_abort_invalid is called with parameters in the wrong order. Broken since its introduction in commit e8f943c3. Signed-off-by: Eric Blake Reviewed-by: Hannes Reinecke Signed-off-by: Stefan Hajnoczi --- hw/scsi/megasas.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c index e3d59b7c83..84b8caf901 100644 --- a/hw/scsi/megasas.c +++ b/hw/scsi/megasas.c @@ -291,7 +291,7 @@ static int megasas_map_sgl(MegasasState *s, MegasasCmd *cmd, union mfi_sgl *sgl) if (cmd->iov_size > iov_size) { trace_megasas_iovec_overflow(cmd->index, iov_size, cmd->iov_size); } else if (cmd->iov_size < iov_size) { - trace_megasas_iovec_underflow(cmd->iov_size, iov_size, cmd->iov_size); + trace_megasas_iovec_underflow(cmd->index, iov_size, cmd->iov_size); } cmd->iov_offset = 0; return 0; @@ -1924,8 +1924,8 @@ static int megasas_handle_abort(MegasasState *s, MegasasCmd *cmd) abort_ctx &= (uint64_t)0xFFFFFFFF; } if (abort_cmd->context != abort_ctx) { - trace_megasas_abort_invalid_context(cmd->index, abort_cmd->index, - abort_cmd->context); + trace_megasas_abort_invalid_context(cmd->index, abort_cmd->context, + abort_cmd->index); s->event_count++; return MFI_STAT_ABORT_NOT_POSSIBLE; } From 0d3ef78829332f2fdc323d1b625b60fe9c89119c Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Mon, 13 Mar 2017 14:55:20 -0500 Subject: [PATCH 3/3] trace: Avoid abuse of amdvi_mmio_read hw/i386/trace-events has an amdvi_mmio_read trace that is used for both normal reads (listing the register name, address, size, and offset) and for an error case (abusing the register name to show an error message, the address to show the maximum value supported, then shoehorning address and size into the size and offset parameters). The change from a wide address to a narrower size parameter could truncate a (rather-large) bogus read attempt, so it's better to create a separate dedicated trace with correct types, rather than abusing the trace mechanism. Broken since its introduction in commit d29a09c. [Change trace event argument type from hwaddr to uint64_t since user-defined types should not be used for trace events. This fixes a build failure with LTTng UST. --Stefan] Signed-off-by: Eric Blake Signed-off-by: Stefan Hajnoczi --- hw/i386/amd_iommu.c | 3 +-- hw/i386/trace-events | 1 + 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c index e0732ccaf1..f86a40aa30 100644 --- a/hw/i386/amd_iommu.c +++ b/hw/i386/amd_iommu.c @@ -572,8 +572,7 @@ static uint64_t amdvi_mmio_read(void *opaque, hwaddr addr, unsigned size) uint64_t val = -1; if (addr + size > AMDVI_MMIO_SIZE) { - trace_amdvi_mmio_read("error: addr outside region: max ", - (uint64_t)AMDVI_MMIO_SIZE, addr, size); + trace_amdvi_mmio_read_invalid(AMDVI_MMIO_SIZE, addr, size); return (uint64_t)-1; } diff --git a/hw/i386/trace-events b/hw/i386/trace-events index 88ad5e4c43..baed874a80 100644 --- a/hw/i386/trace-events +++ b/hw/i386/trace-events @@ -37,6 +37,7 @@ amdvi_cache_update(uint16_t domid, uint8_t bus, uint8_t slot, uint8_t func, uint amdvi_completion_wait_fail(uint64_t addr) "error: fail to write at address 0x%"PRIx64 amdvi_mmio_write(const char *reg, uint64_t addr, unsigned size, uint64_t val, uint64_t offset) "%s write addr 0x%"PRIx64", size %u, val 0x%"PRIx64", offset 0x%"PRIx64 amdvi_mmio_read(const char *reg, uint64_t addr, unsigned size, uint64_t offset) "%s read addr 0x%"PRIx64", size %u offset 0x%"PRIx64 +amdvi_mmio_read_invalid(int max, uint64_t addr, unsigned size) "error: addr outside region (max 0x%x): read addr 0x%" PRIx64 ", size %u" amdvi_command_error(uint64_t status) "error: Executing commands with command buffer disabled 0x%"PRIx64 amdvi_command_read_fail(uint64_t addr, uint32_t head) "error: fail to access memory at 0x%"PRIx64" + 0x%"PRIx32 amdvi_command_exec(uint32_t head, uint32_t tail, uint64_t buf) "command buffer head at 0x%"PRIx32" command buffer tail at 0x%"PRIx32" command buffer base at 0x%"PRIx64