Hi Wojciech, Couple of nits, see below. Konstantin > -----Original Message----- > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Wojciech Andralojc > Sent: Wednesday, January 20, 2016 10:57 AM > To: dev at 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 at 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; > +}