Aviron features #683
Aviron features #683
Conversation
88e66b0 to
1632125
Compare
5ed6544 to
41ae681
Compare
sim/aviron/src/main.zig
Outdated
|
|
||
| const addr_masked: u24 = @intCast(phdr.p_paddr & 0x007F_FFFF); | ||
| const target_addr: u24 = if (phdr.p_paddr >= 0x0080_0000) | ||
| addr_masked - sram.base |
There was a problem hiding this comment.
❯ avr-readelf -s test.elf
Symbol table '.symtab' contains 396 entries:
Num: Value Size Type Bind Vis Ndx Name
0: 00000000 0 NOTYPE LOCAL DEFAULT UND
1: 00800100 0 SECTION LOCAL DEFAULT 1 .data
.data is at 0x00800100, but it should start at 0 in the array, so we mask off the top 8, then adjust by the base (0x100)
There was a problem hiding this comment.
The logic here is flawed as two LOAD headers at 00800100 and 00800200 would both be offset, while the third LOAD header at 00800000 would not.
You have to basically go through cpu.data_write for this instead of hijacking the data in a conditional here.
With a proper mapped SRAM object instead of a single "Static" this would not be a problem.
I think we should introduce a real mapper object here already, as we're piling up workarounds on workarounds just to not have a mapper and introduce more complexity than necessary by that.
The branch would not be necessary if we could just do ram.write(addr_masked, data)
There was a problem hiding this comment.
I've written a mapper, but it only supports writing individual bytes. I could extend it to add a method to write a slice, but with the changes to this part (take a look at new commits) I think this is probably fine.
In your example case, though: How would there be a load at 0x00800000 if SRAM starts at 0x100?
There was a problem hiding this comment.
How would there be a load at 0x00800000 if SRAM starts at 0x100?
It would be a faulty object file not suitable for that system setup, but you could actually construct that and have the ELF loader initialize/perform I/O 😆
I've written a mapper, but it only supports writing individual bytes.
Can you explain that? If the mapper object is just a SRAM interface, it should be no change required on the CPU side and the code here would not change at all
There was a problem hiding this comment.
Can you explain that? If the mapper object is just a SRAM interface, it should be no change required on the CPU side and the code here would not change at all
It delegates to SRAM or IO, but the interface is to read or write single bytes, so instead of using memcpy we'd have to loop over the slice and call write once for each byte
There was a problem hiding this comment.
Just thought of another thing that we should figure out: Should we actually load SRAM at all? A 'real' binary will always have a stub that loads data from flash to sram, since that is how hardware would work. That means we don't need to check the entry point or load anything in the sram range. We just unconditionally copy everything in the flash range to flash and start at 0.
avr-gcc already makes elfs that work like this, but it seems that the zig code does not (e.g. the write-chan.zig test), but of course, we can just make sure _start is at 0 and make that function load everything.
I think that I would rather go this way since it's closer to how the hardware behaves. What do you think?
sim/aviron/src/main.zig
Outdated
| .m = "mcu", | ||
| .I = "info", | ||
| .f = "format", | ||
| .B = "break_pc", |
There was a problem hiding this comment.
could be lower case?
sim/aviron/src/main.zig
Outdated
| .mcu = "Selects the emulated MCU.", | ||
| .info = "Prints information about the given MCUs memory.", | ||
| .format = "Specify file format.", | ||
| .break_pc = "Break when PC reaches this address (hex or dec)", |
There was a problem hiding this comment.
You wanted to call this 'breakpoint'?
There was a problem hiding this comment.
I'd go with either aviron --break-at 0x1000 or aviron --breakpoint 0x1000
sim/aviron/src/main.zig
Outdated
| .sreg => write_masked(@ptrCast(io.sreg), mask, value), | ||
|
|
||
| _ => std.debug.panic("illegal i/o write to undefined register 0x{X:0>2} with value=0x{X:0>2}, mask=0x{X:0>2}", .{ addr, value, mask }), | ||
| _ => { |
There was a problem hiding this comment.
should probably restore the panic.
There was a problem hiding this comment.
We should definitly crash/stop here in any case, and not silently ignoring it
| KEEP(*(.vectors)) | ||
|
|
||
| /* Ensure _start comes first */ | ||
| KEEP(*(.text._start)) |
There was a problem hiding this comment.
This won't always hold true as .text._start will only be present with -ffunction-sections compile flag (which is on in default for Zig code, but not for C and definitly not for Asm)
There was a problem hiding this comment.
is _start not exported normally for gcc? If not, what can we add here as an alternative?
There was a problem hiding this comment.
The point is that _start is a symbol name, but .text._start is a section name only emitted when -ffunction-sections is passed for the object in question being compiled with that flag. And that object is usually crt0.o, which i have no idea if it is compiled that way
| // Some AVR families (e.g., XMEGA) expose extended I/O up to 0xFFF (12 bits). | ||
| pub const Address = u12; |
There was a problem hiding this comment.
Which instructions would use these? Afaik the 1024 byte I/O is only usable via the RAM interface?
There was a problem hiding this comment.
It's not for io instructions, it's for MCUs with IO mapped and read/writable with normal load and stores.
There was a problem hiding this comment.
Thought so, so it's a workaround for not having the ability to map the I/O into a memory space ;)
There was a problem hiding this comment.
Yeah I suppose it is not needed with the MemorySpace now
sim/aviron/src/lib/io.zig
Outdated
| checkExitFn: *const fn (ctx: ?*anyopaque) ?u8, | ||
| translateAddressFn: *const fn (ctx: ?*anyopaque, data_addr: u24) ?Address, |
There was a problem hiding this comment.
We could add default implementations for these two, where both functions will return null by default
There was a problem hiding this comment.
Added a empty() function to return an IO with dummy functions. Would you prefer that the vtable have default values so an empty version can be initialized with.{} instead? I find this more expressive but I don't mind either way.
There was a problem hiding this comment.
Yeah i would use a default function for checkExitFn and best just delete translateAddressFn, as we can solve that better with a mapper object
sim/aviron/src/main.zig
Outdated
|
|
||
| const addr_masked: u24 = @intCast(phdr.p_paddr & 0x007F_FFFF); | ||
| const target_addr: u24 = if (phdr.p_paddr >= 0x0080_0000) | ||
| addr_masked - sram.base |
There was a problem hiding this comment.
The logic here is flawed as two LOAD headers at 00800100 and 00800200 would both be offset, while the third LOAD header at 00800000 would not.
You have to basically go through cpu.data_write for this instead of hijacking the data in a conditional here.
With a proper mapped SRAM object instead of a single "Static" this would not be a problem.
I think we should introduce a real mapper object here already, as we're piling up workarounds on workarounds just to not have a mapper and introduce more complexity than necessary by that.
The branch would not be necessary if we could just do ram.write(addr_masked, data)
sim/aviron/src/main.zig
Outdated
| .mcu = "Selects the emulated MCU.", | ||
| .info = "Prints information about the given MCUs memory.", | ||
| .format = "Specify file format.", | ||
| .break_pc = "Break when PC reaches this address (hex or dec)", |
There was a problem hiding this comment.
I'd go with either aviron --break-at 0x1000 or aviron --breakpoint 0x1000
sim/aviron/src/main.zig
Outdated
| .sreg => write_masked(@ptrCast(io.sreg), mask, value), | ||
|
|
||
| _ => std.debug.panic("illegal i/o write to undefined register 0x{X:0>2} with value=0x{X:0>2}, mask=0x{X:0>2}", .{ addr, value, mask }), | ||
| _ => { |
There was a problem hiding this comment.
We should definitly crash/stop here in any case, and not silently ignoring it
41ae681 to
65393d1
Compare
5240640 to
fd54e9b
Compare
We set some pointers to 0x2** range, which is not mapped on the attiny816. This caused the tests to break once we used the new mapper.
sim/aviron/src/lib/mcu.zig
Outdated
| io_seg_buf[0] = .{ .at = 0, .size = io_size, .backend = memory.Backend.fromIO(io_mem) }; | ||
| const io_space = try memory.MemorySpace.init(alloc, io_seg_buf[0..]); | ||
|
|
||
| // Program space: byte-addressable view over Flash |
There was a problem hiding this comment.
Flash accesses are a bit uglier now since I shoe-horned them into an interface that only does read8. I could add a read16 method to the vtable, and make them error out in the ram/io implementations maybe?
38e0d4b to
0c81d83
Compare
3c57497 to
c1a82cf
Compare
c1a82cf to
0608119
Compare
RAMEND - 1(though gcc-compiled binaries will set this for us)Mapperto the CPU struct. All reads and writes go through it. It will properly delegate to SRAM or IO.in