Hello Michael,

On 22/11/23 18:22, Michael Ellerman wrote:

Sourabh Jain [off-list ref] writes:

quoted

On 22/11/23 10:47, Michael Ellerman wrote:

quoted

Aneesh Kumar K V [off-list ref] writes:
...

quoted

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?

How sure are we of that?

Don't we go through grub when we boot into the 2nd kernel. And so
couldn't it choose to boot a different kernel, for whatever reason.

I don't think we need to support different pt_reg / cpumask sizes, but
requiring the exact same kernel version is too strict I think.

Agree.

quoted

But maybe I'm wrong. Would be good to hear what distro folks think.

How about checking fadump crash info header compatibility in the
following way?

static bool is_fadump_header_compatible(struct fadump_crash_info_header
*fdh)
{
      if (fdh->magic_number == FADUMP_CRASH_INFO_MAGIC_OLD) {
          pr_err("Old magic number, can't process the dump.");
          return false;
      }

      if (fdh->magic_number != FADUMP_CRASH_INFO_MAGIC) {
          pr_err("Fadump header is corrupted.");
          return false;
      }

      /*
       * If the kernel version of the first/crashed kernel and the
second/fadump
       * kernel is not same, then only collect the dump if the size of all
       * non-primitive type members of the fadump header is the same
across kernels.
       */
      if (strcmp(fdh->kernel_version, init_uts_ns.name.release)) {

  
I don't think the kernel version check is necessary?

I didn't find a place where pt_regs members are accessed to take
a decision in fadump kernel. we just copy the pt_regs in fadump kernel.

So I think as long as size is same across kernels, we are good.

quoted

          if (fdh->pt_regs_sz != sizeof(struct pt_regs) || fdh->cpu_mask_sz != sizeof(struct cpumask)) {
              pr_err("Fadump header size mismatch.\n")
              return false;

Yeah I think that works.

quoted

          } else
              pr_warn("Kernel version mismatch; dump data is unreliable.\n");
      }

      return true;
}

And the new fadump crash info header will be: As suggested by Hari.

/* fadump crash info structure */
struct fadump_crash_info_header {
      u64        magic_number;
+  u32        version;

  
Do we need version? We're effectively using the magic_number as a
version already. Having an actual version would allow us to make
backward compatible changes in future, but it's not clear we'll need or
want to do that.

Agree that currently version field is not used but I added a version 
field to
make the fadump header structure compatible with future changes without
changing the magic number.

I will add a comment on how version field should be utilized if one 
changes fadump
header in future.

quoted

      u32        crashing_cpu;
      u64        elfcorehdr_addr;
+  u64        elfcorehdr_size;
+  u64        vmcoreinfo_raddr;
+  u64        vmcoreinfo_size;
+  u8          kernel_version[__NEW_UTS_LEN + 1];
+  u32        pt_regs_sz;
      struct pt_regs    regs;
+  u32        cpu_mask_sz;

  
Typically you would put all the size fields before the variable sized
fields, because otherwise the later size fields may not be where you
expect them to be. But because we're bailing out if any of the sizes
don't match it doesn't actually matter.

Yeah, but I will reorganize fadump header and put the size fields before the
variable sized fields.

quoted

      struct cpumask    cpu_mask;
};

The other issue is endian. I assume we're just declaring that the
1st/2nd kernel must be the same endian? We should still make that
explicit though.

A comment is fine or should we add a explicit check and error out
with relevant error message if endianness is not same across the
kernels?

Something like:

if (fdh->magic_number != FADUMP_CRASH_INFO_MAGIC) {
     if (fdh->magic_number == swab64(FADUMP_CRASH_INFO_MAGIC)) {
         pr_err("Endianness mismatch");
     } else {
         pr_err("Fadump header is corrupted.");
     }
     return false;
}

To make it clearer that this struct is somewhat an ABI, I think we
should move the definition into arch/powerpc/include/uapi/asm/fadump.h

Sure

We don't expect userspace to actually use the header, but it will
hopefully remind everyone that the struct needs to be changed with care :)

Agree

Thanks,
Sourabh Jain