All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v14 00/11] Build ACPI Heterogeneous Memory Attribute Table (HMAT)
@ 2019-10-28  7:52 Tao Xu
  2019-10-28  7:52 ` [PATCH v14 01/11] util/cutils: Add qemu_strtotime_ns() Tao Xu
                   ` (12 more replies)
  0 siblings, 13 replies; 32+ messages in thread
From: Tao Xu @ 2019-10-28  7:52 UTC (permalink / raw)
  To: mst, imammedo, eblake, ehabkost, marcel.apfelbaum, armbru,
	mdroth, thuth, lvivier
  Cc: jingqi.liu, tao3.xu, fan.du, qemu-devel, jonathan.cameron

This series of patches will build Heterogeneous Memory Attribute Table (HMAT)
according to the command line. The ACPI HMAT describes the memory attributes,
such as memory side cache attributes and bandwidth and latency details,
related to the Memory Proximity Domain.
The software is expected to use HMAT information as hint for optimization.

In the linux kernel, the codes in drivers/acpi/hmat/hmat.c parse and report
the platform's HMAT tables.

The V13 patches link:
https://patchwork.kernel.org/cover/11200909/

Changelog:
v14:
    - Reuse the codes of do_strtosz to build qemu_strtotime_ns
      (Eduardo)
    - Squash patch v13 01/12 and 02/12 together (Daniel and Eduardo)
    - Drop time unit picosecond (Eric)
    - Use qemu ctz64 and clz64 instead of builtin function
v13:
    - Modify some text description
    - Drop "initiator_valid" field in struct NodeInfo
    - Reuse Garray to store the raw bandwidth and bandwidth data
    - Calculate common base unit using range bitmap
    - Add a patch to alculate hmat latency and bandwidth entry list
    - Drop the total_levels option and use readable cache size
    - Remove the unnecessary head file
    - Use decimal notation with appropriate suffix for cache size
v12:
    - Fix a bug that a memory-only node without initiator setting
      doesn't report error. (reported by Danmei Wei)
    - Fix a bug that if HMAT is enabled and without hmat-lb setting,
      QEMU will crash. (reported by Danmei Wei)
v11:
    - Move numa option patches forward.
    - Add num_initiator in Numa_state to record the number of
    initiators.
    - Simplify struct HMAT_LB_Info, use uint64_t array to store data.
    - Drop hmat_get_base().
    - Calculate base in build_hmat_lb().
v10:
    - Add qemu_strtotime_ps() to convert strings with time suffixes
    to numbers, and add some tests for it.
    - Add qapi buildin type time, and add some tests for it.
    - Add machine oprion properties "-machine hmat=on|off" for
	  enabling or disabling HMAT in QEMU.

Liu Jingqi (5):
  numa: Extend CLI to provide memory latency and bandwidth information
  numa: Extend CLI to provide memory side cache information
  hmat acpi: Build Memory Proximity Domain Attributes Structure(s)
  hmat acpi: Build System Locality Latency and Bandwidth Information
    Structure(s)
  hmat acpi: Build Memory Side Cache Information Structure(s)

Tao Xu (6):
  util/cutils: Add qemu_strtotime_ns()
  qapi: Add builtin type time
  tests: Add test for QAPI builtin type time
  numa: Extend CLI to provide initiator information for numa nodes
  numa: Calculate hmat latency and bandwidth entry list
  tests/bios-tables-test: add test cases for ACPI HMAT

 hw/acpi/Kconfig                    |   7 +-
 hw/acpi/Makefile.objs              |   1 +
 hw/acpi/hmat.c                     | 263 ++++++++++++++++++++++++++
 hw/acpi/hmat.h                     |  42 +++++
 hw/core/machine.c                  |  64 +++++++
 hw/core/numa.c                     | 284 ++++++++++++++++++++++++++++-
 hw/i386/acpi-build.c               |   5 +
 include/qapi/visitor-impl.h        |   4 +
 include/qapi/visitor.h             |   8 +
 include/qemu/cutils.h              |   1 +
 include/sysemu/numa.h              | 104 +++++++++++
 qapi/machine.json                  | 178 +++++++++++++++++-
 qapi/opts-visitor.c                |  22 +++
 qapi/qapi-visit-core.c             |  12 ++
 qapi/qobject-input-visitor.c       |  18 ++
 qapi/trace-events                  |   1 +
 qemu-options.hx                    |  96 +++++++++-
 scripts/qapi/schema.py             |   1 +
 tests/bios-tables-test.c           |  44 +++++
 tests/test-cutils.c                | 204 +++++++++++++++++++++
 tests/test-keyval.c                | 122 +++++++++++++
 tests/test-qobject-input-visitor.c |  29 +++
 util/cutils.c                      |  89 +++++----
 23 files changed, 1555 insertions(+), 44 deletions(-)
 create mode 100644 hw/acpi/hmat.c
 create mode 100644 hw/acpi/hmat.h

-- 
2.20.1



^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v14 01/11] util/cutils: Add qemu_strtotime_ns()
  2019-10-28  7:52 [PATCH v14 00/11] Build ACPI Heterogeneous Memory Attribute Table (HMAT) Tao Xu
@ 2019-10-28  7:52 ` Tao Xu
  2019-11-06 19:56   ` Eduardo Habkost
  2019-10-28  7:52 ` [PATCH v14 02/11] qapi: Add builtin type time Tao Xu
                   ` (11 subsequent siblings)
  12 siblings, 1 reply; 32+ messages in thread
From: Tao Xu @ 2019-10-28  7:52 UTC (permalink / raw)
  To: mst, imammedo, eblake, ehabkost, marcel.apfelbaum, armbru,
	mdroth, thuth, lvivier
  Cc: jingqi.liu, tao3.xu, fan.du, qemu-devel, jonathan.cameron

To convert strings with time suffixes to numbers, support time unit are
"ns" for nanosecond, "us" for microsecond, "ms" for millisecond or "s"
for second. Add test for qemu_strtotime_ns, test the input of basic,
time suffixes, float, invaild, trailing and overflow.

Signed-off-by: Tao Xu <tao3.xu@intel.com>
---

Changes in v14:
    - Reuse the codes of do_strtosz to build qemu_strtotime_ns
      (Eduardo)
    - Squash patch v13 01/12 and 02/12 together (Daniel and Eduardo)
    - Drop time unit picosecond (Eric)
---
 include/qemu/cutils.h |   1 +
 tests/test-cutils.c   | 204 ++++++++++++++++++++++++++++++++++++++++++
 util/cutils.c         |  89 +++++++++++-------
 3 files changed, 262 insertions(+), 32 deletions(-)

diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
index b54c847e0f..ff2b3f4614 100644
--- a/include/qemu/cutils.h
+++ b/include/qemu/cutils.h
@@ -182,5 +182,6 @@ int uleb128_decode_small(const uint8_t *in, uint32_t *n);
  * *str1 is <, == or > than *str2.
  */
 int qemu_pstrcmp0(const char **str1, const char **str2);
+int qemu_strtotime_ns(const char *nptr, const char **end, uint64_t *result);
 
 #endif
diff --git a/tests/test-cutils.c b/tests/test-cutils.c
index 1aa8351520..d6a0824efd 100644
--- a/tests/test-cutils.c
+++ b/tests/test-cutils.c
@@ -2179,6 +2179,198 @@ static void test_qemu_strtosz_metric(void)
     g_assert(endptr == str + 6);
 }
 
+static void test_qemu_strtotime_ns_simple(void)
+{
+    const char *str;
+    const char *endptr;
+    int err;
+    uint64_t res = 0xbaadf00d;
+
+    str = "0";
+    err = qemu_strtotime_ns(str, &endptr, &res);
+    g_assert_cmpint(err, ==, 0);
+    g_assert_cmpint(res, ==, 0);
+    g_assert(endptr == str + 1);
+
+    str = "56789";
+    err = qemu_strtotime_ns(str, &endptr, &res);
+    g_assert_cmpint(err, ==, 0);
+    g_assert_cmpint(res, ==, 56789);
+    g_assert(endptr == str + 5);
+
+    err = qemu_strtotime_ns(str, NULL, &res);
+    g_assert_cmpint(err, ==, 0);
+    g_assert_cmpint(res, ==, 56789);
+
+    /* Note: precision is 53 bits since we're parsing with strtod() */
+
+    str = "9007199254740991"; /* 2^53-1 */
+    err = qemu_strtotime_ns(str, &endptr, &res);
+    g_assert_cmpint(err, ==, 0);
+    g_assert_cmpint(res, ==, 0x1fffffffffffff);
+    g_assert(endptr == str + 16);
+
+    str = "9007199254740992"; /* 2^53 */
+    err = qemu_strtotime_ns(str, &endptr, &res);
+    g_assert_cmpint(err, ==, 0);
+    g_assert_cmpint(res, ==, 0x20000000000000);
+    g_assert(endptr == str + 16);
+
+    str = "9007199254740993"; /* 2^53+1 */
+    err = qemu_strtotime_ns(str, &endptr, &res);
+    g_assert_cmpint(err, ==, 0);
+    g_assert_cmpint(res, ==, 0x20000000000000); /* rounded to 53 bits */
+    g_assert(endptr == str + 16);
+
+    str = "18446744073709549568"; /* 0xfffffffffffff800 (53 msbs set) */
+    err = qemu_strtotime_ns(str, &endptr, &res);
+    g_assert_cmpint(err, ==, 0);
+    g_assert_cmpint(res, ==, 0xfffffffffffff800);
+    g_assert(endptr == str + 20);
+
+    str = "18446744073709550591"; /* 0xfffffffffffffbff */
+    err = qemu_strtotime_ns(str, &endptr, &res);
+    g_assert_cmpint(err, ==, 0);
+    g_assert_cmpint(res, ==, 0xfffffffffffff800); /* rounded to 53 bits */
+    g_assert(endptr == str + 20);
+
+    /* 0x7ffffffffffffe00..0x7fffffffffffffff get rounded to
+     * 0x8000000000000000, thus -ERANGE; see test_qemu_strtosz_erange() */
+}
+
+static void test_qemu_strtotime_ns_units(void)
+{
+    const char *ns = "1ns";
+    const char *us = "1us";
+    const char *ms = "1ms";
+    const char *s = "1s";
+    int err;
+    const char *endptr;
+    uint64_t res = 0xbaadf00d;
+
+    /* default time unit is ns */
+    err = qemu_strtotime_ns(ns, &endptr, &res);
+    g_assert_cmpint(err, ==, 0);
+    g_assert_cmpint(res, ==, 1);
+    g_assert(endptr == ns + 3);
+
+    err = qemu_strtotime_ns(us, &endptr, &res);
+    g_assert_cmpint(err, ==, 0);
+    g_assert_cmpint(res, ==, 1000);
+    g_assert(endptr == us + 3);
+
+    err = qemu_strtotime_ns(ms, &endptr, &res);
+    g_assert_cmpint(err, ==, 0);
+    g_assert_cmpint(res, ==, 1000000);
+    g_assert(endptr == ms + 3);
+
+    err = qemu_strtotime_ns(s, &endptr, &res);
+    g_assert_cmpint(err, ==, 0);
+    g_assert_cmpint(res, ==, 1000000000LL);
+    g_assert(endptr == s + 2);
+}
+
+static void test_qemu_strtotime_ns_float(void)
+{
+    const char *str = "56.789us";
+    int err;
+    const char *endptr;
+    uint64_t res = 0xbaadf00d;
+
+    err = qemu_strtotime_ns(str, &endptr, &res);
+    g_assert_cmpint(err, ==, 0);
+    g_assert_cmpint(res, ==, 56.789 * 1000);
+    g_assert(endptr == str + 8);
+}
+
+static void test_qemu_strtotime_ns_invalid(void)
+{
+    const char *str;
+    const char *endptr;
+    int err;
+    uint64_t res = 0xbaadf00d;
+
+    str = "";
+    err = qemu_strtotime_ns(str, &endptr, &res);
+    g_assert_cmpint(err, ==, -EINVAL);
+    g_assert(endptr == str);
+
+    str = " \t ";
+    err = qemu_strtotime_ns(str, &endptr, &res);
+    g_assert_cmpint(err, ==, -EINVAL);
+    g_assert(endptr == str);
+
+    str = "crap";
+    err = qemu_strtotime_ns(str, &endptr, &res);
+    g_assert_cmpint(err, ==, -EINVAL);
+    g_assert(endptr == str);
+
+    str = "inf";
+    err = qemu_strtotime_ns(str, &endptr, &res);
+    g_assert_cmpint(err, ==, -EINVAL);
+    g_assert(endptr == str);
+
+    str = "NaN";
+    err = qemu_strtotime_ns(str, &endptr, &res);
+    g_assert_cmpint(err, ==, -EINVAL);
+    g_assert(endptr == str);
+}
+
+static void test_qemu_strtotime_ns_trailing(void)
+{
+    const char *str;
+    const char *endptr;
+    int err;
+    uint64_t res = 0xbaadf00d;
+
+    str = "123xxx";
+
+    err = qemu_strtotime_ns(str, NULL, &res);
+    g_assert_cmpint(err, ==, -EINVAL);
+
+    str = "1msxxx";
+    err = qemu_strtotime_ns(str, &endptr, &res);
+    g_assert_cmpint(err, ==, 0);
+    g_assert_cmpint(res, ==, 1000000);
+    g_assert(endptr == str + 3);
+
+    err = qemu_strtotime_ns(str, NULL, &res);
+    g_assert_cmpint(err, ==, -EINVAL);
+}
+
+static void test_qemu_strtotime_ns_erange(void)
+{
+    const char *str;
+    const char *endptr;
+    int err;
+    uint64_t res = 0xbaadf00d;
+
+    str = "-1";
+    err = qemu_strtotime_ns(str, &endptr, &res);
+    g_assert_cmpint(err, ==, -ERANGE);
+    g_assert(endptr == str + 2);
+
+    str = "18446744073709550592"; /* 0xfffffffffffffc00 */
+    err = qemu_strtotime_ns(str, &endptr, &res);
+    g_assert_cmpint(err, ==, -ERANGE);
+    g_assert(endptr == str + 20);
+
+    str = "18446744073709551615"; /* 2^64-1 */
+    err = qemu_strtotime_ns(str, &endptr, &res);
+    g_assert_cmpint(err, ==, -ERANGE);
+    g_assert(endptr == str + 20);
+
+    str = "18446744073709551616"; /* 2^64 */
+    err = qemu_strtotime_ns(str, &endptr, &res);
+    g_assert_cmpint(err, ==, -ERANGE);
+    g_assert(endptr == str + 20);
+
+    str = "200000000000000ms";
+    err = qemu_strtotime_ns(str, &endptr, &res);
+    g_assert_cmpint(err, ==, -ERANGE);
+    g_assert(endptr == str + 17);
+}
+
 int main(int argc, char **argv)
 {
     g_test_init(&argc, &argv, NULL);
@@ -2456,5 +2648,17 @@ int main(int argc, char **argv)
     g_test_add_func("/cutils/strtosz/metric",
                     test_qemu_strtosz_metric);
 
+    g_test_add_func("/cutils/strtotime/simple",
+                    test_qemu_strtotime_ns_simple);
+    g_test_add_func("/cutils/strtotime/units",
+                    test_qemu_strtotime_ns_units);
+    g_test_add_func("/cutils/strtotime/float",
+                    test_qemu_strtotime_ns_float);
+    g_test_add_func("/cutils/strtotime/invalid",
+                    test_qemu_strtotime_ns_invalid);
+    g_test_add_func("/cutils/strtotime/trailing",
+                    test_qemu_strtotime_ns_trailing);
+    g_test_add_func("/cutils/strtotime/erange",
+                    test_qemu_strtotime_ns_erange);
     return g_test_run();
 }
diff --git a/util/cutils.c b/util/cutils.c
index fd591cadf0..d83825f8b4 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -181,41 +181,38 @@ int fcntl_setfl(int fd, int flag)
 }
 #endif
 
-static int64_t suffix_mul(char suffix, int64_t unit)
-{
-    switch (qemu_toupper(suffix)) {
-    case 'B':
-        return 1;
-    case 'K':
-        return unit;
-    case 'M':
-        return unit * unit;
-    case 'G':
-        return unit * unit * unit;
-    case 'T':
-        return unit * unit * unit * unit;
-    case 'P':
-        return unit * unit * unit * unit * unit;
-    case 'E':
-        return unit * unit * unit * unit * unit * unit;
+static int64_t suffix_mul(const char *suffixes[], int num_suffix,
+                          const char *endptr, int *offset, int64_t unit)
+{
+    int i, suffix_len;
+    int64_t mul = 1;
+
+    for (i = 0; i < num_suffix; i++) {
+        suffix_len = strlen(suffixes[i]);
+        if (strlen(endptr) >= suffix_len &&
+            g_ascii_strncasecmp(suffixes[i], endptr, suffix_len) == 0) {
+            *offset = suffix_len;
+            return mul;
+        }
+        mul *= unit;
     }
+
     return -1;
 }
 
 /*
- * Convert string to bytes, allowing either B/b for bytes, K/k for KB,
- * M/m for MB, G/g for GB or T/t for TB. End pointer will be returned
- * in *end, if not NULL. Return -ERANGE on overflow, and -EINVAL on
- * other error.
+ * Convert string according to different suffixes. End pointer will be returned
+ * in *end, if not NULL. Return -ERANGE on overflow, and -EINVAL on other error.
  */
-static int do_strtosz(const char *nptr, const char **end,
-                      const char default_suffix, int64_t unit,
-                      uint64_t *result)
+static int do_strtomul(const char *nptr, const char **end,
+                       const char *suffixes[], int num_suffix,
+                       const char *default_suffix, int64_t unit,
+                       uint64_t *result)
 {
     int retval;
     const char *endptr;
-    unsigned char c;
     int mul_required = 0;
+    int offset = 0;
     double val, mul, integral, fraction;
 
     retval = qemu_strtod_finite(nptr, &endptr, &val);
@@ -226,12 +223,12 @@ static int do_strtosz(const char *nptr, const char **end,
     if (fraction != 0) {
         mul_required = 1;
     }
-    c = *endptr;
-    mul = suffix_mul(c, unit);
+
+    mul = suffix_mul(suffixes, num_suffix, endptr, &offset, unit);
     if (mul >= 0) {
-        endptr++;
+        endptr += offset;
     } else {
-        mul = suffix_mul(default_suffix, unit);
+        mul = suffix_mul(suffixes, num_suffix, default_suffix, &offset, unit);
         assert(mul >= 0);
     }
     if (mul == 1 && mul_required) {
@@ -259,19 +256,47 @@ out:
     return retval;
 }
 
+/*
+ * Convert string to bytes, allowing either B/b for bytes, K/k for KB,
+ * M/m for MB, G/g for GB or T/t for TB. End pointer will be returned
+ * in *end, if not NULL. Return -ERANGE on overflow, and -EINVAL on
+ * other error.
+ */
+static int do_strtosz(const char *nptr, const char **end,
+                      const char *default_suffix, int64_t unit,
+                      uint64_t *result)
+{
+    static const char *suffixes[] = { "B", "K", "M", "G", "T", "P", "E" };
+
+    return do_strtomul(nptr, end, suffixes, 7, default_suffix, unit, result);
+}
+
 int qemu_strtosz(const char *nptr, const char **end, uint64_t *result)
 {
-    return do_strtosz(nptr, end, 'B', 1024, result);
+    return do_strtosz(nptr, end, "B", 1024, result);
 }
 
 int qemu_strtosz_MiB(const char *nptr, const char **end, uint64_t *result)
 {
-    return do_strtosz(nptr, end, 'M', 1024, result);
+    return do_strtosz(nptr, end, "M", 1024, result);
 }
 
 int qemu_strtosz_metric(const char *nptr, const char **end, uint64_t *result)
 {
-    return do_strtosz(nptr, end, 'B', 1000, result);
+    return do_strtosz(nptr, end, "B", 1000, result);
+}
+
+/*
+ * Convert string to time, support time unit are ns for nanosecond, us for
+ * microsecond, ms for millisecond and s for second. End pointer will be
+ * returned in *end, if not NULL. Return -ERANGE on overflow, and -EINVAL on
+ * other error.
+ */
+int qemu_strtotime_ns(const char *nptr, const char **end, uint64_t *result)
+{
+    static const char *suffixes[] = { "ns", "us", "ms", "s" };
+
+    return do_strtomul(nptr, end, suffixes, 4, "ns", 1000, result);
 }
 
 /**
-- 
2.20.1



^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v14 02/11] qapi: Add builtin type time
  2019-10-28  7:52 [PATCH v14 00/11] Build ACPI Heterogeneous Memory Attribute Table (HMAT) Tao Xu
  2019-10-28  7:52 ` [PATCH v14 01/11] util/cutils: Add qemu_strtotime_ns() Tao Xu
@ 2019-10-28  7:52 ` Tao Xu
  2019-10-28  7:52 ` [PATCH v14 03/11] tests: Add test for QAPI " Tao Xu
                   ` (10 subsequent siblings)
  12 siblings, 0 replies; 32+ messages in thread
From: Tao Xu @ 2019-10-28  7:52 UTC (permalink / raw)
  To: mst, imammedo, eblake, ehabkost, marcel.apfelbaum, armbru,
	mdroth, thuth, lvivier
  Cc: jingqi.liu, tao3.xu, fan.du, qemu-devel, jonathan.cameron

Add optional builtin type time, fallback is uint64. This type use
qemu_strtotime_ns() for pre-converting time suffix to numbers.

Signed-off-by: Tao Xu <tao3.xu@intel.com>
---

Changes in v14:
    - Drop time unit picosecond (Eric)
---
 include/qapi/visitor-impl.h  |  4 ++++
 include/qapi/visitor.h       |  8 ++++++++
 qapi/opts-visitor.c          | 22 ++++++++++++++++++++++
 qapi/qapi-visit-core.c       | 12 ++++++++++++
 qapi/qobject-input-visitor.c | 18 ++++++++++++++++++
 qapi/trace-events            |  1 +
 scripts/qapi/schema.py       |  1 +
 7 files changed, 66 insertions(+)

diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index 8ccb3b6c20..e0979563c7 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -88,6 +88,10 @@ struct Visitor
     void (*type_size)(Visitor *v, const char *name, uint64_t *obj,
                       Error **errp);
 
+    /* Optional; fallback is type_uint64() */
+    void (*type_time)(Visitor *v, const char *name, uint64_t *obj,
+                      Error **errp);
+
     /* Must be set */
     void (*type_bool)(Visitor *v, const char *name, bool *obj, Error **errp);
 
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index c5b23851a1..22242e706f 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -554,6 +554,14 @@ void visit_type_int64(Visitor *v, const char *name, int64_t *obj,
 void visit_type_size(Visitor *v, const char *name, uint64_t *obj,
                      Error **errp);
 
+/*
+ * Visit a uint64_t value.
+ * Like visit_type_uint64(), except that some visitors may choose to
+ * recognize numbers with timeunit suffix, such as "ns", "us" "ms" and "s".
+ */
+void visit_type_time(Visitor *v, const char *name, uint64_t *obj,
+                     Error **errp);
+
 /*
  * Visit a boolean value.
  *
diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index 5fe0276c1c..59b575f0fc 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -526,6 +526,27 @@ opts_type_size(Visitor *v, const char *name, uint64_t *obj, Error **errp)
     processed(ov, name);
 }
 
+static void
+opts_type_time(Visitor *v, const char *name, uint64_t *obj, Error **errp)
+{
+    OptsVisitor *ov = to_ov(v);
+    const QemuOpt *opt;
+    int err;
+
+    opt = lookup_scalar(ov, name, errp);
+    if (!opt) {
+        return;
+    }
+
+    err = qemu_strtotime_ns(opt->str ? opt->str : "", NULL, obj);
+    if (err < 0) {
+        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, opt->name,
+                   "a time value");
+        return;
+    }
+
+    processed(ov, name);
+}
 
 static void
 opts_optional(Visitor *v, const char *name, bool *present)
@@ -573,6 +594,7 @@ opts_visitor_new(const QemuOpts *opts)
     ov->visitor.type_int64  = &opts_type_int64;
     ov->visitor.type_uint64 = &opts_type_uint64;
     ov->visitor.type_size   = &opts_type_size;
+    ov->visitor.type_time   = &opts_type_time;
     ov->visitor.type_bool   = &opts_type_bool;
     ov->visitor.type_str    = &opts_type_str;
 
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index 5365561b07..ac8896455c 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -277,6 +277,18 @@ void visit_type_size(Visitor *v, const char *name, uint64_t *obj,
     }
 }
 
+void visit_type_time(Visitor *v, const char *name, uint64_t *obj,
+                     Error **errp)
+{
+    assert(obj);
+    trace_visit_type_time(v, name, obj);
+    if (v->type_time) {
+        v->type_time(v, name, obj, errp);
+    } else {
+        v->type_uint64(v, name, obj, errp);
+    }
+}
+
 void visit_type_bool(Visitor *v, const char *name, bool *obj, Error **errp)
 {
     assert(obj);
diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
index 32236cbcb1..e476fe0d16 100644
--- a/qapi/qobject-input-visitor.c
+++ b/qapi/qobject-input-visitor.c
@@ -627,6 +627,23 @@ static void qobject_input_type_size_keyval(Visitor *v, const char *name,
     }
 }
 
+static void qobject_input_type_time_keyval(Visitor *v, const char *name,
+                                           uint64_t *obj, Error **errp)
+{
+    QObjectInputVisitor *qiv = to_qiv(v);
+    const char *str = qobject_input_get_keyval(qiv, name, errp);
+
+    if (!str) {
+        return;
+    }
+
+    if (qemu_strtotime_ns(str, NULL, obj) < 0) {
+        /* TODO report -ERANGE more nicely */
+        error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
+                   full_name(qiv, name), "time");
+    }
+}
+
 static void qobject_input_optional(Visitor *v, const char *name, bool *present)
 {
     QObjectInputVisitor *qiv = to_qiv(v);
@@ -708,6 +725,7 @@ Visitor *qobject_input_visitor_new_keyval(QObject *obj)
     v->visitor.type_any = qobject_input_type_any;
     v->visitor.type_null = qobject_input_type_null;
     v->visitor.type_size = qobject_input_type_size_keyval;
+    v->visitor.type_time = qobject_input_type_time_keyval;
     v->keyval = true;
 
     return &v->visitor;
diff --git a/qapi/trace-events b/qapi/trace-events
index 5eb4afa110..c4605a7ccc 100644
--- a/qapi/trace-events
+++ b/qapi/trace-events
@@ -29,6 +29,7 @@ visit_type_int16(void *v, const char *name, int16_t *obj) "v=%p name=%s obj=%p"
 visit_type_int32(void *v, const char *name, int32_t *obj) "v=%p name=%s obj=%p"
 visit_type_int64(void *v, const char *name, int64_t *obj) "v=%p name=%s obj=%p"
 visit_type_size(void *v, const char *name, uint64_t *obj) "v=%p name=%s obj=%p"
+visit_type_time(void *v, const char *name, uint64_t *obj) "v=%p name=%s obj=%p"
 visit_type_bool(void *v, const char *name, bool *obj) "v=%p name=%s obj=%p"
 visit_type_str(void *v, const char *name, char **obj) "v=%p name=%s obj=%p"
 visit_type_number(void *v, const char *name, void *obj) "v=%p name=%s obj=%p"
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index f7d68a35f4..25c1037e75 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -835,6 +835,7 @@ class QAPISchema(object):
                   ('uint32', 'int',     'uint32_t'),
                   ('uint64', 'int',     'uint64_t'),
                   ('size',   'int',     'uint64_t'),
+                  ('time',   'int',     'uint64_t'),
                   ('bool',   'boolean', 'bool'),
                   ('any',    'value',   'QObject' + pointer_suffix),
                   ('null',   'null',    'QNull' + pointer_suffix)]:
-- 
2.20.1



^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v14 03/11] tests: Add test for QAPI builtin type time
  2019-10-28  7:52 [PATCH v14 00/11] Build ACPI Heterogeneous Memory Attribute Table (HMAT) Tao Xu
  2019-10-28  7:52 ` [PATCH v14 01/11] util/cutils: Add qemu_strtotime_ns() Tao Xu
  2019-10-28  7:52 ` [PATCH v14 02/11] qapi: Add builtin type time Tao Xu
@ 2019-10-28  7:52 ` Tao Xu
  2019-11-06 20:53   ` Eduardo Habkost
  2019-10-28  7:52 ` [PATCH v14 04/11] numa: Extend CLI to provide initiator information for numa nodes Tao Xu
                   ` (9 subsequent siblings)
  12 siblings, 1 reply; 32+ messages in thread
From: Tao Xu @ 2019-10-28  7:52 UTC (permalink / raw)
  To: mst, imammedo, eblake, ehabkost, marcel.apfelbaum, armbru,
	mdroth, thuth, lvivier
  Cc: jingqi.liu, tao3.xu, fan.du, qemu-devel, jonathan.cameron

Add tests for time input such as zero, around limit of precision,
signed upper limit, actual upper limit, beyond limits, time suffixes,
and etc.

Signed-off-by: Tao Xu <tao3.xu@intel.com>
---

Changes in v14:
    - Drop time unit picosecond (Eric)
---
 tests/test-keyval.c                | 122 +++++++++++++++++++++++++++++
 tests/test-qobject-input-visitor.c |  29 +++++++
 2 files changed, 151 insertions(+)

diff --git a/tests/test-keyval.c b/tests/test-keyval.c
index 09b0ae3c68..28e2e34084 100644
--- a/tests/test-keyval.c
+++ b/tests/test-keyval.c
@@ -490,6 +490,127 @@ static void test_keyval_visit_size(void)
     visit_free(v);
 }
 
+static void test_keyval_visit_time(void)
+{
+    Error *err = NULL;
+    Visitor *v;
+    QDict *qdict;
+    uint64_t time;
+
+    /* Lower limit zero */
+    qdict = keyval_parse("time1=0", NULL, &error_abort);
+    v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
+    qobject_unref(qdict);
+    visit_start_struct(v, NULL, NULL, 0, &error_abort);
+    visit_type_time(v, "time1", &time, &error_abort);
+    g_assert_cmpuint(time, ==, 0);
+    visit_check_struct(v, &error_abort);
+    visit_end_struct(v, NULL);
+    visit_free(v);
+
+    /* Around limit of precision: 2^53-1, 2^53, 2^53+1 */
+    qdict = keyval_parse("time1=9007199254740991,"
+                         "time2=9007199254740992,"
+                         "time3=9007199254740993",
+                         NULL, &error_abort);
+    v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
+    qobject_unref(qdict);
+    visit_start_struct(v, NULL, NULL, 0, &error_abort);
+    visit_type_time(v, "time1", &time, &error_abort);
+    g_assert_cmphex(time, ==, 0x1fffffffffffff);
+    visit_type_time(v, "time2", &time, &error_abort);
+    g_assert_cmphex(time, ==, 0x20000000000000);
+    visit_type_time(v, "time3", &time, &error_abort);
+    g_assert_cmphex(time, ==, 0x20000000000000);
+    visit_check_struct(v, &error_abort);
+    visit_end_struct(v, NULL);
+    visit_free(v);
+
+    /* Close to signed upper limit 0x7ffffffffffffc00 (53 msbs set) */
+    qdict = keyval_parse("time1=9223372036854774784," /* 7ffffffffffffc00 */
+                         "time2=9223372036854775295", /* 7ffffffffffffdff */
+                         NULL, &error_abort);
+    v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
+    qobject_unref(qdict);
+    visit_start_struct(v, NULL, NULL, 0, &error_abort);
+    visit_type_time(v, "time1", &time, &error_abort);
+    g_assert_cmphex(time, ==, 0x7ffffffffffffc00);
+    visit_type_time(v, "time2", &time, &error_abort);
+    g_assert_cmphex(time, ==, 0x7ffffffffffffc00);
+    visit_check_struct(v, &error_abort);
+    visit_end_struct(v, NULL);
+    visit_free(v);
+
+    /* Close to actual upper limit 0xfffffffffffff800 (53 msbs set) */
+    qdict = keyval_parse("time1=18446744073709549568," /* fffffffffffff800 */
+                         "time2=18446744073709550591", /* fffffffffffffbff */
+                         NULL, &error_abort);
+    v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
+    qobject_unref(qdict);
+    visit_start_struct(v, NULL, NULL, 0, &error_abort);
+    visit_type_time(v, "time1", &time, &error_abort);
+    g_assert_cmphex(time, ==, 0xfffffffffffff800);
+    visit_type_time(v, "time2", &time, &error_abort);
+    g_assert_cmphex(time, ==, 0xfffffffffffff800);
+    visit_check_struct(v, &error_abort);
+    visit_end_struct(v, NULL);
+    visit_free(v);
+
+    /* Beyond limits */
+    qdict = keyval_parse("time1=-1,"
+                         "time2=18446744073709550592", /* fffffffffffffc00 */
+                         NULL, &error_abort);
+    v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
+    qobject_unref(qdict);
+    visit_start_struct(v, NULL, NULL, 0, &error_abort);
+    visit_type_time(v, "time1", &time, &err);
+    error_free_or_abort(&err);
+    visit_type_time(v, "time2", &time, &err);
+    error_free_or_abort(&err);
+    visit_end_struct(v, NULL);
+    visit_free(v);
+
+    /* Suffixes */
+    qdict = keyval_parse("time1=2ns,time2=3.4us,time3=5ms,time4=600s",
+                         NULL, &error_abort);
+    v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
+    qobject_unref(qdict);
+    visit_start_struct(v, NULL, NULL, 0, &error_abort);
+    visit_type_time(v, "time1", &time, &error_abort);
+    g_assert_cmpuint(time, ==, 2);
+    visit_type_time(v, "time2", &time, &error_abort);
+    g_assert_cmpuint(time, ==, 3400);
+    visit_type_time(v, "time3", &time, &error_abort);
+    g_assert_cmphex(time, ==, 5 * 1000 * 1000);
+    visit_type_time(v, "time4", &time, &error_abort);
+    g_assert_cmphex(time, ==, 600 * 1000000000LL);
+    visit_check_struct(v, &error_abort);
+    visit_end_struct(v, NULL);
+    visit_free(v);
+
+    /* Beyond limit with suffix */
+    qdict = keyval_parse("time1=1844674407370955s", NULL, &error_abort);
+    v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
+    qobject_unref(qdict);
+    visit_start_struct(v, NULL, NULL, 0, &error_abort);
+    visit_type_time(v, "time1", &time, &err);
+    error_free_or_abort(&err);
+    visit_end_struct(v, NULL);
+    visit_free(v);
+
+    /* Trailing crap */
+    qdict = keyval_parse("time1=89ks,time2=ns", NULL, &error_abort);
+    v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
+    qobject_unref(qdict);
+    visit_start_struct(v, NULL, NULL, 0, &error_abort);
+    visit_type_time(v, "time1", &time, &err);
+    error_free_or_abort(&err);
+    visit_type_time(v, "time2", &time, &err);;
+    error_free_or_abort(&err);
+    visit_end_struct(v, NULL);
+    visit_free(v);
+}
+
 static void test_keyval_visit_dict(void)
 {
     Error *err = NULL;
@@ -678,6 +799,7 @@ int main(int argc, char *argv[])
     g_test_add_func("/keyval/visit/bool", test_keyval_visit_bool);
     g_test_add_func("/keyval/visit/number", test_keyval_visit_number);
     g_test_add_func("/keyval/visit/size", test_keyval_visit_size);
+    g_test_add_func("/keyval/visit/time", test_keyval_visit_time);
     g_test_add_func("/keyval/visit/dict", test_keyval_visit_dict);
     g_test_add_func("/keyval/visit/list", test_keyval_visit_list);
     g_test_add_func("/keyval/visit/optional", test_keyval_visit_optional);
diff --git a/tests/test-qobject-input-visitor.c b/tests/test-qobject-input-visitor.c
index 6bacabf063..55138042b8 100644
--- a/tests/test-qobject-input-visitor.c
+++ b/tests/test-qobject-input-visitor.c
@@ -366,6 +366,31 @@ static void test_visitor_in_size_str_fail(TestInputVisitorData *data,
     error_free_or_abort(&err);
 }
 
+static void test_visitor_in_time_str_keyval(TestInputVisitorData *data,
+                                            const void *unused)
+{
+    uint64_t res, value = 265 * 1000 * 1000;
+    Visitor *v;
+
+    v = visitor_input_test_init_full(data, true, "\"265ms\"");
+
+    visit_type_time(v, NULL, &res, &error_abort);
+    g_assert_cmpfloat(res, ==, value);
+}
+
+static void test_visitor_in_time_str_fail(TestInputVisitorData *data,
+                                          const void *unused)
+{
+    uint64_t res = 0;
+    Visitor *v;
+    Error *err = NULL;
+
+    v = visitor_input_test_init(data, "\"265ms\"");
+
+    visit_type_time(v, NULL, &res, &err);
+    error_free_or_abort(&err);
+}
+
 static void test_visitor_in_string(TestInputVisitorData *data,
                                    const void *unused)
 {
@@ -1311,6 +1336,10 @@ int main(int argc, char **argv)
                            NULL, test_visitor_in_size_str_keyval);
     input_visitor_test_add("/visitor/input/size_str_fail",
                            NULL, test_visitor_in_size_str_fail);
+    input_visitor_test_add("/visitor/input/time_str_keyval",
+                           NULL, test_visitor_in_time_str_keyval);
+    input_visitor_test_add("/visitor/input/time_str_fail",
+                           NULL, test_visitor_in_time_str_fail);
     input_visitor_test_add("/visitor/input/string",
                            NULL, test_visitor_in_string);
     input_visitor_test_add("/visitor/input/enum",
-- 
2.20.1



^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v14 04/11] numa: Extend CLI to provide initiator information for numa nodes
  2019-10-28  7:52 [PATCH v14 00/11] Build ACPI Heterogeneous Memory Attribute Table (HMAT) Tao Xu
                   ` (2 preceding siblings ...)
  2019-10-28  7:52 ` [PATCH v14 03/11] tests: Add test for QAPI " Tao Xu
@ 2019-10-28  7:52 ` Tao Xu
  2019-11-06 20:29   ` Eric Blake
  2019-10-28  7:52 ` [PATCH v14 05/11] numa: Extend CLI to provide memory latency and bandwidth information Tao Xu
                   ` (8 subsequent siblings)
  12 siblings, 1 reply; 32+ messages in thread
From: Tao Xu @ 2019-10-28  7:52 UTC (permalink / raw)
  To: mst, imammedo, eblake, ehabkost, marcel.apfelbaum, armbru,
	mdroth, thuth, lvivier
  Cc: jingqi.liu, tao3.xu, fan.du, qemu-devel, jonathan.cameron, Dan Williams

In ACPI 6.3 chapter 5.2.27 Heterogeneous Memory Attribute Table (HMAT),
The initiator represents processor which access to memory. And in 5.2.27.3
Memory Proximity Domain Attributes Structure, the attached initiator is
defined as where the memory controller responsible for a memory proximity
domain. With attached initiator information, the topology of heterogeneous
memory can be described.

Extend CLI of "-numa node" option to indicate the initiator numa node-id.
In the linux kernel, the codes in drivers/acpi/hmat/hmat.c parse and report
the platform's HMAT tables.

Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Jingqi Liu <jingqi.liu@intel.com>
Suggested-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Tao Xu <tao3.xu@intel.com>
---

Changes in v14:
    - Drop the error_printf("\n"); (Igor)

Changes in v13:
    - Modify some text description
    - Drop "initiator_valid" field in struct NodeInfo (Igor)
---
 hw/core/machine.c     | 64 +++++++++++++++++++++++++++++++++++++++++++
 hw/core/numa.c        | 23 ++++++++++++++++
 include/sysemu/numa.h |  5 ++++
 qapi/machine.json     | 10 ++++++-
 qemu-options.hx       | 35 +++++++++++++++++++----
 5 files changed, 131 insertions(+), 6 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 1689ad3bf8..e60d0b4552 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -518,6 +518,20 @@ static void machine_set_nvdimm(Object *obj, bool value, Error **errp)
     ms->nvdimms_state->is_enabled = value;
 }
 
+static bool machine_get_hmat(Object *obj, Error **errp)
+{
+    MachineState *ms = MACHINE(obj);
+
+    return ms->numa_state->hmat_enabled;
+}
+
+static void machine_set_hmat(Object *obj, bool value, Error **errp)
+{
+    MachineState *ms = MACHINE(obj);
+
+    ms->numa_state->hmat_enabled = value;
+}
+
 static char *machine_get_nvdimm_persistence(Object *obj, Error **errp)
 {
     MachineState *ms = MACHINE(obj);
@@ -645,6 +659,7 @@ void machine_set_cpu_numa_node(MachineState *machine,
                                const CpuInstanceProperties *props, Error **errp)
 {
     MachineClass *mc = MACHINE_GET_CLASS(machine);
+    NodeInfo *numa_info = machine->numa_state->nodes;
     bool match = false;
     int i;
 
@@ -714,6 +729,17 @@ void machine_set_cpu_numa_node(MachineState *machine,
         match = true;
         slot->props.node_id = props->node_id;
         slot->props.has_node_id = props->has_node_id;
+
+        if (machine->numa_state->hmat_enabled) {
+            if ((numa_info[props->node_id].initiator < MAX_NODES) &&
+                (props->node_id != numa_info[props->node_id].initiator)) {
+                error_setg(errp, "The initiator of CPU NUMA node %" PRId64
+                        " should be itself.", props->node_id);
+                return;
+            }
+            numa_info[props->node_id].has_cpu = true;
+            numa_info[props->node_id].initiator = props->node_id;
+        }
     }
 
     if (!match) {
@@ -960,6 +986,13 @@ static void machine_initfn(Object *obj)
 
     if (mc->numa_mem_supported) {
         ms->numa_state = g_new0(NumaState, 1);
+        object_property_add_bool(obj, "hmat",
+                                 machine_get_hmat, machine_set_hmat,
+                                 &error_abort);
+        object_property_set_description(obj, "hmat",
+                                        "Set on/off to enable/disable "
+                                        "ACPI Heterogeneous Memory Attribute "
+                                        "Table (HMAT)", NULL);
     }
 
     /* Register notifier when init is done for sysbus sanity checks */
@@ -1048,6 +1081,32 @@ static char *cpu_slot_to_string(const CPUArchId *cpu)
     return g_string_free(s, false);
 }
 
+static void numa_validate_initiator(NumaState *numa_state)
+{
+    int i;
+    NodeInfo *numa_info = numa_state->nodes;
+
+    for (i = 0; i < numa_state->num_nodes; i++) {
+        if (numa_info[i].initiator == MAX_NODES) {
+            error_report("The initiator of NUMA node %d is missing, use "
+                         "'-numa node,initiator' option to declare it.", i);
+            exit(1);
+        }
+
+        if (!numa_info[numa_info[i].initiator].present) {
+            error_report("NUMA node %" PRIu16 " is missing, use "
+                         "'-numa node' option to declare it first.",
+                         numa_info[i].initiator);
+            exit(1);
+        }
+
+        if (!numa_info[numa_info[i].initiator].has_cpu) {
+            error_report("The initiator of NUMA node %d is invalid.", i);
+            exit(1);
+        }
+    }
+}
+
 static void machine_numa_finish_cpu_init(MachineState *machine)
 {
     int i;
@@ -1088,6 +1147,11 @@ static void machine_numa_finish_cpu_init(MachineState *machine)
             machine_set_cpu_numa_node(machine, &props, &error_fatal);
         }
     }
+
+    if (machine->numa_state->hmat_enabled) {
+        numa_validate_initiator(machine->numa_state);
+    }
+
     if (s->len && !qtest_enabled()) {
         warn_report("CPU(s) not present in any NUMA nodes: %s",
                     s->str);
diff --git a/hw/core/numa.c b/hw/core/numa.c
index 038c96d4ab..eba66ab768 100644
--- a/hw/core/numa.c
+++ b/hw/core/numa.c
@@ -133,6 +133,29 @@ static void parse_numa_node(MachineState *ms, NumaNodeOptions *node,
         numa_info[nodenr].node_mem = object_property_get_uint(o, "size", NULL);
         numa_info[nodenr].node_memdev = MEMORY_BACKEND(o);
     }
+
+    /*
+     * If not set the initiator, set it to MAX_NODES. And if
+     * HMAT is enabled and this node has no cpus, QEMU will raise error.
+     */
+    numa_info[nodenr].initiator = MAX_NODES;
+    if (node->has_initiator) {
+        if (!ms->numa_state->hmat_enabled) {
+            error_setg(errp, "ACPI Heterogeneous Memory Attribute Table "
+                       "(HMAT) is disabled, enable it with -machine hmat=on "
+                       "before using any of hmat specific options.");
+            return;
+        }
+
+        if (node->initiator >= MAX_NODES) {
+            error_report("The initiator id %" PRIu16 " expects an integer "
+                         "between 0 and %d", node->initiator,
+                         MAX_NODES - 1);
+            return;
+        }
+
+        numa_info[nodenr].initiator = node->initiator;
+    }
     numa_info[nodenr].present = true;
     max_numa_nodeid = MAX(max_numa_nodeid, nodenr + 1);
     ms->numa_state->num_nodes++;
diff --git a/include/sysemu/numa.h b/include/sysemu/numa.h
index ae9c41d02b..788cbec7a2 100644
--- a/include/sysemu/numa.h
+++ b/include/sysemu/numa.h
@@ -18,6 +18,8 @@ struct NodeInfo {
     uint64_t node_mem;
     struct HostMemoryBackend *node_memdev;
     bool present;
+    bool has_cpu;
+    uint16_t initiator;
     uint8_t distance[MAX_NODES];
 };
 
@@ -33,6 +35,9 @@ struct NumaState {
     /* Allow setting NUMA distance for different NUMA nodes */
     bool have_numa_distance;
 
+    /* Detect if HMAT support is enabled. */
+    bool hmat_enabled;
+
     /* NUMA nodes information */
     NodeInfo nodes[MAX_NODES];
 };
diff --git a/qapi/machine.json b/qapi/machine.json
index ca26779f1a..f1b07b3486 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -463,6 +463,13 @@
 # @memdev: memory backend object.  If specified for one node,
 #          it must be specified for all nodes.
 #
+# @initiator: defined in ACPI 6.3 Chapter 5.2.27.3 Table 5-145,
+#             points to the nodeid which has the memory controller
+#             responsible for this NUMA node. This field provides
+#             additional information as to the initiator node that
+#             is closest (as in directly attached) to this node, and
+#             therefore has the best performance (since 4.2)
+#
 # Since: 2.1
 ##
 { 'struct': 'NumaNodeOptions',
@@ -470,7 +477,8 @@
    '*nodeid': 'uint16',
    '*cpus':   ['uint16'],
    '*mem':    'size',
-   '*memdev': 'str' }}
+   '*memdev': 'str',
+   '*initiator': 'uint16' }}
 
 ##
 # @NumaDistOptions:
diff --git a/qemu-options.hx b/qemu-options.hx
index b95bf9fbed..22d2bf28ad 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -43,7 +43,8 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \
     "                suppress-vmdesc=on|off disables self-describing migration (default=off)\n"
     "                nvdimm=on|off controls NVDIMM support (default=off)\n"
     "                enforce-config-section=on|off enforce configuration section migration (default=off)\n"
-    "                memory-encryption=@var{} memory encryption object to use (default=none)\n",
+    "                memory-encryption=@var{} memory encryption object to use (default=none)\n"
+    "                hmat=on|off controls ACPI HMAT support (default=off)\n",
     QEMU_ARCH_ALL)
 STEXI
 @item -machine [type=]@var{name}[,prop=@var{value}[,...]]
@@ -103,6 +104,9 @@ NOTE: this parameter is deprecated. Please use @option{-global}
 @option{migration.send-configuration}=@var{on|off} instead.
 @item memory-encryption=@var{}
 Memory encryption object to use. The default is none.
+@item hmat=on|off
+Enables or disables ACPI Heterogeneous Memory Attribute Table (HMAT) support.
+The default is off.
 @end table
 ETEXI
 
@@ -161,14 +165,14 @@ If any on the three values is given, the total number of CPUs @var{n} can be omi
 ETEXI
 
 DEF("numa", HAS_ARG, QEMU_OPTION_numa,
-    "-numa node[,mem=size][,cpus=firstcpu[-lastcpu]][,nodeid=node]\n"
-    "-numa node[,memdev=id][,cpus=firstcpu[-lastcpu]][,nodeid=node]\n"
+    "-numa node[,mem=size][,cpus=firstcpu[-lastcpu]][,nodeid=node][,initiator=node]\n"
+    "-numa node[,memdev=id][,cpus=firstcpu[-lastcpu]][,nodeid=node][,initiator=node]\n"
     "-numa dist,src=source,dst=destination,val=distance\n"
     "-numa cpu,node-id=node[,socket-id=x][,core-id=y][,thread-id=z]\n",
     QEMU_ARCH_ALL)
 STEXI
-@item -numa node[,mem=@var{size}][,cpus=@var{firstcpu}[-@var{lastcpu}]][,nodeid=@var{node}]
-@itemx -numa node[,memdev=@var{id}][,cpus=@var{firstcpu}[-@var{lastcpu}]][,nodeid=@var{node}]
+@item -numa node[,mem=@var{size}][,cpus=@var{firstcpu}[-@var{lastcpu}]][,nodeid=@var{node}][,initiator=@var{initiator}]
+@itemx -numa node[,memdev=@var{id}][,cpus=@var{firstcpu}[-@var{lastcpu}]][,nodeid=@var{node}][,initiator=@var{initiator}]
 @itemx -numa dist,src=@var{source},dst=@var{destination},val=@var{distance}
 @itemx -numa cpu,node-id=@var{node}[,socket-id=@var{x}][,core-id=@var{y}][,thread-id=@var{z}]
 @findex -numa
@@ -215,6 +219,27 @@ split equally between them.
 @samp{mem} and @samp{memdev} are mutually exclusive. Furthermore,
 if one node uses @samp{memdev}, all of them have to use it.
 
+@samp{initiator} is an additional option that points to an @var{initiator}
+NUMA node that has best performance (the lowest latency or largest bandwidth)
+to this NUMA @var{node}. Note that this option can be set only when
+the machine property 'hmat' is set to 'on'.
+
+Following example creates a machine with 2 NUMA nodes, node 0 has CPU.
+node 1 has only memory, and its initiator is node 0. Note that because
+node 0 has CPU, by default the initiator of node 0 is itself and must be
+itself.
+@example
+-machine hmat=on \
+-m 2G,slots=2,maxmem=4G \
+-object memory-backend-ram,size=1G,id=m0 \
+-object memory-backend-ram,size=1G,id=m1 \
+-numa node,nodeid=0,memdev=m0 \
+-numa node,nodeid=1,memdev=m1,initiator=0 \
+-smp 2,sockets=2,maxcpus=2  \
+-numa cpu,node-id=0,socket-id=0 \
+-numa cpu,node-id=0,socket-id=1
+@end example
+
 @var{source} and @var{destination} are NUMA node IDs.
 @var{distance} is the NUMA distance from @var{source} to @var{destination}.
 The distance from a node to itself is always 10. If any pair of nodes is
-- 
2.20.1



^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v14 05/11] numa: Extend CLI to provide memory latency and bandwidth information
  2019-10-28  7:52 [PATCH v14 00/11] Build ACPI Heterogeneous Memory Attribute Table (HMAT) Tao Xu
                   ` (3 preceding siblings ...)
  2019-10-28  7:52 ` [PATCH v14 04/11] numa: Extend CLI to provide initiator information for numa nodes Tao Xu
@ 2019-10-28  7:52 ` Tao Xu
  2019-10-28  7:52 ` [PATCH v14 06/11] numa: Calculate hmat latency and bandwidth entry list Tao Xu
                   ` (7 subsequent siblings)
  12 siblings, 0 replies; 32+ messages in thread
From: Tao Xu @ 2019-10-28  7:52 UTC (permalink / raw)
  To: mst, imammedo, eblake, ehabkost, marcel.apfelbaum, armbru,
	mdroth, thuth, lvivier
  Cc: jingqi.liu, tao3.xu, fan.du, qemu-devel, jonathan.cameron

From: Liu Jingqi <jingqi.liu@intel.com>

Add -numa hmat-lb option to provide System Locality Latency and
Bandwidth Information. These memory attributes help to build
System Locality Latency and Bandwidth Information Structure(s)
in ACPI Heterogeneous Memory Attribute Table (HMAT).

Signed-off-by: Liu Jingqi <jingqi.liu@intel.com>
Signed-off-by: Tao Xu <tao3.xu@intel.com>
---

Changes in v14:
    - Use qemu ctz64 and clz64 instead of builtin function
    - Improve help message in qemu-options.hx

Changes in v13:
    - Reuse Garray to store the raw bandwidth and bandwidth data
    - Calculate common base unit using range bitmap (Igor)
---
 hw/core/numa.c        | 136 ++++++++++++++++++++++++++++++++++++++++++
 include/sysemu/numa.h |  68 +++++++++++++++++++++
 qapi/machine.json     |  94 ++++++++++++++++++++++++++++-
 qemu-options.hx       |  49 ++++++++++++++-
 4 files changed, 344 insertions(+), 3 deletions(-)

diff --git a/hw/core/numa.c b/hw/core/numa.c
index eba66ab768..f391760c20 100644
--- a/hw/core/numa.c
+++ b/hw/core/numa.c
@@ -23,6 +23,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/units.h"
 #include "sysemu/hostmem.h"
 #include "sysemu/numa.h"
 #include "sysemu/sysemu.h"
@@ -198,6 +199,128 @@ void parse_numa_distance(MachineState *ms, NumaDistOptions *dist, Error **errp)
     ms->numa_state->have_numa_distance = true;
 }
 
+void parse_numa_hmat_lb(NumaState *numa_state, NumaHmatLBOptions *node,
+                        Error **errp)
+{
+    int first_bit, last_bit;
+    uint64_t max_latency, temp_base_la;
+    NodeInfo *numa_info = numa_state->nodes;
+    HMAT_LB_Info *hmat_lb =
+        numa_state->hmat_lb[node->hierarchy][node->data_type];
+    HMAT_LB_Data lb_data;
+
+    /* Error checking */
+    if (node->initiator >= numa_state->num_nodes) {
+        error_setg(errp, "Invalid initiator=%d, it should be less than %d.",
+                   node->initiator, numa_state->num_nodes);
+        return;
+    }
+    if (node->target >= numa_state->num_nodes) {
+        error_setg(errp, "Invalid target=%d, it should be less than %d.",
+                   node->target, numa_state->num_nodes);
+        return;
+    }
+    if (!numa_info[node->initiator].has_cpu) {
+        error_setg(errp, "Invalid initiator=%d, it isn't an "
+                   "initiator proximity domain.", node->initiator);
+        return;
+    }
+    if (!numa_info[node->target].present) {
+        error_setg(errp, "Invalid target=%d, it hasn't a valid NUMA node.",
+                   node->target);
+        return;
+    }
+
+    if (!hmat_lb) {
+        hmat_lb = g_malloc0(sizeof(*hmat_lb));
+        numa_state->hmat_lb[node->hierarchy][node->data_type] = hmat_lb;
+        hmat_lb->latency = g_array_new(false, true, sizeof(HMAT_LB_Data));
+        hmat_lb->bandwidth = g_array_new(false, true, sizeof(HMAT_LB_Data));
+    }
+    hmat_lb->hierarchy = node->hierarchy;
+    hmat_lb->data_type = node->data_type;
+    lb_data.initiator = node->initiator;
+    lb_data.target = node->target;
+
+    /* Input latency data */
+    if (node->data_type <= HMATLB_DATA_TYPE_WRITE_LATENCY) {
+        if (!node->has_latency) {
+            error_setg(errp, "Missing 'latency' option.");
+            return;
+        }
+        if (node->has_bandwidth) {
+            error_setg(errp, "Invalid option 'bandwidth' since "
+                       "the data type is latency.");
+            return;
+        }
+
+        if (hmat_lb->base_latency == 0) {
+            hmat_lb->base_latency = UINT64_MAX;
+        }
+
+        /* Calculate the temporary base and compressed latency */
+        max_latency = node->latency;
+        temp_base_la = 1;
+        while (QEMU_IS_ALIGNED(max_latency, 10)) {
+            max_latency /= 10;
+            temp_base_la *= 10;
+        }
+
+        /* Calculate the max compressed latency */
+        hmat_lb->base_latency = MIN(hmat_lb->base_latency, temp_base_la);
+        max_latency = node->latency / hmat_lb->base_latency;
+        hmat_lb->max_entry_la = MAX(hmat_lb->max_entry_la, max_latency);
+
+        if (hmat_lb->max_entry_la >= UINT16_MAX) {
+            error_setg(errp, "Latency %" PRIu64 " between initiator=%d and "
+                       "target=%d should not differ from previously entered "
+                       "values on more than %d.", node->latency,
+                       node->initiator, node->target, UINT16_MAX - 1);
+            return;
+        }
+
+        lb_data.rawdata = node->latency;
+        g_array_append_val(hmat_lb->latency, lb_data);
+    }
+
+    /* Input bandwidth data */
+    if (node->data_type >= HMATLB_DATA_TYPE_ACCESS_BANDWIDTH) {
+        if (!node->has_bandwidth) {
+            error_setg(errp, "Missing 'bandwidth' option.");
+            return;
+        }
+        if (node->has_latency) {
+            error_setg(errp, "Invalid option 'latency' since "
+                       "the data type is bandwidth.");
+            return;
+        }
+        if (!QEMU_IS_ALIGNED(node->bandwidth, MiB)) {
+            error_setg(errp, "Bandwidth %" PRIu64 " between initiator=%d and "
+                       "target=%d should be 1MB aligned.", node->bandwidth,
+                       node->initiator, node->target);
+            return;
+        }
+
+        hmat_lb->range_bitmap_bw |= node->bandwidth;
+
+        first_bit = ctz64(hmat_lb->range_bitmap_bw);
+        hmat_lb->base_bandwidth = UINT64_C(1) << first_bit;
+        last_bit = 64 - clz64(hmat_lb->range_bitmap_bw);
+        if ((last_bit - first_bit) > UINT16_BITS ||
+            (MAKE_64BIT_MASK(first_bit, UINT16_BITS) == node->bandwidth)) {
+            error_setg(errp, "Bandwidth %" PRIu64 " between initiator=%d and "
+                       "target=%d should not differ from previously entered "
+                       "values on more than %d.", node->bandwidth,
+                       node->initiator, node->target, UINT16_MAX - 1);
+            return;
+        }
+
+        hmat_lb->base_bandwidth = UINT64_C(1) << first_bit;
+        lb_data.rawdata = node->bandwidth;
+        g_array_append_val(hmat_lb->bandwidth, lb_data);
+    }
+}
+
 void set_numa_options(MachineState *ms, NumaOptions *object, Error **errp)
 {
     Error *err = NULL;
@@ -236,6 +359,19 @@ void set_numa_options(MachineState *ms, NumaOptions *object, Error **errp)
         machine_set_cpu_numa_node(ms, qapi_NumaCpuOptions_base(&object->u.cpu),
                                   &err);
         break;
+    case NUMA_OPTIONS_TYPE_HMAT_LB:
+        if (!ms->numa_state->hmat_enabled) {
+            error_setg(errp, "ACPI Heterogeneous Memory Attribute Table "
+                       "(HMAT) is disabled, enable it with -machine hmat=on "
+                       "before using any of hmat specific options.");
+            return;
+        }
+
+        parse_numa_hmat_lb(ms->numa_state, &object->u.hmat_lb, &err);
+        if (err) {
+            goto end;
+        }
+        break;
     default:
         abort();
     }
diff --git a/include/sysemu/numa.h b/include/sysemu/numa.h
index 788cbec7a2..36e1b4dece 100644
--- a/include/sysemu/numa.h
+++ b/include/sysemu/numa.h
@@ -14,6 +14,29 @@ struct CPUArchId;
 #define NUMA_DISTANCE_MAX         254
 #define NUMA_DISTANCE_UNREACHABLE 255
 
+/* the value of AcpiHmatLBInfo flags */
+enum {
+    HMAT_LB_MEM_MEMORY           = 0,
+    HMAT_LB_MEM_CACHE_1ST_LEVEL  = 1,
+    HMAT_LB_MEM_CACHE_2ND_LEVEL  = 2,
+    HMAT_LB_MEM_CACHE_3RD_LEVEL  = 3,
+};
+
+/* the value of AcpiHmatLBInfo data type */
+enum {
+    HMAT_LB_DATA_ACCESS_LATENCY   = 0,
+    HMAT_LB_DATA_READ_LATENCY     = 1,
+    HMAT_LB_DATA_WRITE_LATENCY    = 2,
+    HMAT_LB_DATA_ACCESS_BANDWIDTH = 3,
+    HMAT_LB_DATA_READ_BANDWIDTH   = 4,
+    HMAT_LB_DATA_WRITE_BANDWIDTH  = 5,
+};
+
+#define UINT16_BITS       16
+
+#define HMAT_LB_LEVELS    (HMAT_LB_MEM_CACHE_3RD_LEVEL + 1)
+#define HMAT_LB_TYPES     (HMAT_LB_DATA_WRITE_BANDWIDTH + 1)
+
 struct NodeInfo {
     uint64_t node_mem;
     struct HostMemoryBackend *node_memdev;
@@ -28,6 +51,46 @@ struct NumaNodeMem {
     uint64_t node_plugged_mem;
 };
 
+struct HMAT_LB_Data {
+    uint8_t     initiator;
+    uint8_t     target;
+    uint64_t    rawdata;
+};
+typedef struct HMAT_LB_Data HMAT_LB_Data;
+
+struct HMAT_LB_Info {
+    /* Indicates it's memory or the specified level memory side cache. */
+    uint8_t     hierarchy;
+
+    /* Present the type of data, access/read/write latency or bandwidth. */
+    uint8_t     data_type;
+
+    /* The max compressed latency for calculating common latency base */
+    uint64_t    max_entry_la;
+
+    /* The range bitmap of bandwidth for calculating common bandwidth base */
+    uint64_t    range_bitmap_bw;
+
+    /* The common base unit for latencies */
+    uint64_t    base_latency;
+
+    /* The common base unit for bandwidths */
+    uint64_t    base_bandwidth;
+
+    /* Array to store the compressed latencies */
+    uint16_t    *entry_latency;
+
+    /* Array to store the compressed latencies */
+    uint16_t    *entry_bandwidth;
+
+    /* Array to store the latencies */
+    GArray      *latency;
+
+    /* Array to store the bandwidthes */
+    GArray      *bandwidth;
+};
+typedef struct HMAT_LB_Info HMAT_LB_Info;
+
 struct NumaState {
     /* Number of NUMA nodes */
     int num_nodes;
@@ -40,11 +103,16 @@ struct NumaState {
 
     /* NUMA nodes information */
     NodeInfo nodes[MAX_NODES];
+
+    /* NUMA nodes HMAT Locality Latency and Bandwidth Information */
+    HMAT_LB_Info *hmat_lb[HMAT_LB_LEVELS][HMAT_LB_TYPES];
 };
 typedef struct NumaState NumaState;
 
 void set_numa_options(MachineState *ms, NumaOptions *object, Error **errp);
 void parse_numa_opts(MachineState *ms);
+void parse_numa_hmat_lb(NumaState *numa_state, NumaHmatLBOptions *node,
+                        Error **errp);
 void numa_complete_configuration(MachineState *ms);
 void query_numa_node_mem(NumaNodeMem node_mem[], MachineState *ms);
 extern QemuOptsList qemu_numa_opts;
diff --git a/qapi/machine.json b/qapi/machine.json
index f1b07b3486..0b3a8d09f0 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -426,10 +426,12 @@
 #
 # @cpu: property based CPU(s) to node mapping (Since: 2.10)
 #
+# @hmat-lb: memory latency and bandwidth information (Since: 4.2)
+#
 # Since: 2.1
 ##
 { 'enum': 'NumaOptionsType',
-  'data': [ 'node', 'dist', 'cpu' ] }
+  'data': [ 'node', 'dist', 'cpu', 'hmat-lb' ] }
 
 ##
 # @NumaOptions:
@@ -444,7 +446,8 @@
   'data': {
     'node': 'NumaNodeOptions',
     'dist': 'NumaDistOptions',
-    'cpu': 'NumaCpuOptions' }}
+    'cpu': 'NumaCpuOptions',
+    'hmat-lb': 'NumaHmatLBOptions' }}
 
 ##
 # @NumaNodeOptions:
@@ -557,6 +560,93 @@
    'base': 'CpuInstanceProperties',
    'data' : {} }
 
+##
+# @HmatLBMemoryHierarchy:
+#
+# The memory hierarchy in the System Locality Latency
+# and Bandwidth Information Structure of HMAT (Heterogeneous
+# Memory Attribute Table)
+#
+# For more information of @HmatLBMemoryHierarchy see
+# the chapter 5.2.27.4: Table 5-142: Field "Flags" of ACPI 6.3 spec.
+#
+# @memory: the structure represents the memory performance
+#
+# @first-level: first level memory of memory side cached memory
+#
+# @second-level: second level memory of memory side cached memory
+#
+# @third-level: third level memory of memory side cached memory
+#
+# Since: 4.2
+##
+{ 'enum': 'HmatLBMemoryHierarchy',
+  'data': [ 'memory', 'first-level', 'second-level', 'third-level' ] }
+
+##
+# @HmatLBDataType:
+#
+# Data type in the System Locality Latency
+# and Bandwidth Information Structure of HMAT (Heterogeneous
+# Memory Attribute Table)
+#
+# For more information of @HmatLBDataType see
+# the chapter 5.2.27.4: Table 5-142:  Field "Data Type" of ACPI 6.3 spec.
+#
+# @access-latency: access latency (nanoseconds)
+#
+# @read-latency: read latency (nanoseconds)
+#
+# @write-latency: write latency (nanoseconds)
+#
+# @access-bandwidth: access bandwidth (B/s)
+#
+# @read-bandwidth: read bandwidth (B/s)
+#
+# @write-bandwidth: write bandwidth (B/s)
+#
+# Since: 4.2
+##
+{ 'enum': 'HmatLBDataType',
+  'data': [ 'access-latency', 'read-latency', 'write-latency',
+            'access-bandwidth', 'read-bandwidth', 'write-bandwidth' ] }
+
+##
+# @NumaHmatLBOptions:
+#
+# Set the system locality latency and bandwidth information
+# between Initiator and Target proximity Domains.
+#
+# For more information of @NumaHmatLBOptions see
+# the chapter 5.2.27.4: Table 5-142 of ACPI 6.3 spec.
+#
+# @initiator: the Initiator Proximity Domain.
+#
+# @target: the Target Proximity Domain.
+#
+# @hierarchy: the Memory Hierarchy. Indicates the performance
+#             of memory or side cache.
+#
+# @data-type: presents the type of data, access/read/write
+#             latency or hit latency.
+#
+# @latency: the value of latency from @initiator to @target proximity domain,
+#           the latency unit is "ns(nanosecond)".
+#
+# @bandwidth: the value of bandwidth between @initiator and @target proximity
+#             domain, the bandwidth unit is "B(/s)".
+#
+# Since: 4.2
+##
+{ 'struct': 'NumaHmatLBOptions',
+    'data': {
+    'initiator': 'uint16',
+    'target': 'uint16',
+    'hierarchy': 'HmatLBMemoryHierarchy',
+    'data-type': 'HmatLBDataType',
+    '*latency': 'time',
+    '*bandwidth': 'size' }}
+
 ##
 # @HostMemPolicy:
 #
diff --git a/qemu-options.hx b/qemu-options.hx
index 22d2bf28ad..d24197d59b 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -168,16 +168,19 @@ DEF("numa", HAS_ARG, QEMU_OPTION_numa,
     "-numa node[,mem=size][,cpus=firstcpu[-lastcpu]][,nodeid=node][,initiator=node]\n"
     "-numa node[,memdev=id][,cpus=firstcpu[-lastcpu]][,nodeid=node][,initiator=node]\n"
     "-numa dist,src=source,dst=destination,val=distance\n"
-    "-numa cpu,node-id=node[,socket-id=x][,core-id=y][,thread-id=z]\n",
+    "-numa cpu,node-id=node[,socket-id=x][,core-id=y][,thread-id=z]\n"
+    "-numa hmat-lb,initiator=node,target=node,hierarchy=memory|first-level|second-level|third-level,data-type=access-latency|read-latency|write-latency[,latency=lat][,bandwidth=bw]\n",
     QEMU_ARCH_ALL)
 STEXI
 @item -numa node[,mem=@var{size}][,cpus=@var{firstcpu}[-@var{lastcpu}]][,nodeid=@var{node}][,initiator=@var{initiator}]
 @itemx -numa node[,memdev=@var{id}][,cpus=@var{firstcpu}[-@var{lastcpu}]][,nodeid=@var{node}][,initiator=@var{initiator}]
 @itemx -numa dist,src=@var{source},dst=@var{destination},val=@var{distance}
 @itemx -numa cpu,node-id=@var{node}[,socket-id=@var{x}][,core-id=@var{y}][,thread-id=@var{z}]
+@itemx -numa hmat-lb,initiator=@var{node},target=@var{node},hierarchy=@var{hierarchy},data-type=@var{tpye}[,latency=@var{lat}][,bandwidth=@var{bw}]
 @findex -numa
 Define a NUMA node and assign RAM and VCPUs to it.
 Set the NUMA distance from a source node to a destination node.
+Set the ACPI Heterogeneous Memory Attributes for the given nodes.
 
 Legacy VCPU assignment uses @samp{cpus} option where
 @var{firstcpu} and @var{lastcpu} are CPU indexes. Each
@@ -256,6 +259,50 @@ specified resources, it just assigns existing resources to NUMA
 nodes. This means that one still has to use the @option{-m},
 @option{-smp} options to allocate RAM and VCPUs respectively.
 
+Use @samp{hmat-lb} to set System Locality Latency and Bandwidth Information
+between initiator and target NUMA nodes in ACPI Heterogeneous Attribute Memory Table (HMAT).
+Initiator NUMA node can create memory requests, usually it has one or more processors.
+Target NUMA node contains addressable memory.
+
+In @samp{hmat-lb} option, @var{node} are NUMA node IDs. @var{hierarchy} is the memory
+hierarchy of the target NUMA node: if @var{hierarchy} is 'memory', the structure
+represents the memory performance; if @var{hierarchy} is 'first-level|second-level|third-level',
+this structure represents aggregated performance of memory side caches for each domain.
+@var{type} of 'data-type' is type of data represented by this structure instance:
+if 'hierarchy' is 'memory', 'data-type' is 'access|read|write' latency or 'access|read|write'
+bandwidth of the target memory; if 'hierarchy' is 'first-level|second-level|third-level',
+'data-type' is 'access|read|write' hit latency or 'access|read|write' hit bandwidth of the
+target memory side cache.
+
+@var{lat} of 'latency' is latency value, the possible value and units are
+NUM[ns|us|ms] (nanosecond|microsecond|millisecond), the recommended unit is 'ns'.
+@var{bw} is bandwidth value, the possible value and units are NUM[M|G|T], mean that
+the bandwidth value are NUM MB/s, GB/s or TB/s. Note that for @var{lat} and @var{bw}
+max NUM is 65534, if NUM is 0, means the corresponding latency or bandwidth information
+is not provided. And if input numbers without any unit, the latency unit will be 'ns'
+and the bandwidth will be MB/s.
+
+For example, the following option assigns NUMA node 0 and 1. Node 0 has 2 cpus and
+a ram, node 1 has only a ram. The processors in node 0 access memory in node
+0 with access-latency 5 nanoseconds, access-bandwidth is 200 MB/s;
+The processors in NUMA node 0 access memory in NUMA node 1 with access-latency 10
+nanoseconds, access-bandwidth is 100 MB/s.
+@example
+-machine hmat=on \
+-m 2G \
+-object memory-backend-ram,size=1G,id=m0 \
+-object memory-backend-ram,size=1G,id=m1 \
+-smp 2 \
+-numa node,nodeid=0,memdev=m0 \
+-numa node,nodeid=1,memdev=m1,initiator=0 \
+-numa cpu,node-id=0,socket-id=0 \
+-numa cpu,node-id=0,socket-id=1 \
+-numa hmat-lb,initiator=0,target=0,hierarchy=memory,data-type=access-latency,latency=5ns \
+-numa hmat-lb,initiator=0,target=0,hierarchy=memory,data-type=access-bandwidth,bandwidth=200M \
+-numa hmat-lb,initiator=0,target=1,hierarchy=memory,data-type=access-latency,latency=10ns \
+-numa hmat-lb,initiator=0,target=1,hierarchy=memory,data-type=access-bandwidth,bandwidth=100M
+@end example
+
 ETEXI
 
 DEF("add-fd", HAS_ARG, QEMU_OPTION_add_fd,
-- 
2.20.1



^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v14 06/11] numa: Calculate hmat latency and bandwidth entry list
  2019-10-28  7:52 [PATCH v14 00/11] Build ACPI Heterogeneous Memory Attribute Table (HMAT) Tao Xu
                   ` (4 preceding siblings ...)
  2019-10-28  7:52 ` [PATCH v14 05/11] numa: Extend CLI to provide memory latency and bandwidth information Tao Xu
@ 2019-10-28  7:52 ` Tao Xu
  2019-10-28  7:52 ` [PATCH v14 07/11] numa: Extend CLI to provide memory side cache information Tao Xu
                   ` (6 subsequent siblings)
  12 siblings, 0 replies; 32+ messages in thread
From: Tao Xu @ 2019-10-28  7:52 UTC (permalink / raw)
  To: mst, imammedo, eblake, ehabkost, marcel.apfelbaum, armbru,
	mdroth, thuth, lvivier
  Cc: jingqi.liu, tao3.xu, fan.du, qemu-devel, jonathan.cameron

Compress HMAT latency and bandwidth raw data into uint16_t data,
which can be stored in HMAT table.

Suggested-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Tao Xu <tao3.xu@intel.com>
---

Changes in v14:
    - Convert latency from ns to ps, because ACPI 6.3 HMAT table use
      ps as minimum unit
---
 hw/core/numa.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 58 insertions(+), 1 deletion(-)

diff --git a/hw/core/numa.c b/hw/core/numa.c
index f391760c20..523dd80822 100644
--- a/hw/core/numa.c
+++ b/hw/core/numa.c
@@ -483,6 +483,47 @@ static void complete_init_numa_distance(MachineState *ms)
     }
 }
 
+static void calculate_hmat_entry_list(HMAT_LB_Info *hmat_lb, int num_nodes)
+{
+    int i, index;
+    uint16_t *entry_list;
+    uint64_t base;
+    GArray *lb_data_list;
+    HMAT_LB_Data *lb_data;
+
+    if (hmat_lb->data_type <= HMAT_LB_DATA_WRITE_LATENCY) {
+        base = hmat_lb->base_latency;
+        lb_data_list = hmat_lb->latency;
+    } else {
+        base = hmat_lb->base_bandwidth;
+        lb_data_list = hmat_lb->bandwidth;
+    }
+
+    entry_list = g_malloc0(lb_data_list->len * sizeof(uint16_t));
+    for (i = 0; i < lb_data_list->len; i++) {
+        lb_data = &g_array_index(lb_data_list, HMAT_LB_Data, i);
+        index = lb_data->initiator * num_nodes + lb_data->target;
+        if (entry_list[index]) {
+            error_report("Duplicate configuration of the latency for "
+                "initiator=%d and target=%d.", lb_data->initiator,
+                lb_data->target);
+            exit(1);
+        }
+
+        entry_list[index] = (uint16_t)(lb_data->rawdata / base);
+    }
+
+    if (hmat_lb->data_type <= HMAT_LB_DATA_WRITE_LATENCY) {
+        /* Convert latency base from nanoseconds to picosecond */
+        hmat_lb->base_latency = base * 1000;
+        hmat_lb->entry_latency = entry_list;
+    } else {
+        /* Convert bandwidth base from Byte to Megabyte */
+        hmat_lb->base_bandwidth = base / MiB;
+        hmat_lb->entry_bandwidth = entry_list;
+    }
+}
+
 void numa_legacy_auto_assign_ram(MachineClass *mc, NodeInfo *nodes,
                                  int nb_nodes, ram_addr_t size)
 {
@@ -521,9 +562,10 @@ void numa_default_auto_assign_ram(MachineClass *mc, NodeInfo *nodes,
 
 void numa_complete_configuration(MachineState *ms)
 {
-    int i;
+    int i, hierarchy, type;
     MachineClass *mc = MACHINE_GET_CLASS(ms);
     NodeInfo *numa_info = ms->numa_state->nodes;
+    HMAT_LB_Info *numa_hmat_lb;
 
     /*
      * If memory hotplug is enabled (slots > 0) but without '-numa'
@@ -620,6 +662,21 @@ void numa_complete_configuration(MachineState *ms)
             /* Validation succeeded, now fill in any missing distances. */
             complete_init_numa_distance(ms);
         }
+
+        if (ms->numa_state->hmat_enabled) {
+            for (hierarchy = HMAT_LB_MEM_MEMORY;
+                 hierarchy <= HMAT_LB_MEM_CACHE_3RD_LEVEL; hierarchy++) {
+                for (type = HMAT_LB_DATA_ACCESS_LATENCY;
+                    type <= HMAT_LB_DATA_WRITE_BANDWIDTH; type++) {
+                    numa_hmat_lb = ms->numa_state->hmat_lb[hierarchy][type];
+
+                    if (numa_hmat_lb) {
+                        calculate_hmat_entry_list(numa_hmat_lb,
+                                                  ms->numa_state->num_nodes);
+                    }
+                }
+            }
+        }
     }
 }
 
-- 
2.20.1



^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v14 07/11] numa: Extend CLI to provide memory side cache information
  2019-10-28  7:52 [PATCH v14 00/11] Build ACPI Heterogeneous Memory Attribute Table (HMAT) Tao Xu
                   ` (5 preceding siblings ...)
  2019-10-28  7:52 ` [PATCH v14 06/11] numa: Calculate hmat latency and bandwidth entry list Tao Xu
@ 2019-10-28  7:52 ` Tao Xu
  2019-10-28  7:52 ` [PATCH v14 08/11] hmat acpi: Build Memory Proximity Domain Attributes Structure(s) Tao Xu
                   ` (5 subsequent siblings)
  12 siblings, 0 replies; 32+ messages in thread
From: Tao Xu @ 2019-10-28  7:52 UTC (permalink / raw)
  To: mst, imammedo, eblake, ehabkost, marcel.apfelbaum, armbru,
	mdroth, thuth, lvivier
  Cc: jingqi.liu, tao3.xu, fan.du, qemu-devel, Daniel Black, jonathan.cameron

From: Liu Jingqi <jingqi.liu@intel.com>

Add -numa hmat-cache option to provide Memory Side Cache Information.
These memory attributes help to build Memory Side Cache Information
Structure(s) in ACPI Heterogeneous Memory Attribute Table (HMAT).

Reviewed-by: Daniel Black <daniel@linux.ibm.com>
Signed-off-by: Liu Jingqi <jingqi.liu@intel.com>
Signed-off-by: Tao Xu <tao3.xu@intel.com>
---

No changes in v14.

Changes in v13:
    - Drop the total_levels option.
    - Use readable cache size (Igor)
---
 hw/core/numa.c        | 66 ++++++++++++++++++++++++++++++++++++
 include/sysemu/numa.h | 31 +++++++++++++++++
 qapi/machine.json     | 78 +++++++++++++++++++++++++++++++++++++++++--
 qemu-options.hx       | 16 +++++++--
 4 files changed, 187 insertions(+), 4 deletions(-)

diff --git a/hw/core/numa.c b/hw/core/numa.c
index 523dd80822..165b38d74b 100644
--- a/hw/core/numa.c
+++ b/hw/core/numa.c
@@ -321,6 +321,59 @@ void parse_numa_hmat_lb(NumaState *numa_state, NumaHmatLBOptions *node,
     }
 }
 
+void parse_numa_hmat_cache(MachineState *ms, NumaHmatCacheOptions *node,
+                           Error **errp)
+{
+    int nb_numa_nodes = ms->numa_state->num_nodes;
+    HMAT_Cache_Info *hmat_cache = NULL;
+
+    if (node->node_id >= nb_numa_nodes) {
+        error_setg(errp, "Invalid node-id=%" PRIu32
+                   ", it should be less than %d.",
+                   node->node_id, nb_numa_nodes);
+        return;
+    }
+
+    if (node->level > MAX_HMAT_CACHE_LEVEL) {
+        error_setg(errp, "Invalid level=%" PRIu8
+                   ", it should be less than or equal to %d.",
+                   node->level, MAX_HMAT_CACHE_LEVEL);
+        return;
+    }
+    if (ms->numa_state->hmat_cache[node->node_id][node->level]) {
+        error_setg(errp, "Duplicate configuration of the side cache for "
+                   "node-id=%" PRIu32 " and level=%" PRIu8 ".",
+                   node->node_id, node->level);
+        return;
+    }
+
+    if ((node->level > 1) &&
+        ms->numa_state->hmat_cache[node->node_id][node->level - 1] &&
+        (node->size >=
+            ms->numa_state->hmat_cache[node->node_id][node->level - 1]->size)) {
+        error_setg(errp, "Invalid size=0x%" PRIx64
+                   ", the size of level=%" PRIu8
+                   " should be less than the size(0x%" PRIx64
+                   ") of level=%" PRIu8 ".",
+                   node->size, node->level,
+                   ms->numa_state->hmat_cache[node->node_id]
+                                             [node->level - 1]->size,
+                   node->level - 1);
+        return;
+    }
+
+    hmat_cache = g_malloc0(sizeof(*hmat_cache));
+
+    hmat_cache->proximity = node->node_id;
+    hmat_cache->size = node->size;
+    hmat_cache->level = node->level;
+    hmat_cache->associativity = node->assoc;
+    hmat_cache->write_policy = node->policy;
+    hmat_cache->line_size = node->line;
+
+    ms->numa_state->hmat_cache[node->node_id][node->level] = hmat_cache;
+}
+
 void set_numa_options(MachineState *ms, NumaOptions *object, Error **errp)
 {
     Error *err = NULL;
@@ -372,6 +425,19 @@ void set_numa_options(MachineState *ms, NumaOptions *object, Error **errp)
             goto end;
         }
         break;
+    case NUMA_OPTIONS_TYPE_HMAT_CACHE:
+        if (!ms->numa_state->hmat_enabled) {
+            error_setg(errp, "ACPI Heterogeneous Memory Attribute Table "
+                       "(HMAT) is disabled, enable it with -machine hmat=on "
+                       "before using any of hmat specific options.");
+            return;
+        }
+
+        parse_numa_hmat_cache(ms, &object->u.hmat_cache, &err);
+        if (err) {
+            goto end;
+        }
+        break;
     default:
         abort();
     }
diff --git a/include/sysemu/numa.h b/include/sysemu/numa.h
index 36e1b4dece..50554709e7 100644
--- a/include/sysemu/numa.h
+++ b/include/sysemu/numa.h
@@ -37,6 +37,8 @@ enum {
 #define HMAT_LB_LEVELS    (HMAT_LB_MEM_CACHE_3RD_LEVEL + 1)
 #define HMAT_LB_TYPES     (HMAT_LB_DATA_WRITE_BANDWIDTH + 1)
 
+#define MAX_HMAT_CACHE_LEVEL    HMAT_LB_MEM_CACHE_3RD_LEVEL
+
 struct NodeInfo {
     uint64_t node_mem;
     struct HostMemoryBackend *node_memdev;
@@ -91,6 +93,30 @@ struct HMAT_LB_Info {
 };
 typedef struct HMAT_LB_Info HMAT_LB_Info;
 
+struct HMAT_Cache_Info {
+    /* The memory proximity domain to which the memory belongs. */
+    uint32_t    proximity;
+
+    /* Size of memory side cache in bytes. */
+    uint64_t    size;
+
+    /* Total cache levels for this memory proximity domain. */
+    uint8_t     total_levels;
+
+    /* Cache level described in this structure. */
+    uint8_t     level;
+
+    /* Cache Associativity: None/Direct Mapped/Comple Cache Indexing */
+    uint8_t     associativity;
+
+    /* Write Policy: None/Write Back(WB)/Write Through(WT) */
+    uint8_t     write_policy;
+
+    /* Cache Line size in bytes. */
+    uint16_t    line_size;
+};
+typedef struct HMAT_Cache_Info HMAT_Cache_Info;
+
 struct NumaState {
     /* Number of NUMA nodes */
     int num_nodes;
@@ -106,6 +132,9 @@ struct NumaState {
 
     /* NUMA nodes HMAT Locality Latency and Bandwidth Information */
     HMAT_LB_Info *hmat_lb[HMAT_LB_LEVELS][HMAT_LB_TYPES];
+
+    /* Memory Side Cache Information Structure */
+    HMAT_Cache_Info *hmat_cache[MAX_NODES][MAX_HMAT_CACHE_LEVEL + 1];
 };
 typedef struct NumaState NumaState;
 
@@ -113,6 +142,8 @@ void set_numa_options(MachineState *ms, NumaOptions *object, Error **errp);
 void parse_numa_opts(MachineState *ms);
 void parse_numa_hmat_lb(NumaState *numa_state, NumaHmatLBOptions *node,
                         Error **errp);
+void parse_numa_hmat_cache(MachineState *ms, NumaHmatCacheOptions *node,
+                           Error **errp);
 void numa_complete_configuration(MachineState *ms);
 void query_numa_node_mem(NumaNodeMem node_mem[], MachineState *ms);
 extern QemuOptsList qemu_numa_opts;
diff --git a/qapi/machine.json b/qapi/machine.json
index 0b3a8d09f0..9309fd333a 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -428,10 +428,12 @@
 #
 # @hmat-lb: memory latency and bandwidth information (Since: 4.2)
 #
+# @hmat-cache: memory side cache information (Since: 4.2)
+#
 # Since: 2.1
 ##
 { 'enum': 'NumaOptionsType',
-  'data': [ 'node', 'dist', 'cpu', 'hmat-lb' ] }
+  'data': [ 'node', 'dist', 'cpu', 'hmat-lb', 'hmat-cache' ] }
 
 ##
 # @NumaOptions:
@@ -447,7 +449,8 @@
     'node': 'NumaNodeOptions',
     'dist': 'NumaDistOptions',
     'cpu': 'NumaCpuOptions',
-    'hmat-lb': 'NumaHmatLBOptions' }}
+    'hmat-lb': 'NumaHmatLBOptions',
+    'hmat-cache': 'NumaHmatCacheOptions' }}
 
 ##
 # @NumaNodeOptions:
@@ -647,6 +650,77 @@
     '*latency': 'time',
     '*bandwidth': 'size' }}
 
+##
+# @HmatCacheAssociativity:
+#
+# Cache associativity in the Memory Side Cache
+# Information Structure of HMAT
+#
+# For more information of @HmatCacheAssociativity see
+# the chapter 5.2.27.5: Table 5-143 of ACPI 6.3 spec.
+#
+# @none: None
+#
+# @direct: Direct Mapped
+#
+# @complex: Complex Cache Indexing (implementation specific)
+#
+# Since: 4.2
+##
+{ 'enum': 'HmatCacheAssociativity',
+  'data': [ 'none', 'direct', 'complex' ] }
+
+##
+# @HmatCacheWritePolicy:
+#
+# Cache write policy in the Memory Side Cache
+# Information Structure of HMAT
+#
+# For more information of @HmatCacheWritePolicy see
+# the chapter 5.2.27.5: Table 5-143: Field "Cache Attributes" of ACPI 6.3 spec.
+#
+# @none: None
+#
+# @write-back: Write Back (WB)
+#
+# @write-through: Write Through (WT)
+#
+# Since: 4.2
+##
+{ 'enum': 'HmatCacheWritePolicy',
+  'data': [ 'none', 'write-back', 'write-through' ] }
+
+##
+# @NumaHmatCacheOptions:
+#
+# Set the memory side cache information for a given memory domain.
+#
+# For more information of @NumaHmatCacheOptions see
+# the chapter 5.2.27.5: Table 5-143: Field "Cache Attributes" of ACPI 6.3 spec.
+#
+# @node-id: the memory proximity domain to which the memory belongs.
+#
+# @size: the size of memory side cache in bytes.
+#
+# @level: the cache level described in this structure.
+#
+# @assoc: the cache associativity, none/direct-mapped/complex(complex cache indexing).
+#
+# @policy: the write policy, none/write-back/write-through.
+#
+# @line: the cache Line size in bytes.
+#
+# Since: 4.2
+##
+{ 'struct': 'NumaHmatCacheOptions',
+  'data': {
+   'node-id': 'uint32',
+   'size': 'size',
+   'level': 'uint8',
+   'assoc': 'HmatCacheAssociativity',
+   'policy': 'HmatCacheWritePolicy',
+   'line': 'uint16' }}
+
 ##
 # @HostMemPolicy:
 #
diff --git a/qemu-options.hx b/qemu-options.hx
index d24197d59b..edf79cfe46 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -169,7 +169,8 @@ DEF("numa", HAS_ARG, QEMU_OPTION_numa,
     "-numa node[,memdev=id][,cpus=firstcpu[-lastcpu]][,nodeid=node][,initiator=node]\n"
     "-numa dist,src=source,dst=destination,val=distance\n"
     "-numa cpu,node-id=node[,socket-id=x][,core-id=y][,thread-id=z]\n"
-    "-numa hmat-lb,initiator=node,target=node,hierarchy=memory|first-level|second-level|third-level,data-type=access-latency|read-latency|write-latency[,latency=lat][,bandwidth=bw]\n",
+    "-numa hmat-lb,initiator=node,target=node,hierarchy=memory|first-level|second-level|third-level,data-type=access-latency|read-latency|write-latency[,latency=lat][,bandwidth=bw]\n"
+    "-numa hmat-cache,node-id=node,size=size,level=level[,assoc=none|direct|complex][,policy=none|write-back|write-through][,line=size]\n",
     QEMU_ARCH_ALL)
 STEXI
 @item -numa node[,mem=@var{size}][,cpus=@var{firstcpu}[-@var{lastcpu}]][,nodeid=@var{node}][,initiator=@var{initiator}]
@@ -177,6 +178,7 @@ STEXI
 @itemx -numa dist,src=@var{source},dst=@var{destination},val=@var{distance}
 @itemx -numa cpu,node-id=@var{node}[,socket-id=@var{x}][,core-id=@var{y}][,thread-id=@var{z}]
 @itemx -numa hmat-lb,initiator=@var{node},target=@var{node},hierarchy=@var{hierarchy},data-type=@var{tpye}[,latency=@var{lat}][,bandwidth=@var{bw}]
+@itemx -numa hmat-cache,node-id=@var{node},size=@var{size},level=@var{level}[,assoc=@var{str}][,policy=@var{str}][,line=@var{size}]
 @findex -numa
 Define a NUMA node and assign RAM and VCPUs to it.
 Set the NUMA distance from a source node to a destination node.
@@ -282,11 +284,19 @@ max NUM is 65534, if NUM is 0, means the corresponding latency or bandwidth info
 is not provided. And if input numbers without any unit, the latency unit will be 'ns'
 and the bandwidth will be MB/s.
 
+In @samp{hmat-cache} option, @var{node-id} is the NUMA-id of the memory belongs.
+@var{size} is the size of memory side cache in bytes. @var{level} is the cache
+level described in this structure. @var{assoc} is the cache associativity,
+the possible value is 'none/direct(direct-mapped)/complex(complex cache indexing)'.
+@var{policy} is the write policy. @var{line} is the cache Line size in bytes.
+
 For example, the following option assigns NUMA node 0 and 1. Node 0 has 2 cpus and
 a ram, node 1 has only a ram. The processors in node 0 access memory in node
 0 with access-latency 5 nanoseconds, access-bandwidth is 200 MB/s;
 The processors in NUMA node 0 access memory in NUMA node 1 with access-latency 10
 nanoseconds, access-bandwidth is 100 MB/s.
+And for memory side cache information, NUMA node 0 and 1 both have 1 level memory
+cache, size is 10KB, policy is write-back, the cache Line size is 8 bytes:
 @example
 -machine hmat=on \
 -m 2G \
@@ -300,7 +310,9 @@ nanoseconds, access-bandwidth is 100 MB/s.
 -numa hmat-lb,initiator=0,target=0,hierarchy=memory,data-type=access-latency,latency=5ns \
 -numa hmat-lb,initiator=0,target=0,hierarchy=memory,data-type=access-bandwidth,bandwidth=200M \
 -numa hmat-lb,initiator=0,target=1,hierarchy=memory,data-type=access-latency,latency=10ns \
--numa hmat-lb,initiator=0,target=1,hierarchy=memory,data-type=access-bandwidth,bandwidth=100M
+-numa hmat-lb,initiator=0,target=1,hierarchy=memory,data-type=access-bandwidth,bandwidth=100M \
+-numa hmat-cache,node-id=0,size=10K,level=1,assoc=direct,policy=write-back,line=8 \
+-numa hmat-cache,node-id=1,size=10K,level=1,assoc=direct,policy=write-back,line=8
 @end example
 
 ETEXI
-- 
2.20.1



^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v14 08/11] hmat acpi: Build Memory Proximity Domain Attributes Structure(s)
  2019-10-28  7:52 [PATCH v14 00/11] Build ACPI Heterogeneous Memory Attribute Table (HMAT) Tao Xu
                   ` (6 preceding siblings ...)
  2019-10-28  7:52 ` [PATCH v14 07/11] numa: Extend CLI to provide memory side cache information Tao Xu
@ 2019-10-28  7:52 ` Tao Xu
  2019-10-28  7:52 ` [PATCH v14 09/11] hmat acpi: Build System Locality Latency and Bandwidth Information Structure(s) Tao Xu
                   ` (4 subsequent siblings)
  12 siblings, 0 replies; 32+ messages in thread
From: Tao Xu @ 2019-10-28  7:52 UTC (permalink / raw)
  To: mst, imammedo, eblake, ehabkost, marcel.apfelbaum, armbru,
	mdroth, thuth, lvivier
  Cc: jingqi.liu, tao3.xu, fan.du, qemu-devel, Daniel Black, Jonathan Cameron

From: Liu Jingqi <jingqi.liu@intel.com>

HMAT is defined in ACPI 6.3: 5.2.27 Heterogeneous Memory Attribute Table
(HMAT). The specification references below link:
http://www.uefi.org/sites/default/files/resources/ACPI_6_3_final_Jan30.pdf

It describes the memory attributes, such as memory side cache
attributes and bandwidth and latency details, related to the
Memory Proximity Domain. The software is
expected to use this information as hint for optimization.

This structure describes Memory Proximity Domain Attributes by memory
subsystem and its associativity with processor proximity domain as well as
hint for memory usage.

In the linux kernel, the codes in drivers/acpi/hmat/hmat.c parse and report
the platform's HMAT tables.

Reviewed-by: Daniel Black <daniel@linux.ibm.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Liu Jingqi <jingqi.liu@intel.com>
Signed-off-by: Tao Xu <tao3.xu@intel.com>
---

No changes in v14.

Changes in v13:
    - Remove the unnecessary head file.
---
 hw/acpi/Kconfig       |  7 ++-
 hw/acpi/Makefile.objs |  1 +
 hw/acpi/hmat.c        | 99 +++++++++++++++++++++++++++++++++++++++++++
 hw/acpi/hmat.h        | 42 ++++++++++++++++++
 hw/i386/acpi-build.c  |  5 +++
 5 files changed, 152 insertions(+), 2 deletions(-)
 create mode 100644 hw/acpi/hmat.c
 create mode 100644 hw/acpi/hmat.h

diff --git a/hw/acpi/Kconfig b/hw/acpi/Kconfig
index 12e3f1e86e..54209c6f2f 100644
--- a/hw/acpi/Kconfig
+++ b/hw/acpi/Kconfig
@@ -7,6 +7,7 @@ config ACPI_X86
     select ACPI_NVDIMM
     select ACPI_CPU_HOTPLUG
     select ACPI_MEMORY_HOTPLUG
+    select ACPI_HMAT
 
 config ACPI_X86_ICH
     bool
@@ -23,6 +24,10 @@ config ACPI_NVDIMM
     bool
     depends on ACPI
 
+config ACPI_HMAT
+    bool
+    depends on ACPI
+
 config ACPI_PCI
     bool
     depends on ACPI && PCI
@@ -33,5 +38,3 @@ config ACPI_VMGENID
     depends on PC
 
 config ACPI_HW_REDUCED
-    bool
-    depends on ACPI
diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
index 655a9c1973..517bd88704 100644
--- a/hw/acpi/Makefile.objs
+++ b/hw/acpi/Makefile.objs
@@ -7,6 +7,7 @@ common-obj-$(CONFIG_ACPI_CPU_HOTPLUG) += cpu.o
 common-obj-$(CONFIG_ACPI_NVDIMM) += nvdimm.o
 common-obj-$(CONFIG_ACPI_VMGENID) += vmgenid.o
 common-obj-$(CONFIG_ACPI_HW_REDUCED) += generic_event_device.o
+common-obj-$(CONFIG_ACPI_HMAT) += hmat.o
 common-obj-$(call lnot,$(CONFIG_ACPI_X86)) += acpi-stub.o
 
 common-obj-y += acpi_interface.o
diff --git a/hw/acpi/hmat.c b/hw/acpi/hmat.c
new file mode 100644
index 0000000000..c595098ba7
--- /dev/null
+++ b/hw/acpi/hmat.c
@@ -0,0 +1,99 @@
+/*
+ * HMAT ACPI Implementation
+ *
+ * Copyright(C) 2019 Intel Corporation.
+ *
+ * Author:
+ *  Liu jingqi <jingqi.liu@linux.intel.com>
+ *  Tao Xu <tao3.xu@intel.com>
+ *
+ * HMAT is defined in ACPI 6.3: 5.2.27 Heterogeneous Memory Attribute Table
+ * (HMAT)
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>
+ */
+
+#include "qemu/osdep.h"
+#include "sysemu/numa.h"
+#include "hw/acpi/hmat.h"
+
+/*
+ * ACPI 6.3:
+ * 5.2.27.3 Memory Proximity Domain Attributes Structure: Table 5-145
+ */
+static void build_hmat_mpda(GArray *table_data, uint16_t flags,
+                            uint16_t initiator, uint16_t mem_node)
+{
+
+    /* Memory Proximity Domain Attributes Structure */
+    /* Type */
+    build_append_int_noprefix(table_data, 0, 2);
+    /* Reserved */
+    build_append_int_noprefix(table_data, 0, 2);
+    /* Length */
+    build_append_int_noprefix(table_data, 40, 4);
+    /* Flags */
+    build_append_int_noprefix(table_data, flags, 2);
+    /* Reserved */
+    build_append_int_noprefix(table_data, 0, 2);
+    /* Proximity Domain for the Attached Initiator */
+    build_append_int_noprefix(table_data, initiator, 4);
+    /* Proximity Domain for the Memory */
+    build_append_int_noprefix(table_data, mem_node, 4);
+    /* Reserved */
+    build_append_int_noprefix(table_data, 0, 4);
+    /*
+     * Reserved:
+     * Previously defined as the Start Address of the System Physical
+     * Address Range. Deprecated since ACPI Spec 6.3.
+     */
+    build_append_int_noprefix(table_data, 0, 8);
+    /*
+     * Reserved:
+     * Previously defined as the Range Length of the region in bytes.
+     * Deprecated since ACPI Spec 6.3.
+     */
+    build_append_int_noprefix(table_data, 0, 8);
+}
+
+/* Build HMAT sub table structures */
+static void hmat_build_table_structs(GArray *table_data, NumaState *numa_state)
+{
+    uint16_t flags;
+    int i;
+
+    for (i = 0; i < numa_state->num_nodes; i++) {
+        flags = 0;
+
+        if (numa_state->nodes[i].initiator < MAX_NODES) {
+            flags |= HMAT_PROXIMITY_INITIATOR_VALID;
+        }
+
+        build_hmat_mpda(table_data, flags, numa_state->nodes[i].initiator, i);
+    }
+}
+
+void build_hmat(GArray *table_data, BIOSLinker *linker, NumaState *numa_state)
+{
+    int hmat_start = table_data->len;
+
+    /* reserve space for HMAT header  */
+    acpi_data_push(table_data, 40);
+
+    hmat_build_table_structs(table_data, numa_state);
+
+    build_header(linker, table_data,
+                 (void *)(table_data->data + hmat_start),
+                 "HMAT", table_data->len - hmat_start, 2, NULL, NULL);
+}
diff --git a/hw/acpi/hmat.h b/hw/acpi/hmat.h
new file mode 100644
index 0000000000..437dbc6872
--- /dev/null
+++ b/hw/acpi/hmat.h
@@ -0,0 +1,42 @@
+/*
+ * HMAT ACPI Implementation Header
+ *
+ * Copyright(C) 2019 Intel Corporation.
+ *
+ * Author:
+ *  Liu jingqi <jingqi.liu@linux.intel.com>
+ *  Tao Xu <tao3.xu@intel.com>
+ *
+ * HMAT is defined in ACPI 6.3: 5.2.27 Heterogeneous Memory Attribute Table
+ * (HMAT)
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>
+ */
+
+#ifndef HMAT_H
+#define HMAT_H
+
+#include "hw/acpi/aml-build.h"
+
+/*
+ * ACPI 6.3: 5.2.27.3 Memory Proximity Domain Attributes Structure,
+ * Table 5-145, Field "flag", Bit [0]: set to 1 to indicate that data in
+ * the Proximity Domain for the Attached Initiator field is valid.
+ * Other bits reserved.
+ */
+#define HMAT_PROXIMITY_INITIATOR_VALID  0x1
+
+void build_hmat(GArray *table_data, BIOSLinker *linker, NumaState *numa_state);
+
+#endif
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index d9435ba0b3..3c936cd262 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -68,6 +68,7 @@
 #include "hw/i386/intel_iommu.h"
 
 #include "hw/acpi/ipmi.h"
+#include "hw/acpi/hmat.h"
 
 /* These are used to size the ACPI tables for -M pc-i440fx-1.7 and
  * -M pc-i440fx-2.0.  Even if the actual amount of AML generated grows
@@ -2718,6 +2719,10 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
             acpi_add_table(table_offsets, tables_blob);
             build_slit(tables_blob, tables->linker, machine);
         }
+        if (machine->numa_state->hmat_enabled) {
+            acpi_add_table(table_offsets, tables_blob);
+            build_hmat(tables_blob, tables->linker, machine->numa_state);
+        }
     }
     if (acpi_get_mcfg(&mcfg)) {
         acpi_add_table(table_offsets, tables_blob);
-- 
2.20.1



^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v14 09/11] hmat acpi: Build System Locality Latency and Bandwidth Information Structure(s)
  2019-10-28  7:52 [PATCH v14 00/11] Build ACPI Heterogeneous Memory Attribute Table (HMAT) Tao Xu
                   ` (7 preceding siblings ...)
  2019-10-28  7:52 ` [PATCH v14 08/11] hmat acpi: Build Memory Proximity Domain Attributes Structure(s) Tao Xu
@ 2019-10-28  7:52 ` Tao Xu
  2019-10-28  7:52 ` [PATCH v14 10/11] hmat acpi: Build Memory Side Cache " Tao Xu
                   ` (3 subsequent siblings)
  12 siblings, 0 replies; 32+ messages in thread
From: Tao Xu @ 2019-10-28  7:52 UTC (permalink / raw)
  To: mst, imammedo, eblake, ehabkost, marcel.apfelbaum, armbru,
	mdroth, thuth, lvivier
  Cc: jingqi.liu, tao3.xu, fan.du, qemu-devel, jonathan.cameron

From: Liu Jingqi <jingqi.liu@intel.com>

This structure describes the memory access latency and bandwidth
information from various memory access initiator proximity domains.
The latency and bandwidth numbers represented in this structure
correspond to rated latency and bandwidth for the platform.
The software could use this information as hint for optimization.

Signed-off-by: Liu Jingqi <jingqi.liu@intel.com>
Signed-off-by: Tao Xu <tao3.xu@intel.com>
---

No changes in v14.

Changes in v13:
    - Calculate the entries in a new patch.
---
 hw/acpi/hmat.c | 96 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 95 insertions(+), 1 deletion(-)

diff --git a/hw/acpi/hmat.c b/hw/acpi/hmat.c
index c595098ba7..6ec1310e62 100644
--- a/hw/acpi/hmat.c
+++ b/hw/acpi/hmat.c
@@ -27,6 +27,7 @@
 #include "qemu/osdep.h"
 #include "sysemu/numa.h"
 #include "hw/acpi/hmat.h"
+#include "qemu/error-report.h"
 
 /*
  * ACPI 6.3:
@@ -67,11 +68,81 @@ static void build_hmat_mpda(GArray *table_data, uint16_t flags,
     build_append_int_noprefix(table_data, 0, 8);
 }
 
+/*
+ * ACPI 6.3: 5.2.27.4 System Locality Latency and Bandwidth Information
+ * Structure: Table 5-146
+ */
+static void build_hmat_lb(GArray *table_data, HMAT_LB_Info *hmat_lb,
+                          uint32_t num_initiator, uint32_t num_target,
+                          uint32_t *initiator_list)
+{
+    int i;
+    uint16_t *lb_data;
+    uint32_t base;
+    /*
+     * Length in bytes for entire structure, including 32 bytes of
+     * fixed length, length of initiator proximity domain list,
+     * length of target proximity domain list and length of entries
+     * provides latency/bandwidth values.
+     */
+    uint32_t lb_length = 32 + 4 * num_initiator + 4 * num_target +
+                              2 * num_initiator * num_target;
+
+    /* Type */
+    build_append_int_noprefix(table_data, 1, 2);
+    /* Reserved */
+    build_append_int_noprefix(table_data, 0, 2);
+    /* Length */
+    build_append_int_noprefix(table_data, lb_length, 4);
+    /* Flags: Bits [3:0] Memory Hierarchy, Bits[7:4] Reserved */
+    assert(!(hmat_lb->hierarchy >> 4));
+    build_append_int_noprefix(table_data, hmat_lb->hierarchy, 1);
+    /* Data Type */
+    build_append_int_noprefix(table_data, hmat_lb->data_type, 1);
+    /* Reserved */
+    build_append_int_noprefix(table_data, 0, 2);
+    /* Number of Initiator Proximity Domains (s) */
+    build_append_int_noprefix(table_data, num_initiator, 4);
+    /* Number of Target Proximity Domains (t) */
+    build_append_int_noprefix(table_data, num_target, 4);
+    /* Reserved */
+    build_append_int_noprefix(table_data, 0, 4);
+
+    if (hmat_lb->data_type <= HMAT_LB_DATA_WRITE_LATENCY) {
+        base = hmat_lb->base_latency;
+        lb_data = hmat_lb->entry_latency;
+    } else {
+        base = hmat_lb->base_bandwidth;
+        lb_data = hmat_lb->entry_bandwidth;
+    }
+
+    /* Entry Base Unit */
+    build_append_int_noprefix(table_data, base, 8);
+
+    /* Initiator Proximity Domain List */
+    for (i = 0; i < num_initiator; i++) {
+        build_append_int_noprefix(table_data, initiator_list[i], 4);
+    }
+
+    /* Target Proximity Domain List */
+    for (i = 0; i < num_target; i++) {
+        build_append_int_noprefix(table_data, i, 4);
+    }
+
+    /* Latency or Bandwidth Entries */
+    for (i = 0; i < num_initiator * num_target; i++) {
+        build_append_int_noprefix(table_data, lb_data[i], 2);
+    }
+}
+
 /* Build HMAT sub table structures */
 static void hmat_build_table_structs(GArray *table_data, NumaState *numa_state)
 {
     uint16_t flags;
-    int i;
+    uint32_t num_initiator = 0;
+    uint32_t initiator_list[MAX_NODES];
+    int i, hierarchy, type;
+    HMAT_LB_Info *hmat_lb;
 
     for (i = 0; i < numa_state->num_nodes; i++) {
         flags = 0;
@@ -82,6 +153,29 @@ static void hmat_build_table_structs(GArray *table_data, NumaState *numa_state)
 
         build_hmat_mpda(table_data, flags, numa_state->nodes[i].initiator, i);
     }
+
+    for (i = 0; i < numa_state->num_nodes; i++) {
+        if (numa_state->nodes[i].has_cpu) {
+            initiator_list[num_initiator++] = i;
+        }
+    }
+
+    /*
+     * ACPI 6.3: 5.2.27.4 System Locality Latency and Bandwidth Information
+     * Structure: Table 5-146
+     */
+    for (hierarchy = HMAT_LB_MEM_MEMORY;
+         hierarchy <= HMAT_LB_MEM_CACHE_3RD_LEVEL; hierarchy++) {
+        for (type = HMAT_LB_DATA_ACCESS_LATENCY;
+             type <= HMAT_LB_DATA_WRITE_BANDWIDTH; type++) {
+            hmat_lb = numa_state->hmat_lb[hierarchy][type];
+
+            if (hmat_lb) {
+                build_hmat_lb(table_data, hmat_lb, num_initiator,
+                              numa_state->num_nodes, initiator_list);
+            }
+        }
+    }
 }
 
 void build_hmat(GArray *table_data, BIOSLinker *linker, NumaState *numa_state)
-- 
2.20.1



^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v14 10/11] hmat acpi: Build Memory Side Cache Information Structure(s)
  2019-10-28  7:52 [PATCH v14 00/11] Build ACPI Heterogeneous Memory Attribute Table (HMAT) Tao Xu
                   ` (8 preceding siblings ...)
  2019-10-28  7:52 ` [PATCH v14 09/11] hmat acpi: Build System Locality Latency and Bandwidth Information Structure(s) Tao Xu
@ 2019-10-28  7:52 ` Tao Xu
  2019-10-28  7:52 ` [PATCH v14 11/11] tests/bios-tables-test: add test cases for ACPI HMAT Tao Xu
                   ` (2 subsequent siblings)
  12 siblings, 0 replies; 32+ messages in thread
From: Tao Xu @ 2019-10-28  7:52 UTC (permalink / raw)
  To: mst, imammedo, eblake, ehabkost, marcel.apfelbaum, armbru,
	mdroth, thuth, lvivier
  Cc: jingqi.liu, tao3.xu, fan.du, qemu-devel, Daniel Black, Jonathan Cameron

From: Liu Jingqi <jingqi.liu@intel.com>

This structure describes memory side cache information for memory
proximity domains if the memory side cache is present and the
physical device forms the memory side cache.
The software could use this information to effectively place
the data in memory to maximize the performance of the system
memory that use the memory side cache.

Reviewed-by: Daniel Black <daniel@linux.ibm.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Liu Jingqi <jingqi.liu@intel.com>
Signed-off-by: Tao Xu <tao3.xu@intel.com>
---

No changes in v14.

Changes in v13:
    - rename level as cache_level
---
 hw/acpi/hmat.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 71 insertions(+), 1 deletion(-)

diff --git a/hw/acpi/hmat.c b/hw/acpi/hmat.c
index 6ec1310e62..931aa60678 100644
--- a/hw/acpi/hmat.c
+++ b/hw/acpi/hmat.c
@@ -135,14 +135,63 @@ static void build_hmat_lb(GArray *table_data, HMAT_LB_Info *hmat_lb,
     }
 }
 
+/* ACPI 6.3: 5.2.27.5 Memory Side Cache Information Structure: Table 5-147 */
+static void build_hmat_cache(GArray *table_data, HMAT_Cache_Info *hmat_cache)
+{
+    /*
+     * Cache Attributes: Bits [3:0] – Total Cache Levels
+     * for this Memory Proximity Domain
+     */
+    uint32_t cache_attr = hmat_cache->total_levels & 0xF;
+
+    /* Bits [7:4] : Cache Level described in this structure */
+    cache_attr |= (hmat_cache->level & 0xF) << 4;
+
+    /* Bits [11:8] - Cache Associativity */
+    cache_attr |= (hmat_cache->associativity & 0x7) << 8;
+
+    /* Bits [15:12] - Write Policy */
+    cache_attr |= (hmat_cache->write_policy & 0x7) << 12;
+
+    /* Bits [31:16] - Cache Line size in bytes */
+    cache_attr |= (hmat_cache->line_size & 0xFFFF) << 16;
+
+    cache_attr = cpu_to_le32(cache_attr);
+
+    /* Type */
+    build_append_int_noprefix(table_data, 2, 2);
+    /* Reserved */
+    build_append_int_noprefix(table_data, 0, 2);
+    /* Length */
+    build_append_int_noprefix(table_data, 32, 4);
+    /* Proximity Domain for the Memory */
+    build_append_int_noprefix(table_data, hmat_cache->proximity, 4);
+    /* Reserved */
+    build_append_int_noprefix(table_data, 0, 4);
+    /* Memory Side Cache Size */
+    build_append_int_noprefix(table_data, hmat_cache->size, 8);
+    /* Cache Attributes */
+    build_append_int_noprefix(table_data, cache_attr, 4);
+    /* Reserved */
+    build_append_int_noprefix(table_data, 0, 2);
+    /*
+     * Number of SMBIOS handles (n)
+     * Linux kernel uses Memory Side Cache Information Structure
+     * without SMBIOS entries for now, so set Number of SMBIOS handles
+     * as 0.
+     */
+    build_append_int_noprefix(table_data, 0, 2);
+}
+
 /* Build HMAT sub table structures */
 static void hmat_build_table_structs(GArray *table_data, NumaState *numa_state)
 {
     uint16_t flags;
     uint32_t num_initiator = 0;
     uint32_t initiator_list[MAX_NODES];
-    int i, hierarchy, type;
+    int i, hierarchy, type, cache_level, total_levels;
     HMAT_LB_Info *hmat_lb;
+    HMAT_Cache_Info *hmat_cache;
 
     for (i = 0; i < numa_state->num_nodes; i++) {
         flags = 0;
@@ -176,6 +225,27 @@ static void hmat_build_table_structs(GArray *table_data, NumaState *numa_state)
             }
         }
     }
+
+    /*
+     * ACPI 6.3: 5.2.27.5 Memory Side Cache Information Structure:
+     * Table 5-147
+     */
+    for (i = 0; i < numa_state->num_nodes; i++) {
+        total_levels = 0;
+        for (cache_level = 1; cache_level <= MAX_HMAT_CACHE_LEVEL;
+             cache_level++) {
+            if (numa_state->hmat_cache[i][cache_level]) {
+                total_levels++;
+            }
+        }
+        for (cache_level = 0; cache_level <= total_levels; cache_level++) {
+            hmat_cache = numa_state->hmat_cache[i][cache_level];
+            if (hmat_cache) {
+                hmat_cache->total_levels = total_levels;
+                build_hmat_cache(table_data, hmat_cache);
+            }
+        }
+    }
 }
 
 void build_hmat(GArray *table_data, BIOSLinker *linker, NumaState *numa_state)
-- 
2.20.1



^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v14 11/11] tests/bios-tables-test: add test cases for ACPI HMAT
  2019-10-28  7:52 [PATCH v14 00/11] Build ACPI Heterogeneous Memory Attribute Table (HMAT) Tao Xu
                   ` (9 preceding siblings ...)
  2019-10-28  7:52 ` [PATCH v14 10/11] hmat acpi: Build Memory Side Cache " Tao Xu
@ 2019-10-28  7:52 ` Tao Xu
  2019-10-28  8:39   ` Michael S. Tsirkin
  2019-10-28  8:36 ` [PATCH v14 00/11] Build ACPI Heterogeneous Memory Attribute Table (HMAT) no-reply
  2019-11-06  8:39 ` Tao Xu
  12 siblings, 1 reply; 32+ messages in thread
From: Tao Xu @ 2019-10-28  7:52 UTC (permalink / raw)
  To: mst, imammedo, eblake, ehabkost, marcel.apfelbaum, armbru,
	mdroth, thuth, lvivier
  Cc: Jingqi Liu, tao3.xu, fan.du, qemu-devel, Daniel Black, jonathan.cameron

ACPI table HMAT has been introduced, QEMU now builds HMAT tables for
Heterogeneous Memory with boot option '-numa node'.

Add test cases on PC and Q35 machines with 2 numa nodes.
Because HMAT is generated when system enable numa, the
following tables need to be added for this test:
  tests/acpi-test-data/pc/*.acpihmat
  tests/acpi-test-data/pc/HMAT.*
  tests/acpi-test-data/q35/*.acpihmat
  tests/acpi-test-data/q35/HMAT.*

Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Daniel Black <daniel@linux.ibm.com>
Reviewed-by: Jingqi Liu <Jingqi.liu@intel.com>
Suggested-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Tao Xu <tao3.xu@intel.com>
---

No changes in v14.

Changes in v13:
    - Use decimal notation with appropriate suffix for cache size
---
 tests/bios-tables-test.c | 44 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index 0b33fb265f..96803c1f20 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -947,6 +947,48 @@ static void test_acpi_virt_tcg_numamem(void)
 
 }
 
+static void test_acpi_tcg_acpi_hmat(const char *machine)
+{
+    test_data data;
+
+    memset(&data, 0, sizeof(data));
+    data.machine = machine;
+    data.variant = ".acpihmat";
+    test_acpi_one(" -machine hmat=on"
+                  " -smp 2,sockets=2"
+                  " -m 128M,slots=2,maxmem=1G"
+                  " -object memory-backend-ram,size=64M,id=m0"
+                  " -object memory-backend-ram,size=64M,id=m1"
+                  " -numa node,nodeid=0,memdev=m0"
+                  " -numa node,nodeid=1,memdev=m1,initiator=0"
+                  " -numa cpu,node-id=0,socket-id=0"
+                  " -numa cpu,node-id=0,socket-id=1"
+                  " -numa hmat-lb,initiator=0,target=0,hierarchy=memory,"
+                  "data-type=access-latency,latency=5ns"
+                  " -numa hmat-lb,initiator=0,target=0,hierarchy=memory,"
+                  "data-type=access-bandwidth,bandwidth=500M"
+                  " -numa hmat-lb,initiator=0,target=1,hierarchy=memory,"
+                  "data-type=access-latency,latency=10ns"
+                  " -numa hmat-lb,initiator=0,target=1,hierarchy=memory,"
+                  "data-type=access-bandwidth,bandwidth=100M"
+                  " -numa hmat-cache,node-id=0,size=10K,level=1,assoc=direct,"
+                  "policy=write-back,line=8"
+                  " -numa hmat-cache,node-id=1,size=10K,level=1,assoc=direct,"
+                  "policy=write-back,line=8",
+                  &data);
+    free_test_data(&data);
+}
+
+static void test_acpi_q35_tcg_acpi_hmat(void)
+{
+    test_acpi_tcg_acpi_hmat(MACHINE_Q35);
+}
+
+static void test_acpi_piix4_tcg_acpi_hmat(void)
+{
+    test_acpi_tcg_acpi_hmat(MACHINE_PC);
+}
+
 static void test_acpi_virt_tcg(void)
 {
     test_data data = {
@@ -991,6 +1033,8 @@ int main(int argc, char *argv[])
         qtest_add_func("acpi/q35/numamem", test_acpi_q35_tcg_numamem);
         qtest_add_func("acpi/piix4/dimmpxm", test_acpi_piix4_tcg_dimm_pxm);
         qtest_add_func("acpi/q35/dimmpxm", test_acpi_q35_tcg_dimm_pxm);
+        qtest_add_func("acpi/piix4/acpihmat", test_acpi_piix4_tcg_acpi_hmat);
+        qtest_add_func("acpi/q35/acpihmat", test_acpi_q35_tcg_acpi_hmat);
     } else if (strcmp(arch, "aarch64") == 0) {
         qtest_add_func("acpi/virt", test_acpi_virt_tcg);
         qtest_add_func("acpi/virt/numamem", test_acpi_virt_tcg_numamem);
-- 
2.20.1



^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v14 00/11] Build ACPI Heterogeneous Memory Attribute Table (HMAT)
  2019-10-28  7:52 [PATCH v14 00/11] Build ACPI Heterogeneous Memory Attribute Table (HMAT) Tao Xu
                   ` (10 preceding siblings ...)
  2019-10-28  7:52 ` [PATCH v14 11/11] tests/bios-tables-test: add test cases for ACPI HMAT Tao Xu
@ 2019-10-28  8:36 ` no-reply
  2019-11-06  8:39 ` Tao Xu
  12 siblings, 0 replies; 32+ messages in thread
From: no-reply @ 2019-10-28  8:36 UTC (permalink / raw)
  To: tao3.xu
  Cc: lvivier, thuth, ehabkost, mst, qemu-devel, jingqi.liu, tao3.xu,
	fan.du, armbru, mdroth, jonathan.cameron, imammedo

Patchew URL: https://patchew.org/QEMU/20191028075220.25673-1-tao3.xu@intel.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

Looking for expected file 'tests/data/acpi/pc/SRAT.acpihmat'
Looking for expected file 'tests/data/acpi/pc/SRAT'
**
ERROR:/tmp/qemu-test/src/tests/bios-tables-test.c:354:load_expected_aml: assertion failed: (exp_sdt.aml_file)
ERROR - Bail out! ERROR:/tmp/qemu-test/src/tests/bios-tables-test.c:354:load_expected_aml: assertion failed: (exp_sdt.aml_file)
make: *** [check-qtest-x86_64] Error 1
make: *** Waiting for unfinished jobs....
  TEST    iotest-qcow2: 035
  TEST    iotest-qcow2: 036
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=1a4bf9c587854f81918532b75d1e65b8', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-3ruj_rni/src/docker-src.2019-10-28-04.25.27.15934:/var/tmp/qemu:z,ro', 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=1a4bf9c587854f81918532b75d1e65b8
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-3ruj_rni/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    11m10.684s
user    0m8.369s


The full log is available at
http://patchew.org/logs/20191028075220.25673-1-tao3.xu@intel.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v14 11/11] tests/bios-tables-test: add test cases for ACPI HMAT
  2019-10-28  7:52 ` [PATCH v14 11/11] tests/bios-tables-test: add test cases for ACPI HMAT Tao Xu
@ 2019-10-28  8:39   ` Michael S. Tsirkin
  2019-10-28  8:50     ` Tao Xu
  0 siblings, 1 reply; 32+ messages in thread
From: Michael S. Tsirkin @ 2019-10-28  8:39 UTC (permalink / raw)
  To: Tao Xu
  Cc: lvivier, thuth, ehabkost, qemu-devel, jingqi.liu, fan.du, mdroth,
	Daniel Black, armbru, jonathan.cameron, imammedo

On Mon, Oct 28, 2019 at 03:52:20PM +0800, Tao Xu wrote:
> ACPI table HMAT has been introduced, QEMU now builds HMAT tables for
> Heterogeneous Memory with boot option '-numa node'.
> 
> Add test cases on PC and Q35 machines with 2 numa nodes.
> Because HMAT is generated when system enable numa, the
> following tables need to be added for this test:
>   tests/acpi-test-data/pc/*.acpihmat
>   tests/acpi-test-data/pc/HMAT.*
>   tests/acpi-test-data/q35/*.acpihmat
>   tests/acpi-test-data/q35/HMAT.*
> 
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> Reviewed-by: Daniel Black <daniel@linux.ibm.com>
> Reviewed-by: Jingqi Liu <Jingqi.liu@intel.com>
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Tao Xu <tao3.xu@intel.com>
> ---
> 
> No changes in v14.
> 
> Changes in v13:
>     - Use decimal notation with appropriate suffix for cache size

As you have seen adding tests like this breaks CI.
Pls see the comment at the beginning of tests/bios-tables-test.c
for how to add tests without breaking CI.



> ---
>  tests/bios-tables-test.c | 44 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> index 0b33fb265f..96803c1f20 100644
> --- a/tests/bios-tables-test.c
> +++ b/tests/bios-tables-test.c
> @@ -947,6 +947,48 @@ static void test_acpi_virt_tcg_numamem(void)
>  
>  }
>  
> +static void test_acpi_tcg_acpi_hmat(const char *machine)
> +{
> +    test_data data;
> +
> +    memset(&data, 0, sizeof(data));
> +    data.machine = machine;
> +    data.variant = ".acpihmat";
> +    test_acpi_one(" -machine hmat=on"
> +                  " -smp 2,sockets=2"
> +                  " -m 128M,slots=2,maxmem=1G"
> +                  " -object memory-backend-ram,size=64M,id=m0"
> +                  " -object memory-backend-ram,size=64M,id=m1"
> +                  " -numa node,nodeid=0,memdev=m0"
> +                  " -numa node,nodeid=1,memdev=m1,initiator=0"
> +                  " -numa cpu,node-id=0,socket-id=0"
> +                  " -numa cpu,node-id=0,socket-id=1"
> +                  " -numa hmat-lb,initiator=0,target=0,hierarchy=memory,"
> +                  "data-type=access-latency,latency=5ns"
> +                  " -numa hmat-lb,initiator=0,target=0,hierarchy=memory,"
> +                  "data-type=access-bandwidth,bandwidth=500M"
> +                  " -numa hmat-lb,initiator=0,target=1,hierarchy=memory,"
> +                  "data-type=access-latency,latency=10ns"
> +                  " -numa hmat-lb,initiator=0,target=1,hierarchy=memory,"
> +                  "data-type=access-bandwidth,bandwidth=100M"
> +                  " -numa hmat-cache,node-id=0,size=10K,level=1,assoc=direct,"
> +                  "policy=write-back,line=8"
> +                  " -numa hmat-cache,node-id=1,size=10K,level=1,assoc=direct,"
> +                  "policy=write-back,line=8",
> +                  &data);
> +    free_test_data(&data);
> +}
> +
> +static void test_acpi_q35_tcg_acpi_hmat(void)
> +{
> +    test_acpi_tcg_acpi_hmat(MACHINE_Q35);
> +}
> +
> +static void test_acpi_piix4_tcg_acpi_hmat(void)
> +{
> +    test_acpi_tcg_acpi_hmat(MACHINE_PC);
> +}
> +
>  static void test_acpi_virt_tcg(void)
>  {
>      test_data data = {
> @@ -991,6 +1033,8 @@ int main(int argc, char *argv[])
>          qtest_add_func("acpi/q35/numamem", test_acpi_q35_tcg_numamem);
>          qtest_add_func("acpi/piix4/dimmpxm", test_acpi_piix4_tcg_dimm_pxm);
>          qtest_add_func("acpi/q35/dimmpxm", test_acpi_q35_tcg_dimm_pxm);
> +        qtest_add_func("acpi/piix4/acpihmat", test_acpi_piix4_tcg_acpi_hmat);
> +        qtest_add_func("acpi/q35/acpihmat", test_acpi_q35_tcg_acpi_hmat);
>      } else if (strcmp(arch, "aarch64") == 0) {
>          qtest_add_func("acpi/virt", test_acpi_virt_tcg);
>          qtest_add_func("acpi/virt/numamem", test_acpi_virt_tcg_numamem);
> -- 
> 2.20.1


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v14 11/11] tests/bios-tables-test: add test cases for ACPI HMAT
  2019-10-28  8:39   ` Michael S. Tsirkin
@ 2019-10-28  8:50     ` Tao Xu
  0 siblings, 0 replies; 32+ messages in thread
From: Tao Xu @ 2019-10-28  8:50 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: lvivier, thuth, ehabkost, qemu-devel, Liu, Jingqi, Du, Fan,
	mdroth, Daniel Black, armbru, jonathan.cameron, imammedo

On 10/28/2019 4:39 PM, Michael S. Tsirkin wrote:
> On Mon, Oct 28, 2019 at 03:52:20PM +0800, Tao Xu wrote:
>> ACPI table HMAT has been introduced, QEMU now builds HMAT tables for
>> Heterogeneous Memory with boot option '-numa node'.
>>
>> Add test cases on PC and Q35 machines with 2 numa nodes.
>> Because HMAT is generated when system enable numa, the
>> following tables need to be added for this test:
>>    tests/acpi-test-data/pc/*.acpihmat
>>    tests/acpi-test-data/pc/HMAT.*
>>    tests/acpi-test-data/q35/*.acpihmat
>>    tests/acpi-test-data/q35/HMAT.*
>>
>> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
>> Reviewed-by: Daniel Black <daniel@linux.ibm.com>
>> Reviewed-by: Jingqi Liu <Jingqi.liu@intel.com>
>> Suggested-by: Igor Mammedov <imammedo@redhat.com>
>> Signed-off-by: Tao Xu <tao3.xu@intel.com>
>> ---
>>
>> No changes in v14.
>>
>> Changes in v13:
>>      - Use decimal notation with appropriate suffix for cache size
> 
> As you have seen adding tests like this breaks CI.
> Pls see the comment at the beginning of tests/bios-tables-test.c
> for how to add tests without breaking CI.
> 

Thanks for reminding me.



^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v14 00/11] Build ACPI Heterogeneous Memory Attribute Table (HMAT)
  2019-10-28  7:52 [PATCH v14 00/11] Build ACPI Heterogeneous Memory Attribute Table (HMAT) Tao Xu
                   ` (11 preceding siblings ...)
  2019-10-28  8:36 ` [PATCH v14 00/11] Build ACPI Heterogeneous Memory Attribute Table (HMAT) no-reply
@ 2019-11-06  8:39 ` Tao Xu
  12 siblings, 0 replies; 32+ messages in thread
From: Tao Xu @ 2019-11-06  8:39 UTC (permalink / raw)
  To: mst, imammedo, eblake, ehabkost, marcel.apfelbaum, armbru,
	mdroth, thuth, lvivier
  Cc: Liu, Jingqi, Du, Fan, qemu-devel, jonathan.cameron

Ping for comments:)

On 10/28/2019 3:52 PM, Xu, Tao3 wrote:
> This series of patches will build Heterogeneous Memory Attribute Table (HMAT)
> according to the command line. The ACPI HMAT describes the memory attributes,
> such as memory side cache attributes and bandwidth and latency details,
> related to the Memory Proximity Domain.
> The software is expected to use HMAT information as hint for optimization.
> 
> In the linux kernel, the codes in drivers/acpi/hmat/hmat.c parse and report
> the platform's HMAT tables.
> 
> The V13 patches link:
> https://patchwork.kernel.org/cover/11200909/
> 
> Changelog:
> v14:
>      - Reuse the codes of do_strtosz to build qemu_strtotime_ns
>        (Eduardo)
>      - Squash patch v13 01/12 and 02/12 together (Daniel and Eduardo)
>      - Drop time unit picosecond (Eric)
>      - Use qemu ctz64 and clz64 instead of builtin function
> v13:
>      - Modify some text description
>      - Drop "initiator_valid" field in struct NodeInfo
>      - Reuse Garray to store the raw bandwidth and bandwidth data
>      - Calculate common base unit using range bitmap
>      - Add a patch to alculate hmat latency and bandwidth entry list
>      - Drop the total_levels option and use readable cache size
>      - Remove the unnecessary head file
>      - Use decimal notation with appropriate suffix for cache size
> v12:
>      - Fix a bug that a memory-only node without initiator setting
>        doesn't report error. (reported by Danmei Wei)
>      - Fix a bug that if HMAT is enabled and without hmat-lb setting,
>        QEMU will crash. (reported by Danmei Wei)
> v11:
>      - Move numa option patches forward.
>      - Add num_initiator in Numa_state to record the number of
>      initiators.
>      - Simplify struct HMAT_LB_Info, use uint64_t array to store data.
>      - Drop hmat_get_base().
>      - Calculate base in build_hmat_lb().
> v10:
>      - Add qemu_strtotime_ps() to convert strings with time suffixes
>      to numbers, and add some tests for it.
>      - Add qapi buildin type time, and add some tests for it.
>      - Add machine oprion properties "-machine hmat=on|off" for
> 	  enabling or disabling HMAT in QEMU.
> 
> Liu Jingqi (5):
>    numa: Extend CLI to provide memory latency and bandwidth information
>    numa: Extend CLI to provide memory side cache information
>    hmat acpi: Build Memory Proximity Domain Attributes Structure(s)
>    hmat acpi: Build System Locality Latency and Bandwidth Information
>      Structure(s)
>    hmat acpi: Build Memory Side Cache Information Structure(s)
> 
> Tao Xu (6):
>    util/cutils: Add qemu_strtotime_ns()
>    qapi: Add builtin type time
>    tests: Add test for QAPI builtin type time
>    numa: Extend CLI to provide initiator information for numa nodes
>    numa: Calculate hmat latency and bandwidth entry list
>    tests/bios-tables-test: add test cases for ACPI HMAT
> 
>   hw/acpi/Kconfig                    |   7 +-
>   hw/acpi/Makefile.objs              |   1 +
>   hw/acpi/hmat.c                     | 263 ++++++++++++++++++++++++++
>   hw/acpi/hmat.h                     |  42 +++++
>   hw/core/machine.c                  |  64 +++++++
>   hw/core/numa.c                     | 284 ++++++++++++++++++++++++++++-
>   hw/i386/acpi-build.c               |   5 +
>   include/qapi/visitor-impl.h        |   4 +
>   include/qapi/visitor.h             |   8 +
>   include/qemu/cutils.h              |   1 +
>   include/sysemu/numa.h              | 104 +++++++++++
>   qapi/machine.json                  | 178 +++++++++++++++++-
>   qapi/opts-visitor.c                |  22 +++
>   qapi/qapi-visit-core.c             |  12 ++
>   qapi/qobject-input-visitor.c       |  18 ++
>   qapi/trace-events                  |   1 +
>   qemu-options.hx                    |  96 +++++++++-
>   scripts/qapi/schema.py             |   1 +
>   tests/bios-tables-test.c           |  44 +++++
>   tests/test-cutils.c                | 204 +++++++++++++++++++++
>   tests/test-keyval.c                | 122 +++++++++++++
>   tests/test-qobject-input-visitor.c |  29 +++
>   util/cutils.c                      |  89 +++++----
>   23 files changed, 1555 insertions(+), 44 deletions(-)
>   create mode 100644 hw/acpi/hmat.c
>   create mode 100644 hw/acpi/hmat.h
> 



^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v14 01/11] util/cutils: Add qemu_strtotime_ns()
  2019-10-28  7:52 ` [PATCH v14 01/11] util/cutils: Add qemu_strtotime_ns() Tao Xu
@ 2019-11-06 19:56   ` Eduardo Habkost
  2019-11-07  1:38     ` Tao Xu
  0 siblings, 1 reply; 32+ messages in thread
From: Eduardo Habkost @ 2019-11-06 19:56 UTC (permalink / raw)
  To: Tao Xu
  Cc: lvivier, thuth, mst, qemu-devel, jingqi.liu, fan.du, mdroth,
	armbru, jonathan.cameron, imammedo

On Mon, Oct 28, 2019 at 03:52:10PM +0800, Tao Xu wrote:
> To convert strings with time suffixes to numbers, support time unit are
> "ns" for nanosecond, "us" for microsecond, "ms" for millisecond or "s"
> for second. Add test for qemu_strtotime_ns, test the input of basic,
> time suffixes, float, invaild, trailing and overflow.
> 
> Signed-off-by: Tao Xu <tao3.xu@intel.com>
> ---
> 
> Changes in v14:
>     - Reuse the codes of do_strtosz to build qemu_strtotime_ns
>       (Eduardo)
>     - Squash patch v13 01/12 and 02/12 together (Daniel and Eduardo)
>     - Drop time unit picosecond (Eric)

Suggestion for the next version: if you are refactoring existing
do_strtosz() code, please refactor it in one patch, and add new
functionality in another patch.

> ---
>  include/qemu/cutils.h |   1 +
>  tests/test-cutils.c   | 204 ++++++++++++++++++++++++++++++++++++++++++
>  util/cutils.c         |  89 +++++++++++-------
>  3 files changed, 262 insertions(+), 32 deletions(-)
> 
> diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
> index b54c847e0f..ff2b3f4614 100644
> --- a/include/qemu/cutils.h
> +++ b/include/qemu/cutils.h
> @@ -182,5 +182,6 @@ int uleb128_decode_small(const uint8_t *in, uint32_t *n);
>   * *str1 is <, == or > than *str2.
>   */
>  int qemu_pstrcmp0(const char **str1, const char **str2);
> +int qemu_strtotime_ns(const char *nptr, const char **end, uint64_t *result);
>  
>  #endif
> diff --git a/tests/test-cutils.c b/tests/test-cutils.c
> index 1aa8351520..d6a0824efd 100644
> --- a/tests/test-cutils.c
> +++ b/tests/test-cutils.c
[...]
> +static void test_qemu_strtotime_ns_trailing(void)
> +{
> +    const char *str;
> +    const char *endptr;
> +    int err;
> +    uint64_t res = 0xbaadf00d;
> +
> +    str = "123xxx";
> +
> +    err = qemu_strtotime_ns(str, NULL, &res);
> +    g_assert_cmpint(err, ==, -EINVAL);
> +
> +    str = "1msxxx";
> +    err = qemu_strtotime_ns(str, &endptr, &res);
> +    g_assert_cmpint(err, ==, 0);
> +    g_assert_cmpint(res, ==, 1000000);
> +    g_assert(endptr == str + 3);
> +
> +    err = qemu_strtotime_ns(str, NULL, &res);
> +    g_assert_cmpint(err, ==, -EINVAL);
> +}

This is better than the test case in v13, where trailing strings
were not handled consistently.  Good.

[...]
> diff --git a/util/cutils.c b/util/cutils.c
> index fd591cadf0..d83825f8b4 100644
> --- a/util/cutils.c
> +++ b/util/cutils.c
> @@ -181,41 +181,38 @@ int fcntl_setfl(int fd, int flag)
>  }
>  #endif
>  
> -static int64_t suffix_mul(char suffix, int64_t unit)
> -{
> -    switch (qemu_toupper(suffix)) {
> -    case 'B':
> -        return 1;
> -    case 'K':
> -        return unit;
> -    case 'M':
> -        return unit * unit;
> -    case 'G':
> -        return unit * unit * unit;
> -    case 'T':
> -        return unit * unit * unit * unit;
> -    case 'P':
> -        return unit * unit * unit * unit * unit;
> -    case 'E':
> -        return unit * unit * unit * unit * unit * unit;
> +static int64_t suffix_mul(const char *suffixes[], int num_suffix,
> +                          const char *endptr, int *offset, int64_t unit)
> +{
> +    int i, suffix_len;
> +    int64_t mul = 1;
> +
> +    for (i = 0; i < num_suffix; i++) {
> +        suffix_len = strlen(suffixes[i]);
> +        if (strlen(endptr) >= suffix_len &&

Is the strlen(endptr) check here really necessary?


> +            g_ascii_strncasecmp(suffixes[i], endptr, suffix_len) == 0) {
> +            *offset = suffix_len;
> +            return mul;
> +        }
> +        mul *= unit;
>      }
> +
>      return -1;
>  }
>  
>  /*
> - * Convert string to bytes, allowing either B/b for bytes, K/k for KB,
> - * M/m for MB, G/g for GB or T/t for TB. End pointer will be returned
> - * in *end, if not NULL. Return -ERANGE on overflow, and -EINVAL on
> - * other error.
> + * Convert string according to different suffixes. End pointer will be returned
> + * in *end, if not NULL. Return -ERANGE on overflow, and -EINVAL on other error.
>   */
> -static int do_strtosz(const char *nptr, const char **end,
> -                      const char default_suffix, int64_t unit,
> -                      uint64_t *result)
> +static int do_strtomul(const char *nptr, const char **end,
> +                       const char *suffixes[], int num_suffix,
> +                       const char *default_suffix, int64_t unit,
> +                       uint64_t *result)
>  {
>      int retval;
>      const char *endptr;
> -    unsigned char c;
>      int mul_required = 0;
> +    int offset = 0;
>      double val, mul, integral, fraction;
>  
>      retval = qemu_strtod_finite(nptr, &endptr, &val);
> @@ -226,12 +223,12 @@ static int do_strtosz(const char *nptr, const char **end,
>      if (fraction != 0) {
>          mul_required = 1;
>      }
> -    c = *endptr;
> -    mul = suffix_mul(c, unit);
> +
> +    mul = suffix_mul(suffixes, num_suffix, endptr, &offset, unit);
>      if (mul >= 0) {
> -        endptr++;
> +        endptr += offset;
>      } else {
> -        mul = suffix_mul(default_suffix, unit);
> +        mul = suffix_mul(suffixes, num_suffix, default_suffix, &offset, unit);
>          assert(mul >= 0);
>      }
>      if (mul == 1 && mul_required) {
> @@ -259,19 +256,47 @@ out:
>      return retval;
>  }
>  
> +/*
> + * Convert string to bytes, allowing either B/b for bytes, K/k for KB,
> + * M/m for MB, G/g for GB or T/t for TB. End pointer will be returned
> + * in *end, if not NULL. Return -ERANGE on overflow, and -EINVAL on
> + * other error.
> + */
> +static int do_strtosz(const char *nptr, const char **end,
> +                      const char *default_suffix, int64_t unit,
> +                      uint64_t *result)
> +{
> +    static const char *suffixes[] = { "B", "K", "M", "G", "T", "P", "E" };
> +
> +    return do_strtomul(nptr, end, suffixes, 7, default_suffix, unit, result);

[1] You can use ARRAY_SIZE(suffixes) instead of hardcoding the
array size.

> +}
> +
>  int qemu_strtosz(const char *nptr, const char **end, uint64_t *result)
>  {
> -    return do_strtosz(nptr, end, 'B', 1024, result);
> +    return do_strtosz(nptr, end, "B", 1024, result);
>  }
>  
>  int qemu_strtosz_MiB(const char *nptr, const char **end, uint64_t *result)
>  {
> -    return do_strtosz(nptr, end, 'M', 1024, result);
> +    return do_strtosz(nptr, end, "M", 1024, result);
>  }
>  
>  int qemu_strtosz_metric(const char *nptr, const char **end, uint64_t *result)
>  {
> -    return do_strtosz(nptr, end, 'B', 1000, result);
> +    return do_strtosz(nptr, end, "B", 1000, result);
> +}
> +
> +/*
> + * Convert string to time, support time unit are ns for nanosecond, us for
> + * microsecond, ms for millisecond and s for second. End pointer will be
> + * returned in *end, if not NULL. Return -ERANGE on overflow, and -EINVAL on
> + * other error.
> + */
> +int qemu_strtotime_ns(const char *nptr, const char **end, uint64_t *result)
> +{
> +    static const char *suffixes[] = { "ns", "us", "ms", "s" };
> +
> +    return do_strtomul(nptr, end, suffixes, 4, "ns", 1000, result);

Same as above[1].

>  }
>  
>  /**
> -- 
> 2.20.1
> 

-- 
Eduardo



^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v14 04/11] numa: Extend CLI to provide initiator information for numa nodes
  2019-10-28  7:52 ` [PATCH v14 04/11] numa: Extend CLI to provide initiator information for numa nodes Tao Xu
@ 2019-11-06 20:29   ` Eric Blake
  2019-11-07  1:51     ` Tao Xu
  0 siblings, 1 reply; 32+ messages in thread
From: Eric Blake @ 2019-11-06 20:29 UTC (permalink / raw)
  To: Tao Xu, mst, imammedo, ehabkost, marcel.apfelbaum, armbru,
	mdroth, thuth, lvivier
  Cc: jingqi.liu, Dan Williams, fan.du, qemu-devel, jonathan.cameron

On 10/28/19 2:52 AM, Tao Xu wrote:
> In ACPI 6.3 chapter 5.2.27 Heterogeneous Memory Attribute Table (HMAT),
> The initiator represents processor which access to memory. And in 5.2.27.3
> Memory Proximity Domain Attributes Structure, the attached initiator is
> defined as where the memory controller responsible for a memory proximity
> domain. With attached initiator information, the topology of heterogeneous
> memory can be described.
> 
> Extend CLI of "-numa node" option to indicate the initiator numa node-id.
> In the linux kernel, the codes in drivers/acpi/hmat/hmat.c parse and report
> the platform's HMAT tables.
> 
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> Reviewed-by: Jingqi Liu <jingqi.liu@intel.com>
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Tao Xu <tao3.xu@intel.com>
> ---
> 

> +++ b/qapi/machine.json
> @@ -463,6 +463,13 @@
>   # @memdev: memory backend object.  If specified for one node,
>   #          it must be specified for all nodes.
>   #
> +# @initiator: defined in ACPI 6.3 Chapter 5.2.27.3 Table 5-145,
> +#             points to the nodeid which has the memory controller
> +#             responsible for this NUMA node. This field provides
> +#             additional information as to the initiator node that
> +#             is closest (as in directly attached) to this node, and
> +#             therefore has the best performance (since 4.2)
> +#

I'm sad to say, but we've now missed soft freeze for 4.2.  This feels 
like enough of a feature that we'll probably have to defer the series to 
the 5.0 release, which will have ripple effects to your patches.

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



^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v14 03/11] tests: Add test for QAPI builtin type time
  2019-10-28  7:52 ` [PATCH v14 03/11] tests: Add test for QAPI " Tao Xu
@ 2019-11-06 20:53   ` Eduardo Habkost
  2019-11-07  6:24     ` Tao Xu
  0 siblings, 1 reply; 32+ messages in thread
From: Eduardo Habkost @ 2019-11-06 20:53 UTC (permalink / raw)
  To: Tao Xu
  Cc: lvivier, thuth, mst, qemu-devel, jingqi.liu, fan.du, mdroth,
	armbru, jonathan.cameron, imammedo

On Mon, Oct 28, 2019 at 03:52:12PM +0800, Tao Xu wrote:
> Add tests for time input such as zero, around limit of precision,
> signed upper limit, actual upper limit, beyond limits, time suffixes,
> and etc.
> 
> Signed-off-by: Tao Xu <tao3.xu@intel.com>
> ---
[...]
> +    /* Close to signed upper limit 0x7ffffffffffffc00 (53 msbs set) */
> +    qdict = keyval_parse("time1=9223372036854774784," /* 7ffffffffffffc00 */
> +                         "time2=9223372036854775295", /* 7ffffffffffffdff */
> +                         NULL, &error_abort);
> +    v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
> +    qobject_unref(qdict);
> +    visit_start_struct(v, NULL, NULL, 0, &error_abort);
> +    visit_type_time(v, "time1", &time, &error_abort);
> +    g_assert_cmphex(time, ==, 0x7ffffffffffffc00);
> +    visit_type_time(v, "time2", &time, &error_abort);
> +    g_assert_cmphex(time, ==, 0x7ffffffffffffc00);

I'm confused by this test case and the one below[1].  Are these
known bugs?  Shouldn't we document them as known bugs?

> +    visit_check_struct(v, &error_abort);
> +    visit_end_struct(v, NULL);
> +    visit_free(v);
> +
> +    /* Close to actual upper limit 0xfffffffffffff800 (53 msbs set) */
> +    qdict = keyval_parse("time1=18446744073709549568," /* fffffffffffff800 */
> +                         "time2=18446744073709550591", /* fffffffffffffbff */
> +                         NULL, &error_abort);
> +    v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
> +    qobject_unref(qdict);
> +    visit_start_struct(v, NULL, NULL, 0, &error_abort);
> +    visit_type_time(v, "time1", &time, &error_abort);
> +    g_assert_cmphex(time, ==, 0xfffffffffffff800);
> +    visit_type_time(v, "time2", &time, &error_abort);
> +    g_assert_cmphex(time, ==, 0xfffffffffffff800);

[1]

> +    visit_check_struct(v, &error_abort);
> +    visit_end_struct(v, NULL);
> +    visit_free(v);
[...]

-- 
Eduardo



^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v14 01/11] util/cutils: Add qemu_strtotime_ns()
  2019-11-06 19:56   ` Eduardo Habkost
@ 2019-11-07  1:38     ` Tao Xu
  0 siblings, 0 replies; 32+ messages in thread
From: Tao Xu @ 2019-11-07  1:38 UTC (permalink / raw)
  To: Eduardo Habkost
  Cc: lvivier, thuth, mst, qemu-devel, Liu, Jingqi, Du, Fan, mdroth,
	armbru, jonathan.cameron, imammedo

On 11/7/2019 3:56 AM, Eduardo Habkost wrote:
> On Mon, Oct 28, 2019 at 03:52:10PM +0800, Tao Xu wrote:
>> To convert strings with time suffixes to numbers, support time unit are
>> "ns" for nanosecond, "us" for microsecond, "ms" for millisecond or "s"
>> for second. Add test for qemu_strtotime_ns, test the input of basic,
>> time suffixes, float, invaild, trailing and overflow.
>>
>> Signed-off-by: Tao Xu <tao3.xu@intel.com>
>> ---
>>
>> Changes in v14:
>>      - Reuse the codes of do_strtosz to build qemu_strtotime_ns
>>        (Eduardo)
>>      - Squash patch v13 01/12 and 02/12 together (Daniel and Eduardo)
>>      - Drop time unit picosecond (Eric)
> 
> Suggestion for the next version: if you are refactoring existing
> do_strtosz() code, please refactor it in one patch, and add new
> functionality in another patch.
> 
Thank you for your suggestions and comments blew. I will improve in next 
version.

>> ---
>>   include/qemu/cutils.h |   1 +
>>   tests/test-cutils.c   | 204 ++++++++++++++++++++++++++++++++++++++++++
>>   util/cutils.c         |  89 +++++++++++-------
>>   3 files changed, 262 insertions(+), 32 deletions(-)
>>
>> diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
>> index b54c847e0f..ff2b3f4614 100644
>> --- a/include/qemu/cutils.h
>> +++ b/include/qemu/cutils.h
>> @@ -182,5 +182,6 @@ int uleb128_decode_small(const uint8_t *in, uint32_t *n);
>>    * *str1 is <, == or > than *str2.
>>    */
>>   int qemu_pstrcmp0(const char **str1, const char **str2);
>> +int qemu_strtotime_ns(const char *nptr, const char **end, uint64_t *result);
>>   
>>   #endif
>> diff --git a/tests/test-cutils.c b/tests/test-cutils.c
>> index 1aa8351520..d6a0824efd 100644
>> --- a/tests/test-cutils.c
>> +++ b/tests/test-cutils.c
> [...]
>> +static void test_qemu_strtotime_ns_trailing(void)
>> +{
>> +    const char *str;
>> +    const char *endptr;
>> +    int err;
>> +    uint64_t res = 0xbaadf00d;
>> +
>> +    str = "123xxx";
>> +
>> +    err = qemu_strtotime_ns(str, NULL, &res);
>> +    g_assert_cmpint(err, ==, -EINVAL);
>> +
>> +    str = "1msxxx";
>> +    err = qemu_strtotime_ns(str, &endptr, &res);
>> +    g_assert_cmpint(err, ==, 0);
>> +    g_assert_cmpint(res, ==, 1000000);
>> +    g_assert(endptr == str + 3);
>> +
>> +    err = qemu_strtotime_ns(str, NULL, &res);
>> +    g_assert_cmpint(err, ==, -EINVAL);
>> +}
> 
> This is better than the test case in v13, where trailing strings
> were not handled consistently.  Good.
> 
> [...]
>> diff --git a/util/cutils.c b/util/cutils.c
>> index fd591cadf0..d83825f8b4 100644
>> --- a/util/cutils.c
>> +++ b/util/cutils.c
>> @@ -181,41 +181,38 @@ int fcntl_setfl(int fd, int flag)
>>   }
>>   #endif
>>   
>> -static int64_t suffix_mul(char suffix, int64_t unit)
>> -{
>> -    switch (qemu_toupper(suffix)) {
>> -    case 'B':
>> -        return 1;
>> -    case 'K':
>> -        return unit;
>> -    case 'M':
>> -        return unit * unit;
>> -    case 'G':
>> -        return unit * unit * unit;
>> -    case 'T':
>> -        return unit * unit * unit * unit;
>> -    case 'P':
>> -        return unit * unit * unit * unit * unit;
>> -    case 'E':
>> -        return unit * unit * unit * unit * unit * unit;
>> +static int64_t suffix_mul(const char *suffixes[], int num_suffix,
>> +                          const char *endptr, int *offset, int64_t unit)
>> +{
>> +    int i, suffix_len;
>> +    int64_t mul = 1;
>> +
>> +    for (i = 0; i < num_suffix; i++) {
>> +        suffix_len = strlen(suffixes[i]);
>> +        if (strlen(endptr) >= suffix_len &&
> 
> Is the strlen(endptr) check here really necessary?
> 
> 
>> +            g_ascii_strncasecmp(suffixes[i], endptr, suffix_len) == 0) {
>> +            *offset = suffix_len;
>> +            return mul;
>> +        }
>> +        mul *= unit;
>>       }
>> +
>>       return -1;
>>   }
>>   
>>   /*
>> - * Convert string to bytes, allowing either B/b for bytes, K/k for KB,
>> - * M/m for MB, G/g for GB or T/t for TB. End pointer will be returned
>> - * in *end, if not NULL. Return -ERANGE on overflow, and -EINVAL on
>> - * other error.
>> + * Convert string according to different suffixes. End pointer will be returned
>> + * in *end, if not NULL. Return -ERANGE on overflow, and -EINVAL on other error.
>>    */
>> -static int do_strtosz(const char *nptr, const char **end,
>> -                      const char default_suffix, int64_t unit,
>> -                      uint64_t *result)
>> +static int do_strtomul(const char *nptr, const char **end,
>> +                       const char *suffixes[], int num_suffix,
>> +                       const char *default_suffix, int64_t unit,
>> +                       uint64_t *result)
>>   {
>>       int retval;
>>       const char *endptr;
>> -    unsigned char c;
>>       int mul_required = 0;
>> +    int offset = 0;
>>       double val, mul, integral, fraction;
>>   
>>       retval = qemu_strtod_finite(nptr, &endptr, &val);
>> @@ -226,12 +223,12 @@ static int do_strtosz(const char *nptr, const char **end,
>>       if (fraction != 0) {
>>           mul_required = 1;
>>       }
>> -    c = *endptr;
>> -    mul = suffix_mul(c, unit);
>> +
>> +    mul = suffix_mul(suffixes, num_suffix, endptr, &offset, unit);
>>       if (mul >= 0) {
>> -        endptr++;
>> +        endptr += offset;
>>       } else {
>> -        mul = suffix_mul(default_suffix, unit);
>> +        mul = suffix_mul(suffixes, num_suffix, default_suffix, &offset, unit);
>>           assert(mul >= 0);
>>       }
>>       if (mul == 1 && mul_required) {
>> @@ -259,19 +256,47 @@ out:
>>       return retval;
>>   }
>>   
>> +/*
>> + * Convert string to bytes, allowing either B/b for bytes, K/k for KB,
>> + * M/m for MB, G/g for GB or T/t for TB. End pointer will be returned
>> + * in *end, if not NULL. Return -ERANGE on overflow, and -EINVAL on
>> + * other error.
>> + */
>> +static int do_strtosz(const char *nptr, const char **end,
>> +                      const char *default_suffix, int64_t unit,
>> +                      uint64_t *result)
>> +{
>> +    static const char *suffixes[] = { "B", "K", "M", "G", "T", "P", "E" };
>> +
>> +    return do_strtomul(nptr, end, suffixes, 7, default_suffix, unit, result);
> 
> [1] You can use ARRAY_SIZE(suffixes) instead of hardcoding the
> array size.
> 
>> +}
>> +
>>   int qemu_strtosz(const char *nptr, const char **end, uint64_t *result)
>>   {
>> -    return do_strtosz(nptr, end, 'B', 1024, result);
>> +    return do_strtosz(nptr, end, "B", 1024, result);
>>   }
>>   
>>   int qemu_strtosz_MiB(const char *nptr, const char **end, uint64_t *result)
>>   {
>> -    return do_strtosz(nptr, end, 'M', 1024, result);
>> +    return do_strtosz(nptr, end, "M", 1024, result);
>>   }
>>   
>>   int qemu_strtosz_metric(const char *nptr, const char **end, uint64_t *result)
>>   {
>> -    return do_strtosz(nptr, end, 'B', 1000, result);
>> +    return do_strtosz(nptr, end, "B", 1000, result);
>> +}
>> +
>> +/*
>> + * Convert string to time, support time unit are ns for nanosecond, us for
>> + * microsecond, ms for millisecond and s for second. End pointer will be
>> + * returned in *end, if not NULL. Return -ERANGE on overflow, and -EINVAL on
>> + * other error.
>> + */
>> +int qemu_strtotime_ns(const char *nptr, const char **end, uint64_t *result)
>> +{
>> +    static const char *suffixes[] = { "ns", "us", "ms", "s" };
>> +
>> +    return do_strtomul(nptr, end, suffixes, 4, "ns", 1000, result);
> 
> Same as above[1].
> 
>>   }
>>   
>>   /**
>> -- 
>> 2.20.1
>>
> 



^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v14 04/11] numa: Extend CLI to provide initiator information for numa nodes
  2019-11-06 20:29   ` Eric Blake
@ 2019-11-07  1:51     ` Tao Xu
  0 siblings, 0 replies; 32+ messages in thread
From: Tao Xu @ 2019-11-07  1:51 UTC (permalink / raw)
  To: Eric Blake, mst, imammedo, ehabkost, marcel.apfelbaum, armbru,
	mdroth, thuth, lvivier
  Cc: Liu, Jingqi, Williams, Dan J, Du, Fan, qemu-devel, jonathan.cameron

On 11/7/2019 4:29 AM, Eric Blake wrote:
> On 10/28/19 2:52 AM, Tao Xu wrote:
>> In ACPI 6.3 chapter 5.2.27 Heterogeneous Memory Attribute Table (HMAT),
>> The initiator represents processor which access to memory. And in 5.2.27.3
>> Memory Proximity Domain Attributes Structure, the attached initiator is
>> defined as where the memory controller responsible for a memory proximity
>> domain. With attached initiator information, the topology of heterogeneous
>> memory can be described.
>>
>> Extend CLI of "-numa node" option to indicate the initiator numa node-id.
>> In the linux kernel, the codes in drivers/acpi/hmat/hmat.c parse and report
>> the platform's HMAT tables.
>>
>> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
>> Reviewed-by: Jingqi Liu <jingqi.liu@intel.com>
>> Suggested-by: Dan Williams <dan.j.williams@intel.com>
>> Signed-off-by: Tao Xu <tao3.xu@intel.com>
>> ---
>>
> 
>> +++ b/qapi/machine.json
>> @@ -463,6 +463,13 @@
>>    # @memdev: memory backend object.  If specified for one node,
>>    #          it must be specified for all nodes.
>>    #
>> +# @initiator: defined in ACPI 6.3 Chapter 5.2.27.3 Table 5-145,
>> +#             points to the nodeid which has the memory controller
>> +#             responsible for this NUMA node. This field provides
>> +#             additional information as to the initiator node that
>> +#             is closest (as in directly attached) to this node, and
>> +#             therefore has the best performance (since 4.2)
>> +#
> 
> I'm sad to say, but we've now missed soft freeze for 4.2.  This feels
> like enough of a feature that we'll probably have to defer the series to
> the 5.0 release, which will have ripple effects to your patches.
> 
OK, I will change the version flag.


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v14 03/11] tests: Add test for QAPI builtin type time
  2019-11-06 20:53   ` Eduardo Habkost
@ 2019-11-07  6:24     ` Tao Xu
  2019-11-07 13:31       ` Eduardo Habkost
  0 siblings, 1 reply; 32+ messages in thread
From: Tao Xu @ 2019-11-07  6:24 UTC (permalink / raw)
  To: Eduardo Habkost
  Cc: lvivier, thuth, mst, qemu-devel, Liu, Jingqi, Du, Fan, mdroth,
	armbru, jonathan.cameron, imammedo

On 11/7/2019 4:53 AM, Eduardo Habkost wrote:
> On Mon, Oct 28, 2019 at 03:52:12PM +0800, Tao Xu wrote:
>> Add tests for time input such as zero, around limit of precision,
>> signed upper limit, actual upper limit, beyond limits, time suffixes,
>> and etc.
>>
>> Signed-off-by: Tao Xu <tao3.xu@intel.com>
>> ---
> [...]
>> +    /* Close to signed upper limit 0x7ffffffffffffc00 (53 msbs set) */
>> +    qdict = keyval_parse("time1=9223372036854774784," /* 7ffffffffffffc00 */
>> +                         "time2=9223372036854775295", /* 7ffffffffffffdff */
>> +                         NULL, &error_abort);
>> +    v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
>> +    qobject_unref(qdict);
>> +    visit_start_struct(v, NULL, NULL, 0, &error_abort);
>> +    visit_type_time(v, "time1", &time, &error_abort);
>> +    g_assert_cmphex(time, ==, 0x7ffffffffffffc00);
>> +    visit_type_time(v, "time2", &time, &error_abort);
>> +    g_assert_cmphex(time, ==, 0x7ffffffffffffc00);
> 
> I'm confused by this test case and the one below[1].  Are these
> known bugs?  Shouldn't we document them as known bugs?

Because do_strtosz() or do_strtomul() actually parse with strtod(), so 
the precision is 53 bits, so in these cases, 7ffffffffffffdff and 
fffffffffffffbff are rounded.
> 
>> +    visit_check_struct(v, &error_abort);
>> +    visit_end_struct(v, NULL);
>> +    visit_free(v);
>> +
>> +    /* Close to actual upper limit 0xfffffffffffff800 (53 msbs set) */
>> +    qdict = keyval_parse("time1=18446744073709549568," /* fffffffffffff800 */
>> +                         "time2=18446744073709550591", /* fffffffffffffbff */
>> +                         NULL, &error_abort);
>> +    v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
>> +    qobject_unref(qdict);
>> +    visit_start_struct(v, NULL, NULL, 0, &error_abort);
>> +    visit_type_time(v, "time1", &time, &error_abort);
>> +    g_assert_cmphex(time, ==, 0xfffffffffffff800);
>> +    visit_type_time(v, "time2", &time, &error_abort);
>> +    g_assert_cmphex(time, ==, 0xfffffffffffff800);
> 
> [1]
> 
>> +    visit_check_struct(v, &error_abort);
>> +    visit_end_struct(v, NULL);
>> +    visit_free(v);
> [...]
> 



^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v14 03/11] tests: Add test for QAPI builtin type time
  2019-11-07  6:24     ` Tao Xu
@ 2019-11-07 13:31       ` Eduardo Habkost
  2019-11-08  5:25         ` Tao Xu
  0 siblings, 1 reply; 32+ messages in thread
From: Eduardo Habkost @ 2019-11-07 13:31 UTC (permalink / raw)
  To: Tao Xu
  Cc: lvivier, thuth, mst, qemu-devel, Liu, Jingqi, Du, Fan, mdroth,
	armbru, jonathan.cameron, imammedo

On Thu, Nov 07, 2019 at 02:24:52PM +0800, Tao Xu wrote:
> On 11/7/2019 4:53 AM, Eduardo Habkost wrote:
> > On Mon, Oct 28, 2019 at 03:52:12PM +0800, Tao Xu wrote:
> > > Add tests for time input such as zero, around limit of precision,
> > > signed upper limit, actual upper limit, beyond limits, time suffixes,
> > > and etc.
> > > 
> > > Signed-off-by: Tao Xu <tao3.xu@intel.com>
> > > ---
> > [...]
> > > +    /* Close to signed upper limit 0x7ffffffffffffc00 (53 msbs set) */
> > > +    qdict = keyval_parse("time1=9223372036854774784," /* 7ffffffffffffc00 */
> > > +                         "time2=9223372036854775295", /* 7ffffffffffffdff */
> > > +                         NULL, &error_abort);
> > > +    v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
> > > +    qobject_unref(qdict);
> > > +    visit_start_struct(v, NULL, NULL, 0, &error_abort);
> > > +    visit_type_time(v, "time1", &time, &error_abort);
> > > +    g_assert_cmphex(time, ==, 0x7ffffffffffffc00);
> > > +    visit_type_time(v, "time2", &time, &error_abort);
> > > +    g_assert_cmphex(time, ==, 0x7ffffffffffffc00);
> > 
> > I'm confused by this test case and the one below[1].  Are these
> > known bugs?  Shouldn't we document them as known bugs?
> 
> Because do_strtosz() or do_strtomul() actually parse with strtod(), so the
> precision is 53 bits, so in these cases, 7ffffffffffffdff and
> fffffffffffffbff are rounded.

My questions remain: why isn't this being treated like a bug?

-- 
Eduardo



^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v14 03/11] tests: Add test for QAPI builtin type time
  2019-11-07 13:31       ` Eduardo Habkost
@ 2019-11-08  5:25         ` Tao Xu
  2019-11-08  8:05           ` Markus Armbruster
  0 siblings, 1 reply; 32+ messages in thread
From: Tao Xu @ 2019-11-08  5:25 UTC (permalink / raw)
  To: Eduardo Habkost, armbru
  Cc: lvivier, thuth, mst, Liu, Jingqi, Du, Fan, mdroth, qemu-devel,
	jonathan.cameron, imammedo

On 11/7/2019 9:31 PM, Eduardo Habkost wrote:
> On Thu, Nov 07, 2019 at 02:24:52PM +0800, Tao Xu wrote:
>> On 11/7/2019 4:53 AM, Eduardo Habkost wrote:
>>> On Mon, Oct 28, 2019 at 03:52:12PM +0800, Tao Xu wrote:
>>>> Add tests for time input such as zero, around limit of precision,
>>>> signed upper limit, actual upper limit, beyond limits, time suffixes,
>>>> and etc.
>>>>
>>>> Signed-off-by: Tao Xu <tao3.xu@intel.com>
>>>> ---
>>> [...]
>>>> +    /* Close to signed upper limit 0x7ffffffffffffc00 (53 msbs set) */
>>>> +    qdict = keyval_parse("time1=9223372036854774784," /* 7ffffffffffffc00 */
>>>> +                         "time2=9223372036854775295", /* 7ffffffffffffdff */
>>>> +                         NULL, &error_abort);
>>>> +    v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
>>>> +    qobject_unref(qdict);
>>>> +    visit_start_struct(v, NULL, NULL, 0, &error_abort);
>>>> +    visit_type_time(v, "time1", &time, &error_abort);
>>>> +    g_assert_cmphex(time, ==, 0x7ffffffffffffc00);
>>>> +    visit_type_time(v, "time2", &time, &error_abort);
>>>> +    g_assert_cmphex(time, ==, 0x7ffffffffffffc00);
>>>
>>> I'm confused by this test case and the one below[1].  Are these
>>> known bugs?  Shouldn't we document them as known bugs?
>>
>> Because do_strtosz() or do_strtomul() actually parse with strtod(), so the
>> precision is 53 bits, so in these cases, 7ffffffffffffdff and
>> fffffffffffffbff are rounded.
> 
> My questions remain: why isn't this being treated like a bug?
> 
Hi Markus,

I am confused about the code here too. Because in do_strtosz(), the 
upper limit is

val * mul >= 0xfffffffffffffc00

So some data near 53 bit may be rounded. Is there a bug?


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v14 03/11] tests: Add test for QAPI builtin type time
  2019-11-08  5:25         ` Tao Xu
@ 2019-11-08  8:05           ` Markus Armbruster
  2019-11-08  8:41             ` Igor Mammedov
  2019-11-12 20:15             ` Eduardo Habkost
  0 siblings, 2 replies; 32+ messages in thread
From: Markus Armbruster @ 2019-11-08  8:05 UTC (permalink / raw)
  To: Tao Xu
  Cc: lvivier, thuth, Eduardo Habkost, mst, Liu, Jingqi, Du, Fan,
	mdroth, qemu-devel, jonathan.cameron, imammedo

Tao Xu <tao3.xu@intel.com> writes:

> On 11/7/2019 9:31 PM, Eduardo Habkost wrote:
>> On Thu, Nov 07, 2019 at 02:24:52PM +0800, Tao Xu wrote:
>>> On 11/7/2019 4:53 AM, Eduardo Habkost wrote:
>>>> On Mon, Oct 28, 2019 at 03:52:12PM +0800, Tao Xu wrote:
>>>>> Add tests for time input such as zero, around limit of precision,
>>>>> signed upper limit, actual upper limit, beyond limits, time suffixes,
>>>>> and etc.
>>>>>
>>>>> Signed-off-by: Tao Xu <tao3.xu@intel.com>
>>>>> ---
>>>> [...]
>>>>> +    /* Close to signed upper limit 0x7ffffffffffffc00 (53 msbs set) */
>>>>> +    qdict = keyval_parse("time1=9223372036854774784," /* 7ffffffffffffc00 */
>>>>> +                         "time2=9223372036854775295", /* 7ffffffffffffdff */
>>>>> +                         NULL, &error_abort);
>>>>> +    v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
>>>>> +    qobject_unref(qdict);
>>>>> +    visit_start_struct(v, NULL, NULL, 0, &error_abort);
>>>>> +    visit_type_time(v, "time1", &time, &error_abort);
>>>>> +    g_assert_cmphex(time, ==, 0x7ffffffffffffc00);
>>>>> +    visit_type_time(v, "time2", &time, &error_abort);
>>>>> +    g_assert_cmphex(time, ==, 0x7ffffffffffffc00);
>>>>
>>>> I'm confused by this test case and the one below[1].  Are these
>>>> known bugs?  Shouldn't we document them as known bugs?
>>>
>>> Because do_strtosz() or do_strtomul() actually parse with strtod(), so the
>>> precision is 53 bits, so in these cases, 7ffffffffffffdff and
>>> fffffffffffffbff are rounded.
>>
>> My questions remain: why isn't this being treated like a bug?
>>
> Hi Markus,
>
> I am confused about the code here too. Because in do_strtosz(), the
> upper limit is
>
> val * mul >= 0xfffffffffffffc00
>
> So some data near 53 bit may be rounded. Is there a bug?

No, but the design is surprising, and the functions lack written
contracts, except for the do_strtosz() helper, which has one that sucks.

qemu_strtosz() & friends are designed to accept fraction * unit
multiplier.  Example: 1.5M means 1.5 * 1024 * 1024 with qemu_strtosz()
and qemu_strtosz_MiB(), and 1.5 * 1000 * 1000 with
qemu_strtosz_metric().  Whether supporting fractions is a good idea is
debatable, but it's what we've got.

The implementation limits the numeric part to the precision of double,
i.e. 53 bits.  "8PiB should be enough for anybody."

Switching it from double to long double raises the limit to the
precision of long double.  At least 64 bit on common hosts, but hosts
exist where it's the same 53 bits.  Do we support any such hosts?  If
yes, then we'd make the precision depend on the host, which feels like a
bad idea.

A possible alternative is to parse the numeric part both as a double and
as a 64 bit unsigned integer, then use whatever consumes more
characters.  This enables providing full 64 bits unless you actually use
a fraction.

As far as I remember, the only problem we've ever had with the 53 bits
limit is developer confusion :)

Patches welcome.



^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v14 03/11] tests: Add test for QAPI builtin type time
  2019-11-08  8:05           ` Markus Armbruster
@ 2019-11-08  8:41             ` Igor Mammedov
  2019-11-11  3:12               ` Tao Xu
  2019-11-12 20:15             ` Eduardo Habkost
  1 sibling, 1 reply; 32+ messages in thread
From: Igor Mammedov @ 2019-11-08  8:41 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: lvivier, thuth, Eduardo Habkost, mst, Liu, Jingqi, Tao Xu, Du,
	Fan, mdroth, qemu-devel, jonathan.cameron

On Fri, 08 Nov 2019 09:05:52 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Tao Xu <tao3.xu@intel.com> writes:
> 
> > On 11/7/2019 9:31 PM, Eduardo Habkost wrote:  
> >> On Thu, Nov 07, 2019 at 02:24:52PM +0800, Tao Xu wrote:  
> >>> On 11/7/2019 4:53 AM, Eduardo Habkost wrote:  
> >>>> On Mon, Oct 28, 2019 at 03:52:12PM +0800, Tao Xu wrote:  
> >>>>> Add tests for time input such as zero, around limit of precision,
> >>>>> signed upper limit, actual upper limit, beyond limits, time suffixes,
> >>>>> and etc.
> >>>>>
> >>>>> Signed-off-by: Tao Xu <tao3.xu@intel.com>
> >>>>> ---  
> >>>> [...]  
> >>>>> +    /* Close to signed upper limit 0x7ffffffffffffc00 (53 msbs set) */
> >>>>> +    qdict = keyval_parse("time1=9223372036854774784," /* 7ffffffffffffc00 */
> >>>>> +                         "time2=9223372036854775295", /* 7ffffffffffffdff */
> >>>>> +                         NULL, &error_abort);
> >>>>> +    v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
> >>>>> +    qobject_unref(qdict);
> >>>>> +    visit_start_struct(v, NULL, NULL, 0, &error_abort);
> >>>>> +    visit_type_time(v, "time1", &time, &error_abort);
> >>>>> +    g_assert_cmphex(time, ==, 0x7ffffffffffffc00);
> >>>>> +    visit_type_time(v, "time2", &time, &error_abort);
> >>>>> +    g_assert_cmphex(time, ==, 0x7ffffffffffffc00);  
> >>>>
> >>>> I'm confused by this test case and the one below[1].  Are these
> >>>> known bugs?  Shouldn't we document them as known bugs?  
> >>>
> >>> Because do_strtosz() or do_strtomul() actually parse with strtod(), so the
> >>> precision is 53 bits, so in these cases, 7ffffffffffffdff and
> >>> fffffffffffffbff are rounded.  
> >>
> >> My questions remain: why isn't this being treated like a bug?
> >>  
> > Hi Markus,
> >
> > I am confused about the code here too. Because in do_strtosz(), the
> > upper limit is
> >
> > val * mul >= 0xfffffffffffffc00
> >
> > So some data near 53 bit may be rounded. Is there a bug?  
> 
> No, but the design is surprising, and the functions lack written
> contracts, except for the do_strtosz() helper, which has one that sucks.
> 
> qemu_strtosz() & friends are designed to accept fraction * unit
> multiplier.  Example: 1.5M means 1.5 * 1024 * 1024 with qemu_strtosz()
> and qemu_strtosz_MiB(), and 1.5 * 1000 * 1000 with
> qemu_strtosz_metric().  Whether supporting fractions is a good idea is
> debatable, but it's what we've got.
> 
> The implementation limits the numeric part to the precision of double,
> i.e. 53 bits.  "8PiB should be enough for anybody."
>
> Switching it from double to long double raises the limit to the
> precision of long double.  At least 64 bit on common hosts, but hosts
> exist where it's the same 53 bits.  Do we support any such hosts?  If
> yes, then we'd make the precision depend on the host, which feels like a
> bad idea.
> 
> A possible alternative is to parse the numeric part both as a double and
> as a 64 bit unsigned integer, then use whatever consumes more
> characters.  This enables providing full 64 bits unless you actually use
> a fraction.
> 
> As far as I remember, the only problem we've ever had with the 53 bits
> limit is developer confusion :)

On CLI, we could (a)use full 64bit (-1) lat/bw to mark unreachable nodes.
Also it would be more consistent for both QMP and CLI to be able
handle the same range. This way what was configured over QMP could be
also configured using CLI.

> Patches welcome.



^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v14 03/11] tests: Add test for QAPI builtin type time
  2019-11-08  8:41             ` Igor Mammedov
@ 2019-11-11  3:12               ` Tao Xu
  2019-11-11 10:02                 ` Igor Mammedov
  0 siblings, 1 reply; 32+ messages in thread
From: Tao Xu @ 2019-11-11  3:12 UTC (permalink / raw)
  To: Igor Mammedov, Markus Armbruster
  Cc: lvivier, thuth, Eduardo Habkost, mst, Liu, Jingqi, Du, Fan,
	mdroth, qemu-devel, jonathan.cameron

On 11/8/2019 4:41 PM, Igor Mammedov wrote:
> On Fri, 08 Nov 2019 09:05:52 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
> 
>> Tao Xu <tao3.xu@intel.com> writes:
>>
>>> On 11/7/2019 9:31 PM, Eduardo Habkost wrote:
>>>> On Thu, Nov 07, 2019 at 02:24:52PM +0800, Tao Xu wrote:
>>>>> On 11/7/2019 4:53 AM, Eduardo Habkost wrote:
>>>>>> On Mon, Oct 28, 2019 at 03:52:12PM +0800, Tao Xu wrote:
>>>>>>> Add tests for time input such as zero, around limit of precision,
>>>>>>> signed upper limit, actual upper limit, beyond limits, time suffixes,
>>>>>>> and etc.
>>>>>>>
>>>>>>> Signed-off-by: Tao Xu <tao3.xu@intel.com>
>>>>>>> ---
>>>>>> [...]
>>>>>>> +    /* Close to signed upper limit 0x7ffffffffffffc00 (53 msbs set) */
>>>>>>> +    qdict = keyval_parse("time1=9223372036854774784," /* 7ffffffffffffc00 */
>>>>>>> +                         "time2=9223372036854775295", /* 7ffffffffffffdff */
>>>>>>> +                         NULL, &error_abort);
>>>>>>> +    v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
>>>>>>> +    qobject_unref(qdict);
>>>>>>> +    visit_start_struct(v, NULL, NULL, 0, &error_abort);
>>>>>>> +    visit_type_time(v, "time1", &time, &error_abort);
>>>>>>> +    g_assert_cmphex(time, ==, 0x7ffffffffffffc00);
>>>>>>> +    visit_type_time(v, "time2", &time, &error_abort);
>>>>>>> +    g_assert_cmphex(time, ==, 0x7ffffffffffffc00);
>>>>>>
>>>>>> I'm confused by this test case and the one below[1].  Are these
>>>>>> known bugs?  Shouldn't we document them as known bugs?
>>>>>
>>>>> Because do_strtosz() or do_strtomul() actually parse with strtod(), so the
>>>>> precision is 53 bits, so in these cases, 7ffffffffffffdff and
>>>>> fffffffffffffbff are rounded.
>>>>
>>>> My questions remain: why isn't this being treated like a bug?
>>>>   
>>> Hi Markus,
>>>
>>> I am confused about the code here too. Because in do_strtosz(), the
>>> upper limit is
>>>
>>> val * mul >= 0xfffffffffffffc00
>>>
>>> So some data near 53 bit may be rounded. Is there a bug?
>>
>> No, but the design is surprising, and the functions lack written
>> contracts, except for the do_strtosz() helper, which has one that sucks.
>>
>> qemu_strtosz() & friends are designed to accept fraction * unit
>> multiplier.  Example: 1.5M means 1.5 * 1024 * 1024 with qemu_strtosz()
>> and qemu_strtosz_MiB(), and 1.5 * 1000 * 1000 with
>> qemu_strtosz_metric().  Whether supporting fractions is a good idea is
>> debatable, but it's what we've got.
>>
>> The implementation limits the numeric part to the precision of double,
>> i.e. 53 bits.  "8PiB should be enough for anybody."
>>
>> Switching it from double to long double raises the limit to the
>> precision of long double.  At least 64 bit on common hosts, but hosts
>> exist where it's the same 53 bits.  Do we support any such hosts?  If
>> yes, then we'd make the precision depend on the host, which feels like a
>> bad idea.
>>
>> A possible alternative is to parse the numeric part both as a double and
>> as a 64 bit unsigned integer, then use whatever consumes more
>> characters.  This enables providing full 64 bits unless you actually use
>> a fraction.
>>
>> As far as I remember, the only problem we've ever had with the 53 bits
>> limit is developer confusion :)
> 
> On CLI, we could (a)use full 64bit (-1) lat/bw to mark unreachable nodes.
> Also it would be more consistent for both QMP and CLI to be able
> handle the same range. This way what was configured over QMP could be
> also configured using CLI.
> 

OK. I will add a new patch to do this. Next version we will submit
separated patches for QAPI builtin type changes.




^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v14 03/11] tests: Add test for QAPI builtin type time
  2019-11-11  3:12               ` Tao Xu
@ 2019-11-11 10:02                 ` Igor Mammedov
  0 siblings, 0 replies; 32+ messages in thread
From: Igor Mammedov @ 2019-11-11 10:02 UTC (permalink / raw)
  To: Tao Xu
  Cc: lvivier, thuth, Eduardo Habkost, mst, qemu-devel, Liu, Jingqi,
	Du, Fan, Markus Armbruster, mdroth, jonathan.cameron

On Mon, 11 Nov 2019 11:12:28 +0800
Tao Xu <tao3.xu@intel.com> wrote:

> On 11/8/2019 4:41 PM, Igor Mammedov wrote:
> > On Fri, 08 Nov 2019 09:05:52 +0100
> > Markus Armbruster <armbru@redhat.com> wrote:
> >   
> >> Tao Xu <tao3.xu@intel.com> writes:
> >>  
> >>> On 11/7/2019 9:31 PM, Eduardo Habkost wrote:  
> >>>> On Thu, Nov 07, 2019 at 02:24:52PM +0800, Tao Xu wrote:  
> >>>>> On 11/7/2019 4:53 AM, Eduardo Habkost wrote:  
> >>>>>> On Mon, Oct 28, 2019 at 03:52:12PM +0800, Tao Xu wrote:  
> >>>>>>> Add tests for time input such as zero, around limit of precision,
> >>>>>>> signed upper limit, actual upper limit, beyond limits, time suffixes,
> >>>>>>> and etc.
> >>>>>>>
> >>>>>>> Signed-off-by: Tao Xu <tao3.xu@intel.com>
> >>>>>>> ---  
> >>>>>> [...]  
> >>>>>>> +    /* Close to signed upper limit 0x7ffffffffffffc00 (53 msbs set) */
> >>>>>>> +    qdict = keyval_parse("time1=9223372036854774784," /* 7ffffffffffffc00 */
> >>>>>>> +                         "time2=9223372036854775295", /* 7ffffffffffffdff */
> >>>>>>> +                         NULL, &error_abort);
> >>>>>>> +    v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
> >>>>>>> +    qobject_unref(qdict);
> >>>>>>> +    visit_start_struct(v, NULL, NULL, 0, &error_abort);
> >>>>>>> +    visit_type_time(v, "time1", &time, &error_abort);
> >>>>>>> +    g_assert_cmphex(time, ==, 0x7ffffffffffffc00);
> >>>>>>> +    visit_type_time(v, "time2", &time, &error_abort);
> >>>>>>> +    g_assert_cmphex(time, ==, 0x7ffffffffffffc00);  
> >>>>>>
> >>>>>> I'm confused by this test case and the one below[1].  Are these
> >>>>>> known bugs?  Shouldn't we document them as known bugs?  
> >>>>>
> >>>>> Because do_strtosz() or do_strtomul() actually parse with strtod(), so the
> >>>>> precision is 53 bits, so in these cases, 7ffffffffffffdff and
> >>>>> fffffffffffffbff are rounded.  
> >>>>
> >>>> My questions remain: why isn't this being treated like a bug?
> >>>>     
> >>> Hi Markus,
> >>>
> >>> I am confused about the code here too. Because in do_strtosz(), the
> >>> upper limit is
> >>>
> >>> val * mul >= 0xfffffffffffffc00
> >>>
> >>> So some data near 53 bit may be rounded. Is there a bug?  
> >>
> >> No, but the design is surprising, and the functions lack written
> >> contracts, except for the do_strtosz() helper, which has one that sucks.
> >>
> >> qemu_strtosz() & friends are designed to accept fraction * unit
> >> multiplier.  Example: 1.5M means 1.5 * 1024 * 1024 with qemu_strtosz()
> >> and qemu_strtosz_MiB(), and 1.5 * 1000 * 1000 with
> >> qemu_strtosz_metric().  Whether supporting fractions is a good idea is
> >> debatable, but it's what we've got.
> >>
> >> The implementation limits the numeric part to the precision of double,
> >> i.e. 53 bits.  "8PiB should be enough for anybody."
> >>
> >> Switching it from double to long double raises the limit to the
> >> precision of long double.  At least 64 bit on common hosts, but hosts
> >> exist where it's the same 53 bits.  Do we support any such hosts?  If
> >> yes, then we'd make the precision depend on the host, which feels like a
> >> bad idea.
> >>
> >> A possible alternative is to parse the numeric part both as a double and
> >> as a 64 bit unsigned integer, then use whatever consumes more
> >> characters.  This enables providing full 64 bits unless you actually use
> >> a fraction.
> >>
> >> As far as I remember, the only problem we've ever had with the 53 bits
> >> limit is developer confusion :)  
> > 
> > On CLI, we could (a)use full 64bit (-1) lat/bw to mark unreachable nodes.
> > Also it would be more consistent for both QMP and CLI to be able
> > handle the same range. This way what was configured over QMP could be
> > also configured using CLI.
> >   
> 
> OK. I will add a new patch to do this. Next version we will submit
> separated patches for QAPI builtin type changes.

Since you've got rid of magic handling from CLI parsing, it's not
must have feauture but it would be nice to have and probably could
be done on top of HMAT patches.



^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v14 03/11] tests: Add test for QAPI builtin type time
  2019-11-08  8:05           ` Markus Armbruster
  2019-11-08  8:41             ` Igor Mammedov
@ 2019-11-12 20:15             ` Eduardo Habkost
  2019-11-13  1:01               ` Tao Xu
  1 sibling, 1 reply; 32+ messages in thread
From: Eduardo Habkost @ 2019-11-12 20:15 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: lvivier, thuth, mst, Liu, Jingqi, Tao Xu, Du, Fan, mdroth,
	qemu-devel, jonathan.cameron, imammedo

On Fri, Nov 08, 2019 at 09:05:52AM +0100, Markus Armbruster wrote:
> Tao Xu <tao3.xu@intel.com> writes:
> 
> > On 11/7/2019 9:31 PM, Eduardo Habkost wrote:
> >> On Thu, Nov 07, 2019 at 02:24:52PM +0800, Tao Xu wrote:
> >>> On 11/7/2019 4:53 AM, Eduardo Habkost wrote:
> >>>> On Mon, Oct 28, 2019 at 03:52:12PM +0800, Tao Xu wrote:
> >>>>> Add tests for time input such as zero, around limit of precision,
> >>>>> signed upper limit, actual upper limit, beyond limits, time suffixes,
> >>>>> and etc.
> >>>>>
> >>>>> Signed-off-by: Tao Xu <tao3.xu@intel.com>
> >>>>> ---
> >>>> [...]
> >>>>> +    /* Close to signed upper limit 0x7ffffffffffffc00 (53 msbs set) */
> >>>>> +    qdict = keyval_parse("time1=9223372036854774784," /* 7ffffffffffffc00 */
> >>>>> +                         "time2=9223372036854775295", /* 7ffffffffffffdff */
> >>>>> +                         NULL, &error_abort);
> >>>>> +    v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
> >>>>> +    qobject_unref(qdict);
> >>>>> +    visit_start_struct(v, NULL, NULL, 0, &error_abort);
> >>>>> +    visit_type_time(v, "time1", &time, &error_abort);
> >>>>> +    g_assert_cmphex(time, ==, 0x7ffffffffffffc00);
> >>>>> +    visit_type_time(v, "time2", &time, &error_abort);
> >>>>> +    g_assert_cmphex(time, ==, 0x7ffffffffffffc00);
> >>>>
> >>>> I'm confused by this test case and the one below[1].  Are these
> >>>> known bugs?  Shouldn't we document them as known bugs?
> >>>
> >>> Because do_strtosz() or do_strtomul() actually parse with strtod(), so the
> >>> precision is 53 bits, so in these cases, 7ffffffffffffdff and
> >>> fffffffffffffbff are rounded.
> >>
> >> My questions remain: why isn't this being treated like a bug?
> >>
> > Hi Markus,
> >
> > I am confused about the code here too. Because in do_strtosz(), the
> > upper limit is
> >
> > val * mul >= 0xfffffffffffffc00
> >
> > So some data near 53 bit may be rounded. Is there a bug?
> 
> No, but the design is surprising, and the functions lack written
> contracts, except for the do_strtosz() helper, which has one that sucks.
> 
> qemu_strtosz() & friends are designed to accept fraction * unit
> multiplier.  Example: 1.5M means 1.5 * 1024 * 1024 with qemu_strtosz()
> and qemu_strtosz_MiB(), and 1.5 * 1000 * 1000 with
> qemu_strtosz_metric().  Whether supporting fractions is a good idea is
> debatable, but it's what we've got.
> 
> The implementation limits the numeric part to the precision of double,
> i.e. 53 bits.  "8PiB should be enough for anybody."
> 
> Switching it from double to long double raises the limit to the
> precision of long double.  At least 64 bit on common hosts, but hosts
> exist where it's the same 53 bits.  Do we support any such hosts?  If
> yes, then we'd make the precision depend on the host, which feels like a
> bad idea.
> 
> A possible alternative is to parse the numeric part both as a double and
> as a 64 bit unsigned integer, then use whatever consumes more
> characters.  This enables providing full 64 bits unless you actually use
> a fraction.
> 

This sounds like the right thing to do, if user input is an
integer and the code in the other end is consuming an integer.


> As far as I remember, the only problem we've ever had with the 53 bits
> limit is developer confusion :)
> 

Developer confusion, I can deal with.  However, exposing this
behavior on external interfaces is a bug to me.

I don't know how serious the bug is because I don't know which
interfaces are affected by it.  Do we have a list?

> Patches welcome.

My first goal is to get the maintainers of that code to recognize
it as a bug.  Then I hope this will motivate somebody else to fix
it.  :)

-- 
Eduardo



^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v14 03/11] tests: Add test for QAPI builtin type time
  2019-11-12 20:15             ` Eduardo Habkost
@ 2019-11-13  1:01               ` Tao Xu
  2019-11-13 22:06                 ` Eduardo Habkost
  0 siblings, 1 reply; 32+ messages in thread
From: Tao Xu @ 2019-11-13  1:01 UTC (permalink / raw)
  To: Eduardo Habkost, Markus Armbruster
  Cc: lvivier, thuth, mst, Liu, Jingqi, Du, Fan, mdroth, qemu-devel,
	jonathan.cameron, imammedo

On 11/13/2019 4:15 AM, Eduardo Habkost wrote:
> On Fri, Nov 08, 2019 at 09:05:52AM +0100, Markus Armbruster wrote:
>> Tao Xu <tao3.xu@intel.com> writes:
>>
>>> On 11/7/2019 9:31 PM, Eduardo Habkost wrote:
>>>> On Thu, Nov 07, 2019 at 02:24:52PM +0800, Tao Xu wrote:
>>>>> On 11/7/2019 4:53 AM, Eduardo Habkost wrote:
>>>>>> On Mon, Oct 28, 2019 at 03:52:12PM +0800, Tao Xu wrote:
>>>>>>> Add tests for time input such as zero, around limit of precision,
>>>>>>> signed upper limit, actual upper limit, beyond limits, time suffixes,
>>>>>>> and etc.
>>>>>>>
>>>>>>> Signed-off-by: Tao Xu <tao3.xu@intel.com>
>>>>>>> ---
>>>>>> [...]
>>>>>>> +    /* Close to signed upper limit 0x7ffffffffffffc00 (53 msbs set) */
>>>>>>> +    qdict = keyval_parse("time1=9223372036854774784," /* 7ffffffffffffc00 */
>>>>>>> +                         "time2=9223372036854775295", /* 7ffffffffffffdff */
>>>>>>> +                         NULL, &error_abort);
>>>>>>> +    v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
>>>>>>> +    qobject_unref(qdict);
>>>>>>> +    visit_start_struct(v, NULL, NULL, 0, &error_abort);
>>>>>>> +    visit_type_time(v, "time1", &time, &error_abort);
>>>>>>> +    g_assert_cmphex(time, ==, 0x7ffffffffffffc00);
>>>>>>> +    visit_type_time(v, "time2", &time, &error_abort);
>>>>>>> +    g_assert_cmphex(time, ==, 0x7ffffffffffffc00);
>>>>>>
>>>>>> I'm confused by this test case and the one below[1].  Are these
>>>>>> known bugs?  Shouldn't we document them as known bugs?
>>>>>
>>>>> Because do_strtosz() or do_strtomul() actually parse with strtod(), so the
>>>>> precision is 53 bits, so in these cases, 7ffffffffffffdff and
>>>>> fffffffffffffbff are rounded.
>>>>
>>>> My questions remain: why isn't this being treated like a bug?
>>>>
>>> Hi Markus,
>>>
>>> I am confused about the code here too. Because in do_strtosz(), the
>>> upper limit is
>>>
>>> val * mul >= 0xfffffffffffffc00
>>>
>>> So some data near 53 bit may be rounded. Is there a bug?
>>
>> No, but the design is surprising, and the functions lack written
>> contracts, except for the do_strtosz() helper, which has one that sucks.
>>
>> qemu_strtosz() & friends are designed to accept fraction * unit
>> multiplier.  Example: 1.5M means 1.5 * 1024 * 1024 with qemu_strtosz()
>> and qemu_strtosz_MiB(), and 1.5 * 1000 * 1000 with
>> qemu_strtosz_metric().  Whether supporting fractions is a good idea is
>> debatable, but it's what we've got.
>>
>> The implementation limits the numeric part to the precision of double,
>> i.e. 53 bits.  "8PiB should be enough for anybody."
>>
>> Switching it from double to long double raises the limit to the
>> precision of long double.  At least 64 bit on common hosts, but hosts
>> exist where it's the same 53 bits.  Do we support any such hosts?  If
>> yes, then we'd make the precision depend on the host, which feels like a
>> bad idea.
>>
>> A possible alternative is to parse the numeric part both as a double and
>> as a 64 bit unsigned integer, then use whatever consumes more
>> characters.  This enables providing full 64 bits unless you actually use
>> a fraction.
>>
> 
> This sounds like the right thing to do, if user input is an
> integer and the code in the other end is consuming an integer.
> 
> 
>> As far as I remember, the only problem we've ever had with the 53 bits
>> limit is developer confusion :)
>>
> 
> Developer confusion, I can deal with.  However, exposing this
> behavior on external interfaces is a bug to me.
> 
> I don't know how serious the bug is because I don't know which
> interfaces are affected by it.  Do we have a list?
> 
>> Patches welcome.
> 
> My first goal is to get the maintainers of that code to recognize
> it as a bug.  Then I hope this will motivate somebody else to fix
> it.  :)
> 

Hi Eduardo,

If it is a bug, could the fix patch merged during rc1-rc3? Because I 
made 2 patches, and I want to submit before HMAT (HMAT patches is big, 
so submit together may be slow).

Tao


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v14 03/11] tests: Add test for QAPI builtin type time
  2019-11-13  1:01               ` Tao Xu
@ 2019-11-13 22:06                 ` Eduardo Habkost
  2019-11-14  0:51                   ` Tao Xu
  0 siblings, 1 reply; 32+ messages in thread
From: Eduardo Habkost @ 2019-11-13 22:06 UTC (permalink / raw)
  To: Tao Xu
  Cc: lvivier, thuth, mst, qemu-devel, Liu, Jingqi, Du, Fan,
	Markus Armbruster, mdroth, jonathan.cameron, imammedo

On Wed, Nov 13, 2019 at 09:01:29AM +0800, Tao Xu wrote:
> On 11/13/2019 4:15 AM, Eduardo Habkost wrote:
> > On Fri, Nov 08, 2019 at 09:05:52AM +0100, Markus Armbruster wrote:
> > > Tao Xu <tao3.xu@intel.com> writes:
> > > 
> > > > On 11/7/2019 9:31 PM, Eduardo Habkost wrote:
> > > > > On Thu, Nov 07, 2019 at 02:24:52PM +0800, Tao Xu wrote:
> > > > > > On 11/7/2019 4:53 AM, Eduardo Habkost wrote:
> > > > > > > On Mon, Oct 28, 2019 at 03:52:12PM +0800, Tao Xu wrote:
> > > > > > > > Add tests for time input such as zero, around limit of precision,
> > > > > > > > signed upper limit, actual upper limit, beyond limits, time suffixes,
> > > > > > > > and etc.
> > > > > > > > 
> > > > > > > > Signed-off-by: Tao Xu <tao3.xu@intel.com>
> > > > > > > > ---
> > > > > > > [...]
> > > > > > > > +    /* Close to signed upper limit 0x7ffffffffffffc00 (53 msbs set) */
> > > > > > > > +    qdict = keyval_parse("time1=9223372036854774784," /* 7ffffffffffffc00 */
> > > > > > > > +                         "time2=9223372036854775295", /* 7ffffffffffffdff */
> > > > > > > > +                         NULL, &error_abort);
> > > > > > > > +    v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
> > > > > > > > +    qobject_unref(qdict);
> > > > > > > > +    visit_start_struct(v, NULL, NULL, 0, &error_abort);
> > > > > > > > +    visit_type_time(v, "time1", &time, &error_abort);
> > > > > > > > +    g_assert_cmphex(time, ==, 0x7ffffffffffffc00);
> > > > > > > > +    visit_type_time(v, "time2", &time, &error_abort);
> > > > > > > > +    g_assert_cmphex(time, ==, 0x7ffffffffffffc00);
> > > > > > > 
> > > > > > > I'm confused by this test case and the one below[1].  Are these
> > > > > > > known bugs?  Shouldn't we document them as known bugs?
> > > > > > 
> > > > > > Because do_strtosz() or do_strtomul() actually parse with strtod(), so the
> > > > > > precision is 53 bits, so in these cases, 7ffffffffffffdff and
> > > > > > fffffffffffffbff are rounded.
> > > > > 
> > > > > My questions remain: why isn't this being treated like a bug?
> > > > > 
> > > > Hi Markus,
> > > > 
> > > > I am confused about the code here too. Because in do_strtosz(), the
> > > > upper limit is
> > > > 
> > > > val * mul >= 0xfffffffffffffc00
> > > > 
> > > > So some data near 53 bit may be rounded. Is there a bug?
> > > 
> > > No, but the design is surprising, and the functions lack written
> > > contracts, except for the do_strtosz() helper, which has one that sucks.
> > > 
> > > qemu_strtosz() & friends are designed to accept fraction * unit
> > > multiplier.  Example: 1.5M means 1.5 * 1024 * 1024 with qemu_strtosz()
> > > and qemu_strtosz_MiB(), and 1.5 * 1000 * 1000 with
> > > qemu_strtosz_metric().  Whether supporting fractions is a good idea is
> > > debatable, but it's what we've got.
> > > 
> > > The implementation limits the numeric part to the precision of double,
> > > i.e. 53 bits.  "8PiB should be enough for anybody."
> > > 
> > > Switching it from double to long double raises the limit to the
> > > precision of long double.  At least 64 bit on common hosts, but hosts
> > > exist where it's the same 53 bits.  Do we support any such hosts?  If
> > > yes, then we'd make the precision depend on the host, which feels like a
> > > bad idea.
> > > 
> > > A possible alternative is to parse the numeric part both as a double and
> > > as a 64 bit unsigned integer, then use whatever consumes more
> > > characters.  This enables providing full 64 bits unless you actually use
> > > a fraction.
> > > 
> > 
> > This sounds like the right thing to do, if user input is an
> > integer and the code in the other end is consuming an integer.
> > 
> > 
> > > As far as I remember, the only problem we've ever had with the 53 bits
> > > limit is developer confusion :)
> > > 
> > 
> > Developer confusion, I can deal with.  However, exposing this
> > behavior on external interfaces is a bug to me.
> > 
> > I don't know how serious the bug is because I don't know which
> > interfaces are affected by it.  Do we have a list?
> > 
> > > Patches welcome.
> > 
> > My first goal is to get the maintainers of that code to recognize
> > it as a bug.  Then I hope this will motivate somebody else to fix
> > it.  :)
> > 
> 
> Hi Eduardo,
> 
> If it is a bug, could the fix patch merged during rc1-rc3? Because I made 2
> patches, and I want to submit before HMAT (HMAT patches is big, so submit
> together may be slow).

Even if I convince other maintainers it is a bug, I don't think
it is serious enough to require a fix in QEMU 4.2.  I suggest
finishing the ongoing HMAT work first, and worry about this issue
later.

Or, if you really prefer to address it before HMAT, it's OK to
make the next version of the HMAT series depend on a series
that's not merged yet.  Just make this explicit in the series
cover letter (publishing a git branch to help review and testing
is also appreciated).

-- 
Eduardo



^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v14 03/11] tests: Add test for QAPI builtin type time
  2019-11-13 22:06                 ` Eduardo Habkost
@ 2019-11-14  0:51                   ` Tao Xu
  0 siblings, 0 replies; 32+ messages in thread
From: Tao Xu @ 2019-11-14  0:51 UTC (permalink / raw)
  To: Eduardo Habkost
  Cc: lvivier, thuth, mst, qemu-devel, Liu, Jingqi, Du, Fan,
	Markus Armbruster, mdroth, jonathan.cameron, imammedo

On 11/14/2019 6:06 AM, Eduardo Habkost wrote:
> On Wed, Nov 13, 2019 at 09:01:29AM +0800, Tao Xu wrote:
>> On 11/13/2019 4:15 AM, Eduardo Habkost wrote:
>>> On Fri, Nov 08, 2019 at 09:05:52AM +0100, Markus Armbruster wrote:
>>>> Tao Xu <tao3.xu@intel.com> writes:
>>>>
>>>>> On 11/7/2019 9:31 PM, Eduardo Habkost wrote:
>>>>>> On Thu, Nov 07, 2019 at 02:24:52PM +0800, Tao Xu wrote:
>>>>>>> On 11/7/2019 4:53 AM, Eduardo Habkost wrote:
>>>>>>>> On Mon, Oct 28, 2019 at 03:52:12PM +0800, Tao Xu wrote:
>>>>>>>>> Add tests for time input such as zero, around limit of precision,
>>>>>>>>> signed upper limit, actual upper limit, beyond limits, time suffixes,
>>>>>>>>> and etc.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Tao Xu <tao3.xu@intel.com>
>>>>>>>>> ---
>>>>>>>> [...]
>>>>>>>>> +    /* Close to signed upper limit 0x7ffffffffffffc00 (53 msbs set) */
>>>>>>>>> +    qdict = keyval_parse("time1=9223372036854774784," /* 7ffffffffffffc00 */
>>>>>>>>> +                         "time2=9223372036854775295", /* 7ffffffffffffdff */
>>>>>>>>> +                         NULL, &error_abort);
>>>>>>>>> +    v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
>>>>>>>>> +    qobject_unref(qdict);
>>>>>>>>> +    visit_start_struct(v, NULL, NULL, 0, &error_abort);
>>>>>>>>> +    visit_type_time(v, "time1", &time, &error_abort);
>>>>>>>>> +    g_assert_cmphex(time, ==, 0x7ffffffffffffc00);
>>>>>>>>> +    visit_type_time(v, "time2", &time, &error_abort);
>>>>>>>>> +    g_assert_cmphex(time, ==, 0x7ffffffffffffc00);
>>>>>>>>
>>>>>>>> I'm confused by this test case and the one below[1].  Are these
>>>>>>>> known bugs?  Shouldn't we document them as known bugs?
>>>>>>>
>>>>>>> Because do_strtosz() or do_strtomul() actually parse with strtod(), so the
>>>>>>> precision is 53 bits, so in these cases, 7ffffffffffffdff and
>>>>>>> fffffffffffffbff are rounded.
>>>>>>
>>>>>> My questions remain: why isn't this being treated like a bug?
>>>>>>
>>>>> Hi Markus,
>>>>>
>>>>> I am confused about the code here too. Because in do_strtosz(), the
>>>>> upper limit is
>>>>>
>>>>> val * mul >= 0xfffffffffffffc00
>>>>>
>>>>> So some data near 53 bit may be rounded. Is there a bug?
>>>>
>>>> No, but the design is surprising, and the functions lack written
>>>> contracts, except for the do_strtosz() helper, which has one that sucks.
>>>>
>>>> qemu_strtosz() & friends are designed to accept fraction * unit
>>>> multiplier.  Example: 1.5M means 1.5 * 1024 * 1024 with qemu_strtosz()
>>>> and qemu_strtosz_MiB(), and 1.5 * 1000 * 1000 with
>>>> qemu_strtosz_metric().  Whether supporting fractions is a good idea is
>>>> debatable, but it's what we've got.
>>>>
>>>> The implementation limits the numeric part to the precision of double,
>>>> i.e. 53 bits.  "8PiB should be enough for anybody."
>>>>
>>>> Switching it from double to long double raises the limit to the
>>>> precision of long double.  At least 64 bit on common hosts, but hosts
>>>> exist where it's the same 53 bits.  Do we support any such hosts?  If
>>>> yes, then we'd make the precision depend on the host, which feels like a
>>>> bad idea.
>>>>
>>>> A possible alternative is to parse the numeric part both as a double and
>>>> as a 64 bit unsigned integer, then use whatever consumes more
>>>> characters.  This enables providing full 64 bits unless you actually use
>>>> a fraction.
>>>>
>>>
>>> This sounds like the right thing to do, if user input is an
>>> integer and the code in the other end is consuming an integer.
>>>
>>>
>>>> As far as I remember, the only problem we've ever had with the 53 bits
>>>> limit is developer confusion :)
>>>>
>>>
>>> Developer confusion, I can deal with.  However, exposing this
>>> behavior on external interfaces is a bug to me.
>>>
>>> I don't know how serious the bug is because I don't know which
>>> interfaces are affected by it.  Do we have a list?
>>>
>>>> Patches welcome.
>>>
>>> My first goal is to get the maintainers of that code to recognize
>>> it as a bug.  Then I hope this will motivate somebody else to fix
>>> it.  :)
>>>
>>
>> Hi Eduardo,
>>
>> If it is a bug, could the fix patch merged during rc1-rc3? Because I made 2
>> patches, and I want to submit before HMAT (HMAT patches is big, so submit
>> together may be slow).
> 
> Even if I convince other maintainers it is a bug, I don't think
> it is serious enough to require a fix in QEMU 4.2.  I suggest
> finishing the ongoing HMAT work first, and worry about this issue
> later.
> 
> Or, if you really prefer to address it before HMAT, it's OK to
> make the next version of the HMAT series depend on a series
> that's not merged yet.  Just make this explicit in the series
> cover letter (publishing a git branch to help review and testing
> is also appreciated).
> 
OK I will submit them together.


^ permalink raw reply	[flat|nested] 32+ messages in thread

end of thread, other threads:[~2019-11-14  0:59 UTC | newest]

Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-28  7:52 [PATCH v14 00/11] Build ACPI Heterogeneous Memory Attribute Table (HMAT) Tao Xu
2019-10-28  7:52 ` [PATCH v14 01/11] util/cutils: Add qemu_strtotime_ns() Tao Xu
2019-11-06 19:56   ` Eduardo Habkost
2019-11-07  1:38     ` Tao Xu
2019-10-28  7:52 ` [PATCH v14 02/11] qapi: Add builtin type time Tao Xu
2019-10-28  7:52 ` [PATCH v14 03/11] tests: Add test for QAPI " Tao Xu
2019-11-06 20:53   ` Eduardo Habkost
2019-11-07  6:24     ` Tao Xu
2019-11-07 13:31       ` Eduardo Habkost
2019-11-08  5:25         ` Tao Xu
2019-11-08  8:05           ` Markus Armbruster
2019-11-08  8:41             ` Igor Mammedov
2019-11-11  3:12               ` Tao Xu
2019-11-11 10:02                 ` Igor Mammedov
2019-11-12 20:15             ` Eduardo Habkost
2019-11-13  1:01               ` Tao Xu
2019-11-13 22:06                 ` Eduardo Habkost
2019-11-14  0:51                   ` Tao Xu
2019-10-28  7:52 ` [PATCH v14 04/11] numa: Extend CLI to provide initiator information for numa nodes Tao Xu
2019-11-06 20:29   ` Eric Blake
2019-11-07  1:51     ` Tao Xu
2019-10-28  7:52 ` [PATCH v14 05/11] numa: Extend CLI to provide memory latency and bandwidth information Tao Xu
2019-10-28  7:52 ` [PATCH v14 06/11] numa: Calculate hmat latency and bandwidth entry list Tao Xu
2019-10-28  7:52 ` [PATCH v14 07/11] numa: Extend CLI to provide memory side cache information Tao Xu
2019-10-28  7:52 ` [PATCH v14 08/11] hmat acpi: Build Memory Proximity Domain Attributes Structure(s) Tao Xu
2019-10-28  7:52 ` [PATCH v14 09/11] hmat acpi: Build System Locality Latency and Bandwidth Information Structure(s) Tao Xu
2019-10-28  7:52 ` [PATCH v14 10/11] hmat acpi: Build Memory Side Cache " Tao Xu
2019-10-28  7:52 ` [PATCH v14 11/11] tests/bios-tables-test: add test cases for ACPI HMAT Tao Xu
2019-10-28  8:39   ` Michael S. Tsirkin
2019-10-28  8:50     ` Tao Xu
2019-10-28  8:36 ` [PATCH v14 00/11] Build ACPI Heterogeneous Memory Attribute Table (HMAT) no-reply
2019-11-06  8:39 ` Tao Xu

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.