[PATCH v5 1/3] powerpc: make fadump resilient with memory add/remove events

[Michael Ellerman]

Hi Sourabh,

This seems like a good change to the design, but I'm confused by
some things, more below ...

Sourabh Jain <sourabhjain at 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.

> 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?

cheers