Message-ID: <20170104061620.GA19466@dhcp-128-65.nay.redhat.com>
Date:   Wed, 4 Jan 2017 14:16:20 +0800
From:   Dave Young <dyoung@...hat.com>
To:     Nicholas Mc Guire <der.herr@...r.at>
Cc:     Vivek Goyal <vgoyal@...hat.com>,
        Nicholas Mc Guire <hofrat@...dl.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>, x86@...nel.org,
        linux-kernel@...r.kernel.org, Baoquan He <bhe@...hat.com>
Subject: Re: [PATCH RFC V2] purgatory: fix up declarations

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@...dl.org>
> > > ---
> > > 
> > > V2: after kbuild test robot <lkp@...el.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

Powered by blists - more mailing lists