On 11/25/13 16:32, Markus Armbruster wrote:
> Laszlo Ersek <ler...@redhat.com> writes:
>
>> This patch allows the user to usefully specify
>>
>> -drive file=img_1,if=pflash,format=raw,readonly \
>> -drive file=img_2,if=pflash,format=raw
>>
>> on the command line. The flash images will be mapped under 4G in their
>> reverse unit order -- that is, with their base addresses progressing
>> downwards, in increasing unit order.
>>
>> (The unit number increases with command line order if not explicitly
>> specified.)
>>
>> This accommodates the following use case: suppose that OVMF is split in
>> two parts, a writeable host file for non-volatile variable storage, and a
>> read-only part for bootstrap and decompressible executable code.
>>
>> The binary code part would be read-only, centrally managed on the host
>> system, and passed in as unit 0. The variable store would be writeable,
>> VM-specific, and passed in as unit 1.
>>
>> 00000000ffe00000-00000000ffe1ffff (prio 0, R-): system.flash1
>> 00000000ffe20000-00000000ffffffff (prio 0, R-): system.flash0
>>
>> (If the guest tries to write to the flash range that is backed by the
>> read-only drive, bdrv_write() in pflash_update() will correctly deny the
>> write with -EACCES, and pflash_update() won't care, which suits us well.)
>>
>> Signed-off-by: Laszlo Ersek <ler...@redhat.com>
>> ---
>> hw/i386/pc_sysfw.c | 60
>> ++++++++++++++++++++++++++++++++++++++++--------------
>> 1 file changed, 45 insertions(+), 15 deletions(-)
>>
>> diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
>> index e917c83..1c3e3d6 100644
>> --- a/hw/i386/pc_sysfw.c
>> +++ b/hw/i386/pc_sysfw.c
>> @@ -72,35 +72,65 @@ static void pc_isa_bios_init(MemoryRegion *rom_memory,
>> memory_region_set_readonly(isa_bios, true);
>> }
>>
>> +/* This function maps flash drives from 4G downward, in order of their unit
>> + * numbers. Addressing within one flash drive is of course not reversed.
>> + *
>> + * The drive with unit number 0 is mapped at the highest address, and it is
>> + * passed to pc_isa_bios_init(). Merging severral drives for isa-bios is not
>> + * supported.
>> + *
>> + * The caller is responsible to pass in the non-NULL @pflash_drv that
>> + * corresponds to the flash drive with unit number 0.
>> + */
>> static void pc_system_flash_init(MemoryRegion *rom_memory,
>> DriveInfo *pflash_drv)
>> {
>> + int unit = 0;
>> BlockDriverState *bdrv;
>> int64_t size;
>> - hwaddr phys_addr;
>> + hwaddr phys_addr = 0x100000000ULL;
>> int sector_bits, sector_size;
>> pflash_t *system_flash;
>> MemoryRegion *flash_mem;
>> + char name[64];
>>
>> - 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);
>> - }
>> + do {
>> + bdrv = pflash_drv->bdrv;
>> + size = bdrv_getlength(bdrv);
>> + if ((size % sector_size) != 0) {
>> + fprintf(stderr,
>> + "qemu: PC system firmware (pflash segment %d) must be a
>> "
>> + "multiple of 0x%x\n", unit, sector_size);
>> + exit(1);
>> + }
>> + if (size > phys_addr) {
>> + fprintf(stderr, "qemu: pflash segments must fit under 4G "
>> + "cumulatively\n");
>
> I suspect things go haywire long before you hit address zero.
>
> Note that both pc_piix.c and pc_q35.c leave a hole in RAM just below 4G.
> The former's hole starts at 0xe0000000, the latter's at 0xb0000000.
> Should that be the limit?
I wanted to do the bare minimal here, to catch obviously wrong backing
drives (like a 10G file). This is already more verification than what
the current code does.
I wouldn't mind more a specific check, but I don't want to suggest (with
more specific code) that it's actually *safe* to go down to the limit
that I'd put here.
For example, the IO-APIC mmio range is [0xFEE00000..0xFEE01000[, leaving
free 18MB-4KB just below 4G. (Of which the current OVMF, including
variable store, takes up 2MB.) Grep IO_APIC_DEFAULT_ADDRESS.
I just don't want to send the message "it's safe to go all the way down
there". Right now the responsibility is with the user (you can specify a
single pflash device that's 20MB in size even now), and I'd like to
stick with that.
I believe that
pflash_cfi01_register()
sysbus_mmio_map()
sysbus_mmio_map_common()
memory_region_add_subregion()
memory_region_add_subregion_common()
could, in theory, find a conflict at runtime (see the #if 0-surrounded
collision warning in memory_region_add_subregion_common()). But the
memory API doesn't consider such collisions hard errors, and no status
code is propagated to the caller.
So, if a saner / more reliable limit can be identified, I wouldn't mind
checking against that, but right now I know of no such sane / general
enough limit.
>
>> + 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);
>> + phys_addr -= size;
>>
>> - pc_isa_bios_init(rom_memory, flash_mem, size);
>> + /* pflash_cfi01_register() creates a deep copy of the name */
>> + snprintf(name, sizeof name, "system.flash%d", unit);
>> + system_flash = pflash_cfi01_register(phys_addr, NULL /* qdev */,
>> name,
>> + size, bdrv, sector_size,
>> + size >> sector_bits,
>> + 1 /* width */,
>> + 0x0000 /* id0 */,
>> + 0x0000 /* id1 */,
>> + 0x0000 /* id2 */,
>> + 0x0000 /* id3 */,
>> + 0 /* be */);
>> + if (unit == 0) {
>> + flash_mem = pflash_cfi01_get_memory(system_flash);
>> + pc_isa_bios_init(rom_memory, flash_mem, size);
>> + }
>> + pflash_drv = drive_get(IF_PFLASH, 0, ++unit);
>> + } while (pflash_drv != NULL);
>> }
>>
>> static void old_pc_system_rom_init(MemoryRegion *rom_memory, bool
>> isapc_ram_fw)
>
> The drive with index 0 is passed as parameter pflash_drv. The others
> the function gets itself. I find that ugly. Have you considered
> dropping the parameter?
I didn't like drive_get() to begin with. It always starts to scan the
drive list from the beginning, which makes the new loop in
pc_system_flash_init() O(n^2).
I was missing an interator-style interface for the drives, but I found
none, and I thought that iterating myself through them in O(n) (and
checking for the types) would break the current DriveInfo encapsulation.
So I kinda gave up on "elegance".
Ideally, what should be dropped is the "unit" local variable in
pc_system_flash_init(). The function should continue to take
"pflash_drv", which should however qualify as a pre-initialized
iterator. Then pc_system_flash_init() should traverse it until it runs out.
I can of course remove the parameter and start a "while" loop
(rather than a "do" loop) with drive_get(IF_PFLASH, 0, 0), if you
consider that an improvement.
Thanks!
Laszlo
