* Fwd: Re: [PATCH v17 02/10] of: Add a common kexec FDT setup function
[not found] <202102120032.Bv0MoYv7-lkp@intel.com>
@ 2021-02-11 17:42 ` Lakshmi Ramasubramanian
2021-02-11 17:47 ` Lakshmi Ramasubramanian
0 siblings, 1 reply; 7+ messages in thread
From: Lakshmi Ramasubramanian @ 2021-02-11 17:42 UTC (permalink / raw)
To: Rob Herring, Thiago Jung Bauermann, Mimi Zohar, linux-integrity,
linux-arm-kernel, linux-integrity, linuxppc-dev
[-- Attachment #1: Type: text/plain, Size: 7614 bytes --]
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 <lkp@intel.com>
To: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
CC: kbuild-all@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 <lkp@intel.com>
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 }
100
101 /* Remove memory reservation for the current device tree. */
102 ret = fdt_find_and_del_mem_rsv(fdt, __pa(initial_boot_params),
103 fdt_totalsize(initial_boot_params));
104 if (ret == -EINVAL) {
105 pr_err("Error removing memory reservation.\n");
106 goto out;
107 }
108
109 chosen_node = fdt_path_offset(fdt, "/chosen");
110 if (chosen_node == -FDT_ERR_NOTFOUND)
111 chosen_node = fdt_add_subnode(fdt, fdt_path_offset(fdt, "/"),
112 "chosen");
113 if (chosen_node < 0) {
114 ret = chosen_node;
115 goto out;
116 }
117
118 ret = fdt_delprop(fdt, chosen_node, FDT_PROP_KEXEC_ELFHDR);
119 if (ret && ret != -FDT_ERR_NOTFOUND)
120 goto out;
121 ret = fdt_delprop(fdt, chosen_node, FDT_PROP_MEM_RANGE);
122 if (ret && ret != -FDT_ERR_NOTFOUND)
123 goto out;
124
125 /* Did we boot using an initrd? */
126 prop = fdt_getprop(fdt, chosen_node, "linux,initrd-start", NULL);
127 if (prop) {
128 u64 tmp_start, tmp_end, tmp_size;
129
130 tmp_start = fdt64_to_cpu(*((const fdt64_t *) prop));
131
132 prop = fdt_getprop(fdt, chosen_node, "linux,initrd-end", NULL);
133 if (!prop) {
134 ret = -EINVAL;
135 goto out;
136 }
137
138 tmp_end = fdt64_to_cpu(*((const fdt64_t *) prop));
139
140 /*
141 * kexec reserves exact initrd size, while firmware may
142 * reserve a multiple of PAGE_SIZE, so check for both.
143 */
144 tmp_size = tmp_end - tmp_start;
145 ret = fdt_find_and_del_mem_rsv(fdt, tmp_start, tmp_size);
146 if (ret == -ENOENT)
147 ret = fdt_find_and_del_mem_rsv(fdt, tmp_start,
148 round_up(tmp_size, PAGE_SIZE));
149 if (ret == -EINVAL)
150 goto out;
151 }
152
153 /* add initrd-* */
154 if (initrd_load_addr) {
155 ret = fdt_setprop_u64(fdt, chosen_node, FDT_PROP_INITRD_START,
156 initrd_load_addr);
157 if (ret)
158 goto out;
159
160 ret = fdt_setprop_u64(fdt, chosen_node, FDT_PROP_INITRD_END,
161 initrd_load_addr + initrd_len);
162 if (ret)
163 goto out;
164
165 ret = fdt_add_mem_rsv(fdt, initrd_load_addr, initrd_len);
166 if (ret)
167 goto out;
168
169 } else {
170 ret = fdt_delprop(fdt, chosen_node, FDT_PROP_INITRD_START);
171 if (ret && (ret != -FDT_ERR_NOTFOUND))
172 goto out;
173
174 ret = fdt_delprop(fdt, chosen_node, FDT_PROP_INITRD_END);
175 if (ret && (ret != -FDT_ERR_NOTFOUND))
176 goto out;
177 }
178
179 if (image->type == KEXEC_TYPE_CRASH) {
180 /* add linux,elfcorehdr */
181 ret = fdt_appendprop_addrrange(fdt, 0, chosen_node,
182 FDT_PROP_KEXEC_ELFHDR,
> 183 image->arch.elf_headers_mem,
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 37859 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Fwd: Re: [PATCH v17 02/10] of: Add a common kexec FDT setup function
2021-02-11 17:42 ` Fwd: Re: [PATCH v17 02/10] of: Add a common kexec FDT setup function Lakshmi Ramasubramanian
@ 2021-02-11 17:47 ` Lakshmi Ramasubramanian
2021-02-11 23:59 ` Thiago Jung Bauermann
0 siblings, 1 reply; 7+ messages in thread
From: Lakshmi Ramasubramanian @ 2021-02-11 17:47 UTC (permalink / raw)
To: Rob Herring, Thiago Jung Bauermann, Mimi Zohar, linux-integrity,
linux-arm-kernel, linux-integrity, linuxppc-dev
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 <lkp@intel.com>
> To: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
> CC: kbuild-all@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 <lkp@intel.com>
>
> 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 }
> 100
> 101 /* Remove memory reservation for the current device tree. */
> 102 ret = fdt_find_and_del_mem_rsv(fdt,
> __pa(initial_boot_params),
> 103 fdt_totalsize(initial_boot_params));
> 104 if (ret == -EINVAL) {
> 105 pr_err("Error removing memory reservation.\n");
> 106 goto out;
> 107 }
> 108
> 109 chosen_node = fdt_path_offset(fdt, "/chosen");
> 110 if (chosen_node == -FDT_ERR_NOTFOUND)
> 111 chosen_node = fdt_add_subnode(fdt,
> fdt_path_offset(fdt, "/"),
> 112 "chosen");
> 113 if (chosen_node < 0) {
> 114 ret = chosen_node;
> 115 goto out;
> 116 }
> 117
> 118 ret = fdt_delprop(fdt, chosen_node, FDT_PROP_KEXEC_ELFHDR);
> 119 if (ret && ret != -FDT_ERR_NOTFOUND)
> 120 goto out;
> 121 ret = fdt_delprop(fdt, chosen_node, FDT_PROP_MEM_RANGE);
> 122 if (ret && ret != -FDT_ERR_NOTFOUND)
> 123 goto out;
> 124
> 125 /* Did we boot using an initrd? */
> 126 prop = fdt_getprop(fdt, chosen_node,
> "linux,initrd-start", NULL);
> 127 if (prop) {
> 128 u64 tmp_start, tmp_end, tmp_size;
> 129
> 130 tmp_start = fdt64_to_cpu(*((const fdt64_t *) prop));
> 131
> 132 prop = fdt_getprop(fdt, chosen_node,
> "linux,initrd-end", NULL);
> 133 if (!prop) {
> 134 ret = -EINVAL;
> 135 goto out;
> 136 }
> 137
> 138 tmp_end = fdt64_to_cpu(*((const fdt64_t *) prop));
> 139
> 140 /*
> 141 * kexec reserves exact initrd size, while firmware may
> 142 * reserve a multiple of PAGE_SIZE, so check for both.
> 143 */
> 144 tmp_size = tmp_end - tmp_start;
> 145 ret = fdt_find_and_del_mem_rsv(fdt, tmp_start,
> tmp_size);
> 146 if (ret == -ENOENT)
> 147 ret = fdt_find_and_del_mem_rsv(fdt, tmp_start,
> 148 round_up(tmp_size, PAGE_SIZE));
> 149 if (ret == -EINVAL)
> 150 goto out;
> 151 }
> 152
> 153 /* add initrd-* */
> 154 if (initrd_load_addr) {
> 155 ret = fdt_setprop_u64(fdt, chosen_node,
> FDT_PROP_INITRD_START,
> 156 initrd_load_addr);
> 157 if (ret)
> 158 goto out;
> 159
> 160 ret = fdt_setprop_u64(fdt, chosen_node,
> FDT_PROP_INITRD_END,
> 161 initrd_load_addr + initrd_len);
> 162 if (ret)
> 163 goto out;
> 164
> 165 ret = fdt_add_mem_rsv(fdt, initrd_load_addr,
> initrd_len);
> 166 if (ret)
> 167 goto out;
> 168
> 169 } else {
> 170 ret = fdt_delprop(fdt, chosen_node,
> FDT_PROP_INITRD_START);
> 171 if (ret && (ret != -FDT_ERR_NOTFOUND))
> 172 goto out;
> 173
> 174 ret = fdt_delprop(fdt, chosen_node,
> FDT_PROP_INITRD_END);
> 175 if (ret && (ret != -FDT_ERR_NOTFOUND))
> 176 goto out;
> 177 }
> 178
> 179 if (image->type == KEXEC_TYPE_CRASH) {
> 180 /* add linux,elfcorehdr */
> 181 ret = fdt_appendprop_addrrange(fdt, 0, chosen_node,
> 182 FDT_PROP_KEXEC_ELFHDR,
> > 183 image->arch.elf_headers_mem,
>
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Fwd: Re: [PATCH v17 02/10] of: Add a common kexec FDT setup function
2021-02-11 17:47 ` Lakshmi Ramasubramanian
@ 2021-02-11 23:59 ` Thiago Jung Bauermann
2021-02-12 1:09 ` Lakshmi Ramasubramanian
0 siblings, 1 reply; 7+ messages in thread
From: Thiago Jung Bauermann @ 2021-02-11 23:59 UTC (permalink / raw)
To: Lakshmi Ramasubramanian
Cc: Rob Herring, Mimi Zohar, linux-arm-kernel, linux-integrity, linuxppc-dev
Lakshmi Ramasubramanian <nramas@linux.microsoft.com> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Fwd: Re: [PATCH v17 02/10] of: Add a common kexec FDT setup function
2021-02-11 23:59 ` Thiago Jung Bauermann
@ 2021-02-12 1:09 ` Lakshmi Ramasubramanian
2021-02-12 2:11 ` Thiago Jung Bauermann
0 siblings, 1 reply; 7+ messages in thread
From: Lakshmi Ramasubramanian @ 2021-02-12 1:09 UTC (permalink / raw)
To: Thiago Jung Bauermann
Cc: Rob Herring, Mimi Zohar, linux-arm-kernel, linux-integrity, linuxppc-dev
On 2/11/21 3:59 PM, Thiago Jung Bauermann wrote:
>
> Lakshmi Ramasubramanian <nramas@linux.microsoft.com> 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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Fwd: Re: [PATCH v17 02/10] of: Add a common kexec FDT setup function
2021-02-12 1:09 ` Lakshmi Ramasubramanian
@ 2021-02-12 2:11 ` Thiago Jung Bauermann
2021-02-12 2:28 ` Lakshmi Ramasubramanian
0 siblings, 1 reply; 7+ messages in thread
From: Thiago Jung Bauermann @ 2021-02-12 2:11 UTC (permalink / raw)
To: Lakshmi Ramasubramanian
Cc: Rob Herring, Mimi Zohar, linux-arm-kernel, linux-integrity, linuxppc-dev
Lakshmi Ramasubramanian <nramas@linux.microsoft.com> writes:
> On 2/11/21 3:59 PM, Thiago Jung Bauermann wrote:
>> Lakshmi Ramasubramanian <nramas@linux.microsoft.com> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Fwd: Re: [PATCH v17 02/10] of: Add a common kexec FDT setup function
2021-02-12 2:11 ` Thiago Jung Bauermann
@ 2021-02-12 2:28 ` Lakshmi Ramasubramanian
2021-02-12 3:21 ` Thiago Jung Bauermann
0 siblings, 1 reply; 7+ messages in thread
From: Lakshmi Ramasubramanian @ 2021-02-12 2:28 UTC (permalink / raw)
To: Thiago Jung Bauermann
Cc: Rob Herring, Mimi Zohar, linux-arm-kernel, linux-integrity, linuxppc-dev
On 2/11/21 6:11 PM, Thiago Jung Bauermann wrote:
>
> Lakshmi Ramasubramanian <nramas@linux.microsoft.com> writes:
>
>> On 2/11/21 3:59 PM, Thiago Jung Bauermann wrote:
>>> Lakshmi Ramasubramanian <nramas@linux.microsoft.com> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Fwd: Re: [PATCH v17 02/10] of: Add a common kexec FDT setup function
2021-02-12 2:28 ` Lakshmi Ramasubramanian
@ 2021-02-12 3:21 ` Thiago Jung Bauermann
0 siblings, 0 replies; 7+ messages in thread
From: Thiago Jung Bauermann @ 2021-02-12 3:21 UTC (permalink / raw)
To: Lakshmi Ramasubramanian
Cc: Rob Herring, Mimi Zohar, linux-arm-kernel, linux-integrity, linuxppc-dev
Lakshmi Ramasubramanian <nramas@linux.microsoft.com> writes:
> On 2/11/21 6:11 PM, Thiago Jung Bauermann wrote:
>> Lakshmi Ramasubramanian <nramas@linux.microsoft.com> writes:
>>
>>> On 2/11/21 3:59 PM, Thiago Jung Bauermann wrote:
>>>> Lakshmi Ramasubramanian <nramas@linux.microsoft.com> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-02-12 3:22 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <202102120032.Bv0MoYv7-lkp@intel.com>
2021-02-11 17:42 ` Fwd: Re: [PATCH v17 02/10] of: Add a common kexec FDT setup function Lakshmi Ramasubramanian
2021-02-11 17:47 ` Lakshmi Ramasubramanian
2021-02-11 23:59 ` Thiago Jung Bauermann
2021-02-12 1:09 ` Lakshmi Ramasubramanian
2021-02-12 2:11 ` Thiago Jung Bauermann
2021-02-12 2:28 ` Lakshmi Ramasubramanian
2021-02-12 3:21 ` Thiago Jung Bauermann
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).