[Qemu-devel] [PATCH v2 0/4] more rom loader patches.

[Qemu-devel] [PATCH v2 0/4] more rom loader patches.

[Gerd Hoffmann]

  Hi,

A collection of rom loader bits, check individual patches for details.

New in v2: split loader-target.c into two files so we can compile it
once as suggested by blueswirl.

cheers,
  Gerd

[Qemu-devel] [PATCH 1/4] rom loader: use qemu_strdup.

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/loader.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/loader.c b/hw/loader.c
index 7aa1a67..6baafa8 100644
--- a/hw/loader.c
+++ b/hw/loader.c
@@ -559,7 +559,7 @@ int rom_add_file(const char *file,
     rom->name = qemu_strdup(file);
     rom->path = qemu_find_file(QEMU_FILE_TYPE_BIOS, rom->name);
     if (rom->path == NULL) {
-        rom->path = strdup(file);
+        rom->path = qemu_strdup(file);
     }
 
     fd = open(rom->path, O_RDONLY | O_BINARY);
-- 
1.6.2.5

[Qemu-devel] [PATCH 2/4] rom loader: make vga+rom loading target specific

[Gerd Hoffmann]

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
+
 obj-y += virtio.o
 obj-y += fw_cfg.o
 obj-y += watchdog.o
diff --git a/hw/loader-dummy.c b/hw/loader-dummy.c
new file mode 100644
index 0000000..31e12ce
--- /dev/null
+++ b/hw/loader-dummy.c
@@ -0,0 +1,18 @@
+/*
+ * target specific rom code -- dummy
+ */
+
+#include "hw.h"
+#include "loader.h"
+
+int rom_add_vga(const char *file)
+{
+    /* nothing */
+    return 0;
+}
+
+int rom_add_option(const char *file)
+{
+    /* nothing */
+    return 0;
+}
diff --git a/hw/loader-i386.c b/hw/loader-i386.c
new file mode 100644
index 0000000..06eaf4d
--- /dev/null
+++ b/hw/loader-i386.c
@@ -0,0 +1,16 @@
+/*
+ * target specific rom code -- i386
+ */
+
+#include "hw.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..d013253 100644
--- a/hw/loader.h
+++ b/hw/loader.h
@@ -38,9 +38,7 @@ 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)
+int rom_add_vga(const char *file);
+int rom_add_option(const char *file);
 
 #endif
-- 
1.6.2.5

[Qemu-devel] [PATCH 3/4] vga roms: move loading from pc.c to vga drivers.

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/cirrus_vga.c |    6 +++++-
 hw/pc.c         |   12 ------------
 hw/vga-isa.c    |    3 +++
 hw/vga-pci.c    |    4 ++++
 hw/vga_int.h    |    2 ++
 5 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
index 9dfe76a..f4c9cdf 100644
--- a/hw/cirrus_vga.c
+++ b/hw/cirrus_vga.c
@@ -32,6 +32,7 @@
 #include "console.h"
 #include "vga_int.h"
 #include "kvm.h"
+#include "loader.h"
 
 /*
  * TODO:
@@ -3162,6 +3163,7 @@ void isa_cirrus_vga_init(void)
                                      s->vga.screen_dump, s->vga.text_update,
                                      &s->vga);
     vmstate_register(0, &vmstate_cirrus_vga, s);
+    rom_add_vga(VGABIOS_CIRRUS_FILENAME);
     /* XXX ISA-LFB support */
 }
 
@@ -3245,7 +3247,9 @@ static int pci_cirrus_vga_initfn(PCIDevice *dev)
                           PCI_ADDRESS_SPACE_MEM, cirrus_pci_mmio_map);
      }
      vmstate_register(0, &vmstate_pci_cirrus_vga, d);
-     /* XXX: ROM BIOS */
+
+     /* ROM BIOS */
+     rom_add_vga(VGABIOS_CIRRUS_FILENAME);
      return 0;
 }
 
diff --git a/hw/pc.c b/hw/pc.c
index 408d6d6..7fcc4b0 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -47,8 +47,6 @@
 //#define DEBUG_MULTIBOOT
 
 #define BIOS_FILENAME "bios.bin"
-#define VGABIOS_FILENAME "vgabios.bin"
-#define VGABIOS_CIRRUS_FILENAME "vgabios-cirrus.bin"
 
 #define PC_MAX_BIOS_SIZE (4 * 1024 * 1024)
 
@@ -1050,7 +1048,6 @@ static void pc_init1(ram_addr_t ram_size,
     IsaIrqState *isa_irq_state;
     DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
     DriveInfo *fd[MAX_FD];
-    int using_vga = cirrus_vga_enabled || std_vga_enabled || vmsvga_enabled;
     void *fw_cfg;
 
     if (ram_size >= 0xe0000000 ) {
@@ -1140,15 +1137,6 @@ static void pc_init1(ram_addr_t ram_size,
     option_rom_offset = qemu_ram_alloc(PC_ROM_SIZE);
     cpu_register_physical_memory(PC_ROM_MIN_VGA, PC_ROM_SIZE, option_rom_offset);
 
-    if (using_vga) {
-        /* VGA BIOS load */
-        if (cirrus_vga_enabled) {
-            rom_add_vga(VGABIOS_CIRRUS_FILENAME);
-        } else {
-            rom_add_vga(VGABIOS_FILENAME);
-        }
-    }
-
     /* map all the bios at the top of memory */
     cpu_register_physical_memory((uint32_t)(-bios_size),
                                  bios_size, bios_offset | IO_MEM_ROM);
diff --git a/hw/vga-isa.c b/hw/vga-isa.c
index 7fa31d3..6b387af 100644
--- a/hw/vga-isa.c
+++ b/hw/vga-isa.c
@@ -27,6 +27,7 @@
 #include "vga_int.h"
 #include "pixel_ops.h"
 #include "qemu-timer.h"
+#include "loader.h"
 
 int isa_vga_init(void)
 {
@@ -46,5 +47,7 @@ int isa_vga_init(void)
     cpu_register_physical_memory(VBE_DISPI_LFB_PHYSICAL_ADDRESS,
                                  VGA_RAM_SIZE, s->vram_offset);
 #endif
+    /* ROM BIOS */
+    rom_add_vga(VGABIOS_FILENAME);
     return 0;
 }
diff --git a/hw/vga-pci.c b/hw/vga-pci.c
index 5e75938..b5fd666 100644
--- a/hw/vga-pci.c
+++ b/hw/vga-pci.c
@@ -28,6 +28,7 @@
 #include "vga_int.h"
 #include "pixel_ops.h"
 #include "qemu-timer.h"
+#include "loader.h"
 
 typedef struct PCIVGAState {
     PCIDevice dev;
@@ -117,6 +118,9 @@ static int pci_vga_initfn(PCIDevice *dev)
         pci_register_bar(&d->dev, PCI_ROM_SLOT, bios_total_size,
                          PCI_ADDRESS_SPACE_MEM_PREFETCH, vga_map);
      }
+
+     /* ROM BIOS */
+     rom_add_vga(VGABIOS_FILENAME);
      return 0;
 }
 
diff --git a/hw/vga_int.h b/hw/vga_int.h
index c162c07..b843e52 100644
--- a/hw/vga_int.h
+++ b/hw/vga_int.h
@@ -220,6 +220,8 @@ extern const uint8_t sr_mask[8];
 extern const uint8_t gr_mask[16];
 
 #define VGA_RAM_SIZE (8192 * 1024)
+#define VGABIOS_FILENAME "vgabios.bin"
+#define VGABIOS_CIRRUS_FILENAME "vgabios-cirrus.bin"
 
 extern CPUReadMemoryFunc * const vga_mem_read[3];
 extern CPUWriteMemoryFunc * const vga_mem_write[3];
-- 
1.6.2.5

[Qemu-devel] [PATCH 4/4] use rom loader for pc bios.

[Gerd Hoffmann]

The pc bios shows up in 'info roms' now.

Note that the BIOS is mapped to two places: The complete rom at the top
of the memory, and the first 128k at 0xe0000.  Only the first place is
listed in 'info roms'.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/pc.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index 7fcc4b0..9a9d196 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -1115,8 +1115,8 @@ static void pc_init1(ram_addr_t ram_size,
         goto bios_error;
     }
     bios_offset = qemu_ram_alloc(bios_size);
-    ret = load_image(filename, qemu_get_ram_ptr(bios_offset));
-    if (ret != bios_size) {
+    ret = rom_add_file_fixed(bios_name, (uint32_t)(-bios_size));
+    if (ret != 0) {
     bios_error:
         fprintf(stderr, "qemu: could not load PC BIOS '%s'\n", bios_name);
         exit(1);
-- 
1.6.2.5

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

[Juan Quintela]

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

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
-- 
1.6.2.5

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

[Gerd Hoffmann]

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