Re: [PATCH v8 03/13] kexec_file,x86,powerpc: factor out kexec_file_ops functions
On Wed, Feb 28, 2018 at 08:38:00PM +0800, Dave Young wrote:
> On 02/26/18 at 07:01pm, AKASHI Takahiro wrote:
> > On Fri, Feb 23, 2018 at 05:24:59PM +0800, Dave Young wrote:
> > > Hi AKASHI,
> > >
> > > On 02/22/18 at 08:17pm, AKASHI Takahiro wrote:
> > > > As arch_kexec_kernel_*_{probe,load}(),
> > > > arch_kimage_file_post_load_cleanup()
> > > > and arch_kexec_kernel_verify_sg can be parameterized with a
> > > > kexec_file_ops
> > > > array and now duplicated among some architectures, let's factor them
> > > > out.
> > > >
> > > > Signed-off-by: AKASHI Takahiro
> > > > Cc: Dave Young
> > > > Cc: Vivek Goyal
> > > > Cc: Baoquan He
> > > > Cc: Michael Ellerman
> > > > Cc: Thiago Jung Bauermann
> > > > ---
> > > > arch/powerpc/include/asm/kexec.h| 2 +-
> > > > arch/powerpc/kernel/kexec_elf_64.c | 2 +-
> > > > arch/powerpc/kernel/machine_kexec_file_64.c | 39 ++--
> > > > arch/x86/include/asm/kexec-bzimage64.h | 2 +-
> > > > arch/x86/kernel/kexec-bzimage64.c | 2 +-
> > > > arch/x86/kernel/machine_kexec_64.c | 45
> > > > +--
> > > > include/linux/kexec.h | 15
> > > > kernel/kexec_file.c | 57
> > > > +++--
> > > > 8 files changed, 70 insertions(+), 94 deletions(-)
> > > >
> > >
> > > [snip]
> > >
> > > > diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> > > > index 990adae52151..a6d14a768b3e 100644
> > > > --- a/kernel/kexec_file.c
> > > > +++ b/kernel/kexec_file.c
> > > > @@ -26,34 +26,83 @@
> > > > #include
> > > > #include "kexec_internal.h"
> > > >
> > > > +const __weak struct kexec_file_ops * const kexec_file_loaders[] =
> > > > {NULL};
> > > > +
> > > > #ifdef CONFIG_ARCH_HAS_KEXEC_PURGATORY
> > > > static int kexec_calculate_store_digests(struct kimage *image);
> > > > #else
> > > > static int kexec_calculate_store_digests(struct kimage *image) {
> > > > return 0; };
> > > > #endif
> > > >
> > > > +int _kexec_kernel_image_probe(struct kimage *image, void *buf,
> > > > +unsigned long buf_len)
> > > > +{
> > > > + const struct kexec_file_ops * const *fops;
> > > > + int ret = -ENOEXEC;
> > > > +
> > > > + for (fops = &kexec_file_loaders[0]; *fops && (*fops)->probe;
> > > > ++fops) {
> > > > + ret = (*fops)->probe(buf, buf_len);
> > > > + if (!ret) {
> > > > + image->fops = *fops;
> > > > + return ret;
> > > > + }
> > > > + }
> > > > +
> > > > + return ret;
> > > > +}
> > > > +
> > > > /* Architectures can provide this probe function */
> > > > int __weak arch_kexec_kernel_image_probe(struct kimage *image, void
> > > > *buf,
> > > > unsigned long buf_len)
> > > > {
> > > > - return -ENOEXEC;
> > > > + return _kexec_kernel_image_probe(image, buf, buf_len);
> > >
> > >
> > > I vaguely remember previously I suggest split the
> > > _kexec_kernel_image_probe
> > > because arch code can call them, and common code also use it like above.
> > > But in your new series I do not find where else calls this function
> > > except the common code arch_kexec_kernel_image_probe. If nobody use
> > > them then it is not worth to split them out, it is better to just embed
> > > them in the __weak functions.
> >
> > Powerpc's arch_kexec_kernel_image_probe() uses
> > _kexec_kekrnel_image_probe() as it needs an extra check to rule out
> > crash dump for now.
>
> Oops, I missed that, but what about other similar functions? Such as:
> _kexec_kernel_image_load
> _kimage_file_post_load_cleanup
> _kexec_kernel_verify_sig
Current no users.
Given those functions are so simple, we could go either way.
-Takahiro AKASHI
> >
> > Thanks,
> > -Takahiro AKASHI
> >
> >
> > > Ditto for other similar functions.
> > >
> > > [snip]
> > >
> > > Thanks
> > > Dave
>
> Thanks
> Dave
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v8 03/13] kexec_file,x86,powerpc: factor out kexec_file_ops functions
On 02/26/18 at 07:01pm, AKASHI Takahiro wrote:
> On Fri, Feb 23, 2018 at 05:24:59PM +0800, Dave Young wrote:
> > Hi AKASHI,
> >
> > On 02/22/18 at 08:17pm, AKASHI Takahiro wrote:
> > > As arch_kexec_kernel_*_{probe,load}(),
> > > arch_kimage_file_post_load_cleanup()
> > > and arch_kexec_kernel_verify_sg can be parameterized with a kexec_file_ops
> > > array and now duplicated among some architectures, let's factor them out.
> > >
> > > Signed-off-by: AKASHI Takahiro
> > > Cc: Dave Young
> > > Cc: Vivek Goyal
> > > Cc: Baoquan He
> > > Cc: Michael Ellerman
> > > Cc: Thiago Jung Bauermann
> > > ---
> > > arch/powerpc/include/asm/kexec.h| 2 +-
> > > arch/powerpc/kernel/kexec_elf_64.c | 2 +-
> > > arch/powerpc/kernel/machine_kexec_file_64.c | 39 ++--
> > > arch/x86/include/asm/kexec-bzimage64.h | 2 +-
> > > arch/x86/kernel/kexec-bzimage64.c | 2 +-
> > > arch/x86/kernel/machine_kexec_64.c | 45 +--
> > > include/linux/kexec.h | 15
> > > kernel/kexec_file.c | 57
> > > +++--
> > > 8 files changed, 70 insertions(+), 94 deletions(-)
> > >
> >
> > [snip]
> >
> > > diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> > > index 990adae52151..a6d14a768b3e 100644
> > > --- a/kernel/kexec_file.c
> > > +++ b/kernel/kexec_file.c
> > > @@ -26,34 +26,83 @@
> > > #include
> > > #include "kexec_internal.h"
> > >
> > > +const __weak struct kexec_file_ops * const kexec_file_loaders[] = {NULL};
> > > +
> > > #ifdef CONFIG_ARCH_HAS_KEXEC_PURGATORY
> > > static int kexec_calculate_store_digests(struct kimage *image);
> > > #else
> > > static int kexec_calculate_store_digests(struct kimage *image) { return
> > > 0; };
> > > #endif
> > >
> > > +int _kexec_kernel_image_probe(struct kimage *image, void *buf,
> > > + unsigned long buf_len)
> > > +{
> > > + const struct kexec_file_ops * const *fops;
> > > + int ret = -ENOEXEC;
> > > +
> > > + for (fops = &kexec_file_loaders[0]; *fops && (*fops)->probe; ++fops) {
> > > + ret = (*fops)->probe(buf, buf_len);
> > > + if (!ret) {
> > > + image->fops = *fops;
> > > + return ret;
> > > + }
> > > + }
> > > +
> > > + return ret;
> > > +}
> > > +
> > > /* Architectures can provide this probe function */
> > > int __weak arch_kexec_kernel_image_probe(struct kimage *image, void *buf,
> > >unsigned long buf_len)
> > > {
> > > - return -ENOEXEC;
> > > + return _kexec_kernel_image_probe(image, buf, buf_len);
> >
> >
> > I vaguely remember previously I suggest split the _kexec_kernel_image_probe
> > because arch code can call them, and common code also use it like above.
> > But in your new series I do not find where else calls this function
> > except the common code arch_kexec_kernel_image_probe. If nobody use
> > them then it is not worth to split them out, it is better to just embed
> > them in the __weak functions.
>
> Powerpc's arch_kexec_kernel_image_probe() uses
> _kexec_kekrnel_image_probe() as it needs an extra check to rule out
> crash dump for now.
Oops, I missed that, but what about other similar functions? Such as:
_kexec_kernel_image_load
_kimage_file_post_load_cleanup
_kexec_kernel_verify_sig
>
> Thanks,
> -Takahiro AKASHI
>
>
> > Ditto for other similar functions.
> >
> > [snip]
> >
> > Thanks
> > Dave
Thanks
Dave
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v8 03/13] kexec_file, x86, powerpc: factor out kexec_file_ops functions
On Tue, 27 Feb 2018 11:03:07 +0900
AKASHI Takahiro wrote:
> On Mon, Feb 26, 2018 at 12:17:18PM +0100, Philipp Rudo wrote:
> > Hi AKASHI
> >
> > On Thu, 22 Feb 2018 20:17:22 +0900
> > AKASHI Takahiro wrote:
> >
> > [...]
> >
> > > diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> > > index 990adae52151..a6d14a768b3e 100644
> > > --- a/kernel/kexec_file.c
> > > +++ b/kernel/kexec_file.c
> > > @@ -26,34 +26,83 @@
> > > #include
> > > #include "kexec_internal.h"
> > >
> > > +const __weak struct kexec_file_ops * const kexec_file_loaders[] = {NULL};
> > > +
> >
> > Having a weak definition of kexec_file_loaders causes trouble on s390 with
> > gcc 4.8 (newer versions seem to work fine). For me it looks like that in
> > this
> > version gcc doesn't recognize __weak but use the default value for
> > optimization. This leads to _kexec_kernel_image_probe to always return
> > ENOEXEC
> > because the for-loop gets optimized out.
>
> I gave it a try to compile with gcc 4.9 (not 4.8) for arm64
> and didn't see any errors or warnings, but
I talked to our compiler guys, and it looks like its a bug in gcc which was
introduced with gcc 4.8 and removed again with gcc 4.9. So I was just extremely
lucky hitting the sweat spot...
> > The problem can easily be worked around by declaring kexec_file_loaders in
> > include/linux/kexec.h and defining it in arch code. In particular doing this
> >
> > diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> > index 37e9dce518aa..fc0788540d90 100644
> > --- a/include/linux/kexec.h
> > +++ b/include/linux/kexec.h
> > @@ -139,6 +139,8 @@ struct kexec_file_ops {
> > #endif
> > };
> >
> > +extern const struct kexec_file_ops * const kexec_file_loaders[];
> > +
> > /**
> > * struct kexec_buf - parameters for finding a place for a buffer in memory
> > * @image: kexec image in which memory to search.
> > diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> > index 17ba407d0e79..4e3d1e4bc7f6 100644
> > --- a/kernel/kexec_file.c
> > +++ b/kernel/kexec_file.c
> > @@ -31,8 +31,6 @@
> > #include
> > #include "kexec_internal.h"
> >
> > -const __weak struct kexec_file_ops * const kexec_file_loaders[] = {NULL};
> > -
> > #ifdef CONFIG_ARCH_HAS_KEXEC_PURGATORY
> > static int kexec_calculate_store_digests(struct kimage *image);
> > #else
>
> Your change is just fine with me, too.
> I will incorporate it in my next version.
Thanks a lot
Philipp
> Thanks,
> -Takahiro AKASHI
>
> > A nice side effect of this solution is, that a developer who forgets to
> > define
> > kexec_file_loaders gets a linker error. So he directly knows what's missing
> > instead of first having to find out where/why an error gets returned.
> >
> > Otherwise the series is fine for me.
> >
> > Thanks
> > Philipp
> >
> > > #ifdef CONFIG_ARCH_HAS_KEXEC_PURGATORY
> > > static int kexec_calculate_store_digests(struct kimage *image);
> > > #else
> > > static int kexec_calculate_store_digests(struct kimage *image) { return
> > > 0; };
> > > #endif
> > >
> > > +int _kexec_kernel_image_probe(struct kimage *image, void *buf,
> > > + unsigned long buf_len)
> > > +{
> > > + const struct kexec_file_ops * const *fops;
> > > + int ret = -ENOEXEC;
> > > +
> > > + for (fops = &kexec_file_loaders[0]; *fops && (*fops)->probe; ++fops) {
> > > + ret = (*fops)->probe(buf, buf_len);
> > > + if (!ret) {
> > > + image->fops = *fops;
> > > + return ret;
> > > + }
> > > + }
> > > +
> > > + return ret;
> > > +}
> > > +
> > > /* Architectures can provide this probe function */
> > > int __weak arch_kexec_kernel_image_probe(struct kimage *image, void *buf,
> > >unsigned long buf_len)
> > > {
> > > - return -ENOEXEC;
> > > + return _kexec_kernel_image_probe(image, buf, buf_len);
> > > +}
> > > +
> > > +void *_kexec_kernel_image_load(struct kimage *image)
> > > +{
> > > + if (!image->fops || !image->fops->load)
> > > + return ERR_PTR(-ENOEXEC);
> > > +
> > > + return image->fops->load(image, image->kernel_buf,
> > > + image->kernel_buf_len, image->initrd_buf,
> > > + image->initrd_buf_len, image->cmdline_buf,
> > > + image->cmdline_buf_len);
> > > }
> > >
> > > void * __weak arch_kexec_kernel_image_load(struct kimage *image)
> > > {
> > > - return ERR_PTR(-ENOEXEC);
> > > + return _kexec_kernel_image_load(image);
> > > +}
> > > +
> > > +int _kimage_file_post_load_cleanup(struct kimage *image)
> > > +{
> > > + if (!image->fops || !image->fops->cleanup)
> > > + return 0;
> > > +
> > > + return image->fops->cleanup(image->image_loader_data);
> > > }
> > >
> > > int __weak arch_kimage_file_post_load_cleanup(struct kimage *image)
> > > {
> > > - return -EINVAL;
> > > + return _kimage_file_post_load_cleanup(image);
> > > }
> > >
> > > #ifdef CONFIG_KEXEC_VERIFY_SIG
> > > +int _kexec_
Re: [PATCH v8 03/13] kexec_file, x86, powerpc: factor out kexec_file_ops functions
On Mon, Feb 26, 2018 at 12:17:18PM +0100, Philipp Rudo wrote:
> Hi AKASHI
>
> On Thu, 22 Feb 2018 20:17:22 +0900
> AKASHI Takahiro wrote:
>
> [...]
>
> > diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> > index 990adae52151..a6d14a768b3e 100644
> > --- a/kernel/kexec_file.c
> > +++ b/kernel/kexec_file.c
> > @@ -26,34 +26,83 @@
> > #include
> > #include "kexec_internal.h"
> >
> > +const __weak struct kexec_file_ops * const kexec_file_loaders[] = {NULL};
> > +
>
> Having a weak definition of kexec_file_loaders causes trouble on s390 with
> gcc 4.8 (newer versions seem to work fine). For me it looks like that in this
> version gcc doesn't recognize __weak but use the default value for
> optimization. This leads to _kexec_kernel_image_probe to always return ENOEXEC
> because the for-loop gets optimized out.
I gave it a try to compile with gcc 4.9 (not 4.8) for arm64
and didn't see any errors or warnings, but
> The problem can easily be worked around by declaring kexec_file_loaders in
> include/linux/kexec.h and defining it in arch code. In particular doing this
>
> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> index 37e9dce518aa..fc0788540d90 100644
> --- a/include/linux/kexec.h
> +++ b/include/linux/kexec.h
> @@ -139,6 +139,8 @@ struct kexec_file_ops {
> #endif
> };
>
> +extern const struct kexec_file_ops * const kexec_file_loaders[];
> +
> /**
> * struct kexec_buf - parameters for finding a place for a buffer in memory
> * @image: kexec image in which memory to search.
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index 17ba407d0e79..4e3d1e4bc7f6 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -31,8 +31,6 @@
> #include
> #include "kexec_internal.h"
>
> -const __weak struct kexec_file_ops * const kexec_file_loaders[] = {NULL};
> -
> #ifdef CONFIG_ARCH_HAS_KEXEC_PURGATORY
> static int kexec_calculate_store_digests(struct kimage *image);
> #else
Your change is just fine with me, too.
I will incorporate it in my next version.
Thanks,
-Takahiro AKASHI
> A nice side effect of this solution is, that a developer who forgets to define
> kexec_file_loaders gets a linker error. So he directly knows what's missing
> instead of first having to find out where/why an error gets returned.
>
> Otherwise the series is fine for me.
>
> Thanks
> Philipp
>
> > #ifdef CONFIG_ARCH_HAS_KEXEC_PURGATORY
> > static int kexec_calculate_store_digests(struct kimage *image);
> > #else
> > static int kexec_calculate_store_digests(struct kimage *image) { return 0;
> > };
> > #endif
> >
> > +int _kexec_kernel_image_probe(struct kimage *image, void *buf,
> > +unsigned long buf_len)
> > +{
> > + const struct kexec_file_ops * const *fops;
> > + int ret = -ENOEXEC;
> > +
> > + for (fops = &kexec_file_loaders[0]; *fops && (*fops)->probe; ++fops) {
> > + ret = (*fops)->probe(buf, buf_len);
> > + if (!ret) {
> > + image->fops = *fops;
> > + return ret;
> > + }
> > + }
> > +
> > + return ret;
> > +}
> > +
> > /* Architectures can provide this probe function */
> > int __weak arch_kexec_kernel_image_probe(struct kimage *image, void *buf,
> > unsigned long buf_len)
> > {
> > - return -ENOEXEC;
> > + return _kexec_kernel_image_probe(image, buf, buf_len);
> > +}
> > +
> > +void *_kexec_kernel_image_load(struct kimage *image)
> > +{
> > + if (!image->fops || !image->fops->load)
> > + return ERR_PTR(-ENOEXEC);
> > +
> > + return image->fops->load(image, image->kernel_buf,
> > +image->kernel_buf_len, image->initrd_buf,
> > +image->initrd_buf_len, image->cmdline_buf,
> > +image->cmdline_buf_len);
> > }
> >
> > void * __weak arch_kexec_kernel_image_load(struct kimage *image)
> > {
> > - return ERR_PTR(-ENOEXEC);
> > + return _kexec_kernel_image_load(image);
> > +}
> > +
> > +int _kimage_file_post_load_cleanup(struct kimage *image)
> > +{
> > + if (!image->fops || !image->fops->cleanup)
> > + return 0;
> > +
> > + return image->fops->cleanup(image->image_loader_data);
> > }
> >
> > int __weak arch_kimage_file_post_load_cleanup(struct kimage *image)
> > {
> > - return -EINVAL;
> > + return _kimage_file_post_load_cleanup(image);
> > }
> >
> > #ifdef CONFIG_KEXEC_VERIFY_SIG
> > +int _kexec_kernel_verify_sig(struct kimage *image, void *buf,
> > + unsigned long buf_len)
> > +{
> > + if (!image->fops || !image->fops->verify_sig) {
> > + pr_debug("kernel loader does not support signature
> > verification.\n");
> > + return -EKEYREJECTED;
> > + }
> > +
> > + return image->fops->verify_sig(buf, buf_len);
> > +}
> > +
> > int __weak arch_kexec_kernel_verify_sig(struct kimage *image, void *buf,
> > unsigned long buf_
Re: [PATCH v8 03/13] kexec_file,x86,powerpc: factor out kexec_file_ops functions
On Mon, 26 Feb 2018 19:01:39 +0900
AKASHI Takahiro wrote:
> On Fri, Feb 23, 2018 at 05:24:59PM +0800, Dave Young wrote:
> > Hi AKASHI,
> >
> > On 02/22/18 at 08:17pm, AKASHI Takahiro wrote:
> > > As arch_kexec_kernel_*_{probe,load}(),
> > > arch_kimage_file_post_load_cleanup()
> > > and arch_kexec_kernel_verify_sg can be parameterized with a kexec_file_ops
> > > array and now duplicated among some architectures, let's factor them out.
> > >
> > > Signed-off-by: AKASHI Takahiro
> > > Cc: Dave Young
> > > Cc: Vivek Goyal
> > > Cc: Baoquan He
> > > Cc: Michael Ellerman
> > > Cc: Thiago Jung Bauermann
> > > ---
> > > arch/powerpc/include/asm/kexec.h| 2 +-
> > > arch/powerpc/kernel/kexec_elf_64.c | 2 +-
> > > arch/powerpc/kernel/machine_kexec_file_64.c | 39 ++--
> > > arch/x86/include/asm/kexec-bzimage64.h | 2 +-
> > > arch/x86/kernel/kexec-bzimage64.c | 2 +-
> > > arch/x86/kernel/machine_kexec_64.c | 45 +--
> > > include/linux/kexec.h | 15
> > > kernel/kexec_file.c | 57
> > > +++--
> > > 8 files changed, 70 insertions(+), 94 deletions(-)
> > >
> >
> > [snip]
> >
> > > diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> > > index 990adae52151..a6d14a768b3e 100644
> > > --- a/kernel/kexec_file.c
> > > +++ b/kernel/kexec_file.c
> > > @@ -26,34 +26,83 @@
> > > #include
> > > #include "kexec_internal.h"
> > >
> > > +const __weak struct kexec_file_ops * const kexec_file_loaders[] = {NULL};
> > > +
> > > #ifdef CONFIG_ARCH_HAS_KEXEC_PURGATORY
> > > static int kexec_calculate_store_digests(struct kimage *image);
> > > #else
> > > static int kexec_calculate_store_digests(struct kimage *image) { return
> > > 0; };
> > > #endif
> > >
> > > +int _kexec_kernel_image_probe(struct kimage *image, void *buf,
> > > + unsigned long buf_len)
> > > +{
> > > + const struct kexec_file_ops * const *fops;
> > > + int ret = -ENOEXEC;
> > > +
> > > + for (fops = &kexec_file_loaders[0]; *fops && (*fops)->probe; ++fops) {
> > > + ret = (*fops)->probe(buf, buf_len);
> > > + if (!ret) {
> > > + image->fops = *fops;
> > > + return ret;
> > > + }
> > > + }
> > > +
> > > + return ret;
> > > +}
> > > +
> > > /* Architectures can provide this probe function */
> > > int __weak arch_kexec_kernel_image_probe(struct kimage *image, void *buf,
> > >unsigned long buf_len)
> > > {
> > > - return -ENOEXEC;
> > > + return _kexec_kernel_image_probe(image, buf, buf_len);
> >
> >
> > I vaguely remember previously I suggest split the _kexec_kernel_image_probe
> > because arch code can call them, and common code also use it like above.
> > But in your new series I do not find where else calls this function
> > except the common code arch_kexec_kernel_image_probe. If nobody use
> > them then it is not worth to split them out, it is better to just embed
> > them in the __weak functions.
>
> Powerpc's arch_kexec_kernel_image_probe() uses
> _kexec_kekrnel_image_probe() as it needs an extra check to rule out
> crash dump for now.
s390 has to use it too. We have to write to a fixed address in the buffer. So
we need to check if the buffer is large enough to write to that address.
Philipp
> Thanks,
> -Takahiro AKASHI
>
>
> > Ditto for other similar functions.
> >
> > [snip]
> >
> > Thanks
> > Dave
>
> ___
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec
>
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v8 03/13] kexec_file, x86, powerpc: factor out kexec_file_ops functions
Hi AKASHI
On Thu, 22 Feb 2018 20:17:22 +0900
AKASHI Takahiro wrote:
[...]
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index 990adae52151..a6d14a768b3e 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -26,34 +26,83 @@
> #include
> #include "kexec_internal.h"
>
> +const __weak struct kexec_file_ops * const kexec_file_loaders[] = {NULL};
> +
Having a weak definition of kexec_file_loaders causes trouble on s390 with
gcc 4.8 (newer versions seem to work fine). For me it looks like that in this
version gcc doesn't recognize __weak but use the default value for
optimization. This leads to _kexec_kernel_image_probe to always return ENOEXEC
because the for-loop gets optimized out.
The problem can easily be worked around by declaring kexec_file_loaders in
include/linux/kexec.h and defining it in arch code. In particular doing this
diff --git a/include/linux/kexec.h b/include/linux/kexec.h
index 37e9dce518aa..fc0788540d90 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -139,6 +139,8 @@ struct kexec_file_ops {
#endif
};
+extern const struct kexec_file_ops * const kexec_file_loaders[];
+
/**
* struct kexec_buf - parameters for finding a place for a buffer in memory
* @image: kexec image in which memory to search.
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index 17ba407d0e79..4e3d1e4bc7f6 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -31,8 +31,6 @@
#include
#include "kexec_internal.h"
-const __weak struct kexec_file_ops * const kexec_file_loaders[] = {NULL};
-
#ifdef CONFIG_ARCH_HAS_KEXEC_PURGATORY
static int kexec_calculate_store_digests(struct kimage *image);
#else
A nice side effect of this solution is, that a developer who forgets to define
kexec_file_loaders gets a linker error. So he directly knows what's missing
instead of first having to find out where/why an error gets returned.
Otherwise the series is fine for me.
Thanks
Philipp
> #ifdef CONFIG_ARCH_HAS_KEXEC_PURGATORY
> static int kexec_calculate_store_digests(struct kimage *image);
> #else
> static int kexec_calculate_store_digests(struct kimage *image) { return 0; };
> #endif
>
> +int _kexec_kernel_image_probe(struct kimage *image, void *buf,
> + unsigned long buf_len)
> +{
> + const struct kexec_file_ops * const *fops;
> + int ret = -ENOEXEC;
> +
> + for (fops = &kexec_file_loaders[0]; *fops && (*fops)->probe; ++fops) {
> + ret = (*fops)->probe(buf, buf_len);
> + if (!ret) {
> + image->fops = *fops;
> + return ret;
> + }
> + }
> +
> + return ret;
> +}
> +
> /* Architectures can provide this probe function */
> int __weak arch_kexec_kernel_image_probe(struct kimage *image, void *buf,
>unsigned long buf_len)
> {
> - return -ENOEXEC;
> + return _kexec_kernel_image_probe(image, buf, buf_len);
> +}
> +
> +void *_kexec_kernel_image_load(struct kimage *image)
> +{
> + if (!image->fops || !image->fops->load)
> + return ERR_PTR(-ENOEXEC);
> +
> + return image->fops->load(image, image->kernel_buf,
> + image->kernel_buf_len, image->initrd_buf,
> + image->initrd_buf_len, image->cmdline_buf,
> + image->cmdline_buf_len);
> }
>
> void * __weak arch_kexec_kernel_image_load(struct kimage *image)
> {
> - return ERR_PTR(-ENOEXEC);
> + return _kexec_kernel_image_load(image);
> +}
> +
> +int _kimage_file_post_load_cleanup(struct kimage *image)
> +{
> + if (!image->fops || !image->fops->cleanup)
> + return 0;
> +
> + return image->fops->cleanup(image->image_loader_data);
> }
>
> int __weak arch_kimage_file_post_load_cleanup(struct kimage *image)
> {
> - return -EINVAL;
> + return _kimage_file_post_load_cleanup(image);
> }
>
> #ifdef CONFIG_KEXEC_VERIFY_SIG
> +int _kexec_kernel_verify_sig(struct kimage *image, void *buf,
> + unsigned long buf_len)
> +{
> + if (!image->fops || !image->fops->verify_sig) {
> + pr_debug("kernel loader does not support signature
> verification.\n");
> + return -EKEYREJECTED;
> + }
> +
> + return image->fops->verify_sig(buf, buf_len);
> +}
> +
> int __weak arch_kexec_kernel_verify_sig(struct kimage *image, void *buf,
> unsigned long buf_len)
> {
> - return -EKEYREJECTED;
> + return _kexec_kernel_verify_sig(image, buf, buf_len);
> }
> #endif
>
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v8 03/13] kexec_file,x86,powerpc: factor out kexec_file_ops functions
On Fri, Feb 23, 2018 at 05:24:59PM +0800, Dave Young wrote:
> Hi AKASHI,
>
> On 02/22/18 at 08:17pm, AKASHI Takahiro wrote:
> > As arch_kexec_kernel_*_{probe,load}(), arch_kimage_file_post_load_cleanup()
> > and arch_kexec_kernel_verify_sg can be parameterized with a kexec_file_ops
> > array and now duplicated among some architectures, let's factor them out.
> >
> > Signed-off-by: AKASHI Takahiro
> > Cc: Dave Young
> > Cc: Vivek Goyal
> > Cc: Baoquan He
> > Cc: Michael Ellerman
> > Cc: Thiago Jung Bauermann
> > ---
> > arch/powerpc/include/asm/kexec.h| 2 +-
> > arch/powerpc/kernel/kexec_elf_64.c | 2 +-
> > arch/powerpc/kernel/machine_kexec_file_64.c | 39 ++--
> > arch/x86/include/asm/kexec-bzimage64.h | 2 +-
> > arch/x86/kernel/kexec-bzimage64.c | 2 +-
> > arch/x86/kernel/machine_kexec_64.c | 45 +--
> > include/linux/kexec.h | 15
> > kernel/kexec_file.c | 57
> > +++--
> > 8 files changed, 70 insertions(+), 94 deletions(-)
> >
>
> [snip]
>
> > diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> > index 990adae52151..a6d14a768b3e 100644
> > --- a/kernel/kexec_file.c
> > +++ b/kernel/kexec_file.c
> > @@ -26,34 +26,83 @@
> > #include
> > #include "kexec_internal.h"
> >
> > +const __weak struct kexec_file_ops * const kexec_file_loaders[] = {NULL};
> > +
> > #ifdef CONFIG_ARCH_HAS_KEXEC_PURGATORY
> > static int kexec_calculate_store_digests(struct kimage *image);
> > #else
> > static int kexec_calculate_store_digests(struct kimage *image) { return 0;
> > };
> > #endif
> >
> > +int _kexec_kernel_image_probe(struct kimage *image, void *buf,
> > +unsigned long buf_len)
> > +{
> > + const struct kexec_file_ops * const *fops;
> > + int ret = -ENOEXEC;
> > +
> > + for (fops = &kexec_file_loaders[0]; *fops && (*fops)->probe; ++fops) {
> > + ret = (*fops)->probe(buf, buf_len);
> > + if (!ret) {
> > + image->fops = *fops;
> > + return ret;
> > + }
> > + }
> > +
> > + return ret;
> > +}
> > +
> > /* Architectures can provide this probe function */
> > int __weak arch_kexec_kernel_image_probe(struct kimage *image, void *buf,
> > unsigned long buf_len)
> > {
> > - return -ENOEXEC;
> > + return _kexec_kernel_image_probe(image, buf, buf_len);
>
>
> I vaguely remember previously I suggest split the _kexec_kernel_image_probe
> because arch code can call them, and common code also use it like above.
> But in your new series I do not find where else calls this function
> except the common code arch_kexec_kernel_image_probe. If nobody use
> them then it is not worth to split them out, it is better to just embed
> them in the __weak functions.
Powerpc's arch_kexec_kernel_image_probe() uses
_kexec_kekrnel_image_probe() as it needs an extra check to rule out
crash dump for now.
Thanks,
-Takahiro AKASHI
> Ditto for other similar functions.
>
> [snip]
>
> Thanks
> Dave
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v8 03/13] kexec_file,x86,powerpc: factor out kexec_file_ops functions
Hi AKASHI,
On 02/22/18 at 08:17pm, AKASHI Takahiro wrote:
> As arch_kexec_kernel_*_{probe,load}(), arch_kimage_file_post_load_cleanup()
> and arch_kexec_kernel_verify_sg can be parameterized with a kexec_file_ops
> array and now duplicated among some architectures, let's factor them out.
>
> Signed-off-by: AKASHI Takahiro
> Cc: Dave Young
> Cc: Vivek Goyal
> Cc: Baoquan He
> Cc: Michael Ellerman
> Cc: Thiago Jung Bauermann
> ---
> arch/powerpc/include/asm/kexec.h| 2 +-
> arch/powerpc/kernel/kexec_elf_64.c | 2 +-
> arch/powerpc/kernel/machine_kexec_file_64.c | 39 ++--
> arch/x86/include/asm/kexec-bzimage64.h | 2 +-
> arch/x86/kernel/kexec-bzimage64.c | 2 +-
> arch/x86/kernel/machine_kexec_64.c | 45 +--
> include/linux/kexec.h | 15
> kernel/kexec_file.c | 57
> +++--
> 8 files changed, 70 insertions(+), 94 deletions(-)
>
[snip]
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index 990adae52151..a6d14a768b3e 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -26,34 +26,83 @@
> #include
> #include "kexec_internal.h"
>
> +const __weak struct kexec_file_ops * const kexec_file_loaders[] = {NULL};
> +
> #ifdef CONFIG_ARCH_HAS_KEXEC_PURGATORY
> static int kexec_calculate_store_digests(struct kimage *image);
> #else
> static int kexec_calculate_store_digests(struct kimage *image) { return 0; };
> #endif
>
> +int _kexec_kernel_image_probe(struct kimage *image, void *buf,
> + unsigned long buf_len)
> +{
> + const struct kexec_file_ops * const *fops;
> + int ret = -ENOEXEC;
> +
> + for (fops = &kexec_file_loaders[0]; *fops && (*fops)->probe; ++fops) {
> + ret = (*fops)->probe(buf, buf_len);
> + if (!ret) {
> + image->fops = *fops;
> + return ret;
> + }
> + }
> +
> + return ret;
> +}
> +
> /* Architectures can provide this probe function */
> int __weak arch_kexec_kernel_image_probe(struct kimage *image, void *buf,
>unsigned long buf_len)
> {
> - return -ENOEXEC;
> + return _kexec_kernel_image_probe(image, buf, buf_len);
I vaguely remember previously I suggest split the _kexec_kernel_image_probe
because arch code can call them, and common code also use it like above.
But in your new series I do not find where else calls this function
except the common code arch_kexec_kernel_image_probe. If nobody use
them then it is not worth to split them out, it is better to just embed
them in the __weak functions.
Ditto for other similar functions.
[snip]
Thanks
Dave
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
[PATCH v8 03/13] kexec_file, x86, powerpc: factor out kexec_file_ops functions
As arch_kexec_kernel_*_{probe,load}(), arch_kimage_file_post_load_cleanup()
and arch_kexec_kernel_verify_sg can be parameterized with a kexec_file_ops
array and now duplicated among some architectures, let's factor them out.
Signed-off-by: AKASHI Takahiro
Cc: Dave Young
Cc: Vivek Goyal
Cc: Baoquan He
Cc: Michael Ellerman
Cc: Thiago Jung Bauermann
---
arch/powerpc/include/asm/kexec.h| 2 +-
arch/powerpc/kernel/kexec_elf_64.c | 2 +-
arch/powerpc/kernel/machine_kexec_file_64.c | 39 ++--
arch/x86/include/asm/kexec-bzimage64.h | 2 +-
arch/x86/kernel/kexec-bzimage64.c | 2 +-
arch/x86/kernel/machine_kexec_64.c | 45 +--
include/linux/kexec.h | 15
kernel/kexec_file.c | 57 +++--
8 files changed, 70 insertions(+), 94 deletions(-)
diff --git a/arch/powerpc/include/asm/kexec.h b/arch/powerpc/include/asm/kexec.h
index d8b1e8e7e035..4a585cba1787 100644
--- a/arch/powerpc/include/asm/kexec.h
+++ b/arch/powerpc/include/asm/kexec.h
@@ -95,7 +95,7 @@ static inline bool kdump_in_progress(void)
}
#ifdef CONFIG_KEXEC_FILE
-extern struct kexec_file_ops kexec_elf64_ops;
+extern const struct kexec_file_ops kexec_elf64_ops;
#ifdef CONFIG_IMA_KEXEC
#define ARCH_HAS_KIMAGE_ARCH
diff --git a/arch/powerpc/kernel/kexec_elf_64.c
b/arch/powerpc/kernel/kexec_elf_64.c
index 9a42309b091a..6c78c11c7faf 100644
--- a/arch/powerpc/kernel/kexec_elf_64.c
+++ b/arch/powerpc/kernel/kexec_elf_64.c
@@ -657,7 +657,7 @@ static void *elf64_load(struct kimage *image, char
*kernel_buf,
return ret ? ERR_PTR(ret) : fdt;
}
-struct kexec_file_ops kexec_elf64_ops = {
+const struct kexec_file_ops kexec_elf64_ops = {
.probe = elf64_probe,
.load = elf64_load,
};
diff --git a/arch/powerpc/kernel/machine_kexec_file_64.c
b/arch/powerpc/kernel/machine_kexec_file_64.c
index e4395f937d63..a27ec647350c 100644
--- a/arch/powerpc/kernel/machine_kexec_file_64.c
+++ b/arch/powerpc/kernel/machine_kexec_file_64.c
@@ -31,52 +31,19 @@
#define SLAVE_CODE_SIZE256
-static struct kexec_file_ops *kexec_file_loaders[] = {
+const struct kexec_file_ops * const kexec_file_loaders[] = {
&kexec_elf64_ops,
+ NULL
};
int arch_kexec_kernel_image_probe(struct kimage *image, void *buf,
unsigned long buf_len)
{
- int i, ret = -ENOEXEC;
- struct kexec_file_ops *fops;
-
/* We don't support crash kernels yet. */
if (image->type == KEXEC_TYPE_CRASH)
return -ENOTSUPP;
- for (i = 0; i < ARRAY_SIZE(kexec_file_loaders); i++) {
- fops = kexec_file_loaders[i];
- if (!fops || !fops->probe)
- continue;
-
- ret = fops->probe(buf, buf_len);
- if (!ret) {
- image->fops = fops;
- return ret;
- }
- }
-
- return ret;
-}
-
-void *arch_kexec_kernel_image_load(struct kimage *image)
-{
- if (!image->fops || !image->fops->load)
- return ERR_PTR(-ENOEXEC);
-
- return image->fops->load(image, image->kernel_buf,
-image->kernel_buf_len, image->initrd_buf,
-image->initrd_buf_len, image->cmdline_buf,
-image->cmdline_buf_len);
-}
-
-int arch_kimage_file_post_load_cleanup(struct kimage *image)
-{
- if (!image->fops || !image->fops->cleanup)
- return 0;
-
- return image->fops->cleanup(image->image_loader_data);
+ return _kexec_kernel_image_probe(image, buf, buf_len);
}
/**
diff --git a/arch/x86/include/asm/kexec-bzimage64.h
b/arch/x86/include/asm/kexec-bzimage64.h
index 9f07cff43705..df89ee7d3e9e 100644
--- a/arch/x86/include/asm/kexec-bzimage64.h
+++ b/arch/x86/include/asm/kexec-bzimage64.h
@@ -2,6 +2,6 @@
#ifndef _ASM_KEXEC_BZIMAGE64_H
#define _ASM_KEXEC_BZIMAGE64_H
-extern struct kexec_file_ops kexec_bzImage64_ops;
+extern const struct kexec_file_ops kexec_bzImage64_ops;
#endif /* _ASM_KEXE_BZIMAGE64_H */
diff --git a/arch/x86/kernel/kexec-bzimage64.c
b/arch/x86/kernel/kexec-bzimage64.c
index fb095ba0c02f..705654776c0c 100644
--- a/arch/x86/kernel/kexec-bzimage64.c
+++ b/arch/x86/kernel/kexec-bzimage64.c
@@ -538,7 +538,7 @@ static int bzImage64_verify_sig(const char *kernel,
unsigned long kernel_len)
}
#endif
-struct kexec_file_ops kexec_bzImage64_ops = {
+const struct kexec_file_ops kexec_bzImage64_ops = {
.probe = bzImage64_probe,
.load = bzImage64_load,
.cleanup = bzImage64_cleanup,
diff --git a/arch/x86/kernel/machine_kexec_64.c
b/arch/x86/kernel/machine_kexec_64.c
index 1f790cf9d38f..2cdd29d64181 100644
--- a/arch/x86/kernel/machine_kexec_64.c
+++ b/arch/x86/kernel/machine_kexec_64.c
@@ -30,8 +30,9 @@
#include
#ifdef CONFIG_KEXEC_FILE
-
