Hello Michael,
On 09/11/23 17:44, Michael Ellerman wrote:
Hi Sourabh,
This seems like a good change to the design, but I'm confused by
some things, more below ...
Thanks.
Sourabh Jain <sourabhj...@linux.ibm.com> writes:
...
Table 1 below illustrates kernel's ability to collect dump if either the
first/crashed kernel or the second/fadump kernel does not have the
changes introduced here. Consider the 'old kernel' as the kernel without
this patch, and the 'new kernel' as the kernel with this patch included.
+----------+------------------------+------------------------+-------+
| scenario | first/crashed kernel | second/fadump kernel | Dump |
+----------+------------------------+------------------------+-------+
| 1 | old kernel | new kernel | Yes |
+----------+------------------------+------------------------+-------+
| 2 | new kernel | old kernel | No |
+----------+------------------------+------------------------+-------+
Table 1
Scenario 1:
-----------
Since the magic number of fadump header is updated, the second kernel
can differentiate the crashed kernel is of type 'new kernel' or
'old kernel' and act accordingly. In this scenario, since the crashed
kernel is of type 'old kernel,' the fadump kernel skips elfcorehdr
creation and uses the one prepared in the first kernel itself to collect
the dump.
Scenario 2:
-----------
Since 'old kernel' as the fadump kernel is NOT capable of processing
fadump header with updated magic number from 'new kernel' hence it
gracefully fails with the below error and dump collection fails in this
scenario.
[ 0.007365] rtas fadump: Crash info header is not valid.
Add a version field to the fadump_crash_info_header structure to avoid
the need to change its magic number in the future. Adding a version
field to the fadump header was one of the TODO items listed in
Documentation/powerpc/firmware-assisted-dump.rst.
This is a good analysis of the issues with different kernel versions,
and seems like an OK trade off, ie. that old kernels can't process dumps
from new kernels.
But do we actually support using different kernel versions for the
crash/dump kernel?
Because AFAICS the fadump_crash_info_header is not safe across kernel
versions, in its current form or with your changes.
Yeah, I was also under the impression that it is not supported, but I
was not aware
that the size of pt_regs and cpumask can change based on the configuration.
diff --git a/arch/powerpc/include/asm/fadump-internal.h
b/arch/powerpc/include/asm/fadump-internal.h
index 27f9e11eda28..7be3d8894520 100644
--- a/arch/powerpc/include/asm/fadump-internal.h
+++ b/arch/powerpc/include/asm/fadump-internal.h
@@ -42,7 +42,25 @@ static inline u64 fadump_str_to_u64(const char *str)
#define FADUMP_CPU_UNKNOWN (~((u32)0))
-#define FADUMP_CRASH_INFO_MAGIC fadump_str_to_u64("FADMPINF")
+/*
+ * The introduction of new fields in the fadump crash info header has
+ * led to a change in the magic key, from `FADMPINF` to `FADMPSIG`.
+ * This alteration ensures backward compatibility, enabling the kernel
+ * with the updated fadump crash info to handle kernel dumps from older
+ * kernels.
+ *
+ * To prevent the need for further changes to the magic number in the
+ * event of future modifications to the fadump header, a version field
+ * has been introduced to track the fadump crash info header version.
+ *
+ * Historically, there was no connection between the magic number and
+ * the fadump crash info header version. However, moving forward, the
+ * `FADMPINF` magic number in header will be treated as version 0, while
+ * the `FADMPSIG` magic number in header will include a version field to
+ * determine its version.
+ */
+#define FADUMP_CRASH_INFO_MAGIC fadump_str_to_u64("FADMPSIG")
+#define FADUMP_VERSION 1
/* fadump crash info structure */
struct fadump_crash_info_header {
@@ -51,6 +69,10 @@ struct fadump_crash_info_header {
struct fadump_crash_info_header {
u64 magic_number;
u64 elfcorehdr_addr;
u32 crashing_cpu;
struct pt_regs regs;
struct cpumask cpu_mask;
+ u32 version;
+ u64 elfcorehdr_size;
+ u64 vmcoreinfo_raddr;
+ u64 vmcoreinfo_size;
};
The reason I say it's not safe is because pt_regs and especially cpumask
can change size depending on the kernel configuration.
pt_regs probably doesn't change size in practice for common kernel
configurations, but some of the fields are under #ifdef.
cpumask on the other hand is directly controlled by CONFIG_NR_CPUS. So
if the first and second kernel have a different value for NR_CPUS they
will disagree on the size of the struct.
That has presumably worked OK so far because folks tend to use the same, or
similar kernels for the first/second kernel. And also the cpumask is the
last element of the struct, so a disagreement about it size doesn't
affect the location of other fields.
However with your new design the version field will move based on the
cpumask size, which will potentially break detection of the version by
the second kernel.
At least that's how it looks to me, I don't see any checks anywhere that
will save us, or did I miss something?
I agree with you. If the sizes of pt_regs and cpu_mask are different between
the first/crash kernel and the fadump/second kernel, the dump collection
might not work correctly.
Given that dump capture is not supported across kernel versions, I think
it is
better to keep new attributes introduced in this patch to struct
fadump_crash_info_header
before pt_regs and cpumask. For example:
/* fadump crash info structure */
struct fadump_crash_info_header {
u64 magic_number;
+ u32 version;
+ u64 vmcoreinfo_raddr;
+ u64 vmcoreinfo_size;
u64 elfcorehdr_addr;
+ u64 elfcorehdr_size;
u32 crashing_cpu;
struct pt_regs regs;
struct cpumask cpu_mask;
};
Kernel with this patch included will not process the dump if
fadump_crash_info_header
holds old magic number.
Something like:
if (fdh->magic_number == fadump_str_to_u64("FADMPINF")) {
pr_err("Incompatible fadump header, can't process the dump");
}
Does this approach look good to you?
Thanks,
Sourabh
