From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by dpdk.org (Postfix) with ESMTP id 04FFC5F5D for ; Thu, 22 Mar 2018 18:39:34 +0100 (CET) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id ED8394068056; Thu, 22 Mar 2018 17:39:33 +0000 (UTC) Received: from dhcp-25.97.bos.redhat.com (unknown [10.18.25.61]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 53F5484433; Thu, 22 Mar 2018 17:39:30 +0000 (UTC) From: Aaron Conole To: "Wiles\, Keith" Cc: "dev\@dpdk.org" , Timothy Redaelli , "Richardson\, Bruce" , Jan Viktorin , Jianbo Liu , Jerin Jacob , Chao Zhu References: <20180322153549.17910-1-aconole@redhat.com> Date: Thu, 22 Mar 2018 13:39:29 -0400 In-Reply-To: (Keith Wiles's message of "Thu, 22 Mar 2018 16:08:16 +0000") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.0.90 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Scanned-By: MIMEDefang 2.79 on 10.11.54.5 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.5]); Thu, 22 Mar 2018 17:39:34 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.5]); Thu, 22 Mar 2018 17:39:34 +0000 (UTC) for IP:'10.11.54.5' DOMAIN:'int-mx05.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'aconole@redhat.com' RCPT:'' Subject: Re: [dpdk-dev] [PATCH] eal: abstract away the auxiliary vector X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 22 Mar 2018 17:39:35 -0000 "Wiles, Keith" writes: >> On Mar 22, 2018, at 10:35 AM, Aaron Conole 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 >> Signed-off-by: Timothy Redaelli >> --- >> 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 >> +#include >> #include >> +#include >> +#include >> +#include >> +#include >> + >> +#if defined(__GLIBC__) && defined(__GLIBC_PREREQ) >> +#if __GLIBC_PREREQ(2, 16) >> +#include >> +#define HAS_AUXV 1 >> +#endif >> +#endif >> >> #include >> #include >> >> +#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