Re: [Qemu-devel] [PATCH v9 3/3] pc: Support system flash memory with pflash

[Stefan Hajnoczi]

On Mon, Dec 19, 2011 at 01:41:03PM -0600, Anthony Liguori wrote:
> On 12/15/2011 02:51 PM, Jordan Justen wrote:
> >If a pflash image is found, then it is used for the system
> >firmware image.
> >
> >If a pflash image is not initially found, then a read-only
> >pflash device is created using the -bios filename.
> >
> >KVM cannot execute from a pflash region currently.
> >Therefore, when KVM is enabled, a (read-only) ram memory
> >region is created and filled with the contents of the
> >pflash drive.
> >
> >Signed-off-by: Jordan Justen<address@hidden>
> >Cc: Anthony Liguori<address@hidden>
> >---
> >  Makefile.target                    |    1 +
> >  default-configs/i386-softmmu.mak   |    1 +
> >  default-configs/x86_64-softmmu.mak |    1 +
> >  hw/boards.h                        |    1 +
> >  hw/pc.c                            |   55 +-------
> >  hw/pc.h                            |    4 +
> >  hw/pc_sysfw.c                      |  255 
> > ++++++++++++++++++++++++++++++++++++
> >  vl.c                               |    2 +-
> >  8 files changed, 269 insertions(+), 51 deletions(-)
> >  create mode 100644 hw/pc_sysfw.c
> >
> >diff --git a/Makefile.target b/Makefile.target
> >index a111521..b1dc882 100644
> >--- a/Makefile.target
> >+++ b/Makefile.target
> >@@ -236,6 +236,7 @@ obj-i386-y += vmport.o
> >  obj-i386-y += device-hotplug.o pci-hotplug.o smbios.o wdt_ib700.o
> >  obj-i386-y += debugcon.o multiboot.o
> >  obj-i386-y += pc_piix.o
> >+obj-i386-y += pc_sysfw.o
> >  obj-i386-$(CONFIG_KVM) += kvmclock.o
> >  obj-i386-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o
> >
> >diff --git a/default-configs/i386-softmmu.mak 
> >b/default-configs/i386-softmmu.mak
> >index e67ebb3..cd407a9 100644
> >--- a/default-configs/i386-softmmu.mak
> >+++ b/default-configs/i386-softmmu.mak
> >@@ -22,3 +22,4 @@ CONFIG_SOUND=y
> >  CONFIG_HPET=y
> >  CONFIG_APPLESMC=y
> >  CONFIG_I8259=y
> >+CONFIG_PFLASH_CFI01=y
> >diff --git a/default-configs/x86_64-softmmu.mak 
> >b/default-configs/x86_64-softmmu.mak
> >index b75757e..47734ea 100644
> >--- a/default-configs/x86_64-softmmu.mak
> >+++ b/default-configs/x86_64-softmmu.mak
> >@@ -22,3 +22,4 @@ CONFIG_SOUND=y
> >  CONFIG_HPET=y
> >  CONFIG_APPLESMC=y
> >  CONFIG_I8259=y
> >+CONFIG_PFLASH_CFI01=y
> >diff --git a/hw/boards.h b/hw/boards.h
> >index 716fd7b..45a31a1 100644
> >--- a/hw/boards.h
> >+++ b/hw/boards.h
> >@@ -33,6 +33,7 @@ typedef struct QEMUMachine {
> >  } QEMUMachine;
> >
> >  int qemu_register_machine(QEMUMachine *m);
> >+QEMUMachine *find_default_machine(void);
> >
> >  extern QEMUMachine *current_machine;
> >
> >diff --git a/hw/pc.c b/hw/pc.c
> >index cc6cfad..e5550ca 100644
> >--- a/hw/pc.c
> >+++ b/hw/pc.c
> >@@ -57,10 +57,6 @@
> >  #define DPRINTF(fmt, ...)
> >  #endif
> >
> >-#define BIOS_FILENAME "bios.bin"
> >-
> >-#define PC_MAX_BIOS_SIZE (4 * 1024 * 1024)
> >-
> >  /* Leave a chunk of memory at the top of RAM for the BIOS ACPI tables.  */
> >  #define ACPI_DATA_SIZE       0x10000
> >  #define BIOS_CFG_IOPORT 0x510
> >@@ -974,11 +970,9 @@ void pc_memory_init(MemoryRegion *system_memory,
> >                      MemoryRegion **ram_memory,
> >                      int system_firmware_enabled)
> >  {
> >-    char *filename;
> >-    int ret, linux_boot, i;
> >-    MemoryRegion *ram, *bios, *isa_bios, *option_rom_mr;
> >+    int linux_boot, i;
> >+    MemoryRegion *ram, *option_rom_mr;
> >      MemoryRegion *ram_below_4g, *ram_above_4g;
> >-    int bios_size, isa_bios_size;
> >      void *fw_cfg;
> >
> >      linux_boot = (kernel_filename != NULL);
> >@@ -1003,43 +997,9 @@ void pc_memory_init(MemoryRegion *system_memory,
> >                                      ram_above_4g);
> >      }
> >
> >-    /* BIOS load */
> >-    if (bios_name == NULL)
> >-        bios_name = BIOS_FILENAME;
> >-    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
> >-    if (filename) {
> >-        bios_size = get_image_size(filename);
> >-    } else {
> >-        bios_size = -1;
> >-    }
> >-    if (bios_size<= 0 ||
> >-        (bios_size % 65536) != 0) {
> >-        goto bios_error;
> >-    }
> >-    bios = g_malloc(sizeof(*bios));
> >-    memory_region_init_ram(bios, NULL, "pc.bios", bios_size);
> >-    memory_region_set_readonly(bios, true);
> >-    ret = rom_add_file_fixed(bios_name, (uint32_t)(-bios_size), -1);
> >-    if (ret != 0) {
> >-    bios_error:
> >-        fprintf(stderr, "qemu: could not load PC BIOS '%s'\n", bios_name);
> >-        exit(1);
> >-    }
> >-    if (filename) {
> >-        g_free(filename);
> >-    }
> >-    /* 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;
> >-    isa_bios = g_malloc(sizeof(*isa_bios));
> >-    memory_region_init_alias(isa_bios, "isa-bios", bios,
> >-                             bios_size - isa_bios_size, isa_bios_size);
> >-    memory_region_add_subregion_overlap(rom_memory,
> >-                                        0x100000 - isa_bios_size,
> >-                                        isa_bios,
> >-                                        1);
> >-    memory_region_set_readonly(isa_bios, true);
> >+
> >+    /* Initialize ROM or flash ranges for PC firmware */
> >+    pc_system_firmware_init(rom_memory, system_firmware_enabled);
> >
> >      option_rom_mr = g_malloc(sizeof(*option_rom_mr));
> >      memory_region_init_ram(option_rom_mr, NULL, "pc.rom", PC_ROM_SIZE);
> >@@ -1048,11 +1008,6 @@ void pc_memory_init(MemoryRegion *system_memory,
> >                                          option_rom_mr,
> >                                          1);
> >
> >-    /* map all the bios at the top of memory */
> >-    memory_region_add_subregion(rom_memory,
> >-                                (uint32_t)(-bios_size),
> >-                                bios);
> >-
> >      fw_cfg = bochs_bios_init();
> >      rom_set_fw(fw_cfg);
> >
> >diff --git a/hw/pc.h b/hw/pc.h
> >index 49471cb..727e231 100644
> >--- a/hw/pc.h
> >+++ b/hw/pc.h
> >@@ -246,6 +246,10 @@ static inline bool isa_ne2000_init(int base, int irq, 
> >NICInfo *nd)
> >      return true;
> >  }
> >
> >+/* pc_sysfw.c */
> >+void pc_system_firmware_init(MemoryRegion *rom_memory,
> >+                             int system_firmware_enabled);
> >+
> >  /* e820 types */
> >  #define E820_RAM        1
> >  #define E820_RESERVED   2
> >diff --git a/hw/pc_sysfw.c b/hw/pc_sysfw.c
> >new file mode 100644
> >index 0000000..20027b2
> >--- /dev/null
> >+++ b/hw/pc_sysfw.c
> >@@ -0,0 +1,255 @@
> >+/*
> >+ * QEMU PC System Firmware
> >+ *
> >+ * Copyright (c) 2003-2004 Fabrice Bellard
> >+ * Copyright (c) 2011 Intel Corporation
> >+ *
> >+ * Permission is hereby granted, free of charge, to any person obtaining a 
> >copy
> >+ * of this software and associated documentation files (the "Software"), to 
> >deal
> >+ * in the Software without restriction, including without limitation the 
> >rights
> >+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> >+ * copies of the Software, and to permit persons to whom the Software is
> >+ * furnished to do so, subject to the following conditions:
> >+ *
> >+ * The above copyright notice and this permission notice shall be included 
> >in
> >+ * all copies or substantial portions of the Software.
> >+ *
> >+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS 
> >OR
> >+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> >+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> >+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR 
> >OTHER
> >+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
> >FROM,
> >+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> >+ * THE SOFTWARE.
> >+ */
> >+
> >+#include "hw.h"
> >+#include "pc.h"
> >+#include "hw/boards.h"
> >+#include "loader.h"
> >+#include "sysemu.h"
> >+#include "flash.h"
> >+#include "kvm.h"
> >+
> >+#define BIOS_FILENAME "bios.bin"
> >+
> >+static void pc_isa_bios_init(MemoryRegion *rom_memory,
> >+                             MemoryRegion *flash_mem,
> >+                             int ram_size)
> >+{
> >+    int isa_bios_size;
> >+    MemoryRegion *isa_bios;
> >+    uint64_t flash_size;
> >+    void *flash_ptr, *isa_bios_ptr;
> >+
> >+    flash_size = memory_region_size(flash_mem);
> >+
> >+    /* map the last 128KB of the BIOS in ISA space */
> >+    isa_bios_size = flash_size;
> >+    if (isa_bios_size>  (128 * 1024)) {
> >+        isa_bios_size = 128 * 1024;
> >+    }
> >+    isa_bios = g_malloc(sizeof(*isa_bios));
> >+    memory_region_init_ram(isa_bios, NULL, "isa-bios", isa_bios_size);
> >+    memory_region_add_subregion_overlap(rom_memory,
> >+                                        0x100000 - isa_bios_size,
> >+                                        isa_bios,
> >+                                        1);
> >+
> >+    /* copy ISA rom image from top of flash memory */
> >+    flash_ptr = memory_region_get_ram_ptr(flash_mem);
> >+    isa_bios_ptr = memory_region_get_ram_ptr(isa_bios);
> >+    memcpy(isa_bios_ptr,
> >+           ((uint8_t*)flash_ptr) + (flash_size - isa_bios_size),
> >+           isa_bios_size);
> >+
> >+    memory_region_set_readonly(isa_bios, true);
> >+}
> >+
> >+static void pc_fw_add_pflash_drv(void)
> >+{
> >+    QemuOpts *opts;
> >+    QEMUMachine *machine;
> >+    char *filename;
> >+
> >+    if (bios_name == NULL) {
> >+        bios_name = BIOS_FILENAME;
> >+    }
> >+    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
> >+
> >+    opts = drive_add(IF_PFLASH, -1, filename, "readonly=on");
> >+    if (opts == NULL) {
> >+      return;
> >+    }
> >+
> >+    machine = find_default_machine();
> >+    if (machine == NULL) {
> >+      return;
> >+    }
> >+
> >+    drive_init(opts, machine->use_scsi);
> >+}
> >+
> >+static void pc_system_flash_init(MemoryRegion *rom_memory,
> >+                                 DriveInfo *pflash_drv)
> >+{
> >+    BlockDriverState *bdrv;
> >+    int64_t size;
> >+    target_phys_addr_t phys_addr;
> >+    int sector_bits, sector_size;
> >+    pflash_t *system_flash;
> >+    MemoryRegion *flash_mem;
> >+
> >+    bdrv = pflash_drv->bdrv;
> >+    size = bdrv_getlength(pflash_drv->bdrv);
> >+    sector_bits = 12;
> >+    sector_size = 1<<  sector_bits;
> >+
> >+    if ((size % sector_size) != 0) {
> >+        fprintf(stderr,
> >+                "qemu: PC system firmware (pflash) must be a multiple of 
> >0x%x\n",
> >+                sector_size);
> >+        exit(1);
> >+    }
> >+
> >+    phys_addr = 0x100000000ULL - size;
> >+    system_flash = pflash_cfi01_register(phys_addr, NULL, "system.flash", 
> >size,
> >+                                         bdrv, sector_size, size>>  
> >sector_bits,
> >+                                         1, 0x0000, 0x0000, 0x0000, 0x0000, 
> >0);
> >+    flash_mem = pflash_cfi01_get_memory(system_flash);
> >+
> >+    pc_isa_bios_init(rom_memory, flash_mem, size);
> >+}
> >+
> >+static void pc_system_rom_init(MemoryRegion *rom_memory,
> >+                               DriveInfo *pflash_drv)
> >+{
> >+    BlockDriverState *bdrv;
> >+    int64_t size;
> >+    target_phys_addr_t phys_addr;
> >+    int sector_bits, sector_size;
> >+    MemoryRegion *sys_rom;
> >+    void *buffer;
> >+    int ret;
> >+
> >+    bdrv = pflash_drv->bdrv;
> >+    size = bdrv_getlength(pflash_drv->bdrv);
> >+    sector_bits = 9;
> >+    sector_size = 1<<  sector_bits;
> >+
> >+    if ((size % sector_size) != 0) {
> >+        fprintf(stderr,
> >+                "qemu: PC system rom (pflash) must be a multiple of 0x%x\n",
> >+                sector_size);
> >+        exit(1);
> >+    }
> >+
> >+    phys_addr = 0x100000000ULL - size;
> >+    sys_rom = g_malloc(sizeof(*sys_rom));
> >+    memory_region_init_ram(sys_rom, NULL, "system.rom", size);
> >+    buffer = memory_region_get_ram_ptr(sys_rom);
> >+    memory_region_add_subregion(rom_memory, phys_addr, sys_rom);
> >+
> >+    /* read the rom content */
> >+    ret = bdrv_read(bdrv, 0, buffer, size>>  sector_bits);
> 
> I think we're trying to get rid of synchronous block I/O in machine
> initialization for a number of reasons.
> 
> Kevin/Stefan, care to comment?  Will this be problematic in the future?

This looks okay to me because it is not called from vcpu context.  We
need to avoid device emulation that uses synchronous I/O for pio/mmio
because that blocks the guest from executing code.  In this case the
code runs before the VM starts and should be easy to update if we ever
drop the bdrv_read() interface.

Stefan