Skip to content

Conversation

Grazfather
Copy link
Collaborator

@Grazfather Grazfather commented Sep 24, 2025

  • Add a feature to detect infinite loop halts.
  • Initialize SP properly to RAMEND - 1 (though gcc-compiled binaries will set this for us)
  • Add a Mapper to the CPU struct. All reads and writes go through it. It will properly delegate to SRAM or IO.
  • Map SRAM to 0x100 at atmega328p. Needed for when low registers are modified via writes rather than in
  • Detect when killed via IO, and return normally.
  • Dump full state when the program halts, when trace is set.
    • Dump ram, with ASCII representation
    • Elide repeated lines of all zeroes

@Grazfather Grazfather requested a review from ikskuh September 24, 2025 08:42
@Grazfather Grazfather marked this pull request as ready for review September 26, 2025 12:53
@Grazfather Grazfather marked this pull request as draft September 26, 2025 16:03
@Grazfather Grazfather changed the base branch from aviron_fixes to main September 27, 2025 11:53
@Grazfather Grazfather marked this pull request as ready for review September 27, 2025 12:16

const addr_masked: u24 = @intCast(phdr.p_paddr & 0x007F_FFFF);
const target_addr: u24 = if (phdr.p_paddr >= 0x0080_0000)
addr_masked - sram.base
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❯ 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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@Grazfather Grazfather Oct 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

.m = "mcu",
.I = "info",
.f = "format",
.B = "break_pc",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could be lower case?

.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)",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You wanted to call this 'breakpoint'?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd go with either aviron --break-at 0x1000 or aviron --breakpoint 0x1000

.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 }),
_ => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should probably restore the panic.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should definitly crash/stop here in any case, and not silently ignoring it

Copy link
Contributor

@ikskuh ikskuh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the main pain point is that we should introduce a proper memory mapper instead of doing all of these workarounds, as it will actually simplify the code a lot

KEEP(*(.vectors))

/* Ensure _start comes first */
KEEP(*(.text._start))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is _start not exported normally for gcc? If not, what can we add here as an alternative?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +180 to +181
// Some AVR families (e.g., XMEGA) expose extended I/O up to 0xFFF (12 bits).
pub const Address = u12;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which instructions would use these? Afaik the 1024 byte I/O is only usable via the RAM interface?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not for io instructions, it's for MCUs with IO mapped and read/writable with normal load and stores.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thought so, so it's a workaround for not having the ability to map the I/O into a memory space ;)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I suppose it is not needed with the MemorySpace now

Comment on lines 214 to 215
checkExitFn: *const fn (ctx: ?*anyopaque) ?u8,
translateAddressFn: *const fn (ctx: ?*anyopaque, data_addr: u24) ?Address,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could add default implementations for these two, where both functions will return null by default

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah i would use a default function for checkExitFn and best just delete translateAddressFn, as we can solve that better with a mapper object


const addr_masked: u24 = @intCast(phdr.p_paddr & 0x007F_FFFF);
const target_addr: u24 = if (phdr.p_paddr >= 0x0080_0000)
addr_masked - sram.base
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

.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)",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd go with either aviron --break-at 0x1000 or aviron --breakpoint 0x1000

.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 }),
_ => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should definitly crash/stop here in any case, and not silently ignoring it

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.
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
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@Grazfather Grazfather force-pushed the aviron_features branch 2 times, most recently from 3c57497 to c1a82cf Compare October 5, 2025 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants