Patchwork [U-Boot,13/57] x86: Move new_gd_addr to arch_global_data

login
register
mail settings
Submitter Simon Glass
Date Nov. 16, 2012, 9:19 p.m.
Message ID <1353100842-20126-14-git-send-email-sjg@chromium.org>
Download mbox | patch
Permalink /patch/199741/
State Superseded, archived
Delegated to: Tom Rini
Headers show

Comments

Simon Glass - Nov. 16, 2012, 9:19 p.m.
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(-)
Graeme Russ - Nov. 18, 2012, 1:07 a.m.
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
Simon Glass - Dec. 14, 2012, 6:34 a.m.
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

Patch

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;