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

[Vivek Goyal <vgoyal@redhat.com>]

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