On Mon, 27 Nov 2023 at 03:52, Gihun Nam <gihun....@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....@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
