Expand the I/O table to support the ATmega2560

[edgar-bonet]

I had simavr segfault on me by running sts 0x01ff, r0 on a simulated ATmega2560. This turned out to be _avr_set_r() calling an invalid function pointer found past the end of the array avr->io.

This pull request fixes the issue by expanding the avr->io array in order to cover the full I/O space of the ATmega2560.

[buserror]

Hmm are you sure about that? AVR use 16 bits addresses, and I ran a LOT of code on 256's without that problem showing at all. That code has been there for a LONG time I should think this would have been picked up by now

[edgar-bonet]

Hmm are you sure about that?

Absolutely. I can reliably segfault simavr with this:

echo -e "sts 0x01ff, r0\nsleep" > crash.s
avr-gcc -mmcu=atmega2560 -nostdlib crash.s -o crash.elf
./run_avr -f 1000000 -m atmega2560 crash.elf

That code has been there for a LONG time I should think this would have been picked up by now

The thing is, 0x01ff is not a valid I/O port: it's one of those “reserved” locations at the top of the I/O space. No legit AVR program will expose this bug. A buggy program, however, could poke into one of these reserved locations. Such program may crash, but it should not crash the simulator itself.

[buserror]

In that case I'd really like some sort of error message to signal the problem. I'd MUCH, MUCH prefer simavr crashing because of an out of bounds problem than simavr silently working, and then have a random crash on the hardware I can't debug because "it works in simavr"!
Ultimately, simavr is suposed to be a tool that let you develop/debug your firmware.

[edgar-bonet]

In that case I'd really like some sort of error message to signal the problem.

Indeed, it would be really nice if simavr could print a warning whenever the firmware attempts to access a reserved memory location.

I'd MUCH, MUCH prefer simavr crashing because of an out of bounds problem than simavr silently working, and then have a random crash on the hardware I can't debug because "it works in simavr"!

I wouldn't. The error message “Segmentation fault” is a very clear indication that the crash is due to a bug in the simulator, not a bug in the firmware. I lost a lot of time figuring out that the simulator bug was triggered by a firmware bug.

Besides, even if we say segfaulting is an expected behavior (in which case it should be documented), the current behavior of simavr in this respect is inconsistent:

• Simavr seems to emulate most reserved memory locations as regular R/W memory: writes are harmless, and reads return the last value written.

• A few of these locations make simavr crash.

I just checked the behavior of an actual AVR chip. It's a mega328P, as I don't have a 2560 handy. I read and wrote all the reserved locations between the last I/O register and RAMSTART: 0xc7 through 0xff. From this experiment it seems that:

• Writes are harmless and ignored.

• All reads return 0, save from 0xc7 which returns 0x20.

[gatk555]

How about:

static inline void _avr_set_ram(avr_t * avr, uint16_t addr, uint8_t v)
{
    if (addr <= avr->ioend) {
        if (addr < MAX_IOs + 31) {
            _avr_set_r(avr, addr, v);
        } else {
             AVR_LOG(avr, LOG_ERROR,
                    "%sCORE: *** %04x: Write to undefined IO address %04x.%s\n",
                    simavr_font.red, avr->pc, addr, simavr_font.normal);
            avr_core_watch_write(avr, addr, v);
         }
    } else {
        avr_core_watch_write(avr, addr, v);
    }
}

That seems the right way to me. A more complex alternative: it could get cleverer with ELF and find the highest symbol in IO space.

[edgar-bonet]

Interesting! I guess it should work, save for an off-by-one error in the second test. I believe that test should be addr < MAX_IOs + 32.

But then I am wondering... other than AVR_LOG(), what should we do with such an invalid write? Since MAX_IOs is an artifact of sim_avr that has no equivalent on the hardware, I think the effect of writing to an invalid register (one marked as “reserved” in the datasheet) should be the same whether or not it happens to be past MAX_IOs. With this code:

• if we are below MAX_IOs, this calls _avr_set_r(). On an invalid register, this function should just set avr->data[r] and call _call_sram_irqs()

• if we are above MAX_IOs, then avr_core_watch_write() will set avr->data[addr], then call _call_register_irqs() (which should be a no-op) and _call_sram_irqs().

So both cases seem to yield the same results, although it is not quite obvious.

What about having _avr_set_r() handle both cases with the same code paths? This should make the equivalence of these two cases more obvious. It should also be less extra code. Here is my attempt:

diff --git a/simavr/sim/sim_core.c b/simavr/sim/sim_core.c
index fbc6bbe..304fac6 100644
--- a/simavr/sim/sim_core.c
+++ b/simavr/sim/sim_core.c
@@ -302,11 +302,11 @@ static inline void _avr_set_r(avr_t * avr, uint16_t r, uint8_t v)
 	}
 	if (r > 31) {
 		avr_io_addr_t io = AVR_DATA_TO_IO(r);
-		if (avr->io[io].w.c) {
+		if (io < MAX_IOs && avr->io[io].w.c) {
 			avr->io[io].w.c(avr, r, v, avr->io[io].w.param);
 		} else {
 			avr->data[r] = v;
-			if (avr->io[io].irq) {
+			if (io < MAX_IOs && avr->io[io].irq) {
 				avr_raise_irq(avr->io[io].irq + AVR_IOMEM_IRQ_ALL, v);
 				for (int i = 0; i < 8; i++)
 					avr_raise_irq(avr->io[io].irq + i, (v >> i) & 1);

[gatk555]

That looks neater but has lost the message. I now have:

static inline void _avr_set_ram(avr_t * avr, uint16_t addr, uint8_t v)
{
    if (addr <= avr->ioend) {
        if (addr < MAX_IOs + 32) {
            _avr_set_r(avr, addr, v);
            return;
        } else {
            AVR_LOG(avr, LOG_ERROR,
                    "%sCORE: *** %04x: Write to undefined IO address %04x.%s\n"\,
                    simavr_font.red, avr->pc, addr, simavr_font.normal);
        }
    }
    avr_core_watch_write(avr, addr, v);
}

Corrected and a bit shorter. If you want to push yout version (with message), I can pull that, otherwise I can use the one above.

[edgar-bonet]

I think your version is nice.

Ideally, one would want to have the same message for all invalid (“reserved” per the datasheet) I/O addresses, whether they are above or below MAX_IOs. I do not see, however, any obvious way of achieving that. Until someone steps up and implements it, your version of _avr_set_ram() is perfectly fine, AFAIAC.

[gatk555]

Commited as [ 8466134].