[PATCH 18/19] arm64: kdump: update a kernel doc

[AKASHI Takahiro]

On 01/21/2016 09:02 PM, Mark Rutland wrote:
> On Thu, Jan 21, 2016 at 03:53:42PM +0900, AKASHI Takahiro wrote:
>> On 01/20/2016 08:49 PM, Mark Rutland wrote:
>>> On Wed, Jan 20, 2016 at 03:07:53PM +0900, AKASHI Takahiro wrote:
>>>> On 01/20/2016 11:49 AM, Dave Young wrote:
>>>>> On 01/19/16 at 02:01pm, Mark Rutland wrote:
>>>>>> On Tue, Jan 19, 2016 at 09:45:53PM +0800, Dave Young wrote:
>>>>>>> On 01/19/16 at 12:51pm, Mark Rutland wrote:
>>>>>>>> On Tue, Jan 19, 2016 at 08:28:48PM +0800, Dave Young wrote:
>>>>>>>>> On 01/19/16 at 02:35pm, AKASHI Takahiro wrote:
>>>>>>>>>> On 01/19/2016 10:43 AM, Dave Young wrote:
>>>>>>>>>>> X86 takes another way in latest kexec-tools and kexec_file_load, that is
>>>>>>>>>>> recreating E820 table and pass it to kexec/kdump kernel, if the entries
>>>>>>>>>>> are over E820 limitation then turn to use setup_data list for remain
>>>>>>>>>>> entries.
>>>>>>>>>>
>>>>>>>>>> Thanks. I will visit x86 code again.
>>>>>>>>>>
>>>>>>>>>>> I think it is X86 specific. Personally I think device tree property is
>>>>>>>>>>> better.
>>>>>>>>>>
>>>>>>>>>> Do you think so?
>>>>>>>>>
>>>>>>>>> I'm not sure it is the best way. For X86 we run into problem with
>>>>>>>>> memmap= design, one example is pci domain X (X>1) need the pci memory
>>>>>>>>> ranges being passed to kdump kernel. When we passed reserved ranges in /proc/iomem
>>>>>>>>> to 2nd kernel we find that cmdline[] array is not big enough.
>>>>>>>>
>>>>>>>> I'm not sure how PCI ranges relate to the memory map used for normal
>>>>>>>> memory (i.e. RAM), though I'm probably missing some caveat with the way
>>>>>>>> ACPI and UEFI describe PCI. Why does memmap= affect PCI memory?
>>>>>>>
>>>>>>> Here is the old patch which was rejected in kexec-tools:
>>>>>>> http://lists.infradead.org/pipermail/kexec/2013-February/007924.html
>>>>>>>
>>>>>>>>
>>>>>>>> If the kernel got the rest of its system topology from DT, the PCI
>>>>>>>> regions would be described there.
>>>>>>>
>>>>>>> Yes, if kdump kernel use same DT as 1st kernel.
>>>>>>
>>>>>> Other than for testing purposes, I don't see why you'd pass the kdump
>>>>>> kernel a DTB inconsistent with that the 1st kernel was passsed (other
>>>>>> than some proerties under /chosen).
>>>>>>
>>>>>> We added /sys/firmware/fdt specifically to allow the kexec tools to get
>>>>>> the exact DTB the first kernel used. There's no reason for tools to have
>>>>>> to make something up.
>>>>>
>>>>> Agreed, but kexec-tools has an option to pass in any dtb files. Who knows
>>>>> how one will use it unless dropping the option and use /sys/firmware/fdt
>>>>> unconditionally.
>>>>
>>>> As a matter of fact, specifying proper command line parameters as well as
>>>> dtb is partly users' responsibility for kdump to work correctly.
>>>> (especially for BE kernel)
>>>>
>>>>> If we choose to implement kexec_file_load only in kernel, the interfaces
>>>>> provided are kernel, initrd and cmdline. We can always use same dtb.
>>>>
>>>> I would say that we can always use the same dtb even with kexec_load
>>> >from user's perspective. Right?
>>>
>>> No.
>>>
>>> This breaks using kexec for boot-loader purposes, and imposes a policy.
>>
>> What kind of policy?
>> I said "can", but if we want to use other setting/configuration, we can
>> still have a full control.
>
> Apologies, I misunderstood.
>
> In most cases, using the existing DTB (with minor modifications to
> /chosen for bootargs and such) is fine. If the user just wants to boot
> another Linux kernel, that's generally what they should do.
>
> I think we're agreed on that.

Yes.

> However, there are cases when the user may want to use a different DTB,
> or use a different purgatory. So we cannot mandate that the existing DTB
> is reused, nor that an in-kernel purgatory must be used, as that imposes
> a policy.

Agree!

>>> For better or worse kexec_file_load has always imposed a constrained
>>> Linux-only policy, so that's a different story.
>>>
>>>>>> There's a horrible edge case I've spotted if performing a chain of
>>>>>> cross-endian kexecs: LE -> BE -> LE, as the BE kernel would have to
>>>>>> respect the EFI memory map so as to avoid corrupting it for the
>>>>>> subsequent LE kernel. Other than this I believe everything should just
>>>>>> work.
>>>>>
>>>>> Firmware do not know kernel endianniess, kernel should respect firmware
>>>>> maps and adapt to it, it sounds like a generic issue not specfic to kexec.
>>>>
>>>> On arm64, a kernel image header has a bit field to specify the image's endianness.
>>>> Anyway, our current implementation replies on a user-supplied dtb to start BE kernel.
>>>
>>> The firmware should _never_ care about the kernel's endianness. The
>>> bootlaoder or first kernel shouldn't care about the next kernel's
>>> endianness apart from in exceptional circumstances. The DTB for a LE
>>> kernel should look identical to that passed to a BE kernel.
>>
>> Please note that I didn't say anything different from your last two statements.
>> The current arm64 kexec implementation doesn't do anything specific to BE,
>> but as far as BE kernel doesn't support UEFI, users are responsible for
>> providing a proper dtb.
>
> I'm just confused as to what you mean by a "proper dtb" in that case.
>
> If you just mean one with memory nodes hacked in, then that would
> currently be a way to make that work, yes.

One of useful cases that I have in my mind is kdump.
We may want to use a small sub-set of dtb, especially devices, to
make the reboot more reliable. Device drivers are likely to be vulnerable
at crash.

> It seems like the better option is to fix the BE kernel to support a
> UEFI memory map, as that solves other issues.

Why did Ard throw away his patch?

>>
>>> In my mind, the only valid reason to look at that bit is so that
>>> bootloaders can provide a warning if the CPU does not implement that
>>> endianness.
>>>
>>> The issue I mention above should be solved by changes to the BE kernel.
>>>
>>>>>>> Is it possible to modify uefi memmap for kdump case?
>>>>>>
>>>>>> Technically it would be possible, however I don't think it's necessary,
>>>>>> and I think it would be disadvantageous to do so.
>>>>>>
>>>>>> Describing the range(s) the crash kernel can use in separate properties
>>>>>> under /chosen has a number of advantages.
>>>>>
>>>>> Ok, I got the points. We have a is_kdump_kernel() by checking if there is
>>>>> elfcorehdr_addr kernel cmdline. This is mainly for some drivers which
>>>>> do not work well in kdump kernel some uncertain reasons. But ideally I
>>>>> think kernel should handle things just like in 1st kernel and avoid to use
>>>>> it.
>>>>
>>>> So I'm not still sure about what are advantages of a property under /chosen
>>>> over "memmap=" kernel parameter.
>>>> Both are simple and can have the same effect with minimizing changes to dtb.
>>>> (But if, in the latter case, we have to provide *all* the memory-related information
>>>> through "memmap=" parameters, it would be much complicated.)
>>>
>>> The reason I prefer a property over command line additions include:
>>
>> Take some examples:
>> (a) a property under /chosen
>>    {
>>      chosen = {
>>        cmdline = "elfcorehdr=AA at BB maxcpus=1 ...";
>>      }
>>      usable-memory = <XX YY>;
>>      memory {
>>        ...
>>      }
>>    }
>>
>> (b) a kernel command line parameter
>>    (I use the same name, "usable-memory", to show the similarity. may use another name though.)
>>    {
>>      chosen = {
>>        cmdline = "elfcorehdr=AA at BB maxcpus=1 usable-memory=YY at XX ...";
>>      }
>>      memory {
>>        ...
>>      }
>>    }
>>
>>> * It keeps the command line simple (as you mention the opposite is
>>>    "complicated").
>>
>> I think both are simple.
>>
>>> * It is logically separate from options the user may pass to the kernel
>>>    in that the restricted region(s) of memory avaialble are effectively
>>>    properties of the system (in that the crashed OS is part of the system
>>>    state).
>>
>> "elfcorehdr=" parameter already breaks your point.
>> "elfcorehdr=" looks to be, what you say, a system property, and is actually
>> added by kexec-tools on all architectures, and "usable-memory", whether it is
>> a DT property or a kernel parameter, will also be added by kexec-tools.
>> (Users don't have to care.)
>
> Just because architectures do one thing today does not mean that we have
> to follow it.
>
> I don't think that breaks my point so much as shows that a different
> approach is taken by others today.
>
> There's also no reason this cannot be a property under /chosen.

No, but no strong reason to be so IMO.

>>> * The semantics of the command line parsing can change subtly over time
>>>    (for example, see 51e158c12aca3c9a, which terminates command line
>>>    parseing at "--"). Maknig sure that a command line option will
>>>    actually be parsed by the next kernel is not trivial.
>>>
>>>    Keeping this information isolated from the command line is more
>>>    robust.
>>
>> Even so, who wants to use kdump without testing?
>> and this is not a kdump specific issue.
>>
>>> * Addition of a property is a self-contained operation, that doesn't
>>>    require any knowledge about the command line.
>>
>> I don't get your point here.
>> For a kernel parameter, early_param() can encapsulate all the stuffs necessary.
>> Once the kernel recognizes a usable memory region, limiting available
>> memory should be done in the exact same way.
>
> I mean when modifying the command line.

OK, I understand what you mean.

> To place "elfcorehdr=" or "memmap="/"usable-memory=" into the command
> line, one needs to know where it is valid to place it. Appending doesn't
> always work, as per the example above with 51e158c12aca3c9a.

So, are you now suggesting that we put both "elfcorehdr=" and
"usable-memory=" under /chosen in dtb? That's fair enough.
(as far as nobody cares about incompatibility with other archs.)

-Takahiro AKASHI


> For both of these my point was that generally there is some fragility in
> this area. While it's easy to say that breaking this would be someone
> else's fault, we can easily avoid the possibility of that happening, and
> avoid a set of problems trying to maintain backwards compatibility if
> there were a sensible change that happened to break things.
>
> Thanks,
> Mark.
>