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

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

[Philippe Mathieu-Daudé]

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>
---
 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)"
-- 
2.20.1

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

[Paolo Bonzini]

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)"
> 

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

[Peter Maydell]

On Fri, 20 Sep 2019 at 15:17, Paolo Bonzini <pbonzini@redhat.com> wrote:
> I think it's simplest if all series (RISC-V, remove unassigned_access,
> this one) go through the RISC-V tree.

I don't inherently object but IME the risc-v tree tends to move
comparatively slowly. The initial risc-v conversion patchset
should definitely go via the risc-v tree, anyway.

thanks
-- PMM

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

[Palmer Dabbelt]

On Fri, 20 Sep 2019 07:20:34 PDT (-0700), Peter Maydell wrote:
> On Fri, 20 Sep 2019 at 15:17, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> I think it's simplest if all series (RISC-V, remove unassigned_access,
>> this one) go through the RISC-V tree.
>
> I don't inherently object but IME the risc-v tree tends to move
> comparatively slowly. The initial risc-v conversion patchset
> should definitely go via the risc-v tree, anyway.

We still don't have the riscv_cpu_unassigned_access() removal patches in, which 
IIRC got blocked on review but I can no longer dig out of my inbox.  IIRC the 
patches Alistair sent were still "From: Palmer", which means I can't review 
them.

I'm fine taking this on top of those, but it looks like there's still some 
debate about the patch itself.  I don't see a v2.

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

[Alistair Francis]

On Tue, Oct 8, 2019 at 1:41 PM Palmer Dabbelt <palmer@sifive.com> wrote:
>
> On Fri, 20 Sep 2019 07:20:34 PDT (-0700), Peter Maydell wrote:
> > On Fri, 20 Sep 2019 at 15:17, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >> I think it's simplest if all series (RISC-V, remove unassigned_access,
> >> this one) go through the RISC-V tree.
> >
> > I don't inherently object but IME the risc-v tree tends to move
> > comparatively slowly. The initial risc-v conversion patchset
> > should definitely go via the risc-v tree, anyway.
>
> We still don't have the riscv_cpu_unassigned_access() removal patches in, which
> IIRC got blocked on review but I can no longer dig out of my inbox.  IIRC the
> patches Alistair sent were still "From: Palmer", which means I can't review
> them.

The patches are reviewed by Richard and Philippe, they should be ready to merge.

Alistair

>
> I'm fine taking this on top of those, but it looks like there's still some
> debate about the patch itself.  I don't see a v2.
>

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

[Alistair Francis]

On Tue, Oct 8, 2019 at 1:41 PM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Tue, Oct 8, 2019 at 1:41 PM Palmer Dabbelt <palmer@sifive.com> wrote:
> >
> > On Fri, 20 Sep 2019 07:20:34 PDT (-0700), Peter Maydell wrote:
> > > On Fri, 20 Sep 2019 at 15:17, Paolo Bonzini <pbonzini@redhat.com> wrote:
> > >> I think it's simplest if all series (RISC-V, remove unassigned_access,
> > >> this one) go through the RISC-V tree.
> > >
> > > I don't inherently object but IME the risc-v tree tends to move
> > > comparatively slowly. The initial risc-v conversion patchset
> > > should definitely go via the risc-v tree, anyway.
> >
> > We still don't have the riscv_cpu_unassigned_access() removal patches in, which
> > IIRC got blocked on review but I can no longer dig out of my inbox.  IIRC the
> > patches Alistair sent were still "From: Palmer", which means I can't review
> > them.
>
> The patches are reviewed by Richard and Philippe, they should be ready to merge.

Just sent a v2, so you should be able to find them now.

Alistair

>
> Alistair
>
> >
> > I'm fine taking this on top of those, but it looks like there's still some
> > debate about the patch itself.  I don't see a v2.
> >

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

[Peter Maydell]

On Fri, 20 Sep 2019 at 15:12, Philippe Mathieu-Daudé <philmd@redhat.com> 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>
> @@ -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 */

This FIXME comment is not entirely correct.

Unassigned memory will RAZ/WI and the 0 will be seen by:
 * guest CPUs which don't implement a do_transaction_failed hook
   (or which have a hook that doesn't always raise an exception)
 * other transaction masters, such as DMA controllers, if they
   choose to ignore the MemTxResult (or use an API that doesn't
   expose the MemTxResult)

> 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

Do all our trace backends support format strings which use the
"dynamic format width specified via '*'" syntax ?

thanks
-- PMM

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

[Philippe Mathieu-Daudé]

On 9/20/19 4:19 PM, Peter Maydell wrote:
> On Fri, 20 Sep 2019 at 15:12, Philippe Mathieu-Daudé <philmd@redhat.com> 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>
>> @@ -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 */
> 
> This FIXME comment is not entirely correct.
> 
> Unassigned memory will RAZ/WI and the 0 will be seen by:
>  * guest CPUs which don't implement a do_transaction_failed hook
>    (or which have a hook that doesn't always raise an exception)

OK, I thought targets always had to implement do_transaction_failed.

>  * other transaction masters, such as DMA controllers, if they
>    choose to ignore the MemTxResult (or use an API that doesn't
>    expose the MemTxResult)

Didn't think about this one. OK.

I'll replace my FIXME by your 2 comments to make it clear.

>> 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
> 
> Do all our trace backends support format strings which use the
> "dynamic format width specified via '*'" syntax ?

I thought I read a comment about it between Eric/Stefan but I can't find
it, maybe I dreamed it. (Cc'ed Eric).

Regards,

Phil.

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

[Peter Maydell]

On Fri, 20 Sep 2019 at 15:29, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> On 9/20/19 4:19 PM, Peter Maydell wrote:
> > On Fri, 20 Sep 2019 at 15:12, Philippe Mathieu-Daudé <philmd@redhat.com> 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>
> >> @@ -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 */
> >
> > This FIXME comment is not entirely correct.
> >
> > Unassigned memory will RAZ/WI and the 0 will be seen by:
> >  * guest CPUs which don't implement a do_transaction_failed hook
> >    (or which have a hook that doesn't always raise an exception)
>
> OK, I thought targets always had to implement do_transaction_failed.

No, and in fact most don't (only 8 out of 21 architectures have the hook).
In some cases that might be that nobody's got around to it; in other
cases if the RAZ/WI default and no guest CPU exception is what you want,
there's no real need to write a hook function.

> >> 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
> >
> > Do all our trace backends support format strings which use the
> > "dynamic format width specified via '*'" syntax ?
>
> I thought I read a comment about it between Eric/Stefan but I can't find
> it, maybe I dreamed it. (Cc'ed Eric).

If my grep is correct we currently use the syntax already in
gt64120_read, gt64120_write, pflash_io_read, pflash_io_write,
pflash_data_read and pflash_data_write trace events.

thanks
-- PMM

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

[Philippe Mathieu-Daudé]

On 9/20/19 4:35 PM, Peter Maydell wrote:
> On Fri, 20 Sep 2019 at 15:29, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>
>> On 9/20/19 4:19 PM, Peter Maydell wrote:
>>> On Fri, 20 Sep 2019 at 15:12, Philippe Mathieu-Daudé <philmd@redhat.com> 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>
>>>> @@ -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 */
>>>
>>> This FIXME comment is not entirely correct.
>>>
>>> Unassigned memory will RAZ/WI and the 0 will be seen by:
>>>  * guest CPUs which don't implement a do_transaction_failed hook
>>>    (or which have a hook that doesn't always raise an exception)
>>
>> OK, I thought targets always had to implement do_transaction_failed.
> 
> No, and in fact most don't (only 8 out of 21 architectures have the hook).
> In some cases that might be that nobody's got around to it; in other
> cases if the RAZ/WI default and no guest CPU exception is what you want,
> there's no real need to write a hook function.

OK!

>>>> 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
>>>
>>> Do all our trace backends support format strings which use the
>>> "dynamic format width specified via '*'" syntax ?
>>
>> I thought I read a comment about it between Eric/Stefan but I can't find
>> it, maybe I dreamed it. (Cc'ed Eric).
> 
> If my grep is correct we currently use the syntax already in
> gt64120_read, gt64120_write, pflash_io_read, pflash_io_write,
> pflash_data_read and pflash_data_write trace events.

If you use 'git blame' you'll notice I added all of them, so better
let's get a proper confirmation from Stefan :)

I plan to use them more, I find them helpful to directly see the access
size looking at the value width.

Regards,

Phil.

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

[Eric Blake]

[-- Attachment #1.1: Type: text/plain, Size: 1363 bytes --]

On 9/20/19 9:39 AM, Philippe Mathieu-Daudé wrote:

>>> I thought I read a comment about it between Eric/Stefan but I can't find
>>> it, maybe I dreamed it. (Cc'ed Eric).

Not from me.  But looking at scripts/tracetool/format/log_stap.py, I
suspect the dtrace via stap backend cannot support it.

Researching further,

https://sourceware.org/systemtap/langref.pdf

section 9.2 printf, states:

"The printf formatting directives are similar to those of C, except that
they are fully checked for type by the translator."

and does NOT list handling for '*' under precision or width.


>>
>> If my grep is correct we currently use the syntax already in
>> gt64120_read, gt64120_write, pflash_io_read, pflash_io_write,
>> pflash_data_read and pflash_data_write trace events.
> 
> If you use 'git blame' you'll notice I added all of them, so better
> let's get a proper confirmation from Stefan :)
> 
> I plan to use them more, I find them helpful to directly see the access
> size looking at the value width.

You'll probably have to revert that, or else teach the various backend
generators how to dumb-down a format string containing it when coupled
with a backend that doesn't support it natively.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Philippe Mathieu-Daudé]

Cc'ing Daniel and Lluís,

On 9/20/19 5:01 PM, Eric Blake wrote:
> On 9/20/19 9:39 AM, Philippe Mathieu-Daudé wrote:
> 
>>>> I thought I read a comment about it between Eric/Stefan but I can't find
>>>> it, maybe I dreamed it. (Cc'ed Eric).
> 
> Not from me.  But looking at scripts/tracetool/format/log_stap.py, I
> suspect the dtrace via stap backend cannot support it.
> 
> Researching further,
> 
> https://sourceware.org/systemtap/langref.pdf
> 
> section 9.2 printf, states:
> 
> "The printf formatting directives are similar to those of C, except that
> they are fully checked for type by the translator."
> 
> and does NOT list handling for '*' under precision or width.
> 

Thanks for checking this.

>>>
>>> If my grep is correct we currently use the syntax already in
>>> gt64120_read, gt64120_write, pflash_io_read, pflash_io_write,
>>> pflash_data_read and pflash_data_write trace events.
>>
>> If you use 'git blame' you'll notice I added all of them, so better
>> let's get a proper confirmation from Stefan :)
>>
>> I plan to use them more, I find them helpful to directly see the access
>> size looking at the value width.
> 
> You'll probably have to revert that, or else teach the various backend
> generators how to dumb-down a format string containing it when coupled
> with a backend that doesn't support it natively.

OK, I'll see what is doable with the backend generators, else revert/fix
the trace events already introduced :(

Regards,

Phil.