[Cbe-oss-dev] [PATCH 1/1] Fix another user-visible SPU coredump bug

[John DelSignore]

Hi Michael,

Thanks much for pro-actively fixing this problem. I had noticed this myself, but I have not gotten to the point in porting our debugger where it matters, so I hadn't reported it yet.

Two things:

1) In order to provide compatibility with old broken core files and the new working core files, I'd like to know how the various fields were/are justified in the note. For example, for lslr a nominally 4-byte register was placed in 11 bytes and will now be placed in 8 bytes. Which 4 bytes of the 11 or 8 byte note hold the value? lslr is easy to figure out by inspection of the bits because lslr is normally 0x3ffff, but some of the other fields it's harder to figure out where the value lives. Is there some rule I can use to figure out where the value lives in the note, possibly as a function of the note's data length?

2) A while back there was a discussion about the absence of npc in the core file, and the final consensus was that the npc should be written to the core file. It was suggested that I generate a patch to include npc in a core file and submit it. But, I'm not familiar enough with the kernel to make such a change, and I was hoping that someone else, possibly you, could make that change. Is this something you'd be willing to do?

Thanks, John D.


Michael Ellerman wrote:
> The SPU coredump code has an array of "spufs_coredump_reader"s, each of
> which has either a read or a get callback, as well as the size of the
> data it will generate. Those with read callbacks can generate an arbitrary
> amount of data, however the get callbacks all return a u64, 8 bytes, and
> this is enforced by do_coredump_read().
> 
> There seems to be a bit of confusion however, and some of the get
> callbacks supposedly return quantities other than 8 bytes. It looks as
> though there might have been some ascii vs binary mixup,
> eg. "0x11112222\n" == 11, and "0x1111222233334444\n" == 19.
> 
> Although this bug doesn't lead to unloadable coredumps, it does make the
> contents of the SPU notes incomprehensible to user space (without hacks)
> - so I think we should fix it asap.
> 
> Signed-off-by: Michael Ellerman <michael at ellerman.id.au>
> Acked-by: Jeremy Kerr <jk at ozlabs.org>
> ---
> 
> Paulus, please apply for 23, Jeremy has already acked this. Apologies this
> is late in the cycle, I've only just had time to look into it.
> 
> ---
>  arch/powerpc/platforms/cell/spufs/file.c |   12 ++++++------
>  1 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/cell/spufs/file.c b/arch/powerpc/platforms/cell/spufs/file.c
> index 4100ddc..240a382 100644
> --- a/arch/powerpc/platforms/cell/spufs/file.c
> +++ b/arch/powerpc/platforms/cell/spufs/file.c
> @@ -2233,14 +2233,14 @@ struct tree_descr spufs_dir_nosched_contents[] = {
>  struct spufs_coredump_reader spufs_coredump_read[] = {
>  	{ "regs", __spufs_regs_read, NULL, 128 * 16 },
>  	{ "fpcr", __spufs_fpcr_read, NULL, 16 },
> -	{ "lslr", NULL, __spufs_lslr_get, 11 },
> -	{ "decr", NULL, __spufs_decr_get, 11 },
> -	{ "decr_status", NULL, __spufs_decr_status_get, 11 },
> +	{ "lslr", NULL, __spufs_lslr_get, 8 },
> +	{ "decr", NULL, __spufs_decr_get, 8 },
> +	{ "decr_status", NULL, __spufs_decr_status_get, 8 },
>  	{ "mem", __spufs_mem_read, NULL, 256 * 1024, },
>  	{ "signal1", __spufs_signal1_read, NULL, 4 },
> -	{ "signal1_type", NULL, __spufs_signal1_type_get, 2 },
> +	{ "signal1_type", NULL, __spufs_signal1_type_get, 8 },
>  	{ "signal2", __spufs_signal2_read, NULL, 4 },
> -	{ "signal2_type", NULL, __spufs_signal2_type_get, 2 },
> +	{ "signal2_type", NULL, __spufs_signal2_type_get, 8 },
>  	{ "event_mask", NULL, __spufs_event_mask_get, 8 },
>  	{ "event_status", NULL, __spufs_event_status_get, 8 },
>  	{ "mbox_info", __spufs_mbox_info_read, NULL, 4 },
> @@ -2248,7 +2248,7 @@ struct spufs_coredump_reader spufs_coredump_read[] = {
>  	{ "wbox_info", __spufs_wbox_info_read, NULL, 16 },
>  	{ "dma_info", __spufs_dma_info_read, NULL, 69 * 8 },
>  	{ "proxydma_info", __spufs_proxydma_info_read, NULL, 35 * 8 },
> -	{ "object-id", NULL, __spufs_object_id_get, 19 },
> +	{ "object-id", NULL, __spufs_object_id_get, 8 },
>  	{ },
>  };
>  int spufs_coredump_num_notes = ARRAY_SIZE(spufs_coredump_read) - 1;