Re: [PATCH v4 06/44] x86/boot: convert consider_modules to struct boot_module

[Daniel P. Smith]

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