[PATCH v2 0/3] qdev-clock: Minor improvements to the Clock API

[PATCH v2 0/3] qdev-clock: Minor improvements to the Clock API

[Philippe Mathieu-Daudé]

Handy patches while using the Clock API:
- display frequency in SI scaled unit
- display error hint when device lack clock support

Since v1:
- Avoid out-of-bound array access (Luc)

Patch missing review:
- #1 "Introduce freq_to_str() to display Hertz units"

Philippe Mathieu-Daudé (3):
  util/cutils: Introduce freq_to_str() to display Hertz units
  qdev-monitor: Display frequencies scaled to SI unit
  hw/qdev-clock: Display error hint when clock is missing from device

 include/qemu/cutils.h | 12 ++++++++++++
 hw/core/qdev-clock.c  | 11 +++++++++++
 qdev-monitor.c        |  8 +++++---
 util/cutils.c         | 14 ++++++++++++++
 4 files changed, 42 insertions(+), 3 deletions(-)

-- 
2.26.2

[PATCH v2 1/3] util/cutils: Introduce freq_to_str() to display Hertz units

[Philippe Mathieu-Daudé]

Introduce freq_to_str() to convert frequency values in human
friendly units using the SI units for Hertz.

Suggested-by: Luc Michel <luc@lmichel.fr>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/qemu/cutils.h | 12 ++++++++++++
 util/cutils.c         | 14 ++++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
index 3a86ec0321..4bbf4834ea 100644
--- a/include/qemu/cutils.h
+++ b/include/qemu/cutils.h
@@ -158,6 +158,18 @@ int qemu_strtosz_metric(const char *nptr, const char **end, uint64_t *result);
 
 char *size_to_str(uint64_t val);
 
+/**
+ * freq_to_str:
+ * @freq_hz: frequency to stringify
+ *
+ * Return human readable string for frequency @freq_hz.
+ * Use SI units like KHz, MHz, and so forth.
+ *
+ * The caller is responsible for releasing the value returned
+ * with g_free() after use.
+ */
+char *freq_to_str(uint64_t freq_hz);
+
 /* used to print char* safely */
 #define STR_OR_NULL(str) ((str) ? (str) : "null")
 
diff --git a/util/cutils.c b/util/cutils.c
index 8da34e04b0..be4e43a9ef 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -885,6 +885,20 @@ char *size_to_str(uint64_t val)
     return g_strdup_printf("%0.3g %sB", (double)val / div, suffixes[i]);
 }
 
+char *freq_to_str(uint64_t freq_hz)
+{
+    static const char *const suffixes[] = { "", "K", "M", "G", "T", "P", "E" };
+    double freq = freq_hz;
+    size_t idx = 0;
+
+    while (freq >= 1000.0 && idx < ARRAY_SIZE(suffixes)) {
+        freq /= 1000.0;
+        idx++;
+    }
+
+    return g_strdup_printf("%0.3g %sHz", freq, suffixes[idx]);
+}
+
 int qemu_pstrcmp0(const char **str1, const char **str2)
 {
     return g_strcmp0(*str1, *str2);
-- 
2.26.2

Re: [PATCH v2 1/3] util/cutils: Introduce freq_to_str() to display Hertz units

[Alistair Francis]

On Thu, Oct 1, 2020 at 9:57 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Introduce freq_to_str() to convert frequency values in human
> friendly units using the SI units for Hertz.
>
> Suggested-by: Luc Michel <luc@lmichel.fr>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  include/qemu/cutils.h | 12 ++++++++++++
>  util/cutils.c         | 14 ++++++++++++++
>  2 files changed, 26 insertions(+)
>
> diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
> index 3a86ec0321..4bbf4834ea 100644
> --- a/include/qemu/cutils.h
> +++ b/include/qemu/cutils.h
> @@ -158,6 +158,18 @@ int qemu_strtosz_metric(const char *nptr, const char **end, uint64_t *result);
>
>  char *size_to_str(uint64_t val);
>
> +/**
> + * freq_to_str:
> + * @freq_hz: frequency to stringify
> + *
> + * Return human readable string for frequency @freq_hz.
> + * Use SI units like KHz, MHz, and so forth.
> + *
> + * The caller is responsible for releasing the value returned
> + * with g_free() after use.
> + */
> +char *freq_to_str(uint64_t freq_hz);
> +
>  /* used to print char* safely */
>  #define STR_OR_NULL(str) ((str) ? (str) : "null")
>
> diff --git a/util/cutils.c b/util/cutils.c
> index 8da34e04b0..be4e43a9ef 100644
> --- a/util/cutils.c
> +++ b/util/cutils.c
> @@ -885,6 +885,20 @@ char *size_to_str(uint64_t val)
>      return g_strdup_printf("%0.3g %sB", (double)val / div, suffixes[i]);
>  }
>
> +char *freq_to_str(uint64_t freq_hz)
> +{
> +    static const char *const suffixes[] = { "", "K", "M", "G", "T", "P", "E" };
> +    double freq = freq_hz;
> +    size_t idx = 0;
> +
> +    while (freq >= 1000.0 && idx < ARRAY_SIZE(suffixes)) {
> +        freq /= 1000.0;
> +        idx++;
> +    }
> +
> +    return g_strdup_printf("%0.3g %sHz", freq, suffixes[idx]);
> +}
> +
>  int qemu_pstrcmp0(const char **str1, const char **str2)
>  {
>      return g_strcmp0(*str1, *str2);
> --
> 2.26.2
>
>

Re: [PATCH v2 1/3] util/cutils: Introduce freq_to_str() to display Hertz units

[Eduardo Habkost]

On Thu, Oct 01, 2020 at 06:43:20PM +0200, Philippe Mathieu-Daudé wrote:
> Introduce freq_to_str() to convert frequency values in human
> friendly units using the SI units for Hertz.
> 
> Suggested-by: Luc Michel <luc@lmichel.fr>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/qemu/cutils.h | 12 ++++++++++++
>  util/cutils.c         | 14 ++++++++++++++
>  2 files changed, 26 insertions(+)
> 
> diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
> index 3a86ec0321..4bbf4834ea 100644
> --- a/include/qemu/cutils.h
> +++ b/include/qemu/cutils.h
> @@ -158,6 +158,18 @@ int qemu_strtosz_metric(const char *nptr, const char **end, uint64_t *result);
>  
>  char *size_to_str(uint64_t val);
>  
> +/**
> + * freq_to_str:
> + * @freq_hz: frequency to stringify
> + *
> + * Return human readable string for frequency @freq_hz.
> + * Use SI units like KHz, MHz, and so forth.
> + *
> + * The caller is responsible for releasing the value returned
> + * with g_free() after use.
> + */
> +char *freq_to_str(uint64_t freq_hz);
> +
>  /* used to print char* safely */
>  #define STR_OR_NULL(str) ((str) ? (str) : "null")
>  
> diff --git a/util/cutils.c b/util/cutils.c
> index 8da34e04b0..be4e43a9ef 100644
> --- a/util/cutils.c
> +++ b/util/cutils.c
> @@ -885,6 +885,20 @@ char *size_to_str(uint64_t val)
>      return g_strdup_printf("%0.3g %sB", (double)val / div, suffixes[i]);
>  }
>  
> +char *freq_to_str(uint64_t freq_hz)
> +{
> +    static const char *const suffixes[] = { "", "K", "M", "G", "T", "P", "E" };
> +    double freq = freq_hz;
> +    size_t idx = 0;
> +
> +    while (freq >= 1000.0 && idx < ARRAY_SIZE(suffixes)) {
> +        freq /= 1000.0;
> +        idx++;
> +    }
> +
> +    return g_strdup_printf("%0.3g %sHz", freq, suffixes[idx]);

The only thing protecting this from out of bounds array access is
the fact that UINT64_MAX is smaller than 1000E.  I wonder if this
causes a Coverity warning.

> +}
> +
>  int qemu_pstrcmp0(const char **str1, const char **str2)
>  {
>      return g_strcmp0(*str1, *str2);
> -- 
> 2.26.2
> 

-- 
Eduardo

Re: [PATCH v2 1/3] util/cutils: Introduce freq_to_str() to display Hertz units

[Philippe Mathieu-Daudé]

On 10/1/20 8:42 PM, Eduardo Habkost wrote:
> On Thu, Oct 01, 2020 at 06:43:20PM +0200, Philippe Mathieu-Daudé wrote:
>> Introduce freq_to_str() to convert frequency values in human
>> friendly units using the SI units for Hertz.
>>
>> Suggested-by: Luc Michel <luc@lmichel.fr>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>   include/qemu/cutils.h | 12 ++++++++++++
>>   util/cutils.c         | 14 ++++++++++++++
>>   2 files changed, 26 insertions(+)
>>
>> diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
>> index 3a86ec0321..4bbf4834ea 100644
>> --- a/include/qemu/cutils.h
>> +++ b/include/qemu/cutils.h
>> @@ -158,6 +158,18 @@ int qemu_strtosz_metric(const char *nptr, const char **end, uint64_t *result);
>>   
>>   char *size_to_str(uint64_t val);
>>   
>> +/**
>> + * freq_to_str:
>> + * @freq_hz: frequency to stringify
>> + *
>> + * Return human readable string for frequency @freq_hz.
>> + * Use SI units like KHz, MHz, and so forth.
>> + *
>> + * The caller is responsible for releasing the value returned
>> + * with g_free() after use.
>> + */
>> +char *freq_to_str(uint64_t freq_hz);
>> +
>>   /* used to print char* safely */
>>   #define STR_OR_NULL(str) ((str) ? (str) : "null")
>>   
>> diff --git a/util/cutils.c b/util/cutils.c
>> index 8da34e04b0..be4e43a9ef 100644
>> --- a/util/cutils.c
>> +++ b/util/cutils.c
>> @@ -885,6 +885,20 @@ char *size_to_str(uint64_t val)
>>       return g_strdup_printf("%0.3g %sB", (double)val / div, suffixes[i]);
>>   }
>>   
>> +char *freq_to_str(uint64_t freq_hz)
>> +{
>> +    static const char *const suffixes[] = { "", "K", "M", "G", "T", "P", "E" };
>> +    double freq = freq_hz;
>> +    size_t idx = 0;
>> +
>> +    while (freq >= 1000.0 && idx < ARRAY_SIZE(suffixes)) {
>> +        freq /= 1000.0;
>> +        idx++;
>> +    }
>> +
>> +    return g_strdup_printf("%0.3g %sHz", freq, suffixes[idx]);
> 
> The only thing protecting this from out of bounds array access is
> the fact that UINT64_MAX is smaller than 1000E.  I wonder if this
> causes a Coverity warning.

Aren't we protected by the "idx < ARRAY_SIZE(suffixes)" check?

> 
>> +}
>> +
>>   int qemu_pstrcmp0(const char **str1, const char **str2)
>>   {
>>       return g_strcmp0(*str1, *str2);
>> -- 
>> 2.26.2
>>
> 

Re: [PATCH v2 1/3] util/cutils: Introduce freq_to_str() to display Hertz units

[Luc Michel]

On 18:43 Thu 01 Oct     , Philippe Mathieu-Daudé wrote:
> Introduce freq_to_str() to convert frequency values in human
> friendly units using the SI units for Hertz.
> 
> Suggested-by: Luc Michel <luc@lmichel.fr>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Luc Michel <luc@lmichel.fr>

> ---
>  include/qemu/cutils.h | 12 ++++++++++++
>  util/cutils.c         | 14 ++++++++++++++
>  2 files changed, 26 insertions(+)
> 
> diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
> index 3a86ec0321..4bbf4834ea 100644
> --- a/include/qemu/cutils.h
> +++ b/include/qemu/cutils.h
> @@ -158,6 +158,18 @@ int qemu_strtosz_metric(const char *nptr, const char **end, uint64_t *result);
>  
>  char *size_to_str(uint64_t val);
>  
> +/**
> + * freq_to_str:
> + * @freq_hz: frequency to stringify
> + *
> + * Return human readable string for frequency @freq_hz.
> + * Use SI units like KHz, MHz, and so forth.
> + *
> + * The caller is responsible for releasing the value returned
> + * with g_free() after use.
> + */
> +char *freq_to_str(uint64_t freq_hz);
> +
>  /* used to print char* safely */
>  #define STR_OR_NULL(str) ((str) ? (str) : "null")
>  
> diff --git a/util/cutils.c b/util/cutils.c
> index 8da34e04b0..be4e43a9ef 100644
> --- a/util/cutils.c
> +++ b/util/cutils.c
> @@ -885,6 +885,20 @@ char *size_to_str(uint64_t val)
>      return g_strdup_printf("%0.3g %sB", (double)val / div, suffixes[i]);
>  }
>  
> +char *freq_to_str(uint64_t freq_hz)
> +{
> +    static const char *const suffixes[] = { "", "K", "M", "G", "T", "P", "E" };
> +    double freq = freq_hz;
> +    size_t idx = 0;
> +
> +    while (freq >= 1000.0 && idx < ARRAY_SIZE(suffixes)) {
> +        freq /= 1000.0;
> +        idx++;
> +    }
> +
> +    return g_strdup_printf("%0.3g %sHz", freq, suffixes[idx]);
> +}
> +
>  int qemu_pstrcmp0(const char **str1, const char **str2)
>  {
>      return g_strcmp0(*str1, *str2);
> -- 
> 2.26.2
> 

-- 

[PATCH v2 2/3] qdev-monitor: Display frequencies scaled to SI unit

[Philippe Mathieu-Daudé]

Since commit 9f2ff99c7f2 ("qdev-monitor: print the device's clock
with info qtree") we can display the clock frequencies in the
monitor. Use the recently introduced freq_to_str() to display
the frequencies using the closest SI unit (human friendlier).

Before:

  (qemu) info qtree
  [...]
  dev: xilinx,zynq_slcr, id ""
    clock-in "ps_clk" freq_hz=3.333333e+07
    mmio 00000000f8000000/0000000000001000

After:

  dev: xilinx,zynq_slcr, id ""
    clock-in "ps_clk" freq_hz=33.3 MHz
    mmio 00000000f8000000/0000000000001000

Reviewed-by: Luc Michel <luc@lmichel.fr>
Reviewed-by: Damien Hedde <damien.hedde@greensocs.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 qdev-monitor.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/qdev-monitor.c b/qdev-monitor.c
index e9b7228480..a0301cfca8 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -747,11 +747,13 @@ static void qdev_print(Monitor *mon, DeviceState *dev, int indent)
         }
     }
     QLIST_FOREACH(ncl, &dev->clocks, node) {
-        qdev_printf("clock-%s%s \"%s\" freq_hz=%e\n",
+        g_autofree char *freq = NULL;
+
+        freq = freq_to_str(clock_get_hz(ncl->clock));
+        qdev_printf("clock-%s%s \"%s\" freq_hz=%s\n",
                     ncl->output ? "out" : "in",
                     ncl->alias ? " (alias)" : "",
-                    ncl->name,
-                    CLOCK_PERIOD_TO_HZ(1.0 * clock_get(ncl->clock)));
+                    ncl->name, freq);
     }
     class = object_get_class(OBJECT(dev));
     do {
-- 
2.26.2

Re: [PATCH v2 2/3] qdev-monitor: Display frequencies scaled to SI unit

[Alistair Francis]

On Thu, Oct 1, 2020 at 9:43 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Since commit 9f2ff99c7f2 ("qdev-monitor: print the device's clock
> with info qtree") we can display the clock frequencies in the
> monitor. Use the recently introduced freq_to_str() to display
> the frequencies using the closest SI unit (human friendlier).
>
> Before:
>
>   (qemu) info qtree
>   [...]
>   dev: xilinx,zynq_slcr, id ""
>     clock-in "ps_clk" freq_hz=3.333333e+07
>     mmio 00000000f8000000/0000000000001000
>
> After:
>
>   dev: xilinx,zynq_slcr, id ""
>     clock-in "ps_clk" freq_hz=33.3 MHz
>     mmio 00000000f8000000/0000000000001000
>
> Reviewed-by: Luc Michel <luc@lmichel.fr>
> Reviewed-by: Damien Hedde <damien.hedde@greensocs.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  qdev-monitor.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index e9b7228480..a0301cfca8 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -747,11 +747,13 @@ static void qdev_print(Monitor *mon, DeviceState *dev, int indent)
>          }
>      }
>      QLIST_FOREACH(ncl, &dev->clocks, node) {
> -        qdev_printf("clock-%s%s \"%s\" freq_hz=%e\n",
> +        g_autofree char *freq = NULL;
> +
> +        freq = freq_to_str(clock_get_hz(ncl->clock));
> +        qdev_printf("clock-%s%s \"%s\" freq_hz=%s\n",
>                      ncl->output ? "out" : "in",
>                      ncl->alias ? " (alias)" : "",
> -                    ncl->name,
> -                    CLOCK_PERIOD_TO_HZ(1.0 * clock_get(ncl->clock)));
> +                    ncl->name, freq);
>      }
>      class = object_get_class(OBJECT(dev));
>      do {
> --
> 2.26.2
>
>

[PATCH v2 3/3] hw/qdev-clock: Display error hint when clock is missing from device

[Philippe Mathieu-Daudé]

Instead of directly aborting, display a hint to help the developer
figure out the problem (likely trying to connect a clock to a device
pre-dating the Clock API, thus not expecting clocks).

Reviewed-by: Luc Michel <luc@lmichel.fr>
Reviewed-by: Damien Hedde <damien.hedde@greensocs.com>
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/core/qdev-clock.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/hw/core/qdev-clock.c b/hw/core/qdev-clock.c
index 47ecb5b4fa..33bd4a9d52 100644
--- a/hw/core/qdev-clock.c
+++ b/hw/core/qdev-clock.c
@@ -12,6 +12,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/error-report.h"
 #include "hw/qdev-clock.h"
 #include "hw/qdev-core.h"
 #include "qapi/error.h"
@@ -153,6 +154,11 @@ Clock *qdev_get_clock_in(DeviceState *dev, const char *name)
     assert(name);
 
     ncl = qdev_get_clocklist(dev, name);
+    if (!ncl) {
+        error_report("can not find clock-in '%s' for device type '%s'",
+                     name, object_get_typename(OBJECT(dev)));
+        abort();
+    }
     assert(!ncl->output);
 
     return ncl->clock;
@@ -165,6 +171,11 @@ Clock *qdev_get_clock_out(DeviceState *dev, const char *name)
     assert(name);
 
     ncl = qdev_get_clocklist(dev, name);
+    if (!ncl) {
+        error_report("can not find clock-out '%s' for device type '%s'",
+                     name, object_get_typename(OBJECT(dev)));
+        abort();
+    }
     assert(ncl->output);
 
     return ncl->clock;
-- 
2.26.2