Re: [Xen-devel] [PATCH] xen: Add EFI_LOAD_OPTION support

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 02.01.18 at 16:56, <tamas@xxxxxxxxxxxxx> wrote:
> When booting Xen via UEFI the Xen config file can contain multiple sections
> each describing different boot options. It is currently only possible to 
> choose
> which section to boot with if Xen is started through an EFI Shell.

Is this true? I thought that EFI Boot Manager entries can very well
have command line options. And other boot loaders (e.g. grub2)
should provide their own means to hand over options.

> @@ -375,12 +375,40 @@ static void __init PrintErrMesg(const CHAR16 *mesg, 
> EFI_STATUS ErrCode)
>  
>  static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv,
>                                      CHAR16 *cmdline, UINTN cmdsize,
> -                                    CHAR16 **options)
> +                                    CHAR16 **options, bool *elo_active)
>  {
>      CHAR16 *ptr = (CHAR16 *)(argv + argc + 1), *prev = NULL;
>      bool prev_sep = true;
>  
> -    for ( ; cmdsize > sizeof(*cmdline) && *cmdline;
> +    if ( cmdsize > sizeof(EFI_LOAD_OPTION) )
> +    {
> +        /*
> +         * See include/efi/efiapi.h for more info about the following
> +         */

This should be a single line comment (also elsewhere).

> +        EFI_LOAD_OPTION *elo = (EFI_LOAD_OPTION*)cmdline;

Missing blank before * (also elsewhere). And please make this a
pointer to const (and wherever else this would be suitable).

> +        if ( elo->Attributes & LOAD_OPTION_ACTIVE )

Without any other (earlier) check, how can you reliably tell this
being a pointer to EFI_LOAD_OPTION from it being the
currently used one to CHAR16? Is the distinction perhaps UEFI
version dependent? In the 2.3 spec I can't find any information
on the layout of what ->LoadOptions points to.

> +        {
> +            UINT8 *_opts = (UINT8*)elo;
> +            UINTN _cmdsize = cmdsize;

Leading underscores should only be used on file scope variables.

> @@ -1074,6 +1102,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
> *SystemTable)
>      bool base_video = false;
>      char *option_str;
>      bool use_cfg_file;
> +    bool elo_active = false;

Please add to one of the existing bool lines instead of introducing
yet another one.

> --- a/xen/include/efi/efiapi.h
> +++ b/xen/include/efi/efiapi.h

Generally additions to this file are expected to come from the gnu-efi
package, which it was originally cloned from. I've just checked and
see that 3.0.6 doesn't appear to have any of this (yet). In such a
case at the very least you should match pre-existing style (e.g.
indentation). Commonly, however, we introduce such private
(and temporary) definitions into the source file that needs them (see
e.g. the Apple Properties interface).

> +typedef struct __packed _EFI_LOAD_OPTION {

Is the __packed here really needed? I'd much rather uncomment
the Description[] field (allowing you to get at it without using
sizeof(the-whole-structure).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel