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

[Qemu-devel] [PATCH] 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.

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

diff --git a/hw/pc.c b/hw/pc.c
index f9cfd1f..cef68eb 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -763,9 +763,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,6 +856,18 @@ 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);
     }
+    /* map the last 128KB of the BIOS in ISA space */
+    isa_bios_size = bios_size;
+    if (isa_bios_size > (128 * 1024))
+        isa_bios_size = 128 * 1024;
+    cpu_register_physical_memory(0x100000 - isa_bios_size,
+                                 isa_bios_size,
+                                 (bios_offset + bios_size - isa_bios_size) | IO_MEM_ROM);
+
+
+
+    option_rom_offset = qemu_ram_alloc(0x20000);
+    oprom_area_size = 0;
 
     if (using_vga) {
         /* VGA BIOS load */
@@ -864,76 +876,53 @@ static void pc_init1(ram_addr_t ram_size, int vga_ram_size,
         } 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)
+        oprom_area_size = get_image_size(buf);
+        if (oprom_area_size <= 0 || oprom_area_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) {
+        ret = load_image(buf, phys_ram_base + option_rom_offset);
+        if (ret != oprom_area_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))
-        isa_bios_size = 128 * 1024;
-    cpu_register_physical_memory(0x100000 - isa_bios_size,
-                                 isa_bios_size,
-                                 (bios_offset + bios_size - isa_bios_size) | IO_MEM_ROM);
+    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;
+    }
 
-    {
-        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++) {
+        int size;
+        /* Round up optiom rom start addr to the next 2k boundary */
+        oprom_area_size = (oprom_area_size + 2047) & ~2047;
+        size = get_image_size(option_rom[i]);
+        if (size < 0) {
+            fprintf(stderr, "Could not load option rom '%s'\n",
+                    option_rom[i]);
+            exit(1);
         }
-
-        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;
+        if (size > (0x20000  - oprom_area_size))
+            goto option_rom_error;
+        ret = load_image(option_rom[i], phys_ram_base + option_rom_offset + oprom_area_size);
+        if (ret != size) {
+        option_rom_error:
+            fprintf(stderr, "Could not fit %soption roms in available space\n", using_vga ? "VGA bios and " : "");
+            exit(1);
         }
+        oprom_area_size += size;
     }
 
+    cpu_register_physical_memory(0xc0000, 0x20000, option_rom_offset | IO_MEM_ROM);
+
     /* 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] Re: [PATCH] register a single area for vga bios and option roms

[Anthony Liguori]

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.
>
> It also has the nice side effect of improving the loading code
> a little bit.
>
> Signed-off-by: Glauber Costa <glommer@redhat.com>
>   

The vga bios is just an option rom, right?  So why can't we treat the 
vga bios as an option rom and eliminate the specific code for dealing 
with VGA bios.

The real problem here is that we should be registering the option rom 
space once and doing that in page boundaries.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH] 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.

As we're rewriting this code, can we make it use load_image_targphys at the 
same time please.

Paul

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

[Carl-Daniel Hailfinger]

On 09.04.2009 19:48, Anthony Liguori wrote:
> 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.
>>
>> It also has the nice side effect of improving the loading code
>> a little bit.
>>
>> Signed-off-by: Glauber Costa <glommer@redhat.com>
>>   
>
> The vga bios is just an option rom, right?  So why can't we treat the
> vga bios as an option rom and eliminate the specific code for dealing
> with VGA bios.
>
> The real problem here is that we should be registering the option rom
> space once and doing that in page boundaries.

AFAIK shadowed option ROMs can be aligned at 1 kB boundaries and may
modify themselves. Quite a few VGA BIOSes do that to reduce their memory
footprint after hardware initialization is complete.


Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/

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

[Glauber Costa]

>
> AFAIK shadowed option ROMs can be aligned at 1 kB boundaries and may
> modify themselves. Quite a few VGA BIOSes do that to reduce their memory
> footprint after hardware initialization is complete.
>
Are you suggesting we drop the IO_MEM_ROM flag from the registration function?
KVM already does that for its own purposes, I would not oppose such a
change in any way.



-- 
Glauber  Costa.
"Free as in Freedom"
http://glommer.net

"The less confident you are, the more serious you have to act."

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

[Zachary Amsden]

Anthony Liguori wrote:
> 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.
>>
>> It also has the nice side effect of improving the loading code
>> a little bit.
>>
>> Signed-off-by: Glauber Costa <glommer@redhat.com>
>>   
> 
> The vga bios is just an option rom, right?  So why can't we treat the
> vga bios as an option rom and eliminate the specific code for dealing
> with VGA bios.
> 
> The real problem here is that we should be registering the option rom
> space once and doing that in page boundaries.

Um... are we sure about this?  First off, option roms can be written to,
it's part of the PnP option ROM spec.. they are to be backed in RAM and
the init code will actually write to the ROM to resize it (allowing it
to jettison startup / init code); they get remapped read-only again
later.  Will registering the roms as one physical memory region prevent
this possibility?  Sorry I am still naive and coming up to speed on qemu
if this is a naive question.

Second thing, option roms are not necessarily on page boundaries,
technically they are mapped in chunks, which are 512 byte blocks, at the
discretion of the BIOS / chipset.  This is mostly a non-issue as all
modern BIOS will map them on page boundaries but worth noting.

Zach