Re: [PATCH] memory: Replace DEBUG_UNASSIGNED printf calls by trace events

[Paolo Bonzini <pbonzini@redhat.com>]

On 20/09/19 16:12, Philippe Mathieu-Daudé wrote:
> Now that the unassigned_access CPU hooks have been removed,
> the unassigned_mem_read/write functions are only used for
> debugging purpose.
> Simplify by converting them to in-place trace events.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> Based-on: <20190920125008.13604-1-peter.maydell@linaro.org>
> https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg04668.html
> https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg03705.html
> 
> I first wrote:
> 
>   These functions are declared using the CPUReadMemoryFunc/
>   CPUWriteMemoryFunc prototypes. Since it is confusing to
>   have such prototype only use for debugging, convert them
>   to in-place trace events.
> 
> But it doesn't provide helpful information and is rather confusing.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

I think it's simplest if all series (RISC-V, remove unassigned_access,
this one) go through the RISC-V tree.

Paolo

> ---
>  memory.c     | 24 +++---------------------
>  trace-events |  2 ++
>  2 files changed, 5 insertions(+), 21 deletions(-)
> 
> diff --git a/memory.c b/memory.c
> index 93a05395cf..07e80a637a 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -35,8 +35,6 @@
>  #include "hw/boards.h"
>  #include "migration/vmstate.h"
>  
> -//#define DEBUG_UNASSIGNED
> -
>  static unsigned memory_region_transaction_depth;
>  static bool memory_region_update_pending;
>  static bool ioeventfd_update_pending;
> @@ -1272,23 +1270,6 @@ static void iommu_memory_region_initfn(Object *obj)
>      mr->is_iommu = true;
>  }
>  
> -static uint64_t unassigned_mem_read(void *opaque, hwaddr addr,
> -                                    unsigned size)
> -{
> -#ifdef DEBUG_UNASSIGNED
> -    printf("Unassigned mem read " TARGET_FMT_plx "\n", addr);
> -#endif
> -    return 0;
> -}
> -
> -static void unassigned_mem_write(void *opaque, hwaddr addr,
> -                                 uint64_t val, unsigned size)
> -{
> -#ifdef DEBUG_UNASSIGNED
> -    printf("Unassigned mem write " TARGET_FMT_plx " = 0x%"PRIx64"\n", addr, val);
> -#endif
> -}
> -
>  static bool unassigned_mem_accepts(void *opaque, hwaddr addr,
>                                     unsigned size, bool is_write,
>                                     MemTxAttrs attrs)
> @@ -1437,7 +1418,8 @@ MemTxResult memory_region_dispatch_read(MemoryRegion *mr,
>      MemTxResult r;
>  
>      if (!memory_region_access_valid(mr, addr, size, false, attrs)) {
> -        *pval = unassigned_mem_read(mr, addr, size);
> +        trace_memory_region_invalid_read(size, addr);
> +        *pval = 0; /* FIXME now this value shouldn't be accessed in guest */
>          return MEMTX_DECODE_ERROR;
>      }
>  
> @@ -1481,7 +1463,7 @@ MemTxResult memory_region_dispatch_write(MemoryRegion *mr,
>      unsigned size = memop_size(op);
>  
>      if (!memory_region_access_valid(mr, addr, size, true, attrs)) {
> -        unassigned_mem_write(mr, addr, data, size);
> +        trace_memory_region_invalid_write(size, addr, size << 1, data);
>          return MEMTX_DECODE_ERROR;
>      }
>  
> diff --git a/trace-events b/trace-events
> index 823a4ae64e..83dbeb4b46 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -62,6 +62,8 @@ memory_region_tb_read(int cpu_index, uint64_t addr, uint64_t value, unsigned siz
>  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"
> +memory_region_invalid_read(unsigned size, uint64_t addr) "invalid read size %u addr 0x%"PRIx64
> +memory_region_invalid_write(unsigned size, uint64_t addr, int fmt_width, uint64_t value) "invalid write size %u addr 0x%"PRIx64" value 0x%0*"PRIx64
>  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)"
>