Re: [PATCH 1/2] xen/arm: Add support to load initrd in dom0

[Julien Grall <julien.grall@linaro.org>]

On 09/25/2013 04:15 PM, Ian Campbell wrote:
> On Mon, 2013-09-16 at 16:20 +0100, Julien Grall wrote:
>> @@ -806,7 +821,8 @@ static int prepare_dtb(struct domain *d, struct kernel_info *kinfo)
>>          goto err;
>>  
>>      /* Actual new size */
>> -    new_size = fdt_totalsize(kinfo->fdt);
>> +    initrd_len = early_info.modules.module[MOD_INITRD].size;
> 
> I think you need to check nr_modules here and in write_properties (which
> I already trimmed by mistake.
> 

Even if we check nr_modules, we can't assume MOD_INITRD is the last
modules in the array, so it's possible to have nr_modules greater than
MOD_INITRD but the module is not set. That's why I only choose to rely
on size.

> .size may not be initialised otherwise. (in reality it's probably
> in .bss not sure I want to rely on that though)

It's in .bss, on common/device_tree.c we already rely that this
structure is zeroed (nr_modules is never initialized to 0).


> 
>> +    new_size = fdt_totalsize(kinfo->fdt) + initrd_len;
>>  
>>      /*
>>       * DTB must be loaded such that it does not conflict with the
>> @@ -815,15 +831,20 @@ static int prepare_dtb(struct domain *d, struct kernel_info *kinfo)
>>       * the recommendation in Documentation/arm64/booting.txt is below
>>       * 512MB. Place at 128MB, (or, if we have less RAM, as high as
>>       * possible) in order to satisfy both.
>> +     * If the bootloader provides an initrd, it will be loaded just
>> +     * after the DTB.
>>       */
>>      end = kinfo->mem.bank[0].start + kinfo->mem.bank[0].size;
>>      end = MIN(kinfo->mem.bank[0].start + (128<<20) + new_size, end);
>>  
>> -    kinfo->dtb_paddr = end - fdt_totalsize(kinfo->fdt);
>> +    kinfo->initrd_paddr = end - initrd_len;
>> +    kinfo->initrd_paddr &= ~((1 << 20) - 1);
> 
> 1MB aligned, why not 2 like most things?

A mistake, I will fix it in the next patch series.

> 
>> +    kinfo->dtb_paddr = kinfo->initrd_paddr - fdt_totalsize(kinfo->fdt);
>>      /* Align the address to 2Mb. Linux only requires 4 byte alignment */
>>      kinfo->dtb_paddr &= ~((2 << 20) - 1);
>>  
>> -    if ( fdt_totalsize(kinfo->fdt) > end )
>> +    if ( new_size > (end - kinfo->mem.bank[0].size) )
>>      {
>>          printk(XENLOG_ERR "Not enough memory in the first bank for "
>>                 "the device tree.");
>> @@ -854,6 +875,61 @@ static void dtb_load(struct kernel_info *kinfo)
>>      xfree(kinfo->fdt);
>>  }
>>  
>> +static void initrd_load(struct kernel_info *kinfo)
>> +{
>> +    paddr_t load_addr = kinfo->initrd_paddr;
>> +    paddr_t paddr = early_info.modules.module[MOD_INITRD].start;
>> +    paddr_t len = early_info.modules.module[MOD_INITRD].size;
>> +    unsigned long offs;
>> +    int node;
>> +    int res;
>> +
>> +    if ( !len )
>> +        return;
>> +
>> +    printk("Loading dom0 initrd from %"PRIpaddr" to 0x%"PRIpaddr"-0x%"PRIpaddr"\n",
>> +           paddr, load_addr, load_addr + len);
>> +
>> +    /* Fix up linux,initrd-start and linux,initrd-end in /chosen */
>> +    node = fdt_path_offset(kinfo->fdt, "/chosen");
>> +    if ( node < 0 )
>> +        panic("Cannot find the /chosen node");
>> +
>> +    res = fdt_setprop_inplace_cell(kinfo->fdt, node, "linux,initrd-start",
>> +                                   load_addr);
>> +    if ( res )
>> +        panic("Cannot fix up \"linux,initrd-start\" property\n");
>> +
>> +    res = fdt_setprop_inplace_cell(kinfo->fdt, node, "linux,initrd-end",
>> +                                   load_addr + len);
>> +    if ( res )
>> +        panic("Cannot fix up \"linux,initrd-end\" property\n");
>> +
>> +    for ( offs = 0; offs < len; )
> 
> Can you refactor kernel_zimage_load into a more generic function which
> can be used here?

I will do.

-- 
Julien Grall