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

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

Posted by Gihun Nam 1 year ago

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);
+
     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;
 };
 
 /**
-- 
2.39.2

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

Posted by Philippe Mathieu-Daudé 1 year ago

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>

>   };
>   
>   /**

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

Posted by Philippe Mathieu-Daudé 12 months ago

On 27/11/23 20:22, Philippe Mathieu-Daudé wrote:
> 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/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>

Since this is a fix, I'll queue the patch as it is. We can reduce
the property to 16-bit later, if we find it helpful.

Thanks!

Phil.