diff mbox

Re: [PATCH 2/4] rom loader: make vga+rom loading target specific

Message ID m3pr8mz85d.fsf@neno.mitica
State New
Headers show

Commit Message

Juan Quintela Oct. 17, 2009, 9:23 a.m. UTC 
Gerd Hoffmann <kraxel@redhat.com> wrote:
> This patch adds a loader-target.c file for target-specific
> rom loading functions.  The rom_add_vga() and rom_add_option()
> macros are transformed into functions and sticked in there.  They
> load the bios on TARGET_I386 and no nothing on other targets.
>
> With this in place we can move the rom loading calls from pc.c to the
> individual drivers.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  Makefile.hw       |    7 +++++++
>  hw/loader-dummy.c |   18 ++++++++++++++++++
>  hw/loader-i386.c  |   16 ++++++++++++++++
>  hw/loader.h       |    6 ++----
>  4 files changed, 43 insertions(+), 4 deletions(-)
>  create mode 100644 hw/loader-dummy.c
>  create mode 100644 hw/loader-i386.c
>
> diff --git a/Makefile.hw b/Makefile.hw
> index f4b4469..7da6775 100644
> --- a/Makefile.hw
> +++ b/Makefile.hw
> @@ -12,7 +12,14 @@ VPATH=$(SRC_PATH):$(SRC_PATH)/hw
>  QEMU_CFLAGS+=-I.. -I$(SRC_PATH)/fpu
>  
>  obj-y =
> +
>  obj-y += loader.o
> +ifdef TARGET_I386
> +obj-y += loader-i386.o
> +else
> +obj-y += loader-dummy.o
> +endif
> +

This is wrong (tm).
a- TARGET_I386 on Makefiles is only defined for 32bits, not for 64bits
   (don't blame me, I just did a direct conversion of what was there).
b- TARGET_* is only defined in {i386,x86_64,...}-{softmmu,linux-user,...}
   i.e. not a chance to be read at hw/Makefile.

</me does patch>

More complex that I thought:
- if you only want to compile it once, you need to compile it on
  Makefile, not Makefile.hw: no cookie, loader.h uses target_phys_addr.
- ok, move it to Makefile.hw -> your option didn't work (it compiles)
  because you allways compile loader-dummy.o, TARGET_I386 is not defined
  in Makefile.hw (it is hardware independent drivers after all :)
- More imagination: Use only loader-i386.c, and #ifdef TARGET_I386
  to an empty macro/real thing -> no cookie either, TARGET_I386 is
  poisoned in Makefile.hw (*)
- define CONFIG_LOADER_I386 for I386 and X86_64.

And here is the patch.  I tested that it compiles for several
combinations of x86_64 and not x86_64, and that it works as expected.


<alternative>
This makes the file be compiled only once.
If you can live with file being compiled twice, it is easier to:

add to Makefile.target
obj-i386-y += loader-i386.o

s/CONFIG_LOADER_I386/TARGET_I386/ in loader.h (on my patch)

And then you don't have to define CONFIG_LOADER_I386.
You don't need to include "config-all-devices.h" if you decided this
option. (*)

It makes things easier to understand, I think.

<alternative/>

I preffer very much to add the "dummy" cases in one ifdef in a header
file.

Later, Juan.

(*): This is cool. Haven't noticed that feature yet.
(**): Yes, config-all-devices.h needs a better way to be included. I
      didn't noticed it because by definition nothing old depended on symbols
      defined there.  All symbols that are there were introduced to
      compile some devices that were compiled unconditionally before.
      More thought here is needed.

From 4ea2193db8da38064787a4079b6d67536220e9f8 Mon Sep 17 00:00:00 2001
From: Juan Quintela <quintela@redhat.com>
Date: Sat, 17 Oct 2009 10:43:58 +0200
Subject: [PATCH] Fix loader-i386.c compilation


Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 Makefile.hw                        |    1 +
 default-configs/i386-softmmu.mak   |    2 ++
 default-configs/x86_64-softmmu.mak |    2 ++
 hw/loader-i386.c                   |   17 +++++++++++++++++
 hw/loader.h                        |   11 +++++++----
 5 files changed, 29 insertions(+), 4 deletions(-)
 create mode 100644 hw/loader-i386.c

Comments

Gerd Hoffmann Oct. 19, 2009, 7:58 a.m. UTC | #1 
On 10/17/09 11:23, Juan Quintela wrote:
>> +ifdef TARGET_I386
>> +obj-y += loader-i386.o
>> +else
>> +obj-y += loader-dummy.o
>> +endif
>> +
>
> This is wrong (tm).
> a- TARGET_I386 on Makefiles is only defined for 32bits, not for 64bits
>     (don't blame me, I just did a direct conversion of what was there).

Oh.  Joy of inconsistency ...

> b- TARGET_* is only defined in {i386,x86_64,...}-{softmmu,linux-user,...}
>     i.e. not a chance to be read at hw/Makefile.
>
> </me does patch>
>
> More complex that I thought:
> - if you only want to compile it once, you need to compile it on
>    Makefile, not Makefile.hw: no cookie, loader.h uses target_phys_addr.

Exactly.  Due to target_phys_addr it must go to Makefile.hw, so I get it 
compiled once for 32bit targphys and once for 64bit targphys.

> - ok, move it to Makefile.hw ->  your option didn't work (it compiles)
>    because you allways compile loader-dummy.o, TARGET_I386 is not defined
>    in Makefile.hw (it is hardware independent drivers after all :)

Oops.

> - More imagination: Use only loader-i386.c, and #ifdef TARGET_I386
>    to an empty macro/real thing ->  no cookie either, TARGET_I386 is
>    poisoned in Makefile.hw (*)

Thats why I don't want #ifdef.

> - define CONFIG_LOADER_I386 for I386 and X86_64.

No.  This avoids the TARGET_I386 poisoned error, but does *not* fix the 
underlying problem.

> I preffer very much to add the "dummy" cases in one ifdef in a header
> file.

No.  Any user of rom_add_option() must be compiled per target then.

Guess easierst way is the v1 patch then:  Have target-specific code in 
loader-target.c and use #ifdefs there ...

cheers,
   Gerd
diff mbox

Patch

diff --git a/Makefile.hw b/Makefile.hw
index f4b4469..b9e6646 100644
--- a/Makefile.hw
+++ b/Makefile.hw
@@ -36,6 +36,7 @@  obj-$(CONFIG_ESP) += esp.o

 obj-y += dma-helpers.o sysbus.o isa-bus.o
 obj-$(CONFIG_QDEV_ADDR) += qdev-addr.o
+obj-$(CONFIG_LOADER_I386) += loader-i386.o

 all: $(HWLIB)
 # Dummy command so that make thinks it has done something
diff --git a/default-configs/i386-softmmu.mak b/default-configs/i386-softmmu.mak
index 15586a0..887dab8 100644
--- a/default-configs/i386-softmmu.mak
+++ b/default-configs/i386-softmmu.mak
@@ -1 +1,3 @@ 
 # Default configuration for i386-softmmu
+
+CONFIG_LOADER_I386=y
diff --git a/default-configs/x86_64-softmmu.mak b/default-configs/x86_64-softmmu.mak
index ec98af2..d287861 100644
--- a/default-configs/x86_64-softmmu.mak
+++ b/default-configs/x86_64-softmmu.mak
@@ -1 +1,3 @@ 
 # Default configuration for x86_64-softmmu
+
+CONFIG_LOADER_I386=y
diff --git a/hw/loader-i386.c b/hw/loader-i386.c
new file mode 100644
index 0000000..320d79d
--- /dev/null
+++ b/hw/loader-i386.c
@@ -0,0 +1,17 @@ 
+/*
+ * target specific rom code -- i386
+ */
+
+#include "hw.h"
+#include "config-all-devices.h"
+#include "loader.h"
+
+int rom_add_vga(const char *file)
+{
+    return rom_add_file(file, PC_ROM_MIN_VGA, PC_ROM_MAX, PC_ROM_ALIGN);
+}
+
+int rom_add_option(const char *file)
+{
+    return rom_add_file(file, PC_ROM_MIN_OPTION, PC_ROM_MAX, PC_ROM_ALIGN);
+}
diff --git a/hw/loader.h b/hw/loader.h
index 945c662..62a6ac7 100644
--- a/hw/loader.h
+++ b/hw/loader.h
@@ -38,9 +38,12 @@  void do_info_roms(Monitor *mon);
 #define PC_ROM_ALIGN       0x800
 #define PC_ROM_SIZE        (PC_ROM_MAX - PC_ROM_MIN_VGA)

-#define rom_add_vga(_f)                                                 \
-    rom_add_file(_f, PC_ROM_MIN_VGA,    PC_ROM_MAX, PC_ROM_ALIGN)
-#define rom_add_option(_f)                                              \
-    rom_add_file(_f, PC_ROM_MIN_OPTION, PC_ROM_MAX, PC_ROM_ALIGN)
+#ifdef CONFIG_LOADER_I386
+int rom_add_vga(const char *file);
+int rom_add_option(const char *file);
+#else
+#define rom_add_vga(_file) 0
+#define rom_add_option(_file) 0
+#endif

 #endif