[dpdk-dev] [PATCH] Patch introducing API to read/write Intel Architecture Model Specific Registers (MSR), rte_msr_read and rte_msr_write functions.

[dpdk-dev] [PATCH] Patch introducing API to read/write Intel Architecture Model Specific Registers (MSR), rte_msr_read and rte_msr_write functions.

[Wojciech Andralojc]

There is work in progress to implement Intel Cache Allocation Technology (CAT) support in DPDK, this technology is programmed through MSRs.
In the future it will be possible to program CAT through Linux cgroups and DPDK CAT implementation will take advantage of it.

MSR R/W's are privileged ring 0 operations and they must be done in kernel space. For this reason implementation utilizes Linux MSR driver.

Signed-off-by: Wojciech Andralojc <wojciechx.andralojc@intel.com>
---
 lib/librte_eal/common/Makefile                     |   1 +
 lib/librte_eal/common/include/arch/arm/rte_msr.h   |  65 ++++++++++
 .../common/include/arch/ppc_64/rte_msr.h           |  65 ++++++++++
 lib/librte_eal/common/include/arch/tile/rte_msr.h  |  65 ++++++++++
 lib/librte_eal/common/include/arch/x86/rte_msr.h   | 143 +++++++++++++++++++++
 lib/librte_eal/common/include/generic/rte_msr.h    |  78 +++++++++++
 lib/librte_eal/common/include/rte_lcore.h          |  18 +++
 7 files changed, 435 insertions(+)
 create mode 100644 lib/librte_eal/common/include/arch/arm/rte_msr.h
 create mode 100644 lib/librte_eal/common/include/arch/ppc_64/rte_msr.h
 create mode 100644 lib/librte_eal/common/include/arch/tile/rte_msr.h
 create mode 100644 lib/librte_eal/common/include/arch/x86/rte_msr.h
 create mode 100644 lib/librte_eal/common/include/generic/rte_msr.h

diff --git a/lib/librte_eal/common/Makefile b/lib/librte_eal/common/Makefile
index f5ea0ee..567c206 100644
--- a/lib/librte_eal/common/Makefile
+++ b/lib/librte_eal/common/Makefile
@@ -48,6 +48,7 @@ endif
 
 GENERIC_INC := rte_atomic.h rte_byteorder.h rte_cycles.h rte_prefetch.h
 GENERIC_INC += rte_spinlock.h rte_memcpy.h rte_cpuflags.h rte_rwlock.h
+GENERIC_INC += rte_msr.h
 # defined in mk/arch/$(RTE_ARCH)/rte.vars.mk
 ARCH_DIR ?= $(RTE_ARCH)
 ARCH_INC := $(notdir $(wildcard $(RTE_SDK)/lib/librte_eal/common/include/arch/$(ARCH_DIR)/*.h))
diff --git a/lib/librte_eal/common/include/arch/arm/rte_msr.h b/lib/librte_eal/common/include/arch/arm/rte_msr.h
new file mode 100644
index 0000000..85c009c
--- /dev/null
+++ b/lib/librte_eal/common/include/arch/arm/rte_msr.h
@@ -0,0 +1,65 @@
+/*-
+ *   BSD LICENSE
+ *
+ *   Copyright(c) 2015 Intel Corporation. All rights reserved.
+ *   All rights reserved.
+ *
+ *   Redistribution and use in source and binary forms, with or without
+ *   modification, are permitted provided that the following conditions
+ *   are met:
+ *
+ *     * Redistributions of source code must retain the above copyright
+ *       notice, this list of conditions and the following disclaimer.
+ *     * Redistributions in binary form must reproduce the above copyright
+ *       notice, this list of conditions and the following disclaimer in
+ *       the documentation and/or other materials provided with the
+ *       distribution.
+ *     * Neither the name of Intel Corporation nor the names of its
+ *       contributors may be used to endorse or promote products derived
+ *       from this software without specific prior written permission.
+ *
+ *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#ifndef _RTE_MSR_ARM_H_
+#define _RTE_MSR_ARM_H_
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#include "generic/rte_msr.h"
+
+/* Function to read CPU's MSR */
+static inline int
+rte_msr_read(__attribute__((unused)) const unsigned lcore,
+		__attribute__((unused)) const uint32_t reg,
+		__attribute__((unused)) uint64_t *value)
+{
+	return -1;
+}
+
+/* Function to write CPU's MSR */
+static inline int
+rte_msr_write(__attribute__((unused)) const unsigned lcore,
+		__attribute__((unused)) const uint32_t reg,
+		__attribute__((unused)) const uint64_t value)
+{
+	return -1;
+}
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* _RTE_MSR_ARM_H_ */
diff --git a/lib/librte_eal/common/include/arch/ppc_64/rte_msr.h b/lib/librte_eal/common/include/arch/ppc_64/rte_msr.h
new file mode 100644
index 0000000..44f3de2
--- /dev/null
+++ b/lib/librte_eal/common/include/arch/ppc_64/rte_msr.h
@@ -0,0 +1,65 @@
+/*-
+ *   BSD LICENSE
+ *
+ *   Copyright(c) 2015 Intel Corporation. All rights reserved.
+ *   All rights reserved.
+ *
+ *   Redistribution and use in source and binary forms, with or without
+ *   modification, are permitted provided that the following conditions
+ *   are met:
+ *
+ *     * Redistributions of source code must retain the above copyright
+ *       notice, this list of conditions and the following disclaimer.
+ *     * Redistributions in binary form must reproduce the above copyright
+ *       notice, this list of conditions and the following disclaimer in
+ *       the documentation and/or other materials provided with the
+ *       distribution.
+ *     * Neither the name of Intel Corporation nor the names of its
+ *       contributors may be used to endorse or promote products derived
+ *       from this software without specific prior written permission.
+ *
+ *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#ifndef _RTE_MSR_PPC_64_H_
+#define _RTE_MSR_PPC_64_H_
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#include "generic/rte_msr.h"
+
+/* Function to read CPU's MSR */
+static inline int
+rte_msr_read(__attribute__((unused)) const unsigned lcore,
+		__attribute__((unused)) const uint32_t reg,
+		__attribute__((unused)) uint64_t *value)
+{
+	return -1;
+}
+
+/* Function to write CPU's MSR */
+static inline int
+rte_msr_write(__attribute__((unused)) const unsigned lcore,
+		__attribute__((unused)) const uint32_t reg,
+		__attribute__((unused)) const uint64_t value)
+{
+	return -1;
+}
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* _RTE_MSR_PPC_64_H_ */
diff --git a/lib/librte_eal/common/include/arch/tile/rte_msr.h b/lib/librte_eal/common/include/arch/tile/rte_msr.h
new file mode 100644
index 0000000..8a446f7
--- /dev/null
+++ b/lib/librte_eal/common/include/arch/tile/rte_msr.h
@@ -0,0 +1,65 @@
+/*-
+ *   BSD LICENSE
+ *
+ *   Copyright(c) 2015 Intel Corporation. All rights reserved.
+ *   All rights reserved.
+ *
+ *   Redistribution and use in source and binary forms, with or without
+ *   modification, are permitted provided that the following conditions
+ *   are met:
+ *
+ *     * Redistributions of source code must retain the above copyright
+ *       notice, this list of conditions and the following disclaimer.
+ *     * Redistributions in binary form must reproduce the above copyright
+ *       notice, this list of conditions and the following disclaimer in
+ *       the documentation and/or other materials provided with the
+ *       distribution.
+ *     * Neither the name of Intel Corporation nor the names of its
+ *       contributors may be used to endorse or promote products derived
+ *       from this software without specific prior written permission.
+ *
+ *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#ifndef _RTE_MSR_TILE_H_
+#define _RTE_MSR_TILE_H_
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#include "generic/rte_msr.h"
+
+/* Function to read CPU's MSR */
+static inline int
+rte_msr_read(__attribute__((unused)) const unsigned lcore,
+		__attribute__((unused)) const uint32_t reg,
+		__attribute__((unused)) uint64_t *value)
+{
+	return -1;
+}
+
+/* Function to write CPU's MSR */
+static inline int
+rte_msr_write(__attribute__((unused)) const unsigned lcore,
+		__attribute__((unused)) const uint32_t reg,
+		__attribute__((unused)) const uint64_t value)
+{
+	return -1;
+}
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* _RTE_MSR_TILE_H_ */
diff --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..7632208
--- /dev/null
+++ b/lib/librte_eal/common/include/arch/x86/rte_msr.h
@@ -0,0 +1,143 @@
+/*-
+ *   BSD LICENSE
+ *
+ *   Copyright(c) 2015 Intel Corporation. All rights reserved.
+ *   All rights reserved.
+ *
+ *   Redistribution and use in source and binary forms, with or without
+ *   modification, are permitted provided that the following conditions
+ *   are met:
+ *
+ *     * Redistributions of source code must retain the above copyright
+ *       notice, this list of conditions and the following disclaimer.
+ *     * Redistributions in binary form must reproduce the above copyright
+ *       notice, this list of conditions and the following disclaimer in
+ *       the documentation and/or other materials provided with the
+ *       distribution.
+ *     * Neither the name of Intel Corporation nor the names of its
+ *       contributors may be used to endorse or promote products derived
+ *       from this software without specific prior written permission.
+ *
+ *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#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 */
+
+#include <rte_debug.h>
+#include <rte_lcore.h>
+#include <rte_log.h>
+
+#include "generic/rte_msr.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)
+{
+	RTE_VERIFY(rte_lcore_is_detected(lcore) == 1);
+	if (rte_lcore_is_detected(lcore) != 1)
+		return -1;
+
+	int fd = -1;
+
+	char fname[CPU_MSR_PATH_MAX_LEN];
+
+	memset(fname, 0, sizeof(fname));
+	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 */
+static inline int
+rte_msr_read(const unsigned lcore, const uint32_t reg, uint64_t *value)
+{
+	RTE_VERIFY(value != NULL);
+	RTE_VERIFY(rte_lcore_is_detected(lcore) == 1);
+	if (value == NULL || rte_lcore_is_detected(lcore) != 1)
+		return -1;
+
+	int ret = -1;
+	int fd = -1;
+
+	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;
+}
+
+/* Function to write CPU's MSR */
+static inline int
+rte_msr_write(const unsigned lcore, const uint32_t reg, const uint64_t value)
+{
+	RTE_VERIFY(rte_lcore_is_detected(lcore) == 1);
+	if (rte_lcore_is_detected(lcore) != 1)
+		return -1;
+
+	int ret = -1;
+	int fd = -1;
+
+	fd = __msr_open_file(lcore, O_WRONLY);
+
+	if (fd >= 0) {
+		ssize_t write_ret = 0;
+
+		write_ret = pwrite(fd, &value, sizeof(value), (off_t)reg);
+		if (write_ret != sizeof(value)) {
+			RTE_LOG(ERR, PQOS, "WRMSR failed for reg[0x%x] <- value[0x%llx] on "
+					"lcore %u\n", (unsigned)reg, (unsigned long long)value, lcore);
+		} else
+			ret = 0;
+
+		close(fd);
+	}
+
+	return ret;
+}
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* _RTE_MSR_X86_64_H_ */
diff --git a/lib/librte_eal/common/include/generic/rte_msr.h b/lib/librte_eal/common/include/generic/rte_msr.h
new file mode 100644
index 0000000..62f0530
--- /dev/null
+++ b/lib/librte_eal/common/include/generic/rte_msr.h
@@ -0,0 +1,78 @@
+/*-
+ *   BSD LICENSE
+ *
+ *   Copyright(c) 2015 Intel Corporation. All rights reserved.
+ *   All rights reserved.
+ *
+ *   Redistribution and use in source and binary forms, with or without
+ *   modification, are permitted provided that the following conditions
+ *   are met:
+ *
+ *     * Redistributions of source code must retain the above copyright
+ *       notice, this list of conditions and the following disclaimer.
+ *     * Redistributions in binary form must reproduce the above copyright
+ *       notice, this list of conditions and the following disclaimer in
+ *       the documentation and/or other materials provided with the
+ *       distribution.
+ *     * Neither the name of Intel Corporation nor the names of its
+ *       contributors may be used to endorse or promote products derived
+ *       from this software without specific prior written permission.
+ *
+ *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#ifndef _RTE_MSR_H_
+#define _RTE_MSR_H_
+
+/**
+ * @file
+ * Architecture specific API to read/write CPU's MSR.
+ */
+
+#include <stdint.h>
+
+/**
+ * Function to read CPU's MSR
+ *
+ * @param [in] lcore
+ *  CPU logical core id
+ *
+ * @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);
+
+/**
+ * Function to write CPU's MSR
+ *
+ * @param [in] lcore
+ *  CPU logical core id
+ *
+ * @param [in] reg
+ *  MSR reg to write
+ *
+ * @param [out] value
+ *  Value to be written to MSR reg
+ *
+ * @return
+ *  Operations status
+ */
+static inline int rte_msr_write(const unsigned lcore, const uint32_t reg, const uint64_t value);
+
+#endif /* _RTE_MSR_H_ */
diff --git a/lib/librte_eal/common/include/rte_lcore.h b/lib/librte_eal/common/include/rte_lcore.h
index 25460b9..1b72d36 100644
--- a/lib/librte_eal/common/include/rte_lcore.h
+++ b/lib/librte_eal/common/include/rte_lcore.h
@@ -268,6 +268,24 @@ void rte_thread_get_affinity(rte_cpuset_t *cpusetp);
 #endif
 #endif
 
+/**
+ * Test if lcore is detected lcore in system.
+ *
+ * @param lcore_id
+ *   The identifier of the lcore, which MUST be between 0 and
+ *   RTE_MAX_LCORE-1.
+ * @return
+ *   True if the given lcore is detected; false otherwise.
+ */
+static inline unsigned
+rte_lcore_is_detected(const unsigned lcore_id)
+{
+    if (lcore_id >= RTE_MAX_LCORE)
+	return 0;
+
+    return lcore_config[lcore_id].detected;
+}
+
 #ifdef __cplusplus
 }
 #endif
-- 
1.9.3

--------------------------------------------------------------
Intel Research and Development Ireland Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263


This e-mail and any attachments may contain confidential material for the sole
use of the intended recipient(s). Any review or distribution by others is
strictly prohibited. If you are not the intended recipient, please contact the
sender and delete all copies.

Re: [dpdk-dev] [PATCH] Patch introducing API to read/write Intel Architecture Model Specific Registers (MSR), rte_msr_read and rte_msr_write functions.

[Jerin Jacob]

On Wed, Jan 06, 2016 at 11:47:28AM +0000, Andralojc, WojciechX wrote:
> > From: Andralojc, WojciechX
> > Sent: Thursday, December 17, 2015 12:13 PM
> > To: dev@dpdk.org
> > Cc: Andralojc, WojciechX
> > Subject: [PATCH] Patch introducing API to read/write Intel Architecture Model
> > Specific Registers (MSR), rte_msr_read and rte_msr_write functions.
> > 
> > There is work in progress to implement Intel Cache Allocation Technology (CAT)
> > support in DPDK, this technology is programmed through MSRs.
> > In the future it will be possible to program CAT through Linux cgroups and DPDK
> > CAT implementation will take advantage of it.
> > 
> > MSR R/W's are privileged ring 0 operations and they must be done in kernel
> > space. For this reason implementation utilizes Linux MSR driver.
> > 
> > Signed-off-by: Wojciech Andralojc <wojciechx.andralojc@intel.com>
> 
> I've got suggestion offline that as MSRs are IA specific,
> I should not give the dummy APIs for the other arches
> and move MSR access functions into the EAL specific APIs
> or some place more arch specific. 
> Do you find submitted MSR patch OK?
> or do you agree with the above feedback and patch should be re-worked?

+1

IMO, No need to expose this function as EAL as other archs can't
implement this.I think, a IA specific function under
lib/librte_eal/common/include/arch/x86/ and removing rte_* from
internal architecture functions looks more appropriate

Jerin

> I am looking forward to your feedback
> 
> Thank you!
> 
> Wojciech Andralojc
> --------------------------------------------------------------
> Intel Research and Development Ireland Limited
> Registered in Ireland
> Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
> Registered Number: 308263
> 
> 
> This e-mail and any attachments may contain confidential material for the sole
> use of the intended recipient(s). Any review or distribution by others is
> strictly prohibited. If you are not the intended recipient, please contact the
> sender and delete all copies.
> 

[dpdk-dev] [PATCH v2] Patch introducing API to read/write Intel Architecture Model Specific Registers (MSR)...

[Wojciech Andralojc]

Patch rework based on feedback, only x86 specific functions left under lib/librte_eal/common/include/arch/x86/.

Signed-off-by: Wojciech Andralojc <wojciechx.andralojc@intel.com>
---
 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.h

diff --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
@@ -0,0 +1,158 @@
+/*-
+ *   BSD LICENSE
+ *
+ *   Copyright(c) 2016 Intel Corporation. All rights reserved.
+ *   All rights reserved.
+ *
+ *   Redistribution and use in source and binary forms, with or without
+ *   modification, are permitted provided that the following conditions
+ *   are met:
+ *
+ *     * Redistributions of source code must retain the above copyright
+ *       notice, this list of conditions and the following disclaimer.
+ *     * Redistributions in binary form must reproduce the above copyright
+ *       notice, this list of conditions and the following disclaimer in
+ *       the documentation and/or other materials provided with the
+ *       distribution.
+ *     * Neither the name of Intel Corporation nor the names of its
+ *       contributors may be used to endorse or promote products derived
+ *       from this software without specific prior written permission.
+ *
+ *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#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
+
+#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};
+	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
+ *
+ * @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)
+{
+	int fd = -1;
+	int ret = -1;
+
+	RTE_VERIFY(value != NULL);
+	if (value == NULL)
+		return -1;
+
+	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;
+}
+
+/**
+ * Function to write CPU's MSR
+ *
+ * @param [in] lcore
+ *  CPU logical core id
+ *
+ * @param [in] reg
+ *  MSR reg to write
+ *
+ * @param [in] value
+ *  Value to be written to MSR reg
+ *
+ * @return
+ *  Operations status
+*/
+static inline int
+rte_msr_write(const unsigned lcore, const uint32_t reg, const uint64_t value)
+{
+	int fd = -1;
+	int ret = -1;
+
+	fd = __msr_open_file(lcore, O_WRONLY);
+
+	if (fd >= 0) {
+		ssize_t write_ret = 0;
+
+		write_ret = pwrite(fd, &value, sizeof(value), (off_t)reg);
+		if (write_ret != sizeof(value)) {
+			RTE_LOG(ERR, PQOS, "WRMSR failed for reg[0x%x] <- value[0x%llx] on "
+					"lcore %u\n", (unsigned)reg, (unsigned long long)value, lcore);
+		} else
+			ret = 0;
+
+		close(fd);
+	}
+
+	return ret;
+}
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* _RTE_MSR_X86_64_H_ */
-- 
1.9.3

Re: [dpdk-dev] [PATCH v2] Patch introducing API to read/write Intel Architecture Model Specific Registers (MSR)...

[Ananyev, Konstantin]

Hi Wojciech,
Couple of nits, see below.
Konstantin

> -----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 <wojciechx.andralojc@intel.com>
> ---
>  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.h
> 
> diff --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;
> +}

[dpdk-dev] [PATCH v3] Patch introducing API to read/write Intel Architecture Model Specific Registers (MSR)...

[Wojciech Andralojc]

Patch reworked.

Signed-off-by: Wojciech Andralojc <wojciechx.andralojc@intel.com>
---
 lib/librte_eal/common/include/arch/x86/rte_msr.h |  88 +++++++++++++++++
 lib/librte_eal/linuxapp/eal/Makefile             |   1 +
 lib/librte_eal/linuxapp/eal/arch/x86/rte_msr.c   | 116 +++++++++++++++++++++++
 3 files changed, 205 insertions(+)
 create mode 100644 lib/librte_eal/common/include/arch/x86/rte_msr.h
 create mode 100644 lib/librte_eal/linuxapp/eal/arch/x86/rte_msr.c

diff --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..fc49107
--- /dev/null
+++ b/lib/librte_eal/common/include/arch/x86/rte_msr.h
@@ -0,0 +1,88 @@
+/*-
+ *   BSD LICENSE
+ *
+ *   Copyright(c) 2016 Intel Corporation. All rights reserved.
+ *   All rights reserved.
+ *
+ *   Redistribution and use in source and binary forms, with or without
+ *   modification, are permitted provided that the following conditions
+ *   are met:
+ *
+ *     * Redistributions of source code must retain the above copyright
+ *       notice, this list of conditions and the following disclaimer.
+ *     * Redistributions in binary form must reproduce the above copyright
+ *       notice, this list of conditions and the following disclaimer in
+ *       the documentation and/or other materials provided with the
+ *       distribution.
+ *     * Neither the name of Intel Corporation nor the names of its
+ *       contributors may be used to endorse or promote products derived
+ *       from this software without specific prior written permission.
+ *
+ *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#ifndef _RTE_MSR_X86_64_H_
+#define _RTE_MSR_X86_64_H_
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/**
+ * @file
+ *
+ * API to read/write Intel Architecture Model Specific Registers (MSR).
+ *
+ */
+
+/**
+ * Function to read CPU's MSR
+ *
+ * @param [in] cpuid
+ *  CPU logical core id
+ *
+ * @param [in] reg
+ *  MSR reg to read
+ *
+ * @param [out] value
+ *  Read value of MSR reg
+ *
+ * @return
+ *  Operations status
+*/
+extern int rte_msr_read(const unsigned cpuid, const uint32_t reg,
+		uint64_t *value);
+
+/**
+ * Function to write CPU's MSR
+ *
+ * @param [in] cpuid
+ *  CPU logical core id
+ *
+ * @param [in] reg
+ *  MSR reg to write
+ *
+ * @param [in] value
+ *  Value to be written to MSR reg
+ *
+ * @return
+ *  Operations status
+*/
+extern int rte_msr_write(const unsigned cpuid, const uint32_t reg,
+		const uint64_t value);
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* _RTE_MSR_X86_64_H_ */
diff --git a/lib/librte_eal/linuxapp/eal/Makefile b/lib/librte_eal/linuxapp/eal/Makefile
index 26eced5..4b6047f 100644
--- a/lib/librte_eal/linuxapp/eal/Makefile
+++ b/lib/librte_eal/linuxapp/eal/Makefile
@@ -68,6 +68,7 @@ SRCS-$(CONFIG_RTE_LIBRTE_EAL_LINUXAPP) += eal_alarm.c
 ifeq ($(CONFIG_RTE_LIBRTE_IVSHMEM),y)
 SRCS-$(CONFIG_RTE_LIBRTE_EAL_LINUXAPP) += eal_ivshmem.c
 endif
+SRCS-$(CONFIG_RTE_LIBRTE_EAL_LINUXAPP) += ./arch/x86/rte_msr.c
 
 # from common dir
 SRCS-$(CONFIG_RTE_LIBRTE_EAL_LINUXAPP) += eal_common_lcore.c
diff --git a/lib/librte_eal/linuxapp/eal/arch/x86/rte_msr.c b/lib/librte_eal/linuxapp/eal/arch/x86/rte_msr.c
new file mode 100644
index 0000000..a702b6c
--- /dev/null
+++ b/lib/librte_eal/linuxapp/eal/arch/x86/rte_msr.c
@@ -0,0 +1,116 @@
+/*-
+ *   BSD LICENSE
+ *
+ *   Copyright(c) 2016 Intel Corporation. All rights reserved.
+ *   All rights reserved.
+ *
+ *   Redistribution and use in source and binary forms, with or without
+ *   modification, are permitted provided that the following conditions
+ *   are met:
+ *
+ *     * Redistributions of source code must retain the above copyright
+ *       notice, this list of conditions and the following disclaimer.
+ *     * Redistributions in binary form must reproduce the above copyright
+ *       notice, this list of conditions and the following disclaimer in
+ *       the documentation and/or other materials provided with the
+ *       distribution.
+ *     * Neither the name of Intel Corporation nor the names of its
+ *       contributors may be used to endorse or promote products derived
+ *       from this software without specific prior written permission.
+ *
+ *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#include <errno.h>
+#include <fcntl.h>
+#include <limits.h>
+#include <unistd.h>
+
+#include <rte_debug.h>
+#include <rte_log.h>
+
+#include <rte_msr.h>
+
+#define CPU_MSR_PATH "/dev/cpu/%u/msr"
+
+/**
+ * Function to open CPU's MSR file
+ */
+static int
+__msr_open_file(const unsigned cpuid, int flags)
+{
+	char fname[PATH_MAX] = {0};
+	int fd = -1;
+
+	snprintf(fname, sizeof(fname) - 1, CPU_MSR_PATH, cpuid);
+
+	fd = open(fname, flags);
+
+	if (fd < 0)
+		RTE_LOG(ERR, EAL, "Error opening file '%s'!\n", fname);
+
+	return fd;
+}
+
+int
+rte_msr_read(const unsigned cpuid, const uint32_t reg, uint64_t *value)
+{
+	int fd = -1;
+	int ret = -1;
+
+	if (value == NULL)
+		return -EINVAL;
+
+	fd = __msr_open_file(cpuid, 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, EAL, "RDMSR failed for reg[0x%x] on CPU lcore %u\n",
+				(unsigned)reg, cpuid);
+		} else
+			ret = 0;
+
+		close(fd);
+	}
+
+	return ret;
+}
+
+int
+rte_msr_write(const unsigned cpuid, const uint32_t reg, const uint64_t value)
+{
+	int fd = -1;
+	int ret = -1;
+
+	fd = __msr_open_file(cpuid, O_WRONLY);
+
+	if (fd >= 0) {
+		ssize_t write_ret = 0;
+
+		write_ret = pwrite(fd, &value, sizeof(value), (off_t)reg);
+		if (write_ret != sizeof(value)) {
+			RTE_LOG(ERR, EAL, "WRMSR failed for reg[0x%x] <- value[0x%llx] "
+					"on CPU lcore %u\n", (unsigned)reg,
+					(unsigned long long)value, cpuid);
+		} else
+			ret = 0;
+
+		close(fd);
+	}
+
+	return ret;
+}
-- 
1.9.3

Re: [dpdk-dev] [PATCH v3] Patch introducing API to read/write Intel Architecture Model Specific Registers (MSR)...

[Thomas Monjalon]

Hi,

2016-01-21 08:18, Wojciech Andralojc:
> Patch reworked.
> 
> Signed-off-by: Wojciech Andralojc <wojciechx.andralojc@intel.com>

It cannot be applied without a proper commit log.

Please reword also the title to make it shorter and similar to
what you can find in the git history.
You can find some help in the doc:
	http://dpdk.org/doc/guides/contributing/patches.html#commit-messages-subject-line

Re: [dpdk-dev] [PATCH v3] Patch introducing API to read/write Intel Architecture Model Specific Registers (MSR)...

[Panu Matilainen]

On 01/21/2016 10:18 AM, Wojciech Andralojc wrote:
> Patch reworked.
>
> Signed-off-by: Wojciech Andralojc <wojciechx.andralojc@intel.com>
> ---
>   lib/librte_eal/common/include/arch/x86/rte_msr.h |  88 +++++++++++++++++
>   lib/librte_eal/linuxapp/eal/Makefile             |   1 +
>   lib/librte_eal/linuxapp/eal/arch/x86/rte_msr.c   | 116 +++++++++++++++++++++++
>   3 files changed, 205 insertions(+)
>   create mode 100644 lib/librte_eal/common/include/arch/x86/rte_msr.h
>   create mode 100644 lib/librte_eal/linuxapp/eal/arch/x86/rte_msr.c

This creates a new arch-specific public API, with rte_msr.h installed as 
a public header and implementation in the library (as opposed to 
inline), and so the new functions would have to be added to 
rte_eal_version.map.

However that is a bit of a problem since it only exists on IA 
architectures, so it'd mean dummy entries in the version map for all 
other architectures. All the other arch-specific APIs are inline code so 
this is the first of its kind.

Jerin Jacob suggested [1] adding these as internal (inline) functions
which to me looks like a more sensible approach, arch-specific APIs tend 
to be problematic.

[1] http://dpdk.org/ml/archives/dev/2016-January/031095.html

	- Panu -

Re: [dpdk-dev] [PATCH v3] Patch introducing API to read/write Intel Architecture Model Specific Registers (MSR)...

[Ananyev, Konstantin]

Hi Panu,

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Panu Matilainen
> Sent: Thursday, January 21, 2016 10:39 AM
> To: Andralojc, WojciechX
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3] Patch introducing API to read/write Intel Architecture Model Specific Registers (MSR)...
> 
> On 01/21/2016 10:18 AM, Wojciech Andralojc wrote:
> > Patch reworked.
> >
> > Signed-off-by: Wojciech Andralojc <wojciechx.andralojc@intel.com>
> > ---
> >   lib/librte_eal/common/include/arch/x86/rte_msr.h |  88 +++++++++++++++++
> >   lib/librte_eal/linuxapp/eal/Makefile             |   1 +
> >   lib/librte_eal/linuxapp/eal/arch/x86/rte_msr.c   | 116 +++++++++++++++++++++++
> >   3 files changed, 205 insertions(+)
> >   create mode 100644 lib/librte_eal/common/include/arch/x86/rte_msr.h
> >   create mode 100644 lib/librte_eal/linuxapp/eal/arch/x86/rte_msr.c
> 
> This creates a new arch-specific public API, with rte_msr.h installed as
> a public header and implementation in the library (as opposed to
> inline), and so the new functions would have to be added to
> rte_eal_version.map.
> 
> However that is a bit of a problem since it only exists on IA
> architectures, so it'd mean dummy entries in the version map for all
> other architectures. All the other arch-specific APIs are inline code so
> this is the first of its kind.

My thought was:
1. implementation is linux specific (as I know not supposed to work under freebsd).
2. they are not supposed to be used at run-time cide-path, so no need to be inlined.
3. As I understand we plan to  have a library that will use these functions anyway.

About dummy entries in the .map file: if we'll create a 'weak' generic implementation,
that would just return an error - would it solve the issue? 

Konstantin

> 
> Jerin Jacob suggested [1] adding these as internal (inline) functions
> which to me looks like a more sensible approach, arch-specific APIs tend
> to be problematic.
> 
> [1] http://dpdk.org/ml/archives/dev/2016-January/031095.html
> 
> 	- Panu -

Re: [dpdk-dev] [PATCH v3] Patch introducing API to read/write Intel Architecture Model Specific Registers (MSR)...

[Panu Matilainen]

On 01/21/2016 12:51 PM, Ananyev, Konstantin wrote:
> Hi Panu,
>
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Panu Matilainen
>> Sent: Thursday, January 21, 2016 10:39 AM
>> To: Andralojc, WojciechX
>> Cc: dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH v3] Patch introducing API to read/write Intel Architecture Model Specific Registers (MSR)...
>>
>> On 01/21/2016 10:18 AM, Wojciech Andralojc wrote:
>>> Patch reworked.
>>>
>>> Signed-off-by: Wojciech Andralojc <wojciechx.andralojc@intel.com>
>>> ---
>>>    lib/librte_eal/common/include/arch/x86/rte_msr.h |  88 +++++++++++++++++
>>>    lib/librte_eal/linuxapp/eal/Makefile             |   1 +
>>>    lib/librte_eal/linuxapp/eal/arch/x86/rte_msr.c   | 116 +++++++++++++++++++++++
>>>    3 files changed, 205 insertions(+)
>>>    create mode 100644 lib/librte_eal/common/include/arch/x86/rte_msr.h
>>>    create mode 100644 lib/librte_eal/linuxapp/eal/arch/x86/rte_msr.c
>>
>> This creates a new arch-specific public API, with rte_msr.h installed as
>> a public header and implementation in the library (as opposed to
>> inline), and so the new functions would have to be added to
>> rte_eal_version.map.
>>
>> However that is a bit of a problem since it only exists on IA
>> architectures, so it'd mean dummy entries in the version map for all
>> other architectures. All the other arch-specific APIs are inline code so
>> this is the first of its kind.
>
> My thought was:
> 1. implementation is linux specific (as I know not supposed to work under freebsd).
> 2. they are not supposed to be used at run-time cide-path, so no need to be inlined.

Speed is not the only interesting attribute of inlining, inlined code 
also effectively escapes the library ABI so it does not need versioning 
/ exporting.

> 3. As I understand we plan to  have a library that will use these functions anyway.

Is this library going to be a generic or specific to Intel CPUs?
Also, if there are no other uses for the MSR API at the moment, perhaps 
the best place for this code would be within that library anyway?

> About dummy entries in the .map file: if we'll create a 'weak' generic implementation,
> that would just return an error - would it solve the issue?

Sure it'd solve the issue of dummy entries in .map but people seemed 
opposed to having a highly arch-specific API exposed to all architectures.

	- Panu -

Re: [dpdk-dev] [PATCH v3] Patch introducing API to read/write Intel Architecture Model Specific Registers (MSR)...

[Ananyev, Konstantin]

> -----Original Message-----
> From: Panu Matilainen [mailto:pmatilai@redhat.com]
> Sent: Friday, January 22, 2016 10:06 AM
> To: Ananyev, Konstantin; Andralojc, WojciechX
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3] Patch introducing API to read/write Intel Architecture Model Specific Registers (MSR)...
> 
> On 01/21/2016 12:51 PM, Ananyev, Konstantin wrote:
> > Hi Panu,
> >
> >> -----Original Message-----
> >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Panu Matilainen
> >> Sent: Thursday, January 21, 2016 10:39 AM
> >> To: Andralojc, WojciechX
> >> Cc: dev@dpdk.org
> >> Subject: Re: [dpdk-dev] [PATCH v3] Patch introducing API to read/write Intel Architecture Model Specific Registers (MSR)...
> >>
> >> On 01/21/2016 10:18 AM, Wojciech Andralojc wrote:
> >>> Patch reworked.
> >>>
> >>> Signed-off-by: Wojciech Andralojc <wojciechx.andralojc@intel.com>
> >>> ---
> >>>    lib/librte_eal/common/include/arch/x86/rte_msr.h |  88 +++++++++++++++++
> >>>    lib/librte_eal/linuxapp/eal/Makefile             |   1 +
> >>>    lib/librte_eal/linuxapp/eal/arch/x86/rte_msr.c   | 116 +++++++++++++++++++++++
> >>>    3 files changed, 205 insertions(+)
> >>>    create mode 100644 lib/librte_eal/common/include/arch/x86/rte_msr.h
> >>>    create mode 100644 lib/librte_eal/linuxapp/eal/arch/x86/rte_msr.c
> >>
> >> This creates a new arch-specific public API, with rte_msr.h installed as
> >> a public header and implementation in the library (as opposed to
> >> inline), and so the new functions would have to be added to
> >> rte_eal_version.map.
> >>
> >> However that is a bit of a problem since it only exists on IA
> >> architectures, so it'd mean dummy entries in the version map for all
> >> other architectures. All the other arch-specific APIs are inline code so
> >> this is the first of its kind.
> >
> > My thought was:
> > 1. implementation is linux specific (as I know not supposed to work under freebsd).
> > 2. they are not supposed to be used at run-time cide-path, so no need to be inlined.
> 
> Speed is not the only interesting attribute of inlining, inlined code
> also effectively escapes the library ABI so it does not need versioning
> / exporting.
> 
> > 3. As I understand we plan to  have a library that will use these functions anyway.
> 
> Is this library going to be a generic or specific to Intel CPUs?

As I understand - yes.
It supposed to utilise new Intel chips CAT abilities. 
Wojciech, please correct me if I missed something here.

> Also, if there are no other uses for the MSR API at the moment, perhaps
> the best place for this code would be within that library anyway?

Sounds good to me.

Konstantin

> 
> > About dummy entries in the .map file: if we'll create a 'weak' generic implementation,
> > that would just return an error - would it solve the issue?
> 
> Sure it'd solve the issue of dummy entries in .map but people seemed
> opposed to having a highly arch-specific API exposed to all architectures.
> 
> 	- Panu -
> 

Re: [dpdk-dev] [PATCH v3] Patch introducing API to read/write Intel Architecture Model Specific Registers (MSR)...

[Andralojc, WojciechX]

> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Friday, January 22, 2016 11:05 AM
> To: Panu Matilainen; Andralojc, WojciechX
> Cc: dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v3] Patch introducing API to read/write Intel
> Architecture Model Specific Registers (MSR)...
> 
> > -----Original Message-----
> > From: Panu Matilainen [mailto:pmatilai@redhat.com]
> > Sent: Friday, January 22, 2016 10:06 AM
> > To: Ananyev, Konstantin; Andralojc, WojciechX
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v3] Patch introducing API to read/write Intel
> Architecture Model Specific Registers (MSR)...
> >
> > On 01/21/2016 12:51 PM, Ananyev, Konstantin wrote:
> > > Hi Panu,
> > >
> > >> -----Original Message-----
> > >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Panu
> > >> Matilainen
> > >> Sent: Thursday, January 21, 2016 10:39 AM
> > >> To: Andralojc, WojciechX
> > >> Cc: dev@dpdk.org
> > >> Subject: Re: [dpdk-dev] [PATCH v3] Patch introducing API to read/write
> Intel Architecture Model Specific Registers (MSR)...
> > >>
> > >> On 01/21/2016 10:18 AM, Wojciech Andralojc wrote:
> > >>> Patch reworked.
> > >>>
> > >>> Signed-off-by: Wojciech Andralojc <wojciechx.andralojc@intel.com>
> > >>> ---
> > >>>    lib/librte_eal/common/include/arch/x86/rte_msr.h |  88
> +++++++++++++++++
> > >>>    lib/librte_eal/linuxapp/eal/Makefile             |   1 +
> > >>>    lib/librte_eal/linuxapp/eal/arch/x86/rte_msr.c   | 116
> +++++++++++++++++++++++
> > >>>    3 files changed, 205 insertions(+)
> > >>>    create mode 100644
> lib/librte_eal/common/include/arch/x86/rte_msr.h
> > >>>    create mode 100644
> > >>> lib/librte_eal/linuxapp/eal/arch/x86/rte_msr.c
> > >>
> > >> This creates a new arch-specific public API, with rte_msr.h
> > >> installed as a public header and implementation in the library (as
> > >> opposed to inline), and so the new functions would have to be added
> > >> to rte_eal_version.map.
> > >>
> > >> However that is a bit of a problem since it only exists on IA
> > >> architectures, so it'd mean dummy entries in the version map for
> > >> all other architectures. All the other arch-specific APIs are
> > >> inline code so this is the first of its kind.
> > >
> > > My thought was:
> > > 1. implementation is linux specific (as I know not supposed to work under
> freebsd).
> > > 2. they are not supposed to be used at run-time cide-path, so no need to be
> inlined.
> >
> > Speed is not the only interesting attribute of inlining, inlined code
> > also effectively escapes the library ABI so it does not need
> > versioning / exporting.
> >
> > > 3. As I understand we plan to  have a library that will use these functions
> anyway.
> >
> > Is this library going to be a generic or specific to Intel CPUs?
> 
> As I understand - yes.
> It supposed to utilise new Intel chips CAT abilities.
> Wojciech, please correct me if I missed something here.

Konstantin, yes, you are right,
CAT enabling lib is Intel CPUs specific.

> 
> > Also, if there are no other uses for the MSR API at the moment,
> > perhaps the best place for this code would be within that library anyway?
> 
> Sounds good to me.

OK, sounds good.
Thank you both for your input.

> 
> Konstantin
> 
> >
> > > About dummy entries in the .map file: if we'll create a 'weak'
> > > generic implementation, that would just return an error - would it solve the
> issue?
> >
> > Sure it'd solve the issue of dummy entries in .map but people seemed
> > opposed to having a highly arch-specific API exposed to all architectures.
> >
> > 	- Panu -
> >

Wojciech Andralojc