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

2023-11-28 Thread Philippe Mathieu-Daudé 

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 
---
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é 


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.

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

2023-11-27 Thread 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 
---
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", >cpu, mc->cpu_type);
+
+object_property_set_uint(OBJECT(>cpu), "init-sp",
+ mc->io_size + mc->sram_size - 1, _abort);


Since the CPU implements the QDev interface, you can use:

   qdev_prop_set_uint32(DEVICE(>cpu), "init-sp",
mc->io_size + mc->sram_size - 1);


  qdev_realize(DEVICE(>cpu), NULL, _abort);
  cpudev = DEVICE(>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, >parent_realize);
  
+device_class_set_props(dc, avr_cpu_properties);

+
  resettable_class_set_parent_phases(rc, NULL, avr_cpu_reset_hold, NULL,
 >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é 


  };
  
  /**

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

2023-11-27 Thread Gihun Nam 
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 
---
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", >cpu, mc->cpu_type);
+
+object_property_set_uint(OBJECT(>cpu), "init-sp",
+ mc->io_size + mc->sram_size - 1, _abort);
+
 qdev_realize(DEVICE(>cpu), NULL, _abort);
 cpudev = DEVICE(>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, 
>parent_realize);
 
+device_class_set_props(dc, avr_cpu_properties);
+
 resettable_class_set_parent_phases(rc, NULL, avr_cpu_reset_hold, NULL,
>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