Re: [Qemu-devel] [qemu PATCH] hw/i386/pc_sysfw: support more than one flash drive

[Markus Armbruster]

Laszlo Ersek <address@hidden> writes:

> On 11/25/13 16:32, Markus Armbruster wrote:
>> Laszlo Ersek <address@hidden> 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 <address@hidden>
>>> ---
>>>  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");

You're just following existing bad practice here, but correct use of
error_report() would give you nicer messages.  Happy to explain if
you're interested.

>> 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.

If the caller knows the true limit, it could pass it.

If the true limit isn't practical to find, then what about a comment
explaining the problem?

>>> +            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).

Yes, it's pretty stupid, but n is small, and the code runs only during
initialization.

> 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".

Legacy drives and elegance are about as attracted to each other as oil
and water.

> 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.

Yes, that would work, but requires a bit of new blockdev infrastructure.

> 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.

I'm partial to for-loops that let me see the complete loop control in
one glance:

    for (unit = 0; drv = drive_get(IF_PFLASH, 0, unit); unit++)