* [PATCH v4 1/6] x86/boot: Prep work for 32bit object changes
2024-10-14 8:53 [PATCH v4 0/6] Reuse 32 bit C code more safely Frediano Ziglio
@ 2024-10-14 8:53 ` Frediano Ziglio
2024-10-15 6:44 ` Jan Beulich
2024-10-14 8:53 ` [PATCH v4 2/6] x86/boot: create a C bundle for 32 bit boot code and use it Frediano Ziglio
` (4 subsequent siblings)
5 siblings, 1 reply; 22+ messages in thread
From: Frediano Ziglio @ 2024-10-14 8:53 UTC (permalink / raw)
To: xen-devel
Cc: Frediano Ziglio, Jan Beulich, Andrew Cooper, Roger Pau Monné
Broken out of the subsequent patch for clarity.
* Rename head-bin-objs to obj32
* Use a .32.o suffix to distinguish these objects
* Factor out $(LD32)
No functional change.
Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
xen/arch/x86/boot/Makefile | 25 +++++++++++++++----------
1 file changed, 15 insertions(+), 10 deletions(-)
diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile
index ff0f965876..1199291d2b 100644
--- a/xen/arch/x86/boot/Makefile
+++ b/xen/arch/x86/boot/Makefile
@@ -1,15 +1,16 @@
obj-bin-y += head.o
-head-bin-objs := cmdline.o reloc.o
+obj32 := cmdline.32.o
+obj32 += reloc.32.o
-nocov-y += $(head-bin-objs)
-noubsan-y += $(head-bin-objs)
-targets += $(head-bin-objs)
+nocov-y += $(obj32)
+noubsan-y += $(obj32)
+targets += $(obj32)
-head-bin-objs := $(addprefix $(obj)/,$(head-bin-objs))
+obj32 := $(addprefix $(obj)/,$(obj32))
$(obj)/head.o: AFLAGS-y += -Wa$(comma)-I$(obj)
-$(obj)/head.o: $(head-bin-objs:.o=.bin)
+$(obj)/head.o: $(obj32:.32.o=.bin)
CFLAGS_x86_32 := $(subst -m64,-m32 -march=i686,$(XEN_TREEWIDE_CFLAGS))
$(call cc-options-add,CFLAGS_x86_32,CC,$(EMBEDDED_EXTRA_CFLAGS))
@@ -18,16 +19,20 @@ CFLAGS_x86_32 += -nostdinc -include $(filter %/include/xen/config.h,$(XEN_CFLAGS
CFLAGS_x86_32 += $(filter -I% -O%,$(XEN_CFLAGS)) -D__XEN__
# override for 32bit binaries
-$(head-bin-objs): CFLAGS_stack_boundary :=
-$(head-bin-objs): XEN_CFLAGS := $(CFLAGS_x86_32) -fpic
+$(obj32): CFLAGS_stack_boundary :=
+$(obj32): XEN_CFLAGS := $(CFLAGS_x86_32) -fpic
+
+$(obj)/%.32.o: $(src)/%.c FORCE
+ $(call if_changed_rule,cc_o_c)
LDFLAGS_DIRECT-$(call ld-option,--warn-rwx-segments) := --no-warn-rwx-segments
LDFLAGS_DIRECT += $(LDFLAGS_DIRECT-y)
+LD32 := $(LD) $(subst x86_64,i386,$(LDFLAGS_DIRECT))
%.bin: %.lnk
$(OBJCOPY) -j .text -O binary $< $@
-%.lnk: %.o $(src)/build32.lds
- $(LD) $(subst x86_64,i386,$(LDFLAGS_DIRECT)) -N -T $(filter %.lds,$^) -o $@ $<
+%.lnk: %.32.o $(src)/build32.lds
+ $(LD32) -N -T $(filter %.lds,$^) -o $@ $<
clean-files := *.lnk *.bin
--
2.34.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH v4 2/6] x86/boot: create a C bundle for 32 bit boot code and use it
2024-10-14 8:53 [PATCH v4 0/6] Reuse 32 bit C code more safely Frediano Ziglio
2024-10-14 8:53 ` [PATCH v4 1/6] x86/boot: Prep work for 32bit object changes Frediano Ziglio
@ 2024-10-14 8:53 ` Frediano Ziglio
2024-10-14 15:31 ` Anthony PERARD
2024-10-15 13:54 ` Andrew Cooper
2024-10-14 8:53 ` [PATCH v4 3/6] x86/boot: Reuse code to relocate trampoline Frediano Ziglio
` (3 subsequent siblings)
5 siblings, 2 replies; 22+ messages in thread
From: Frediano Ziglio @ 2024-10-14 8:53 UTC (permalink / raw)
To: xen-devel
Cc: Frediano Ziglio, Jan Beulich, Andrew Cooper, Roger Pau Monné,
Julien Grall, Stefano Stabellini
The current method to include 32 bit C boot code is:
- compile each function we want to use into a separate object file;
- each function is compiled with -fpic option;
- convert these object files to binary files. This operation removes GOP
which we don't want in the executable;
- a small assembly part in each file add the entry point;
- code can't have external references, all possible variables are passed
by value or pointer;
- include these binary files in head.S.
There are currently some limitations:
- code is compiled separately, it's not possible to share a function
(like memcpy) between different functions to use;
- although code is compiled with -fpic there's no certainty there are
no relocations, specifically data ones. This can lead into hard to
find bugs;
- it's hard to add a simple function;
- having to pass external variables makes hard to do multiple things
otherwise functions would require a lot of parameters so code would
have to be split into multiple functions which is not easy.
Current change extends the current process:
- all object files are linked together before getting converted making
possible to share code between the function we want to call;
- a single object file is generated with all functions to use and
exported symbols to easily call;
- variables to use are declared in linker script and easily used inside
C code. Declaring them manually could be annoying but makes also
easier to check them. Using external pointers can be still an issue if
they are not fixed. If an external symbol is not declared this gives a
link error.
Some details of the implementation:
- C code is compiled with -fpic flags (as before);
- object files from C code are linked together;
- the single bundled object file is linked with 2 slightly different
script files to generate 2 bundled object files;
- the 2 bundled object files are converted to binary removing the need
for global offset tables;
- a Python script is used to generate assembly source from the 2
binaries;
- the single assembly file is compiled to generate final bundled object
file;
- to detect possible unwanted relocation in data/code code is generated
with different addresses. This is enforced starting .text section at
different positions and adding a fixed "gap" at the beginning.
This makes sure code and data is position independent;
- to detect used symbols in data/code symbols are placed in .text
section at different offsets (based on the line in the linker script).
This is needed as potentially a reference to a symbol is converted to
a reference to the containing section so multiple symbols could be
converted to reference to same symbol (section name) and we need to
distinguish them;
- --orphan-handling=error option to linker is used to make sure we
account for all possible sections from C code;
Current limitations:
- the main one is the lack of support for 64 bit code. It would make
sure that even the code used for 64 bit (at the moment EFI code) is
code and data position independent. We cannot assume that code that
came from code compiled for 32 bit and compiled for 64 bit is code and
data position independent, different compiler options lead to
different code/data.
Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
---
Changes since v2:
- removed W^X limitation, allowing data;
- added some comments to python script;
- added extension to python script;
- added header to generated assembly code from python script;
- added starting symbol to generated assembly code from python script
to make disassembly more clear;
- other minor style changes to python script.
---
xen/arch/x86/boot/.gitignore | 5 +-
xen/arch/x86/boot/Makefile | 38 +++-
.../x86/boot/{build32.lds => build32.lds.S} | 35 ++-
xen/arch/x86/boot/cmdline.c | 12 -
xen/arch/x86/boot/head.S | 12 -
xen/arch/x86/boot/reloc.c | 14 --
xen/tools/combine_two_binaries.py | 207 ++++++++++++++++++
7 files changed, 268 insertions(+), 55 deletions(-)
rename xen/arch/x86/boot/{build32.lds => build32.lds.S} (70%)
create mode 100755 xen/tools/combine_two_binaries.py
diff --git a/xen/arch/x86/boot/.gitignore b/xen/arch/x86/boot/.gitignore
index a379db7988..ebad650e5c 100644
--- a/xen/arch/x86/boot/.gitignore
+++ b/xen/arch/x86/boot/.gitignore
@@ -1,3 +1,4 @@
/mkelf32
-/*.bin
-/*.lnk
+/build32.*.lds
+/built_in_32.*.bin
+/built_in_32.*.map
diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile
index 1199291d2b..23ad274c89 100644
--- a/xen/arch/x86/boot/Makefile
+++ b/xen/arch/x86/boot/Makefile
@@ -1,4 +1,5 @@
obj-bin-y += head.o
+obj-bin-y += built_in_32.o
obj32 := cmdline.32.o
obj32 += reloc.32.o
@@ -9,9 +10,6 @@ targets += $(obj32)
obj32 := $(addprefix $(obj)/,$(obj32))
-$(obj)/head.o: AFLAGS-y += -Wa$(comma)-I$(obj)
-$(obj)/head.o: $(obj32:.32.o=.bin)
-
CFLAGS_x86_32 := $(subst -m64,-m32 -march=i686,$(XEN_TREEWIDE_CFLAGS))
$(call cc-options-add,CFLAGS_x86_32,CC,$(EMBEDDED_EXTRA_CFLAGS))
CFLAGS_x86_32 += -Werror -fno-builtin -g0 -msoft-float -mregparm=3
@@ -25,14 +23,34 @@ $(obj32): XEN_CFLAGS := $(CFLAGS_x86_32) -fpic
$(obj)/%.32.o: $(src)/%.c FORCE
$(call if_changed_rule,cc_o_c)
+orphan-handling-$(call ld-option,--orphan-handling=error) := --orphan-handling=error
LDFLAGS_DIRECT-$(call ld-option,--warn-rwx-segments) := --no-warn-rwx-segments
LDFLAGS_DIRECT += $(LDFLAGS_DIRECT-y)
LD32 := $(LD) $(subst x86_64,i386,$(LDFLAGS_DIRECT))
-%.bin: %.lnk
- $(OBJCOPY) -j .text -O binary $< $@
-
-%.lnk: %.32.o $(src)/build32.lds
- $(LD32) -N -T $(filter %.lds,$^) -o $@ $<
-
-clean-files := *.lnk *.bin
+$(obj)/build32.final.lds: AFLAGS-y += -DFINAL
+$(obj)/build32.other.lds $(obj)/build32.final.lds: $(src)/build32.lds.S FORCE
+ $(call if_changed_dep,cpp_lds_S)
+
+# link all 32bit objects together
+$(obj)/built_in_32.tmp.o: $(obj32)
+ $(LD32) -r -o $@ $^
+
+$(obj)/built_in_32.%.bin: $(obj)/build32.%.lds $(obj)/built_in_32.tmp.o
+## link bundle with a given layout
+ $(LD32) $(orphan-handling-y) -N -T $< -Map $(obj)/built_in_32.$(*F).map -o $(obj)/built_in_32.$(*F).o $(obj)/built_in_32.tmp.o
+## extract binaries from object
+ $(OBJCOPY) -j .text -O binary $(obj)/built_in_32.$(*F).o $@
+ rm -f $(obj)/built_in_32.$(*F).o
+
+# generate final object file combining and checking above binaries
+$(obj)/built_in_32.S: $(obj)/built_in_32.other.bin $(obj)/built_in_32.final.bin
+ $(PYTHON) $(srctree)/tools/combine_two_binaries.py \
+ --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 \
+ --output $@
+
+clean-files := built_in_32.*.bin built_in_32.*.map build32.*.lds
diff --git a/xen/arch/x86/boot/build32.lds b/xen/arch/x86/boot/build32.lds.S
similarity index 70%
rename from xen/arch/x86/boot/build32.lds
rename to xen/arch/x86/boot/build32.lds.S
index 56edaa727b..50c167aef0 100644
--- a/xen/arch/x86/boot/build32.lds
+++ b/xen/arch/x86/boot/build32.lds.S
@@ -15,22 +15,47 @@
* 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 : {
+ .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.*)
+ *(.data)
+ *(.data.*)
*(.bss)
*(.bss.*)
}
-
+ /DISCARD/ : {
+ *(.comment)
+ *(.comment.*)
+ *(.note.*)
+ }
/* Dynamic linkage sections. Collected simply so we can check they're empty. */
.got : {
*(.got)
diff --git a/xen/arch/x86/boot/cmdline.c b/xen/arch/x86/boot/cmdline.c
index fc9241ede9..196c580e91 100644
--- a/xen/arch/x86/boot/cmdline.c
+++ b/xen/arch/x86/boot/cmdline.c
@@ -18,18 +18,6 @@
* Linux kernel source (linux/lib/string.c).
*/
-/*
- * This entry point is entered from xen/arch/x86/boot/head.S with:
- * - %eax = &cmdline,
- * - %edx = &early_boot_opts.
- */
-asm (
- " .text \n"
- " .globl _start \n"
- "_start: \n"
- " jmp cmdline_parse_early \n"
- );
-
#include <xen/compiler.h>
#include <xen/kconfig.h>
#include <xen/macros.h>
diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index c4de1dfab5..e0776e3896 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -759,18 +759,6 @@ trampoline_setup:
/* Jump into the relocated trampoline. */
lret
- /*
- * cmdline and reloc are written in C, and linked to be 32bit PIC with
- * entrypoints at 0 and using the fastcall convention.
- */
-FUNC_LOCAL(cmdline_parse_early)
- .incbin "cmdline.bin"
-END(cmdline_parse_early)
-
-FUNC_LOCAL(reloc)
- .incbin "reloc.bin"
-END(reloc)
-
ENTRY(trampoline_start)
#include "trampoline.S"
ENTRY(trampoline_end)
diff --git a/xen/arch/x86/boot/reloc.c b/xen/arch/x86/boot/reloc.c
index 8c58affcd9..94b078d7b1 100644
--- a/xen/arch/x86/boot/reloc.c
+++ b/xen/arch/x86/boot/reloc.c
@@ -12,20 +12,6 @@
* Daniel Kiper <daniel.kiper@oracle.com>
*/
-/*
- * This entry point is entered from xen/arch/x86/boot/head.S with:
- * - %eax = MAGIC,
- * - %edx = INFORMATION_ADDRESS,
- * - %ecx = TOPMOST_LOW_MEMORY_STACK_ADDRESS.
- * - 0x04(%esp) = BOOT_VIDEO_INFO_ADDRESS.
- */
-asm (
- " .text \n"
- " .globl _start \n"
- "_start: \n"
- " jmp reloc \n"
- );
-
#include <xen/compiler.h>
#include <xen/macros.h>
#include <xen/types.h>
diff --git a/xen/tools/combine_two_binaries.py b/xen/tools/combine_two_binaries.py
new file mode 100755
index 0000000000..138c59287e
--- /dev/null
+++ b/xen/tools/combine_two_binaries.py
@@ -0,0 +1,207 @@
+#!/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='.section .init.text, "ax", @progbits',
+ 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.
+# Next regex matches expanded DECLARE_IMPORT lines in linker script.
+symbol_re = re.compile(r'\s+(\S+)\s*=\s*\.\s*\+\s*\((\d+)\s*\*\s*0\s*\)\s*;')
+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),
+ 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))
+if internals != 0:
+ raise Exception("Previous relocations found")
+
+def line_bytes(buf, out):
+ '''Output an assembly line with all bytes in "buf"'''
+ # Python 2 compatibility
+ 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)
+
+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),
+ file=out)
+
+def output(out):
+ prev = 0
+ exports_by_addr = {}
+ for (sym, addr) in exports.items():
+ exports_by_addr.setdefault(addr, []).append(sym)
+ positions = list(references.keys())
+ positions += list(exports_by_addr.keys())
+ for pos in sorted(positions):
+ part(prev, pos, out)
+ prev = pos
+ if pos in references:
+ reference(pos, out)
+ prev = pos + 4
+ if pos in exports_by_addr:
+ for sym in exports_by_addr[pos]:
+ print("\t.global %s\n"
+ "\t.hidden %s\n"
+ "%s:" % (sym, sym, sym),
+ file=out)
+ part(prev, size1, out)
+
+out = sys.stdout
+if args.output is not None:
+ out = open(args.output, 'w')
+print('''/*
+ * File autogenerated by combine_two_binaries.py DO NOT EDIT
+ */''', file=out)
+print('\t' + args.section_header, file=out)
+print('obj_start:', file=out)
+output(out)
+print('\n\t.section\t.note.GNU-stack,"",@progbits', file=out)
+out.flush()
--
2.34.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v4 2/6] x86/boot: create a C bundle for 32 bit boot code and use it
2024-10-14 8:53 ` [PATCH v4 2/6] x86/boot: create a C bundle for 32 bit boot code and use it Frediano Ziglio
@ 2024-10-14 15:31 ` Anthony PERARD
2024-10-14 15:46 ` Jan Beulich
` (2 more replies)
2024-10-15 13:54 ` Andrew Cooper
1 sibling, 3 replies; 22+ messages in thread
From: Anthony PERARD @ 2024-10-14 15:31 UTC (permalink / raw)
To: Frediano Ziglio
Cc: xen-devel, Jan Beulich, Andrew Cooper, Roger Pau Monné,
Julien Grall, Stefano Stabellini
On Mon, Oct 14, 2024 at 09:53:28AM +0100, Frediano Ziglio wrote:
> diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile
> index 1199291d2b..23ad274c89 100644
> --- a/xen/arch/x86/boot/Makefile
> +++ b/xen/arch/x86/boot/Makefile
> @@ -1,4 +1,5 @@
> obj-bin-y += head.o
> +obj-bin-y += built_in_32.o
I don't quite like that this new object name is "built_in_32.o",
It's really closed to "built_in.*" which is used by Rules.mk to collect
all objects in a subdirectory. I don't really have a better suggestion at
the moment.
> @@ -9,9 +10,6 @@ targets += $(obj32)
>
> obj32 := $(addprefix $(obj)/,$(obj32))
>
> -$(obj)/head.o: AFLAGS-y += -Wa$(comma)-I$(obj)
> -$(obj)/head.o: $(obj32:.32.o=.bin)
> -
> CFLAGS_x86_32 := $(subst -m64,-m32 -march=i686,$(XEN_TREEWIDE_CFLAGS))
> $(call cc-options-add,CFLAGS_x86_32,CC,$(EMBEDDED_EXTRA_CFLAGS))
> CFLAGS_x86_32 += -Werror -fno-builtin -g0 -msoft-float -mregparm=3
> @@ -25,14 +23,34 @@ $(obj32): XEN_CFLAGS := $(CFLAGS_x86_32) -fpic
> $(obj)/%.32.o: $(src)/%.c FORCE
> $(call if_changed_rule,cc_o_c)
>
> +orphan-handling-$(call ld-option,--orphan-handling=error) := --orphan-handling=error
> LDFLAGS_DIRECT-$(call ld-option,--warn-rwx-segments) := --no-warn-rwx-segments
> LDFLAGS_DIRECT += $(LDFLAGS_DIRECT-y)
> LD32 := $(LD) $(subst x86_64,i386,$(LDFLAGS_DIRECT))
>
> -%.bin: %.lnk
> - $(OBJCOPY) -j .text -O binary $< $@
> -
> -%.lnk: %.32.o $(src)/build32.lds
> - $(LD32) -N -T $(filter %.lds,$^) -o $@ $<
> -
> -clean-files := *.lnk *.bin
> +$(obj)/build32.final.lds: AFLAGS-y += -DFINAL
> +$(obj)/build32.other.lds $(obj)/build32.final.lds: $(src)/build32.lds.S FORCE
> + $(call if_changed_dep,cpp_lds_S)
> +
> +# link all 32bit objects together
> +$(obj)/built_in_32.tmp.o: $(obj32)
> + $(LD32) -r -o $@ $^
> +
> +$(obj)/built_in_32.%.bin: $(obj)/build32.%.lds $(obj)/built_in_32.tmp.o
> +## link bundle with a given layout
Could you put the comment aligned with the rest of the recipe? That way
it won't visually separate the rule into several pieces. You can
prefix it with @ so it doesn't show in build logs:
@# comments
> + $(LD32) $(orphan-handling-y) -N -T $< -Map $(obj)/built_in_32.$(*F).map -o $(obj)/built_in_32.$(*F).o $(obj)/built_in_32.tmp.o
I think this wants to be: -T $(filter %.lds,$^) -Map $(patsubst %.bin,%.map,$@) -o $(patsubst %.bin,%.o,$@) $(filter %.o,$^)
:-(, maybe that's lots of $(patsubst,), not sure which is better between
$(patsubst,) and using the stem $*.
Also, if something tries to use built_in_32.tmp.bin, we have a rule that
remove it's prerequisite.
BTW, everything is kind of temporary in a build system, beside the few
files that we want to install on a machine, so having a target named
with "*tmp*" isn't great. But having a rule that create "*tmp*" file but
remove them before the end of its recipe is fine (or those *tmp* aren't
use outside of this recipe).
> +## extract binaries from object
> + $(OBJCOPY) -j .text -O binary $(obj)/built_in_32.$(*F).o $@
> + rm -f $(obj)/built_in_32.$(*F).o
> +
> +# generate final object file combining and checking above binaries
> +$(obj)/built_in_32.S: $(obj)/built_in_32.other.bin $(obj)/built_in_32.final.bin
So, "other" isn't part of "final", I don't really know what those two
things contains so naming wise I can't suggest anything useful.
But, is "built_in_32.S" really only depends on those two .bin? SHouldn't
it get regenerated if the script changes, or the .lds that --script
option seems to use? What is the "--map" option, an input or output?
But I guess we can ignore the ".map" file because it's part of the
".bin".
Another thing that might be useful to do, is to use the "if_changed"
make macro, that way if the command line of the script changes, make
can remake the output. But maybe it's a bit complicated for this recipe
because it doesn't uses $< or $^.
> + $(PYTHON) $(srctree)/tools/combine_two_binaries.py \
> + --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 \
> + --output $@
> +
> +clean-files := built_in_32.*.bin built_in_32.*.map build32.*.lds
> diff --git a/xen/arch/x86/boot/build32.lds b/xen/arch/x86/boot/build32.lds.S
> similarity index 70%
> rename from xen/arch/x86/boot/build32.lds
> rename to xen/arch/x86/boot/build32.lds.S
> index 56edaa727b..50c167aef0 100644
> --- a/xen/arch/x86/boot/build32.lds
> +++ b/xen/arch/x86/boot/build32.lds.S
> @@ -15,22 +15,47 @@
> * 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)
Is ^ this a stray space?
Overall, it's kind of mostly style comment that I had, so feel free to
ignore most of them.
Cheers,
--
Anthony Perard | Vates XCP-ng Developer
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v4 2/6] x86/boot: create a C bundle for 32 bit boot code and use it
2024-10-14 15:31 ` Anthony PERARD
@ 2024-10-14 15:46 ` Jan Beulich
2024-10-14 15:52 ` Andrew Cooper
2024-10-14 16:32 ` Frediano Ziglio
2 siblings, 0 replies; 22+ messages in thread
From: Jan Beulich @ 2024-10-14 15:46 UTC (permalink / raw)
To: Anthony PERARD, Frediano Ziglio
Cc: xen-devel, Andrew Cooper, Roger Pau Monné, Julien Grall,
Stefano Stabellini
On 14.10.2024 17:31, Anthony PERARD wrote:
> On Mon, Oct 14, 2024 at 09:53:28AM +0100, Frediano Ziglio wrote:
>> diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile
>> index 1199291d2b..23ad274c89 100644
>> --- a/xen/arch/x86/boot/Makefile
>> +++ b/xen/arch/x86/boot/Makefile
>> @@ -1,4 +1,5 @@
>> obj-bin-y += head.o
>> +obj-bin-y += built_in_32.o
>
> I don't quite like that this new object name is "built_in_32.o",
> It's really closed to "built_in.*" which is used by Rules.mk to collect
> all objects in a subdirectory. I don't really have a better suggestion at
> the moment.
blob32.o? bundle32.o?
Jan
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 2/6] x86/boot: create a C bundle for 32 bit boot code and use it
2024-10-14 15:31 ` Anthony PERARD
2024-10-14 15:46 ` Jan Beulich
@ 2024-10-14 15:52 ` Andrew Cooper
2024-10-14 16:32 ` Frediano Ziglio
2 siblings, 0 replies; 22+ messages in thread
From: Andrew Cooper @ 2024-10-14 15:52 UTC (permalink / raw)
To: Anthony PERARD, Frediano Ziglio
Cc: xen-devel, Jan Beulich, Roger Pau Monné, Julien Grall,
Stefano Stabellini
On 14/10/2024 4:31 pm, Anthony PERARD wrote:
> On Mon, Oct 14, 2024 at 09:53:28AM +0100, Frediano Ziglio wrote:
>> diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile
>> index 1199291d2b..23ad274c89 100644
>> --- a/xen/arch/x86/boot/Makefile
>> +++ b/xen/arch/x86/boot/Makefile
>> @@ -1,4 +1,5 @@
>> obj-bin-y += head.o
>> +obj-bin-y += built_in_32.o
> I don't quite like that this new object name is "built_in_32.o",
> It's really closed to "built_in.*" which is used by Rules.mk to collect
> all objects in a subdirectory. I don't really have a better suggestion at
> the moment.
Similarity was the point of suggesting it.
It is the end result of "the 32bit objects we've done magic with to
shoehorn into 64bit", and a improvement on "cbundle" from earlier
iterations.
~Andrew
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 2/6] x86/boot: create a C bundle for 32 bit boot code and use it
2024-10-14 15:31 ` Anthony PERARD
2024-10-14 15:46 ` Jan Beulich
2024-10-14 15:52 ` Andrew Cooper
@ 2024-10-14 16:32 ` Frediano Ziglio
2024-10-15 5:59 ` Jan Beulich
2024-10-15 13:51 ` Anthony PERARD
2 siblings, 2 replies; 22+ messages in thread
From: Frediano Ziglio @ 2024-10-14 16:32 UTC (permalink / raw)
To: Anthony PERARD
Cc: xen-devel, Jan Beulich, Andrew Cooper, Roger Pau Monné,
Julien Grall, Stefano Stabellini
On Mon, Oct 14, 2024 at 4:31 PM Anthony PERARD
<anthony.perard@vates.tech> wrote:
>
> On Mon, Oct 14, 2024 at 09:53:28AM +0100, Frediano Ziglio wrote:
> > diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile
> > index 1199291d2b..23ad274c89 100644
> > --- a/xen/arch/x86/boot/Makefile
> > +++ b/xen/arch/x86/boot/Makefile
> > @@ -1,4 +1,5 @@
> > obj-bin-y += head.o
> > +obj-bin-y += built_in_32.o
>
> I don't quite like that this new object name is "built_in_32.o",
> It's really closed to "built_in.*" which is used by Rules.mk to collect
> all objects in a subdirectory. I don't really have a better suggestion at
> the moment.
>
It was cbundle.o before, but people preferred built_in_32.o.
It's a collection of object files like built_in.o so it does not seem
so bad to me.
But seen other replies, some other people prefer "bundle".
> > @@ -9,9 +10,6 @@ targets += $(obj32)
> >
> > obj32 := $(addprefix $(obj)/,$(obj32))
> >
> > -$(obj)/head.o: AFLAGS-y += -Wa$(comma)-I$(obj)
> > -$(obj)/head.o: $(obj32:.32.o=.bin)
> > -
> > CFLAGS_x86_32 := $(subst -m64,-m32 -march=i686,$(XEN_TREEWIDE_CFLAGS))
> > $(call cc-options-add,CFLAGS_x86_32,CC,$(EMBEDDED_EXTRA_CFLAGS))
> > CFLAGS_x86_32 += -Werror -fno-builtin -g0 -msoft-float -mregparm=3
> > @@ -25,14 +23,34 @@ $(obj32): XEN_CFLAGS := $(CFLAGS_x86_32) -fpic
> > $(obj)/%.32.o: $(src)/%.c FORCE
> > $(call if_changed_rule,cc_o_c)
> >
> > +orphan-handling-$(call ld-option,--orphan-handling=error) := --orphan-handling=error
> > LDFLAGS_DIRECT-$(call ld-option,--warn-rwx-segments) := --no-warn-rwx-segments
> > LDFLAGS_DIRECT += $(LDFLAGS_DIRECT-y)
> > LD32 := $(LD) $(subst x86_64,i386,$(LDFLAGS_DIRECT))
> >
> > -%.bin: %.lnk
> > - $(OBJCOPY) -j .text -O binary $< $@
> > -
> > -%.lnk: %.32.o $(src)/build32.lds
> > - $(LD32) -N -T $(filter %.lds,$^) -o $@ $<
> > -
> > -clean-files := *.lnk *.bin
> > +$(obj)/build32.final.lds: AFLAGS-y += -DFINAL
> > +$(obj)/build32.other.lds $(obj)/build32.final.lds: $(src)/build32.lds.S FORCE
> > + $(call if_changed_dep,cpp_lds_S)
> > +
> > +# link all 32bit objects together
> > +$(obj)/built_in_32.tmp.o: $(obj32)
> > + $(LD32) -r -o $@ $^
> > +
> > +$(obj)/built_in_32.%.bin: $(obj)/build32.%.lds $(obj)/built_in_32.tmp.o
> > +## link bundle with a given layout
>
> Could you put the comment aligned with the rest of the recipe? That way
> it won't visually separate the rule into several pieces. You can
> prefix it with @ so it doesn't show in build logs:
>
> @# comments
>
Yes, they look more or less the same but technically pretty different.
The "## XXX" is a comment for make command, the "\t@#comment" is a way
to tell make to not print the command before launching it so make will
launch a shell command "# comment".
Yes, indentation does not make it clear. Not that launching a shell to
execute a comment will take a long time. On the other hand, here that
comment is more for the rule than for the shell. Maybe something like
target:
command \
# do something
(personally I found it even more ugly)
> > + $(LD32) $(orphan-handling-y) -N -T $< -Map $(obj)/built_in_32.$(*F).map -o $(obj)/built_in_32.$(*F).o $(obj)/built_in_32.tmp.o
>
> I think this wants to be: -T $(filter %.lds,$^) -Map $(patsubst %.bin,%.map,$@) -o $(patsubst %.bin,%.o,$@) $(filter %.o,$^)
>
> :-(, maybe that's lots of $(patsubst,), not sure which is better between
> $(patsubst,) and using the stem $*.
>
Not strong about stem or patsubst.
The 2 filters seem good, they suggest lds for the script and objects
for the input, which makes sense.
> Also, if something tries to use built_in_32.tmp.bin, we have a rule that
> remove it's prerequisite.
>
> BTW, everything is kind of temporary in a build system, beside the few
> files that we want to install on a machine, so having a target named
> with "*tmp*" isn't great. But having a rule that create "*tmp*" file but
> remove them before the end of its recipe is fine (or those *tmp* aren't
> use outside of this recipe).
>
Mumble... yes, I think the XX.tmp.o was a temporary internal rule file.
So we still don't agree on one name, and now we want to find also
another, tricky.
More or less if it can help, one is a 32 bit object file that bundle
together multiple 32 bits object files while the other is the
conversion of the 32 bits bundle file to 64 bits.
XXX_32.o and XXX_32as64.o ??
> > +## extract binaries from object
> > + $(OBJCOPY) -j .text -O binary $(obj)/built_in_32.$(*F).o $@
> > + rm -f $(obj)/built_in_32.$(*F).o
> > +
> > +# generate final object file combining and checking above binaries
> > +$(obj)/built_in_32.S: $(obj)/built_in_32.other.bin $(obj)/built_in_32.final.bin
>
> So, "other" isn't part of "final", I don't really know what those two
> things contains so naming wise I can't suggest anything useful.
>
> But, is "built_in_32.S" really only depends on those two .bin? SHouldn't
> it get regenerated if the script changes, or the .lds that --script
> option seems to use? What is the "--map" option, an input or output?
Both map file and linker script are dependency of the bin files so no problems.
Yes, potentially you want to generate if the python script change.
Potentially also if Makefile, make, ld or any other command change,
but that possibly is kind of "we don't usually care".
> But I guess we can ignore the ".map" file because it's part of the
> ".bin".
>
Yes, I suppose they are, although we can make it explicit both
generating them and using as dependencies.
> Another thing that might be useful to do, is to use the "if_changed"
> make macro, that way if the command line of the script changes, make
> can remake the output. But maybe it's a bit complicated for this recipe
> because it doesn't uses $< or $^.
>
I usually simply add Makefile to the dependencies. It works too.
> > + $(PYTHON) $(srctree)/tools/combine_two_binaries.py \
> > + --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 \
> > + --output $@
> > +
> > +clean-files := built_in_32.*.bin built_in_32.*.map build32.*.lds
> > diff --git a/xen/arch/x86/boot/build32.lds b/xen/arch/x86/boot/build32.lds.S
> > similarity index 70%
> > rename from xen/arch/x86/boot/build32.lds
> > rename to xen/arch/x86/boot/build32.lds.S
> > index 56edaa727b..50c167aef0 100644
> > --- a/xen/arch/x86/boot/build32.lds
> > +++ b/xen/arch/x86/boot/build32.lds.S
> > @@ -15,22 +15,47 @@
> > * 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)
>
> Is ^ this a stray space?
>
Yes, a single ASCII character 32. Surely we don't want tabs. Other
parts of the file use 2 spaces indentation, others 8 spaces. Or are
you suggesting to not indent them?
>
> Overall, it's kind of mostly style comment that I had, so feel free to
> ignore most of them.
>
Naming can be cumbersome but usually helps readability.
> Cheers,
>
> --
>
> Anthony Perard | Vates XCP-ng Developer
>
> XCP-ng & Xen Orchestra - Vates solutions
>
> web: https://vates.tech
>
Frediano
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v4 2/6] x86/boot: create a C bundle for 32 bit boot code and use it
2024-10-14 16:32 ` Frediano Ziglio
@ 2024-10-15 5:59 ` Jan Beulich
2024-10-15 13:51 ` Anthony PERARD
1 sibling, 0 replies; 22+ messages in thread
From: Jan Beulich @ 2024-10-15 5:59 UTC (permalink / raw)
To: Frediano Ziglio, Anthony PERARD
Cc: xen-devel, Andrew Cooper, Roger Pau Monné, Julien Grall,
Stefano Stabellini
On 14.10.2024 18:32, Frediano Ziglio wrote:
> On Mon, Oct 14, 2024 at 4:31 PM Anthony PERARD
> <anthony.perard@vates.tech> wrote:
>>
>> On Mon, Oct 14, 2024 at 09:53:28AM +0100, Frediano Ziglio wrote:
>>> diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile
>>> index 1199291d2b..23ad274c89 100644
>>> --- a/xen/arch/x86/boot/Makefile
>>> +++ b/xen/arch/x86/boot/Makefile
>>> @@ -1,4 +1,5 @@
>>> obj-bin-y += head.o
>>> +obj-bin-y += built_in_32.o
>>
>> I don't quite like that this new object name is "built_in_32.o",
>> It's really closed to "built_in.*" which is used by Rules.mk to collect
>> all objects in a subdirectory. I don't really have a better suggestion at
>> the moment.
>>
>
> It was cbundle.o before, but people preferred built_in_32.o.
> It's a collection of object files like built_in.o so it does not seem
> so bad to me.
> But seen other replies, some other people prefer "bundle".
Well, I for one don't really _prefer_ bundle. I merely see it as a possible
option to address Anthony's name ambiguity concern.
>>> + $(LD32) $(orphan-handling-y) -N -T $< -Map $(obj)/built_in_32.$(*F).map -o $(obj)/built_in_32.$(*F).o $(obj)/built_in_32.tmp.o
>>
>> I think this wants to be: -T $(filter %.lds,$^) -Map $(patsubst %.bin,%.map,$@) -o $(patsubst %.bin,%.o,$@) $(filter %.o,$^)
>>
>> :-(, maybe that's lots of $(patsubst,), not sure which is better between
>> $(patsubst,) and using the stem $*.
>>
>
> Not strong about stem or patsubst.
> The 2 filters seem good, they suggest lds for the script and objects
> for the input, which makes sense.
>
>> Also, if something tries to use built_in_32.tmp.bin, we have a rule that
>> remove it's prerequisite.
>>
>> BTW, everything is kind of temporary in a build system, beside the few
>> files that we want to install on a machine, so having a target named
>> with "*tmp*" isn't great. But having a rule that create "*tmp*" file but
>> remove them before the end of its recipe is fine (or those *tmp* aren't
>> use outside of this recipe).
>>
>
> Mumble... yes, I think the XX.tmp.o was a temporary internal rule file.
> So we still don't agree on one name, and now we want to find also
> another, tricky.
> More or less if it can help, one is a 32 bit object file that bundle
> together multiple 32 bits object files while the other is the
> conversion of the 32 bits bundle file to 64 bits.
> XXX_32.o and XXX_32as64.o ??
Whatever the eventual name (I don't care all that much), just one request:
Dashes instead of underscores please.
Jan
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 2/6] x86/boot: create a C bundle for 32 bit boot code and use it
2024-10-14 16:32 ` Frediano Ziglio
2024-10-15 5:59 ` Jan Beulich
@ 2024-10-15 13:51 ` Anthony PERARD
2024-10-16 8:33 ` Frediano Ziglio
1 sibling, 1 reply; 22+ messages in thread
From: Anthony PERARD @ 2024-10-15 13:51 UTC (permalink / raw)
To: Frediano Ziglio
Cc: xen-devel, Jan Beulich, Andrew Cooper, Roger Pau Monné,
Julien Grall, Stefano Stabellini
On Mon, Oct 14, 2024 at 05:32:26PM +0100, Frediano Ziglio wrote:
> On Mon, Oct 14, 2024 at 4:31 PM Anthony PERARD
> <anthony.perard@vates.tech> wrote:
> >
> > On Mon, Oct 14, 2024 at 09:53:28AM +0100, Frediano Ziglio wrote:
> > > diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile
> > > index 1199291d2b..23ad274c89 100644
> > > --- a/xen/arch/x86/boot/Makefile
> > > +++ b/xen/arch/x86/boot/Makefile
> > > @@ -1,4 +1,5 @@
> > > obj-bin-y += head.o
> > > +obj-bin-y += built_in_32.o
> >
> > I don't quite like that this new object name is "built_in_32.o",
> > It's really closed to "built_in.*" which is used by Rules.mk to collect
> > all objects in a subdirectory. I don't really have a better suggestion at
> > the moment.
> >
>
> It was cbundle.o before, but people preferred built_in_32.o.
> It's a collection of object files like built_in.o so it does not seem
> so bad to me.
> But seen other replies, some other people prefer "bundle".
Yeah, I guess it's ok (built_in_32 that is; now that I gave a better
review of the rest of the Makefile changes).
> > > @@ -9,9 +10,6 @@ targets += $(obj32)
> > >
> > > obj32 := $(addprefix $(obj)/,$(obj32))
> > >
> > > -$(obj)/head.o: AFLAGS-y += -Wa$(comma)-I$(obj)
> > > -$(obj)/head.o: $(obj32:.32.o=.bin)
> > > -
> > > CFLAGS_x86_32 := $(subst -m64,-m32 -march=i686,$(XEN_TREEWIDE_CFLAGS))
> > > $(call cc-options-add,CFLAGS_x86_32,CC,$(EMBEDDED_EXTRA_CFLAGS))
> > > CFLAGS_x86_32 += -Werror -fno-builtin -g0 -msoft-float -mregparm=3
> > > @@ -25,14 +23,34 @@ $(obj32): XEN_CFLAGS := $(CFLAGS_x86_32) -fpic
> > > $(obj)/%.32.o: $(src)/%.c FORCE
> > > $(call if_changed_rule,cc_o_c)
> > >
> > > +orphan-handling-$(call ld-option,--orphan-handling=error) := --orphan-handling=error
> > > LDFLAGS_DIRECT-$(call ld-option,--warn-rwx-segments) := --no-warn-rwx-segments
> > > LDFLAGS_DIRECT += $(LDFLAGS_DIRECT-y)
> > > LD32 := $(LD) $(subst x86_64,i386,$(LDFLAGS_DIRECT))
> > >
> > > -%.bin: %.lnk
> > > - $(OBJCOPY) -j .text -O binary $< $@
> > > -
> > > -%.lnk: %.32.o $(src)/build32.lds
> > > - $(LD32) -N -T $(filter %.lds,$^) -o $@ $<
> > > -
> > > -clean-files := *.lnk *.bin
> > > +$(obj)/build32.final.lds: AFLAGS-y += -DFINAL
> > > +$(obj)/build32.other.lds $(obj)/build32.final.lds: $(src)/build32.lds.S FORCE
> > > + $(call if_changed_dep,cpp_lds_S)
Could you add:
targets += build32.final.lds build32.other.lds
otherwise, the if_changes macro doesn't work, and the target keeps been
rebuilt. `make V=2` will tell you the same thing.
> > > +# link all 32bit objects together
> > > +$(obj)/built_in_32.tmp.o: $(obj32)
> > > + $(LD32) -r -o $@ $^
> > > +
> > > +$(obj)/built_in_32.%.bin: $(obj)/build32.%.lds $(obj)/built_in_32.tmp.o
> > > +## link bundle with a given layout
> >
> > Could you put the comment aligned with the rest of the recipe? That way
> > it won't visually separate the rule into several pieces. You can
> > prefix it with @ so it doesn't show in build logs:
> >
> > @# comments
> >
>
> Yes, they look more or less the same but technically pretty different.
> The "## XXX" is a comment for make command, the "\t@#comment" is a way
> to tell make to not print the command before launching it so make will
> launch a shell command "# comment".
> Yes, indentation does not make it clear. Not that launching a shell to
> execute a comment will take a long time. On the other hand, here that
> comment is more for the rule than for the shell. Maybe something like
>
> target:
> command \
> # do something
>
> (personally I found it even more ugly)
How about removing the comments? I mean the command line is
self-explanatory here. Same thing about the objcopy comment.
(Or a comment before the rule, when really needed)
> > > + $(LD32) $(orphan-handling-y) -N -T $< -Map $(obj)/built_in_32.$(*F).map -o $(obj)/built_in_32.$(*F).o $(obj)/built_in_32.tmp.o
> >
> > I think this wants to be: -T $(filter %.lds,$^) -Map $(patsubst %.bin,%.map,$@) -o $(patsubst %.bin,%.o,$@) $(filter %.o,$^)
> >
> > :-(, maybe that's lots of $(patsubst,), not sure which is better between
> > $(patsubst,) and using the stem $*.
> >
>
> Not strong about stem or patsubst.
Actually, I found a better suggestion (after reading the previous path
which remind me about an existing feature that I don't really use), the
use of Substitution References
(https://www.gnu.org/software/make/manual/make.html#Substitution-Refs)
This is a shorter version than patsubst, but works very well here, the
recipe can look like this:
$(LD32) $(orphan-handling-y) -N -T $< -Map $(@:bin=map) -o $(@:bin=o) $(filter %.o,$^)
$(OBJCOPY) -j .text -O binary $(@:bin=o) $@
rm -f $(@:bin=o)
(It is likely fine to keep the reference to the lds script as $<,
instead of using $(filter,).)
BTW, do we need the rules that generate the input of this rules
(built_in_32.tmp.o that is), or can this one takes all 32bit objects as
input?
> The 2 filters seem good, they suggest lds for the script and objects
> for the input, which makes sense.
> > Also, if something tries to use built_in_32.tmp.bin, we have a rule that
> > remove it's prerequisite.
> >
> > BTW, everything is kind of temporary in a build system, beside the few
> > files that we want to install on a machine, so having a target named
> > with "*tmp*" isn't great. But having a rule that create "*tmp*" file but
> > remove them before the end of its recipe is fine (or those *tmp* aren't
> > use outside of this recipe).
> >
>
> Mumble... yes, I think the XX.tmp.o was a temporary internal rule file.
> So we still don't agree on one name, and now we want to find also
> another, tricky.
> More or less if it can help, one is a 32 bit object file that bundle
> together multiple 32 bits object files while the other is the
> conversion of the 32 bits bundle file to 64 bits.
> XXX_32.o and XXX_32as64.o ??
We may not need the rule for it :-).
> > > +## extract binaries from object
> > > + $(OBJCOPY) -j .text -O binary $(obj)/built_in_32.$(*F).o $@
> > > + rm -f $(obj)/built_in_32.$(*F).o
> > > +
> > > +# generate final object file combining and checking above binaries
> > > +$(obj)/built_in_32.S: $(obj)/built_in_32.other.bin $(obj)/built_in_32.final.bin
> >
> > So, "other" isn't part of "final", I don't really know what those two
> > things contains so naming wise I can't suggest anything useful.
Instead of "other", is "control" (like in science experiment where you
have a control group), or "offseted" (which seems to be how this second
binary is constructed) would be better names for this *.bin? It seems
the script take both input and play the game of the 7 differences, to
find clues about the location of some symbols, right?.
> > But, is "built_in_32.S" really only depends on those two .bin? SHouldn't
> > it get regenerated if the script changes, or the .lds that --script
> > option seems to use? What is the "--map" option, an input or output?
>
> Both map file and linker script are dependency of the bin files so no problems.
> Yes, potentially you want to generate if the python script change.
> Potentially also if Makefile, make, ld or any other command change,
> but that possibly is kind of "we don't usually care".
Actually, for the hypervisor, we usually care about changes which could
lead to a different output. While changes of build toolchain aren't
really track (even if there's a patch that have a small improvement
toward it that was never committed), many other changes are been tracked
and act upon. The macros $(if_changes*,) from Linux's Kbuild are a great
help.
If the build system doesn't regenerate targets when a developer try to
debug some complex problem, it isn't going to help. We can easily have a
situation where the developer adds printf earlier and earlier and
earlier to try to find out where Xen crash, to find out later that the
build system was working against them by not rebuilding the file that
should be rebuilt. So if we can avoid some frustration and time lost,
it's better :-).
> > But I guess we can ignore the ".map" file because it's part of the
> > ".bin".
> >
>
> Yes, I suppose they are, although we can make it explicit both
> generating them and using as dependencies.
Having a rule that generate several targets is complicated to write in
GNU Make 3.80, the rule is more likely to be called several time in
parallel. So let's ignored the .map file, as it should only be used
along side the .bin.
> > Another thing that might be useful to do, is to use the "if_changed"
> > make macro, that way if the command line of the script changes, make
> > can remake the output. But maybe it's a bit complicated for this recipe
> > because it doesn't uses $< or $^.
> >
>
> I usually simply add Makefile to the dependencies. It works too.
Having "Makefile" as prerequisite is both too much and not enough. This
lead to rebuild due to unrelated changes in the Makefile and if the
command line change (like more command line option modified in a variable
in a different Makefile or the environment) the target doesn't gets
rebuilt.
That's where $(if_changed,) helps.
> > > + $(PYTHON) $(srctree)/tools/combine_two_binaries.py \
> > > + --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 \
> > > + --output $@
I can think of one example where $(if_changed,) is going to really help,
by looking at this command line:
One does update the .c file to add a function that they like to
export, run `make`, realize they forgot to update the makefile so
update it, run `make`, it's still doesn't work...
Maybe run `make clean; make`, or something else...
So, could you use $(if_changed,) ?
Probably:
quiet_cmd_combine = GEN $@
cmd_combine = $(PYTHON) ...
$(obj)/built_in_32.S: $(obj)/built_in_32.other.bin $(obj)/built_in_32.final.bin FORCE
$(call if_changes,combine)
targets += built_in_32.S
GEN, for generate, or it could be PY instead, because python script can
be slow to compile which could explain why the build system output is
making a pause on this target (on slow machines that is). Or it could be
COMBINE, or something else, but it's not really necessary to explain,
the target name is often enough to figure out what's happening, when
needed.
> > > +
> > > +clean-files := built_in_32.*.bin built_in_32.*.map build32.*.lds
> > > diff --git a/xen/arch/x86/boot/build32.lds b/xen/arch/x86/boot/build32.lds.S
> > > similarity index 70%
> > > rename from xen/arch/x86/boot/build32.lds
> > > rename to xen/arch/x86/boot/build32.lds.S
> > > index 56edaa727b..50c167aef0 100644
> > > --- a/xen/arch/x86/boot/build32.lds
> > > +++ b/xen/arch/x86/boot/build32.lds.S
> > > @@ -15,22 +15,47 @@
> > > * 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
So I've read the python script a bit, and notice that the two values GAP
and TEXT_START are also written verbatim in it, without comment, so
looks like magic number there. Could you at least put a comment in the
python script? (Something thing that could be better were if the
Makefile itself provided those number to both the lds script and the
python script, but I don't know if it's possible.)
> > > +#endif
> > > +# define DECLARE_IMPORT(name) name = . + (__LINE__ * MULT)
> >
> > Is ^ this a stray space?
> >
>
> Yes, a single ASCII character 32. Surely we don't want tabs. Other
> parts of the file use 2 spaces indentation, others 8 spaces. Or are
> you suggesting to not indent them?
Yes, that what "stray" imply, also I mean only the last line as imply by
the use singular. Sorry, I could have try to be a bit more precise.
All the indentation within the #ifdef #endif are fine, but why is
#define still indented after the last #endif?
> >
> > Overall, it's kind of mostly style comment that I had, so feel free to
> > ignore most of them.
> >
>
> Naming can be cumbersome but usually helps readability.
Yes, also sorry for the half-baked previous review, it could have been better.
Cheers,
--
Anthony Perard | Vates XCP-ng Developer
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v4 2/6] x86/boot: create a C bundle for 32 bit boot code and use it
2024-10-15 13:51 ` Anthony PERARD
@ 2024-10-16 8:33 ` Frediano Ziglio
2024-10-16 11:25 ` Anthony PERARD
0 siblings, 1 reply; 22+ messages in thread
From: Frediano Ziglio @ 2024-10-16 8:33 UTC (permalink / raw)
To: Anthony PERARD
Cc: xen-devel, Jan Beulich, Andrew Cooper, Roger Pau Monné,
Julien Grall, Stefano Stabellini
On Tue, Oct 15, 2024 at 2:51 PM Anthony PERARD
<anthony.perard@vates.tech> wrote:
>
> On Mon, Oct 14, 2024 at 05:32:26PM +0100, Frediano Ziglio wrote:
> > On Mon, Oct 14, 2024 at 4:31 PM Anthony PERARD
> > <anthony.perard@vates.tech> wrote:
> > >
> > > On Mon, Oct 14, 2024 at 09:53:28AM +0100, Frediano Ziglio wrote:
> > > > diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile
> > > > index 1199291d2b..23ad274c89 100644
> > > > --- a/xen/arch/x86/boot/Makefile
> > > > +++ b/xen/arch/x86/boot/Makefile
> > > > @@ -1,4 +1,5 @@
> > > > obj-bin-y += head.o
> > > > +obj-bin-y += built_in_32.o
> > >
> > > I don't quite like that this new object name is "built_in_32.o",
> > > It's really closed to "built_in.*" which is used by Rules.mk to collect
> > > all objects in a subdirectory. I don't really have a better suggestion at
> > > the moment.
> > >
> >
> > It was cbundle.o before, but people preferred built_in_32.o.
> > It's a collection of object files like built_in.o so it does not seem
> > so bad to me.
> > But seen other replies, some other people prefer "bundle".
>
> Yeah, I guess it's ok (built_in_32 that is; now that I gave a better
> review of the rest of the Makefile changes).
>
Renamed to built-in-32 for CODING_STYLE
> > > > @@ -9,9 +10,6 @@ targets += $(obj32)
> > > >
> > > > obj32 := $(addprefix $(obj)/,$(obj32))
> > > >
> > > > -$(obj)/head.o: AFLAGS-y += -Wa$(comma)-I$(obj)
> > > > -$(obj)/head.o: $(obj32:.32.o=.bin)
> > > > -
> > > > CFLAGS_x86_32 := $(subst -m64,-m32 -march=i686,$(XEN_TREEWIDE_CFLAGS))
> > > > $(call cc-options-add,CFLAGS_x86_32,CC,$(EMBEDDED_EXTRA_CFLAGS))
> > > > CFLAGS_x86_32 += -Werror -fno-builtin -g0 -msoft-float -mregparm=3
> > > > @@ -25,14 +23,34 @@ $(obj32): XEN_CFLAGS := $(CFLAGS_x86_32) -fpic
> > > > $(obj)/%.32.o: $(src)/%.c FORCE
> > > > $(call if_changed_rule,cc_o_c)
> > > >
> > > > +orphan-handling-$(call ld-option,--orphan-handling=error) := --orphan-handling=error
> > > > LDFLAGS_DIRECT-$(call ld-option,--warn-rwx-segments) := --no-warn-rwx-segments
> > > > LDFLAGS_DIRECT += $(LDFLAGS_DIRECT-y)
> > > > LD32 := $(LD) $(subst x86_64,i386,$(LDFLAGS_DIRECT))
> > > >
> > > > -%.bin: %.lnk
> > > > - $(OBJCOPY) -j .text -O binary $< $@
> > > > -
> > > > -%.lnk: %.32.o $(src)/build32.lds
> > > > - $(LD32) -N -T $(filter %.lds,$^) -o $@ $<
> > > > -
> > > > -clean-files := *.lnk *.bin
> > > > +$(obj)/build32.final.lds: AFLAGS-y += -DFINAL
> > > > +$(obj)/build32.other.lds $(obj)/build32.final.lds: $(src)/build32.lds.S FORCE
> > > > + $(call if_changed_dep,cpp_lds_S)
>
> Could you add:
>
> targets += build32.final.lds build32.other.lds
>
Added
> otherwise, the if_changes macro doesn't work, and the target keeps been
> rebuilt. `make V=2` will tell you the same thing.
>
> > > > +# link all 32bit objects together
> > > > +$(obj)/built_in_32.tmp.o: $(obj32)
> > > > + $(LD32) -r -o $@ $^
> > > > +
> > > > +$(obj)/built_in_32.%.bin: $(obj)/build32.%.lds $(obj)/built_in_32.tmp.o
> > > > +## link bundle with a given layout
> > >
> > > Could you put the comment aligned with the rest of the recipe? That way
> > > it won't visually separate the rule into several pieces. You can
> > > prefix it with @ so it doesn't show in build logs:
> > >
> > > @# comments
> > >
> >
> > Yes, they look more or less the same but technically pretty different.
> > The "## XXX" is a comment for make command, the "\t@#comment" is a way
> > to tell make to not print the command before launching it so make will
> > launch a shell command "# comment".
> > Yes, indentation does not make it clear. Not that launching a shell to
> > execute a comment will take a long time. On the other hand, here that
> > comment is more for the rule than for the shell. Maybe something like
> >
> > target:
> > command \
> > # do something
> >
> > (personally I found it even more ugly)
>
> How about removing the comments? I mean the command line is
> self-explanatory here. Same thing about the objcopy comment.
> (Or a comment before the rule, when really needed)
>
Moved on top of the rule (slightly changed).
> > > > + $(LD32) $(orphan-handling-y) -N -T $< -Map $(obj)/built_in_32.$(*F).map -o $(obj)/built_in_32.$(*F).o $(obj)/built_in_32.tmp.o
> > >
> > > I think this wants to be: -T $(filter %.lds,$^) -Map $(patsubst %.bin,%.map,$@) -o $(patsubst %.bin,%.o,$@) $(filter %.o,$^)
> > >
> > > :-(, maybe that's lots of $(patsubst,), not sure which is better between
> > > $(patsubst,) and using the stem $*.
> > >
> >
> > Not strong about stem or patsubst.
>
> Actually, I found a better suggestion (after reading the previous path
> which remind me about an existing feature that I don't really use), the
> use of Substitution References
> (https://www.gnu.org/software/make/manual/make.html#Substitution-Refs)
>
> This is a shorter version than patsubst, but works very well here, the
> recipe can look like this:
> $(LD32) $(orphan-handling-y) -N -T $< -Map $(@:bin=map) -o $(@:bin=o) $(filter %.o,$^)
> $(OBJCOPY) -j .text -O binary $(@:bin=o) $@
> rm -f $(@:bin=o)
>
> (It is likely fine to keep the reference to the lds script as $<,
> instead of using $(filter,).)
>
Done
> BTW, do we need the rules that generate the input of this rules
> (built_in_32.tmp.o that is), or can this one takes all 32bit objects as
> input?
>
Better not to do it In some conditions it can generate slightly
different results (like different object alignments) making the
algorithm fail.
> > The 2 filters seem good, they suggest lds for the script and objects
> > for the input, which makes sense.
>
> > > Also, if something tries to use built_in_32.tmp.bin, we have a rule that
> > > remove it's prerequisite.
> > >
> > > BTW, everything is kind of temporary in a build system, beside the few
> > > files that we want to install on a machine, so having a target named
> > > with "*tmp*" isn't great. But having a rule that create "*tmp*" file but
> > > remove them before the end of its recipe is fine (or those *tmp* aren't
> > > use outside of this recipe).
> > >
> >
> > Mumble... yes, I think the XX.tmp.o was a temporary internal rule file.
> > So we still don't agree on one name, and now we want to find also
> > another, tricky.
> > More or less if it can help, one is a 32 bit object file that bundle
> > together multiple 32 bits object files while the other is the
> > conversion of the 32 bits bundle file to 64 bits.
> > XXX_32.o and XXX_32as64.o ??
>
> We may not need the rule for it :-).
>
Not sure if we are asking a change and how.
> > > > +## extract binaries from object
> > > > + $(OBJCOPY) -j .text -O binary $(obj)/built_in_32.$(*F).o $@
> > > > + rm -f $(obj)/built_in_32.$(*F).o
> > > > +
> > > > +# generate final object file combining and checking above binaries
> > > > +$(obj)/built_in_32.S: $(obj)/built_in_32.other.bin $(obj)/built_in_32.final.bin
> > >
> > > So, "other" isn't part of "final", I don't really know what those two
> > > things contains so naming wise I can't suggest anything useful.
>
> Instead of "other", is "control" (like in science experiment where you
> have a control group), or "offseted" (which seems to be how this second
> binary is constructed) would be better names for this *.bin? It seems
> the script take both input and play the game of the 7 differences, to
> find clues about the location of some symbols, right?.
>
I don't know the game and I think people not familiar with it won't
find new names more readable but less.
Not saying that current names are good, they just need to be located
at different addresses with some "magic" in the middle.
> > > But, is "built_in_32.S" really only depends on those two .bin? SHouldn't
> > > it get regenerated if the script changes, or the .lds that --script
> > > option seems to use? What is the "--map" option, an input or output?
> >
> > Both map file and linker script are dependency of the bin files so no problems.
> > Yes, potentially you want to generate if the python script change.
> > Potentially also if Makefile, make, ld or any other command change,
> > but that possibly is kind of "we don't usually care".
>
> Actually, for the hypervisor, we usually care about changes which could
> lead to a different output. While changes of build toolchain aren't
> really track (even if there's a patch that have a small improvement
> toward it that was never committed), many other changes are been tracked
> and act upon. The macros $(if_changes*,) from Linux's Kbuild are a great
> help.
>
> If the build system doesn't regenerate targets when a developer try to
> debug some complex problem, it isn't going to help. We can easily have a
> situation where the developer adds printf earlier and earlier and
> earlier to try to find out where Xen crash, to find out later that the
> build system was working against them by not rebuilding the file that
> should be rebuilt. So if we can avoid some frustration and time lost,
> it's better :-).
>
> > > But I guess we can ignore the ".map" file because it's part of the
> > > ".bin".
> > >
> >
> > Yes, I suppose they are, although we can make it explicit both
> > generating them and using as dependencies.
>
> Having a rule that generate several targets is complicated to write in
> GNU Make 3.80, the rule is more likely to be called several time in
> parallel. So let's ignored the .map file, as it should only be used
> along side the .bin.
>
> > > Another thing that might be useful to do, is to use the "if_changed"
> > > make macro, that way if the command line of the script changes, make
> > > can remake the output. But maybe it's a bit complicated for this recipe
> > > because it doesn't uses $< or $^.
> > >
> >
> > I usually simply add Makefile to the dependencies. It works too.
>
> Having "Makefile" as prerequisite is both too much and not enough. This
> lead to rebuild due to unrelated changes in the Makefile and if the
> command line change (like more command line option modified in a variable
> in a different Makefile or the environment) the target doesn't gets
> rebuilt.
>
> That's where $(if_changed,) helps.
>
> > > > + $(PYTHON) $(srctree)/tools/combine_two_binaries.py \
> > > > + --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 \
> > > > + --output $@
>
> I can think of one example where $(if_changed,) is going to really help,
> by looking at this command line:
> One does update the .c file to add a function that they like to
> export, run `make`, realize they forgot to update the makefile so
> update it, run `make`, it's still doesn't work...
> Maybe run `make clean; make`, or something else...
>
> So, could you use $(if_changed,) ?
> Probably:
> quiet_cmd_combine = GEN $@
> cmd_combine = $(PYTHON) ...
> $(obj)/built_in_32.S: $(obj)/built_in_32.other.bin $(obj)/built_in_32.final.bin FORCE
> $(call if_changes,combine)
> targets += built_in_32.S
>
> GEN, for generate, or it could be PY instead, because python script can
> be slow to compile which could explain why the build system output is
> making a pause on this target (on slow machines that is). Or it could be
> COMBINE, or something else, but it's not really necessary to explain,
> the target name is often enough to figure out what's happening, when
> needed.
>
It just looks more complicated to me. It happened to me, if you don't
use the exported symbol no reason to have it exported from the object,
if you use it the link will fail and it won't take long to fix it.
> > > > +
> > > > +clean-files := built_in_32.*.bin built_in_32.*.map build32.*.lds
> > > > diff --git a/xen/arch/x86/boot/build32.lds b/xen/arch/x86/boot/build32.lds.S
> > > > similarity index 70%
> > > > rename from xen/arch/x86/boot/build32.lds
> > > > rename to xen/arch/x86/boot/build32.lds.S
> > > > index 56edaa727b..50c167aef0 100644
> > > > --- a/xen/arch/x86/boot/build32.lds
> > > > +++ b/xen/arch/x86/boot/build32.lds.S
> > > > @@ -15,22 +15,47 @@
> > > > * 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
>
> So I've read the python script a bit, and notice that the two values GAP
> and TEXT_START are also written verbatim in it, without comment, so
> looks like magic number there. Could you at least put a comment in the
> python script? (Something thing that could be better were if the
> Makefile itself provided those number to both the lds script and the
> python script, but I don't know if it's possible.)
>
It can be done I suppose:
- 2 macros in Makefile
- pass values into linker scripts using variables
- 2 options for Python script
- pass values to Python script
> > > > +#endif
> > > > +# define DECLARE_IMPORT(name) name = . + (__LINE__ * MULT)
> > >
> > > Is ^ this a stray space?
> > >
> >
> > Yes, a single ASCII character 32. Surely we don't want tabs. Other
> > parts of the file use 2 spaces indentation, others 8 spaces. Or are
> > you suggesting to not indent them?
>
> Yes, that what "stray" imply, also I mean only the last line as imply by
> the use singular. Sorry, I could have try to be a bit more precise.
>
> All the indentation within the #ifdef #endif are fine, but why is
> #define still indented after the last #endif?
>
Oh.. that single space. Historic, copy&paste, there were 2 defines in
the past one for each block. Removed.
> > >
> > > Overall, it's kind of mostly style comment that I had, so feel free to
> > > ignore most of them.
> > >
> >
> > Naming can be cumbersome but usually helps readability.
>
> Yes, also sorry for the half-baked previous review, it could have been better.
>
> Cheers,
>
> --
>
> Anthony Perard | Vates XCP-ng Developer
>
> XCP-ng & Xen Orchestra - Vates solutions
>
> web: https://vates.tech
>
Frediano
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 2/6] x86/boot: create a C bundle for 32 bit boot code and use it
2024-10-16 8:33 ` Frediano Ziglio
@ 2024-10-16 11:25 ` Anthony PERARD
2024-10-16 15:05 ` Frediano Ziglio
0 siblings, 1 reply; 22+ messages in thread
From: Anthony PERARD @ 2024-10-16 11:25 UTC (permalink / raw)
To: Frediano Ziglio
Cc: xen-devel, Jan Beulich, Andrew Cooper, Roger Pau Monné,
Julien Grall, Stefano Stabellini
On Wed, Oct 16, 2024 at 09:33:32AM +0100, Frediano Ziglio wrote:
> On Tue, Oct 15, 2024 at 2:51 PM Anthony PERARD <anthony.perard@vates.tech> wrote:
> > On Mon, Oct 14, 2024 at 05:32:26PM +0100, Frediano Ziglio wrote:
> > > On Mon, Oct 14, 2024 at 4:31 PM Anthony PERARD <anthony.perard@vates.tech> wrote:
> > > > On Mon, Oct 14, 2024 at 09:53:28AM +0100, Frediano Ziglio wrote:
> > BTW, do we need the rules that generate the input of this rules
> > (built_in_32.tmp.o that is), or can this one takes all 32bit objects as
> > input?
> >
>
> Better not to do it In some conditions it can generate slightly
> different results (like different object alignments) making the
> algorithm fail.
Ok. Thanks for the explanation.
> > > > > +# generate final object file combining and checking above binaries
> > > > > +$(obj)/built_in_32.S: $(obj)/built_in_32.other.bin $(obj)/built_in_32.final.bin
> > > >
> > > > So, "other" isn't part of "final", I don't really know what those two
> > > > things contains so naming wise I can't suggest anything useful.
> >
> > Instead of "other", is "control" (like in science experiment where you
> > have a control group), or "offseted" (which seems to be how this second
> > binary is constructed) would be better names for this *.bin? It seems
> > the script take both input and play the game of the 7 differences, to
> > find clues about the location of some symbols, right?.
> >
>
> I don't know the game and I think people not familiar with it won't
> find new names more readable but less.
Sorry, the "game" as nothing to do with the name I've proposed. I was
just asking if the script take both *.bin and was looking for
differences.
(The game of 7 differences is simple: there's two similar pictures and
you just look for the 7 differences between them, that's it.)
> Not saying that current names are good, they just need to be located
> at different addresses with some "magic" in the middle.
Well to me "other" evoke a binary that contains functions that are not
in "final", but instead they both contain the sames functions with
slight variation of placement in the file (with added offset, gap), as I
understand. But if you don't like my proposal, so be it.
> > I can think of one example where $(if_changed,) is going to really help,
> > by looking at this command line:
> > One does update the .c file to add a function that they like to
> > export, run `make`, realize they forgot to update the makefile so
> > update it, run `make`, it's still doesn't work...
> > Maybe run `make clean; make`, or something else...
> >
> > So, could you use $(if_changed,) ?
> > Probably:
> > quiet_cmd_combine = GEN $@
> > cmd_combine = $(PYTHON) ...
> > $(obj)/built_in_32.S: $(obj)/built_in_32.other.bin $(obj)/built_in_32.final.bin FORCE
> > $(call if_changes,combine)
> > targets += built_in_32.S
> >
> > GEN, for generate, or it could be PY instead, because python script can
> > be slow to compile which could explain why the build system output is
> > making a pause on this target (on slow machines that is). Or it could be
> > COMBINE, or something else, but it's not really necessary to explain,
> > the target name is often enough to figure out what's happening, when
> > needed.
> >
>
> It just looks more complicated to me.
I'm sorry if writing makefile is complicated. GNU make doesn't help with
writing build system that work well, especially when doing incremental
builds. So we need to use more complicated construction, especially for
a complex project like Xen.
Cheers,
--
Anthony Perard | Vates XCP-ng Developer
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 2/6] x86/boot: create a C bundle for 32 bit boot code and use it
2024-10-16 11:25 ` Anthony PERARD
@ 2024-10-16 15:05 ` Frediano Ziglio
2024-10-17 10:58 ` Frediano Ziglio
2024-10-17 12:36 ` Anthony PERARD
0 siblings, 2 replies; 22+ messages in thread
From: Frediano Ziglio @ 2024-10-16 15:05 UTC (permalink / raw)
To: Anthony PERARD
Cc: xen-devel, Jan Beulich, Andrew Cooper, Roger Pau Monné,
Julien Grall, Stefano Stabellini
On Wed, Oct 16, 2024 at 12:25 PM Anthony PERARD
<anthony.perard@vates.tech> wrote:
>
> On Wed, Oct 16, 2024 at 09:33:32AM +0100, Frediano Ziglio wrote:
> > On Tue, Oct 15, 2024 at 2:51 PM Anthony PERARD <anthony.perard@vates.tech> wrote:
> > > On Mon, Oct 14, 2024 at 05:32:26PM +0100, Frediano Ziglio wrote:
> > > > On Mon, Oct 14, 2024 at 4:31 PM Anthony PERARD <anthony.perard@vates.tech> wrote:
> > > > > On Mon, Oct 14, 2024 at 09:53:28AM +0100, Frediano Ziglio wrote:
>
> > > BTW, do we need the rules that generate the input of this rules
> > > (built_in_32.tmp.o that is), or can this one takes all 32bit objects as
> > > input?
> > >
> >
> > Better not to do it In some conditions it can generate slightly
> > different results (like different object alignments) making the
> > algorithm fail.
>
> Ok. Thanks for the explanation.
>
> > > > > > +# generate final object file combining and checking above binaries
> > > > > > +$(obj)/built_in_32.S: $(obj)/built_in_32.other.bin $(obj)/built_in_32.final.bin
> > > > >
> > > > > So, "other" isn't part of "final", I don't really know what those two
> > > > > things contains so naming wise I can't suggest anything useful.
> > >
> > > Instead of "other", is "control" (like in science experiment where you
> > > have a control group), or "offseted" (which seems to be how this second
> > > binary is constructed) would be better names for this *.bin? It seems
> > > the script take both input and play the game of the 7 differences, to
> > > find clues about the location of some symbols, right?.
> > >
> >
> > I don't know the game and I think people not familiar with it won't
> > find new names more readable but less.
>
> Sorry, the "game" as nothing to do with the name I've proposed. I was
> just asking if the script take both *.bin and was looking for
> differences.
>
> (The game of 7 differences is simple: there's two similar pictures and
> you just look for the 7 differences between them, that's it.)
>
> > Not saying that current names are good, they just need to be located
> > at different addresses with some "magic" in the middle.
>
> Well to me "other" evoke a binary that contains functions that are not
> in "final", but instead they both contain the sames functions with
> slight variation of placement in the file (with added offset, gap), as I
> understand. But if you don't like my proposal, so be it.
>
What about "base" and "offsetted" ? I don't know why "offsetted"
sounds weird to me but I didn't find anything better. I hope some
native English speaker could come with a better proposal.
> > > I can think of one example where $(if_changed,) is going to really help,
> > > by looking at this command line:
> > > One does update the .c file to add a function that they like to
> > > export, run `make`, realize they forgot to update the makefile so
> > > update it, run `make`, it's still doesn't work...
> > > Maybe run `make clean; make`, or something else...
> > >
> > > So, could you use $(if_changed,) ?
> > > Probably:
> > > quiet_cmd_combine = GEN $@
> > > cmd_combine = $(PYTHON) ...
> > > $(obj)/built_in_32.S: $(obj)/built_in_32.other.bin $(obj)/built_in_32.final.bin FORCE
> > > $(call if_changes,combine)
> > > targets += built_in_32.S
> > >
> > > GEN, for generate, or it could be PY instead, because python script can
> > > be slow to compile which could explain why the build system output is
> > > making a pause on this target (on slow machines that is). Or it could be
> > > COMBINE, or something else, but it's not really necessary to explain,
> > > the target name is often enough to figure out what's happening, when
> > > needed.
> > >
> >
> > It just looks more complicated to me.
>
> I'm sorry if writing makefile is complicated. GNU make doesn't help with
> writing build system that work well, especially when doing incremental
> builds. So we need to use more complicated construction, especially for
> a complex project like Xen.
>
It was more a balance consideration. Considering the cases that seem
to solve (and your case did not much apply) I don't feel that worth.
Also, dependency to Makefile would solve without additional macros and
indirection. Do you mind posting a full working change?
Frediano
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 2/6] x86/boot: create a C bundle for 32 bit boot code and use it
2024-10-16 15:05 ` Frediano Ziglio
@ 2024-10-17 10:58 ` Frediano Ziglio
2024-10-17 11:01 ` Andrew Cooper
2024-10-17 12:36 ` Anthony PERARD
1 sibling, 1 reply; 22+ messages in thread
From: Frediano Ziglio @ 2024-10-17 10:58 UTC (permalink / raw)
To: Anthony PERARD
Cc: xen-devel, Jan Beulich, Andrew Cooper, Roger Pau Monné,
Julien Grall, Stefano Stabellini
On Wed, Oct 16, 2024 at 4:05 PM Frediano Ziglio
<frediano.ziglio@cloud.com> wrote:
>
> On Wed, Oct 16, 2024 at 12:25 PM Anthony PERARD
> <anthony.perard@vates.tech> wrote:
> >
> > On Wed, Oct 16, 2024 at 09:33:32AM +0100, Frediano Ziglio wrote:
> > > On Tue, Oct 15, 2024 at 2:51 PM Anthony PERARD <anthony.perard@vates.tech> wrote:
> > > > On Mon, Oct 14, 2024 at 05:32:26PM +0100, Frediano Ziglio wrote:
> > > > > On Mon, Oct 14, 2024 at 4:31 PM Anthony PERARD <anthony.perard@vates.tech> wrote:
> > > > > > On Mon, Oct 14, 2024 at 09:53:28AM +0100, Frediano Ziglio wrote:
> >
> > > > BTW, do we need the rules that generate the input of this rules
> > > > (built_in_32.tmp.o that is), or can this one takes all 32bit objects as
> > > > input?
> > > >
> > >
> > > Better not to do it In some conditions it can generate slightly
> > > different results (like different object alignments) making the
> > > algorithm fail.
> >
> > Ok. Thanks for the explanation.
> >
> > > > > > > +# generate final object file combining and checking above binaries
> > > > > > > +$(obj)/built_in_32.S: $(obj)/built_in_32.other.bin $(obj)/built_in_32.final.bin
> > > > > >
> > > > > > So, "other" isn't part of "final", I don't really know what those two
> > > > > > things contains so naming wise I can't suggest anything useful.
> > > >
> > > > Instead of "other", is "control" (like in science experiment where you
> > > > have a control group), or "offseted" (which seems to be how this second
> > > > binary is constructed) would be better names for this *.bin? It seems
> > > > the script take both input and play the game of the 7 differences, to
> > > > find clues about the location of some symbols, right?.
> > > >
> > >
> > > I don't know the game and I think people not familiar with it won't
> > > find new names more readable but less.
> >
> > Sorry, the "game" as nothing to do with the name I've proposed. I was
> > just asking if the script take both *.bin and was looking for
> > differences.
> >
> > (The game of 7 differences is simple: there's two similar pictures and
> > you just look for the 7 differences between them, that's it.)
> >
> > > Not saying that current names are good, they just need to be located
> > > at different addresses with some "magic" in the middle.
> >
> > Well to me "other" evoke a binary that contains functions that are not
> > in "final", but instead they both contain the sames functions with
> > slight variation of placement in the file (with added offset, gap), as I
> > understand. But if you don't like my proposal, so be it.
> >
>
> What about "base" and "offsetted" ? I don't know why "offsetted"
> sounds weird to me but I didn't find anything better. I hope some
> native English speaker could come with a better proposal.
>
What about "base" and "bias"/"biased" ?
Frediano
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 2/6] x86/boot: create a C bundle for 32 bit boot code and use it
2024-10-17 10:58 ` Frediano Ziglio
@ 2024-10-17 11:01 ` Andrew Cooper
0 siblings, 0 replies; 22+ messages in thread
From: Andrew Cooper @ 2024-10-17 11:01 UTC (permalink / raw)
To: Frediano Ziglio, Anthony PERARD
Cc: xen-devel, Jan Beulich, Roger Pau Monné, Julien Grall,
Stefano Stabellini
On 17/10/2024 11:58 am, Frediano Ziglio wrote:
> On Wed, Oct 16, 2024 at 4:05 PM Frediano Ziglio
> <frediano.ziglio@cloud.com> wrote:
>> On Wed, Oct 16, 2024 at 12:25 PM Anthony PERARD
>> <anthony.perard@vates.tech> wrote:
>>> On Wed, Oct 16, 2024 at 09:33:32AM +0100, Frediano Ziglio wrote:
>>>> On Tue, Oct 15, 2024 at 2:51 PM Anthony PERARD <anthony.perard@vates.tech> wrote:
>>>>> On Mon, Oct 14, 2024 at 05:32:26PM +0100, Frediano Ziglio wrote:
>>>>>> On Mon, Oct 14, 2024 at 4:31 PM Anthony PERARD <anthony.perard@vates.tech> wrote:
>>>>>>> On Mon, Oct 14, 2024 at 09:53:28AM +0100, Frediano Ziglio wrote:
>>>>>>>> +# generate final object file combining and checking above binaries
>>>>>>>> +$(obj)/built_in_32.S: $(obj)/built_in_32.other.bin $(obj)/built_in_32.final.bin
>>>>>>> So, "other" isn't part of "final", I don't really know what those two
>>>>>>> things contains so naming wise I can't suggest anything useful.
>>>>> Instead of "other", is "control" (like in science experiment where you
>>>>> have a control group), or "offseted" (which seems to be how this second
>>>>> binary is constructed) would be better names for this *.bin? It seems
>>>>> the script take both input and play the game of the 7 differences, to
>>>>> find clues about the location of some symbols, right?.
>>>>>
>>>> I don't know the game and I think people not familiar with it won't
>>>> find new names more readable but less.
>>> Sorry, the "game" as nothing to do with the name I've proposed. I was
>>> just asking if the script take both *.bin and was looking for
>>> differences.
>>>
>>> (The game of 7 differences is simple: there's two similar pictures and
>>> you just look for the 7 differences between them, that's it.)
>>>
>>>> Not saying that current names are good, they just need to be located
>>>> at different addresses with some "magic" in the middle.
>>> Well to me "other" evoke a binary that contains functions that are not
>>> in "final", but instead they both contain the sames functions with
>>> slight variation of placement in the file (with added offset, gap), as I
>>> understand. But if you don't like my proposal, so be it.
>>>
>> What about "base" and "offsetted" ? I don't know why "offsetted"
>> sounds weird to me but I didn't find anything better. I hope some
>> native English speaker could come with a better proposal.
>>
> What about "base" and "bias"/"biased" ?
A plain "offset" would be fine in this context.
"offsetted" is indeed wrong English.
~Andrew
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 2/6] x86/boot: create a C bundle for 32 bit boot code and use it
2024-10-16 15:05 ` Frediano Ziglio
2024-10-17 10:58 ` Frediano Ziglio
@ 2024-10-17 12:36 ` Anthony PERARD
1 sibling, 0 replies; 22+ messages in thread
From: Anthony PERARD @ 2024-10-17 12:36 UTC (permalink / raw)
To: Frediano Ziglio
Cc: xen-devel, Jan Beulich, Andrew Cooper, Roger Pau Monné,
Julien Grall, Stefano Stabellini
On Wed, Oct 16, 2024 at 04:05:00PM +0100, Frediano Ziglio wrote:
> On Wed, Oct 16, 2024 at 12:25 PM Anthony PERARD > <anthony.perard@vates.tech> wrote:
> > On Wed, Oct 16, 2024 at 09:33:32AM +0100, Frediano Ziglio wrote:
> > > On Tue, Oct 15, 2024 at 2:51 PM Anthony PERARD <anthony.perard@vates.tech> wrote:
> > > > I can think of one example where $(if_changed,) is going to really help,
> > > > by looking at this command line:
> > > > One does update the .c file to add a function that they like to
> > > > export, run `make`, realize they forgot to update the makefile so
> > > > update it, run `make`, it's still doesn't work...
> > > > Maybe run `make clean; make`, or something else...
> > > >
> > > > So, could you use $(if_changed,) ?
> > > > Probably:
> > > > quiet_cmd_combine = GEN $@
> > > > cmd_combine = $(PYTHON) ...
> > > > $(obj)/built_in_32.S: $(obj)/built_in_32.other.bin $(obj)/built_in_32.final.bin FORCE
> > > > $(call if_changes,combine)
> > > > targets += built_in_32.S
> > > >
> > > > GEN, for generate, or it could be PY instead, because python script can
> > > > be slow to compile which could explain why the build system output is
> > > > making a pause on this target (on slow machines that is). Or it could be
> > > > COMBINE, or something else, but it's not really necessary to explain,
> > > > the target name is often enough to figure out what's happening, when
> > > > needed.
> > > >
> > >
> > > It just looks more complicated to me.
> >
> > I'm sorry if writing makefile is complicated. GNU make doesn't help with
> > writing build system that work well, especially when doing incremental
> > builds. So we need to use more complicated construction, especially for
> > a complex project like Xen.
> >
>
> It was more a balance consideration. Considering the cases that seem
> to solve (and your case did not much apply) I don't feel that worth.
> Also, dependency to Makefile would solve without additional macros and
> indirection. Do you mind posting a full working change?
Sure, here it is (I notice I've misspell the macro name in what I've written
earlier):
quiet_cmd_combine = GEN $@
cmd_combine = \
$(PYTHON) $(srctree)/tools/combine_two_binaries.py \
--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 \
--output $@
targets += built_in_32.S
$(obj)/built_in_32.S: $(obj)/built_in_32.other.bin $(obj)/built_in_32.final.bin \
$(srctree)/tools/combine_two_binaries.py FORCE
$(call if_changed,combine)
Cheers,
--
Anthony Perard | Vates XCP-ng Developer
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 2/6] x86/boot: create a C bundle for 32 bit boot code and use it
2024-10-14 8:53 ` [PATCH v4 2/6] x86/boot: create a C bundle for 32 bit boot code and use it Frediano Ziglio
2024-10-14 15:31 ` Anthony PERARD
@ 2024-10-15 13:54 ` Andrew Cooper
1 sibling, 0 replies; 22+ messages in thread
From: Andrew Cooper @ 2024-10-15 13:54 UTC (permalink / raw)
To: Frediano Ziglio, xen-devel
Cc: Jan Beulich, Roger Pau Monné, Julien Grall,
Stefano Stabellini
On 14/10/2024 9:53 am, Frediano Ziglio wrote:
> diff --git a/xen/tools/combine_two_binaries.py b/xen/tools/combine_two_binaries.py
> new file mode 100755
> index 0000000000..138c59287e
> --- /dev/null
> +++ b/xen/tools/combine_two_binaries.py
> @@ -0,0 +1,207 @@
>
> +if args.output is not None:
> + out = open(args.output, 'w')
> +print('''/*
> + * File autogenerated by combine_two_binaries.py DO NOT EDIT
> + */''', file=out)
> +print('\t' + args.section_header, file=out)
> +print('obj_start:', file=out)
obj32_start:
This symbol survives into the main Xen binary, where "obj_start" is
entirely devoid of context.
~Andrew
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v4 3/6] x86/boot: Reuse code to relocate trampoline
2024-10-14 8:53 [PATCH v4 0/6] Reuse 32 bit C code more safely Frediano Ziglio
2024-10-14 8:53 ` [PATCH v4 1/6] x86/boot: Prep work for 32bit object changes Frediano Ziglio
2024-10-14 8:53 ` [PATCH v4 2/6] x86/boot: create a C bundle for 32 bit boot code and use it Frediano Ziglio
@ 2024-10-14 8:53 ` Frediano Ziglio
2024-10-15 15:03 ` Anthony PERARD
2024-10-14 8:53 ` [PATCH v4 4/6] x86/boot: Use boot_vid_info variable directly from C code Frediano Ziglio
` (2 subsequent siblings)
5 siblings, 1 reply; 22+ messages in thread
From: Frediano Ziglio @ 2024-10-14 8:53 UTC (permalink / raw)
To: xen-devel
Cc: Frediano Ziglio, Jan Beulich, Andrew Cooper, Roger Pau Monné,
Daniel P. Smith, Marek Marczykowski-Górecki
Move code from efi-boot.h to a separate, new, reloc-trampoline.c file.
Reuse this new code, compiling it for 32bit as well, to replace assembly
code in head.S doing the same thing.
Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
Changes since v3:
- fixed a typo in comment;
- added Reviewed-by.
---
xen/arch/x86/boot/Makefile | 12 ++++++----
xen/arch/x86/boot/build32.lds.S | 5 ++++
xen/arch/x86/boot/head.S | 23 +-----------------
xen/arch/x86/boot/reloc-trampoline.c | 36 ++++++++++++++++++++++++++++
xen/arch/x86/efi/efi-boot.h | 15 ++----------
5 files changed, 52 insertions(+), 39 deletions(-)
create mode 100644 xen/arch/x86/boot/reloc-trampoline.c
diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile
index 23ad274c89..ca258a9729 100644
--- a/xen/arch/x86/boot/Makefile
+++ b/xen/arch/x86/boot/Makefile
@@ -1,12 +1,16 @@
obj-bin-y += head.o
obj-bin-y += built_in_32.o
+obj-bin-y += $(obj64)
obj32 := cmdline.32.o
obj32 += reloc.32.o
+obj32 += reloc-trampoline.32.o
-nocov-y += $(obj32)
-noubsan-y += $(obj32)
-targets += $(obj32)
+obj64 := reloc-trampoline.o
+
+nocov-y += $(obj32) $(obj64)
+noubsan-y += $(obj32) $(obj64)
+targets += $(obj32) $(obj64)
obj32 := $(addprefix $(obj)/,$(obj32))
@@ -50,7 +54,7 @@ $(obj)/built_in_32.S: $(obj)/built_in_32.other.bin $(obj)/built_in_32.final.bin
--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 \
+ --exports cmdline_parse_early,reloc,reloc_trampoline32 \
--output $@
clean-files := built_in_32.*.bin built_in_32.*.map build32.*.lds
diff --git a/xen/arch/x86/boot/build32.lds.S b/xen/arch/x86/boot/build32.lds.S
index 50c167aef0..1110880ad4 100644
--- a/xen/arch/x86/boot/build32.lds.S
+++ b/xen/arch/x86/boot/build32.lds.S
@@ -41,6 +41,11 @@ SECTIONS
* Potentially they should be all variables. */
DECLARE_IMPORT(__base_relocs_start);
DECLARE_IMPORT(__base_relocs_end);
+ DECLARE_IMPORT(__trampoline_rel_start);
+ DECLARE_IMPORT(__trampoline_rel_stop);
+ DECLARE_IMPORT(__trampoline_seg_start);
+ DECLARE_IMPORT(__trampoline_seg_stop);
+ DECLARE_IMPORT(trampoline_phys);
. = . + GAP;
*(.text)
*(.text.*)
diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index e0776e3896..ade2c5c43d 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -706,28 +706,7 @@ trampoline_setup:
mov %edx, sym_offs(l1_bootmap)(%esi, %ecx, 8)
/* Apply relocations to bootstrap trampoline. */
- mov sym_esi(trampoline_phys), %edx
- lea sym_esi(__trampoline_rel_start), %edi
- lea sym_esi(__trampoline_rel_stop), %ecx
-1:
- mov (%edi), %eax
- add %edx, (%edi, %eax)
- add $4,%edi
-
- cmp %ecx, %edi
- jb 1b
-
- /* Patch in the trampoline segment. */
- shr $4,%edx
- lea sym_esi(__trampoline_seg_start), %edi
- lea sym_esi(__trampoline_seg_stop), %ecx
-1:
- mov (%edi), %eax
- mov %dx, (%edi, %eax)
- add $4,%edi
-
- cmp %ecx, %edi
- jb 1b
+ call reloc_trampoline32
/* Do not parse command line on EFI platform here. */
cmpb $0, sym_esi(efi_platform)
diff --git a/xen/arch/x86/boot/reloc-trampoline.c b/xen/arch/x86/boot/reloc-trampoline.c
new file mode 100644
index 0000000000..0a74c1e75a
--- /dev/null
+++ b/xen/arch/x86/boot/reloc-trampoline.c
@@ -0,0 +1,36 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#include <xen/compiler.h>
+#include <xen/stdint.h>
+#include <asm/trampoline.h>
+
+extern const int32_t __trampoline_rel_start[], __trampoline_rel_stop[];
+extern const int32_t __trampoline_seg_start[], __trampoline_seg_stop[];
+
+#if defined(__i386__)
+void reloc_trampoline32(void)
+#elif defined (__x86_64__)
+void reloc_trampoline64(void)
+#else
+#error Unknown architecture
+#endif
+{
+ unsigned long phys = trampoline_phys;
+ const int32_t *trampoline_ptr;
+
+ /*
+ * Apply relocations to trampoline.
+ *
+ * This modifies the trampoline in place within Xen, so that it will
+ * operate correctly when copied into place.
+ */
+ for ( trampoline_ptr = __trampoline_rel_start;
+ trampoline_ptr < __trampoline_rel_stop;
+ ++trampoline_ptr )
+ *(uint32_t *)(*trampoline_ptr + (long)trampoline_ptr) += phys;
+
+ for ( trampoline_ptr = __trampoline_seg_start;
+ trampoline_ptr < __trampoline_seg_stop;
+ ++trampoline_ptr )
+ *(uint16_t *)(*trampoline_ptr + (long)trampoline_ptr) = phys >> 4;
+}
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 94f3443364..1acceec471 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -103,27 +103,16 @@ static void __init efi_arch_relocate_image(unsigned long delta)
}
}
-extern const int32_t __trampoline_rel_start[], __trampoline_rel_stop[];
-extern const int32_t __trampoline_seg_start[], __trampoline_seg_stop[];
+void reloc_trampoline64(void);
static void __init relocate_trampoline(unsigned long phys)
{
- const int32_t *trampoline_ptr;
-
trampoline_phys = phys;
if ( !efi_enabled(EFI_LOADER) )
return;
- /* Apply relocations to trampoline. */
- for ( trampoline_ptr = __trampoline_rel_start;
- trampoline_ptr < __trampoline_rel_stop;
- ++trampoline_ptr )
- *(u32 *)(*trampoline_ptr + (long)trampoline_ptr) += phys;
- for ( trampoline_ptr = __trampoline_seg_start;
- trampoline_ptr < __trampoline_seg_stop;
- ++trampoline_ptr )
- *(u16 *)(*trampoline_ptr + (long)trampoline_ptr) = phys >> 4;
+ reloc_trampoline64();
}
static void __init place_string(u32 *addr, const char *s)
--
2.34.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v4 3/6] x86/boot: Reuse code to relocate trampoline
2024-10-14 8:53 ` [PATCH v4 3/6] x86/boot: Reuse code to relocate trampoline Frediano Ziglio
@ 2024-10-15 15:03 ` Anthony PERARD
0 siblings, 0 replies; 22+ messages in thread
From: Anthony PERARD @ 2024-10-15 15:03 UTC (permalink / raw)
To: Frediano Ziglio
Cc: xen-devel, Jan Beulich, Andrew Cooper, Roger Pau Monné,
Daniel P. Smith, Marek Marczykowski-Górecki
On Mon, Oct 14, 2024 at 09:53:29AM +0100, Frediano Ziglio wrote:
> diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile
> index 23ad274c89..ca258a9729 100644
> --- a/xen/arch/x86/boot/Makefile
> +++ b/xen/arch/x86/boot/Makefile
> @@ -1,12 +1,16 @@
> obj-bin-y += head.o
> obj-bin-y += built_in_32.o
> +obj-bin-y += $(obj64)
Could you move this so $(obj64) is fully set when added to $(obj-bin-y)?
There's no garanties that this is going to keep working. We already have
"obj-y :=", so it is possible we will have "obj-bin-y :=" one day. (Or
that we get rid of $(obj-bin-y) because it becomes unnecessary.)
> obj32 := cmdline.32.o
> obj32 += reloc.32.o
> +obj32 += reloc-trampoline.32.o
>
> -nocov-y += $(obj32)
> -noubsan-y += $(obj32)
> -targets += $(obj32)
> +obj64 := reloc-trampoline.o
> +
> +nocov-y += $(obj32) $(obj64)
> +noubsan-y += $(obj32) $(obj64)
> +targets += $(obj32) $(obj64)
Technically, the change to $(targets) isn't necessary, $(obj-bin-y)
should already be added to it.
Cheers,
--
Anthony Perard | Vates XCP-ng Developer
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v4 4/6] x86/boot: Use boot_vid_info variable directly from C code
2024-10-14 8:53 [PATCH v4 0/6] Reuse 32 bit C code more safely Frediano Ziglio
` (2 preceding siblings ...)
2024-10-14 8:53 ` [PATCH v4 3/6] x86/boot: Reuse code to relocate trampoline Frediano Ziglio
@ 2024-10-14 8:53 ` Frediano Ziglio
2024-10-14 8:53 ` [PATCH v4 5/6] x86/boot: Use trampoline_phys " Frediano Ziglio
2024-10-14 8:53 ` [PATCH v4 6/6] x86/boot: Clarify comment Frediano Ziglio
5 siblings, 0 replies; 22+ messages in thread
From: Frediano Ziglio @ 2024-10-14 8:53 UTC (permalink / raw)
To: xen-devel
Cc: Frediano Ziglio, Jan Beulich, Andrew Cooper, Roger Pau Monné
No more need to pass from assembly code.
Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
Changes since v1:
- split the 2 variable changes into 2 commits.
Changes since v2:
- revert commit order.
---
xen/arch/x86/boot/build32.lds.S | 1 +
xen/arch/x86/boot/head.S | 10 +---------
xen/arch/x86/boot/reloc.c | 13 +++++--------
3 files changed, 7 insertions(+), 17 deletions(-)
diff --git a/xen/arch/x86/boot/build32.lds.S b/xen/arch/x86/boot/build32.lds.S
index 1110880ad4..8c40758834 100644
--- a/xen/arch/x86/boot/build32.lds.S
+++ b/xen/arch/x86/boot/build32.lds.S
@@ -46,6 +46,7 @@ SECTIONS
DECLARE_IMPORT(__trampoline_seg_start);
DECLARE_IMPORT(__trampoline_seg_stop);
DECLARE_IMPORT(trampoline_phys);
+ DECLARE_IMPORT(boot_vid_info);
. = . + GAP;
*(.text)
*(.text.*)
diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index ade2c5c43d..5da7ac138f 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -514,18 +514,10 @@ trampoline_setup:
mov sym_esi(trampoline_phys), %ecx
add $TRAMPOLINE_SPACE,%ecx
-#ifdef CONFIG_VIDEO
- lea sym_esi(boot_vid_info), %edx
-#else
- xor %edx, %edx
-#endif
-
/* Save Multiboot / PVH info struct (after relocation) for later use. */
- push %edx /* Boot video info to be filled from MB2. */
mov %ebx, %edx /* Multiboot / PVH information address. */
- /* reloc(magic/eax, info/edx, trampoline/ecx, video/stk) using fastcall. */
+ /* reloc(magic/eax, info/edx, trampoline/ecx) using fastcall. */
call reloc
- add $4, %esp
#ifdef CONFIG_PVH_GUEST
cmpb $0, sym_esi(pvh_boot)
diff --git a/xen/arch/x86/boot/reloc.c b/xen/arch/x86/boot/reloc.c
index 94b078d7b1..707d9c5f15 100644
--- a/xen/arch/x86/boot/reloc.c
+++ b/xen/arch/x86/boot/reloc.c
@@ -176,7 +176,7 @@ static multiboot_info_t *mbi_reloc(uint32_t mbi_in, memctx *ctx)
return mbi_out;
}
-static multiboot_info_t *mbi2_reloc(uint32_t mbi_in, uint32_t video_out, memctx *ctx)
+static multiboot_info_t *mbi2_reloc(uint32_t mbi_in, memctx *ctx)
{
const multiboot2_fixed_t *mbi_fix = _p(mbi_in);
const multiboot2_memory_map_t *mmap_src;
@@ -185,7 +185,7 @@ static multiboot_info_t *mbi2_reloc(uint32_t mbi_in, uint32_t video_out, memctx
memory_map_t *mmap_dst;
multiboot_info_t *mbi_out;
#ifdef CONFIG_VIDEO
- struct boot_video_info *video = NULL;
+ struct boot_video_info *video = &boot_vid_info;
#endif
uint32_t ptr;
unsigned int i, mod_idx = 0;
@@ -290,12 +290,11 @@ static multiboot_info_t *mbi2_reloc(uint32_t mbi_in, uint32_t video_out, memctx
#ifdef CONFIG_VIDEO
case MULTIBOOT2_TAG_TYPE_VBE:
- if ( video_out )
+ if ( video )
{
const struct vesa_ctrl_info *ci;
const struct vesa_mode_info *mi;
- video = _p(video_out);
ci = (const void *)get_mb2_data(tag, vbe, vbe_control_info);
mi = (const void *)get_mb2_data(tag, vbe, vbe_mode_info);
@@ -321,7 +320,6 @@ static multiboot_info_t *mbi2_reloc(uint32_t mbi_in, uint32_t video_out, memctx
if ( (get_mb2_data(tag, framebuffer, framebuffer_type) !=
MULTIBOOT2_FRAMEBUFFER_TYPE_RGB) )
{
- video_out = 0;
video = NULL;
}
break;
@@ -346,8 +344,7 @@ static multiboot_info_t *mbi2_reloc(uint32_t mbi_in, uint32_t video_out, memctx
}
/* SAF-1-safe */
-void *reloc(uint32_t magic, uint32_t in, uint32_t trampoline,
- uint32_t video_info)
+void *reloc(uint32_t magic, uint32_t in, uint32_t trampoline)
{
memctx ctx = { trampoline };
@@ -357,7 +354,7 @@ void *reloc(uint32_t magic, uint32_t in, uint32_t trampoline,
return mbi_reloc(in, &ctx);
case MULTIBOOT2_BOOTLOADER_MAGIC:
- return mbi2_reloc(in, video_info, &ctx);
+ return mbi2_reloc(in, &ctx);
case XEN_HVM_START_MAGIC_VALUE:
if ( IS_ENABLED(CONFIG_PVH_GUEST) )
--
2.34.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH v4 5/6] x86/boot: Use trampoline_phys variable directly from C code
2024-10-14 8:53 [PATCH v4 0/6] Reuse 32 bit C code more safely Frediano Ziglio
` (3 preceding siblings ...)
2024-10-14 8:53 ` [PATCH v4 4/6] x86/boot: Use boot_vid_info variable directly from C code Frediano Ziglio
@ 2024-10-14 8:53 ` Frediano Ziglio
2024-10-14 8:53 ` [PATCH v4 6/6] x86/boot: Clarify comment Frediano Ziglio
5 siblings, 0 replies; 22+ messages in thread
From: Frediano Ziglio @ 2024-10-14 8:53 UTC (permalink / raw)
To: xen-devel
Cc: Frediano Ziglio, Jan Beulich, Andrew Cooper, Roger Pau Monné
No more need to pass from assembly code.
Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
Changes since v1:
- split the 2 variable changes into 2 commits.
Changes since v2:
- revert commit order;
- avoid useless casts.
---
xen/arch/x86/boot/head.S | 6 +-----
xen/arch/x86/boot/reloc.c | 8 ++++++--
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index 5da7ac138f..dcda91cfda 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -510,13 +510,9 @@ trampoline_setup:
mov %esi, sym_esi(xen_phys_start)
mov %esi, sym_esi(trampoline_xen_phys_start)
- /* Get bottom-most low-memory stack address. */
- mov sym_esi(trampoline_phys), %ecx
- add $TRAMPOLINE_SPACE,%ecx
-
/* Save Multiboot / PVH info struct (after relocation) for later use. */
mov %ebx, %edx /* Multiboot / PVH information address. */
- /* reloc(magic/eax, info/edx, trampoline/ecx) using fastcall. */
+ /* reloc(magic/eax, info/edx) using fastcall. */
call reloc
#ifdef CONFIG_PVH_GUEST
diff --git a/xen/arch/x86/boot/reloc.c b/xen/arch/x86/boot/reloc.c
index 707d9c5f15..e50e161b27 100644
--- a/xen/arch/x86/boot/reloc.c
+++ b/xen/arch/x86/boot/reloc.c
@@ -19,6 +19,9 @@
#include <xen/kconfig.h>
#include <xen/multiboot.h>
#include <xen/multiboot2.h>
+#include <xen/page-size.h>
+
+#include <asm/trampoline.h>
#include <public/arch-x86/hvm/start_info.h>
@@ -344,9 +347,10 @@ static multiboot_info_t *mbi2_reloc(uint32_t mbi_in, memctx *ctx)
}
/* SAF-1-safe */
-void *reloc(uint32_t magic, uint32_t in, uint32_t trampoline)
+void *reloc(uint32_t magic, uint32_t in)
{
- memctx ctx = { trampoline };
+ /* Get bottom-most low-memory stack address. */
+ memctx ctx = { trampoline_phys + TRAMPOLINE_SPACE };
switch ( magic )
{
--
2.34.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH v4 6/6] x86/boot: Clarify comment
2024-10-14 8:53 [PATCH v4 0/6] Reuse 32 bit C code more safely Frediano Ziglio
` (4 preceding siblings ...)
2024-10-14 8:53 ` [PATCH v4 5/6] x86/boot: Use trampoline_phys " Frediano Ziglio
@ 2024-10-14 8:53 ` Frediano Ziglio
5 siblings, 0 replies; 22+ messages in thread
From: Frediano Ziglio @ 2024-10-14 8:53 UTC (permalink / raw)
To: xen-devel
Cc: Frediano Ziglio, Jan Beulich, Andrew Cooper, Roger Pau Monné
Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
---
xen/arch/x86/boot/reloc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xen/arch/x86/boot/reloc.c b/xen/arch/x86/boot/reloc.c
index e50e161b27..e725cfb6eb 100644
--- a/xen/arch/x86/boot/reloc.c
+++ b/xen/arch/x86/boot/reloc.c
@@ -65,7 +65,7 @@ typedef struct memctx {
/*
* Simple bump allocator.
*
- * It starts from the base of the trampoline and allocates downwards.
+ * It starts on top of space reserved for the trampoline and allocates downwards.
*/
uint32_t ptr;
} memctx;
--
2.34.1
^ permalink raw reply related [flat|nested] 22+ messages in thread