From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:39209) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QkjhB-00077X-G0 for qemu-devel@nongnu.org; Sat, 23 Jul 2011 17:25:14 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Qkjh9-0001Ly-Ua for qemu-devel@nongnu.org; Sat, 23 Jul 2011 17:25:13 -0400 Received: from mail-pz0-f43.google.com ([209.85.210.43]:65287) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qkjh9-0001Lu-LR for qemu-devel@nongnu.org; Sat, 23 Jul 2011 17:25:11 -0400 Received: by pzk1 with SMTP id 1so6100087pzk.30 for ; Sat, 23 Jul 2011 14:25:10 -0700 (PDT) Message-ID: <4E2B3C33.3050400@codemonkey.ws> Date: Sat, 23 Jul 2011 16:25:07 -0500 From: Anthony Liguori MIME-Version: 1.0 References: <1310153845-4373-1-git-send-email-jordan.l.justen@intel.com> <4E2AEE1E.8030904@codemonkey.ws> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v4] hw/pc: Support system flash memory with -pflash parameter List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jordan Justen Cc: Kevin Wolf , Jordan Justen , qemu-devel@nongnu.org On 07/23/2011 03:19 PM, Jordan Justen wrote: > On Sat, Jul 23, 2011 at 08:51, Anthony Liguori wrote: >> On 07/08/2011 02:37 PM, Jordan Justen wrote: >>> >>> If -pflash is specified and -bios is specified then pflash will >>> be mapped just below the system rom using hw/pflash_cfi01.c. >>> >>> If -pflash is specified on the command line, but -bios is >>> not specified, then 'bios.bin' will NOT be loaded, and >>> instead the -pflash flash image will be mapped just below >>> 4GB in place of the normal rom image. >> >> This is way too tied to the pc platform to be this generic. >> >> I think a better approach would be to default to having unit=0 of IF_PFLASH >> default to a read-only BDS that points to bios.bin. -bios would just be a >> short cut to use a different file name but you should be able to override >> with -drive too. >> >> And to really simplify things, you could add a readonly flag to -bios such >> that you could do: >> >> -bios foo.img,readonly=off >> >> Which is what I think you're looking for semantically. > > There seemed to be some feedback on the list interested in preserving > a read-only firmware, and just adding a flash region. > > So, for example, the firmware could be read from a common system > location like is generally done with bios.bin today, and VM instance > specific flash region could still be added. You can have multiple flash regions. You're introducing two modes. In one mode, we emulate a flash device and expose it for the BIOS ROM. In the second mode, we don't emulate a device but we expose the BIOS ROM based on a file in a shared read-only location. I'm suggesting always emulating a flash device, but by default make the device read-only and have it be loaded from a file in a shared read-only location. That means we have a single code path and a consistent view from a management tooling perspective. IOW, management tools will always see that there is a BIOS block device, and they need to worry about making sure that BIOS block device is there. > > If the entire firmware is moved to a separate VM instance specific > flash, then firmware update also gets complicated. It is no longer > just a matter of updating the qemu firmware package in your distro's > package management system. I think the bit your misunderstanding is that you should default the firmware to be created from a common file as a read-only device. Regards, Anthony Liguori > > What about taking your idea, but adding a second drive that would be > mapped just below the 1st if it is specified with -drive? > > Thanks, > > -Jordan > >> >> Regards, >> >> Anthony Liguori >> >>> >>> Signed-off-by: Jordan Justen >>> Reviewed-by: Aurelien Jarno >> >> >> >>> --- >>> default-configs/i386-softmmu.mak | 1 + >>> default-configs/x86_64-softmmu.mak | 1 + >>> hw/pc.c | 161 >>> +++++++++++++++++++++++++++--------- >>> 3 files changed, 125 insertions(+), 38 deletions(-) >>> >>> diff --git a/default-configs/i386-softmmu.mak >>> b/default-configs/i386-softmmu.mak >>> index 55589fa..8697cd4 100644 >>> --- a/default-configs/i386-softmmu.mak >>> +++ b/default-configs/i386-softmmu.mak >>> @@ -21,3 +21,4 @@ CONFIG_PIIX_PCI=y >>> CONFIG_SOUND=y >>> CONFIG_HPET=y >>> CONFIG_APPLESMC=y >>> +CONFIG_PFLASH_CFI01=y >>> diff --git a/default-configs/x86_64-softmmu.mak >>> b/default-configs/x86_64-softmmu.mak >>> index 8895028..eca9284 100644 >>> --- a/default-configs/x86_64-softmmu.mak >>> +++ b/default-configs/x86_64-softmmu.mak >>> @@ -21,3 +21,4 @@ CONFIG_PIIX_PCI=y >>> CONFIG_SOUND=y >>> CONFIG_HPET=y >>> CONFIG_APPLESMC=y >>> +CONFIG_PFLASH_CFI01=y >>> diff --git a/hw/pc.c b/hw/pc.c >>> index a3e8539..e25354f 100644 >>> --- a/hw/pc.c >>> +++ b/hw/pc.c >>> @@ -41,6 +41,7 @@ >>> #include "sysemu.h" >>> #include "blockdev.h" >>> #include "ui/qemu-spice.h" >>> +#include "flash.h" >>> >>> /* output Bochs bios info messages */ >>> //#define DEBUG_BIOS >>> @@ -957,70 +958,154 @@ void pc_cpus_init(const char *cpu_model) >>> } >>> } >>> >>> -void pc_memory_init(const char *kernel_filename, >>> - const char *kernel_cmdline, >>> - const char *initrd_filename, >>> - ram_addr_t below_4g_mem_size, >>> - ram_addr_t above_4g_mem_size) >>> +static void pc_isa_bios_init(ram_addr_t ram_offset, int ram_size) >>> { >>> - char *filename; >>> - int ret, linux_boot, i; >>> - ram_addr_t ram_addr, bios_offset, option_rom_offset; >>> - int bios_size, isa_bios_size; >>> - void *fw_cfg; >>> - >>> - linux_boot = (kernel_filename != NULL); >>> + int isa_bios_size; >>> >>> - /* allocate RAM */ >>> - ram_addr = qemu_ram_alloc(NULL, "pc.ram", >>> - below_4g_mem_size + above_4g_mem_size); >>> - cpu_register_physical_memory(0, 0xa0000, ram_addr); >>> - cpu_register_physical_memory(0x100000, >>> - below_4g_mem_size - 0x100000, >>> - ram_addr + 0x100000); >>> - if (above_4g_mem_size> 0) { >>> - cpu_register_physical_memory(0x100000000ULL, above_4g_mem_size, >>> - ram_addr + below_4g_mem_size); >>> + /* map the last 128KB of the BIOS in ISA space */ >>> + isa_bios_size = ram_size; >>> + if (isa_bios_size> (128 * 1024)) { >>> + isa_bios_size = 128 * 1024; >>> } >>> + ram_offset = ram_offset + ram_size - isa_bios_size; >>> + cpu_register_physical_memory(0x100000 - isa_bios_size, >>> + isa_bios_size, >>> + ram_offset | IO_MEM_ROM); >>> +} >>> + >>> +static int pc_system_rom_init(void) >>> +{ >>> + int ret; >>> + int bios_size; >>> + ram_addr_t bios_offset; >>> + char *filename; >>> >>> /* BIOS load */ >>> - if (bios_name == NULL) >>> + 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; >>> + >>> + if (bios_size<= 0 || (bios_size % 65536) != 0) { >>> + ret = -1; >>> + } else { >>> + bios_offset = qemu_ram_alloc(NULL, "pc.bios", bios_size); >>> + ret = rom_add_file_fixed(bios_name, (uint32_t)(-bios_size), -1); >>> } >>> - bios_offset = qemu_ram_alloc(NULL, "pc.bios", bios_size); >>> - 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) { >>> qemu_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; >>> - cpu_register_physical_memory(0x100000 - isa_bios_size, >>> - isa_bios_size, >>> - (bios_offset + bios_size - >>> isa_bios_size) | IO_MEM_ROM); >>> >>> - option_rom_offset = qemu_ram_alloc(NULL, "pc.rom", PC_ROM_SIZE); >>> - cpu_register_physical_memory(PC_ROM_MIN_VGA, PC_ROM_SIZE, >>> option_rom_offset); >>> + pc_isa_bios_init(bios_offset, bios_size); >>> >>> /* map all the bios at the top of memory */ >>> cpu_register_physical_memory((uint32_t)(-bios_size), >>> bios_size, bios_offset | IO_MEM_ROM); >>> >>> + return bios_size; >>> +} >>> + >>> +static void pc_system_flash_init(DriveInfo *pflash_drv, int rom_size) >>> +{ >>> + BlockDriverState *bdrv; >>> + int64_t size; >>> + target_phys_addr_t phys_addr; >>> + ram_addr_t addr; >>> + int sector_bits, sector_size; >>> + >>> + bdrv = NULL; >>> + >>> + 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: -pflash size must be a multiple of 0x%x\n", >>> + sector_size); >>> + exit(1); >>> + } >>> + >>> + phys_addr = 0x100000000ULL - rom_size - size; >>> + addr = qemu_ram_alloc(NULL, "system.flash", size); >>> + DPRINTF("flash addr: 0x%lx\n", (int64_t)phys_addr); >>> + pflash_cfi01_register(phys_addr, addr, bdrv, >>> + sector_size, size>> sector_bits, >>> + 4, 0x0000, 0x0000, 0x0000, 0x0000, 0); >>> + >>> + if (rom_size == 0) { >>> + pc_isa_bios_init(addr, size); >>> + } >>> +} >>> + >>> +static void pc_system_firmware_init(void) >>> +{ >>> + int flash_present, rom_present; >>> + int rom_size; >>> + DriveInfo *pflash_drv; >>> + >>> + pflash_drv = drive_get(IF_PFLASH, 0, 0); >>> + flash_present = (pflash_drv != NULL); >>> + >>> + /* Load rom if -bios is used or if -pflash is not used */ >>> + rom_present = ((bios_name != NULL) || !flash_present); >>> + >>> + /* If rom is present, then it is mapped just below 4GB */ >>> + if (rom_present) { >>> + rom_size = pc_system_rom_init(); >>> + } else { >>> + rom_size = 0; >>> + } >>> + >>> + /* If flash is present, then it is mapped just below the rom, or >>> + * just below 4GB when rom is not present. */ >>> + if (flash_present) { >>> + pc_system_flash_init(pflash_drv, rom_size); >>> + } >>> +} >>> + >>> +void pc_memory_init(const char *kernel_filename, >>> + const char *kernel_cmdline, >>> + const char *initrd_filename, >>> + ram_addr_t below_4g_mem_size, >>> + ram_addr_t above_4g_mem_size) >>> +{ >>> + int linux_boot, i; >>> + ram_addr_t ram_addr, option_rom_offset; >>> + void *fw_cfg; >>> + >>> + linux_boot = (kernel_filename != NULL); >>> + >>> + /* allocate RAM */ >>> + ram_addr = qemu_ram_alloc(NULL, "pc.ram", >>> + below_4g_mem_size + above_4g_mem_size); >>> + cpu_register_physical_memory(0, 0xa0000, ram_addr); >>> + cpu_register_physical_memory(0x100000, >>> + below_4g_mem_size - 0x100000, >>> + ram_addr + 0x100000); >>> + if (above_4g_mem_size> 0) { >>> + cpu_register_physical_memory(0x100000000ULL, above_4g_mem_size, >>> + ram_addr + below_4g_mem_size); >>> + } >>> + >>> + /* Initialize ROM or flash ranges for PC firmware */ >>> + pc_system_firmware_init(); >>> + >>> + option_rom_offset = qemu_ram_alloc(NULL, "pc.rom", PC_ROM_SIZE); >>> + cpu_register_physical_memory(PC_ROM_MIN_VGA, PC_ROM_SIZE, >>> option_rom_offset); >>> + >>> fw_cfg = bochs_bios_init(); >>> rom_set_fw(fw_cfg); >>> >> >> >> >