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

[Laszlo Ersek]

On 11/26/13 14:11, Markus Armbruster wrote:
> 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.

You have the Location thing in mind, eg. automatically binding the error
message to the offending command line option, right?

I've seen you use it before in another patch. Hm... It was commit
ec2df8c10a4585ba4641ae482cf2f5f13daa810e.

I will try to address the rest of your comments too when I get back to
this patch.

Thanks
Laszlo

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