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

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Thu, Aug 25, 2016 at 06:16:35AM -0600, Jan Beulich wrote:
> >>> 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

What is wrong with bool_t? bool is equal to bool_t.

> certain I did ask for the parameter type to be unsigned int, as you
> don't mean to pass negative values.

Sure. However, full blown efi_enabled() calls test_bit(). Latter calls
constant_test_bit() or variable_test_bit() which take bit position as
int. So, I do not think that efi_enabled() should take unsigned int. Or
we should change constant_test_bit() and variable_test_bit() respectively.
Hmmm... No, we should not. It looks that negative offsets are valid.

> > @@ -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?

No. However, in this case memmap_type is always "EFI" and we do not need
to get this value from loader variable. Which, of course, is always also
"EFI" (to be precise pointer to) here but IMO "memmap_type = loader" suggests
that memmap_type could be anything else.

> > @@ -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?

Yep, it should be EFI_LOADER. I am not sure why it has changed to EFI_BOOT.
Or it did not... Anyway, it is my fault. I will fix it.

Daniel

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