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

[Eric Blake]

On 11/14/19 3:26 PM, Philippe Mathieu-Daudé wrote:

> > > -    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.

> 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.

Reporting either 'width:8-bit'/'width:16-bit' (explicit bits) or 
'width:1'/'width:2' (implying byte) is fine by me.  Showing a bus width 
in bytes adequately explains why you are using <<3 (aka converting bits 
to bytes), and how it compares to the previous <<1 (converting bits to 
number of hex characters).  But whichever you pick (tracing bit width 
vs. byte width, and how it differs from previous usage of width as 
output-character count), documenting it in the commit message will make 
life easier to understand the change.

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