qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [PATCH v2 3/3] hw/arm/virt: Support firmware configurat


From: Laszlo Ersek
Subject: Re: [Qemu-block] [PATCH v2 3/3] hw/arm/virt: Support firmware configuration with -blockdev
Date: Tue, 16 Apr 2019 15:26:31 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1

comparing this with v1...

On 04/16/19 11:13, Markus Armbruster wrote:
> The ARM virt machines put firmware in flash memory.  To configure it,
> you 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.  It also makes upgrading
> firmware on the host easier.  Below the hood, we get two separate
> flash devices, because we were too lazy to improve our flash device
> models to support sector protection.
> 
> The problem at hand is to do the same with -blockdev somehow, as one
> more step towards deprecating -drive.
> 
> We recently solved this problem for x86 PC machines, in commit
> ebc29e1beab.  See the commit message for design rationale.
> 
> This commit solves it for ARM virt basically the same way: new machine
> properties pflash0, pflash1 forward to the onboard flash devices'
> properties.  Requires creating the onboard devices in the
> .instance_init() method virt_instance_init().  The existing code to
> pick up drives defined with -drive if=pflash is replaced by code to
> desugar into the machine properties.
> 
> There are a few behavioral differences, though:
> 
> * The flash devices are always present (x86: only present if
>   configured)
> 
> * Flash base addresses and sizes are fixed (x86: sizes depend on
>   images, mapped back to back below a fixed address)
> 
> * -bios configures contents of first pflash (x86: -bios configures ROM
>    contents)
> 
> * -bios is rejected when first pflash is also configured with -machine
>    pflash0=... (x86: bios is silently ignored then)
> 
> * -machine pflash1=... does not require -machine pflash0=... (x86: it
>    does).
> 
> The actual code is a bit simpler than for x86 mostly due to the first
> two differences.
> 
> Before the patch, all the action is in create_flash(), called from the
> machine's .init() method machvirt_init():
> 
>     main()
>         machine_run_board_init()
>             machvirt_init()
>                 create_flash()
>                     create_one_flash() for flash[0]
>                         create
>                         configure
>                             includes obeying -drive if=pflash,unit=0
>                         realize
>                         map
>                         fall back to -bios
>                     create_one_flash() for flash[1]
>                         create
>                         configure
>                             includes obeying -drive if=pflash,unit=1
>                         realize
>                         map
>                     update FDT
> 
> To make the machine properties work, we need to move device creation
> to its .instance_init() method virt_instance_init().
> 
> Another complication is machvirt_init()'s computation of
> @firmware_loaded: it predicts what create_flash() will do.  Instead of
> predicting what create_flash()'s replacement virt_firmware_init() will
> do, I decided to have virt_firmware_init() return what it did.
> Requires calling it a bit earlier.
> 
> Resulting call tree:
> 
>     main()
>         current_machine = object_new()
>             ...
>                 virt_instance_init()
>                     virt_flash_create()
>                         virt_flash_create1() for flash[0]
>                             create
>                             configure: set defaults
>                             become child of machine [NEW]
>                             add machine prop pflash0 as alias for drive [NEW]
>                         virt_flash_create1() for flash[1]
>                             create
>                             configure: set defaults
>                             become child of machine [NEW]
>                             add machine prop pflash1 as alias for drive [NEW]
>         for all machine props from the command line: machine_set_property()
>             ...
>                 property_set_alias() for machine props pflash0, pflash1
>                     ...
>                         set_drive() for cfi.pflash01 prop drive
>                             this is how -machine pflash0=... etc set
>         machine_run_board_init(current_machine);
>             virt_firmware_init()
>                 pflash_cfi01_legacy_drive()
>                     legacy -drive if=pflash,unit=0 and =1 [NEW]
>                 virt_flash_map()
>                     virt_flash_map1() for flash[0]
>                         configure: num-blocks
>                         realize
>                         map
>                     virt_flash_map1() for flash[1]
>                         configure: num-blocks
>                         realize
>                         map
>                 fall back to -bios
>             virt_flash_fdt()
>                 update FDT

Thanks for elaborating the call tree in even more detail :)

> 
> You have László to thank for making me explain this in detail.

... as long as it's not s/thank/curse/ ;)

> 
> Signed-off-by: Markus Armbruster <address@hidden>
> Acked-by: Laszlo Ersek <address@hidden>
> ---
>  hw/arm/virt.c         | 198 +++++++++++++++++++++++++++---------------
>  include/hw/arm/virt.h |   2 +
>  2 files changed, 130 insertions(+), 70 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index ce2664a30b..3b7b5eb429 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -30,6 +30,7 @@
>  
>  #include "qemu/osdep.h"
>  #include "qemu/units.h"
> +#include "qemu/option.h"
>  #include "qapi/error.h"
>  #include "hw/sysbus.h"
>  #include "hw/arm/arm.h"
> @@ -871,25 +872,19 @@ static void create_virtio_devices(const 
> VirtMachineState *vms, qemu_irq *pic)
>      }
>  }
>  
> -static void create_one_flash(const char *name, hwaddr flashbase,
> -                             hwaddr flashsize, const char *file,
> -                             MemoryRegion *sysmem)
> +#define VIRT_FLASH_SECTOR_SIZE (256 * KiB)
> +
> +static PFlashCFI01 *virt_flash_create1(VirtMachineState *vms,

OK, I'm noticing the rename from "virt_pflash_create1".

> +                                        const char *name,
> +                                        const char *alias_prop_name)
>  {
> -    /* Create and map a single flash device. We use the same
> -     * parameters as the flash devices on the Versatile Express board.
> +    /*
> +     * Create a single flash device.  We use the same parameters as
> +     * the flash devices on the Versatile Express board.
>       */
> -    DriveInfo *dinfo = drive_get_next(IF_PFLASH);
>      DeviceState *dev = qdev_create(NULL, TYPE_PFLASH_CFI01);
> -    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> -    const uint64_t sectorlength = 256 * 1024;
>  
> -    if (dinfo) {
> -        qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(dinfo),
> -                            &error_abort);
> -    }
> -
> -    qdev_prop_set_uint32(dev, "num-blocks", flashsize / sectorlength);
> -    qdev_prop_set_uint64(dev, "sector-length", sectorlength);
> +    qdev_prop_set_uint64(dev, "sector-length", VIRT_FLASH_SECTOR_SIZE);
>      qdev_prop_set_uint8(dev, "width", 4);
>      qdev_prop_set_uint8(dev, "device-width", 2);
>      qdev_prop_set_bit(dev, "big-endian", false);
> @@ -898,41 +893,41 @@ static void create_one_flash(const char *name, hwaddr 
> flashbase,
>      qdev_prop_set_uint16(dev, "id2", 0x00);
>      qdev_prop_set_uint16(dev, "id3", 0x00);
>      qdev_prop_set_string(dev, "name", name);
> +    object_property_add_child(OBJECT(vms), name, OBJECT(dev),
> +                              &error_abort);
> +    object_property_add_alias(OBJECT(vms), alias_prop_name,
> +                              OBJECT(dev), "drive", &error_abort);
> +    return PFLASH_CFI01(dev);
> +}
> +
> +static void virt_flash_create(VirtMachineState *vms)
> +{
> +    vms->flash[0] = virt_flash_create1(vms, "virt.flash0", "pflash0");
> +    vms->flash[1] = virt_flash_create1(vms, "virt.flash1", "pflash1");
> +}
> +
> +static void virt_flash_map1(PFlashCFI01 *flash,
> +                            hwaddr base, hwaddr size,
> +                            MemoryRegion *sysmem)
> +{
> +    DeviceState *dev = DEVICE(flash);
> +
> +    assert(size % VIRT_FLASH_SECTOR_SIZE == 0);
> +    assert(size / VIRT_FLASH_SECTOR_SIZE <= UINT32_MAX);
> +    qdev_prop_set_uint32(dev, "num-blocks", size / VIRT_FLASH_SECTOR_SIZE);
>      qdev_init_nofail(dev);
>  
> -    memory_region_add_subregion(sysmem, flashbase,
> -                                sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 
> 0));
> -
> -    if (file) {
> -        char *fn;
> -        int image_size;
> -
> -        if (drive_get(IF_PFLASH, 0, 0)) {
> -            error_report("The contents of the first flash device may be "
> -                         "specified with -bios or with -drive if=pflash... "
> -                         "but you cannot use both options at once");
> -            exit(1);
> -        }
> -        fn = qemu_find_file(QEMU_FILE_TYPE_BIOS, file);
> -        if (!fn) {
> -            error_report("Could not find ROM image '%s'", file);
> -            exit(1);
> -        }
> -        image_size = load_image_mr(fn, sysbus_mmio_get_region(sbd, 0));
> -        g_free(fn);
> -        if (image_size < 0) {
> -            error_report("Could not load ROM image '%s'", file);
> -            exit(1);
> -        }
> -    }
> +    memory_region_add_subregion(sysmem, base,
> +                                sysbus_mmio_get_region(SYS_BUS_DEVICE(dev),
> +                                                       0));
>  }
>  
> -static void create_flash(const VirtMachineState *vms,
> -                         MemoryRegion *sysmem,
> -                         MemoryRegion *secure_sysmem)
> +static void virt_flash_map(VirtMachineState *vms,
> +                           MemoryRegion *sysmem,
> +                           MemoryRegion *secure_sysmem)
>  {
> -    /* Create two flash devices to fill the VIRT_FLASH space in the memmap.
> -     * Any file passed via -bios goes in the first of these.
> +    /*
> +     * Map two flash devices to fill the VIRT_FLASH space in the memmap.
>       * sysmem is the system memory space. secure_sysmem is the secure view
>       * of the system, and the first flash device should be made visible only
>       * there. The second flash device is visible to both secure and 
> nonsecure.
> @@ -941,13 +936,21 @@ static void create_flash(const VirtMachineState *vms,
>       */
>      hwaddr flashsize = vms->memmap[VIRT_FLASH].size / 2;
>      hwaddr flashbase = vms->memmap[VIRT_FLASH].base;
> +
> +    virt_flash_map1(vms->flash[0], flashbase, flashsize,
> +                    secure_sysmem);
> +    virt_flash_map1(vms->flash[1], flashbase + flashsize, flashsize,
> +                    sysmem);
> +}
> +
> +static void virt_flash_fdt(VirtMachineState *vms,
> +                           MemoryRegion *sysmem,
> +                           MemoryRegion *secure_sysmem)
> +{
> +    hwaddr flashsize = vms->memmap[VIRT_FLASH].size / 2;
> +    hwaddr flashbase = vms->memmap[VIRT_FLASH].base;
>      char *nodename;
>  
> -    create_one_flash("virt.flash0", flashbase, flashsize,
> -                     bios_name, secure_sysmem);
> -    create_one_flash("virt.flash1", flashbase + flashsize, flashsize,
> -                     NULL, sysmem);
> -
>      if (sysmem == secure_sysmem) {
>          /* Report both flash devices as a single node in the DT */
>          nodename = g_strdup_printf("/address@hidden" PRIx64, flashbase);
> @@ -959,7 +962,8 @@ static void create_flash(const VirtMachineState *vms,
>          qemu_fdt_setprop_cell(vms->fdt, nodename, "bank-width", 4);
>          g_free(nodename);
>      } else {
> -        /* Report the devices as separate nodes so we can mark one as
> +        /*
> +         * Report the devices as separate nodes so we can mark one as
>           * only visible to the secure world.
>           */
>          nodename = g_strdup_printf("/address@hidden" PRIx64, flashbase);
> @@ -982,6 +986,54 @@ static void create_flash(const VirtMachineState *vms,
>      }
>  }
>  
> +static bool virt_firmware_init(VirtMachineState *vms,
> +                               MemoryRegion *sysmem,
> +                               MemoryRegion *secure_sysmem)
> +{
> +    int i;
> +    BlockBackend *pflash_blk0;
> +
> +    /* Map legacy -drive if=pflash to machine properties */
> +    for (i = 0; i < ARRAY_SIZE(vms->flash); i++) {
> +        pflash_cfi01_legacy_drive(vms->flash[i],
> +                                  drive_get(IF_PFLASH, 0, i));
> +    }

And this v1->v2 update is due to the pflash_cfi01_legacy_drive()
contract change in v2 (i.e. the last two patches in this series).

No other changes that I can see. My ACK stands.

Thank you!
Laszlo

> +
> +    virt_flash_map(vms, sysmem, secure_sysmem);
> +
> +    pflash_blk0 = pflash_cfi01_get_blk(vms->flash[0]);
> +
> +    if (bios_name) {
> +        char *fname;
> +        MemoryRegion *mr;
> +        int image_size;
> +
> +        if (pflash_blk0) {
> +            error_report("The contents of the first flash device may be "
> +                         "specified with -bios or with -drive if=pflash... "
> +                         "but you cannot use both options at once");
> +            exit(1);
> +        }
> +
> +        /* Fall back to -bios */
> +
> +        fname = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
> +        if (!fname) {
> +            error_report("Could not find ROM image '%s'", bios_name);
> +            exit(1);
> +        }
> +        mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(vms->flash[0]), 0);
> +        image_size = load_image_mr(fname, mr);
> +        g_free(fname);
> +        if (image_size < 0) {
> +            error_report("Could not load ROM image '%s'", bios_name);
> +            exit(1);
> +        }
> +    }
> +
> +    return pflash_blk0 || bios_name;
> +}
> +
>  static FWCfgState *create_fw_cfg(const VirtMachineState *vms, AddressSpace 
> *as)
>  {
>      hwaddr base = vms->memmap[VIRT_FW_CFG].base;
> @@ -1421,7 +1473,7 @@ static void machvirt_init(MachineState *machine)
>      MemoryRegion *secure_sysmem = NULL;
>      int n, virt_max_cpus;
>      MemoryRegion *ram = g_new(MemoryRegion, 1);
> -    bool firmware_loaded = bios_name || drive_get(IF_PFLASH, 0, 0);
> +    bool firmware_loaded;
>      bool aarch64 = true;
>  
>      /*
> @@ -1460,6 +1512,27 @@ static void machvirt_init(MachineState *machine)
>          exit(1);
>      }
>  
> +    if (vms->secure) {
> +        if (kvm_enabled()) {
> +            error_report("mach-virt: KVM does not support Security 
> extensions");
> +            exit(1);
> +        }
> +
> +        /*
> +         * The Secure view of the world is the same as the NonSecure,
> +         * but with a few extra devices. Create it as a container region
> +         * containing the system memory at low priority; any secure-only
> +         * devices go in at higher priority and take precedence.
> +         */
> +        secure_sysmem = g_new(MemoryRegion, 1);
> +        memory_region_init(secure_sysmem, OBJECT(machine), "secure-memory",
> +                           UINT64_MAX);
> +        memory_region_add_subregion_overlap(secure_sysmem, 0, sysmem, -1);
> +    }
> +
> +    firmware_loaded = virt_firmware_init(vms, sysmem,
> +                                         secure_sysmem ?: sysmem);
> +
>      /* If we have an EL3 boot ROM then the assumption is that it will
>       * implement PSCI itself, so disable QEMU's internal implementation
>       * so it doesn't get in the way. Instead of starting secondary
> @@ -1505,23 +1578,6 @@ static void machvirt_init(MachineState *machine)
>          exit(1);
>      }
>  
> -    if (vms->secure) {
> -        if (kvm_enabled()) {
> -            error_report("mach-virt: KVM does not support Security 
> extensions");
> -            exit(1);
> -        }
> -
> -        /* The Secure view of the world is the same as the NonSecure,
> -         * but with a few extra devices. Create it as a container region
> -         * containing the system memory at low priority; any secure-only
> -         * devices go in at higher priority and take precedence.
> -         */
> -        secure_sysmem = g_new(MemoryRegion, 1);
> -        memory_region_init(secure_sysmem, OBJECT(machine), "secure-memory",
> -                           UINT64_MAX);
> -        memory_region_add_subregion_overlap(secure_sysmem, 0, sysmem, -1);
> -    }
> -
>      create_fdt(vms);
>  
>      possible_cpus = mc->possible_cpu_arch_ids(machine);
> @@ -1610,7 +1666,7 @@ static void machvirt_init(MachineState *machine)
>                                      &machine->device_memory->mr);
>      }
>  
> -    create_flash(vms, sysmem, secure_sysmem ? secure_sysmem : sysmem);
> +    virt_flash_fdt(vms, sysmem, secure_sysmem);
>  
>      create_gic(vms, pic);
>  
> @@ -1956,6 +2012,8 @@ static void virt_instance_init(Object *obj)
>                                      NULL);
>  
>      vms->irqmap = a15irqmap;
> +
> +    virt_flash_create(vms);
>  }
>  
>  static const TypeInfo virt_machine_info = {
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index 507517c603..424070924e 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -35,6 +35,7 @@
>  #include "qemu/notify.h"
>  #include "hw/boards.h"
>  #include "hw/arm/arm.h"
> +#include "hw/block/flash.h"
>  #include "sysemu/kvm.h"
>  #include "hw/intc/arm_gicv3_common.h"
>  
> @@ -113,6 +114,7 @@ typedef struct {
>      Notifier machine_done;
>      DeviceState *platform_bus_dev;
>      FWCfgState *fw_cfg;
> +    PFlashCFI01 *flash[2];
>      bool secure;
>      bool highmem;
>      bool highmem_ecam;
> 




reply via email to

[Prev in Thread] Current Thread [Next in Thread]