[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] memory: Replace DEBUG_UNASSIGNED printf calls by trace event
From: |
Peter Maydell |
Subject: |
Re: [PATCH] memory: Replace DEBUG_UNASSIGNED printf calls by trace events |
Date: |
Fri, 20 Sep 2019 15:35:28 +0100 |
On Fri, 20 Sep 2019 at 15:29, Philippe Mathieu-Daudé <address@hidden> wrote:
>
> On 9/20/19 4:19 PM, Peter Maydell wrote:
> > On Fri, 20 Sep 2019 at 15:12, Philippe Mathieu-Daudé <address@hidden> 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é <address@hidden>
> >> ---
> >> Based-on: <address@hidden>
> >> 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é <address@hidden>
> >> @@ -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