Re: [PATCH v2] Patch introducing API to read/write Intel Architecture Model Specific Registers (MSR)...
From: Ananyev, Konstantin <hidden>
Date: 2016-01-20 11:25:56
Hi Wojciech, Couple of nits, see below. Konstantin
quoted hunk ↗ jump to hunk
-----Original Message----- From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Wojciech Andralojc Sent: Wednesday, January 20, 2016 10:57 AM To: dev@dpdk.org Subject: [dpdk-dev] [PATCH v2] Patch introducing API to read/write Intel Architecture Model Specific Registers (MSR)... Patch rework based on feedback, only x86 specific functions left under lib/librte_eal/common/include/arch/x86/. Signed-off-by: Wojciech Andralojc <redacted> --- lib/librte_eal/common/include/arch/x86/rte_msr.h | 158 +++++++++++++++++++++++ 1 file changed, 158 insertions(+) create mode 100644 lib/librte_eal/common/include/arch/x86/rte_msr.hdiff --git a/lib/librte_eal/common/include/arch/x86/rte_msr.h b/lib/librte_eal/common/include/arch/x86/rte_msr.h new file mode 100644 index 0000000..9d16633 --- /dev/null +++ b/lib/librte_eal/common/include/arch/x86/rte_msr.h + +#ifndef _RTE_MSR_X86_64_H_ +#define _RTE_MSR_X86_64_H_ + +#ifdef __cplusplus +extern "C" { +#endif + +#include <fcntl.h> //O_RDONLY +#include <unistd.h> //pread
Pls remove '//' comments here.
+
+#include <rte_debug.h>
+#include <rte_log.h>
+
+#define CPU_MSR_PATH "/dev/cpu/%u/msr"
+#define CPU_MSR_PATH_MAX_LEN 32
+
+/**
+ * This function should not be called directly.
+ * Function to open CPU's MSR file
+ */
+static int
+__msr_open_file(const unsigned lcore, int flags)
+{
+ char fname[CPU_MSR_PATH_MAX_LEN] = {0};Why not just use PATH_MAX here?
+ int fd = -1; + + snprintf(fname, sizeof(fname) - 1, CPU_MSR_PATH, lcore); + + fd = open(fname, flags); + + if (fd < 0) + RTE_LOG(ERR, PQOS, "Error opening file '%s'!\n", fname); + + return fd; +} + +/** + * Function to read CPU's MSR + * + * @param [in] lcore + * CPU logical core id
Hmm, are you aware that DPDK lcore id != CPU lcore id? Might be better to use 'cpuid' name here? Just to avoid confusion.
+ * + * @param [in] reg + * MSR reg to read + * + * @param [out] value + * Read value of MSR reg + * + * @return + * Operations status +*/ + +static inline int +rte_msr_read(const unsigned lcore, const uint32_t reg, uint64_t *value)
I don't think there is a need to put rte_msr_read/rte_msr_write() Definition into a header file and make them static inline. Just normal external function definition seems sufficient here.
+{
+ int fd = -1;
+ int ret = -1;
+
+ RTE_VERIFY(value != NULL);That's a a public API. No need to coredump if one of the input parameters is invalid.
+ if (value == NULL) + return -1;
Might be better -EINVAL;
+
+ fd = __msr_open_file(lcore, O_RDONLY);
+
+ if (fd >= 0) {
+ ssize_t read_ret = 0;
+
+ read_ret = pread(fd, value, sizeof(value[0]), (off_t)reg);
+
+ if (read_ret != sizeof(value[0])) {
+ RTE_LOG(ERR, PQOS, "RDMSR failed for reg[0x%x] on lcore %u\n",
+ (unsigned)reg, lcore);
+ } else
+ ret = 0;
+
+ close(fd);
+ }
+
+ return ret;
+}