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

[Maynard J. <may...@us...>]

Bob Nelson wrote:
> Maynard Johnson wrote:
>> 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 <12...@ti...>
>>>>>                 
>>>> 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
>>   
> Actually the physical size of SPE memory is 0x40000 (256k).  The number 
Well, I was only off by an order of magnitude -- heh.  Thanks for that 
correction, Bob.
> used, at least for __send_to_spe, seemed sufficiently large that any 
> future version of the Cell SPEs wouldn't be too likely to have more than 
> that.
>> 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 <><
>>>       
>>
>>
>>   
> 
>