Re: Removing armfpe (was STACKCHECKS)

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

(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

 STACKCHECKS:
        fill between USPACE_SVC_STACK_BOTTOM and USPACE_SVC_STACK_TOP

        The size is 0x1e70

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)