Re: [PATCH v3 1/3] kexec_load: Use new kexec flag for hotplug support

[Sourabh Jain <sourabhjain@xxxxxxxxxxxxx>]

Hello Baoquan,

On 03/07/24 10:01, Baoquan He wrote:
> On 07/02/24 at 10:00am, Sourabh Jain wrote:
> ......
> > diff --git a/kexec/arch/i386/kexec-x86.c b/kexec/arch/i386/kexec-x86.c
> > index 444cb69..b4947a0 100644
> > --- a/kexec/arch/i386/kexec-x86.c
> > +++ b/kexec/arch/i386/kexec-x86.c
> > @@ -208,3 +208,11 @@ void arch_update_purgatory(struct kexec_info *info)
> >   	elf_rel_set_symbol(&info->rhdr, "panic_kernel",
> >   		&panic_kernel, sizeof(panic_kernel));
> >   }
> > +
> > +int arch_do_exclude_segment(struct kexec_segment *seg_ptr, struct kexec_info *info)
> > +{
> > +	if (info->elfcorehdr == (unsigned long) seg_ptr->mem)
> > +		return 1;
> > +
> > +	return 0;
> > +}
> I know the similar question has been asked in earlier version, I may not
> get it. still raise concern here to ask why x86_64 returns 0 directly,
> while i386 will have the different cases.

Currently, info->elfcorehdr is only getting initialized in 
load_crashdump_segments() in
kexec/arch/i386/crashdump-x86.c. This led me think that elfcorehdr is 
only skipped
for i386 and NOT x86_64.

However, info->elfcorehdr is actually getting initialized for x86_64 
too. Because
load_crashdump_segments() defined under kexec/arch/i386/crashdump-x86.c 
is called for
both i386 and x86_64.

You and Hari are both right; the arch_do_exclude_segment() 
implementation should be
the same for both i386 and x86_64.

I will update the patch.

> > diff --git a/kexec/arch/ia64/kexec-ia64.c b/kexec/arch/ia64/kexec-ia64.c
> > index 418d997..8d9c1f3 100644
> > --- a/kexec/arch/ia64/kexec-ia64.c
> > +++ b/kexec/arch/ia64/kexec-ia64.c
> > @@ -245,3 +245,7 @@ void arch_update_purgatory(struct kexec_info *UNUSED(info))
> >   {
> >   }
> >   
> > +int arch_do_exclude_segment(struct kexec_segment *UNUSED(seg_ptr), struct kexec_info *UNUSED(info))
>                                                              ~~~~~~~
> A tiny nit about the parameter naming, since the existing is taking
> 'struct kexec_segment *segment', can we also take the same way? Not
> strong opinion.
>
> =====
> kexec-tools/kexec/kexec.c:
> static int valid_memory_segment(struct kexec_info *info,
>                                  struct kexec_segment *segment)
> {
>          unsigned long sstart, send;
>          sstart = (unsigned long)segment->mem;
>          send   = sstart + segment->memsz - 1;
>
>          return valid_memory_range(info, sstart, send);
> }

Sure, I will rename the parameter and reorder them just like in the 
above function.

> > +{
> > +	return 0;
> > +}
> > diff --git a/kexec/arch/loongarch/kexec-loongarch.c b/kexec/arch/loongarch/kexec-loongarch.c
> > index ac75030..ee7b9f1 100644
> > --- a/kexec/arch/loongarch/kexec-loongarch.c
> > +++ b/kexec/arch/loongarch/kexec-loongarch.c
> > @@ -381,3 +381,8 @@ unsigned long add_buffer(struct kexec_info *info, const void *buf,
> >   	return add_buffer_phys_virt(info, buf, bufsz, memsz, buf_align,
> >   				    buf_min, buf_max, buf_end, 1);
> >   }
> > +
> > +int arch_do_exclude_segment(struct kexec_segment *UNUSED(seg_ptr), struct kexec_info *UNUSED(info))
> > +{
> > +	return 0;
> > +}
> > diff --git a/kexec/arch/m68k/kexec-m68k.c b/kexec/arch/m68k/kexec-m68k.c
> > index cb54927..0c7dbaf 100644
> > --- a/kexec/arch/m68k/kexec-m68k.c
> > +++ b/kexec/arch/m68k/kexec-m68k.c
> > @@ -108,3 +108,8 @@ void add_segment(struct kexec_info *info, const void *buf, size_t bufsz,
> >   {
> >   	add_segment_phys_virt(info, buf, bufsz, base, memsz, 1);
> >   }
> > +
> > +int arch_do_exclude_segment(struct kexec_segment *UNUSED(seg_ptr), struct kexec_info *UNUSED(info))
> > +{
> > +	return 0;
> > +}
> > diff --git a/kexec/arch/mips/kexec-mips.c b/kexec/arch/mips/kexec-mips.c
> > index d8cbea8..94224ee 100644
> > --- a/kexec/arch/mips/kexec-mips.c
> > +++ b/kexec/arch/mips/kexec-mips.c
> > @@ -189,3 +189,7 @@ unsigned long add_buffer(struct kexec_info *info, const void *buf,
> >   				    buf_min, buf_max, buf_end, 1);
> >   }
> >   
> > +int arch_do_exclude_segment(const void *UNUSED(seg_ptr), struct kexec_info *UNUSED(info))
> > +{
> > +	return 0;
> > +}
> > diff --git a/kexec/arch/ppc/kexec-ppc.c b/kexec/arch/ppc/kexec-ppc.c
> > index 03bec36..c8af870 100644
> > --- a/kexec/arch/ppc/kexec-ppc.c
> > +++ b/kexec/arch/ppc/kexec-ppc.c
> > @@ -966,3 +966,7 @@ void arch_update_purgatory(struct kexec_info *UNUSED(info))
> >   {
> >   }
> >   
> > +int arch_do_exclude_segment(struct kexec_segment *UNUSED(seg_ptr), struct kexec_info *UNUSED(info))
> > +{
> > +	return 0;
> > +}
> > diff --git a/kexec/arch/ppc64/kexec-ppc64.c b/kexec/arch/ppc64/kexec-ppc64.c
> > index bd5274c..fb27b6b 100644
> > --- a/kexec/arch/ppc64/kexec-ppc64.c
> > +++ b/kexec/arch/ppc64/kexec-ppc64.c
> > @@ -967,3 +967,8 @@ int arch_compat_trampoline(struct kexec_info *UNUSED(info))
> >   void arch_update_purgatory(struct kexec_info *UNUSED(info))
> >   {
> >   }
> > +
> > +int arch_do_exclude_segment(struct kexec_segment *UNUSED(seg_ptr), struct kexec_info *UNUSED(info))
> > +{
> > +	return 0;
> > +}
> > diff --git a/kexec/arch/s390/kexec-s390.c b/kexec/arch/s390/kexec-s390.c
> > index 33ba6b9..0561ee7 100644
> > --- a/kexec/arch/s390/kexec-s390.c
> > +++ b/kexec/arch/s390/kexec-s390.c
> > @@ -267,3 +267,8 @@ int get_crash_kernel_load_range(uint64_t *start, uint64_t *end)
> >   {
> >   	return parse_iomem_single("Crash kernel\n", start, end);
> >   }
> > +
> > +int arch_do_exclude_segment(struct kexec_segment *UNUSED(seg_ptr), struct kexec_info *UNUSED(info))
> > +{
> > +	return 0;
> > +}
> > diff --git a/kexec/arch/sh/kexec-sh.c b/kexec/arch/sh/kexec-sh.c
> > index ce341c8..f84c40c 100644
> > --- a/kexec/arch/sh/kexec-sh.c
> > +++ b/kexec/arch/sh/kexec-sh.c
> > @@ -257,3 +257,8 @@ unsigned long add_buffer(struct kexec_info *info, const void *buf,
> >   	return add_buffer_phys_virt(info, buf, bufsz, memsz, buf_align,
> >   				    buf_min, buf_max, buf_end, 1);
> >   }
> > +
> > +int arch_do_exclude_segment(struct kexec_segment *UNUSED(seg_ptr), struct kexec_info *UNUSED(info))
> > +{
> > +	return 0;
> > +}
> > diff --git a/kexec/arch/x86_64/kexec-x86_64.c b/kexec/arch/x86_64/kexec-x86_64.c
> > index ffd84f0..42af90a 100644
> > --- a/kexec/arch/x86_64/kexec-x86_64.c
> > +++ b/kexec/arch/x86_64/kexec-x86_64.c
> > @@ -188,3 +188,8 @@ void arch_update_purgatory(struct kexec_info *info)
> >   	elf_rel_set_symbol(&info->rhdr, "panic_kernel",
> >   				&panic_kernel, sizeof(panic_kernel));
> >   }
> > +
> > +int arch_do_exclude_segment(struct kexec_segment *UNUSED(seg_ptr), struct kexec_info *UNUSED(info))
> > +{
> > +	return 0;
> > +}
> > diff --git a/kexec/kexec-syscall.h b/kexec/kexec-syscall.h
> > index 73e5254..4675c46 100644
> > --- a/kexec/kexec-syscall.h
> > +++ b/kexec/kexec-syscall.h
> > @@ -112,7 +112,7 @@ static inline long kexec_file_load(int kernel_fd, int initrd_fd,
> >   
> >   #define KEXEC_ON_CRASH		0x00000001
> >   #define KEXEC_PRESERVE_CONTEXT	0x00000002
> > -#define KEXEC_UPDATE_ELFCOREHDR	0x00000004
> > +#define KEXEC_CRASH_HOTPLUG_SUPPORT	0x00000008
> >   #define KEXEC_ARCH_MASK		0xffff0000
> Yeah, removing KEXEC_UPDATE_ELFCOREHDR sounds good to avoid collision
> with xen code.

Thanks for the review.

I will make the necessary changes as mentioned above and send v4.

Thanks,
Sourabh Jain

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