On 01/26/22 at 11:32am, Eric DeVolder wrote:
..snip....
> > > > > +void arch_crash_hotplug_handler(struct kimage *image,
> > > > > + unsigned int hp_action, unsigned long a, unsigned long b)
> > > > > +{
> > > > > + /*
> > > > > + * To accurately reflect hot un/plug changes, the elfcorehdr (which
> > > > > + * is passed to the crash kernel via the elfcorehdr= parameter)
> > > > > + * must be updated with the new list of CPUs and memories. The new
> > > > > + * elfcorehdr is prepared in a kernel buffer, and if no errors,
> > > > > + * then it is written on top of the existing/old elfcorehdr.
> > > > > + *
> > > > > + * Due to the change to the elfcorehdr, purgatory must explicitly
> > > > > + * exclude the elfcorehdr from the list of segments it checks.
> > > > > + */
> > > >
> > > > Please move this code comment to above function as kernel-doc if you
> > > > this it benefits the entire function. Otherwise should move them above
> > > > the code block they are explaining. For this place, I think moving them
> > > > to above arch_crash_hotplug_handler() is better.
> > >
> > > ok, I will do that!
> > >
> > > >
> > > > > + struct kexec_segment *ksegment;
> > > > > + unsigned char *ptr = NULL;
> > > > > + unsigned long elfsz = 0;
> > > > > + void *elfbuf = NULL;
> > > > > + unsigned long mem, memsz;
> > > > > + unsigned int n;
> > > > > +
> > > > > + /*
> > > > > + * When the struct kimage is alloced, it is wiped to zero, so
> > > > > + * the elf_index_valid defaults to false. It is set on the
> > > > > + * kexec_file_load path, or here for kexec_load.
> > > > > + */
> > > >
> > > > I think this kexec loading part should be taken out and post after this
> > > > whole patchset being accepted. At least, it's worth to put them in a
> > > > separate patch.
> > >
> > > This little bit of code that identifies the incoming elfcorehdr is all that
> > > is needed to support kexec_load (and the userspace changes of course). I'm
> > > happy to split as a separate patch, but I would think that be maintaining it
> > > with this series, then when it is accepted, both the kexec_load and
> > > kexec_file_load paths would be supported? Your call.
> >
> > Hmm, at first, let's split it out from this patch since it's an
> > independent action to kdump. I would suggest we don't carry it in this
> > series. After this series is done, you can post another patchset
> > including this part as kernel patch, and also the code change in
> > kexec_tools as user space patch.
> >
> > ......
> >
>
> OK, I'll remove the bit of code that supports kexec_load, so it can be introduced
> later coincident with the changes to kexec-tools.
>
> In a previous message you mentioned making changes to the order of the patches,
> was this it, or is there more to come?
Yeah, replied to cover letter, please check it there.
_______________________________________________
kexec mailing list
kexec@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/kexec
