Re: Removing armfpe (was STACKCHECKS)

[Matt Thomas <matt%3am-software.com@localhost>]

On Dec 5, 2012, at 9:39 AM, 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.

Agreed.  But some of them will need to adapted for VFP.

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

The bug was not putting the fpcontext into the pcb in the first place.