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

["Jan Beulich" <JBeulich@suse.com>]

>>> On 30.08.16 at 19:19, <daniel.kiper@oracle.com> wrote:
> On Thu, Aug 25, 2016 at 06:16:35AM -0600, Jan Beulich wrote:
>> >>> On 20.08.16 at 00:43, <daniel.kiper@oracle.com> 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.

With the (recent) introduction of bool we want to switch away from
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.

But in the context here you mean an unsigned value, so you should
use an unsigned type. Feel free to ASSERT(... <= INT_MAX) if you
really care.

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

Well - it's been okay so far, so I see no reason to change it, unless
you want to do this in a separate patch with an acceptable explanation.

Jan


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