Fwd: Re: [PATCH v17 02/10] of: Add a common kexec FDT setup function

Fwd: Re: [PATCH v17 02/10] of: Add a common kexec FDT setup function

[Lakshmi Ramasubramanian]

[-- 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 --]

[-- Attachment #3: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: Fwd: Re: [PATCH v17 02/10] of: Add a common kexec FDT setup function

[Lakshmi Ramasubramanian]

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
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: Fwd: Re: [PATCH v17 02/10] of: Add a common kexec FDT setup function

[Thiago Jung Bauermann]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: Fwd: Re: [PATCH v17 02/10] of: Add a common kexec FDT setup function

[Lakshmi Ramasubramanian]

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
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: Fwd: Re: [PATCH v17 02/10] of: Add a common kexec FDT setup function

[Thiago Jung Bauermann]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: Fwd: Re: [PATCH v17 02/10] of: Add a common kexec FDT setup function

[Lakshmi Ramasubramanian]

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: Fwd: Re: [PATCH v17 02/10] of: Add a common kexec FDT setup function

[Thiago Jung Bauermann]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel