From: Rob Herring <robh@kernel.org>
To: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
Cc: Mimi Zohar <zohar@linux.ibm.com>,
Thiago Jung Bauermann <bauerman@linux.ibm.com>,
"AKASHI, Takahiro" <takahiro.akashi@linaro.org>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Will Deacon <will@kernel.org>, Joe Perches <joe@perches.com>,
Catalin Marinas <catalin.marinas@arm.com>,
Michael Ellerman <mpe@ellerman.id.au>,
James Morse <james.morse@arm.com>,
Sasha Levin <sashal@kernel.org>,
Benjamin Herrenschmidt <benh@kernel.crashing.org>,
Paul Mackerras <paulus@samba.org>,
Frank Rowand <frowand.list@gmail.com>,
vincenzo.frascino@arm.com, Mark Rutland <mark.rutland@arm.com>,
dmitry.kasatkin@gmail.com, James Morris <jmorris@namei.org>,
"Serge E. Hallyn" <serge@hallyn.com>,
Pavel Tatashin <pasha.tatashin@soleen.com>,
Allison Randal <allison@lohutok.net>,
Masahiro Yamada <masahiroy@kernel.org>,
Bhupesh Sharma <bhsharma@redhat.com>,
Matthias Brugger <mbrugger@suse.com>,
Hsin-Yi Wang <hsinyi@chromium.org>,
tao.li@vivo.com, Christophe Leroy <christophe.leroy@c-s.fr>,
Prakhar Srivastava <prsriva@linux.microsoft.com>,
balajib@linux.microsoft.com, linux-integrity@vger.kernel.org,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
devicetree@vger.kernel.org,
linuxppc-dev <linuxppc-dev@lists.ozlabs.org>
Subject: Re: [PATCH v16 11/12] powerpc: Use OF alloc and free for FDT
Date: Thu, 4 Feb 2021 17:36:15 -0600 [thread overview]
Message-ID: <CAL_JsqKY9fxOowW=sBVm9s8j=3RWA7Jn9Ft9Edyx5qy5Yvykmw@mail.gmail.com> (raw)
In-Reply-To: <503d42ba-89bf-4ad9-9d4c-acb625580f77@linux.microsoft.com>
On Thu, Feb 4, 2021 at 5:23 PM Lakshmi Ramasubramanian
<nramas@linux.microsoft.com> wrote:
>
> On 2/4/21 11:26 AM, Rob Herring wrote:
> > On Thu, Feb 4, 2021 at 10:42 AM Lakshmi Ramasubramanian
> > <nramas@linux.microsoft.com> wrote:
> >>
> >> of_alloc_and_init_fdt() and of_free_fdt() have been defined in
> >> drivers/of/kexec.c to allocate and free memory for FDT.
> >>
> >> Use of_alloc_and_init_fdt() and of_free_fdt() to allocate and
> >> initialize the FDT, and to free the FDT respectively.
> >>
> >> powerpc sets the FDT address in image_loader_data field in
> >> "struct kimage" and the memory is freed in
> >> kimage_file_post_load_cleanup(). This cleanup function uses kfree()
> >> to free the memory. But since of_alloc_and_init_fdt() uses kvmalloc()
> >> for allocation, the buffer needs to be freed using kvfree().
> >
> > You could just change the kexec core to call kvfree() instead.
>
> >
> >> Define "fdt" field in "struct kimage_arch" for powerpc to store
> >> the address of FDT, and free the memory in powerpc specific
> >> arch_kimage_file_post_load_cleanup().
> >
> > However, given all the other buffers have an explicit field in kimage
> > or kimage_arch, changing powerpc is to match arm64 is better IMO.
>
> Just to be clear:
> I'll leave this as is - free FDT buffer in powerpc's
> arch_kimage_file_post_load_cleanup() to match arm64 behavior.
Yes.
> Will not change "kexec core" to call kvfree() - doing that change would
> require changing all architectures to use kvmalloc() for
> image_loader_data allocation.
Actually, no. AIUI, kvfree() can be used whether you used kvmalloc,
vmalloc, or kmalloc for the alloc.
> >> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
> >> Suggested-by: Rob Herring <robh@kernel.org>
> >> Suggested-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> >> ---
> >> arch/powerpc/include/asm/kexec.h | 2 ++
> >> arch/powerpc/kexec/elf_64.c | 26 ++++++++++++++++----------
> >> arch/powerpc/kexec/file_load_64.c | 3 +++
> >> 3 files changed, 21 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/arch/powerpc/include/asm/kexec.h b/arch/powerpc/include/asm/kexec.h
> >> index 2c0be93d239a..d7d13cac4d31 100644
> >> --- a/arch/powerpc/include/asm/kexec.h
> >> +++ b/arch/powerpc/include/asm/kexec.h
> >> @@ -111,6 +111,8 @@ struct kimage_arch {
> >> unsigned long elf_headers_mem;
> >> unsigned long elf_headers_sz;
> >> void *elf_headers;
> >> +
> >> + void *fdt;
> >> };
> >>
> >> char *setup_kdump_cmdline(struct kimage *image, char *cmdline,
> >> diff --git a/arch/powerpc/kexec/elf_64.c b/arch/powerpc/kexec/elf_64.c
> >> index d0e459bb2f05..51d2d8eb6c1b 100644
> >> --- a/arch/powerpc/kexec/elf_64.c
> >> +++ b/arch/powerpc/kexec/elf_64.c
> >> @@ -19,6 +19,7 @@
> >> #include <linux/kexec.h>
> >> #include <linux/libfdt.h>
> >> #include <linux/module.h>
> >> +#include <linux/of.h>
> >> #include <linux/of_fdt.h>
> >> #include <linux/slab.h>
> >> #include <linux/types.h>
> >> @@ -32,7 +33,7 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
> >> unsigned int fdt_size;
> >> unsigned long kernel_load_addr;
> >> unsigned long initrd_load_addr = 0, fdt_load_addr;
> >> - void *fdt;
> >> + void *fdt = NULL;
> >> const void *slave_code;
> >> struct elfhdr ehdr;
> >> char *modified_cmdline = NULL;
> >> @@ -103,18 +104,12 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
> >> }
> >>
> >> fdt_size = fdt_totalsize(initial_boot_params) * 2;
> >> - fdt = kmalloc(fdt_size, GFP_KERNEL);
> >> + fdt = of_alloc_and_init_fdt(fdt_size);
> >> if (!fdt) {
> >> pr_err("Not enough memory for the device tree.\n");
> >> ret = -ENOMEM;
> >> goto out;
> >> }
> >> - ret = fdt_open_into(initial_boot_params, fdt, fdt_size);
> >> - if (ret < 0) {
> >> - pr_err("Error setting up the new device tree.\n");
> >> - ret = -EINVAL;
> >> - goto out;
> >> - }
> >>
> >> ret = setup_new_fdt_ppc64(image, fdt, initrd_load_addr,
> >
> > The first thing this function does is call setup_new_fdt() which first
> > calls of_kexec_setup_new_fdt(). (Note, I really don't understand the
> > PPC code split. It looks like there's a 32-bit and 64-bit split, but
> > 32-bit looks broken to me. Nothing ever calls setup_new_fdt() except
> > setup_new_fdt_ppc64()). The arm64 version is calling
> > of_alloc_and_init_fdt() and then of_kexec_setup_new_fdt() directly.
> >
> > So we can just make of_alloc_and_init_fdt() also call
> > of_kexec_setup_new_fdt() (really, just tweak of_kexec_setup_new_fdt do
> > the alloc and copy).
> ok - will move fdt allocation into of_kexec_setup_new_fdt().
>
> I don't think the architecture needs to pick the
> > size either. It's doubtful that either one is that sensitive to the
> > amount of extra space.
> I am not clear about the above comment -
> are you saying the architectures don't need to pass FDT size to the
> alloc function?
>
> arm64 is adding command line string length and some extra space to the
> size computed from initial_boot_params for FDT Size:
>
> buf_size = fdt_totalsize(initial_boot_params)
> + cmdline_len + DTB_EXTRA_SPACE;
>
> powerpc is just using twice the size computed from initial_boot_params
>
> fdt_size = fdt_totalsize(initial_boot_params) * 2;
>
> I think it would be safe to let arm64 and powerpc pass the required FDT
> size, along with the other params to of_kexec_setup_new_fdt() - and in
> this function we allocate FDT and set it up.
It's pretty clear that someone just picked something that 'should be
enough'. The only thing I can guess for the difference is that arm
DT's tend to be a bit larger. So doubling the size would be even more
excessive. Either way, we're talking 10s kB to few 100kB. I'd go with
DTB_EXTRA_SPACE and we can bump it up if someone has problems.
Also, I would like for 'initial_boot_params' to be private ultimately,
so removing any references is helpful.
> And, for powerpc leave the remaining code in setup_new_fdt_ppc64().
Right.
Rob
next prev parent reply other threads:[~2021-02-04 23:37 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-02-04 16:41 [PATCH v16 00/12] Carry forward IMA measurement log on kexec on ARM64 Lakshmi Ramasubramanian
2021-02-04 16:41 ` [PATCH v16 01/12] powerpc: Rename kexec elfcorehdr_addr to elf_headers_mem Lakshmi Ramasubramanian
2021-02-04 16:41 ` [PATCH v16 02/12] of: Add a common kexec FDT setup function Lakshmi Ramasubramanian
2021-02-04 16:41 ` [PATCH v16 03/12] arm64: Use common of_kexec_setup_new_fdt() Lakshmi Ramasubramanian
2021-02-04 16:41 ` [PATCH v16 04/12] powerpc: " Lakshmi Ramasubramanian
2021-02-04 16:41 ` [PATCH v16 05/12] powerpc: Move ima buffer fields to struct kimage Lakshmi Ramasubramanian
2021-02-04 16:41 ` [PATCH v16 06/12] powerpc: Move arch independent ima kexec functions to drivers/of/kexec.c Lakshmi Ramasubramanian
2021-02-04 16:41 ` [PATCH v16 07/12] kexec: Use fdt_appendprop_addrrange() to add ima buffer to FDT Lakshmi Ramasubramanian
2021-02-04 16:41 ` [PATCH v16 08/12] powerpc: Delete unused function delete_fdt_mem_rsv() Lakshmi Ramasubramanian
2021-02-04 16:41 ` [PATCH v16 09/12] of: Define functions to allocate and free FDT Lakshmi Ramasubramanian
2021-02-04 16:41 ` [PATCH v16 10/12] arm64: Use OF alloc and free functions for FDT Lakshmi Ramasubramanian
2021-02-04 18:00 ` Will Deacon
2021-02-04 16:41 ` [PATCH v16 11/12] powerpc: Use OF alloc and free " Lakshmi Ramasubramanian
2021-02-04 19:26 ` Rob Herring
2021-02-04 23:23 ` Lakshmi Ramasubramanian
2021-02-04 23:36 ` Rob Herring [this message]
2021-02-04 23:42 ` Lakshmi Ramasubramanian
2021-02-08 4:12 ` Michael Ellerman
2021-02-04 16:41 ` [PATCH v16 12/12] arm64: Enable passing IMA log to next kernel on kexec Lakshmi Ramasubramanian
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAL_JsqKY9fxOowW=sBVm9s8j=3RWA7Jn9Ft9Edyx5qy5Yvykmw@mail.gmail.com' \
--to=robh@kernel.org \
--cc=allison@lohutok.net \
--cc=balajib@linux.microsoft.com \
--cc=bauerman@linux.ibm.com \
--cc=benh@kernel.crashing.org \
--cc=bhsharma@redhat.com \
--cc=catalin.marinas@arm.com \
--cc=christophe.leroy@c-s.fr \
--cc=devicetree@vger.kernel.org \
--cc=dmitry.kasatkin@gmail.com \
--cc=frowand.list@gmail.com \
--cc=gregkh@linuxfoundation.org \
--cc=hsinyi@chromium.org \
--cc=james.morse@arm.com \
--cc=jmorris@namei.org \
--cc=joe@perches.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-integrity@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=mark.rutland@arm.com \
--cc=masahiroy@kernel.org \
--cc=mbrugger@suse.com \
--cc=mpe@ellerman.id.au \
--cc=nramas@linux.microsoft.com \
--cc=pasha.tatashin@soleen.com \
--cc=paulus@samba.org \
--cc=prsriva@linux.microsoft.com \
--cc=sashal@kernel.org \
--cc=serge@hallyn.com \
--cc=takahiro.akashi@linaro.org \
--cc=tao.li@vivo.com \
--cc=vincenzo.frascino@arm.com \
--cc=will@kernel.org \
--cc=zohar@linux.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).