* [dpdk-dev] [PATCH] eal: abstract away the auxiliary vector
@ 2018-03-22 15:35 Aaron Conole
2018-03-22 16:08 ` Wiles, Keith
2018-04-02 18:24 ` [dpdk-dev] [PATCH v2] " Aaron Conole
0 siblings, 2 replies; 6+ messages in thread
From: Aaron Conole @ 2018-03-22 15:35 UTC (permalink / raw)
To: dev
Cc: Timothy Redaelli, Bruce Richardson, Jan Viktorin, Jianbo Liu,
Jerin Jacob, Chao Zhu
Rather than attempting to load the contents of the auxv directly,
prefer to use an exposed API - and if that doesn't exist then attempt
to load the vector. This is because on some systems, when a user
is downgraded, the /proc/self/auxv file retains the old ownership
and permissions. The original method of /proc/self/auxv is retained.
This also removes a potential abort() in the code when compiled with
NDEBUG. A quick parse of the code shows that many (if not all) of
the CPU flag parsing isn't used internally, so it should be okay.
Signed-off-by: Aaron Conole <aconole@redhat.com>
Signed-off-by: Timothy Redaelli <tredaelli@redhat.com>
---
NOTE: These APIs aren't added to the map because I really want
them to be internal-only. No other component really needs
access to them. I couldn't find a good common internal
include file which is why they're in the rte_cpuflags.h -
but if there's a better one, we can spin another version.
lib/librte_eal/common/arch/arm/rte_cpuflags.c | 20 ++----
lib/librte_eal/common/arch/ppc_64/rte_cpuflags.c | 15 +----
lib/librte_eal/common/eal_common_cpuflags.c | 76 ++++++++++++++++++++++
.../common/include/generic/rte_cpuflags.h | 13 ++++
4 files changed, 95 insertions(+), 29 deletions(-)
diff --git a/lib/librte_eal/common/arch/arm/rte_cpuflags.c b/lib/librte_eal/common/arch/arm/rte_cpuflags.c
index 88f1cbe37..496d8b21a 100644
--- a/lib/librte_eal/common/arch/arm/rte_cpuflags.c
+++ b/lib/librte_eal/common/arch/arm/rte_cpuflags.c
@@ -133,22 +133,10 @@ const struct feature_entry rte_cpu_feature_table[] = {
static void
rte_cpu_get_features(hwcap_registers_t out)
{
- int auxv_fd;
- _Elfx_auxv_t auxv;
-
- auxv_fd = open("/proc/self/auxv", O_RDONLY);
- assert(auxv_fd != -1);
- while (read(auxv_fd, &auxv, sizeof(auxv)) == sizeof(auxv)) {
- if (auxv.a_type == AT_HWCAP) {
- out[REG_HWCAP] = auxv.a_un.a_val;
- } else if (auxv.a_type == AT_HWCAP2) {
- out[REG_HWCAP2] = auxv.a_un.a_val;
- } else if (auxv.a_type == AT_PLATFORM) {
- if (!strcmp((const char *)auxv.a_un.a_val, PLATFORM_STR))
- out[REG_PLATFORM] = 0x0001;
- }
- }
- close(auxv_fd);
+ out[REG_HWCAP] = rte_cpu_getauxval(AT_HWCAP);
+ out[REG_HWCAP2] = rte_cpu_getauxval(AT_HWCAP2);
+ if (!rte_cpu_strcmp_auxval(AT_PLATFORM, PLATFORM_STR))
+ out[REG_PLATFORM] = 0x0001;
}
/*
diff --git a/lib/librte_eal/common/arch/ppc_64/rte_cpuflags.c b/lib/librte_eal/common/arch/ppc_64/rte_cpuflags.c
index 970a61c5e..e7a82452b 100644
--- a/lib/librte_eal/common/arch/ppc_64/rte_cpuflags.c
+++ b/lib/librte_eal/common/arch/ppc_64/rte_cpuflags.c
@@ -104,19 +104,8 @@ const struct feature_entry rte_cpu_feature_table[] = {
static void
rte_cpu_get_features(hwcap_registers_t out)
{
- int auxv_fd;
- Elf64_auxv_t auxv;
-
- auxv_fd = open("/proc/self/auxv", O_RDONLY);
- assert(auxv_fd != -1);
- while (read(auxv_fd, &auxv,
- sizeof(Elf64_auxv_t)) == sizeof(Elf64_auxv_t)) {
- if (auxv.a_type == AT_HWCAP)
- out[REG_HWCAP] = auxv.a_un.a_val;
- else if (auxv.a_type == AT_HWCAP2)
- out[REG_HWCAP2] = auxv.a_un.a_val;
- }
- close(auxv_fd);
+ out[REG_HWCAP] = rte_cpu_getauxval(AT_HWCAP);
+ out[REG_HWCAP2] = rte_cpu_getauxval(AT_HWCAP2);
}
/*
diff --git a/lib/librte_eal/common/eal_common_cpuflags.c b/lib/librte_eal/common/eal_common_cpuflags.c
index 3a055f7c7..87c9539ad 100644
--- a/lib/librte_eal/common/eal_common_cpuflags.c
+++ b/lib/librte_eal/common/eal_common_cpuflags.c
@@ -2,11 +2,87 @@
* Copyright(c) 2010-2014 Intel Corporation
*/
+#include <elf.h>
+#include <fcntl.h>
#include <stdio.h>
+#include <string.h>
+#include <sys/stat.h>
+#include <sys/types.h>
+#include <unistd.h>
+
+#if defined(__GLIBC__) && defined(__GLIBC_PREREQ)
+#if __GLIBC_PREREQ(2, 16)
+#include <sys/auxv.h>
+#define HAS_AUXV 1
+#endif
+#endif
#include <rte_common.h>
#include <rte_cpuflags.h>
+#ifndef HAS_AUXV
+static unsigned long
+getauxval(unsigned long type)
+{
+ errno = ENOTSUP;
+ return 0;
+}
+#endif
+
+#if RTE_ARCH_64
+typedef Elf64_auxv_t Internal_Elfx_auxv_t;
+#else
+typedef Elf32_auxv_t Internal_Elfx_auxv_t;
+#endif
+
+
+/**
+ * Provides a method for retrieving values from the auxiliary vector and
+ * possibly running a string comparison.
+ */
+static unsigned long
+_rte_cpu_getauxval(unsigned long type, int cmp_str, const char *str)
+{
+ unsigned long val;
+
+ errno = 0;
+ val = getauxval(type);
+
+ if (!val && (errno == ENOTSUP || errno == ENOENT)) {
+ Internal_Elfx_auxv_t auxv;
+ int auxv_fd = open("/proc/self/auxv", O_RDONLY);
+
+ errno = ENOENT;
+ if (auxv_fd == -1)
+ return 0;
+
+ while (read(auxv_fd, &auxv, sizeof(auxv)) == sizeof(auxv)) {
+ if (auxv.a_type == type) {
+ errno = 0;
+ val = auxv.a_un.a_val;
+ if (cmp_str)
+ val = strcmp((const char *)val, str);
+ break;
+ }
+ }
+ close(auxv_fd);
+ }
+
+ return val;
+}
+
+unsigned long
+rte_cpu_getauxval(unsigned long type)
+{
+ return _rte_cpu_getauxval(type, 0, NULL);
+}
+
+int
+rte_cpu_strcmp_auxval(unsigned long type, const char *str)
+{
+ return _rte_cpu_getauxval(type, 1, str);
+}
+
/**
* Checks if the machine is adequate for running the binary. If it is not, the
* program exits with status 1.
diff --git a/lib/librte_eal/common/include/generic/rte_cpuflags.h b/lib/librte_eal/common/include/generic/rte_cpuflags.h
index 8d31687d8..ffa3b744a 100644
--- a/lib/librte_eal/common/include/generic/rte_cpuflags.h
+++ b/lib/librte_eal/common/include/generic/rte_cpuflags.h
@@ -64,4 +64,17 @@ rte_cpu_check_supported(void);
int
rte_cpu_is_supported(void);
+/**
+ * This function attempts to retrieve a value from the auxiliary vector.
+ */
+unsigned long
+rte_cpu_getauxval(unsigned long type);
+
+/**
+ * This function retrieves a value from the auxiliary vector, and compares it
+ * as a string.
+ */
+int
+rte_cpu_strcmp_auxval(unsigned long type, const char *str);
+
#endif /* _RTE_CPUFLAGS_H_ */
--
2.14.3
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-dev] [PATCH] eal: abstract away the auxiliary vector
2018-03-22 15:35 [dpdk-dev] [PATCH] eal: abstract away the auxiliary vector Aaron Conole
@ 2018-03-22 16:08 ` Wiles, Keith
2018-03-22 17:39 ` Aaron Conole
2018-04-02 18:24 ` [dpdk-dev] [PATCH v2] " Aaron Conole
1 sibling, 1 reply; 6+ messages in thread
From: Wiles, Keith @ 2018-03-22 16:08 UTC (permalink / raw)
To: Aaron Conole
Cc: dev, Timothy Redaelli, Richardson, Bruce, Jan Viktorin,
Jianbo Liu, Jerin Jacob, Chao Zhu
> On Mar 22, 2018, at 10:35 AM, Aaron Conole <aconole@redhat.com> wrote:
>
> Rather than attempting to load the contents of the auxv directly,
> prefer to use an exposed API - and if that doesn't exist then attempt
> to load the vector. This is because on some systems, when a user
> is downgraded, the /proc/self/auxv file retains the old ownership
> and permissions. The original method of /proc/self/auxv is retained.
>
> This also removes a potential abort() in the code when compiled with
> NDEBUG. A quick parse of the code shows that many (if not all) of
> the CPU flag parsing isn't used internally, so it should be okay.
>
> Signed-off-by: Aaron Conole <aconole@redhat.com>
> Signed-off-by: Timothy Redaelli <tredaelli@redhat.com>
> ---
> NOTE: These APIs aren't added to the map because I really want
> them to be internal-only. No other component really needs
> access to them. I couldn't find a good common internal
> include file which is why they're in the rte_cpuflags.h -
> but if there's a better one, we can spin another version.
>
> lib/librte_eal/common/arch/arm/rte_cpuflags.c | 20 ++----
> lib/librte_eal/common/arch/ppc_64/rte_cpuflags.c | 15 +----
> lib/librte_eal/common/eal_common_cpuflags.c | 76 ++++++++++++++++++++++
> .../common/include/generic/rte_cpuflags.h | 13 ++++
> 4 files changed, 95 insertions(+), 29 deletions(-)
>
> diff --git a/lib/librte_eal/common/arch/arm/rte_cpuflags.c b/lib/librte_eal/common/arch/arm/rte_cpuflags.c
> index 88f1cbe37..496d8b21a 100644
> --- a/lib/librte_eal/common/arch/arm/rte_cpuflags.c
> +++ b/lib/librte_eal/common/arch/arm/rte_cpuflags.c
> @@ -133,22 +133,10 @@ const struct feature_entry rte_cpu_feature_table[] = {
> static void
> rte_cpu_get_features(hwcap_registers_t out)
> {
> - int auxv_fd;
> - _Elfx_auxv_t auxv;
> -
> - auxv_fd = open("/proc/self/auxv", O_RDONLY);
> - assert(auxv_fd != -1);
> - while (read(auxv_fd, &auxv, sizeof(auxv)) == sizeof(auxv)) {
> - if (auxv.a_type == AT_HWCAP) {
> - out[REG_HWCAP] = auxv.a_un.a_val;
> - } else if (auxv.a_type == AT_HWCAP2) {
> - out[REG_HWCAP2] = auxv.a_un.a_val;
> - } else if (auxv.a_type == AT_PLATFORM) {
> - if (!strcmp((const char *)auxv.a_un.a_val, PLATFORM_STR))
> - out[REG_PLATFORM] = 0x0001;
> - }
> - }
> - close(auxv_fd);
> + out[REG_HWCAP] = rte_cpu_getauxval(AT_HWCAP);
> + out[REG_HWCAP2] = rte_cpu_getauxval(AT_HWCAP2);
> + if (!rte_cpu_strcmp_auxval(AT_PLATFORM, PLATFORM_STR))
> + out[REG_PLATFORM] = 0x0001;
I like the way you reduced this code here, which does make a lot of sense.
> }
>
> /*
> diff --git a/lib/librte_eal/common/arch/ppc_64/rte_cpuflags.c b/lib/librte_eal/common/arch/ppc_64/rte_cpuflags.c
> index 970a61c5e..e7a82452b 100644
> --- a/lib/librte_eal/common/arch/ppc_64/rte_cpuflags.c
> +++ b/lib/librte_eal/common/arch/ppc_64/rte_cpuflags.c
> @@ -104,19 +104,8 @@ const struct feature_entry rte_cpu_feature_table[] = {
> static void
> rte_cpu_get_features(hwcap_registers_t out)
> {
> - int auxv_fd;
> - Elf64_auxv_t auxv;
> -
> - auxv_fd = open("/proc/self/auxv", O_RDONLY);
> - assert(auxv_fd != -1);
> - while (read(auxv_fd, &auxv,
> - sizeof(Elf64_auxv_t)) == sizeof(Elf64_auxv_t)) {
> - if (auxv.a_type == AT_HWCAP)
> - out[REG_HWCAP] = auxv.a_un.a_val;
> - else if (auxv.a_type == AT_HWCAP2)
> - out[REG_HWCAP2] = auxv.a_un.a_val;
> - }
> - close(auxv_fd);
> + out[REG_HWCAP] = rte_cpu_getauxval(AT_HWCAP);
> + out[REG_HWCAP2] = rte_cpu_getauxval(AT_HWCAP2);
> }
>
> /*
> diff --git a/lib/librte_eal/common/eal_common_cpuflags.c b/lib/librte_eal/common/eal_common_cpuflags.c
> index 3a055f7c7..87c9539ad 100644
> --- a/lib/librte_eal/common/eal_common_cpuflags.c
> +++ b/lib/librte_eal/common/eal_common_cpuflags.c
> @@ -2,11 +2,87 @@
> * Copyright(c) 2010-2014 Intel Corporation
> */
>
> +#include <elf.h>
> +#include <fcntl.h>
> #include <stdio.h>
> +#include <string.h>
> +#include <sys/stat.h>
> +#include <sys/types.h>
> +#include <unistd.h>
> +
> +#if defined(__GLIBC__) && defined(__GLIBC_PREREQ)
> +#if __GLIBC_PREREQ(2, 16)
> +#include <sys/auxv.h>
> +#define HAS_AUXV 1
> +#endif
> +#endif
>
> #include <rte_common.h>
> #include <rte_cpuflags.h>
>
> +#ifndef HAS_AUXV
> +static unsigned long
> +getauxval(unsigned long type)
> +{
> + errno = ENOTSUP;
> + return 0;
> +}
> +#endif
> +
> +#if RTE_ARCH_64
> +typedef Elf64_auxv_t Internal_Elfx_auxv_t;
> +#else
> +typedef Elf32_auxv_t Internal_Elfx_auxv_t;
> +#endif
> +
> +
> +/**
> + * Provides a method for retrieving values from the auxiliary vector and
> + * possibly running a string comparison.
Need to add more docs here as zero is not the normal error return value, same for header file. Please add the correct doxygen formats.
> + */
> +static unsigned long
> +_rte_cpu_getauxval(unsigned long type, int cmp_str, const char *str)
Can we not remove cmp_str and just test for str != NULL to compare strings. This would be cleaner IMO.
> +{
> + unsigned long val;
> +
> + errno = 0;
> + val = getauxval(type);
> +
> + if (!val && (errno == ENOTSUP || errno == ENOENT)) {
> + Internal_Elfx_auxv_t auxv;
Should we set auxv = { 0 }; here, maybe not required.
> + int auxv_fd = open("/proc/self/auxv", O_RDONLY);
> +
> + errno = ENOENT;
> + if (auxv_fd == -1)
> + return 0;
> +
> + while (read(auxv_fd, &auxv, sizeof(auxv)) == sizeof(auxv)) {
> + if (auxv.a_type == type) {
> + errno = 0;
> + val = auxv.a_un.a_val;
> + if (cmp_str)
> + val = strcmp((const char *)val, str);
Does this need to be case sensitive or should we use strcasecmp() instead.
> + break;
> + }
> + }
> + close(auxv_fd);
> + }
> +
> + return val;
> +}
> +
> +unsigned long
> +rte_cpu_getauxval(unsigned long type)
> +{
> + return _rte_cpu_getauxval(type, 0, NULL);
> +}
> +
> +int
> +rte_cpu_strcmp_auxval(unsigned long type, const char *str)
> +{
> + return _rte_cpu_getauxval(type, 1, str);
> +}
> +
> /**
> * Checks if the machine is adequate for running the binary. If it is not, the
> * program exits with status 1.
> diff --git a/lib/librte_eal/common/include/generic/rte_cpuflags.h b/lib/librte_eal/common/include/generic/rte_cpuflags.h
> index 8d31687d8..ffa3b744a 100644
> --- a/lib/librte_eal/common/include/generic/rte_cpuflags.h
> +++ b/lib/librte_eal/common/include/generic/rte_cpuflags.h
> @@ -64,4 +64,17 @@ rte_cpu_check_supported(void);
> int
> rte_cpu_is_supported(void);
>
> +/**
> + * This function attempts to retrieve a value from the auxiliary vector.
Document the return value here as it is not the normal -1 value on error, same for the next one. And the doxygen formats too.
> + */
> +unsigned long
> +rte_cpu_getauxval(unsigned long type);
> +
> +/**
> + * This function retrieves a value from the auxiliary vector, and compares it
> + * as a string.
> + */
> +int
> +rte_cpu_strcmp_auxval(unsigned long type, const char *str);
> +
> #endif /* _RTE_CPUFLAGS_H_ */
> --
> 2.14.3
>
Regards,
Keith
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-dev] [PATCH] eal: abstract away the auxiliary vector
2018-03-22 16:08 ` Wiles, Keith
@ 2018-03-22 17:39 ` Aaron Conole
0 siblings, 0 replies; 6+ messages in thread
From: Aaron Conole @ 2018-03-22 17:39 UTC (permalink / raw)
To: Wiles, Keith
Cc: dev, Timothy Redaelli, Richardson, Bruce, Jan Viktorin,
Jianbo Liu, Jerin Jacob, Chao Zhu
"Wiles, Keith" <keith.wiles@intel.com> writes:
>> On Mar 22, 2018, at 10:35 AM, Aaron Conole <aconole@redhat.com> wrote:
>>
>> Rather than attempting to load the contents of the auxv directly,
>> prefer to use an exposed API - and if that doesn't exist then attempt
>> to load the vector. This is because on some systems, when a user
>> is downgraded, the /proc/self/auxv file retains the old ownership
>> and permissions. The original method of /proc/self/auxv is retained.
>>
>> This also removes a potential abort() in the code when compiled with
>> NDEBUG. A quick parse of the code shows that many (if not all) of
>> the CPU flag parsing isn't used internally, so it should be okay.
>>
>> Signed-off-by: Aaron Conole <aconole@redhat.com>
>> Signed-off-by: Timothy Redaelli <tredaelli@redhat.com>
>> ---
>> NOTE: These APIs aren't added to the map because I really want
>> them to be internal-only. No other component really needs
>> access to them. I couldn't find a good common internal
>> include file which is why they're in the rte_cpuflags.h -
>> but if there's a better one, we can spin another version.
>>
>> lib/librte_eal/common/arch/arm/rte_cpuflags.c | 20 ++----
>> lib/librte_eal/common/arch/ppc_64/rte_cpuflags.c | 15 +----
>> lib/librte_eal/common/eal_common_cpuflags.c | 76 ++++++++++++++++++++++
>> .../common/include/generic/rte_cpuflags.h | 13 ++++
>> 4 files changed, 95 insertions(+), 29 deletions(-)
>>
>> diff --git a/lib/librte_eal/common/arch/arm/rte_cpuflags.c
>> b/lib/librte_eal/common/arch/arm/rte_cpuflags.c
>> index 88f1cbe37..496d8b21a 100644
>> --- a/lib/librte_eal/common/arch/arm/rte_cpuflags.c
>> +++ b/lib/librte_eal/common/arch/arm/rte_cpuflags.c
>> @@ -133,22 +133,10 @@ const struct feature_entry rte_cpu_feature_table[] = {
>> static void
>> rte_cpu_get_features(hwcap_registers_t out)
>> {
>> - int auxv_fd;
>> - _Elfx_auxv_t auxv;
>> -
>> - auxv_fd = open("/proc/self/auxv", O_RDONLY);
>> - assert(auxv_fd != -1);
>> - while (read(auxv_fd, &auxv, sizeof(auxv)) == sizeof(auxv)) {
>> - if (auxv.a_type == AT_HWCAP) {
>> - out[REG_HWCAP] = auxv.a_un.a_val;
>> - } else if (auxv.a_type == AT_HWCAP2) {
>> - out[REG_HWCAP2] = auxv.a_un.a_val;
>> - } else if (auxv.a_type == AT_PLATFORM) {
>> - if (!strcmp((const char *)auxv.a_un.a_val, PLATFORM_STR))
>> - out[REG_PLATFORM] = 0x0001;
>> - }
>> - }
>> - close(auxv_fd);
>> + out[REG_HWCAP] = rte_cpu_getauxval(AT_HWCAP);
>> + out[REG_HWCAP2] = rte_cpu_getauxval(AT_HWCAP2);
>> + if (!rte_cpu_strcmp_auxval(AT_PLATFORM, PLATFORM_STR))
>> + out[REG_PLATFORM] = 0x0001;
>
> I like the way you reduced this code here, which does make a lot of sense.
>
>> }
>>
>> /*
>> diff --git a/lib/librte_eal/common/arch/ppc_64/rte_cpuflags.c
>> b/lib/librte_eal/common/arch/ppc_64/rte_cpuflags.c
>> index 970a61c5e..e7a82452b 100644
>> --- a/lib/librte_eal/common/arch/ppc_64/rte_cpuflags.c
>> +++ b/lib/librte_eal/common/arch/ppc_64/rte_cpuflags.c
>> @@ -104,19 +104,8 @@ const struct feature_entry rte_cpu_feature_table[] = {
>> static void
>> rte_cpu_get_features(hwcap_registers_t out)
>> {
>> - int auxv_fd;
>> - Elf64_auxv_t auxv;
>> -
>> - auxv_fd = open("/proc/self/auxv", O_RDONLY);
>> - assert(auxv_fd != -1);
>> - while (read(auxv_fd, &auxv,
>> - sizeof(Elf64_auxv_t)) == sizeof(Elf64_auxv_t)) {
>> - if (auxv.a_type == AT_HWCAP)
>> - out[REG_HWCAP] = auxv.a_un.a_val;
>> - else if (auxv.a_type == AT_HWCAP2)
>> - out[REG_HWCAP2] = auxv.a_un.a_val;
>> - }
>> - close(auxv_fd);
>> + out[REG_HWCAP] = rte_cpu_getauxval(AT_HWCAP);
>> + out[REG_HWCAP2] = rte_cpu_getauxval(AT_HWCAP2);
>> }
>>
>> /*
>> diff --git a/lib/librte_eal/common/eal_common_cpuflags.c
>> b/lib/librte_eal/common/eal_common_cpuflags.c
>> index 3a055f7c7..87c9539ad 100644
>> --- a/lib/librte_eal/common/eal_common_cpuflags.c
>> +++ b/lib/librte_eal/common/eal_common_cpuflags.c
>> @@ -2,11 +2,87 @@
>> * Copyright(c) 2010-2014 Intel Corporation
>> */
>>
>> +#include <elf.h>
>> +#include <fcntl.h>
>> #include <stdio.h>
>> +#include <string.h>
>> +#include <sys/stat.h>
>> +#include <sys/types.h>
>> +#include <unistd.h>
>> +
>> +#if defined(__GLIBC__) && defined(__GLIBC_PREREQ)
>> +#if __GLIBC_PREREQ(2, 16)
>> +#include <sys/auxv.h>
>> +#define HAS_AUXV 1
>> +#endif
>> +#endif
>>
>> #include <rte_common.h>
>> #include <rte_cpuflags.h>
>>
>> +#ifndef HAS_AUXV
>> +static unsigned long
>> +getauxval(unsigned long type)
>> +{
>> + errno = ENOTSUP;
>> + return 0;
>> +}
>> +#endif
>> +
>> +#if RTE_ARCH_64
>> +typedef Elf64_auxv_t Internal_Elfx_auxv_t;
>> +#else
>> +typedef Elf32_auxv_t Internal_Elfx_auxv_t;
>> +#endif
>> +
>> +
>> +/**
>> + * Provides a method for retrieving values from the auxiliary vector and
>> + * possibly running a string comparison.
>
> Need to add more docs here as zero is not the normal error return
> value, same for header file. Please add the correct doxygen formats.
Will do.
>> + */
>> +static unsigned long
>> +_rte_cpu_getauxval(unsigned long type, int cmp_str, const char *str)
>
> Can we not remove cmp_str and just test for str != NULL to compare
> strings. This would be cleaner IMO.
Agreed - will fix in v2.
>> +{
>> + unsigned long val;
>> +
>> + errno = 0;
>> + val = getauxval(type);
>> +
>> + if (!val && (errno == ENOTSUP || errno == ENOENT)) {
>> + Internal_Elfx_auxv_t auxv;
>
> Should we set auxv = { 0 }; here, maybe not required.
It shouldn't be needed because we read() into it before ever using it as
an r-value.
>> + int auxv_fd = open("/proc/self/auxv", O_RDONLY);
>> +
>> + errno = ENOENT;
>> + if (auxv_fd == -1)
>> + return 0;
>> +
>> + while (read(auxv_fd, &auxv, sizeof(auxv)) == sizeof(auxv)) {
>> + if (auxv.a_type == type) {
>> + errno = 0;
>> + val = auxv.a_un.a_val;
>> + if (cmp_str)
>> + val = strcmp((const char *)val, str);
>
> Does this need to be case sensitive or should we use strcasecmp()
> instead.
The original code was case sensitive, so I left it. I'd rather
not introduce a functional change like that in this patch.
>> + break;
>> + }
>> + }
>> + close(auxv_fd);
>> + }
>> +
>> + return val;
>> +}
>> +
>> +unsigned long
>> +rte_cpu_getauxval(unsigned long type)
>> +{
>> + return _rte_cpu_getauxval(type, 0, NULL);
>> +}
>> +
>> +int
>> +rte_cpu_strcmp_auxval(unsigned long type, const char *str)
>> +{
>> + return _rte_cpu_getauxval(type, 1, str);
>> +}
>> +
>> /**
>> * Checks if the machine is adequate for running the binary. If it is not, the
>> * program exits with status 1.
>> diff --git a/lib/librte_eal/common/include/generic/rte_cpuflags.h
>> b/lib/librte_eal/common/include/generic/rte_cpuflags.h
>> index 8d31687d8..ffa3b744a 100644
>> --- a/lib/librte_eal/common/include/generic/rte_cpuflags.h
>> +++ b/lib/librte_eal/common/include/generic/rte_cpuflags.h
>> @@ -64,4 +64,17 @@ rte_cpu_check_supported(void);
>> int
>> rte_cpu_is_supported(void);
>>
>> +/**
>> + * This function attempts to retrieve a value from the auxiliary vector.
>
> Document the return value here as it is not the normal -1 value on
> error, same for the next one. And the doxygen formats too.
Good call on these. Will do for v2.
>> + */
>> +unsigned long
>> +rte_cpu_getauxval(unsigned long type);
>> +
>> +/**
>> + * This function retrieves a value from the auxiliary vector, and compares it
>> + * as a string.
>> + */
>> +int
>> +rte_cpu_strcmp_auxval(unsigned long type, const char *str);
>> +
>> #endif /* _RTE_CPUFLAGS_H_ */
>> --
>> 2.14.3
>>
Thanks for the review, Keith!
> Regards,
> Keith
^ permalink raw reply [flat|nested] 6+ messages in thread
* [dpdk-dev] [PATCH v2] eal: abstract away the auxiliary vector
2018-03-22 15:35 [dpdk-dev] [PATCH] eal: abstract away the auxiliary vector Aaron Conole
2018-03-22 16:08 ` Wiles, Keith
@ 2018-04-02 18:24 ` Aaron Conole
2018-04-11 21:34 ` Thomas Monjalon
1 sibling, 1 reply; 6+ messages in thread
From: Aaron Conole @ 2018-04-02 18:24 UTC (permalink / raw)
To: dev
Cc: Timothy Redaelli, Bruce Richardson, Jan Viktorin, Jianbo Liu,
Jerin Jacob, Chao Zhu, Keith Wiles
Rather than attempting to load the contents of the auxv directly,
prefer to use an exposed API - and if that doesn't exist then attempt
to load the vector. This is because on some systems, when a user
is downgraded, the /proc/self/auxv file retains the old ownership
and permissions. The original method of /proc/self/auxv is retained.
This also removes a potential abort() in the code when compiled with
NDEBUG. A quick parse of the code shows that many (if not all) of
the CPU flag parsing isn't used internally, so it should be okay.
Signed-off-by: Aaron Conole <aconole@redhat.com>
Signed-off-by: Timothy Redaelli <tredaelli@redhat.com>
---
lib/librte_eal/common/arch/arm/rte_cpuflags.c | 20 ++----
lib/librte_eal/common/arch/ppc_64/rte_cpuflags.c | 15 +---
lib/librte_eal/common/eal_common_cpuflags.c | 79 ++++++++++++++++++++++
.../common/include/generic/rte_cpuflags.h | 21 ++++++
4 files changed, 106 insertions(+), 29 deletions(-)
diff --git a/lib/librte_eal/common/arch/arm/rte_cpuflags.c b/lib/librte_eal/common/arch/arm/rte_cpuflags.c
index 88f1cbe37..496d8b21a 100644
--- a/lib/librte_eal/common/arch/arm/rte_cpuflags.c
+++ b/lib/librte_eal/common/arch/arm/rte_cpuflags.c
@@ -133,22 +133,10 @@ const struct feature_entry rte_cpu_feature_table[] = {
static void
rte_cpu_get_features(hwcap_registers_t out)
{
- int auxv_fd;
- _Elfx_auxv_t auxv;
-
- auxv_fd = open("/proc/self/auxv", O_RDONLY);
- assert(auxv_fd != -1);
- while (read(auxv_fd, &auxv, sizeof(auxv)) == sizeof(auxv)) {
- if (auxv.a_type == AT_HWCAP) {
- out[REG_HWCAP] = auxv.a_un.a_val;
- } else if (auxv.a_type == AT_HWCAP2) {
- out[REG_HWCAP2] = auxv.a_un.a_val;
- } else if (auxv.a_type == AT_PLATFORM) {
- if (!strcmp((const char *)auxv.a_un.a_val, PLATFORM_STR))
- out[REG_PLATFORM] = 0x0001;
- }
- }
- close(auxv_fd);
+ out[REG_HWCAP] = rte_cpu_getauxval(AT_HWCAP);
+ out[REG_HWCAP2] = rte_cpu_getauxval(AT_HWCAP2);
+ if (!rte_cpu_strcmp_auxval(AT_PLATFORM, PLATFORM_STR))
+ out[REG_PLATFORM] = 0x0001;
}
/*
diff --git a/lib/librte_eal/common/arch/ppc_64/rte_cpuflags.c b/lib/librte_eal/common/arch/ppc_64/rte_cpuflags.c
index 970a61c5e..e7a82452b 100644
--- a/lib/librte_eal/common/arch/ppc_64/rte_cpuflags.c
+++ b/lib/librte_eal/common/arch/ppc_64/rte_cpuflags.c
@@ -104,19 +104,8 @@ const struct feature_entry rte_cpu_feature_table[] = {
static void
rte_cpu_get_features(hwcap_registers_t out)
{
- int auxv_fd;
- Elf64_auxv_t auxv;
-
- auxv_fd = open("/proc/self/auxv", O_RDONLY);
- assert(auxv_fd != -1);
- while (read(auxv_fd, &auxv,
- sizeof(Elf64_auxv_t)) == sizeof(Elf64_auxv_t)) {
- if (auxv.a_type == AT_HWCAP)
- out[REG_HWCAP] = auxv.a_un.a_val;
- else if (auxv.a_type == AT_HWCAP2)
- out[REG_HWCAP2] = auxv.a_un.a_val;
- }
- close(auxv_fd);
+ out[REG_HWCAP] = rte_cpu_getauxval(AT_HWCAP);
+ out[REG_HWCAP2] = rte_cpu_getauxval(AT_HWCAP2);
}
/*
diff --git a/lib/librte_eal/common/eal_common_cpuflags.c b/lib/librte_eal/common/eal_common_cpuflags.c
index 3a055f7c7..1a0c3d8c8 100644
--- a/lib/librte_eal/common/eal_common_cpuflags.c
+++ b/lib/librte_eal/common/eal_common_cpuflags.c
@@ -2,11 +2,90 @@
* Copyright(c) 2010-2014 Intel Corporation
*/
+#include <elf.h>
+#include <fcntl.h>
#include <stdio.h>
+#include <string.h>
+#include <sys/stat.h>
+#include <sys/types.h>
+#include <unistd.h>
+
+#if defined(__GLIBC__) && defined(__GLIBC_PREREQ)
+#if __GLIBC_PREREQ(2, 16)
+#include <sys/auxv.h>
+#define HAS_AUXV 1
+#endif
+#endif
#include <rte_common.h>
#include <rte_cpuflags.h>
+#ifndef HAS_AUXV
+static unsigned long
+getauxval(unsigned long type)
+{
+ errno = ENOTSUP;
+ return 0;
+}
+#endif
+
+#if RTE_ARCH_64
+typedef Elf64_auxv_t Internal_Elfx_auxv_t;
+#else
+typedef Elf32_auxv_t Internal_Elfx_auxv_t;
+#endif
+
+
+/**
+ * Provides a method for retrieving values from the auxiliary vector and
+ * possibly running a string comparison.
+ *
+ * @return Always returns a result. When the result is 0, check errno
+ * to see if an error occurred during processing.
+ */
+static unsigned long
+_rte_cpu_getauxval(unsigned long type, const char *str)
+{
+ unsigned long val;
+
+ errno = 0;
+ val = getauxval(type);
+
+ if (!val && (errno == ENOTSUP || errno == ENOENT)) {
+ int auxv_fd = open("/proc/self/auxv", O_RDONLY);
+ Internal_Elfx_auxv_t auxv;
+
+ if (auxv_fd == -1)
+ return 0;
+
+ errno = ENOENT;
+ while (read(auxv_fd, &auxv, sizeof(auxv)) == sizeof(auxv)) {
+ if (auxv.a_type == type) {
+ errno = 0;
+ val = auxv.a_un.a_val;
+ if (str)
+ val = strcmp((const char *)val, str);
+ break;
+ }
+ }
+ close(auxv_fd);
+ }
+
+ return val;
+}
+
+unsigned long
+rte_cpu_getauxval(unsigned long type)
+{
+ return _rte_cpu_getauxval(type, NULL);
+}
+
+int
+rte_cpu_strcmp_auxval(unsigned long type, const char *str)
+{
+ return _rte_cpu_getauxval(type, str);
+}
+
/**
* Checks if the machine is adequate for running the binary. If it is not, the
* program exits with status 1.
diff --git a/lib/librte_eal/common/include/generic/rte_cpuflags.h b/lib/librte_eal/common/include/generic/rte_cpuflags.h
index 8d31687d8..156ea0029 100644
--- a/lib/librte_eal/common/include/generic/rte_cpuflags.h
+++ b/lib/librte_eal/common/include/generic/rte_cpuflags.h
@@ -64,4 +64,25 @@ rte_cpu_check_supported(void);
int
rte_cpu_is_supported(void);
+/**
+ * This function attempts to retrieve a value from the auxiliary vector.
+ * If it is unsuccessful, the result will be 0, and errno will be set.
+ *
+ * @return A value from the auxiliary vector. When the value is 0, check
+ * errno to determine if an error occurred.
+ */
+unsigned long
+rte_cpu_getauxval(unsigned long type);
+
+/**
+ * This function retrieves a value from the auxiliary vector, and compares it
+ * as a string against the value retrieved.
+ *
+ * @return The result of calling strcmp() against the value retrieved from
+ * the auxiliary vector. When the value is 0 (meaning a match is found),
+ * check errno to determine if an error occurred.
+ */
+int
+rte_cpu_strcmp_auxval(unsigned long type, const char *str);
+
#endif /* _RTE_CPUFLAGS_H_ */
--
2.14.3
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-dev] [PATCH v2] eal: abstract away the auxiliary vector
2018-04-02 18:24 ` [dpdk-dev] [PATCH v2] " Aaron Conole
@ 2018-04-11 21:34 ` Thomas Monjalon
2018-04-25 2:36 ` Thomas Monjalon
0 siblings, 1 reply; 6+ messages in thread
From: Thomas Monjalon @ 2018-04-11 21:34 UTC (permalink / raw)
To: dev
Cc: Aaron Conole, Timothy Redaelli, Bruce Richardson, Jan Viktorin,
Jianbo Liu, Jerin Jacob, Chao Zhu, Keith Wiles
Anyone to review, please?
02/04/2018 20:24, Aaron Conole:
> Rather than attempting to load the contents of the auxv directly,
> prefer to use an exposed API - and if that doesn't exist then attempt
> to load the vector. This is because on some systems, when a user
> is downgraded, the /proc/self/auxv file retains the old ownership
> and permissions. The original method of /proc/self/auxv is retained.
>
> This also removes a potential abort() in the code when compiled with
> NDEBUG. A quick parse of the code shows that many (if not all) of
> the CPU flag parsing isn't used internally, so it should be okay.
>
> Signed-off-by: Aaron Conole <aconole@redhat.com>
> Signed-off-by: Timothy Redaelli <tredaelli@redhat.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-dev] [PATCH v2] eal: abstract away the auxiliary vector
2018-04-11 21:34 ` Thomas Monjalon
@ 2018-04-25 2:36 ` Thomas Monjalon
0 siblings, 0 replies; 6+ messages in thread
From: Thomas Monjalon @ 2018-04-25 2:36 UTC (permalink / raw)
To: Aaron Conole, Timothy Redaelli
Cc: dev, Bruce Richardson, Jan Viktorin, Jianbo Liu, Jerin Jacob,
Chao Zhu, Keith Wiles
11/04/2018 23:34, Thomas Monjalon:
> Anyone to review, please?
>
> 02/04/2018 20:24, Aaron Conole:
> > Rather than attempting to load the contents of the auxv directly,
> > prefer to use an exposed API - and if that doesn't exist then attempt
> > to load the vector. This is because on some systems, when a user
> > is downgraded, the /proc/self/auxv file retains the old ownership
> > and permissions. The original method of /proc/self/auxv is retained.
> >
> > This also removes a potential abort() in the code when compiled with
> > NDEBUG. A quick parse of the code shows that many (if not all) of
> > the CPU flag parsing isn't used internally, so it should be okay.
> >
> > Signed-off-by: Aaron Conole <aconole@redhat.com>
> > Signed-off-by: Timothy Redaelli <tredaelli@redhat.com>
3 weeks without a single comment.
Applied with below fix, thanks
fix applied:
-#if RTE_ARCH_64
+#ifdef RTE_ARCH_64
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-04-25 2:36 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-22 15:35 [dpdk-dev] [PATCH] eal: abstract away the auxiliary vector Aaron Conole
2018-03-22 16:08 ` Wiles, Keith
2018-03-22 17:39 ` Aaron Conole
2018-04-02 18:24 ` [dpdk-dev] [PATCH v2] " Aaron Conole
2018-04-11 21:34 ` Thomas Monjalon
2018-04-25 2:36 ` Thomas Monjalon
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).