From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:58759) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qkkef-00065F-CM for qemu-devel@nongnu.org; Sat, 23 Jul 2011 18:26:42 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Qkkec-00026Y-J0 for qemu-devel@nongnu.org; Sat, 23 Jul 2011 18:26:41 -0400 Received: from mail-pz0-f43.google.com ([209.85.210.43]:50200) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qkkec-00022T-9Y for qemu-devel@nongnu.org; Sat, 23 Jul 2011 18:26:38 -0400 Received: by pzk1 with SMTP id 1so6164551pzk.30 for ; Sat, 23 Jul 2011 15:26:37 -0700 (PDT) Message-ID: <4E2B4A9A.5080005@codemonkey.ws> Date: Sat, 23 Jul 2011 17:26:34 -0500 From: Anthony Liguori MIME-Version: 1.0 References: <1310153845-4373-1-git-send-email-jordan.l.justen@intel.com> <4E2AEE1E.8030904@codemonkey.ws> <4E2B3C33.3050400@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 05:06 PM, Jordan Justen wrote: > On Sat, Jul 23, 2011 at 14:25, Anthony Liguori wrote: >> 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. > > So, is your recommendation that we support N pflash images in > x86/x86-64? Instance/index 0 is mapped just under 4GB, and the rest > follow below this? No. There should be a flash device, pflash index 0 is fine, but it should be mapped under 4GB and also in the legacy BIOS space. This is the PC firmware flash. By default it should be read-only and it should be created by using ${prefix}/share/bios.bin. But it should be possible to override both the filename and the read-only flag. In terms of other flash devices, I don't think it's that simple. Flash is tied to the mobo layout so I don't think index > 0 really makes sense unless you allow a specific mapping address. I doubt that's terribly useful. Regards, Anthony Liguori > > This seems like a good plan, although I can't see a usage for more > than 2 instances. > > -Jordan > >> 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); >>>>> >>>> >>>> >>>> >>> >> >> >