On 11/17/23 10:03 AM, Sourabh Jain wrote:

Hi Aneesh,

Thanks for reviewing the patch.

On 15/11/23 10:14, Aneesh Kumar K.V wrote:

quoted

Sourabh Jain[off-list ref]  writes:

....

quoted

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

Can we keep the old magic details as

#define FADUMP_CRASH_INFO_MAGIC_OLD        fadump_str_to_u64("FADMPINF")
#define FADUMP_CRASH_INFO_MAGIC        fadump_str_to_u64("FADMPSIG")

Sure.

quoted

Also considering the struct need not be backward compatible, can we just
do

struct fadump_crash_info_header {
    u64        magic_number;
    u32        crashing_cpu;
    u64        elfcorehdr_addr;
    u64        elfcorehdr_size;
    u64        vmcoreinfo_raddr;
    u64        vmcoreinfo_size;
    struct pt_regs    regs;
    struct cpumask    cpu_mask;
};
static inline bool fadump_compatible(struct fadump_crash_info_header
*fdh)
{
    return (fdh->magic_number == FADUMP_CRASH_INFO_MAGIC)
}

and fail fadump if we find it not compatible?

Agree that it is unsafe to collect a dump with an incompatible fadump crash info header.

Given that I am updating the fadump crash info header, we can make a few arrangements
like adding a size filed for the dynamic size attribute like pt_regs and cpumask to ensure
better compatibility in the future.

Additionally, let's introduce a version field to the fadump crash info header to avoid changing
the magic number in the future.

I am not sure whether we need to add all the complexity to enable supporting different fadump kernel
version. Is that even a possible use case with fadump? Can't we always assume that with fadump the
crash kernel and fadump kernel will be same version? if yes we can simply fail with a magic number
mismatch because that indicates an user config error? 

-aneesh