Provide registration mechanism for loaded images, so that we can e.g.

refer to them by name from the command line, or build them into a
multiboot module list.

Use setting image->type to disambiguate between "not my image" and "bad
image"; this avoids relying on specific values of the error code.

src/arch/i386/image/multiboot.c

@@ -24,6 +24,7 @@
  */
 
 #include <errno.h>
+#include <alloca.h>
 #include <multiboot.h>
 #include <gpxe/uaccess.h>
 #include <gpxe/image.h>
@@ -31,6 +32,8 @@
 #include <gpxe/memmap.h>
 #include <gpxe/elf.h>
 
+struct image_type multiboot_image_type __image_type;
+
 /** Multiboot flags that we support */
 #define MB_SUPPORTED_FLAGS ( MB_FLAG_PGALIGN | MB_FLAG_MEMMAP | \
 			     MB_FLAG_VIDMODE | MB_FLAG_RAW )
@@ -58,57 +61,116 @@ struct multiboot_header_info {
 };
 
 /**
- * Execute multiboot image
+ * Build multiboot memory map
  *
- * @v image		ELF file
- * @ret rc		Return status code
+ * @v mbinfo		Multiboot information structure
+ * @v mbmemmap		Multiboot memory map
  */
-static int multiboot_execute ( struct image *image ) {
-	static const char *bootloader_name = "gPXE " VERSION;
-	struct multiboot_info mbinfo;
+static void multiboot_build_memmap ( struct multiboot_info *mbinfo,
+				     struct multiboot_memory_map *mbmemmap ) {
 	struct memory_map memmap;
-	struct multiboot_memory_map mbmemmap[ sizeof ( memmap.regions ) /
-					      sizeof ( memmap.regions[0] ) ];
 	unsigned int i;
 
-	/* Populate multiboot information structure */
-	memset ( &mbinfo, 0, sizeof ( mbinfo ) );
-
-	/* Set boot loader name */
-	mbinfo.flags |= MBI_FLAG_LOADER;
-	mbinfo.boot_loader_name = virt_to_phys ( bootloader_name );
-	
 	/* Get memory map */
 	get_memmap ( &memmap );
-	memset ( mbmemmap, 0, sizeof ( mbmemmap ) );
+
+	/* Translate into multiboot format */
+	memset ( mbmemmap, 0, sizeof ( *mbmemmap ) );
 	for ( i = 0 ; i < memmap.count ; i++ ) {
 		mbmemmap[i].size = sizeof ( mbmemmap[i] );
 		mbmemmap[i].base_addr = memmap.regions[i].start;
 		mbmemmap[i].length = ( memmap.regions[i].end -
 				       memmap.regions[i].start );
 		mbmemmap[i].type = MBMEM_RAM;
-		mbinfo.mmap_length += sizeof ( mbmemmap[i] );
+		mbinfo->mmap_length += sizeof ( mbmemmap[i] );
 		if ( memmap.regions[i].start == 0 )
-			mbinfo.mem_lower = memmap.regions[i].end;
+			mbinfo->mem_lower = memmap.regions[i].end;
 		if ( memmap.regions[i].start == 0x100000 )
-			mbinfo.mem_upper = ( memmap.regions[i].end - 0x100000);
+			mbinfo->mem_upper = ( memmap.regions[i].end -
+					      0x100000 );
 	}
-	mbinfo.flags |= ( MBI_FLAG_MEM | MBI_FLAG_MMAP );
+}
+
+/**
+ * Build multiboot module list
+ *
+ * @v image		Multiboot image
+ * @v modules		Module list to fill, or NULL
+ * @ret count		Number of modules
+ */
+static unsigned int
+multiboot_build_module_list ( struct image *image,
+			      struct multiboot_module *modules ) {
+	struct image *module_image;
+	struct multiboot_module *module;
+	unsigned int count = 0;
+
+	for_each_image ( module_image ) {
+		/* Do not include kernel image as a module */
+		if ( module_image == image )
+			continue;
+		module = &modules[count++];
+		/* Populate module data structure, if applicable */
+		if ( ! modules )
+			continue;
+		module->mod_start = user_to_phys ( module_image->data, 0 );
+		module->mod_end = user_to_phys ( module_image->data,
+						 module_image->len );
+		if ( image->cmdline )
+			module->string = virt_to_phys ( image->cmdline );
+	}
+
+	return count;
+}
+
+/**
+ * Execute multiboot image
+ *
+ * @v image		Multiboot image
+ * @ret rc		Return status code
+ */
+static int multiboot_exec ( struct image *image ) {
+	static const char *bootloader_name = "gPXE " VERSION;
+	struct multiboot_info mbinfo;
+	struct multiboot_memory_map mbmemmap[MAX_MEMORY_REGIONS];
+	struct multiboot_module *modules;
+	unsigned int num_modules;
+
+	/* Populate multiboot information structure */
+	memset ( &mbinfo, 0, sizeof ( mbinfo ) );
+
+	/* Set boot loader name */
+	mbinfo.boot_loader_name = virt_to_phys ( bootloader_name );
+	mbinfo.flags |= MBI_FLAG_LOADER;
+	
+	/* Build memory map */
+	multiboot_build_memmap ( &mbinfo, mbmemmap );
 	mbinfo.mmap_addr = virt_to_phys ( &mbmemmap[0].base_addr );
+	mbinfo.flags |= ( MBI_FLAG_MEM | MBI_FLAG_MMAP );
 
 	/* Set command line, if present */
 	if ( image->cmdline ) {
-		mbinfo.flags |= MBI_FLAG_CMDLINE;
 		mbinfo.cmdline = virt_to_phys ( image->cmdline );
+		mbinfo.flags |= MBI_FLAG_CMDLINE;
 	}
 
+	/* Construct module list */
+	num_modules = multiboot_build_module_list ( image, NULL );
+	modules = alloca ( num_modules * sizeof ( *modules ) );
+	multiboot_build_module_list ( image, modules );
+	mbinfo.mods_count = num_modules;
+	mbinfo.mods_addr = virt_to_phys ( modules );
+	mbinfo.flags |= MBI_FLAG_MODS;
+
 	/* Jump to OS with flat physical addressing */
-	__asm__ __volatile__ ( PHYS_CODE ( "xchgw %%bx,%%bx\n\t"
-					   "call *%%edi\n\t" )
+	__asm__ __volatile__ ( PHYS_CODE ( /* Preserve %ebp for alloca() */
+					   "pushl %%ebp\n\t"
+					   "call *%%edi\n\t"
+					   "popl %%ebp\n\t" )
 			       : : "a" ( MULTIBOOT_BOOTLOADER_MAGIC ),
 			           "b" ( virt_to_phys ( &mbinfo ) ),
 			           "D" ( image->entry )
-			       : "ecx", "edx", "esi", "ebp" );
+			       : "ecx", "edx", "esi", "memory" );
 
 	return -ECANCELED;
 }
@@ -191,7 +253,6 @@ static int multiboot_load_raw ( struct image *image,
 
 	/* Record execution entry point */
 	image->entry = hdr->mb.entry_addr;
-	image->execute = multiboot_execute;
 
 	return 0;
 }
@@ -209,21 +270,9 @@ static int multiboot_load_elf ( struct image *image ) {
 	if ( ( rc = elf_load ( image ) ) != 0 ) {
 		DBG ( "Multiboot ELF image failed to load: %s\n",
 		      strerror ( rc ) );
-		/* We must translate "not an ELF image" (i.e. ENOEXEC)
-		 * into "invalid multiboot image", to avoid screwing
-		 * up the image probing logic.
-		 */
-		if ( rc == -ENOEXEC ) {
-			return -ENOTSUP;
-		} else {
-			return rc;
-		}
+		return rc;
 	}
 
-	/* Capture execution method */
-	if ( image->execute )
-		image->execute = multiboot_execute;
-
 	return 0;
 }
 
@@ -244,6 +293,10 @@ int multiboot_load ( struct image *image ) {
 	}
 	DBG ( "Found multiboot header with flags %08lx\n", hdr.mb.flags );
 
+	/* This is a multiboot image, valid or otherwise */
+	if ( ! image->type )
+		image->type = &multiboot_image_type;
+
 	/* Abort if we detect flags that we cannot support */
 	if ( hdr.mb.flags & MB_UNSUPPORTED_FLAGS ) {
 		DBG ( "Multiboot flags %08lx not supported\n",
@@ -267,4 +320,5 @@ int multiboot_load ( struct image *image ) {
 struct image_type multiboot_image_type __image_type = {
 	.name = "Multiboot",
 	.load = multiboot_load,
+	.exec = multiboot_exec,
 };

src/core/image.c

@@ -1,85 +1,138 @@
-#include "dev.h"
-#include <gpxe/buffer.h>
-#include <console.h>
+/*
+ * Copyright (C) 2006 Michael Brown <mbrown@fensystems.co.uk>.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of the
+ * License, or any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#include <stddef.h>
+#include <string.h>
+#include <stdlib.h>
+#include <errno.h>
+#include <assert.h>
+#include <vsprintf.h>
+#include <gpxe/list.h>
+#include <gpxe/image.h>
+
+/** @file
+ *
+ * Executable/loadable images
+ *
+ */
 
-#if 0
+/** List of registered images */
+struct list_head images = LIST_HEAD_INIT ( images );
 
-static struct image images[0] __image_start;
-static struct image images_end[0] __image_end;
+/** List of image types */
+static struct image_type image_types[0]
+	__table_start ( struct image_type, image_types );
+static struct image_type image_types_end[0]
+	__table_end ( struct image_type, image_types );
 
-/*
- * Print all images
+/**
+ * Register executable/loadable image
  *
+ * @v image		Executable/loadable image
+ * @ret rc		Return status code
  */
-void print_images ( void ) {
-	struct image *image;
+int register_image ( struct image *image ) {
+	static unsigned int imgindex = 0;
 
-	for ( image = images ; image < images_end ; image++ ) {
-		printf ( "%s ", image->name );
-	}
+	/* Create image name */
+	snprintf ( image->name, sizeof ( image->name ), "img%d",
+		   imgindex++ );
+
+	/* Add to image list */
+	list_add_tail ( &image->list, &images );
+	DBGC ( image, "IMAGE %p registered as %s\n", image, image->name );
+
+	return 0;
 }
 
-/*
- * Identify the image format
+/**
+ * Unregister executable/loadable image
  *
+ * @v image		Executable/loadable image
  */
-static struct image * identify_image ( physaddr_t start, physaddr_t len,
-				       void **context ) {
-	struct image *image;
-	
-	for ( image = images ; image < images_end ; image++ ) {
-		if ( image->probe ( start, len, context ) )
-			return image;
-	}
-	
-	return NULL;
+void unregister_image ( struct image *image ) {
+	list_del ( &image->list );
+	DBGC ( image, "IMAGE %p unregistered\n", image );
 }
 
-/*
- * Load an image into memory at a location determined by the image
- * format
+/**
+ * Load executable/loadable image into memory
  *
+ * @v image		Executable/loadable image
+ * @ret rc		Return status code
  */
-int autoload ( struct dev *dev, struct image **image, void **context ) {
-	struct buffer buffer;
-	int rc = 0;
-
-	/* Prepare the load buffer */
-	if ( ! init_load_buffer ( &buffer ) ) {
-		DBG ( "IMAGE could not initialise load buffer\n" );
-		goto out;
-	}
+int image_load ( struct image *image ) {
+	int rc;
 
-	/* Load the image into the load buffer */
-	if ( ! load ( dev, &buffer ) ) {
-		DBG ( "IMAGE could not load image\n" );
-		goto out_free;
+	assert ( image->type != NULL );
+
+	if ( ( rc = image->type->load ( image ) ) != 0 ) {
+		DBGC ( image, "IMAGE %p could not load: %s\n",
+		       image, strerror ( rc ) );
+		return rc;
 	}
 
-	/* Shrink the load buffer */
-	trim_load_buffer ( &buffer );
+	return 0;
+}
 
-	/* Identify the image type */
-	*image = identify_image ( buffer.start, buffer.fill, context );
-	if ( ! *image ) {
-		DBG ( "IMAGE could not identify image type\n" );
-		goto out_free;
-	}
+/**
+ * Autodetect image type and load executable/loadable image into memory
+ *
+ * @v image		Executable/loadable image
+ * @ret rc		Return status code
+ */
+int image_autoload ( struct image *image ) {
+	struct image_type *type;
+	int rc;
 
-	/* Move the image into the target location */
-	if ( ! (*image)->load ( buffer.start, buffer.fill, *context ) ) {
-		DBG ( "IMAGE could not move to target location\n" );
-		goto out_free;
+	for ( type = image_types ; type < image_types_end ; type++ ) {
+		rc = type->load ( image );
+		if ( image->type == NULL )
+			continue;
+		if ( rc != 0 ) {
+			DBGC ( image, "IMAGE %p (%s) could not load: %s\n",
+			       image, image->type->name, strerror ( rc ) );
+			return rc;
+		}
+		return 0;
 	}
 
-	/* Return success */
-	rc = 1;
-
- out_free:
-	/* Free the load buffer */
-	done_load_buffer ( &buffer );
- out:
-	return rc;
+	DBGC ( image, "IMAGE %p format not recognised\n", image );
+	return -ENOEXEC;
 }
 
-#endif
+/**
+ * Execute loaded image
+ *
+ * @v image		Loaded image
+ * @ret rc		Return status code
+ */
+int image_exec ( struct image *image ) {
+	int rc;
+
+	assert ( image->type != NULL );
+
+	if ( ( rc = image->type->exec ( image ) ) != 0 ) {
+		DBGC ( image, "IMAGE %p could not execute: %s\n",
+		       image, strerror ( rc ) );
+		return rc;
+	}
+
+	/* Well, some formats might return... */
+	return 0;
+}

src/image/elf.c

@@ -30,6 +30,8 @@
 #include <gpxe/image.h>
 #include <gpxe/elf.h>
 
+struct image_type elf_image_type __image_type;
+
 typedef Elf32_Ehdr	Elf_Ehdr;
 typedef Elf32_Phdr	Elf_Phdr;
 typedef Elf32_Off	Elf_Off;
@@ -40,7 +42,7 @@ typedef Elf32_Off	Elf_Off;
  * @v image		ELF file
  * @ret rc		Return status code
  */
-static int elf_execute ( struct image *image __unused ) {
+static int elf_exec ( struct image *image __unused ) {
 	return -ENOTSUP;
 }
 
@@ -63,7 +65,7 @@ static int elf_load_segment ( struct image *image, Elf_Phdr *phdr ) {
 	/* Check segment lies within image */
 	if ( ( phdr->p_offset + phdr->p_filesz ) > image->len ) {
 		DBG ( "ELF segment outside ELF file\n" );
-		return -ERANGE;
+		return -ENOEXEC;
 	}
 
 	/* Find start address: use physical address for preference,
@@ -75,7 +77,7 @@ static int elf_load_segment ( struct image *image, Elf_Phdr *phdr ) {
 		dest = phdr->p_vaddr;
 	if ( ! dest ) {
 		DBG ( "ELF segment loads to physical address 0\n" );
-		return -ERANGE;
+		return -ENOEXEC;
 	}
 	buffer = phys_to_user ( dest );
 
@@ -117,13 +119,17 @@ int elf_load ( struct image *image ) {
 		return -ENOEXEC;
 	}
 
+	/* This is an ELF image, valid or otherwise */
+	if ( ! image->type )
+		image->type = &elf_image_type;
+
 	/* Read ELF program headers */
 	for ( phoff = ehdr.e_phoff , phnum = ehdr.e_phnum ; phnum ;
 	      phoff += ehdr.e_phentsize, phnum-- ) {
 		if ( phoff > image->len ) {
 			DBG ( "ELF program header %d outside ELF image\n",
 			      phnum );
-			return -ERANGE;
+			return -ENOEXEC;
 		}
 		copy_from_user ( &phdr, image->data, phoff, sizeof ( phdr ) );
 		if ( ( rc = elf_load_segment ( image, &phdr ) ) != 0 )
@@ -132,7 +138,6 @@ int elf_load ( struct image *image ) {
 
 	/* Fill in entry point address */
 	image->entry = ehdr.e_entry;
-	image->execute = elf_execute;
 
 	return 0;
 }
@@ -141,4 +146,5 @@ int elf_load ( struct image *image ) {
 struct image_type elf_image_type __image_type = {
 	.name = "ELF",
 	.load = elf_load,
+	.exec = elf_exec,
 };

src/include/gpxe/image.h

@@ -4,37 +4,44 @@
 /**
  * @file
  *
- * Executable/loadable image formats
+ * Executable/loadable images
  *
  */
 
 #include <gpxe/tables.h>
+#include <gpxe/list.h>
+#include <gpxe/uaccess.h>
+
+struct image_type;
 
 /** An executable or loadable image */
 struct image {
+	/** Name */
+	char name[16];
+	/** List of registered images */
+	struct list_head list;
+
+	/** Command line to pass to image */
+	const char *cmdline;
+
 	/** Raw file image */
 	userptr_t data;
 	/** Length of raw file image */
 	size_t len;
 
-	/** Execute method
-	 *
-	 * Filled in by the image loader.  If NULL, then the image
-	 * cannot be executed.
-	 */
-	int ( * execute ) ( struct image *image );
 	/** Entry point */
 	physaddr_t entry;
 
-	/** Command line to pass to image */
-	const char *cmdline;
+	/** Image type, if known */
+	struct image_type *type;
 };
 
 /** An executable or loadable image type */
 struct image_type {
 	/** Name of this image type */
 	char *name;
-	/** Load image into memory
+	/**
+	 * Load image into memory
 	 *
 	 * @v image		Executable/loadable image
 	 * @ret rc		Return status code
@@ -44,15 +51,23 @@ struct image_type {
 	 * information it may require later (e.g. the execution
 	 * address) within the @c image structure.
 	 *
-	 * The method should return -ENOEXEC if and only if the image
-	 * is not in the correct format.  Other errors will be
-	 * interpreted as "I claim this image format, but there's
-	 * something wrong with it that makes it unloadable".  In
-	 * particular, returning -ENOEXEC will cause the image probing
-	 * code to try the next available image type, while returning
-	 * any other error will terminate image probing.
+	 * If the file image is in the correct format, the method must
+	 * update @c image->type to point to its own type (unless @c
+	 * image->type is already set).  This allows the autoloading
+	 * code to disambiguate between "this is not my image format"
+	 * and "there is something wrong with this image".  In
+	 * particular, setting @c image->type and then returning an
+	 * error will cause image_autoload() to abort and return an
+	 * error, rather than continuing to the next image type.
 	 */
 	int ( * load ) ( struct image *image );
+	/**
+	 * Execute loaded image
+	 *
+	 * @v image		Loaded image
+	 * @ret rc		Return status code
+	 */
+	int ( * exec ) ( struct image *image );
 };
 
 /** An executable or loadable image type */
@@ -68,4 +83,16 @@ struct image_type {
  */
 #define __default_image_type __table ( struct image_type, image_types, 02 )
 
+extern struct list_head images;
+
+/** Iterate over all registered images */
+#define for_each_image( image ) \
+	list_for_each_entry ( (image), &images, list )
+
+extern int register_image ( struct image *image );
+extern void unregister_image ( struct image *image );
+extern int image_load ( struct image *image );
+extern int image_autoload ( struct image *image );
+extern int image_exec ( struct image *image );
+
 #endif /* _GPXE_IMAGE_H */