On Nov 8, 2007, at 5:22 PM, Todd Poynor wrote:

> Matt Thomas wrote:
>> On Nov 7, 2007, at 4:58 PM, Todd Poynor wrote:
> ...
>>> The ARMv6 Virtually Indexed Physically Tagged cache maintenance  
>>> system control coprocessor instructions that invalidate lines  
>>> based on Modified Virtual Addresses throw Data Abort exceptions  
>>> for virtual addresses not mapped in the PTE. ...
>> I prefer the of onfault since this error shouldn't normally happen.
>
> Here's a patch against -current (but only tested on -4.0rc3) that  
> adds a "safe" wrapper around cpu_icache_sync_range via pcb_onfault  
> and returns zero or EFAULT to the caller.  Some notes on  
> questionable stuff I wrestled with follow.
>
> The wrapper is added to arch/arm/arm32/locore.S as a handy place to  
> put it within the arm32/ family.  This might not be the best place  
> for a cpufunc thing, but it needs some assym.h symbols only  
> generated for arm32.  The protototype is added to arch/arm/include/ 
> cpufunc.h, although it is arm32-only.  (It would be nice to have  
> fault catchers in C and do this in arch/arm/arm32/sys_machdep.c  
> with a setjmp-style call.)
>
> The wrapper knows the called routine doesn't mess with sp or r4.
>
>
> -- 
> Todd
>
> Index: netbsd-current-11082007/src/sys/arch/arm/arm32/genassym.cf
> ===================================================================
> --- netbsd-current-11082007.orig/src/sys/arch/arm/arm32/genassym.cf
> +++ netbsd-current-11082007/src/sys/arch/arm/arm32/genassym.cf
> @@ -130,6 +130,7 @@ define	TRAPFRAMESIZE		sizeof(struct trap
>
>  define	CF_IDCACHE_WBINV_ALL	offsetof(struct cpu_functions,  
> cf_idcache_wbinv_all)
>  define	CF_DCACHE_WB_RANGE	offsetof(struct cpu_functions,  
> cf_dcache_wb_range)
> +define  CF_ICACHE_SYNC_RANGE    offsetof(struct cpu_functions,  
> cf_icache_sync_range)
>  define	CF_TLB_FLUSHID_SE	offsetof(struct cpu_functions,  
> cf_tlb_flushID_SE)
>  define	CF_CONTEXT_SWITCH	offsetof(struct cpu_functions,  
> cf_context_switch)
>  define	CF_SLEEP		offsetof(struct cpu_functions, cf_sleep)
> Index: netbsd-current-11082007/src/sys/arch/arm/arm32/locore.S
> ===================================================================
> --- netbsd-current-11082007.orig/src/sys/arch/arm/arm32/locore.S
> +++ netbsd-current-11082007/src/sys/arch/arm/arm32/locore.S
> @@ -212,5 +212,39 @@ _C_LABEL(esym):	.word	_C_LABEL(end)
>  ENTRY_NP(abort)
>  	b	_C_LABEL(abort)
>
> +#ifdef MULTIPROCESSOR
> +.Lcpu_info:
> +        .word   _C_LABEL(cpu_info)
> +#else
> +.Lcurpcb:
> +        .word   _C_LABEL(curpcb)
> +#endif
> +
> +ENTRY(cpufunc_safe_icache_sync_range)
> +	stmfd	sp!, {lr}
	stmfd  sp!, {r4, lr}
> +#ifdef MULTIPROCESSOR
> +	/* XXX Probably not appropriate for non-Hydra SMPs */
> +	stmfd   sp!, {r0-r1}
> +	bl      _C_LABEL(cpu_number)
> +	ldr     r4, .Lcpu_info
> +	ldr     r4, [r4, r0, lsl #2]
> +	ldr     r4, [r4, #CI_CURPCB]
> +	ldmfd   sp!, {r0-r1}
> +#else
> +	ldr     r4, .Lcurpcb
> +	ldr     r4, [r4]
> +#endif
> +	adr     r3, .Licache_sync_fault
> +	str     r3, [r4, #PCB_ONFAULT]
> +	ldr	r2, .Lcpufuncs
> +	mov	lr, pc

For a call, the stack should be 8-byte aligned.

> +	ldr	pc, [r2, #CF_ICACHE_SYNC_RANGE]
> +	mov     r0, #0x00000000
	str r0, [r4, #PCB_ONFAULT]

> +	ldmfd	sp!, {pc}
	ldmfd sp!, {r4, pc}
> +
> +.Licache_sync_fault:
> +	mov     r1, #0x00000000
> +	str     r1, [r4, #PCB_ONFAULT]
> +	ldmfd	sp!, {pc}
	ldmfd sp!, {r4, pc}

I think these r1's should be r0.

>  /* End of locore.S */
> Index: netbsd-current-11082007/src/sys/arch/arm/arm32/sys_machdep.c
> ===================================================================
> --- netbsd-current-11082007.orig/src/sys/arch/arm/arm32/sys_machdep.c
> +++ netbsd-current-11082007/src/sys/arch/arm/arm32/sys_machdep.c
> @@ -70,10 +70,8 @@ arm32_sync_icache(p, args, retval)
>  	if ((error = copyin(args, &ua, sizeof(ua))) != 0)
>  		return (error);
>
> -	cpu_icache_sync_range(ua.addr, ua.len);
> -
> -	*retval = 0;
> -	return(0);
> +	*retval = cpufunc_safe_icache_sync_range(ua.addr, ua.len);
> +	return(*retval);

retval should stay 0.  0 should be returned.
>  }
>
>  static int
> Index: netbsd-current-11082007/src/sys/arch/arm/include/cpufunc.h
> ===================================================================
> --- netbsd-current-11082007.orig/src/sys/arch/arm/include/cpufunc.h
> +++ netbsd-current-11082007/src/sys/arch/arm/include/cpufunc.h
> @@ -211,6 +211,7 @@ u_int	cpufunc_control		(u_int, u_int);
>  void	cpufunc_domains		(u_int);
>  u_int	cpufunc_faultstatus	(void);
>  u_int	cpufunc_faultaddress	(void);
> +int	cpufunc_safe_icache_sync_range(vaddr_t, vsize_t);
>
>  #ifdef CPU_ARM2
>  u_int	arm2_id			(void);
>