[v3,2/9] kexec_file: Generalize kexec_add_buffer.

Am Donnerstag, 23 Juni 2016, 10:25:06 schrieb Dave Young:
> On 06/22/16 at 08:30pm, Thiago Jung Bauermann wrote:
> > Am Mittwoch, 22 Juni 2016, 18:20:47 schrieb Dave Young:
> > > The patch looks good, but could the subject be more specific?
> > > 
> > > For example just like the first sentence of the patch descriotion:
> > > Allow architectures to specify their own memory walking function
> > 
> > Ok, What about this? I also changed the description to refer to x86 arch
> > instead of Intel arch.
> 
> It looks good to me.

This version has the struct kexec_buf documentation comments that were
in patch 3/9. I fixed the names of the struct members, and changed their
descriptions to try to be clearer.

On 06/28/16 at 07:18pm, Thiago Jung Bauermann wrote:
> Am Donnerstag, 23 Juni 2016, 10:25:06 schrieb Dave Young:
> > On 06/22/16 at 08:30pm, Thiago Jung Bauermann wrote:
> > > Am Mittwoch, 22 Juni 2016, 18:20:47 schrieb Dave Young:
> > > > The patch looks good, but could the subject be more specific?
> > > > 
> > > > For example just like the first sentence of the patch descriotion:
> > > > Allow architectures to specify their own memory walking function
> > > 
> > > Ok, What about this? I also changed the description to refer to x86 arch
> > > instead of Intel arch.
> > 
> > It looks good to me.
> 
> This version has the struct kexec_buf documentation comments that were
> in patch 3/9. I fixed the names of the struct members, and changed their
> descriptions to try to be clearer.
> -- 
> []'s
> Thiago Jung Bauermann
> IBM Linux Technology Center
> 
> 
> Subject: [PATCH 2/9] kexec_file: Allow arch-specific memory walking for
>  kexec_add_buffer
> 
> Allow architectures to specify a different memory walking function for
> kexec_add_buffer. x86 uses iomem to track reserved memory ranges, but
> PowerPC uses the memblock subsystem.
> 
> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
> Cc: Eric Biederman <ebiederm@xmission.com>
> Cc: Dave Young <dyoung@redhat.com>
> Cc: kexec@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  include/linux/kexec.h   | 25 ++++++++++++++++++++++++-
>  kernel/kexec_file.c     | 30 ++++++++++++++++++++++--------
>  kernel/kexec_internal.h | 14 --------------
>  3 files changed, 46 insertions(+), 23 deletions(-)
> 
> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> index e8acb2b43dd9..e16d845d587f 100644
> --- a/include/linux/kexec.h
> +++ b/include/linux/kexec.h
> @@ -146,7 +146,30 @@ struct kexec_file_ops {
>  	kexec_verify_sig_t *verify_sig;
>  #endif
>  };
> -#endif
> +
> +/**
> + * struct kexec_buf - parameters for finding a place for a buffer in memory
> + * @image:	kexec image in which memory to search.
> + * @mem:	On return will have address of the buffer in memory.
> + * @memsz:	Size for the buffer in memory.
> + * @buf_align:	Minimum alignment needed.
> + * @buf_min:	The buffer can't be placed below this address.
> + * @buf_max:	The buffer can't be placed above this address.
> + * @top_down:	Allocate from top of memory.
> + */
> +struct kexec_buf {
> +	struct kimage *image;
> +	unsigned long mem;
> +	unsigned long memsz;
> +	unsigned long buf_align;
> +	unsigned long buf_min;
> +	unsigned long buf_max;
> +	bool top_down;
> +};

Rethink about the first patch, you dropped the user buffer in kexec_buf
But later your passing IMA digests buffer patchset may need use it.

So keep it in kexec_buf should be better.

For the IMA buffer patchset I'm still reading and learning the
background, will reply them later.

> +
> +int __weak arch_kexec_walk_mem(struct kexec_buf *kbuf,
> +			       int (*func)(u64, u64, void *));
> +#endif /* CONFIG_KEXEC_FILE */
>  
>  struct kimage {
>  	kimage_entry_t head;
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index b6eec7527e9f..b1f1f6402518 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -428,6 +428,27 @@ static int locate_mem_hole_callback(u64 start, u64 end, void *arg)
>  	return locate_mem_hole_bottom_up(start, end, kbuf);
>  }
>  
> +/**
> + * arch_kexec_walk_mem - call func(data) on free memory regions
> + * @kbuf:	Context info for the search. Also passed to @func.
> + * @func:	Function to call for each memory region.
> + *
> + * Return: The memory walk will stop when func returns a non-zero value
> + * and that value will be returned. If all free regions are visited without
> + * func returning non-zero, then zero will be returned.
> + */
> +int __weak arch_kexec_walk_mem(struct kexec_buf *kbuf,
> +			       int (*func)(u64, u64, void *))
> +{
> +	if (kbuf->image->type == KEXEC_TYPE_CRASH)
> +		return walk_iomem_res_desc(crashk_res.desc,
> +					   IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY,
> +					   crashk_res.start, crashk_res.end,
> +					   kbuf, func);
> +	else
> +		return walk_system_ram_res(0, ULONG_MAX, kbuf, func);
> +}
> +
>  /*
>   * Helper function for placing a buffer in a kexec segment. This assumes
>   * that kexec_mutex is held.
> @@ -472,14 +493,7 @@ int kexec_add_buffer(struct kimage *image, char *buffer, unsigned long bufsz,
>  	kbuf->top_down = top_down;
>  
>  	/* Walk the RAM ranges and allocate a suitable range for the buffer */
> -	if (image->type == KEXEC_TYPE_CRASH)
> -		ret = walk_iomem_res_desc(crashk_res.desc,
> -				IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY,
> -				crashk_res.start, crashk_res.end, kbuf,
> -				locate_mem_hole_callback);
> -	else
> -		ret = walk_system_ram_res(0, -1, kbuf,
> -					  locate_mem_hole_callback);
> +	ret = arch_kexec_walk_mem(kbuf, locate_mem_hole_callback);
>  	if (ret != 1) {
>  		/* A suitable memory range could not be found for buffer */
>  		return -EADDRNOTAVAIL;
> diff --git a/kernel/kexec_internal.h b/kernel/kexec_internal.h
> index eefd5bf960c2..4cef7e4706b0 100644
> --- a/kernel/kexec_internal.h
> +++ b/kernel/kexec_internal.h
> @@ -20,20 +20,6 @@ struct kexec_sha_region {
>  	unsigned long len;
>  };
>  
> -/*
> - * Keeps track of buffer parameters as provided by caller for requesting
> - * memory placement of buffer.
> - */
> -struct kexec_buf {
> -	struct kimage *image;
> -	unsigned long mem;
> -	unsigned long memsz;
> -	unsigned long buf_align;
> -	unsigned long buf_min;
> -	unsigned long buf_max;
> -	bool top_down;		/* allocate from top of memory hole */
> -};
> -
>  void kimage_file_post_load_cleanup(struct kimage *image);
>  #else /* CONFIG_KEXEC_FILE */
>  static inline void kimage_file_post_load_cleanup(struct kimage *image) { }
> -- 
> 1.9.1
> 
> 

Thanks
Dave

Am Mittwoch, 29 Juni 2016, 15:47:51 schrieb Dave Young:
> On 06/28/16 at 07:18pm, Thiago Jung Bauermann wrote:
> > diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> > index e8acb2b43dd9..e16d845d587f 100644
> > --- a/include/linux/kexec.h
> > +++ b/include/linux/kexec.h
> > @@ -146,7 +146,30 @@ struct kexec_file_ops {
> > 
> >       kexec_verify_sig_t *verify_sig;
> >  
> >  #endif
> >  };
> > 
> > -#endif
> > +
> > +/**
> > + * struct kexec_buf - parameters for finding a place for a buffer in
> > memory + * @image:   kexec image in which memory to search.
> > + * @mem:     On return will have address of the buffer in memory.
> > + * @memsz:   Size for the buffer in memory.
> > + * @buf_align:       Minimum alignment needed.
> > + * @buf_min: The buffer can't be placed below this address.
> > + * @buf_max: The buffer can't be placed above this address.
> > + * @top_down:        Allocate from top of memory.
> > + */
> > +struct kexec_buf {
> > +     struct kimage *image;
> > +     unsigned long mem;
> > +     unsigned long memsz;
> > +     unsigned long buf_align;
> > +     unsigned long buf_min;
> > +     unsigned long buf_max;
> > +     bool top_down;
> > +};
> 
> Rethink about the first patch, you dropped the user buffer in kexec_buf
> But later your passing IMA digests buffer patchset may need use it.
> 
> So keep it in kexec_buf should be better.

I'm not following. The IMA buffer patchset doesn't use kexec_locate_mem_hole 
nor struct kexec_buf.

> For the IMA buffer patchset I'm still reading and learning the
> background, will reply them later.

Thank you!

[]'s
Thiago Jung Bauermann
IBM Linux Technology Center

On 06/29/16 at 06:18pm, Thiago Jung Bauermann wrote:
> Am Mittwoch, 29 Juni 2016, 15:47:51 schrieb Dave Young:
> > On 06/28/16 at 07:18pm, Thiago Jung Bauermann wrote:
> > > diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> > > index e8acb2b43dd9..e16d845d587f 100644
> > > --- a/include/linux/kexec.h
> > > +++ b/include/linux/kexec.h
> > > @@ -146,7 +146,30 @@ struct kexec_file_ops {
> > > 
> > >       kexec_verify_sig_t *verify_sig;
> > >  
> > >  #endif
> > >  };
> > > 
> > > -#endif
> > > +
> > > +/**
> > > + * struct kexec_buf - parameters for finding a place for a buffer in
> > > memory + * @image:   kexec image in which memory to search.
> > > + * @mem:     On return will have address of the buffer in memory.
> > > + * @memsz:   Size for the buffer in memory.
> > > + * @buf_align:       Minimum alignment needed.
> > > + * @buf_min: The buffer can't be placed below this address.
> > > + * @buf_max: The buffer can't be placed above this address.
> > > + * @top_down:        Allocate from top of memory.
> > > + */
> > > +struct kexec_buf {
> > > +     struct kimage *image;
> > > +     unsigned long mem;
> > > +     unsigned long memsz;
> > > +     unsigned long buf_align;
> > > +     unsigned long buf_min;
> > > +     unsigned long buf_max;
> > > +     bool top_down;
> > > +};
> > 
> > Rethink about the first patch, you dropped the user buffer in kexec_buf
> > But later your passing IMA digests buffer patchset may need use it.
> > 
> > So keep it in kexec_buf should be better.
> 
> I'm not following. The IMA buffer patchset doesn't use kexec_locate_mem_hole 
> nor struct kexec_buf.

It does not use kexec_locate_mem_hole, but the buffer being passed is
very similar to a kexec_buf struct, no? 

So you may refactor kexec_add_buffer and your new function to pass only kimage
and a kbuf, it will be better than passing all those arguments separately.

> 
> > For the IMA buffer patchset I'm still reading and learning the
> > background, will reply them later.
> 
> Thank you!
> 
> []'s
> Thiago Jung Bauermann
> IBM Linux Technology Center
> 

Thanks
Dave

Am Donnerstag, 30 Juni 2016, 11:07:00 schrieb Dave Young:
> On 06/29/16 at 06:18pm, Thiago Jung Bauermann wrote:
> > Am Mittwoch, 29 Juni 2016, 15:47:51 schrieb Dave Young:
> > > On 06/28/16 at 07:18pm, Thiago Jung Bauermann wrote:
> > > > +/**
> > > > + * struct kexec_buf - parameters for finding a place for a buffer
> > > > in
> > > > memory + * @image:   kexec image in which memory to search.
> > > > + * @mem:     On return will have address of the buffer in memory.
> > > > + * @memsz:   Size for the buffer in memory.
> > > > + * @buf_align:       Minimum alignment needed.
> > > > + * @buf_min: The buffer can't be placed below this address.
> > > > + * @buf_max: The buffer can't be placed above this address.
> > > > + * @top_down:        Allocate from top of memory.
> > > > + */
> > > > +struct kexec_buf {
> > > > +     struct kimage *image;
> > > > +     unsigned long mem;
> > > > +     unsigned long memsz;
> > > > +     unsigned long buf_align;
> > > > +     unsigned long buf_min;
> > > > +     unsigned long buf_max;
> > > > +     bool top_down;
> > > > +};
> > > 
> > > Rethink about the first patch, you dropped the user buffer in
> > > kexec_buf
> > > But later your passing IMA digests buffer patchset may need use it.
> > > 
> > > So keep it in kexec_buf should be better.
> > 
> > I'm not following. The IMA buffer patchset doesn't use
> > kexec_locate_mem_hole nor struct kexec_buf.
> 
> It does not use kexec_locate_mem_hole, but the buffer being passed is
> very similar to a kexec_buf struct, no?

If what you're saying is that the arguments passed to 
kexec_add_handover_buffer in the IMA buffer patchset are very similar to the 
arguments passed to kexec_add_buffer then yes, it's true.

> So you may refactor kexec_add_buffer and your new function to pass only
> kimage and a kbuf, it will be better than passing all those arguments
> separately.

To be honest I think struct kexec_buf is an implementation detail inside 
kexec_locate_mem_hole, made necessary because the callback functions it uses 
need to access its arguments. Callers of kexec_locate_mem_hole, 
kexec_add_handover_buffer and kexec_add_buffer shouldn't need to know it 
exists.

[]'s
Thiago Jung Bauermann
IBM Linux Technology Center

Am Donnerstag, 30 Juni 2016, 12:49:44 schrieb Thiago Jung Bauermann:
> Am Donnerstag, 30 Juni 2016, 11:07:00 schrieb Dave Young:
> > On 06/29/16 at 06:18pm, Thiago Jung Bauermann wrote:
> > > I'm not following. The IMA buffer patchset doesn't use
> > > kexec_locate_mem_hole nor struct kexec_buf.
> > 
> > It does not use kexec_locate_mem_hole, but the buffer being passed is
> > very similar to a kexec_buf struct, no?
> 
> If what you're saying is that the arguments passed to
> kexec_add_handover_buffer in the IMA buffer patchset are very similar to
> the arguments passed to kexec_add_buffer then yes, it's true.
> 
> > So you may refactor kexec_add_buffer and your new function to pass only
> > kimage and a kbuf, it will be better than passing all those arguments
> > separately.
> 
> To be honest I think struct kexec_buf is an implementation detail inside
> kexec_locate_mem_hole, made necessary because the callback functions it
> uses need to access its arguments. Callers of kexec_locate_mem_hole,
> kexec_add_handover_buffer and kexec_add_buffer shouldn't need to know it
> exists.

Elaborating a bit more: the argument list for these three functions are 
equal or similar because kexec_add_handover_buffer uses kexec_add_buffer, 
which uses kexec_locate_mem_hole.

It could be beneficial to have a struct to collect the arguments to these 
functions if someone using one of them would be likely to use another one 
with the same arguments. In that case, you set up kexec_buf once and then 
just pass it whenever you need to call one of those functions.

But that is unlikely to happen. A user of the kexec API will need to use 
only one of these functions with a given set of arguments, so they don't 
gain anything by setting up a struct.

Syntactically, I also don't think it's clearer to set struct members instead 
of simply passing arguments to a function, even if the argument list is 
long.

[]'s
Thiago Jung Bauermann
IBM Linux Technology Center

On 06/30/16 at 01:42pm, Thiago Jung Bauermann wrote:
> Am Donnerstag, 30 Juni 2016, 12:49:44 schrieb Thiago Jung Bauermann:
> > Am Donnerstag, 30 Juni 2016, 11:07:00 schrieb Dave Young:
> > > On 06/29/16 at 06:18pm, Thiago Jung Bauermann wrote:
> > > > I'm not following. The IMA buffer patchset doesn't use
> > > > kexec_locate_mem_hole nor struct kexec_buf.
> > > 
> > > It does not use kexec_locate_mem_hole, but the buffer being passed is
> > > very similar to a kexec_buf struct, no?
> > 
> > If what you're saying is that the arguments passed to
> > kexec_add_handover_buffer in the IMA buffer patchset are very similar to
> > the arguments passed to kexec_add_buffer then yes, it's true.
> > 
> > > So you may refactor kexec_add_buffer and your new function to pass only
> > > kimage and a kbuf, it will be better than passing all those arguments
> > > separately.
> > 
> > To be honest I think struct kexec_buf is an implementation detail inside
> > kexec_locate_mem_hole, made necessary because the callback functions it
> > uses need to access its arguments. Callers of kexec_locate_mem_hole,
> > kexec_add_handover_buffer and kexec_add_buffer shouldn't need to know it
> > exists.
> 
> Elaborating a bit more: the argument list for these three functions are 
> equal or similar because kexec_add_handover_buffer uses kexec_add_buffer, 
> which uses kexec_locate_mem_hole.
> 
> It could be beneficial to have a struct to collect the arguments to these 
> functions if someone using one of them would be likely to use another one 
> with the same arguments. In that case, you set up kexec_buf once and then 
> just pass it whenever you need to call one of those functions.
> 
> But that is unlikely to happen. A user of the kexec API will need to use 
> only one of these functions with a given set of arguments, so they don't 
> gain anything by setting up a struct.
> 
> Syntactically, I also don't think it's clearer to set struct members instead 
> of simply passing arguments to a function, even if the argument list is 
> long.

Sorry, I'm not sure I get your points but the long argument list really looks ugly,
since you are introducing more callbacks I still think a cleanup is necessary.

kexec_buffer struct is pretty fine to be a abstract of all these buffers.

Thanks
Dave

Am Donnerstag, 30 Juni 2016, 17:43:57 schrieb Dave Young:
> On 06/30/16 at 01:42pm, Thiago Jung Bauermann wrote:
> > Am Donnerstag, 30 Juni 2016, 12:49:44 schrieb Thiago Jung Bauermann:
> > > To be honest I think struct kexec_buf is an implementation detail
> > > inside
> > > kexec_locate_mem_hole, made necessary because the callback functions
> > > it
> > > uses need to access its arguments. Callers of kexec_locate_mem_hole,
> > > kexec_add_handover_buffer and kexec_add_buffer shouldn't need to know
> > > it
> > > exists.
> > 
> > Elaborating a bit more: the argument list for these three functions are
> > equal or similar because kexec_add_handover_buffer uses
> > kexec_add_buffer,
> > which uses kexec_locate_mem_hole.
> > 
> > It could be beneficial to have a struct to collect the arguments to
> > these
> > functions if someone using one of them would be likely to use another
> > one
> > with the same arguments. In that case, you set up kexec_buf once and
> > then
> > just pass it whenever you need to call one of those functions.
> > 
> > But that is unlikely to happen. A user of the kexec API will need to use
> > only one of these functions with a given set of arguments, so they don't
> > gain anything by setting up a struct.
> > 
> > Syntactically, I also don't think it's clearer to set struct members
> > instead of simply passing arguments to a function, even if the argument
> > list is long.
> 
> Sorry, I'm not sure I get your points but the long argument list really
> looks ugly, since you are introducing more callbacks I still think a
> cleanup is necessary.
> 
> kexec_buffer struct is pretty fine to be a abstract of all these buffers.

What I understood from what you said is that making the following change
results in code that is easier to understand:

@@ -650,6 +639,7 @@ static int __kexec_load_purgatory(struct kimage *image, unsigned long min,
 	const Elf_Shdr *sechdrs_c;
 	Elf_Shdr *sechdrs = NULL;
 	void *purgatory_buf = NULL;
+	struct kexec_buf buf;
 
 	/*
 	 * sechdrs_c points to section headers in purgatory and are read
@@ -757,11 +747,19 @@ static int __kexec_load_purgatory(struct kimage *image, unsigned long min,
 		buf_align = bss_align;
 
 	/* Add buffer to segment list */
-	ret = kexec_add_buffer(image, purgatory_buf, buf_sz, memsz,
-				buf_align, min, max, top_down,
-				&pi->purgatory_load_addr);
+	memset(&buf, 0, sizeof(struct kexec_buf));
+	buf.image = image;
+	buf.buffer = purgatory_buf;
+	buf.bufsz = buf_sz,
+	buf.memsz = memsz;
+	buf.buf_align = buf_align;
+	buf.buf_min = min;
+	buf.buf_max = max;
+	buf.top_down = top_down;
+	ret = kexec_add_buffer(&buf);
 	if (ret)
 		goto out;
+	pi->purgatory_load_addr = buf.mem;
 
 	/* Load SHF_ALLOC sections */
 	buf_addr = purgatory_buf;

There are 9 calls to kexec_add_buffer in the kernel (including arch/x86,
arch/powerpc/ and kernel/), plus 1 to kexec_locate_mem_hole
and 1 to kexec_add_handover_buffer, so there would be 11 places in
the code settings up kexec_buf. My opinion is that this change doesn't
improve code readability.

Also, I think that kexec_buf abstracts something that, from the
perspective of the user of the kexec API, lives only for the duration
of a single call to either of kexec_add_buffer, kexec_locate_mem_hole,
or kexec_add_handover_buffer. Because of this, there's no need from the
perspective of the API user to initialize this "object", so this just
adds to their cognitive load without any benefit to them.

I understand that this is all somewhat subjective, so if you still disagree
with my points I can provide a patch set implementing the change above.

[]'s
Thiago Jung Bauermann
IBM Linux Technology Center

On 07/01/16 at 02:51pm, Thiago Jung Bauermann wrote:
> Am Donnerstag, 30 Juni 2016, 17:43:57 schrieb Dave Young:
> > On 06/30/16 at 01:42pm, Thiago Jung Bauermann wrote:
> > > Am Donnerstag, 30 Juni 2016, 12:49:44 schrieb Thiago Jung Bauermann:
> > > > To be honest I think struct kexec_buf is an implementation detail
> > > > inside
> > > > kexec_locate_mem_hole, made necessary because the callback functions
> > > > it
> > > > uses need to access its arguments. Callers of kexec_locate_mem_hole,
> > > > kexec_add_handover_buffer and kexec_add_buffer shouldn't need to know
> > > > it
> > > > exists.
> > > 
> > > Elaborating a bit more: the argument list for these three functions are
> > > equal or similar because kexec_add_handover_buffer uses
> > > kexec_add_buffer,
> > > which uses kexec_locate_mem_hole.
> > > 
> > > It could be beneficial to have a struct to collect the arguments to
> > > these
> > > functions if someone using one of them would be likely to use another
> > > one
> > > with the same arguments. In that case, you set up kexec_buf once and
> > > then
> > > just pass it whenever you need to call one of those functions.
> > > 
> > > But that is unlikely to happen. A user of the kexec API will need to use
> > > only one of these functions with a given set of arguments, so they don't
> > > gain anything by setting up a struct.
> > > 
> > > Syntactically, I also don't think it's clearer to set struct members
> > > instead of simply passing arguments to a function, even if the argument
> > > list is long.
> > 
> > Sorry, I'm not sure I get your points but the long argument list really
> > looks ugly, since you are introducing more callbacks I still think a
> > cleanup is necessary.
> > 
> > kexec_buffer struct is pretty fine to be a abstract of all these buffers.
> 
> What I understood from what you said is that making the following change
> results in code that is easier to understand:
> 
> @@ -650,6 +639,7 @@ static int __kexec_load_purgatory(struct kimage *image, unsigned long min,
>  	const Elf_Shdr *sechdrs_c;
>  	Elf_Shdr *sechdrs = NULL;
>  	void *purgatory_buf = NULL;
> +	struct kexec_buf buf;
>  
>  	/*
>  	 * sechdrs_c points to section headers in purgatory and are read
> @@ -757,11 +747,19 @@ static int __kexec_load_purgatory(struct kimage *image, unsigned long min,
>  		buf_align = bss_align;
>  
>  	/* Add buffer to segment list */
> -	ret = kexec_add_buffer(image, purgatory_buf, buf_sz, memsz,
> -				buf_align, min, max, top_down,
> -				&pi->purgatory_load_addr);
> +	memset(&buf, 0, sizeof(struct kexec_buf));
> +	buf.image = image;
> +	buf.buffer = purgatory_buf;
> +	buf.bufsz = buf_sz,
> +	buf.memsz = memsz;
> +	buf.buf_align = buf_align;
> +	buf.buf_min = min;
> +	buf.buf_max = max;
> +	buf.top_down = top_down;
> +	ret = kexec_add_buffer(&buf);
>  	if (ret)
>  		goto out;
> +	pi->purgatory_load_addr = buf.mem;
>  
>  	/* Load SHF_ALLOC sections */
>  	buf_addr = purgatory_buf;
> 
> There are 9 calls to kexec_add_buffer in the kernel (including arch/x86,
> arch/powerpc/ and kernel/), plus 1 to kexec_locate_mem_hole
> and 1 to kexec_add_handover_buffer, so there would be 11 places in
> the code settings up kexec_buf. My opinion is that this change doesn't
> improve code readability.

But the assignment can be moved to the beginning of the function
__kexec_load_purgatory, and avoid the local variables from the very
beginning. Just use kbuf.member instead.

> 
> Also, I think that kexec_buf abstracts something that, from the
> perspective of the user of the kexec API, lives only for the duration
> of a single call to either of kexec_add_buffer, kexec_locate_mem_hole,
> or kexec_add_handover_buffer. Because of this, there's no need from the
> perspective of the API user to initialize this "object", so this just
> adds to their cognitive load without any benefit to them.
> 
> I understand that this is all somewhat subjective, so if you still disagree
> with my points I can provide a patch set implementing the change above.

I still feel it should be changed if more callbacks being introduced,
though you can regard it is internal api, like above comment we do not
need to assign them seperately, the member values can be assigned
from the beginning.

Thanks
Dave

Am Freitag, 01 Juli 2016, 14:36:02 schrieb Dave Young:
> On 07/01/16 at 02:51pm, Thiago Jung Bauermann wrote:
> > Am Donnerstag, 30 Juni 2016, 17:43:57 schrieb Dave Young:
> > > On 06/30/16 at 01:42pm, Thiago Jung Bauermann wrote:
> > > > Am Donnerstag, 30 Juni 2016, 12:49:44 schrieb Thiago Jung Bauermann:
> > I understand that this is all somewhat subjective, so if you still
> > disagree with my points I can provide a patch set implementing the
> > change above.
> I still feel it should be changed if more callbacks being introduced,
> though you can regard it is internal api, like above comment we do not
> need to assign them seperately, the member values can be assigned
> from the beginning.

Ok, I'll implement the changes and submit a v4. Thanks for your review.

[]'s
Thiago Jung Bauermann
IBM Linux Technology Center

Am Freitag, 01 Juli 2016, 17:02:23 schrieb Thiago Jung Bauermann:
> Am Freitag, 01 Juli 2016, 14:36:02 schrieb Dave Young:
> > On 07/01/16 at 02:51pm, Thiago Jung Bauermann wrote:
> > > Am Donnerstag, 30 Juni 2016, 17:43:57 schrieb Dave Young:
> > > > On 06/30/16 at 01:42pm, Thiago Jung Bauermann wrote:
> > > > > Am Donnerstag, 30 Juni 2016, 12:49:44 schrieb Thiago Jung Bauermann:
> > > I understand that this is all somewhat subjective, so if you still
> > > disagree with my points I can provide a patch set implementing the
> > > change above.
> > 
> > I still feel it should be changed if more callbacks being introduced,
> > though you can regard it is internal api, like above comment we do not
> > need to assign them seperately, the member values can be assigned
> > from the beginning.
> 
> Ok, I'll implement the changes and submit a v4. Thanks for your review.

Sorry for creating more email traffic, but it'll be better if I ask this 
before I change all other places in the code. Is the code below what you
have in mind?

In particular, this version doesn't do the memset(&buf, 0, sizeof(buf))
that the previous code I sent earlier did. Is that ok?

@@ -643,13 +632,14 @@ static int __kexec_load_purgatory(struct kimage *image, unsigned long min,
 				  unsigned long max, int top_down)
 {
 	struct purgatory_info *pi = &image->purgatory_info;
-	unsigned long align, buf_align, bss_align, buf_sz, bss_sz, bss_pad;
-	unsigned long memsz, entry, load_addr, curr_load_addr, bss_addr, offset;
+	unsigned long align, bss_align, bss_sz, bss_pad;
+	unsigned long entry, load_addr, curr_load_addr, bss_addr, offset;
 	unsigned char *buf_addr, *src;
 	int i, ret = 0, entry_sidx = -1;
 	const Elf_Shdr *sechdrs_c;
 	Elf_Shdr *sechdrs = NULL;
-	void *purgatory_buf = NULL;
+	struct kexec_buf buf = { .image = image, .buf_min = min,
+				.buf_max = max, .top_down = top_down };
 
 	/*
 	 * sechdrs_c points to section headers in purgatory and are read
@@ -715,9 +705,9 @@ static int __kexec_load_purgatory(struct kimage *image, unsigned long min,
 	}
 
 	/* Determine how much memory is needed to load relocatable object. */
-	buf_align = 1;
+	buf.buf_align = 1;
 	bss_align = 1;
-	buf_sz = 0;
+	buf.bufsz = 0;
 	bss_sz = 0;
 
 	for (i = 0; i < pi->ehdr->e_shnum; i++) {
@@ -726,10 +716,10 @@ static int __kexec_load_purgatory(struct kimage *image, unsigned long min,
 
 		align = sechdrs[i].sh_addralign;
 		if (sechdrs[i].sh_type != SHT_NOBITS) {
-			if (buf_align < align)
-				buf_align = align;
-			buf_sz = ALIGN(buf_sz, align);
-			buf_sz += sechdrs[i].sh_size;
+			if (buf.buf_align < align)
+				buf.buf_align = align;
+			buf.bufsz = ALIGN(buf.bufsz, align);
+			buf.bufsz += sechdrs[i].sh_size;
 		} else {
 			/* bss section */
 			if (bss_align < align)
@@ -741,32 +731,31 @@ static int __kexec_load_purgatory(struct kimage *image, unsigned long min,
 
 	/* Determine the bss padding required to align bss properly */
 	bss_pad = 0;
-	if (buf_sz & (bss_align - 1))
-		bss_pad = bss_align - (buf_sz & (bss_align - 1));
+	if (buf.bufsz & (bss_align - 1))
+		bss_pad = bss_align - (buf.bufsz & (bss_align - 1));
 
-	memsz = buf_sz + bss_pad + bss_sz;
+	buf.memsz = buf.bufsz + bss_pad + bss_sz;
 
 	/* Allocate buffer for purgatory */
-	purgatory_buf = vzalloc(buf_sz);
-	if (!purgatory_buf) {
+	buf.buffer = vzalloc(buf.bufsz);
+	if (!buf.buffer) {
 		ret = -ENOMEM;
 		goto out;
 	}
 
-	if (buf_align < bss_align)
-		buf_align = bss_align;
+	if (buf.buf_align < bss_align)
+		buf.buf_align = bss_align;
 
 	/* Add buffer to segment list */
-	ret = kexec_add_buffer(image, purgatory_buf, buf_sz, memsz,
-				buf_align, min, max, top_down,
-				&pi->purgatory_load_addr);
+	ret = kexec_add_buffer(&buf);
 	if (ret)
 		goto out;
+	pi->purgatory_load_addr = buf.mem;
 
 	/* Load SHF_ALLOC sections */
-	buf_addr = purgatory_buf;
+	buf_addr = buf.buffer;
 	load_addr = curr_load_addr = pi->purgatory_load_addr;
-	bss_addr = load_addr + buf_sz + bss_pad;
+	bss_addr = load_addr + buf.bufsz + bss_pad;
 
 	for (i = 0; i < pi->ehdr->e_shnum; i++) {
 		if (!(sechdrs[i].sh_flags & SHF_ALLOC))
@@ -812,11 +801,11 @@ static int __kexec_load_purgatory(struct kimage *image, unsigned long min,
 	 * Used later to identify which section is purgatory and skip it
 	 * from checksumming.
 	 */
-	pi->purgatory_buf = purgatory_buf;
+	pi->purgatory_buf = buf.buffer;
 	return ret;
 out:
 	vfree(sechdrs);
-	vfree(purgatory_buf);
+	vfree(buf.buffer);
 	return ret;
 }
 

[]'s
Thiago Jung Bauermann
IBM Linux Technology Center

On 07/01/16 at 05:31pm, Thiago Jung Bauermann wrote:
> Am Freitag, 01 Juli 2016, 17:02:23 schrieb Thiago Jung Bauermann:
> > Am Freitag, 01 Juli 2016, 14:36:02 schrieb Dave Young:
> > > On 07/01/16 at 02:51pm, Thiago Jung Bauermann wrote:
> > > > Am Donnerstag, 30 Juni 2016, 17:43:57 schrieb Dave Young:
> > > > > On 06/30/16 at 01:42pm, Thiago Jung Bauermann wrote:
> > > > > > Am Donnerstag, 30 Juni 2016, 12:49:44 schrieb Thiago Jung Bauermann:
> > > > I understand that this is all somewhat subjective, so if you still
> > > > disagree with my points I can provide a patch set implementing the
> > > > change above.
> > > 
> > > I still feel it should be changed if more callbacks being introduced,
> > > though you can regard it is internal api, like above comment we do not
> > > need to assign them seperately, the member values can be assigned
> > > from the beginning.
> > 
> > Ok, I'll implement the changes and submit a v4. Thanks for your review.
> 
> Sorry for creating more email traffic, but it'll be better if I ask this 
> before I change all other places in the code. Is the code below what you
> have in mind?

Thanks for the update, almost except a nitpick :)

> 
> In particular, this version doesn't do the memset(&buf, 0, sizeof(buf))
> that the previous code I sent earlier did. Is that ok?
> 
> @@ -643,13 +632,14 @@ static int __kexec_load_purgatory(struct kimage *image, unsigned long min,
>  				  unsigned long max, int top_down)
>  {
>  	struct purgatory_info *pi = &image->purgatory_info;
> -	unsigned long align, buf_align, bss_align, buf_sz, bss_sz, bss_pad;
> -	unsigned long memsz, entry, load_addr, curr_load_addr, bss_addr, offset;
> +	unsigned long align, bss_align, bss_sz, bss_pad;
> +	unsigned long entry, load_addr, curr_load_addr, bss_addr, offset;
>  	unsigned char *buf_addr, *src;
>  	int i, ret = 0, entry_sidx = -1;
>  	const Elf_Shdr *sechdrs_c;
>  	Elf_Shdr *sechdrs = NULL;
> -	void *purgatory_buf = NULL;
> +	struct kexec_buf buf = { .image = image, .buf_min = min,
> +				.buf_max = max, .top_down = top_down };
>  
>  	/*
>  	 * sechdrs_c points to section headers in purgatory and are read
> @@ -715,9 +705,9 @@ static int __kexec_load_purgatory(struct kimage *image, unsigned long min,
>  	}
>  
>  	/* Determine how much memory is needed to load relocatable object. */
> -	buf_align = 1;
> +	buf.buf_align = 1;
>  	bss_align = 1;
> -	buf_sz = 0;
> +	buf.bufsz = 0;
>  	bss_sz = 0;

Above chunk can go to the initializatioin of struct kexec buf ealier.

>  
>  	for (i = 0; i < pi->ehdr->e_shnum; i++) {
> @@ -726,10 +716,10 @@ static int __kexec_load_purgatory(struct kimage *image, unsigned long min,
>  
>  		align = sechdrs[i].sh_addralign;
>  		if (sechdrs[i].sh_type != SHT_NOBITS) {
> -			if (buf_align < align)
> -				buf_align = align;
> -			buf_sz = ALIGN(buf_sz, align);
> -			buf_sz += sechdrs[i].sh_size;
> +			if (buf.buf_align < align)
> +				buf.buf_align = align;
> +			buf.bufsz = ALIGN(buf.bufsz, align);
> +			buf.bufsz += sechdrs[i].sh_size;
>  		} else {
>  			/* bss section */
>  			if (bss_align < align)
> @@ -741,32 +731,31 @@ static int __kexec_load_purgatory(struct kimage *image, unsigned long min,
>  
>  	/* Determine the bss padding required to align bss properly */
>  	bss_pad = 0;
> -	if (buf_sz & (bss_align - 1))
> -		bss_pad = bss_align - (buf_sz & (bss_align - 1));
> +	if (buf.bufsz & (bss_align - 1))
> +		bss_pad = bss_align - (buf.bufsz & (bss_align - 1));
>  
> -	memsz = buf_sz + bss_pad + bss_sz;
> +	buf.memsz = buf.bufsz + bss_pad + bss_sz;
>  
>  	/* Allocate buffer for purgatory */
> -	purgatory_buf = vzalloc(buf_sz);
> -	if (!purgatory_buf) {
> +	buf.buffer = vzalloc(buf.bufsz);
> +	if (!buf.buffer) {
>  		ret = -ENOMEM;
>  		goto out;
>  	}
>  
> -	if (buf_align < bss_align)
> -		buf_align = bss_align;
> +	if (buf.buf_align < bss_align)
> +		buf.buf_align = bss_align;
>  
>  	/* Add buffer to segment list */
> -	ret = kexec_add_buffer(image, purgatory_buf, buf_sz, memsz,
> -				buf_align, min, max, top_down,
> -				&pi->purgatory_load_addr);
> +	ret = kexec_add_buffer(&buf);
>  	if (ret)
>  		goto out;
> +	pi->purgatory_load_addr = buf.mem;
>  
>  	/* Load SHF_ALLOC sections */
> -	buf_addr = purgatory_buf;
> +	buf_addr = buf.buffer;
>  	load_addr = curr_load_addr = pi->purgatory_load_addr;
> -	bss_addr = load_addr + buf_sz + bss_pad;
> +	bss_addr = load_addr + buf.bufsz + bss_pad;
>  
>  	for (i = 0; i < pi->ehdr->e_shnum; i++) {
>  		if (!(sechdrs[i].sh_flags & SHF_ALLOC))
> @@ -812,11 +801,11 @@ static int __kexec_load_purgatory(struct kimage *image, unsigned long min,
>  	 * Used later to identify which section is purgatory and skip it
>  	 * from checksumming.
>  	 */
> -	pi->purgatory_buf = purgatory_buf;
> +	pi->purgatory_buf = buf.buffer;
>  	return ret;
>  out:
>  	vfree(sechdrs);
> -	vfree(purgatory_buf);
> +	vfree(buf.buffer);
>  	return ret;
>  }
>  
> 
> []'s
> Thiago Jung Bauermann
> IBM Linux Technology Center
>