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(-)

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;
-- 
2.30.2

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 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 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 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