[Qemu-devel] [PATCH for-1.6] memory: add tracepoints for MMIO reads/writes

[Qemu-devel] [PATCH for-1.6] memory: add tracepoints for MMIO reads/writes

[Paolo Bonzini]

This is quite handy to debug softmmu targets.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 memory.c     | 5 +++++
 trace-events | 4 ++++
 2 files changed, 9 insertions(+)

diff --git a/memory.c b/memory.c
index 1494e95..ac6f3c6 100644
--- a/memory.c
+++ b/memory.c
@@ -19,6 +19,7 @@
 #include "qemu/bitops.h"
 #include "qom/object.h"
 #include "sysemu/kvm.h"
+#include "trace.h"
 #include <assert.h>
 
 #include "exec/memory-internal.h"
@@ -388,6 +389,7 @@ static void memory_region_oldmmio_read_accessor(MemoryRegion *mr,
     uint64_t tmp;
 
     tmp = mr->ops->old_mmio.read[ctz32(size)](mr->opaque, addr);
+    trace_memory_region_ops_read(mr, addr, tmp, size);
     *value |= (tmp & mask) << shift;
 }
 
@@ -404,6 +406,7 @@ static void memory_region_read_accessor(MemoryRegion *mr,
         qemu_flush_coalesced_mmio_buffer();
     }
     tmp = mr->ops->read(mr->opaque, addr, size);
+    trace_memory_region_ops_read(mr, addr, tmp, size);
     *value |= (tmp & mask) << shift;
 }
 
@@ -417,6 +420,7 @@ static void memory_region_oldmmio_write_accessor(MemoryRegion *mr,
     uint64_t tmp;
 
     tmp = (*value >> shift) & mask;
+    trace_memory_region_ops_write(mr, addr, tmp, size);
     mr->ops->old_mmio.write[ctz32(size)](mr->opaque, addr, tmp);
 }
 
@@ -433,6 +437,7 @@ static void memory_region_write_accessor(MemoryRegion *mr,
         qemu_flush_coalesced_mmio_buffer();
     }
     tmp = (*value >> shift) & mask;
+    trace_memory_region_ops_write(mr, addr, tmp, size);
     mr->ops->write(mr->opaque, addr, tmp, size);
 }
 
diff --git a/trace-events b/trace-events
index 002df83..3e0dd74 100644
--- a/trace-events
+++ b/trace-events
@@ -1165,6 +1165,10 @@ kvm_vm_ioctl(int type, void *arg) "type %d, arg %p"
 kvm_vcpu_ioctl(int cpu_index, int type, void *arg) "cpu_index %d, type %d, arg %p"
 kvm_run_exit(int cpu_index, uint32_t reason) "cpu_index %d, reason %d"
 
+# memory.c
+memory_region_ops_read(void *mr, uint64_t addr, uint64_t value, unsigned size) "mr %p addr %#"PRIx64" value %#"PRIx64" size %d"
+memory_region_ops_write(void *mr, uint64_t addr, uint64_t value, unsigned size) "mr %p addr %#"PRIx64" value %#"PRIx64" size %d"
+
 # qom/object.c
 object_dynamic_cast_assert(const char *type, const char *target, const char *file, int line, const char *func) "%s->%s (%s:%d:%s)"
 object_class_dynamic_cast_assert(const char *type, const char *target, const char *file, int line, const char *func) "%s->%s (%s:%d:%s)"
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH for-1.6] memory: add tracepoints for MMIO reads/writes

[Andreas Färber]

Am 28.07.2013 14:35, schrieb Paolo Bonzini:
> This is quite handy to debug softmmu targets.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  memory.c     | 5 +++++
>  trace-events | 4 ++++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/memory.c b/memory.c
> index 1494e95..ac6f3c6 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -19,6 +19,7 @@
>  #include "qemu/bitops.h"
>  #include "qom/object.h"
>  #include "sysemu/kvm.h"
> +#include "trace.h"
>  #include <assert.h>
>  
>  #include "exec/memory-internal.h"
> @@ -388,6 +389,7 @@ static void memory_region_oldmmio_read_accessor(MemoryRegion *mr,
>      uint64_t tmp;
>  
>      tmp = mr->ops->old_mmio.read[ctz32(size)](mr->opaque, addr);
> +    trace_memory_region_ops_read(mr, addr, tmp, size);
>      *value |= (tmp & mask) << shift;
>  }
>  
> @@ -404,6 +406,7 @@ static void memory_region_read_accessor(MemoryRegion *mr,
>          qemu_flush_coalesced_mmio_buffer();
>      }
>      tmp = mr->ops->read(mr->opaque, addr, size);
> +    trace_memory_region_ops_read(mr, addr, tmp, size);
>      *value |= (tmp & mask) << shift;
>  }
>  
> @@ -417,6 +420,7 @@ static void memory_region_oldmmio_write_accessor(MemoryRegion *mr,
>      uint64_t tmp;
>  
>      tmp = (*value >> shift) & mask;
> +    trace_memory_region_ops_write(mr, addr, tmp, size);
>      mr->ops->old_mmio.write[ctz32(size)](mr->opaque, addr, tmp);
>  }
>  
> @@ -433,6 +437,7 @@ static void memory_region_write_accessor(MemoryRegion *mr,
>          qemu_flush_coalesced_mmio_buffer();
>      }
>      tmp = (*value >> shift) & mask;
> +    trace_memory_region_ops_write(mr, addr, tmp, size);
>      mr->ops->write(mr->opaque, addr, tmp, size);
>  }
>  
> diff --git a/trace-events b/trace-events
> index 002df83..3e0dd74 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -1165,6 +1165,10 @@ kvm_vm_ioctl(int type, void *arg) "type %d, arg %p"
>  kvm_vcpu_ioctl(int cpu_index, int type, void *arg) "cpu_index %d, type %d, arg %p"
>  kvm_run_exit(int cpu_index, uint32_t reason) "cpu_index %d, reason %d"
>  
> +# memory.c
> +memory_region_ops_read(void *mr, uint64_t addr, uint64_t value, unsigned size) "mr %p addr %#"PRIx64" value %#"PRIx64" size %d"
> +memory_region_ops_write(void *mr, uint64_t addr, uint64_t value, unsigned size) "mr %p addr %#"PRIx64" value %#"PRIx64" size %d"

%u would seem more correct for unsigned - if you fix that, Reviewed-by:
Andreas Färber <afaerber@suse.de> and ACK that's it's good for 1.6.

Andreas

> +
>  # qom/object.c
>  object_dynamic_cast_assert(const char *type, const char *target, const char *file, int line, const char *func) "%s->%s (%s:%d:%s)"
>  object_class_dynamic_cast_assert(const char *type, const char *target, const char *file, int line, const char *func) "%s->%s (%s:%d:%s)"
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH for-1.6] memory: add tracepoints for MMIO reads/writes

[Paolo Bonzini]

Il 28/07/2013 14:45, Andreas Färber ha scritto:
> Am 28.07.2013 14:35, schrieb Paolo Bonzini:
>> This is quite handy to debug softmmu targets.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  memory.c     | 5 +++++
>>  trace-events | 4 ++++
>>  2 files changed, 9 insertions(+)
>>
>> diff --git a/memory.c b/memory.c
>> index 1494e95..ac6f3c6 100644
>> --- a/memory.c
>> +++ b/memory.c
>> @@ -19,6 +19,7 @@
>>  #include "qemu/bitops.h"
>>  #include "qom/object.h"
>>  #include "sysemu/kvm.h"
>> +#include "trace.h"
>>  #include <assert.h>
>>  
>>  #include "exec/memory-internal.h"
>> @@ -388,6 +389,7 @@ static void memory_region_oldmmio_read_accessor(MemoryRegion *mr,
>>      uint64_t tmp;
>>  
>>      tmp = mr->ops->old_mmio.read[ctz32(size)](mr->opaque, addr);
>> +    trace_memory_region_ops_read(mr, addr, tmp, size);
>>      *value |= (tmp & mask) << shift;
>>  }
>>  
>> @@ -404,6 +406,7 @@ static void memory_region_read_accessor(MemoryRegion *mr,
>>          qemu_flush_coalesced_mmio_buffer();
>>      }
>>      tmp = mr->ops->read(mr->opaque, addr, size);
>> +    trace_memory_region_ops_read(mr, addr, tmp, size);
>>      *value |= (tmp & mask) << shift;
>>  }
>>  
>> @@ -417,6 +420,7 @@ static void memory_region_oldmmio_write_accessor(MemoryRegion *mr,
>>      uint64_t tmp;
>>  
>>      tmp = (*value >> shift) & mask;
>> +    trace_memory_region_ops_write(mr, addr, tmp, size);
>>      mr->ops->old_mmio.write[ctz32(size)](mr->opaque, addr, tmp);
>>  }
>>  
>> @@ -433,6 +437,7 @@ static void memory_region_write_accessor(MemoryRegion *mr,
>>          qemu_flush_coalesced_mmio_buffer();
>>      }
>>      tmp = (*value >> shift) & mask;
>> +    trace_memory_region_ops_write(mr, addr, tmp, size);
>>      mr->ops->write(mr->opaque, addr, tmp, size);
>>  }
>>  
>> diff --git a/trace-events b/trace-events
>> index 002df83..3e0dd74 100644
>> --- a/trace-events
>> +++ b/trace-events
>> @@ -1165,6 +1165,10 @@ kvm_vm_ioctl(int type, void *arg) "type %d, arg %p"
>>  kvm_vcpu_ioctl(int cpu_index, int type, void *arg) "cpu_index %d, type %d, arg %p"
>>  kvm_run_exit(int cpu_index, uint32_t reason) "cpu_index %d, reason %d"
>>  
>> +# memory.c
>> +memory_region_ops_read(void *mr, uint64_t addr, uint64_t value, unsigned size) "mr %p addr %#"PRIx64" value %#"PRIx64" size %d"
>> +memory_region_ops_write(void *mr, uint64_t addr, uint64_t value, unsigned size) "mr %p addr %#"PRIx64" value %#"PRIx64" size %d"
> 
> %u would seem more correct for unsigned - if you fix that, Reviewed-by:
> Andreas Färber <afaerber@suse.de> and ACK that's it's good for 1.6.

Ok, though it can only be 1/2/4/8.

Paolo

Re: [Qemu-devel] [PATCH for-1.6] memory: add tracepoints for MMIO reads/writes

[Andreas Färber]

Am 28.07.2013 14:55, schrieb Paolo Bonzini:
> Il 28/07/2013 14:45, Andreas Färber ha scritto:
>> Am 28.07.2013 14:35, schrieb Paolo Bonzini:
>>> diff --git a/trace-events b/trace-events
>>> index 002df83..3e0dd74 100644
>>> --- a/trace-events
>>> +++ b/trace-events
>>> @@ -1165,6 +1165,10 @@ kvm_vm_ioctl(int type, void *arg) "type %d, arg %p"
>>>  kvm_vcpu_ioctl(int cpu_index, int type, void *arg) "cpu_index %d, type %d, arg %p"
>>>  kvm_run_exit(int cpu_index, uint32_t reason) "cpu_index %d, reason %d"
>>>  
>>> +# memory.c
>>> +memory_region_ops_read(void *mr, uint64_t addr, uint64_t value, unsigned size) "mr %p addr %#"PRIx64" value %#"PRIx64" size %d"
>>> +memory_region_ops_write(void *mr, uint64_t addr, uint64_t value, unsigned size) "mr %p addr %#"PRIx64" value %#"PRIx64" size %d"
>>
>> %u would seem more correct for unsigned - if you fix that, Reviewed-by:
>> Andreas Färber <afaerber@suse.de> and ACK that's it's good for 1.6.
> 
> Ok, though it can only be 1/2/4/8.

I had static analysis in mind. ;)

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH for-1.6] memory: add tracepoints for MMIO reads/writes

[Peter Maydell]

On 28 July 2013 13:35, Paolo Bonzini <pbonzini@redhat.com> wrote:
> This is quite handy to debug softmmu targets.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

(not a nak, just a tangential thought)
It would be cute if you could just insert a "trace this access
and pass it through" container MemoryRegion at an arbitrary
point in the memory region hierarchy. That would let us do
"trace accesses to device X" without device X having to
have boilerplate tracing code in its read/write functions.

-- PMM

Re: [Qemu-devel] [PATCH for-1.6] memory: add tracepoints for MMIO reads/writes

[Andreas Färber]

Am 28.07.2013 16:34, schrieb Peter Maydell:
> On 28 July 2013 13:35, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> This is quite handy to debug softmmu targets.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> (not a nak, just a tangential thought)
> It would be cute if you could just insert a "trace this access
> and pass it through" container MemoryRegion at an arbitrary
> point in the memory region hierarchy. That would let us do
> "trace accesses to device X" without device X having to
> have boilerplate tracing code in its read/write functions.

The SystemTap backend for instance allows you to filter what you print
based on arguments, i.e. address and size.

Are you suggesting to make the trace points in this patch conditional on
some new mr->tracing_enabled flag in the MemoryRegion?

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH for-1.6] memory: add tracepoints for MMIO reads/writes

[Don Slutz]

On 07/28/13 08:35, Paolo Bonzini wrote:
> This is quite handy to debug softmmu targets.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   memory.c     | 5 +++++
>   trace-events | 4 ++++
>   2 files changed, 9 insertions(+)
>
> diff --git a/memory.c b/memory.c
> index 1494e95..ac6f3c6 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -19,6 +19,7 @@
>   #include "qemu/bitops.h"
>   #include "qom/object.h"
>   #include "sysemu/kvm.h"
> +#include "trace.h"
>   #include <assert.h>
>   
>   #include "exec/memory-internal.h"
> @@ -388,6 +389,7 @@ static void memory_region_oldmmio_read_accessor(MemoryRegion *mr,
>       uint64_t tmp;
>   
>       tmp = mr->ops->old_mmio.read[ctz32(size)](mr->opaque, addr);
> +    trace_memory_region_ops_read(mr, addr, tmp, size);
>       *value |= (tmp & mask) << shift;
>   }
>   
> @@ -404,6 +406,7 @@ static void memory_region_read_accessor(MemoryRegion *mr,
>           qemu_flush_coalesced_mmio_buffer();
>       }
>       tmp = mr->ops->read(mr->opaque, addr, size);
> +    trace_memory_region_ops_read(mr, addr, tmp, size);
>       *value |= (tmp & mask) << shift;
>   }
>   
> @@ -417,6 +420,7 @@ static void memory_region_oldmmio_write_accessor(MemoryRegion *mr,
>       uint64_t tmp;
>   
>       tmp = (*value >> shift) & mask;
> +    trace_memory_region_ops_write(mr, addr, tmp, size);
>       mr->ops->old_mmio.write[ctz32(size)](mr->opaque, addr, tmp);
>   }
>   
> @@ -433,6 +437,7 @@ static void memory_region_write_accessor(MemoryRegion *mr,
>           qemu_flush_coalesced_mmio_buffer();
>       }
>       tmp = (*value >> shift) & mask;
> +    trace_memory_region_ops_write(mr, addr, tmp, size);
>       mr->ops->write(mr->opaque, addr, tmp, size);
>   }
>   
> diff --git a/trace-events b/trace-events
> index 002df83..3e0dd74 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -1165,6 +1165,10 @@ kvm_vm_ioctl(int type, void *arg) "type %d, arg %p"
>   kvm_vcpu_ioctl(int cpu_index, int type, void *arg) "cpu_index %d, type %d, arg %p"
>   kvm_run_exit(int cpu_index, uint32_t reason) "cpu_index %d, reason %d"
>   
> +# memory.c
> +memory_region_ops_read(void *mr, uint64_t addr, uint64_t value, unsigned size) "mr %p addr %#"PRIx64" value %#"PRIx64" size %d"
> +memory_region_ops_write(void *mr, uint64_t addr, uint64_t value, unsigned size) "mr %p addr %#"PRIx64" value %#"PRIx64" size %d"
> +
Sorry about the late mail.  Did you mean to have the stderr trace text 
to be the same?
    -Don Slutz
>   # qom/object.c
>   object_dynamic_cast_assert(const char *type, const char *target, const char *file, int line, const char *func) "%s->%s (%s:%d:%s)"
>   object_class_dynamic_cast_assert(const char *type, const char *target, const char *file, int line, const char *func) "%s->%s (%s:%d:%s)"

Re: [Qemu-devel] [PATCH for-1.6] memory: add tracepoints for MMIO reads/writes

[Paolo Bonzini]

On 08/05/2013 05:18 PM, Don Slutz wrote:
>> diff --git a/trace-events b/trace-events
>> index 002df83..3e0dd74 100644
>> --- a/trace-events
>> +++ b/trace-events
>> @@ -1165,6 +1165,10 @@ kvm_vm_ioctl(int type, void *arg) "type %d, arg
>> %p"
>>   kvm_vcpu_ioctl(int cpu_index, int type, void *arg) "cpu_index %d,
>> type %d, arg %p"
>>   kvm_run_exit(int cpu_index, uint32_t reason) "cpu_index %d, reason %d"
>> +# memory.c
>> +memory_region_ops_read(void *mr, uint64_t addr, uint64_t value,
>> unsigned size) "mr %p addr %#"PRIx64" value %#"PRIx64" size %d"
>> +memory_region_ops_write(void *mr, uint64_t addr, uint64_t value,
>> unsigned size) "mr %p addr %#"PRIx64" value %#"PRIx64" size %d"
>> +
> Sorry about the late mail.  Did you mean to have the stderr trace text
> to be the same?

Yes, the name of the tracepoint should come before the text.

See scripts/tracetool/backend/stderr.py for the generator source code.

Paolo