From: Thiago Jung Bauermann <bauerman@linux.ibm.com>
To: Hari Bathini <hbathini@linux.ibm.com>
Cc: Pingfan Liu <piliu@redhat.com>, Nayna Jain <nayna@linux.ibm.com>,
Kexec-ml <kexec@lists.infradead.org>,
Mahesh J Salgaonkar <mahesh@linux.ibm.com>,
Mimi Zohar <zohar@linux.ibm.com>,
lkml <linux-kernel@vger.kernel.org>,
linuxppc-dev <linuxppc-dev@ozlabs.org>,
Sourabh Jain <sourabhjain@linux.ibm.com>,
Petr Tesarik <ptesarik@suse.cz>,
Andrew Morton <akpm@linux-foundation.org>,
Dave Young <dyoung@redhat.com>, Vivek Goyal <vgoyal@redhat.com>,
Eric Biederman <ebiederm@xmission.com>
Subject: Re: [PATCH v3 03/12] powerpc/kexec_file: add helper functions for getting memory ranges
Date: Tue, 14 Jul 2020 20:49:31 -0300 [thread overview]
Message-ID: <874kq98xo4.fsf@morokweng.localdomain> (raw)
In-Reply-To: <159466087136.24747.16494497863685481495.stgit@hbathini.in.ibm.com>
Hello Hari,
Hari Bathini <hbathini@linux.ibm.com> writes:
> In kexec case, the kernel to be loaded uses the same memory layout as
> the running kernel. So, passing on the DT of the running kernel would
> be good enough.
>
> But in case of kdump, different memory ranges are needed to manage
> loading the kdump kernel, booting into it and exporting the elfcore
> of the crashing kernel. The ranges are exlude memory ranges, usable
s/exlude/exclude/
> memory ranges, reserved memory ranges and crash memory ranges.
>
> Exclude memory ranges specify the list of memory ranges to avoid while
> loading kdump segments. Usable memory ranges list the memory ranges
> that could be used for booting kdump kernel. Reserved memory ranges
> list the memory regions for the loading kernel's reserve map. Crash
> memory ranges list the memory ranges to be exported as the crashing
> kernel's elfcore.
>
> Add helper functions for setting up the above mentioned memory ranges.
> This helpers facilitate in understanding the subsequent changes better
> and make it easy to setup the different memory ranges listed above, as
> and when appropriate.
>
> Signed-off-by: Hari Bathini <hbathini@linux.ibm.com>
> Tested-by: Pingfan Liu <piliu@redhat.com>
<snip>
> +/**
> + * get_mem_rngs_size - Get the allocated size of mrngs based on
> + * max_nr_ranges and chunk size.
> + * @mrngs: Memory ranges.
> + *
> + * Returns the maximum no. of ranges.
This isn't correct. It returns the maximum size of @mrngs.
> + */
> +static inline size_t get_mem_rngs_size(struct crash_mem *mrngs)
> +{
> + size_t size;
> +
> + if (!mrngs)
> + return 0;
> +
> + size = (sizeof(struct crash_mem) +
> + (mrngs->max_nr_ranges * sizeof(struct crash_mem_range)));
> +
> + /*
> + * Memory is allocated in size multiple of MEM_RANGE_CHUNK_SZ.
> + * So, align to get the actual length.
> + */
> + return ALIGN(size, MEM_RANGE_CHUNK_SZ);
> +}
<snip>
> +/**
> + * add_tce_mem_ranges - Adds tce-table range to the given memory ranges list.
> + * @mem_ranges: Range list to add the memory range(s) to.
> + *
> + * Returns 0 on success, negative errno on error.
> + */
> +int add_tce_mem_ranges(struct crash_mem **mem_ranges)
> +{
> + struct device_node *dn;
> + int ret;
> +
> + for_each_node_by_type(dn, "pci") {
> + u64 base;
> + u32 size;
> +
> + ret = of_property_read_u64(dn, "linux,tce-base", &base);
> + ret |= of_property_read_u32(dn, "linux,tce-size", &size);
> + if (!ret)
Shouldn't the condition be `ret` instead of `!ret`?
> + continue;
> +
> + ret = add_mem_range(mem_ranges, base, size);
> + if (ret)
> + break;
> + }
> +
> + return ret;
> +}
> +
> +/**
> + * add_initrd_mem_range - Adds initrd range to the given memory ranges list,
> + * if the initrd was retained.
> + * @mem_ranges: Range list to add the memory range to.
> + *
> + * Returns 0 on success, negative errno on error.
> + */
> +int add_initrd_mem_range(struct crash_mem **mem_ranges)
> +{
> + u64 base, end;
> + int ret = 0;
> + char *str;
> +
> + /* This range means something only if initrd was retained */
> + str = strstr(saved_command_line, "retain_initrd");
> + if (!str)
> + return 0;
> +
> + ret = of_property_read_u64(of_chosen, "linux,initrd-start", &base);
> + ret |= of_property_read_u64(of_chosen, "linux,initrd-end", &end);
> + if (!ret)
> + ret = add_mem_range(mem_ranges, base, end - base + 1);
> + return ret;
> +}
> +
> +/**
> + * add_htab_mem_range - Adds htab range to the given memory ranges list,
> + * if it exists
> + * @mem_ranges: Range list to add the memory range to.
> + *
> + * Returns 0 on success, negative errno on error.
> + */
> +int add_htab_mem_range(struct crash_mem **mem_ranges)
> +{
> +#ifdef CONFIG_PPC_BOOK3S_64
> + int ret;
> +
> + if (!htab_address)
> + return 0;
> +
> + ret = add_mem_range(mem_ranges, __pa(htab_address), htab_size_bytes);
> + return ret;
> +#else
> + return 0;
> +#endif
> +}
If I'm not mistaken, this is not the preferred way of having alternative
implementations of a function. The "Conditional Compilation" section of
the coding style document doesn't mention this directly, but does say
that it's better to put the conditionals in a header file.
In this case, I would do this in <asm/kexec_ranges.h>
#ifdef CONFIG_PPC_BOOK3S_64
int add_htab_mem_range(struct crash_mem **mem_ranges);
#else
static inline int add_htab_mem_range(struct crash_mem **mem_ranges)
{
return 0;
}
#endif
And in ranges.c just surround the add_htab_mem_range() definition with
#ifdef CONFIG_PPC_BOOK3S_64 and #endif
Also, there's no need for the ret variable. You can just
`return add_mem_range(...)` directly.
> +
> +/**
> + * add_kernel_mem_range - Adds kernel text region to the given
> + * memory ranges list.
> + * @mem_ranges: Range list to add the memory range to.
> + *
> + * Returns 0 on success, negative errno on error.
> + */
> +int add_kernel_mem_range(struct crash_mem **mem_ranges)
> +{
> + int ret;
> +
> + ret = add_mem_range(mem_ranges, 0, __pa(_end));
> + return ret;
> +}
No need for the ret variable here, just `return add_mem_range()`
directly.
> +
> +/**
> + * add_rtas_mem_range - Adds RTAS region to the given memory ranges list.
> + * @mem_ranges: Range list to add the memory range to.
> + *
> + * Returns 0 on success, negative errno on error.
> + */
> +int add_rtas_mem_range(struct crash_mem **mem_ranges)
> +{
> + struct device_node *dn;
> + int ret = 0;
> +
> + dn = of_find_node_by_path("/rtas");
> + if (dn) {
> + u32 base, size;
> +
> + ret = of_property_read_u32(dn, "linux,rtas-base", &base);
> + ret |= of_property_read_u32(dn, "rtas-size", &size);
> + if (ret)
> + return ret;
> +
> + ret = add_mem_range(mem_ranges, base, size);
You're missing an of_node_put(dn) here (also in the early return in the
line above).
> + }
> + return ret;
> +}
> +
> +/**
> + * add_opal_mem_range - Adds OPAL region to the given memory ranges list.
> + * @mem_ranges: Range list to add the memory range to.
> + *
> + * Returns 0 on success, negative errno on error.
> + */
> +int add_opal_mem_range(struct crash_mem **mem_ranges)
> +{
> + struct device_node *dn;
> + int ret = 0;
> +
> + dn = of_find_node_by_path("/ibm,opal");
> + if (dn) {
> + u64 base, size;
> +
> + ret = of_property_read_u64(dn, "opal-base-address", &base);
> + ret |= of_property_read_u64(dn, "opal-runtime-size", &size);
> + if (ret)
> + return ret;
> +
> + ret = add_mem_range(mem_ranges, base, size);
You're missing an of_node_put(dn) here (also in the early return in the
line above).
> + }
> + return ret;
> +}
> +
> +/**
> + * add_reserved_ranges - Adds "/reserved-ranges" regions exported by f/w
> + * to the given memory ranges list.
> + * @mem_ranges: Range list to add the memory ranges to.
> + *
> + * Returns 0 on success, negative errno on error.
> + */
> +int add_reserved_ranges(struct crash_mem **mem_ranges)
> +{
> + int i, len, ret = 0;
> + const __be32 *prop;
> +
> + prop = of_get_property(of_root, "reserved-ranges", &len);
> + if (!prop)
> + return 0;
> +
> + /*
> + * Each reserved range is an (address,size) pair, 2 cells each,
> + * totalling 4 cells per range.
Can you assume that, or do you need to check the #address-cells and
#size-cells properties of the root node?
> + */
> + for (i = 0; i < len / (sizeof(*prop) * 4); i++) {
> + u64 base, size;
> +
> + base = of_read_number(prop + (i * 4) + 0, 2);
> + size = of_read_number(prop + (i * 4) + 2, 2);
> +
> + ret = add_mem_range(mem_ranges, base, size);
> + if (ret)
> + break;
> + }
> +
> + return ret;
> +}
> +
> +/**
> + * sort_memory_ranges - Sorts the given memory ranges list.
> + * @mem_ranges: Range list to sort.
> + * @merge: If true, merge the list after sorting.
> + *
> + * Returns nothing.
> + */
> +void sort_memory_ranges(struct crash_mem *mrngs, bool merge)
> +{
> + struct crash_mem_range *rngs;
> + struct crash_mem_range rng;
> + int i, j, idx;
> +
> + if (!mrngs)
> + return;
> +
> + /* Sort the ranges in-place */
> + rngs = &mrngs->ranges[0];
> + for (i = 0; i < mrngs->nr_ranges; i++) {
> + idx = i;
> + for (j = (i + 1); j < mrngs->nr_ranges; j++) {
> + if (rngs[idx].start > rngs[j].start)
> + idx = j;
> + }
> + if (idx != i) {
> + rng = rngs[idx];
> + rngs[idx] = rngs[i];
> + rngs[i] = rng;
> + }
> + }
Would it work using sort() from lib/sort.c here?
> +
> + if (merge)
> + __merge_memory_ranges(mrngs);
> +}
--
Thiago Jung Bauermann
IBM Linux Technology Center
next prev parent reply other threads:[~2020-07-14 23:52 UTC|newest]
Thread overview: 45+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-13 17:20 [PATCH v3 00/12] ppc64: enable kdump support for kexec_file_load syscall Hari Bathini
2020-07-13 17:20 ` [PATCH v3 01/12] kexec_file: allow archs to handle special regions while locating memory hole Hari Bathini
2020-07-14 21:00 ` Thiago Jung Bauermann
2020-07-13 17:21 ` [PATCH v3 02/12] powerpc/kexec_file: mark PPC64 specific code Hari Bathini
2020-07-16 1:49 ` Thiago Jung Bauermann
2020-07-17 4:46 ` Hari Bathini
2020-07-17 18:34 ` Thiago Jung Bauermann
2020-07-13 17:21 ` [PATCH v3 03/12] powerpc/kexec_file: add helper functions for getting memory ranges Hari Bathini
2020-07-14 23:49 ` Thiago Jung Bauermann [this message]
2020-07-16 21:08 ` Hari Bathini
2020-07-17 4:32 ` Hari Bathini
2020-07-17 20:00 ` Hari Bathini
2020-07-13 17:21 ` [PATCH v3 04/12] ppc64/kexec_file: avoid stomping memory used by special regions Hari Bathini
2020-07-15 2:39 ` Thiago Jung Bauermann
2020-07-16 5:58 ` Thiago Jung Bauermann
2020-07-16 21:09 ` Hari Bathini
2020-07-16 21:59 ` Thiago Jung Bauermann
2020-07-13 17:21 ` [PATCH v3 05/12] powerpc/drmem: make lmb walk a bit more flexible Hari Bathini
2020-07-15 3:50 ` Thiago Jung Bauermann
2020-07-16 21:09 ` Hari Bathini
2020-07-16 22:01 ` Thiago Jung Bauermann
2020-07-13 17:22 ` [PATCH v3 06/12] ppc64/kexec_file: restrict memory usage of kdump kernel Hari Bathini
2020-07-15 22:52 ` Thiago Jung Bauermann
2020-07-16 21:10 ` Hari Bathini
2020-07-16 22:03 ` Thiago Jung Bauermann
2020-07-17 4:17 ` Hari Bathini
2020-07-13 17:22 ` [PATCH v3 07/12] ppc64/kexec_file: add support to relocate purgatory Hari Bathini
2020-07-16 0:20 ` Thiago Jung Bauermann
2020-07-16 21:11 ` Hari Bathini
2020-07-16 22:12 ` Thiago Jung Bauermann
2020-07-13 17:22 ` [PATCH v3 08/12] ppc64/kexec_file: setup the stack for purgatory Hari Bathini
2020-07-16 0:35 ` Thiago Jung Bauermann
2020-07-16 1:40 ` Thiago Jung Bauermann
2020-07-13 17:22 ` [PATCH v3 09/12] ppc64/kexec_file: setup backup region for kdump kernel Hari Bathini
2020-07-16 1:38 ` Thiago Jung Bauermann
2020-07-16 21:10 ` Hari Bathini
2020-07-16 22:06 ` Thiago Jung Bauermann
2020-07-13 17:23 ` [PATCH v3 10/12] ppc64/kexec_file: prepare elfcore header for crashing kernel Hari Bathini
2020-07-16 2:22 ` Thiago Jung Bauermann
2020-07-16 21:07 ` Hari Bathini
2020-07-16 21:57 ` Thiago Jung Bauermann
2020-07-13 17:23 ` [PATCH v3 11/12] ppc64/kexec_file: add appropriate regions for memory reserve map Hari Bathini
2020-07-16 2:27 ` Thiago Jung Bauermann
2020-07-13 17:23 ` [PATCH v3 12/12] ppc64/kexec_file: fix kexec load failure with lack of memory hole Hari Bathini
2020-07-16 5:43 ` Thiago Jung Bauermann
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=874kq98xo4.fsf@morokweng.localdomain \
--to=bauerman@linux.ibm.com \
--cc=akpm@linux-foundation.org \
--cc=dyoung@redhat.com \
--cc=ebiederm@xmission.com \
--cc=hbathini@linux.ibm.com \
--cc=kexec@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linuxppc-dev@ozlabs.org \
--cc=mahesh@linux.ibm.com \
--cc=nayna@linux.ibm.com \
--cc=piliu@redhat.com \
--cc=ptesarik@suse.cz \
--cc=sourabhjain@linux.ibm.com \
--cc=vgoyal@redhat.com \
--cc=zohar@linux.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).