[comboot] Allow for tail recursion of COMBOOT images
authorMichael Brown <mcb30@etherboot.org>
Tue, 17 Feb 2009 00:47:35 +0000 (00:47 +0000)
committerMichael Brown <mcb30@etherboot.org>
Tue, 17 Feb 2009 00:47:35 +0000 (00:47 +0000)
Multi-level menus via COMBOOT rely on the COMBOOT program being able
to exit and invoke a new COMBOOT program (the next menu).  This works,
but rapidly (within about five iterations) runs out of space in gPXE's
internal stack, since each new image is executed in a new function
context.

Fix by allowing tail recursion between images; an image can now
specify a replacement image for itself, and image_exec() will perform
the necessary tail recursion.

src/arch/i386/image/com32.c
src/arch/i386/image/comboot.c
src/arch/i386/include/bits/errfile.h
src/arch/i386/include/comboot.h
src/arch/i386/interface/syslinux/comboot_call.c
src/core/image.c
src/image/script.c
src/include/gpxe/image.h

index da60462..27ccbc0 100644 (file)
@@ -70,20 +70,20 @@ static int com32_exec ( struct image *image ) {
                }
 
                DBGC ( image, "COM32 %p: available memory top = 0x%x\n",
-                        image, (int)avail_mem_top );
+                      image, avail_mem_top );
 
                assert ( avail_mem_top != 0 );
 
                com32_external_esp = phys_to_virt ( avail_mem_top );
 
                /* Hook COMBOOT API interrupts */
-               hook_comboot_interrupts( );
+               hook_comboot_interrupts();
 
-               /* Temporarily de-register image, so that a "boot" command
-                * doesn't throw us into an execution loop.  Hold a reference
-                * to avoid the image's being freed.
+               /* Unregister image, so that a "boot" command doesn't
+                * throw us into an execution loop.  We never
+                * reregister ourselves; COMBOOT images expect to be
+                * removed on exit.
                 */
-               image_get ( image );
                unregister_image ( image );
 
                __asm__ __volatile__ (
@@ -111,25 +111,31 @@ static int com32_exec ( struct image *image ) {
                        /* %6 */ "r" ( COM32_START_PHYS )
                :
                        "memory" );
+               DBGC ( image, "COM32 %p: returned\n", image );
                break;
 
-       case COMBOOT_RETURN_RUN_KERNEL:
-               DBGC ( image, "COM32 %p: returned to run kernel...\n", image );
-               comboot_run_kernel ( );
+       case COMBOOT_EXIT:
+               DBGC ( image, "COM32 %p: exited\n", image );
                break;
 
-       case COMBOOT_RETURN_EXIT:
+       case COMBOOT_EXIT_RUN_KERNEL:
+               DBGC ( image, "COM32 %p: exited to run kernel %p\n",
+                      image, comboot_replacement_image );
+               image->replacement = comboot_replacement_image;
+               image_autoload ( image->replacement );
                break;
 
-       }
-
-       comboot_force_text_mode ( );
+       case COMBOOT_EXIT_COMMAND:
+               DBGC ( image, "COM32 %p: exited after executing command\n",
+                      image );
+               break;
 
-       DBGC ( image, "COM32 %p returned\n", image );
+       default:
+               assert ( 0 );
+               break;
+       }
 
-       /* Re-register image and return */
-       register_image ( image );
-       image_put ( image );
+       comboot_force_text_mode();
 
        return 0;
 }
index 63d02c0..3ca9d28 100644 (file)
@@ -142,16 +142,16 @@ static int comboot_exec ( struct image *image ) {
                comboot_init_psp ( image, seg_userptr );
 
                /* Hook COMBOOT API interrupts */
-               hook_comboot_interrupts ( );
+               hook_comboot_interrupts();
 
                DBGC ( image, "executing 16-bit COMBOOT image at %4x:0100\n",
-                        COMBOOT_PSP_SEG );
+                      COMBOOT_PSP_SEG );
 
-               /* Temporarily de-register image, so that a "boot" command
-                * doesn't throw us into an execution loop.  Hold a reference
-                * to avoid the image's being freed.
+               /* Unregister image, so that a "boot" command doesn't
+                * throw us into an execution loop.  We never
+                * reregister ourselves; COMBOOT images expect to be
+                * removed on exit.
                 */
-               image_get ( image );
                unregister_image ( image );
 
                /* Store stack segment at 0x38 and stack pointer at 0x3A
@@ -180,26 +180,32 @@ static int comboot_exec ( struct image *image ) {
                                    "xorw %%bp, %%bp\n\t"
                                    "lret\n\t" )
                                         : : "r" ( COMBOOT_PSP_SEG ) : "eax" );
+               DBGC ( image, "COMBOOT %p: returned\n", image );
                break;
 
-       case COMBOOT_RETURN_RUN_KERNEL:
-               DBGC ( image, "COMBOOT %p: returned to run kernel...\n", image );
-               comboot_run_kernel ( );
+       case COMBOOT_EXIT:
+               DBGC ( image, "COMBOOT %p: exited\n", image );
                break;
 
-       case COMBOOT_RETURN_EXIT:
+       case COMBOOT_EXIT_RUN_KERNEL:
+               DBGC ( image, "COMBOOT %p: exited to run kernel %p\n",
+                      image, comboot_replacement_image );
+               image->replacement = comboot_replacement_image;
+               image_autoload ( image->replacement );
                break;
 
-       }
+       case COMBOOT_EXIT_COMMAND:
+               DBGC ( image, "COMBOOT %p: exited after executing command\n",
+                      image );
+               break;
 
-       comboot_force_text_mode ( );
+       default:
+               assert ( 0 );
+               break;
+       }
 
-       DBGC ( image, "COMBOOT %p returned\n", image );
+       comboot_force_text_mode();
 
-       /* Re-register image and return */
-       register_image ( image );
-       image_put ( image );
-       
        return 0;
 }
 
index 1723063..5ea8a31 100644 (file)
@@ -23,6 +23,7 @@
 #define ERRFILE_comboot        ( ERRFILE_ARCH | ERRFILE_IMAGE | 0x00070000 )
 #define ERRFILE_com32          ( ERRFILE_ARCH | ERRFILE_IMAGE | 0x00080000 )
 #define ERRFILE_comboot_resolv ( ERRFILE_ARCH | ERRFILE_IMAGE | 0x00090000 )
+#define ERRFILE_comboot_call   ( ERRFILE_ARCH | ERRFILE_IMAGE | 0x000a0000 )
 
 #define ERRFILE_undi            ( ERRFILE_ARCH | ERRFILE_NET | 0x00000000 )
 #define ERRFILE_undiload        ( ERRFILE_ARCH | ERRFILE_NET | 0x00010000 )
index 1fc3b71..4376650 100644 (file)
@@ -79,18 +79,14 @@ extern int comboot_resolv ( const char *name, struct in_addr *address );
 /* setjmp/longjmp context buffer used to return after loading an image */
 extern jmp_buf comboot_return;
 
-/* Command line to execute when returning via comboot_return 
- * with COMBOOT_RETURN_RUN_KERNEL
- */
-extern char *comboot_kernel_cmdline;
-
-/* Execute comboot_image_cmdline */
-extern void comboot_run_kernel ( );
+/* Replacement image when exiting with COMBOOT_EXIT_RUN_KERNEL */
+extern struct image *comboot_replacement_image;
 
 extern void *com32_external_esp;
 
-#define COMBOOT_RETURN_EXIT 1
-#define COMBOOT_RETURN_RUN_KERNEL 2
+#define COMBOOT_EXIT 1
+#define COMBOOT_EXIT_RUN_KERNEL 2
+#define COMBOOT_EXIT_COMMAND 3
 
 extern void comboot_force_text_mode ( void );
 
index 977d44f..02b2250 100644 (file)
@@ -35,6 +35,8 @@
 #include <gpxe/process.h>
 #include <gpxe/serial.h>
 #include <gpxe/init.h>
+#include <gpxe/image.h>
+#include <usr/imgmgmt.h>
 
 /** The "SYSLINUX" version string */
 static char __data16_array ( syslinux_version, [] ) = "gPXE " VERSION;
@@ -67,10 +69,8 @@ extern void int22_wrapper ( void );
 /* setjmp/longjmp context buffer used to return after loading an image */
 jmp_buf comboot_return;
 
-/* Command line to execute when returning via comboot_return 
- * with COMBOOT_RETURN_RUN_KERNEL
- */
-char *comboot_kernel_cmdline;
+/* Replacement image when exiting with COMBOOT_EXIT_RUN_KERNEL */
+struct image *comboot_replacement_image;
 
 /* Mode flags set by INT 22h AX=0017h */
 static uint16_t comboot_graphics_mode = 0;
@@ -154,58 +154,81 @@ void comboot_force_text_mode ( void ) {
 
 
 /**
- * Run the kernel specified in comboot_kernel_cmdline
+ * Fetch kernel and optional initrd
  */
-void comboot_run_kernel ( )
-{
-       char *initrd;
-
-       comboot_force_text_mode ( );
-
-       DBG ( "COMBOOT: executing image '%s'\n", comboot_kernel_cmdline );
+static int comboot_fetch_kernel ( char *kernel_file, char *cmdline ) {
+       struct image *kernel;
+       struct image *initrd = NULL;
+       char *initrd_file;
+       int rc;
 
        /* Find initrd= parameter, if any */
-       if ( ( initrd = strstr ( comboot_kernel_cmdline, "initrd=" ) ) ) {
-               char old_char = '\0';
-               char *initrd_end = strchr( initrd, ' ' );
-
-               /* Replace space after end of parameter
-                * with a nul terminator if this is not
-                * the last parameter
-                */
-               if ( initrd_end ) {
-                       old_char = *initrd_end;
-                       *initrd_end = '\0';
-               }
+       if ( ( initrd_file = strstr ( cmdline, "initrd=" ) ) != NULL ) {
+               char *initrd_end;
 
-               /* Replace = with space to get 'initrd filename'
-                * command suitable for system()
-                */
-               initrd[6] = ' ';
+               /* skip "initrd=" */
+               initrd_file += 7;
 
-               DBG( "COMBOOT: loading initrd '%s'\n", initrd );
+               /* Find terminating space, if any, and replace with NUL */
+               initrd_end = strchr ( initrd_file, ' ' );
+               if ( initrd_end )
+                       *initrd_end = '\0';
 
-               system ( initrd );
+               DBG ( "COMBOOT: fetching initrd '%s'\n", initrd_file );
 
-               /* Restore space after parameter */
-               if ( initrd_end ) {
-                       *initrd_end = old_char;
+               /* Allocate and fetch initrd */
+               initrd = alloc_image();
+               if ( ! initrd ) {
+                       DBG ( "COMBOOT: could not allocate initrd\n" );
+                       rc = -ENOMEM;
+                       goto err_alloc_initrd;
+               }
+               if ( ( rc = imgfetch ( initrd, initrd_file,
+                                      register_image ) ) != 0 ) {
+                       DBG ( "COMBOOT: could not fetch initrd: %s\n",
+                             strerror ( rc ) );
+                       goto err_fetch_initrd;
                }
 
-               /* Restore = */
-               initrd[6] = '=';
+               /* Restore space after initrd name, if applicable */
+               if ( initrd_end )
+                       *initrd_end = ' ';
        }
 
-       /* Load kernel */
-       DBG ( "COMBOOT: loading kernel '%s'\n", comboot_kernel_cmdline );
-       system ( comboot_kernel_cmdline );
+       DBG ( "COMBOOT: fetching kernel '%s'\n", kernel_file );
 
-       free ( comboot_kernel_cmdline );
+       /* Allocate and fetch kernel */
+       kernel = alloc_image();
+       if ( ! kernel ) {
+               DBG ( "COMBOOT: could not allocate kernel\n" );
+               rc = -ENOMEM;
+               goto err_alloc_kernel;
+       }
+       if ( ( rc = imgfetch ( kernel, kernel_file,
+                              register_image ) ) != 0 ) {
+               DBG ( "COMBOOT: could not fetch kernel: %s\n",
+                     strerror ( rc ) );
+               goto err_fetch_kernel;
+       }
+       if ( ( rc = image_set_cmdline ( kernel, cmdline ) ) != 0 ) {
+               DBG ( "COMBOOT: could not set kernel command line: %s\n",
+                     strerror ( rc ) );
+               goto err_set_cmdline;
+       }
 
-       /* Boot */
-       system ( "boot" );
+       /* Store kernel as replacement image */
+       comboot_replacement_image = kernel;
 
-       DBG ( "COMBOOT: back from executing command\n" );
+       return 0;
+
+ err_set_cmdline:
+ err_fetch_kernel:
+       image_put ( kernel );
+ err_alloc_kernel:
+ err_fetch_initrd:
+       image_put ( initrd );
+ err_alloc_initrd:
+       return rc;
 }
 
 
@@ -213,7 +236,7 @@ void comboot_run_kernel ( )
  * Terminate program interrupt handler
  */
 static __asmcall void int20 ( struct i386_all_regs *ix86 __unused ) {
-       longjmp ( comboot_return, COMBOOT_RETURN_EXIT );
+       longjmp ( comboot_return, COMBOOT_EXIT );
 }
 
 
@@ -226,7 +249,7 @@ static __asmcall void int21 ( struct i386_all_regs *ix86 ) {
        switch ( ix86->regs.ah ) {
        case 0x00:
        case 0x4C: /* Terminate program */
-               longjmp ( comboot_return, COMBOOT_RETURN_EXIT );
+               longjmp ( comboot_return, COMBOOT_EXIT );
                break;
 
        case 0x01: /* Get Key with Echo */
@@ -323,17 +346,15 @@ static __asmcall void int22 ( struct i386_all_regs *ix86 ) {
                        char cmd[len + 1];
                        copy_from_user ( cmd, cmd_u, 0, len + 1 );
                        DBG ( "COMBOOT: executing command '%s'\n", cmd );
-
-                       comboot_kernel_cmdline = strdup ( cmd );
-
-                       DBG ( "COMBOOT: returning to run image...\n" );
-                       longjmp ( comboot_return, COMBOOT_RETURN_RUN_KERNEL );
+                       system ( cmd );
+                       DBG ( "COMBOOT: exiting after executing command...\n" );
+                       longjmp ( comboot_return, COMBOOT_EXIT_COMMAND );
                }
                break;
 
        case 0x0004: /* Run default command */
                /* FIXME: just exit for now */
-               longjmp ( comboot_return, COMBOOT_RETURN_EXIT );
+               longjmp ( comboot_return, COMBOOT_EXIT_COMMAND );
                break;
 
        case 0x0005: /* Force text mode */
@@ -518,21 +539,21 @@ static __asmcall void int22 ( struct i386_all_regs *ix86 ) {
                        userptr_t cmd_u = real_to_user ( ix86->segs.es, ix86->regs.bx );
                        int file_len = strlen_user ( file_u, 0 );
                        int cmd_len = strlen_user ( cmd_u, 0 );
-                       char file[file_len + 1 + cmd_len + 7 + 1];
+                       char file[file_len + 1];
                        char cmd[cmd_len + 1];
 
-                       memcpy( file, "kernel ", 7 );
-                       copy_from_user ( file + 7, file_u, 0, file_len + 1 );
+                       copy_from_user ( file, file_u, 0, file_len + 1 );
                        copy_from_user ( cmd, cmd_u, 0, cmd_len + 1 );
-                       strcat ( file, " " );
-                       strcat ( file, cmd );
-
-                       DBG ( "COMBOOT: run kernel image '%s'\n", file );
 
-                       comboot_kernel_cmdline = strdup ( file );                       
-
-                       DBG ( "COMBOOT: returning to run image...\n" );
-                       longjmp ( comboot_return, COMBOOT_RETURN_RUN_KERNEL );
+                       DBG ( "COMBOOT: run kernel %s %s\n", file, cmd );
+                       comboot_fetch_kernel ( file, cmd );
+                       /* Technically, we should return if we
+                        * couldn't load the kernel, but it's not safe
+                        * to do that since we have just overwritten
+                        * part of the COMBOOT program's memory space.
+                        */
+                       DBG ( "COMBOOT: exiting to run kernel...\n" );
+                       longjmp ( comboot_return, COMBOOT_EXIT_RUN_KERNEL );
                }
                break;
 
index 440a68c..741b054 100644 (file)
@@ -53,6 +53,7 @@ static void free_image ( struct refcnt *refcnt ) {
 
        uri_put ( image->uri );
        ufree ( image->data );
+       image_put ( image->replacement );
        free ( image );
        DBGC ( image, "IMAGE %p freed\n", image );
 }
@@ -142,9 +143,9 @@ int register_image ( struct image *image ) {
  * @v image            Executable/loadable image
  */
 void unregister_image ( struct image *image ) {
+       DBGC ( image, "IMAGE %p unregistered\n", image );
        list_del ( &image->list );
        image_put ( image );
-       DBGC ( image, "IMAGE %p unregistered\n", image );
 }
 
 /**
@@ -237,6 +238,7 @@ int image_autoload ( struct image *image ) {
  * @ret rc             Return status code
  */
 int image_exec ( struct image *image ) {
+       struct image *replacement;
        struct uri *old_cwuri;
        int rc;
 
@@ -257,18 +259,40 @@ int image_exec ( struct image *image ) {
        old_cwuri = uri_get ( cwuri );
        churi ( image->uri );
 
+       /* Take out a temporary reference to the image.  This allows
+        * the image to unregister itself if necessary, without
+        * automatically freeing itself.
+        */
+       image_get ( image );
+
        /* Try executing the image */
        if ( ( rc = image->type->exec ( image ) ) != 0 ) {
                DBGC ( image, "IMAGE %p could not execute: %s\n",
                       image, strerror ( rc ) );
-               goto done;
+               /* Do not return yet; we still have clean-up to do */
        }
 
- done:
+       /* Pick up replacement image before we drop the original
+        * image's temporary reference.
+        */
+       if ( ( replacement = image->replacement ) != NULL )
+               image_get ( replacement );
+
+       /* Drop temporary reference to the original image */
+       image_put ( image );
+
        /* Reset current working directory */
        churi ( old_cwuri );
        uri_put ( old_cwuri );
 
+       /* Tail-recurse into replacement image, if one exists */
+       if ( replacement ) {
+               DBGC ( image, "IMAGE %p replacing self with IMAGE %p\n",
+                      image, replacement );
+               if ( ( rc = image_exec ( replacement ) ) != 0 )
+                       return rc;
+       }
+
        return rc;
 }
 
index fe72288..2d24274 100644 (file)
@@ -43,10 +43,8 @@ static int script_exec ( struct image *image ) {
        int rc;
 
        /* Temporarily de-register image, so that a "boot" command
-        * doesn't throw us into an execution loop.  Hold a reference
-        * to avoid the image's being freed.
+        * doesn't throw us into an execution loop.
         */
-       image_get ( image );
        unregister_image ( image );
 
        while ( offset < image->len ) {
@@ -80,7 +78,6 @@ static int script_exec ( struct image *image ) {
  done:
        /* Re-register image and return */
        register_image ( image );
-       image_put ( image );
        return rc;
 }
 
index 76dc3b8..0163b08 100644 (file)
@@ -46,6 +46,16 @@ struct image {
                userptr_t user;
                unsigned long ul;
        } priv;
+
+       /** Replacement image
+        *
+        * An image wishing to replace itself with another image (in a
+        * style similar to a Unix exec() call) should return from its
+        * exec() method with the replacement image set to point to
+        * the new image.  The new image must already be in a suitable
+        * state for execution.
+        */
+       struct image *replacement;
 };
 
 /** Image is loaded */
@@ -79,6 +89,10 @@ struct image_type {
         *
         * @v image             Loaded image
         * @ret rc              Return status code
+        *
+        * Note that the image may be invalidated by the act of
+        * execution, i.e. an image is allowed to choose to unregister
+        * (and so potentially free) itself.
         */
        int ( * exec ) ( struct image *image );
 };