svn commit: r193712 - head/sys/arm/arm

[Rafal Jaworowski]

On 2009-06-08, at 14:56, Stanislav Sedov wrote:

> On Mon, 8 Jun 2009 12:15:39 +0000 (UTC)
> Rafal Jaworowski <raj at FreeBSD.org> mentioned:
>
>> Author: raj
>> Date: Mon Jun  8 12:15:39 2009
>> New Revision: 193712
>> URL: http://svn.freebsd.org/changeset/base/193712
>>
>> Log:
>>  Invalidate cache in pmap_remove_all() on ARM.
>>
>>  When pages are removed from virtual address space by calling  
>> pmap_remove_all()
>>  CPU caches were not invalidated, which led to read corruption when  
>> another
>>  page got mapped at this same virtual address at later time (the  
>> CPU was
>>  retrieving stale contents).
>>
>>  Submitted by:	Piotr Ziecik
>>  Obtained from:	Semihalf
>>
>> Modified:
>>  head/sys/arm/arm/pmap.c
>>
>> Modified: head/sys/arm/arm/pmap.c
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> =====================================================================
>> --- head/sys/arm/arm/pmap.c	Mon Jun  8 12:10:42 2009	(r193711)
>> +++ head/sys/arm/arm/pmap.c	Mon Jun  8 12:15:39 2009	(r193712)
>> @@ -3124,7 +3124,19 @@ pmap_remove_all(vm_page_t m)
>> 		if (flush == FALSE && (pv->pv_pmap == curpm ||
>> 		    pv->pv_pmap == pmap_kernel()))
>> 			flush = TRUE;
>> +
>> 		PMAP_LOCK(pv->pv_pmap);
>> +		/*
>> +		 * Cached contents were written-back in pmap_remove_write(),
>> +		 * but we still have to invalidate the cache entry to make
>> +		 * sure stale data are not retrieved when another page will be
>> +		 * mapped under this virtual address.
>> +		 */
>> +		if (pmap_is_current(pv->pv_pmap)) {
>> +			cpu_dcache_inv_range(pv->pv_va, PAGE_SIZE);
>> +			cpu_l2cache_inv_range(pv->pv_va, PAGE_SIZE);
>> +		}
>> +
>
> Hi, Rafal!
>
> What about calling the pmap_dcache_wb_range function for each  
> mapping in
> the cycle instead of calling cpu_XXX_inv_range functions directly?   
> I think
> something like this would do the trick:
> % pmap_dcache_wb_range(pv->pv_pmap, pv->pv_va, PAGE_SIZE, FALSE,
> %     (pv->pv_flags & PVF_WRITE) == 0)
>
> This will also take care of the writeback cache.  I don't know if it  
> is really
> needed, though.


Do you see anything wrong with calling cpu_xxx_inv_range directly?  
Writing back (if required) was already performed by  
pmap_remove_write(), and what we only need at this point is  
invalidation. pmap_dcache_wb_range would also eventually call  
cpu_XXX_inv_range if given a proper combination of flags (BTW: the  
do_inv flag in your example should be TRUE for our context to work),  
so it wouldn't be any simpler. I'd rather prefer doing explicitly what  
is needed without extra wrapping.

Rafal