Re: git: 86077f4fd110 - main - stand: use globals for the kernel and module types
- In reply to: Jessica Clarke : "Re: git: 86077f4fd110 - main - stand: use globals for the kernel and module types"
- Go to: [ bottom of page ] [ top of archives ] [ this month ]
Date: Fri, 24 Jan 2025 22:13:19 UTC
On Fri, Jan 24, 2025 at 2:55 PM Jessica Clarke <jrtc27@freebsd.org> wrote:
> 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=86077f4fd11070518a6d04eee7fdb93cbbfb1b52
> >
> > commit 86077f4fd11070518a6d04eee7fdb93cbbfb1b52
> > Author: Ahmad Khalifa <ahmadkhalifa570@gmail.com>
> > AuthorDate: 2024-08-24 15:16:09 +0000
> > Commit: Warner Losh <imp@FreeBSD.org>
> > CommitDate: 2025-01-24 21:29:39 +0000
> >
> > stand: use globals for the kernel and module types
>
> As with the kernel, please just use the macros directly.
>
Both are fine as is, which is why I didn't push the issue in the code
review.
Either a macro or global is fine, and doesn't materially change anything
doing it one way or the other. But if someone wants to convert them to
macros, that's cool too. It's likely marginally better, but both are
light-years
ahead of a dozen strings, some inexplicably different.
Warner
> Jess
>
> > Reviewed by: imp, kib
> > Pull Request: https://github.com/freebsd/freebsd-src/pull/1394
> > ---
> > stand/common/gfx_fb.c | 6 ++++--
> > stand/common/load_elf.c | 12 +++++-------
> > stand/common/load_elf_obj.c | 8 +++-----
> > stand/common/metadata.c | 2 +-
> > stand/common/modinfo.c | 5 +++++
> > stand/common/modinfo.h | 5 +++++
> > stand/efi/loader/arch/amd64/multiboot2.c | 9 +++++----
> > stand/efi/loader/bootinfo.c | 2 +-
> > stand/i386/libi386/bootinfo32.c | 2 +-
> > stand/i386/libi386/bootinfo64.c | 2 +-
> > stand/i386/libi386/multiboot.c | 9 +++++----
> > stand/powerpc/ofw/elf_freebsd.c | 2 +-
> > stand/powerpc/ofw/ppc64_elf_freebsd.c | 2 +-
> > stand/uboot/arch/powerpc/ppc64_elf_freebsd.c | 2 +-
> > stand/uboot/elf_freebsd.c | 2 +-
> > stand/userboot/userboot/bootinfo32.c | 2 +-
> > stand/userboot/userboot/bootinfo64.c | 2 +-
> > 17 files changed, 42 insertions(+), 32 deletions(-)
> >
> > diff --git a/stand/common/gfx_fb.c b/stand/common/gfx_fb.c
> > index 9bda0e7139a9..63036f3c07f6 100644
> > --- a/stand/common/gfx_fb.c
> > +++ b/stand/common/gfx_fb.c
> > @@ -102,6 +102,8 @@
> > #include <vbe.h>
> > #endif
> >
> > +#include "modinfo.h"
> > +
> > /* VGA text mode does use bold font. */
> > #if !defined(VGA_8X16_FONT)
> > #define VGA_8X16_FONT "/boot/fonts/8x16b.fnt"
> > @@ -2982,7 +2984,7 @@ build_font_module(vm_offset_t addr)
> >
> > fi.fi_checksum = -checksum;
> >
> > - fp = file_findfile(NULL, "elf kernel");
> > + fp = file_findfile(NULL, md_kerntype);
> > if (fp == NULL)
> > panic("can't find kernel file");
> >
> > @@ -3024,7 +3026,7 @@ build_splash_module(vm_offset_t addr)
> > return (addr);
> > }
> >
> > - fp = file_findfile(NULL, "elf kernel");
> > + fp = file_findfile(NULL, md_kerntype);
> > if (fp == NULL)
> > panic("can't find kernel file");
> >
> > diff --git a/stand/common/load_elf.c b/stand/common/load_elf.c
> > index d3775b9f0017..e19aefa121e7 100644
> > --- a/stand/common/load_elf.c
> > +++ b/stand/common/load_elf.c
> > @@ -37,6 +37,7 @@
> > #include <sys/link_elf.h>
> >
> > #include "bootstrap.h"
> > +#include "modinfo.h"
> >
> > #define COPYOUT(s,d,l) archsw.arch_copyout((vm_offset_t)(s), d, l)
> >
> > @@ -89,9 +90,6 @@ static int __elfN(parse_modmetadata)(struct
> preloaded_file *mp, elf_file_t ef,
> > static symaddr_fn __elfN(symaddr);
> > static char *fake_modname(const char *name);
> >
> > -const char *__elfN(kerneltype) = "elf kernel";
> > -const char *__elfN(moduletype) = "elf module";
> > -
> > uint64_t __elfN(relocation_offset) = 0;
> >
> > #ifdef __powerpc__
> > @@ -384,7 +382,7 @@ __elfN(loadfile_raw)(char *filename, uint64_t dest,
> > /*
> > * Check to see what sort of module we are.
> > */
> > - kfp = file_findfile(NULL, __elfN(kerneltype));
> > + kfp = file_findfile(NULL, md_kerntype);
> > #ifdef __powerpc__
> > /*
> > * Kernels can be ET_DYN, so just assume the first loaded object is the
> > @@ -435,7 +433,7 @@ __elfN(loadfile_raw)(char *filename, uint64_t dest,
> > err = EPERM;
> > goto oerr;
> > }
> > - if (strcmp(__elfN(kerneltype), kfp->f_type)) {
> > + if (strcmp(md_kerntype, kfp->f_type)) {
> > printf("elf" __XSTRING(__ELF_WORD_SIZE)
> > "_loadfile: can't load module with kernel type '%s'\n",
> > kfp->f_type);
> > @@ -470,9 +468,9 @@ __elfN(loadfile_raw)(char *filename, uint64_t dest,
> > fp->f_name = strdup(filename);
> > if (multiboot == 0)
> > fp->f_type = strdup(ef.kernel ?
> > - __elfN(kerneltype) : __elfN(moduletype));
> > + md_kerntype : md_modtype);
> > else
> > - fp->f_type = strdup("elf multiboot kernel");
> > + fp->f_type = strdup(md_kerntype_mb);
> >
> > if (module_verbose >= MODULE_VERBOSE_FULL) {
> > if (ef.kernel)
> > diff --git a/stand/common/load_elf_obj.c b/stand/common/load_elf_obj.c
> > index 9ff1d238b8c8..1e07828dd8ac 100644
> > --- a/stand/common/load_elf_obj.c
> > +++ b/stand/common/load_elf_obj.c
> > @@ -37,6 +37,7 @@
> > #include <sys/link_elf.h>
> >
> > #include "bootstrap.h"
> > +#include "modinfo.h"
> >
> > #define COPYOUT(s,d,l) archsw.arch_copyout((vm_offset_t)(s), d, l)
> >
> > @@ -77,9 +78,6 @@ static int __elfN(obj_parse_modmetadata)(struct
> preloaded_file *mp,
> > elf_file_t ef);
> > static Elf_Addr __elfN(obj_symaddr)(struct elf_file *ef, Elf_Size
> symidx);
> >
> > -const char *__elfN(obj_kerneltype) = "elf kernel";
> > -const char *__elfN(obj_moduletype) = "elf obj module";
> > -
> > /*
> > * Attempt to load the file (file) as an ELF module. It will be stored
> at
> > * (dest), and a pointer to a module structure describing the loaded
> object
> > @@ -154,7 +152,7 @@ __elfN(obj_loadfile)(char *filename, uint64_t dest,
> > }
> > #endif
> >
> > - kfp = file_findfile(NULL, __elfN(obj_kerneltype));
> > + kfp = file_findfile(NULL, md_kerntype);
> > if (kfp == NULL) {
> > printf("elf" __XSTRING(__ELF_WORD_SIZE)
> > "_obj_loadfile: can't load module before kernel\n");
> > @@ -178,7 +176,7 @@ __elfN(obj_loadfile)(char *filename, uint64_t dest,
> > goto out;
> > }
> > fp->f_name = strdup(filename);
> > - fp->f_type = strdup(__elfN(obj_moduletype));
> > + fp->f_type = strdup(md_modtype_obj);
> >
> > if (module_verbose > MODULE_VERBOSE_SILENT)
> > printf("%s ", filename);
> > diff --git a/stand/common/metadata.c b/stand/common/metadata.c
> > index 8962763061dc..22df6f175791 100644
> > --- a/stand/common/metadata.c
> > +++ b/stand/common/metadata.c
> > @@ -146,7 +146,7 @@ md_load_dual(char *args, vm_offset_t *modulep,
> vm_offset_t *dtb, int kern64)
> > #endif
> >
> > kernend = 0;
> > - kfp = file_findfile(NULL, "elf kernel");
> > + kfp = file_findfile(NULL, md_kerntype);
> > if (kfp == NULL)
> > panic("can't find kernel file");
> > file_addmetadata(kfp, MODINFOMD_HOWTO, sizeof howto, &howto);
> > diff --git a/stand/common/modinfo.c b/stand/common/modinfo.c
> > index 381bd9dfd719..d00548c91c57 100644
> > --- a/stand/common/modinfo.c
> > +++ b/stand/common/modinfo.c
> > @@ -109,6 +109,11 @@
> >
> > #define MOD_ALIGN(l) roundup(l, align)
> >
> > +const char md_modtype[] = MODTYPE;
> > +const char md_kerntype[] = KERNTYPE;
> > +const char md_modtype_obj[] = MODTYPE_OBJ;
> > +const char md_kerntype_mb[] = KERNTYPE_MB;
> > +
> > vm_offset_t
> > md_copymodules(vm_offset_t addr, bool kern64)
> > {
> > diff --git a/stand/common/modinfo.h b/stand/common/modinfo.h
> > index 967367beeeb0..d26129089fb6 100644
> > --- a/stand/common/modinfo.h
> > +++ b/stand/common/modinfo.h
> > @@ -6,6 +6,11 @@
> > #ifndef COMMON_MODINFO_H
> > #define COMMON_MODINFO_H
> >
> > +extern const char md_modtype[];
> > +extern const char md_kerntype[];
> > +extern const char md_modtype_obj[];
> > +extern const char md_kerntype_mb[];
> > +
> > int md_load(char *args, vm_offset_t *modulep, vm_offset_t *dtb);
> > int md_load64(char *args, vm_offset_t *modulep, vm_offset_t *dtb);
> >
> > diff --git a/stand/efi/loader/arch/amd64/multiboot2.c
> b/stand/efi/loader/arch/amd64/multiboot2.c
> > index d09b01fce1fc..eb7362293406 100644
> > --- a/stand/efi/loader/arch/amd64/multiboot2.c
> > +++ b/stand/efi/loader/arch/amd64/multiboot2.c
> > @@ -51,6 +51,7 @@
> > #include "bootstrap.h"
> > #include "multiboot2.h"
> > #include "loader_efi.h"
> > +#include "modinfo.h"
> >
> > extern int elf32_loadfile_raw(char *filename, uint64_t dest,
> > struct preloaded_file **result, int multiboot);
> > @@ -436,7 +437,7 @@ exec(struct preloaded_file *fp)
> > * module 0 module 1
> > */
> >
> > - fp = file_findfile(NULL, "elf kernel");
> > + fp = file_findfile(NULL, md_kerntype);
> > if (fp == NULL) {
> > printf("No FreeBSD kernel provided, aborting\n");
> > error = EINVAL;
> > @@ -498,7 +499,7 @@ obj_loadfile(char *filename, uint64_t dest, struct
> preloaded_file **result)
> > int error;
> >
> > /* See if there's a multiboot kernel loaded */
> > - mfp = file_findfile(NULL, "elf multiboot kernel");
> > + mfp = file_findfile(NULL, md_kerntype_mb);
> > if (mfp == NULL)
> > return (EFTYPE);
> >
> > @@ -506,14 +507,14 @@ obj_loadfile(char *filename, uint64_t dest, struct
> preloaded_file **result)
> > * We have a multiboot kernel loaded, see if there's a FreeBSD
> > * kernel loaded also.
> > */
> > - kfp = file_findfile(NULL, "elf kernel");
> > + kfp = file_findfile(NULL, md_kerntype);
> > if (kfp == NULL) {
> > /*
> > * No kernel loaded, this must be it. The kernel has to
> > * be loaded as a raw file, it will be processed by
> > * Xen and correctly loaded as an ELF file.
> > */
> > - rfp = file_loadraw(filename, "elf kernel", 0);
> > + rfp = file_loadraw(filename, md_kerntype, 0);
> > if (rfp == NULL) {
> > printf(
> > "Unable to load %s as a multiboot payload kernel\n",
> > diff --git a/stand/efi/loader/bootinfo.c b/stand/efi/loader/bootinfo.c
> > index b7d4070c2ffb..3e74a9228b5e 100644
> > --- a/stand/efi/loader/bootinfo.c
> > +++ b/stand/efi/loader/bootinfo.c
> > @@ -420,7 +420,7 @@ bi_load(char *args, vm_offset_t *modulep,
> vm_offset_t *kernendp, bool exit_bs)
> > addr += roundup(dtb_size, PAGE_SIZE);
> > #endif
> >
> > - kfp = file_findfile(NULL, "elf kernel");
> > + kfp = file_findfile(NULL, md_kerntype);
> > if (kfp == NULL)
> > panic("can't find kernel file");
> > kernend = 0; /* fill it in later */
> > diff --git a/stand/i386/libi386/bootinfo32.c
> b/stand/i386/libi386/bootinfo32.c
> > index df715e547795..37b227b913bd 100644
> > --- a/stand/i386/libi386/bootinfo32.c
> > +++ b/stand/i386/libi386/bootinfo32.c
> > @@ -129,7 +129,7 @@ bi_load32(char *args, int *howtop, int *bootdevp,
> vm_offset_t *bip, vm_offset_t
> > /* pad to a page boundary */
> > addr = roundup(addr, PAGE_SIZE);
> >
> > - kfp = file_findfile(NULL, "elf kernel");
> > + kfp = file_findfile(NULL, md_kerntype);
> > if (kfp == NULL)
> > panic("can't find kernel file");
> > kernend = 0; /* fill it in later */
> > diff --git a/stand/i386/libi386/bootinfo64.c
> b/stand/i386/libi386/bootinfo64.c
> > index 4731b10325fe..f7181dcd599f 100644
> > --- a/stand/i386/libi386/bootinfo64.c
> > +++ b/stand/i386/libi386/bootinfo64.c
> > @@ -143,7 +143,7 @@ bi_load64(char *args, vm_offset_t *modulep,
> > /* place the metadata before anything */
> > module = *modulep = addr;
> >
> > - kfp = file_findfile(NULL, "elf kernel");
> > + kfp = file_findfile(NULL, md_kerntype);
> > if (kfp == NULL)
> > panic("can't find kernel file");
> > kernend = 0; /* fill it in later */
> > diff --git a/stand/i386/libi386/multiboot.c
> b/stand/i386/libi386/multiboot.c
> > index b69895de9706..e2bd44fe83f5 100644
> > --- a/stand/i386/libi386/multiboot.c
> > +++ b/stand/i386/libi386/multiboot.c
> > @@ -48,6 +48,7 @@
> > #include "bootstrap.h"
> > #include "multiboot.h"
> > #include "libi386.h"
> > +#include "modinfo.h"
> > #include <btxv86.h>
> >
> > #define MULTIBOOT_SUPPORTED_FLAGS \
> > @@ -256,7 +257,7 @@ multiboot_exec(struct preloaded_file *fp)
> > * module 0 module 1
> > */
> >
> > - fp = file_findfile(NULL, "elf kernel");
> > + fp = file_findfile(NULL, md_kerntype);
> > if (fp == NULL) {
> > printf("No FreeBSD kernel provided, aborting\n");
> > error = EINVAL;
> > @@ -324,7 +325,7 @@ multiboot_obj_loadfile(char *filename, uint64_t dest,
> > int error, mod_num;
> >
> > /* See if there's a multiboot kernel loaded */
> > - mfp = file_findfile(NULL, "elf multiboot kernel");
> > + mfp = file_findfile(NULL, md_kerntype_mb);
> > if (mfp == NULL)
> > return (EFTYPE);
> >
> > @@ -332,14 +333,14 @@ multiboot_obj_loadfile(char *filename, uint64_t
> dest,
> > * We have a multiboot kernel loaded, see if there's a FreeBSD
> > * kernel loaded also.
> > */
> > - kfp = file_findfile(NULL, "elf kernel");
> > + kfp = file_findfile(NULL, md_kerntype);
> > if (kfp == NULL) {
> > /*
> > * No kernel loaded, this must be it. The kernel has to
> > * be loaded as a raw file, it will be processed by
> > * Xen and correctly loaded as an ELF file.
> > */
> > - rfp = file_loadraw(filename, "elf kernel", 0);
> > + rfp = file_loadraw(filename, md_kerntype, 0);
> > if (rfp == NULL) {
> > printf(
> > "Unable to load %s as a multiboot payload kernel\n",
> > diff --git a/stand/powerpc/ofw/elf_freebsd.c
> b/stand/powerpc/ofw/elf_freebsd.c
> > index 21ab834f76fa..4d34fa18c5dd 100644
> > --- a/stand/powerpc/ofw/elf_freebsd.c
> > +++ b/stand/powerpc/ofw/elf_freebsd.c
> > @@ -58,7 +58,7 @@ __elfN(ofw_loadfile)(char *filename, uint64_t dest,
> > * No need to sync the icache for modules: this will
> > * be done by the kernel after relocation.
> > */
> > - if (!strcmp((*result)->f_type, "elf kernel"))
> > + if (!strcmp((*result)->f_type, md_kerntype))
> > __syncicache((void *) (*result)->f_addr, (*result)->f_size);
> > #endif
> > return (0);
> > diff --git a/stand/powerpc/ofw/ppc64_elf_freebsd.c
> b/stand/powerpc/ofw/ppc64_elf_freebsd.c
> > index e0518abe2283..bc68d129f353 100644
> > --- a/stand/powerpc/ofw/ppc64_elf_freebsd.c
> > +++ b/stand/powerpc/ofw/ppc64_elf_freebsd.c
> > @@ -57,7 +57,7 @@ ppc64_ofw_elf_loadfile(char *filename, uint64_t dest,
> > * No need to sync the icache for modules: this will
> > * be done by the kernel after relocation.
> > */
> > - if (!strcmp((*result)->f_type, "elf kernel"))
> > + if (!strcmp((*result)->f_type, md_kerntype))
> > __syncicache((void *) (*result)->f_addr, (*result)->f_size);
> > return (0);
> > }
> > diff --git a/stand/uboot/arch/powerpc/ppc64_elf_freebsd.c
> b/stand/uboot/arch/powerpc/ppc64_elf_freebsd.c
> > index 291fe6b944fb..e500b862de2e 100644
> > --- a/stand/uboot/arch/powerpc/ppc64_elf_freebsd.c
> > +++ b/stand/uboot/arch/powerpc/ppc64_elf_freebsd.c
> > @@ -53,7 +53,7 @@ ppc64_uboot_elf_loadfile(char *filename, uint64_t dest,
> > * No need to sync the icache for modules: this will
> > * be done by the kernel after relocation.
> > */
> > - if (!strcmp((*result)->f_type, "elf kernel"))
> > + if (!strcmp((*result)->f_type, md_kerntype))
> > __syncicache((void *) (*result)->f_addr, (*result)->f_size);
> > return (0);
> > }
> > diff --git a/stand/uboot/elf_freebsd.c b/stand/uboot/elf_freebsd.c
> > index 6c764d143881..3b1bdc542538 100644
> > --- a/stand/uboot/elf_freebsd.c
> > +++ b/stand/uboot/elf_freebsd.c
> > @@ -53,7 +53,7 @@ __elfN(uboot_load)(char *filename, uint64_t dest,
> > * No need to sync the icache for modules: this will
> > * be done by the kernel after relocation.
> > */
> > - if (!strcmp((*result)->f_type, "elf kernel"))
> > + if (!strcmp((*result)->f_type, md_kerntype))
> > __syncicache((void *) (*result)->f_addr, (*result)->f_size);
> > #endif
> > return (0);
> > diff --git a/stand/userboot/userboot/bootinfo32.c
> b/stand/userboot/userboot/bootinfo32.c
> > index 91f1f81f1181..750574912172 100644
> > --- a/stand/userboot/userboot/bootinfo32.c
> > +++ b/stand/userboot/userboot/bootinfo32.c
> > @@ -108,7 +108,7 @@ bi_load32(char *args, int *howtop, int *bootdevp,
> vm_offset_t *bip, vm_offset_t
> > /* pad to a page boundary */
> > addr = roundup(addr, PAGE_SIZE);
> >
> > - kfp = file_findfile(NULL, "elf kernel");
> > + kfp = file_findfile(NULL, md_kerntype);
> > if (kfp == NULL)
> > panic("can't find kernel file");
> > kernend = 0; /* fill it in later */
> > diff --git a/stand/userboot/userboot/bootinfo64.c
> b/stand/userboot/userboot/bootinfo64.c
> > index fb9fd0fb82f1..d20202bf4fbb 100644
> > --- a/stand/userboot/userboot/bootinfo64.c
> > +++ b/stand/userboot/userboot/bootinfo64.c
> > @@ -140,7 +140,7 @@ bi_load64(char *args, vm_offset_t *modulep,
> vm_offset_t *kernendp)
> > /* pad to a page boundary */
> > addr = roundup(addr, PAGE_SIZE);
> >
> > - kfp = file_findfile(NULL, "elf kernel");
> > + kfp = file_findfile(NULL, md_kerntype);
> > if (kfp == NULL)
> > panic("can't find kernel file");
> > kernend = 0; /* fill it in later */
>
>