[Qemu-devel] [PATCH 0/2] Refactor option rom loading code

[Qemu-devel] [PATCH 0/2] Refactor option rom loading code

[Glauber Costa]

Following suggestions from paul. The second patch is an easter bonus,
and make load_linux do not rely on phys_ram_base assumptions.

[Qemu-devel] [PATCH 1/2] register a single area for vga bios and option roms

[Glauber Costa]

Those guys are not different in nature. They're all roms,
not blessed with the graces of being written to. So there's
not need to issue multiple requests to memory registration areas:
just treat them as brothers, and put them all in the same
region.

It also has the nice side effect of improving the loading code
a little bit. Besides some of the ugliness going away, we're now
avoiding phys_ram_base dependencies in option rom code.

load_linux is yet to be handled.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 hw/pc.c |  110 ++++++++++++++++++++++++--------------------------------------
 1 files changed, 43 insertions(+), 67 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index f9cfd1f..166d378 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -754,6 +754,20 @@ static void pc_init_ne2k_isa(NICInfo *nd, qemu_irq *pic)
     nb_ne2k++;
 }
 
+static int load_option_rom(const char *oprom, target_phys_addr_t start, target_phys_addr_t end)
+{
+        int size;
+
+        /* Round up optiom rom start addr to the next 2k boundary */
+        start = (start + 2047) & ~2047;
+        size = load_image_targphys(oprom, start, end - start);
+        if (size < 0) {
+            fprintf(stderr, "Could not load option rom '%s'\n", oprom);
+            exit(1);
+        }
+        return size;
+}
+
 /* PC hardware initialisation */
 static void pc_init1(ram_addr_t ram_size, int vga_ram_size,
                      const char *boot_device,
@@ -763,9 +777,9 @@ static void pc_init1(ram_addr_t ram_size, int vga_ram_size,
 {
     char buf[1024];
     int ret, linux_boot, i;
-    ram_addr_t ram_addr, vga_ram_addr, bios_offset, vga_bios_offset, option_rom_start = 0;
+    ram_addr_t ram_addr, vga_ram_addr, bios_offset, option_rom_offset;
     ram_addr_t below_4g_mem_size, above_4g_mem_size = 0;
-    int bios_size, isa_bios_size, vga_bios_size;
+    int bios_size, isa_bios_size, oprom_area_size;
     PCIBus *pci_bus;
     int piix3_devfn = -1;
     CPUState *env;
@@ -856,40 +870,6 @@ static void pc_init1(ram_addr_t ram_size, int vga_ram_size,
         fprintf(stderr, "qemu: could not load PC BIOS '%s'\n", buf);
         exit(1);
     }
-
-    if (using_vga) {
-        /* VGA BIOS load */
-        if (cirrus_vga_enabled) {
-            snprintf(buf, sizeof(buf), "%s/%s", bios_dir, VGABIOS_CIRRUS_FILENAME);
-        } else {
-            snprintf(buf, sizeof(buf), "%s/%s", bios_dir, VGABIOS_FILENAME);
-        }
-        vga_bios_size = get_image_size(buf);
-        if (vga_bios_size <= 0 || vga_bios_size > 65536)
-            goto vga_bios_error;
-        vga_bios_offset = qemu_ram_alloc(65536);
-
-        ret = load_image(buf, phys_ram_base + vga_bios_offset);
-        if (ret != vga_bios_size) {
-vga_bios_error:
-            fprintf(stderr, "qemu: could not load VGA BIOS '%s'\n", buf);
-            exit(1);
-        }
-	/* Round up vga bios size to the next 2k boundary */
-	vga_bios_size = (vga_bios_size + 2047) & ~2047;
-	option_rom_start = 0xc0000 + vga_bios_size;
-
-        /* setup basic memory access */
-        cpu_register_physical_memory(0xc0000, vga_bios_size,
-                                     vga_bios_offset | IO_MEM_ROM);
-    }
-
-    /* No point in placing option roms before this address, since bochs bios
-     * will only start looking for it at 0xc8000 */
-    if (option_rom_start < 0xc8000)
-	    option_rom_start = 0xc8000;
-
-
     /* map the last 128KB of the BIOS in ISA space */
     isa_bios_size = bios_size;
     if (isa_bios_size > (128 * 1024))
@@ -898,42 +878,38 @@ vga_bios_error:
                                  isa_bios_size,
                                  (bios_offset + bios_size - isa_bios_size) | IO_MEM_ROM);
 
-    {
-        ram_addr_t option_rom_offset;
-        int size, offset;
-
-        offset = option_rom_start;
-        if (linux_boot) {
-            option_rom_offset = qemu_ram_alloc(TARGET_PAGE_SIZE);
-            load_linux(phys_ram_base + option_rom_offset,
-                       kernel_filename, initrd_filename, kernel_cmdline);
-            cpu_register_physical_memory(option_rom_start, TARGET_PAGE_SIZE,
-                                         option_rom_offset | IO_MEM_ROM);
-            offset += TARGET_PAGE_SIZE;
-        }
 
-        for (i = 0; i < nb_option_roms; i++) {
-            size = get_image_size(option_rom[i]);
-            if (size < 0) {
-                fprintf(stderr, "Could not load option rom '%s'\n",
-                        option_rom[i]);
-                exit(1);
-            }
-            if (size > (0xe0000  - offset))
-                goto option_rom_error;
-            option_rom_offset = qemu_ram_alloc(size);
-            ret = load_image(option_rom[i], phys_ram_base + option_rom_offset);
-            if (ret != size) {
-            option_rom_error:
-                fprintf(stderr, "Could not fit %soption roms in available space\n", using_vga ? "VGA bios and " : "");
-                exit(1);
-            }
-            size = (size + 4095) & ~4095;
-            cpu_register_physical_memory(offset, size, option_rom_offset | IO_MEM_ROM);
-            offset += size;
+
+    option_rom_offset = qemu_ram_alloc(0x20000);
+    oprom_area_size = 0;
+    cpu_register_physical_memory(0xc0000, 0x20000, option_rom_offset | IO_MEM_ROM);
+
+    if (using_vga) {
+        /* VGA BIOS load */
+        if (cirrus_vga_enabled) {
+            snprintf(buf, sizeof(buf), "%s/%s", bios_dir, VGABIOS_CIRRUS_FILENAME);
+        } else {
+            snprintf(buf, sizeof(buf), "%s/%s", bios_dir, VGABIOS_FILENAME);
         }
+        oprom_area_size = load_option_rom(buf, 0xc0000, 0xe0000);
+    }
+    /* Although video roms can grow larger than 0x8000, the area between
+     * 0xc0000 - 0xc8000 is reserved for them. It means we won't be looking
+     * for any other kind of option rom inside this area */
+    if (oprom_area_size < 0x8000)
+        oprom_area_size = 0x8000;
+
+    if (linux_boot) {
+        oprom_area_size = (oprom_area_size + 2047) & ~2047;
+        load_linux(phys_ram_base + option_rom_offset + oprom_area_size,
+                   kernel_filename, initrd_filename, kernel_cmdline);
+        oprom_area_size += TARGET_PAGE_SIZE;
     }
 
+    for (i = 0; i < nb_option_roms; i++)
+        oprom_area_size = load_option_rom(option_rom[i], 0xc0000 + oprom_area_size, 0xe0000);
+
+
     /* map all the bios at the top of memory */
     cpu_register_physical_memory((uint32_t)(-bios_size),
                                  bios_size, bios_offset | IO_MEM_ROM);
-- 
1.5.6.6

[Qemu-devel] [PATCH 2/2] get rid of phys_ram_base dependency for load_linux

[Glauber Costa]

use cpu_physical_memory_write_rom instead of memcpy

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 hw/pc.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index 166d378..4858c5d 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -446,7 +446,7 @@ static void bochs_bios_init(void)
 
 /* Generate an initial boot sector which sets state and jump to
    a specified vector */
-static void generate_bootsect(uint8_t *option_rom,
+static void generate_bootsect(target_phys_addr_t option_rom,
                               uint32_t gpr[8], uint16_t segs[6], uint16_t ip)
 {
     uint8_t rom[512], *p, *reloc;
@@ -520,7 +520,7 @@ static void generate_bootsect(uint8_t *option_rom,
         sum += rom[i];
     rom[sizeof(rom) - 1] = -sum;
 
-    memcpy(option_rom, rom, sizeof(rom));
+    cpu_physical_memory_write_rom(option_rom, rom, sizeof(rom));
 }
 
 static long get_file_size(FILE *f)
@@ -537,7 +537,7 @@ static long get_file_size(FILE *f)
     return size;
 }
 
-static void load_linux(uint8_t *option_rom,
+static void load_linux(target_phys_addr_t option_rom,
                        const char *kernel_filename,
 		       const char *initrd_filename,
 		       const char *kernel_cmdline)
@@ -901,7 +901,7 @@ static void pc_init1(ram_addr_t ram_size, int vga_ram_size,
 
     if (linux_boot) {
         oprom_area_size = (oprom_area_size + 2047) & ~2047;
-        load_linux(phys_ram_base + option_rom_offset + oprom_area_size,
+        load_linux(0xc0000 + oprom_area_size,
                    kernel_filename, initrd_filename, kernel_cmdline);
         oprom_area_size += TARGET_PAGE_SIZE;
     }
-- 
1.5.6.6

Re: [Qemu-devel] [PATCH 1/2] register a single area for vga bios and option roms

[Paul Brook]

On Thursday 09 April 2009, Glauber Costa wrote:
> Those guys are not different in nature. They're all roms,
> not blessed with the graces of being written to. So there's
> not need to issue multiple requests to memory registration areas:
> just treat them as brothers, and put them all in the same
> region.

I fixed a bug (missing += when loading option roms), simplified the rounding a 
bit, added a check for overflow, fixed several formatting errors, and applied 
the patch.

Paul

Re: [Qemu-devel] [PATCH 2/2] get rid of phys_ram_base dependency for load_linux

[Paul Brook]

On Thursday 09 April 2009, Glauber Costa wrote:
> use cpu_physical_memory_write_rom instead of memcpy

Applied, thanks.

Paul