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

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

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.