move_memory() is very complicated, and buggy.  In order to fix the latter, we
have to address the former.

Given prior cleanup, bootstrap_map() is now implemented in terms of
bootstrap_map_addr(), meaining that it is counterproductive to plumb the
mapping through module_t.

Delete mod, and introduce two same-sized/named fields.  At this point in boot,
neither fields have their named purpose, so indicate the purpose in comments.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Daniel P. Smith <dpsmith@apertussolutions.com>

This is broken out both to ease review (I recommend git diff --color-words),
and for bisection purposes given how many times I've failed to edit this logic
correctly.

This does not conflict with anything in Hyperlaunch v7, but it does remove the
only callers of bootstrap_map() remaining by the end of the series, allowing
for even more cleanup.

In terms of bugs.  The ones I've spotted so far are:

 * The blksz calculation depends on the previous thing having been unmapped
   first.

 * The calculation halving blksz based on BOOTSTRAP_MAP_BASE is bogus.  It
   might plausibly be a remnent of 32bit Xen with the absence of highmem.

 * The caller of move_memory() is strictly moving modules forward in
   memory (dst > src) with a possibility of partial overlap.  The while loop
   maps dst and src, blksz chunks at a time, ascending, with s < d.  This
   results in memmove() doing an unconditional backwards copy.

   I don't see how this logic could ever have worked for an overlapping move.

 * The caller of move_memory() already has a good mapping for dst, so we don't
   even need to split BOOTSTRAP_MAP_LIMIT in half to map both parts.
---
 xen/arch/x86/setup.c | 35 +++++++++++++++++++----------------
 1 file changed, 19 insertions(+), 16 deletions(-)

diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 059b597331a7..46a0b3093c2c 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -492,26 +492,29 @@ static void __init noinline move_memory(
 
     while ( size )
     {
-        module_t mod;
+        unsigned int start /* frame */;
+        unsigned int end   /* mapsz */;
         unsigned int soffs = src & mask;
         unsigned int doffs = dst & mask;
         unsigned int sz;
         void *d, *s;
 
-        mod.mod_start = (src - soffs) >> PAGE_SHIFT;
-        mod.mod_end = soffs + size;
-        if ( mod.mod_end > blksz )
-            mod.mod_end = blksz;
-        sz = mod.mod_end - soffs;
-        s = bootstrap_map(&mod);
-
-        mod.mod_start = (dst - doffs) >> PAGE_SHIFT;
-        mod.mod_end = doffs + size;
-        if ( mod.mod_end > blksz )
-            mod.mod_end = blksz;
-        if ( sz > mod.mod_end - doffs )
-            sz = mod.mod_end - doffs;
-        d = bootstrap_map(&mod);
+        start = (src - soffs) >> PAGE_SHIFT;
+        end = soffs + size;
+        if ( end > blksz )
+            end = blksz;
+        sz = end - soffs;
+        s = bootstrap_map_addr(pfn_to_paddr(start),
+                               pfn_to_paddr(start) + end);
+
+        start = (dst - doffs) >> PAGE_SHIFT;
+        end = doffs + size;
+        if ( end > blksz )
+            end = blksz;
+        if ( sz > end - doffs )
+            sz = end - doffs;
+        d = bootstrap_map_addr(pfn_to_paddr(start),
+                               pfn_to_paddr(start) + end);
 
         memmove(d + doffs, s + soffs, sz);
 
@@ -519,7 +522,7 @@ static void __init noinline move_memory(
         src += sz;
         size -= sz;
 
-        bootstrap_map(NULL);
+        bootstrap_map_addr(0, 0);
     }
 }
 
-- 
2.39.5

On 10/22/24 18:39, Andrew Cooper wrote:
> move_memory() is very complicated, and buggy.  In order to fix the latter, we
> have to address the former.
> 
> Given prior cleanup, bootstrap_map() is now implemented in terms of
> bootstrap_map_addr(), meaining that it is counterproductive to plumb the
> mapping through module_t.
> 
> Delete mod, and introduce two same-sized/named fields.  At this point in boot,
> neither fields have their named purpose, so indicate the purpose in comments.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Daniel P. Smith <dpsmith@apertussolutions.com>
> 

Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com>