[PATCH v2 0/3] x86/boot: Fix build with LLVM toolchain

[PATCH v2 0/3] x86/boot: Fix build with LLVM toolchain

[Frediano Ziglio]

Fix some compatibility generating 32 bit code.
LLVM has different object files and map files.
Do not use map file as they are not standard.

Changes since v1:
- do not use map files;
- add a fix for file names and preprocessor definition name.

Frediano Ziglio (3):
  x86/boot: Fix intermediate file names to generate 32 bit code
  x86/boot: Uses nm command instead of map file to get symbols
  x86/boot: Fix build with LLVM toolchain

 xen/arch/x86/boot/Makefile        | 10 ++++++----
 xen/arch/x86/boot/build32.lds.S   | 11 ++++++++++-
 xen/tools/combine_two_binaries.py | 28 ++++++++++++++++++----------
 3 files changed, 34 insertions(+), 15 deletions(-)

-- 
2.34.1

[PATCH v2 1/3] x86/boot: Fix intermediate file names to generate 32 bit code

[Frediano Ziglio]

The "base" and "offset" definition were inverted, "base" file
should be the files without offsets applied while "offset" should
have the offsets applied.
Also update an old usage of "final" to "apply offset" to make
more clear and consistent (in former commit messages the "final"
term was used instead of "offset").

Fixes: aa9045e77130 ('x86/boot: Rework how 32bit C is linked/included for early boot')

Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
--
Anthony was right, it was the opposite
---
 xen/arch/x86/boot/Makefile      | 7 ++++---
 xen/arch/x86/boot/build32.lds.S | 2 +-
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile
index e102bd8c70..777b4befeb 100644
--- a/xen/arch/x86/boot/Makefile
+++ b/xen/arch/x86/boot/Makefile
@@ -44,7 +44,8 @@ text_gap := 0x010200
 text_diff := 0x408020
 
 $(obj)/build32.base.lds: AFLAGS-y += -DGAP=$(text_gap) -DTEXT_DIFF=$(text_diff)
-$(obj)/build32.offset.lds: AFLAGS-y += -DGAP=$(text_gap) -DTEXT_DIFF=$(text_diff) -DFINAL
+$(obj)/build32.offset.lds: AFLAGS-y += -DGAP=$(text_gap) -DTEXT_DIFF=$(text_diff) \
+                                       -DAPPLY_OFFSET
 $(obj)/build32.base.lds $(obj)/build32.offset.lds: $(src)/build32.lds.S FORCE
 	$(call if_changed_dep,cpp_lds_S)
 
@@ -75,10 +76,10 @@ cmd_combine = \
     $(PYTHON) $(srctree)/tools/combine_two_binaries.py \
               --gap       $(text_gap) \
               --text-diff $(text_diff) \
-              --script    $(obj)/build32.offset.lds \
+              --script    $(obj)/build32.base.lds \
               --bin1      $(obj)/built-in-32.base.bin \
               --bin2      $(obj)/built-in-32.offset.bin \
-              --map       $(obj)/built-in-32.offset.map \
+              --map       $(obj)/built-in-32.base.map \
               --exports   cmdline_parse_early,reloc,reloc_trampoline32 \
               --output    $@
 
diff --git a/xen/arch/x86/boot/build32.lds.S b/xen/arch/x86/boot/build32.lds.S
index f20fc18977..9b29f0184f 100644
--- a/xen/arch/x86/boot/build32.lds.S
+++ b/xen/arch/x86/boot/build32.lds.S
@@ -15,7 +15,7 @@
  * with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
-#ifdef FINAL
+#ifndef APPLY_OFFSET
 # undef GAP
 # define GAP 0
 # define MULT 0
-- 
2.34.1

Re: [PATCH v2 1/3] x86/boot: Fix intermediate file names to generate 32 bit code

[Andrew Cooper]

On 06/11/2024 11:41 am, Frediano Ziglio wrote:
> The "base" and "offset" definition were inverted, "base" file
> should be the files without offsets applied while "offset" should
> have the offsets applied.
> Also update an old usage of "final" to "apply offset" to make
> more clear and consistent (in former commit messages the "final"
> term was used instead of "offset").
>
> Fixes: aa9045e77130 ('x86/boot: Rework how 32bit C is linked/included for early boot')
>

We don't usually have blank lines between Fixes and other tags.  Can fix
on commit.

> Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
> --

Note, this wants to be 3 dashes not 2, like git automatically inserts ...


> Anthony was right, it was the opposite
> ---

... here.  (Various of the automated tools don't do the right thing
otherwise).

>  xen/arch/x86/boot/Makefile      | 7 ++++---
>  xen/arch/x86/boot/build32.lds.S | 2 +-
>  2 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile
> index e102bd8c70..777b4befeb 100644
> --- a/xen/arch/x86/boot/Makefile
> +++ b/xen/arch/x86/boot/Makefile
> @@ -44,7 +44,8 @@ text_gap := 0x010200
>  text_diff := 0x408020
>  
>  $(obj)/build32.base.lds: AFLAGS-y += -DGAP=$(text_gap) -DTEXT_DIFF=$(text_diff)
> -$(obj)/build32.offset.lds: AFLAGS-y += -DGAP=$(text_gap) -DTEXT_DIFF=$(text_diff) -DFINAL
> +$(obj)/build32.offset.lds: AFLAGS-y += -DGAP=$(text_gap) -DTEXT_DIFF=$(text_diff) \
> +                                       -DAPPLY_OFFSET

I'd prefer to have this on a single line than re-flowed like this.  We
have various exceptions to the default width, and Makefiles are no
exception either.

Also happy to fix on commit.

Overall, Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

~Andrew

[PATCH v2 2/3] x86/boot: Uses nm command instead of map file to get symbols

[Frediano Ziglio]

Map file format is not standard making it code readind it
not portable and potentially hard to maintain.
Use nm command instead to get list of symbols; specifically
BSD format as it does not truncate symbols names like sysv one.

Fixes: aa9045e77130 ('x86/boot: Rework how 32bit C is linked/included for early boot')

Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
---
 xen/arch/x86/boot/Makefile        |  5 +++--
 xen/tools/combine_two_binaries.py | 28 ++++++++++++++++++----------
 2 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile
index 777b4befeb..01100a4b72 100644
--- a/xen/arch/x86/boot/Makefile
+++ b/xen/arch/x86/boot/Makefile
@@ -67,7 +67,8 @@ $(obj)/built-in-32.tmp.o: $(obj32)
 # If possible we use --orphan-handling=error option to make sure we account
 # for all possible sections from C code.
 $(obj)/built-in-32.%.bin: $(obj)/build32.%.lds $(obj)/built-in-32.tmp.o
-	$(LD32) $(orphan-handling-y) -N -T $< -Map $(@:bin=map) -o $(@:bin=o) $(filter %.o,$^)
+	$(LD32) $(orphan-handling-y) -N -T $< -o $(@:bin=o) $(filter %.o,$^)
+	$(NM) -p --format=bsd $(@:bin=o) > $(@:bin=nm)
 	$(OBJCOPY) -j .text -O binary $(@:bin=o) $@
 	rm -f $(@:bin=o)
 
@@ -79,7 +80,7 @@ cmd_combine = \
               --script    $(obj)/build32.base.lds \
               --bin1      $(obj)/built-in-32.base.bin \
               --bin2      $(obj)/built-in-32.offset.bin \
-              --map       $(obj)/built-in-32.base.map \
+              --symbols   $(obj)/built-in-32.base.nm \
               --exports   cmdline_parse_early,reloc,reloc_trampoline32 \
               --output    $@
 
diff --git a/xen/tools/combine_two_binaries.py b/xen/tools/combine_two_binaries.py
index 447c0d3bdb..db02494b28 100755
--- a/xen/tools/combine_two_binaries.py
+++ b/xen/tools/combine_two_binaries.py
@@ -28,8 +28,8 @@ parser.add_argument('--text-diff', dest='text_diff',
                     help='Difference between code section start')
 parser.add_argument('--output', dest='output',
                     help='Output file')
-parser.add_argument('--map', dest='mapfile',
-                    help='Map file to read for symbols to export')
+parser.add_argument('--symbols', dest='symbols_file',
+                    help='Nm command output to read for symbols to export')
 parser.add_argument('--exports', dest='exports',
                     help='Symbols to export')
 parser.add_argument('--section-header', dest='section_header',
@@ -65,15 +65,23 @@ exports = []
 if args.exports is not None:
     exports = dict([(name, None) for name in args.exports.split(',')])
 
-# Parse mapfile, look for ther symbols we want to export.
-if args.mapfile is not None:
-    symbol_re = re.compile(r'\s{15,}0x([0-9a-f]+)\s+(\S+)\n')
-    for line in open(args.mapfile):
-        m = symbol_re.match(line)
-        if not m or m.group(2) not in exports:
+# Parse symbols file, look for symbols we want to export.
+if args.symbols_file is not None:
+    dummy_start = -1
+    for line in open(args.symbols_file):
+        v = line.split()
+        if len(v) != 3 or v[1].upper() != 'T':
             continue
-        addr = int(m.group(1), 16)
-        exports[m.group(2)] = addr
+        addr = int(v[0], 16)
+        if v[2] == 'dummy_start':
+            dummy_start = addr
+            continue
+        if v[2] not in exports:
+            continue
+        exports[v[2]] = addr
+    if dummy_start != 0:
+        raise Exception("dummy_start symbol expected to be present and 0")
+
 for (name, addr) in exports.items():
     if addr is None:
         raise Exception("Required export symbols %s not found" % name)
-- 
2.34.1

Re: [PATCH v2 2/3] x86/boot: Uses nm command instead of map file to get symbols

[Andrew Cooper]

On 06/11/2024 11:41 am, Frediano Ziglio wrote:
> Map file format is not standard making it code readind it
> not portable and potentially hard to maintain.

I think you want to include a sentence along the lines of
"combine_two_binaries.py only understands GNU LD's format, and does not
work with LLVM's LLD."

That makes it more clear why this this gets a Fixes tag.

I'd also suggest having the following sentence in separate paragraph for
clarity.

> Use nm command instead to get list of symbols; specifically
> BSD format as it does not truncate symbols names like sysv one.
>
> Fixes: aa9045e77130 ('x86/boot: Rework how 32bit C is linked/included for early boot')
>
> Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
> ---
>  xen/arch/x86/boot/Makefile        |  5 +++--
>  xen/tools/combine_two_binaries.py | 28 ++++++++++++++++++----------
>  2 files changed, 21 insertions(+), 12 deletions(-)
>
> diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile
> index 777b4befeb..01100a4b72 100644
> --- a/xen/arch/x86/boot/Makefile
> +++ b/xen/arch/x86/boot/Makefile
> @@ -67,7 +67,8 @@ $(obj)/built-in-32.tmp.o: $(obj32)
>  # If possible we use --orphan-handling=error option to make sure we account
>  # for all possible sections from C code.
>  $(obj)/built-in-32.%.bin: $(obj)/build32.%.lds $(obj)/built-in-32.tmp.o
> -	$(LD32) $(orphan-handling-y) -N -T $< -Map $(@:bin=map) -o $(@:bin=o) $(filter %.o,$^)
> +	$(LD32) $(orphan-handling-y) -N -T $< -o $(@:bin=o) $(filter %.o,$^)
> +	$(NM) -p --format=bsd $(@:bin=o) > $(@:bin=nm)
>  	$(OBJCOPY) -j .text -O binary $(@:bin=o) $@
>  	rm -f $(@:bin=o)
>  
> @@ -79,7 +80,7 @@ cmd_combine = \
>                --script    $(obj)/build32.base.lds \
>                --bin1      $(obj)/built-in-32.base.bin \
>                --bin2      $(obj)/built-in-32.offset.bin \
> -              --map       $(obj)/built-in-32.base.map \
> +              --symbols   $(obj)/built-in-32.base.nm \
>                --exports   cmdline_parse_early,reloc,reloc_trampoline32 \
>                --output    $@
>  
> diff --git a/xen/tools/combine_two_binaries.py b/xen/tools/combine_two_binaries.py
> index 447c0d3bdb..db02494b28 100755
> --- a/xen/tools/combine_two_binaries.py
> +++ b/xen/tools/combine_two_binaries.py
> @@ -28,8 +28,8 @@ parser.add_argument('--text-diff', dest='text_diff',
>                      help='Difference between code section start')
>  parser.add_argument('--output', dest='output',
>                      help='Output file')
> -parser.add_argument('--map', dest='mapfile',
> -                    help='Map file to read for symbols to export')
> +parser.add_argument('--symbols', dest='symbols_file',
> +                    help='Nm command output to read for symbols to export')

We call the output of $(NM) uniformly .map elsewhere in Xen, even the
top level System.map

I'd suggest retaining the .map extension, and --map argument, and you
can probably just say help='Map file (NM) to ...' for the help text to
make it explicit.

That in turn reduces the churn ...


>  parser.add_argument('--exports', dest='exports',
>                      help='Symbols to export')
>  parser.add_argument('--section-header', dest='section_header',
> @@ -65,15 +65,23 @@ exports = []
>  if args.exports is not None:
>      exports = dict([(name, None) for name in args.exports.split(',')])
>  
> -# Parse mapfile, look for ther symbols we want to export.
> -if args.mapfile is not None:
> -    symbol_re = re.compile(r'\s{15,}0x([0-9a-f]+)\s+(\S+)\n')
> -    for line in open(args.mapfile):
> -        m = symbol_re.match(line)
> -        if not m or m.group(2) not in exports:
> +# Parse symbols file, look for symbols we want to export.
> +if args.symbols_file is not None:

... here.

> +    dummy_start = -1
> +    for line in open(args.symbols_file):
> +        v = line.split()
> +        if len(v) != 3 or v[1].upper() != 'T':
>              continue

A slightly nicer way of doing this is:

    parts = line.split()

    if len(parts) != 3:
        continue

    addr, type, sym = parts

which means you have more legible code blow.


> -        addr = int(m.group(1), 16)
> -        exports[m.group(2)] = addr
> +        addr = int(v[0], 16)
> +        if v[2] == 'dummy_start':
> +            dummy_start = addr
> +            continue
> +        if v[2] not in exports:
> +            continue
> +        exports[v[2]] = addr
> +    if dummy_start != 0:
> +        raise Exception("dummy_start symbol expected to be present and 0")
> +
>  for (name, addr) in exports.items():
>      if addr is None:
>          raise Exception("Required export symbols %s not found" % name)

Something to consider.  Instead of special casing dummy_start in several
ways, you could, insert it into exports to begin with, then check

    if exports["dummy_start"] != 0:
        raise Exception("dummy_start symbol expected to be present and 0")
    del exports["dummy_start"]

after which you're back to just the real --exports in exports[].


All of this said, it definitely looks like a much more robust solution
to the problem.

~Andrew

[PATCH v2 3/3] x86/boot: Fix build with LLVM toolchain

[Frediano Ziglio]

This toolchain generates different object files.
Object have 3 additional sections which must be handled by the
linker script.
Added sections need to have special type so we put them in
separate sections as linker will copy type from input sections.

Fixes: aa9045e77130 ('x86/boot: Rework how 32bit C is linked/included for early boot')

Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
---
 xen/arch/x86/boot/build32.lds.S | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/xen/arch/x86/boot/build32.lds.S b/xen/arch/x86/boot/build32.lds.S
index 9b29f0184f..1e59732edd 100644
--- a/xen/arch/x86/boot/build32.lds.S
+++ b/xen/arch/x86/boot/build32.lds.S
@@ -66,6 +66,15 @@ SECTIONS
        *(.comment.*)
        *(.note.*)
   }
+  .shstrtab : {
+       *(.shstrtab)
+  }
+  .strtab : {
+       *(.strtab)
+  }
+  .symtab : {
+       *(.symtab)
+  }
   /* Dynamic linkage sections.  Collected simply so we can check they're empty. */
   .got : {
         *(.got)
-- 
2.34.1

Re: [PATCH v2 3/3] x86/boot: Fix build with LLVM toolchain

[Andrew Cooper]

On 06/11/2024 11:41 am, Frediano Ziglio wrote:
> This toolchain generates different object files.
> Object have 3 additional sections which must be handled by the
> linker script.
> Added sections need to have special type so we put them in
> separate sections as linker will copy type from input sections.
>
> Fixes: aa9045e77130 ('x86/boot: Rework how 32bit C is linked/included for early boot')
>
> Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>

The patch itself is fine, but the commit message is quite stale now
you've split the fix into 3 bits.  I'd recommend:

---%<---
x86/boot: Explicitly list .{sym,shstr,str}tab in build32.lds.S

Currently, building with LLVM's LLD fails:

    ld -melf_i386_fbsd  --orphan-handling=error -N -T ...
    ld: error: <internal>:(.symtab) is being placed in '.symtab'
    ld: error: <internal>:(.shstrtab) is being placed in '.shstrtab'
    ld: error: <internal>:(.strtab) is being placed in '.strtab'
    gmake[11]: *** [arch/x86/boot/Makefile:69:
arch/x86/boot/built-in-32.base.bin] Error 1

This is a consequence of --orphan-handling, and it appears that Binutils
doesn't diagnose some orphaned sections even explicitly asked to do so.

List the sections explicitly.

Fixes ...
---%<---

Happy to fix on commit.

~Andrew

Re: [PATCH v2 3/3] x86/boot: Fix build with LLVM toolchain

[Jan Beulich]

On 06.11.2024 13:23, Andrew Cooper wrote:
> On 06/11/2024 11:41 am, Frediano Ziglio wrote:
>> This toolchain generates different object files.
>> Object have 3 additional sections which must be handled by the
>> linker script.
>> Added sections need to have special type so we put them in
>> separate sections as linker will copy type from input sections.
>>
>> Fixes: aa9045e77130 ('x86/boot: Rework how 32bit C is linked/included for early boot')
>>
>> Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
> 
> The patch itself is fine, but the commit message is quite stale now
> you've split the fix into 3 bits.  I'd recommend:
> 
> ---%<---
> x86/boot: Explicitly list .{sym,shstr,str}tab in build32.lds.S
> 
> Currently, building with LLVM's LLD fails:
> 
>     ld -melf_i386_fbsd  --orphan-handling=error -N -T ...
>     ld: error: <internal>:(.symtab) is being placed in '.symtab'
>     ld: error: <internal>:(.shstrtab) is being placed in '.shstrtab'
>     ld: error: <internal>:(.strtab) is being placed in '.strtab'
>     gmake[11]: *** [arch/x86/boot/Makefile:69:
> arch/x86/boot/built-in-32.base.bin] Error 1
> 
> This is a consequence of --orphan-handling, and it appears that Binutils
> doesn't diagnose some orphaned sections even explicitly asked to do so.

With my binutils hat on, I'd like to express that I don't view this as a
fair statement. GNU ld simply doesn't extend the concept of orphaned
sections to purely control ones, which need processing / transforming in
one way or another anyway. Instead I'm puzzled by lld's behavior
requiring such sections to be named explicitly.

Jan