Re: [Qemu-devel] [PATCH v2 03/30] trace: Avoid abuse of amdvi_mmio_read

[Stefan Hajnoczi]

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 <address@hidden>
> ---
>  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