Re: [Xen-devel] [PATCH v5 10/16] efi: create efi_enabled()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 20.08.16 at 00:43, <daniel.kiper@xxxxxxxxxx> wrote:
> --- a/xen/arch/x86/efi/stub.c
> +++ b/xen/arch/x86/efi/stub.c
> @@ -4,9 +4,10 @@
>  #include <xen/lib.h>
>  #include <asm/page.h>
>  
> -#ifndef efi_enabled
> -const bool_t efi_enabled = 0;
> -#endif
> +bool_t efi_enabled(int feature)
> +{
> +    return false;
> +}

Plain bool please, the more that you use false. And I'm relatively
certain I did ask for the parameter type to be unsigned int, as you
don't mean to pass negative values.

> @@ -739,8 +739,10 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>          l3_bootmap[l3_table_offset(BOOTSTRAP_MAP_BASE)] =
>              l3e_from_paddr(__pa(l2_bootmap), __PAGE_HYPERVISOR);
>  
> -        memmap_type = loader;
> +        memmap_type = "EFI";
>      }
> +    else if ( efi_enabled(EFI_BOOT) )
> +        memmap_type = "EFI";

Is the change ahead of the "else" really necessary?

> @@ -1078,7 +1080,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>  
>      if ( !xen_phys_start )
>          panic("Not enough memory to relocate Xen.");
> -    reserve_e820_ram(&boot_e820, efi_enabled ? mbi->mem_upper : 
> __pa(&_start),
> +    reserve_e820_ram(&boot_e820, efi_enabled(EFI_BOOT) ? mbi->mem_upper : 
> __pa(&_start),

Is this really EFI_BOOT and not EFI_LOADER?

> @@ -1153,7 +1160,6 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
> *SystemTable)
>  
>  #ifndef CONFIG_ARM /* TODO - runtime service support */
>  
> -static bool_t __initdata efi_rs_enable = 1;
>  static bool_t __initdata efi_map_uc;
>  
>  static void __init parse_efi_param(char *s)
> @@ -1171,7 +1177,10 @@ static void __init parse_efi_param(char *s)
>              *ss = '\0';
>  
>          if ( !strcmp(s, "rs") )
> -            efi_rs_enable = val;
> +        {
> +            if ( !val )
> +                __clear_bit(EFI_RS, &efi_flags);

"else __set_bit(...)" to continue to allow to override an earlier
"efi=no-rs".

> @@ -43,6 +37,9 @@ UINT64 __read_mostly efi_boot_max_var_store_size;
>  UINT64 __read_mostly efi_boot_remain_var_store_size;
>  UINT64 __read_mostly efi_boot_max_var_size;
>  
> +/* Bit fields representing available EFI features/properties. */

Bit field ...

> +unsigned long efi_flags;

An unsigned int would suffice for now, afaict.

> @@ -53,6 +50,12 @@ struct efi __read_mostly efi = {
>  
>  const struct efi_pci_rom *__read_mostly efi_pci_roms;
>  
> +/* Test whether EFI_* bits are enabled. */
> +bool_t efi_enabled(int feature)
> +{
> +    return test_bit(feature, &efi_flags);
> +}

Please either make the comment useful, or drop it.

> --- a/xen/include/xen/efi.h
> +++ b/xen/include/xen/efi.h
> @@ -2,13 +2,17 @@
>  #define __XEN_EFI_H__
>  
>  #ifndef __ASSEMBLY__
> +#include <xen/bitops.h>

If efi_enabled() doesn't get inlined here I don't see what you need
this include for.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel