Re: [PATCH v4] xen: arm: Only lookup kernel/initrd bootmodule once while building dom0.

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

On 07/21/2014 01:24 PM, Ian Campbell wrote:
> On Mon, 2014-07-21 at 13:20 +0100, Julien Grall wrote:
>> On 07/21/2014 01:09 PM, Ian Campbell wrote:
>>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>>> ---
>>> v4: Call the new field kernel_bootmodule for clarity
>>>     Use the new field throughout kernel.c as well as in domain_build.c
>>>     Add and use initrd_bootmodule too.
>>>     Const up the uses
>>> v3: New patch
>>> ---
>>>  xen/arch/arm/domain_build.c |    6 +++---
>>>  xen/arch/arm/kernel.c       |    2 ++
>>>  xen/arch/arm/kernel.h       |    1 +
>>>  3 files changed, 6 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>> index 154367e..23261e4 100644
>>> --- a/xen/arch/arm/domain_build.c
>>> +++ b/xen/arch/arm/domain_build.c
>>> @@ -405,7 +405,7 @@ static int write_properties(struct domain *d, struct kernel_info *kinfo,
>>>      int res = 0;
>>>      int had_dom0_bootargs = 0;
>>>  
>>> -    struct bootmodule *mod = boot_module_find_by_kind(BOOTMOD_KERNEL);
>>> +    const struct bootmodule *mod = kinfo->kernel_bootmodule;
>>>  
>>>      if ( mod && mod->cmdline[0] )
>>>          bootargs = &mod->cmdline[0];
>>> @@ -455,7 +455,7 @@ static int write_properties(struct domain *d, struct kernel_info *kinfo,
>>>  
>>>      if ( dt_node_path_is_equal(node, "/chosen") )
>>>      {
>>> -        struct bootmodule *mod = boot_module_find_by_kind(BOOTMOD_RAMDISK);
>>> +        const struct bootmodule *mod = kinfo->initrd_bootmodule;
>>
>> Technically speaking, we only execute once this part of the code. So
>> having a new field in kernel_info doesn't seem useful.
> 
> It seemed to me to be consistent to only lookup the ramdisk once too,
> instead of here and in initrd_load().
> 
>> BTW, the function already have a variable "mod" defined at the begining.
>> I would rename this variable to modinitrd or smth different to shadow
>> the previous variable.
> 
> I noticed that too. Please do.

Ok. I will send a follow-up to fix it.

Acked-by: Julien Grall <julien.grall@linaro.org>

Regards,

-- 
Julien Grall