| Message ID | 1353100842-20126-14-git-send-email-sjg@chromium.org |
|---|---|
| State | Superseded, archived |
| Delegated to: | Tom Rini |
| Headers | show |
Hi Simon, On 11/17/2012 08:19 AM, Simon Glass wrote: > Move this field into arch_global_data and tidy up. > > This will certainly break x86, so will need Graeme's help to sort out Yes, it most certainly will break x86 :) > the problem. I would prefer not to put the architecture-specific stuff > at the top of global_data since we relying on that seems even more ugly. The fix is not that hard though... The whole point of putting gdt_addr at the top of the global data structure is to guarantee that is is the very fist void * in gd. The trick is how we use the 'F' segment. By loading the fs register with the physical address of gd, virtual address 0 of fs contains the address of gd. But really, we can put the address of gd anywhere, as long as we set fs to be that address > > Signed-off-by: Simon Glass <sjg@chromium.org> > --- > arch/x86/cpu/cpu.c | 2 +- > arch/x86/include/asm/global_data.h | 12 +++++++++--- > arch/x86/lib/init_helpers.c | 2 +- > 3 files changed, 11 insertions(+), 5 deletions(-) > > diff --git a/arch/x86/cpu/cpu.c b/arch/x86/cpu/cpu.c > index e9bb0d7..c276aa6 100644 > --- a/arch/x86/cpu/cpu.c > +++ b/arch/x86/cpu/cpu.c > @@ -92,7 +92,7 @@ static void load_gdt(const u64 *boot_gdt, u16 num_entries) > > void init_gd(gd_t *id, u64 *gdt_addr) > { > - id->gd_addr = (ulong)id; > + id->arch.gd_addr = (ulong)id; > setup_gdt(id, gdt_addr); If the original code had been: setup_gdt(&(id->gd_addr), gdt_addr); There would have been no reliance on gd_addr being the first member of gd. So change this to: setup_gdt(&(id->arch.gd_addr), gdt_addr); And you should be pretty much set - and gd_addr can be anywhere in the arch gd. > } > > diff --git a/arch/x86/include/asm/global_data.h b/arch/x86/include/asm/global_data.h > index 3df83bb..d2eb00a 100644 > --- a/arch/x86/include/asm/global_data.h > +++ b/arch/x86/include/asm/global_data.h > @@ -28,8 +28,16 @@ > > /* Architecture-specific global data */ > struct arch_global_data { > - unsigned long gdt_addr; /* Location of GDT */ > + /* > + * NOTE: gd_addr MUST be first member of struct global_data! > + * > + * But it now isn't, so this is sure to break x86. Can we change > + * x86 to not require this? I don't think we should put the > + * arch data first in global_data... > + */ Yes we can - see above > unsigned long new_gd_addr; /* New location of Global Data */ > + unsigned long gd_addr; /* Location of Global Data */ > + unsigned long gdt_addr; /* Location of GDT */ > }; > > /* > @@ -41,8 +49,6 @@ struct arch_global_data { > */ > > typedef struct global_data { > - /* NOTE: gd_addr MUST be first member of struct global_data! */ > - unsigned long gd_addr; /* Location of Global Data */ > bd_t *bd; > unsigned long flags; > unsigned int baudrate; > diff --git a/arch/x86/lib/init_helpers.c b/arch/x86/lib/init_helpers.c > index 05cadcd..ac789c2 100644 > --- a/arch/x86/lib/init_helpers.c > +++ b/arch/x86/lib/init_helpers.c > @@ -126,7 +126,7 @@ int copy_gd_to_ram_f_r(void) > * in-RAM copy of Global Data (calculate_relocation_address() > * has already calculated the in-RAM location of the GDT) > */ > - ram_gd->gd_addr = (ulong)ram_gd; > + ram_gd->arch.gd_addr = (ulong)ram_gd; > init_gd(ram_gd, (u64 *)gd->arch.gdt_addr); > > return 0; > Regards, Graeme
Hi Graeme, On Sat, Nov 17, 2012 at 5:07 PM, Graeme Russ <graeme.russ@gmail.com> wrote: > Hi Simon, > > On 11/17/2012 08:19 AM, Simon Glass wrote: >> Move this field into arch_global_data and tidy up. >> >> This will certainly break x86, so will need Graeme's help to sort out > > Yes, it most certainly will break x86 :) > >> the problem. I would prefer not to put the architecture-specific stuff >> at the top of global_data since we relying on that seems even more ugly. > > The fix is not that hard though... > > The whole point of putting gdt_addr at the top of the global data structure > is to guarantee that is is the very fist void * in gd. The trick is how we > use the 'F' segment. By loading the fs register with the physical address > of gd, virtual address 0 of fs contains the address of gd. > > But really, we can put the address of gd anywhere, as long as we set fs to > be that address > >> >> Signed-off-by: Simon Glass <sjg@chromium.org> >> --- >> arch/x86/cpu/cpu.c | 2 +- >> arch/x86/include/asm/global_data.h | 12 +++++++++--- >> arch/x86/lib/init_helpers.c | 2 +- >> 3 files changed, 11 insertions(+), 5 deletions(-) >> >> diff --git a/arch/x86/cpu/cpu.c b/arch/x86/cpu/cpu.c >> index e9bb0d7..c276aa6 100644 >> --- a/arch/x86/cpu/cpu.c >> +++ b/arch/x86/cpu/cpu.c >> @@ -92,7 +92,7 @@ static void load_gdt(const u64 *boot_gdt, u16 num_entries) >> >> void init_gd(gd_t *id, u64 *gdt_addr) >> { >> - id->gd_addr = (ulong)id; >> + id->arch.gd_addr = (ulong)id; >> setup_gdt(id, gdt_addr); > > If the original code had been: > > setup_gdt(&(id->gd_addr), gdt_addr); > > There would have been no reliance on gd_addr being the first member of gd. > So change this to: > > setup_gdt(&(id->arch.gd_addr), gdt_addr); > > And you should be pretty much set - and gd_addr can be anywhere in the arch gd. Thanks for that. I actually understand it now, for better or worse. It seems to work fine. Actually your clean-up of the global_data really has paid dividends as things are so much nicer now. I'm going to resend the whole series rebased to master. Regards, Simon > >> } >> >> diff --git a/arch/x86/include/asm/global_data.h b/arch/x86/include/asm/global_data.h >> index 3df83bb..d2eb00a 100644 >> --- a/arch/x86/include/asm/global_data.h >> +++ b/arch/x86/include/asm/global_data.h >> @@ -28,8 +28,16 @@ >> >> /* Architecture-specific global data */ >> struct arch_global_data { >> - unsigned long gdt_addr; /* Location of GDT */ >> + /* >> + * NOTE: gd_addr MUST be first member of struct global_data! >> + * >> + * But it now isn't, so this is sure to break x86. Can we change >> + * x86 to not require this? I don't think we should put the >> + * arch data first in global_data... >> + */ > > Yes we can - see above > >> unsigned long new_gd_addr; /* New location of Global Data */ >> + unsigned long gd_addr; /* Location of Global Data */ >> + unsigned long gdt_addr; /* Location of GDT */ >> }; >> >> /* >> @@ -41,8 +49,6 @@ struct arch_global_data { >> */ >> >> typedef struct global_data { >> - /* NOTE: gd_addr MUST be first member of struct global_data! */ >> - unsigned long gd_addr; /* Location of Global Data */ >> bd_t *bd; >> unsigned long flags; >> unsigned int baudrate; >> diff --git a/arch/x86/lib/init_helpers.c b/arch/x86/lib/init_helpers.c >> index 05cadcd..ac789c2 100644 >> --- a/arch/x86/lib/init_helpers.c >> +++ b/arch/x86/lib/init_helpers.c >> @@ -126,7 +126,7 @@ int copy_gd_to_ram_f_r(void) >> * in-RAM copy of Global Data (calculate_relocation_address() >> * has already calculated the in-RAM location of the GDT) >> */ >> - ram_gd->gd_addr = (ulong)ram_gd; >> + ram_gd->arch.gd_addr = (ulong)ram_gd; >> init_gd(ram_gd, (u64 *)gd->arch.gdt_addr); >> >> return 0; >> > > Regards, > > Graeme
diff --git a/arch/x86/cpu/cpu.c b/arch/x86/cpu/cpu.c index e9bb0d7..c276aa6 100644 --- a/arch/x86/cpu/cpu.c +++ b/arch/x86/cpu/cpu.c @@ -92,7 +92,7 @@ static void load_gdt(const u64 *boot_gdt, u16 num_entries) void init_gd(gd_t *id, u64 *gdt_addr) { - id->gd_addr = (ulong)id; + id->arch.gd_addr = (ulong)id; setup_gdt(id, gdt_addr); } diff --git a/arch/x86/include/asm/global_data.h b/arch/x86/include/asm/global_data.h index 3df83bb..d2eb00a 100644 --- a/arch/x86/include/asm/global_data.h +++ b/arch/x86/include/asm/global_data.h @@ -28,8 +28,16 @@ /* Architecture-specific global data */ struct arch_global_data { - unsigned long gdt_addr; /* Location of GDT */ + /* + * NOTE: gd_addr MUST be first member of struct global_data! + * + * But it now isn't, so this is sure to break x86. Can we change + * x86 to not require this? I don't think we should put the + * arch data first in global_data... + */ unsigned long new_gd_addr; /* New location of Global Data */ + unsigned long gd_addr; /* Location of Global Data */ + unsigned long gdt_addr; /* Location of GDT */ }; /* @@ -41,8 +49,6 @@ struct arch_global_data { */ typedef struct global_data { - /* NOTE: gd_addr MUST be first member of struct global_data! */ - unsigned long gd_addr; /* Location of Global Data */ bd_t *bd; unsigned long flags; unsigned int baudrate; diff --git a/arch/x86/lib/init_helpers.c b/arch/x86/lib/init_helpers.c index 05cadcd..ac789c2 100644 --- a/arch/x86/lib/init_helpers.c +++ b/arch/x86/lib/init_helpers.c @@ -126,7 +126,7 @@ int copy_gd_to_ram_f_r(void) * in-RAM copy of Global Data (calculate_relocation_address() * has already calculated the in-RAM location of the GDT) */ - ram_gd->gd_addr = (ulong)ram_gd; + ram_gd->arch.gd_addr = (ulong)ram_gd; init_gd(ram_gd, (u64 *)gd->arch.gdt_addr); return 0;
Move this field into arch_global_data and tidy up. This will certainly break x86, so will need Graeme's help to sort out the problem. I would prefer not to put the architecture-specific stuff at the top of global_data since we relying on that seems even more ugly. Signed-off-by: Simon Glass <sjg@chromium.org> --- arch/x86/cpu/cpu.c | 2 +- arch/x86/include/asm/global_data.h | 12 +++++++++--- arch/x86/lib/init_helpers.c | 2 +- 3 files changed, 11 insertions(+), 5 deletions(-)