oprofile: vma_map: fix test on overlay_tbl_offset

PATCH In review Roel Kluin 3 participants 4 messages Apr 17, 2008 – Jul 1, 2008 PowerPC

Share & tools

Roel Kluin

author

2008-04-17 13:22 UTC #0 lore raw

PATCH 1/1 oprofile: vma_map: fix test on overlay_tbl_offset

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 <[email protected]>

---

arch/powerpc/oprofile/cell/vma_map.c +1 -1

index 9a93217..1c28e2e 100644
@@ -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__);

Maynard Johnson

2008-04-23 13:37 UTC #1 lore raw

Quoted 5 lines · 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 <[email protected]>

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

Quoted 23 lines

---
arch/powerpc/oprofile/cell/vma_map.c +1 -2
index 9a93217..1c28e2e 100644
@@ -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
[email protected]
https://lists.sourceforge.net/lists/listinfo/oprofile-list

Arnd Bergmann

2008-07-01 14:01 UTC #2 lore raw

Quoted 6 lines · Maynard Johnson

Quoted 5 lines · 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 <[email protected]>

Quoted 7 lines

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:

Quoted 6 lines

arch/powerpc/oprofile/cell/vma_map.c +0 -0

index 9a93217..1c28e2e 100644
@@ -229,7 +229,7 @@ struct vma_to_fileoffset_map *create_vma_map(const struct spu *aSpu,
 	 */

Quoted 8 lines

arch/powerpc/oprofile/cell/vma_map.c struct vma_to_fileoffset_map *create_vma_map(const struct spu *aSpu, · line 235

Quoted 8 lines

arch/powerpc/oprofile/cell/vma_map.c +1 -1

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

Maynard Johnson

2008-07-01 19:10 UTC #3 lore raw

Quoted 34 lines · Arnd Bergmann

Quoted 8 lines · Maynard Johnson
Quoted 6 lines · 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 <[email protected]>
Quoted 8 lines
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:
Quoted 11 lines
Quoted 11 lines
arch/powerpc/oprofile/cell/vma_map.c +1 -1
index 9a93217..1c28e2e 100644
@@ -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: "

Quoted 9 lines

Quoted 4 lines
arch/powerpc/oprofile/cell/vma_map.c struct vma_to_fileoffset_map *create_vma_map(const struct spu *aSpu, · line 235
Quoted 4 lines
arch/powerpc/oprofile/cell/vma_map.c +0 -0
 		       "%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

Quoted 2 lines

Also, where does the 0x10000000 number come from? Is that the maximum

See above.

Thanks.
-Maynard

Quoted 4 lines

size that all overlays can consume in one binary?
	Arnd <><