Re: [PATCH v15 09/10] arm64: Call kmalloc() to allocate DTB buffer

2021-02-01 Thread Thiago Jung Bauermann 


Joe Perches  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

Re: [PATCH v15 09/10] arm64: Call kmalloc() to allocate DTB buffer

2021-01-27 Thread Joe Perches 
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.

Re: [PATCH v15 09/10] arm64: Call kmalloc() to allocate DTB buffer

2021-01-27 Thread Lakshmi Ramasubramanian 

On 1/27/21 8:14 PM, Thiago Jung Bauermann wrote:


Lakshmi Ramasubramanian  writes:


On 1/27/21 7:52 PM, Thiago Jung Bauermann wrote:

Will Deacon  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 
Signed-off-by: Prakhar Srivastava 
Signed-off-by: Lakshmi Ramasubramanian 
---
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-r...@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

Re: [PATCH v15 09/10] arm64: Call kmalloc() to allocate DTB buffer

2021-01-27 Thread Thiago Jung Bauermann 


Lakshmi Ramasubramanian  writes:

> On 1/27/21 7:52 PM, Thiago Jung Bauermann wrote:
>> Will Deacon  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 
>> Signed-off-by: Prakhar Srivastava 
>> Signed-off-by: Lakshmi Ramasubramanian 
>> ---
>>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-r...@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

Re: [PATCH v15 09/10] arm64: Call kmalloc() to allocate DTB buffer

2021-01-27 Thread Lakshmi Ramasubramanian 

On 1/27/21 7:52 PM, Thiago Jung Bauermann wrote:


Will Deacon  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 
Signed-off-by: Prakhar Srivastava 
Signed-off-by: Lakshmi Ramasubramanian 
---
   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-r...@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

Re: [PATCH v15 09/10] arm64: Call kmalloc() to allocate DTB buffer

2021-01-27 Thread Thiago Jung Bauermann 


Will Deacon  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 
>> > > Signed-off-by: Prakhar Srivastava 
>> > > Signed-off-by: Lakshmi Ramasubramanian 
>> > > ---
>> > >   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-r...@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

Re: [PATCH v15 09/10] arm64: Call kmalloc() to allocate DTB buffer

2021-01-27 Thread Will Deacon 
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 
> > > Signed-off-by: Prakhar Srivastava 
> > > Signed-off-by: Lakshmi Ramasubramanian 
> > > ---
> > >   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

Re: [PATCH v15 09/10] arm64: Call kmalloc() to allocate DTB buffer

2021-01-17 Thread Mimi Zohar 
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 
> Signed-off-by: Prakhar Srivastava 
> Signed-off-by: Lakshmi Ramasubramanian 
> ---
>  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;
>  }

[PATCH v15 09/10] arm64: Call kmalloc() to allocate DTB buffer

2021-01-15 Thread Lakshmi Ramasubramanian 
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 
Signed-off-by: Prakhar Srivastava 
Signed-off-by: Lakshmi Ramasubramanian 
---
 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;
 }
-- 
2.30.0