Re: [PATCH v2 1/2] x86/mkelf32: pad load segment to 2Mb boundary

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Aug 11, 2025 at 12:49:57PM +0200, Jan Beulich wrote:
> In order to legitimately set up initial mappings past _end[], we need
> to make sure that the entire mapped range is inside a RAM region.
> Therefore we need to inform the bootloader (or alike) that our allocated
> size is larger than just the next SECTION_ALIGN-ed boundary past _end[].
> 
> This allows dropping a command line option from the tool, which was
> introduced to work around a supposed linker bug, when the problem was
> really Xen's.
> 
> While adjusting adjacent code, correct the argc check to also cover the
> case correctly when --notes was passed.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> There's no good Fixes: tag, I don't think, as in theory the issue could
> even have happened when we still required to be loaded at a fixed
> physical address (1Mb originally, later 2Mb), and when we statically
> mapped the low 16Mb. If we assumed such can't happen below 16Mb, these
> two should be added:
> Fixes: e4dd91ea85a3 ("x86: Ensure RAM holes really are not mapped in Xen's 
> ongoing 1:1 physmap")
> Fixes: 7cd7f2f5e116 ("x86/boot: Remove the preconstructed low 16M superpage 
> mappings")
> ---
> v2: New.
> 
> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -130,8 +130,7 @@ orphan-handling-$(call ld-option,--orpha
>  
>  $(TARGET): TMP = $(dot-target).elf32
>  $(TARGET): $(TARGET)-syms $(efi-y) $(obj)/boot/mkelf32
> -     $(obj)/boot/mkelf32 $(notes_phdrs) $(TARGET)-syms $(TMP) 
> $(XEN_IMG_OFFSET) \
> -                    `$(NM) $(TARGET)-syms | sed -ne 's/^\([^ ]*\) . 
> __2M_rwdata_end$$/0x\1/p'`
> +     $(obj)/boot/mkelf32 $(notes_phdrs) $(TARGET)-syms $(TMP) 
> $(XEN_IMG_OFFSET)
>       od -t x4 -N 8192 $(TMP)  | grep 1badb002 > /dev/null || \
>               { echo "No Multiboot1 header found" >&2; false; }
>       od -t x4 -N 32768 $(TMP) | grep e85250d6 > /dev/null || \
> --- a/xen/arch/x86/boot/mkelf32.c
> +++ b/xen/arch/x86/boot/mkelf32.c
> @@ -248,7 +248,6 @@ static void do_read(int fd, void *data,
>  
>  int main(int argc, char **argv)
>  {
> -    uint64_t   final_exec_addr;
>      uint32_t   loadbase, dat_siz, mem_siz, note_base, note_sz, offset;
>      char      *inimage, *outimage;
>      int        infd, outfd;
> @@ -261,22 +260,24 @@ int main(int argc, char **argv)
>      Elf64_Ehdr in64_ehdr;
>      Elf64_Phdr in64_phdr;
>  
> -    if ( argc < 5 )
> +    if ( argc < 4 )
>      {
> +    help:
>          fprintf(stderr, "Usage: mkelf32 [--notes] <in-image> <out-image> "
> -                "<load-base> <final-exec-addr>\n");
> +                "<load-base>\n");
>          return 1;
>      }
>  
>      if ( !strcmp(argv[1], "--notes") )
>      {
> +        if ( argc < 5 )
> +            goto help;
>          i = 2;
>          num_phdrs = 2;
>      }
>      inimage  = argv[i++];
>      outimage = argv[i++];
>      loadbase = strtoul(argv[i++], NULL, 16);
> -    final_exec_addr = strtoull(argv[i++], NULL, 16);
>  
>      infd = open(inimage, O_RDONLY);
>      if ( infd == -1 )
> @@ -339,9 +340,12 @@ int main(int argc, char **argv)
>      (void)lseek(infd, in64_phdr.p_offset, SEEK_SET);
>      dat_siz = (uint32_t)in64_phdr.p_filesz;
>  
> -    /* Do not use p_memsz: it does not include BSS alignment padding. */
> -    /*mem_siz = (uint32_t)in64_phdr.p_memsz;*/
> -    mem_siz = (uint32_t)(final_exec_addr - in64_phdr.p_vaddr);
> +    /*
> +     * We don't pad .bss in the linker script, but during early boot we map
> +     * the Xen image using 2M pages.  To avoid running into adjacent non-RAM
> +     * regions, pad the segment to the next 2M boundary.

Won't it be easier to pad in the linker script?  We could still have
__bss_end before the padding, so that initialization isn't done to the
extra padding area.  Otherwise it would be helpful to mention why the
padding must be done here (opposed to being done in the linker
script).

Thanks, Roger.