[PATCH RFC V2] purgatory: fix up declarations

[PATCH RFC V2] purgatory: fix up declarations

[Nicholas Mc Guire]

Add the missing declarations of basic purgatory functions and variables
used with kexec_purgatory_get_set_symbol() to allow a clean build.

Fixes: commit 8fc5b4d4121c ("purgatory: core purgatory functionality")
Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
---

V2: after kbuild test robot <lkp@intel.com> reported a build failure
    removed incorrect declaration of copy_backup_region which is static
    anyway.

sparse complained about:
  CHECK   arch/x86/purgatory/purgatory.c
arch/x86/purgatory/purgatory.c:21:15: warning: symbol 'backup_dest' was not declared. Should it be static?
arch/x86/purgatory/purgatory.c:22:15: warning: symbol 'backup_src' was not declared. Should it be static?
arch/x86/purgatory/purgatory.c:23:15: warning: symbol 'backup_sz' was not declared. Should it be static?
arch/x86/purgatory/purgatory.c:25:4: warning: symbol 'sha256_digest' was not declared. Should it be static?
arch/x86/purgatory/purgatory.c:27:19: warning: symbol 'sha_regions' was not declared. Should it be static?
arch/x86/purgatory/purgatory.c:42:5: warning: symbol 'verify_sha256_digest' was not declared. Should it be static?
arch/x86/purgatory/purgatory.c:61:6: warning: symbol 'purgatory' was not declared. Should it be static?
  CC      arch/x86/purgatory/purgatory.o

Numerous sparse messages regarding functions not being declared, these
functions are resolved via kexec_purgatory_get_set_symbol() and not
directly called anywhere. To resolve the sparse issues appropriate
declarations were added to asm/kexec-bzimage64.h and the appropriate
reference included in purgatory.c. Adding this to kexec-bzimage64.h
was done as setup_purgatory() from machine_kexec_file_64.c uses
these symbols - not sure if this is the proper place to add this.

While at it the initialization of sha_regions to {{0,0}} was added.

Patch was compile tested with: x86_64_defconfig (implies CONFIG_KEXEC=y)

Patch is against 4.9.0 (localversion-next is next-20161223)

 arch/x86/include/asm/kexec-bzimage64.h | 16 ++++++++++++++++
 arch/x86/purgatory/purgatory.c         |  8 ++------
 2 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/arch/x86/include/asm/kexec-bzimage64.h b/arch/x86/include/asm/kexec-bzimage64.h
index d1b5d19..fd7f776 100644
--- a/arch/x86/include/asm/kexec-bzimage64.h
+++ b/arch/x86/include/asm/kexec-bzimage64.h
@@ -1,6 +1,21 @@
 #ifndef _ASM_KEXEC_BZIMAGE64_H
 #define _ASM_KEXEC_BZIMAGE64_H
 
+struct sha_region {
+	unsigned long start;
+	unsigned long len;
+};
+
 extern struct kexec_file_ops kexec_bzImage64_ops;
 
+/* needed for kexec_purgatory_get_set_symbol() */
+extern unsigned long backup_dest;
+extern unsigned long backup_src;
+extern unsigned long backup_sz;
+extern u8 sha256_digest[];
+extern struct sha_region sha_regions[];
+
+void purgatory(void);
+int verify_sha256_digest(void);
+
 #endif  /* _ASM_KEXE_BZIMAGE64_H */
diff --git a/arch/x86/purgatory/purgatory.c b/arch/x86/purgatory/purgatory.c
index 25e068b..26c8367 100644
--- a/arch/x86/purgatory/purgatory.c
+++ b/arch/x86/purgatory/purgatory.c
@@ -12,11 +12,7 @@
 
 #include "sha256.h"
 #include "../boot/string.h"
-
-struct sha_region {
-	unsigned long start;
-	unsigned long len;
-};
+#include <asm/kexec-bzimage64.h>
 
 unsigned long backup_dest = 0;
 unsigned long backup_src = 0;
@@ -24,7 +20,7 @@ unsigned long backup_sz = 0;
 
 u8 sha256_digest[SHA256_DIGEST_SIZE] = { 0 };
 
-struct sha_region sha_regions[16] = {};
+struct sha_region sha_regions[16] = { { 0, 0 } };
 
 /*
  * On x86, second kernel requries first 640K of memory to boot. Copy
-- 
2.1.4

Re: [PATCH RFC V2] purgatory: fix up declarations

[Vivek Goyal]

On Fri, Dec 23, 2016 at 12:43:07PM +0100, Nicholas Mc Guire wrote:
> Add the missing declarations of basic purgatory functions and variables
> used with kexec_purgatory_get_set_symbol() to allow a clean build.
> 
> Fixes: commit 8fc5b4d4121c ("purgatory: core purgatory functionality")
> Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> ---
> 
> V2: after kbuild test robot <lkp@intel.com> reported a build failure
>     removed incorrect declaration of copy_backup_region which is static
>     anyway.
> 
> sparse complained about:
>   CHECK   arch/x86/purgatory/purgatory.c
> arch/x86/purgatory/purgatory.c:21:15: warning: symbol 'backup_dest' was not declared. Should it be static?
> arch/x86/purgatory/purgatory.c:22:15: warning: symbol 'backup_src' was not declared. Should it be static?
> arch/x86/purgatory/purgatory.c:23:15: warning: symbol 'backup_sz' was not declared. Should it be static?
> arch/x86/purgatory/purgatory.c:25:4: warning: symbol 'sha256_digest' was not declared. Should it be static?
> arch/x86/purgatory/purgatory.c:27:19: warning: symbol 'sha_regions' was not declared. Should it be static?
> arch/x86/purgatory/purgatory.c:42:5: warning: symbol 'verify_sha256_digest' was not declared. Should it be static?
> arch/x86/purgatory/purgatory.c:61:6: warning: symbol 'purgatory' was not declared. Should it be static?
>   CC      arch/x86/purgatory/purgatory.o
> 
> Numerous sparse messages regarding functions not being declared, these
> functions are resolved via kexec_purgatory_get_set_symbol() and not
> directly called anywhere.

Hi Nicholas,

So purgatory() is a separate piece of small binary which does not link
against kernel. And we don't want these symbols static as kernel
obtains the values of these symbols and modifies binary in place on
the fly. I am assuming if we make them static, then we will lose this
capability to be able to read elf headers and be able to modify value
of these symbols.

Now question is how to supress warnings from sparse. If just declaring
them extern in header file and including that header file in some other
.c file make the sparse warning go away, so be it. Atleast we need
to make explicit comment that this is being done just to take care
of sparse warning.

I am not very happy with the solution though. In future it will make
people scratch their head that why are we including this header file
and why some symbols are being declared extern. So if there is another
way to tell sparse to not worry about it, would be even better.


> To resolve the sparse issues appropriate
> declarations were added to asm/kexec-bzimage64.h and the appropriate
> reference included in purgatory.c. Adding this to kexec-bzimage64.h
> was done as setup_purgatory() from machine_kexec_file_64.c uses
> these symbols - not sure if this is the proper place to add this.
> 
> While at it the initialization of sha_regions to {{0,0}} was added.
> 

Is it really required. I thought all these global variable are will be
set to 0 without doing anything explicit.

Vivek

> Patch was compile tested with: x86_64_defconfig (implies CONFIG_KEXEC=y)
> 
> Patch is against 4.9.0 (localversion-next is next-20161223)
> 
>  arch/x86/include/asm/kexec-bzimage64.h | 16 ++++++++++++++++
>  arch/x86/purgatory/purgatory.c         |  8 ++------
>  2 files changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kexec-bzimage64.h b/arch/x86/include/asm/kexec-bzimage64.h
> index d1b5d19..fd7f776 100644
> --- a/arch/x86/include/asm/kexec-bzimage64.h
> +++ b/arch/x86/include/asm/kexec-bzimage64.h
> @@ -1,6 +1,21 @@
>  #ifndef _ASM_KEXEC_BZIMAGE64_H
>  #define _ASM_KEXEC_BZIMAGE64_H
>  
> +struct sha_region {
> +	unsigned long start;
> +	unsigned long len;
> +};
> +
>  extern struct kexec_file_ops kexec_bzImage64_ops;
>  
> +/* needed for kexec_purgatory_get_set_symbol() */
> +extern unsigned long backup_dest;
> +extern unsigned long backup_src;
> +extern unsigned long backup_sz;
> +extern u8 sha256_digest[];
> +extern struct sha_region sha_regions[];
> +
> +void purgatory(void);
> +int verify_sha256_digest(void);
> +
>  #endif  /* _ASM_KEXE_BZIMAGE64_H */
> diff --git a/arch/x86/purgatory/purgatory.c b/arch/x86/purgatory/purgatory.c
> index 25e068b..26c8367 100644
> --- a/arch/x86/purgatory/purgatory.c
> +++ b/arch/x86/purgatory/purgatory.c
> @@ -12,11 +12,7 @@
>  
>  #include "sha256.h"
>  #include "../boot/string.h"
> -
> -struct sha_region {
> -	unsigned long start;
> -	unsigned long len;
> -};
> +#include <asm/kexec-bzimage64.h>
>  
>  unsigned long backup_dest = 0;
>  unsigned long backup_src = 0;
> @@ -24,7 +20,7 @@ unsigned long backup_sz = 0;
>  
>  u8 sha256_digest[SHA256_DIGEST_SIZE] = { 0 };
>  
> -struct sha_region sha_regions[16] = {};
> +struct sha_region sha_regions[16] = { { 0, 0 } };
>  
>  /*
>   * On x86, second kernel requries first 640K of memory to boot. Copy
> -- 
> 2.1.4

Re: [PATCH RFC V2] purgatory: fix up declarations

[Nicholas Mc Guire]

On Tue, Jan 03, 2017 at 10:38:14AM -0500, Vivek Goyal wrote:
> On Fri, Dec 23, 2016 at 12:43:07PM +0100, Nicholas Mc Guire wrote:
> > Add the missing declarations of basic purgatory functions and variables
> > used with kexec_purgatory_get_set_symbol() to allow a clean build.
> > 
> > Fixes: commit 8fc5b4d4121c ("purgatory: core purgatory functionality")
> > Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> > ---
> > 
> > V2: after kbuild test robot <lkp@intel.com> reported a build failure
> >     removed incorrect declaration of copy_backup_region which is static
> >     anyway.
> > 
> > sparse complained about:
> >   CHECK   arch/x86/purgatory/purgatory.c
> > arch/x86/purgatory/purgatory.c:21:15: warning: symbol 'backup_dest' was not declared. Should it be static?
> > arch/x86/purgatory/purgatory.c:22:15: warning: symbol 'backup_src' was not declared. Should it be static?
> > arch/x86/purgatory/purgatory.c:23:15: warning: symbol 'backup_sz' was not declared. Should it be static?
> > arch/x86/purgatory/purgatory.c:25:4: warning: symbol 'sha256_digest' was not declared. Should it be static?
> > arch/x86/purgatory/purgatory.c:27:19: warning: symbol 'sha_regions' was not declared. Should it be static?
> > arch/x86/purgatory/purgatory.c:42:5: warning: symbol 'verify_sha256_digest' was not declared. Should it be static?
> > arch/x86/purgatory/purgatory.c:61:6: warning: symbol 'purgatory' was not declared. Should it be static?
> >   CC      arch/x86/purgatory/purgatory.o
> > 
> > Numerous sparse messages regarding functions not being declared, these
> > functions are resolved via kexec_purgatory_get_set_symbol() and not
> > directly called anywhere.
> 
> Hi Nicholas,
> 
> So purgatory() is a separate piece of small binary which does not link
> against kernel. And we don't want these symbols static as kernel
> obtains the values of these symbols and modifies binary in place on
> the fly. I am assuming if we make them static, then we will lose this
> capability to be able to read elf headers and be able to modify value
> of these symbols.

I don´t understand why this would be lost - the symbols are not being
used by kernel code other than kexec code it self -  in what way
would declaring them extern change there handling ? 
kexec_purgatory_find_symbol is using the elf header to resolve the 
symbol location and declaring it extern should not change that in any 
way - am I overlooking something ?

> 
> Now question is how to supress warnings from sparse. If just declaring
> them extern in header file and including that header file in some other
> .c file make the sparse warning go away, so be it. Atleast we need
> to make explicit comment that this is being done just to take care
> of sparse warning.
> 
> I am not very happy with the solution though. In future it will make
> people scratch their head that why are we including this header file
> and why some symbols are being declared extern. So if there is another
> way to tell sparse to not worry about it, would be even better.
>

The assumtion was that these changes would be side-effect free - if they are
not then this is probably the wrong path to go - the intent is to remove 
the sparse warnings only.
 
> 
> > To resolve the sparse issues appropriate
> > declarations were added to asm/kexec-bzimage64.h and the appropriate
> > reference included in purgatory.c. Adding this to kexec-bzimage64.h
> > was done as setup_purgatory() from machine_kexec_file_64.c uses
> > these symbols - not sure if this is the proper place to add this.
> > 
> > While at it the initialization of sha_regions to {{0,0}} was added.
> > 
> 
> Is it really required. I thought all these global variable are will be
> set to 0 without doing anything explicit.
>

No - technically it is not needed - but rather just a consistency
issue as 
    u8 sha256_digest[SHA256_DIGEST_SIZE] = { 0 };
is being initialized explicidly I just found it consostent to initialize
   struct sha_region sha_regions[16] = {};
explicidly as well - but that can be dropped if its usual practice.

thx!
hofrat
 
> Vivek
> 
> > Patch was compile tested with: x86_64_defconfig (implies CONFIG_KEXEC=y)
> > 
> > Patch is against 4.9.0 (localversion-next is next-20161223)
> > 
> >  arch/x86/include/asm/kexec-bzimage64.h | 16 ++++++++++++++++
> >  arch/x86/purgatory/purgatory.c         |  8 ++------
> >  2 files changed, 18 insertions(+), 6 deletions(-)
> > 
> > diff --git a/arch/x86/include/asm/kexec-bzimage64.h b/arch/x86/include/asm/kexec-bzimage64.h
> > index d1b5d19..fd7f776 100644
> > --- a/arch/x86/include/asm/kexec-bzimage64.h
> > +++ b/arch/x86/include/asm/kexec-bzimage64.h
> > @@ -1,6 +1,21 @@
> >  #ifndef _ASM_KEXEC_BZIMAGE64_H
> >  #define _ASM_KEXEC_BZIMAGE64_H
> >  
> > +struct sha_region {
> > +	unsigned long start;
> > +	unsigned long len;
> > +};
> > +
> >  extern struct kexec_file_ops kexec_bzImage64_ops;
> >  
> > +/* needed for kexec_purgatory_get_set_symbol() */
> > +extern unsigned long backup_dest;
> > +extern unsigned long backup_src;
> > +extern unsigned long backup_sz;
> > +extern u8 sha256_digest[];
> > +extern struct sha_region sha_regions[];
> > +
> > +void purgatory(void);
> > +int verify_sha256_digest(void);
> > +
> >  #endif  /* _ASM_KEXE_BZIMAGE64_H */
> > diff --git a/arch/x86/purgatory/purgatory.c b/arch/x86/purgatory/purgatory.c
> > index 25e068b..26c8367 100644
> > --- a/arch/x86/purgatory/purgatory.c
> > +++ b/arch/x86/purgatory/purgatory.c
> > @@ -12,11 +12,7 @@
> >  
> >  #include "sha256.h"
> >  #include "../boot/string.h"
> > -
> > -struct sha_region {
> > -	unsigned long start;
> > -	unsigned long len;
> > -};
> > +#include <asm/kexec-bzimage64.h>
> >  
> >  unsigned long backup_dest = 0;
> >  unsigned long backup_src = 0;
> > @@ -24,7 +20,7 @@ unsigned long backup_sz = 0;
> >  
> >  u8 sha256_digest[SHA256_DIGEST_SIZE] = { 0 };
> >  
> > -struct sha_region sha_regions[16] = {};
> > +struct sha_region sha_regions[16] = { { 0, 0 } };
> >  
> >  /*
> >   * On x86, second kernel requries first 640K of memory to boot. Copy
> > -- 
> > 2.1.4

Re: [PATCH RFC V2] purgatory: fix up declarations

[Dave Young]

Vivek, thanks for ccing me..

On 01/03/17 at 04:34pm, Nicholas Mc Guire wrote:
> On Tue, Jan 03, 2017 at 10:38:14AM -0500, Vivek Goyal wrote:
> > On Fri, Dec 23, 2016 at 12:43:07PM +0100, Nicholas Mc Guire wrote:
> > > Add the missing declarations of basic purgatory functions and variables
> > > used with kexec_purgatory_get_set_symbol() to allow a clean build.
> > > 
> > > Fixes: commit 8fc5b4d4121c ("purgatory: core purgatory functionality")
> > > Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> > > ---
> > > 
> > > V2: after kbuild test robot <lkp@intel.com> reported a build failure
> > >     removed incorrect declaration of copy_backup_region which is static
> > >     anyway.
> > > 
> > > sparse complained about:
> > >   CHECK   arch/x86/purgatory/purgatory.c
> > > arch/x86/purgatory/purgatory.c:21:15: warning: symbol 'backup_dest' was not declared. Should it be static?
> > > arch/x86/purgatory/purgatory.c:22:15: warning: symbol 'backup_src' was not declared. Should it be static?
> > > arch/x86/purgatory/purgatory.c:23:15: warning: symbol 'backup_sz' was not declared. Should it be static?
> > > arch/x86/purgatory/purgatory.c:25:4: warning: symbol 'sha256_digest' was not declared. Should it be static?
> > > arch/x86/purgatory/purgatory.c:27:19: warning: symbol 'sha_regions' was not declared. Should it be static?
> > > arch/x86/purgatory/purgatory.c:42:5: warning: symbol 'verify_sha256_digest' was not declared. Should it be static?
> > > arch/x86/purgatory/purgatory.c:61:6: warning: symbol 'purgatory' was not declared. Should it be static?
> > >   CC      arch/x86/purgatory/purgatory.o
> > > 
> > > Numerous sparse messages regarding functions not being declared, these
> > > functions are resolved via kexec_purgatory_get_set_symbol() and not
> > > directly called anywhere.
> > 
> > Hi Nicholas,
> > 
> > So purgatory() is a separate piece of small binary which does not link
> > against kernel. And we don't want these symbols static as kernel
> > obtains the values of these symbols and modifies binary in place on
> > the fly. I am assuming if we make them static, then we will lose this
> > capability to be able to read elf headers and be able to modify value
> > of these symbols.
> 
> I don´t understand why this would be lost - the symbols are not being
> used by kernel code other than kexec code it self -  in what way
> would declaring them extern change there handling ? 
> kexec_purgatory_find_symbol is using the elf header to resolve the 
> symbol location and declaring it extern should not change that in any 
> way - am I overlooking something ?
> 
> > 
> > Now question is how to supress warnings from sparse. If just declaring
> > them extern in header file and including that header file in some other
> > .c file make the sparse warning go away, so be it. Atleast we need
> > to make explicit comment that this is being done just to take care
> > of sparse warning.
> > 
> > I am not very happy with the solution though. In future it will make
> > people scratch their head that why are we including this header file
> > and why some symbols are being declared extern. So if there is another
> > way to tell sparse to not worry about it, would be even better.
> >
> 
> The assumtion was that these changes would be side-effect free - if they are
> not then this is probably the wrong path to go - the intent is to remove 
> the sparse warnings only.

Another way is do not include the header file, but declare them in the c
file just for avoiding the sparse warnings with some comments to explain
it.

>  
> > 
> > > To resolve the sparse issues appropriate
> > > declarations were added to asm/kexec-bzimage64.h and the appropriate
> > > reference included in purgatory.c. Adding this to kexec-bzimage64.h
> > > was done as setup_purgatory() from machine_kexec_file_64.c uses
> > > these symbols - not sure if this is the proper place to add this.
> > > 
> > > While at it the initialization of sha_regions to {{0,0}} was added.
> > > 
> > 
> > Is it really required. I thought all these global variable are will be
> > set to 0 without doing anything explicit.
> >
> 
> No - technically it is not needed - but rather just a consistency
> issue as 
>     u8 sha256_digest[SHA256_DIGEST_SIZE] = { 0 };
> is being initialized explicidly I just found it consostent to initialize
>    struct sha_region sha_regions[16] = {};
> explicidly as well - but that can be dropped if its usual practice.

It can be in another patch, removing the explicit zero initializing
looks better..

> 
> thx!
> hofrat
>  
> > Vivek
> > 
> > > Patch was compile tested with: x86_64_defconfig (implies CONFIG_KEXEC=y)
> > > 
> > > Patch is against 4.9.0 (localversion-next is next-20161223)
> > > 
> > >  arch/x86/include/asm/kexec-bzimage64.h | 16 ++++++++++++++++
> > >  arch/x86/purgatory/purgatory.c         |  8 ++------
> > >  2 files changed, 18 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/arch/x86/include/asm/kexec-bzimage64.h b/arch/x86/include/asm/kexec-bzimage64.h
> > > index d1b5d19..fd7f776 100644
> > > --- a/arch/x86/include/asm/kexec-bzimage64.h
> > > +++ b/arch/x86/include/asm/kexec-bzimage64.h
> > > @@ -1,6 +1,21 @@
> > >  #ifndef _ASM_KEXEC_BZIMAGE64_H
> > >  #define _ASM_KEXEC_BZIMAGE64_H
> > >  
> > > +struct sha_region {
> > > +	unsigned long start;
> > > +	unsigned long len;
> > > +};
> > > +
> > >  extern struct kexec_file_ops kexec_bzImage64_ops;
> > >  
> > > +/* needed for kexec_purgatory_get_set_symbol() */
> > > +extern unsigned long backup_dest;
> > > +extern unsigned long backup_src;
> > > +extern unsigned long backup_sz;
> > > +extern u8 sha256_digest[];
> > > +extern struct sha_region sha_regions[];
> > > +
> > > +void purgatory(void);
> > > +int verify_sha256_digest(void);
> > > +
> > >  #endif  /* _ASM_KEXE_BZIMAGE64_H */
> > > diff --git a/arch/x86/purgatory/purgatory.c b/arch/x86/purgatory/purgatory.c
> > > index 25e068b..26c8367 100644
> > > --- a/arch/x86/purgatory/purgatory.c
> > > +++ b/arch/x86/purgatory/purgatory.c
> > > @@ -12,11 +12,7 @@
> > >  
> > >  #include "sha256.h"
> > >  #include "../boot/string.h"
> > > -
> > > -struct sha_region {
> > > -	unsigned long start;
> > > -	unsigned long len;
> > > -};
> > > +#include <asm/kexec-bzimage64.h>
> > >  
> > >  unsigned long backup_dest = 0;
> > >  unsigned long backup_src = 0;
> > > @@ -24,7 +20,7 @@ unsigned long backup_sz = 0;
> > >  
> > >  u8 sha256_digest[SHA256_DIGEST_SIZE] = { 0 };
> > >  
> > > -struct sha_region sha_regions[16] = {};
> > > +struct sha_region sha_regions[16] = { { 0, 0 } };
> > >  
> > >  /*
> > >   * On x86, second kernel requries first 640K of memory to boot. Copy
> > > -- 
> > > 2.1.4

Thanks
Dave

Re: [PATCH RFC V2] purgatory: fix up declarations

[Nicholas Mc Guire]

On Wed, Jan 04, 2017 at 02:16:20PM +0800, Dave Young wrote:
> Vivek, thanks for ccing me..
> 
> On 01/03/17 at 04:34pm, Nicholas Mc Guire wrote:
> > On Tue, Jan 03, 2017 at 10:38:14AM -0500, Vivek Goyal wrote:
> > > On Fri, Dec 23, 2016 at 12:43:07PM +0100, Nicholas Mc Guire wrote:
> > > > Add the missing declarations of basic purgatory functions and variables
> > > > used with kexec_purgatory_get_set_symbol() to allow a clean build.
> > > > 
> > > > Fixes: commit 8fc5b4d4121c ("purgatory: core purgatory functionality")
> > > > Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> > > > ---
> > > > 
> > > > V2: after kbuild test robot <lkp@intel.com> reported a build failure
> > > >     removed incorrect declaration of copy_backup_region which is static
> > > >     anyway.
> > > > 
> > > > sparse complained about:
> > > >   CHECK   arch/x86/purgatory/purgatory.c
> > > > arch/x86/purgatory/purgatory.c:21:15: warning: symbol 'backup_dest' was not declared. Should it be static?
> > > > arch/x86/purgatory/purgatory.c:22:15: warning: symbol 'backup_src' was not declared. Should it be static?
> > > > arch/x86/purgatory/purgatory.c:23:15: warning: symbol 'backup_sz' was not declared. Should it be static?
> > > > arch/x86/purgatory/purgatory.c:25:4: warning: symbol 'sha256_digest' was not declared. Should it be static?
> > > > arch/x86/purgatory/purgatory.c:27:19: warning: symbol 'sha_regions' was not declared. Should it be static?
> > > > arch/x86/purgatory/purgatory.c:42:5: warning: symbol 'verify_sha256_digest' was not declared. Should it be static?
> > > > arch/x86/purgatory/purgatory.c:61:6: warning: symbol 'purgatory' was not declared. Should it be static?
> > > >   CC      arch/x86/purgatory/purgatory.o
> > > > 
> > > > Numerous sparse messages regarding functions not being declared, these
> > > > functions are resolved via kexec_purgatory_get_set_symbol() and not
> > > > directly called anywhere.
> > > 
> > > Hi Nicholas,
> > > 
> > > So purgatory() is a separate piece of small binary which does not link
> > > against kernel. And we don't want these symbols static as kernel
> > > obtains the values of these symbols and modifies binary in place on
> > > the fly. I am assuming if we make them static, then we will lose this
> > > capability to be able to read elf headers and be able to modify value
> > > of these symbols.
> > 
> > I don´t understand why this would be lost - the symbols are not being
> > used by kernel code other than kexec code it self -  in what way
> > would declaring them extern change there handling ? 
> > kexec_purgatory_find_symbol is using the elf header to resolve the 
> > symbol location and declaring it extern should not change that in any 
> > way - am I overlooking something ?
> > 
> > > 
> > > Now question is how to supress warnings from sparse. If just declaring
> > > them extern in header file and including that header file in some other
> > > .c file make the sparse warning go away, so be it. Atleast we need
> > > to make explicit comment that this is being done just to take care
> > > of sparse warning.
> > > 
> > > I am not very happy with the solution though. In future it will make
> > > people scratch their head that why are we including this header file
> > > and why some symbols are being declared extern. So if there is another
> > > way to tell sparse to not worry about it, would be even better.
> > >
> > 
> > The assumtion was that these changes would be side-effect free - if they are
> > not then this is probably the wrong path to go - the intent is to remove 
> > the sparse warnings only.
> 
> Another way is do not include the header file, but declare them in the c
> file just for avoiding the sparse warnings with some comments to explain
> it.
>

ok - will go back and try that route - sounds a lot less invasive.
 
> >  
> > > 
> > > > To resolve the sparse issues appropriate
> > > > declarations were added to asm/kexec-bzimage64.h and the appropriate
> > > > reference included in purgatory.c. Adding this to kexec-bzimage64.h
> > > > was done as setup_purgatory() from machine_kexec_file_64.c uses
> > > > these symbols - not sure if this is the proper place to add this.
> > > > 
> > > > While at it the initialization of sha_regions to {{0,0}} was added.
> > > > 
> > > 
> > > Is it really required. I thought all these global variable are will be
> > > set to 0 without doing anything explicit.
> > >
> > 
> > No - technically it is not needed - but rather just a consistency
> > issue as 
> >     u8 sha256_digest[SHA256_DIGEST_SIZE] = { 0 };
> > is being initialized explicidly I just found it consostent to initialize
> >    struct sha_region sha_regions[16] = {};
> > explicidly as well - but that can be dropped if its usual practice.
> 
> It can be in another patch, removing the explicit zero initializing
> looks better..
>
fine - that also achieves consistency - will put that into a separate patch
then and repost.

thanks for the review notes.

thx!
hofrat
 
> > > 
> > > > Patch was compile tested with: x86_64_defconfig (implies CONFIG_KEXEC=y)
> > > > 
> > > > Patch is against 4.9.0 (localversion-next is next-20161223)
> > > > 
> > > >  arch/x86/include/asm/kexec-bzimage64.h | 16 ++++++++++++++++
> > > >  arch/x86/purgatory/purgatory.c         |  8 ++------
> > > >  2 files changed, 18 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/arch/x86/include/asm/kexec-bzimage64.h b/arch/x86/include/asm/kexec-bzimage64.h
> > > > index d1b5d19..fd7f776 100644
> > > > --- a/arch/x86/include/asm/kexec-bzimage64.h
> > > > +++ b/arch/x86/include/asm/kexec-bzimage64.h
> > > > @@ -1,6 +1,21 @@
> > > >  #ifndef _ASM_KEXEC_BZIMAGE64_H
> > > >  #define _ASM_KEXEC_BZIMAGE64_H
> > > >  
> > > > +struct sha_region {
> > > > +	unsigned long start;
> > > > +	unsigned long len;
> > > > +};
> > > > +
> > > >  extern struct kexec_file_ops kexec_bzImage64_ops;
> > > >  
> > > > +/* needed for kexec_purgatory_get_set_symbol() */
> > > > +extern unsigned long backup_dest;
> > > > +extern unsigned long backup_src;
> > > > +extern unsigned long backup_sz;
> > > > +extern u8 sha256_digest[];
> > > > +extern struct sha_region sha_regions[];
> > > > +
> > > > +void purgatory(void);
> > > > +int verify_sha256_digest(void);
> > > > +
> > > >  #endif  /* _ASM_KEXE_BZIMAGE64_H */
> > > > diff --git a/arch/x86/purgatory/purgatory.c b/arch/x86/purgatory/purgatory.c
> > > > index 25e068b..26c8367 100644
> > > > --- a/arch/x86/purgatory/purgatory.c
> > > > +++ b/arch/x86/purgatory/purgatory.c
> > > > @@ -12,11 +12,7 @@
> > > >  
> > > >  #include "sha256.h"
> > > >  #include "../boot/string.h"
> > > > -
> > > > -struct sha_region {
> > > > -	unsigned long start;
> > > > -	unsigned long len;
> > > > -};
> > > > +#include <asm/kexec-bzimage64.h>
> > > >  
> > > >  unsigned long backup_dest = 0;
> > > >  unsigned long backup_src = 0;
> > > > @@ -24,7 +20,7 @@ unsigned long backup_sz = 0;
> > > >  
> > > >  u8 sha256_digest[SHA256_DIGEST_SIZE] = { 0 };
> > > >  
> > > > -struct sha_region sha_regions[16] = {};
> > > > +struct sha_region sha_regions[16] = { { 0, 0 } };
> > > >  
> > > >  /*
> > > >   * On x86, second kernel requries first 640K of memory to boot. Copy
> > > > -- 
> > > > 2.1.4
> 
> Thanks
> Dave

Re: [PATCH RFC V2] purgatory: fix up declarations

[Nicholas Mc Guire]

On Wed, Jan 04, 2017 at 02:16:20PM +0800, Dave Young wrote:
> Vivek, thanks for ccing me..
> 
> On 01/03/17 at 04:34pm, Nicholas Mc Guire wrote:
> > On Tue, Jan 03, 2017 at 10:38:14AM -0500, Vivek Goyal wrote:
> > > On Fri, Dec 23, 2016 at 12:43:07PM +0100, Nicholas Mc Guire wrote:
> > > > Add the missing declarations of basic purgatory functions and variables
> > > > used with kexec_purgatory_get_set_symbol() to allow a clean build.
> > > > 
> > > > Fixes: commit 8fc5b4d4121c ("purgatory: core purgatory functionality")
> > > > Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> > > > ---
> > > > 
> > > > V2: after kbuild test robot <lkp@intel.com> reported a build failure
> > > >     removed incorrect declaration of copy_backup_region which is static
> > > >     anyway.
> > > > 
> > > > sparse complained about:
> > > >   CHECK   arch/x86/purgatory/purgatory.c
> > > > arch/x86/purgatory/purgatory.c:21:15: warning: symbol 'backup_dest' was not declared. Should it be static?
> > > > arch/x86/purgatory/purgatory.c:22:15: warning: symbol 'backup_src' was not declared. Should it be static?
> > > > arch/x86/purgatory/purgatory.c:23:15: warning: symbol 'backup_sz' was not declared. Should it be static?
> > > > arch/x86/purgatory/purgatory.c:25:4: warning: symbol 'sha256_digest' was not declared. Should it be static?
> > > > arch/x86/purgatory/purgatory.c:27:19: warning: symbol 'sha_regions' was not declared. Should it be static?
> > > > arch/x86/purgatory/purgatory.c:42:5: warning: symbol 'verify_sha256_digest' was not declared. Should it be static?
> > > > arch/x86/purgatory/purgatory.c:61:6: warning: symbol 'purgatory' was not declared. Should it be static?
> > > >   CC      arch/x86/purgatory/purgatory.o
> > > > 
> > > > Numerous sparse messages regarding functions not being declared, these
> > > > functions are resolved via kexec_purgatory_get_set_symbol() and not
> > > > directly called anywhere.
> > > 
> > > Hi Nicholas,
> > > 
> > > So purgatory() is a separate piece of small binary which does not link
> > > against kernel. And we don't want these symbols static as kernel
> > > obtains the values of these symbols and modifies binary in place on
> > > the fly. I am assuming if we make them static, then we will lose this
> > > capability to be able to read elf headers and be able to modify value
> > > of these symbols.
> > 
> > I don´t understand why this would be lost - the symbols are not being
> > used by kernel code other than kexec code it self -  in what way
> > would declaring them extern change there handling ? 
> > kexec_purgatory_find_symbol is using the elf header to resolve the 
> > symbol location and declaring it extern should not change that in any 
> > way - am I overlooking something ?
> > 
> > > 
> > > Now question is how to supress warnings from sparse. If just declaring
> > > them extern in header file and including that header file in some other
> > > .c file make the sparse warning go away, so be it. Atleast we need
> > > to make explicit comment that this is being done just to take care
> > > of sparse warning.
> > > 
> > > I am not very happy with the solution though. In future it will make
> > > people scratch their head that why are we including this header file
> > > and why some symbols are being declared extern. So if there is another
> > > way to tell sparse to not worry about it, would be even better.
> > >
> > 
> > The assumtion was that these changes would be side-effect free - if they are
> > not then this is probably the wrong path to go - the intent is to remove 
> > the sparse warnings only.
> 
> Another way is do not include the header file, but declare them in the c
> file just for avoiding the sparse warnings with some comments to explain
> it.
>
that would make sparse happy as you suggest but now checkpatch is fussing.

...
WARNING: externs should be avoided in .c files
#62: FILE: arch/x86/purgatory/purgatory.c:27:
+extern unsigned long backup_src;

WARNING: externs should be avoided in .c files
#63: FILE: arch/x86/purgatory/purgatory.c:28:
+extern unsigned long backup_sz;
...

unfortunately it seems that both of these tools do not permit marking 
something as a false positive for this case (__force in sparse will
not work here). At the same time I do think that would not be a very
clean solution ither.

So the alternative solution is to create arch/x86/purgatory.h and put it
all into there - V3 containting that solution just sent out. 

thx!
hofrat