Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

+ Support ELF files with multiple segments that go into flash memory. #504

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

dmantione
Copy link

Hello,

I am working with AVR firmwares written in assembler that have multiple ELF sections that go to flash. These are not properly handled by SimAVR, I performed the changes in this patch to make them work. Please review and check if this is suitable for merging.

Best regards,

Daniël Mantione

  + Use p_filesz and p_memsz in the ELF header according to the ELF specification.
@gatk555
Copy link
Collaborator

gatk555 commented Sep 4, 2022

It would be interesting to know how such an ELF file is made, and why?

I see several problems with the change:

If all flash segments have non-zero p_vaddr, the flash is loaded at the wrong address. The buffer is always 0-based but will be copied to non-zero offset firmware->flashbase.

In a C program containing static variables with non-zero initialisation, there will be a RAM section and malloc_usable_size() will be called with a pointer that was not returned by the heap allocator. It will probably crash.

A crash seems likely if there are no flash segments.

After the change, multiple segments with the same memory type do not cause an error, but only multiple flash segments are handled.

The manual page for malloc_usable_size() says it is GNU-only so building on non-Linux OSs may fail. There may also be portability problems with bzero(), which is deprecated.

Since elf_handle_segment() now has no purpose, it should be removed.

I think the problems can be easily fixed by adding a new argument to elf_copy_segment() - a pointer to the relevant memory size variable. That eliminates the need for malloc_usable_size() and the size can be set correctly for all memory types. Also remove firmware->flashbase. The handling of RAM segments can be fixed by creating a fake PHT entry for them.

One disadvantage of using a 0-based flash buffer is that loading multiple ELF files will no longer work, but did anyone ever do that?

@dmantione
Copy link
Author

It took me a while to produce a betters, partially because I had to research a bit more into the ELF specifications and also the Simavr source code and also, because some other stuff did pop up that kept my spare time occupied. However, I think I now have much better patch.

I had previously noted myself the point about flashbase, but I didn't dare to remove it in order to not to be true intrusive to the source code, and tried to keep it. However, as you suggested, it has now been removed and simplifies the source code. I think removing it makes sense, because in AVR, the reset vector and other interrupt vectors resides at address zero, so in practise flashbase would already have been equal in all cases.

elf_handle_segment() has also been removed, indeed, it is obsolete and I kept it previously to minimize the amount of code modified.

I have added the buffer size parameter to elf_copy_segment as you did suggest and then malloc_usable_size() is no longer necessary. I did rename the "dest" parameter to "buffer" to make more clear that it should point to an allocated buffer and cannot point anywhere in memory.

Regarding the special handling of the initialized data: This is actually not needed. The GNU linker makes a distinction between the load address (LMA) and runtime address (VMA) of a section. The LMA is store in paddr in the elf program header, while the VMA is stored in vaddr in the program header. The code was previously checking the runtime address (VMA) of the initialized data, and indeed this is moved to SRAM by the C runtime, indicated in the elf file by using a base address of $810000. However, the load address is (of course) still in flash, indicated by paddr. So by changing the code to look at the load addresses (we are loading an elf file after all), initialized data becomes a normal flash segment and doesn't need any special code to handle it anymore.

Bzero has been replacement by memset.

I'd say the source code is now both more elegant as well as more ELF compliant. I have tested it with both C and assembler firmwares and it does the right thing for me.

As to your question what I am doing and how I am generating ELF files: The SwinSID is a popular replacement for the SID chip in the Commodore 64 and is based on an Atmega88. The author of the SwinSID has unfortunately disappeared and the project is abandonned. I have adopted the project and reconstructed the source code:

https://github.com/dmantione/swinsid

Timing requirements in a parallel computer bus are tight, the SwinSID needs to respond extremely fast to chip-select requests. It makes use of global registers and data that is placed at strategic places in SRAM and flash in order to speed up the code.

Allthough you could do something with .org to get data placed at the right place in memory, a much better and flexible way is to use sections and have the linker sort it out. Therefore the assembler has to put everything in the right section, and then the linker script decides the load and run addresses of each section, so, a linker is used to create the executable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants