Re: [PATCH v4 14/22] modpost: emit module descriptors as assembly

[Lorenzo Stoakes (ARM)]

On Fri, Oct 16, 2026 at 02:14:16PM +0200, Petr Pavlu wrote:
> This version looks mostly ok to me, from a modules perspective. Below
> are a few nits plus one more useful comment about handling
> MODULE_ARCH_INIT.

Ack thanks :)

>
> > diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> > index 0fd43c8a89ea..9d96459e492c 100644
> > --- a/scripts/mod/modpost.c
> > +++ b/scripts/mod/modpost.c
...
> > +/* The equivalent of KSYMTAB_FUNC()/KSYMTAB_DATA(). */
> > +static void add_asm_ksymtab(struct buffer *buf, const struct symbol *sym)
> > +{
> > +	const char *name = sym->name;
> > +
> > +	add_asm_kstrtab(buf, sym);
> > +
> > +	buf_printf(buf, "\t.section \"___ksymtab+%s\", \"a\"\n", name);
> > +	buf_printf(buf, "\t.balign %d\n", KSYM_ALIGN);
> > +	buf_printf(buf, "__ksymtab_%s:\n", name);
> > +	add_asm_ksym_ref(buf, sym->is_func ? MOD_FUNC_PREFIX : "", name);
> > +	add_asm_ksym_ref(buf, "__kstrtab_", name);
> > +	add_asm_ksym_ref(buf, "__kstrtabns_", name);
> > +	buf_printf(buf, "\t.previous\n");
> > +
> > +	add_asm_kflagstab(buf, sym);
> > +}
>
> add_asm_kstrtab() and add_asm_kflagstab() are only called from
> add_asm_ksymtab() and their logic is fairly short. I suggest merging
> these functions.

Ack, done for v5.

>
> > +
> > +/* The equivalent of SYMBOL_CRC(). */
> > +static void add_asm_crc(struct buffer *buf, const struct symbol *sym)
> > +{
> > +	buf_printf(buf, "\t.section \"___kcrctab+%s\",\"a\"\n", sym->name);
> > +	buf_printf(buf, "\t.balign 4\n");
> > +	buf_printf(buf, "__crc_%s:\n", sym->name);
> > +	buf_printf(buf, "\t.long 0x%08x\n", sym->crc);
> > +	buf_printf(buf, "\t.previous\n");
> > +}
> > +
> > +static bool export_is_kept(const struct symbol *sym)
> > +{
> > +	return !trim_unused_exports || sym->used;
> > +}
>
> export_is_kept() is only called once from add_asm_exported_symbols().
> The condition can be inlined directly there.

Ack done.

> > +/*
> > + * The equivalent of:
> > + *
> > + *	__visible struct module __this_module __section(".gnu.linkonce.this_module")
> > + *		= { .name = KBUILD_MODNAME, .init = init_module, .exit = cleanup_module,
> > + *		    .arch = MODULE_ARCH_INIT };
> > + *
> > + * Everything not listed is zero, MODULE_ARCH_INIT included, except on m68k.
> > + */
> > +static void add_asm_this_module(struct buffer *buf, const struct module *mod,
> > +				const char *modname)
> > +{
> > +	struct this_module_field fields[THIS_MODULE_MAX_FIELDS];
> > +	const unsigned int nr_fields = get_this_module_fields(mod, modname, fields);
>
> The const qualifier can be removed from the nr_fields definition to
> match the usual kernel style.

Ack done.

> > +/*
> > + * The equivalent of:
> > + *	static const u32 ____version_ext_crcs[] __section("__version_ext_crcs") = { crc, ... };
> > + *	static const char ____version_ext_names[] __section("__version_ext_names") = "name\0" ...;
> > + *
> > + * for unresolved symbols.
> > + */
> > +static void add_asm_extended_versions(struct buffer *buf, struct module *mod)
> > +{
> > +	struct symbol *sym;
> > +
> > +	if (!extended_modversions)
> > +		return;
> > +
> > +	buf_printf(buf, "\n\t.section __version_ext_crcs,\"a\",%%progbits\n");
> > +	buf_printf(buf, "\t.balign 4\n");
> > +	list_for_each_entry(sym, &mod->unresolved_symbols, list) {
> > +		if (skip_unversioned(mod, sym))
> > +			continue;
> > +
> > +		buf_printf(buf, "\t.long 0x%08x\n", sym->crc);
> > +	}
> > +
> > +	/*
> > +	 * A symbol without a CRC was warned about above, and is skipped here
> > +	 * too so that the names line up with the CRCs.
> > +	 */
> > +	buf_printf(buf, "\t.section __version_ext_names,\"a\",%%progbits\n");
> > +	list_for_each_entry(sym, &mod->unresolved_symbols, list) {
> > +		if (!sym->module || !sym->crc_valid)
> > +			continue;
>
> This condition must match the logic in skip_unversioned(). It would be
> clearer to keep it only in one place by calling skip_unversioned() here
> and adding a "silent" parameter to the function.

Ack done.

>
> > diff --git a/scripts/mod/module-offsets.c b/scripts/mod/module-offsets.c
> > new file mode 100644
> > index 000000000000..a336dd47aa33
> > --- /dev/null
> > +++ b/scripts/mod/module-offsets.c

> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Layout of the structures modpost emits into *.mod.S, extracted from the
> > + * target headers as devicetable-offsets.c does for the device tables.
> > + */
>
> The second part of the comment, after the comma, is superfluous. It is
> expected that the various parts of modpost use consistent mechanisms
> where appropriate.

Ack done.

> > +int main(void)
> > +{
> > +	DEFINE(MOD_SIZEOF_LONG, sizeof(long));
> > +	DEFINE(MOD_PREL32_RELOCATIONS, IS_ENABLED(CONFIG_HAVE_ARCH_PREL32_RELOCATIONS));
> > +	DEFINE(MOD_FUNC_PLABEL, IS_ENABLED(CONFIG_PARISC) && IS_ENABLED(CONFIG_64BIT));
> > +
> > +	DEFINE(MOD_SIZEOF_struct_module, sizeof(struct module));
> > +	DEFINE(MOD_ALIGNOF_struct_module, __alignof__(struct module));
> > +	OFFSET(MOD_OFF_module_name, module, name);
> > +	OFFSET(MOD_OFF_module_init, module, init);
> > +#ifdef CONFIG_MODULE_UNLOAD
> > +	OFFSET(MOD_OFF_module_exit, module, exit);
> > +#endif
> > +#if defined(CONFIG_M68K) && defined(CONFIG_MMU)
> > +	/* MODULE_ARCH_INIT: the only architecture where it is not all zeroes. */
> > +	OFFSET(MOD_OFF_module_arch_fixup_start, module, arch.fixup_start);
> > +	OFFSET(MOD_OFF_module_arch_fixup_end, module, arch.fixup_end);
> > +#endif
>
> I feel it is a bit of a layering violation for modpost to know how
> individual architectures need to initialize module::arch.

Yeah that is kinda gross :)

>
> For now, this is probably ok. I would just remove MODULE_ARCH_INIT from
> include/linux/module.h and arch/m68k/include/asm/module.h, along with
> the comment referring to it in scripts/mod/modpost.c.

ack done.

>
> In the future, this could be avoided by having the m68k module support
> look up the .m68k_fixup section at runtime, as other architectures
> typically do for similar sections.

Yes, arch stuff should live with the arch-specific stuff.

--
Cheers, Lorenzo