Re: [PATCH v17 02/10] of: Add a common kexec FDT setup function
On Tue, Feb 09, 2021 at 10:21:52AM -0800, Lakshmi Ramasubramanian wrote:
> From: Rob Herring
>
> Both arm64 and powerpc do essentially the same FDT /chosen setup for
> kexec. The differences are either omissions that arm64 should have
> or additional properties that will be ignored. The setup code can be
> combined and shared by both powerpc and arm64.
>
> The differences relative to the arm64 version:
> - If /chosen doesn't exist, it will be created (should never happen).
> - Any old dtb and initrd reserved memory will be released.
> - The new initrd and elfcorehdr are marked reserved.
> - "linux,booted-from-kexec" is set.
>
> The differences relative to the powerpc version:
> - "kaslr-seed" and "rng-seed" may be set.
> - "linux,elfcorehdr" is set.
> - Any existing "linux,usable-memory-range" is removed.
>
> Combine the code for setting up the /chosen node in the FDT and updating
> the memory reservation for kexec, for powerpc and arm64, in
> of_kexec_alloc_and_setup_fdt() and move it to "drivers/of/kexec.c".
>
> Signed-off-by: Rob Herring
> Signed-off-by: Lakshmi Ramasubramanian
> ---
> drivers/of/Makefile | 6 ++
> drivers/of/kexec.c | 258
> include/linux/of.h | 13 +++
> 3 files changed, 277 insertions(+)
> create mode 100644 drivers/of/kexec.c
>
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index 6e1e5212f058..c13b982084a3 100644
> --- a/drivers/of/Makefile
> +++ b/drivers/of/Makefile
> @@ -14,4 +14,10 @@ obj-$(CONFIG_OF_RESOLVE) += resolver.o
> obj-$(CONFIG_OF_OVERLAY) += overlay.o
> obj-$(CONFIG_OF_NUMA) += of_numa.o
>
> +ifdef CONFIG_KEXEC_FILE
> +ifdef CONFIG_OF_FLATTREE
> +obj-y+= kexec.o
> +endif
> +endif
> +
> obj-$(CONFIG_OF_UNITTEST) += unittest-data/
> diff --git a/drivers/of/kexec.c b/drivers/of/kexec.c
> new file mode 100644
> index ..469e09613cdd
> --- /dev/null
> +++ b/drivers/of/kexec.c
> @@ -0,0 +1,258 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2020 Arm Limited
> + *
> + * Based on arch/arm64/kernel/machine_kexec_file.c:
> + * Copyright (C) 2018 Linaro Limited
> + *
> + * And arch/powerpc/kexec/file_load.c:
> + * Copyright (C) 2016 IBM Corporation
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +/* relevant device tree properties */
> +#define FDT_PROP_KEXEC_ELFHDR"linux,elfcorehdr"
> +#define FDT_PROP_MEM_RANGE "linux,usable-memory-range"
> +#define FDT_PROP_INITRD_START"linux,initrd-start"
> +#define FDT_PROP_INITRD_END "linux,initrd-end"
> +#define FDT_PROP_BOOTARGS"bootargs"
> +#define FDT_PROP_KASLR_SEED "kaslr-seed"
> +#define FDT_PROP_RNG_SEED"rng-seed"
> +#define RNG_SEED_SIZE128
> +
> +/**
> + * fdt_find_and_del_mem_rsv - delete memory reservation with given address
> and size
> + *
> + * @fdt: Flattened device tree for the current kernel.
> + * @start: Starting address of the reserved memory.
> + * @size:Size of the reserved memory.
> + *
> + * Return: 0 on success, or negative errno on error.
> + */
> +static int fdt_find_and_del_mem_rsv(void *fdt, unsigned long start, unsigned
> long size)
> +{
> + int i, ret, num_rsvs = fdt_num_mem_rsv(fdt);
> +
> + for (i = 0; i < num_rsvs; i++) {
> + u64 rsv_start, rsv_size;
> +
> + ret = fdt_get_mem_rsv(fdt, i, &rsv_start, &rsv_size);
> + if (ret) {
> + pr_err("Malformed device tree.\n");
> + return -EINVAL;
> + }
> +
> + if (rsv_start == start && rsv_size == size) {
> + ret = fdt_del_mem_rsv(fdt, i);
> + if (ret) {
> + pr_err("Error deleting device tree
> reservation.\n");
> + return -EINVAL;
> + }
> +
> + return 0;
> + }
> + }
> +
> + return -ENOENT;
> +}
> +
> +/*
> + * of_kexec_alloc_and_setup_fdt - Alloc and setup a new Flattened Device Tree
> + *
> + * @image: kexec image being loaded.
> + * @initrd_load_addr:Address where the next initrd will be loaded.
> + * @initrd_len: Size of the next initrd, or 0 if there will be
> none.
> + * @cmdline: Command line for the next kernel, or NULL if there will
> + * be none.
> + *
> + * Return: fdt on success, or NULL errno on error.
> + */
> +void *of_kexec_alloc_and_setup_fdt(const struct kimage *image,
> +unsigned long initrd_load_addr,
> +unsigned long initrd_len,
> +const char *cmdline)
> +{
> + void *fdt;
> + int ret, chosen_node;
> + const void *prop;
> + unsigned long fdt_size;
> +
> + fdt_size = fdt_totalsize(initial_boot_params) +
> +(cmdline ? strlen(cmdline) : 0) +
> +
Re: [PATCH v17 02/10] of: Add a common kexec FDT setup function
On 2/10/21 9:23 AM, Rob Herring wrote: On Tue, Feb 09, 2021 at 10:21:52AM -0800, Lakshmi Ramasubramanian wrote: From: Rob Herring Both arm64 and powerpc do essentially the same FDT /chosen setup for kexec. The differences are either omissions that arm64 should have or additional properties that will be ignored. The setup code can be combined and shared by both powerpc and arm64. The differences relative to the arm64 version: - If /chosen doesn't exist, it will be created (should never happen). - Any old dtb and initrd reserved memory will be released. - The new initrd and elfcorehdr are marked reserved. - "linux,booted-from-kexec" is set. The differences relative to the powerpc version: - "kaslr-seed" and "rng-seed" may be set. - "linux,elfcorehdr" is set. - Any existing "linux,usable-memory-range" is removed. Combine the code for setting up the /chosen node in the FDT and updating the memory reservation for kexec, for powerpc and arm64, in of_kexec_alloc_and_setup_fdt() and move it to "drivers/of/kexec.c". Signed-off-by: Rob Herring Signed-off-by: Lakshmi Ramasubramanian --- drivers/of/Makefile | 6 ++ drivers/of/kexec.c | 258 include/linux/of.h | 13 +++ 3 files changed, 277 insertions(+) create mode 100644 drivers/of/kexec.c diff --git a/include/linux/of.h b/include/linux/of.h index 4b27c9a27df3..f0eff5e84353 100644 --- a/include/linux/of.h +++ b/include/linux/of.h @@ -560,6 +560,19 @@ int of_map_id(struct device_node *np, u32 id, phys_addr_t of_dma_get_max_cpu_address(struct device_node *np); +/* + * Additional space needed for the buffer to build the new FDT + * so that we can add initrd, bootargs, kaslr-seed, rng-seed, + * userable-memory-range and elfcorehdr. + */ +#define FDT_EXTRA_SPACE 0x1000 No need for this to be public now. Move it to of/kexec.c. Will do. -lakshmi
Re: [PATCH v17 02/10] of: Add a common kexec FDT setup function
Lakshmi Ramasubramanian writes: > From: Rob Herring > > Both arm64 and powerpc do essentially the same FDT /chosen setup for > kexec. The differences are either omissions that arm64 should have > or additional properties that will be ignored. The setup code can be > combined and shared by both powerpc and arm64. > > The differences relative to the arm64 version: > - If /chosen doesn't exist, it will be created (should never happen). > - Any old dtb and initrd reserved memory will be released. > - The new initrd and elfcorehdr are marked reserved. > - "linux,booted-from-kexec" is set. > > The differences relative to the powerpc version: > - "kaslr-seed" and "rng-seed" may be set. > - "linux,elfcorehdr" is set. > - Any existing "linux,usable-memory-range" is removed. > > Combine the code for setting up the /chosen node in the FDT and updating > the memory reservation for kexec, for powerpc and arm64, in > of_kexec_alloc_and_setup_fdt() and move it to "drivers/of/kexec.c". > > Signed-off-by: Rob Herring > Signed-off-by: Lakshmi Ramasubramanian > --- > drivers/of/Makefile | 6 ++ > drivers/of/kexec.c | 258 > include/linux/of.h | 13 +++ > 3 files changed, 277 insertions(+) > create mode 100644 drivers/of/kexec.c Reviewed-by: Thiago Jung Bauermann -- Thiago Jung Bauermann IBM Linux Technology Center
Re: [PATCH v17 02/10] of: Add a common kexec FDT setup function
There's actually a complication that I just noticed and needs to be
addressed. More below.
Lakshmi Ramasubramanian writes:
> From: Rob Herring
>
> Both arm64 and powerpc do essentially the same FDT /chosen setup for
> kexec. The differences are either omissions that arm64 should have
> or additional properties that will be ignored. The setup code can be
> combined and shared by both powerpc and arm64.
>
> The differences relative to the arm64 version:
> - If /chosen doesn't exist, it will be created (should never happen).
> - Any old dtb and initrd reserved memory will be released.
> - The new initrd and elfcorehdr are marked reserved.
> - "linux,booted-from-kexec" is set.
>
> The differences relative to the powerpc version:
> - "kaslr-seed" and "rng-seed" may be set.
> - "linux,elfcorehdr" is set.
> - Any existing "linux,usable-memory-range" is removed.
>
> Combine the code for setting up the /chosen node in the FDT and updating
> the memory reservation for kexec, for powerpc and arm64, in
> of_kexec_alloc_and_setup_fdt() and move it to "drivers/of/kexec.c".
>
> Signed-off-by: Rob Herring
> Signed-off-by: Lakshmi Ramasubramanian
> ---
> drivers/of/Makefile | 6 ++
> drivers/of/kexec.c | 258
> include/linux/of.h | 13 +++
> 3 files changed, 277 insertions(+)
> create mode 100644 drivers/of/kexec.c
>
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index 6e1e5212f058..c13b982084a3 100644
> --- a/drivers/of/Makefile
> +++ b/drivers/of/Makefile
> @@ -14,4 +14,10 @@ obj-$(CONFIG_OF_RESOLVE) += resolver.o
> obj-$(CONFIG_OF_OVERLAY) += overlay.o
> obj-$(CONFIG_OF_NUMA) += of_numa.o
>
> +ifdef CONFIG_KEXEC_FILE
> +ifdef CONFIG_OF_FLATTREE
> +obj-y+= kexec.o
> +endif
> +endif
> +
> obj-$(CONFIG_OF_UNITTEST) += unittest-data/
> diff --git a/drivers/of/kexec.c b/drivers/of/kexec.c
> new file mode 100644
> index ..469e09613cdd
> --- /dev/null
> +++ b/drivers/of/kexec.c
> @@ -0,0 +1,258 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2020 Arm Limited
> + *
> + * Based on arch/arm64/kernel/machine_kexec_file.c:
> + * Copyright (C) 2018 Linaro Limited
> + *
> + * And arch/powerpc/kexec/file_load.c:
> + * Copyright (C) 2016 IBM Corporation
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +/* relevant device tree properties */
> +#define FDT_PROP_KEXEC_ELFHDR"linux,elfcorehdr"
> +#define FDT_PROP_MEM_RANGE "linux,usable-memory-range"
> +#define FDT_PROP_INITRD_START"linux,initrd-start"
> +#define FDT_PROP_INITRD_END "linux,initrd-end"
> +#define FDT_PROP_BOOTARGS"bootargs"
> +#define FDT_PROP_KASLR_SEED "kaslr-seed"
> +#define FDT_PROP_RNG_SEED"rng-seed"
> +#define RNG_SEED_SIZE128
> +
> +/**
> + * fdt_find_and_del_mem_rsv - delete memory reservation with given address
> and size
> + *
> + * @fdt: Flattened device tree for the current kernel.
> + * @start: Starting address of the reserved memory.
> + * @size:Size of the reserved memory.
> + *
> + * Return: 0 on success, or negative errno on error.
> + */
> +static int fdt_find_and_del_mem_rsv(void *fdt, unsigned long start, unsigned
> long size)
> +{
> + int i, ret, num_rsvs = fdt_num_mem_rsv(fdt);
> +
> + for (i = 0; i < num_rsvs; i++) {
> + u64 rsv_start, rsv_size;
> +
> + ret = fdt_get_mem_rsv(fdt, i, &rsv_start, &rsv_size);
> + if (ret) {
> + pr_err("Malformed device tree.\n");
> + return -EINVAL;
> + }
> +
> + if (rsv_start == start && rsv_size == size) {
> + ret = fdt_del_mem_rsv(fdt, i);
> + if (ret) {
> + pr_err("Error deleting device tree
> reservation.\n");
> + return -EINVAL;
> + }
> +
> + return 0;
> + }
> + }
> +
> + return -ENOENT;
> +}
> +
> +/*
> + * of_kexec_alloc_and_setup_fdt - Alloc and setup a new Flattened Device Tree
> + *
> + * @image: kexec image being loaded.
> + * @initrd_load_addr:Address where the next initrd will be loaded.
> + * @initrd_len: Size of the next initrd, or 0 if there will be
> none.
> + * @cmdline: Command line for the next kernel, or NULL if there will
> + * be none.
> + *
> + * Return: fdt on success, or NULL errno on error.
> + */
> +void *of_kexec_alloc_and_setup_fdt(const struct kimage *image,
> +unsigned long initrd_load_addr,
> +unsigned long initrd_len,
> +const char *cmdline)
> +{
> + void *fdt;
> + int ret, chosen_node;
> + const void *prop;
> + unsigned long fdt_size;
> +
> + fdt_size = fdt_totalsize(initial_boot_params) +
> +(
Re: [PATCH v17 02/10] of: Add a common kexec FDT setup function
On 2/11/21 5:09 PM, Thiago Jung Bauermann wrote:
There's actually a complication that I just noticed and needs to be
addressed. More below.
<...>
+
+/*
+ * of_kexec_alloc_and_setup_fdt - Alloc and setup a new Flattened Device Tree
+ *
+ * @image: kexec image being loaded.
+ * @initrd_load_addr: Address where the next initrd will be loaded.
+ * @initrd_len:Size of the next initrd, or 0 if there will be
none.
+ * @cmdline: Command line for the next kernel, or NULL if there will
+ * be none.
+ *
+ * Return: fdt on success, or NULL errno on error.
+ */
+void *of_kexec_alloc_and_setup_fdt(const struct kimage *image,
+ unsigned long initrd_load_addr,
+ unsigned long initrd_len,
+ const char *cmdline)
+{
+ void *fdt;
+ int ret, chosen_node;
+ const void *prop;
+ unsigned long fdt_size;
+
+ fdt_size = fdt_totalsize(initial_boot_params) +
+ (cmdline ? strlen(cmdline) : 0) +
+ FDT_EXTRA_SPACE;
Just adding 4 KB to initial_boot_params won't be enough for crash
kernels on ppc64. The current powerpc code doubles the size of
initial_boot_params (which is normally larger than 4 KB) and even that
isn't enough. A patch was added to powerpc/next today which uses a more
precise (but arch-specific) formula:
https://lore.kernel.org/linuxppc-dev/161243826811.119001.14083048209224609814.stgit@hbathini/
So I believe we need a hook here where architectures can provide their
own specific calculation for the size of the fdt. Perhaps a weakly
defined function providing a default implementation which an
arch-specific file can override (a la arch_kexec_kernel_image_load())?
Then the powerpc specific hook would be the kexec_fdt_totalsize_ppc64()
function from the patch I linked above.
Do you think it'd better to add "fdt_size" parameter to
of_kexec_alloc_and_setup_fdt() so that the caller can provide the
desired FDT buffer size?
thanks,
-lakshmi
Re: [PATCH v17 02/10] of: Add a common kexec FDT setup function
Lakshmi Ramasubramanian writes:
> On 2/11/21 5:09 PM, Thiago Jung Bauermann wrote:
>> There's actually a complication that I just noticed and needs to be
>> addressed. More below.
>>
>
> <...>
>
>>> +
>>> +/*
>>> + * of_kexec_alloc_and_setup_fdt - Alloc and setup a new Flattened Device
>>> Tree
>>> + *
>>> + * @image: kexec image being loaded.
>>> + * @initrd_load_addr: Address where the next initrd will be loaded.
>>> + * @initrd_len:Size of the next initrd, or 0 if there will be
>>> none.
>>> + * @cmdline: Command line for the next kernel, or NULL if
>>> there will
>>> + * be none.
>>> + *
>>> + * Return: fdt on success, or NULL errno on error.
>>> + */
>>> +void *of_kexec_alloc_and_setup_fdt(const struct kimage *image,
>>> + unsigned long initrd_load_addr,
>>> + unsigned long initrd_len,
>>> + const char *cmdline)
>>> +{
>>> + void *fdt;
>>> + int ret, chosen_node;
>>> + const void *prop;
>>> + unsigned long fdt_size;
>>> +
>>> + fdt_size = fdt_totalsize(initial_boot_params) +
>>> + (cmdline ? strlen(cmdline) : 0) +
>>> + FDT_EXTRA_SPACE;
>> Just adding 4 KB to initial_boot_params won't be enough for crash
>> kernels on ppc64. The current powerpc code doubles the size of
>> initial_boot_params (which is normally larger than 4 KB) and even that
>> isn't enough. A patch was added to powerpc/next today which uses a more
>> precise (but arch-specific) formula:
>> https://lore.kernel.org/linuxppc-dev/161243826811.119001.14083048209224609814.stgit@hbathini/
>> So I believe we need a hook here where architectures can provide their
>> own specific calculation for the size of the fdt. Perhaps a weakly
>> defined function providing a default implementation which an
>> arch-specific file can override (a la arch_kexec_kernel_image_load())?
>> Then the powerpc specific hook would be the kexec_fdt_totalsize_ppc64()
>> function from the patch I linked above.
>>
>
> Do you think it'd better to add "fdt_size" parameter to
> of_kexec_alloc_and_setup_fdt() so that the caller can provide the
> desired FDT buffer size?
Yes, that is actually simpler and better than my idea. :-)
--
Thiago Jung Bauermann
IBM Linux Technology Center
Re: [PATCH v17 02/10] of: Add a common kexec FDT setup function
On Thu, Feb 11, 2021 at 7:17 PM Lakshmi Ramasubramanian
wrote:
>
> On 2/11/21 5:09 PM, Thiago Jung Bauermann wrote:
> >
> > There's actually a complication that I just noticed and needs to be
> > addressed. More below.
> >
>
> <...>
>
> >> +
> >> +/*
> >> + * of_kexec_alloc_and_setup_fdt - Alloc and setup a new Flattened Device
> >> Tree
> >> + *
> >> + * @image: kexec image being loaded.
> >> + * @initrd_load_addr: Address where the next initrd will be loaded.
> >> + * @initrd_len: Size of the next initrd, or 0 if there will
> >> be none.
> >> + * @cmdline:Command line for the next kernel, or NULL if
> >> there will
> >> + * be none.
> >> + *
> >> + * Return: fdt on success, or NULL errno on error.
> >> + */
> >> +void *of_kexec_alloc_and_setup_fdt(const struct kimage *image,
> >> + unsigned long initrd_load_addr,
> >> + unsigned long initrd_len,
> >> + const char *cmdline)
> >> +{
> >> +void *fdt;
> >> +int ret, chosen_node;
> >> +const void *prop;
> >> +unsigned long fdt_size;
> >> +
> >> +fdt_size = fdt_totalsize(initial_boot_params) +
> >> + (cmdline ? strlen(cmdline) : 0) +
> >> + FDT_EXTRA_SPACE;
> >
> > Just adding 4 KB to initial_boot_params won't be enough for crash
> > kernels on ppc64. The current powerpc code doubles the size of
> > initial_boot_params (which is normally larger than 4 KB) and even that
> > isn't enough. A patch was added to powerpc/next today which uses a more
> > precise (but arch-specific) formula:
> >
> > https://lore.kernel.org/linuxppc-dev/161243826811.119001.14083048209224609814.stgit@hbathini/
> >
> > So I believe we need a hook here where architectures can provide their
> > own specific calculation for the size of the fdt. Perhaps a weakly
> > defined function providing a default implementation which an
> > arch-specific file can override (a la arch_kexec_kernel_image_load())?
> >
> > Then the powerpc specific hook would be the kexec_fdt_totalsize_ppc64()
> > function from the patch I linked above.
> >
>
> Do you think it'd better to add "fdt_size" parameter to
> of_kexec_alloc_and_setup_fdt() so that the caller can provide the
> desired FDT buffer size?
Yes, I guess so. But please define the param as extra size, not total
size. The kernel command line size addition can be in the common code.
The above change is also going to conflict, so I think this may have
to wait. Or I'll take the common and arm bits and powerpc can be
converted next cycle (or after the merge window).
Rob
Re: [PATCH v17 02/10] of: Add a common kexec FDT setup function
On 2/12/21 6:38 AM, Rob Herring wrote:
On Thu, Feb 11, 2021 at 7:17 PM Lakshmi Ramasubramanian
wrote:
On 2/11/21 5:09 PM, Thiago Jung Bauermann wrote:
There's actually a complication that I just noticed and needs to be
addressed. More below.
<...>
+
+/*
+ * of_kexec_alloc_and_setup_fdt - Alloc and setup a new Flattened Device Tree
+ *
+ * @image: kexec image being loaded.
+ * @initrd_load_addr: Address where the next initrd will be loaded.
+ * @initrd_len: Size of the next initrd, or 0 if there will be
none.
+ * @cmdline:Command line for the next kernel, or NULL if there
will
+ * be none.
+ *
+ * Return: fdt on success, or NULL errno on error.
+ */
+void *of_kexec_alloc_and_setup_fdt(const struct kimage *image,
+ unsigned long initrd_load_addr,
+ unsigned long initrd_len,
+ const char *cmdline)
+{
+void *fdt;
+int ret, chosen_node;
+const void *prop;
+unsigned long fdt_size;
+
+fdt_size = fdt_totalsize(initial_boot_params) +
+ (cmdline ? strlen(cmdline) : 0) +
+ FDT_EXTRA_SPACE;
Just adding 4 KB to initial_boot_params won't be enough for crash
kernels on ppc64. The current powerpc code doubles the size of
initial_boot_params (which is normally larger than 4 KB) and even that
isn't enough. A patch was added to powerpc/next today which uses a more
precise (but arch-specific) formula:
https://lore.kernel.org/linuxppc-dev/161243826811.119001.14083048209224609814.stgit@hbathini/
So I believe we need a hook here where architectures can provide their
own specific calculation for the size of the fdt. Perhaps a weakly
defined function providing a default implementation which an
arch-specific file can override (a la arch_kexec_kernel_image_load())?
Then the powerpc specific hook would be the kexec_fdt_totalsize_ppc64()
function from the patch I linked above.
Do you think it'd better to add "fdt_size" parameter to
of_kexec_alloc_and_setup_fdt() so that the caller can provide the
desired FDT buffer size?
Yes, I guess so. But please define the param as extra size, not total
size. The kernel command line size addition can be in the common code.
Will do. Just to clarify -
The common code will do:
fdt_totalsize(initial_boot_params) + strlen(cmdline) + extra_fdt_size
The caller will pass "extra_fdt_size"
ARM64 => 4KB
PPC64 => fdt_totalsize(initial_boot_params) - which will be updated when
the patch Thiago had referred to is merged.
The above change is also going to conflict, so I think this may have
to wait. Or I'll take the common and arm bits and powerpc can be
converted next cycle (or after the merge window).
thanks.
-lakshmi
Re: [PATCH v17 02/10] of: Add a common kexec FDT setup function
On Fri, Feb 12, 2021 at 11:19 AM Lakshmi Ramasubramanian
wrote:
>
> On 2/12/21 6:38 AM, Rob Herring wrote:
> > On Thu, Feb 11, 2021 at 7:17 PM Lakshmi Ramasubramanian
> > wrote:
> >>
> >> On 2/11/21 5:09 PM, Thiago Jung Bauermann wrote:
> >>>
> >>> There's actually a complication that I just noticed and needs to be
> >>> addressed. More below.
> >>>
> >>
> >> <...>
> >>
> +
> +/*
> + * of_kexec_alloc_and_setup_fdt - Alloc and setup a new Flattened
> Device Tree
> + *
> + * @image: kexec image being loaded.
> + * @initrd_load_addr: Address where the next initrd will be
> loaded.
> + * @initrd_len: Size of the next initrd, or 0 if there will
> be none.
> + * @cmdline:Command line for the next kernel, or NULL
> if there will
> + * be none.
> + *
> + * Return: fdt on success, or NULL errno on error.
> + */
> +void *of_kexec_alloc_and_setup_fdt(const struct kimage *image,
> + unsigned long initrd_load_addr,
> + unsigned long initrd_len,
> + const char *cmdline)
> +{
> +void *fdt;
> +int ret, chosen_node;
> +const void *prop;
> +unsigned long fdt_size;
> +
> +fdt_size = fdt_totalsize(initial_boot_params) +
> + (cmdline ? strlen(cmdline) : 0) +
> + FDT_EXTRA_SPACE;
> >>>
> >>> Just adding 4 KB to initial_boot_params won't be enough for crash
> >>> kernels on ppc64. The current powerpc code doubles the size of
> >>> initial_boot_params (which is normally larger than 4 KB) and even that
> >>> isn't enough. A patch was added to powerpc/next today which uses a more
> >>> precise (but arch-specific) formula:
> >>>
> >>> https://lore.kernel.org/linuxppc-dev/161243826811.119001.14083048209224609814.stgit@hbathini/
> >>>
> >>> So I believe we need a hook here where architectures can provide their
> >>> own specific calculation for the size of the fdt. Perhaps a weakly
> >>> defined function providing a default implementation which an
> >>> arch-specific file can override (a la arch_kexec_kernel_image_load())?
> >>>
> >>> Then the powerpc specific hook would be the kexec_fdt_totalsize_ppc64()
> >>> function from the patch I linked above.
> >>>
> >>
> >> Do you think it'd better to add "fdt_size" parameter to
> >> of_kexec_alloc_and_setup_fdt() so that the caller can provide the
> >> desired FDT buffer size?
> >
> > Yes, I guess so. But please define the param as extra size, not total
> > size. The kernel command line size addition can be in the common code.
>
> Will do. Just to clarify -
>
> The common code will do:
>
> fdt_totalsize(initial_boot_params) + strlen(cmdline) + extra_fdt_size
>
> The caller will pass "extra_fdt_size"
> ARM64 => 4KB
> PPC64 => fdt_totalsize(initial_boot_params) - which will be updated when
> the patch Thiago had referred to is merged.
Yes, I'd leave the 4KB in there by default and arm64 use 0.
Rob
Re: [PATCH v17 02/10] of: Add a common kexec FDT setup function
On 2/12/21 10:24 AM, Rob Herring wrote:
On Fri, Feb 12, 2021 at 11:19 AM Lakshmi Ramasubramanian
wrote:
On 2/12/21 6:38 AM, Rob Herring wrote:
On Thu, Feb 11, 2021 at 7:17 PM Lakshmi Ramasubramanian
wrote:
On 2/11/21 5:09 PM, Thiago Jung Bauermann wrote:
There's actually a complication that I just noticed and needs to be
addressed. More below.
<...>
+
+/*
+ * of_kexec_alloc_and_setup_fdt - Alloc and setup a new Flattened Device Tree
+ *
+ * @image: kexec image being loaded.
+ * @initrd_load_addr: Address where the next initrd will be loaded.
+ * @initrd_len: Size of the next initrd, or 0 if there will be
none.
+ * @cmdline:Command line for the next kernel, or NULL if there
will
+ * be none.
+ *
+ * Return: fdt on success, or NULL errno on error.
+ */
+void *of_kexec_alloc_and_setup_fdt(const struct kimage *image,
+ unsigned long initrd_load_addr,
+ unsigned long initrd_len,
+ const char *cmdline)
+{
+void *fdt;
+int ret, chosen_node;
+const void *prop;
+unsigned long fdt_size;
+
+fdt_size = fdt_totalsize(initial_boot_params) +
+ (cmdline ? strlen(cmdline) : 0) +
+ FDT_EXTRA_SPACE;
Just adding 4 KB to initial_boot_params won't be enough for crash
kernels on ppc64. The current powerpc code doubles the size of
initial_boot_params (which is normally larger than 4 KB) and even that
isn't enough. A patch was added to powerpc/next today which uses a more
precise (but arch-specific) formula:
https://lore.kernel.org/linuxppc-dev/161243826811.119001.14083048209224609814.stgit@hbathini/
So I believe we need a hook here where architectures can provide their
own specific calculation for the size of the fdt. Perhaps a weakly
defined function providing a default implementation which an
arch-specific file can override (a la arch_kexec_kernel_image_load())?
Then the powerpc specific hook would be the kexec_fdt_totalsize_ppc64()
function from the patch I linked above.
Do you think it'd better to add "fdt_size" parameter to
of_kexec_alloc_and_setup_fdt() so that the caller can provide the
desired FDT buffer size?
Yes, I guess so. But please define the param as extra size, not total
size. The kernel command line size addition can be in the common code.
Will do. Just to clarify -
The common code will do:
fdt_totalsize(initial_boot_params) + strlen(cmdline) + extra_fdt_size
The caller will pass "extra_fdt_size"
ARM64 => 4KB
PPC64 => fdt_totalsize(initial_boot_params) - which will be updated when
the patch Thiago had referred to is merged.
Yes, I'd leave the 4KB in there by default and arm64 use 0.
Sounds good.
common:
fdt_totalsize(initial_boot_params) + strlen(cmdline) + 0x1000 + extra
arm64 => 0 for extra
ppc => fdt_totalsize(initial_boot_params) for extra.
-lakshmi
Re: [PATCH v17 02/10] of: Add a common kexec FDT setup function
Lakshmi Ramasubramanian writes:
> On 2/12/21 10:24 AM, Rob Herring wrote:
>> On Fri, Feb 12, 2021 at 11:19 AM Lakshmi Ramasubramanian
>> wrote:
>>>
>>> On 2/12/21 6:38 AM, Rob Herring wrote:
On Thu, Feb 11, 2021 at 7:17 PM Lakshmi Ramasubramanian
wrote:
>
> On 2/11/21 5:09 PM, Thiago Jung Bauermann wrote:
>>
>> There's actually a complication that I just noticed and needs to be
>> addressed. More below.
>>
>
> <...>
>
>>> +
>>> +/*
>>> + * of_kexec_alloc_and_setup_fdt - Alloc and setup a new Flattened
>>> Device Tree
>>> + *
>>> + * @image: kexec image being loaded.
>>> + * @initrd_load_addr: Address where the next initrd will be
>>> loaded.
>>> + * @initrd_len: Size of the next initrd, or 0 if there
>>> will be none.
>>> + * @cmdline:Command line for the next kernel, or NULL
>>> if there will
>>> + * be none.
>>> + *
>>> + * Return: fdt on success, or NULL errno on error.
>>> + */
>>> +void *of_kexec_alloc_and_setup_fdt(const struct kimage *image,
>>> + unsigned long initrd_load_addr,
>>> + unsigned long initrd_len,
>>> + const char *cmdline)
>>> +{
>>> +void *fdt;
>>> +int ret, chosen_node;
>>> +const void *prop;
>>> +unsigned long fdt_size;
>>> +
>>> +fdt_size = fdt_totalsize(initial_boot_params) +
>>> + (cmdline ? strlen(cmdline) : 0) +
>>> + FDT_EXTRA_SPACE;
>>
>> Just adding 4 KB to initial_boot_params won't be enough for crash
>> kernels on ppc64. The current powerpc code doubles the size of
>> initial_boot_params (which is normally larger than 4 KB) and even that
>> isn't enough. A patch was added to powerpc/next today which uses a more
>> precise (but arch-specific) formula:
>>
>> https://lore.kernel.org/linuxppc-dev/161243826811.119001.14083048209224609814.stgit@hbathini/
>>
>> So I believe we need a hook here where architectures can provide their
>> own specific calculation for the size of the fdt. Perhaps a weakly
>> defined function providing a default implementation which an
>> arch-specific file can override (a la arch_kexec_kernel_image_load())?
>>
>> Then the powerpc specific hook would be the kexec_fdt_totalsize_ppc64()
>> function from the patch I linked above.
>>
>
> Do you think it'd better to add "fdt_size" parameter to
> of_kexec_alloc_and_setup_fdt() so that the caller can provide the
> desired FDT buffer size?
Yes, I guess so. But please define the param as extra size, not total
size. The kernel command line size addition can be in the common code.
>>>
>>> Will do. Just to clarify -
>>>
>>> The common code will do:
>>>
>>> fdt_totalsize(initial_boot_params) + strlen(cmdline) + extra_fdt_size
>>>
>>> The caller will pass "extra_fdt_size"
>>> ARM64 => 4KB
>>> PPC64 => fdt_totalsize(initial_boot_params) - which will be updated when
>>> the patch Thiago had referred to is merged.
>> Yes, I'd leave the 4KB in there by default and arm64 use 0.
>>
>
> Sounds good.
>
> common:
> fdt_totalsize(initial_boot_params) + strlen(cmdline) + 0x1000 + extra
>
> arm64 => 0 for extra
> ppc => fdt_totalsize(initial_boot_params) for extra.
Looks good to me.
--
Thiago Jung Bauermann
IBM Linux Technology Center
Fwd: Re: [PATCH v17 02/10] of: Add a common kexec FDT setup function
Hi Rob,
[PATCH] powerpc: Rename kexec elfcorehdr_addr to elf_headers_mem
This change causes build problem for x86_64 architecture (please see the
mail from kernel test bot below) since arch/x86/include/asm/kexec.h uses
"elf_load_addr" for the ELF header buffer address and not "elf_headers_mem".
struct kimage_arch {
...
/* Core ELF header buffer */
void *elf_headers;
unsigned long elf_headers_sz;
unsigned long elf_load_addr;
};
I am thinking of limiting of_kexec_alloc_and_setup_fdt() to ARM64 and
PPC64 since they are the only ones using this function now.
#if defined(CONFIG_ARM64) && defined(CONFIG_PPC64)
void *of_kexec_alloc_and_setup_fdt(const struct kimage *image,
unsigned long initrd_load_addr,
unsigned long initrd_len,
const char *cmdline)
{
...
}
#endif /* defined(CONFIG_ARM64) && defined(CONFIG_PPC64) */
Please let me know if you have any concerns.
thanks,
-lakshmi
Forwarded Message --------
Subject: Re: [PATCH v17 02/10] of: Add a common kexec FDT setup function
Date: Fri, 12 Feb 2021 00:50:20 +0800
From: kernel test robot
To: Lakshmi Ramasubramanian
CC: kbuild-...@lists.01.org
Hi Lakshmi,
I love your patch! Yet something to improve:
[auto build test ERROR on integrity/next-integrity]
[also build test ERROR on v5.11-rc7 next-20210211]
[cannot apply to powerpc/next robh/for-next arm64/for-next/core]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url:
https://github.com/0day-ci/linux/commits/Lakshmi-Ramasubramanian/Carry-forward-IMA-measurement-log-on-kexec-on-ARM64/20210211-071924
base:
https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git
next-integrity
config: x86_64-randconfig-m001-20210211 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce (this is a W=1 build):
#
https://github.com/0day-ci/linux/commit/12ae86067d115b84092353109e8798693d102f0d
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review
Lakshmi-Ramasubramanian/Carry-forward-IMA-measurement-log-on-kexec-on-ARM64/20210211-071924
git checkout 12ae86067d115b84092353109e8798693d102f0d
# save the attached .config to linux build tree
make W=1 ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
All errors (new ones prefixed by >>):
drivers/of/kexec.c: In function 'of_kexec_alloc_and_setup_fdt':
drivers/of/kexec.c:183:17: error: 'const struct kimage_arch' has no member
named 'elf_headers_mem'; did you mean 'elf_headers_sz'?
183 | image->arch.elf_headers_mem,
| ^~~
| elf_headers_sz
drivers/of/kexec.c:192:42: error: 'const struct kimage_arch' has no
member named 'elf_headers_mem'; did you mean 'elf_headers_sz'?
192 | ret = fdt_add_mem_rsv(fdt, image->arch.elf_headers_mem,
| ^~~
| elf_headers_sz
vim +183 drivers/of/kexec.c
65
66 /*
67 * of_kexec_alloc_and_setup_fdt - Alloc and setup a new
Flattened Device Tree
68 *
69 * @image: kexec image being loaded.
70 * @initrd_load_addr: Address where the next initrd will be loaded.
71 * @initrd_len: Size of the next initrd, or 0 if there will be
none.
72 * @cmdline: Command line for the next kernel, or NULL if there
will
73 * be none.
74 *
75 * Return: fdt on success, or NULL errno on error.
76 */
77 void *of_kexec_alloc_and_setup_fdt(const struct kimage *image,
78 unsigned long initrd_load_addr,
79 unsigned long initrd_len,
80 const char *cmdline)
81 {
82 void *fdt;
83 int ret, chosen_node;
84 const void *prop;
85 unsigned long fdt_size;
86
87 fdt_size = fdt_totalsize(initial_boot_params) +
88 (cmdline ? strlen(cmdline) : 0) +
89 FDT_EXTRA_SPACE;
90
91 fdt = kvmalloc(fdt_size, GFP_KERNEL);
92 if (!fdt)
93 return NULL;
94
95 ret = fdt_open_into(initial_boot_params, fdt, fdt_size);
96 if (ret < 0) {
97 pr_err("Error %d setting up the new device tree.\n",
ret);
98 goto out;
99
Re: Fwd: Re: [PATCH v17 02/10] of: Add a common kexec FDT setup function
On 2/11/21 9:42 AM, Lakshmi Ramasubramanian wrote:
Hi Rob,
[PATCH] powerpc: Rename kexec elfcorehdr_addr to elf_headers_mem
This change causes build problem for x86_64 architecture (please see the
mail from kernel test bot below) since arch/x86/include/asm/kexec.h uses
"elf_load_addr" for the ELF header buffer address and not
"elf_headers_mem".
struct kimage_arch {
...
/* Core ELF header buffer */
void *elf_headers;
unsigned long elf_headers_sz;
unsigned long elf_load_addr;
};
I am thinking of limiting of_kexec_alloc_and_setup_fdt() to ARM64 and
PPC64 since they are the only ones using this function now.
#if defined(CONFIG_ARM64) && defined(CONFIG_PPC64)
Sorry - I meant to say
#if defined(CONFIG_ARM64) || defined(CONFIG_PPC64)
void *of_kexec_alloc_and_setup_fdt(const struct kimage *image,
unsigned long initrd_load_addr,
unsigned long initrd_len,
const char *cmdline)
{
...
}
#endif /* defined(CONFIG_ARM64) && defined(CONFIG_PPC64) */
Please let me know if you have any concerns.
thanks,
-lakshmi
Forwarded Message --------
Subject: Re: [PATCH v17 02/10] of: Add a common kexec FDT setup function
Date: Fri, 12 Feb 2021 00:50:20 +0800
From: kernel test robot
To: Lakshmi Ramasubramanian
CC: kbuild-...@lists.01.org
Hi Lakshmi,
I love your patch! Yet something to improve:
[auto build test ERROR on integrity/next-integrity]
[also build test ERROR on v5.11-rc7 next-20210211]
[cannot apply to powerpc/next robh/for-next arm64/for-next/core]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url:
https://github.com/0day-ci/linux/commits/Lakshmi-Ramasubramanian/Carry-forward-IMA-measurement-log-on-kexec-on-ARM64/20210211-071924
base:
https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git next-integrity
config: x86_64-randconfig-m001-20210211 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce (this is a W=1 build):
#
https://github.com/0day-ci/linux/commit/12ae86067d115b84092353109e8798693d102f0d
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review
Lakshmi-Ramasubramanian/Carry-forward-IMA-measurement-log-on-kexec-on-ARM64/20210211-071924
git checkout 12ae86067d115b84092353109e8798693d102f0d
# save the attached .config to linux build tree
make W=1 ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
All errors (new ones prefixed by >>):
drivers/of/kexec.c: In function 'of_kexec_alloc_and_setup_fdt':
drivers/of/kexec.c:183:17: error: 'const struct kimage_arch' has no
member named 'elf_headers_mem'; did you mean 'elf_headers_sz'?
183 | image->arch.elf_headers_mem,
| ^~~
| elf_headers_sz
drivers/of/kexec.c:192:42: error: 'const struct kimage_arch' has no
member named 'elf_headers_mem'; did you mean 'elf_headers_sz'?
192 | ret = fdt_add_mem_rsv(fdt, image->arch.elf_headers_mem,
| ^~~
| elf_headers_sz
vim +183 drivers/of/kexec.c
65
66 /*
67 * of_kexec_alloc_and_setup_fdt - Alloc and setup a new
Flattened Device Tree
68 *
69 * @image: kexec image being loaded.
70 * @initrd_load_addr: Address where the next initrd will
be loaded.
71 * @initrd_len: Size of the next initrd, or 0 if there
will be none.
72 * @cmdline: Command line for the next kernel, or NULL
if there will
73 * be none.
74 *
75 * Return: fdt on success, or NULL errno on error.
76 */
77 void *of_kexec_alloc_and_setup_fdt(const struct kimage *image,
78 unsigned long initrd_load_addr,
79 unsigned long initrd_len,
80 const char *cmdline)
81 {
82 void *fdt;
83 int ret, chosen_node;
84 const void *prop;
85 unsigned long fdt_size;
86
87 fdt_size = fdt_totalsize(initial_boot_params) +
88 (cmdline ? strlen(cmdline) : 0) +
89 FDT_EXTRA_SPACE;
90
91 fdt = kvmalloc(fdt_size, GFP_KERNEL);
92 if (!fdt)
93 return NULL;
94
95 ret = fdt_open_into(initial_boot_params, fdt, fdt_size);
96 if (ret < 0) {
97 pr_err("Error %d setting up the new device tree.\n
Re: Fwd: Re: [PATCH v17 02/10] of: Add a common kexec FDT setup function
Lakshmi Ramasubramanian writes:
> On 2/11/21 9:42 AM, Lakshmi Ramasubramanian wrote:
>> Hi Rob,
>> [PATCH] powerpc: Rename kexec elfcorehdr_addr to elf_headers_mem
>> This change causes build problem for x86_64 architecture (please see the
>> mail from kernel test bot below) since arch/x86/include/asm/kexec.h uses
>> "elf_load_addr" for the ELF header buffer address and not
>> "elf_headers_mem".
>> struct kimage_arch {
>> ...
>> /* Core ELF header buffer */
>> void *elf_headers;
>> unsigned long elf_headers_sz;
>> unsigned long elf_load_addr;
>> };
>> I am thinking of limiting of_kexec_alloc_and_setup_fdt() to ARM64 and
>> PPC64 since they are the only ones using this function now.
>> #if defined(CONFIG_ARM64) && defined(CONFIG_PPC64)
> Sorry - I meant to say
> #if defined(CONFIG_ARM64) || defined(CONFIG_PPC64)
>
Does it build correctly if you rename elf_headers_mem to elf_load_addr?
Or the other way around, renaming x86's elf_load_addr to
elf_headers_mem. I don't really have a preference.
That would be better than adding an #if condition.
>> void *of_kexec_alloc_and_setup_fdt(const struct kimage *image,
>> unsigned long initrd_load_addr,
>> unsigned long initrd_len,
>> const char *cmdline)
>> {
>> ...
>> }
>> #endif /* defined(CONFIG_ARM64) && defined(CONFIG_PPC64) */
>> Please let me know if you have any concerns.
>> thanks,
>> -lakshmi
--
Thiago Jung Bauermann
IBM Linux Technology Center
Re: Fwd: Re: [PATCH v17 02/10] of: Add a common kexec FDT setup function
On 2/11/21 3:59 PM, Thiago Jung Bauermann wrote:
Lakshmi Ramasubramanian writes:
On 2/11/21 9:42 AM, Lakshmi Ramasubramanian wrote:
Hi Rob,
[PATCH] powerpc: Rename kexec elfcorehdr_addr to elf_headers_mem
This change causes build problem for x86_64 architecture (please see the
mail from kernel test bot below) since arch/x86/include/asm/kexec.h uses
"elf_load_addr" for the ELF header buffer address and not
"elf_headers_mem".
struct kimage_arch {
...
/* Core ELF header buffer */
void *elf_headers;
unsigned long elf_headers_sz;
unsigned long elf_load_addr;
};
I am thinking of limiting of_kexec_alloc_and_setup_fdt() to ARM64 and
PPC64 since they are the only ones using this function now.
#if defined(CONFIG_ARM64) && defined(CONFIG_PPC64)
Sorry - I meant to say
#if defined(CONFIG_ARM64) || defined(CONFIG_PPC64)
Does it build correctly if you rename elf_headers_mem to elf_load_addr?
Or the other way around, renaming x86's elf_load_addr to
elf_headers_mem. I don't really have a preference.
Yes - changing arm64 and ppc from "elf_headers_mem" to "elf_load_addr"
builds fine.
But I am concerned about a few other architectures that also define
"struct kimage_arch" such as "parisc", "arm" which do not have any ELF
related fields. They would not build if the config defines
CONFIG_KEXEC_FILE and CONFIG_OF_FLATTREE.
Do you think that could be an issue?
thanks,
-lakshmi
That would be better than adding an #if condition.
void *of_kexec_alloc_and_setup_fdt(const struct kimage *image,
unsigned long initrd_load_addr,
unsigned long initrd_len,
const char *cmdline)
{
...
}
#endif /* defined(CONFIG_ARM64) && defined(CONFIG_PPC64) */
Please let me know if you have any concerns.
thanks,
-lakshmi
Re: Fwd: Re: [PATCH v17 02/10] of: Add a common kexec FDT setup function
Lakshmi Ramasubramanian writes:
> On 2/11/21 3:59 PM, Thiago Jung Bauermann wrote:
>> Lakshmi Ramasubramanian writes:
>>
>>> On 2/11/21 9:42 AM, Lakshmi Ramasubramanian wrote:
Hi Rob,
[PATCH] powerpc: Rename kexec elfcorehdr_addr to elf_headers_mem
This change causes build problem for x86_64 architecture (please see the
mail from kernel test bot below) since arch/x86/include/asm/kexec.h uses
"elf_load_addr" for the ELF header buffer address and not
"elf_headers_mem".
struct kimage_arch {
...
/* Core ELF header buffer */
void *elf_headers;
unsigned long elf_headers_sz;
unsigned long elf_load_addr;
};
I am thinking of limiting of_kexec_alloc_and_setup_fdt() to ARM64 and
PPC64 since they are the only ones using this function now.
#if defined(CONFIG_ARM64) && defined(CONFIG_PPC64)
>>> Sorry - I meant to say
>>> #if defined(CONFIG_ARM64) || defined(CONFIG_PPC64)
>>>
>> Does it build correctly if you rename elf_headers_mem to elf_load_addr?
>> Or the other way around, renaming x86's elf_load_addr to
>> elf_headers_mem. I don't really have a preference.
>
> Yes - changing arm64 and ppc from "elf_headers_mem" to "elf_load_addr" builds
> fine.
>
> But I am concerned about a few other architectures that also define "struct
> kimage_arch" such as "parisc", "arm" which do not have any ELF related fields.
> They would not build if the config defines CONFIG_KEXEC_FILE and
> CONFIG_OF_FLATTREE.
>
> Do you think that could be an issue?
That's a good point. But in practice, arm doesn't support
CONFIG_KEXEC_FILE. And while parisc does support CONFIG_KEXEC_FILE, as
far as I could determine it doesn't support CONFIG_OF.
So IMHO we don't need to worry about them. We'll cross that bridge if we
get there. If they ever implement KEXEC_FILE or OF_FLATTREE support,
then (again, IMHO) the natural solution would be for them to name the
ELF header member the same way the other arches do.
And since no other architecture defines struct kimage_arch, those are
the only ones we need to consider.
--
Thiago Jung Bauermann
IBM Linux Technology Center
Re: Fwd: Re: [PATCH v17 02/10] of: Add a common kexec FDT setup function
On 2/11/21 6:11 PM, Thiago Jung Bauermann wrote:
Lakshmi Ramasubramanian writes:
On 2/11/21 3:59 PM, Thiago Jung Bauermann wrote:
Lakshmi Ramasubramanian writes:
On 2/11/21 9:42 AM, Lakshmi Ramasubramanian wrote:
Hi Rob,
[PATCH] powerpc: Rename kexec elfcorehdr_addr to elf_headers_mem
This change causes build problem for x86_64 architecture (please see the
mail from kernel test bot below) since arch/x86/include/asm/kexec.h uses
"elf_load_addr" for the ELF header buffer address and not
"elf_headers_mem".
struct kimage_arch {
...
/* Core ELF header buffer */
void *elf_headers;
unsigned long elf_headers_sz;
unsigned long elf_load_addr;
};
I am thinking of limiting of_kexec_alloc_and_setup_fdt() to ARM64 and
PPC64 since they are the only ones using this function now.
#if defined(CONFIG_ARM64) && defined(CONFIG_PPC64)
Sorry - I meant to say
#if defined(CONFIG_ARM64) || defined(CONFIG_PPC64)
Does it build correctly if you rename elf_headers_mem to elf_load_addr?
Or the other way around, renaming x86's elf_load_addr to
elf_headers_mem. I don't really have a preference.
Yes - changing arm64 and ppc from "elf_headers_mem" to "elf_load_addr" builds
fine.
But I am concerned about a few other architectures that also define "struct
kimage_arch" such as "parisc", "arm" which do not have any ELF related fields.
They would not build if the config defines CONFIG_KEXEC_FILE and
CONFIG_OF_FLATTREE.
Do you think that could be an issue?
That's a good point. But in practice, arm doesn't support
CONFIG_KEXEC_FILE. And while parisc does support CONFIG_KEXEC_FILE, as
far as I could determine it doesn't support CONFIG_OF.
So IMHO we don't need to worry about them. We'll cross that bridge if we
get there. If they ever implement KEXEC_FILE or OF_FLATTREE support,
then (again, IMHO) the natural solution would be for them to name the
ELF header member the same way the other arches do.
And since no other architecture defines struct kimage_arch, those are
the only ones we need to consider.
Sounds good Thiago.
I'll rename arm64 and ppc kimage_arch ELF address field to match that
defined for x86/x64.
Also, will add "fdt_size" param to of_kexec_alloc_and_setup_fdt(). For
now, I'll use 2*fdt_totalsize(initial_boot_params) for ppc.
Will send the updated patches shortly.
-lakshmi
Re: Fwd: Re: [PATCH v17 02/10] of: Add a common kexec FDT setup function
Lakshmi Ramasubramanian writes:
> On 2/11/21 6:11 PM, Thiago Jung Bauermann wrote:
>> Lakshmi Ramasubramanian writes:
>>
>>> On 2/11/21 3:59 PM, Thiago Jung Bauermann wrote:
Lakshmi Ramasubramanian writes:
> On 2/11/21 9:42 AM, Lakshmi Ramasubramanian wrote:
>> Hi Rob,
>> [PATCH] powerpc: Rename kexec elfcorehdr_addr to elf_headers_mem
>> This change causes build problem for x86_64 architecture (please see the
>> mail from kernel test bot below) since arch/x86/include/asm/kexec.h uses
>> "elf_load_addr" for the ELF header buffer address and not
>> "elf_headers_mem".
>> struct kimage_arch {
>>...
>>/* Core ELF header buffer */
>>void *elf_headers;
>>unsigned long elf_headers_sz;
>>unsigned long elf_load_addr;
>> };
>> I am thinking of limiting of_kexec_alloc_and_setup_fdt() to ARM64 and
>> PPC64 since they are the only ones using this function now.
>> #if defined(CONFIG_ARM64) && defined(CONFIG_PPC64)
> Sorry - I meant to say
> #if defined(CONFIG_ARM64) || defined(CONFIG_PPC64)
>
Does it build correctly if you rename elf_headers_mem to elf_load_addr?
Or the other way around, renaming x86's elf_load_addr to
elf_headers_mem. I don't really have a preference.
>>>
>>> Yes - changing arm64 and ppc from "elf_headers_mem" to "elf_load_addr"
>>> builds
>>> fine.
>>>
>>> But I am concerned about a few other architectures that also define "struct
>>> kimage_arch" such as "parisc", "arm" which do not have any ELF related
>>> fields.
>>> They would not build if the config defines CONFIG_KEXEC_FILE and
>>> CONFIG_OF_FLATTREE.
>>>
>>> Do you think that could be an issue?
>> That's a good point. But in practice, arm doesn't support
>> CONFIG_KEXEC_FILE. And while parisc does support CONFIG_KEXEC_FILE, as
>> far as I could determine it doesn't support CONFIG_OF.
>> So IMHO we don't need to worry about them. We'll cross that bridge if we
>> get there. If they ever implement KEXEC_FILE or OF_FLATTREE support,
>> then (again, IMHO) the natural solution would be for them to name the
>> ELF header member the same way the other arches do.
>> And since no other architecture defines struct kimage_arch, those are
>> the only ones we need to consider.
>>
>
> Sounds good Thiago.
>
> I'll rename arm64 and ppc kimage_arch ELF address field to match that defined
> for x86/x64.
>
> Also, will add "fdt_size" param to of_kexec_alloc_and_setup_fdt(). For now,
> I'll
> use 2*fdt_totalsize(initial_boot_params) for ppc.
>
> Will send the updated patches shortly.
Sounds good. There will be a small conflict with powerpc/next because of
the patch I mentioned, but it's simple to fix by whoever merges the
series.
--
Thiago Jung Bauermann
IBM Linux Technology Center
