[PATCH] oprofile: vma_map: fix test on overlay_tbl_offset

[PATCH] oprofile: vma_map: fix test on overlay_tbl_offset

[Roel Kluin]

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>
---
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__);

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

[Maynard Johnson]

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.

-Maynard
> ---
> 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__);
>
>
> -------------------------------------------------------------------------
> This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
> Don't miss this year's exciting event. There's still time to save $100. 
> Use priority code J8TL2D2. 
> http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone
> _______________________________________________
> oprofile-list mailing list
> oprofile-list@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/oprofile-list
>   

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

[Arnd Bergmann]

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.

Also, where does the 0x10000000 number come from? Is that the maximum
size that all overlays can consume in one binary?

	Arnd <><

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

[Maynard Johnson]

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