ARM: vDSO gettimeofday using generic timer architecture

arch/arm/kernel/vdso.c line 68

Quoted 7 lines · Nathan Lynch

arch/arm/kernel/vdso.c new file +7 -0

+	/* Grab the vDSO code pages. */
+	for (i = 0; i < vdso_pages; i++) {
+		pg = virt_to_page(&vdso_start + i*PAGE_SIZE);
+		ClearPageReserved(pg);
+		get_page(pg);
+		vdso_pagelist[i] = pg;
+	}

Why do we want to clear the reserved status? This looks over complicated to me.

Quoted 7 lines

arch/arm/kernel/vdso.c new file +7 -0

+
+	/* Sanity check the shared object header. */
+	vbase = vmap(vdso_pagelist, 1, 0, PAGE_KERNEL);
+	if (vbase == NULL) {
+		pr_err("Failed to map vDSO pagelist!\n");
+		return -ENOMEM;
+	} else if (memcmp(vbase, "\177ELF", 4)) {

arch/arm/kernel/vdso.c line 79

Quoted 4 lines

arch/arm/kernel/vdso.c new file +4 -0

+		pr_err("vDSO is not a valid ELF object!\n");
+		ret = -EINVAL;
+		goto unmap;
+	}

Why do we need to vmap() pages which are already accessible - vdso_start must be part of the kernel image, and therefore will be accessible via standard mappings.

arch/arm/kernel/vdso.c line 84

Quoted 5 lines

arch/arm/kernel/vdso.c new file +5 -0

+
+	/* Grab the vDSO data page. */
+	pg = virt_to_page(vdso_data);
+	get_page(pg);
+	vdso_pagelist[i] = pg;

Same goes for this.

Hi Russell,

Quoted 12 lines · Russell King - ARM Linux

arch/arm/kernel/vdso.c line 68

Quoted 7 lines · Nathan Lynch

arch/arm/kernel/vdso.c new file +7 -0

+	/* Grab the vDSO code pages. */
+	for (i = 0; i < vdso_pages; i++) {
+		pg = virt_to_page(&vdso_start + i*PAGE_SIZE);
+		ClearPageReserved(pg);
+		get_page(pg);
+		vdso_pagelist[i] = pg;
+	}

Why do we want to clear the reserved status? This looks over complicated to me.

This looks like it was inherited from the PowerPC code where the behaviour of set_pte_at would change dependent on whether or not the page was reserved (set_pte_at->set_pte_filter->maybe_pte_to_page). I think we can safely remove this from ARM and ARM64.

Quoted 7 lines

Quoted 7 lines

arch/arm/kernel/vdso.c new file +7 -0

+
+	/* Sanity check the shared object header. */
+	vbase = vmap(vdso_pagelist, 1, 0, PAGE_KERNEL);
+	if (vbase == NULL) {
+		pr_err("Failed to map vDSO pagelist!\n");
+		return -ENOMEM;
+	} else if (memcmp(vbase, "\177ELF", 4)) {

Quoted 9 lines

arch/arm/kernel/vdso.c line 79

Quoted 4 lines

arch/arm/kernel/vdso.c new file +4 -0

+		pr_err("vDSO is not a valid ELF object!\n");
+		ret = -EINVAL;
+		goto unmap;
+	}

Why do we need to vmap() pages which are already accessible - vdso_start must be part of the kernel image, and therefore will be accessible via standard mappings.

This is a dress rehersal for install_special_mapping more than anything. If we map the page, and look at the first 4 bytes, are they what we expect?

Quoted 16 lines · Steve Capper

Hi Russell,

Quoted 12 lines · Russell King - ARM Linux

arch/arm/kernel/vdso.c line 68

Quoted 7 lines · Nathan Lynch

arch/arm/kernel/vdso.c new file +7 -0

+	/* Grab the vDSO code pages. */
+	for (i = 0; i < vdso_pages; i++) {
+		pg = virt_to_page(&vdso_start + i*PAGE_SIZE);
+		ClearPageReserved(pg);
+		get_page(pg);
+		vdso_pagelist[i] = pg;
+	}

Why do we want to clear the reserved status? This looks over complicated to me.

Quoted 4 lines

This looks like it was inherited from the PowerPC code where the behaviour of set_pte_at would change dependent on whether or not the page was reserved (set_pte_at->set_pte_filter->maybe_pte_to_page). I think we can safely remove this from ARM and ARM64.

Great, so we can get rid of that and the get_page() on the vdso data page below.

Quoted 17 lines

Quoted 16 lines

arch/arm/kernel/vdso.c line 79

Quoted 11 lines

arch/arm/kernel/vdso.c new file +11 -0

+
+	/* Sanity check the shared object header. */
+	vbase = vmap(vdso_pagelist, 1, 0, PAGE_KERNEL);
+	if (vbase == NULL) {
+		pr_err("Failed to map vDSO pagelist!\n");
+		return -ENOMEM;
+	} else if (memcmp(vbase, "\177ELF", 4)) {
+		pr_err("vDSO is not a valid ELF object!\n");
+		ret = -EINVAL;
+		goto unmap;
+	}

Why do we need to vmap() pages which are already accessible - vdso_start must be part of the kernel image, and therefore will be accessible via standard mappings.

Quoted 3 lines

This is a dress rehersal for install_special_mapping more than anything. If we map the page, and look at the first 4 bytes, are they what we expect?

My point is that we can already view this page directly by dereferencing vdso_start - do we really need to perform this apparant test of the MMU? If the MMU isn't working in this way, we have much bigger and more fundamental problems...

arch/arm/kernel/vdso.c line 68

Quoted 7 lines · Nathan Lynch

arch/arm/kernel/vdso.c new file +7 -0

+	/* Grab the vDSO code pages. */
+	for (i = 0; i < vdso_pages; i++) {
+		pg = virt_to_page(&vdso_start + i*PAGE_SIZE);
+		ClearPageReserved(pg);
+		get_page(pg);
+		vdso_pagelist[i] = pg;
+	}

Why do we want to clear the reserved status? This looks over complicated to me.

arch/arm/kernel/vdso.c line 79

Quoted 11 lines

arch/arm/kernel/vdso.c new file +11 -0

+
+	/* Sanity check the shared object header. */
+	vbase = vmap(vdso_pagelist, 1, 0, PAGE_KERNEL);
+	if (vbase == NULL) {
+		pr_err("Failed to map vDSO pagelist!\n");
+		return -ENOMEM;
+	} else if (memcmp(vbase, "\177ELF", 4)) {
+		pr_err("vDSO is not a valid ELF object!\n");
+		ret = -EINVAL;
+		goto unmap;
+	}

Why do we need to vmap() pages which are already accessible - vdso_start must be part of the kernel image, and therefore will be accessible via standard mappings.

arch/arm/kernel/vdso/vgettimeofday.c line 65

Quoted 8 lines

arch/arm/kernel/vdso/vgettimeofday.c new file +8 -0

+static long clock_gettime_fallback(clockid_t _clkid, struct timespec *_ts)
+{
+	register struct timespec *ts asm("r1") = _ts;
+	register clockid_t clkid asm("r0") = _clkid;
+	register long ret asm ("r0");
+	register long nr asm("r7") = __NR_clock_gettime;
+
+	asm("swi #0" : "=r" (ret) : "r" (clkid), "r" (ts), "r" (nr) : "memory");

This isn't compatible with OABI, so either this must not be enabled when AEABI is disabled, or this needs to be fixed.

Quoted 12 lines · Russell King - ARM Linux

arch/arm/kernel/vdso.c line 68

Quoted 7 lines · Nathan Lynch

arch/arm/kernel/vdso.c new file +7 -0

+	/* Grab the vDSO code pages. */
+	for (i = 0; i < vdso_pages; i++) {
+		pg = virt_to_page(&vdso_start + i*PAGE_SIZE);
+		ClearPageReserved(pg);
+		get_page(pg);
+		vdso_pagelist[i] = pg;
+	}

Why do we want to clear the reserved status? This looks over complicated to me.

Quoted 15 lines

arch/arm/kernel/vdso.c line 79

Quoted 11 lines

arch/arm/kernel/vdso.c new file +11 -0

+
+	/* Sanity check the shared object header. */
+	vbase = vmap(vdso_pagelist, 1, 0, PAGE_KERNEL);
+	if (vbase == NULL) {
+		pr_err("Failed to map vDSO pagelist!\n");
+		return -ENOMEM;
+	} else if (memcmp(vbase, "\177ELF", 4)) {
+		pr_err("vDSO is not a valid ELF object!\n");
+		ret = -EINVAL;
+		goto unmap;
+	}

Why do we need to vmap() pages which are already accessible - vdso_start must be part of the kernel image, and therefore will be accessible via standard mappings.

Right, this stuff doesn't appear to be necessary. Removed the vmap, get_page, and ClearPageReserved calls for v3.

Quoted 8 lines

arch/arm/kernel/vdso/vgettimeofday.c line 65

Quoted 8 lines

arch/arm/kernel/vdso/vgettimeofday.c new file +8 -0

+static long clock_gettime_fallback(clockid_t _clkid, struct timespec *_ts)
+{
+	register struct timespec *ts asm("r1") = _ts;
+	register clockid_t clkid asm("r0") = _clkid;
+	register long ret asm ("r0");
+	register long nr asm("r7") = __NR_clock_gettime;
+
+	asm("swi #0" : "=r" (ret) : "r" (clkid), "r" (ts), "r" (nr) : "memory");

Quoted 3 lines · Nathan Lynch

Right, this stuff doesn't appear to be necessary. Removed the vmap, get_page, and ClearPageReserved calls for v3.

Can you make the corresponding change for arm64 too, please?

Quoted 8 lines

Quoted 8 lines

arch/arm/kernel/vdso/vgettimeofday.c line 65

Quoted 8 lines

arch/arm/kernel/vdso/vgettimeofday.c new file +8 -0

+static long clock_gettime_fallback(clockid_t _clkid, struct timespec *_ts)
+{
+	register struct timespec *ts asm("r1") = _ts;
+	register clockid_t clkid asm("r0") = _clkid;
+	register long ret asm ("r0");
+	register long nr asm("r7") = __NR_clock_gettime;
+
+	asm("swi #0" : "=r" (ret) : "r" (clkid), "r" (ts), "r" (nr) : "memory");

Might be worth making this volatile, rather than depend on the use of ret.

Also, placing both _clkid and ret into "r0" worries me slightly -- is GCC smart enough to realise that writing to ret kills _clkid?