Re: [Xen-devel] [PATCH v4 11/19] efi: create efi_enabled()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.08.16 at 01:04, <daniel.kiper@xxxxxxxxxx> wrote:
> --- a/xen/arch/x86/domain_page.c
> +++ b/xen/arch/x86/domain_page.c
> @@ -36,7 +36,7 @@ static inline struct vcpu *mapcache_current_vcpu(void)
>       * domain's page tables but current may point at another domain's VCPU.
>       * Return NULL as though current is not properly set up yet.
>       */
> -    if ( efi_enabled && efi_rs_using_pgtables() )
> +    if ( efi_enabled(EFI_BOOT) && efi_rs_using_pgtables() )

This looks like it'll need to change again when you introduce EFI_RS.
What's wrong with introducing all three flags right away, to avoid
touching places like this multiple times?

> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -438,8 +438,8 @@ static void __init parse_video_info(void)
>  {
>      struct boot_video_info *bvi = &bootsym(boot_vid_info);
>  
> -    /* The EFI loader fills vga_console_info directly. */
> -    if ( efi_enabled )
> +    /* vga_console_info is filled directly on EFI platform. */
> +    if ( efi_enabled(EFI_BOOT) )

Along the lines of the above - wouldn't this then also become
EFI_LOADER? I.e. another place needing to change multiple times?
And there are more below.

> @@ -40,6 +41,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 defined EFI_* bits are enabled. */
> +static inline unsigned int efi_enabled(int feature)
> +{
> +    return !!test_bit(feature, &efi.flags);
> +}

Why is this returning unsigned int, yet its argument plain int? Note that
we have proper bool now, so by making it return bool you won't even
need the !! anymore.

Jan


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