* [PATCH-for-5.2] memory: Add trace events to audit MemoryRegionOps fields
@ 2020-08-06 15:26 Philippe Mathieu-Daudé
2020-08-18 6:32 ` Paolo Bonzini
0 siblings, 1 reply; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-08-06 15:26 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini, Philippe Mathieu-Daudé
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
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]
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)"
--
2.21.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH-for-5.2] memory: Add trace events to audit MemoryRegionOps fields
2020-08-06 15:26 [PATCH-for-5.2] memory: Add trace events to audit MemoryRegionOps fields Philippe Mathieu-Daudé
@ 2020-08-18 6:32 ` Paolo Bonzini
2020-08-18 7:56 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 6+ messages in thread
From: Paolo Bonzini @ 2020-08-18 6:32 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel
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
> 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]
Can they be detected using Coccinelle instead?
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)"
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH-for-5.2] memory: Add trace events to audit MemoryRegionOps fields
2020-08-18 6:32 ` Paolo Bonzini
@ 2020-08-18 7:56 ` Philippe Mathieu-Daudé
2020-08-19 9:14 ` Stefan Hajnoczi
0 siblings, 1 reply; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-08-18 7:56 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel
Cc: Peter Maydell, Stefan Hajnoczi, Michael S. Tsirkin
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)"
>>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH-for-5.2] memory: Add trace events to audit MemoryRegionOps fields
2020-08-18 7:56 ` Philippe Mathieu-Daudé
@ 2020-08-19 9:14 ` Stefan Hajnoczi
2020-08-19 10:10 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 6+ messages in thread
From: Stefan Hajnoczi @ 2020-08-19 9:14 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Paolo Bonzini, Michael S. Tsirkin, qemu-devel, Peter Maydell
[-- Attachment #1: Type: text/plain, Size: 2875 bytes --]
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH-for-5.2] memory: Add trace events to audit MemoryRegionOps fields
2020-08-19 9:14 ` Stefan Hajnoczi
@ 2020-08-19 10:10 ` Philippe Mathieu-Daudé
2020-08-19 14:07 ` Stefan Hajnoczi
0 siblings, 1 reply; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-08-19 10:10 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Paolo Bonzini, Michael S. Tsirkin, qemu-devel, Peter Maydell
On 8/19/20 11:14 AM, Stefan Hajnoczi wrote:
> 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.
Thanks for the tips!
From the list 1. seems the easier to implement to me (no brain effort).
But for now I'm not sure the check has to be enforced, because I'm not
sure what we really want to do. First we need to figure out the 'bus'
component of a a MemoryRegion (where it sits), as it affects the
MemoryRegionOps possible range values.
>
> Stefan
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH-for-5.2] memory: Add trace events to audit MemoryRegionOps fields
2020-08-19 10:10 ` Philippe Mathieu-Daudé
@ 2020-08-19 14:07 ` Stefan Hajnoczi
0 siblings, 0 replies; 6+ messages in thread
From: Stefan Hajnoczi @ 2020-08-19 14:07 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Paolo Bonzini, Michael S. Tsirkin, qemu-devel, Peter Maydell
[-- Attachment #1: Type: text/plain, Size: 579 bytes --]
On Wed, Aug 19, 2020 at 12:10:20PM +0200, Philippe Mathieu-Daudé wrote:
> But for now I'm not sure the check has to be enforced, because I'm not
> sure what we really want to do. First we need to figure out the 'bus'
> component of a a MemoryRegion (where it sits), as it affects the
> MemoryRegionOps possible range values.
The natural place to enforce bus-specific constraints is when
MemoryRegions are registered with their bus. For example,
pci_register_bar().
SysBus devices need to specify constraints manually since there is no
real bus emulation.
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-08-19 14:08 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-06 15:26 [PATCH-for-5.2] memory: Add trace events to audit MemoryRegionOps fields Philippe Mathieu-Daudé
2020-08-18 6:32 ` Paolo Bonzini
2020-08-18 7:56 ` Philippe Mathieu-Daudé
2020-08-19 9:14 ` Stefan Hajnoczi
2020-08-19 10:10 ` Philippe Mathieu-Daudé
2020-08-19 14:07 ` Stefan Hajnoczi
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.