Re: [PATCH v14 3/6] crash: add a new kexec flag for FDT update

[Baoquan He <bhe@redhat.com>]

On 12/15/23 at 12:17pm, Sourabh Jain wrote:
...... 
> > > diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> > > index 0f6ea35879ee..bcedb7625b1f 100644
> > > --- a/include/linux/kexec.h
> > > +++ b/include/linux/kexec.h
> > > @@ -319,6 +319,7 @@ struct kimage {
> > >   #ifdef CONFIG_CRASH_HOTPLUG
> > >   	/* If set, allow changes to elfcorehdr of kexec_load'd image */
> > >   	unsigned int update_elfcorehdr:1;
> > > +	unsigned int update_fdt:1;
> > Can we unify this to one flag, e.g hotplug_update?
> > 
> > With this, on x86_64, we will skip the sha calculation for elfcorehdr.
> > On ppc, we will skip the sha calculation for elfcorehdr and fdt.
> Yeah, that's what I suggested to Eric. I can do that, but I see one
> problem with powerpc or other platforms that need to skip SHA
> for more kexec segments in addition to elfcorehdr.
> 
> `update_elfcorehdr` is set when the kexec tool sends the
> `KEXEC_UPDATE_ELFCOREHDR`
> flag to the kernel for the `kexec_load` system call.
> 
> Given that the kexec tool has already been updated to send the
> `KEXEC_UPDATE_ELFCOREHDR` flag only when elfcorehdr is skipped from
> SHA verification in generic code, now it would be tricky for architectures
> to
> determine whether kexec has skipped SHA verification for just elfcorehdr
> or all segments needed on the platform with the same flag.

In kexec-tools, it's judged by do_hotplug to skip the elfcorehdr
segment. I am wondering how you skip the fdt segment when calculating
and verifying sha, only saw the update_fdt mark.

About the existing KEXEC_UPDATE_ELFCOREHDR, we only rename the macro,
but still use the same value, could you think of what problem could be
caused between kernel and kexec-tools utility, the old and new version
compatibility?

If it's about the new header files installed on older kernel, we can
change it like below? Fortunately only one release, 6.6 passed.

diff --git a/include/uapi/linux/kexec.h b/include/uapi/linux/kexec.h
index 3d5b3d757bed..df6a6505e267 100644
--- a/include/uapi/linux/kexec.h
+++ b/include/uapi/linux/kexec.h
@@ -13,7 +13,7 @@
 #define KEXEC_ON_CRASH         0x00000001
 #define KEXEC_PRESERVE_CONTEXT 0x00000002
-#define KEXEC_UPDATE_FDT       0x00000008
+#define KEXEC_CRASH_HOTPLUG_UPDATE 0x00000004
 #define KEXEC_UPDATE_ELFCOREHDR        0x00000004
 #define KEXEC_ARCH_MASK                0xffff0000
 
 /*

With my understanding, the kexec flag should be indicating the action,
the mem/cpu hotplug, but not relating to any detail. Imagine later
another segment need be skipped on one ARCH again, then another flag
need be added, this sounds not reasonable.

> 
> Code snippet from the kexec tool:
> 
> main() {
>     ...
>     /* NOTE: Xen KEXEC_LIVE_UPDATE and KEXEC_UPDATE_ELFCOREHDR collide */
>     if (do_hotplug) {
>         ...
> 
>         /* Indicate to the kernel it is ok to modify the elfcorehdr */
>         kexec_flags |= KEXEC_UPDATE_ELFCOREHDR;
>     }
>     ...
> }
> 
> Any suggestion how to handle this with just one kexec flag?
> 
> Thanks for the review.
> 
> Thanks,
> Sourabh Jain
> 
> > 
> > >   #endif
> > >   #ifdef ARCH_HAS_KIMAGE_ARCH
> > > @@ -396,9 +397,10 @@ bool kexec_load_permitted(int kexec_image_type);
> > >   /* List of defined/legal kexec flags */
> > >   #ifndef CONFIG_KEXEC_JUMP
> > > -#define KEXEC_FLAGS    (KEXEC_ON_CRASH | KEXEC_UPDATE_ELFCOREHDR)
> > > +#define KEXEC_FLAGS    (KEXEC_ON_CRASH | KEXEC_UPDATE_ELFCOREHDR | KEXEC_UPDATE_FDT)
> > >   #else
> > > -#define KEXEC_FLAGS    (KEXEC_ON_CRASH | KEXEC_PRESERVE_CONTEXT | KEXEC_UPDATE_ELFCOREHDR)
> > > +#define KEXEC_FLAGS    (KEXEC_ON_CRASH | KEXEC_PRESERVE_CONTEXT | KEXEC_UPDATE_ELFCOREHDR | \
> > > +			KEXEC_UPDATE_FDT)
> > >   #endif
> > >   /* List of defined/legal kexec file flags */
> > > diff --git a/include/uapi/linux/kexec.h b/include/uapi/linux/kexec.h
> > > index 01766dd839b0..3d5b3d757bed 100644
> > > --- a/include/uapi/linux/kexec.h
> > > +++ b/include/uapi/linux/kexec.h
> > > @@ -13,6 +13,7 @@
> > >   #define KEXEC_ON_CRASH		0x00000001
> > >   #define KEXEC_PRESERVE_CONTEXT	0x00000002
> > >   #define KEXEC_UPDATE_ELFCOREHDR	0x00000004
> > > +#define KEXEC_UPDATE_FDT	0x00000008
> > >   #define KEXEC_ARCH_MASK		0xffff0000
> > >   /*
> > > diff --git a/kernel/kexec.c b/kernel/kexec.c
> > > index 8f35a5a42af8..97eb151cd931 100644
> > > --- a/kernel/kexec.c
> > > +++ b/kernel/kexec.c
> > > @@ -132,6 +132,8 @@ static int do_kexec_load(unsigned long entry, unsigned long nr_segments,
> > >   #ifdef CONFIG_CRASH_HOTPLUG
> > >   	if (flags & KEXEC_UPDATE_ELFCOREHDR)
> > >   		image->update_elfcorehdr = 1;
> > > +	if (flags & KEXEC_UPDATE_FDT)
> > > +		image->update_fdt = 1;
> > >   #endif
> > >   	ret = machine_kexec_prepare(image);
> > > -- 
> > > 2.41.0
> > > 
> 


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec