All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v9 0/3] PC system flash support
@ 2011-12-15 20:51 Jordan Justen
  2011-12-15 20:51 ` [Qemu-devel] [PATCH v9 1/3] pc: Add pc-1.1 machine type Jordan Justen
                   ` (3 more replies)
  0 siblings, 4 replies; 15+ messages in thread
From: Jordan Justen @ 2011-12-15 20:51 UTC (permalink / raw)
  To: qemu-devel; +Cc: Jordan Justen

Enable flash emulation in a PC system using pflash_cfi01.

v9:
* Add pc-1.1
* pc-1.0 uses previous rom firmware init code path

v8:
* Cleanup two chunks of debug code (printf messages)
* Fix comment in pc.h (pcflash.c => pc_sysfw.c)

v7:
* Do not add system firmware to qemu roms
* If kvm is enabled, copy pflash drive contents into a
  read-only ram region, since kvm cannot currently execute
  code from a pflash device.
* Rename pcflash.c to pc_sysfw.c

v6:
* Rebase for memory API
* pflash_cfi01: Set error in status register when a write to
  erase is attempted in read-only mode.
* Add system firmware to qemu roms

v5:
* Enable pflash read-only mode
* Enable -drive with if=pflash to define system firmware image

v4:
* Rebase

v3:
* Fix code style issues
* Add additional comments

v2:
* Convert debug printf to DPRINTF

Jordan Justen (3):
  pc: Add pc-1.1 machine type
  pflash: Support read-only mode
  pc: Support system flash memory with pflash

 Makefile.target                    |    1 +
 blockdev.c                         |    3 +-
 default-configs/i386-softmmu.mak   |    1 +
 default-configs/x86_64-softmmu.mak |    1 +
 hw/boards.h                        |    1 +
 hw/pc.c                            |   58 +-------
 hw/pc.h                            |    7 +-
 hw/pc_piix.c                       |   40 +++++-
 hw/pc_sysfw.c                      |  255 ++++++++++++++++++++++++++++++++++++
 hw/pflash_cfi01.c                  |   44 ++++--
 hw/pflash_cfi02.c                  |   83 +++++++------
 vl.c                               |    2 +-
 12 files changed, 382 insertions(+), 114 deletions(-)
 create mode 100644 hw/pc_sysfw.c

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [Qemu-devel] [PATCH v9 1/3] pc: Add pc-1.1 machine type
  2011-12-15 20:51 [Qemu-devel] [PATCH v9 0/3] PC system flash support Jordan Justen
@ 2011-12-15 20:51 ` Jordan Justen
  2011-12-15 20:51 ` [Qemu-devel] [PATCH v9 2/3] pflash: Support read-only mode Jordan Justen
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 15+ messages in thread
From: Jordan Justen @ 2011-12-15 20:51 UTC (permalink / raw)
  To: qemu-devel; +Cc: Jordan Justen, Avi Kivity

This machine type adds a new system_firmware_enabled
parameter which is passed into pc_memory_init.

This will be used by the system firmware support which
enables a flash memory to be used with qemu.

Signed-off-by: Jordan Justen <jordan.l.justen@intel.com>
Cc: Avi Kivity <avi@redhat.com>
---
 hw/pc.c      |    3 ++-
 hw/pc.h      |    3 ++-
 hw/pc_piix.c |   40 ++++++++++++++++++++++++++++++++--------
 3 files changed, 36 insertions(+), 10 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index 33778fe..cc6cfad 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -971,7 +971,8 @@ void pc_memory_init(MemoryRegion *system_memory,
                     ram_addr_t below_4g_mem_size,
                     ram_addr_t above_4g_mem_size,
                     MemoryRegion *rom_memory,
-                    MemoryRegion **ram_memory)
+                    MemoryRegion **ram_memory,
+                    int system_firmware_enabled)
 {
     char *filename;
     int ret, linux_boot, i;
diff --git a/hw/pc.h b/hw/pc.h
index b7b7e40..49471cb 100644
--- a/hw/pc.h
+++ b/hw/pc.h
@@ -138,7 +138,8 @@ void pc_memory_init(MemoryRegion *system_memory,
                     ram_addr_t below_4g_mem_size,
                     ram_addr_t above_4g_mem_size,
                     MemoryRegion *rom_memory,
-                    MemoryRegion **ram_memory);
+                    MemoryRegion **ram_memory,
+                    int system_firmware_enabled);
 qemu_irq *pc_allocate_cpu_irq(void);
 void pc_vga_init(PCIBus *pci_bus);
 void pc_basic_device_init(qemu_irq *gsi,
diff --git a/hw/pc_piix.c b/hw/pc_piix.c
index 970f43c..007f10c 100644
--- a/hw/pc_piix.c
+++ b/hw/pc_piix.c
@@ -79,7 +79,8 @@ static void pc_init1(MemoryRegion *system_memory,
                      const char *initrd_filename,
                      const char *cpu_model,
                      int pci_enabled,
-                     int kvmclock_enabled)
+                     int kvmclock_enabled,
+                     int system_firmware_enabled)
 {
     int i;
     ram_addr_t below_4g_mem_size, above_4g_mem_size;
@@ -128,7 +129,8 @@ static void pc_init1(MemoryRegion *system_memory,
         pc_memory_init(system_memory,
                        kernel_filename, kernel_cmdline, initrd_filename,
                        below_4g_mem_size, above_4g_mem_size,
-                       pci_enabled ? rom_memory : system_memory, &ram_memory);
+                       pci_enabled ? rom_memory : system_memory, &ram_memory,
+                       system_firmware_enabled);
     }
 
     gsi_state = g_malloc0(sizeof(*gsi_state));
@@ -246,7 +248,21 @@ static void pc_init_pci(ram_addr_t ram_size,
              get_system_io(),
              ram_size, boot_device,
              kernel_filename, kernel_cmdline,
-             initrd_filename, cpu_model, 1, 1);
+             initrd_filename, cpu_model, 1, 1, 1);
+}
+
+static void pc_init_pci_system_rom_only(ram_addr_t ram_size,
+                                        const char *boot_device,
+                                        const char *kernel_filename,
+                                        const char *kernel_cmdline,
+                                        const char *initrd_filename,
+                                        const char *cpu_model)
+{
+    pc_init1(get_system_memory(),
+             get_system_io(),
+             ram_size, boot_device,
+             kernel_filename, kernel_cmdline,
+             initrd_filename, cpu_model, 1, 1, 0);
 }
 
 static void pc_init_pci_no_kvmclock(ram_addr_t ram_size,
@@ -260,7 +276,7 @@ static void pc_init_pci_no_kvmclock(ram_addr_t ram_size,
              get_system_io(),
              ram_size, boot_device,
              kernel_filename, kernel_cmdline,
-             initrd_filename, cpu_model, 1, 0);
+             initrd_filename, cpu_model, 1, 0, 0);
 }
 
 static void pc_init_isa(ram_addr_t ram_size,
@@ -276,7 +292,7 @@ static void pc_init_isa(ram_addr_t ram_size,
              get_system_io(),
              ram_size, boot_device,
              kernel_filename, kernel_cmdline,
-             initrd_filename, cpu_model, 0, 1);
+             initrd_filename, cpu_model, 0, 1, 0);
 }
 
 #ifdef CONFIG_XEN
@@ -297,8 +313,8 @@ static void pc_xen_hvm_init(ram_addr_t ram_size,
 }
 #endif
 
-static QEMUMachine pc_machine_v1_0 = {
-    .name = "pc-1.0",
+static QEMUMachine pc_machine_v1_1 = {
+    .name = "pc-1.1",
     .alias = "pc",
     .desc = "Standard PC",
     .init = pc_init_pci,
@@ -306,10 +322,17 @@ static QEMUMachine pc_machine_v1_0 = {
     .is_default = 1,
 };
 
+static QEMUMachine pc_machine_v1_0 = {
+    .name = "pc-1.0",
+    .desc = "Standard PC",
+    .init = pc_init_pci_system_rom_only,
+    .max_cpus = 255,
+};
+
 static QEMUMachine pc_machine_v0_14 = {
     .name = "pc-0.14",
     .desc = "Standard PC",
-    .init = pc_init_pci,
+    .init = pc_init_pci_system_rom_only,
     .max_cpus = 255,
     .compat_props = (GlobalProperty[]) {
         {
@@ -556,6 +579,7 @@ static QEMUMachine xenfv_machine = {
 
 static void pc_machine_init(void)
 {
+    qemu_register_machine(&pc_machine_v1_1);
     qemu_register_machine(&pc_machine_v1_0);
     qemu_register_machine(&pc_machine_v0_14);
     qemu_register_machine(&pc_machine_v0_13);
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [Qemu-devel] [PATCH v9 2/3] pflash: Support read-only mode
  2011-12-15 20:51 [Qemu-devel] [PATCH v9 0/3] PC system flash support Jordan Justen
  2011-12-15 20:51 ` [Qemu-devel] [PATCH v9 1/3] pc: Add pc-1.1 machine type Jordan Justen
@ 2011-12-15 20:51 ` Jordan Justen
  2011-12-15 20:51 ` [Qemu-devel] [PATCH v9 3/3] pc: Support system flash memory with pflash Jordan Justen
  2011-12-15 21:02 ` [Qemu-devel] [PATCH v9 0/3] PC system flash support Jordan Justen
  3 siblings, 0 replies; 15+ messages in thread
From: Jordan Justen @ 2011-12-15 20:51 UTC (permalink / raw)
  To: qemu-devel; +Cc: Jordan Justen

When read-only mode is enabled, no changes will be made
to the flash image in memory, and no bdrv_write calls will be
made.

For pflash_cfi01 (Intel), if the flash is in read-only mode
then the status register will signal block erase error or
program error when these operations are attempted.

For pflash_cfi02 (AMD), if the flash is in read-only mode
then the pflash will silently ignore all write/erase commands.

Signed-off-by: Jordan Justen <jordan.l.justen@intel.com>
---
 blockdev.c        |    3 +-
 hw/pflash_cfi01.c |   44 +++++++++++++++++++---------
 hw/pflash_cfi02.c |   83 ++++++++++++++++++++++++++++------------------------
 3 files changed, 77 insertions(+), 53 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index dbf0251..9096ae6 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -556,7 +556,8 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
         /* CDROM is fine for any interface, don't check.  */
         ro = 1;
     } else if (ro == 1) {
-        if (type != IF_SCSI && type != IF_VIRTIO && type != IF_FLOPPY && type != IF_NONE) {
+        if (type != IF_SCSI && type != IF_VIRTIO && type != IF_FLOPPY &&
+            type != IF_NONE && type != IF_PFLASH) {
             error_report("readonly not supported by this bus type");
             goto err;
         }
diff --git a/hw/pflash_cfi01.c b/hw/pflash_cfi01.c
index 69b8e3d..1e0a053 100644
--- a/hw/pflash_cfi01.c
+++ b/hw/pflash_cfi01.c
@@ -283,8 +283,12 @@ static void pflash_write(pflash_t *pfl, target_phys_addr_t offset,
                     TARGET_FMT_plx "\n",
                     __func__, offset, pfl->sector_len);
 
-            memset(p + offset, 0xff, pfl->sector_len);
-            pflash_update(pfl, offset, pfl->sector_len);
+            if (!pfl->ro) {
+                memset(p + offset, 0xff, pfl->sector_len);
+                pflash_update(pfl, offset, pfl->sector_len);
+            } else {
+                pfl->status |= 0x20; /* Block erase error */
+            }
             pfl->status |= 0x80; /* Ready! */
             break;
         case 0x50: /* Clear status bits */
@@ -323,8 +327,12 @@ static void pflash_write(pflash_t *pfl, target_phys_addr_t offset,
         case 0x10: /* Single Byte Program */
         case 0x40: /* Single Byte Program */
             DPRINTF("%s: Single Byte Program\n", __func__);
-            pflash_data_write(pfl, offset, value, width, be);
-            pflash_update(pfl, offset, width);
+            if (!pfl->ro) {
+                pflash_data_write(pfl, offset, value, width, be);
+                pflash_update(pfl, offset, width);
+            } else {
+                pfl->status |= 0x10; /* Programming error */
+            }
             pfl->status |= 0x80; /* Ready! */
             pfl->wcycle = 0;
         break;
@@ -372,7 +380,11 @@ static void pflash_write(pflash_t *pfl, target_phys_addr_t offset,
     case 2:
         switch (pfl->cmd) {
         case 0xe8: /* Block write */
-            pflash_data_write(pfl, offset, value, width, be);
+            if (!pfl->ro) {
+                pflash_data_write(pfl, offset, value, width, be);
+            } else {
+                pfl->status |= 0x10; /* Programming error */
+            }
 
             pfl->status |= 0x80;
 
@@ -382,8 +394,12 @@ static void pflash_write(pflash_t *pfl, target_phys_addr_t offset,
 
                 DPRINTF("%s: block write finished\n", __func__);
                 pfl->wcycle++;
-                /* Flush the entire write buffer onto backing storage.  */
-                pflash_update(pfl, offset & mask, pfl->writeblock_size);
+                if (!pfl->ro) {
+                    /* Flush the entire write buffer onto backing storage.  */
+                    pflash_update(pfl, offset & mask, pfl->writeblock_size);
+                } else {
+                    pfl->status |= 0x10; /* Programming error */
+                }
             }
 
             pfl->counter--;
@@ -605,13 +621,13 @@ pflash_t *pflash_cfi01_register(target_phys_addr_t base,
         }
         bdrv_attach_dev_nofail(pfl->bs, pfl);
     }
-#if 0 /* XXX: there should be a bit to set up read-only,
-       *      the same way the hardware does (with WP pin).
-       */
-    pfl->ro = 1;
-#else
-    pfl->ro = 0;
-#endif
+
+    if (pfl->bs) {
+        pfl->ro = bdrv_is_read_only(pfl->bs);
+    } else {
+        pfl->ro = 0;
+    }
+
     pfl->timer = qemu_new_timer_ns(vm_clock, pflash_timer, pfl);
     pfl->base = base;
     pfl->sector_len = sector_len;
diff --git a/hw/pflash_cfi02.c b/hw/pflash_cfi02.c
index e5a63da..9e91bdd 100644
--- a/hw/pflash_cfi02.c
+++ b/hw/pflash_cfi02.c
@@ -329,35 +329,37 @@ static void pflash_write (pflash_t *pfl, target_phys_addr_t offset,
             DPRINTF("%s: write data offset " TARGET_FMT_plx " %08x %d\n",
                     __func__, offset, value, width);
             p = pfl->storage;
-            switch (width) {
-            case 1:
-                p[offset] &= value;
-                pflash_update(pfl, offset, 1);
-                break;
-            case 2:
-                if (be) {
-                    p[offset] &= value >> 8;
-                    p[offset + 1] &= value;
-                } else {
+            if (!pfl->ro) {
+                switch (width) {
+                case 1:
                     p[offset] &= value;
-                    p[offset + 1] &= value >> 8;
+                    pflash_update(pfl, offset, 1);
+                    break;
+                case 2:
+                    if (be) {
+                        p[offset] &= value >> 8;
+                        p[offset + 1] &= value;
+                    } else {
+                        p[offset] &= value;
+                        p[offset + 1] &= value >> 8;
+                    }
+                    pflash_update(pfl, offset, 2);
+                    break;
+                case 4:
+                    if (be) {
+                        p[offset] &= value >> 24;
+                        p[offset + 1] &= value >> 16;
+                        p[offset + 2] &= value >> 8;
+                        p[offset + 3] &= value;
+                    } else {
+                        p[offset] &= value;
+                        p[offset + 1] &= value >> 8;
+                        p[offset + 2] &= value >> 16;
+                        p[offset + 3] &= value >> 24;
+                    }
+                    pflash_update(pfl, offset, 4);
+                    break;
                 }
-                pflash_update(pfl, offset, 2);
-                break;
-            case 4:
-                if (be) {
-                    p[offset] &= value >> 24;
-                    p[offset + 1] &= value >> 16;
-                    p[offset + 2] &= value >> 8;
-                    p[offset + 3] &= value;
-                } else {
-                    p[offset] &= value;
-                    p[offset + 1] &= value >> 8;
-                    p[offset + 2] &= value >> 16;
-                    p[offset + 3] &= value >> 24;
-                }
-                pflash_update(pfl, offset, 4);
-                break;
             }
             pfl->status = 0x00 | ~(value & 0x80);
             /* Let's pretend write is immediate */
@@ -403,9 +405,11 @@ static void pflash_write (pflash_t *pfl, target_phys_addr_t offset,
             }
             /* Chip erase */
             DPRINTF("%s: start chip erase\n", __func__);
-            memset(pfl->storage, 0xFF, pfl->chip_len);
+            if (!pfl->ro) {
+                memset(pfl->storage, 0xFF, pfl->chip_len);
+                pflash_update(pfl, 0, pfl->chip_len);
+            }
             pfl->status = 0x00;
-            pflash_update(pfl, 0, pfl->chip_len);
             /* Let's wait 5 seconds before chip erase is done */
             qemu_mod_timer(pfl->timer,
                            qemu_get_clock_ns(vm_clock) + (get_ticks_per_sec() * 5));
@@ -416,8 +420,10 @@ static void pflash_write (pflash_t *pfl, target_phys_addr_t offset,
             offset &= ~(pfl->sector_len - 1);
             DPRINTF("%s: start sector erase at " TARGET_FMT_plx "\n", __func__,
                     offset);
-            memset(p + offset, 0xFF, pfl->sector_len);
-            pflash_update(pfl, offset, pfl->sector_len);
+            if (!pfl->ro) {
+                memset(p + offset, 0xFF, pfl->sector_len);
+                pflash_update(pfl, offset, pfl->sector_len);
+            }
             pfl->status = 0x00;
             /* Let's wait 1/2 second before sector erase is done */
             qemu_mod_timer(pfl->timer,
@@ -643,16 +649,17 @@ pflash_t *pflash_cfi02_register(target_phys_addr_t base,
         }
         bdrv_attach_dev_nofail(pfl->bs, pfl);
     }
+
     pflash_setup_mappings(pfl);
     pfl->rom_mode = 1;
     memory_region_add_subregion(get_system_memory(), pfl->base, &pfl->mem);
-#if 0 /* XXX: there should be a bit to set up read-only,
-       *      the same way the hardware does (with WP pin).
-       */
-    pfl->ro = 1;
-#else
-    pfl->ro = 0;
-#endif
+
+    if (pfl->bs) {
+        pfl->ro = bdrv_is_read_only(pfl->bs);
+    } else {
+        pfl->ro = 0;
+    }
+
     pfl->timer = qemu_new_timer_ns(vm_clock, pflash_timer, pfl);
     pfl->sector_len = sector_len;
     pfl->width = width;
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [Qemu-devel] [PATCH v9 3/3] pc: Support system flash memory with pflash
  2011-12-15 20:51 [Qemu-devel] [PATCH v9 0/3] PC system flash support Jordan Justen
  2011-12-15 20:51 ` [Qemu-devel] [PATCH v9 1/3] pc: Add pc-1.1 machine type Jordan Justen
  2011-12-15 20:51 ` [Qemu-devel] [PATCH v9 2/3] pflash: Support read-only mode Jordan Justen
@ 2011-12-15 20:51 ` Jordan Justen
  2011-12-19 19:41   ` Anthony Liguori
  2011-12-15 21:02 ` [Qemu-devel] [PATCH v9 0/3] PC system flash support Jordan Justen
  3 siblings, 1 reply; 15+ messages in thread
From: Jordan Justen @ 2011-12-15 20:51 UTC (permalink / raw)
  To: qemu-devel; +Cc: Jordan Justen, Anthony Liguori

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 <jordan.l.justen@intel.com>
Cc: Anthony Liguori <aliguori@us.ibm.com>
---
 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);
+    if (ret < 0) {
+        memory_region_destroy(sys_rom);
+        g_free(sys_rom);
+        fprintf(stderr,
+                "qemu: Failed to read rom image from pflash drive\n");
+        exit(1);
+    }
+
+    memory_region_set_readonly(sys_rom, true);
+
+    pc_isa_bios_init(rom_memory, sys_rom, size);
+}
+
+static void old_pc_system_rom_init(MemoryRegion *rom_memory)
+{
+    char *filename;
+    MemoryRegion *bios, *isa_bios;
+    int bios_size, isa_bios_size;
+    int ret;
+
+    /* 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);
+
+    /* map all the bios at the top of memory */
+    memory_region_add_subregion(rom_memory,
+                                (uint32_t)(-bios_size),
+                                bios);
+
+}
+
+void pc_system_firmware_init(MemoryRegion *rom_memory,
+                             int system_firmware_enabled)
+{
+    int flash_present;
+    DriveInfo *pflash_drv;
+
+    if (!system_firmware_enabled) {
+        old_pc_system_rom_init(rom_memory);
+        return;
+    }
+
+    pflash_drv = drive_get(IF_PFLASH, 0, 0);
+    flash_present = (pflash_drv != NULL);
+
+    if (!flash_present) {
+        pc_fw_add_pflash_drv();
+        pflash_drv = drive_get(IF_PFLASH, 0, 0);
+        flash_present = (pflash_drv != NULL);
+    }
+
+    if (!flash_present) {
+        fprintf(stderr, "qemu: PC system firmware (pflash) not available\n");
+        exit(1);
+    }
+
+    if (!kvm_enabled()) {
+        pc_system_flash_init(rom_memory, pflash_drv);
+    } else {
+        pc_system_rom_init(rom_memory, pflash_drv);
+    }
+}
+
+
diff --git a/vl.c b/vl.c
index 5372a96..9f77742 100644
--- a/vl.c
+++ b/vl.c
@@ -1186,7 +1186,7 @@ static QEMUMachine *find_machine(const char *name)
     return NULL;
 }
 
-static QEMUMachine *find_default_machine(void)
+QEMUMachine *find_default_machine(void)
 {
     QEMUMachine *m;
 
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH v9 0/3] PC system flash support
  2011-12-15 20:51 [Qemu-devel] [PATCH v9 0/3] PC system flash support Jordan Justen
                   ` (2 preceding siblings ...)
  2011-12-15 20:51 ` [Qemu-devel] [PATCH v9 3/3] pc: Support system flash memory with pflash Jordan Justen
@ 2011-12-15 21:02 ` Jordan Justen
  2011-12-18 10:04   ` Avi Kivity
  3 siblings, 1 reply; 15+ messages in thread
From: Jordan Justen @ 2011-12-15 21:02 UTC (permalink / raw)
  To: qemu-devel, Avi Kivity

[-- Attachment #1: Type: text/plain, Size: 4065 bytes --]

I verified that 'info mtree' and 'info qdev' are equivalent for pc-1.0
when using master and with my patches.

However, I did discover that v1.0 seems to differ from master for this
same test.

I've attached the logs.

--- v1.0-pc.log	2011-12-15 12:14:53.000000000 -0800
+++ master-pc-1.0.log	2011-12-15 12:25:04.000000000 -0800
@@ -1,4 +1,4 @@
-QEMU 1.0 monitor - type 'help' for more information
+QEMU 1.0.50 monitor - type 'help' for more information
 (qemu) info mtree
 memory
 0000000000000000-7ffffffffffffffe (prio 0): system
@@ -18,6 +18,8 @@
   00000000000ec000-00000000000effff (prio 1): alias pam-ram @pc.ram
00000000000ec000-00000000000effff
   00000000000f0000-00000000000fffff (prio 1): alias pam-rom @pc.ram
00000000000f0000-00000000000fffff
   0000000008000000-00000000ffffffff (prio 0): alias pci-hole @pci
0000000008000000-00000000ffffffff
+  00000000fec00000-00000000fec00fff (prio 0): ioapic
+  00000000fed00000-00000000fed003ff (prio 0): hpet
   00000000fee00000-00000000feefffff (prio 0): apic
   4000000000000000-7fffffffffffffff (prio 0): alias pci-hole64 @pci
4000000000000000-7fffffffffffffff
 pc.ram
@@ -56,6 +58,7 @@
   00000000000003f8-00000000000003ff (prio 0): serial
   00000000000004d0-00000000000004d0 (prio 0): elcr
   00000000000004d1-00000000000004d1 (prio 0): elcr
+  0000000000000510-0000000000000511 (prio 0): fwcfg
   0000000000000cf8-0000000000000cfb (prio 0): pci-conf-idx
   0000000000000cfc-0000000000000cff (prio 0): pci-conf-data
   0000000000005658-0000000000005658 (prio 0): vmport
@@ -126,7 +129,7 @@
             dev-prop: opt_io_size = 0
             dev-prop: bootindex = -1
             dev-prop: discard_granularity = 0
-            dev-prop: ver = "1.0"
+            dev-prop: ver = "1.0.50"
             dev-prop: serial = "QM00003"
             bus-prop: unit = 0
         bus: ide.0
@@ -218,7 +221,7 @@
     dev-prop: data_iobase = 0x511
     irq 0
     mmio ffffffffffffffff/0000000000000002
-    mmio ffffffffffffffff/0000000000000002
+    mmio ffffffffffffffff/0000000000000001
   dev: apic, id ""
     dev-prop: id = 0
     irq 0

Thanks,

-Jordan

On Thu, Dec 15, 2011 at 12:51, Jordan Justen <jordan.l.justen@intel.com> wrote:
> Enable flash emulation in a PC system using pflash_cfi01.
>
> v9:
> * Add pc-1.1
> * pc-1.0 uses previous rom firmware init code path
>
> v8:
> * Cleanup two chunks of debug code (printf messages)
> * Fix comment in pc.h (pcflash.c => pc_sysfw.c)
>
> v7:
> * Do not add system firmware to qemu roms
> * If kvm is enabled, copy pflash drive contents into a
>  read-only ram region, since kvm cannot currently execute
>  code from a pflash device.
> * Rename pcflash.c to pc_sysfw.c
>
> v6:
> * Rebase for memory API
> * pflash_cfi01: Set error in status register when a write to
>  erase is attempted in read-only mode.
> * Add system firmware to qemu roms
>
> v5:
> * Enable pflash read-only mode
> * Enable -drive with if=pflash to define system firmware image
>
> v4:
> * Rebase
>
> v3:
> * Fix code style issues
> * Add additional comments
>
> v2:
> * Convert debug printf to DPRINTF
>
> Jordan Justen (3):
>  pc: Add pc-1.1 machine type
>  pflash: Support read-only mode
>  pc: Support system flash memory with pflash
>
>  Makefile.target                    |    1 +
>  blockdev.c                         |    3 +-
>  default-configs/i386-softmmu.mak   |    1 +
>  default-configs/x86_64-softmmu.mak |    1 +
>  hw/boards.h                        |    1 +
>  hw/pc.c                            |   58 +-------
>  hw/pc.h                            |    7 +-
>  hw/pc_piix.c                       |   40 +++++-
>  hw/pc_sysfw.c                      |  255 ++++++++++++++++++++++++++++++++++++
>  hw/pflash_cfi01.c                  |   44 ++++--
>  hw/pflash_cfi02.c                  |   83 +++++++------
>  vl.c                               |    2 +-
>  12 files changed, 382 insertions(+), 114 deletions(-)
>  create mode 100644 hw/pc_sysfw.c
>
>

[-- Attachment #2: master-pc-1.0.log --]
[-- Type: application/octet-stream, Size: 10364 bytes --]

[-- Attachment #3: v1.0-pc.log --]
[-- Type: application/octet-stream, Size: 10199 bytes --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH v9 0/3] PC system flash support
  2011-12-15 21:02 ` [Qemu-devel] [PATCH v9 0/3] PC system flash support Jordan Justen
@ 2011-12-18 10:04   ` Avi Kivity
  2011-12-19 18:27     ` Jordan Justen
  0 siblings, 1 reply; 15+ messages in thread
From: Avi Kivity @ 2011-12-18 10:04 UTC (permalink / raw)
  To: Jordan Justen; +Cc: qemu-devel

On 12/15/2011 11:02 PM, Jordan Justen wrote:
> I verified that 'info mtree' and 'info qdev' are equivalent for pc-1.0
> when using master and with my patches.
>
> However, I did discover that v1.0 seems to differ from master for this
> same test.
>

That's are fine; pre-memory-API devices don't show up in info mtree.

> @@ -218,7 +221,7 @@
>      dev-prop: data_iobase = 0x511
>      irq 0
>      mmio ffffffffffffffff/0000000000000002
> -    mmio ffffffffffffffff/0000000000000002
> +    mmio ffffffffffffffff/0000000000000001
>

This one isn't nice.  Doesn't affect pc though.

-- 
error compiling committee.c: too many arguments to function

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH v9 0/3] PC system flash support
  2011-12-18 10:04   ` Avi Kivity
@ 2011-12-19 18:27     ` Jordan Justen
  2011-12-20 10:53       ` Avi Kivity
  0 siblings, 1 reply; 15+ messages in thread
From: Jordan Justen @ 2011-12-19 18:27 UTC (permalink / raw)
  To: Avi Kivity; +Cc: qemu-devel

On Sun, Dec 18, 2011 at 02:04, Avi Kivity <avi@redhat.com> wrote:
> On 12/15/2011 11:02 PM, Jordan Justen wrote:
>> I verified that 'info mtree' and 'info qdev' are equivalent for pc-1.0
>> when using master and with my patches.
>>
>> However, I did discover that v1.0 seems to differ from master for this
>> same test.
>>
>
> That's are fine; pre-memory-API devices don't show up in info mtree.
>
>> @@ -218,7 +221,7 @@
>>      dev-prop: data_iobase = 0x511
>>      irq 0
>>      mmio ffffffffffffffff/0000000000000002
>> -    mmio ffffffffffffffff/0000000000000002
>> +    mmio ffffffffffffffff/0000000000000001
>>
>
> This one isn't nice.  Doesn't affect pc though.

Thanks Avi.  Does this patch series now address your concerns about
qtree/mtree and pc-1.0 with regards to the x86 flash feature?

-Jordan

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH v9 3/3] pc: Support system flash memory with pflash
  2011-12-15 20:51 ` [Qemu-devel] [PATCH v9 3/3] pc: Support system flash memory with pflash Jordan Justen
@ 2011-12-19 19:41   ` Anthony Liguori
  2011-12-19 21:25     ` Jordan Justen
  2011-12-20  8:11     ` Stefan Hajnoczi
  0 siblings, 2 replies; 15+ messages in thread
From: Anthony Liguori @ 2011-12-19 19:41 UTC (permalink / raw)
  To: Jordan Justen; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi

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<jordan.l.justen@intel.com>
> Cc: Anthony Liguori<aliguori@us.ibm.com>
> ---
>   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?

Regards,

Anthony Liguori

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH v9 3/3] pc: Support system flash memory with pflash
  2011-12-19 19:41   ` Anthony Liguori
@ 2011-12-19 21:25     ` Jordan Justen
  2011-12-19 22:19       ` Anthony Liguori
  2011-12-20  8:11     ` Stefan Hajnoczi
  1 sibling, 1 reply; 15+ messages in thread
From: Jordan Justen @ 2011-12-19 21:25 UTC (permalink / raw)
  To: Anthony Liguori; +Cc: Kevin Wolf, Jordan Justen, qemu-devel, Stefan Hajnoczi

On Mon, Dec 19, 2011 at 11:41, Anthony Liguori <aliguori@us.ibm.com> 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<jordan.l.justen@intel.com>
>> Cc: Anthony Liguori<aliguori@us.ibm.com>
>> ---
>>  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?

I was hoping pc-1.1 with and without kvm could be as close as
possible, but I guess I can make pc-1.1 with kvm behave the same as
pc-1.0.  Then I can delete pc_system_rom_init.

-Jordan

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH v9 3/3] pc: Support system flash memory with pflash
  2011-12-19 21:25     ` Jordan Justen
@ 2011-12-19 22:19       ` Anthony Liguori
  2012-01-09  9:28         ` Kevin Wolf
  0 siblings, 1 reply; 15+ messages in thread
From: Anthony Liguori @ 2011-12-19 22:19 UTC (permalink / raw)
  To: Jordan Justen
  Cc: Kevin Wolf, Jordan Justen, Anthony Liguori, qemu-devel, Stefan Hajnoczi

On 12/19/2011 03:25 PM, Jordan Justen wrote:
> On Mon, Dec 19, 2011 at 11:41, Anthony Liguori<aliguori@us.ibm.com>  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<jordan.l.justen@intel.com>
>>> Cc: Anthony Liguori<aliguori@us.ibm.com>
>>> ---
>>>   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?
>
> I was hoping pc-1.1 with and without kvm could be as close as
> possible, but I guess I can make pc-1.1 with kvm behave the same as
> pc-1.0.  Then I can delete pc_system_rom_init.

I think your general approach is right, I'm just not sure what we're going to do 
short term about synchronous I/O in the machine init routines.  It may just be a 
matter of structuring this in such a way that you can use an async interface.

Regards,

Anthony Liguori

>
> -Jordan
>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH v9 3/3] pc: Support system flash memory with pflash
  2011-12-19 19:41   ` Anthony Liguori
  2011-12-19 21:25     ` Jordan Justen
@ 2011-12-20  8:11     ` Stefan Hajnoczi
  1 sibling, 0 replies; 15+ messages in thread
From: Stefan Hajnoczi @ 2011-12-20  8:11 UTC (permalink / raw)
  To: Anthony Liguori; +Cc: Kevin Wolf, Jordan Justen, qemu-devel, 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<jordan.l.justen@intel.com>
> >Cc: Anthony Liguori<aliguori@us.ibm.com>
> >---
> >  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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH v9 0/3] PC system flash support
  2011-12-19 18:27     ` Jordan Justen
@ 2011-12-20 10:53       ` Avi Kivity
  0 siblings, 0 replies; 15+ messages in thread
From: Avi Kivity @ 2011-12-20 10:53 UTC (permalink / raw)
  To: Jordan Justen; +Cc: qemu-devel

On 12/19/2011 08:27 PM, Jordan Justen wrote:
> On Sun, Dec 18, 2011 at 02:04, Avi Kivity <avi@redhat.com> wrote:
> > On 12/15/2011 11:02 PM, Jordan Justen wrote:
> >> I verified that 'info mtree' and 'info qdev' are equivalent for pc-1.0
> >> when using master and with my patches.
> >>
> >> However, I did discover that v1.0 seems to differ from master for this
> >> same test.
> >>
> >
> > That's are fine; pre-memory-API devices don't show up in info mtree.
> >
> >> @@ -218,7 +221,7 @@
> >>      dev-prop: data_iobase = 0x511
> >>      irq 0
> >>      mmio ffffffffffffffff/0000000000000002
> >> -    mmio ffffffffffffffff/0000000000000002
> >> +    mmio ffffffffffffffff/0000000000000001
> >>
> >
> > This one isn't nice.  Doesn't affect pc though.
>
> Thanks Avi.  Does this patch series now address your concerns about
> qtree/mtree and pc-1.0 with regards to the x86 flash feature?
>

It's hard to review, since you move a large function and edit it in the
same patch.  Usually we separate this into two patches so the changes
are visible.

The test however indicates that it's probably fine.

-- 
error compiling committee.c: too many arguments to function

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH v9 3/3] pc: Support system flash memory with pflash
  2011-12-19 22:19       ` Anthony Liguori
@ 2012-01-09  9:28         ` Kevin Wolf
  2012-01-28 23:13           ` Jordan Justen
  2012-02-20  0:39           ` Jordan Justen
  0 siblings, 2 replies; 15+ messages in thread
From: Kevin Wolf @ 2012-01-09  9:28 UTC (permalink / raw)
  To: Anthony Liguori
  Cc: Jordan Justen, Anthony Liguori, Jordan Justen, qemu-devel,
	Stefan Hajnoczi

Am 19.12.2011 23:19, schrieb Anthony Liguori:
> On 12/19/2011 03:25 PM, Jordan Justen wrote:
>> On Mon, Dec 19, 2011 at 11:41, Anthony Liguori<aliguori@us.ibm.com>  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<jordan.l.justen@intel.com>
>>>> Cc: Anthony Liguori<aliguori@us.ibm.com>
>>>> ---
>>>>   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?
>>
>> I was hoping pc-1.1 with and without kvm could be as close as
>> possible, but I guess I can make pc-1.1 with kvm behave the same as
>> pc-1.0.  Then I can delete pc_system_rom_init.
> 
> I think your general approach is right, I'm just not sure what we're going to do 
> short term about synchronous I/O in the machine init routines.  It may just be a 
> matter of structuring this in such a way that you can use an async interface.

I don't think there is a problem with using the synchronous interface
here. But I'm not sure if having any I/O in the machine init is a good
idea with respect to migration. Didn't we already have to move some code
there to a later stage to make sure that the destination doesn't use
outdated data?

Kevin

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH v9 3/3] pc: Support system flash memory with pflash
  2012-01-09  9:28         ` Kevin Wolf
@ 2012-01-28 23:13           ` Jordan Justen
  2012-02-20  0:39           ` Jordan Justen
  1 sibling, 0 replies; 15+ messages in thread
From: Jordan Justen @ 2012-01-28 23:13 UTC (permalink / raw)
  To: Kevin Wolf, Anthony Liguori, Anthony Liguori
  Cc: Jordan Justen, qemu-devel, Stefan Hajnoczi

On Mon, Jan 9, 2012 at 01:28, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 19.12.2011 23:19, schrieb Anthony Liguori:
>> On 12/19/2011 03:25 PM, Jordan Justen wrote:
>>> On Mon, Dec 19, 2011 at 11:41, Anthony Liguori<aliguori@us.ibm.com>  wrote:
>>>> On 12/15/2011 02:51 PM, Jordan Justen wrote:
>>>>> +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?
>>>
>>> I was hoping pc-1.1 with and without kvm could be as close as
>>> possible, but I guess I can make pc-1.1 with kvm behave the same as
>>> pc-1.0.  Then I can delete pc_system_rom_init.
>>
>> I think your general approach is right, I'm just not sure what we're going to do
>> short term about synchronous I/O in the machine init routines.  It may just be a
>> matter of structuring this in such a way that you can use an async interface.
>
> I don't think there is a problem with using the synchronous interface
> here. But I'm not sure if having any I/O in the machine init is a good
> idea with respect to migration. Didn't we already have to move some code
> there to a later stage to make sure that the destination doesn't use
> outdated data?

The pc_system_rom_init function is only used when kvm is enabled.
Since kvm cannot currently execute code from device memory, we have to
create a ram region, and transfer the pflash contents into the ram
region.

Instead of bdrv_read, should I get the pflash memory region buffer,
and use memcpy?  Or, would still suffer from the same issue of getting
outdated data?

Thanks,

-Jordan

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH v9 3/3] pc: Support system flash memory with pflash
  2012-01-09  9:28         ` Kevin Wolf
  2012-01-28 23:13           ` Jordan Justen
@ 2012-02-20  0:39           ` Jordan Justen
  1 sibling, 0 replies; 15+ messages in thread
From: Jordan Justen @ 2012-02-20  0:39 UTC (permalink / raw)
  To: Kevin Wolf, Anthony Liguori
  Cc: Jordan Justen, Anthony Liguori, qemu-devel, Stefan Hajnoczi

On Mon, Jan 9, 2012 at 01:28, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 19.12.2011 23:19, schrieb Anthony Liguori:
>> On 12/19/2011 03:25 PM, Jordan Justen wrote:
>>> On Mon, Dec 19, 2011 at 11:41, Anthony Liguori<aliguori@us.ibm.com>  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<jordan.l.justen@intel.com>
>>>>> Cc: Anthony Liguori<aliguori@us.ibm.com>
>>>>> ---
>>>>>   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?
>>>
>>> I was hoping pc-1.1 with and without kvm could be as close as
>>> possible, but I guess I can make pc-1.1 with kvm behave the same as
>>> pc-1.0.  Then I can delete pc_system_rom_init.
>>
>> I think your general approach is right, I'm just not sure what we're going to do
>> short term about synchronous I/O in the machine init routines.  It may just be a
>> matter of structuring this in such a way that you can use an async interface.
>
> I don't think there is a problem with using the synchronous interface
> here. But I'm not sure if having any I/O in the machine init is a good
> idea with respect to migration. Didn't we already have to move some code
> there to a later stage to make sure that the destination doesn't use
> outdated data?

Since the bdrv_read call seems to be the concern here, I noticed that
in hw/pflash_cfi01.c pflash_cfi01_register there is a call to
bdrv_read.  So, it seems like it is not possible to initialize a
pflash/cfi device without a bdrv_read call being made.

So, what should pflash/cfi be doing instead?  Should these devices
somehow hold off on initializing their contents from the drive until a
later stage?  If so, is there a later call into the devices before the
firmware is launched that could be used for this later initialization
step?

-Jordan

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2012-02-20  0:39 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-12-15 20:51 [Qemu-devel] [PATCH v9 0/3] PC system flash support Jordan Justen
2011-12-15 20:51 ` [Qemu-devel] [PATCH v9 1/3] pc: Add pc-1.1 machine type Jordan Justen
2011-12-15 20:51 ` [Qemu-devel] [PATCH v9 2/3] pflash: Support read-only mode Jordan Justen
2011-12-15 20:51 ` [Qemu-devel] [PATCH v9 3/3] pc: Support system flash memory with pflash Jordan Justen
2011-12-19 19:41   ` Anthony Liguori
2011-12-19 21:25     ` Jordan Justen
2011-12-19 22:19       ` Anthony Liguori
2012-01-09  9:28         ` Kevin Wolf
2012-01-28 23:13           ` Jordan Justen
2012-02-20  0:39           ` Jordan Justen
2011-12-20  8:11     ` Stefan Hajnoczi
2011-12-15 21:02 ` [Qemu-devel] [PATCH v9 0/3] PC system flash support Jordan Justen
2011-12-18 10:04   ` Avi Kivity
2011-12-19 18:27     ` Jordan Justen
2011-12-20 10:53       ` Avi Kivity

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.