| Message ID | 20170313195547.21466-4-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <eblake@redhat.com> > --- > hw/i386/amd_iommu.c | 3 +-- > hw/i386/trace-events | 1 + > 2 files changed, 2 insertions(+), 2 deletions(-) Thanks, applied to my tracing-next tree: https://github.com/stefanha/qemu/commits/tracing-next Stefan
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 <eblake@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
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" 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
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 <eblake@redhat.com> --- hw/i386/amd_iommu.c | 3 +-- hw/i386/trace-events | 1 + 2 files changed, 2 insertions(+), 2 deletions(-)