[U-Boot] [PATCH 3/4] x86: Force the lib_sysinfo structure to be in the .data section

[Graeme Russ]

Hi Gabe,

On 03/12/11 08:16, Gabe Black wrote:
> 
> 
> On Fri, Dec 2, 2011 at 1:10 PM, Graeme Russ <graeme.russ at gmail.com
> <mailto:graeme.russ at gmail.com>> wrote:
> 
>     Hi Gabe,
> 
>     On 30/11/11 17:07, Gabe Black wrote:
>     > Otherwise it ends up in the .bss section. U-boot assumes that it doesn't
>     > need to copy it over during relocation, and instead fills that whole
>     > section with zeroes. If we really were booting from ROM that would be
>     > appropriate, but we need some information from the coreboot tables
>     (memory
>     > size) before then and have to fill that structure before relocation. We
>     > skirt u-boot's assumption by putting this in .data where it assumes there
>     > is still read only but non-zero data.
>     >
>     > Signed-off-by: Gabe Black <gabeblack at chromium.org
>     <mailto:gabeblack at chromium.org>>
>     > ---
>     >  arch/x86/cpu/coreboot/sysinfo.c |    8 +++++++-
>     >  1 files changed, 7 insertions(+), 1 deletions(-)
>     >
>     > diff --git a/arch/x86/cpu/coreboot/sysinfo.c
>     b/arch/x86/cpu/coreboot/sysinfo.c
>     > index 464f8a1..e74fe0a 100644
>     > --- a/arch/x86/cpu/coreboot/sysinfo.c
>     > +++ b/arch/x86/cpu/coreboot/sysinfo.c
>     > @@ -30,4 +30,10 @@
>     >
>     >  #include <asm/ic/coreboot/sysinfo.h>
>     >
>     > -struct sysinfo_t lib_sysinfo;
>     > +/*
>     > + * This needs to be in the .data section so that it's copied over during
>     > + * relocation. By default it's put in the .bss section which is
>     simply filled
>     > + * with zeroes when transitioning from "ROM", which is really RAM,
>     to other
>     > + * RAM.
>     > + */
>     > +struct sysinfo_t lib_sysinfo __attribute__((section(".data")));
> 
>     I think this can be logically folded into the first patch
> 
>     Regards,
> 
>     Graeme
> 
> 
> I would rather not do that since the first patch is, modulo checkpatch and
> build fixes, just importing that code from coreboot's libpayload. This
> change is making an important, non-obvious change which makes it work
> properly within u-boot. These are two different things, and one way or the
> other this one is important enough to be singled out as its own change and
> not lost in the midst of the other.

I agree that is a logical position to take, however I think that the fact
that you introduce a commit which is known to be 'broken' is the overriding
factor - Include to fact that the modification is made in the commit
message and the comment like:

/*
 * Note: U-Boot needs this to be in the .data section so....
 * ...
 */

Also, can you supply any specific commit id(s) for the originating source -
We typically try to include as much canonical information to identify the
source to a particular commit

Regards,

Graeme