Re: Removing armfpe (was STACKCHECKS)

[SAITOH Masanobu <msaitoh%execsw.org@localhost>]

(2012/12/06 2:39), SAITOH Masanobu wrote:
> (2012/12/05 22:21), Matt Thomas wrote:
>>
>> On Dec 5, 2012, at 1:36 AM, Masanobu SAITOH wrote:
>>
>>> Hi.
>>>
>>> It seems we can't compile kernel with STACKCHECKS.
>>>
>>> Is the following patch correct?
>>>
>>> Index: vm_machdep.c
>>> ===================================================================
>>> RCS file: /cvsroot/src/sys/arch/arm/arm32/vm_machdep.c,v
>>> retrieving revision 1.61
>>> diff -u -r1.61 vm_machdep.c
>>> --- vm_machdep.c    23 Oct 2012 22:50:00 -0000      1.61
>>> +++ vm_machdep.c    5 Dec 2012 09:26:43 -0000
>>> @@ -210,10 +210,12 @@
>>> #ifdef STACKCHECKS
>>>     /* Report how much stack has been used - debugging */
>>>     if (l) {
>>> +           vaddr_t uv;
>>>             u_char *ptr;
>>>             int loop;
>>>
>>> -           ptr = (u_char *)pcb + USPACE_SVC_STACK_BOTTOM;
>>> +           uv = uvm_lwp_getuarea(l);
>>> +           ptr = (u_char *)(uv + USPACE_SVC_STACK_BOTTOM);
>>>             for (loop = 0; loop < (USPACE_SVC_STACK_TOP - 
>>> USPACE_SVC_STACK_BOTTOM)
>>>                 && *ptr == 0xdd; ++loop, ++ptr) ;
>>>             log(LOG_INFO, "%d bytes of svc stack fill pattern\n", loop);
>>>
>>>
>>> -- 
>>
>> Since armfpe hasn't compiled since we switched to lwps in NetBSD
>> 5.0, I think we should just remove it.
> 
> 
> Do you mean the first five lines in cpu_lwp_free() should be removed? right?
> 
>> void
>> cpu_lwp_free(struct lwp *l, int proc)
>> {
>> #ifdef ARMFPE
>>      /* Abort any active FP operation and deactivate the context */
>>      arm_fpe_core_abort(FP_CONTEXT(l), NULL, NULL);
>>      arm_fpe_core_changecontext(0);
>> #endif       /* ARMFPE */
> 
> We can see a lot of #ifdef ARMFPEs. If those will not be used in future,
> we should remove all of them.
> 
>> The log seems to be noisy
>> and probably should only print if a new "stack low" is encountered.
> 
>  Offcourse I think so, but I had thought that the existence of #ifdef
> STACKCHECKS is to show the usage at every exit. If we don't show it,
> MI KSTACK_CHECK_MAGIC has more functions than arm's STACKCHECKS, so
> we should remove STACKCHECKS function completely if KSTACKC_CHECK_MAGIC
> works correctly on arm.
> 
>  The real reason why I ask this change, I noticed that the filling area
> between KSTACK_CHECK_MAGIC and STACKCHECK is different.
> 
>  KSTACK_CHECK_MAGIC:
>       fill between KSTACK_LOWEST_ADDR(l) and STACK_LOWEST_ADDR(l)+KSTACK_SIZE
> 
>       The size on arm is 0x1d66

This is 0x1e70

>  STACKCHECKS:
>       fill between USPACE_SVC_STACK_BOTTOM and USPACE_SVC_STACK_TOP
> 
>       The size is 0x1e70

This is 0x1d66

> The USPACE structure on arm is as follows:
>> /*
>>  * The USPACE area contains :
>>  * 1. the pcb structure for the process
>>  * 2. the fp context for FP emulation
>>  * 3. the kernel (svc) stack
>>  *
>>  * The layout of the area looks like this
>>  *
>>  * | uarea | FP context | kernel stack |
>>  *
>>  * The size of the uarea is known.
>>  * The size of the FP context is variable depending of the FP emulator
>>  * in use and whether there is hardware FP support. However we can put
>>  * an upper limit on it.
>>  * The kernel stack should be at least 4K is size.
>>  *
>>  * The stack top addresses are used to set the stack pointers. The stack 
>> bottom
>>  * addresses at the addresses monitored by the diagnostic code for stack 
>> overflows
>>  *
>>  */
>>
>> #define FPCONTEXTSIZE                        (0x100)
>> #define USPACE_SVC_STACK_TOP         (USPACE)
>> #define USPACE_SVC_STACK_BOTTOM              (sizeof(struct pcb) + 
>> FPCONTEXTSIZE + 10)
> 
> And KSTACK_LOWEST_ADDR and KSTACK_SIZE are (usually) defined in sys/proc.h:
>> /*
>>  * Kernel stack parameters.
>>  *
>>  * KSTACK_LOWEST_ADDR: return the lowest address of the LWP's kernel stack,
>>  * excluding red-zone.
>>  *
>>  * KSTACK_SIZE: the size kernel stack for a LWP, excluding red-zone.
>>  *
>>  * if <machine/proc.h> provides the MD definition, it will be used.
>>  */
>> #ifndef KSTACK_LOWEST_ADDR
>> #define      KSTACK_LOWEST_ADDR(l)   ((void *)ALIGN((struct pcb 
>> *)((l)->l_addr) + 1))
>> #endif
>> #ifndef KSTACK_SIZE
>> #define      KSTACK_SIZE             (USPACE - ALIGN(sizeof(struct pcb)))
>> #endif
> 
> Both KSTACK_LOWEST_ADDR and KSTACK_SIZE are not defined in <machine/proc.h>.
> 
> Is this a bug?
> 


-- 
-----------------------------------------------
                SAITOH Masanobu (msaitoh%execsw.org@localhost
                                 msaitoh%netbsd.org@localhost)