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

[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 <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.