[dpdk-dev,v3] Patch introducing API to read/write Intel Architecture Model Specific Registers (MSR)...
Commit Message
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
Comments
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
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 -
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 -
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 -
> -----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 -
>
> -----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
new file mode 100644
@@ -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_ */
@@ -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
new file mode 100644
@@ -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;
+}