Re: [PATCH v3 1/5] selftests/x86: Simplify the code to get vdso base address in sgx

[Tianjia Zhang <tianjia.zhang@linux.alibaba.com>]

Hi,

On 1/26/21 2:12 AM, Jarkko Sakkinen wrote:
> What the short summary is saying now, is that this commit would make the
> existing code to use vDSO base address. It's already doing that.
> 
> You could instead just "Use getauxval() to simplify the code".
> 
> Also, I'd prefer to properly use upper and lower case letter, e.g.  vDSO
> instead of vdso.
> 
> Reply-To:
> In-Reply-To: <20210124062907.88229-2-tianjia.zhang@linux.alibaba.com>
> 
> On Sun, Jan 24, 2021 at 02:29:03PM +0800, Tianjia Zhang wrote:
>> This patch uses the library function `getauxval(AT_SYSINFO_EHDR)`
>> instead of the custom function `vdso_get_base_addr` to obtain the
> 
> Use either double or single quotation mark instead of hyphen.
> 
>> base address of vDSO, which will simplify the code implementation.
>>
>> Signed-off-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com>
> 
> This needs to be imperative form, e.g. "Simplify the code implemntation
> by using getauxval() instead of a custom function."
> 
>> ---
>>   tools/testing/selftests/sgx/main.c | 24 ++++--------------------
>>   1 file changed, 4 insertions(+), 20 deletions(-)
>>
>> diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
>> index 724cec700926..365d01dea67b 100644
>> --- a/tools/testing/selftests/sgx/main.c
>> +++ b/tools/testing/selftests/sgx/main.c
>> @@ -15,6 +15,7 @@
>>   #include <sys/stat.h>
>>   #include <sys/time.h>
>>   #include <sys/types.h>
>> +#include <sys/auxv.h>
>>   #include "defines.h"
>>   #include "main.h"
>>   #include "../kselftest.h"
>> @@ -28,24 +29,6 @@ struct vdso_symtab {
>>   	Elf64_Word *elf_hashtab;
>>   };
>>   
>> -static void *vdso_get_base_addr(char *envp[])
>> -{
>> -	Elf64_auxv_t *auxv;
>> -	int i;
>> -
>> -	for (i = 0; envp[i]; i++)
>> -		;
>> -
>> -	auxv = (Elf64_auxv_t *)&envp[i + 1];
>> -
>> -	for (i = 0; auxv[i].a_type != AT_NULL; i++) {
>> -		if (auxv[i].a_type == AT_SYSINFO_EHDR)
>> -			return (void *)auxv[i].a_un.a_val;
>> -	}
>> -
>> -	return NULL;
>> -}
>> -
>>   static Elf64_Dyn *vdso_get_dyntab(void *addr)
>>   {
>>   	Elf64_Ehdr *ehdr = addr;
>> @@ -162,7 +145,7 @@ static int user_handler(long rdi, long rsi, long rdx, long ursp, long r8, long r
>>   	return 0;
>>   }
>>   
>> -int main(int argc, char *argv[], char *envp[])
>> +int main(int argc, char *argv[])
>>   {
>>   	struct sgx_enclave_run run;
>>   	struct vdso_symtab symtab;
>> @@ -203,7 +186,8 @@ int main(int argc, char *argv[], char *envp[])
>>   	memset(&run, 0, sizeof(run));
>>   	run.tcs = encl.encl_base;
>>   
>> -	addr = vdso_get_base_addr(envp);
>> +	/* Get vDSO base address */
>> +	addr = (void *)(uintptr_t)getauxval(AT_SYSINFO_EHDR);
> 
> You could just case the result the result directly to void *.
> 
>>   	if (!addr)
>>   		goto err;
>>   
>> -- 
>> 2.19.1.3.ge56e4f7
>>
>>
> 
> /Jarkko
> 

Thanks for your suggestions, I will fix it in the v4 patchset.

Best regards,
Tianjia