Re: [PATCH v5 17/23] arm64:ilp32: add vdso-ilp32 and use for signal return

[Dr. Philipp Tomsich]

Yury,

this patch has been based on an earlier version of vdso and mainly adjusted to match
the requirements of commit 601255ae3c98fdeeee3a8bb4696425e4f868b4f1.

As these are mainly style-changes, please feel free to revise and adjust as needed.

Regards,
Philipp.

> On 01 Oct 2015, at 21:44, Yury Norov <ynorov@xxxxxxxxxxxxxxxxxx> wrote:
> 
> On Tue, Sep 29, 2015 at 11:06:13PM -0500, Nathan Lynch wrote:
>> On 09/29/2015 05:14 PM, Yury Norov wrote:
>>> From: Philipp Tomsich <philipp.tomsich@xxxxxxxxxxxxxxxxxxxxx>
>>> 
>>> Adjusted to move the move data page before code pages in sync with
>>> commit 601255ae3c98fdeeee3a8bb4696425e4f868b4f1
>> 
>> This commit message needs more information about how the ilp32 VDSO uses
>> the existing arm64 code.  I had to really hunt through the Makefile to
>> figure out what's going on.
>> 
>> The commit message should also identify the APIs that are supported.
>> The subject line mentions signal return, but gettimeofday, clock_gettime
>> and clock_getres are being added here too, and it is not obvious.
>> 
>> 
>>> Signed-off-by: Philipp Tomsich <philipp.tomsich@xxxxxxxxxxxxxxxxxxxxx>
>>> Signed-off-by: Christoph Muellner <christoph.muellner@xxxxxxxxxxxxxxxxxxxxx>
>>> Signed-off-by: Yury Norov <ynorov@xxxxxxxxxxxxxxxxxx>
>>> 
>>> create mode 100644 arch/arm64/kernel/vdso-ilp32/.gitignore
>>> create mode 100644 arch/arm64/kernel/vdso-ilp32/Makefile
>>> copy arch/arm64/{include/asm/vdso.h => kernel/vdso-ilp32/vdso-ilp32.S} (56%)
>>> create mode 100644 arch/arm64/kernel/vdso-ilp32/vdso-ilp32.lds.S
>> 
>> How are you invoking git-format-patch?  The copy detection in this case
>> is not conducive to review.
>> 
>> It looks like the existing arm64 vdso Makefile has been copied to
>> vdso-ilp32/ and adjusted for paths and naming.  While the gettimeofday
>> assembly implementation is reused, the build logic is duplicated.  x86
>> produces VDSOs for multiple ABIs with a single Makefile; is a similar
>> approach not appropriate for arm64?
>> 
>> 
>>> diff --git a/arch/arm64/kernel/vdso-ilp32/vdso-ilp32.lds.S b/arch/arm64/kernel/vdso-ilp32/vdso-ilp32.lds.S
>>> new file mode 100644
>>> index 0000000..ac8029b
>>> --- /dev/null
>>> +++ b/arch/arm64/kernel/vdso-ilp32/vdso-ilp32.lds.S
>>> @@ -0,0 +1,98 @@
>> 
>> [...]
>> 
>>> +#include <linux/const.h>
>>> +#include <asm/page.h>
>>> +#include <asm/vdso.h>
>>> +
>>> +/*OUTPUT_FORMAT("elf32-littleaarch64", "elf32-bigaarch64", "elf32-littleaarch64")
>>> +OUTPUT_ARCH(aarch64)
>>> +*/
>> 
>> If these lines aren't needed then omit them.
>> 
>> [...]
>> 
>> 
>>> +/*
>>> + * This controls what symbols we export from the DSO.
>>> + */
>>> +VERSION
>>> +{
>>> +	LINUX_2.6.39 {
>>> +	global:
>>> +		__kernel_rt_sigreturn;
>>> +		__kernel_gettimeofday;
>>> +		__kernel_clock_gettime;
>>> +		__kernel_clock_getres;
>>> +	local: *;
>>> +	};
>>> +}
>> 
>> Something that came up during review of arch/arm's VDSO code: consider
>> using version and names that match x86, i.e. LINUX_2.6, __vdso_gettimeofday.
>> 
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-June/267940.html
>> 
>> Using LINUX_2.6.39 for this code is nonsensical.
>> 
>> 
>>> diff --git a/arch/arm64/kernel/vdso.c b/arch/arm64/kernel/vdso.c
>>> index b239b9b..bed6cf1 100644
>>> --- a/arch/arm64/kernel/vdso.c
>>> +++ b/arch/arm64/kernel/vdso.c
>>> @@ -40,6 +40,12 @@ extern char vdso_start, vdso_end;
>>> static unsigned long vdso_pages;
>>> static struct page **vdso_pagelist;
>>> 
>>> +#ifdef CONFIG_ARM64_ILP32
>>> +extern char vdso_ilp32_start, vdso_ilp32_end;
>>> +static unsigned long vdso_ilp32_pages;
>>> +static struct page **vdso_ilp32_pagelist;
>>> +#endif
>>> +
>>> /*
>>>  * The vDSO data page.
>>>  */
>>> @@ -117,24 +123,29 @@ int aarch32_setup_vectors_page(struct linux_binprm *bprm, int uses_interp)
>>> }
>>> #endif /* CONFIG_AARCH32_EL0 */
>>> 
>>> -static struct vm_special_mapping vdso_spec[2];
>>> -
>>> -static int __init vdso_init(void)
>>> +static inline int __init vdso_init_common(char *vdso_start, char *vdso_end,
>> 
>> No inline please.
>> 
>> 
>>> +					  unsigned long *vdso_pagesp,
>>> +					  struct page ***vdso_pagelistp,
>>> +					  struct vm_special_mapping* vdso_spec)
>>> {
>> 
>> [...]
>> 
>>> int arch_setup_additional_pages(struct linux_binprm *bprm,
>>> 				int uses_interp)
>>> {
>>> 	struct mm_struct *mm = current->mm;
>>> 	unsigned long vdso_base, vdso_text_len, vdso_mapping_len;
>>> -	void *ret;
>>> +	void* ret;
>> 
>> Gratuitous (and incorrect) style change.
>> 
>> 
>>> +	unsigned long pages = vdso_pages;
>>> +	struct vm_special_mapping* spec = vdso_spec;
>> 
>> Incorrect style:                  *spec
> 
> Hi Nathan,
> 
> If Philipp Philipp Tomsich will not answer soon, I'll fix all this.
> 
> BR,
> Yury.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/