diff mbox series

x86: Drop duplicate declaration of emulator state

Message ID 20200905191524.1603618-1-sjg@chromium.org
State New
Delegated to: Bin Meng
Headers show
Series x86: Drop duplicate declaration of emulator state | expand

Commit Message

Simon Glass Sept. 5, 2020, 7:15 p.m. UTC
With x86 we can execute an option ROM either natively or using the x86
emulator (if enabled with CONFIG_BIOSEMU). Both of these share the
_X86EMU_env variable, with the native code using it to hold register state
during interrupt processing.

At present, in 32-bit U-Boot, the variable is declared twice, once in
common code and once in code only compiled with CONFIG_BIOSEMU.

With gcc-10 this causes a 'multiple definitions' error on boards with
CONFIG_BIOSEMU.

Drop the emulator definition, except for 64-bit builds.

Also drop inclusion of the emulator in 64-bit U-Boot since this does not
work at present, and generally isn't needed if 32-bit code has already set
up the option ROMs.

Reported-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
Signed-off-by: Simon Glass <sjg@chromium.org>
---

 drivers/bios_emulator/x86emu/sys.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Wolfgang Wallner Sept. 14, 2020, 8:11 a.m. UTC | #1
Hi Simon,

-----"U-Boot" <u-boot-bounces@lists.denx.de> schrieb: -----
> Betreff: [PATCH] x86: Drop duplicate declaration of emulator state
> 
> With x86 we can execute an option ROM either natively or using the x86
> emulator (if enabled with CONFIG_BIOSEMU). Both of these share the
> _X86EMU_env variable, with the native code using it to hold register state
> during interrupt processing.
> 
> At present, in 32-bit U-Boot, the variable is declared twice, once in
> common code and once in code only compiled with CONFIG_BIOSEMU.
> 
> With gcc-10 this causes a 'multiple definitions' error on boards with
> CONFIG_BIOSEMU.
> 
> Drop the emulator definition, except for 64-bit builds.
> 
> Also drop inclusion of the emulator in 64-bit U-Boot since this does not
> work at present, and generally isn't needed if 32-bit code has already set
> up the option ROMs.
> 
> Reported-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
>  drivers/bios_emulator/x86emu/sys.c | 4 ++++
>  1 file changed, 4 insertions(+)

Tested-by: Wolfgang Wallner <wolfgang.wallner@br-automation.com>
Tested by building chromebook_coral_defconfig on an Arch Linux with GCC 10.1.0

regards, Wolfgang
Bin Meng Sept. 21, 2020, 1:58 a.m. UTC | #2
Hi Simon,

On Sun, Sep 6, 2020 at 3:15 AM Simon Glass <sjg@chromium.org> wrote:
>
> With x86 we can execute an option ROM either natively or using the x86
> emulator (if enabled with CONFIG_BIOSEMU). Both of these share the
> _X86EMU_env variable, with the native code using it to hold register state
> during interrupt processing.
>
> At present, in 32-bit U-Boot, the variable is declared twice, once in
> common code and once in code only compiled with CONFIG_BIOSEMU.
>
> With gcc-10 this causes a 'multiple definitions' error on boards with
> CONFIG_BIOSEMU.
>
> Drop the emulator definition, except for 64-bit builds.
>
> Also drop inclusion of the emulator in 64-bit U-Boot since this does not
> work at present, and generally isn't needed if 32-bit code has already set
> up the option ROMs.
>
> Reported-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  drivers/bios_emulator/x86emu/sys.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/bios_emulator/x86emu/sys.c b/drivers/bios_emulator/x86emu/sys.c
> index c2db1213fe6..146586b3ceb 100644
> --- a/drivers/bios_emulator/x86emu/sys.c
> +++ b/drivers/bios_emulator/x86emu/sys.c
> @@ -44,7 +44,11 @@
>
>  /*------------------------- Global Variables ------------------------------*/
>
> +/* Definite this here since the emulator is not present on 64-bit */
> +#ifdef CONFIG_X86_64

I believe the correct fix is to drop the one in arch/x86/lib/bios.c

>  X86EMU_sysEnv _X86EMU_env;     /* Global emulator machine state */
> +#endif
> +
>  X86EMU_intrFuncs _X86EMU_intrTab[256];
>
>  int debug_intr;
> --

Regards,
Bin
Heinrich Schuchardt Sept. 21, 2020, 5:55 a.m. UTC | #3
Am 21. September 2020 03:58:31 MESZ schrieb Bin Meng <bmeng.cn@gmail.com>:
>Hi Simon,
>
>On Sun, Sep 6, 2020 at 3:15 AM Simon Glass <sjg@chromium.org> wrote:
>>
>> With x86 we can execute an option ROM either natively or using the
>x86
>> emulator (if enabled with CONFIG_BIOSEMU). Both of these share the
>> _X86EMU_env variable, with the native code using it to hold register
>state
>> during interrupt processing.
>>
>> At present, in 32-bit U-Boot, the variable is declared twice, once in
>> common code and once in code only compiled with CONFIG_BIOSEMU.
>>
>> With gcc-10 this causes a 'multiple definitions' error on boards with
>> CONFIG_BIOSEMU.
>>
>> Drop the emulator definition, except for 64-bit builds.
>>
>> Also drop inclusion of the emulator in 64-bit U-Boot since this does
>not
>> work at present, and generally isn't needed if 32-bit code has
>already set
>> up the option ROMs.
>>
>> Reported-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> ---
>>
>>  drivers/bios_emulator/x86emu/sys.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/bios_emulator/x86emu/sys.c
>b/drivers/bios_emulator/x86emu/sys.c
>> index c2db1213fe6..146586b3ceb 100644
>> --- a/drivers/bios_emulator/x86emu/sys.c
>> +++ b/drivers/bios_emulator/x86emu/sys.c
>> @@ -44,7 +44,11 @@
>>
>>  /*------------------------- Global Variables
>------------------------------*/
>>
>> +/* Definite this here since the emulator is not present on 64-bit */

%s/Definite/Define/

>> +#ifdef CONFIG_X86_64
>
>I believe the correct fix is to drop the one in arch/x86/lib/bios.c
>
>>  X86EMU_sysEnv _X86EMU_env;     /* Global emulator machine state */
>> +#endif
>> +
>>  X86EMU_intrFuncs _X86EMU_intrTab[256];
>>
>>  int debug_intr;
>> --
>
>Regards,
>Bin
Simon Glass Sept. 22, 2020, 1:51 p.m. UTC | #4
Hi Bin,

On Sun, 20 Sep 2020 at 19:58, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Simon,
>
> On Sun, Sep 6, 2020 at 3:15 AM Simon Glass <sjg@chromium.org> wrote:
> >
> > With x86 we can execute an option ROM either natively or using the x86
> > emulator (if enabled with CONFIG_BIOSEMU). Both of these share the
> > _X86EMU_env variable, with the native code using it to hold register state
> > during interrupt processing.
> >
> > At present, in 32-bit U-Boot, the variable is declared twice, once in
> > common code and once in code only compiled with CONFIG_BIOSEMU.
> >
> > With gcc-10 this causes a 'multiple definitions' error on boards with
> > CONFIG_BIOSEMU.
> >
> > Drop the emulator definition, except for 64-bit builds.
> >
> > Also drop inclusion of the emulator in 64-bit U-Boot since this does not
> > work at present, and generally isn't needed if 32-bit code has already set
> > up the option ROMs.
> >
> > Reported-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> >  drivers/bios_emulator/x86emu/sys.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/bios_emulator/x86emu/sys.c b/drivers/bios_emulator/x86emu/sys.c
> > index c2db1213fe6..146586b3ceb 100644
> > --- a/drivers/bios_emulator/x86emu/sys.c
> > +++ b/drivers/bios_emulator/x86emu/sys.c
> > @@ -44,7 +44,11 @@
> >
> >  /*------------------------- Global Variables ------------------------------*/
> >
> > +/* Definite this here since the emulator is not present on 64-bit */
> > +#ifdef CONFIG_X86_64
>
> I believe the correct fix is to drop the one in arch/x86/lib/bios.c
>
> >  X86EMU_sysEnv _X86EMU_env;     /* Global emulator machine state */
> > +#endif
> > +
> >  X86EMU_intrFuncs _X86EMU_intrTab[256];
> >
> >  int debug_intr;

This currently causes a build error though, which is why I went with
the other approach.

Regards,
Simon
diff mbox series

Patch

diff --git a/drivers/bios_emulator/x86emu/sys.c b/drivers/bios_emulator/x86emu/sys.c
index c2db1213fe6..146586b3ceb 100644
--- a/drivers/bios_emulator/x86emu/sys.c
+++ b/drivers/bios_emulator/x86emu/sys.c
@@ -44,7 +44,11 @@ 
 
 /*------------------------- Global Variables ------------------------------*/
 
+/* Definite this here since the emulator is not present on 64-bit */
+#ifdef CONFIG_X86_64
 X86EMU_sysEnv _X86EMU_env;	/* Global emulator machine state */
+#endif
+
 X86EMU_intrFuncs _X86EMU_intrTab[256];
 
 int debug_intr;