Re: [PATCH] oprofile: vma_map: fix test on overlay_tbl_offset

[Maynard Johnson <maynardj@us.ibm.com>]

Arnd Bergmann wrote:
> On Wednesday 23 April 2008, Maynard Johnson wrote:
>   
>> Roel Kluin wrote:
>>     
>>> Offset is unsigned and when an address isn't found in the vma map
>>> vma_map_lookup() returns the vma physical address + 0x10000000.
>>>
>>> Signed-off-by: Roel Kluin <12o3l@tiscali.nl>
>>>   
>>>       
>> Patch looks correct.  vma_map_lookup used to return 0xffffffff on a 
>> failed lookup, but a change was recently made to return the vma physical 
>> address + 0x10000000 (as Roel notes above).  There are two callers of 
>> vam_map_lookup: one of them correctly deals with this new return value, 
>> but the other (below) did not.  Roel's patch fixes that hole.  Thanks!
>>
>> Arnd, can you put this into the Cell tree for pushing upstream?  Thanks.
>>     
>
> Sorry for not having looked at this earlier. I'm now queuing up patches
> for 2.6.27, but this one looks incorrect:
>
>   
>>> diff --git a/arch/powerpc/oprofile/cell/vma_map.c b/arch/powerpc/oprofile/cell/vma_map.c
>>> index 9a93217..1c28e2e 100644
>>> --- a/arch/powerpc/oprofile/cell/vma_map.c
>>> +++ b/arch/powerpc/oprofile/cell/vma_map.c
>>> @@ -229,7 +229,7 @@ struct vma_to_fileoffset_map *create_vma_map(const struct spu *aSpu,
>>>  	 */
>>>  	overlay_tbl_offset = vma_map_lookup(map, ovly_table_sym,
>>>  					    aSpu, &grd_val);
>>> -	if (overlay_tbl_offset < 0) {
>>> +	if (overlay_tbl_offset >= 0x10000000) {
>>>  		printk(KERN_ERR "SPU_PROF: "
>>>  		       "%s, line %d: Error finding SPU overlay table\n",
>>>  		       __FUNCTION__, __LINE__);
>>>
>>>       
>
> You mention that the kernel can handle the return code correctly, but
> this code still prints an error message in that case, which does not
> make sense then.
>   
The above patch is actually correct.  There are two places that call the
vma_map_lookup function.  The above invocation is being done during
initial parsing of the SPU binary, and here, we expect to find the
offset for the overlay table symbol at a valid VMA.  Any value greater
than the physical size of SPE memory (0x10000000) is considered an error
here.  In the other invocation of vma_map_lookup (in spu_task_sync.c),
we're looking up the fileoffset for a sampled instruction address.  In
this case, a return value of greater than 0x10000000 is interpreted as a
sample taken from the __send_to_ppe call stub that is dynamically
generated and placed on the SPU stack
> Also, where does the 0x10000000 number come from? Is that the maximum
>   
See above.

Thanks.
-Maynard
> size that all overlays can consume in one binary?
>
> 	Arnd <><
>