A patch for RTEMS4.10.0 PowerPC heap space initialization
Kate Feng
feng1 at bnl.gov
Wed May 11 03:02:49 UTC 2011
Till Straumann wrote:
> I agree with Kate that ppc/sbrk is currently broken in 4.10
> (bsp_libc_init() is always called with a zero sbrk_amount).
>
> I also suggest that this should be fixed but I'm not sure
> the proposed approach (propagating information via global
> variables) is the best solution.
I had the same consideration. However, the BSP_mem_size
is propagated via a global variable as well, for the
PowerPC port only. Maybe that should be cleaned up on all
the BSPs as well. BSP_heap_start was a global variable for
RTEMS release, which is older than 4.10 (e.g. 4.9.x). Thus only
BSP_heap_sbrk_spared is a new global variable.
>
> It may be cleaner to add an additional argument to
> bsp_get_work_area() and bootcard_bsp_libc_helper()
> to propagate the sbrk_amount.
Mostly bsp_get_work_area() and bsp_pretasking_hook()
on all of the involved BSPs, which would impose a
global change, even on a non-PowerPC one, although
it would be meaningless on those non-PowerPC ones.
Thus, it might look cleaner for the PPC BSPs, but I
do not know about other ports.
I guess that Joel and some others would have to approve
for additional arguments on global functions:
bsp_get_work_area() and bsp_pretasking_hook().
Either way is fine with me.
Thanks,
Kate
>
> -- Till
>
>
> On 05/10/2011 05:40 PM, Kate Feng wrote:
>> Sebastian Huber wrote:
>>> Hello,
>>>
>>> I have some questions about this patch.
>> Can you or anyone see if the attached new patch acceptable, please ?
>> I am thinking to submit a PR by tomorrow afternoon if there is
>> no further question, feedback or suggestion. It seems a critical bug.
>>
>> Cheers,
>> Kate
>>> On 05/07/2011 02:44 PM, Kate Feng wrote:
>>>> Hi Joel,
>>>>
>>>> There are bugs in RTEMS4.10.0 and its CVS for the PowerPC heap space
>>>> allocation. All PPC boards are limited to 32M bytes of memory space
>>>> due
>>>> to the change in RTEMS4.10 for its work space. The attached patch
>>>> fixed
>>>> this.
>>>> I consider that it is critical. Thus, I will submit a PR.
>>>>
>>>> Cheers,
>>>> Kate Feng
>>>>
>>>>
>>>> rtems-4.10.0-powerpc-heapspace.diff
>>>>
>>>>
>>>> diff -Naur rtems-4.10.0.orig/c/src/lib/libbsp/powerpc/ChangeLog
>>>> rtems-4.10.0/c/src/lib/libbsp/powerpc/ChangeLog
>>>> --- rtems-4.10.0.orig/c/src/lib/libbsp/powerpc/ChangeLog 2011-03-04
>>>> 11:56:23.000000000 -0500
>>>> +++ rtems-4.10.0/c/src/lib/libbsp/powerpc/ChangeLog 2011-05-07
>>>> 08:15:17.000000000 -0400
>>>> @@ -1,3 +1,8 @@
>>>> +2011-05-07 Kate Feng <feng at bnl.gov>
>>>> +
>>>> + * shared/startup/bspgetworkarea.c : Moved _bsp_sbrk_init to +
>>>> c/src/lib/libbsp/shared/bootcard.c
>>>> +
>>>> 2011-03-04 Joel Sherrill <joel.sherrilL at OARcorp.com>
>>>>
>>>> * shared/start/start.S: Remove conflict markers in comment.
>>>> diff -Naur
>>>> rtems-4.10.0.orig/c/src/lib/libbsp/powerpc/shared/startup/bspgetworkarea.c
>>>>
>>>> rtems-4.10.0/c/src/lib/libbsp/powerpc/shared/startup/bspgetworkarea.c
>>>> ---
>>>> rtems-4.10.0.orig/c/src/lib/libbsp/powerpc/shared/startup/bspgetworkarea.c
>>>>
>>>> 2009-11-29 23:29:47.000000000 -0500
>>>> +++
>>>> rtems-4.10.0/c/src/lib/libbsp/powerpc/shared/startup/bspgetworkarea.c
>>>> 2011-05-07 08:21:08.000000000 -0400
>>>> @@ -31,15 +31,12 @@
>>>> )
>>>> {
>>>> uintptr_t work_size;
>>>> - uintptr_t spared;
>>>> uintptr_t work_area;
>>>>
>>>> work_area = (uintptr_t)&__rtems_end +
>>>> rtems_configuration_get_interrupt_stack_size();
>>>> work_size = (uintptr_t)BSP_mem_size - work_area;
>>>>
>>>> - spared = _bsp_sbrk_init( work_area, &work_size );
>>>> -
>>>> *work_area_start = (void *)work_area,
>>>> *work_area_size = work_size;
>>>> *heap_start = BSP_BOOTCARD_HEAP_USES_WORK_AREA;
>>>> @@ -54,7 +51,7 @@
>>>> void *sp = __builtin_frame_address(0);
>>>> void *end = *work_area_start + *work_area_size;
>>>> printk(
>>>> - "work_area_start = 0x%p\n"
>>>> + "work_area_start = 0x%08\n"
>>>
>>> What about 64-bit architectures?
>>>
>>>> "work_area_size = %d 0x%08x\n"
>>>> "end = 0x%p\n"
>>>> "heap_start = 0x%p\n"
>>>> diff -Naur rtems-4.10.0.orig/c/src/lib/libbsp/shared/bootcard.c
>>>> rtems-4.10.0/c/src/lib/libbsp/shared/bootcard.c
>>>> --- rtems-4.10.0.orig/c/src/lib/libbsp/shared/bootcard.c 2009-10-12
>>>> 16:22:18.000000000 -0400
>>>> +++ rtems-4.10.0/c/src/lib/libbsp/shared/bootcard.c 2011-05-07
>>>> 07:58:07.000000000 -0400
>>>> @@ -77,8 +77,10 @@
>>>> uintptr_t heap_size
>>>> )
>>>> {
>>>> - if ( !rtems_unified_work_area &&
>>>> - heap_start == BSP_BOOTCARD_HEAP_USES_WORK_AREA) {
>>>> + uintptr_t spared;
>>>> +
>>>> + if ( (!rtems_unified_work_area) &&
>>>> + (heap_start == BSP_BOOTCARD_HEAP_USES_WORK_AREA)) {
>>>
>>> Why the extra parentheses?
>>>
>>>> uintptr_t work_space_size = rtems_configuration_get_work_space_size();
>>>>
>>>> heap_start = (char *) work_area_start + work_space_size;
>>>> @@ -89,8 +91,13 @@
>>>> heap_size = heap_size_default;
>>>> }
>>>> }
>>>> + else /* Kate Feng: if (rtems_unified_work_area) */ {kkk
>>>
>>> What is the purpose of this comment?
>>>
>>>> + heap_start = (char *) work_area_start ;
>>>
>>> What is the purpose of this cast? Why the extra ' ' before the ';'?
>>>
>>>> + heap_size = work_area_size;
>>>> + }
>>>> + spared = _bsp_sbrk_init( (uintptr_t) heap_start , &heap_size );
>>>
>>> Here we force every BSP to provide this sbrk() mechanic. I don't think
>>> this is
>>> good. This _bsp_sbrk_init() only exists for the PowerPC at the moment.
>>>
>>>>
>>>> - bsp_libc_init(heap_start, heap_size, 0);
>>>> + bsp_libc_init((void *) heap_start, heap_size, spared);
>>>
>>> What is the purpose of this cast?
>>>
>>>> }
>>>>
>>>> /*
>>>
>>
>>
>>
>> _______________________________________________
>> rtems-users mailing list
>> rtems-users at rtems.org
>> http://www.rtems.org/mailman/listinfo/rtems-users
>
> _______________________________________________
> rtems-users mailing list
> rtems-users at rtems.org
> http://www.rtems.org/mailman/listinfo/rtems-users
More information about the users
mailing list