Re: [PATCH-for-5.2] memory: Add trace events to audit MemoryRegionOps fields

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

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é <philmd@redhat.com>
>> ---
>> Based-on: <20200805130221.24487-1-philmd@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)"
>>
>