On 8/18/20 8:32 AM, Paolo Bonzini wrote: > On 06/08/20 17:26, Philippe Mathieu-Daudé wrote: >> Add trace events to audit MemoryRegionOps field such: >> - are all the valid/impl fields provided? >> - is the region a power of two? >> >> These cases are accepted, but it is interesting to list them. >> >> Example: >> >> $ qemu-system-i386 -S -trace memory_region_io_check\* >> memory_region_io_check_odd_size mr name:'dma-page' size:0x3
(a) >> memory_region_io_check_access_size_incomplete mr name:'acpi-tmr' >> min/max:[valid:1/4 impl:4/0] >> memory_region_io_check_access_size_incomplete mr name:'acpi-evt' >> min/max:[valid:1/2 impl:2/0] >> memory_region_io_check_access_size_incomplete mr name:'acpi-cnt' >> min/max:[valid:1/2 impl:2/0] (b) > > Can they be detected using Coccinelle instead? For static declarations, probably. (a) is not really fixable (because some datasheets don't count the reserved space in the device address map [1]), but is interesting to audit. I believe (b) has to be updated per maintainers preference, not by an individual developer. IIUC Michael said [2] while there is no bus information in MemoryRegionOps (and way to report a bus specific error), it is pointless to blindly fill the zero access sizes. Meanwhile I prefer to share my debugging helpers as trace events instead of ./configure --enable-maintainer and #ifdef'ry. [1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg431171.html [2] https://www.mail-archive.com/qemu-devel@nongnu.org/msg709171.html > > Paolo > >> >> Signed-off-by: Philippe Mathieu-Daudé <phi...@redhat.com> >> --- >> Based-on: <20200805130221.24487-1-phi...@redhat.com> >> "softmmu: Add missing trace-events file" >> --- >> softmmu/memory.c | 11 +++++++++++ >> softmmu/trace-events | 2 ++ >> 2 files changed, 13 insertions(+) >> >> diff --git a/softmmu/memory.c b/softmmu/memory.c >> index d030eb6f7c..daa0daf2a8 100644 >> --- a/softmmu/memory.c >> +++ b/softmmu/memory.c >> @@ -1488,6 +1488,17 @@ void memory_region_init_io(MemoryRegion *mr, >> mr->ops = ops ? ops : &unassigned_mem_ops; >> mr->opaque = opaque; >> mr->terminates = true; >> + if (size != UINT64_MAX && !is_power_of_2(size)) { >> + trace_memory_region_io_check_odd_size(name, size); >> + } >> + if (ops && (!ops->impl.min_access_size || !ops->impl.max_access_size || >> + !ops->valid.min_access_size || >> !ops->valid.max_access_size)) { >> + trace_memory_region_io_check_access_size_incomplete(name, >> + mr->ops->valid.min_access_size, >> + mr->ops->valid.max_access_size, >> + mr->ops->impl.min_access_size, >> + mr->ops->impl.max_access_size); >> + } >> } >> >> void memory_region_init_ram_nomigrate(MemoryRegion *mr, >> diff --git a/softmmu/trace-events b/softmmu/trace-events >> index b80ca042e1..00eb316aef 100644 >> --- a/softmmu/trace-events >> +++ b/softmmu/trace-events >> @@ -18,6 +18,8 @@ memory_region_ram_device_write(int cpu_index, void *mr, >> uint64_t addr, uint64_t >> flatview_new(void *view, void *root) "%p (root %p)" >> flatview_destroy(void *view, void *root) "%p (root %p)" >> flatview_destroy_rcu(void *view, void *root) "%p (root %p)" >> +memory_region_io_check_odd_size(const char *name, uint64_t size) "mr >> name:'%s' size:0x%"PRIx64 >> +memory_region_io_check_access_size_incomplete(const char *name, unsigned >> vmin, unsigned vmax, unsigned imin, unsigned imax) "mr name:'%s' >> min/max:[valid:%u/%u impl:%u/%u]" >> >> # vl.c >> vm_state_notify(int running, int reason, const char *reason_str) "running >> %d reason %d (%s)" >> >
