From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.2 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 130CDC73C46 for ; Tue, 9 Jul 2019 15:54:10 +0000 (UTC) Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 99086216C4 for ; Tue, 9 Jul 2019 15:54:09 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 99086216C4 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=intel.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Received: from localhost ([::1]:51306 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.86_2) (envelope-from ) id 1hksRU-0002sQ-PS for qemu-devel@archiver.kernel.org; Tue, 09 Jul 2019 11:54:08 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:50869) by lists.gnu.org with esmtp (Exim 4.86_2) (envelope-from ) id 1hksMi-0001CY-5N for qemu-devel@nongnu.org; Tue, 09 Jul 2019 11:49:15 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1hksMW-0007Iz-6h for qemu-devel@nongnu.org; Tue, 09 Jul 2019 11:49:03 -0400 Received: from mga01.intel.com ([192.55.52.88]:3860) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1hksMR-0007F9-NG for qemu-devel@nongnu.org; Tue, 09 Jul 2019 11:48:57 -0400 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga007.jf.intel.com ([10.7.209.58]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 09 Jul 2019 08:48:51 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.63,470,1557212400"; d="scan'208";a="156213960" Received: from txu2-mobl.ccr.corp.intel.com (HELO [10.255.30.194]) ([10.255.30.194]) by orsmga007.jf.intel.com with ESMTP; 09 Jul 2019 08:48:48 -0700 To: Jonathan Cameron References: <20190707142958.31316-1-tao3.xu@intel.com> <20190707142958.31316-8-tao3.xu@intel.com> <20190709165001.0000657d@huawei.com> From: Tao Xu Message-ID: Date: Tue, 9 Jul 2019 23:48:48 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.7.2 MIME-Version: 1.0 In-Reply-To: <20190709165001.0000657d@huawei.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 192.55.52.88 Subject: Re: [Qemu-devel] [PATCH v6 07/14] hmat acpi: Build System Locality Latency and Bandwidth Information Structure(s) X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: ehabkost@redhat.com, jingqi.liu@intel.com, fan.du@intel.com, qemu-devel@nongnu.org, imammedo@redhat.com, dan.j.williams@intel.com Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" On 7/9/2019 4:50 PM, Jonathan Cameron wrote: > On Sun, 7 Jul 2019 22:29:51 +0800 > Tao Xu wrote: > >> From: Liu Jingqi >> >> 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 >> Signed-off-by: Tao Xu > > I think you've missed one of the 6.3 changes. Otherwise a few nitpicks > but looks good to me. > > Reviewed-by: Jonathan Cameron > > I've focused on the patches actually dealing with the ACPI tables because > I'm far less familiar with the opt parsing etc in qemu so don't > feel confident expressing a view on those. > > Thanks, > > Jonathan > Thank you for your review and suggestions :) I will improve the issues in my next version. Tao >> --- >> >> Changes in v6: >> - Update the describes in ACPI 6.3 >> - remove num target and target_pxm, because all numa node can be >> target(no matter it can be reached or not, The Entry Base Unit for >> latency 0xFFFF means the initiator and target domains are >> unreachable from each other) >> --- >> hw/acpi/hmat.c | 94 ++++++++++++++++++++++++++++++++++++++++- >> hw/acpi/hmat.h | 39 +++++++++++++++++ >> include/qemu/typedefs.h | 1 + >> include/sysemu/numa.h | 3 ++ >> include/sysemu/sysemu.h | 22 ++++++++++ >> 5 files changed, 158 insertions(+), 1 deletion(-) >> >> diff --git a/hw/acpi/hmat.c b/hw/acpi/hmat.c >> index abf99b1adc..6dd39b0c85 100644 >> --- a/hw/acpi/hmat.c >> +++ b/hw/acpi/hmat.c >> @@ -67,11 +67,80 @@ static void build_hmat_mpda(GArray *table_data, uint16_t flags, int initiator, >> build_append_int_noprefix(table_data, 0, 8); >> } >> >> +/* >> + * ACPI 6.3: 5.2.27.4 System Locality Latency and Bandwidth Information >> + * Structure: Table 5-142 >> + */ >> +static void build_hmat_lb(GArray *table_data, HMAT_LB_Info *numa_hmat_lb, >> + uint32_t num_initiator, uint32_t num_target, >> + uint32_t *initiator_pxm, int type) >> +{ >> + uint32_t s = num_initiator; >> + uint32_t t = num_target; >> + uint8_t m, n; >> + int i; >> + >> + /* 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); >> + /* Flags */ >> + build_append_int_noprefix(table_data, numa_hmat_lb->hierarchy, 1); > > There might be a small argument in favour of masking this as half the bits > are reserved. Thanks, I will add a reserved bit mask. >> + /* Data Type */ >> + build_append_int_noprefix(table_data, numa_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, 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); >> + >> + /* Entry Base Unit */ >> + if (type <= HMAT_LB_DATA_WRITE_LATENCY) { > > I'd slightly prefer this as an explicit switch with all the elements laid out. > That would make it 'obvious' that this is all the Latency cases and the > next path is all the Bandwidth ones. > (up to you though!) > >> + build_append_int_noprefix(table_data, numa_hmat_lb->base_lat, 8); >> + } else { >> + build_append_int_noprefix(table_data, numa_hmat_lb->base_bw, 8); >> + } >> + >> + /* Initiator Proximity Domain List */ >> + for (i = 0; i < s; i++) { >> + build_append_int_noprefix(table_data, initiator_pxm[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; i++) { >> + m = initiator_pxm[i]; >> + for (n = 0; n < t; n++) { >> + uint16_t entry; >> + >> + if (type <= HMAT_LB_DATA_WRITE_LATENCY) { > As above, a switch is more code, but it's clearer. Alternative would be > a macro HMAT_IS_LATENCY() > Thank you, indeed a macro is clearer. >> + entry = numa_hmat_lb->latency[m][n]; >> + } else { >> + entry = numa_hmat_lb->bandwidth[m][n]; >> + } >> + >> + 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 num_initiator = 0; >> + uint32_t initiator_pxm[MAX_NODES]; >> + int i, hrchy, type; >> + HMAT_LB_Info *numa_hmat_lb; >> >> for (i = 0; i < nstat->num_nodes; i++) { >> flags = 0; >> @@ -82,6 +151,29 @@ static void hmat_build_table_structs(GArray *table_data, NumaState *nstat) >> >> build_hmat_mpda(table_data, flags, nstat->nodes[i].initiator, i); >> } >> + >> + for (i = 0; i < nstat->num_nodes; i++) { >> + if (nstat->nodes[i].has_cpu) { >> + initiator_pxm[num_initiator++] = i; >> + } >> + } >> + >> + /* >> + * ACPI 6.3: 5.2.27.4 System Locality Latency and Bandwidth Information >> + * Structure: Table 5-142 >> + */ >> + 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, num_initiator, >> + nstat->num_nodes, initiator_pxm, type); >> + } >> + } >> + } >> } >> >> void build_hmat(GArray *table_data, BIOSLinker *linker, NumaState *nstat) >> diff --git a/hw/acpi/hmat.h b/hw/acpi/hmat.h >> index 574cfba60a..9d5f407b8a 100644 >> --- a/hw/acpi/hmat.h >> +++ b/hw/acpi/hmat.h >> @@ -40,6 +40,45 @@ >> */ >> #define HMAT_PROX_INIT_VALID 0x1 >> >> +struct HMAT_LB_Info { >> + /* >> + * Indicates total number of Proximity Domains >> + * that can initiate memory access requests. >> + */ >> + uint32_t num_initiator; >> + /* >> + * Indicates total number of Proximity Domains >> + * that can act as target. >> + */ >> + uint32_t num_target; >> + /* >> + * 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 base unit for latency in nanoseconds. */ >> + uint64_t base_lat; >> + /* The base unit for bandwidth in megabytes per second(MB/s). */ >> + uint64_t base_bw; >> + /* >> + * latency[i][j]: >> + * Indicates the latency based on base_lat >> + * from Initiator Proximity Domain i to Target Proximity Domain j. >> + */ >> + uint16_t latency[MAX_NODES][MAX_NODES]; >> + /* >> + * bandwidth[i][j]: >> + * Indicates the bandwidth based on base_bw >> + * from Initiator Proximity Domain i to Target Proximity Domain j. >> + */ >> + uint16_t bandwidth[MAX_NODES][MAX_NODES]; >> +}; >> + >> void build_hmat(GArray *table_data, BIOSLinker *linker, NumaState *nstat); >> >> #endif >> diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h >> index fcdaae58c4..c0257e936b 100644 >> --- a/include/qemu/typedefs.h >> +++ b/include/qemu/typedefs.h >> @@ -33,6 +33,7 @@ typedef struct FWCfgEntry FWCfgEntry; >> typedef struct FWCfgIoState FWCfgIoState; >> typedef struct FWCfgMemState FWCfgMemState; >> typedef struct FWCfgState FWCfgState; >> +typedef struct HMAT_LB_Info HMAT_LB_Info; >> typedef struct HVFX86EmulatorState HVFX86EmulatorState; >> typedef struct I2CBus I2CBus; >> typedef struct I2SCodec I2SCodec; >> diff --git a/include/sysemu/numa.h b/include/sysemu/numa.h >> index 357aaeda80..0b80bc2fa2 100644 >> --- a/include/sysemu/numa.h >> +++ b/include/sysemu/numa.h >> @@ -32,6 +32,9 @@ struct NumaState { >> >> /* NUMA nodes information */ >> NodeInfo nodes[MAX_NODES]; >> + >> + /* NUMA modes HMAT Locality Latency and Bandwidth Information */ >> + HMAT_LB_Info *hmat_lb[HMAT_LB_LEVELS][HMAT_LB_TYPES]; >> }; >> typedef struct NumaState NumaState; >> >> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h >> index 61579ae71e..3f83fc0d58 100644 >> --- a/include/sysemu/sysemu.h >> +++ b/include/sysemu/sysemu.h >> @@ -124,6 +124,28 @@ extern int mem_prealloc; >> #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_LAST_LEVEL = 1, > > This changed in 6.3. There isn't a last level one now. > >> + HMAT_LB_MEM_CACHE_1ST_LEVEL = 2, >> + HMAT_LB_MEM_CACHE_2ND_LEVEL = 3, >> + HMAT_LB_MEM_CACHE_3RD_LEVEL = 4, >> +}; >> + >> +/* 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 HMAT_LB_LEVELS (HMAT_LB_MEM_CACHE_3RD_LEVEL + 1) >> +#define HMAT_LB_TYPES (HMAT_LB_DATA_WRITE_BANDWIDTH + 1) >> + >> #define MAX_OPTION_ROMS 16 >> typedef struct QEMUOptionRom { >> const char *name; > >