Re: [Xen-devel] [PATCH for-xen-4.5 v4 12/18] x86: move modules data from mbi to boot_info and remove mbi

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.10.14 at 00:35, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 17/10/2014 15:12, Daniel Kiper wrote:
>> @@ -593,16 +582,17 @@ static void __init efi_arch_handle_module(struct file 
>> *file, const CHAR16 *name,
>>   
>>       /*
>>        * If options are provided, put them in
>> -     * mb_modules[mbi.mods_count].string after the filename, with a space
>> -     * separating them.  place_string_u32() prepends strings and adds 
>> separating
>> +     * boot_info_efi.mods[boot_info_efi.mods_nr].cmdline
>> +     * after the filename, with a space separating them.
>> +     * place_string_u32() prepends strings and adds separating
>>        * spaces, so the call order is reversed.
>>        */
>>       if ( options )
>> -        place_string_u32(&mb_modules[mbi.mods_count].string, options);
>> -    place_string_u32(&mb_modules[mbi.mods_count].string, local_name.s);
>> -    mb_modules[mbi.mods_count].mod_start = file->addr >> PAGE_SHIFT;
>> -    mb_modules[mbi.mods_count].mod_end = file->size;
>> -    ++mbi.mods_count;
>> +        
>> place_string_u32(&boot_info_efi.mods[boot_info_efi.mods_nr].cmdline, 
>> options);
>> +    place_string_u32(&boot_info_efi.mods[boot_info_efi.mods_nr].cmdline, 
>> local_name.s);
>> +    boot_info_efi.mods[boot_info_efi.mods_nr].start = file->addr >> 
>> PAGE_SHIFT;
>> +    boot_info_efi.mods[boot_info_efi.mods_nr].end = file->size;
> 
> end is surely start + size (give or take a fencepost) ?  How did the 
> pre-existing code work?

You should look at pre-existing code first: setup.c, following the
"Iterate backwards over all superpage-aligned RAM regions"
comment, has

    for ( i = 0; !efi_enabled && i < mbi->mods_count; i++ )
    {
        if ( mod[i].mod_start & (PAGE_SIZE - 1) )
            panic("Bootloader didn't honor module alignment request.");
        mod[i].mod_end -= mod[i].mod_start;
        mod[i].mod_start >>= PAGE_SHIFT;
        mod[i].reserved = 0;
    }

i.e. mod_start after this point no longer holds a byte address, and
mod_end now holds only the size. The EFI code leverages this to
make sure addresses beyond 4Gb can be expressed (which can't
be in multiboot).

>> @@ -723,16 +720,15 @@ void __init noreturn __start_xen(unsigned long mbi_p, 
>> boot_info_t *boot_info_ptr
>>        * we can relocate the dom0 kernel and other multiboot modules. Also, 
>> on
>>        * x86/64, we relocate Xen to higher memory.
>>        */
>> -    for ( i = 0; !efi_enabled && i < mbi->mods_count; i++ )
>> +    for ( i = 0; !efi_enabled && i < boot_info->mods_nr; i++ )
>>       {
>> -        if ( mod[i].mod_start & (PAGE_SIZE - 1) )
>> +        if ( boot_info->mods[i].start & (PAGE_SIZE - 1) )
>>               panic("Bootloader didn't honor module alignment request.");
>> -        mod[i].mod_end -= mod[i].mod_start;
>> -        mod[i].mod_start >>= PAGE_SHIFT;
>> -        mod[i].reserved = 0;
>> +        boot_info->mods[i].end -= boot_info->mods[i].start;
>> +        boot_info->mods[i].start >>= PAGE_SHIFT;
>>       }
>>   
>> -    modules_headroom = bzimage_headroom(bootstrap_map(mod), mod->mod_end);
>> +    modules_headroom = bzimage_headroom(bootstrap_map(boot_info->mods), 
>> boot_info->mods->end);
> 
> The old code was playing a little fast-and-loose with pointers vs arrays.
> 
> Please convert to the more-appropriate mods[0] as you are transforming 
> the code anyway, which helps highlight the bzimage_headroom() check 
> applies to the dom0 kernel only.

I disagree - arrays getting converted to pointers anyway when used
in expressions makes it rather pointless to use [0], being more code
to type, read, and parse, and hence making things (even if only
slightly) less legible.

Jan


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