| Message ID | 20190918052641.21300-2-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Move notdirty handling to cputlb | expand |
On 18.09.19 07:26, Richard Henderson wrote: > The memory_region_tb_read tracepoint is unreachable, since notdirty > is supposed to apply only to reads. The memory_region_tb_write > tracepoint is mis-named, because notdirty is not only used for TB > invalidation. It is also used for e.g. VGA RAM updates. > > Replace memory_region_tb_write with memory_notdirty_write, and > place it in memory_notdirty_write_prepare where it can catch all > of the instances. Add memory_notdirty_dirty to log when we no > longer intercept writes to a page. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > exec.c | 3 +++ > memory.c | 4 ---- > trace-events | 4 ++-- > 3 files changed, 5 insertions(+), 6 deletions(-) > > diff --git a/exec.c b/exec.c > index 8b998974f8..9babe57615 100644 > --- a/exec.c > +++ b/exec.c > @@ -2755,6 +2755,8 @@ void memory_notdirty_write_prepare(NotDirtyInfo *ndi, > ndi->size = size; > ndi->pages = NULL; > > + trace_memory_notdirty_write(mem_vaddr, ram_addr, size); > + > assert(tcg_enabled()); > if (!cpu_physical_memory_get_dirty_flag(ram_addr, DIRTY_MEMORY_CODE)) { > ndi->pages = page_collection_lock(ram_addr, ram_addr + size); > @@ -2779,6 +2781,7 @@ void memory_notdirty_write_complete(NotDirtyInfo *ndi) > /* we remove the notdirty callback only if the code has been > flushed */ > if (!cpu_physical_memory_is_clean(ndi->ram_addr)) { > + trace_memory_notdirty_dirty(ndi->mem_vaddr); > tlb_set_dirty(ndi->cpu, ndi->mem_vaddr); > } > } > diff --git a/memory.c b/memory.c > index b9dd6b94ca..57c44c97db 100644 > --- a/memory.c > +++ b/memory.c > @@ -438,7 +438,6 @@ static MemTxResult memory_region_read_accessor(MemoryRegion *mr, > /* Accesses to code which has previously been translated into a TB show > * up in the MMIO path, as accesses to the io_mem_notdirty > * MemoryRegion. */ > - trace_memory_region_tb_read(get_cpu_index(), addr, tmp, size); > } else if (TRACE_MEMORY_REGION_OPS_READ_ENABLED) { > hwaddr abs_addr = memory_region_to_absolute_addr(mr, addr); > trace_memory_region_ops_read(get_cpu_index(), mr, abs_addr, tmp, size); > @@ -465,7 +464,6 @@ static MemTxResult memory_region_read_with_attrs_accessor(MemoryRegion *mr, > /* Accesses to code which has previously been translated into a TB show > * up in the MMIO path, as accesses to the io_mem_notdirty > * MemoryRegion. */ > - trace_memory_region_tb_read(get_cpu_index(), addr, tmp, size); > } else if (TRACE_MEMORY_REGION_OPS_READ_ENABLED) { > hwaddr abs_addr = memory_region_to_absolute_addr(mr, addr); > trace_memory_region_ops_read(get_cpu_index(), mr, abs_addr, tmp, size); > @@ -490,7 +488,6 @@ static MemTxResult memory_region_write_accessor(MemoryRegion *mr, > /* Accesses to code which has previously been translated into a TB show > * up in the MMIO path, as accesses to the io_mem_notdirty > * MemoryRegion. */ > - trace_memory_region_tb_write(get_cpu_index(), addr, tmp, size); > } else if (TRACE_MEMORY_REGION_OPS_WRITE_ENABLED) { > hwaddr abs_addr = memory_region_to_absolute_addr(mr, addr); > trace_memory_region_ops_write(get_cpu_index(), mr, abs_addr, tmp, size); > @@ -515,7 +512,6 @@ static MemTxResult memory_region_write_with_attrs_accessor(MemoryRegion *mr, > /* Accesses to code which has previously been translated into a TB show > * up in the MMIO path, as accesses to the io_mem_notdirty > * MemoryRegion. */ > - trace_memory_region_tb_write(get_cpu_index(), addr, tmp, size); > } else if (TRACE_MEMORY_REGION_OPS_WRITE_ENABLED) { > hwaddr abs_addr = memory_region_to_absolute_addr(mr, addr); > trace_memory_region_ops_write(get_cpu_index(), mr, abs_addr, tmp, size); > diff --git a/trace-events b/trace-events > index 823a4ae64e..5c9a1631e7 100644 > --- a/trace-events > +++ b/trace-events > @@ -52,14 +52,14 @@ dma_map_wait(void *dbs) "dbs=%p" > find_ram_offset(uint64_t size, uint64_t offset) "size: 0x%" PRIx64 " @ 0x%" PRIx64 > find_ram_offset_loop(uint64_t size, uint64_t candidate, uint64_t offset, uint64_t next, uint64_t mingap) "trying size: 0x%" PRIx64 " @ 0x%" PRIx64 ", offset: 0x%" PRIx64" next: 0x%" PRIx64 " mingap: 0x%" PRIx64 > ram_block_discard_range(const char *rbname, void *hva, size_t length, bool need_madvise, bool need_fallocate, int ret) "%s@%p + 0x%zx: madvise: %d fallocate: %d ret: %d" > +memory_notdirty_write(uint64_t vaddr, uint64_t ram_addr, unsigned size) "0x%" PRIx64 " ram_addr 0x%" PRIx64 " size %u" > +memory_notdirty_dirty(uint64_t vaddr) "0x%" PRIx64 My only suggestion would be to give slightly better names like memory_notdirty_write_access() memory_notdirty_set_dirty() -- Thanks, David / dhildenb
diff --git a/exec.c b/exec.c index 8b998974f8..9babe57615 100644 --- a/exec.c +++ b/exec.c @@ -2755,6 +2755,8 @@ void memory_notdirty_write_prepare(NotDirtyInfo *ndi, ndi->size = size; ndi->pages = NULL; + trace_memory_notdirty_write(mem_vaddr, ram_addr, size); + assert(tcg_enabled()); if (!cpu_physical_memory_get_dirty_flag(ram_addr, DIRTY_MEMORY_CODE)) { ndi->pages = page_collection_lock(ram_addr, ram_addr + size); @@ -2779,6 +2781,7 @@ void memory_notdirty_write_complete(NotDirtyInfo *ndi) /* we remove the notdirty callback only if the code has been flushed */ if (!cpu_physical_memory_is_clean(ndi->ram_addr)) { + trace_memory_notdirty_dirty(ndi->mem_vaddr); tlb_set_dirty(ndi->cpu, ndi->mem_vaddr); } } diff --git a/memory.c b/memory.c index b9dd6b94ca..57c44c97db 100644 --- a/memory.c +++ b/memory.c @@ -438,7 +438,6 @@ static MemTxResult memory_region_read_accessor(MemoryRegion *mr, /* Accesses to code which has previously been translated into a TB show * up in the MMIO path, as accesses to the io_mem_notdirty * MemoryRegion. */ - trace_memory_region_tb_read(get_cpu_index(), addr, tmp, size); } else if (TRACE_MEMORY_REGION_OPS_READ_ENABLED) { hwaddr abs_addr = memory_region_to_absolute_addr(mr, addr); trace_memory_region_ops_read(get_cpu_index(), mr, abs_addr, tmp, size); @@ -465,7 +464,6 @@ static MemTxResult memory_region_read_with_attrs_accessor(MemoryRegion *mr, /* Accesses to code which has previously been translated into a TB show * up in the MMIO path, as accesses to the io_mem_notdirty * MemoryRegion. */ - trace_memory_region_tb_read(get_cpu_index(), addr, tmp, size); } else if (TRACE_MEMORY_REGION_OPS_READ_ENABLED) { hwaddr abs_addr = memory_region_to_absolute_addr(mr, addr); trace_memory_region_ops_read(get_cpu_index(), mr, abs_addr, tmp, size); @@ -490,7 +488,6 @@ static MemTxResult memory_region_write_accessor(MemoryRegion *mr, /* Accesses to code which has previously been translated into a TB show * up in the MMIO path, as accesses to the io_mem_notdirty * MemoryRegion. */ - trace_memory_region_tb_write(get_cpu_index(), addr, tmp, size); } else if (TRACE_MEMORY_REGION_OPS_WRITE_ENABLED) { hwaddr abs_addr = memory_region_to_absolute_addr(mr, addr); trace_memory_region_ops_write(get_cpu_index(), mr, abs_addr, tmp, size); @@ -515,7 +512,6 @@ static MemTxResult memory_region_write_with_attrs_accessor(MemoryRegion *mr, /* Accesses to code which has previously been translated into a TB show * up in the MMIO path, as accesses to the io_mem_notdirty * MemoryRegion. */ - trace_memory_region_tb_write(get_cpu_index(), addr, tmp, size); } else if (TRACE_MEMORY_REGION_OPS_WRITE_ENABLED) { hwaddr abs_addr = memory_region_to_absolute_addr(mr, addr); trace_memory_region_ops_write(get_cpu_index(), mr, abs_addr, tmp, size); diff --git a/trace-events b/trace-events index 823a4ae64e..5c9a1631e7 100644 --- a/trace-events +++ b/trace-events @@ -52,14 +52,14 @@ dma_map_wait(void *dbs) "dbs=%p" find_ram_offset(uint64_t size, uint64_t offset) "size: 0x%" PRIx64 " @ 0x%" PRIx64 find_ram_offset_loop(uint64_t size, uint64_t candidate, uint64_t offset, uint64_t next, uint64_t mingap) "trying size: 0x%" PRIx64 " @ 0x%" PRIx64 ", offset: 0x%" PRIx64" next: 0x%" PRIx64 " mingap: 0x%" PRIx64 ram_block_discard_range(const char *rbname, void *hva, size_t length, bool need_madvise, bool need_fallocate, int ret) "%s@%p + 0x%zx: madvise: %d fallocate: %d ret: %d" +memory_notdirty_write(uint64_t vaddr, uint64_t ram_addr, unsigned size) "0x%" PRIx64 " ram_addr 0x%" PRIx64 " size %u" +memory_notdirty_dirty(uint64_t vaddr) "0x%" PRIx64 # 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_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_tb_read(int cpu_index, uint64_t addr, uint64_t value, unsigned size) "cpu %d addr 0x%"PRIx64" value 0x%"PRIx64" size %u" -memory_region_tb_write(int cpu_index, uint64_t addr, uint64_t value, unsigned size) "cpu %d addr 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" memory_region_ram_device_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" flatview_new(void *view, void *root) "%p (root %p)"
The memory_region_tb_read tracepoint is unreachable, since notdirty is supposed to apply only to reads. The memory_region_tb_write tracepoint is mis-named, because notdirty is not only used for TB invalidation. It is also used for e.g. VGA RAM updates. Replace memory_region_tb_write with memory_notdirty_write, and place it in memory_notdirty_write_prepare where it can catch all of the instances. Add memory_notdirty_dirty to log when we no longer intercept writes to a page. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- exec.c | 3 +++ memory.c | 4 ---- trace-events | 4 ++-- 3 files changed, 5 insertions(+), 6 deletions(-) -- 2.17.1