From: Aaron Conole <aconole@redhat.com>
To: "Wiles\, Keith" <keith.wiles@intel.com>
Cc: "dev\@dpdk.org" <dev@dpdk.org>,
Timothy Redaelli <tredaelli@redhat.com>, "Richardson\,
Bruce" <bruce.richardson@intel.com>,
Jan Viktorin <viktorin@rehivetech.com>,
Jianbo Liu <jianbo.liu@arm.com>,
Jerin Jacob <jerin.jacob@caviumnetworks.com>,
Chao Zhu <chaozhu@linux.vnet.ibm.com>
Subject: Re: [dpdk-dev] [PATCH] eal: abstract away the auxiliary vector
Date: Thu, 22 Mar 2018 13:39:29 -0400 [thread overview]
Message-ID: <f7tpo3w1032.fsf@dhcp-25.97.bos.redhat.com> (raw)
In-Reply-To: <DA59090E-7F53-4AB9-AA4A-8ED8AC812A2C@intel.com> (Keith Wiles's message of "Thu, 22 Mar 2018 16:08:16 +0000")
"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
next prev parent reply other threads:[~2018-03-22 17:39 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-03-22 15:35 Aaron Conole
2018-03-22 16:08 ` Wiles, Keith
2018-03-22 17:39 ` Aaron Conole [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f7tpo3w1032.fsf@dhcp-25.97.bos.redhat.com \
--to=aconole@redhat.com \
--cc=bruce.richardson@intel.com \
--cc=chaozhu@linux.vnet.ibm.com \
--cc=dev@dpdk.org \
--cc=jerin.jacob@caviumnetworks.com \
--cc=jianbo.liu@arm.com \
--cc=keith.wiles@intel.com \
--cc=tredaelli@redhat.com \
--cc=viktorin@rehivetech.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).