Re: [python] [PATCH v3 1/5] x86/boot: create a C bundle for 32 bit boot code and use it

[Frediano Ziglio <frediano.ziglio@xxxxxxxxx>]

On Fri, Oct 11, 2024 at 2:17 PM Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
>
> On 11/10/2024 9:52 am, Frediano Ziglio wrote:
> > diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile
> > index ff0f965876..4cf0d7e140 100644
> > --- a/xen/arch/x86/boot/Makefile
> > +++ b/xen/arch/x86/boot/Makefile
> > ...
> > +$(obj)/built_in_32.o: $(obj)/built_in_32.other.bin 
> > $(obj)/built_in_32.final.bin
> > +     $(PYTHON) $(srctree)/tools/combine_two_binaries \
> > +             --script $(obj)/build32.final.lds \
> > +             --bin1 $(obj)/built_in_32.other.bin --bin2 
> > $(obj)/built_in_32.final.bin \
> > +             --map $(obj)/built_in_32.final.map \
> > +             --exports cmdline_parse_early,reloc \
> > +             --section-header '.section .init.text, "ax", @progbits' \
> > +             --output $(obj)/built_in_32.s
>
> I can't see a case where we'd want this anywhere other than .init.text,
> so I'd drop the --section-header and just write it out unconditionally.

Could we just change the default in Python code and remove the option
calling the script?

> However, looking at the output:
>
> xen$ head arch/x86/boot/built_in_32.S
>     .section .init.text, "ax", @progbits
>     .byte 137,194,128,56,0,116,6,64,128,56,0,117,250,41,208,195
>     .byte 133,201,116,42,86,83,141,52,8,64,15,182,72,255,66,15
>     .byte 182,90,255,56,217,117,15,132,201,116,25,57,198,117,234,184
>
> This wants to start with a comment saying that it was automatically
> generated by combine_two_binaries.
>

Added a

print('''/*
 * File autogenerated by combine_two_binaries.py DO NOT EDIT
 */''', file=out)

statement

And renamed the script file adding the ".py".

> Next, we need some kind of symbol at the start.  Right now, the
> disassembly reads:
>
>     arch/x86/boot/built_in_32.o:     file format elf64-x86-64
>     Disassembly of section .init.text:
>     0000000000000000 <cmdline_parse_early-0x391>:
>        0:   89 c2                   mov    %eax,%edx
>        2:   80 38 00                cmpb   $0x0,(%eax)
>
>
> because most metadata is lost by this transformation, and it doesn't
> know that this is in fact strlen().  I'd suggest suggest obj32_start: or
> equivalent.
>

Would "obj_start" fine too? Maybe in the future it could be used for
64 bit too (to avoid mistakes like the relocation of error strings we
had).

> > diff --git a/xen/arch/x86/boot/build32.lds b/xen/arch/x86/boot/build32.lds.S
> > similarity index 63%
> > rename from xen/arch/x86/boot/build32.lds
> > rename to xen/arch/x86/boot/build32.lds.S
> > index 56edaa727b..72a4c5c614 100644
> > --- a/xen/arch/x86/boot/build32.lds
> > +++ b/xen/arch/x86/boot/build32.lds.S
> > @@ -15,22 +15,52 @@
> >   * with this program.  If not, see <http://www.gnu.org/licenses/>.
> >   */
> >
> > -ENTRY(_start)
> > +#ifdef FINAL
> > +# define GAP 0
> > +# define MULT 0
> > +# define TEXT_START
> > +#else
> > +# define GAP 0x010200
> > +# define MULT 1
> > +# define TEXT_START 0x408020
> > +#endif
> > +# define DECLARE_IMPORT(name) name = . + (__LINE__ * MULT)
> > +
> > +ENTRY(dummy_start)
> >
> >  SECTIONS
> >  {
> > -  /* Merge code and data into one section. */
> > -  .text : {
> > +  /* Merge code and read-only data into one section. */
> > +  .text TEXT_START : {
> > +        /* Silence linker warning, we are not going to use it */
> > +        dummy_start = .;
> > +
> > +        /* Declare below any symbol name needed.
> > +         * Each symbol should be on its own line.
> > +         * It looks like a tedious work but we make sure the things we use.
> > +         * Potentially they should be all variables. */
> > +        DECLARE_IMPORT(__base_relocs_start);
> > +        DECLARE_IMPORT(__base_relocs_end);
> > +        . = . + GAP;
> >          *(.text)
> >          *(.text.*)
> > -        *(.data)
> > -        *(.data.*)
> >          *(.rodata)
> >          *(.rodata.*)
> > +  }
> > +
> > +  /* Writeable data sections. Check empty.
> > +   * We collapse all into code section and we don't want it to be 
> > writeable. */
> > +  .data : {
> > +        *(.data)
> > +        *(.data.*)
> >          *(.bss)
> >          *(.bss.*)
> >    }
> > -
> > +  /DISCARD/ : {
> > +       *(.comment)
> > +       *(.comment.*)
> > +       *(.note.*)
> > +  }
> >    /* Dynamic linkage sections.  Collected simply so we can check they're 
> > empty. */
> >    .got : {
> >          *(.got)
> > @@ -64,3 +94,4 @@ ASSERT(SIZEOF(.igot.plt) == 0,    ".igot.plt non-empty")
> >  ASSERT(SIZEOF(.iplt) == 0,        ".iplt non-empty")
> >  ASSERT(SIZEOF(.plt) == 0,         ".plt non-empty")
> >  ASSERT(SIZEOF(.rel) == 0,         "leftover relocations")
> > +ASSERT(SIZEOF(.data) == 0,        "we don't want data")
>
> 3mdeb are getting around to rebasing/resubmitting the Trenchboot work
> (Intel TXT and AMD SKINIT) backing QubeOS Anti-Evil-Maid.
>
> While most of the cleanup is proving very helpful (i.e. reducing their
> work), the lack of .data was seen as likely to be a blocker.  Thinking
> about this more, I'm now fairly certain we do not need to exclude data.
>

We could change if needed in the future.
Can I order:
- .text
- .rodata
- .data
- .bss

instead of old

- .text
- .data
- .rodata
- .bss

So all readonly code/data are more close?

> This object executes during boot in 32bit flat unpaged mode (i.e. there
> are no actual restrictions during execution), and because it's all
> wrapped in .init.text, its "just code" to the outside world.  This means
> it does not impact R^X that we're trying to arrange for the EFI section
> headers.
>
> Therefore the data arrangements should stay as they were before, and I
> think the result will be fine.  We obviously don't want gratuitous use
> of .data, but we don't need to prohibit it either.
>
> I've got various other suggestions for improvements of the result, but
> they can all be deferred until later.  This is complicated enough.
>
> > diff --git a/xen/tools/combine_two_binaries b/xen/tools/combine_two_binaries
> > new file mode 100755
> > index 0000000000..ea2d6ddc4e
> > --- /dev/null
> > +++ b/xen/tools/combine_two_binaries
> > @@ -0,0 +1,198 @@
> > +#!/usr/bin/env python3
> > +
> > +from __future__ import print_function
> > +import argparse
> > +import re
> > +import struct
> > +import sys
> > +
> > +parser = argparse.ArgumentParser(description='Generate assembly file to 
> > merge into other code.')
> > +parser.add_argument('--script', dest='script',
> > +                    required=True,
> > +                    help='Linker script for extracting symbols')
> > +parser.add_argument('--bin1', dest='bin1',
> > +                    required=True,
> > +                    help='First binary')
> > +parser.add_argument('--bin2', dest='bin2',
> > +                    required=True,
> > +                    help='Second binary')
> > +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('--exports', dest='exports',
> > +                    help='Symbols to export')
> > +parser.add_argument('--section-header', dest='section_header',
> > +                    default='.text',
> > +                    help='Section header declaration')
> > +parser.add_argument('-v', '--verbose',
> > +                    action='store_true')
> > +args = parser.parse_args()
> > +
> > +gap = 0x010200
> > +text_diff = 0x408020
> > +
> > +# Parse linker script for external symbols to use.
> > +symbol_re = 
> > re.compile(r'\s+(\S+)\s*=\s*\.\s*\+\s*\((\d+)\s*\*\s*0\s*\)\s*;')
>
> What is this looking for?  I'd expect the DECLARE_IMPORT() lines, but
> this is as clear as regexes...
>

Added a

# Next regex matches expanded DECLARE_IMPORT lines in linker script.

comment

> > +symbols = {}
> > +lines = {}
> > +for line in open(args.script):
> > +    m = symbol_re.match(line)
> > +    if not m:
> > +        continue
> > +    (name, line_num) = (m.group(1), int(m.group(2)))
> > +    if line_num == 0:
> > +        raise Exception("Invalid line number found:\n\t" + line)
> > +    if line_num in symbols:
> > +        raise Exception("Symbol with this line already present:\n\t" + 
> > line)
> > +    if name in lines:
> > +        raise Exception("Symbol with this name already present:\n\t" + 
> > name)
> > +    symbols[line_num] = name
> > +    lines[name] = line_num
> > +
> > +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:
> > +            continue
> > +        addr = int(m.group(1), 16)
> > +        exports[m.group(2)] = addr
> > +for (name, addr) in exports.items():
> > +    if addr is None:
> > +        raise Exception("Required export symbols %s not found" % name)
> > +
> > +file1 = open(args.bin1, 'rb')
> > +file2 = open(args.bin2, 'rb')
> > +file1.seek(0, 2)
> > +size1 = file1.tell()
> > +file2.seek(0, 2)
> > +size2 = file2.tell()
> > +if size1 > size2:
> > +    file1, file2 = file2, file1
> > +    size1, size2 = size2, size1
> > +if size2 != size1 + gap:
> > +    raise Exception('File sizes do not match')
> > +
> > +file1.seek(0, 0)
> > +data1 = file1.read(size1)
> > +file2.seek(gap, 0)
> > +data2 = file2.read(size1)
> > +
> > +max_line = max(symbols.keys())
> > +
> > +def to_int32(n):
> > +    '''Convert a number to signed 32 bit integer truncating if needed'''
> > +    mask = (1 << 32) - 1
> > +    h = 1 << 31
> > +    return (n & mask) ^ h - h
> > +
> > +i = 0
> > +references = {}
> > +internals = 0
> > +while i <= size1 - 4:
> > +    n1 = struct.unpack('<I', data1[i:i+4])[0]
> > +    n2 = struct.unpack('<I', data2[i:i+4])[0]
> > +    i += 1
> > +    # The two numbers are the same, no problems
> > +    if n1 == n2:
> > +        continue
> > +    # Try to understand why they are different
> > +    diff = to_int32(n1 - n2)
> > +    if diff == -gap: # this is an internal relocation
> > +        pos = i - 1
> > +        print(("Internal relocation found at position %#x "
> > +               "n1=%#x n2=%#x diff=%#x") % (pos, n1, n2, diff),
>
> Here and elsewhere, you don't need brackets around around the string
> itself.  Python strings are like C strings and will auto-concatenate.
>

Done (also another similar occurency below).

> > +              file=sys.stderr)
> > +        i += 3
> > +        internals += 1
> > +        if internals >= 10:
> > +            break
> > +        continue
> > +    # This is a relative relocation to a symbol, accepted, code/data is
> > +    # relocatable.
> > +    if diff < gap and diff >= gap - max_line:
> > +        n = gap - diff
> > +        symbol = symbols.get(n)
> > +        # check we have a symbol
> > +        if symbol is None:
> > +            raise Exception("Cannot find symbol for line %d" % n)
> > +        pos = i - 1
> > +        if args.verbose:
> > +            print('Position %#x %d %s' % (pos, n, symbol), file=sys.stderr)
> > +        i += 3
> > +        references[pos] = symbol
> > +        continue
> > +    # First byte is the same, move to next byte
> > +    if diff & 0xff == 0 and i <= size1 - 4:
> > +       continue
> > +    # Probably a type of relocation we don't want or support
> > +    pos = i - 1
> > +    suggestion = ''
> > +    symbol = symbols.get(-diff - text_diff)
> > +    if symbol is not None:
> > +        suggestion = " Maybe %s is not defined as hidden?" % symbol
> > +    raise Exception(("Unexpected difference found at %#x "
> > +                     "n1=%#x n2=%#x diff=%#x gap=%#x.%s") % \
> > +                     (pos, n1, n2, diff, gap, suggestion))

Here removed other parenthesis

> > +if internals != 0:
> > +    raise Exception("Previous relocations found")
> > +
> > +def line_bytes(buf, out):
> > +    '''Output an assembly line with all bytes in "buf"'''
> > +    if type(buf) == str:
> > +        print("\t.byte " + ','.join([str(ord(c)) for c in buf]), file=out)
> > +    else:
> > +        print("\t.byte " + ','.join([str(n) for n in buf]), file=out)
>
> I'm guessing this is Py2/3 compatibility?
>

Yes, as added

# Python 2 compatibility

will state

> > +
> > +def part(start, end, out):
> > +    '''Output bytes of "data" from "start" to "end"'''
> > +    while start < end:
> > +        e = min(start + 16, end)
> > +        line_bytes(data1[start:e], out)
> > +        start = e
> > +
> > +def reference(pos, out):
> > +    name = references[pos]
> > +    n = struct.unpack('<I', data1[pos:pos+4])[0]
> > +    sign = '+'
> > +    if n >= (1 << 31):
> > +        n -= (1 << 32)
> > +    n += pos
> > +    if n < 0:
> > +        n = -n
> > +        sign = '-'
> > +    print("\t.hidden %s\n\t.long %s %s %#x - ." % (name, name, sign, n),
>
> Personally, I think this is easier to read as:
>
>     print("\t.hidden %s\n"
>           "\t.long %s %s %#x - ." % (name, name, sign, n),
>           file=out)
>
> so it visually matches the output too.  Same for .globl/hidden lower.
>

Done

> ~Andrew

Frediano