[PATCH v1 0/3] Use CNTVCTSS_EL0 in gettimeofday()

[PATCH v1 0/3] Use CNTVCTSS_EL0 in gettimeofday()

[Joey Gouly]

Hi all,

This patchset adds support for using CNTVCTSS_EL0 in gettimeofday() in the vDSO.
This can improve the accuracy and efficiency of some applications that call
gettimeofday() a lot.

This has been tested on an internal platform and a publicly available platform
that has FEAT_ECV.

Thanks,
Joey

Joey Gouly (3):
  arm64: module: move find_section to header
  arm64: alternative: patch alternatives in the vDSO
  arm64: vdso: use SYS_CNTVCTSS_EL0 for gettimeofday

 arch/arm64/include/asm/module.h            | 15 +++++++++++++
 arch/arm64/include/asm/vdso.h              |  3 +++
 arch/arm64/include/asm/vdso/gettimeofday.h | 14 ++++++++----
 arch/arm64/kernel/alternative.c            | 25 ++++++++++++++++++++++
 arch/arm64/kernel/module.c                 | 15 -------------
 arch/arm64/kernel/vdso.c                   |  3 ---
 arch/arm64/kernel/vdso/vdso.lds.S          |  7 ++++++
 7 files changed, 60 insertions(+), 22 deletions(-)

-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 1/3] arm64: module: move find_section to header

[Joey Gouly]

Move it to the header so that the implementation can be shared
by the alternatives code.

Signed-off-by: Joey Gouly <joey.gouly@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/include/asm/module.h | 15 +++++++++++++++
 arch/arm64/kernel/module.c      | 15 ---------------
 2 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/arch/arm64/include/asm/module.h b/arch/arm64/include/asm/module.h
index 4e7fa2623896..22f1be47cc92 100644
--- a/arch/arm64/include/asm/module.h
+++ b/arch/arm64/include/asm/module.h
@@ -65,4 +65,19 @@ static inline bool plt_entry_is_initialized(const struct plt_entry *e)
 	return e->adrp || e->add || e->br;
 }
 
+static inline const Elf_Shdr *find_section(const Elf_Ehdr *hdr,
+				    const Elf_Shdr *sechdrs,
+				    const char *name)
+{
+	const Elf_Shdr *s, *se;
+	const char *secstrs = (void *)hdr + sechdrs[hdr->e_shstrndx].sh_offset;
+
+	for (s = sechdrs, se = sechdrs + hdr->e_shnum; s < se; s++) {
+		if (strcmp(name, secstrs + s->sh_name) == 0)
+			return s;
+	}
+
+	return NULL;
+}
+
 #endif /* __ASM_MODULE_H */
diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
index f2d4bb14bfab..76b41e4ca9fa 100644
--- a/arch/arm64/kernel/module.c
+++ b/arch/arm64/kernel/module.c
@@ -476,21 +476,6 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
 	return -ENOEXEC;
 }
 
-static const Elf_Shdr *find_section(const Elf_Ehdr *hdr,
-				    const Elf_Shdr *sechdrs,
-				    const char *name)
-{
-	const Elf_Shdr *s, *se;
-	const char *secstrs = (void *)hdr + sechdrs[hdr->e_shstrndx].sh_offset;
-
-	for (s = sechdrs, se = sechdrs + hdr->e_shnum; s < se; s++) {
-		if (strcmp(name, secstrs + s->sh_name) == 0)
-			return s;
-	}
-
-	return NULL;
-}
-
 static inline void __init_plt(struct plt_entry *plt, unsigned long addr)
 {
 	*plt = get_plt_entry(addr, plt);
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 2/3] arm64: alternative: patch alternatives in the vDSO

[Joey Gouly]

Make it possible to use alternatives in the vDSO, so that better
implementations can be used if possible.

Signed-off-by: Joey Gouly <joey.gouly@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Vincenzo Frascino <vincenzo.frascino@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
---
 arch/arm64/include/asm/vdso.h     |  3 +++
 arch/arm64/kernel/alternative.c   | 25 +++++++++++++++++++++++++
 arch/arm64/kernel/vdso.c          |  3 ---
 arch/arm64/kernel/vdso/vdso.lds.S |  7 +++++++
 4 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/include/asm/vdso.h b/arch/arm64/include/asm/vdso.h
index f99dcb94b438..b4ae32109932 100644
--- a/arch/arm64/include/asm/vdso.h
+++ b/arch/arm64/include/asm/vdso.h
@@ -26,6 +26,9 @@
 	(void *)(vdso_offset_##name - VDSO_LBASE + (unsigned long)(base)); \
 })
 
+extern char vdso_start[], vdso_end[];
+extern char vdso32_start[], vdso32_end[];
+
 #endif /* !__ASSEMBLY__ */
 
 #endif /* __ASM_VDSO_H */
diff --git a/arch/arm64/kernel/alternative.c b/arch/arm64/kernel/alternative.c
index 9bcaa5eacf16..a4036194a5cd 100644
--- a/arch/arm64/kernel/alternative.c
+++ b/arch/arm64/kernel/alternative.c
@@ -10,11 +10,14 @@
 
 #include <linux/init.h>
 #include <linux/cpu.h>
+#include <linux/elf.h>
 #include <asm/cacheflush.h>
 #include <asm/alternative.h>
 #include <asm/cpufeature.h>
 #include <asm/insn.h>
+#include <asm/module.h>
 #include <asm/sections.h>
+#include <asm/vdso.h>
 #include <linux/stop_machine.h>
 
 #define __ALT_PTR(a, f)		((void *)&(a)->f + (a)->f)
@@ -192,6 +195,27 @@ static void __nocfi __apply_alternatives(struct alt_region *region, bool is_modu
 	}
 }
 
+void apply_alternatives_vdso(unsigned long *feature_mask)
+{
+	struct alt_region region;
+	const struct elf64_hdr *hdr;
+	const struct elf64_shdr *shdr;
+	const struct elf64_shdr *alt;
+
+	hdr = (struct elf64_hdr *)vdso_start;
+	shdr = (void *)hdr + hdr->e_shoff;
+	alt = find_section(hdr, shdr, ".altinstructions");
+	if (!alt)
+		return;
+
+	region = (struct alt_region){
+		.begin	= (void *)hdr + alt->sh_offset,
+		.end	= (void *)hdr + alt->sh_offset + alt->sh_size,
+	};
+
+	__apply_alternatives(&region, false, feature_mask);
+}
+
 /*
  * We might be patching the stop_machine state machine, so implement a
  * really simple polling protocol here.
@@ -216,6 +240,7 @@ static int __apply_alternatives_multi_stop(void *unused)
 
 		BUG_ON(all_alternatives_applied);
 		__apply_alternatives(&region, false, remaining_capabilities);
+		apply_alternatives_vdso(remaining_capabilities);
 		/* Barriers provided by the cache flushing */
 		all_alternatives_applied = 1;
 	}
diff --git a/arch/arm64/kernel/vdso.c b/arch/arm64/kernel/vdso.c
index a61fc4f989b3..ac93a2ee9c07 100644
--- a/arch/arm64/kernel/vdso.c
+++ b/arch/arm64/kernel/vdso.c
@@ -29,9 +29,6 @@
 #include <asm/signal32.h>
 #include <asm/vdso.h>
 
-extern char vdso_start[], vdso_end[];
-extern char vdso32_start[], vdso32_end[];
-
 enum vdso_abi {
 	VDSO_ABI_AA64,
 	VDSO_ABI_AA32,
diff --git a/arch/arm64/kernel/vdso/vdso.lds.S b/arch/arm64/kernel/vdso/vdso.lds.S
index e69fb4aaaf3e..6028f1fe2d1c 100644
--- a/arch/arm64/kernel/vdso/vdso.lds.S
+++ b/arch/arm64/kernel/vdso/vdso.lds.S
@@ -48,6 +48,13 @@ SECTIONS
 	PROVIDE (_etext = .);
 	PROVIDE (etext = .);
 
+	. = ALIGN(4);
+	.altinstructions : {
+		__alt_instructions = .;
+		*(.altinstructions)
+		__alt_instructions_end = .;
+	}
+
 	.dynamic	: { *(.dynamic) }		:text	:dynamic
 
 	.rela.dyn	: ALIGN(8) { *(.rela .rela*) }
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 3/3] arm64: vdso: use SYS_CNTVCTSS_EL0 for gettimeofday

[Joey Gouly]

If FEAT_ECV is implemented, the self-synchronized counter CNTVCTSS_EL0 can
be used, removing the need for an ISB.

Signed-off-by: Joey Gouly <joey.gouly@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Vincenzo Frascino <vincenzo.frascino@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Andre Przywara <andre.przywara@arm.com>
---
 arch/arm64/include/asm/vdso/gettimeofday.h | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/include/asm/vdso/gettimeofday.h b/arch/arm64/include/asm/vdso/gettimeofday.h
index 4f7a629df81f..d8aff747019c 100644
--- a/arch/arm64/include/asm/vdso/gettimeofday.h
+++ b/arch/arm64/include/asm/vdso/gettimeofday.h
@@ -9,6 +9,8 @@
 
 #include <asm/barrier.h>
 #include <asm/unistd.h>
+#include <asm/sysreg.h>
+#include <asm/alternative.h>
 
 #define VDSO_HAS_CLOCK_GETRES		1
 
@@ -78,11 +80,15 @@ static __always_inline u64 __arch_get_hw_counter(s32 clock_mode,
 		return 0;
 
 	/*
-	 * This isb() is required to prevent that the counter value
+	 * If FEAT_ECV is available, use the self-synchronizing counter.
+	 * Otherwise the isb is required to prevent that the counter value
 	 * is speculated.
-	 */
-	isb();
-	asm volatile("mrs %0, cntvct_el0" : "=r" (res) :: "memory");
+	*/
+	asm volatile(ALTERNATIVE("isb\nmrs %0, cntvct_el0",
+			"nop\n" __mrs_s("%0", SYS_CNTVCTSS_EL0),
+			ARM64_HAS_ECV)
+		     : "=r" (res) :: "memory");
+
 	arch_counter_enforce_ordering(res);
 
 	return res;
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 2/3] arm64: alternative: patch alternatives in the vDSO

[Mark Rutland]

On Thu, Aug 25, 2022 at 11:20:24AM +0100, Joey Gouly wrote:
> Make it possible to use alternatives in the vDSO, so that better
> implementations can be used if possible.
> 
> Signed-off-by: Joey Gouly <joey.gouly@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Vincenzo Frascino <vincenzo.frascino@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> ---
>  arch/arm64/include/asm/vdso.h     |  3 +++
>  arch/arm64/kernel/alternative.c   | 25 +++++++++++++++++++++++++
>  arch/arm64/kernel/vdso.c          |  3 ---
>  arch/arm64/kernel/vdso/vdso.lds.S |  7 +++++++
>  4 files changed, 35 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/vdso.h b/arch/arm64/include/asm/vdso.h
> index f99dcb94b438..b4ae32109932 100644
> --- a/arch/arm64/include/asm/vdso.h
> +++ b/arch/arm64/include/asm/vdso.h
> @@ -26,6 +26,9 @@
>  	(void *)(vdso_offset_##name - VDSO_LBASE + (unsigned long)(base)); \
>  })
>  
> +extern char vdso_start[], vdso_end[];
> +extern char vdso32_start[], vdso32_end[];
> +
>  #endif /* !__ASSEMBLY__ */
>  
>  #endif /* __ASM_VDSO_H */
> diff --git a/arch/arm64/kernel/alternative.c b/arch/arm64/kernel/alternative.c
> index 9bcaa5eacf16..a4036194a5cd 100644
> --- a/arch/arm64/kernel/alternative.c
> +++ b/arch/arm64/kernel/alternative.c
> @@ -10,11 +10,14 @@
>  
>  #include <linux/init.h>
>  #include <linux/cpu.h>
> +#include <linux/elf.h>
>  #include <asm/cacheflush.h>
>  #include <asm/alternative.h>
>  #include <asm/cpufeature.h>
>  #include <asm/insn.h>
> +#include <asm/module.h>
>  #include <asm/sections.h>
> +#include <asm/vdso.h>
>  #include <linux/stop_machine.h>
>  
>  #define __ALT_PTR(a, f)		((void *)&(a)->f + (a)->f)
> @@ -192,6 +195,27 @@ static void __nocfi __apply_alternatives(struct alt_region *region, bool is_modu
>  	}
>  }
>  
> +void apply_alternatives_vdso(unsigned long *feature_mask)
> +{
> +	struct alt_region region;
> +	const struct elf64_hdr *hdr;
> +	const struct elf64_shdr *shdr;
> +	const struct elf64_shdr *alt;
> +
> +	hdr = (struct elf64_hdr *)vdso_start;
> +	shdr = (void *)hdr + hdr->e_shoff;
> +	alt = find_section(hdr, shdr, ".altinstructions");
> +	if (!alt)
> +		return;
> +
> +	region = (struct alt_region){
> +		.begin	= (void *)hdr + alt->sh_offset,
> +		.end	= (void *)hdr + alt->sh_offset + alt->sh_size,
> +	};
> +
> +	__apply_alternatives(&region, false, feature_mask);
> +}
> +
>  /*
>   * We might be patching the stop_machine state machine, so implement a
>   * really simple polling protocol here.
> @@ -216,6 +240,7 @@ static int __apply_alternatives_multi_stop(void *unused)
>  
>  		BUG_ON(all_alternatives_applied);
>  		__apply_alternatives(&region, false, remaining_capabilities);
> +		apply_alternatives_vdso(remaining_capabilities);

Since we didn't patch the vdso in apply_boot_alternatives(), using
`remaining_capabilities` means that we could in theory miss alternatives for
features which were detected on the boot CPU.

Since the VDSO cannot be concurrently executed within the kernel, we could
hoist the call to apply_alternatives_vdso() out of
__apply_alternatives_multi_stop(), and call it before the stop_machine in
apply_alternatives_all().

That would keep __apply_alternatives_multi_stop() simple (and easier to make
noinstr-safe), and we could use the same mask logic as
apply_alternatives_module(), e.g.

void apply_alternatives_vdso(void)
{
	DECLARE_BITMAP(all_capabilities, ARM64_NPATCHABLE);
	bitmap_fill(all_capabilities, ARM64_NPATCHABLE);

	struct alt_region region;
	const struct elf64_hdr *hdr;
	const struct elf64_shdr *shdr;
	const struct elf64_shdr *alt;

	hdr = (struct elf64_hdr *)vdso_start;
	shdr = (void *)hdr + hdr->e_shoff;
	alt = find_section(hdr, shdr, ".altinstructions");
	if (!alt)
	return;

	region = (struct alt_region){
		.begin	= (void *)hdr + alt->sh_offset,
		.end	= (void *)hdr + alt->sh_offset + alt->sh_size,
	};

	__apply_alternatives(&region, false, &all_capabilities[0]);
}

... does that sound ok to you?

Thanks,
Mark.

>  		/* Barriers provided by the cache flushing */
>  		all_alternatives_applied = 1;
>  	}
> diff --git a/arch/arm64/kernel/vdso.c b/arch/arm64/kernel/vdso.c
> index a61fc4f989b3..ac93a2ee9c07 100644
> --- a/arch/arm64/kernel/vdso.c
> +++ b/arch/arm64/kernel/vdso.c
> @@ -29,9 +29,6 @@
>  #include <asm/signal32.h>
>  #include <asm/vdso.h>
>  
> -extern char vdso_start[], vdso_end[];
> -extern char vdso32_start[], vdso32_end[];
> -
>  enum vdso_abi {
>  	VDSO_ABI_AA64,
>  	VDSO_ABI_AA32,
> diff --git a/arch/arm64/kernel/vdso/vdso.lds.S b/arch/arm64/kernel/vdso/vdso.lds.S
> index e69fb4aaaf3e..6028f1fe2d1c 100644
> --- a/arch/arm64/kernel/vdso/vdso.lds.S
> +++ b/arch/arm64/kernel/vdso/vdso.lds.S
> @@ -48,6 +48,13 @@ SECTIONS
>  	PROVIDE (_etext = .);
>  	PROVIDE (etext = .);
>  
> +	. = ALIGN(4);
> +	.altinstructions : {
> +		__alt_instructions = .;
> +		*(.altinstructions)
> +		__alt_instructions_end = .;
> +	}
> +
>  	.dynamic	: { *(.dynamic) }		:text	:dynamic
>  
>  	.rela.dyn	: ALIGN(8) { *(.rela .rela*) }
> -- 
> 2.17.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 1/3] arm64: module: move find_section to header

[Mark Rutland]

On Thu, Aug 25, 2022 at 11:20:23AM +0100, Joey Gouly wrote:
> Move it to the header so that the implementation can be shared
> by the alternatives code.
> 
> Signed-off-by: Joey Gouly <joey.gouly@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>

Acked-by: Mark Rutland <mark.rutland@arm.com>

Mark.

> ---
>  arch/arm64/include/asm/module.h | 15 +++++++++++++++
>  arch/arm64/kernel/module.c      | 15 ---------------
>  2 files changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/module.h b/arch/arm64/include/asm/module.h
> index 4e7fa2623896..22f1be47cc92 100644
> --- a/arch/arm64/include/asm/module.h
> +++ b/arch/arm64/include/asm/module.h
> @@ -65,4 +65,19 @@ static inline bool plt_entry_is_initialized(const struct plt_entry *e)
>  	return e->adrp || e->add || e->br;
>  }
>  
> +static inline const Elf_Shdr *find_section(const Elf_Ehdr *hdr,
> +				    const Elf_Shdr *sechdrs,
> +				    const char *name)
> +{
> +	const Elf_Shdr *s, *se;
> +	const char *secstrs = (void *)hdr + sechdrs[hdr->e_shstrndx].sh_offset;
> +
> +	for (s = sechdrs, se = sechdrs + hdr->e_shnum; s < se; s++) {
> +		if (strcmp(name, secstrs + s->sh_name) == 0)
> +			return s;
> +	}
> +
> +	return NULL;
> +}
> +
>  #endif /* __ASM_MODULE_H */
> diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
> index f2d4bb14bfab..76b41e4ca9fa 100644
> --- a/arch/arm64/kernel/module.c
> +++ b/arch/arm64/kernel/module.c
> @@ -476,21 +476,6 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
>  	return -ENOEXEC;
>  }
>  
> -static const Elf_Shdr *find_section(const Elf_Ehdr *hdr,
> -				    const Elf_Shdr *sechdrs,
> -				    const char *name)
> -{
> -	const Elf_Shdr *s, *se;
> -	const char *secstrs = (void *)hdr + sechdrs[hdr->e_shstrndx].sh_offset;
> -
> -	for (s = sechdrs, se = sechdrs + hdr->e_shnum; s < se; s++) {
> -		if (strcmp(name, secstrs + s->sh_name) == 0)
> -			return s;
> -	}
> -
> -	return NULL;
> -}
> -
>  static inline void __init_plt(struct plt_entry *plt, unsigned long addr)
>  {
>  	*plt = get_plt_entry(addr, plt);
> -- 
> 2.17.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 3/3] arm64: vdso: use SYS_CNTVCTSS_EL0 for gettimeofday

[Mark Rutland]

On Thu, Aug 25, 2022 at 11:20:25AM +0100, Joey Gouly wrote:
> If FEAT_ECV is implemented, the self-synchronized counter CNTVCTSS_EL0 can
> be used, removing the need for an ISB.
> 
> Signed-off-by: Joey Gouly <joey.gouly@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Vincenzo Frascino <vincenzo.frascino@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Andre Przywara <andre.przywara@arm.com>
> ---
>  arch/arm64/include/asm/vdso/gettimeofday.h | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/vdso/gettimeofday.h b/arch/arm64/include/asm/vdso/gettimeofday.h
> index 4f7a629df81f..d8aff747019c 100644
> --- a/arch/arm64/include/asm/vdso/gettimeofday.h
> +++ b/arch/arm64/include/asm/vdso/gettimeofday.h
> @@ -9,6 +9,8 @@
>  
>  #include <asm/barrier.h>
>  #include <asm/unistd.h>
> +#include <asm/sysreg.h>
> +#include <asm/alternative.h>

Nit: please sort this alphabetically.

>  
>  #define VDSO_HAS_CLOCK_GETRES		1
>  
> @@ -78,11 +80,15 @@ static __always_inline u64 __arch_get_hw_counter(s32 clock_mode,
>  		return 0;
>  
>  	/*
> -	 * This isb() is required to prevent that the counter value
> +	 * If FEAT_ECV is available, use the self-synchronizing counter.
> +	 * Otherwise the isb is required to prevent that the counter value
>  	 * is speculated.
> -	 */
> -	isb();
> -	asm volatile("mrs %0, cntvct_el0" : "=r" (res) :: "memory");
> +	*/
> +	asm volatile(ALTERNATIVE("isb\nmrs %0, cntvct_el0",
> +			"nop\n" __mrs_s("%0", SYS_CNTVCTSS_EL0),
> +			ARM64_HAS_ECV)
> +		     : "=r" (res) :: "memory");
> +

For legibility (and ease of future modification), could we please format this
with the fields all aligned, e.g.

	asm volatile(
	ALTERNATIVE("isb\n"
		    "mrs %0, cntvct_el0",
		    "nop\n"
		    __mrs_s("%0", SYS_CNTVCTSS_EL0),
		    ARM64_HAS_ECV)
	: "=r" (res)
	:
	: "memory");


Regardless:

	Acked-by: Mark Rutland <mark.rutland@arm.com>

Thanks,
Mark.

>  	arch_counter_enforce_ordering(res);
>  
>  	return res;
> -- 
> 2.17.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 2/3] arm64: alternative: patch alternatives in the vDSO

[Joey Gouly]

Hi Mark,

Thanks for the review!

On Thu, Aug 25, 2022 at 02:19:32PM +0100, Mark Rutland wrote:
> On Thu, Aug 25, 2022 at 11:20:24AM +0100, Joey Gouly wrote:
> > Make it possible to use alternatives in the vDSO, so that better
> > implementations can be used if possible.
> > 
> > Signed-off-by: Joey Gouly <joey.gouly@arm.com>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Will Deacon <will@kernel.org>
> > Cc: Vincenzo Frascino <vincenzo.frascino@arm.com>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > ---
> >  arch/arm64/include/asm/vdso.h     |  3 +++
> >  arch/arm64/kernel/alternative.c   | 25 +++++++++++++++++++++++++
> >  arch/arm64/kernel/vdso.c          |  3 ---
> >  arch/arm64/kernel/vdso/vdso.lds.S |  7 +++++++
> >  4 files changed, 35 insertions(+), 3 deletions(-)
> > 
[..]
> > +void apply_alternatives_vdso(unsigned long *feature_mask)
> > +{
> > +	struct alt_region region;
> > +	const struct elf64_hdr *hdr;
> > +	const struct elf64_shdr *shdr;
> > +	const struct elf64_shdr *alt;
> > +
> > +	hdr = (struct elf64_hdr *)vdso_start;
> > +	shdr = (void *)hdr + hdr->e_shoff;
> > +	alt = find_section(hdr, shdr, ".altinstructions");
> > +	if (!alt)
> > +		return;
> > +
> > +	region = (struct alt_region){
> > +		.begin	= (void *)hdr + alt->sh_offset,
> > +		.end	= (void *)hdr + alt->sh_offset + alt->sh_size,
> > +	};
> > +
> > +	__apply_alternatives(&region, false, feature_mask);
> > +}
> > +
> >  /*
> >   * We might be patching the stop_machine state machine, so implement a
> >   * really simple polling protocol here.
> > @@ -216,6 +240,7 @@ static int __apply_alternatives_multi_stop(void *unused)
> >  
> >  		BUG_ON(all_alternatives_applied);
> >  		__apply_alternatives(&region, false, remaining_capabilities);
> > +		apply_alternatives_vdso(remaining_capabilities);
> 
> Since we didn't patch the vdso in apply_boot_alternatives(), using
> `remaining_capabilities` means that we could in theory miss alternatives for
> features which were detected on the boot CPU.
> 
> Since the VDSO cannot be concurrently executed within the kernel, we could
> hoist the call to apply_alternatives_vdso() out of
> __apply_alternatives_multi_stop(), and call it before the stop_machine in
> apply_alternatives_all().
> 
> That would keep __apply_alternatives_multi_stop() simple (and easier to make
> noinstr-safe), and we could use the same mask logic as
> apply_alternatives_module(), e.g.
> 
> void apply_alternatives_vdso(void)
> {
> 	DECLARE_BITMAP(all_capabilities, ARM64_NPATCHABLE);
> 	bitmap_fill(all_capabilities, ARM64_NPATCHABLE);
> 
> 	struct alt_region region;
> 	const struct elf64_hdr *hdr;
> 	const struct elf64_shdr *shdr;
> 	const struct elf64_shdr *alt;
> 
> 	hdr = (struct elf64_hdr *)vdso_start;
> 	shdr = (void *)hdr + hdr->e_shoff;
> 	alt = find_section(hdr, shdr, ".altinstructions");
> 	if (!alt)
> 	return;
> 
> 	region = (struct alt_region){
> 		.begin	= (void *)hdr + alt->sh_offset,
> 		.end	= (void *)hdr + alt->sh_offset + alt->sh_size,
> 	};
> 
> 	__apply_alternatives(&region, false, &all_capabilities[0]);
> }
> 
> ... does that sound ok to you?

Yep, this makes sense, will send a v2 with this and the fixes for the other patch.

Thanks,
Joey

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel