[Skiboot] [PATCH 2/2] core: Implement non-FSP dump region opal call

[Joel Stanley]

On 25 May 2018 at 15:46, Vasant Hegde <hegdevasant at linux.vnet.ibm.com> wrote:
> On 05/24/2018 07:07 AM, Joel Stanley wrote:
>>
>> This provides an implementation of the OPAL_REGISTER_DUMP_REGION and
>> OPAL_UNREGISTER_DUMP_REGION calls that can be used by non-FSP systems,
>> and performs that registration for all astbmc platforms.
>>
>> This pointer will be used by eg. pdbg to fetch the host kernel's log
>> buffer over FSI.
>
>
> Cool!
>
>>
>> We unconditionally clear the value on a reboot (both fast-reboot and
>> reliable reboot) to reduce the chance of debug tools getting the wrong
>> buffer.
>>
>> Signed-off-by: Joel Stanley <joel at jms.id.au>
>> ---
>>   core/Makefile.inc         |  2 +-
>>   core/dump-region.c        | 61 +++++++++++++++++++++++++++++++++++++++
>
>
> I'm bit concerned about the filename here. I've patchset to add MPIPL
> support in OPAL.
> I've added core/opal-dump.c file. Also note that I've renamed
> hw/fsp/fsp-mdst-table.c as
> fsp-sysdump.c. So having multiple files with *dump* may confuse. But I can't
> think of
> any better name here. May be I should rename mpipl stuff as opal-mpipl.c or
> opal-fadump.c

I've named it after the opal call that it implements.

I don't think there's anything wrong with having fsp-sysdump (or even
fsp-dump-region), as it's pretty clear what is going on.

I'd suggest we name things after the opal/skiboot features they
implement, and if there are external features that use these features
that's fine, but it doesn't need to influcence the naming. (For
example, I didn't call this file pdbg-dmesg, even though that's what
the first user will be).

An enhancement that could be made here is to have the one
dump-region.c that registers the debug descriptor for all platforms,
and if it's on FSP also calls into the mdst/sysdump platform code. I
played with this idea but the code didn't come out very clean, so it
would require more effort

>>   core/init.c               |  2 ++
>>   include/dump.h            | 26 +++++++++++++++++
>
>
> Better rename this  as dump-region.h or just move declaration to skiboot.h
> itself?

I'll rename it.

>> diff --git a/core/init.c b/core/init.c
>> index 3b887a24d11c..1ef7d800ae9a 100644
>> --- a/core/init.c
>> +++ b/core/init.c
>> @@ -563,6 +563,8 @@ void __noreturn load_and_boot_kernel(bool is_reboot)
>>         mem_dump_free();
>>
>
>
> May be add comment here explaining why you are clearing log_buf_phsy here?

Ok.

Thanks for the review.

Cheers,

Joel