diff mbox

[v11,03/15] powerpc, kexec_file: factor out memblock-based arch_kexec_walk_mem()

Message ID 20180711074203.3019-4-takahiro.akashi@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

AKASHI Takahiro July 11, 2018, 7:41 a.m. UTC
Memblock list is another source for usable system memory layout.
So powerpc's arch_kexec_walk_mem() is moved to kexec_file.c so that
other memblock-based architectures, particularly arm64, can also utilise
it. A moved function is now renamed to kexec_walk_memblock() and merged
into the existing arch_kexec_walk_mem() for general use, either resource
list or memblock list.

A consequent function will not work for kdump with memblock list, but
this will be fixed in the next patch.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Dave Young <dyoung@redhat.com>
Cc: Vivek Goyal <vgoyal@redhat.com>
Cc: Baoquan He <bhe@redhat.com>
Acked-by: James Morse <james.morse@arm.com>
---
 arch/powerpc/kernel/machine_kexec_file_64.c | 54 ---------------------
 kernel/kexec_file.c                         | 54 +++++++++++++++++++++
 2 files changed, 54 insertions(+), 54 deletions(-)

Comments

Dave Young July 14, 2018, 1:52 a.m. UTC | #1
On 07/11/18 at 04:41pm, AKASHI Takahiro wrote:
> Memblock list is another source for usable system memory layout.
> So powerpc's arch_kexec_walk_mem() is moved to kexec_file.c so that
> other memblock-based architectures, particularly arm64, can also utilise
> it. A moved function is now renamed to kexec_walk_memblock() and merged
> into the existing arch_kexec_walk_mem() for general use, either resource
> list or memblock list.
> 
> A consequent function will not work for kdump with memblock list, but
> this will be fixed in the next patch.
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> Cc: Dave Young <dyoung@redhat.com>
> Cc: Vivek Goyal <vgoyal@redhat.com>
> Cc: Baoquan He <bhe@redhat.com>
> Acked-by: James Morse <james.morse@arm.com>
> ---
>  arch/powerpc/kernel/machine_kexec_file_64.c | 54 ---------------------
>  kernel/kexec_file.c                         | 54 +++++++++++++++++++++
>  2 files changed, 54 insertions(+), 54 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/machine_kexec_file_64.c b/arch/powerpc/kernel/machine_kexec_file_64.c
> index 0bd23dc789a4..5357b09902c5 100644
> --- a/arch/powerpc/kernel/machine_kexec_file_64.c
> +++ b/arch/powerpc/kernel/machine_kexec_file_64.c
> @@ -24,7 +24,6 @@
>  
>  #include <linux/slab.h>
>  #include <linux/kexec.h>
> -#include <linux/memblock.h>
>  #include <linux/of_fdt.h>
>  #include <linux/libfdt.h>
>  #include <asm/ima.h>
> @@ -46,59 +45,6 @@ int arch_kexec_kernel_image_probe(struct kimage *image, void *buf,
>  	return kexec_image_probe_default(image, buf, buf_len);
>  }
>  
> -/**
> - * arch_kexec_walk_mem - call func(data) for each unreserved memory block
> - * @kbuf:	Context info for the search. Also passed to @func.
> - * @func:	Function to call for each memory block.
> - *
> - * This function is used by kexec_add_buffer and kexec_locate_mem_hole
> - * to find unreserved memory to load kexec segments into.
> - *
> - * 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 arch_kexec_walk_mem(struct kexec_buf *kbuf,
> -			int (*func)(struct resource *, void *))
> -{
> -	int ret = 0;
> -	u64 i;
> -	phys_addr_t mstart, mend;
> -	struct resource res = { };
> -
> -	if (kbuf->top_down) {
> -		for_each_free_mem_range_reverse(i, NUMA_NO_NODE, 0,
> -						&mstart, &mend, NULL) {
> -			/*
> -			 * In memblock, end points to the first byte after the
> -			 * range while in kexec, end points to the last byte
> -			 * in the range.
> -			 */
> -			res.start = mstart;
> -			res.end = mend - 1;
> -			ret = func(&res, kbuf);
> -			if (ret)
> -				break;
> -		}
> -	} else {
> -		for_each_free_mem_range(i, NUMA_NO_NODE, 0, &mstart, &mend,
> -					NULL) {
> -			/*
> -			 * In memblock, end points to the first byte after the
> -			 * range while in kexec, end points to the last byte
> -			 * in the range.
> -			 */
> -			res.start = mstart;
> -			res.end = mend - 1;
> -			ret = func(&res, kbuf);
> -			if (ret)
> -				break;
> -		}
> -	}
> -
> -	return ret;
> -}
> -
>  /**
>   * setup_purgatory - initialize the purgatory's global variables
>   * @image:		kexec image.
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index 63c7ce1c0c3e..b088324fb3ad 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -16,6 +16,7 @@
>  #include <linux/file.h>
>  #include <linux/slab.h>
>  #include <linux/kexec.h>
> +#include <linux/memblock.h>
>  #include <linux/mutex.h>
>  #include <linux/list.h>
>  #include <linux/fs.h>
> @@ -501,6 +502,55 @@ static int locate_mem_hole_callback(struct resource *res, void *arg)
>  	return locate_mem_hole_bottom_up(start, end, kbuf);
>  }
>  
> +#if defined(CONFIG_HAVE_MEMBLOCK) && !defined(CONFIG_ARCH_DISCARD_MEMBLOCK)
> +static int kexec_walk_memblock(struct kexec_buf *kbuf,
> +			       int (*func)(struct resource *, void *))
> +{
> +	int ret = 0;
> +	u64 i;
> +	phys_addr_t mstart, mend;
> +	struct resource res = { };
> +
> +	if (kbuf->top_down) {
> +		for_each_free_mem_range_reverse(i, NUMA_NO_NODE, 0,
> +						&mstart, &mend, NULL) {
> +			/*
> +			 * In memblock, end points to the first byte after the
> +			 * range while in kexec, end points to the last byte
> +			 * in the range.
> +			 */
> +			res.start = mstart;
> +			res.end = mend - 1;
> +			ret = func(&res, kbuf);
> +			if (ret)
> +				break;
> +		}
> +	} else {
> +		for_each_free_mem_range(i, NUMA_NO_NODE, 0, &mstart, &mend,
> +					NULL) {
> +			/*
> +			 * In memblock, end points to the first byte after the
> +			 * range while in kexec, end points to the last byte
> +			 * in the range.
> +			 */
> +			res.start = mstart;
> +			res.end = mend - 1;
> +			ret = func(&res, kbuf);
> +			if (ret)
> +				break;
> +		}
> +	}
> +
> +	return ret;
> +}
> +#else
> +static int kexec_walk_memblock(struct kexec_buf *kbuf,
> +			       int (*func)(struct resource *, void *))
> +{
> +	return 0;
> +}
> +#endif
> +
>  /**
>   * arch_kexec_walk_mem - call func(data) on free memory regions
>   * @kbuf:	Context info for the search. Also passed to @func.
> @@ -513,6 +563,10 @@ static int locate_mem_hole_callback(struct resource *res, void *arg)
>  int __weak arch_kexec_walk_mem(struct kexec_buf *kbuf,
>  			       int (*func)(struct resource *, void *))
>  {
> +	if (IS_ENABLED(CONFIG_HAVE_MEMBLOCK) &&
> +			!IS_ENABLED(CONFIG_ARCH_DISCARD_MEMBLOCK))
> +		return kexec_walk_memblock(kbuf, func);

AKASHI, I'm not sure if this works on all arches, for example I chekced
the .config on my Nokia N900 kernel tree, there is HAVE_MEMBLOCK=y and
no CONFIG_ARCH_DISCARD_MEMBLOCK,  in 32bit arm code no
arch_kexec_walk_mem() 

> +
>  	if (kbuf->image->type == KEXEC_TYPE_CRASH)
>  		return walk_iomem_res_desc(crashk_res.desc,
>  					   IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY,
> -- 
> 2.17.0
> 

Thanks
Dave
James Morse July 16, 2018, 11:04 a.m. UTC | #2
Hi Dave,

On 14/07/18 02:52, Dave Young wrote:
> On 07/11/18 at 04:41pm, AKASHI Takahiro wrote:
>> Memblock list is another source for usable system memory layout.
>> So powerpc's arch_kexec_walk_mem() is moved to kexec_file.c so that
>> other memblock-based architectures, particularly arm64, can also utilise
>> it. A moved function is now renamed to kexec_walk_memblock() and merged
>> into the existing arch_kexec_walk_mem() for general use, either resource
>> list or memblock list.
>>
>> A consequent function will not work for kdump with memblock list, but
>> this will be fixed in the next patch.

>> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c

>> @@ -513,6 +563,10 @@ static int locate_mem_hole_callback(struct resource *res, void *arg)
>>  int __weak arch_kexec_walk_mem(struct kexec_buf *kbuf,
>>  			       int (*func)(struct resource *, void *))
>>  {
>> +	if (IS_ENABLED(CONFIG_HAVE_MEMBLOCK) &&
>> +			!IS_ENABLED(CONFIG_ARCH_DISCARD_MEMBLOCK))
>> +		return kexec_walk_memblock(kbuf, func);
> 
> AKASHI, I'm not sure if this works on all arches, for example I chekced
> the .config on my Nokia N900 kernel tree, there is HAVE_MEMBLOCK=y and
> no CONFIG_ARCH_DISCARD_MEMBLOCK, in 32bit arm code no arch_kexec_walk_mem()
By doesn't work you mean it's a change in behaviour?
I think this is fine because 32bit arm doesn't support KEXEC_FILE, (this file is
kexec_file specific right?).

It only affects architectures with MEMBLOCK and KEXEC_FILE: powerpc, s390 and
soon arm64. s390 keeps its behaviour because it provides arch_kexec_walk_mem(),
and powerpc's is copied in here as its generic 'memblock describes my memory'
stuff. The implementation would be the same on arm64, so we're doing this to
avoid duplicating otherwise generic arch code. I think 32bit arm should be able
to use this too if it gets KEXEC_FILE support. (32bit arms' KEXEC already
depends on MEMBLOCK).


Thanks,

James
Dave Young July 16, 2018, 12:24 p.m. UTC | #3
On 07/16/18 at 12:04pm, James Morse wrote:
> Hi Dave,
> 
> On 14/07/18 02:52, Dave Young wrote:
> > On 07/11/18 at 04:41pm, AKASHI Takahiro wrote:
> >> Memblock list is another source for usable system memory layout.
> >> So powerpc's arch_kexec_walk_mem() is moved to kexec_file.c so that
> >> other memblock-based architectures, particularly arm64, can also utilise
> >> it. A moved function is now renamed to kexec_walk_memblock() and merged
> >> into the existing arch_kexec_walk_mem() for general use, either resource
> >> list or memblock list.
> >>
> >> A consequent function will not work for kdump with memblock list, but
> >> this will be fixed in the next patch.
> 
> >> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> 
> >> @@ -513,6 +563,10 @@ static int locate_mem_hole_callback(struct resource *res, void *arg)
> >>  int __weak arch_kexec_walk_mem(struct kexec_buf *kbuf,
> >>  			       int (*func)(struct resource *, void *))
> >>  {
> >> +	if (IS_ENABLED(CONFIG_HAVE_MEMBLOCK) &&
> >> +			!IS_ENABLED(CONFIG_ARCH_DISCARD_MEMBLOCK))
> >> +		return kexec_walk_memblock(kbuf, func);
> > 
> > AKASHI, I'm not sure if this works on all arches, for example I chekced
> > the .config on my Nokia N900 kernel tree, there is HAVE_MEMBLOCK=y and
> > no CONFIG_ARCH_DISCARD_MEMBLOCK, in 32bit arm code no arch_kexec_walk_mem()
> By doesn't work you mean it's a change in behaviour?
> I think this is fine because 32bit arm doesn't support KEXEC_FILE, (this file is
> kexec_file specific right?).

Ah, replied on a train, I forgot this is only for kexec_file, sorry
about that.  Please ignore the comment.

But since we have a weak function arch_kexec_walk_mem, adding another
condition branch within this weak function looks not good.
Something like below would be better:
 
int kexec_locate_mem_hole(struct kexec_buf *kbuf)
{
        int ret;

	+ if use memblock
	+	ret = kexec_walk_memblock()
	+ else
        	ret = arch_kexec_walk_mem(kbuf, locate_mem_hole_callback);

        return ret == 1 ? 0 : -EADDRNOTAVAIL;
}


> 
> It only affects architectures with MEMBLOCK and KEXEC_FILE: powerpc, s390 and
> soon arm64. s390 keeps its behaviour because it provides arch_kexec_walk_mem(),
> and powerpc's is copied in here as its generic 'memblock describes my memory'
> stuff. The implementation would be the same on arm64, so we're doing this to
> avoid duplicating otherwise generic arch code. I think 32bit arm should be able
> to use this too if it gets KEXEC_FILE support. (32bit arms' KEXEC already
> depends on MEMBLOCK).
> 
> 
> Thanks,
> 
> James

Thanks
Dave
Dave Young July 16, 2018, 12:26 p.m. UTC | #4
Hi Akashi,

On 07/11/18 at 04:41pm, AKASHI Takahiro wrote:
> Memblock list is another source for usable system memory layout.
> So powerpc's arch_kexec_walk_mem() is moved to kexec_file.c so that
> other memblock-based architectures, particularly arm64, can also utilise
> it. A moved function is now renamed to kexec_walk_memblock() and merged
> into the existing arch_kexec_walk_mem() for general use, either resource
> list or memblock list.
> 
> A consequent function will not work for kdump with memblock list, but
> this will be fixed in the next patch.

If this breaks something, then it would be good to fold the following
patch in this patch so that bisect can still work?

Thanks
Dave
AKASHI Takahiro July 17, 2018, 5:31 a.m. UTC | #5
Hi Dave,

On Mon, Jul 16, 2018 at 08:24:12PM +0800, Dave Young wrote:
> On 07/16/18 at 12:04pm, James Morse wrote:
> > Hi Dave,
> > 
> > On 14/07/18 02:52, Dave Young wrote:
> > > On 07/11/18 at 04:41pm, AKASHI Takahiro wrote:
> > >> Memblock list is another source for usable system memory layout.
> > >> So powerpc's arch_kexec_walk_mem() is moved to kexec_file.c so that
> > >> other memblock-based architectures, particularly arm64, can also utilise
> > >> it. A moved function is now renamed to kexec_walk_memblock() and merged
> > >> into the existing arch_kexec_walk_mem() for general use, either resource
> > >> list or memblock list.
> > >>
> > >> A consequent function will not work for kdump with memblock list, but
> > >> this will be fixed in the next patch.
> > 
> > >> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> > 
> > >> @@ -513,6 +563,10 @@ static int locate_mem_hole_callback(struct resource *res, void *arg)
> > >>  int __weak arch_kexec_walk_mem(struct kexec_buf *kbuf,
> > >>  			       int (*func)(struct resource *, void *))
> > >>  {
> > >> +	if (IS_ENABLED(CONFIG_HAVE_MEMBLOCK) &&
> > >> +			!IS_ENABLED(CONFIG_ARCH_DISCARD_MEMBLOCK))
> > >> +		return kexec_walk_memblock(kbuf, func);
> > > 
> > > AKASHI, I'm not sure if this works on all arches, for example I chekced
> > > the .config on my Nokia N900 kernel tree, there is HAVE_MEMBLOCK=y and
> > > no CONFIG_ARCH_DISCARD_MEMBLOCK, in 32bit arm code no arch_kexec_walk_mem()
> > By doesn't work you mean it's a change in behaviour?
> > I think this is fine because 32bit arm doesn't support KEXEC_FILE, (this file is
> > kexec_file specific right?).
> 
> Ah, replied on a train, I forgot this is only for kexec_file, sorry
> about that.  Please ignore the comment.
> 
> But since we have a weak function arch_kexec_walk_mem, adding another
> condition branch within this weak function looks not good.
> Something like below would be better:

I see your concern here, but


> int kexec_locate_mem_hole(struct kexec_buf *kbuf)
> {
>         int ret;
> 
> 	+ if use memblock
> 	+	ret = kexec_walk_memblock()
> 	+ else
>         	ret = arch_kexec_walk_mem(kbuf, locate_mem_hole_callback);
> 
>         return ret == 1 ? 0 : -EADDRNOTAVAIL;
> }

what if yet another architecture comes to kexec_file and wanna
take a third approach? How can it override those functions?
Depending on kernel configuration, it might re-define either
kexec_walk_memblock() or arch_kexec_walk_mem(). It sounds weird to me.

Thanks,
-Takahiro AKASHI

> 
> > 
> > It only affects architectures with MEMBLOCK and KEXEC_FILE: powerpc, s390 and
> > soon arm64. s390 keeps its behaviour because it provides arch_kexec_walk_mem(),
> > and powerpc's is copied in here as its generic 'memblock describes my memory'
> > stuff. The implementation would be the same on arm64, so we're doing this to
> > avoid duplicating otherwise generic arch code. I think 32bit arm should be able
> > to use this too if it gets KEXEC_FILE support. (32bit arms' KEXEC already
> > depends on MEMBLOCK).
> > 
> > 
> > Thanks,
> > 
> > James
> 
> Thanks
> Dave
Dave Young July 17, 2018, 7:49 a.m. UTC | #6
Hi AKASHI,
On 07/17/18 at 02:31pm, AKASHI Takahiro wrote:
> Hi Dave,
> 
> On Mon, Jul 16, 2018 at 08:24:12PM +0800, Dave Young wrote:
> > On 07/16/18 at 12:04pm, James Morse wrote:
> > > Hi Dave,
> > > 
> > > On 14/07/18 02:52, Dave Young wrote:
> > > > On 07/11/18 at 04:41pm, AKASHI Takahiro wrote:
> > > >> Memblock list is another source for usable system memory layout.
> > > >> So powerpc's arch_kexec_walk_mem() is moved to kexec_file.c so that
> > > >> other memblock-based architectures, particularly arm64, can also utilise
> > > >> it. A moved function is now renamed to kexec_walk_memblock() and merged
> > > >> into the existing arch_kexec_walk_mem() for general use, either resource
> > > >> list or memblock list.
> > > >>
> > > >> A consequent function will not work for kdump with memblock list, but
> > > >> this will be fixed in the next patch.
> > > 
> > > >> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> > > 
> > > >> @@ -513,6 +563,10 @@ static int locate_mem_hole_callback(struct resource *res, void *arg)
> > > >>  int __weak arch_kexec_walk_mem(struct kexec_buf *kbuf,
> > > >>  			       int (*func)(struct resource *, void *))
> > > >>  {
> > > >> +	if (IS_ENABLED(CONFIG_HAVE_MEMBLOCK) &&
> > > >> +			!IS_ENABLED(CONFIG_ARCH_DISCARD_MEMBLOCK))
> > > >> +		return kexec_walk_memblock(kbuf, func);
> > > > 
> > > > AKASHI, I'm not sure if this works on all arches, for example I chekced
> > > > the .config on my Nokia N900 kernel tree, there is HAVE_MEMBLOCK=y and
> > > > no CONFIG_ARCH_DISCARD_MEMBLOCK, in 32bit arm code no arch_kexec_walk_mem()
> > > By doesn't work you mean it's a change in behaviour?
> > > I think this is fine because 32bit arm doesn't support KEXEC_FILE, (this file is
> > > kexec_file specific right?).
> > 
> > Ah, replied on a train, I forgot this is only for kexec_file, sorry
> > about that.  Please ignore the comment.
> > 
> > But since we have a weak function arch_kexec_walk_mem, adding another
> > condition branch within this weak function looks not good.
> > Something like below would be better:
> 
> I see your concern here, but
> 
> 
> > int kexec_locate_mem_hole(struct kexec_buf *kbuf)
> > {
> >         int ret;
> > 
> > 	+ if use memblock
> > 	+	ret = kexec_walk_memblock()
> > 	+ else
> >         	ret = arch_kexec_walk_mem(kbuf, locate_mem_hole_callback);
> > 
> >         return ret == 1 ? 0 : -EADDRNOTAVAIL;
> > }
> 
> what if yet another architecture comes to kexec_file and wanna
> take a third approach? How can it override those functions?
> Depending on kernel configuration, it might re-define either
> kexec_walk_memblock() or arch_kexec_walk_mem(). It sounds weird to me.

I also feel this weird, but it is slightly better because currently no
user need another overriding requirement, and I feel it is not expected to have in
the future for the memblock use.

Rethinking about this issue, we can just remove the weak function and
just use general function.

Currently with your patch applied only s390 use arch_kexec_walk_mem like
below:
/*
 * The kernel is loaded to a fixed location. Turn off kexec_locate_mem_hole
 * and provide kbuf->mem by hand.
 */
int arch_kexec_walk_mem(struct kexec_buf *kbuf,
                        int (*func)(struct resource *, void *))
{
        return 1;
}

AFAIK, all other users initialize kbuf->mem as NULL, so we can check
kbuf->mem in int kexec_locate_mem_hole:

if (kbuf->mem)
	return 0;

if use memblock
	kexec_walk_memblock
else
	kexec_walk_mem

> 
> Thanks,
> -Takahiro AKASHI
> 
> > 
> > > 
> > > It only affects architectures with MEMBLOCK and KEXEC_FILE: powerpc, s390 and
> > > soon arm64. s390 keeps its behaviour because it provides arch_kexec_walk_mem(),
> > > and powerpc's is copied in here as its generic 'memblock describes my memory'
> > > stuff. The implementation would be the same on arm64, so we're doing this to
> > > avoid duplicating otherwise generic arch code. I think 32bit arm should be able
> > > to use this too if it gets KEXEC_FILE support. (32bit arms' KEXEC already
> > > depends on MEMBLOCK).
> > > 
> > > 
> > > Thanks,
> > > 
> > > James
> > 
> > Thanks
> > Dave

Thanks
Dave
AKASHI Takahiro July 18, 2018, 5:38 a.m. UTC | #7
Dave,

On Tue, Jul 17, 2018 at 03:49:23PM +0800, Dave Young wrote:
> Hi AKASHI,
> On 07/17/18 at 02:31pm, AKASHI Takahiro wrote:
> > Hi Dave,
> > 
> > On Mon, Jul 16, 2018 at 08:24:12PM +0800, Dave Young wrote:
> > > On 07/16/18 at 12:04pm, James Morse wrote:
> > > > Hi Dave,
> > > > 
> > > > On 14/07/18 02:52, Dave Young wrote:
> > > > > On 07/11/18 at 04:41pm, AKASHI Takahiro wrote:
> > > > >> Memblock list is another source for usable system memory layout.
> > > > >> So powerpc's arch_kexec_walk_mem() is moved to kexec_file.c so that
> > > > >> other memblock-based architectures, particularly arm64, can also utilise
> > > > >> it. A moved function is now renamed to kexec_walk_memblock() and merged
> > > > >> into the existing arch_kexec_walk_mem() for general use, either resource
> > > > >> list or memblock list.
> > > > >>
> > > > >> A consequent function will not work for kdump with memblock list, but
> > > > >> this will be fixed in the next patch.
> > > > 
> > > > >> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> > > > 
> > > > >> @@ -513,6 +563,10 @@ static int locate_mem_hole_callback(struct resource *res, void *arg)
> > > > >>  int __weak arch_kexec_walk_mem(struct kexec_buf *kbuf,
> > > > >>  			       int (*func)(struct resource *, void *))
> > > > >>  {
> > > > >> +	if (IS_ENABLED(CONFIG_HAVE_MEMBLOCK) &&
> > > > >> +			!IS_ENABLED(CONFIG_ARCH_DISCARD_MEMBLOCK))
> > > > >> +		return kexec_walk_memblock(kbuf, func);
> > > > > 
> > > > > AKASHI, I'm not sure if this works on all arches, for example I chekced
> > > > > the .config on my Nokia N900 kernel tree, there is HAVE_MEMBLOCK=y and
> > > > > no CONFIG_ARCH_DISCARD_MEMBLOCK, in 32bit arm code no arch_kexec_walk_mem()
> > > > By doesn't work you mean it's a change in behaviour?
> > > > I think this is fine because 32bit arm doesn't support KEXEC_FILE, (this file is
> > > > kexec_file specific right?).
> > > 
> > > Ah, replied on a train, I forgot this is only for kexec_file, sorry
> > > about that.  Please ignore the comment.
> > > 
> > > But since we have a weak function arch_kexec_walk_mem, adding another
> > > condition branch within this weak function looks not good.
> > > Something like below would be better:
> > 
> > I see your concern here, but
> > 
> > 
> > > int kexec_locate_mem_hole(struct kexec_buf *kbuf)
> > > {
> > >         int ret;
> > > 
> > > 	+ if use memblock
> > > 	+	ret = kexec_walk_memblock()
> > > 	+ else
> > >         	ret = arch_kexec_walk_mem(kbuf, locate_mem_hole_callback);
> > > 
> > >         return ret == 1 ? 0 : -EADDRNOTAVAIL;
> > > }
> > 
> > what if yet another architecture comes to kexec_file and wanna
> > take a third approach? How can it override those functions?
> > Depending on kernel configuration, it might re-define either
> > kexec_walk_memblock() or arch_kexec_walk_mem(). It sounds weird to me.
> 
> I also feel this weird, but it is slightly better because currently no
> user need another overriding requirement, and I feel it is not expected to have in
> the future for the memblock use.
> 
> Rethinking about this issue, we can just remove the weak function and
> just use general function.

Do you really want to remove "weak" attribute?

> Currently with your patch applied only s390 use arch_kexec_walk_mem like
> below:
> /*
>  * The kernel is loaded to a fixed location. Turn off kexec_locate_mem_hole
>  * and provide kbuf->mem by hand.
>  */
> int arch_kexec_walk_mem(struct kexec_buf *kbuf,
>                         int (*func)(struct resource *, void *))
> {
>         return 1;
> }
> 
> AFAIK, all other users initialize kbuf->mem as NULL, so we can check

As a matter of fact, nobody initializes kbuf->mem before calling
kexec_add_buffer (in turn, kexec_locate_mem_hole()).

> kbuf->mem in int kexec_locate_mem_hole:
> 
> if (kbuf->mem)
> 	return 0;
> 
> if use memblock
> 	kexec_walk_memblock
> else
> 	kexec_walk_mem

I think that your solution will work for existing architectures
with appropriate patches, but to take your approach, as I said above,
we will have to modify every call site on all kexec_file-capable architectures.

If this is what you expect, I will work on it, but I don't think
that it would be a better idea.

Thanks,
-Takahiro AKASHI

> > 
> > Thanks,
> > -Takahiro AKASHI
> > 
> > > 
> > > > 
> > > > It only affects architectures with MEMBLOCK and KEXEC_FILE: powerpc, s390 and
> > > > soon arm64. s390 keeps its behaviour because it provides arch_kexec_walk_mem(),
> > > > and powerpc's is copied in here as its generic 'memblock describes my memory'
> > > > stuff. The implementation would be the same on arm64, so we're doing this to
> > > > avoid duplicating otherwise generic arch code. I think 32bit arm should be able
> > > > to use this too if it gets KEXEC_FILE support. (32bit arms' KEXEC already
> > > > depends on MEMBLOCK).
> > > > 
> > > > 
> > > > Thanks,
> > > > 
> > > > James
> > > 
> > > Thanks
> > > Dave
> 
> Thanks
> Dave
Dave Young July 18, 2018, 6:13 a.m. UTC | #8
Hi AKASHI,

On 07/18/18 at 02:38pm, AKASHI Takahiro wrote:
> Dave,
> 
> On Tue, Jul 17, 2018 at 03:49:23PM +0800, Dave Young wrote:
> > Hi AKASHI,
> > On 07/17/18 at 02:31pm, AKASHI Takahiro wrote:
> > > Hi Dave,
> > > 
> > > On Mon, Jul 16, 2018 at 08:24:12PM +0800, Dave Young wrote:
> > > > On 07/16/18 at 12:04pm, James Morse wrote:
> > > > > Hi Dave,
> > > > > 
> > > > > On 14/07/18 02:52, Dave Young wrote:
> > > > > > On 07/11/18 at 04:41pm, AKASHI Takahiro wrote:
> > > > > >> Memblock list is another source for usable system memory layout.
> > > > > >> So powerpc's arch_kexec_walk_mem() is moved to kexec_file.c so that
> > > > > >> other memblock-based architectures, particularly arm64, can also utilise
> > > > > >> it. A moved function is now renamed to kexec_walk_memblock() and merged
> > > > > >> into the existing arch_kexec_walk_mem() for general use, either resource
> > > > > >> list or memblock list.
> > > > > >>
> > > > > >> A consequent function will not work for kdump with memblock list, but
> > > > > >> this will be fixed in the next patch.
> > > > > 
> > > > > >> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> > > > > 
> > > > > >> @@ -513,6 +563,10 @@ static int locate_mem_hole_callback(struct resource *res, void *arg)
> > > > > >>  int __weak arch_kexec_walk_mem(struct kexec_buf *kbuf,
> > > > > >>  			       int (*func)(struct resource *, void *))
> > > > > >>  {
> > > > > >> +	if (IS_ENABLED(CONFIG_HAVE_MEMBLOCK) &&
> > > > > >> +			!IS_ENABLED(CONFIG_ARCH_DISCARD_MEMBLOCK))
> > > > > >> +		return kexec_walk_memblock(kbuf, func);
> > > > > > 
> > > > > > AKASHI, I'm not sure if this works on all arches, for example I chekced
> > > > > > the .config on my Nokia N900 kernel tree, there is HAVE_MEMBLOCK=y and
> > > > > > no CONFIG_ARCH_DISCARD_MEMBLOCK, in 32bit arm code no arch_kexec_walk_mem()
> > > > > By doesn't work you mean it's a change in behaviour?
> > > > > I think this is fine because 32bit arm doesn't support KEXEC_FILE, (this file is
> > > > > kexec_file specific right?).
> > > > 
> > > > Ah, replied on a train, I forgot this is only for kexec_file, sorry
> > > > about that.  Please ignore the comment.
> > > > 
> > > > But since we have a weak function arch_kexec_walk_mem, adding another
> > > > condition branch within this weak function looks not good.
> > > > Something like below would be better:
> > > 
> > > I see your concern here, but
> > > 
> > > 
> > > > int kexec_locate_mem_hole(struct kexec_buf *kbuf)
> > > > {
> > > >         int ret;
> > > > 
> > > > 	+ if use memblock
> > > > 	+	ret = kexec_walk_memblock()
> > > > 	+ else
> > > >         	ret = arch_kexec_walk_mem(kbuf, locate_mem_hole_callback);
> > > > 
> > > >         return ret == 1 ? 0 : -EADDRNOTAVAIL;
> > > > }
> > > 
> > > what if yet another architecture comes to kexec_file and wanna
> > > take a third approach? How can it override those functions?
> > > Depending on kernel configuration, it might re-define either
> > > kexec_walk_memblock() or arch_kexec_walk_mem(). It sounds weird to me.
> > 
> > I also feel this weird, but it is slightly better because currently no
> > user need another overriding requirement, and I feel it is not expected to have in
> > the future for the memblock use.
> > 
> > Rethinking about this issue, we can just remove the weak function and
> > just use general function.
> 
> Do you really want to remove "weak" attribute?
> 
> > Currently with your patch applied only s390 use arch_kexec_walk_mem like
> > below:
> > /*
> >  * The kernel is loaded to a fixed location. Turn off kexec_locate_mem_hole
> >  * and provide kbuf->mem by hand.
> >  */
> > int arch_kexec_walk_mem(struct kexec_buf *kbuf,
> >                         int (*func)(struct resource *, void *))
> > {
> >         return 1;
> > }
> > 
> > AFAIK, all other users initialize kbuf->mem as NULL, so we can check
> 
> As a matter of fact, nobody initializes kbuf->mem before calling
> kexec_add_buffer (in turn, kexec_locate_mem_hole()).

Not sure we understand each other..
Let's take an example in arch/x86/kernel/kexec-bzimage64.c:
bzImage64_load() :
	struct kexec_buf kbuf = { .image = image, .buf_max = ULONG_MAX,
				.top_down = true };

Except the three fields above other members will be initialized as zero
when compiling including the kbuf->mem

> 
> > kbuf->mem in int kexec_locate_mem_hole:
> > 
> > if (kbuf->mem)
> > 	return 0;
> > 
> > if use memblock
> > 	kexec_walk_memblock
> > else
> > 	kexec_walk_mem

kexec_walk_resource will be better than kexec_walk_mem

> 
> I think that your solution will work for existing architectures
> with appropriate patches, but to take your approach, as I said above,
> we will have to modify every call site on all kexec_file-capable architectures.
> 
> If this is what you expect, I will work on it, but I don't think
> that it would be a better idea.
> 
> Thanks,
> -Takahiro AKASHI
> 
> > > 
> > > Thanks,
> > > -Takahiro AKASHI
> > > 
> > > > 
> > > > > 
> > > > > It only affects architectures with MEMBLOCK and KEXEC_FILE: powerpc, s390 and
> > > > > soon arm64. s390 keeps its behaviour because it provides arch_kexec_walk_mem(),
> > > > > and powerpc's is copied in here as its generic 'memblock describes my memory'
> > > > > stuff. The implementation would be the same on arm64, so we're doing this to
> > > > > avoid duplicating otherwise generic arch code. I think 32bit arm should be able
> > > > > to use this too if it gets KEXEC_FILE support. (32bit arms' KEXEC already
> > > > > depends on MEMBLOCK).
> > > > > 
> > > > > 
> > > > > Thanks,
> > > > > 
> > > > > James
> > > > 
> > > > Thanks
> > > > Dave
> > 
> > Thanks
> > Dave

Thanks
dave
AKASHI Takahiro July 18, 2018, 6:40 a.m. UTC | #9
On Wed, Jul 18, 2018 at 02:13:50PM +0800, Dave Young wrote:
> Hi AKASHI,
> 
> On 07/18/18 at 02:38pm, AKASHI Takahiro wrote:
> > Dave,
> > 
> > On Tue, Jul 17, 2018 at 03:49:23PM +0800, Dave Young wrote:
> > > Hi AKASHI,
> > > On 07/17/18 at 02:31pm, AKASHI Takahiro wrote:
> > > > Hi Dave,
> > > > 
> > > > On Mon, Jul 16, 2018 at 08:24:12PM +0800, Dave Young wrote:
> > > > > On 07/16/18 at 12:04pm, James Morse wrote:
> > > > > > Hi Dave,
> > > > > > 
> > > > > > On 14/07/18 02:52, Dave Young wrote:
> > > > > > > On 07/11/18 at 04:41pm, AKASHI Takahiro wrote:
> > > > > > >> Memblock list is another source for usable system memory layout.
> > > > > > >> So powerpc's arch_kexec_walk_mem() is moved to kexec_file.c so that
> > > > > > >> other memblock-based architectures, particularly arm64, can also utilise
> > > > > > >> it. A moved function is now renamed to kexec_walk_memblock() and merged
> > > > > > >> into the existing arch_kexec_walk_mem() for general use, either resource
> > > > > > >> list or memblock list.
> > > > > > >>
> > > > > > >> A consequent function will not work for kdump with memblock list, but
> > > > > > >> this will be fixed in the next patch.
> > > > > > 
> > > > > > >> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> > > > > > 
> > > > > > >> @@ -513,6 +563,10 @@ static int locate_mem_hole_callback(struct resource *res, void *arg)
> > > > > > >>  int __weak arch_kexec_walk_mem(struct kexec_buf *kbuf,
> > > > > > >>  			       int (*func)(struct resource *, void *))
> > > > > > >>  {
> > > > > > >> +	if (IS_ENABLED(CONFIG_HAVE_MEMBLOCK) &&
> > > > > > >> +			!IS_ENABLED(CONFIG_ARCH_DISCARD_MEMBLOCK))
> > > > > > >> +		return kexec_walk_memblock(kbuf, func);
> > > > > > > 
> > > > > > > AKASHI, I'm not sure if this works on all arches, for example I chekced
> > > > > > > the .config on my Nokia N900 kernel tree, there is HAVE_MEMBLOCK=y and
> > > > > > > no CONFIG_ARCH_DISCARD_MEMBLOCK, in 32bit arm code no arch_kexec_walk_mem()
> > > > > > By doesn't work you mean it's a change in behaviour?
> > > > > > I think this is fine because 32bit arm doesn't support KEXEC_FILE, (this file is
> > > > > > kexec_file specific right?).
> > > > > 
> > > > > Ah, replied on a train, I forgot this is only for kexec_file, sorry
> > > > > about that.  Please ignore the comment.
> > > > > 
> > > > > But since we have a weak function arch_kexec_walk_mem, adding another
> > > > > condition branch within this weak function looks not good.
> > > > > Something like below would be better:
> > > > 
> > > > I see your concern here, but
> > > > 
> > > > 
> > > > > int kexec_locate_mem_hole(struct kexec_buf *kbuf)
> > > > > {
> > > > >         int ret;
> > > > > 
> > > > > 	+ if use memblock
> > > > > 	+	ret = kexec_walk_memblock()
> > > > > 	+ else
> > > > >         	ret = arch_kexec_walk_mem(kbuf, locate_mem_hole_callback);
> > > > > 
> > > > >         return ret == 1 ? 0 : -EADDRNOTAVAIL;
> > > > > }
> > > > 
> > > > what if yet another architecture comes to kexec_file and wanna
> > > > take a third approach? How can it override those functions?
> > > > Depending on kernel configuration, it might re-define either
> > > > kexec_walk_memblock() or arch_kexec_walk_mem(). It sounds weird to me.
> > > 
> > > I also feel this weird, but it is slightly better because currently no
> > > user need another overriding requirement, and I feel it is not expected to have in
> > > the future for the memblock use.
> > > 
> > > Rethinking about this issue, we can just remove the weak function and
> > > just use general function.
> > 
> > Do you really want to remove "weak" attribute?
> > 
> > > Currently with your patch applied only s390 use arch_kexec_walk_mem like
> > > below:
> > > /*
> > >  * The kernel is loaded to a fixed location. Turn off kexec_locate_mem_hole
> > >  * and provide kbuf->mem by hand.
> > >  */
> > > int arch_kexec_walk_mem(struct kexec_buf *kbuf,
> > >                         int (*func)(struct resource *, void *))
> > > {
> > >         return 1;
> > > }
> > > 
> > > AFAIK, all other users initialize kbuf->mem as NULL, so we can check
> > 
> > As a matter of fact, nobody initializes kbuf->mem before calling
> > kexec_add_buffer (in turn, kexec_locate_mem_hole()).
> 
> Not sure we understand each other..
> Let's take an example in arch/x86/kernel/kexec-bzimage64.c:
> bzImage64_load() :
> 	struct kexec_buf kbuf = { .image = image, .buf_max = ULONG_MAX,
> 				.top_down = true };
> 
> Except the three fields above other members will be initialized as zero
> when compiling including the kbuf->mem

Ah, you're right.
(My armr64 patch doesn't use struct initializer, though.)

> > 
> > > kbuf->mem in int kexec_locate_mem_hole:
> > > 
> > > if (kbuf->mem)
> > > 	return 0;
> > > 
> > > if use memblock
> > > 	kexec_walk_memblock
> > > else
> > > 	kexec_walk_mem
> 
> kexec_walk_resource will be better than kexec_walk_mem
> 
> > 
> > I think that your solution will work for existing architectures
> > with appropriate patches, but to take your approach, as I said above,
> > we will have to modify every call site on all kexec_file-capable architectures.
> > 
> > If this is what you expect, I will work on it, but I don't think
> > that it would be a better idea.

So you would expect me to modify my own arm64 code as well as s390.

-Takahiro AKASHI

> > 
> > Thanks,
> > -Takahiro AKASHI
> > 
> > > > 
> > > > Thanks,
> > > > -Takahiro AKASHI
> > > > 
> > > > > 
> > > > > > 
> > > > > > It only affects architectures with MEMBLOCK and KEXEC_FILE: powerpc, s390 and
> > > > > > soon arm64. s390 keeps its behaviour because it provides arch_kexec_walk_mem(),
> > > > > > and powerpc's is copied in here as its generic 'memblock describes my memory'
> > > > > > stuff. The implementation would be the same on arm64, so we're doing this to
> > > > > > avoid duplicating otherwise generic arch code. I think 32bit arm should be able
> > > > > > to use this too if it gets KEXEC_FILE support. (32bit arms' KEXEC already
> > > > > > depends on MEMBLOCK).
> > > > > > 
> > > > > > 
> > > > > > Thanks,
> > > > > > 
> > > > > > James
> > > > > 
> > > > > Thanks
> > > > > Dave
> > > 
> > > Thanks
> > > Dave
> 
> Thanks
> dave
Dave Young July 18, 2018, 6:45 a.m. UTC | #10
On 07/18/18 at 03:40pm, AKASHI Takahiro wrote:
> On Wed, Jul 18, 2018 at 02:13:50PM +0800, Dave Young wrote:
> > Hi AKASHI,
> > 
> > On 07/18/18 at 02:38pm, AKASHI Takahiro wrote:
> > > Dave,
> > > 
> > > On Tue, Jul 17, 2018 at 03:49:23PM +0800, Dave Young wrote:
> > > > Hi AKASHI,
> > > > On 07/17/18 at 02:31pm, AKASHI Takahiro wrote:
> > > > > Hi Dave,
> > > > > 
> > > > > On Mon, Jul 16, 2018 at 08:24:12PM +0800, Dave Young wrote:
> > > > > > On 07/16/18 at 12:04pm, James Morse wrote:
> > > > > > > Hi Dave,
> > > > > > > 
> > > > > > > On 14/07/18 02:52, Dave Young wrote:
> > > > > > > > On 07/11/18 at 04:41pm, AKASHI Takahiro wrote:
> > > > > > > >> Memblock list is another source for usable system memory layout.
> > > > > > > >> So powerpc's arch_kexec_walk_mem() is moved to kexec_file.c so that
> > > > > > > >> other memblock-based architectures, particularly arm64, can also utilise
> > > > > > > >> it. A moved function is now renamed to kexec_walk_memblock() and merged
> > > > > > > >> into the existing arch_kexec_walk_mem() for general use, either resource
> > > > > > > >> list or memblock list.
> > > > > > > >>
> > > > > > > >> A consequent function will not work for kdump with memblock list, but
> > > > > > > >> this will be fixed in the next patch.
> > > > > > > 
> > > > > > > >> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> > > > > > > 
> > > > > > > >> @@ -513,6 +563,10 @@ static int locate_mem_hole_callback(struct resource *res, void *arg)
> > > > > > > >>  int __weak arch_kexec_walk_mem(struct kexec_buf *kbuf,
> > > > > > > >>  			       int (*func)(struct resource *, void *))
> > > > > > > >>  {
> > > > > > > >> +	if (IS_ENABLED(CONFIG_HAVE_MEMBLOCK) &&
> > > > > > > >> +			!IS_ENABLED(CONFIG_ARCH_DISCARD_MEMBLOCK))
> > > > > > > >> +		return kexec_walk_memblock(kbuf, func);
> > > > > > > > 
> > > > > > > > AKASHI, I'm not sure if this works on all arches, for example I chekced
> > > > > > > > the .config on my Nokia N900 kernel tree, there is HAVE_MEMBLOCK=y and
> > > > > > > > no CONFIG_ARCH_DISCARD_MEMBLOCK, in 32bit arm code no arch_kexec_walk_mem()
> > > > > > > By doesn't work you mean it's a change in behaviour?
> > > > > > > I think this is fine because 32bit arm doesn't support KEXEC_FILE, (this file is
> > > > > > > kexec_file specific right?).
> > > > > > 
> > > > > > Ah, replied on a train, I forgot this is only for kexec_file, sorry
> > > > > > about that.  Please ignore the comment.
> > > > > > 
> > > > > > But since we have a weak function arch_kexec_walk_mem, adding another
> > > > > > condition branch within this weak function looks not good.
> > > > > > Something like below would be better:
> > > > > 
> > > > > I see your concern here, but
> > > > > 
> > > > > 
> > > > > > int kexec_locate_mem_hole(struct kexec_buf *kbuf)
> > > > > > {
> > > > > >         int ret;
> > > > > > 
> > > > > > 	+ if use memblock
> > > > > > 	+	ret = kexec_walk_memblock()
> > > > > > 	+ else
> > > > > >         	ret = arch_kexec_walk_mem(kbuf, locate_mem_hole_callback);
> > > > > > 
> > > > > >         return ret == 1 ? 0 : -EADDRNOTAVAIL;
> > > > > > }
> > > > > 
> > > > > what if yet another architecture comes to kexec_file and wanna
> > > > > take a third approach? How can it override those functions?
> > > > > Depending on kernel configuration, it might re-define either
> > > > > kexec_walk_memblock() or arch_kexec_walk_mem(). It sounds weird to me.
> > > > 
> > > > I also feel this weird, but it is slightly better because currently no
> > > > user need another overriding requirement, and I feel it is not expected to have in
> > > > the future for the memblock use.
> > > > 
> > > > Rethinking about this issue, we can just remove the weak function and
> > > > just use general function.
> > > 
> > > Do you really want to remove "weak" attribute?
> > > 
> > > > Currently with your patch applied only s390 use arch_kexec_walk_mem like
> > > > below:
> > > > /*
> > > >  * The kernel is loaded to a fixed location. Turn off kexec_locate_mem_hole
> > > >  * and provide kbuf->mem by hand.
> > > >  */
> > > > int arch_kexec_walk_mem(struct kexec_buf *kbuf,
> > > >                         int (*func)(struct resource *, void *))
> > > > {
> > > >         return 1;
> > > > }
> > > > 
> > > > AFAIK, all other users initialize kbuf->mem as NULL, so we can check
> > > 
> > > As a matter of fact, nobody initializes kbuf->mem before calling
> > > kexec_add_buffer (in turn, kexec_locate_mem_hole()).
> > 
> > Not sure we understand each other..
> > Let's take an example in arch/x86/kernel/kexec-bzimage64.c:
> > bzImage64_load() :
> > 	struct kexec_buf kbuf = { .image = image, .buf_max = ULONG_MAX,
> > 				.top_down = true };
> > 
> > Except the three fields above other members will be initialized as zero
> > when compiling including the kbuf->mem
> 
> Ah, you're right.
> (My armr64 patch doesn't use struct initializer, though.)
> 
> > > 
> > > > kbuf->mem in int kexec_locate_mem_hole:
> > > > 
> > > > if (kbuf->mem)
> > > > 	return 0;
> > > > 
> > > > if use memblock
> > > > 	kexec_walk_memblock
> > > > else
> > > > 	kexec_walk_mem
> > 
> > kexec_walk_resource will be better than kexec_walk_mem
> > 
> > > 
> > > I think that your solution will work for existing architectures
> > > with appropriate patches, but to take your approach, as I said above,
> > > we will have to modify every call site on all kexec_file-capable architectures.
> > > 
> > > If this is what you expect, I will work on it, but I don't think
> > > that it would be a better idea.
> 
> So you would expect me to modify my own arm64 code as well as s390.

Yes :)  But I had not get time to read all your patches so I was not
aware the struct initialization in arm64 code so I assumed only s390
need a change..

Thanks
Dave
James Morse July 18, 2018, 4:52 p.m. UTC | #11
Hi Dave, Akashi,

On 16/07/18 13:26, Dave Young wrote:
> On 07/11/18 at 04:41pm, AKASHI Takahiro wrote:
>> Memblock list is another source for usable system memory layout.
>> So powerpc's arch_kexec_walk_mem() is moved to kexec_file.c so that
>> other memblock-based architectures, particularly arm64, can also utilise
>> it. A moved function is now renamed to kexec_walk_memblock() and merged
>> into the existing arch_kexec_walk_mem() for general use, either resource
>> list or memblock list.
>>
>> A consequent function will not work for kdump with memblock list, but
>> this will be fixed in the next patch.
> 
> If this breaks something, then it would be good to fold the following
> patch in this patch so that bisect can still work?

This patch is just moving code from arch/powerpc that is generic.
powerpc doesn't support kdump via kexec_file, so nothing is damaged by adding
this new code in the next patch.

arm64 would need this kdump support, but it doesn't use it until patch 11.


Thanks,

James
Dave Young July 19, 2018, 2:23 a.m. UTC | #12
Hi James,
On 07/18/18 at 05:52pm, James Morse wrote:
> Hi Dave, Akashi,
> 
> On 16/07/18 13:26, Dave Young wrote:
> > On 07/11/18 at 04:41pm, AKASHI Takahiro wrote:
> >> Memblock list is another source for usable system memory layout.
> >> So powerpc's arch_kexec_walk_mem() is moved to kexec_file.c so that
> >> other memblock-based architectures, particularly arm64, can also utilise
> >> it. A moved function is now renamed to kexec_walk_memblock() and merged
> >> into the existing arch_kexec_walk_mem() for general use, either resource
> >> list or memblock list.
> >>
> >> A consequent function will not work for kdump with memblock list, but
> >> this will be fixed in the next patch.
> > 
> > If this breaks something, then it would be good to fold the following
> > patch in this patch so that bisect can still work?
> 
> This patch is just moving code from arch/powerpc that is generic.
> powerpc doesn't support kdump via kexec_file, so nothing is damaged by adding
> this new code in the next patch.
> 
> arm64 would need this kdump support, but it doesn't use it until patch 11.

Ok, then I'm fine with it, thanks for the explanation.

> 
> 
> Thanks,
> 
> James

Thanks
Dave
AKASHI Takahiro July 20, 2018, 5:33 a.m. UTC | #13
Dave,

On Wed, Jul 18, 2018 at 02:45:19PM +0800, Dave Young wrote:
> On 07/18/18 at 03:40pm, AKASHI Takahiro wrote:
> > On Wed, Jul 18, 2018 at 02:13:50PM +0800, Dave Young wrote:
> > > Hi AKASHI,
> > > 
> > > On 07/18/18 at 02:38pm, AKASHI Takahiro wrote:
> > > > Dave,
> > > > 
> > > > On Tue, Jul 17, 2018 at 03:49:23PM +0800, Dave Young wrote:
> > > > > Hi AKASHI,
> > > > > On 07/17/18 at 02:31pm, AKASHI Takahiro wrote:
> > > > > > Hi Dave,
> > > > > > 
> > > > > > On Mon, Jul 16, 2018 at 08:24:12PM +0800, Dave Young wrote:
> > > > > > > On 07/16/18 at 12:04pm, James Morse wrote:
> > > > > > > > Hi Dave,
> > > > > > > > 
> > > > > > > > On 14/07/18 02:52, Dave Young wrote:
> > > > > > > > > On 07/11/18 at 04:41pm, AKASHI Takahiro wrote:
> > > > > > > > >> Memblock list is another source for usable system memory layout.
> > > > > > > > >> So powerpc's arch_kexec_walk_mem() is moved to kexec_file.c so that
> > > > > > > > >> other memblock-based architectures, particularly arm64, can also utilise
> > > > > > > > >> it. A moved function is now renamed to kexec_walk_memblock() and merged
> > > > > > > > >> into the existing arch_kexec_walk_mem() for general use, either resource
> > > > > > > > >> list or memblock list.
> > > > > > > > >>
> > > > > > > > >> A consequent function will not work for kdump with memblock list, but
> > > > > > > > >> this will be fixed in the next patch.
> > > > > > > > 
> > > > > > > > >> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> > > > > > > > 
> > > > > > > > >> @@ -513,6 +563,10 @@ static int locate_mem_hole_callback(struct resource *res, void *arg)
> > > > > > > > >>  int __weak arch_kexec_walk_mem(struct kexec_buf *kbuf,
> > > > > > > > >>  			       int (*func)(struct resource *, void *))
> > > > > > > > >>  {
> > > > > > > > >> +	if (IS_ENABLED(CONFIG_HAVE_MEMBLOCK) &&
> > > > > > > > >> +			!IS_ENABLED(CONFIG_ARCH_DISCARD_MEMBLOCK))
> > > > > > > > >> +		return kexec_walk_memblock(kbuf, func);
> > > > > > > > > 
> > > > > > > > > AKASHI, I'm not sure if this works on all arches, for example I chekced
> > > > > > > > > the .config on my Nokia N900 kernel tree, there is HAVE_MEMBLOCK=y and
> > > > > > > > > no CONFIG_ARCH_DISCARD_MEMBLOCK, in 32bit arm code no arch_kexec_walk_mem()
> > > > > > > > By doesn't work you mean it's a change in behaviour?
> > > > > > > > I think this is fine because 32bit arm doesn't support KEXEC_FILE, (this file is
> > > > > > > > kexec_file specific right?).
> > > > > > > 
> > > > > > > Ah, replied on a train, I forgot this is only for kexec_file, sorry
> > > > > > > about that.  Please ignore the comment.
> > > > > > > 
> > > > > > > But since we have a weak function arch_kexec_walk_mem, adding another
> > > > > > > condition branch within this weak function looks not good.
> > > > > > > Something like below would be better:
> > > > > > 
> > > > > > I see your concern here, but
> > > > > > 
> > > > > > 
> > > > > > > int kexec_locate_mem_hole(struct kexec_buf *kbuf)
> > > > > > > {
> > > > > > >         int ret;
> > > > > > > 
> > > > > > > 	+ if use memblock
> > > > > > > 	+	ret = kexec_walk_memblock()
> > > > > > > 	+ else
> > > > > > >         	ret = arch_kexec_walk_mem(kbuf, locate_mem_hole_callback);
> > > > > > > 
> > > > > > >         return ret == 1 ? 0 : -EADDRNOTAVAIL;
> > > > > > > }
> > > > > > 
> > > > > > what if yet another architecture comes to kexec_file and wanna
> > > > > > take a third approach? How can it override those functions?
> > > > > > Depending on kernel configuration, it might re-define either
> > > > > > kexec_walk_memblock() or arch_kexec_walk_mem(). It sounds weird to me.
> > > > > 
> > > > > I also feel this weird, but it is slightly better because currently no
> > > > > user need another overriding requirement, and I feel it is not expected to have in
> > > > > the future for the memblock use.
> > > > > 
> > > > > Rethinking about this issue, we can just remove the weak function and
> > > > > just use general function.
> > > > 
> > > > Do you really want to remove "weak" attribute?
> > > > 
> > > > > Currently with your patch applied only s390 use arch_kexec_walk_mem like
> > > > > below:
> > > > > /*
> > > > >  * The kernel is loaded to a fixed location. Turn off kexec_locate_mem_hole
> > > > >  * and provide kbuf->mem by hand.
> > > > >  */
> > > > > int arch_kexec_walk_mem(struct kexec_buf *kbuf,
> > > > >                         int (*func)(struct resource *, void *))
> > > > > {
> > > > >         return 1;
> > > > > }
> > > > > 
> > > > > AFAIK, all other users initialize kbuf->mem as NULL, so we can check
> > > > 
> > > > As a matter of fact, nobody initializes kbuf->mem before calling
> > > > kexec_add_buffer (in turn, kexec_locate_mem_hole()).
> > > 
> > > Not sure we understand each other..
> > > Let's take an example in arch/x86/kernel/kexec-bzimage64.c:
> > > bzImage64_load() :
> > > 	struct kexec_buf kbuf = { .image = image, .buf_max = ULONG_MAX,
> > > 				.top_down = true };
> > > 
> > > Except the three fields above other members will be initialized as zero
> > > when compiling including the kbuf->mem
> > 
> > Ah, you're right.
> > (My armr64 patch doesn't use struct initializer, though.)
> > 
> > > > 
> > > > > kbuf->mem in int kexec_locate_mem_hole:
> > > > > 
> > > > > if (kbuf->mem)
> > > > > 	return 0;
> > > > > 
> > > > > if use memblock
> > > > > 	kexec_walk_memblock
> > > > > else
> > > > > 	kexec_walk_mem
> > > 
> > > kexec_walk_resource will be better than kexec_walk_mem
> > > 
> > > > 
> > > > I think that your solution will work for existing architectures
> > > > with appropriate patches, but to take your approach, as I said above,
> > > > we will have to modify every call site on all kexec_file-capable architectures.
> > > > 
> > > > If this is what you expect, I will work on it, but I don't think
> > > > that it would be a better idea.
> > 
> > So you would expect me to modify my own arm64 code as well as s390.
> 
> Yes :)  But I had not get time to read all your patches so I was not
> aware the struct initialization in arm64 code so I assumed only s390
> need a change..

Okay, but I don't want to mix cross-arch changes into a single patch,
prefer to leave the current patch as it is and add an additional patch
as you suggested here.

Is that OK for you?

Thanks,
-Takahiro AKASHI


> Thanks
> Dave
Dave Young July 20, 2018, 5:57 a.m. UTC | #14
On 07/20/18 at 02:33pm, AKASHI Takahiro wrote:
> Dave,
> 
> On Wed, Jul 18, 2018 at 02:45:19PM +0800, Dave Young wrote:
> > On 07/18/18 at 03:40pm, AKASHI Takahiro wrote:
> > > On Wed, Jul 18, 2018 at 02:13:50PM +0800, Dave Young wrote:
> > > > Hi AKASHI,
> > > > 
> > > > On 07/18/18 at 02:38pm, AKASHI Takahiro wrote:
> > > > > Dave,
> > > > > 
> > > > > On Tue, Jul 17, 2018 at 03:49:23PM +0800, Dave Young wrote:
> > > > > > Hi AKASHI,
> > > > > > On 07/17/18 at 02:31pm, AKASHI Takahiro wrote:
> > > > > > > Hi Dave,
> > > > > > > 
> > > > > > > On Mon, Jul 16, 2018 at 08:24:12PM +0800, Dave Young wrote:
> > > > > > > > On 07/16/18 at 12:04pm, James Morse wrote:
> > > > > > > > > Hi Dave,
> > > > > > > > > 
> > > > > > > > > On 14/07/18 02:52, Dave Young wrote:
> > > > > > > > > > On 07/11/18 at 04:41pm, AKASHI Takahiro wrote:
> > > > > > > > > >> Memblock list is another source for usable system memory layout.
> > > > > > > > > >> So powerpc's arch_kexec_walk_mem() is moved to kexec_file.c so that
> > > > > > > > > >> other memblock-based architectures, particularly arm64, can also utilise
> > > > > > > > > >> it. A moved function is now renamed to kexec_walk_memblock() and merged
> > > > > > > > > >> into the existing arch_kexec_walk_mem() for general use, either resource
> > > > > > > > > >> list or memblock list.
> > > > > > > > > >>
> > > > > > > > > >> A consequent function will not work for kdump with memblock list, but
> > > > > > > > > >> this will be fixed in the next patch.
> > > > > > > > > 
> > > > > > > > > >> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> > > > > > > > > 
> > > > > > > > > >> @@ -513,6 +563,10 @@ static int locate_mem_hole_callback(struct resource *res, void *arg)
> > > > > > > > > >>  int __weak arch_kexec_walk_mem(struct kexec_buf *kbuf,
> > > > > > > > > >>  			       int (*func)(struct resource *, void *))
> > > > > > > > > >>  {
> > > > > > > > > >> +	if (IS_ENABLED(CONFIG_HAVE_MEMBLOCK) &&
> > > > > > > > > >> +			!IS_ENABLED(CONFIG_ARCH_DISCARD_MEMBLOCK))
> > > > > > > > > >> +		return kexec_walk_memblock(kbuf, func);
> > > > > > > > > > 
> > > > > > > > > > AKASHI, I'm not sure if this works on all arches, for example I chekced
> > > > > > > > > > the .config on my Nokia N900 kernel tree, there is HAVE_MEMBLOCK=y and
> > > > > > > > > > no CONFIG_ARCH_DISCARD_MEMBLOCK, in 32bit arm code no arch_kexec_walk_mem()
> > > > > > > > > By doesn't work you mean it's a change in behaviour?
> > > > > > > > > I think this is fine because 32bit arm doesn't support KEXEC_FILE, (this file is
> > > > > > > > > kexec_file specific right?).
> > > > > > > > 
> > > > > > > > Ah, replied on a train, I forgot this is only for kexec_file, sorry
> > > > > > > > about that.  Please ignore the comment.
> > > > > > > > 
> > > > > > > > But since we have a weak function arch_kexec_walk_mem, adding another
> > > > > > > > condition branch within this weak function looks not good.
> > > > > > > > Something like below would be better:
> > > > > > > 
> > > > > > > I see your concern here, but
> > > > > > > 
> > > > > > > 
> > > > > > > > int kexec_locate_mem_hole(struct kexec_buf *kbuf)
> > > > > > > > {
> > > > > > > >         int ret;
> > > > > > > > 
> > > > > > > > 	+ if use memblock
> > > > > > > > 	+	ret = kexec_walk_memblock()
> > > > > > > > 	+ else
> > > > > > > >         	ret = arch_kexec_walk_mem(kbuf, locate_mem_hole_callback);
> > > > > > > > 
> > > > > > > >         return ret == 1 ? 0 : -EADDRNOTAVAIL;
> > > > > > > > }
> > > > > > > 
> > > > > > > what if yet another architecture comes to kexec_file and wanna
> > > > > > > take a third approach? How can it override those functions?
> > > > > > > Depending on kernel configuration, it might re-define either
> > > > > > > kexec_walk_memblock() or arch_kexec_walk_mem(). It sounds weird to me.
> > > > > > 
> > > > > > I also feel this weird, but it is slightly better because currently no
> > > > > > user need another overriding requirement, and I feel it is not expected to have in
> > > > > > the future for the memblock use.
> > > > > > 
> > > > > > Rethinking about this issue, we can just remove the weak function and
> > > > > > just use general function.
> > > > > 
> > > > > Do you really want to remove "weak" attribute?
> > > > > 
> > > > > > Currently with your patch applied only s390 use arch_kexec_walk_mem like
> > > > > > below:
> > > > > > /*
> > > > > >  * The kernel is loaded to a fixed location. Turn off kexec_locate_mem_hole
> > > > > >  * and provide kbuf->mem by hand.
> > > > > >  */
> > > > > > int arch_kexec_walk_mem(struct kexec_buf *kbuf,
> > > > > >                         int (*func)(struct resource *, void *))
> > > > > > {
> > > > > >         return 1;
> > > > > > }
> > > > > > 
> > > > > > AFAIK, all other users initialize kbuf->mem as NULL, so we can check
> > > > > 
> > > > > As a matter of fact, nobody initializes kbuf->mem before calling
> > > > > kexec_add_buffer (in turn, kexec_locate_mem_hole()).
> > > > 
> > > > Not sure we understand each other..
> > > > Let's take an example in arch/x86/kernel/kexec-bzimage64.c:
> > > > bzImage64_load() :
> > > > 	struct kexec_buf kbuf = { .image = image, .buf_max = ULONG_MAX,
> > > > 				.top_down = true };
> > > > 
> > > > Except the three fields above other members will be initialized as zero
> > > > when compiling including the kbuf->mem
> > > 
> > > Ah, you're right.
> > > (My armr64 patch doesn't use struct initializer, though.)
> > > 
> > > > > 
> > > > > > kbuf->mem in int kexec_locate_mem_hole:
> > > > > > 
> > > > > > if (kbuf->mem)
> > > > > > 	return 0;
> > > > > > 
> > > > > > if use memblock
> > > > > > 	kexec_walk_memblock
> > > > > > else
> > > > > > 	kexec_walk_mem
> > > > 
> > > > kexec_walk_resource will be better than kexec_walk_mem
> > > > 
> > > > > 
> > > > > I think that your solution will work for existing architectures
> > > > > with appropriate patches, but to take your approach, as I said above,
> > > > > we will have to modify every call site on all kexec_file-capable architectures.
> > > > > 
> > > > > If this is what you expect, I will work on it, but I don't think
> > > > > that it would be a better idea.
> > > 
> > > So you would expect me to modify my own arm64 code as well as s390.
> > 
> > Yes :)  But I had not get time to read all your patches so I was not
> > aware the struct initialization in arm64 code so I assumed only s390
> > need a change..
> 
> Okay, but I don't want to mix cross-arch changes into a single patch,
> prefer to leave the current patch as it is and add an additional patch
> as you suggested here.
Hi AKASHI,

Maybe add another patch to drop s390 walk function first, then follow
with this patch with the modification about common code restructure.

Is this better? For example:
03/15 s390, drop s390 arch_kexec_mem_walk
04/15 powerpc, kexec_file: factor out memblock-based arch_kexec_walk_mem
> 
> Is that OK for you?
> 
> Thanks,
> -Takahiro AKASHI
> 
> 
> > Thanks
> > Dave

Thanks
Dave
AKASHI Takahiro July 20, 2018, 6:25 a.m. UTC | #15
On Fri, Jul 20, 2018 at 01:57:27PM +0800, Dave Young wrote:
> On 07/20/18 at 02:33pm, AKASHI Takahiro wrote:
> > Dave,
> > 
> > On Wed, Jul 18, 2018 at 02:45:19PM +0800, Dave Young wrote:
> > > On 07/18/18 at 03:40pm, AKASHI Takahiro wrote:
> > > > On Wed, Jul 18, 2018 at 02:13:50PM +0800, Dave Young wrote:
> > > > > Hi AKASHI,
> > > > > 
> > > > > On 07/18/18 at 02:38pm, AKASHI Takahiro wrote:
> > > > > > Dave,
> > > > > > 
> > > > > > On Tue, Jul 17, 2018 at 03:49:23PM +0800, Dave Young wrote:
> > > > > > > Hi AKASHI,
> > > > > > > On 07/17/18 at 02:31pm, AKASHI Takahiro wrote:
> > > > > > > > Hi Dave,
> > > > > > > > 
> > > > > > > > On Mon, Jul 16, 2018 at 08:24:12PM +0800, Dave Young wrote:
> > > > > > > > > On 07/16/18 at 12:04pm, James Morse wrote:
> > > > > > > > > > Hi Dave,
> > > > > > > > > > 
> > > > > > > > > > On 14/07/18 02:52, Dave Young wrote:
> > > > > > > > > > > On 07/11/18 at 04:41pm, AKASHI Takahiro wrote:
> > > > > > > > > > >> Memblock list is another source for usable system memory layout.
> > > > > > > > > > >> So powerpc's arch_kexec_walk_mem() is moved to kexec_file.c so that
> > > > > > > > > > >> other memblock-based architectures, particularly arm64, can also utilise
> > > > > > > > > > >> it. A moved function is now renamed to kexec_walk_memblock() and merged
> > > > > > > > > > >> into the existing arch_kexec_walk_mem() for general use, either resource
> > > > > > > > > > >> list or memblock list.
> > > > > > > > > > >>
> > > > > > > > > > >> A consequent function will not work for kdump with memblock list, but
> > > > > > > > > > >> this will be fixed in the next patch.
> > > > > > > > > > 
> > > > > > > > > > >> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> > > > > > > > > > 
> > > > > > > > > > >> @@ -513,6 +563,10 @@ static int locate_mem_hole_callback(struct resource *res, void *arg)
> > > > > > > > > > >>  int __weak arch_kexec_walk_mem(struct kexec_buf *kbuf,
> > > > > > > > > > >>  			       int (*func)(struct resource *, void *))
> > > > > > > > > > >>  {
> > > > > > > > > > >> +	if (IS_ENABLED(CONFIG_HAVE_MEMBLOCK) &&
> > > > > > > > > > >> +			!IS_ENABLED(CONFIG_ARCH_DISCARD_MEMBLOCK))
> > > > > > > > > > >> +		return kexec_walk_memblock(kbuf, func);
> > > > > > > > > > > 
> > > > > > > > > > > AKASHI, I'm not sure if this works on all arches, for example I chekced
> > > > > > > > > > > the .config on my Nokia N900 kernel tree, there is HAVE_MEMBLOCK=y and
> > > > > > > > > > > no CONFIG_ARCH_DISCARD_MEMBLOCK, in 32bit arm code no arch_kexec_walk_mem()
> > > > > > > > > > By doesn't work you mean it's a change in behaviour?
> > > > > > > > > > I think this is fine because 32bit arm doesn't support KEXEC_FILE, (this file is
> > > > > > > > > > kexec_file specific right?).
> > > > > > > > > 
> > > > > > > > > Ah, replied on a train, I forgot this is only for kexec_file, sorry
> > > > > > > > > about that.  Please ignore the comment.
> > > > > > > > > 
> > > > > > > > > But since we have a weak function arch_kexec_walk_mem, adding another
> > > > > > > > > condition branch within this weak function looks not good.
> > > > > > > > > Something like below would be better:
> > > > > > > > 
> > > > > > > > I see your concern here, but
> > > > > > > > 
> > > > > > > > 
> > > > > > > > > int kexec_locate_mem_hole(struct kexec_buf *kbuf)
> > > > > > > > > {
> > > > > > > > >         int ret;
> > > > > > > > > 
> > > > > > > > > 	+ if use memblock
> > > > > > > > > 	+	ret = kexec_walk_memblock()
> > > > > > > > > 	+ else
> > > > > > > > >         	ret = arch_kexec_walk_mem(kbuf, locate_mem_hole_callback);
> > > > > > > > > 
> > > > > > > > >         return ret == 1 ? 0 : -EADDRNOTAVAIL;
> > > > > > > > > }
> > > > > > > > 
> > > > > > > > what if yet another architecture comes to kexec_file and wanna
> > > > > > > > take a third approach? How can it override those functions?
> > > > > > > > Depending on kernel configuration, it might re-define either
> > > > > > > > kexec_walk_memblock() or arch_kexec_walk_mem(). It sounds weird to me.
> > > > > > > 
> > > > > > > I also feel this weird, but it is slightly better because currently no
> > > > > > > user need another overriding requirement, and I feel it is not expected to have in
> > > > > > > the future for the memblock use.
> > > > > > > 
> > > > > > > Rethinking about this issue, we can just remove the weak function and
> > > > > > > just use general function.
> > > > > > 
> > > > > > Do you really want to remove "weak" attribute?
> > > > > > 
> > > > > > > Currently with your patch applied only s390 use arch_kexec_walk_mem like
> > > > > > > below:
> > > > > > > /*
> > > > > > >  * The kernel is loaded to a fixed location. Turn off kexec_locate_mem_hole
> > > > > > >  * and provide kbuf->mem by hand.
> > > > > > >  */
> > > > > > > int arch_kexec_walk_mem(struct kexec_buf *kbuf,
> > > > > > >                         int (*func)(struct resource *, void *))
> > > > > > > {
> > > > > > >         return 1;
> > > > > > > }
> > > > > > > 
> > > > > > > AFAIK, all other users initialize kbuf->mem as NULL, so we can check
> > > > > > 
> > > > > > As a matter of fact, nobody initializes kbuf->mem before calling
> > > > > > kexec_add_buffer (in turn, kexec_locate_mem_hole()).
> > > > > 
> > > > > Not sure we understand each other..
> > > > > Let's take an example in arch/x86/kernel/kexec-bzimage64.c:
> > > > > bzImage64_load() :
> > > > > 	struct kexec_buf kbuf = { .image = image, .buf_max = ULONG_MAX,
> > > > > 				.top_down = true };
> > > > > 
> > > > > Except the three fields above other members will be initialized as zero
> > > > > when compiling including the kbuf->mem
> > > > 
> > > > Ah, you're right.
> > > > (My armr64 patch doesn't use struct initializer, though.)
> > > > 
> > > > > > 
> > > > > > > kbuf->mem in int kexec_locate_mem_hole:
> > > > > > > 
> > > > > > > if (kbuf->mem)
> > > > > > > 	return 0;
> > > > > > > 
> > > > > > > if use memblock
> > > > > > > 	kexec_walk_memblock
> > > > > > > else
> > > > > > > 	kexec_walk_mem
> > > > > 
> > > > > kexec_walk_resource will be better than kexec_walk_mem
> > > > > 
> > > > > > 
> > > > > > I think that your solution will work for existing architectures
> > > > > > with appropriate patches, but to take your approach, as I said above,
> > > > > > we will have to modify every call site on all kexec_file-capable architectures.
> > > > > > 
> > > > > > If this is what you expect, I will work on it, but I don't think
> > > > > > that it would be a better idea.
> > > > 
> > > > So you would expect me to modify my own arm64 code as well as s390.
> > > 
> > > Yes :)  But I had not get time to read all your patches so I was not
> > > aware the struct initialization in arm64 code so I assumed only s390
> > > need a change..
> > 
> > Okay, but I don't want to mix cross-arch changes into a single patch,
> > prefer to leave the current patch as it is and add an additional patch
> > as you suggested here.
> Hi AKASHI,
> 
> Maybe add another patch to drop s390 walk function first, then follow
> with this patch with the modification about common code restructure.
> 
> Is this better? For example:
> 03/15 s390, drop s390 arch_kexec_mem_walk
> 04/15 powerpc, kexec_file: factor out memblock-based arch_kexec_walk_mem

That's fine to me, too.

Thanks,
-Takahiro AKASHI

> > 
> > Is that OK for you?
> > 
> > Thanks,
> > -Takahiro AKASHI
> > 
> > 
> > > Thanks
> > > Dave
> 
> Thanks
> Dave
diff mbox

Patch

diff --git a/arch/powerpc/kernel/machine_kexec_file_64.c b/arch/powerpc/kernel/machine_kexec_file_64.c
index 0bd23dc789a4..5357b09902c5 100644
--- a/arch/powerpc/kernel/machine_kexec_file_64.c
+++ b/arch/powerpc/kernel/machine_kexec_file_64.c
@@ -24,7 +24,6 @@ 
 
 #include <linux/slab.h>
 #include <linux/kexec.h>
-#include <linux/memblock.h>
 #include <linux/of_fdt.h>
 #include <linux/libfdt.h>
 #include <asm/ima.h>
@@ -46,59 +45,6 @@  int arch_kexec_kernel_image_probe(struct kimage *image, void *buf,
 	return kexec_image_probe_default(image, buf, buf_len);
 }
 
-/**
- * arch_kexec_walk_mem - call func(data) for each unreserved memory block
- * @kbuf:	Context info for the search. Also passed to @func.
- * @func:	Function to call for each memory block.
- *
- * This function is used by kexec_add_buffer and kexec_locate_mem_hole
- * to find unreserved memory to load kexec segments into.
- *
- * 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 arch_kexec_walk_mem(struct kexec_buf *kbuf,
-			int (*func)(struct resource *, void *))
-{
-	int ret = 0;
-	u64 i;
-	phys_addr_t mstart, mend;
-	struct resource res = { };
-
-	if (kbuf->top_down) {
-		for_each_free_mem_range_reverse(i, NUMA_NO_NODE, 0,
-						&mstart, &mend, NULL) {
-			/*
-			 * In memblock, end points to the first byte after the
-			 * range while in kexec, end points to the last byte
-			 * in the range.
-			 */
-			res.start = mstart;
-			res.end = mend - 1;
-			ret = func(&res, kbuf);
-			if (ret)
-				break;
-		}
-	} else {
-		for_each_free_mem_range(i, NUMA_NO_NODE, 0, &mstart, &mend,
-					NULL) {
-			/*
-			 * In memblock, end points to the first byte after the
-			 * range while in kexec, end points to the last byte
-			 * in the range.
-			 */
-			res.start = mstart;
-			res.end = mend - 1;
-			ret = func(&res, kbuf);
-			if (ret)
-				break;
-		}
-	}
-
-	return ret;
-}
-
 /**
  * setup_purgatory - initialize the purgatory's global variables
  * @image:		kexec image.
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index 63c7ce1c0c3e..b088324fb3ad 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -16,6 +16,7 @@ 
 #include <linux/file.h>
 #include <linux/slab.h>
 #include <linux/kexec.h>
+#include <linux/memblock.h>
 #include <linux/mutex.h>
 #include <linux/list.h>
 #include <linux/fs.h>
@@ -501,6 +502,55 @@  static int locate_mem_hole_callback(struct resource *res, void *arg)
 	return locate_mem_hole_bottom_up(start, end, kbuf);
 }
 
+#if defined(CONFIG_HAVE_MEMBLOCK) && !defined(CONFIG_ARCH_DISCARD_MEMBLOCK)
+static int kexec_walk_memblock(struct kexec_buf *kbuf,
+			       int (*func)(struct resource *, void *))
+{
+	int ret = 0;
+	u64 i;
+	phys_addr_t mstart, mend;
+	struct resource res = { };
+
+	if (kbuf->top_down) {
+		for_each_free_mem_range_reverse(i, NUMA_NO_NODE, 0,
+						&mstart, &mend, NULL) {
+			/*
+			 * In memblock, end points to the first byte after the
+			 * range while in kexec, end points to the last byte
+			 * in the range.
+			 */
+			res.start = mstart;
+			res.end = mend - 1;
+			ret = func(&res, kbuf);
+			if (ret)
+				break;
+		}
+	} else {
+		for_each_free_mem_range(i, NUMA_NO_NODE, 0, &mstart, &mend,
+					NULL) {
+			/*
+			 * In memblock, end points to the first byte after the
+			 * range while in kexec, end points to the last byte
+			 * in the range.
+			 */
+			res.start = mstart;
+			res.end = mend - 1;
+			ret = func(&res, kbuf);
+			if (ret)
+				break;
+		}
+	}
+
+	return ret;
+}
+#else
+static int kexec_walk_memblock(struct kexec_buf *kbuf,
+			       int (*func)(struct resource *, void *))
+{
+	return 0;
+}
+#endif
+
 /**
  * arch_kexec_walk_mem - call func(data) on free memory regions
  * @kbuf:	Context info for the search. Also passed to @func.
@@ -513,6 +563,10 @@  static int locate_mem_hole_callback(struct resource *res, void *arg)
 int __weak arch_kexec_walk_mem(struct kexec_buf *kbuf,
 			       int (*func)(struct resource *, void *))
 {
+	if (IS_ENABLED(CONFIG_HAVE_MEMBLOCK) &&
+			!IS_ENABLED(CONFIG_ARCH_DISCARD_MEMBLOCK))
+		return kexec_walk_memblock(kbuf, func);
+
 	if (kbuf->image->type == KEXEC_TYPE_CRASH)
 		return walk_iomem_res_desc(crashk_res.desc,
 					   IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY,