Re: [PATCH for-xen-4.5 v4 09/18] x86: move boot_loader_name from mbi to boot_info

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 17/10/2014 15:12, Daniel Kiper wrote:
> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
> ---
> v4 - suggestions/fixes:
>     - change boot_loader_name type to "const char *"
>       (suggested by Andrew Cooper).
> ---
>   xen/arch/x86/boot_info.c        |   12 ++++++------
>   xen/arch/x86/efi/efi-boot.h     |    3 +--
>   xen/arch/x86/setup.c            |   13 +++++--------
>   xen/common/efi/runtime.c        |    1 +
>   xen/include/asm-x86/boot_info.h |    3 +++
>   5 files changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/xen/arch/x86/boot_info.c b/xen/arch/x86/boot_info.c
> index c5f2a18..a81e98b 100644
> --- a/xen/arch/x86/boot_info.c
> +++ b/xen/arch/x86/boot_info.c
> @@ -29,6 +29,7 @@
>   static multiboot_info_t __read_mostly mbi;
>   
>   static boot_info_t __read_mostly boot_info_mb = {
> +    .boot_loader_name = "UNKNOWN",

This should be lowercase if you wish to match the code removed lower down.

>       .warn_msg = NULL,
>       .err_msg = NULL
>   };
> @@ -39,12 +40,6 @@ unsigned long __init __init_mbi(u32 mbd_pa)
>   
>       enable_bsp_exception_support();
>   
> -    if ( mbd->boot_loader_name )
> -    {
> -        mbi.flags = MBI_LOADERNAME;

I have only noticed in this patch, but the above line should strictly be 
an |=, when it is introduced in the earlier patch.

> -        mbi.boot_loader_name = mbd->boot_loader_name;
> -    }
> -
>       if ( mbd->cmdline )
>       {
>           mbi.flags |= MBI_CMDLINE;
> @@ -77,5 +72,10 @@ unsigned long __init __init_mbi(u32 mbd_pa)
>   
>   boot_info_t __init *__init_boot_info(u32 mbd_pa)
>   {
> +    mbd_t *mbd = __va(mbd_pa);
> +
> +    if ( mbd->boot_loader_name )
> +        boot_info_mb.boot_loader_name = __va(mbd->boot_loader_name);
> +
>       return &boot_info_mb;
>   }
> diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
> index 3da1a2a..19be165 100644
> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -11,7 +11,7 @@
>   
>   static struct file __initdata ucode;
>   static multiboot_info_t __initdata mbi = {
> -    .flags = MBI_MODULES | MBI_LOADERNAME
> +    .flags = MBI_MODULES
>   };
>   static module_t __initdata mb_modules[3];
>   
> @@ -310,7 +310,6 @@ static void __init efi_arch_handle_cmdline(CHAR16 *image_name,
>        * These must not be initialized statically, since the value must
>        * not get relocated when processing base relocations later.
>        */
> -    mbi.boot_loader_name = (long)"EFI";
>       mbi.mods_addr = (long)mb_modules;
>   }
>   
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index 18fa3e4..a9dbc05 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -552,7 +552,7 @@ void __init enable_bsp_exception_support(void)
>   void __init noreturn __start_xen(unsigned long mbi_p, boot_info_t *boot_info_ptr)
>   {
>       char *memmap_type = NULL;
> -    char *cmdline, *kextra, *loader;
> +    char *cmdline, *kextra;
>       unsigned int initrdidx, domcr_flags = DOMCRF_s3_integrity;
>       multiboot_info_t *mbi = (multiboot_info_t *)mbi_p;
>       module_t *mod = (module_t *)__va(mbi->mods_addr);
> @@ -577,13 +577,10 @@ void __init noreturn __start_xen(unsigned long mbi_p, boot_info_t *boot_info_ptr
>           /* Exception support was enabled before __start_xen() call. */
>       }
>   
> -    loader = (mbi->flags & MBI_LOADERNAME)
> -        ? (char *)__va(mbi->boot_loader_name) : "unknown";
> -
>       /* Parse the command-line options. */
>       cmdline = cmdline_cook((mbi->flags & MBI_CMDLINE) ?
>                              __va(mbi->cmdline) : NULL,
> -                           loader);
> +                           boot_info->boot_loader_name);
>       if ( (kextra = strstr(cmdline, " -- ")) != NULL )
>       {
>           /*
> @@ -623,7 +620,7 @@ void __init noreturn __start_xen(unsigned long mbi_p, boot_info_t *boot_info_ptr
>       if ( boot_info->warn_msg )
>           printk(boot_info->warn_msg);
>   
> -    printk("Bootloader: %s\n", loader);
> +    printk("Bootloader: %s\n", boot_info->boot_loader_name);
>   
>       printk("Command line: %s\n", cmdline);
>   
> @@ -697,7 +694,7 @@ void __init noreturn __start_xen(unsigned long mbi_p, boot_info_t *boot_info_ptr
>           l3_bootmap[l3_table_offset(BOOTSTRAP_MAP_BASE)] =
>               l3e_from_paddr(__pa(l2_bootmap), __PAGE_HYPERVISOR);
>   
> -        memmap_type = loader;
> +        memmap_type = boot_info->boot_loader_name;
>       }
>       else if ( e820_raw_nr != 0 )
>       {
> @@ -1379,7 +1376,7 @@ void __init noreturn __start_xen(unsigned long mbi_p, boot_info_t *boot_info_ptr
>       {
>           static char __initdata dom0_cmdline[MAX_GUEST_CMDLINE];
>   
> -        cmdline = cmdline_cook(cmdline, loader);
> +        cmdline = cmdline_cook(cmdline, boot_info->boot_loader_name);
>           safe_strcpy(dom0_cmdline, cmdline);
>   
>           if ( kextra != NULL )
> diff --git a/xen/common/efi/runtime.c b/xen/common/efi/runtime.c
> index eb0acae..7846b2b 100644
> --- a/xen/common/efi/runtime.c
> +++ b/xen/common/efi/runtime.c
> @@ -54,6 +54,7 @@ const struct efi_pci_rom *__read_mostly efi_pci_roms;
>   
>   #ifndef CONFIG_ARM /* TODO - disabled until implemented on ARM */
>   boot_info_t __read_mostly boot_info_efi = {
> +    .boot_loader_name = "EFI",
>       .warn_msg = NULL,
>       .err_msg = NULL
>   };
> diff --git a/xen/include/asm-x86/boot_info.h b/xen/include/asm-x86/boot_info.h
> index f0a76b2..9e68447 100644
> --- a/xen/include/asm-x86/boot_info.h
> +++ b/xen/include/asm-x86/boot_info.h
> @@ -30,6 +30,9 @@
>    * are compatible as much as possible with relevant EFI/ACPI types.
>    */
>   typedef struct {
> +    /* Boot loader name. */
> +    const char *boot_loader_name;
> +

I think this can reasonably be named simply "loader".

~Andrew

>       /*
>        * Info about warning occurred during boot_info initialization.
>        * NULL if everything went OK.