Re: [PATCH v2 1/3] hw/block/pflash: Remove dynamic field width from trace events
On 11/18/19 8:21 PM, Eric Blake wrote: 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. Yes you are right, I should have documented to avoid wasting review time clearing the confusion.
Re: [PATCH v2 1/3] hw/block/pflash: Remove dynamic field width from trace events
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
Re: [PATCH v2 1/3] hw/block/pflash: Remove dynamic field width from trace events
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
Buglink: https://bugs.launchpad.net/qemu/+bug/1844817
Signed-off-by: Philippe Mathieu-Daudé
---
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.
Re: [PATCH v2 1/3] hw/block/pflash: Remove dynamic field width from trace events
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
Buglink: https://bugs.launchpad.net/qemu/+bug/1844817
Signed-off-by: Philippe Mathieu-Daudé
---
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.
@@ -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.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
[PATCH v2 1/3] hw/block/pflash: Remove dynamic field width from trace events
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
Buglink: https://bugs.launchpad.net/qemu/+bug/1844817
Signed-off-by: Philippe Mathieu-Daudé
---
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);
return ret;
}
@@ -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);
return ret;
}
@@ -414,7 +414,7 @@ static inline void pflash_data_write(PFlashCFI01 *pfl,
hwaddr offset,
{
uint8_t *p = pfl->storage;
-trace_pflash_data_write(offset, width << 1, value, pfl->counter);
+trace_pflash_data_write(offset, width << 3, value, pfl->counter);
switch (width) {
case 1:
p[offset] = value;
@@ -453,7 +453,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
cmd = value;
-trace_pflash_io_write(offset, width, width << 1, value, pfl->wcycle);
+trace_pflash_io_write(offset, width << 3, value, pfl->wcycle);
if (!pfl->wcycle) {
/* Set the device in I/O access mode */
memory_region_rom_device_set_romd(&pfl->mem, false);
diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index 4baca701b7..f2993cdfaa 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -260,7 +260,7 @@ static uint64_t pflash_data_read(PFlashCFI02 *pfl, hwaddr
offset,
{
uint8_t *p = (uint8_t *)pfl->storage + offset;
uint64_t ret = pfl->be ? ldn_be_p(p, width) : ldn_le_p(p, width);
-trace_pflash_data_read(offset, width << 1, ret);
+trace_pflash_data_read(offset, width << 3, ret);
return ret;
}
@@ -385,7 +385,7 @@ static uint64_t pflash_read(void *opaque, hwaddr offset,
unsigned int width)
}
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);
return ret;
}
@@ -432,7 +432,7 @@ static void pflash_write(void *opaque, hwaddr offset,
uint64_t value,
uint8_t *p;
uint8_t cmd;
-trace_pflash_io_write(offset, width, width << 1, value, pfl->wcycle);
+trace_pflash_io_write(offset, width << 3, value, pfl->wcycle);
cmd = value;
if (pfl->cmd != 0xA0) {
/* Reset does nothing during chip erase and sector erase. */
@@ -542,7 +542,7 @@ static void pflash_write(void *opaque, hwaddr offset,
uint64_t value,
}
goto reset_flash;
}
-trace_pflash_data_write(offset, width << 1, value, 0);
+trace_pflash_data_write(offset, width << 3, value, 0);
if (!pfl->ro) {
p = (uint8_t *)pfl->storage + offset;
if (pfl->be) {
diff --git a/hw/block/trace-events b/hw/block/trace-events
index 13d1b21dd4..b9e195e172 100644
--- a/hw/block/trace-events
+++ b/hw/block/trace-events
@@ -8,10 +8,10 @@ fdc_ioport_write(uint8_t reg, uint8_t value) "write reg
0x%02x val 0x%02x"
# pflash_cfi01.c
pflash_reset(void) "reset"
pflash_timer_expired(uint8_t cmd) "command 0x%02x done"
-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_write(uint64_t offset, int width, int fmt_width, uint32_t value,
uint8_t wcycle) "offset:0x%04"PRIx64" width:%d value:0x%0*x wcycle:%u"
-pflash_data_read(uint64_t offset, int width, uint32_t value) "data
offset:0x%04"PRIx64" value:0x%0*x"
-pflash_data_write(uint64_t offset, int width, uint32_t value, uint64_t
counter) "data offset:0x%04"PRIx64" value:0x%0*x counter:0x%016"PRIx64
+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"
+pflash_io_write(uint64_t offset, int width, uint32_t value, uint8_t wcycle)
"offset:0x%04"PRIx64" width:%d value:0x%04x wcycle:%u"
+pflash_data_read(uint64_t offset, int width, uint32_t value) "data
offset:0x%04"PRIx64" width:%d value:0x%04x"
+pflash_data_write(uint64_t offset, int width, uint32_t value, uint64_t
counter) "data offset:0x%04"PRIx64" width:%d value:0x%04x counter:0x%016"PRIx64
pflash_manufacturer_id(uint16_t id) "Read Ma
