Re: [PATCH v2 1/3] hw/block/pflash: Remove dynamic field width from trace events

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

Hi Eric,

On 11/8/19 4:56 PM, Eric Blake wrote:
> On 11/8/19 8:40 AM, Philippe Mathieu-Daudé wrote:
>> Since not all trace backends support dynamic field width in
>> format (dtrace via stap does not), replace by a static field
>> width instead.
>>
>> Reported-by: Eric Blake <eblake@redhat.com>
>> Buglink: https://bugs.launchpad.net/qemu/+bug/1844817
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>   hw/block/pflash_cfi01.c | 8 ++++----
>>   hw/block/pflash_cfi02.c | 8 ++++----
>>   hw/block/trace-events   | 8 ++++----
>>   3 files changed, 12 insertions(+), 12 deletions(-)
>>
>> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
>> index 566c0acb77..787d1196f2 100644
>> --- a/hw/block/pflash_cfi01.c
>> +++ b/hw/block/pflash_cfi01.c
>> @@ -276,7 +276,7 @@ static uint32_t pflash_data_read(PFlashCFI01 *pfl, 
>> hwaddr offset,
>>           DPRINTF("BUG in %s\n", __func__);
>>           abort();
>>       }
>> -    trace_pflash_data_read(offset, width << 1, ret);
>> +    trace_pflash_data_read(offset, width << 3, ret);
> 
> Umm, why is width changing?  That's not mentioned in the commit message.

Previously it was used to set the format width: [1, 2, 4] -> [2, 4, 8].

We usually log the width in byte (accessed at memory location) or bits 
(used by the bus). When using this device I'm custom to think in bus 
access width.

Regardless whichever format we prefer, a change is needed.

> 
>> @@ -389,7 +389,7 @@ static uint32_t pflash_read(PFlashCFI01 *pfl, 
>> hwaddr offset,
>>           break;
>>       }
>> -    trace_pflash_io_read(offset, width, width << 1, ret, pfl->cmd, 
>> pfl->wcycle);
>> +    trace_pflash_io_read(offset, width << 3, ret, pfl->cmd, 
>> pfl->wcycle);
> 
> And even this one is odd.  Matching up to the trace messages:
> 
> 
>> -pflash_io_read(uint64_t offset, int width, int fmt_width, uint32_t 
>> value, uint8_t cmd, uint8_t wcycle) "offset:0x%04"PRIx64" width:%d 
>> value:0x%0*x cmd:0x%02x wcycle:%u"
> 
>> +pflash_io_read(uint64_t offset, int width, uint32_t value, uint8_t 
>> cmd, uint8_t wcycle) "offset:0x%04"PRIx64" width:%d value:0x%04x 
>> cmd:0x%02x wcycle:%u"
> 
> you are changing from:
> 
> "%04"PRIx64" %d %0*x...", offset, width, width << 1, ret,...
> 
> (where width<<1, ret matches *x)
> 
> into
> 
> "%04"PRIx64" %d %04x...", offset, width << 3, ret,...
> 
> where you are now printing a different value for width.

Do you prefer using a "-bit" suffix? As

"offset:0x%04"PRIx64" width:%d-bit value:0x%04x cmd:0x%02x wcycle:%u"

I can also simply remove this information. Ideally I'd revert this patch 
once the we get this format parsable by the SystemTap backend.