[PATCH-for-5.2 0/3] hw/misc/unimp: Improve how offset/value are displayed

[PATCH-for-5.2 0/3] hw/misc/unimp: Improve how offset/value are displayed

[Philippe Mathieu-Daudé]

This series aims to ease looking at the '-d unimp' output reported
by the UnimplementedDevice.

- read/write accesses are now aligned
- the value format width uses the access size
- the offset (address) uses the size of the memory region it belongs

Philippe Mathieu-Daudé (3):
  hw/misc/unimp: Display value after offset
  hw/misc/unimp: Display the value with width of the access size
  hw/misc/unimp: Display the offset with width of the region size

 include/hw/misc/unimp.h |  1 +
 hw/misc/unimp.c         | 14 ++++++++------
 2 files changed, 9 insertions(+), 6 deletions(-)

-- 
2.21.3

[PATCH-for-5.2 1/3] hw/misc/unimp: Display value after offset

[Philippe Mathieu-Daudé]

To better align the read/write accesses, display the value after
the offset (read accesses only display the offset).

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/misc/unimp.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/misc/unimp.c b/hw/misc/unimp.c
index bc4084d344..ee2e536c8b 100644
--- a/hw/misc/unimp.c
+++ b/hw/misc/unimp.c
@@ -22,7 +22,7 @@ static uint64_t unimp_read(void *opaque, hwaddr offset, unsigned size)
 {
     UnimplementedDeviceState *s = UNIMPLEMENTED_DEVICE(opaque);
 
-    qemu_log_mask(LOG_UNIMP, "%s: unimplemented device read "
+    qemu_log_mask(LOG_UNIMP, "%s: unimplemented device read  "
                   "(size %d, offset 0x%" HWADDR_PRIx ")\n",
                   s->name, size, offset);
     return 0;
@@ -34,9 +34,9 @@ static void unimp_write(void *opaque, hwaddr offset,
     UnimplementedDeviceState *s = UNIMPLEMENTED_DEVICE(opaque);
 
     qemu_log_mask(LOG_UNIMP, "%s: unimplemented device write "
-                  "(size %d, value 0x%" PRIx64
-                  ", offset 0x%" HWADDR_PRIx ")\n",
-                  s->name, size, value, offset);
+                  "(size %d, offset 0x%" HWADDR_PRIx
+                  ", value 0x%" PRIx64 ")\n",
+                  s->name, size, offset, value);
 }
 
 static const MemoryRegionOps unimp_ops = {
-- 
2.21.3

Re: [PATCH-for-5.2 1/3] hw/misc/unimp: Display value after offset

[Richard Henderson]

On 8/7/20 7:37 AM, Philippe Mathieu-Daudé wrote:
> To better align the read/write accesses, display the value after
> the offset (read accesses only display the offset).
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[PATCH-for-5.2 2/3] hw/misc/unimp: Display the value with width of the access size

[Philippe Mathieu-Daudé]

To quickly notice the access size, display the value with the
width of the access (i.e. 16-bit access is displayed 0x0000,
while 8-bit access 0x00).

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/misc/unimp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/misc/unimp.c b/hw/misc/unimp.c
index ee2e536c8b..b4b318db1c 100644
--- a/hw/misc/unimp.c
+++ b/hw/misc/unimp.c
@@ -35,8 +35,8 @@ static void unimp_write(void *opaque, hwaddr offset,
 
     qemu_log_mask(LOG_UNIMP, "%s: unimplemented device write "
                   "(size %d, offset 0x%" HWADDR_PRIx
-                  ", value 0x%" PRIx64 ")\n",
-                  s->name, size, offset, value);
+                  ", value 0x%0*" PRIx64 ")\n",
+                  s->name, size, offset, size << 1, value);
 }
 
 static const MemoryRegionOps unimp_ops = {
-- 
2.21.3

Re: [PATCH-for-5.2 2/3] hw/misc/unimp: Display the value with width of the access size

[Richard Henderson]

On 8/7/20 7:37 AM, Philippe Mathieu-Daudé wrote:
> To quickly notice the access size, display the value with the
> width of the access (i.e. 16-bit access is displayed 0x0000,
> while 8-bit access 0x00).
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/misc/unimp.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[PATCH-for-5.2 3/3] hw/misc/unimp: Display the offset with width of the region size

[Philippe Mathieu-Daudé]

To have a better idea of how big is the region where the offset
belongs, display the value with the width of the region size
(i.e. a region of 0x1000 bytes uses 0x000 format).

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/misc/unimp.h |  1 +
 hw/misc/unimp.c         | 10 ++++++----
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/include/hw/misc/unimp.h b/include/hw/misc/unimp.h
index 4c1d13c9bf..c63968a2cd 100644
--- a/include/hw/misc/unimp.h
+++ b/include/hw/misc/unimp.h
@@ -20,6 +20,7 @@
 typedef struct {
     SysBusDevice parent_obj;
     MemoryRegion iomem;
+    unsigned offset_fmt_width;
     char *name;
     uint64_t size;
 } UnimplementedDeviceState;
diff --git a/hw/misc/unimp.c b/hw/misc/unimp.c
index b4b318db1c..7146bfaf77 100644
--- a/hw/misc/unimp.c
+++ b/hw/misc/unimp.c
@@ -23,8 +23,8 @@ static uint64_t unimp_read(void *opaque, hwaddr offset, unsigned size)
     UnimplementedDeviceState *s = UNIMPLEMENTED_DEVICE(opaque);
 
     qemu_log_mask(LOG_UNIMP, "%s: unimplemented device read  "
-                  "(size %d, offset 0x%" HWADDR_PRIx ")\n",
-                  s->name, size, offset);
+                  "(size %d, offset 0x%0*" HWADDR_PRIx ")\n",
+                  s->name, size, s->offset_fmt_width, offset);
     return 0;
 }
 
@@ -34,9 +34,9 @@ static void unimp_write(void *opaque, hwaddr offset,
     UnimplementedDeviceState *s = UNIMPLEMENTED_DEVICE(opaque);
 
     qemu_log_mask(LOG_UNIMP, "%s: unimplemented device write "
-                  "(size %d, offset 0x%" HWADDR_PRIx
+                  "(size %d, offset 0x%0*" HWADDR_PRIx
                   ", value 0x%0*" PRIx64 ")\n",
-                  s->name, size, offset, size << 1, value);
+                  s->name, size, s->offset_fmt_width, offset, size << 1, value);
 }
 
 static const MemoryRegionOps unimp_ops = {
@@ -63,6 +63,8 @@ static void unimp_realize(DeviceState *dev, Error **errp)
         return;
     }
 
+    s->offset_fmt_width = ROUND_UP(64 - clz64(s->size - 1), 4) >> 2;
+
     memory_region_init_io(&s->iomem, OBJECT(s), &unimp_ops, s,
                           s->name, s->size);
     sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->iomem);
-- 
2.21.3

Re: [PATCH-for-5.2 3/3] hw/misc/unimp: Display the offset with width of the region size

[Richard Henderson]

On 8/7/20 7:37 AM, Philippe Mathieu-Daudé wrote:
> +    s->offset_fmt_width = ROUND_UP(64 - clz64(s->size - 1), 4) >> 2;

Better with DIV_ROUND_UP, I think.  Otherwise,

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH-for-5.2 3/3] hw/misc/unimp: Display the offset with width of the region size

[Philippe Mathieu-Daudé]

On 8/12/20 7:34 PM, Richard Henderson wrote:
> On 8/7/20 7:37 AM, Philippe Mathieu-Daudé wrote:
>> +    s->offset_fmt_width = ROUND_UP(64 - clz64(s->size - 1), 4) >> 2;
> 
> Better with DIV_ROUND_UP, I think.  Otherwise,

Thanks for the tip! (and for reviewing the other series) :)

> 
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> 
> 
> r~
>