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

[Stefan Hajnoczi]

On Tue, Aug 18, 2020 at 09:56:37AM +0200, Philippe Mathieu-Daudé wrote:
> 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.

Regarding the MemoryRegionOps checks, the following grep command-line
shows that all MemoryRegionOps definitions are global:

  $ git grep 'MemoryRegionOps [^*]' hw/

This means static checking is possible.

> 
> (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.

Can they be turned into a CI check instead of debugging helpers? For
example, all existing violations are exempt but new MemoryRegionOps must
comply. This way it's not necessary to audit and fix everything right
now but code quality is improved in new code.

Static checking is nice because there is no need to execute QEMU and
trigger all the code paths that lead to MemoryRegionOps usage.

Here are more static checker ideas:

1. Rename memory_region_init_io() to memory_region_init_io_unsafe() (or
   "legacy", "nocheck", etc) and then add a checkpatch.pl error if a new
   memory_region_init_io_unsafe() call is added.

2. Walk the debuginfo looking for MemoryRegionOps structs and check
   them. For example, a Python script that uses a DWARF parser. There
   can be list of exempted source files that are allowed to violate the
   rules (existing code).

3. Use __attribute__((__section__())) on MemoryRegionOps so they can
   easily be located in ELF files and check them. For example, a C
   program that opens the ELF and finds the "memoryregions" section.

Stefan

signature.asc
Description: PGP signature