On Tue, 19 Dec 2023 11:50:32 +0800, fuqiang wang <fuqiang.wang@xxxxxxxxxxxx> wrote: > 在 2023/12/19 10:47, Yuntao Wang 写道: > > > Hi fuqiang, > > > > Yesterday, I posted two patches that happen to address the bugs you an Baoquan > > are currently discussing here, I wasn't aware that you both were also working > > on fixing these issues. > > > > Baoquan suggested I talk to you about it. > > > > If you're interested, you can take a look at my patches and review them to see > > if there are any issues. If everything is fine, and if you're willing, you can > > also add a 'Reviewed-by' tag there. > > > > The following link is for the two patches I posted yesterday: > > > > https://lore.kernel.org/lkml/20231218081915.24120-3-ytcoode@xxxxxxxxx/t/#u > > > > Sincerely, > > Yuntao > > Hi Yuntao, > > I'm glad you've also noticed this issue. But I'm sorry, I want to solve this > problem myself because this is my first time posting a patch in the community, > and I cherish this opportunity very much. I can truly understand your feelings because I still remember how thrilled I was when my first patch got merged. So keep it up! > > I have carefully reviewed your patch. There is some changes where my views differ > from yours: > diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c > index c92d88680dbf..3be46f4b441e 100644 > --- a/arch/x86/kernel/crash.c > +++ b/arch/x86/kernel/crash.c > @@ -282,10 +282,6 @@ int crash_setup_memmap_entries(struct kimage *image, struct boot_params *params) > struct crash_memmap_data cmd; > struct crash_mem *cmem; > > - cmem = vzalloc(struct_size(cmem, ranges, 1)); > - if (!cmem) > - return -ENOMEM; > - > memset(&cmd, 0, sizeof(struct crash_memmap_data)); > cmd.params = params; > > @@ -321,6 +317,11 @@ int crash_setup_memmap_entries(struct kimage *image, struct boot_params *params) > } > > /* Exclude some ranges from crashk_res and add rest to memmap */ > + cmem = vzalloc(struct_size(cmem, ranges, 1)); > + if (!cmem) > + return -ENOMEM; > + cmem->max_nr_ranges = 1; > + > ret = memmap_exclude_ranges(image, cmem, crashk_res.start, crashk_res.end); > if (ret) > goto out; > > 1. I don't feel very good that you have moved vzalloc() to in front of > memmap_exclude_ranges. Because if memory allocation fails, there is no need to > do anything else afterwards. I moved it here because only memmap_exclude_ranges() and the code below it use cmem. I think it is a good practice to put related code together, which also improves code readability. > > 2. The cmem->max_nr_ranges should be set to 2. Because in > memmap_exclude_ranges, a cmem->ranges[] will be filled in and if a split occurs > later, another one will be added. With the current code, image->elf_load_addr should be equal to crashk_res.start, so split will not occur in crash_exclude_mem_range(). Therefore, setting cmem->max_nr_ranges to 1 is safe. > > Thanks > fuqiang _______________________________________________ kexec mailing list kexec@xxxxxxxxxxxxxxxxxxx http://lists.infradead.org/mailman/listinfo/kexec
