[PATCH 0/3] x86/boot: Untangling

[PATCH 0/3] x86/boot: Untangling

[Andrew Cooper]

This started out as another series trying to fix CPUID handling for SEV.

I'm still trying to sort out the SEV side of things, but I might need to
resort to something *far* more unpleasant in order to hit 4.19.

This series is some cleanup of module handling from the work I've done so far.
It interacts with the HyperLaunch Boot Module cleanup, but should make it
simpler overall.

Andrew Cooper (3):
  x86/boot: Explain how moving mod[0] works
  x86/boot: Explain discard_initial_images() and untangle PV initrd handling
  x86/boot: Refactor pvh_load_kernel() to have an initrd_len local

 xen/arch/x86/hvm/dom0_build.c |  9 +++++----
 xen/arch/x86/pv/dom0_build.c  | 22 +++++++++++-----------
 xen/arch/x86/setup.c          | 14 +++++++++++++-
 3 files changed, 29 insertions(+), 16 deletions(-)

-- 
2.30.2

[PATCH 1/3] x86/boot: Explain how moving mod[0] works

[Andrew Cooper]

modules_headroom is a misleading name as it applies strictly to mod[0] only,
and the movement loop is deeply unintuitive and completely undocumented.

Provide help to whomever needs to look at this code next.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Daniel Smith <dpsmith@apertussolutions.com>
CC: Christopher Clark <christopher.w.clark@gmail.com>
---
 xen/arch/x86/setup.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index caf235c6286d..59907fae095f 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1432,6 +1432,11 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p)
         /* Is the region suitable for relocating the multiboot modules? */
         for ( j = mbi->mods_count - 1; j >= 0; j-- )
         {
+            /*
+             * 'headroom' is a guess for the decompressed size and
+             * decompressor overheads of mod[0] (the dom0 kernel).  When we
+             * move mod[0], we incorperate this as extra space at the start.
+             */
             unsigned long headroom = j ? 0 : modules_headroom;
             unsigned long size = PAGE_ALIGN(headroom + mod[j].mod_end);
 
-- 
2.30.2

[PATCH 2/3] x86/boot: Explain discard_initial_images() and untangle PV initrd handling

[Andrew Cooper]

discard_initial_images() frees the memory backing the boot modules.  It is
called by dom0_construct_pv{,h}(), but obtains the module list by global
pointer which is why there is no apparent link with the initrd pointer.

Generally, module contents are copied into dom0 as it's being built, but the
initrd for PV dom0 might be directly mapped instead of copied.

dom0_construct_pv() does it's own ad-hoc freeing of the module in the copy
case, and points the global reference at the new copy, then sets the size to
0.  This only functions because init_domheap_pages(x, x) happens to be a nop.

Delete the ad-hoc freeing, and leave it to discard_initial_images().  This
requires (not) adjusting initd->mod_start in the copy case, and only setting
the size to 0 in the mapping case.

Alter discard_initial_images() to explicitly check for an ignored module, and
explain what's going on.  This is more robust and will allow for fixing other
problems with module handling.

The later logic in dom0_construct_pv() now cannot use initrd->mod_start, but
that's fine because initrd_mfn is already a duplicate of the information
wanted, and is more consistent with initrd_{pfn,len} used elsewhere.

Invalidate the initrd pointer with LIST_POISON1 to make it clearer that it
shouldn't be used.

No practical change in behaviour, but a substantial reduction in the
complexity of how this works.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Daniel Smith <dpsmith@apertussolutions.com>
CC: Christopher Clark <christopher.w.clark@gmail.com>

In other akward questions, why does initial_images_nrpages() account for all
modules when only 1 or 2 are relevant for how we construct dom0 ?
---
 xen/arch/x86/pv/dom0_build.c | 22 +++++++++++-----------
 xen/arch/x86/setup.c         |  9 ++++++++-
 2 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
index d8043fa58a27..64d9984b8308 100644
--- a/xen/arch/x86/pv/dom0_build.c
+++ b/xen/arch/x86/pv/dom0_build.c
@@ -630,18 +630,20 @@ int __init dom0_construct_pv(struct domain *d,
                 }
             memcpy(page_to_virt(page), mfn_to_virt(initrd->mod_start),
                    initrd_len);
-            mpt_alloc = (paddr_t)initrd->mod_start << PAGE_SHIFT;
-            init_domheap_pages(mpt_alloc,
-                               mpt_alloc + PAGE_ALIGN(initrd_len));
-            initrd->mod_start = initrd_mfn = mfn_x(page_to_mfn(page));
+            initrd_mfn = mfn_x(page_to_mfn(page));
         }
         else
         {
             while ( count-- )
                 if ( assign_pages(mfn_to_page(_mfn(mfn++)), 1, d, 0) )
                     BUG();
+            /*
+             * Mapped rather than copied.  Tell discard_initial_images() to
+             * ignore it.
+             */
+            initrd->mod_end = 0;
         }
-        initrd->mod_end = 0;
+        initrd = LIST_POISON1; /* No longer valid to use. */
 
         iommu_memory_setup(d, "initrd", mfn_to_page(_mfn(initrd_mfn)),
                            PFN_UP(initrd_len), &flush_flags);
@@ -653,12 +655,10 @@ int __init dom0_construct_pv(struct domain *d,
     if ( domain_tot_pages(d) < nr_pages )
         printk(" (%lu pages to be allocated)",
                nr_pages - domain_tot_pages(d));
-    if ( initrd )
-    {
-        mpt_alloc = (paddr_t)initrd->mod_start << PAGE_SHIFT;
+    if ( initrd_len )
         printk("\n Init. ramdisk: %"PRIpaddr"->%"PRIpaddr,
-               mpt_alloc, mpt_alloc + initrd_len);
-    }
+               pfn_to_paddr(initrd_mfn),
+               pfn_to_paddr(initrd_mfn) + initrd_len);
 
     printk("\nVIRTUAL MEMORY ARRANGEMENT:\n");
     printk(" Loaded kernel: %p->%p\n", _p(vkern_start), _p(vkern_end));
@@ -881,7 +881,7 @@ int __init dom0_construct_pv(struct domain *d,
         if ( pfn >= initrd_pfn )
         {
             if ( pfn < initrd_pfn + PFN_UP(initrd_len) )
-                mfn = initrd->mod_start + (pfn - initrd_pfn);
+                mfn = initrd_mfn + (pfn - initrd_pfn);
             else
                 mfn -= PFN_UP(initrd_len);
         }
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 59907fae095f..785a46a44995 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -294,7 +294,7 @@ unsigned long __init initial_images_nrpages(nodeid_t node)
     return nr;
 }
 
-void __init discard_initial_images(void)
+void __init discard_initial_images(void) /* a.k.a. free multiboot modules */
 {
     unsigned int i;
 
@@ -302,6 +302,13 @@ void __init discard_initial_images(void)
     {
         uint64_t start = (uint64_t)initial_images[i].mod_start << PAGE_SHIFT;
 
+        /*
+         * Sometimes the initrd is mapped, rather than copied, into dom0.
+         * end=0 signifies that we should leave it alone.
+         */
+        if ( initial_images[i].mod_end == 0 )
+            continue;
+
         init_domheap_pages(start,
                            start + PAGE_ALIGN(initial_images[i].mod_end));
     }
-- 
2.30.2

[PATCH 3/3] x86/boot: Refactor pvh_load_kernel() to have an initrd_len local

[Andrew Cooper]

The expression get more complicated when ->mod_end isn't being abused as a
size field.  Introduce and use a initrd_len local variable.

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: Stefano Stabellini <sstabellini@kernel.org>
CC: Daniel Smith <dpsmith@apertussolutions.com>
CC: Christopher Clark <christopher.w.clark@gmail.com>
---
 xen/arch/x86/hvm/dom0_build.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
index ac71d43a6b3f..b0cb96c3bc76 100644
--- a/xen/arch/x86/hvm/dom0_build.c
+++ b/xen/arch/x86/hvm/dom0_build.c
@@ -650,6 +650,7 @@ static int __init pvh_load_kernel(struct domain *d, const module_t *image,
 {
     void *image_start = image_base + image_headroom;
     unsigned long image_len = image->mod_end;
+    unsigned long initrd_len = initrd ? initrd->mod_end : 0;
     struct elf_binary elf;
     struct elf_dom_parms parms;
     paddr_t last_addr;
@@ -710,7 +711,7 @@ static int __init pvh_load_kernel(struct domain *d, const module_t *image,
      * simplify it.
      */
     last_addr = find_memory(d, &elf, sizeof(start_info) +
-                            (initrd ? ROUNDUP(initrd->mod_end, PAGE_SIZE) +
+                            (initrd ? ROUNDUP(initrd_len, PAGE_SIZE) +
                                       sizeof(mod)
                                     : 0) +
                             (cmdline ? ROUNDUP(strlen(cmdline) + 1,
@@ -725,7 +726,7 @@ static int __init pvh_load_kernel(struct domain *d, const module_t *image,
     if ( initrd != NULL )
     {
         rc = hvm_copy_to_guest_phys(last_addr, mfn_to_virt(initrd->mod_start),
-                                    initrd->mod_end, v);
+                                    initrd_len, v);
         if ( rc )
         {
             printk("Unable to copy initrd to guest\n");
@@ -733,8 +734,8 @@ static int __init pvh_load_kernel(struct domain *d, const module_t *image,
         }
 
         mod.paddr = last_addr;
-        mod.size = initrd->mod_end;
-        last_addr += ROUNDUP(initrd->mod_end, elf_64bit(&elf) ? 8 : 4);
+        mod.size = initrd_len;
+        last_addr += ROUNDUP(initrd_len, elf_64bit(&elf) ? 8 : 4);
         if ( initrd->string )
         {
             char *str = __va(initrd->string);
-- 
2.30.2

Re: [PATCH 1/3] x86/boot: Explain how moving mod[0] works

[Jan Beulich]

On 26.04.2024 16:01, Andrew Cooper wrote:
> modules_headroom is a misleading name as it applies strictly to mod[0] only,
> and the movement loop is deeply unintuitive and completely undocumented.
> 
> Provide help to whomever needs to look at this code next.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

Re: [PATCH 2/3] x86/boot: Explain discard_initial_images() and untangle PV initrd handling

[Jan Beulich]

On 26.04.2024 16:01, Andrew Cooper wrote:
> discard_initial_images() frees the memory backing the boot modules.  It is
> called by dom0_construct_pv{,h}(), but obtains the module list by global
> pointer which is why there is no apparent link with the initrd pointer.
> 
> Generally, module contents are copied into dom0 as it's being built, but the
> initrd for PV dom0 might be directly mapped instead of copied.
> 
> dom0_construct_pv() does it's own ad-hoc freeing of the module in the copy
> case, and points the global reference at the new copy, then sets the size to
> 0.  This only functions because init_domheap_pages(x, x) happens to be a nop.
> 
> Delete the ad-hoc freeing, and leave it to discard_initial_images().  This
> requires (not) adjusting initd->mod_start in the copy case, and only setting
> the size to 0 in the mapping case.
> 
> Alter discard_initial_images() to explicitly check for an ignored module, and
> explain what's going on.  This is more robust and will allow for fixing other
> problems with module handling.
> 
> The later logic in dom0_construct_pv() now cannot use initrd->mod_start, but
> that's fine because initrd_mfn is already a duplicate of the information
> wanted, and is more consistent with initrd_{pfn,len} used elsewhere.
> 
> Invalidate the initrd pointer with LIST_POISON1 to make it clearer that it
> shouldn't be used.
> 
> No practical change in behaviour, but a substantial reduction in the
> complexity of how this works.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Daniel Smith <dpsmith@apertussolutions.com>
> CC: Christopher Clark <christopher.w.clark@gmail.com>
> 
> In other akward questions, why does initial_images_nrpages() account for all
> modules when only 1 or 2 are relevant for how we construct dom0 ?
> ---
>  xen/arch/x86/pv/dom0_build.c | 22 +++++++++++-----------
>  xen/arch/x86/setup.c         |  9 ++++++++-
>  2 files changed, 19 insertions(+), 12 deletions(-)
> 
> diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
> index d8043fa58a27..64d9984b8308 100644
> --- a/xen/arch/x86/pv/dom0_build.c
> +++ b/xen/arch/x86/pv/dom0_build.c
> @@ -630,18 +630,20 @@ int __init dom0_construct_pv(struct domain *d,
>                  }
>              memcpy(page_to_virt(page), mfn_to_virt(initrd->mod_start),
>                     initrd_len);
> -            mpt_alloc = (paddr_t)initrd->mod_start << PAGE_SHIFT;
> -            init_domheap_pages(mpt_alloc,
> -                               mpt_alloc + PAGE_ALIGN(initrd_len));
> -            initrd->mod_start = initrd_mfn = mfn_x(page_to_mfn(page));
> +            initrd_mfn = mfn_x(page_to_mfn(page));
>          }
>          else
>          {
>              while ( count-- )
>                  if ( assign_pages(mfn_to_page(_mfn(mfn++)), 1, d, 0) )
>                      BUG();
> +            /*
> +             * Mapped rather than copied.  Tell discard_initial_images() to
> +             * ignore it.
> +             */
> +            initrd->mod_end = 0;
>          }
> -        initrd->mod_end = 0;
> +        initrd = LIST_POISON1; /* No longer valid to use. */
>  
>          iommu_memory_setup(d, "initrd", mfn_to_page(_mfn(initrd_mfn)),
>                             PFN_UP(initrd_len), &flush_flags);
> @@ -653,12 +655,10 @@ int __init dom0_construct_pv(struct domain *d,
>      if ( domain_tot_pages(d) < nr_pages )
>          printk(" (%lu pages to be allocated)",
>                 nr_pages - domain_tot_pages(d));
> -    if ( initrd )
> -    {
> -        mpt_alloc = (paddr_t)initrd->mod_start << PAGE_SHIFT;
> +    if ( initrd_len )
>          printk("\n Init. ramdisk: %"PRIpaddr"->%"PRIpaddr,
> -               mpt_alloc, mpt_alloc + initrd_len);
> -    }
> +               pfn_to_paddr(initrd_mfn),
> +               pfn_to_paddr(initrd_mfn) + initrd_len);
>  
>      printk("\nVIRTUAL MEMORY ARRANGEMENT:\n");
>      printk(" Loaded kernel: %p->%p\n", _p(vkern_start), _p(vkern_end));

Between what this and the following hunk touch there is

        if ( count < initrd_pfn || count >= initrd_pfn + PFN_UP(initrd_len) )
            mfn = pfn++;
        else
            mfn = initrd_mfn++;

I can't help thinking that this invalidates ...

> @@ -881,7 +881,7 @@ int __init dom0_construct_pv(struct domain *d,
>          if ( pfn >= initrd_pfn )
>          {
>              if ( pfn < initrd_pfn + PFN_UP(initrd_len) )
> -                mfn = initrd->mod_start + (pfn - initrd_pfn);
> +                mfn = initrd_mfn + (pfn - initrd_pfn);
>              else
>                  mfn -= PFN_UP(initrd_len);
>          }

... the use of the variable here.

> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -294,7 +294,7 @@ unsigned long __init initial_images_nrpages(nodeid_t node)
>      return nr;
>  }
>  
> -void __init discard_initial_images(void)
> +void __init discard_initial_images(void) /* a.k.a. free multiboot modules */
>  {
>      unsigned int i;
>  
> @@ -302,6 +302,13 @@ void __init discard_initial_images(void)
>      {
>          uint64_t start = (uint64_t)initial_images[i].mod_start << PAGE_SHIFT;
>  
> +        /*
> +         * Sometimes the initrd is mapped, rather than copied, into dom0.
> +         * end=0 signifies that we should leave it alone.
> +         */
> +        if ( initial_images[i].mod_end == 0 )
> +            continue;
> +
>          init_domheap_pages(start,
>                             start + PAGE_ALIGN(initial_images[i].mod_end));
>      }

While I don't strictly mind the addition, it isn't really needed, as calling
init_domheap_pages() with twice the same address is simply a no-op (and
.mod_end being 0 had to work correctly already before anyway).

Jan

Re: [PATCH 3/3] x86/boot: Refactor pvh_load_kernel() to have an initrd_len local

[Jan Beulich]

On 26.04.2024 16:01, Andrew Cooper wrote:
> The expression get more complicated when ->mod_end isn't being abused as a
> size field.  Introduce and use a initrd_len local variable.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

Re: [PATCH 1/3] x86/boot: Explain how moving mod[0] works

[Jason Andryuk]

On 2024-04-26 10:01, Andrew Cooper wrote:
> modules_headroom is a misleading name as it applies strictly to mod[0] only,
> and the movement loop is deeply unintuitive and completely undocumented.
> 
> Provide help to whomever needs to look at this code next.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Daniel Smith <dpsmith@apertussolutions.com>
> CC: Christopher Clark <christopher.w.clark@gmail.com>
> ---
>   xen/arch/x86/setup.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index caf235c6286d..59907fae095f 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1432,6 +1432,11 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p)
>           /* Is the region suitable for relocating the multiboot modules? */
>           for ( j = mbi->mods_count - 1; j >= 0; j-- )
>           {
> +            /*
> +             * 'headroom' is a guess for the decompressed size and
> +             * decompressor overheads of mod[0] (the dom0 kernel).  When we
> +             * move mod[0], we incorperate this as extra space at the start.

                                   incorporate

With that:
Reviewed-by: Jason Andryuk <jason.andryuk@gmail.com>

Thanks,
Jason

> +             */
>               unsigned long headroom = j ? 0 : modules_headroom;
>               unsigned long size = PAGE_ALIGN(headroom + mod[j].mod_end);
>