The current implementation initializes the stack pointer of AVR devices
to 0, but it should be set to RAMEND according to the specs.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1525
Signed-off-by: Gihun Nam <gihun.nam@outlook.com>
---
 hw/avr/atmega.c  | 3 +++
 target/avr/cpu.c | 2 +-
 target/avr/cpu.h | 3 +++
 3 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/hw/avr/atmega.c b/hw/avr/atmega.c
index a34803e642..3a8caccf99 100644
--- a/hw/avr/atmega.c
+++ b/hw/avr/atmega.c
@@ -233,6 +233,9 @@ static void atmega_realize(DeviceState *dev, Error **errp)
 
     /* CPU */
     object_initialize_child(OBJECT(dev), "cpu", &s->cpu, mc->cpu_type);
+
+    s->cpu.init_sp = mc->io_size + mc->sram_size - 1;
+
     qdev_realize(DEVICE(&s->cpu), NULL, &error_abort);
     cpudev = DEVICE(&s->cpu);
 
diff --git a/target/avr/cpu.c b/target/avr/cpu.c
index 44de1e18d1..1da7d7dbf3 100644
--- a/target/avr/cpu.c
+++ b/target/avr/cpu.c
@@ -95,7 +95,7 @@ static void avr_cpu_reset_hold(Object *obj)
     env->rampY = 0;
     env->rampZ = 0;
     env->eind = 0;
-    env->sp = 0;
+    env->sp = cpu->init_sp;
 
     env->skip = 0;
 
diff --git a/target/avr/cpu.h b/target/avr/cpu.h
index 8a17862737..7960c5c57a 100644
--- a/target/avr/cpu.h
+++ b/target/avr/cpu.h
@@ -145,6 +145,9 @@ struct ArchCPU {
     CPUState parent_obj;
 
     CPUAVRState env;
+
+    /* Initial value of stack pointer */
+    uint32_t init_sp;
 };
 
 /**
-- 
2.39.2

On Mon, 27 Nov 2023 at 03:52, Gihun Nam <gihun.nam@outlook.com> wrote:
>
> The current implementation initializes the stack pointer of AVR devices
> to 0, but it should be set to RAMEND according to the specs.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1525
> Signed-off-by: Gihun Nam <gihun.nam@outlook.com>

Hi; thanks for sending in this patch!

> ---
>  hw/avr/atmega.c  | 3 +++
>  target/avr/cpu.c | 2 +-
>  target/avr/cpu.h | 3 +++
>  3 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/hw/avr/atmega.c b/hw/avr/atmega.c
> index a34803e642..3a8caccf99 100644
> --- a/hw/avr/atmega.c
> +++ b/hw/avr/atmega.c
> @@ -233,6 +233,9 @@ static void atmega_realize(DeviceState *dev, Error **errp)
>
>      /* CPU */
>      object_initialize_child(OBJECT(dev), "cpu", &s->cpu, mc->cpu_type);
> +
> +    s->cpu.init_sp = mc->io_size + mc->sram_size - 1;

The board code should not directly touch fields inside the
CPU object. You want to set up a QOM property on the CPU
which the board code can then initialize using a QOM
property-setting function.

For an example of how to do this, have a look at how the
target/microblaze code handles its "base-vectors" property
(in fact any CPU that does a DEFINE_PROP_UINT32() will
be similar):
 * you have a Property array with a DEFINE_PROP_UINT32()
   line to define the property name, the struct field
   that it corresponds to, and the default value
   (you can list a struct field directly, don't
   put it in a substruct 'cfg' the way MicroBlaze does),
   plus a DEFINE_PROP_END_OF_LIST() terminator
 * the CPU class init function calls device_class_set_props()
   to say that that array is its properties
 * the board code sets the correct value using either
   object_property_set_uint() or qdev_prop_set_uint32()
   (doesn't matter which)

> +
>      qdev_realize(DEVICE(&s->cpu), NULL, &error_abort);
>      cpudev = DEVICE(&s->cpu);

As a side note, one of the answers to
https://stackoverflow.com/questions/46949227/compiling-an-assembly-program-using-avr
says that older AVR CPUs set the SP to 0 on reset, and
it's only newer ones that set it to RAMEND. That's
probably why we reset to 0.

thanks
-- PMM