Re: [PATCH v3 03/14] xen: arm: allocate dom0 memory separately from preparing the dtb

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

On 11/08/2013 09:36 AM, Ian Campbell wrote:
> On Thu, 2013-11-07 at 23:18 -0800, Julien Grall wrote:
> I think
> 	/ {
> 		"memory" {
> 			#address-cells = 2;
> 	 		#size-cells = 2;
> 			device_type = "memory";
> 			reg = <...>;
> 		}
> 	}
> 
> Won't work because the #foo-cells only applies to children.
> 
> I could do
> 	/ {
> 		"memory" {
> 			#address-cells = 2;
> 	        	#size-cells = 2;
> 			"memory@foo" {
> 				device_type = "memory"
> 				reg = <...>;
> 	 		}
> 	  	}
> 	}
> 
> which puts the size under my control.

I have noticed that we have the same issue on the other nodes (gic,
timer,...). Perhaps we can create a node "xen" which contains all the
devices?

"xen" {
   #address-cells = 2;
   #size-cells = 2;
   memory {
     ...
   }
   timer {
     ...
   }
}

>>> This allows us to move kernel parsing before DTB setup.
>>
>> Why do you want to move the kernel parsing earlier? Xen don't use 
>> d->arch.type during dom0 building.
> 
> In this series prepare_dtb needs to know which kind of guest it is,
> which requires us to have parsed the kernel.

Thanks, I didn't pay attention that patch #4 uses the kernel type via
is_pv64_domain.

> 
> I also think it is an independently worthwhile change to separate
> prepping the dtb from the memory allocation, since it makes things
> cleaner overall (i.e. we can drop the overlap_check() which is quite a
> hacky way to do things which came about because of the intertwining of
> the dtb and memory alloc.
> 
>>>   static void kernel_elf_load(struct kernel_info *info)
>>>   {
>>> +    place_modules(info,
>>> +                  info->elf.parms.virt_kstart,
>>> +                  info->elf.parms.virt_kend);
>>> +
>>>       printk("Loading ELF image into guest memory\n");
>>>       info->elf.elf.dest_base = (void*)(unsigned long)info->elf.parms.virt_kstart;
>>>       info->elf.elf.dest_size =
>>>            info->elf.parms.virt_kend - info->elf.parms.virt_kstart;
>>> +
>>
>> spurious line?
> 
> Just aesthetics I think. Could drop it I guess.

Ok.


-- 
Julien Grall