| Message ID | 20210115173017.30617-10-nramas@linux.microsoft.com |
|---|---|
| State | New |
| Headers | show |
| Series | Carry forward IMA measurement log on kexec on ARM64 | expand |
Hi Ard, On Fri, 2021-01-15 at 09:30 -0800, Lakshmi Ramasubramanian wrote: > create_dtb() function allocates kernel virtual memory for > the device tree blob (DTB). This is not consistent with other > architectures, such as powerpc, which calls kmalloc() for allocating > memory for the DTB. > > Call kmalloc() to allocate memory for the DTB, and kfree() to free > the allocated memory. The vmalloc() function description says, "vmalloc - allocate virtually contiguous memory". I'd appreciate your reviewing this patch, in particular, which replaces vmalloc() with kmalloc(). thanks, Mimi > > Co-developed-by: Prakhar Srivastava <prsriva@linux.microsoft.com> > Signed-off-by: Prakhar Srivastava <prsriva@linux.microsoft.com> > Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com> > --- > arch/arm64/kernel/machine_kexec_file.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/arch/arm64/kernel/machine_kexec_file.c b/arch/arm64/kernel/machine_kexec_file.c > index 7de9c47dee7c..51c40143d6fa 100644 > --- a/arch/arm64/kernel/machine_kexec_file.c > +++ b/arch/arm64/kernel/machine_kexec_file.c > @@ -29,7 +29,7 @@ const struct kexec_file_ops * const kexec_file_loaders[] = { > > int arch_kimage_file_post_load_cleanup(struct kimage *image) > { > - vfree(image->arch.dtb); > + kfree(image->arch.dtb); > image->arch.dtb = NULL; > > vfree(image->arch.elf_headers); > @@ -59,19 +59,21 @@ static int create_dtb(struct kimage *image, > + cmdline_len + DTB_EXTRA_SPACE; > > for (;;) { > - buf = vmalloc(buf_size); > + buf = kmalloc(buf_size, GFP_KERNEL); > if (!buf) > return -ENOMEM; > > /* duplicate a device tree blob */ > ret = fdt_open_into(initial_boot_params, buf, buf_size); > - if (ret) > + if (ret) { > + kfree(buf); > return -EINVAL; > + } > > ret = of_kexec_setup_new_fdt(image, buf, initrd_load_addr, > initrd_len, cmdline); > if (ret) { > - vfree(buf); > + kfree(buf); > if (ret == -ENOMEM) { > /* unlikely, but just in case */ > buf_size += DTB_EXTRA_SPACE; > @@ -217,6 +219,6 @@ int load_other_segments(struct kimage *image, > return 0; > > out_err: > - vfree(dtb); > + kfree(dtb); > return ret; > }
On Fri, Jan 15, 2021 at 09:30:16AM -0800, Lakshmi Ramasubramanian wrote: > create_dtb() function allocates kernel virtual memory for > the device tree blob (DTB). This is not consistent with other > architectures, such as powerpc, which calls kmalloc() for allocating > memory for the DTB. > > Call kmalloc() to allocate memory for the DTB, and kfree() to free > the allocated memory. > > Co-developed-by: Prakhar Srivastava <prsriva@linux.microsoft.com> > Signed-off-by: Prakhar Srivastava <prsriva@linux.microsoft.com> > Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com> > --- > arch/arm64/kernel/machine_kexec_file.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/arch/arm64/kernel/machine_kexec_file.c b/arch/arm64/kernel/machine_kexec_file.c > index 7de9c47dee7c..51c40143d6fa 100644 > --- a/arch/arm64/kernel/machine_kexec_file.c > +++ b/arch/arm64/kernel/machine_kexec_file.c > @@ -29,7 +29,7 @@ const struct kexec_file_ops * const kexec_file_loaders[] = { > > int arch_kimage_file_post_load_cleanup(struct kimage *image) > { > - vfree(image->arch.dtb); > + kfree(image->arch.dtb); > image->arch.dtb = NULL; > > vfree(image->arch.elf_headers); > @@ -59,19 +59,21 @@ static int create_dtb(struct kimage *image, > + cmdline_len + DTB_EXTRA_SPACE; > > for (;;) { > - buf = vmalloc(buf_size); > + buf = kmalloc(buf_size, GFP_KERNEL); Is there a functional need for this patch? I build the 'dtbs' target just now and sdm845-db845c.dtb is approaching 100K, which feels quite large for kmalloc(). Will
On 1/27/21 8:52 AM, Will Deacon wrote: Hi Will, > On Fri, Jan 15, 2021 at 09:30:16AM -0800, Lakshmi Ramasubramanian wrote: >> create_dtb() function allocates kernel virtual memory for >> the device tree blob (DTB). This is not consistent with other >> architectures, such as powerpc, which calls kmalloc() for allocating >> memory for the DTB. >> >> Call kmalloc() to allocate memory for the DTB, and kfree() to free >> the allocated memory. >> >> Co-developed-by: Prakhar Srivastava <prsriva@linux.microsoft.com> >> Signed-off-by: Prakhar Srivastava <prsriva@linux.microsoft.com> >> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com> >> --- >> arch/arm64/kernel/machine_kexec_file.c | 12 +++++++----- >> 1 file changed, 7 insertions(+), 5 deletions(-) >> >> diff --git a/arch/arm64/kernel/machine_kexec_file.c b/arch/arm64/kernel/machine_kexec_file.c >> index 7de9c47dee7c..51c40143d6fa 100644 >> --- a/arch/arm64/kernel/machine_kexec_file.c >> +++ b/arch/arm64/kernel/machine_kexec_file.c >> @@ -29,7 +29,7 @@ const struct kexec_file_ops * const kexec_file_loaders[] = { >> >> int arch_kimage_file_post_load_cleanup(struct kimage *image) >> { >> - vfree(image->arch.dtb); >> + kfree(image->arch.dtb); >> image->arch.dtb = NULL; >> >> vfree(image->arch.elf_headers); >> @@ -59,19 +59,21 @@ static int create_dtb(struct kimage *image, >> + cmdline_len + DTB_EXTRA_SPACE; >> >> for (;;) { >> - buf = vmalloc(buf_size); >> + buf = kmalloc(buf_size, GFP_KERNEL); > > Is there a functional need for this patch? I build the 'dtbs' target just > now and sdm845-db845c.dtb is approaching 100K, which feels quite large > for kmalloc(). Changing the allocation from vmalloc() to kmalloc() would help us further consolidate the DTB setup code for powerpc and arm64. thanks, -lakshmi
On Wed, Jan 27, 2021 at 09:59:38AM -0800, Lakshmi Ramasubramanian wrote: > On 1/27/21 8:52 AM, Will Deacon wrote: > > Hi Will, > > > On Fri, Jan 15, 2021 at 09:30:16AM -0800, Lakshmi Ramasubramanian wrote: > > > create_dtb() function allocates kernel virtual memory for > > > the device tree blob (DTB). This is not consistent with other > > > architectures, such as powerpc, which calls kmalloc() for allocating > > > memory for the DTB. > > > > > > Call kmalloc() to allocate memory for the DTB, and kfree() to free > > > the allocated memory. > > > > > > Co-developed-by: Prakhar Srivastava <prsriva@linux.microsoft.com> > > > Signed-off-by: Prakhar Srivastava <prsriva@linux.microsoft.com> > > > Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com> > > > --- > > > arch/arm64/kernel/machine_kexec_file.c | 12 +++++++----- > > > 1 file changed, 7 insertions(+), 5 deletions(-) > > > > > > diff --git a/arch/arm64/kernel/machine_kexec_file.c b/arch/arm64/kernel/machine_kexec_file.c > > > index 7de9c47dee7c..51c40143d6fa 100644 > > > --- a/arch/arm64/kernel/machine_kexec_file.c > > > +++ b/arch/arm64/kernel/machine_kexec_file.c > > > @@ -29,7 +29,7 @@ const struct kexec_file_ops * const kexec_file_loaders[] = { > > > int arch_kimage_file_post_load_cleanup(struct kimage *image) > > > { > > > - vfree(image->arch.dtb); > > > + kfree(image->arch.dtb); > > > image->arch.dtb = NULL; > > > vfree(image->arch.elf_headers); > > > @@ -59,19 +59,21 @@ static int create_dtb(struct kimage *image, > > > + cmdline_len + DTB_EXTRA_SPACE; > > > for (;;) { > > > - buf = vmalloc(buf_size); > > > + buf = kmalloc(buf_size, GFP_KERNEL); > > > > Is there a functional need for this patch? I build the 'dtbs' target just > > now and sdm845-db845c.dtb is approaching 100K, which feels quite large > > for kmalloc(). > > Changing the allocation from vmalloc() to kmalloc() would help us further > consolidate the DTB setup code for powerpc and arm64. Ok, but at the risk of allocation failure. Can powerpc use vmalloc() instead? Will
Will Deacon <will@kernel.org> writes: > On Wed, Jan 27, 2021 at 09:59:38AM -0800, Lakshmi Ramasubramanian wrote: >> On 1/27/21 8:52 AM, Will Deacon wrote: >> >> Hi Will, >> >> > On Fri, Jan 15, 2021 at 09:30:16AM -0800, Lakshmi Ramasubramanian wrote: >> > > create_dtb() function allocates kernel virtual memory for >> > > the device tree blob (DTB). This is not consistent with other >> > > architectures, such as powerpc, which calls kmalloc() for allocating >> > > memory for the DTB. >> > > >> > > Call kmalloc() to allocate memory for the DTB, and kfree() to free >> > > the allocated memory. >> > > >> > > Co-developed-by: Prakhar Srivastava <prsriva@linux.microsoft.com> >> > > Signed-off-by: Prakhar Srivastava <prsriva@linux.microsoft.com> >> > > Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com> >> > > --- >> > > arch/arm64/kernel/machine_kexec_file.c | 12 +++++++----- >> > > 1 file changed, 7 insertions(+), 5 deletions(-) >> > > >> > > diff --git a/arch/arm64/kernel/machine_kexec_file.c b/arch/arm64/kernel/machine_kexec_file.c >> > > index 7de9c47dee7c..51c40143d6fa 100644 >> > > --- a/arch/arm64/kernel/machine_kexec_file.c >> > > +++ b/arch/arm64/kernel/machine_kexec_file.c >> > > @@ -29,7 +29,7 @@ const struct kexec_file_ops * const kexec_file_loaders[] = { >> > > int arch_kimage_file_post_load_cleanup(struct kimage *image) >> > > { >> > > - vfree(image->arch.dtb); >> > > + kfree(image->arch.dtb); >> > > image->arch.dtb = NULL; >> > > vfree(image->arch.elf_headers); >> > > @@ -59,19 +59,21 @@ static int create_dtb(struct kimage *image, >> > > + cmdline_len + DTB_EXTRA_SPACE; >> > > for (;;) { >> > > - buf = vmalloc(buf_size); >> > > + buf = kmalloc(buf_size, GFP_KERNEL); >> > >> > Is there a functional need for this patch? I build the 'dtbs' target just >> > now and sdm845-db845c.dtb is approaching 100K, which feels quite large >> > for kmalloc(). >> >> Changing the allocation from vmalloc() to kmalloc() would help us further >> consolidate the DTB setup code for powerpc and arm64. > > Ok, but at the risk of allocation failure. Can powerpc use vmalloc() > instead? I believe this patch stems from this suggestion by Rob Herring: > This could be taken a step further and do the allocation of the new > FDT. The difference is arm64 uses vmalloc and powerpc uses kmalloc. The > arm64 version also retries with a bigger allocation. That seems > unnecessary. in https://lore.kernel.org/linux-integrity/20201211221006.1052453-3-robh@kernel.org/ The problem is that this patch implements only part of the suggestion, which isn't useful in itself. So the patch series should either drop this patch or consolidate the FDT allocation between the arches. I just tested on powernv and pseries platforms and powerpc can use vmalloc for the FDT buffer. -- Thiago Jung Bauermann IBM Linux Technology Center
On 1/27/21 7:52 PM, Thiago Jung Bauermann wrote: > > Will Deacon <will@kernel.org> writes: > >> On Wed, Jan 27, 2021 at 09:59:38AM -0800, Lakshmi Ramasubramanian wrote: >>> On 1/27/21 8:52 AM, Will Deacon wrote: >>> >>> Hi Will, >>> >>>> On Fri, Jan 15, 2021 at 09:30:16AM -0800, Lakshmi Ramasubramanian wrote: >>>>> create_dtb() function allocates kernel virtual memory for >>>>> the device tree blob (DTB). This is not consistent with other >>>>> architectures, such as powerpc, which calls kmalloc() for allocating >>>>> memory for the DTB. >>>>> >>>>> Call kmalloc() to allocate memory for the DTB, and kfree() to free >>>>> the allocated memory. >>>>> >>>>> Co-developed-by: Prakhar Srivastava <prsriva@linux.microsoft.com> >>>>> Signed-off-by: Prakhar Srivastava <prsriva@linux.microsoft.com> >>>>> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com> >>>>> --- >>>>> arch/arm64/kernel/machine_kexec_file.c | 12 +++++++----- >>>>> 1 file changed, 7 insertions(+), 5 deletions(-) >>>>> >>>>> diff --git a/arch/arm64/kernel/machine_kexec_file.c b/arch/arm64/kernel/machine_kexec_file.c >>>>> index 7de9c47dee7c..51c40143d6fa 100644 >>>>> --- a/arch/arm64/kernel/machine_kexec_file.c >>>>> +++ b/arch/arm64/kernel/machine_kexec_file.c >>>>> @@ -29,7 +29,7 @@ const struct kexec_file_ops * const kexec_file_loaders[] = { >>>>> int arch_kimage_file_post_load_cleanup(struct kimage *image) >>>>> { >>>>> - vfree(image->arch.dtb); >>>>> + kfree(image->arch.dtb); >>>>> image->arch.dtb = NULL; >>>>> vfree(image->arch.elf_headers); >>>>> @@ -59,19 +59,21 @@ static int create_dtb(struct kimage *image, >>>>> + cmdline_len + DTB_EXTRA_SPACE; >>>>> for (;;) { >>>>> - buf = vmalloc(buf_size); >>>>> + buf = kmalloc(buf_size, GFP_KERNEL); >>>> >>>> Is there a functional need for this patch? I build the 'dtbs' target just >>>> now and sdm845-db845c.dtb is approaching 100K, which feels quite large >>>> for kmalloc(). >>> >>> Changing the allocation from vmalloc() to kmalloc() would help us further >>> consolidate the DTB setup code for powerpc and arm64. >> >> Ok, but at the risk of allocation failure. Can powerpc use vmalloc() >> instead? > > I believe this patch stems from this suggestion by Rob Herring: > >> This could be taken a step further and do the allocation of the new >> FDT. The difference is arm64 uses vmalloc and powerpc uses kmalloc. The >> arm64 version also retries with a bigger allocation. That seems >> unnecessary. > > in https://lore.kernel.org/linux-integrity/20201211221006.1052453-3-robh@kernel.org/ > > The problem is that this patch implements only part of the suggestion, > which isn't useful in itself. So the patch series should either drop > this patch or consolidate the FDT allocation between the arches. > > I just tested on powernv and pseries platforms and powerpc can use > vmalloc for the FDT buffer. > Thanks for verifying on powerpc platform Thiago. I'll update the patch to do the following: => Use vmalloc for FDT buffer allocation on powerpc => Keep vmalloc for arm64, but remove the retry on allocation. => Also, there was a memory leak of FDT buffer in the error code path on arm64, which I'll fix as well. Did I miss anything? thanks, -lakshmi
Lakshmi Ramasubramanian <nramas@linux.microsoft.com> writes: > On 1/27/21 7:52 PM, Thiago Jung Bauermann wrote: >> Will Deacon <will@kernel.org> writes: >> >>> On Wed, Jan 27, 2021 at 09:59:38AM -0800, Lakshmi Ramasubramanian wrote: >>>> On 1/27/21 8:52 AM, Will Deacon wrote: >>>> >>>> Hi Will, >>>> >>>>> On Fri, Jan 15, 2021 at 09:30:16AM -0800, Lakshmi Ramasubramanian wrote: >>>>>> create_dtb() function allocates kernel virtual memory for >>>>>> the device tree blob (DTB). This is not consistent with other >>>>>> architectures, such as powerpc, which calls kmalloc() for allocating >>>>>> memory for the DTB. >>>>>> >>>>>> Call kmalloc() to allocate memory for the DTB, and kfree() to free >>>>>> the allocated memory. >>>>>> >>>>>> Co-developed-by: Prakhar Srivastava <prsriva@linux.microsoft.com> >>>>>> Signed-off-by: Prakhar Srivastava <prsriva@linux.microsoft.com> >>>>>> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com> >>>>>> --- >>>>>> arch/arm64/kernel/machine_kexec_file.c | 12 +++++++----- >>>>>> 1 file changed, 7 insertions(+), 5 deletions(-) >>>>>> >>>>>> diff --git a/arch/arm64/kernel/machine_kexec_file.c b/arch/arm64/kernel/machine_kexec_file.c >>>>>> index 7de9c47dee7c..51c40143d6fa 100644 >>>>>> --- a/arch/arm64/kernel/machine_kexec_file.c >>>>>> +++ b/arch/arm64/kernel/machine_kexec_file.c >>>>>> @@ -29,7 +29,7 @@ const struct kexec_file_ops * const kexec_file_loaders[] = { >>>>>> int arch_kimage_file_post_load_cleanup(struct kimage *image) >>>>>> { >>>>>> - vfree(image->arch.dtb); >>>>>> + kfree(image->arch.dtb); >>>>>> image->arch.dtb = NULL; >>>>>> vfree(image->arch.elf_headers); >>>>>> @@ -59,19 +59,21 @@ static int create_dtb(struct kimage *image, >>>>>> + cmdline_len + DTB_EXTRA_SPACE; >>>>>> for (;;) { >>>>>> - buf = vmalloc(buf_size); >>>>>> + buf = kmalloc(buf_size, GFP_KERNEL); >>>>> >>>>> Is there a functional need for this patch? I build the 'dtbs' target just >>>>> now and sdm845-db845c.dtb is approaching 100K, which feels quite large >>>>> for kmalloc(). >>>> >>>> Changing the allocation from vmalloc() to kmalloc() would help us further >>>> consolidate the DTB setup code for powerpc and arm64. >>> >>> Ok, but at the risk of allocation failure. Can powerpc use vmalloc() >>> instead? >> I believe this patch stems from this suggestion by Rob Herring: >> >>> This could be taken a step further and do the allocation of the new >>> FDT. The difference is arm64 uses vmalloc and powerpc uses kmalloc. The >>> arm64 version also retries with a bigger allocation. That seems >>> unnecessary. >> in >> https://lore.kernel.org/linux-integrity/20201211221006.1052453-3-robh@kernel.org/ >> The problem is that this patch implements only part of the suggestion, >> which isn't useful in itself. So the patch series should either drop >> this patch or consolidate the FDT allocation between the arches. >> I just tested on powernv and pseries platforms and powerpc can use >> vmalloc for the FDT buffer. >> > > Thanks for verifying on powerpc platform Thiago. > > I'll update the patch to do the following: > > => Use vmalloc for FDT buffer allocation on powerpc > => Keep vmalloc for arm64, but remove the retry on allocation. > => Also, there was a memory leak of FDT buffer in the error code path on arm64, > which I'll fix as well. > > Did I miss anything? Yes, you missed the second part of Rob's suggestion I was mentioning, which is factoring out the code which allocates the new FDT from both arm64 and powerpc. -- Thiago Jung Bauermann IBM Linux Technology Center
On 1/27/21 8:14 PM, Thiago Jung Bauermann wrote: > > Lakshmi Ramasubramanian <nramas@linux.microsoft.com> writes: > >> On 1/27/21 7:52 PM, Thiago Jung Bauermann wrote: >>> Will Deacon <will@kernel.org> writes: >>> >>>> On Wed, Jan 27, 2021 at 09:59:38AM -0800, Lakshmi Ramasubramanian wrote: >>>>> On 1/27/21 8:52 AM, Will Deacon wrote: >>>>> >>>>> Hi Will, >>>>> >>>>>> On Fri, Jan 15, 2021 at 09:30:16AM -0800, Lakshmi Ramasubramanian wrote: >>>>>>> create_dtb() function allocates kernel virtual memory for >>>>>>> the device tree blob (DTB). This is not consistent with other >>>>>>> architectures, such as powerpc, which calls kmalloc() for allocating >>>>>>> memory for the DTB. >>>>>>> >>>>>>> Call kmalloc() to allocate memory for the DTB, and kfree() to free >>>>>>> the allocated memory. >>>>>>> >>>>>>> Co-developed-by: Prakhar Srivastava <prsriva@linux.microsoft.com> >>>>>>> Signed-off-by: Prakhar Srivastava <prsriva@linux.microsoft.com> >>>>>>> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com> >>>>>>> --- >>>>>>> arch/arm64/kernel/machine_kexec_file.c | 12 +++++++----- >>>>>>> 1 file changed, 7 insertions(+), 5 deletions(-) >>>>>>> >>>>>>> diff --git a/arch/arm64/kernel/machine_kexec_file.c b/arch/arm64/kernel/machine_kexec_file.c >>>>>>> index 7de9c47dee7c..51c40143d6fa 100644 >>>>>>> --- a/arch/arm64/kernel/machine_kexec_file.c >>>>>>> +++ b/arch/arm64/kernel/machine_kexec_file.c >>>>>>> @@ -29,7 +29,7 @@ const struct kexec_file_ops * const kexec_file_loaders[] = { >>>>>>> int arch_kimage_file_post_load_cleanup(struct kimage *image) >>>>>>> { >>>>>>> - vfree(image->arch.dtb); >>>>>>> + kfree(image->arch.dtb); >>>>>>> image->arch.dtb = NULL; >>>>>>> vfree(image->arch.elf_headers); >>>>>>> @@ -59,19 +59,21 @@ static int create_dtb(struct kimage *image, >>>>>>> + cmdline_len + DTB_EXTRA_SPACE; >>>>>>> for (;;) { >>>>>>> - buf = vmalloc(buf_size); >>>>>>> + buf = kmalloc(buf_size, GFP_KERNEL); >>>>>> >>>>>> Is there a functional need for this patch? I build the 'dtbs' target just >>>>>> now and sdm845-db845c.dtb is approaching 100K, which feels quite large >>>>>> for kmalloc(). >>>>> >>>>> Changing the allocation from vmalloc() to kmalloc() would help us further >>>>> consolidate the DTB setup code for powerpc and arm64. >>>> >>>> Ok, but at the risk of allocation failure. Can powerpc use vmalloc() >>>> instead? >>> I believe this patch stems from this suggestion by Rob Herring: >>> >>>> This could be taken a step further and do the allocation of the new >>>> FDT. The difference is arm64 uses vmalloc and powerpc uses kmalloc. The >>>> arm64 version also retries with a bigger allocation. That seems >>>> unnecessary. >>> in >>> https://lore.kernel.org/linux-integrity/20201211221006.1052453-3-robh@kernel.org/ >>> The problem is that this patch implements only part of the suggestion, >>> which isn't useful in itself. So the patch series should either drop >>> this patch or consolidate the FDT allocation between the arches. >>> I just tested on powernv and pseries platforms and powerpc can use >>> vmalloc for the FDT buffer. >>> >> >> Thanks for verifying on powerpc platform Thiago. >> >> I'll update the patch to do the following: >> >> => Use vmalloc for FDT buffer allocation on powerpc >> => Keep vmalloc for arm64, but remove the retry on allocation. >> => Also, there was a memory leak of FDT buffer in the error code path on arm64, >> which I'll fix as well. >> >> Did I miss anything? > > Yes, you missed the second part of Rob's suggestion I was mentioning, > which is factoring out the code which allocates the new FDT from both > arm64 and powerpc. > Sure - I'll address that. thanks, -lakshmi
On Thu, 2021-01-28 at 00:52 -0300, Thiago Jung Bauermann wrote: > The problem is that this patch implements only part of the suggestion, > which isn't useful in itself. So the patch series should either drop > this patch or consolidate the FDT allocation between the arches. > > I just tested on powernv and pseries platforms and powerpc can use > vmalloc for the FDT buffer. Perhaps more sensible to use kvmalloc/kvfree.
Joe Perches <joe@perches.com> writes: > On Thu, 2021-01-28 at 00:52 -0300, Thiago Jung Bauermann wrote: >> The problem is that this patch implements only part of the suggestion, >> which isn't useful in itself. So the patch series should either drop >> this patch or consolidate the FDT allocation between the arches. >> >> I just tested on powernv and pseries platforms and powerpc can use >> vmalloc for the FDT buffer. > > Perhaps more sensible to use kvmalloc/kvfree. That's true. Converting both arm64 to powerpc to kvmalloc/kvfree is a good option. I don't think it's that much better though, because kexec_file_load() is called infrequently and doesn't need to be fast so the vmalloc() overhead isn't important in practice. -- Thiago Jung Bauermann IBM Linux Technology Center
diff --git a/arch/arm64/kernel/machine_kexec_file.c b/arch/arm64/kernel/machine_kexec_file.c index 7de9c47dee7c..51c40143d6fa 100644 --- a/arch/arm64/kernel/machine_kexec_file.c +++ b/arch/arm64/kernel/machine_kexec_file.c @@ -29,7 +29,7 @@ const struct kexec_file_ops * const kexec_file_loaders[] = { int arch_kimage_file_post_load_cleanup(struct kimage *image) { - vfree(image->arch.dtb); + kfree(image->arch.dtb); image->arch.dtb = NULL; vfree(image->arch.elf_headers); @@ -59,19 +59,21 @@ static int create_dtb(struct kimage *image, + cmdline_len + DTB_EXTRA_SPACE; for (;;) { - buf = vmalloc(buf_size); + buf = kmalloc(buf_size, GFP_KERNEL); if (!buf) return -ENOMEM; /* duplicate a device tree blob */ ret = fdt_open_into(initial_boot_params, buf, buf_size); - if (ret) + if (ret) { + kfree(buf); return -EINVAL; + } ret = of_kexec_setup_new_fdt(image, buf, initrd_load_addr, initrd_len, cmdline); if (ret) { - vfree(buf); + kfree(buf); if (ret == -ENOMEM) { /* unlikely, but just in case */ buf_size += DTB_EXTRA_SPACE; @@ -217,6 +219,6 @@ int load_other_segments(struct kimage *image, return 0; out_err: - vfree(dtb); + kfree(dtb); return ret; }