Re: [Minios-devel] [PATCH 25/40] arm64: set the stack for the arm_start_thread

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi Shijie,7

On 09/11/17 03:11, Huang Shijie wrote:
> On Wed, Nov 08, 2017 at 09:48:49AM +0000, Julien Grall wrote:
> > Hi,
> >
> > On 08/11/17 06:26, Huang Shijie wrote:
> > > On Tue, Nov 07, 2017 at 10:22:02AM +0000, Julien Grall wrote:
> > > > Hi Shijie,
> > > >
> > > > On 07/11/17 08:25, Huang Shijie wrote:
> > > > > On Mon, Nov 06, 2017 at 02:39:30PM +0000, Julien Grall wrote:
> > > > > > Hi Shijie,
> > > > > >
> > > > > > On 03/11/17 03:12, Huang Shijie wrote:
> > > > > > > When a thread (which is picked by the scheduler) runs at the first time,
> > > > > > > it needs a stack to store the data.
> > > > > > >
> > > > > > > This patch set the stack for the arm_start_thread.
> > > > > > >
> > > > > > > Change-Id: I4cfa18b199cb920e9ebe19bbbe2cf508264b0ba8
> > > > > > > Jira: ENTOS-247
> > > > > > > Signed-off-by: Huang Shijie <shijie.huang@xxxxxxx>
> > > > > > > ---
> > > > > > >      arch/arm/sched.c | 6 ++++++
> > > > > > >      1 file changed, 6 insertions(+)
> > > > > > >
> > > > > > > diff --git a/arch/arm/sched.c b/arch/arm/sched.c
> > > > > > > index 448fd3e..f691d7d 100644
> > > > > > > --- a/arch/arm/sched.c
> > > > > > > +++ b/arch/arm/sched.c
> > > > > > > @@ -32,6 +32,12 @@ struct thread* arch_create_thread(char *name, void 
> > > > > > > (*function)(void *),
> > > > > > >           * for the initial switch. */
> > > > > > >          thread->sp = (unsigned long) sp - sizeof(unsigned long) * 
> > > > > > > CALLEE_SAVED_REGISTERS;
> > > > > > > +    /*
> > > > > > > +     * Please refer to the stack layout in the comment for 
> > > > > > > __arch_switch_threads.
> > > > > > > +     * We need to set the sp register for arm_start_thread when a thread runs
> > > > > > > +     * at the first time.
> > > > > > > +     */
> > > > > > > +    *(--sp) = thread->sp;
> > > > > > But this is common code between Arm64 and Arm32. How come you don't update
> > > > > > arm32?
> > > > > >
> > > > > > Why do you need to save sp on Arm64 and not Arm32?
> > > > > The arm32 also need to save sp.. this is bug for arm32..
> > > > How is it a bug given you said you were unable to test Arm32 guest? At the
> > > > moment, it looks like a bit random...
> > > You can simulate it in mind, and you will know it...
> > I am not in your mind neither have time to spend trying to figure out how
> > you think that. So how about you explain your reasoning?
> If you do not set it, you will get the wrong SP stack. and the program
> will go wildly.
That a description of the symptoms... And I already guessed that from 
you previous e-mail...
By justification, I was expecting you to describe the layout of the 
Arm32 stack and explain why you think this was not set...
Anyway, I had a look at it. On Arm32, sp and lr are read with:

        stmia   r0, {sp, lr}    @ Store current sp and ip to prev's struct 
thread

As the comment says, the sp/lr is stored in struct thread. r0 actually 
points to &thread.sp. So it is not stored on the stack and can't see how 
you say this will go wrong.
Looking at the implementation for Arm64, you never use sp/lr from the 
thread structure. But instead store sp on the stack... Can you explain 
why this choice?
Cheers,

--
Julien Grall

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/cgi-bin/mailman/listinfo/minios-devel