Re: [Xen-devel] [PATCH v2 09/23] efi: create efi_enabled()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 20.07.15 at 16:29, <daniel.kiper@xxxxxxxxxx> wrote:
> --- a/xen/arch/x86/efi/stub.c
> +++ b/xen/arch/x86/efi/stub.c
> @@ -4,9 +4,14 @@
>  #include <xen/lib.h>
>  #include <asm/page.h>
>  
> -#ifndef efi_enabled
> -const bool_t efi_enabled = 0;
> -#endif
> +struct efi __read_mostly efi = {
> +     .flags   = 0, /* Initialized later. */
> +     .acpi    = EFI_INVALID_TABLE_ADDR,
> +     .acpi20  = EFI_INVALID_TABLE_ADDR,
> +     .mps     = EFI_INVALID_TABLE_ADDR,
> +     .smbios  = EFI_INVALID_TABLE_ADDR,
> +     .smbios3 = EFI_INVALID_TABLE_ADDR
> +};

How is this change related to the subject of the patch?

> --- a/xen/arch/x86/xen.lds.S
> +++ b/xen/arch/x86/xen.lds.S
> @@ -191,8 +191,6 @@ SECTIONS
>    .pad : {
>      . = ALIGN(MB(16));
>    } :text
> -#else
> -  efi = .;
>  #endif

Same here.

> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -717,6 +717,10 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
> *SystemTable)
>      char *option_str;
>      bool_t use_cfg_file;
>  
> +#ifndef CONFIG_ARM /* Disabled until runtime services implemented. */
> +    set_bit(EFI_PLATFORM, &efi.flags);
> +#endif

Just for this to work? I don't see the need for all the pointers in
the stub case - why can't this be a separate variable? We don't
need to follow Linux with where the flags actually live...

> --- a/xen/common/efi/runtime.c
> +++ b/xen/common/efi/runtime.c
> @@ -10,14 +10,10 @@ DEFINE_XEN_GUEST_HANDLE(CHAR16);
>  
>  #ifndef COMPAT
>  
> -#ifdef CONFIG_ARM  /* Disabled until runtime services implemented */
> -const bool_t efi_enabled = 0;
> -#else
> +#ifndef CONFIG_ARM
>  # include <asm/i387.h>
>  # include <asm/xstate.h>
>  # include <public/platform.h>
> -
> -const bool_t efi_enabled = 1;
>  #endif

Afaict the proper replacement (at least at this point in the series) for
this would be to statically initialize the flag set in this xen.efi case.

> @@ -40,6 +42,16 @@ int efi_runtime_call(struct xenpf_efi_runtime_call *);
>  int efi_compat_get_info(uint32_t idx, union compat_pf_efi_info *);
>  int efi_compat_runtime_call(struct compat_pf_efi_runtime_call *);
>  
> +/*
> + * Test whether the above EFI_* bits are enabled.
> + *
> + * Stolen from Linux Kernel.

I don't think this second part of the comment makes a lot of sense
for a minor piece of functionality like this.

Jan


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