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

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

[Ian Campbell]

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;
 
         if ( bootargs )
         {
@@ -1224,7 +1224,7 @@ static void dtb_load(struct kernel_info *kinfo)
 
 static void initrd_load(struct kernel_info *kinfo)
 {
-    struct bootmodule *mod = boot_module_find_by_kind(BOOTMOD_RAMDISK);
+    const struct bootmodule *mod = kinfo->initrd_bootmodule;
     paddr_t load_addr = kinfo->initrd_paddr;
     paddr_t paddr, len;
     unsigned long offs;
diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index 230ff8f..eaeadb5 100644
--- a/xen/arch/arm/kernel.c
+++ b/xen/arch/arm/kernel.c
@@ -116,6 +116,7 @@ static void place_modules(struct kernel_info *info,
 
     info->dtb_paddr = modbase;
     info->initrd_paddr = info->dtb_paddr + dtb_len;
+    info->initrd_bootmodule = mod;
 }
 
 static paddr_t kernel_zimage_place(struct kernel_info *info)
@@ -383,6 +384,7 @@ int kernel_probe(struct kernel_info *info)
         return -ENOENT;
     }
 
+    info->kernel_bootmodule = mod;
     start = mod->start;
     size = mod->size;
 
diff --git a/xen/arch/arm/kernel.h b/xen/arch/arm/kernel.h
index 7c7f624..0050dfb 100644
--- a/xen/arch/arm/kernel.h
+++ b/xen/arch/arm/kernel.h
@@ -23,6 +23,7 @@ struct kernel_info {
     paddr_t entry;
 
     /* boot blob load addresses */
+    const struct bootmodule *kernel_bootmodule, *initrd_bootmodule;
     paddr_t dtb_paddr;
     paddr_t initrd_paddr;
 
-- 
1.7.10.4

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

[Julien Grall]

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.

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.

Regards,

-- 
Julien Grall

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

[Ian Campbell]

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.

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

[Julien Grall]

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

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

[Ian Campbell]

On Mon, 2014-07-21 at 13:30 +0100, Julien Grall wrote:
> >>> @@ -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().

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

Thanks, applied.