Re: [PATCH v2 6/6] crash: option to let arch decide mem range is usable

Sourabh Jain
2025-01-24T10:28:27Z

Hello Hari,

On 24/01/25 15:22, Hari Bathini wrote:
> Hi Sourabh,
>
> On 21/01/25 5:24 pm, Sourabh Jain wrote:
>> On PowerPC, the memory reserved for the crashkernel can contain
>> components like RTAS, TCE, OPAL, etc., which should be avoided when
>> loading kexec segments into crashkernel memory. Due to these special
>> components, PowerPC has its own set of functions to locate holes in the
>> crashkernel memory for loading kexec segments for kdump. However, for
>> loading kexec segments in the kexec case, PowerPC uses generic functions
>> to locate holes.
>>
>> So, let's use generic functions to locate memory holes for kdump on
>> PowerPC by adding an arch hook to handle such special regions while
>> loading kexec segments, and remove the PowerPC functions to locate
>> holes.
>>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: Baoquan he <bhe@redhat.com>
>> Cc: Hari Bathini <hbathini@linux.ibm.com>
>> Cc: Madhavan Srinivasan <maddy@linux.ibm.com>
>> Cc: Mahesh Salgaonkar <mahesh@linux.ibm.com>
>> Cc: Michael Ellerman <mpe@ellerman.id.au>
>> Cc: kexec@lists.infradead.org
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: Sourabh Jain <sourabhjain@linux.ibm.com>
>> ---
>>   arch/powerpc/include/asm/kexec.h  |   6 +-
>>   arch/powerpc/kexec/file_load_64.c | 259 ++----------------------------
>>   include/linux/kexec.h             |   9 ++
>>   kernel/kexec_file.c               |  12 ++
>>   4 files changed, 34 insertions(+), 252 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/kexec.h
>> b/arch/powerpc/include/asm/kexec.h
>> index 64741558071f..5e4680f9ff35 100644
>> --- a/arch/powerpc/include/asm/kexec.h
>> +++ b/arch/powerpc/include/asm/kexec.h
>> @@ -95,8 +95,10 @@ int arch_kexec_kernel_image_probe(struct kimage
>> *image, void *buf, unsigned long
>>   int arch_kimage_file_post_load_cleanup(struct kimage *image);
>>   #define arch_kimage_file_post_load_cleanup
>> arch_kimage_file_post_load_cleanup
>>   -int arch_kexec_locate_mem_hole(struct kexec_buf *kbuf);
>> -#define arch_kexec_locate_mem_hole arch_kexec_locate_mem_hole
>> +int arch_check_excluded_range(struct kimage *image, unsigned long
>> start,
>> +                  unsigned long end);
>> +#define arch_check_excluded_range  arch_check_excluded_range
>> +
>>     int load_crashdump_segments_ppc64(struct kimage *image,
>>                     struct kexec_buf *kbuf);
>> diff --git a/arch/powerpc/kexec/file_load_64.c
>> b/arch/powerpc/kexec/file_load_64.c
>> index dc65c1391157..e7ef8b2a2554 100644
>> --- a/arch/powerpc/kexec/file_load_64.c
>> +++ b/arch/powerpc/kexec/file_load_64.c
>> @@ -49,201 +49,18 @@ const struct kexec_file_ops * const
>> kexec_file_loaders[] = {
>>       NULL
>>   };
>>   -/**
>> - * __locate_mem_hole_top_down - Looks top down for a large enough
>> memory hole
>> - *                              in the memory regions between
>> buf_min & buf_max
>> - *                              for the buffer. If found, sets
>> kbuf->mem.
>> - * @kbuf:                       Buffer contents and memory parameters.
>> - * @buf_min:                    Minimum address for the buffer.
>> - * @buf_max:                    Maximum address for the buffer.
>> - *
>> - * Returns 0 on success, negative errno on error.
>> - */
>> -static int __locate_mem_hole_top_down(struct kexec_buf *kbuf,
>> -                      u64 buf_min, u64 buf_max)
>> -{
>> -    int ret = -EADDRNOTAVAIL;
>> -    phys_addr_t start, end;
>> -    u64 i;
>> -
>> -    for_each_mem_range_rev(i, &start, &end) {
>> -        /*
>> -         * memblock uses [start, end) convention while it is
>> -         * [start, end] here. Fix the off-by-one to have the
>> -         * same convention.
>> -         */
>> -        end -= 1;
>> -
>> -        if (start > buf_max)
>> -            continue;
>> -
>> -        /* Memory hole not found */
>> -        if (end < buf_min)
>> -            break;
>> -
>> -        /* Adjust memory region based on the given range */
>> -        if (start < buf_min)
>> -            start = buf_min;
>> -        if (end > buf_max)
>> -            end = buf_max;
>> -
>> -        start = ALIGN(start, kbuf->buf_align);
>> -        if (start < end && (end - start + 1) >= kbuf->memsz) {
>> -            /* Suitable memory range found. Set kbuf->mem */
>> -            kbuf->mem = ALIGN_DOWN(end - kbuf->memsz + 1,
>> -                           kbuf->buf_align);
>> -            ret = 0;
>> -            break;
>> -        }
>> -    }
>> -
>> -    return ret;
>> -}
>> -
>> -/**
>> - * locate_mem_hole_top_down_ppc64 - Skip special memory regions to
>> find a
>> - *                                  suitable buffer with top down
>> approach.
>> - * @kbuf:                           Buffer contents and memory
>> parameters.
>> - * @buf_min:                        Minimum address for the buffer.
>> - * @buf_max:                        Maximum address for the buffer.
>> - * @emem:                           Exclude memory ranges.
>> - *
>> - * Returns 0 on success, negative errno on error.
>> - */
>> -static int locate_mem_hole_top_down_ppc64(struct kexec_buf *kbuf,
>> -                      u64 buf_min, u64 buf_max,
>> -                      const struct crash_mem *emem)
>> +int arch_check_excluded_range(struct kimage *image, unsigned long
>> start,
>> +                  unsigned long end)
>>   {
>> -    int i, ret = 0, err = -EADDRNOTAVAIL;
>> -    u64 start, end, tmin, tmax;
>> -
>> -    tmax = buf_max;
>> -    for (i = (emem->nr_ranges - 1); i >= 0; i--) {
>> -        start = emem->ranges[i].start;
>> -        end = emem->ranges[i].end;
>> -
>> -        if (start > tmax)
>> -            continue;
>> -
>> -        if (end < tmax) {
>> -            tmin = (end < buf_min ? buf_min : end + 1);
>> -            ret = __locate_mem_hole_top_down(kbuf, tmin, tmax);
>> -            if (!ret)
>> -                return 0;
>> -        }
>> -
>> -        tmax = start - 1;
>> -
>> -        if (tmax < buf_min) {
>> -            ret = err;
>> -            break;
>> -        }
>> -        ret = 0;
>> -    }
>> -
>> -    if (!ret) {
>> -        tmin = buf_min;
>> -        ret = __locate_mem_hole_top_down(kbuf, tmin, tmax);
>> -    }
>> -    return ret;
>> -}
>> -
>> -/**
>> - * __locate_mem_hole_bottom_up - Looks bottom up for a large enough
>> memory hole
>> - *                               in the memory regions between
>> buf_min & buf_max
>> - *                               for the buffer. If found, sets
>> kbuf->mem.
>> - * @kbuf:                        Buffer contents and memory parameters.
>> - * @buf_min:                     Minimum address for the buffer.
>> - * @buf_max:                     Maximum address for the buffer.
>> - *
>> - * Returns 0 on success, negative errno on error.
>> - */
>> -static int __locate_mem_hole_bottom_up(struct kexec_buf *kbuf,
>> -                       u64 buf_min, u64 buf_max)
>> -{
>> -    int ret = -EADDRNOTAVAIL;
>> -    phys_addr_t start, end;
>> -    u64 i;
>> -
>> -    for_each_mem_range(i, &start, &end) {
>> -        /*
>> -         * memblock uses [start, end) convention while it is
>> -         * [start, end] here. Fix the off-by-one to have the
>> -         * same convention.
>> -         */
>> -        end -= 1;
>> -
>> -        if (end < buf_min)
>> -            continue;
>> -
>> -        /* Memory hole not found */
>> -        if (start > buf_max)
>> -            break;
>> -
>> -        /* Adjust memory region based on the given range */
>> -        if (start < buf_min)
>> -            start = buf_min;
>> -        if (end > buf_max)
>> -            end = buf_max;
>> -
>> -        start = ALIGN(start, kbuf->buf_align);
>> -        if (start < end && (end - start + 1) >= kbuf->memsz) {
>> -            /* Suitable memory range found. Set kbuf->mem */
>> -            kbuf->mem = start;
>> -            ret = 0;
>> -            break;
>> -        }
>> -    }
>> -
>> -    return ret;
>> -}
>> -
>> -/**
>> - * locate_mem_hole_bottom_up_ppc64 - Skip special memory regions to
>> find a
>> - *                                   suitable buffer with bottom up
>> approach.
>> - * @kbuf:                            Buffer contents and memory
>> parameters.
>> - * @buf_min:                         Minimum address for the buffer.
>> - * @buf_max:                         Maximum address for the buffer.
>> - * @emem:                            Exclude memory ranges.
>> - *
>> - * Returns 0 on success, negative errno on error.
>> - */
>> -static int locate_mem_hole_bottom_up_ppc64(struct kexec_buf *kbuf,
>> -                       u64 buf_min, u64 buf_max,
>> -                       const struct crash_mem *emem)
>> -{
>> -    int i, ret = 0, err = -EADDRNOTAVAIL;
>> -    u64 start, end, tmin, tmax;
>> -
>> -    tmin = buf_min;
>> -    for (i = 0; i < emem->nr_ranges; i++) {
>> -        start = emem->ranges[i].start;
>> -        end = emem->ranges[i].end;
>> -
>> -        if (end < tmin)
>> -            continue;
>> -
>> -        if (start > tmin) {
>> -            tmax = (start > buf_max ? buf_max : start - 1);
>> -            ret = __locate_mem_hole_bottom_up(kbuf, tmin, tmax);
>> -            if (!ret)
>> -                return 0;
>> -        }
>> -
>> -        tmin = end + 1;
>> +    struct crash_mem *emem;
>> +    int i;
>>   -        if (tmin > buf_max) {
>> -            ret = err;
>> -            break;
>> -        }
>> -        ret = 0;
>> -    }
>> +    emem = image->arch.exclude_ranges;
>> +    for (i = 0; i < emem->nr_ranges; i++)
>> +        if (start < emem->ranges[i].end && end > emem->ranges[i].start)
>> +            return 1;
>>   -    if (!ret) {
>> -        tmax = buf_max;
>> -        ret = __locate_mem_hole_bottom_up(kbuf, tmin, tmax);
>> -    }
>> -    return ret;
>> +    return 0;
>>   }
>>     #ifdef CONFIG_CRASH_DUMP
>> @@ -1004,64 +821,6 @@ int setup_new_fdt_ppc64(const struct kimage
>> *image, void *fdt, struct crash_mem
>>       return ret;
>>   }
>>   -/**
>> - * arch_kexec_locate_mem_hole - Skip special memory regions like
>> rtas, opal,
>> - *                              tce-table, reserved-ranges & such
>> (exclude
>> - *                              memory ranges) as they can't be used
>> for kexec
>> - *                              segment buffer. Sets kbuf->mem when
>> a suitable
>> - *                              memory hole is found.
>> - * @kbuf:                       Buffer contents and memory parameters.
>> - *
>> - * Assumes minimum of PAGE_SIZE alignment for kbuf->memsz &
>> kbuf->buf_align.
>> - *
>> - * Returns 0 on success, negative errno on error.
>> - */
>> -int arch_kexec_locate_mem_hole(struct kexec_buf *kbuf)
>> -{
>> -    struct crash_mem **emem;
>> -    u64 buf_min, buf_max;
>> -    int ret;
>> -
>> -    /* Look up the exclude ranges list while locating the memory
>> hole */
>> -    emem = &(kbuf->image->arch.exclude_ranges);
>> -    if (!(*emem) || ((*emem)->nr_ranges == 0)) {
>> -        pr_warn("No exclude range list. Using the default locate mem
>> hole method\n");
>> -        return kexec_locate_mem_hole(kbuf);
>> -    }
>> -
>> -    buf_min = kbuf->buf_min;
>> -    buf_max = kbuf->buf_max;
>> -    /* Segments for kdump kernel should be within crashkernel region */
>> -    if (IS_ENABLED(CONFIG_CRASH_DUMP) && kbuf->image->type ==
>> KEXEC_TYPE_CRASH) {
>> -        buf_min = (buf_min < crashk_res.start ?
>> -               crashk_res.start : buf_min);
>> -        buf_max = (buf_max > crashk_res.end ?
>> -               crashk_res.end : buf_max);
>> -    }
>> -
>> -    if (buf_min > buf_max) {
>> -        pr_err("Invalid buffer min and/or max values\n");
>> -        return -EINVAL;
>> -    }
>> -
>> -    if (kbuf->top_down)
>> -        ret = locate_mem_hole_top_down_ppc64(kbuf, buf_min, buf_max,
>> -                             *emem);
>> -    else
>> -        ret = locate_mem_hole_bottom_up_ppc64(kbuf, buf_min, buf_max,
>> -                              *emem);
>> -
>> -    /* Add the buffer allocated to the exclude list for the next
>> lookup */
>> -    if (!ret) {
>> -        add_mem_range(emem, kbuf->mem, kbuf->memsz);
>> -        sort_memory_ranges(*emem, true);
>> -    } else {
>> -        pr_err("Failed to locate memory buffer of size %lu\n",
>> -               kbuf->memsz);
>> -    }
>> -    return ret;
>> -}
>> -
>>   /**
>>    * arch_kexec_kernel_image_probe - Does additional handling needed
>> to setup
>>    *                                 kexec segments.
>
>
>> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
>> index f0e9f8eda7a3..407f8b0346aa 100644
>> --- a/include/linux/kexec.h
>> +++ b/include/linux/kexec.h
>> @@ -205,6 +205,15 @@ static inline int
>> arch_kimage_file_post_load_cleanup(struct kimage *image)
>>   }
>>   #endif
>>   +#ifndef arch_check_excluded_range
>> +static inline int arch_check_excluded_range(struct kimage *image,
>> +                        unsigned long start,
>> +                        unsigned long end)
>> +{
>> +    return 0;
>> +}
>> +#endif
>> +
>>   #ifdef CONFIG_KEXEC_SIG
>>   #ifdef CONFIG_SIGNED_PE_FILE_VERIFICATION
>>   int kexec_kernel_verify_pe_sig(const char *kernel, unsigned long
>> kernel_len);
>> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
>> index 3eedb8c226ad..fba686487e3b 100644
>> --- a/kernel/kexec_file.c
>> +++ b/kernel/kexec_file.c
>> @@ -464,6 +464,12 @@ static int locate_mem_hole_top_down(unsigned
>> long start, unsigned long end,
>>               continue;
>>           }
>>   +        /* Make sure this does not conflict with exclude range */
>> +        if (arch_check_excluded_range(image, temp_start, temp_end)) {
>> +            temp_start = temp_start - PAGE_SIZE;
>> +            continue;
>> +        }
>> +
>>           /* We found a suitable memory range */
>>           break;
>>       } while (1);
>> @@ -498,6 +504,12 @@ static int locate_mem_hole_bottom_up(unsigned
>> long start, unsigned long end,
>>               continue;
>>           }
>>   +        /* Make sure this does not conflict with exclude range */
>> +        if (arch_check_excluded_range(image, temp_start, temp_end)) {
>> +            temp_start = temp_start + PAGE_SIZE;
>> +            continue;
>> +        }
>> +
>>           /* We found a suitable memory range */
>>           break;
>>       } while (1);
>
> Please split this arch-independent patch and have it as a preceding
> patch. Arch-specific changes can go in a separate patch.

Yes, I will split this patch in the next version.

Thanks,
Sourabh Jain