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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> CC: Daniel Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> CC: Christopher Clark <christopher.w.clark@xxxxxxxxx>
> 
> 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