Re: [Qemu-devel] [PATCH v3 11/12] pc: Support firmware configuration with -blockdev

[Paolo Bonzini <pbonzini@redhat.com>]

On 08/03/19 14:14, Markus Armbruster wrote:
> The PC machines put firmware in ROM by default.  To get it put into
> flash memory (required by OVMF), you have to use -drive
> if=pflash,unit=0,... and optionally -drive if=pflash,unit=1,...
> 
> Why two -drive?  This permits setting up one part of the flash memory
> read-only, and the other part read/write.  Below the hood, it creates
> two separate flash devices, because we were too lazy to improve our
> flash device models to support sector protection.

Not just that, it avoids the need to "upgrade the firmware" of old
virtual machines.  Instead, you just need to upgrade the files on the
host and restart QEMU, just like with non-UEFI firmware.

> This is actually an instance of a wider problem: our general device
> configuration interface doesn't cover onboard devices.  Instead, we
> have a zoo of ad hoc interfaces that are much more limited.  Some of
> them we'd rather deprecate (-drive, -net), but can't until we have
> suitable replacements.

-net already has a better replacement, -nic, which is basically "like
-netdev but also tell board creation code about it".  I suppose you'd
prefer that it were also accessible as "-netdev ...,id=foo -machine
net0=foo"?

> +static void pc_system_flash_cleanup_unused(PCMachineState *pcms)
> +{
> +    char *prop_name;
> +    int i;
> +    Object *dev_obj;
> +
> +    assert(PC_MACHINE_GET_CLASS(pcms)->pci_enabled);
> +
> +    for (i = 0; i < ARRAY_SIZE(pcms->flash); i++) {
> +        dev_obj = OBJECT(pcms->flash[i]);
> +        if (!object_property_get_bool(dev_obj, "realized", &error_abort)) {
> +            prop_name = g_strdup_printf("pflash%d", i);
> +            object_property_del(OBJECT(pcms), prop_name, &error_abort);

Why didn't this already call object->class->unparent?

> +            g_free(prop_name);
> +            /*
> +             * Delete @dev_obj.  Straight object_unref() is wrong,
> +             * since it leaves dangling references in the parent bus
> +             * behind.  object_unparent() doesn't work, either: since
> +             * @dev_obj hasn't been realized, dev_obj->parent is null,
> +             * and object_unparent() does nothing.

Does it work if you add the device yourself as a child of /machine,
instead of relying on /machine/unattached?

>   DeviceClass method
> +             * device_unparent() works, but we have to take a
> +             * temporary reference, or else device_unparent() commits
> +             * a use-after-free error.
> +             */
> +            object_ref(dev_obj);
> +            object_get_class(dev_obj)->unparent(dev_obj);
> +            object_unref(dev_obj);
> +            pcms->flash[i] = NULL;
> +        }

Paolo

> +    }
> +}
> +
> +/*
> + * Map the pcms->flash[] from 4GiB downward, and realize.
> + * Map them in descending order, i.e. pcms->flash[0] at the top,
> + * without gaps.
> + * Stop at the first pcms->flash[0] lacking a block backend.
> + * Set each flash's size from its block backend.  Fatal error if the
> + * size isn't a non-zero multiple of 4KiB, or the total size exceeds
> + * FLASH_SIZE_LIMIT.
>   *
> - * 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
> + * If pcms->flash[0] has a block backend, its memory is passed to
> + * pc_isa_bios_init().  Merging several flash devices for isa-bios is
>   * not supported.
>   */
> -static void pc_system_flash_init(MemoryRegion *rom_memory)
> +static void pc_system_flash_map(PCMachineState *pcms,
> +                                MemoryRegion *rom_memory)
>  {
> -    int unit;
> -    DriveInfo *pflash_drv;
> +    hwaddr total_size = 0;
> +    int i;
>      BlockBackend *blk;
>      int64_t size;
> -    char *fatal_errmsg = NULL;
> -    hwaddr phys_addr = 0x100000000ULL;
> -    uint32_t sector_size = 4096;
>      PFlashCFI01 *system_flash;
>      MemoryRegion *flash_mem;
> -    char name[64];
>      void *flash_ptr;
>      int ret, flash_size;
>  
> -    for (unit = 0;
> -         (unit < FLASH_MAP_UNIT_MAX &&
> -          (pflash_drv = drive_get(IF_PFLASH, 0, unit)) != NULL);
> -         ++unit) {
> -        blk = blk_by_legacy_dinfo(pflash_drv);
> +    assert(PC_MACHINE_GET_CLASS(pcms)->pci_enabled);
> +
> +    for (i = 0; i < ARRAY_SIZE(pcms->flash); i++) {
> +        system_flash = pcms->flash[i];
> +        blk = pflash_cfi01_get_blk(system_flash);
> +        if (!blk) {
> +            break;
> +        }
>          size = blk_getlength(blk);
>          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 "
> -                               TARGET_FMT_plx, FLASH_MAP_BASE_MIN);
> +            error_report("can't get size of block device %s: %s",
> +                         blk_name(blk), strerror(-size));
> +            exit(1);
>          }
> -        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
> -             */
> -            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);
> +        if (size == 0 || size % FLASH_SECTOR_SIZE != 0) {
> +            error_report("system firmware block device %s has invalid size "
> +                         "%" PRId64,
> +                         blk_name(blk), size);
> +            info_report("its size must be a non-zero multiple of 0x%x",
> +                        FLASH_SECTOR_SIZE);
> +            exit(1);
> +        }
> +        if ((hwaddr)size != size
> +            || total_size > HWADDR_MAX - size
> +            || total_size + size > FLASH_SIZE_LIMIT) {
> +            error_report("combined size of system firmware exceeds "
> +                         "%" PRIu64 " bytes",
> +                         FLASH_SIZE_LIMIT);
>              exit(1);
>          }
>  
> -        phys_addr -= size;
> +        total_size += size;
> +        qdev_prop_set_uint32(DEVICE(system_flash), "num-blocks",
> +                             size / FLASH_SECTOR_SIZE);
> +        qdev_init_nofail(DEVICE(system_flash));
> +        sysbus_mmio_map(SYS_BUS_DEVICE(system_flash), 0,
> +                        0x100000000ULL - total_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, name,
> -                                             size, blk, sector_size,
> -                                             1      /* width */,
> -                                             0x0000 /* id0 */,
> -                                             0x0000 /* id1 */,
> -                                             0x0000 /* id2 */,
> -                                             0x0000 /* id3 */,
> -                                             0      /* be */);
> -        if (unit == 0) {
> +        if (i == 0) {
>              flash_mem = pflash_cfi01_get_memory(system_flash);
>              pc_isa_bios_init(rom_memory, flash_mem, size);
>  
> @@ -235,23 +278,59 @@ void pc_system_firmware_init(PCMachineState *pcms,
>                               MemoryRegion *rom_memory)
>  {
>      PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
> -    bool isapc_ram_fw = !pcmc->pci_enabled;
> +    int i;
>      DriveInfo *pflash_drv;
> +    BlockBackend *pflash_blk[ARRAY_SIZE(pcms->flash)];
> +    Location loc;
>  
> -    pflash_drv = drive_get(IF_PFLASH, 0, 0);
> -
> -    if (isapc_ram_fw || pflash_drv == NULL) {
> -        /* When a pflash drive is not found, use rom-mode */
> -        old_pc_system_rom_init(rom_memory, isapc_ram_fw);
> +    if (!pcmc->pci_enabled) {
> +        old_pc_system_rom_init(rom_memory, true);
>          return;
>      }
>  
> -    if (kvm_enabled() && !kvm_readonly_mem_enabled()) {
> -        /* Older KVM cannot execute from device memory. So, flash memory
> -         * cannot be used unless the readonly memory kvm capability is present. */
> -        fprintf(stderr, "qemu: pflash with kvm requires KVM readonly memory support\n");
> -        exit(1);
> +    /* Map legacy -drive if=pflash to machine properties */
> +    for (i = 0; i < ARRAY_SIZE(pcms->flash); i++) {
> +        pflash_blk[i] = pflash_cfi01_get_blk(pcms->flash[i]);
> +        pflash_drv = drive_get(IF_PFLASH, 0, i);
> +        if (!pflash_drv) {
> +            continue;
> +        }
> +        loc_push_none(&loc);
> +        qemu_opts_loc_restore(pflash_drv->opts);
> +        if (pflash_blk[i]) {
> +            error_report("clashes with -machine");
> +            exit(1);
> +        }
> +        pflash_blk[i] = blk_by_legacy_dinfo(pflash_drv);
> +        qdev_prop_set_drive(DEVICE(pcms->flash[i]),
> +                            "drive", pflash_blk[i], &error_fatal);
> +        loc_pop(&loc);
>      }
>  
> -    pc_system_flash_init(rom_memory);
> +    /* Reject gaps */
> +    for (i = 1; i < ARRAY_SIZE(pcms->flash); i++) {
> +        if (pflash_blk[i] && !pflash_blk[i - 1]) {
> +            error_report("pflash%d requires pflash%d", i, i - 1);
> +            exit(1);
> +        }
> +    }
> +
> +    if (!pflash_blk[0]) {
> +        /* Machine property pflash0 not set, use ROM mode */
> +        old_pc_system_rom_init(rom_memory, false);
> +    } else {
> +        if (kvm_enabled() && !kvm_readonly_mem_enabled()) {
> +            /*
> +             * Older KVM cannot execute from device memory. So, flash
> +             * memory cannot be used unless the readonly memory kvm
> +             * capability is present.
> +             */
> +            error_report("pflash with kvm requires KVM readonly memory support");
> +            exit(1);
> +        }
> +
> +        pc_system_flash_map(pcms, rom_memory);
> +    }
> +
> +    pc_system_flash_cleanup_unused(pcms);
>  }
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 4f5ed7cefc..276ff15d4d 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -6,6 +6,7 @@
>  #include "hw/boards.h"
>  #include "hw/isa/isa.h"
>  #include "hw/block/fdc.h"
> +#include "hw/block/flash.h"
>  #include "net/net.h"
>  #include "hw/i386/ioapic.h"
>  
> @@ -39,6 +40,7 @@ struct PCMachineState {
>      PCIBus *bus;
>      FWCfgState *fw_cfg;
>      qemu_irq *gsi;
> +    PFlashCFI01 *flash[2];
>  
>      /* Configuration options: */
>      uint64_t max_ram_below_4g;
> @@ -277,6 +279,7 @@ extern PCIDevice *piix4_dev;
>  int piix4_init(PCIBus *bus, ISABus **isa_bus, int devfn);
>  
>  /* pc_sysfw.c */
> +void pc_system_flash_create(PCMachineState *pcms);
>  void pc_system_firmware_init(PCMachineState *pcms, MemoryRegion *rom_memory);
>  
>  /* acpi-build.c */
>