> On Mar 22, 2018, at 10:35 AM, Aaron Conole <acon...@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 <acon...@redhat.com>
> Signed-off-by: Timothy Redaelli <tredae...@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
