Re: git: 4d213c595ac3 - main - sys: use globals for the ELF kernel and module type strings

In reply to: Warner Losh : "git: 4d213c595ac3 - main - sys: use globals for the ELF kernel and module type strings"
Go to: [ bottom of page ] [ top of archives ] [ this month ]
From: Jessica Clarke <jrtc27_at_freebsd.org>
Date: Fri, 24 Jan 2025 21:53:57 UTC
On 24 Jan 2025, at 21:37, Warner Losh <imp@FreeBSD.org> wrote:
> 
> The branch main has been updated by imp:
> 
> URL: https://cgit.FreeBSD.org/src/commit/?id=4d213c595ac3247a85cea5d3ea521db14151a427
> 
> commit 4d213c595ac3247a85cea5d3ea521db14151a427
> Author:     Ahmad Khalifa <ahmadkhalifa570@gmail.com>
> AuthorDate: 2024-08-24 15:12:52 +0000
> Commit:     Warner Losh <imp@FreeBSD.org>
> CommitDate: 2025-01-24 21:29:39 +0000
> 
>    sys: use globals for the ELF kernel and module type strings

Why do we need the globals rather than just using the macros? String
literals, via the macros, have the advantage that they can be merged by
the linker, whereas initialising a global requires allocating a
non-overlapping object for it.

I see kib@ argued for doing this, but the justification is flawed.
Unless you explicitly pass -O0 to the linker, string literals are at
least as efficient as named globals. The only advantages of the named
global are:

(a) guaranteeing only one copy of the string exists (i.e. for when you
require address equality), but any worthwhile linker will perform that
optimisation for SHF_MERGE sections anyway so if it’s just for
optimisation then that’s not worthwhile

(b) guaranteeing the address is unique compared with other unrelated
instances of the string

The downside is it cannot be merged with other strings (and in some
cases you can end up with less efficient code due to symbol preemption,
though that’s not true here). That is, if you have:

    foo("more than a short string", "a short string");

then the string literals can, and with ld.lld -O2 (I believe the
default in GNU ld; LLD defaults to -O1 which only merges identical
strings) will, be combined such that the second is a pointer to the
tail substring of the first.

So I don’t see a good reason for using explicit globals here. Using the
macros directly should be just as performant whilst being simpler and
shorter.

Jess

>    Initialize the globals with macros so we can use the same values in the
>    loader.
> 
>    Also remove unnecessary "elfN module" checks.
> 
>    Reviewed by: imp, kib
>    Pull Request: https://github.com/freebsd/freebsd-src/pull/1394
> ---
> sys/arm/arm/machdep_boot.c     |  8 ++++----
> sys/arm64/arm64/machdep_boot.c |  2 +-
> sys/kern/link_elf.c            |  4 +---
> sys/kern/link_elf_obj.c        |  5 +----
> sys/kern/subr_module.c         |  6 +++++-
> sys/powerpc/powerpc/machdep.c  |  9 ++++-----
> sys/riscv/riscv/machdep.c      |  2 +-
> sys/sys/linker.h               | 11 +++++++++++
> 8 files changed, 28 insertions(+), 19 deletions(-)
> 
> diff --git a/sys/arm/arm/machdep_boot.c b/sys/arm/arm/machdep_boot.c
> index 534d34acabe2..e2416f86ad23 100644
> --- a/sys/arm/arm/machdep_boot.c
> +++ b/sys/arm/arm/machdep_boot.c
> @@ -356,12 +356,12 @@ fake_preload_metadata(struct arm_boot_params *abp __unused, void *dtb_ptr,
> 
> fake_preload[i++] = MODINFO_NAME;
> fake_preload[i++] = strlen("kernel") + 1;
> - strcpy((char*)&fake_preload[i++], "kernel");
> + strcpy((char *)&fake_preload[i++], "kernel");
> i += 1;
> fake_preload[i++] = MODINFO_TYPE;
> - fake_preload[i++] = strlen("elf kernel") + 1;
> - strcpy((char*)&fake_preload[i++], "elf kernel");
> - i += 2;
> + fake_preload[i++] = strlen(preload_kerntype) + 1;
> + strcpy((char *)&fake_preload[i], preload_kerntype);
> + i += howmany(fake_preload[i - 1], sizeof(uint32_t));
> fake_preload[i++] = MODINFO_ADDR;
> fake_preload[i++] = sizeof(vm_offset_t);
> fake_preload[i++] = KERNVIRTADDR;
> diff --git a/sys/arm64/arm64/machdep_boot.c b/sys/arm64/arm64/machdep_boot.c
> index 029ae23530ff..83bd74ea7317 100644
> --- a/sys/arm64/arm64/machdep_boot.c
> +++ b/sys/arm64/arm64/machdep_boot.c
> @@ -98,7 +98,7 @@ fake_preload_metadata(void *dtb_ptr, size_t dtb_size)
> PRELOAD_PUSH_STRING("kernel");
> 
> PRELOAD_PUSH_VALUE(uint32_t, MODINFO_TYPE);
> - PRELOAD_PUSH_STRING("elf kernel");
> + PRELOAD_PUSH_STRING(preload_kerntype);
> 
> PRELOAD_PUSH_VALUE(uint32_t, MODINFO_ADDR);
> PRELOAD_PUSH_VALUE(uint32_t, sizeof(vm_offset_t));
> diff --git a/sys/kern/link_elf.c b/sys/kern/link_elf.c
> index 23a924636ca2..53af1e164980 100644
> --- a/sys/kern/link_elf.c
> +++ b/sys/kern/link_elf.c
> @@ -895,9 +895,7 @@ link_elf_link_preload(linker_class_t cls, const char *filename,
> sizeptr = preload_search_info(modptr, MODINFO_SIZE);
> dynptr = preload_search_info(modptr,
>    MODINFO_METADATA | MODINFOMD_DYNAMIC);
> - if (type == NULL ||
> -    (strcmp(type, "elf" __XSTRING(__ELF_WORD_SIZE) " module") != 0 &&
> -     strcmp(type, "elf module") != 0))
> + if (type == NULL || strcmp(type, preload_modtype) != 0)
> return (EFTYPE);
> if (baseptr == NULL || sizeptr == NULL || dynptr == NULL)
> return (EINVAL);
> diff --git a/sys/kern/link_elf_obj.c b/sys/kern/link_elf_obj.c
> index a7c7d4826322..02fd4caffcd9 100644
> --- a/sys/kern/link_elf_obj.c
> +++ b/sys/kern/link_elf_obj.c
> @@ -365,11 +365,8 @@ link_elf_link_preload(linker_class_t cls, const char *filename,
>    MODINFOMD_ELFHDR);
> shdr = (Elf_Shdr *)preload_search_info(modptr, MODINFO_METADATA |
>    MODINFOMD_SHDR);
> - if (type == NULL || (strcmp(type, "elf" __XSTRING(__ELF_WORD_SIZE)
> -    " obj module") != 0 &&
> -    strcmp(type, "elf obj module") != 0)) {
> + if (type == NULL || strcmp(type, preload_modtype_obj) != 0)
> return (EFTYPE);
> - }
> if (baseptr == NULL || sizeptr == NULL || hdr == NULL ||
>    shdr == NULL)
> return (EINVAL);
> diff --git a/sys/kern/subr_module.c b/sys/kern/subr_module.c
> index 14272bd913f8..596961606577 100644
> --- a/sys/kern/subr_module.c
> +++ b/sys/kern/subr_module.c
> @@ -46,10 +46,14 @@
> vm_offset_t preload_addr_relocate = 0;
> caddr_t preload_metadata, preload_kmdp;
> 
> +const char preload_modtype[] = MODTYPE;
> +const char preload_kerntype[] = KERNTYPE;
> +const char preload_modtype_obj[] = MODTYPE_OBJ;
> +
> void
> preload_initkmdp(bool fatal)
> {
> - preload_kmdp = preload_search_by_type("elf kernel");
> + preload_kmdp = preload_search_by_type(preload_kerntype);
> 
> if (preload_kmdp == NULL && fatal)
> panic("unable to find kernel metadata");
> diff --git a/sys/powerpc/powerpc/machdep.c b/sys/powerpc/powerpc/machdep.c
> index 96f3b292854d..e9979712aa9c 100644
> --- a/sys/powerpc/powerpc/machdep.c
> +++ b/sys/powerpc/powerpc/machdep.c
> @@ -644,15 +644,14 @@ fake_preload_metadata(void) {
> 
> fake_preload[i++] = MODINFO_NAME;
> fake_preload[i++] = strlen("kernel") + 1;
> - strcpy((char*)&fake_preload[i], "kernel");
> + strcpy((char *)&fake_preload[i], "kernel");
> /* ['k' 'e' 'r' 'n'] ['e' 'l' '\0' ..] */
> i += 2;
> 
> fake_preload[i++] = MODINFO_TYPE;
> - fake_preload[i++] = strlen("elf kernel") + 1;
> - strcpy((char*)&fake_preload[i], "elf kernel");
> - /* ['e' 'l' 'f' ' '] ['k' 'e' 'r' 'n'] ['e' 'l' '\0' ..] */
> - i += 3;
> + fake_preload[i++] = strlen(preload_kerntype) + 1;
> + strcpy((char *)&fake_preload[i], preload_kerntype);
> + i += howmany(fake_preload[i - 1], sizeof(uint32_t));
> 
> #ifdef __powerpc64__
> /* Padding -- Fields start on u_long boundaries */
> diff --git a/sys/riscv/riscv/machdep.c b/sys/riscv/riscv/machdep.c
> index a17cff55f2cc..8d32d348df7d 100644
> --- a/sys/riscv/riscv/machdep.c
> +++ b/sys/riscv/riscv/machdep.c
> @@ -365,7 +365,7 @@ fake_preload_metadata(struct riscv_bootparams *rvbp)
> PRELOAD_PUSH_VALUE(uint32_t, MODINFO_NAME);
> PRELOAD_PUSH_STRING("kernel");
> PRELOAD_PUSH_VALUE(uint32_t, MODINFO_TYPE);
> - PRELOAD_PUSH_STRING("elf kernel");
> + PRELOAD_PUSH_STRING(preload_kerntype);
> 
> PRELOAD_PUSH_VALUE(uint32_t, MODINFO_ADDR);
> PRELOAD_PUSH_VALUE(uint32_t, sizeof(vm_offset_t));
> diff --git a/sys/sys/linker.h b/sys/sys/linker.h
> index 77dd437c7ffe..85c50be6c969 100644
> --- a/sys/sys/linker.h
> +++ b/sys/sys/linker.h
> @@ -220,6 +220,14 @@ void linker_kldload_unbusy(int flags);
> 
> #endif /* _KERNEL */
> 
> +/*
> + * ELF file types
> + */
> +#define KERNTYPE_MB "elf multiboot kernel"
> +#define KERNTYPE "elf kernel"
> +#define MODTYPE_OBJ "elf obj module"
> +#define MODTYPE "elf module"
> +
> /*
>  * Module information subtypes
>  */
> @@ -273,6 +281,9 @@ void linker_kldload_unbusy(int flags);
>  */
> extern vm_offset_t preload_addr_relocate;
> extern caddr_t preload_metadata, preload_kmdp;
> +extern const char preload_modtype[];
> +extern const char preload_kerntype[];
> +extern const char preload_modtype_obj[];
> 
> extern void * preload_fetch_addr(caddr_t _mod);
> extern size_t preload_fetch_size(caddr_t _mod);