A patch for RTEMS4.10.0 PowerPC heap space initialization

[Till Straumann]

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.

It may be cleaner to add an additional argument to
bsp_get_work_area() and bootcard_bsp_libc_helper()
to propagate the sbrk_amount.

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