diff mbox

[1/2] trace: include CPU index in trace_memory_region_ops_*()

Message ID 1455744555-22101-1-git-send-email-hollis_blanchard@mentor.com (mailing list archive)
State New, archived
Headers show

Commit Message

Hollis Blanchard Feb. 17, 2016, 9:29 p.m. UTC 
Knowing which CPU performed an action is essential for understanding SMP guest
behavior.

However, cpu_physical_memory_rw() may be executed by a machine init function,
before any VCPUs are running, when there is no CPU running ('current_cpu' is
NULL). In this case, store -1 in the trace record as the CPU index. Trace
analysis tools may need to be aware of this special case.

Signed-off-by: Hollis Blanchard <hollis_blanchard@mentor.com>
---
 memory.c     | 48 ++++++++++++++++++++++++++++++++++++------------
 trace-events |  8 ++++----
 2 files changed, 40 insertions(+), 16 deletions(-)

Comments

Stefan Hajnoczi Feb. 24, 2016, 2:12 p.m. UTC | #1 
On Wed, Feb 17, 2016 at 01:29:14PM -0800, Hollis Blanchard wrote:
> Knowing which CPU performed an action is essential for understanding SMP guest
> behavior.
> 
> However, cpu_physical_memory_rw() may be executed by a machine init function,
> before any VCPUs are running, when there is no CPU running ('current_cpu' is
> NULL). In this case, store -1 in the trace record as the CPU index. Trace
> analysis tools may need to be aware of this special case.
> 
> Signed-off-by: Hollis Blanchard <hollis_blanchard@mentor.com>
> ---
>  memory.c     | 48 ++++++++++++++++++++++++++++++++++++------------
>  trace-events |  8 ++++----
>  2 files changed, 40 insertions(+), 16 deletions(-)
> 
> diff --git a/memory.c b/memory.c
> index 2d87c21..6ae7bae 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -395,13 +395,17 @@ static MemTxResult memory_region_oldmmio_read_accessor(MemoryRegion *mr,
>                                                         MemTxAttrs attrs)
>  {
>      uint64_t tmp;
> +    int cpu_index = -1;
> +
> +    if (current_cpu)
> +        cpu_index = current_cpu->cpu_index;

QEMU coding style always uses curly braces, even when the if statement
body only has one line.

Cases like these should be caught by scripts/checkpatch.pl.  I use a git
hook to run it automatically on commit:
http://blog.vmsplice.net/2011/03/how-to-automatically-run-checkpatchpl.html


A helper function would avoid the code duplication throughout this patch:

static int get_cpu_index(void) {
    if (current_cpu) {
        return current_cpu->cpu_index;
    }
    return -1;
}
diff mbox

Patch

diff --git a/memory.c b/memory.c
index 2d87c21..6ae7bae 100644
--- a/memory.c
+++ b/memory.c
@@ -395,13 +395,17 @@  static MemTxResult memory_region_oldmmio_read_accessor(MemoryRegion *mr,
                                                        MemTxAttrs attrs)
 {
     uint64_t tmp;
+    int cpu_index = -1;
+
+    if (current_cpu)
+        cpu_index = current_cpu->cpu_index;
 
     tmp = mr->ops->old_mmio.read[ctz32(size)](mr->opaque, addr);
     if (mr->subpage) {
-        trace_memory_region_subpage_read(mr, addr, tmp, size);
+        trace_memory_region_subpage_read(cpu_index, mr, addr, tmp, size);
     } else if (TRACE_MEMORY_REGION_OPS_READ_ENABLED) {
         hwaddr abs_addr = memory_region_to_absolute_addr(mr, addr);
-        trace_memory_region_ops_read(mr, abs_addr, tmp, size);
+        trace_memory_region_ops_read(cpu_index, mr, abs_addr, tmp, size);
     }
     *value |= (tmp & mask) << shift;
     return MEMTX_OK;
@@ -416,13 +420,17 @@  static MemTxResult  memory_region_read_accessor(MemoryRegion *mr,
                                                 MemTxAttrs attrs)
 {
     uint64_t tmp;
+    int cpu_index = -1;
+
+    if (current_cpu)
+        cpu_index = current_cpu->cpu_index;
 
     tmp = mr->ops->read(mr->opaque, addr, size);
     if (mr->subpage) {
-        trace_memory_region_subpage_read(mr, addr, tmp, size);
+        trace_memory_region_subpage_read(cpu_index, mr, addr, tmp, size);
     } else if (TRACE_MEMORY_REGION_OPS_READ_ENABLED) {
         hwaddr abs_addr = memory_region_to_absolute_addr(mr, addr);
-        trace_memory_region_ops_read(mr, abs_addr, tmp, size);
+        trace_memory_region_ops_read(cpu_index, mr, abs_addr, tmp, size);
     }
     *value |= (tmp & mask) << shift;
     return MEMTX_OK;
@@ -438,13 +446,17 @@  static MemTxResult memory_region_read_with_attrs_accessor(MemoryRegion *mr,
 {
     uint64_t tmp = 0;
     MemTxResult r;
+    int cpu_index = -1;
+
+    if (current_cpu)
+        cpu_index = current_cpu->cpu_index;
 
     r = mr->ops->read_with_attrs(mr->opaque, addr, &tmp, size, attrs);
     if (mr->subpage) {
-        trace_memory_region_subpage_read(mr, addr, tmp, size);
+        trace_memory_region_subpage_read(cpu_index, mr, addr, tmp, size);
     } else if (TRACE_MEMORY_REGION_OPS_READ_ENABLED) {
         hwaddr abs_addr = memory_region_to_absolute_addr(mr, addr);
-        trace_memory_region_ops_read(mr, abs_addr, tmp, size);
+        trace_memory_region_ops_read(cpu_index, mr, abs_addr, tmp, size);
     }
     *value |= (tmp & mask) << shift;
     return r;
@@ -459,13 +471,17 @@  static MemTxResult memory_region_oldmmio_write_accessor(MemoryRegion *mr,
                                                         MemTxAttrs attrs)
 {
     uint64_t tmp;
+    int cpu_index = -1;
+
+    if (current_cpu)
+        cpu_index = current_cpu->cpu_index;
 
     tmp = (*value >> shift) & mask;
     if (mr->subpage) {
-        trace_memory_region_subpage_write(mr, addr, tmp, size);
+        trace_memory_region_subpage_write(cpu_index, mr, addr, tmp, size);
     } else if (TRACE_MEMORY_REGION_OPS_WRITE_ENABLED) {
         hwaddr abs_addr = memory_region_to_absolute_addr(mr, addr);
-        trace_memory_region_ops_write(mr, abs_addr, tmp, size);
+        trace_memory_region_ops_write(cpu_index, mr, abs_addr, tmp, size);
     }
     mr->ops->old_mmio.write[ctz32(size)](mr->opaque, addr, tmp);
     return MEMTX_OK;
@@ -480,13 +496,17 @@  static MemTxResult memory_region_write_accessor(MemoryRegion *mr,
                                                 MemTxAttrs attrs)
 {
     uint64_t tmp;
+    int cpu_index = -1;
+
+    if (current_cpu)
+        cpu_index = current_cpu->cpu_index;
 
     tmp = (*value >> shift) & mask;
     if (mr->subpage) {
-        trace_memory_region_subpage_write(mr, addr, tmp, size);
+        trace_memory_region_subpage_write(cpu_index, mr, addr, tmp, size);
     } else if (TRACE_MEMORY_REGION_OPS_WRITE_ENABLED) {
         hwaddr abs_addr = memory_region_to_absolute_addr(mr, addr);
-        trace_memory_region_ops_write(mr, abs_addr, tmp, size);
+        trace_memory_region_ops_write(cpu_index, mr, abs_addr, tmp, size);
     }
     mr->ops->write(mr->opaque, addr, tmp, size);
     return MEMTX_OK;
@@ -501,13 +521,17 @@  static MemTxResult memory_region_write_with_attrs_accessor(MemoryRegion *mr,
                                                            MemTxAttrs attrs)
 {
     uint64_t tmp;
+    int cpu_index = -1;
+
+    if (current_cpu)
+        cpu_index = current_cpu->cpu_index;
 
     tmp = (*value >> shift) & mask;
     if (mr->subpage) {
-        trace_memory_region_subpage_write(mr, addr, tmp, size);
+        trace_memory_region_subpage_write(cpu_index, mr, addr, tmp, size);
     } else if (TRACE_MEMORY_REGION_OPS_WRITE_ENABLED) {
         hwaddr abs_addr = memory_region_to_absolute_addr(mr, addr);
-        trace_memory_region_ops_write(mr, abs_addr, tmp, size);
+        trace_memory_region_ops_write(cpu_index, mr, abs_addr, tmp, size);
     }
     return mr->ops->write_with_attrs(mr->opaque, addr, tmp, size, attrs);
 }
diff --git a/trace-events b/trace-events
index a80dc1c..756ce86 100644
--- a/trace-events
+++ b/trace-events
@@ -1626,10 +1626,10 @@  disable exec_tb_exit(void *next_tb, unsigned int flags) "tb:%p flags=%x"
 translate_block(void *tb, uintptr_t pc, uint8_t *tb_code) "tb:%p, pc:0x%"PRIxPTR", tb_code:%p"
 
 # memory.c
-memory_region_ops_read(void *mr, uint64_t addr, uint64_t value, unsigned size) "mr %p addr %#"PRIx64" value %#"PRIx64" size %u"
-memory_region_ops_write(void *mr, uint64_t addr, uint64_t value, unsigned size) "mr %p addr %#"PRIx64" value %#"PRIx64" size %u"
-memory_region_subpage_read(void *mr, uint64_t offset, uint64_t value, unsigned size) "mr %p offset %#"PRIx64" value %#"PRIx64" size %u"
-memory_region_subpage_write(void *mr, uint64_t offset, uint64_t value, unsigned size) "mr %p offset %#"PRIx64" value %#"PRIx64" size %u"
+memory_region_ops_read(int cpu_index, void *mr, uint64_t addr, uint64_t value, unsigned size) "cpu %d mr %p addr %#"PRIx64" value %#"PRIx64" size %u"
+memory_region_ops_write(int cpu_index, void *mr, uint64_t addr, uint64_t value, unsigned size) "cpu %d mr %p addr %#"PRIx64" value %#"PRIx64" size %u"
+memory_region_subpage_read(int cpu_index, void *mr, uint64_t offset, uint64_t value, unsigned size) "cpu %d mr %p offset %#"PRIx64" value %#"PRIx64" size %u"
+memory_region_subpage_write(int cpu_index, void *mr, uint64_t offset, uint64_t value, unsigned size) "cpu %d mr %p offset %#"PRIx64" value %#"PRIx64" size %u"
 
 # 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)"