Re: [PATCH v2] avr: Fix wrong initial value of stack pointer

[Philippe Mathieu-Daudé]

Hi Gihun,

On 27/11/23 03:54, Gihun Nam wrote:
> The current implementation initializes the stack pointer of AVR devices
> to 0. Although older AVR devices used to be like that, newer ones set
> it to RAMEND.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1525
> Signed-off-by: Gihun Nam <gihun.nam@outlook.com>
> ---
> Edit code to use QOM property and add more description to commit message
> about the changes
>
> Thanks for the detailed help, Mr. Peter!
>
> P.S. I don't understand how replies work with git send-email, so
>       if I've done something wrong, please bear with me.
>
>   hw/avr/atmega.c  |  4 ++++
>   target/avr/cpu.c | 10 +++++++++-
>   target/avr/cpu.h |  3 +++
>   3 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/hw/avr/atmega.c b/hw/avr/atmega.c
> index a34803e642..31c8992d75 100644
> --- a/hw/avr/atmega.c
> +++ b/hw/avr/atmega.c
> @@ -233,6 +233,10 @@ static void atmega_realize(DeviceState *dev, Error **errp)
>   
>       /* CPU */
>       object_initialize_child(OBJECT(dev), "cpu", &s->cpu, mc->cpu_type);
> +
> +    object_property_set_uint(OBJECT(&s->cpu), "init-sp",
> +                             mc->io_size + mc->sram_size - 1, &error_abort);
Since the CPU implements the QDev interface, you can use:

       qdev_prop_set_uint32(DEVICE(&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..999c010ded 100644
> --- a/target/avr/cpu.c
> +++ b/target/avr/cpu.c
> @@ -25,6 +25,7 @@
>   #include "cpu.h"
>   #include "disas/dis-asm.h"
>   #include "tcg/debug-assert.h"
> +#include "hw/qdev-properties.h"
>   
>   static void avr_cpu_set_pc(CPUState *cs, vaddr value)
>   {
> @@ -95,7 +96,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;
>   
> @@ -152,6 +153,11 @@ static void avr_cpu_initfn(Object *obj)
>                         sizeof(cpu->env.intsrc) * 8);
>   }
>   
> +static Property avr_cpu_properties[] = {
> +    DEFINE_PROP_UINT32("init-sp", AVRCPU, init_sp, 0),
> +    DEFINE_PROP_END_OF_LIST()
> +};
> +
>   static ObjectClass *avr_cpu_class_by_name(const char *cpu_model)
>   {
>       ObjectClass *oc;
> @@ -228,6 +234,8 @@ static void avr_cpu_class_init(ObjectClass *oc, void *data)
>   
>       device_class_set_parent_realize(dc, avr_cpu_realizefn, &mcc->parent_realize);
>   
> +    device_class_set_props(dc, avr_cpu_properties);
> +
>       resettable_class_set_parent_phases(rc, NULL, avr_cpu_reset_hold, NULL,
>                                          &mcc->parent_phases);
>   
> 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;
Hmm the stack is 16-bit wide. I suppose AVRCPU::sp is 32-bit
wide because tcg_global_mem_new_i32() forces us to (the smaller
TCG register is 16-bit).

Preferably using uint16_t/DEFINE_PROP_UINT16/qdev_prop_set_uint16:

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

>   };
>   
>   /**