[Skiboot] [PATCH v7 18/22] fadump: Add documentation

[Nicholas Piggin]

Mahesh J Salgaonkar's on May 21, 2019 2:40 am:
> On 2019-05-16 15:35:21 Thu, Nicholas Piggin wrote:
>> Vasant Hegde's on May 14, 2019 9:23 pm:
>> > On 05/09/2019 10:28 AM, Nicholas Piggin wrote:
>> >> Vasant Hegde's on April 13, 2019 7:15 pm:
>> > Kernel informs OPAL about range of memory to be preserved during MPIPL
>> > (source, destination, size).
>> 
>> Well it also contains crashing_cpu, type, and comes in this clunky
>> structure.
>> 
>> > After reboot, we will result range from hostboot . We pass that to kernel via
>> > device tree.
>> > 
>> >> 
>> >> Why not just an API which can add a range, and delete a range, and
>> >> that's it? Range would just be physical start, end, plus an arbitrary
>> >> tag (which caller can use to retrieve metadata that is used to
>> >> decipher the dump).
>> > 
>> > We want one to one mapping between source and destination.
>> 
>> Ah yes, sure that too. So two calls, one which adds or removes
>> (source, dest, length) entries, and another which sets a tag.
> 
> The list of memory ranges that are selected by kernel to be
> moved/preserved are based on the fact that they will be overwritten on
> next boot after crash either by new kernel, initrd or some f/w code load.
> Hence it is important to move and preserve all that memory content for a
> successful dump creation which is valid. Failure to move/preserve even
> one memory range would result into an invalid/incomplete vmcore.
> 
> Hence, the idea was to have a single call to add all the ranges at once
> or fail the registration. Splitting the calls for a range at a time
> increases the chances/window of getting partial ranges registered for
> MPIPL. If OPAL crashes during the registration (between add API calls),
> subsequent MPIPL would result in the kernel creating incomplete/incorrect
> vmcore. If we decide to have multiple API to add memory ranges then we
> may also need an API to complete the registration where kernel can
> indicate OPAL that it is done adding the ranges and changes can be
> committed.

I don't see correctness as a reason to do an array of structures API.

 init()
 {
   ... register MPIPL ranges ...
   success:
     mpipl_registered = true;
 }

 crash()
 {
   if (mpipl_registered)
      opal_reboot(MPIPL);
 }

I won't quibble over the exact look of the API if it has no crash
metadata in it, but building an array and passing that in one call
seems worse to me if we don't explicitly need performance or
atomicity etc you get with the single call.

Thanks,
Nick