On Mon, Mar 13, 2017 at 02:55:20PM -0500, Eric Blake wrote:
> 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.
>
> Signed-off-by: Eric Blake <ebl...@redhat.com>
> ---
> 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 e0732cc..f86a40a 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 88ad5e4..a213bfd 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, hwaddr addr, unsigned size) "error: addr
> outside region (max 0x%x): read addr 0x%" HWADDR_PRIx ", size %u"
This breaks the ust backend because hwaddr isn't defined.
docs/tracing.txt documents restrictions on types:
Trace events should use types as follows:
* Use stdint.h types for fixed-size types. Most offsets and guest memory
addresses are best represented with uint32_t or uint64_t. Use fixed-size
types over primitive types whose size may change depending on the host
(32-bit versus 64-bit) so trace events don't truncate values or break
the build.
* Use void * for pointers to structs or for arrays. The trace.h header
cannot include all user-defined struct declarations and it is therefore
necessary to use void * for pointers to structs.
* For everything else, use primitive scalar types (char, int, long) with the
appropriate signedness.
I think it is technically possible to allow user-defined types but it
would require full testing with all backends. The tracetool generator
for some backends will require tweaks to teach them about user-defined
types (e.g. #include "qemu-common.h").
I have merged this patch and changed hwaddr to uint64_t, HWADDR_PRIx to
PRIx64.
Stefan
signature.asc
Description: PGP signature
