On Fri, 20 Sep 2019 15:43:47 +0800
Tao Xu <address@hidden> wrote:
From: Liu Jingqi <address@hidden>
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 <address@hidden>
Signed-off-by: Tao Xu <address@hidden>
---
Changes in v12:
- Fix a bug that if HMAT is enabled and without hmat-lb setting,
QEMU will crash. (reported by Danmei Wei)
Changes in v11:
- Calculate base in build_hmat_lb().
---
hw/acpi/hmat.c | 126 ++++++++++++++++++++++++++++++++++++++++++++++++-
hw/acpi/hmat.h | 2 +
2 files changed, 127 insertions(+), 1 deletion(-)
diff --git a/hw/acpi/hmat.c b/hw/acpi/hmat.c
index 1368fce7ee..e7be849581 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,105 @@ static void build_hmat_mpda(GArray *table_data, uint16_t
flags, int initiator,
build_append_int_noprefix(table_data, 0, 8);
}
+static bool entry_overflow(uint64_t *lb_data, uint64_t base, int len)
+{
+ int i;
+
+ for (i = 0; i < len; i++) {
+ if (lb_data[i] / base >= UINT16_MAX) {
+ return true;
+ }
+ }
+
+ return false;
+}
I suggest to do this check at CLI parsing time
+/*
+ * 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 type)
+{
+ uint8_t mask = 0x0f;
+ uint32_t s = num_initiator;
+ uint32_t t = num_target;
drop this locals and use arguments directly
+ uint64_t base = 1;
+ uint64_t *lb_data;
+ int i, unit;
+
+ /* 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, 32 + 4 * s + 4 * t + 2 * s * t, 4);
^^^^
to me above looks like /dev/random output, absolutely unreadable.
Suggest to use local var (like: lb_length) for expression with comments
beside magic numbers.
+ /* Flags: Bits [3:0] Memory Hierarchy, Bits[7:4] Reserved */
+ build_append_int_noprefix(table_data, hmat_lb->hierarchy & mask, 1);
why do you need to use mask here?
+ /* Reserved */
+ build_append_int_noprefix(table_data, 0, 2);
+ /* Number of Initiator Proximity Domains (s) */
+ build_append_int_noprefix(table_data, s, 4);
+ /* Number of Target Proximity Domains (t) */
+ build_append_int_noprefix(table_data, t, 4);
+ /* Reserved */
+ build_append_int_noprefix(table_data, 0, 4);
+
+ if (HMAT_IS_LATENCY(type)) {
+ unit = 1000;
+ lb_data = hmat_lb->latency;
+ } else {
+ unit = 1024;
+ lb_data = hmat_lb->bandwidth;
+ }
+
+ while (entry_overflow(lb_data, base, s * t)) {
+ for (i = 0; i < s * t; i++) {
+ if (!QEMU_IS_ALIGNED(lb_data[i], unit * base)) {
+ error_report("Invalid latency/bandwidth input, all "
+ "latencies/bandwidths should be specified in the same units.");
+ exit(1);
+ }
+ }
+ base *= unit;
+ }
Can you clarify what you are trying to check here?
+
+ /* Entry Base Unit */
+ build_append_int_noprefix(table_data, base, 8);
+
+ /* Initiator Proximity Domain List */
+ for (i = 0; i < s; i++) {
+ build_append_int_noprefix(table_data, initiator_list[i], 4);
+ }
+
+ /* Target Proximity Domain List */
+ for (i = 0; i < t; i++) {
+ build_append_int_noprefix(table_data, i, 4);
+ }
+
+ /* Latency or Bandwidth Entries */
+ for (i = 0; i < s * t; i++) {
+ uint16_t entry;
+
+ if (HMAT_IS_LATENCY(type)) {
drop if condition and reuse lb_data, that you've just initialized above
+ entry = hmat_lb->latency[i] / base;
...
+ entry = hmat_lb->bandwidth[i] / base;
I'm not sure that above is correct.
Pls clarify math behind above 2 expressions
+ }
+
+ build_append_int_noprefix(table_data, entry, 2);
+ }
+}
+
/* Build HMAT sub table structures */
static void hmat_build_table_structs(GArray *table_data, NumaState *nstat)
{
uint16_t flags;
- int i;
+ uint32_t *initiator_list = NULL;
+ int i, j, hrchy, type;
s/hrchy/hierarchy/
+ HMAT_LB_Info *numa_hmat_lb;
for (i = 0; i < nstat->num_nodes; i++) {
flags = 0;
@@ -82,6 +177,35 @@ static void hmat_build_table_structs(GArray *table_data,
NumaState *nstat)
build_hmat_mpda(table_data, flags, nstat->nodes[i].initiator, i);
}
+
+ if (nstat->num_initiator) {
+ initiator_list = g_malloc0(nstat->num_initiator * sizeof(uint32_t));
+ for (i = 0, j = 0; i < nstat->num_nodes; i++) {
+ if (nstat->nodes[i].has_cpu) {
+ initiator_list[j] = i;
+ j++;
+ }
+ }
+ }
+
+ /*
+ * ACPI 6.3: 5.2.27.4 System Locality Latency and Bandwidth Information
+ * Structure: Table 5-146
+ */
+ for (hrchy = HMAT_LB_MEM_MEMORY;
+ hrchy <= HMAT_LB_MEM_CACHE_3RD_LEVEL; hrchy++) {
+ for (type = HMAT_LB_DATA_ACCESS_LATENCY;
+ type <= HMAT_LB_DATA_WRITE_BANDWIDTH; type++) {
+ numa_hmat_lb = nstat->hmat_lb[hrchy][type];
+
+ if (numa_hmat_lb) {
+ build_hmat_lb(table_data, numa_hmat_lb, nstat->num_initiator,
+ nstat->num_nodes, initiator_list, type);
+ }
+ }
+ }
+
+ g_free(initiator_list);
}
void build_hmat(GArray *table_data, BIOSLinker *linker, NumaState *nstat)
diff --git a/hw/acpi/hmat.h b/hw/acpi/hmat.h
index 0c1839cf6f..1154dfb48e 100644
--- a/hw/acpi/hmat.h
+++ b/hw/acpi/hmat.h
@@ -40,6 +40,8 @@
*/
#define HMAT_PROX_INIT_VALID 0x1
+#define HMAT_IS_LATENCY(type) (type <= HMAT_LB_DATA_WRITE_LATENCY)
it's not worth to create macro for 1-off calculation, just drop it
and s/if (HMAT_IS_LATENCY(type))/if(type <= HMAT_LB_DATA_WRITE_LATENCY)/
+
void build_hmat(GArray *table_data, BIOSLinker *linker, NumaState *nstat);
#endif