Re: [PATCH v5 4/5] efi: Enable booting unified hypervisor/kernel/initrd images

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.09.2020 17:40, Trammell Hudson wrote:
> @@ -624,6 +626,29 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, 
> CHAR16 *name,
>      return true;
>  }
>  
> +static bool __init read_section(const EFI_LOADED_IMAGE *image,
> +                                char *const name, struct file *file,
> +                                const char *options)
> +{
> +    /* skip the leading "." in the section name */
> +    union string name_string = { .s = name + 1 };

Instead of forcing the caller to pass in a dot-prefixed name
and you assuming it's a dot here, how about ...

> +    file->ptr = pe_find_section(image->ImageBase, image->ImageSize,
> +                                name, &file->size);

... pe_find_section() looking for '.' followed by <name>?

> +    if ( !file->ptr )
> +        return false;
> +
> +    file->need_to_free = false;
> +
> +    if ( !s2w(&name_string) )
> +        return false;

You shouldn't give the false impression to the caller that the
section was not found, the more that ...

> +    handle_file_info(name_string.w, file, options);
> +    efi_bs->FreePool(name_string.w);

... you really need the transformation solely for producing
output. Just output a static surrogate message instead if this
fails? Of course, for x86'es sake you'd then need to pass the
ASCII string instead, which would save it from doing the
backwards translation, which is its only use of the parameter
(Arm doesn't use it at all).

> @@ -1208,9 +1233,11 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
> *SystemTable)
>          /* Get the file system interface. */
>          dir_handle = get_parent_handle(loaded_image, &file_name);
>  
> -        /* Read and parse the config file. */
> -        if ( !cfg_file_name )
> +        if ( read_section(loaded_image, ".config", &cfg, NULL) )
> +            PrintStr(L"Using unified config file\r\n");

Maybe "embedded" instead of "unified"? The config file isn't unified
after all, it's the Xen binary which is.

Jan