[syslinux] [PATCH 0/1] EFI image booting capabilities
Gene Cumm
gene.cumm at gmail.com
Sun Mar 20 07:44:18 PDT 2016
On Fri, Feb 20, 2015 at 8:08 AM, Patrick Masotta via Syslinux
<syslinux at zytor.com> wrote:
> This patch adds to the core EFI image booting capabilities.
> It was tested on VMware EFI clients and HP Elitebook EFI notebooks,
> only on PXE environments but it should work on non-PXE scenarios as well.
>
> Feedback appreciated.
If you've made any changes to this, I'd definitely like to see it. If
you have any interest in working with git and want some help, feel
free to email me privately.
For starters, this deserves to be split, unrelated changes to their
own commits, the body and then finally a glue commit to put it
together. Changing existing functionality like this may have negative
consequences and splitting helps if a bisection is needed. I have a
very strong preference towards micro-commits of changing functionality
for this reason. It helps point to fewer lines of changes. On the
other hand, adding new functionality is sometimes better done in fewer
commits, often 1-2 and if two, the first adds the code while the
second adds the glue to tie it into the existing code.
> Signed-off-by: Patrick Masotta <masottaus at yahoo.com>
> ---
> diff -uprN a/com32/elflink/ldlinux/execute.c b/com32/elflink/ldlinux/execute.c
> --- a/com32/elflink/ldlinux/execute.c 2014-10-06 10:27:44.000000000 -0600
> +++ b/com32/elflink/ldlinux/execute.c 2015-02-18 18:46:02.193886584 -0700
> @@ -41,6 +41,7 @@ const struct image_types image_boot_type
> { "fdimage", IMAGE_TYPE_FDIMAGE },
> { "com32", IMAGE_TYPE_COM32 },
> { "config", IMAGE_TYPE_CONFIG },
> + { "efi", IMAGE_TYPE_EFI },
> { NULL, 0 },
> };
>
> @@ -89,6 +90,13 @@ __export void execute(const char *cmdlin
> do_sysappend(q);
> }
>
> +#ifdef __FIRMWARE_BIOS__
> + if(type==IMAGE_TYPE_EFI) {
> + printf("Bios core cannot load efi image %s\n",kernel);
> + return;
> + }
> +#endif
> +
> dprintf("kernel is %s, args = %s type = %d \n", kernel, args, type);
>
> if (kernel[0] == '.') {
> @@ -114,7 +122,9 @@ __export void execute(const char *cmdlin
> }
> }
>
> - if (type == IMAGE_TYPE_COM32) {
> + if (type == IMAGE_TYPE_EFI) {
> + new_efi_image((char *)kernel, (char *)args);
> + } else if (type == IMAGE_TYPE_COM32) {
> /*
> * We may be called with the console in an unknown
> * state, so initialise it.
Looks good. Glue.
> diff -uprN a/com32/elflink/ldlinux/kernel.c b/com32/elflink/ldlinux/kernel.c
> --- a/com32/elflink/ldlinux/kernel.c 2014-10-06 10:27:44.000000000 -0600
> +++ b/com32/elflink/ldlinux/kernel.c 2015-02-18 19:16:02.569004945 -0700
> @@ -1,3 +1,7 @@
> +/*
> + * EFI image boot capabilities by Patrick Masotta (Serva) (c)2015
> + */
> +
> #include <stdbool.h>
> #include <stdlib.h>
> #include <stdio.h>
> @@ -7,6 +11,7 @@
> #include <syslinux/loadfile.h>
> #include <syslinux/linux.h>
> #include <syslinux/pxe.h>
> +#include <syslinux/firmware.h>
> #include "core.h"
>
> const char *globaldefault = NULL;
> @@ -23,6 +28,9 @@ int new_linux_kernel(char *okernel, char
> bool opt_quiet = false;
> char *initrd_name, *cmdline;
>
> + if(firmware && firmware->clear_screen)
> + firmware->clear_screen();
> +
> dprintf("okernel = %s, ocmdline = %s", okernel, ocmdline);
>
> if (okernel)
The firmware NULL check seems first, unnecessary, and second, inconsistent.
> @@ -129,3 +137,70 @@ bail:
> printf("%s\n", strerror(errno));
> return 1;
> }
> +
> +
> +int new_efi_image(char *okernel, char *ocmdline)
> +{
> + const char *kernel_name = NULL, *args = NULL;
> + char *temp;
> + void *kernel_data;
> + size_t kernel_len, cmdline_len;
> + char *cmdline=NULL;
> +
> +
> + //lets clear the screen before loading the efi image
> + if(firmware && firmware->clear_screen)
> + firmware->clear_screen();
> +
> +
> + dprintf("okernel = %s, ocmdline = %s", okernel, ocmdline);
> +
> + if (okernel)
> + kernel_name = okernel;
> + else if (globaldefault)
> + kernel_name = globaldefault;
> +
> + if (ocmdline)
> + args = ocmdline;
> + else
> + if (append)
> + args = append;
> +
> + if(args!=NULL && *args!=0)
> + {
> + int i;
> + int args_len;
> + args_len = strlen(args);
> + cmdline_len = (args_len+1) * 2 ;
> + cmdline = malloc(cmdline_len);
> + if (!cmdline)
> + {
> + printf("Failed to alloc memory for cmdline\n");
> + return 1;
> + }
> + memset(cmdline,0,cmdline_len);
> + for(i=0; i < args_len ; i++) cmdline[2*i]=args[i];
> +
> + }
> + else
> + cmdline_len=0;
> +
> +
> + printf("Loading %s... ", kernel_name);
> + if (loadfile(kernel_name, &kernel_data, &kernel_len)) {
> + printf("failed: ");
> + goto bail;
> + }
> + printf("ok\n");
> +
> +
> + /* This should not return... */
> + syslinux_boot_efi(kernel_data, kernel_len, cmdline, cmdline_len);
> + printf("Booting efi image failed: ");
> +
> +bail:
> + if(cmdline)
> + free(cmdline);
> + printf("%s\n", strerror(errno));
> + return 1;
> +}
Body. Needs some styling.
> diff -uprN a/com32/elflink/ldlinux/ldlinux.c b/com32/elflink/ldlinux/ldlinux.c
> --- a/com32/elflink/ldlinux/ldlinux.c 2014-10-06 10:27:44.000000000 -0600
> +++ b/com32/elflink/ldlinux/ldlinux.c 2015-02-18 18:33:39.370445843 -0700
> @@ -28,6 +28,7 @@ static const struct file_ext file_extens
> { ".bin", IMAGE_TYPE_BOOT },
> { ".bs", IMAGE_TYPE_BOOT },
> { ".0", IMAGE_TYPE_PXE },
> + { ".efi", IMAGE_TYPE_EFI },
> { NULL, 0 },
> };
>
Looks good. Glue
> diff -uprN a/com32/include/syslinux/boot.h b/com32/include/syslinux/boot.h
> --- a/com32/include/syslinux/boot.h 2014-10-06 10:27:44.000000000 -0600
> +++ b/com32/include/syslinux/boot.h 2015-02-18 18:38:09.045503622 -0700
> @@ -67,6 +67,8 @@ extern const struct image_types image_bo
> #define IMAGE_TYPE_COM32 7
> #define IMAGE_TYPE_CONFIG 8
> #define IMAGE_TYPE_LOCALBOOT 9
> +#define IMAGE_TYPE_EFI 10
> +
>
> uint32_t parse_image_type(const char *cmdline);
> void syslinux_run_kernel_image(const char *filename, const char *cmdline,
Looks good. Body.
> diff -uprN a/com32/include/syslinux/firmware.h b/com32/include/syslinux/firmware.h
> --- a/com32/include/syslinux/firmware.h 2014-10-06 10:27:44.000000000 -0600
> +++ b/com32/include/syslinux/firmware.h 2015-02-18 18:36:46.155334267 -0700
> @@ -60,6 +60,8 @@ struct firmware {
> struct adv_ops *adv_ops;
> int (*boot_linux)(void *, size_t, struct initramfs *,
> struct setup_data *, char *);
> + int (*boot_efi)(void *, size_t, char *, int);
> + void (*clear_screen)(void);
> struct vesa_ops *vesa;
> struct mem_ops *mem;
> };
This change I was uncertain on at first. Extend the firmware struct
instead of exporting the functions and doing an #ifdef. It seems the
struct is the cleaner way considering the source is partially split.
Body.
> diff -uprN a/com32/lib/syslinux/load_efi.c b/com32/lib/syslinux/load_efi.c
> --- a/com32/lib/syslinux/load_efi.c 1969-12-31 17:00:00.000000000 -0700
> +++ b/com32/lib/syslinux/load_efi.c 2015-02-18 20:08:04.430708483 -0700
> @@ -0,0 +1,37 @@
> +/*
> + * EFI image boot capabilities by Patrick Masotta (Serva) (c)2015
> + */
> +
> +/*
> + * load_efi.c
> + *
> + * Load an efi image
> + */
Why not put the copyright as the 5th line of a single comment and let
the file name and description go first?
> +
> +#include <ctype.h>
> +#include <stdbool.h>
> +#include <stdlib.h>
> +#include <inttypes.h>
> +#include <string.h>
> +#include <minmax.h>
> +#include <errno.h>
> +#include <suffix_number.h>
> +#include <dprintf.h>
> +
> +#include <syslinux/align.h>
> +#include <syslinux/linux.h>
> +#include <syslinux/bootrm.h>
> +#include <syslinux/movebits.h>
> +#include <syslinux/firmware.h>
> +#include <syslinux/video.h>
> +
Seems like a lot of unnecessary #includes.
> +
> +
> +int syslinux_boot_efi(void *kernel_buf, size_t kernel_size,
> + char *cmdline, int cmdlineSize)
> +{
> + if (firmware->boot_efi)
> + return firmware->boot_efi(kernel_buf, kernel_size, cmdline, cmdlineSize);
> +
> + return -1;
> +}
Seems good otherwise.
> diff -uprN a/efi/main.c b/efi/main.c
> --- a/efi/main.c 2014-10-06 10:27:44.000000000 -0600
> +++ b/efi/main.c 2015-02-18 19:48:27.215899105 -0700
> @@ -1,4 +1,8 @@
> /*
> + * EFI image boot capabilities by Patrick Masotta (Serva) (c)2015
> + */
> +
> +/*
> * Copyright 2011-2014 Intel Corporation - All Rights Reserved
> */
I'm not a legal expert but I've more typically seen newer lines below
the old lines and as a part of the same comment.
> @@ -348,7 +352,11 @@ char efi_getchar(char *hi)
>
> do {
> status = uefi_call_wrapper(in->ReadKeyStroke, 2, in, &key);
> - } while (status == EFI_NOT_READY);
> + } while (status != EFI_NOT_READY);
> +
> + status = WaitForSingleEvent(in->WaitForKey, 0);
> + status = uefi_call_wrapper(in->ReadKeyStroke, 2, in, &key);
> +
>
> if (!key.ScanCode)
> return (char)key.UnicodeChar;
> @@ -356,10 +364,21 @@ char efi_getchar(char *hi)
> /*
> * We currently only handle scan codes that fit in 8 bits.
> */
> - *hi = (char)key.ScanCode;
> + if(hi)
> + *hi = (char)key.ScanCode;
> return 0;
> }
This pair of changes warrant their own commits.
For starters, I see you added a NULL check that should have been there
in the second chunk. However, the first chunk changes the underlying
behavior and warrants some notes for the commit message as to why it's
better, especially if the old behavior violated the EFI spec or caused
a busy-wait state.
> +void efi_clear_screen(void)
> +{
> +
> + //simple form feed leaving only the background if any
> + char buf[55];
> + memset(&buf,'\n',sizeof(buf));
> + printf("%s",buf);
> +
> +}
> +
> int efi_pollchar(void)
> {
> SIMPLE_INPUT_INTERFACE *in = ST->ConIn;
This probably deserves a NULL terminator but perhaps calling
EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL.ClearScreen() may be better.
> @@ -1042,6 +1061,97 @@ static int exit_boot(struct boot_params
> return 0;
> }
>
> +
> +/* efi_boot_efi:
> + * Boots an efi image
> + */
> +int efi_boot_efi(void *kernel_buf, size_t kernel_size,
> + char *cmdline, int cmdlineSize)
> +{
> +
> +char* szLoadImage = "LoadImage()";
> +char* szHandleProtocol = "HandleProtocol()";
> +char* szStartImage = "StartImage()";
> +
> +char* action = NULL;
> +
> +EFI_LOADED_IMAGE * image_info = NULL;
> +EFI_HANDLE Child_image_handle;
> +EFI_LOADED_IMAGE * Child_image_info = NULL;
> +EFI_STATUS status;
> +
> +CHAR16 w_emptyCmdLine [4]={0,0,0,0};
> +
> +
> +
> +status = uefi_call_wrapper(BS->HandleProtocol, 3, image_handle,
> + &LoadedImageProtocol,(void**)&image_info);
> +if(status != EFI_SUCCESS)
> + {
> + action=szHandleProtocol;
> + goto bail;
> + }
> +
> +status = uefi_call_wrapper(BS->LoadImage, 6, TRUE,
> + image_handle,
> + NULL,
> + kernel_buf,
> + kernel_size,
> + &Child_image_handle);
> +if(status != EFI_SUCCESS)
> + {
> + action=szLoadImage;
> + goto bail;
> + }
> +
> +
> +status = uefi_call_wrapper(BS->HandleProtocol, 3, Child_image_handle,
> + &LoadedImageProtocol,(void**)&Child_image_info);
> +if(status != EFI_SUCCESS)
> + {
> + uefi_call_wrapper(BS->UnloadImage,1, Child_image_handle );
> + action=szHandleProtocol;
> + goto bail;
> + }
> +
> +
> +
> +Child_image_info->ParentHandle = image_info;
> +Child_image_info->DeviceHandle = image_info->DeviceHandle;
> +Child_image_info->LoadOptionsSize = (UINT32)cmdlineSize;
> +if(cmdline!=NULL && cmdlineSize != 0)
> + Child_image_info->LoadOptions = (VOID *)cmdline;
> +else
> + Child_image_info->LoadOptions = (VOID *)w_emptyCmdLine;
> +
> +
> +
> +
> +status = uefi_call_wrapper(BS->StartImage, 3, Child_image_handle,
> + NULL,
> + NULL);
> +if(status != EFI_SUCCESS)
> + {
> + uefi_call_wrapper(BS->UnloadImage,1, Child_image_handle );
> + action=szStartImage;
> + goto bail;
> + }
> +
> +
> +//If we are here the child image has run/finished/returned
> +efi_console_restore();
> +
> +return 0;
> +
> +bail:
> + printf("EFI Err: %s failed; EFI_STATUS=%d\n", action,status);
> + printf("Press any key...\n");
> + efi_getchar(NULL);
> + return -1;
> +
> +}
> +
> +
> /* efi_boot_linux:
> * Boots the linux kernel using the image and parameters to boot with.
> * The EFI boot loader is reworked taking the cue from
Body. Style changes warranted.
> @@ -1200,6 +1310,8 @@ struct firmware efi_fw = {
> .get_serial_console_info = serialcfg,
> .adv_ops = &efi_adv_ops,
> .boot_linux = efi_boot_linux,
> + .boot_efi = efi_boot_efi,
> + .clear_screen = efi_clear_screen,
> .vesa = &efi_vesa_ops,
> .mem = &efi_mem_ops,
> };
Looks good. Glue.
> diff -uprN a/efi/pxe.c b/efi/pxe.c
> --- a/efi/pxe.c 2014-10-06 10:27:44.000000000 -0600
> +++ b/efi/pxe.c 2015-02-18 19:39:43.118182728 -0700
> @@ -154,7 +154,12 @@ void net_parse_dhcp(void)
> * Get the boot file and other info. This lives in the CACHED_REPLY
> * packet (query info 3)
> */
> - parse_dhcp(&mode->PxeReply.Dhcpv4, pkt_len);
> +
> + if(&mode->ProxyOfferReceived)
> + parse_dhcp(&mode->ProxyOffer.Dhcpv4, pkt_len);
> + else
> + parse_dhcp(&mode->PxeReply.Dhcpv4, pkt_len);
> +
> Print(L"\n");
>
> /*
Already changed. Discard chunk.
> diff -uprN a/mk/lib.mk b/mk/lib.mk
> --- a/mk/lib.mk 2014-10-06 10:27:44.000000000 -0600
> +++ b/mk/lib.mk 2015-02-18 20:15:07.333776455 -0700
> @@ -161,7 +161,9 @@ LIBLOAD_OBJS = \
> \
> syslinux/load_linux.o syslinux/initramfs.o \
> syslinux/initramfs_file.o syslinux/initramfs_loadfile.o \
> - syslinux/initramfs_archive.o
> + syslinux/initramfs_archive.o \
> + syslinux/load_efi.o
> +
>
> LIBMODULE_OBJS = \
> sys/module/common.o sys/module/$(ARCH)/elf_module.o \
Looks good. Body.
--
-Gene
More information about the Syslinux
mailing list