Message ID | 20210307074833.143106-1-f4bug@amsat.org (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | memory: Display MemoryRegion name in read/write ops trace events | expand |
ping? On 3/7/21 8:48 AM, Philippe Mathieu-Daudé wrote: > MemoryRegion names is cached on first call to memory_region_name(), > so displaying the name is trace events is cheap. Add it for read / > write ops. > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > softmmu/memory.c | 12 ++++++++---- > softmmu/trace-events | 4 ++-- > 2 files changed, 10 insertions(+), 6 deletions(-) > > diff --git a/softmmu/memory.c b/softmmu/memory.c > index 874a8fccdee..d4d9ab8828e 100644 > --- a/softmmu/memory.c > +++ b/softmmu/memory.c > @@ -444,7 +444,8 @@ static MemTxResult memory_region_read_accessor(MemoryRegion *mr, > trace_memory_region_subpage_read(get_cpu_index(), mr, addr, tmp, size); > } else if (trace_event_get_state_backends(TRACE_MEMORY_REGION_OPS_READ)) { > hwaddr abs_addr = memory_region_to_absolute_addr(mr, addr); > - trace_memory_region_ops_read(get_cpu_index(), mr, abs_addr, tmp, size); > + trace_memory_region_ops_read(get_cpu_index(), mr, abs_addr, tmp, size, > + memory_region_name(mr)); > } > memory_region_shift_read_access(value, shift, mask, tmp); > return MEMTX_OK; > @@ -466,7 +467,8 @@ static MemTxResult memory_region_read_with_attrs_accessor(MemoryRegion *mr, > trace_memory_region_subpage_read(get_cpu_index(), mr, addr, tmp, size); > } else if (trace_event_get_state_backends(TRACE_MEMORY_REGION_OPS_READ)) { > hwaddr abs_addr = memory_region_to_absolute_addr(mr, addr); > - trace_memory_region_ops_read(get_cpu_index(), mr, abs_addr, tmp, size); > + trace_memory_region_ops_read(get_cpu_index(), mr, abs_addr, tmp, size, > + memory_region_name(mr)); > } > memory_region_shift_read_access(value, shift, mask, tmp); > return r; > @@ -486,7 +488,8 @@ static MemTxResult memory_region_write_accessor(MemoryRegion *mr, > trace_memory_region_subpage_write(get_cpu_index(), mr, addr, tmp, size); > } else if (trace_event_get_state_backends(TRACE_MEMORY_REGION_OPS_WRITE)) { > hwaddr abs_addr = memory_region_to_absolute_addr(mr, addr); > - trace_memory_region_ops_write(get_cpu_index(), mr, abs_addr, tmp, size); > + trace_memory_region_ops_write(get_cpu_index(), mr, abs_addr, tmp, size, > + memory_region_name(mr)); > } > mr->ops->write(mr->opaque, addr, tmp, size); > return MEMTX_OK; > @@ -506,7 +509,8 @@ static MemTxResult memory_region_write_with_attrs_accessor(MemoryRegion *mr, > trace_memory_region_subpage_write(get_cpu_index(), mr, addr, tmp, size); > } else if (trace_event_get_state_backends(TRACE_MEMORY_REGION_OPS_WRITE)) { > hwaddr abs_addr = memory_region_to_absolute_addr(mr, addr); > - trace_memory_region_ops_write(get_cpu_index(), mr, abs_addr, tmp, size); > + trace_memory_region_ops_write(get_cpu_index(), mr, abs_addr, tmp, size, > + memory_region_name(mr)); > } > return mr->ops->write_with_attrs(mr->opaque, addr, tmp, size, attrs); > } > diff --git a/softmmu/trace-events b/softmmu/trace-events > index b80ca042e1f..359fb37cc8d 100644 > --- a/softmmu/trace-events > +++ b/softmmu/trace-events > @@ -9,8 +9,8 @@ cpu_in(unsigned int addr, char size, unsigned int val) "addr 0x%x(%c) value %u" > cpu_out(unsigned int addr, char size, unsigned int val) "addr 0x%x(%c) value %u" > > # memory.c > -memory_region_ops_read(int cpu_index, void *mr, uint64_t addr, uint64_t value, unsigned size) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"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 0x%"PRIx64" value 0x%"PRIx64" size %u" > +memory_region_ops_read(int cpu_index, void *mr, uint64_t addr, uint64_t value, unsigned size, const char *name) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64" size %u name '%s'" > +memory_region_ops_write(int cpu_index, void *mr, uint64_t addr, uint64_t value, unsigned size, const char *name) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64" size %u name '%s'" > memory_region_subpage_read(int cpu_index, void *mr, uint64_t offset, uint64_t value, unsigned size) "cpu %d mr %p offset 0x%"PRIx64" value 0x%"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 0x%"PRIx64" value 0x%"PRIx64" size %u" > memory_region_ram_device_read(int cpu_index, void *mr, uint64_t addr, uint64_t value, unsigned size) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64" size %u" >
Cc'ing qemu-trivial@ On 3/18/21 4:39 PM, Philippe Mathieu-Daudé wrote: > ping? > > On 3/7/21 8:48 AM, Philippe Mathieu-Daudé wrote: >> MemoryRegion names is cached on first call to memory_region_name(), >> so displaying the name is trace events is cheap. Add it for read / >> write ops. >> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> --- >> softmmu/memory.c | 12 ++++++++---- >> softmmu/trace-events | 4 ++-- >> 2 files changed, 10 insertions(+), 6 deletions(-) >> >> diff --git a/softmmu/memory.c b/softmmu/memory.c >> index 874a8fccdee..d4d9ab8828e 100644 >> --- a/softmmu/memory.c >> +++ b/softmmu/memory.c >> @@ -444,7 +444,8 @@ static MemTxResult memory_region_read_accessor(MemoryRegion *mr, >> trace_memory_region_subpage_read(get_cpu_index(), mr, addr, tmp, size); >> } else if (trace_event_get_state_backends(TRACE_MEMORY_REGION_OPS_READ)) { >> hwaddr abs_addr = memory_region_to_absolute_addr(mr, addr); >> - trace_memory_region_ops_read(get_cpu_index(), mr, abs_addr, tmp, size); >> + trace_memory_region_ops_read(get_cpu_index(), mr, abs_addr, tmp, size, >> + memory_region_name(mr)); >> } >> memory_region_shift_read_access(value, shift, mask, tmp); >> return MEMTX_OK; >> @@ -466,7 +467,8 @@ static MemTxResult memory_region_read_with_attrs_accessor(MemoryRegion *mr, >> trace_memory_region_subpage_read(get_cpu_index(), mr, addr, tmp, size); >> } else if (trace_event_get_state_backends(TRACE_MEMORY_REGION_OPS_READ)) { >> hwaddr abs_addr = memory_region_to_absolute_addr(mr, addr); >> - trace_memory_region_ops_read(get_cpu_index(), mr, abs_addr, tmp, size); >> + trace_memory_region_ops_read(get_cpu_index(), mr, abs_addr, tmp, size, >> + memory_region_name(mr)); >> } >> memory_region_shift_read_access(value, shift, mask, tmp); >> return r; >> @@ -486,7 +488,8 @@ static MemTxResult memory_region_write_accessor(MemoryRegion *mr, >> trace_memory_region_subpage_write(get_cpu_index(), mr, addr, tmp, size); >> } else if (trace_event_get_state_backends(TRACE_MEMORY_REGION_OPS_WRITE)) { >> hwaddr abs_addr = memory_region_to_absolute_addr(mr, addr); >> - trace_memory_region_ops_write(get_cpu_index(), mr, abs_addr, tmp, size); >> + trace_memory_region_ops_write(get_cpu_index(), mr, abs_addr, tmp, size, >> + memory_region_name(mr)); >> } >> mr->ops->write(mr->opaque, addr, tmp, size); >> return MEMTX_OK; >> @@ -506,7 +509,8 @@ static MemTxResult memory_region_write_with_attrs_accessor(MemoryRegion *mr, >> trace_memory_region_subpage_write(get_cpu_index(), mr, addr, tmp, size); >> } else if (trace_event_get_state_backends(TRACE_MEMORY_REGION_OPS_WRITE)) { >> hwaddr abs_addr = memory_region_to_absolute_addr(mr, addr); >> - trace_memory_region_ops_write(get_cpu_index(), mr, abs_addr, tmp, size); >> + trace_memory_region_ops_write(get_cpu_index(), mr, abs_addr, tmp, size, >> + memory_region_name(mr)); >> } >> return mr->ops->write_with_attrs(mr->opaque, addr, tmp, size, attrs); >> } >> diff --git a/softmmu/trace-events b/softmmu/trace-events >> index b80ca042e1f..359fb37cc8d 100644 >> --- a/softmmu/trace-events >> +++ b/softmmu/trace-events >> @@ -9,8 +9,8 @@ cpu_in(unsigned int addr, char size, unsigned int val) "addr 0x%x(%c) value %u" >> cpu_out(unsigned int addr, char size, unsigned int val) "addr 0x%x(%c) value %u" >> >> # memory.c >> -memory_region_ops_read(int cpu_index, void *mr, uint64_t addr, uint64_t value, unsigned size) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"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 0x%"PRIx64" value 0x%"PRIx64" size %u" >> +memory_region_ops_read(int cpu_index, void *mr, uint64_t addr, uint64_t value, unsigned size, const char *name) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64" size %u name '%s'" >> +memory_region_ops_write(int cpu_index, void *mr, uint64_t addr, uint64_t value, unsigned size, const char *name) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64" size %u name '%s'" >> memory_region_subpage_read(int cpu_index, void *mr, uint64_t offset, uint64_t value, unsigned size) "cpu %d mr %p offset 0x%"PRIx64" value 0x%"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 0x%"PRIx64" value 0x%"PRIx64" size %u" >> memory_region_ram_device_read(int cpu_index, void *mr, uint64_t addr, uint64_t value, unsigned size) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64" size %u" >> >
Le 07/03/2021 à 08:48, Philippe Mathieu-Daudé a écrit : > MemoryRegion names is cached on first call to memory_region_name(), > so displaying the name is trace events is cheap. Add it for read / > write ops. > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > softmmu/memory.c | 12 ++++++++---- > softmmu/trace-events | 4 ++-- > 2 files changed, 10 insertions(+), 6 deletions(-) > > diff --git a/softmmu/memory.c b/softmmu/memory.c > index 874a8fccdee..d4d9ab8828e 100644 > --- a/softmmu/memory.c > +++ b/softmmu/memory.c > @@ -444,7 +444,8 @@ static MemTxResult memory_region_read_accessor(MemoryRegion *mr, > trace_memory_region_subpage_read(get_cpu_index(), mr, addr, tmp, size); > } else if (trace_event_get_state_backends(TRACE_MEMORY_REGION_OPS_READ)) { > hwaddr abs_addr = memory_region_to_absolute_addr(mr, addr); > - trace_memory_region_ops_read(get_cpu_index(), mr, abs_addr, tmp, size); > + trace_memory_region_ops_read(get_cpu_index(), mr, abs_addr, tmp, size, > + memory_region_name(mr)); > } > memory_region_shift_read_access(value, shift, mask, tmp); > return MEMTX_OK; > @@ -466,7 +467,8 @@ static MemTxResult memory_region_read_with_attrs_accessor(MemoryRegion *mr, > trace_memory_region_subpage_read(get_cpu_index(), mr, addr, tmp, size); > } else if (trace_event_get_state_backends(TRACE_MEMORY_REGION_OPS_READ)) { > hwaddr abs_addr = memory_region_to_absolute_addr(mr, addr); > - trace_memory_region_ops_read(get_cpu_index(), mr, abs_addr, tmp, size); > + trace_memory_region_ops_read(get_cpu_index(), mr, abs_addr, tmp, size, > + memory_region_name(mr)); > } > memory_region_shift_read_access(value, shift, mask, tmp); > return r; > @@ -486,7 +488,8 @@ static MemTxResult memory_region_write_accessor(MemoryRegion *mr, > trace_memory_region_subpage_write(get_cpu_index(), mr, addr, tmp, size); > } else if (trace_event_get_state_backends(TRACE_MEMORY_REGION_OPS_WRITE)) { > hwaddr abs_addr = memory_region_to_absolute_addr(mr, addr); > - trace_memory_region_ops_write(get_cpu_index(), mr, abs_addr, tmp, size); > + trace_memory_region_ops_write(get_cpu_index(), mr, abs_addr, tmp, size, > + memory_region_name(mr)); > } > mr->ops->write(mr->opaque, addr, tmp, size); > return MEMTX_OK; > @@ -506,7 +509,8 @@ static MemTxResult memory_region_write_with_attrs_accessor(MemoryRegion *mr, > trace_memory_region_subpage_write(get_cpu_index(), mr, addr, tmp, size); > } else if (trace_event_get_state_backends(TRACE_MEMORY_REGION_OPS_WRITE)) { > hwaddr abs_addr = memory_region_to_absolute_addr(mr, addr); > - trace_memory_region_ops_write(get_cpu_index(), mr, abs_addr, tmp, size); > + trace_memory_region_ops_write(get_cpu_index(), mr, abs_addr, tmp, size, > + memory_region_name(mr)); > } > return mr->ops->write_with_attrs(mr->opaque, addr, tmp, size, attrs); > } > diff --git a/softmmu/trace-events b/softmmu/trace-events > index b80ca042e1f..359fb37cc8d 100644 > --- a/softmmu/trace-events > +++ b/softmmu/trace-events > @@ -9,8 +9,8 @@ cpu_in(unsigned int addr, char size, unsigned int val) "addr 0x%x(%c) value %u" > cpu_out(unsigned int addr, char size, unsigned int val) "addr 0x%x(%c) value %u" > > # memory.c > -memory_region_ops_read(int cpu_index, void *mr, uint64_t addr, uint64_t value, unsigned size) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"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 0x%"PRIx64" value 0x%"PRIx64" size %u" > +memory_region_ops_read(int cpu_index, void *mr, uint64_t addr, uint64_t value, unsigned size, const char *name) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64" size %u name '%s'" > +memory_region_ops_write(int cpu_index, void *mr, uint64_t addr, uint64_t value, unsigned size, const char *name) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64" size %u name '%s'" > memory_region_subpage_read(int cpu_index, void *mr, uint64_t offset, uint64_t value, unsigned size) "cpu %d mr %p offset 0x%"PRIx64" value 0x%"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 0x%"PRIx64" value 0x%"PRIx64" size %u" > memory_region_ram_device_read(int cpu_index, void *mr, uint64_t addr, uint64_t value, unsigned size) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64" size %u" > Reviewed-by: Laurent Vivier <laurent@vivier.eu>
Le 02/06/2021 à 12:49, Philippe Mathieu-Daudé a écrit : > Cc'ing qemu-trivial@ > > On 3/18/21 4:39 PM, Philippe Mathieu-Daudé wrote: >> ping? >> >> On 3/7/21 8:48 AM, Philippe Mathieu-Daudé wrote: >>> MemoryRegion names is cached on first call to memory_region_name(), It is cached on first call but now that it is used in the trace function, does it mean it will be always allocated in memory? Thanks, Laurent >>> so displaying the name is trace events is cheap. Add it for read / >>> write ops. >>> >>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >>> --- >>> softmmu/memory.c | 12 ++++++++---- >>> softmmu/trace-events | 4 ++-- >>> 2 files changed, 10 insertions(+), 6 deletions(-) >>> >>> diff --git a/softmmu/memory.c b/softmmu/memory.c >>> index 874a8fccdee..d4d9ab8828e 100644 >>> --- a/softmmu/memory.c >>> +++ b/softmmu/memory.c >>> @@ -444,7 +444,8 @@ static MemTxResult memory_region_read_accessor(MemoryRegion *mr, >>> trace_memory_region_subpage_read(get_cpu_index(), mr, addr, tmp, size); >>> } else if (trace_event_get_state_backends(TRACE_MEMORY_REGION_OPS_READ)) { >>> hwaddr abs_addr = memory_region_to_absolute_addr(mr, addr); >>> - trace_memory_region_ops_read(get_cpu_index(), mr, abs_addr, tmp, size); >>> + trace_memory_region_ops_read(get_cpu_index(), mr, abs_addr, tmp, size, >>> + memory_region_name(mr)); >>> } >>> memory_region_shift_read_access(value, shift, mask, tmp); >>> return MEMTX_OK; >>> @@ -466,7 +467,8 @@ static MemTxResult memory_region_read_with_attrs_accessor(MemoryRegion *mr, >>> trace_memory_region_subpage_read(get_cpu_index(), mr, addr, tmp, size); >>> } else if (trace_event_get_state_backends(TRACE_MEMORY_REGION_OPS_READ)) { >>> hwaddr abs_addr = memory_region_to_absolute_addr(mr, addr); >>> - trace_memory_region_ops_read(get_cpu_index(), mr, abs_addr, tmp, size); >>> + trace_memory_region_ops_read(get_cpu_index(), mr, abs_addr, tmp, size, >>> + memory_region_name(mr)); >>> } >>> memory_region_shift_read_access(value, shift, mask, tmp); >>> return r; >>> @@ -486,7 +488,8 @@ static MemTxResult memory_region_write_accessor(MemoryRegion *mr, >>> trace_memory_region_subpage_write(get_cpu_index(), mr, addr, tmp, size); >>> } else if (trace_event_get_state_backends(TRACE_MEMORY_REGION_OPS_WRITE)) { >>> hwaddr abs_addr = memory_region_to_absolute_addr(mr, addr); >>> - trace_memory_region_ops_write(get_cpu_index(), mr, abs_addr, tmp, size); >>> + trace_memory_region_ops_write(get_cpu_index(), mr, abs_addr, tmp, size, >>> + memory_region_name(mr)); >>> } >>> mr->ops->write(mr->opaque, addr, tmp, size); >>> return MEMTX_OK; >>> @@ -506,7 +509,8 @@ static MemTxResult memory_region_write_with_attrs_accessor(MemoryRegion *mr, >>> trace_memory_region_subpage_write(get_cpu_index(), mr, addr, tmp, size); >>> } else if (trace_event_get_state_backends(TRACE_MEMORY_REGION_OPS_WRITE)) { >>> hwaddr abs_addr = memory_region_to_absolute_addr(mr, addr); >>> - trace_memory_region_ops_write(get_cpu_index(), mr, abs_addr, tmp, size); >>> + trace_memory_region_ops_write(get_cpu_index(), mr, abs_addr, tmp, size, >>> + memory_region_name(mr)); >>> } >>> return mr->ops->write_with_attrs(mr->opaque, addr, tmp, size, attrs); >>> } >>> diff --git a/softmmu/trace-events b/softmmu/trace-events >>> index b80ca042e1f..359fb37cc8d 100644 >>> --- a/softmmu/trace-events >>> +++ b/softmmu/trace-events >>> @@ -9,8 +9,8 @@ cpu_in(unsigned int addr, char size, unsigned int val) "addr 0x%x(%c) value %u" >>> cpu_out(unsigned int addr, char size, unsigned int val) "addr 0x%x(%c) value %u" >>> >>> # memory.c >>> -memory_region_ops_read(int cpu_index, void *mr, uint64_t addr, uint64_t value, unsigned size) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"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 0x%"PRIx64" value 0x%"PRIx64" size %u" >>> +memory_region_ops_read(int cpu_index, void *mr, uint64_t addr, uint64_t value, unsigned size, const char *name) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64" size %u name '%s'" >>> +memory_region_ops_write(int cpu_index, void *mr, uint64_t addr, uint64_t value, unsigned size, const char *name) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64" size %u name '%s'" >>> memory_region_subpage_read(int cpu_index, void *mr, uint64_t offset, uint64_t value, unsigned size) "cpu %d mr %p offset 0x%"PRIx64" value 0x%"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 0x%"PRIx64" value 0x%"PRIx64" size %u" >>> memory_region_ram_device_read(int cpu_index, void *mr, uint64_t addr, uint64_t value, unsigned size) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64" size %u" >>> >> >
On 6/7/21 9:33 AM, Laurent Vivier wrote: > Le 02/06/2021 à 12:49, Philippe Mathieu-Daudé a écrit : >> Cc'ing qemu-trivial@ >> >> On 3/18/21 4:39 PM, Philippe Mathieu-Daudé wrote: >>> ping? >>> >>> On 3/7/21 8:48 AM, Philippe Mathieu-Daudé wrote: >>>> MemoryRegion names is cached on first call to memory_region_name(), > > It is cached on first call but now that it is used in the trace function, does it mean it will be > always allocated in memory? Yes, this is how memory_region_name() works: const char *memory_region_name(const MemoryRegion *mr) { if (!mr->name) { ((MemoryRegion *)mr)->name = g_strdup(object_get_canonical_path_component(OBJECT(mr))); } return mr->name; } > > Thanks, > Laurent
Le 07/06/2021 à 10:28, Philippe Mathieu-Daudé a écrit : > On 6/7/21 9:33 AM, Laurent Vivier wrote: >> Le 02/06/2021 à 12:49, Philippe Mathieu-Daudé a écrit : >>> Cc'ing qemu-trivial@ >>> >>> On 3/18/21 4:39 PM, Philippe Mathieu-Daudé wrote: >>>> ping? >>>> >>>> On 3/7/21 8:48 AM, Philippe Mathieu-Daudé wrote: >>>>> MemoryRegion names is cached on first call to memory_region_name(), >> >> It is cached on first call but now that it is used in the trace function, does it mean it will be >> always allocated in memory? > > Yes, this is how memory_region_name() works: > > const char *memory_region_name(const MemoryRegion *mr) > { > if (!mr->name) { > ((MemoryRegion *)mr)->name = > g_strdup(object_get_canonical_path_component(OBJECT(mr))); > } > return mr->name; > } OK, in fact I didn't see it was called from inside a "if (trace_event_get_state_backends(TRACE_MEMORY_REGION_OPS_XXX))" and was worrying about the memory for the string always allocated. So it looks good. Thanks, Laurent
Hi Laurent, On 6/7/21 1:29 PM, Laurent Vivier wrote: > Le 07/06/2021 à 10:28, Philippe Mathieu-Daudé a écrit : >> On 6/7/21 9:33 AM, Laurent Vivier wrote: >>> Le 02/06/2021 à 12:49, Philippe Mathieu-Daudé a écrit : >>>> Cc'ing qemu-trivial@ >>>> >>>> On 3/18/21 4:39 PM, Philippe Mathieu-Daudé wrote: >>>>> ping? >>>>> >>>>> On 3/7/21 8:48 AM, Philippe Mathieu-Daudé wrote: >>>>>> MemoryRegion names is cached on first call to memory_region_name(), >>> >>> It is cached on first call but now that it is used in the trace function, does it mean it will be >>> always allocated in memory? >> >> Yes, this is how memory_region_name() works: >> >> const char *memory_region_name(const MemoryRegion *mr) >> { >> if (!mr->name) { >> ((MemoryRegion *)mr)->name = >> g_strdup(object_get_canonical_path_component(OBJECT(mr))); >> } >> return mr->name; >> } > > OK, in fact I didn't see it was called from inside a "if > (trace_event_get_state_backends(TRACE_MEMORY_REGION_OPS_XXX))" and was worrying about the memory for > the string always allocated. > > So it looks good. Is there something else you expect me to do with this patch? Maybe you expect another R-b to take it, so I should keep pinging for review?
Le 21/06/2021 à 12:08, Philippe Mathieu-Daudé a écrit : > Hi Laurent, > > On 6/7/21 1:29 PM, Laurent Vivier wrote: >> Le 07/06/2021 à 10:28, Philippe Mathieu-Daudé a écrit : >>> On 6/7/21 9:33 AM, Laurent Vivier wrote: >>>> Le 02/06/2021 à 12:49, Philippe Mathieu-Daudé a écrit : >>>>> Cc'ing qemu-trivial@ >>>>> >>>>> On 3/18/21 4:39 PM, Philippe Mathieu-Daudé wrote: >>>>>> ping? >>>>>> >>>>>> On 3/7/21 8:48 AM, Philippe Mathieu-Daudé wrote: >>>>>>> MemoryRegion names is cached on first call to memory_region_name(), >>>> >>>> It is cached on first call but now that it is used in the trace function, does it mean it will be >>>> always allocated in memory? >>> >>> Yes, this is how memory_region_name() works: >>> >>> const char *memory_region_name(const MemoryRegion *mr) >>> { >>> if (!mr->name) { >>> ((MemoryRegion *)mr)->name = >>> g_strdup(object_get_canonical_path_component(OBJECT(mr))); >>> } >>> return mr->name; >>> } >> >> OK, in fact I didn't see it was called from inside a "if >> (trace_event_get_state_backends(TRACE_MEMORY_REGION_OPS_XXX))" and was worrying about the memory for >> the string always allocated. >> >> So it looks good. > > Is there something else you expect me to do with this patch? > Maybe you expect another R-b to take it, so I should keep > pinging for review? > No, I'm working on a linux-user pull request, once done I'll do a new trivial branch pull request. Thanks, Laurent
On 6/21/21 12:13 PM, Laurent Vivier wrote: > Le 21/06/2021 à 12:08, Philippe Mathieu-Daudé a écrit : >> Hi Laurent, >> >> On 6/7/21 1:29 PM, Laurent Vivier wrote: >>> Le 07/06/2021 à 10:28, Philippe Mathieu-Daudé a écrit : >>>> On 6/7/21 9:33 AM, Laurent Vivier wrote: >>>>> Le 02/06/2021 à 12:49, Philippe Mathieu-Daudé a écrit : >>>>>> Cc'ing qemu-trivial@ >>> >>> So it looks good. >> >> Is there something else you expect me to do with this patch? >> Maybe you expect another R-b to take it, so I should keep >> pinging for review? >> > > No, I'm working on a linux-user pull request, once done I'll do a new trivial branch pull request. No hurry, I just wanted to know if there was something missing on my side :) > > Thanks, > Laurent >
Le 21/06/2021 à 14:17, Philippe Mathieu-Daudé a écrit : > On 6/21/21 12:13 PM, Laurent Vivier wrote: >> Le 21/06/2021 à 12:08, Philippe Mathieu-Daudé a écrit : >>> Hi Laurent, >>> >>> On 6/7/21 1:29 PM, Laurent Vivier wrote: >>>> Le 07/06/2021 à 10:28, Philippe Mathieu-Daudé a écrit : >>>>> On 6/7/21 9:33 AM, Laurent Vivier wrote: >>>>>> Le 02/06/2021 à 12:49, Philippe Mathieu-Daudé a écrit : >>>>>>> Cc'ing qemu-trivial@ >>>> >>>> So it looks good. >>> >>> Is there something else you expect me to do with this patch? >>> Maybe you expect another R-b to take it, so I should keep >>> pinging for review? >>> >> >> No, I'm working on a linux-user pull request, once done I'll do a new trivial branch pull request. > > No hurry, I just wanted to know if there was something missing on my side :) Applied to my trivial-patches branch. Thanks, Laurent
diff --git a/softmmu/memory.c b/softmmu/memory.c index 874a8fccdee..d4d9ab8828e 100644 --- a/softmmu/memory.c +++ b/softmmu/memory.c @@ -444,7 +444,8 @@ static MemTxResult memory_region_read_accessor(MemoryRegion *mr, trace_memory_region_subpage_read(get_cpu_index(), mr, addr, tmp, size); } else if (trace_event_get_state_backends(TRACE_MEMORY_REGION_OPS_READ)) { hwaddr abs_addr = memory_region_to_absolute_addr(mr, addr); - trace_memory_region_ops_read(get_cpu_index(), mr, abs_addr, tmp, size); + trace_memory_region_ops_read(get_cpu_index(), mr, abs_addr, tmp, size, + memory_region_name(mr)); } memory_region_shift_read_access(value, shift, mask, tmp); return MEMTX_OK; @@ -466,7 +467,8 @@ static MemTxResult memory_region_read_with_attrs_accessor(MemoryRegion *mr, trace_memory_region_subpage_read(get_cpu_index(), mr, addr, tmp, size); } else if (trace_event_get_state_backends(TRACE_MEMORY_REGION_OPS_READ)) { hwaddr abs_addr = memory_region_to_absolute_addr(mr, addr); - trace_memory_region_ops_read(get_cpu_index(), mr, abs_addr, tmp, size); + trace_memory_region_ops_read(get_cpu_index(), mr, abs_addr, tmp, size, + memory_region_name(mr)); } memory_region_shift_read_access(value, shift, mask, tmp); return r; @@ -486,7 +488,8 @@ static MemTxResult memory_region_write_accessor(MemoryRegion *mr, trace_memory_region_subpage_write(get_cpu_index(), mr, addr, tmp, size); } else if (trace_event_get_state_backends(TRACE_MEMORY_REGION_OPS_WRITE)) { hwaddr abs_addr = memory_region_to_absolute_addr(mr, addr); - trace_memory_region_ops_write(get_cpu_index(), mr, abs_addr, tmp, size); + trace_memory_region_ops_write(get_cpu_index(), mr, abs_addr, tmp, size, + memory_region_name(mr)); } mr->ops->write(mr->opaque, addr, tmp, size); return MEMTX_OK; @@ -506,7 +509,8 @@ static MemTxResult memory_region_write_with_attrs_accessor(MemoryRegion *mr, trace_memory_region_subpage_write(get_cpu_index(), mr, addr, tmp, size); } else if (trace_event_get_state_backends(TRACE_MEMORY_REGION_OPS_WRITE)) { hwaddr abs_addr = memory_region_to_absolute_addr(mr, addr); - trace_memory_region_ops_write(get_cpu_index(), mr, abs_addr, tmp, size); + trace_memory_region_ops_write(get_cpu_index(), mr, abs_addr, tmp, size, + memory_region_name(mr)); } return mr->ops->write_with_attrs(mr->opaque, addr, tmp, size, attrs); } diff --git a/softmmu/trace-events b/softmmu/trace-events index b80ca042e1f..359fb37cc8d 100644 --- a/softmmu/trace-events +++ b/softmmu/trace-events @@ -9,8 +9,8 @@ cpu_in(unsigned int addr, char size, unsigned int val) "addr 0x%x(%c) value %u" cpu_out(unsigned int addr, char size, unsigned int val) "addr 0x%x(%c) value %u" # memory.c -memory_region_ops_read(int cpu_index, void *mr, uint64_t addr, uint64_t value, unsigned size) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"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 0x%"PRIx64" value 0x%"PRIx64" size %u" +memory_region_ops_read(int cpu_index, void *mr, uint64_t addr, uint64_t value, unsigned size, const char *name) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64" size %u name '%s'" +memory_region_ops_write(int cpu_index, void *mr, uint64_t addr, uint64_t value, unsigned size, const char *name) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64" size %u name '%s'" memory_region_subpage_read(int cpu_index, void *mr, uint64_t offset, uint64_t value, unsigned size) "cpu %d mr %p offset 0x%"PRIx64" value 0x%"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 0x%"PRIx64" value 0x%"PRIx64" size %u" memory_region_ram_device_read(int cpu_index, void *mr, uint64_t addr, uint64_t value, unsigned size) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64" size %u"
MemoryRegion names is cached on first call to memory_region_name(), so displaying the name is trace events is cheap. Add it for read / write ops. Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- softmmu/memory.c | 12 ++++++++---- softmmu/trace-events | 4 ++-- 2 files changed, 10 insertions(+), 6 deletions(-)