memory.c | 24 +++--------------------- trace-events | 2 ++ 2 files changed, 5 insertions(+), 21 deletions(-)
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
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)" >
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
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.
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. >
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. > >
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
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.
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
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.
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
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.
© 2016 - 2024 Red Hat, Inc.