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 <hof...@osadl.org> > > --- > > > > V2: after kbuild test robot <l...@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