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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.04.16 at 14:33, <daniel.kiper@xxxxxxxxxx> wrote:
> --- a/xen/arch/x86/efi/stub.c
> +++ b/xen/arch/x86/efi/stub.c
> @@ -4,11 +4,8 @@
>  #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. */

This is pointless to add - the field will get zero-initialized anyway.

> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -934,6 +934,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

Surely this can be __set_bit()? It's also hard to see what setting this
flag has got to do with runtime services. But more on this below.

> @@ -42,11 +38,12 @@ UINT64 __read_mostly efi_boot_remain_var_store_size;
>  UINT64 __read_mostly efi_boot_max_var_size;
>  
>  struct efi __read_mostly efi = {
> -     .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,
> +     .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
>  };

This, again, is an unnecessary hunk. And in no case should you drop
the trailing comma - that's there for a reason.

> --- a/xen/include/xen/efi.h
> +++ b/xen/include/xen/efi.h
> @@ -2,15 +2,17 @@
>  #define __XEN_EFI_H__
>  
>  #ifndef __ASSEMBLY__
> +#include <xen/bitops.h>
>  #include <xen/types.h>
>  #endif
>  
> -extern const bool_t efi_enabled;
> -
>  #define EFI_INVALID_TABLE_ADDR (~0UL)
>  
> +#define EFI_PLATFORM 0

So what does "platform" mean? Did you consider using the more fine
grained set of flags Linux uses nowadays? That would also eliminate
the odd connection to runtime services mentioned earlier.

And please add a comment making clear that these values are bit
positions to be used in the flags field below. I might also help to
move this right next to the structure field.

> @@ -40,6 +42,12 @@ 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. */

The comment leaves open which EFI_* values you actually refer to.
Hence another option would be to move those #define-s here.

> +static inline bool_t efi_enabled(int feature)

unsigned int

> +{
> +    return test_bit(feature, &efi.flags) != 0;
> +}

Please use the more conventional !! found elsewhere in our code.

Jan


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