[SeaBIOS] Handling SMI problem
Hello, recently I've found the-sea-watcher project and now I'm trying to repeat it. https://scumjr.github.io/2016/01/10/from-smm-to-userland-in-a-few-bytes/ But I have a strange problem when I trigger a SMI my VM instantly reboot. I use that function to send SMI: #define PORT_SMI_CMD 0x00b2 int main(void) { if (ioperm(PORT_SMI_CMD, 1, 1) != 0) err(EXIT_FAILURE, "ioperm"); outb(0x61, PORT_SMI_CMD); printf("done\n"); return EXIT_SUCCESS; } I have rebuild SeaBios with CONFIG_DEBUG_LEVEL=8 and found out that SMI's reboot VM with a rather big chance. This is a part of SeaBios log: VBE current mode=3 stub vbe_104fXX:406: a=4f09 b=0001 c=0100 d= ds=0040 es= ss=0100 si= di=2000 bp= sp=1000 cs= ip=0600 f=3200 VBE mode set: c18f set VGA mode 18f//at this point Ubuntu is loaded handle_smi cmd=61 smbase=0x000a //I send first handle_smi cmd=61 smbase=0x000a //and second SMI In resume (status=0) //have reboot on third In 32bit resume Attempting a hard reboot enabling shadow ram SeaBIOS (version rel-1.13.0-15-gde88a96-dirty-20200406_111431-test-Lenovo-ideapad-320S-13IKB) BUILD: gcc: (Ubuntu 7.5.0-3ubuntu1~18.04) 7.5.0 binutils: (GNU Binutils for Ubuntu) 2.30 I've started to explore the source code, but I don't found anything that could help me. Can you give me an advice or a direction for my research? Best regards. ___ SeaBIOS mailing list -- seabios@seabios.org To unsubscribe send an email to seabios-le...@seabios.org
[SeaBIOS] Re: [PATCH 1/9] virtio-mmio: device registry
On Fri, Apr 03, 2020 at 10:31:13AM +0200, Gerd Hoffmann wrote: > Add a new virtio-mmio.c source file, providing a function > to register virtio-mmio devices. > > Signed-off-by: Gerd Hoffmann > --- > Makefile | 2 +- > src/hw/virtio-mmio.h | 6 ++ > src/hw/virtio-mmio.c | 30 ++ > 3 files changed, 37 insertions(+), 1 deletion(-) > create mode 100644 src/hw/virtio-mmio.h > create mode 100644 src/hw/virtio-mmio.c > > diff --git a/Makefile b/Makefile > index 5f7d5370198a..985ef591a13b 100644 > --- a/Makefile > +++ b/Makefile > @@ -43,7 +43,7 @@ SRC32FLAT=$(SRCBOTH) post.c e820map.c malloc.c romfile.c > x86.c optionroms.c \ > fw/coreboot.c fw/lzmadecode.c fw/multiboot.c fw/csm.c fw/biostables.c \ > fw/paravirt.c fw/shadow.c fw/pciinit.c fw/smm.c fw/smp.c fw/mtrr.c > fw/xen.c \ > fw/acpi.c fw/mptable.c fw/pirtable.c fw/smbios.c fw/romfile_loader.c \ > -hw/virtio-ring.c hw/virtio-pci.c hw/virtio-blk.c hw/virtio-scsi.c \ > +hw/virtio-ring.c hw/virtio-pci.c hw/virtio-mmio.c hw/virtio-blk.c > hw/virtio-scsi.c \ > hw/tpm_drivers.c hw/nvme.c > SRC32SEG=string.c output.c pcibios.c apm.c stacks.c hw/pci.c hw/serialio.c > DIRS=src src/hw src/fw vgasrc > diff --git a/src/hw/virtio-mmio.h b/src/hw/virtio-mmio.h > new file mode 100644 > index ..751984241f49 > --- /dev/null > +++ b/src/hw/virtio-mmio.h > @@ -0,0 +1,6 @@ > +#ifndef _VIRTIO_MMIO_H > +#define _VIRTIO_MMIO_H > + > +void virtio_mmio_register(u64 mmio); > + > +#endif /* _VIRTIO_MMIO_H */ > diff --git a/src/hw/virtio-mmio.c b/src/hw/virtio-mmio.c > new file mode 100644 > index ..6c969d787ec7 > --- /dev/null > +++ b/src/hw/virtio-mmio.c > @@ -0,0 +1,30 @@ > +#include "config.h" // CONFIG_DEBUG_LEVEL > +#include "malloc.h" // free > +#include "output.h" // dprintf > +#include "virtio-pci.h" > +#include "virtio-ring.h" > + > +/* qemu microvm supports 8 virtio-mmio devices */ > +static u64 devs[8]; It would be preferable to avoid using global variables for temporary state. Because of all the weird linker rules and segment rules, the use of global variables is conceptually harder in SeaBIOS. If I understand this patch series correctly, the ultimate result is an acpi parser that walks the dsdt and calls virtio_mmio_register(). Could that code directly launch the appropriate hardware registration directly? -Kevin > + > +void virtio_mmio_register(u64 mmio) > +{ > +int i; > + > +for (i = 0; i < ARRAY_SIZE(devs); i++) { > +if (devs[i] == mmio) { > +/* > + * This can happen in case we have multiple scsi devices > + * attached to a single virtio-scsi controller > + */ > +dprintf(3, "virtio-mmio: duplicate device at 0x%llx, > ignoring\n", mmio); > +return; > +} > +if (devs[i] == 0) { > +dprintf(3, "virtio-mmio: register device at 0x%llx\n", mmio); > +devs[i] = mmio; > +return; > +} > +} > +dprintf(1, "virtio-mmio: device list full\n"); > +} > -- > 2.18.2 > ___ > SeaBIOS mailing list -- seabios@seabios.org > To unsubscribe send an email to seabios-le...@seabios.org ___ SeaBIOS mailing list -- seabios@seabios.org To unsubscribe send an email to seabios-le...@seabios.org
[SeaBIOS] Re: [PATCH 8/9] acpi: skip kbd init if not present
On Fri, Apr 03, 2020 at 10:31:20AM +0200, Gerd Hoffmann wrote: > Don't initialize the ps/2 keyboard in case the device is not > listed in the ACPi DSDT table. > > Signed-off-by: Gerd Hoffmann > --- > src/hw/ps2port.c | 4 > 1 file changed, 4 insertions(+) > > diff --git a/src/hw/ps2port.c b/src/hw/ps2port.c > index 2c334c06b7eb..c82521b42e16 100644 > --- a/src/hw/ps2port.c > +++ b/src/hw/ps2port.c > @@ -542,6 +542,10 @@ ps2port_setup(void) > ASSERT32FLAT(); > if (! CONFIG_PS2PORT) > return; > +if (acpi_dsdt_present_eisaid(0x0303) == 0) { > +dprintf(1, "ACPI: no PS/2 keyboard present\n"); > +return; > +} Unless I'm missing something, if the dsdt parser is enabled, but the dsdt is not actually found, this would turn off the keyboard. I think it should only disable the keyboard detection if the dsdt is actually found and doesn't contain a ps2 port entry. -Kevin > dprintf(3, "init ps2port\n"); > > enable_hwirq(1, FUNC16(entry_09)); > -- > 2.18.2 > ___ > SeaBIOS mailing list -- seabios@seabios.org > To unsubscribe send an email to seabios-le...@seabios.org ___ SeaBIOS mailing list -- seabios@seabios.org To unsubscribe send an email to seabios-le...@seabios.org
[SeaBIOS] Re: [PATCH 7/9] acpi: add dsdt parser
On Fri, Apr 03, 2020 at 10:31:19AM +0200, Gerd Hoffmann wrote: > Create a list of devices found in the DSDT table. Add helper functions > to find devices, walk the list and figure device informations like mmio > ranges and irqs. > > Signed-off-by: Gerd Hoffmann > --- > src/util.h | 10 + > src/fw/biostables.c | 622 > src/post.c | 2 + > src/Kconfig | 7 + > 4 files changed, 641 insertions(+) > > diff --git a/src/util.h b/src/util.h > index 4f27fc307439..8ee0370492b8 100644 > --- a/src/util.h > +++ b/src/util.h > @@ -94,6 +94,16 @@ void display_uuid(void); > void copy_table(void *pos); > void smbios_setup(void); > > +struct acpi_device; > +void acpi_dsdt_parse(void); > +struct acpi_device *acpi_dsdt_find_string(struct acpi_device *prev, const > char *hid); > +struct acpi_device *acpi_dsdt_find_eisaid(struct acpi_device *prev, u16 > eisaid); > +char *acpi_dsdt_name(struct acpi_device *dev); > +int acpi_dsdt_present_eisaid(u16 eisaid); > +int acpi_dsdt_find_io(struct acpi_device *dev, u64 *min, u64 *max); > +int acpi_dsdt_find_mem(struct acpi_device *dev, u64 *min, u64 *max); > +int acpi_dsdt_find_irq(struct acpi_device *dev, u64 *irq); > + > // fw/coreboot.c > extern const char *CBvendor, *CBpart; > struct cbfs_file; > diff --git a/src/fw/biostables.c b/src/fw/biostables.c > index 0d4fdb9c22e8..ebe1c90fca5e 100644 > --- a/src/fw/biostables.c > +++ b/src/fw/biostables.c > @@ -17,6 +17,7 @@ > #include "std/smbios.h" // struct smbios_entry_point > #include "string.h" // memcpy > #include "util.h" // copy_table > +#include "list.h" // hlist_* > #include "x86.h" // outb > > struct pir_header *PirAddr VARFSEG; > @@ -509,3 +510,624 @@ copy_table(void *pos) > copy_acpi_rsdp(pos); > copy_smbios(pos); > } > + > +/ > + * DSDT parser > + / I think this code is sufficiently large to demand it's own C file - for example src/fw/dsdt_parser.c . > + > +struct acpi_device { > +struct hlist_node node; > +char name[16]; > +u8 *hid_aml; > +u8 *sta_aml; > +u8 *crs_data; > +int crs_size; > +}; > +static struct hlist_head acpi_devices; It would be good to add VARVERIFY32INIT to this global. > + > +static int parse_error = 0; > +static int parse_dumptree = 0; > +static char parse_name[32]; > +static struct acpi_device *parse_dev; I think it would be preferable to not use global variables for temporary state. I think the above could be moved into a new "struct dsdt_parsing_state" and passed between functions. (I suspect the "u8 *ptr" could be moved into that struct as well.) > + > +static void parse_termlist(u8 *ptr, int offset, int pkglength); I'm a little concerned about the unbounded recursion in this parsing code. The main SeaBIOS execution stack is pretty large, but nothing stops the dsdt table from doing something goofy. I think a sanity check on recursion depth may be worthwhile. > + > +static void hex(const u8 *ptr, int count, int lvl, const char *item) > +{ > +int l = 0, i; > + > +do { > +dprintf(lvl, "%s: %04x: ", item, l); > +for (i = l; i < l+16; i += 4) > +dprintf(lvl, "%02x %02x %02x %02x ", > +ptr[i+0], ptr[i+1], ptr[i+2], ptr[i+3]); > +for (i = l; i < l+16; i++) > +dprintf(lvl, "%c", (ptr[i] > 0x20 && ptr[i] < 0x80) ? ptr[i] : > '.'); > +dprintf(lvl, "\n"); > +l += 16; > +} while (l < count); > +} > + > +static u64 parse_resource_int(u8 *ptr, int count) > +{ > +u64 value = 0; > +int index = 0; > + > +for (index = 0; index < count; index++) > +value |= (u64)ptr[index] << (index * 8); > +return value; > +} > + > +static int parse_resource_bit(u8 *ptr, int count) > +{ > +int bit; > + > +for (bit = 0; bit < count*8; bit++) > +if (ptr[bit/8] & (1 << (bit%8))) > +return bit; > +return 0; > +} > + > +static int parse_resource(u8 *ptr, int length, int *type, u64 *min, u64 *max) > +{ > +int rname, rsize; > +u64 len; > + > +*type = -1; > +*min = 0; > +*max = 0; > +len = 0; > +if (!(ptr[0] & 0x80)) { > +/* small resource */ > +rname = (ptr[0] >> 3) & 0x0f; > +rsize = ptr[0] & 0x07; > +rsize++; > +switch (rname) { > +case 0x04: /* irq */ > +*min = parse_resource_bit(ptr + 1, rsize); > +*max = *min; > +*type = 3; > +break; > +case 0x0f: /* end marker */ > +return 0; > +case 0x08: /* io */ > +*min = parse_resource_int(ptr + 2, 2); > +*max = parse_resource_int(ptr + 4, 2); > +if (*min == *max) { > +*max = *min + ptr[7] - 1; > +*type = 1; > +} > +break; > +case 0x09:
[SeaBIOS] Re: [PATCH v2] Use readelf -WSrs instead of objdump -thr so that llvm-readelf can be used as a replacement
On Wed, Apr 01, 2020 at 06:12:09PM -0700, Fangrui Song wrote: > objdump -h relies heavily on BFD internals and the BFD flags are difficult to > emulate in llvm-objdump. > llvm-objdump -h has a different output (https://reviews.llvm.org/D57146) > > Switch to readelf, which is generally better than objdump when dumping ELF > section/symbol information. > > Signed-off-by: Fangrui Song At a high-level, this change makes sense to me. What Linux distros/versions have you tested this on? (Or, do you have reason to believe the readelf output has been stable for many years?) -Kevin ___ SeaBIOS mailing list -- seabios@seabios.org To unsubscribe send an email to seabios-le...@seabios.org
[SeaBIOS] Re: [PATCH 1/4] Make rom16.o linkable with lld
On Wed, Apr 01, 2020 at 10:29:12AM -0700, Fangrui Song wrote: > (1) In romlayout.S, .fixedaddr.\addr sections do have not the SHF_ALLOC flag. > It does not make sense to reference a SHF_ALLOC section from a non-SHF_ALLOC > section via R_386_PC16. > GNU ld allows it but lld will warn. Add the SHF_ALLOC flag. > > (2) lld requires output section descriptions to be sorted by address. > Just sort the addresses beforehand. This looks like it should be two separate patches. I know it's a pain to redo patches, but separating out changes helps greatly when tracking down regressions via "git bisect". > > Signed-off-by: Fangrui Song > --- > scripts/layoutrom.py | 4 > src/romlayout.S | 2 +- > 2 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/scripts/layoutrom.py b/scripts/layoutrom.py > index 6616721..4c55390 100755 > --- a/scripts/layoutrom.py > +++ b/scripts/layoutrom.py > @@ -321,6 +321,10 @@ def outXRefs(sections, useseg=0, exportsyms=[], > forcedelta=0): > > # Write LD script includes for the given sections > def outSections(sections, useseg=0): > +if useseg: > +sections.sort(key=lambda x: x.finalsegloc) > +else: > +sections.sort(key=lambda x: x.finalloc) This looks odd to me - there shouldn't be a need to change the sort order based on useseg, as finalloc should always have the same order as finalsegloc. Also, this code alters the input list which is confusing - perhaps use "sections = sorted(sections, key=...)". > out = "" > for section in sections: > loc = section.finalloc > diff --git a/src/romlayout.S b/src/romlayout.S > index c4a4635..a854783 100644 > --- a/src/romlayout.S > +++ b/src/romlayout.S > @@ -587,7 +587,7 @@ entry_18: > > // Specify a location in the fixed part of bios area. > .macro ORG addr > -.section .fixedaddr.\addr > +.section .fixedaddr.\addr,"a" > .endm > > ORG 0xe05b > -- > 2.26.0.rc2.310.g2932bb562d-goog > ___ > SeaBIOS mailing list -- seabios@seabios.org > To unsubscribe send an email to seabios-le...@seabios.org ___ SeaBIOS mailing list -- seabios@seabios.org To unsubscribe send an email to seabios-le...@seabios.org
[SeaBIOS] Re: [PATCH 2/4] Makefile: Change ET_EXEC to ET_REL so that lld can link bios.bin.elf
On Wed, Apr 01, 2020 at 10:29:13AM -0700, Fangrui Song wrote: > Accepting ET_EXEC as an input file is an extremely rare GNU ld feature > that lld does not intend to support, because this is error-prone. > > See Linux kernel commit 90ceddcb495008ac8ba7a3dce297841efcd7d584 for > another use of the dd trick. > > -- > Changes v1 -> v2 > * Add status=none to the dd command line. This suppresses dd's stderr output. > * Move dd command to a separate command > > Signed-off-by: Fangrui Song > --- > Makefile | 4 > 1 file changed, 4 insertions(+) > > diff --git a/Makefile b/Makefile > index 5f7d537..118dec0 100644 > --- a/Makefile > +++ b/Makefile > @@ -177,10 +177,14 @@ $(OUT)romlayout32seg.lds $(OUT)romlayout32flat.lds > $(OUT)code32flat.o $(OUT)code > $(OUT)rom16.o: $(OUT)code16.o $(OUT)romlayout16.lds > @echo " Linking $@" > $(Q)$(LD) -T $(OUT)romlayout16.lds $< -o $@ > + # Change e_type to ET_REL so that it can be used to link rom.o. > + # Unlike GNU ld, lld does not allow an ET_EXEC input. > + printf '\1' | dd of=$@ conv=notrunc bs=1 seek=16 status=none > > $(OUT)rom32seg.o: $(OUT)code32seg.o $(OUT)romlayout32seg.lds > @echo " Linking $@" > $(Q)$(LD) -T $(OUT)romlayout32seg.lds $< -o $@ > + printf '\1' | dd of=$@ conv=notrunc bs=1 seek=16 status=none My high-level feedback is that the above is very fragile. I'd be reluctant to adopt that hack. What is the underlying issue that needs to be addressed? -Kevin > > $(OUT)rom.o: $(OUT)rom16.strip.o $(OUT)rom32seg.strip.o $(OUT)code32flat.o > $(OUT)romlayout32flat.lds > @echo " Linking $@" > -- > 2.26.0.rc2.310.g2932bb562d-goog > ___ > SeaBIOS mailing list -- seabios@seabios.org > To unsubscribe send an email to seabios-le...@seabios.org ___ SeaBIOS mailing list -- seabios@seabios.org To unsubscribe send an email to seabios-le...@seabios.org
[SeaBIOS] Re: [PATCH 3/4] romlayout32flag.lds: Use `. +=` instead of `. =`
On Wed, Apr 01, 2020 at 10:29:14AM -0700, Fangrui Song wrote: > This improves the portability of the linker script and allows lld to link > rom.o > > Dot assignment inside an output section has an inconsistent behavior > which makes lld difficult to implement. > See https://bugs.llvm.org/show_bug.cgi?id=43083 > > Dropping `. =` turns out to be beneficial to older GNU ld as well > because we can delete an ld check detecting "cannot move location > counter backwards". > > Signed-off-by: Fangrui Song > --- > scripts/layoutrom.py | 7 ++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/scripts/layoutrom.py b/scripts/layoutrom.py > index 4c55390..94d4412 100755 > --- a/scripts/layoutrom.py > +++ b/scripts/layoutrom.py > @@ -339,14 +339,19 @@ def outRelSections(sections, startsym, useseg=0): > if section.finalloc is not None] > sections.sort(key=operator.itemgetter(0)) > out = "" > +location = "_reloc_init_end" > for addr, section in sections: > loc = section.finalloc > if useseg: > loc = section.finalsegloc > -out += ". = ( 0x%x - %s ) ;\n" % (loc, startsym) > +if location == "_reloc_init_end": > +out += ". += 0x%x - %s ;\n" % (loc, location) > +elif location < loc: > +out += ". += 0x%x ;\n" % (loc-location,) > if section.name in ('.rodata.str1.1', '.rodata'): > out += "_rodata%s = . ;\n" % (section.fileid,) > out += "*%s.*(%s)\n" % (section.fileid, section.name) > +location = loc + section.size I'm finding this code confusing. I would recommend "location" always be a string or always be an integer - mixing them makes the code hard to follow. Also, this code removes the only reference to startsym, but doesn't update the function signature. Similarly, it hardcodes "_reloc_init_end" when I think that should be passed to the function. -Kevin > return out > > # Build linker script output for a list of relocations. > -- > 2.26.0.rc2.310.g2932bb562d-goog > ___ > SeaBIOS mailing list -- seabios@seabios.org > To unsubscribe send an email to seabios-le...@seabios.org ___ SeaBIOS mailing list -- seabios@seabios.org To unsubscribe send an email to seabios-le...@seabios.org
[SeaBIOS] Re: [PATCH v2] Use readelf -WSrs instead of objdump -thr so that llvm-readelf can be used as a replacement
On 2020-04-06, Kevin O'Connor wrote: On Wed, Apr 01, 2020 at 06:12:09PM -0700, Fangrui Song wrote: objdump -h relies heavily on BFD internals and the BFD flags are difficult to emulate in llvm-objdump. llvm-objdump -h has a different output (https://reviews.llvm.org/D57146) Switch to readelf, which is generally better than objdump when dumping ELF section/symbol information. Signed-off-by: Fangrui Song At a high-level, this change makes sense to me. What Linux distros/versions have you tested this on? (Or, do you have reason to believe the readelf output has been stable for many years?) -Kevin Tested on Debian testing. readelf -S -r -s output is stable. It just prints raw ELF fields. objdump -t -h -r output is not. The output goes through various BFD abstractions. "CONTENTS, ALLOC, LOAD, READONLY, DATA" -> these are really BFD_* constants. The genuine ELF values are SHT_PROGBITS, SHF_ALLOC, non-SHF_WRITE, etc. The LMA column is not really recorded in ELF. objdump internally does some translation like sh_addr-p_vaddr+p_paddr. I made llvm-objdump's output closer to objdump as long as it is reasonable. https://github.com/llvm/llvm-project/commits/master/llvm/tools/llvm-objdump/llvm-objdump.cpp?author=maskray These BFD flags may be difficult to implement and may not provide enough value. I actually claim myself as a maintainer of llvm-{readobj,objdump,...} and have reported many binutils issues as well. llvm-readelf, on the other side, has great compatibility with GNU readelf. ___ SeaBIOS mailing list -- seabios@seabios.org To unsubscribe send an email to seabios-le...@seabios.org
[SeaBIOS] Re: [PATCH 4/4] test-build.sh: Delete unneeded LD capability test
On Wed, Apr 01, 2020 at 10:29:15AM -0700, Fangrui Song wrote: > The previous commit changed romlayout32flag.lds to use `. += ` instead > of `. =`. > We no longer need the LD capability test checking > https://sourceware.org/bugzilla/show_bug.cgi?id=12726 Thanks. It would be great to remove this test. Were you able to bring up that old version of Ubuntu and verify that it produces bootable code (or definitely errors) with the new code though? I'd like to make sure the new code doesn't result in silently broken code. -Kevin > > Signed-off-by: Fangrui Song > --- > scripts/test-build.sh | 42 +- > 1 file changed, 1 insertion(+), 41 deletions(-) > > diff --git a/scripts/test-build.sh b/scripts/test-build.sh > index 25cc2f2..8b35d6f 100755 > --- a/scripts/test-build.sh > +++ b/scripts/test-build.sh > @@ -4,50 +4,10 @@ > mkdir -p ${OUT} > TMPFILE1=${OUT}/tmp_testcompile1.c > TMPFILE1o=${OUT}/tmp_testcompile1.o > -TMPFILE1_ld=${OUT}/tmp_testcompile1.lds > TMPFILE2=${OUT}/tmp_testcompile2.c > TMPFILE2o=${OUT}/tmp_testcompile2.o > TMPFILE3o=${OUT}/tmp_testcompile3.o > > -# Test if ld's alignment handling is correct. This is a known problem > -# with the linker that ships with Ubuntu 11.04. > -cat - > $TMPFILE1 < -const char v1[] __attribute__((section(".text.v1"))) = "0123456789"; > -const char v2[] __attribute__((section(".text.v2"))) = "0123456789"; > -EOF > -cat - > $TMPFILE1_ld < -SECTIONS > -{ > - .mysection 0x88f0 : { > -. = 0x10 ; > -*(.text.v1) > -. = 0x20 ; > -*(.text.v2) > -. = 0x30 ; > - } > -} > -EOF > -$CC -O -g -c $TMPFILE1 -o $TMPFILE1o > /dev/null 2>&1 > -if [ $? -ne 0 ]; then > -echo "Unable to execute the C compiler ($CC)." >&2 > -echo "" >&2 > -echo "Please install a working compiler and retry." >&2 > -echo -1 > -exit 0 > -fi > -$LD -T $TMPFILE1_ld $TMPFILE1o -o $TMPFILE2o > /dev/null 2>&1 > -if [ $? -ne 0 ]; then > -echo "The version of LD on this system ($LD) does not properly handle" > >&2 > -echo "alignments. As a result, this project can not be built." >&2 > -echo "" >&2 > -echo "The problem may be the result of this LD bug report:" >&2 > -echo " http://sourceware.org/bugzilla/show_bug.cgi?id=12726"; >&2 > -echo "" >&2 > -echo "Please update to a working version of binutils and retry." >&2 > -echo -1 > -exit 0 > -fi > - > # Test for "-fwhole-program". Older versions of gcc (pre v4.1) don't > # support the whole-program optimization - detect that. > $CC -fwhole-program -S -o /dev/null -xc /dev/null > /dev/null 2>&1 > @@ -87,4 +47,4 @@ echo 0 > # "ebp" register is clobberred in an "asm" statement. The code has > # been modified to not clobber "ebp" - no test is available yet. > > -rm -f $TMPFILE1 $TMPFILE1o $TMPFILE1_ld $TMPFILE2 $TMPFILE2o $TMPFILE3o > +rm -f $TMPFILE1 $TMPFILE1o $TMPFILE2 $TMPFILE2o $TMPFILE3o > -- > 2.26.0.rc2.310.g2932bb562d-goog > ___ > SeaBIOS mailing list -- seabios@seabios.org > To unsubscribe send an email to seabios-le...@seabios.org ___ SeaBIOS mailing list -- seabios@seabios.org To unsubscribe send an email to seabios-le...@seabios.org
[SeaBIOS] Re: [PATCH 2/4] Makefile: Change ET_EXEC to ET_REL so that lld can link bios.bin.elf
On 2020-04-06, Kevin O'Connor wrote: On Wed, Apr 01, 2020 at 10:29:13AM -0700, Fangrui Song wrote: Accepting ET_EXEC as an input file is an extremely rare GNU ld feature that lld does not intend to support, because this is error-prone. See Linux kernel commit 90ceddcb495008ac8ba7a3dce297841efcd7d584 for another use of the dd trick. -- Changes v1 -> v2 * Add status=none to the dd command line. This suppresses dd's stderr output. * Move dd command to a separate command Signed-off-by: Fangrui Song --- Makefile | 4 1 file changed, 4 insertions(+) diff --git a/Makefile b/Makefile index 5f7d537..118dec0 100644 --- a/Makefile +++ b/Makefile @@ -177,10 +177,14 @@ $(OUT)romlayout32seg.lds $(OUT)romlayout32flat.lds $(OUT)code32flat.o $(OUT)code $(OUT)rom16.o: $(OUT)code16.o $(OUT)romlayout16.lds @echo " Linking $@" $(Q)$(LD) -T $(OUT)romlayout16.lds $< -o $@ + # Change e_type to ET_REL so that it can be used to link rom.o. + # Unlike GNU ld, lld does not allow an ET_EXEC input. + printf '\1' | dd of=$@ conv=notrunc bs=1 seek=16 status=none $(OUT)rom32seg.o: $(OUT)code32seg.o $(OUT)romlayout32seg.lds @echo " Linking $@" $(Q)$(LD) -T $(OUT)romlayout32seg.lds $< -o $@ + printf '\1' | dd of=$@ conv=notrunc bs=1 seek=16 status=none My high-level feedback is that the above is very fragile. I'd be reluctant to adopt that hack. What is the underlying issue that needs to be addressed? -Kevin lld does not take ET_EXEC as input. This is a deliberate choice. GNU gold does not accept ET_EXEC as well: % gold a gold: error: a: unsupported ELF file type 2 1 ET_REL represents object files (.o) 2 ET_EXEC represents position-dependent executables. 3 ET_DYN represents shared objects (.so) (or PIE; for linking purposes, PIE cannot be accepted) 4 ET_CORE represents core files. For linking purposes, they cannot be accepted. I don't know how GNU ld ends up accepting ET_EXEC. I am not even sure it is an intentional decision. A lot of sections will not be meaningful to the linker and accidentally mixing an ET_EXEC can likely lead to hard-to-debug linking issues. I made a similar change to Linux recently. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=90ceddcb495008ac8ba7a3dce297841efcd7d584 $(OUT)rom.o: $(OUT)rom16.strip.o $(OUT)rom32seg.strip.o $(OUT)code32flat.o $(OUT)romlayout32flat.lds @echo " Linking $@" -- 2.26.0.rc2.310.g2932bb562d-goog ___ SeaBIOS mailing list -- seabios@seabios.org To unsubscribe send an email to seabios-le...@seabios.org ___ SeaBIOS mailing list -- seabios@seabios.org To unsubscribe send an email to seabios-le...@seabios.org
[SeaBIOS] Re: [PATCH 4/4] test-build.sh: Delete unneeded LD capability test
On 2020-04-06, Kevin O'Connor wrote: On Wed, Apr 01, 2020 at 10:29:15AM -0700, Fangrui Song wrote: The previous commit changed romlayout32flag.lds to use `. += ` instead of `. =`. We no longer need the LD capability test checking https://sourceware.org/bugzilla/show_bug.cgi?id=12726 Thanks. It would be great to remove this test. Were you able to bring up that old version of Ubuntu and verify that it produces bootable code (or definitely errors) with the new code though? I'd like to make sure the new code doesn't result in silently broken code. -Kevin I cannot test it easily. Can you do that for me, please? Signed-off-by: Fangrui Song --- scripts/test-build.sh | 42 +- 1 file changed, 1 insertion(+), 41 deletions(-) diff --git a/scripts/test-build.sh b/scripts/test-build.sh index 25cc2f2..8b35d6f 100755 --- a/scripts/test-build.sh +++ b/scripts/test-build.sh @@ -4,50 +4,10 @@ mkdir -p ${OUT} TMPFILE1=${OUT}/tmp_testcompile1.c TMPFILE1o=${OUT}/tmp_testcompile1.o -TMPFILE1_ld=${OUT}/tmp_testcompile1.lds TMPFILE2=${OUT}/tmp_testcompile2.c TMPFILE2o=${OUT}/tmp_testcompile2.o TMPFILE3o=${OUT}/tmp_testcompile3.o -# Test if ld's alignment handling is correct. This is a known problem -# with the linker that ships with Ubuntu 11.04. -cat - > $TMPFILE1 < $TMPFILE1_ld < /dev/null 2>&1 -if [ $? -ne 0 ]; then -echo "Unable to execute the C compiler ($CC)." >&2 -echo "" >&2 -echo "Please install a working compiler and retry." >&2 -echo -1 -exit 0 -fi -$LD -T $TMPFILE1_ld $TMPFILE1o -o $TMPFILE2o > /dev/null 2>&1 -if [ $? -ne 0 ]; then -echo "The version of LD on this system ($LD) does not properly handle" >&2 -echo "alignments. As a result, this project can not be built." >&2 -echo "" >&2 -echo "The problem may be the result of this LD bug report:" >&2 -echo " http://sourceware.org/bugzilla/show_bug.cgi?id=12726"; >&2 -echo "" >&2 -echo "Please update to a working version of binutils and retry." >&2 -echo -1 -exit 0 -fi - # Test for "-fwhole-program". Older versions of gcc (pre v4.1) don't # support the whole-program optimization - detect that. $CC -fwhole-program -S -o /dev/null -xc /dev/null > /dev/null 2>&1 @@ -87,4 +47,4 @@ echo 0 # "ebp" register is clobberred in an "asm" statement. The code has # been modified to not clobber "ebp" - no test is available yet. -rm -f $TMPFILE1 $TMPFILE1o $TMPFILE1_ld $TMPFILE2 $TMPFILE2o $TMPFILE3o +rm -f $TMPFILE1 $TMPFILE1o $TMPFILE2 $TMPFILE2o $TMPFILE3o -- 2.26.0.rc2.310.g2932bb562d-goog ___ SeaBIOS mailing list -- seabios@seabios.org To unsubscribe send an email to seabios-le...@seabios.org ___ SeaBIOS mailing list -- seabios@seabios.org To unsubscribe send an email to seabios-le...@seabios.org
[SeaBIOS] Re: [PATCH 1/4] Make rom16.o linkable with lld
On 2020-04-06, Kevin O'Connor wrote: On Wed, Apr 01, 2020 at 10:29:12AM -0700, Fangrui Song wrote: (1) In romlayout.S, .fixedaddr.\addr sections do have not the SHF_ALLOC flag. It does not make sense to reference a SHF_ALLOC section from a non-SHF_ALLOC section via R_386_PC16. GNU ld allows it but lld will warn. Add the SHF_ALLOC flag. (2) lld requires output section descriptions to be sorted by address. Just sort the addresses beforehand. This looks like it should be two separate patches. I know it's a pain to redo patches, but separating out changes helps greatly when tracking down regressions via "git bisect". I would hope [PATCH 2/4] Makefile: Change ET_EXEC to ET_REL so that lld can link bios.bin.elf can be applied first it it is acceptable. Like I said, these 1~3 have no dependency at all. Redoing a patch series and resending as a whole may mess up the list. Signed-off-by: Fangrui Song --- scripts/layoutrom.py | 4 src/romlayout.S | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/scripts/layoutrom.py b/scripts/layoutrom.py index 6616721..4c55390 100755 --- a/scripts/layoutrom.py +++ b/scripts/layoutrom.py @@ -321,6 +321,10 @@ def outXRefs(sections, useseg=0, exportsyms=[], forcedelta=0): # Write LD script includes for the given sections def outSections(sections, useseg=0): +if useseg: +sections.sort(key=lambda x: x.finalsegloc) +else: +sections.sort(key=lambda x: x.finalloc) This looks odd to me - there shouldn't be a need to change the sort order based on useseg, as finalloc should always have the same order as finalsegloc. Also, this code alters the input list which is confusing - perhaps use "sections = sorted(sections, key=...)". Just to confirm, I should use: if not useseg: sections = sorted(sections, key=lambda x: x.finalloc) out = "" for section in sections: loc = section.finalloc diff --git a/src/romlayout.S b/src/romlayout.S index c4a4635..a854783 100644 --- a/src/romlayout.S +++ b/src/romlayout.S @@ -587,7 +587,7 @@ entry_18: // Specify a location in the fixed part of bios area. .macro ORG addr -.section .fixedaddr.\addr +.section .fixedaddr.\addr,"a" .endm ORG 0xe05b -- 2.26.0.rc2.310.g2932bb562d-goog ___ SeaBIOS mailing list -- seabios@seabios.org To unsubscribe send an email to seabios-le...@seabios.org ___ SeaBIOS mailing list -- seabios@seabios.org To unsubscribe send an email to seabios-le...@seabios.org
[SeaBIOS] Re: [PATCH 1/4] Make rom16.o linkable with lld
On Mon, Apr 06, 2020 at 05:26:35PM -0700, Fangrui Song wrote: > On 2020-04-06, Kevin O'Connor wrote: > > On Wed, Apr 01, 2020 at 10:29:12AM -0700, Fangrui Song wrote: > > > (1) In romlayout.S, .fixedaddr.\addr sections do have not the SHF_ALLOC > > > flag. > > > It does not make sense to reference a SHF_ALLOC section from a > > > non-SHF_ALLOC section via R_386_PC16. > > > GNU ld allows it but lld will warn. Add the SHF_ALLOC flag. > > > > > > (2) lld requires output section descriptions to be sorted by address. > > > Just sort the addresses beforehand. > > > > This looks like it should be two separate patches. > > > > I know it's a pain to redo patches, but separating out changes helps > > greatly when tracking down regressions via "git bisect". > > I would hope [PATCH 2/4] Makefile: Change ET_EXEC to ET_REL so that lld can > link bios.bin.elf > can be applied first it it is acceptable. > > Like I said, these 1~3 have no dependency at all. > Redoing a patch series and resending as a whole may mess up the list. > > > > > > > Signed-off-by: Fangrui Song > > > --- > > > scripts/layoutrom.py | 4 > > > src/romlayout.S | 2 +- > > > 2 files changed, 5 insertions(+), 1 deletion(-) > > > > > > diff --git a/scripts/layoutrom.py b/scripts/layoutrom.py > > > index 6616721..4c55390 100755 > > > --- a/scripts/layoutrom.py > > > +++ b/scripts/layoutrom.py > > > @@ -321,6 +321,10 @@ def outXRefs(sections, useseg=0, exportsyms=[], > > > forcedelta=0): > > > > > > # Write LD script includes for the given sections > > > def outSections(sections, useseg=0): > > > +if useseg: > > > +sections.sort(key=lambda x: x.finalsegloc) > > > +else: > > > +sections.sort(key=lambda x: x.finalloc) > > > > This looks odd to me - there shouldn't be a need to change the sort > > order based on useseg, as finalloc should always have the same order > > as finalsegloc. Also, this code alters the input list which is > > confusing - perhaps use "sections = sorted(sections, key=...)". > > Just to confirm, I should use: > > if not useseg: > sections = sorted(sections, key=lambda x: x.finalloc) I was proposing to unconditionally use: sections = sorted(sections, key=lambda x: x.finalloc) -Kevin > > > > > out = "" > > > for section in sections: > > > loc = section.finalloc > > > diff --git a/src/romlayout.S b/src/romlayout.S > > > index c4a4635..a854783 100644 > > > --- a/src/romlayout.S > > > +++ b/src/romlayout.S > > > @@ -587,7 +587,7 @@ entry_18: > > > > > > // Specify a location in the fixed part of bios area. > > > .macro ORG addr > > > -.section .fixedaddr.\addr > > > +.section .fixedaddr.\addr,"a" > > > .endm > > > > > > ORG 0xe05b > > > -- > > > 2.26.0.rc2.310.g2932bb562d-goog > > > ___ > > > SeaBIOS mailing list -- seabios@seabios.org > > > To unsubscribe send an email to seabios-le...@seabios.org ___ SeaBIOS mailing list -- seabios@seabios.org To unsubscribe send an email to seabios-le...@seabios.org
[SeaBIOS] [PATCH v3 5/5] test-build.sh: Delete unneeded LD capability test
The previous commit changed romlayout32flag.lds to use `. += ` instead of `. =`. We no longer need the LD capability test checking https://sourceware.org/bugzilla/show_bug.cgi?id=12726 Signed-off-by: Fangrui Song --- scripts/test-build.sh | 42 +- 1 file changed, 1 insertion(+), 41 deletions(-) diff --git a/scripts/test-build.sh b/scripts/test-build.sh index 25cc2f2..8b35d6f 100755 --- a/scripts/test-build.sh +++ b/scripts/test-build.sh @@ -4,50 +4,10 @@ mkdir -p ${OUT} TMPFILE1=${OUT}/tmp_testcompile1.c TMPFILE1o=${OUT}/tmp_testcompile1.o -TMPFILE1_ld=${OUT}/tmp_testcompile1.lds TMPFILE2=${OUT}/tmp_testcompile2.c TMPFILE2o=${OUT}/tmp_testcompile2.o TMPFILE3o=${OUT}/tmp_testcompile3.o -# Test if ld's alignment handling is correct. This is a known problem -# with the linker that ships with Ubuntu 11.04. -cat - > $TMPFILE1 < $TMPFILE1_ld < /dev/null 2>&1 -if [ $? -ne 0 ]; then -echo "Unable to execute the C compiler ($CC)." >&2 -echo "" >&2 -echo "Please install a working compiler and retry." >&2 -echo -1 -exit 0 -fi -$LD -T $TMPFILE1_ld $TMPFILE1o -o $TMPFILE2o > /dev/null 2>&1 -if [ $? -ne 0 ]; then -echo "The version of LD on this system ($LD) does not properly handle" >&2 -echo "alignments. As a result, this project can not be built." >&2 -echo "" >&2 -echo "The problem may be the result of this LD bug report:" >&2 -echo " http://sourceware.org/bugzilla/show_bug.cgi?id=12726"; >&2 -echo "" >&2 -echo "Please update to a working version of binutils and retry." >&2 -echo -1 -exit 0 -fi - # Test for "-fwhole-program". Older versions of gcc (pre v4.1) don't # support the whole-program optimization - detect that. $CC -fwhole-program -S -o /dev/null -xc /dev/null > /dev/null 2>&1 @@ -87,4 +47,4 @@ echo 0 # "ebp" register is clobberred in an "asm" statement. The code has # been modified to not clobber "ebp" - no test is available yet. -rm -f $TMPFILE1 $TMPFILE1o $TMPFILE1_ld $TMPFILE2 $TMPFILE2o $TMPFILE3o +rm -f $TMPFILE1 $TMPFILE1o $TMPFILE2 $TMPFILE2o $TMPFILE3o -- 2.26.0.292.g33ef6b2f38-goog ___ SeaBIOS mailing list -- seabios@seabios.org To unsubscribe send an email to seabios-le...@seabios.org
[SeaBIOS] [PATCH v3 2/5] Make rom16.o linkable with lld
lld requires output section descriptions to be sorted by address. Just sort the addresses beforehand. -- Changes v2 -> v3 * Sort sections by finalloc unconditionally Signed-off-by: Fangrui Song --- scripts/layoutrom.py | 1 + 1 file changed, 1 insertion(+) diff --git a/scripts/layoutrom.py b/scripts/layoutrom.py index 6616721..caed387 100755 --- a/scripts/layoutrom.py +++ b/scripts/layoutrom.py @@ -321,6 +321,7 @@ def outXRefs(sections, useseg=0, exportsyms=[], forcedelta=0): # Write LD script includes for the given sections def outSections(sections, useseg=0): +sections = sorted(sections, key=lambda x: x.finalloc) out = "" for section in sections: loc = section.finalloc -- 2.26.0.292.g33ef6b2f38-goog ___ SeaBIOS mailing list -- seabios@seabios.org To unsubscribe send an email to seabios-le...@seabios.org
[SeaBIOS] [PATCH v3 1/5] romlayout.S: Add missing SHF_ALLOC flag to .fixedaddr.\addr
It does not make sense to reference a SHF_ALLOC section from a non-SHF_ALLOC section via R_386_PC16. Conceptually, even if a non-SHF_ALLOC is loaded as part of the memory image, the distance between it and a SHF_ALLOC section may not be a constant, so the linker cannot reasonably resolve the relocation. GNU ld allows it but lld will warn. Add the SHF_ALLOC flag. Signed-off-by: Fangrui Song --- src/romlayout.S | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/romlayout.S b/src/romlayout.S index c4a4635..a854783 100644 --- a/src/romlayout.S +++ b/src/romlayout.S @@ -587,7 +587,7 @@ entry_18: // Specify a location in the fixed part of bios area. .macro ORG addr -.section .fixedaddr.\addr +.section .fixedaddr.\addr,"a" .endm ORG 0xe05b -- 2.26.0.292.g33ef6b2f38-goog ___ SeaBIOS mailing list -- seabios@seabios.org To unsubscribe send an email to seabios-le...@seabios.org
[SeaBIOS] [PATCH v3 3/5] Makefile: Change ET_EXEC to ET_REL so that lld can link bios.bin.elf
Accepting ET_EXEC as an input file is an extremely rare GNU ld feature that lld does not intend to support, because this is error-prone. See Linux kernel commit 90ceddcb495008ac8ba7a3dce297841efcd7d584 for another use of the dd trick. -- Changes v1 -> v2 * Add status=none to the dd command line. This suppresses dd's stderr output. * Move dd command to a separate command Signed-off-by: Fangrui Song --- Makefile | 4 1 file changed, 4 insertions(+) diff --git a/Makefile b/Makefile index 5f7d537..118dec0 100644 --- a/Makefile +++ b/Makefile @@ -177,10 +177,14 @@ $(OUT)romlayout32seg.lds $(OUT)romlayout32flat.lds $(OUT)code32flat.o $(OUT)code $(OUT)rom16.o: $(OUT)code16.o $(OUT)romlayout16.lds @echo " Linking $@" $(Q)$(LD) -T $(OUT)romlayout16.lds $< -o $@ + # Change e_type to ET_REL so that it can be used to link rom.o. + # Unlike GNU ld, lld does not allow an ET_EXEC input. + printf '\1' | dd of=$@ conv=notrunc bs=1 seek=16 status=none $(OUT)rom32seg.o: $(OUT)code32seg.o $(OUT)romlayout32seg.lds @echo " Linking $@" $(Q)$(LD) -T $(OUT)romlayout32seg.lds $< -o $@ + printf '\1' | dd of=$@ conv=notrunc bs=1 seek=16 status=none $(OUT)rom.o: $(OUT)rom16.strip.o $(OUT)rom32seg.strip.o $(OUT)code32flat.o $(OUT)romlayout32flat.lds @echo " Linking $@" -- 2.26.0.292.g33ef6b2f38-goog ___ SeaBIOS mailing list -- seabios@seabios.org To unsubscribe send an email to seabios-le...@seabios.org
[SeaBIOS] [PATCH v3 0/5] Make seabios linkable with lld
This patch series make seabios linkable with lld. This is beneficial for FreeBSD ports as well https://svnweb.freebsd.org/ports/head/misc/seabios/Makefile as they can drop an LLD_UNSAFE As a maintainer of lld ELF, I have triaged numerous pieces of software. seabios is the only one making use of This drops the only instance https://sourceware.org/bugzilla/show_bug.cgi?id=12726 1, 2, 3 and 4 are really independent and can be applied in arbitrary order. 5 depends on 4. I originally combined 4 and 5 and that was why I don't think a patch series made sense. Fangrui Song (5): romlayout.S: Add missing SHF_ALLOC flag to .fixedaddr.\addr Make rom16.o linkable with lld Makefile: Change ET_EXEC to ET_REL so that lld can link bios.bin.elf romlayout32flag.lds: Use `. +=` instead of `. =` test-build.sh: Delete unneeded LD capability test Makefile | 4 scripts/layoutrom.py | 13 ++--- scripts/test-build.sh | 42 +- src/romlayout.S | 2 +- 4 files changed, 16 insertions(+), 45 deletions(-) -- 2.26.0.292.g33ef6b2f38-goog ___ SeaBIOS mailing list -- seabios@seabios.org To unsubscribe send an email to seabios-le...@seabios.org
[SeaBIOS] [PATCH v3 4/5] romlayout32flag.lds: Use `. +=` instead of `. =`
This improves the portability of the linker script and allows lld to link rom.o Dot assignment inside an output section has an inconsistent behavior which makes lld difficult to implement. See https://bugs.llvm.org/show_bug.cgi?id=43083 Dropping `. =` turns out to be beneficial to older GNU ld as well because we can delete an ld check detecting "cannot move location counter backwards". -- Changes v2 -> v3 * Add `first` to avoid overloading `location` * Delete startsym from the parameters Signed-off-by: Fangrui Song --- scripts/layoutrom.py | 12 +--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/scripts/layoutrom.py b/scripts/layoutrom.py index caed387..caa182a 100755 --- a/scripts/layoutrom.py +++ b/scripts/layoutrom.py @@ -331,19 +331,25 @@ def outSections(sections, useseg=0): return out # Write LD script includes for the given sections using relative offsets -def outRelSections(sections, startsym, useseg=0): +def outRelSections(sections, useseg=0): sections = [(section.finalloc, section) for section in sections if section.finalloc is not None] sections.sort(key=operator.itemgetter(0)) out = "" +first = True for addr, section in sections: loc = section.finalloc if useseg: loc = section.finalsegloc -out += ". = ( 0x%x - %s ) ;\n" % (loc, startsym) +if first: +out += ". += 0x%x - _reloc_init_end ;\n" % (loc,) +first = False +elif location != loc: +out += ". += 0x%x ;\n" % (loc-location,) if section.name in ('.rodata.str1.1', '.rodata'): out += "_rodata%s = . ;\n" % (section.fileid,) out += "*%s.*(%s)\n" % (section.fileid, section.name) +location = loc + section.size return out # Build linker script output for a list of relocations. @@ -444,7 +450,7 @@ def writeLinkerScripts(li, out16, out32seg, out32flat): sec32all_start, multiboot_header, relocstr, - outRelSections(li.sections, 'code32flat_start')) + outRelSections(li.sections)) out = COMMONHEADER + out + COMMONTRAILER + """ ENTRY(%s) PHDRS -- 2.26.0.292.g33ef6b2f38-goog ___ SeaBIOS mailing list -- seabios@seabios.org To unsubscribe send an email to seabios-le...@seabios.org
[SeaBIOS] Re: [PATCH 3/4] romlayout32flag.lds: Use `. +=` instead of `. =`
On 2020-04-06, Kevin O'Connor wrote: On Wed, Apr 01, 2020 at 10:29:14AM -0700, Fangrui Song wrote: This improves the portability of the linker script and allows lld to link rom.o Dot assignment inside an output section has an inconsistent behavior which makes lld difficult to implement. See https://bugs.llvm.org/show_bug.cgi?id=43083 Dropping `. =` turns out to be beneficial to older GNU ld as well because we can delete an ld check detecting "cannot move location counter backwards". Signed-off-by: Fangrui Song --- scripts/layoutrom.py | 7 ++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/scripts/layoutrom.py b/scripts/layoutrom.py index 4c55390..94d4412 100755 --- a/scripts/layoutrom.py +++ b/scripts/layoutrom.py @@ -339,14 +339,19 @@ def outRelSections(sections, startsym, useseg=0): if section.finalloc is not None] sections.sort(key=operator.itemgetter(0)) out = "" +location = "_reloc_init_end" for addr, section in sections: loc = section.finalloc if useseg: loc = section.finalsegloc -out += ". = ( 0x%x - %s ) ;\n" % (loc, startsym) +if location == "_reloc_init_end": +out += ". += 0x%x - %s ;\n" % (loc, location) +elif location < loc: +out += ". += 0x%x ;\n" % (loc-location,) if section.name in ('.rodata.str1.1', '.rodata'): out += "_rodata%s = . ;\n" % (section.fileid,) out += "*%s.*(%s)\n" % (section.fileid, section.name) +location = loc + section.size I'm finding this code confusing. I would recommend "location" always be a string or always be an integer - mixing them makes the code hard to follow. Also, this code removes the only reference to startsym, but doesn't update the function signature. Similarly, it hardcodes "_reloc_init_end" when I think that should be passed to the function. -Kevin Thanks. Addressed in [PATCH v3 4/5] romlayout32flag.lds: Use `. +=` instead of `. =` Non-zero useseg does not appear to be used, but I don't understand it enough to fix it. return out # Build linker script output for a list of relocations. -- 2.26.0.rc2.310.g2932bb562d-goog ___ SeaBIOS mailing list -- seabios@seabios.org To unsubscribe send an email to seabios-le...@seabios.org ___ SeaBIOS mailing list -- seabios@seabios.org To unsubscribe send an email to seabios-le...@seabios.org