| Message ID | 20240830214730.1621-7-dpsmith@apertussolutions.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Boot modules for Hyperlaunch | expand |
On 30.08.2024 23:46, Daniel P. Smith wrote: > --- a/xen/arch/x86/setup.c > +++ b/xen/arch/x86/setup.c > @@ -632,7 +632,7 @@ static void __init noinline move_xen(void) > #undef BOOTSTRAP_MAP_LIMIT > > static uint64_t __init consider_modules( > - uint64_t s, uint64_t e, uint32_t size, const module_t *mod, > + uint64_t s, uint64_t e, uint32_t size, const struct boot_module *mods, As an array is meant, may I ask to switch to mods[] at this occasion? > @@ -642,20 +642,20 @@ static uint64_t __init consider_modules( > > for ( i = 0; i < nr_mods ; ++i ) > { > - uint64_t start = (uint64_t)mod[i].mod_start << PAGE_SHIFT; > - uint64_t end = start + PAGE_ALIGN(mod[i].mod_end); > + uint64_t start = (uint64_t)mods[i].early_mod->mod_start << PAGE_SHIFT; Similarly, may I ask to stop open-coding {,__}pfn_to_paddr() while transforming this? > @@ -1447,7 +1447,7 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p) > { > /* Don't overlap with modules. */ > end = consider_modules(s, e, reloc_size + mask, > - mod, boot_info->nr_mods, -1); > + boot_info->mods, boot_info->nr_mods, -1); > end &= ~mask; > } > else > @@ -1482,7 +1482,7 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p) > continue; > > /* Don't overlap with other modules (or Xen itself). */ > - end = consider_modules(s, e, size, mod, > + end = consider_modules(s, e, size, boot_info->mods, > boot_info->nr_mods + relocated, j); > > if ( highmem_start && end > highmem_start ) > @@ -1509,7 +1509,7 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p) > while ( !kexec_crash_area.start ) > { > /* Don't overlap with modules (or Xen itself). */ > - e = consider_modules(s, e, PAGE_ALIGN(kexec_crash_area.size), mod, > + e = consider_modules(s, e, PAGE_ALIGN(kexec_crash_area.size), boot_info->mods, > boot_info->nr_mods + relocated, -1); All of these show a meaningful increase of line lengths, up to the point of ending up with too long a line here. I really wonder if the variable name "boot_info" isn't too long for something that's going to be used quite frequently. Just "bi" maybe? Jan
On 04/09/2024 7:40 am, Jan Beulich wrote: > On 30.08.2024 23:46, Daniel P. Smith wrote: >> @@ -1447,7 +1447,7 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p) >> { >> /* Don't overlap with modules. */ >> end = consider_modules(s, e, reloc_size + mask, >> - mod, boot_info->nr_mods, -1); >> + boot_info->mods, boot_info->nr_mods, -1); >> end &= ~mask; >> } >> else >> @@ -1482,7 +1482,7 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p) >> continue; >> >> /* Don't overlap with other modules (or Xen itself). */ >> - end = consider_modules(s, e, size, mod, >> + end = consider_modules(s, e, size, boot_info->mods, >> boot_info->nr_mods + relocated, j); >> >> if ( highmem_start && end > highmem_start ) >> @@ -1509,7 +1509,7 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p) >> while ( !kexec_crash_area.start ) >> { >> /* Don't overlap with modules (or Xen itself). */ >> - e = consider_modules(s, e, PAGE_ALIGN(kexec_crash_area.size), mod, >> + e = consider_modules(s, e, PAGE_ALIGN(kexec_crash_area.size), boot_info->mods, >> boot_info->nr_mods + relocated, -1); > All of these show a meaningful increase of line lengths, up to the point of > ending up with too long a line here. I really wonder if the variable name > "boot_info" isn't too long for something that's going to be used quite > frequently. Just "bi" maybe? Actually I noticed that too. It's boot_info-> in setup.c, and bi-> everywhere else (with bm later too). We should just use "bi" uniformly even in setup.c (Or some other name, but I'm happy with bi here - it's very easily qualified by it's field names.) ~Andrew
On 9/4/24 02:40, Jan Beulich wrote: > On 30.08.2024 23:46, Daniel P. Smith wrote: >> --- a/xen/arch/x86/setup.c >> +++ b/xen/arch/x86/setup.c >> @@ -632,7 +632,7 @@ static void __init noinline move_xen(void) >> #undef BOOTSTRAP_MAP_LIMIT >> >> static uint64_t __init consider_modules( >> - uint64_t s, uint64_t e, uint32_t size, const module_t *mod, >> + uint64_t s, uint64_t e, uint32_t size, const struct boot_module *mods, > > As an array is meant, may I ask to switch to mods[] at this occasion? Sure. >> @@ -642,20 +642,20 @@ static uint64_t __init consider_modules( >> >> for ( i = 0; i < nr_mods ; ++i ) >> { >> - uint64_t start = (uint64_t)mod[i].mod_start << PAGE_SHIFT; >> - uint64_t end = start + PAGE_ALIGN(mod[i].mod_end); >> + uint64_t start = (uint64_t)mods[i].early_mod->mod_start << PAGE_SHIFT; > > Similarly, may I ask to stop open-coding {,__}pfn_to_paddr() while > transforming this? Yep, I can convert it. I was trying to be conscious of this, and you should see it in other places. >> @@ -1447,7 +1447,7 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p) >> { >> /* Don't overlap with modules. */ >> end = consider_modules(s, e, reloc_size + mask, >> - mod, boot_info->nr_mods, -1); >> + boot_info->mods, boot_info->nr_mods, -1); >> end &= ~mask; >> } >> else >> @@ -1482,7 +1482,7 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p) >> continue; >> >> /* Don't overlap with other modules (or Xen itself). */ >> - end = consider_modules(s, e, size, mod, >> + end = consider_modules(s, e, size, boot_info->mods, >> boot_info->nr_mods + relocated, j); >> >> if ( highmem_start && end > highmem_start ) >> @@ -1509,7 +1509,7 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p) >> while ( !kexec_crash_area.start ) >> { >> /* Don't overlap with modules (or Xen itself). */ >> - e = consider_modules(s, e, PAGE_ALIGN(kexec_crash_area.size), mod, >> + e = consider_modules(s, e, PAGE_ALIGN(kexec_crash_area.size), boot_info->mods, >> boot_info->nr_mods + relocated, -1); > > All of these show a meaningful increase of line lengths, up to the point of > ending up with too long a line here. I really wonder if the variable name > "boot_info" isn't too long for something that's going to be used quite > frequently. Just "bi" maybe? Yes, in fact, my apologies as this appears to be a comment you made from a previous review. The suggestion from Alejandro will make this easier since it will become a local variable to __start_xen(). v/r, dps
On 9/4/24 06:41, Andrew Cooper wrote: > On 04/09/2024 7:40 am, Jan Beulich wrote: >> On 30.08.2024 23:46, Daniel P. Smith wrote: >>> @@ -1447,7 +1447,7 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p) >>> { >>> /* Don't overlap with modules. */ >>> end = consider_modules(s, e, reloc_size + mask, >>> - mod, boot_info->nr_mods, -1); >>> + boot_info->mods, boot_info->nr_mods, -1); >>> end &= ~mask; >>> } >>> else >>> @@ -1482,7 +1482,7 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p) >>> continue; >>> >>> /* Don't overlap with other modules (or Xen itself). */ >>> - end = consider_modules(s, e, size, mod, >>> + end = consider_modules(s, e, size, boot_info->mods, >>> boot_info->nr_mods + relocated, j); >>> >>> if ( highmem_start && end > highmem_start ) >>> @@ -1509,7 +1509,7 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p) >>> while ( !kexec_crash_area.start ) >>> { >>> /* Don't overlap with modules (or Xen itself). */ >>> - e = consider_modules(s, e, PAGE_ALIGN(kexec_crash_area.size), mod, >>> + e = consider_modules(s, e, PAGE_ALIGN(kexec_crash_area.size), boot_info->mods, >>> boot_info->nr_mods + relocated, -1); >> All of these show a meaningful increase of line lengths, up to the point of >> ending up with too long a line here. I really wonder if the variable name >> "boot_info" isn't too long for something that's going to be used quite >> frequently. Just "bi" maybe? > > Actually I noticed that too. > > It's boot_info-> in setup.c, and bi-> everywhere else (with bm later too). > > We should just use "bi" uniformly even in setup.c (Or some other name, > but I'm happy with bi here - it's very easily qualified by it's field > names.) Yes, I did make it "bi" as it was passed around, and will move setup.c to that as well. As for "bm", I will be moving the array into struct boot_info. When I do, is there a desire to see the element name to be "bm" or "bms"? v/r, dps
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index 28fdbf4d4c2b..8912956ee7f1 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -632,7 +632,7 @@ static void __init noinline move_xen(void) #undef BOOTSTRAP_MAP_LIMIT static uint64_t __init consider_modules( - uint64_t s, uint64_t e, uint32_t size, const module_t *mod, + uint64_t s, uint64_t e, uint32_t size, const struct boot_module *mods, unsigned int nr_mods, unsigned int this_mod) { unsigned int i; @@ -642,20 +642,20 @@ static uint64_t __init consider_modules( for ( i = 0; i < nr_mods ; ++i ) { - uint64_t start = (uint64_t)mod[i].mod_start << PAGE_SHIFT; - uint64_t end = start + PAGE_ALIGN(mod[i].mod_end); + uint64_t start = (uint64_t)mods[i].early_mod->mod_start << PAGE_SHIFT; + uint64_t end = start + PAGE_ALIGN(mods[i].early_mod->mod_end); if ( i == this_mod ) continue; if ( s < end && start < e ) { - end = consider_modules(end, e, size, mod + i + 1, + end = consider_modules(end, e, size, &mods[i + 1], nr_mods - i - 1, this_mod - i - 1); if ( end ) return end; - return consider_modules(s, start, size, mod + i + 1, + return consider_modules(s, start, size, &mods[i + 1], nr_mods - i - 1, this_mod - i - 1); } } @@ -1447,7 +1447,7 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p) { /* Don't overlap with modules. */ end = consider_modules(s, e, reloc_size + mask, - mod, boot_info->nr_mods, -1); + boot_info->mods, boot_info->nr_mods, -1); end &= ~mask; } else @@ -1482,7 +1482,7 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p) continue; /* Don't overlap with other modules (or Xen itself). */ - end = consider_modules(s, e, size, mod, + end = consider_modules(s, e, size, boot_info->mods, boot_info->nr_mods + relocated, j); if ( highmem_start && end > highmem_start ) @@ -1509,7 +1509,7 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p) while ( !kexec_crash_area.start ) { /* Don't overlap with modules (or Xen itself). */ - e = consider_modules(s, e, PAGE_ALIGN(kexec_crash_area.size), mod, + e = consider_modules(s, e, PAGE_ALIGN(kexec_crash_area.size), boot_info->mods, boot_info->nr_mods + relocated, -1); if ( s >= e ) break;
To start transitioning consider_modules() over to struct boot_module, begin with taking the array of struct boot_modules but use the temporary struct element early_mod. No functional change intended. Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> --- xen/arch/x86/setup.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)