| Message ID | 1405944556-8372-1-git-send-email-ian.campbell@citrix.com |
|---|---|
| State | Accepted |
| Commit | 0040b649d6df262e7aa3f903bbe1279f1aebac5c |
| Headers | show |
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,
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.
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,
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.
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;
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(-)