Re: [Qemu-devel] [PATCH v2] hw/i386/pc_sysfw: support two flash drives

[Laszlo Ersek <lersek@redhat.com>]

On 12/03/13 18:23, Markus Armbruster wrote:
> Laszlo Ersek <lersek@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, pflash_update() is never called; various flash
>> programming/erase errors are returned to the guest instead. See the
>> callers of pflash_update(), and the initialization of "pfl->ro", in
>> "hw/block/pflash_cfi01.c".)
>>
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>  v2:
>>  - don't map flash devices beyond unit#1 [Markus]
>>  - explicit low bound on cumulative base address [Markus]
>>  - updated comment block on pc_system_flash_init() [Laszlo]
>>  - dropped (DriveInfo*) param of pc_system_flash_init(), use drive_get()
>>    internally for unit#0 too [Markus]
>>  - turn do-loop into for-loop [Markus]
>>  - check bdrv_getlength() for errors [Laszlo]
>>  - reject zero-sized flash [Laszlo]
>>  - use Location of -pflash / -drive if=pflash,... option in error
>>    reporting [Markus]
>>  - describe the real spots where write attempts to r/o flash are caught
>>    [Laszlo]
>>
>>  hw/i386/pc_sysfw.c | 105 +++++++++++++++++++++++++++++++++++++++++++----------
>>  1 file changed, 86 insertions(+), 19 deletions(-)
>>
>> diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
>> index e917c83..75a7ebb 100644
>> --- a/hw/i386/pc_sysfw.c
>> +++ b/hw/i386/pc_sysfw.c
>> @@ -72,35 +72,102 @@ static void pc_isa_bios_init(MemoryRegion *rom_memory,
>>      memory_region_set_readonly(isa_bios, true);
>>  }
>>  
>> -static void pc_system_flash_init(MemoryRegion *rom_memory,
>> -                                 DriveInfo *pflash_drv)
>> +#define FLASH_MAP_UNIT_MAX 2
>> +
>> +/* We don't have a theoretically justifiable exact lower bound on the base
>> + * address of any flash mapping. In practice, the IO-APIC MMIO range is
>> + * [0xFEE00000..0xFEE01000[ -- see IO_APIC_DEFAULT_ADDRESS --, leaving free
>> + * only 18MB-4KB below 4G. For now, restrict the cumulative mapping to 8MB in
>> + * size.
>> + */
>> +#define FLASH_MAP_BASE_MIN ((hwaddr)(0x100000000ULL - 8*1024*1024))
>> +
>> +/* This function maps flash drives from 4G downward, in order of their unit
>> + * numbers. The mapping starts at unit#0, with unit number increments of 1, and
>> + * stops before the first missing flash drive, or before
>> + * unit#FLASH_MAP_UNIT_MAX, whichever is reached first.
>> + *
>> + * Addressing within one flash drive is of course not reversed.
>> + *
>> + * An error message is printed and the process exits if:
>> + * - the size of the backing file for a flash drive is non-positive, or not a
>> + *   multiple of the required sector size, or
>> + * - the current mapping's base address would fall below FLASH_MAP_BASE_MIN.
>> + *
>> + * The drive with unit#0 (if available) is mapped at the highest address, and
>> + * it is passed to pc_isa_bios_init(). Merging several drives for isa-bios is
>> + * not supported.
>> + */
>> +static void pc_system_flash_init(MemoryRegion *rom_memory)
>>  {
>> +    int unit;
>> +    DriveInfo *pflash_drv;
>>      BlockDriverState *bdrv;
>>      int64_t size;
>> -    hwaddr phys_addr;
>> +    char *fatal_errmsg = NULL;
>> +    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);
>> +    for (unit = 0;
>> +         (unit < FLASH_MAP_UNIT_MAX &&
>> +          (pflash_drv = drive_get(IF_PFLASH, 0, unit)) != NULL);
>> +         ++unit) {
>> +        bdrv = pflash_drv->bdrv;
> 
> Used just twice, sure it's worth a variable?  Your choice, of course.

I sought to keep the initial block of definitions intact as much as I
could. Personally I would have wanted to kill off most of those, and
(re)introduce anything I'd need in the narrowest scope possible. But, I
remembered that you hate narrow scope declarations and like to keep
everything at the top of the function, so I didn't mess with that block
precisely to keep you happy.

:)

> 
>> +        size = bdrv_getlength(bdrv);
>> +        if (size < 0) {
>> +            fatal_errmsg = g_strdup_printf("failed to get backing file size");
>> +        } else if (size == 0) {
>> +            fatal_errmsg = g_strdup_printf("PC system firmware (pflash) "
>> +                               "cannot have zero size");
>> +        } else if ((size % sector_size) != 0) {
>> +            fatal_errmsg = g_strdup_printf("PC system firmware (pflash) "
>> +                               "must be a multiple of 0x%x", sector_size);
>> +        } else if (phys_addr < size || phys_addr - size < FLASH_MAP_BASE_MIN) {
>> +            fatal_errmsg = g_strdup_printf("oversized backing file, pflash "
>> +                               "segments cannot be mapped under "
> 
> "mapped below "?

Too much finesse, can't care about everything! :)

> 
>> +                               TARGET_FMT_plx, FLASH_MAP_BASE_MIN);
>> +        }
>> +        if (fatal_errmsg != NULL) {
>> +            Location loc;
>> +
>> +            /* push a new, "none" location on the location stack; overwrite its
>> +             * contents with the location saved in the option; print the error
>> +             * (includes location); pop the top
>> +             */
> 
> Sure this comment is worthwhile?  Your decision.

It shows you that I did my homework on the Location thing.

More importantly, it shows the casual reader what's going on.
Considering that I found exactly one other such pattern in the entire
source, I think we can qualify all developers "casual readers" in this
regard. I myself would have been greatly helped by such a comment
(anywhere), so I added it.


> 
> I suspect we should have error_report_loc().

I was happy that I managed to use the existing functions correctly.
Please feel free to introduce the new function and refactor this call
site :)

> 
>> +            loc_push_none(&loc);
>> +            if (pflash_drv->opts != NULL) {
>> +                qemu_opts_loc_restore(pflash_drv->opts);
>> +            }
>> +            error_report("%s", fatal_errmsg);
>> +            loc_pop(&loc);
>> +            g_free(fatal_errmsg);
>> +            exit(1);
> 
> You hoist the reporting of a fatal error out of the conditionals.  If we
> had error_report_loc(), we could leave it there.  But we don't.

Correct, I do that, with a smile on my face. :)

> 
>> +        }
>> +
>> +        phys_addr -= 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,
> 
> pflash_cfi01_register() doesn't use its second parameter, and all
> callers pass NULL, ugh.

I looked too.

> 
> I looked because I don't like nor trust /* name of parameter */
> comments.  Matter of taste.

I don't care for such in-call comments, but I saw MST use them, so I
thought what the heck.


> 
>> +                                             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);
>> +        }
>>      }
>> -
>> -    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);
>> -
>> -    pc_isa_bios_init(rom_memory, flash_mem, size);
>>  }
>>  
>>  static void old_pc_system_rom_init(MemoryRegion *rom_memory, bool isapc_ram_fw)
>> @@ -181,5 +248,5 @@ void pc_system_firmware_init(MemoryRegion *rom_memory, bool isapc_ram_fw)
>>          exit(1);
>>      }
>>  
>> -    pc_system_flash_init(rom_memory, pflash_drv);
>> +    pc_system_flash_init(rom_memory);
>>  }
> 
> Nothing but nits, therefore
> 
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Why thank you! :)

> 
> Still pining for that star destroyer, though... ;)
> 

I'm pining for 36-hour days! :)

Thanks!
Laszlo