From nobody Fri Jan 24 21:53:57 2025 X-Original-To: dev-commits-src-all@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4Yfs5x1WdJz5lT01 for ; Fri, 24 Jan 2025 21:54:13 +0000 (UTC) (envelope-from jrtc27@jrtc27.com) Received: from mail-wm1-f41.google.com (mail-wm1-f41.google.com [209.85.128.41]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "WR4" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4Yfs5w1b1hz3mdb for ; Fri, 24 Jan 2025 21:54:12 +0000 (UTC) (envelope-from jrtc27@jrtc27.com) Authentication-Results: mx1.freebsd.org; none Received: by mail-wm1-f41.google.com with SMTP id 5b1f17b1804b1-43625c4a50dso16856095e9.0 for ; Fri, 24 Jan 2025 13:54:12 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1737755650; x=1738360450; h=to:references:message-id:content-transfer-encoding:cc:date :in-reply-to:from:subject:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=+atPXhcjdb+wiLPqosVItcbvpbpO5RxaV2onX1zPZMU=; b=O0xXFUMR3EDptfvQYSdYzeexwNCZ2ouYHfJkwHjZtrOCopr2+j/ZsCbLtnCGbomXsi yGod0cgkCF9O47wZLlavxM5nXW9T0F1wou43ey2gWflXIj/arYx2esnnxXAc7/lAEWe4 HlcymZVKmyK0C1ZylU05LHy5uwYw05OvStnXg8f5idPpwnGW48ock5v63n8ZlZInHBFO PmAXX+Go6Vbl5HAzDu0WLtuQ/a+cDZgZWgJHKLm6YQYAG2tApG0LtKUMk34ZwgtZMbq4 za/rjKtI0nRySz/quLWGxcaBxskyfP6Ax668VU4Hy26Yk0xr0SII0sdeD3ujixzUCRjQ MwlA== X-Forwarded-Encrypted: i=1; AJvYcCUUU+VMMwryLoJjBt9edCmbnmgRwMgTE8IeAoPIw9Tbit8rafXbX7UUw91NCfddegZBqwRtZuIe4vL2eOVZi3jkwcl5@freebsd.org X-Gm-Message-State: AOJu0YxFOIjt0guOEZtsg16jqSMgrlqXYJ2cCboCnPlAhd6xtlbf5XhM AzFENxTPLEX9X449DFJyYmq0FdlGHumPQvKIxPSXj3pkxYgSToIt4hh0tuGcU8A= X-Gm-Gg: ASbGnctjV+FBTj7+VeSSsBH2SggZAF8n0fxakVe9YqfYQLCHXEIh6PsMpcpkunsBECg aAv2+9r4L53NFA6pTCGarjDicXlFHNDnbNCmNR8SEPyKWvVxVHufbyNNS77amKnl/ZGWg6XpmUk 6dXKHb1A/P4J+zM9gMnLyUnQpHWIoZoBB6jU4q2BldiJZCMjFHQLWGvQ/ExFAaxJZEx8QBHPT+s dgFkaoc19wux5RSl+/33eD/iOWHEdQPeLB5BulSbMVXjQO/1DRREdwgZg/Fcy5u/z+VYTqSNbzC amfxerIGh31aWcGI2XmH22T/nQ2H8g== X-Google-Smtp-Source: AGHT+IH2K2BTVALY2EBj5DUGShHoBclCbPlKKfietiKmkSNyyi17/FYJYxoE0I5H7nIgMepTqUa9aw== X-Received: by 2002:adf:e005:0:b0:38a:41f8:8abf with SMTP id ffacd0b85a97d-38bf566d296mr21829301f8f.31.1737755649484; Fri, 24 Jan 2025 13:54:09 -0800 (PST) Received: from smtpclient.apple ([131.111.5.201]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-38c2a176449sm3892529f8f.11.2025.01.24.13.54.08 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Fri, 24 Jan 2025 13:54:08 -0800 (PST) Content-Type: text/plain; charset=utf-8 List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: X-BeenThere: dev-commits-src-all@freebsd.org Sender: owner-dev-commits-src-all@FreeBSD.org Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3826.300.87.4.3\)) Subject: Re: git: 4d213c595ac3 - main - sys: use globals for the ELF kernel and module type strings From: Jessica Clarke In-Reply-To: <202501242137.50OLbEur094850@gitrepo.freebsd.org> Date: Fri, 24 Jan 2025 21:53:57 +0000 Cc: "src-committers@freebsd.org" , "dev-commits-src-all@freebsd.org" , "dev-commits-src-main@freebsd.org" Content-Transfer-Encoding: quoted-printable Message-Id: <5156861D-A6F2-4CB2-A0CD-C8568D3A6A6A@freebsd.org> References: <202501242137.50OLbEur094850@gitrepo.freebsd.org> To: Warner Losh X-Mailer: Apple Mail (2.3826.300.87.4.3) X-Rspamd-Queue-Id: 4Yfs5w1b1hz3mdb X-Spamd-Bar: ---- X-Rspamd-Pre-Result: action=no action; module=replies; Message is reply to one we originated X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[]; ASN(0.00)[asn:15169, ipnet:209.85.128.0/17, country:US] On 24 Jan 2025, at 21:37, Warner Losh wrote: >=20 > The branch main has been updated by imp: >=20 > URL: = https://cgit.FreeBSD.org/src/commit/?id=3D4d213c595ac3247a85cea5d3ea521db1= 4151a427 >=20 > commit 4d213c595ac3247a85cea5d3ea521db14151a427 > Author: Ahmad Khalifa > AuthorDate: 2024-08-24 15:12:52 +0000 > Commit: Warner Losh > CommitDate: 2025-01-24 21:29:39 +0000 >=20 > 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=E2=80=99s just for optimisation then that=E2=80=99s 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=E2=80=99s 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=E2=80=99t 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. >=20 > Also remove unnecessary "elfN module" checks. >=20 > 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(-) >=20 > 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, >=20 > fake_preload[i++] =3D MODINFO_NAME; > fake_preload[i++] =3D strlen("kernel") + 1; > - strcpy((char*)&fake_preload[i++], "kernel"); > + strcpy((char *)&fake_preload[i++], "kernel"); > i +=3D 1; > fake_preload[i++] =3D MODINFO_TYPE; > - fake_preload[i++] =3D strlen("elf kernel") + 1; > - strcpy((char*)&fake_preload[i++], "elf kernel"); > - i +=3D 2; > + fake_preload[i++] =3D strlen(preload_kerntype) + 1; > + strcpy((char *)&fake_preload[i], preload_kerntype); > + i +=3D howmany(fake_preload[i - 1], sizeof(uint32_t)); > fake_preload[i++] =3D MODINFO_ADDR; > fake_preload[i++] =3D sizeof(vm_offset_t); > fake_preload[i++] =3D 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"); >=20 > PRELOAD_PUSH_VALUE(uint32_t, MODINFO_TYPE); > - PRELOAD_PUSH_STRING("elf kernel"); > + PRELOAD_PUSH_STRING(preload_kerntype); >=20 > 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 =3D preload_search_info(modptr, MODINFO_SIZE); > dynptr =3D preload_search_info(modptr, > MODINFO_METADATA | MODINFOMD_DYNAMIC); > - if (type =3D=3D NULL || > - (strcmp(type, "elf" __XSTRING(__ELF_WORD_SIZE) " module") !=3D 0 = && > - strcmp(type, "elf module") !=3D 0)) > + if (type =3D=3D NULL || strcmp(type, preload_modtype) !=3D 0) > return (EFTYPE); > if (baseptr =3D=3D NULL || sizeptr =3D=3D NULL || dynptr =3D=3D 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 =3D (Elf_Shdr *)preload_search_info(modptr, MODINFO_METADATA | > MODINFOMD_SHDR); > - if (type =3D=3D NULL || (strcmp(type, "elf" = __XSTRING(__ELF_WORD_SIZE) > - " obj module") !=3D 0 && > - strcmp(type, "elf obj module") !=3D 0)) { > + if (type =3D=3D NULL || strcmp(type, preload_modtype_obj) !=3D 0) > return (EFTYPE); > - } > if (baseptr =3D=3D NULL || sizeptr =3D=3D NULL || hdr =3D=3D NULL || > shdr =3D=3D 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 =3D 0; > caddr_t preload_metadata, preload_kmdp; >=20 > +const char preload_modtype[] =3D MODTYPE; > +const char preload_kerntype[] =3D KERNTYPE; > +const char preload_modtype_obj[] =3D MODTYPE_OBJ; > + > void > preload_initkmdp(bool fatal) > { > - preload_kmdp =3D preload_search_by_type("elf kernel"); > + preload_kmdp =3D preload_search_by_type(preload_kerntype); >=20 > if (preload_kmdp =3D=3D 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) { >=20 > fake_preload[i++] =3D MODINFO_NAME; > fake_preload[i++] =3D strlen("kernel") + 1; > - strcpy((char*)&fake_preload[i], "kernel"); > + strcpy((char *)&fake_preload[i], "kernel"); > /* ['k' 'e' 'r' 'n'] ['e' 'l' '\0' ..] */ > i +=3D 2; >=20 > fake_preload[i++] =3D MODINFO_TYPE; > - fake_preload[i++] =3D strlen("elf kernel") + 1; > - strcpy((char*)&fake_preload[i], "elf kernel"); > - /* ['e' 'l' 'f' ' '] ['k' 'e' 'r' 'n'] ['e' 'l' '\0' ..] */ > - i +=3D 3; > + fake_preload[i++] =3D strlen(preload_kerntype) + 1; > + strcpy((char *)&fake_preload[i], preload_kerntype); > + i +=3D howmany(fake_preload[i - 1], sizeof(uint32_t)); >=20 > #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); >=20 > 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); >=20 > #endif /* _KERNEL */ >=20 > +/* > + * 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[]; >=20 > extern void * preload_fetch_addr(caddr_t _mod); > extern size_t preload_fetch_size(caddr_t _mod);