Patch Detail
get:
Show a patch.
patch:
Update a patch.
put:
Update a patch.
GET /api/patches/86420/?format=api
{ "id": 86420, "url": "http://patchwork.dpdk.org/api/patches/86420/?format=api", "web_url": "http://patchwork.dpdk.org/project/dpdk/patch/7f1f53ad88f1af5063f8d3e571a12a001d4ceb3b.1610473000.git.anatoly.burakov@intel.com/", "project": { "id": 1, "url": "http://patchwork.dpdk.org/api/projects/1/?format=api", "name": "DPDK", "link_name": "dpdk", "list_id": "dev.dpdk.org", "list_email": "dev@dpdk.org", "web_url": "http://core.dpdk.org", "scm_url": "git://dpdk.org/dpdk", "webscm_url": "http://git.dpdk.org/dpdk", "list_archive_url": "https://inbox.dpdk.org/dev", "list_archive_url_format": "https://inbox.dpdk.org/dev/{}", "commit_url_format": "" }, "msgid": "<7f1f53ad88f1af5063f8d3e571a12a001d4ceb3b.1610473000.git.anatoly.burakov@intel.com>", "list_archive_url": "https://inbox.dpdk.org/dev/7f1f53ad88f1af5063f8d3e571a12a001d4ceb3b.1610473000.git.anatoly.burakov@intel.com", "date": "2021-01-12T17:37:10", "name": "[v16,02/11] eal: avoid invalid API usage in power intrinsics", "commit_ref": null, "pull_url": null, "state": "superseded", "archived": true, "hash": "22809aba7f5fb732c2ee71f83b91a3826780d960", "submitter": { "id": 4, "url": "http://patchwork.dpdk.org/api/people/4/?format=api", "name": "Anatoly Burakov", "email": "anatoly.burakov@intel.com" }, "delegate": { "id": 1, "url": "http://patchwork.dpdk.org/api/users/1/?format=api", "username": "tmonjalo", "first_name": "Thomas", "last_name": "Monjalon", "email": "thomas@monjalon.net" }, "mbox": "http://patchwork.dpdk.org/project/dpdk/patch/7f1f53ad88f1af5063f8d3e571a12a001d4ceb3b.1610473000.git.anatoly.burakov@intel.com/mbox/", "series": [ { "id": 14673, "url": "http://patchwork.dpdk.org/api/series/14673/?format=api", "web_url": "http://patchwork.dpdk.org/project/dpdk/list/?series=14673", "date": "2021-01-12T17:37:08", "name": "Add PMD power management", "version": 16, "mbox": "http://patchwork.dpdk.org/series/14673/mbox/" } ], "comments": "http://patchwork.dpdk.org/api/patches/86420/comments/", "check": "success", "checks": "http://patchwork.dpdk.org/api/patches/86420/checks/", "tags": {}, "related": [], "headers": { "Return-Path": "<dev-bounces@dpdk.org>", "X-Original-To": "patchwork@inbox.dpdk.org", "Delivered-To": "patchwork@inbox.dpdk.org", "Received": [ "from mails.dpdk.org (mails.dpdk.org [217.70.189.124])\n\tby inbox.dpdk.org (Postfix) with ESMTP id DCECFA04B5;\n\tTue, 12 Jan 2021 18:37:43 +0100 (CET)", "from [217.70.189.124] (localhost [127.0.0.1])\n\tby mails.dpdk.org (Postfix) with ESMTP id 78EC8140EB3;\n\tTue, 12 Jan 2021 18:37:30 +0100 (CET)", "from mga04.intel.com (mga04.intel.com [192.55.52.120])\n by mails.dpdk.org (Postfix) with ESMTP id A50AF140EA7\n for <dev@dpdk.org>; Tue, 12 Jan 2021 18:37:28 +0100 (CET)", "from fmsmga001.fm.intel.com ([10.253.24.23])\n by fmsmga104.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384;\n 12 Jan 2021 09:37:28 -0800", "from silpixa00399498.ir.intel.com (HELO\n silpixa00399498.ger.corp.intel.com) ([10.237.222.179])\n by fmsmga001.fm.intel.com with ESMTP; 12 Jan 2021 09:37:25 -0800" ], "IronPort-SDR": [ "\n pa8KJsHhMQNCc2s0BxGuMITdhfnz6y2WXNwWDXzQsH5L6kDaijxKQaOh3qP7nOewz4dcLsHmjv\n MOyNY/Z32SZQ==", "\n arsZSoEuJloQQOLKHIGXRLU1fi/o/q/zJJqDxIwbMulmDBnFnBck3j8CBpId9lSrbqKXmlyR1h\n dCFeg6ELiRZQ==" ], "X-IronPort-AV": [ "E=McAfee;i=\"6000,8403,9862\"; a=\"175497977\"", "E=Sophos;i=\"5.79,342,1602572400\"; d=\"scan'208\";a=\"175497977\"", "E=Sophos;i=\"5.79,342,1602572400\"; d=\"scan'208\";a=\"464604165\"" ], "X-ExtLoop1": "1", "From": "Anatoly Burakov <anatoly.burakov@intel.com>", "To": "dev@dpdk.org", "Cc": "Jan Viktorin <viktorin@rehivetech.com>,\n Ruifeng Wang <ruifeng.wang@arm.com>, Jerin Jacob <jerinj@marvell.com>,\n David Christensen <drc@linux.vnet.ibm.com>,\n Bruce Richardson <bruce.richardson@intel.com>,\n Konstantin Ananyev <konstantin.ananyev@intel.com>, thomas@monjalon.net,\n timothy.mcdaniel@intel.com, david.hunt@intel.com, chris.macnamara@intel.com", "Date": "Tue, 12 Jan 2021 17:37:10 +0000", "Message-Id": "\n <7f1f53ad88f1af5063f8d3e571a12a001d4ceb3b.1610473000.git.anatoly.burakov@intel.com>", "X-Mailer": "git-send-email 2.25.1", "In-Reply-To": "<cover.1610473000.git.anatoly.burakov@intel.com>", "References": "<cover.1610377084.git.anatoly.burakov@intel.com>\n <cover.1610473000.git.anatoly.burakov@intel.com>", "MIME-Version": "1.0", "Content-Transfer-Encoding": "8bit", "Subject": "[dpdk-dev] [PATCH v16 02/11] eal: avoid invalid API usage in power\n intrinsics", "X-BeenThere": "dev@dpdk.org", "X-Mailman-Version": "2.1.29", "Precedence": "list", "List-Id": "DPDK patches and discussions <dev.dpdk.org>", "List-Unsubscribe": "<https://mails.dpdk.org/options/dev>,\n <mailto:dev-request@dpdk.org?subject=unsubscribe>", "List-Archive": "<http://mails.dpdk.org/archives/dev/>", "List-Post": "<mailto:dev@dpdk.org>", "List-Help": "<mailto:dev-request@dpdk.org?subject=help>", "List-Subscribe": "<https://mails.dpdk.org/listinfo/dev>,\n <mailto:dev-request@dpdk.org?subject=subscribe>", "Errors-To": "dev-bounces@dpdk.org", "Sender": "\"dev\" <dev-bounces@dpdk.org>" }, "content": "Currently, the API documentation mandates that if the user wants to use\nthe power management intrinsics, they need to call the\n`rte_cpu_get_intrinsics_support` API and check support for specific\nintrinsics.\n\nHowever, if the user does not do that, it is possible to get illegal\ninstruction error because we're using raw instruction opcodes, which may\nor may not be supported at runtime.\n\nNow that we have everything in a C file, we can check for support at\nstartup and prevent the user from possibly encountering illegal\ninstruction errors.\n\nWe also add return values to the API's as well, because why not.\n\nSigned-off-by: Anatoly Burakov <anatoly.burakov@intel.com>\nAcked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>\n---\n\nNotes:\n v16:\n - Add return values and proper error handling to the API\n \n v15:\n - Remove accidental whitespace changes\n \n v14:\n - Replace uint8_t with bool\n \n v14:\n - Replace uint8_t with bool\n\n lib/librte_eal/arm/rte_power_intrinsics.c | 12 +++-\n .../include/generic/rte_power_intrinsics.h | 24 +++++--\n lib/librte_eal/ppc/rte_power_intrinsics.c | 12 +++-\n lib/librte_eal/x86/rte_power_intrinsics.c | 64 +++++++++++++++++--\n 4 files changed, 94 insertions(+), 18 deletions(-)", "diff": "diff --git a/lib/librte_eal/arm/rte_power_intrinsics.c b/lib/librte_eal/arm/rte_power_intrinsics.c\nindex ab1f44f611..7e7552fa8a 100644\n--- a/lib/librte_eal/arm/rte_power_intrinsics.c\n+++ b/lib/librte_eal/arm/rte_power_intrinsics.c\n@@ -7,7 +7,7 @@\n /**\n * This function is not supported on ARM.\n */\n-void\n+int\n rte_power_monitor(const volatile void *p, const uint64_t expected_value,\n \t\tconst uint64_t value_mask, const uint64_t tsc_timestamp,\n \t\tconst uint8_t data_sz)\n@@ -17,12 +17,14 @@ rte_power_monitor(const volatile void *p, const uint64_t expected_value,\n \tRTE_SET_USED(value_mask);\n \tRTE_SET_USED(tsc_timestamp);\n \tRTE_SET_USED(data_sz);\n+\n+\treturn -ENOTSUP;\n }\n \n /**\n * This function is not supported on ARM.\n */\n-void\n+int\n rte_power_monitor_sync(const volatile void *p, const uint64_t expected_value,\n \t\tconst uint64_t value_mask, const uint64_t tsc_timestamp,\n \t\tconst uint8_t data_sz, rte_spinlock_t *lck)\n@@ -33,13 +35,17 @@ rte_power_monitor_sync(const volatile void *p, const uint64_t expected_value,\n \tRTE_SET_USED(tsc_timestamp);\n \tRTE_SET_USED(lck);\n \tRTE_SET_USED(data_sz);\n+\n+\treturn -ENOTSUP;\n }\n \n /**\n * This function is not supported on ARM.\n */\n-void\n+int\n rte_power_pause(const uint64_t tsc_timestamp)\n {\n \tRTE_SET_USED(tsc_timestamp);\n+\n+\treturn -ENOTSUP;\n }\ndiff --git a/lib/librte_eal/include/generic/rte_power_intrinsics.h b/lib/librte_eal/include/generic/rte_power_intrinsics.h\nindex 67977bd511..37e4ec0414 100644\n--- a/lib/librte_eal/include/generic/rte_power_intrinsics.h\n+++ b/lib/librte_eal/include/generic/rte_power_intrinsics.h\n@@ -34,7 +34,6 @@\n *\n * @warning It is responsibility of the user to check if this function is\n * supported at runtime using `rte_cpu_get_intrinsics_support()` API call.\n- * Failing to do so may result in an illegal CPU instruction error.\n *\n * @param p\n * Address to monitor for changes.\n@@ -50,9 +49,14 @@\n * Data size (in bytes) that will be used to compare expected value with the\n * memory address. Can be 1, 2, 4 or 8. Supplying any other value will lead\n * to undefined result.\n+ *\n+ * @return\n+ * 0 on success\n+ * -EINVAL on invalid parameters\n+ * -ENOTSUP if unsupported\n */\n __rte_experimental\n-void rte_power_monitor(const volatile void *p,\n+int rte_power_monitor(const volatile void *p,\n \t\tconst uint64_t expected_value, const uint64_t value_mask,\n \t\tconst uint64_t tsc_timestamp, const uint8_t data_sz);\n \n@@ -75,7 +79,6 @@ void rte_power_monitor(const volatile void *p,\n *\n * @warning It is responsibility of the user to check if this function is\n * supported at runtime using `rte_cpu_get_intrinsics_support()` API call.\n- * Failing to do so may result in an illegal CPU instruction error.\n *\n * @param p\n * Address to monitor for changes.\n@@ -95,9 +98,14 @@ void rte_power_monitor(const volatile void *p,\n * A spinlock that must be locked before entering the function, will be\n * unlocked while the CPU is sleeping, and will be locked again once the CPU\n * wakes up.\n+ *\n+ * @return\n+ * 0 on success\n+ * -EINVAL on invalid parameters\n+ * -ENOTSUP if unsupported\n */\n __rte_experimental\n-void rte_power_monitor_sync(const volatile void *p,\n+int rte_power_monitor_sync(const volatile void *p,\n \t\tconst uint64_t expected_value, const uint64_t value_mask,\n \t\tconst uint64_t tsc_timestamp, const uint8_t data_sz,\n \t\trte_spinlock_t *lck);\n@@ -111,13 +119,17 @@ void rte_power_monitor_sync(const volatile void *p,\n *\n * @warning It is responsibility of the user to check if this function is\n * supported at runtime using `rte_cpu_get_intrinsics_support()` API call.\n- * Failing to do so may result in an illegal CPU instruction error.\n *\n * @param tsc_timestamp\n * Maximum TSC timestamp to wait for. Note that the wait behavior is\n * architecture-dependent.\n+ *\n+ * @return\n+ * 0 on success\n+ * -EINVAL on invalid parameters\n+ * -ENOTSUP if unsupported\n */\n __rte_experimental\n-void rte_power_pause(const uint64_t tsc_timestamp);\n+int rte_power_pause(const uint64_t tsc_timestamp);\n \n #endif /* _RTE_POWER_INTRINSIC_H_ */\ndiff --git a/lib/librte_eal/ppc/rte_power_intrinsics.c b/lib/librte_eal/ppc/rte_power_intrinsics.c\nindex 84340ca2a4..929e0611b0 100644\n--- a/lib/librte_eal/ppc/rte_power_intrinsics.c\n+++ b/lib/librte_eal/ppc/rte_power_intrinsics.c\n@@ -7,7 +7,7 @@\n /**\n * This function is not supported on PPC64.\n */\n-void\n+int\n rte_power_monitor(const volatile void *p, const uint64_t expected_value,\n \t\tconst uint64_t value_mask, const uint64_t tsc_timestamp,\n \t\tconst uint8_t data_sz)\n@@ -17,12 +17,14 @@ rte_power_monitor(const volatile void *p, const uint64_t expected_value,\n \tRTE_SET_USED(value_mask);\n \tRTE_SET_USED(tsc_timestamp);\n \tRTE_SET_USED(data_sz);\n+\n+\treturn -ENOTSUP;\n }\n \n /**\n * This function is not supported on PPC64.\n */\n-void\n+int\n rte_power_monitor_sync(const volatile void *p, const uint64_t expected_value,\n \t\tconst uint64_t value_mask, const uint64_t tsc_timestamp,\n \t\tconst uint8_t data_sz, rte_spinlock_t *lck)\n@@ -33,13 +35,17 @@ rte_power_monitor_sync(const volatile void *p, const uint64_t expected_value,\n \tRTE_SET_USED(tsc_timestamp);\n \tRTE_SET_USED(lck);\n \tRTE_SET_USED(data_sz);\n+\n+\treturn -ENOTSUP;\n }\n \n /**\n * This function is not supported on PPC64.\n */\n-void\n+int\n rte_power_pause(const uint64_t tsc_timestamp)\n {\n \tRTE_SET_USED(tsc_timestamp);\n+\n+\treturn -ENOTSUP;\n }\ndiff --git a/lib/librte_eal/x86/rte_power_intrinsics.c b/lib/librte_eal/x86/rte_power_intrinsics.c\nindex 34c5fd9c3e..2a38440bec 100644\n--- a/lib/librte_eal/x86/rte_power_intrinsics.c\n+++ b/lib/librte_eal/x86/rte_power_intrinsics.c\n@@ -4,6 +4,8 @@\n \n #include \"rte_power_intrinsics.h\"\n \n+static bool wait_supported;\n+\n static inline uint64_t\n __get_umwait_val(const volatile void *p, const uint8_t sz)\n {\n@@ -17,24 +19,47 @@ __get_umwait_val(const volatile void *p, const uint8_t sz)\n \tcase sizeof(uint64_t):\n \t\treturn *(const volatile uint64_t *)p;\n \tdefault:\n-\t\t/* this is an intrinsic, so we can't have any error handling */\n+\t\t/* shouldn't happen */\n \t\tRTE_ASSERT(0);\n \t\treturn 0;\n \t}\n }\n \n+static inline int\n+__check_val_size(const uint8_t sz)\n+{\n+\tswitch (sz) {\n+\tcase sizeof(uint8_t): /* fall-through */\n+\tcase sizeof(uint16_t): /* fall-through */\n+\tcase sizeof(uint32_t): /* fall-through */\n+\tcase sizeof(uint64_t): /* fall-through */\n+\t\treturn 0;\n+\tdefault:\n+\t\t/* unexpected size */\n+\t\treturn -1;\n+\t}\n+}\n+\n /**\n * This function uses UMONITOR/UMWAIT instructions and will enter C0.2 state.\n * For more information about usage of these instructions, please refer to\n * Intel(R) 64 and IA-32 Architectures Software Developer's Manual.\n */\n-void\n+int\n rte_power_monitor(const volatile void *p, const uint64_t expected_value,\n \t\tconst uint64_t value_mask, const uint64_t tsc_timestamp,\n \t\tconst uint8_t data_sz)\n {\n \tconst uint32_t tsc_l = (uint32_t)tsc_timestamp;\n \tconst uint32_t tsc_h = (uint32_t)(tsc_timestamp >> 32);\n+\n+\t/* prevent user from running this instruction if it's not supported */\n+\tif (!wait_supported)\n+\t\treturn -ENOTSUP;\n+\n+\tif (__check_val_size(data_sz) < 0)\n+\t\treturn -EINVAL;\n+\n \t/*\n \t * we're using raw byte codes for now as only the newest compiler\n \t * versions support this instruction natively.\n@@ -51,13 +76,15 @@ rte_power_monitor(const volatile void *p, const uint64_t expected_value,\n \n \t\t/* if the masked value is already matching, abort */\n \t\tif (masked == expected_value)\n-\t\t\treturn;\n+\t\t\treturn 0;\n \t}\n \t/* execute UMWAIT */\n \tasm volatile(\".byte 0xf2, 0x0f, 0xae, 0xf7;\"\n \t\t\t: /* ignore rflags */\n \t\t\t: \"D\"(0), /* enter C0.2 */\n \t\t\t \"a\"(tsc_l), \"d\"(tsc_h));\n+\n+\treturn 0;\n }\n \n /**\n@@ -65,13 +92,21 @@ rte_power_monitor(const volatile void *p, const uint64_t expected_value,\n * For more information about usage of these instructions, please refer to\n * Intel(R) 64 and IA-32 Architectures Software Developer's Manual.\n */\n-void\n+int\n rte_power_monitor_sync(const volatile void *p, const uint64_t expected_value,\n \t\tconst uint64_t value_mask, const uint64_t tsc_timestamp,\n \t\tconst uint8_t data_sz, rte_spinlock_t *lck)\n {\n \tconst uint32_t tsc_l = (uint32_t)tsc_timestamp;\n \tconst uint32_t tsc_h = (uint32_t)(tsc_timestamp >> 32);\n+\n+\t/* prevent user from running this instruction if it's not supported */\n+\tif (!wait_supported)\n+\t\treturn -ENOTSUP;\n+\n+\tif (__check_val_size(data_sz) < 0)\n+\t\treturn -EINVAL;\n+\n \t/*\n \t * we're using raw byte codes for now as only the newest compiler\n \t * versions support this instruction natively.\n@@ -88,7 +123,7 @@ rte_power_monitor_sync(const volatile void *p, const uint64_t expected_value,\n \n \t\t/* if the masked value is already matching, abort */\n \t\tif (masked == expected_value)\n-\t\t\treturn;\n+\t\t\treturn 0;\n \t}\n \trte_spinlock_unlock(lck);\n \n@@ -99,6 +134,8 @@ rte_power_monitor_sync(const volatile void *p, const uint64_t expected_value,\n \t\t\t \"a\"(tsc_l), \"d\"(tsc_h));\n \n \trte_spinlock_lock(lck);\n+\n+\treturn 0;\n }\n \n /**\n@@ -106,15 +143,30 @@ rte_power_monitor_sync(const volatile void *p, const uint64_t expected_value,\n * information about usage of this instruction, please refer to Intel(R) 64 and\n * IA-32 Architectures Software Developer's Manual.\n */\n-void\n+int\n rte_power_pause(const uint64_t tsc_timestamp)\n {\n \tconst uint32_t tsc_l = (uint32_t)tsc_timestamp;\n \tconst uint32_t tsc_h = (uint32_t)(tsc_timestamp >> 32);\n \n+\t/* prevent user from running this instruction if it's not supported */\n+\tif (!wait_supported)\n+\t\treturn -ENOTSUP;\n+\n \t/* execute TPAUSE */\n \tasm volatile(\".byte 0x66, 0x0f, 0xae, 0xf7;\"\n \t\t\t: /* ignore rflags */\n \t\t\t: \"D\"(0), /* enter C0.2 */\n \t\t\t\"a\"(tsc_l), \"d\"(tsc_h));\n+\n+\treturn 0;\n+}\n+\n+RTE_INIT(rte_power_intrinsics_init) {\n+\tstruct rte_cpu_intrinsics i;\n+\n+\trte_cpu_get_intrinsics_support(&i);\n+\n+\tif (i.power_monitor && i.power_pause)\n+\t\twait_supported = 1;\n }\n", "prefixes": [ "v16", "02/11" ] }