Re: [PATCH] remoteproc: zynqmp: Add coredump support

[Leonard Crestez <cdleonard@gmail.com>]

On 4/4/24 23:14, Tanmay Shah wrote:
> Hello,
> 
> Thanks for your patch. Patch looks good to me.
> Please find some comments below.
> 
> On 3/16/24 1:16 PM, Leonard Crestez wrote:
>> Supporting remoteproc coredump requires the platform-specific driver to
>> register coredump segments to be dumped. Do this by calling
>> rproc_coredump_add_segment for every carveout.
>>
>> Also call rproc_coredump_set_elf_info when then rproc is created. If the
>> ELFCLASS parameter is not provided then coredump fails with an error.
>> Other drivers seem to pass EM_NONE for the machine argument but for me
>> this shows a warning in gdb. Pass EM_ARM because this is an ARM R5.
>>
>> Signed-off-by: Leonard Crestez <cdleonard@gmail.com>
>> ---
>>
>> Tests were done by triggering an deliberate crash using remoteproc
>> debugfs: echo 2 > /sys/kernel/debug/remoteproc/remoteproc0/crash
>>
>> The R5 cache is not flushed so RAM might be out of date which is
>> actually very bad because information most relevant to determining the
>> cause of a crash is lost. Possible workaround would be to flush caches
>> in some sort of R5 crash handler? I don't think Linux can do anything
>> about this limitation.
>>
> 
> Correct Linux can't. One workaround is that R5 firmware can mark
> required memory regions as non-cachable in MPU setting. This way information
> loss can be avoided.

The solution I ended up with is to add cache flushing in some sort of R5-side
crash handler.

>> The generated coredump doesn't contain registers, this seems to be a
>> limitation shared with other rproc coredumps. It's not clear how the apu
>> could access rpu registers on zynqmp, my only idea would be to use the
>> coresight dap but that sounds difficult.
> 
> Linux doesn't really have access to R5 control registers due to security.
> Instead EEMI calls to platform management controller are used to control R5.
> So R5 control register dump shouldn't needed.
> 
> Mathieu,
> I am okay to merge this patch.

Thanks for the review.