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

[Markus Armbruster]

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");

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?

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