Re: [ECOS] HAL_THREAD_INIT_CONTEXT

[Sergei Organov <osv at javad dot ru>]

Nick Garnett <nickg@cygnus.co.uk> writes:
> Sergei Organov <osv@javad.ru> writes:
>
> > And as far as I remember, TMS320C3/4x also have stack that grows upward, so
> > there are at least 2 such processors that are supported by GCC. The problem
> > with such processors could be solved by allowing HAL to define STACK_GROWS_UP
> > macro that will be used then by the kernel to decide which end of the stack
> > area to pass to the HAL as the stack base though.
>
> Indeed, there are several areas where this would be needed, the
> current code cannot cope as it is.
>
> One of the reasons for only passing a stack pointer to
> HAL_THREAD_INIT_CONTEXT() was to insulate it from stack direction
> considerations. This also has unpleasant complications with
> Cyg_HardwareThread::increment_stack_limit(), since the value passed to
> the HAL may not remain valid, or the HAL would have to update
> Cyg_HardwareThread::stack_limit which would be difficult to
> implement cleanly without breaking the abstraction layers.

IMHO, how stack is organized and used is entirely HAL's responsibility. For me
it seems to be possible to define HAL interface so that kernel will be
insulated from stack direction considerations, but not possible to insulate
HAL from these considerations. Besides the sole purpose of HAL is to isolate
kernel from hardware details, and I believe stack direction is one of these
hardware specific details. So I think it's better to don't use statements
like, e.g, stack_limit+=size in the kernel, but to call the HAL in such places
instead.

I also don't see how updating of stack_limit by the HAL breaks the abstraction
layers. For me it seems to be similar to the HAL_THREAD_INIT_CONTEXT
and HAL_THREAD_SWITCH_CONTEXT both changing stack pointer passed to them by
the kernel. Does it mean that abstraction layers are already broken by these
two macros? Or am I missing something?

BTW, could you please explain what is the purpose of having
Cyg_HardwareThread::increment_stack_limit() at all? I didn't find any
references to this routine in the sources, just an implementation. Anyway
it seems that the name of the routine doesn't match reality for architectures
where stack grows up.

>
> >
> > As for FP support, while the issue with the interface is not resolved, I've
> > decided to allocate space for FP context on the bottom of stack. For PPC it
> > means all threads (even those that never use FP) need to have additional ~300
> > bytes of stack. I'd like to have some graceful solution to the problem in the
> > future though.
> >
>
> The approach I intended to take was to "statically" allocate the first
> FP save area during HAL_THREAD_INIT_CONTEXT() and then dynamically
> choose to allocate further FP save areas on the stack, below the
> standard save area, depending on whether the current thread was using
> the FPU and whether its previous FP save area was already in
> use. Such an allocation would be cheap to do, since it is just an
> extra decrement of the SP, and could be done at the same time as
> disabling the FPU for use detection. If the thread retains FPU
> ownership then this space is unused, but that is benign, since it must
> have enough space on the stack to allocate it anyway. This approach
> also allows us to use FP operations in interrupts, DSR and exceptions
> more easily.

Please explain in more details what does it mean "statically"?
Is it one global "static" area, or per-thread "static" area? Is it to be
allocated on the thread stack? On which end of the stack?

>
> However, see the end of this message for what I believe is a better
> approach to allocating the initial save area.
>
>
> > My suggestion is to add 'stack_size' parameter to the HAL_THREAD_INIT_CONTEXT
> > as soon as possible.
>
> I would be happier with parameters giving the stack_base and
> stack_top, since it is then up to the HAL which one is used to build
> the initial context. Giving stack_ptr and stack_size continues to
> introduce complication with stack direction.

Yes, it's better. Or it could be stack_base and stack_size, where stack_base
is always low address of the area. But this means more changes both in HALs
and in the kernel will be required. Maybe it's worth to do though.

>
>
> > The later it will be done the more HALs will require to
> > be changed. Two good things about such a change are that it doesn't break HALs
> > silently and changing a HAL to the new interface is trivial. Anyway, it should
> > bring less troubles to HAL developers than, for example, switch to the CDL
> > based configuration brought, I believe.
> >
>
> It is indeed a simple change, but we can only do it at a well defined
> release point. I do not think it would be a good idea to change it
> now in anoncvs when we have just made a release. It is something that
> will need to be saved for the next proper release.

I didn't insist on making the changes immediately.

>
> However, I am still not convinced that we actually need to make this
> change, and have become even less convinced as I have been writing
> this message.
>
> I now think that the best approach to this is for the kernel to export
> a C wrapper function: cyg_thread_increment_stack_limit() to the HAL
> and for the HAL to call this when it needs to allocate the FP save
> area. This makes the HAL->Kernel interface clean and well defined and
> would be similar to existing functions exported from the kernel to the
> HAL like cyg_interrupt_post_dsr() and interrupt_end().

This may bring other complications though. HAL will need to allocate the FP
area when it detects usage of FP by the thread. It means the routine should be
called from the "fp unavailable" exception handler. Then it will be required
to save all the registers that could be clobbered by the C routine and
establish stack frame. Also, the routine should better claim to don't use FP.

This is apparently one possible approach, but letting HAL to handle all the
stack details seems to be more straightforward. Well, at least at first
glance.


BR,
Sergei Organov.