From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:42148) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RclY4-0000pi-SI for qemu-devel@nongnu.org; Mon, 19 Dec 2011 17:19:10 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RclY3-0005u9-7q for qemu-devel@nongnu.org; Mon, 19 Dec 2011 17:19:08 -0500 Received: from mail-iy0-f173.google.com ([209.85.210.173]:40804) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RclY3-0005u5-1d for qemu-devel@nongnu.org; Mon, 19 Dec 2011 17:19:07 -0500 Received: by iagj37 with SMTP id j37so10374150iag.4 for ; Mon, 19 Dec 2011 14:19:06 -0800 (PST) Message-ID: <4EEFB856.6060401@codemonkey.ws> Date: Mon, 19 Dec 2011 16:19:02 -0600 From: Anthony Liguori MIME-Version: 1.0 References: <1323982273-13623-1-git-send-email-jordan.l.justen@intel.com> <1323982273-13623-4-git-send-email-jordan.l.justen@intel.com> <4EEF934F.8030907@us.ibm.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v9 3/3] pc: Support system flash memory with pflash List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jordan Justen Cc: Kevin Wolf , Jordan Justen , Anthony Liguori , qemu-devel@nongnu.org, Stefan Hajnoczi On 12/19/2011 03:25 PM, Jordan Justen wrote: > On Mon, Dec 19, 2011 at 11:41, 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 >>> Cc: Anthony Liguori >>> --- >>> 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 >