Re: [Qemu-devel] [RFC PATCH 6/6] pc: Support firmware configuration with -blockdev

[Laszlo Ersek]

On 02/26/19 13:35, Markus Armbruster wrote:
> Laszlo Ersek <address@hidden> writes:

>>> -#define FLASH_MAP_UNIT_MAX 2
>>> +static PFlashCFI01 *pc_pflash_create(const char *name)
>>> +{
>>> +    DeviceState *dev = qdev_create(NULL, TYPE_CFI_PFLASH01);
>>> +
>>> +    qdev_prop_set_uint64(dev, "sector-length", 4096);
>>> +    qdev_prop_set_uint8(dev, "width", 1);
>>> +    qdev_prop_set_string(dev, "name", name);
>>> +    return CFI_PFLASH01(dev);
>>> +}
>>> +
>>> +void pc_system_flash_create(PCMachineState *pcms)
>>> +{
>>> +    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
>>> +
>>> +    if (pcmc->pci_enabled) {
>>> +        pcms->flash[0] = pc_pflash_create("system.flash0");
>>> +        pcms->flash[1] = pc_pflash_create("system.flash1");
>>> +        object_property_add_alias(OBJECT(pcms), "pflash0",
>>> +                                  OBJECT(pcms->flash[0]), "drive",
>>> +                                  &error_abort);
>>> +        object_property_add_alias(OBJECT(pcms), "pflash1",
>>> +                                  OBJECT(pcms->flash[1]), "drive",
>>> +                                  &error_abort);
>>> +    }
>>> +}
>>> +
>>> +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);
>>> +            g_free(prop_name);
>>> +            /*
>>> +             * FIXME 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.
>>> +             */
>>> +            pcms->flash[i] = NULL;
>>> +        }
>>> +    }
>>> +}
>>
>> I totally can't recommend a way to resolve this FIXME, alas.
> 
> I'm hoping for Paolo to don his shining armor and rescue me ;)
> 
>>>  
>>>  /* 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
>>> @@ -84,88 +135,78 @@ static void pc_isa_bios_init(MemoryRegion *rom_memory,
>>>   * only 18MB-4KB below 4G. For now, restrict the cumulative mapping to 8MB 
>>> in
>>>   * size.
>>>   */
>>> -#define FLASH_MAP_BASE_MIN ((hwaddr)(4 * GiB - 8 * MiB))
>>> +#define FLASH_SIZE_LIMIT (8 * MiB)
>>>  
>>> -/* 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.
>>> +/*
>>> + * 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 multiples of 4KiB, or the total size exceeds
>>> + * FLASH_SIZE_LIMIT.
>>>   *
>>> - * 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
>>> + * 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;
>>
>> (1) Rather than duplicate this constant here, can we get it inside the
>> loop, via the "sector-length" property? We set the property in
>> pc_pflash_create() (identically for both chips, but that's not a problem
>> I think).
> 
> We can.  It's slightly bothersome, since PFlashCFI01 is incomplete here.
> 
> Simpler: #define FLASH_SECTOR_SIZE 4096, and use it in both places.  Not
> quite as clean, because while the property value cannot change now, we
> can't completely exclude the possibility for the future.

I agree, a macro works too.

Thanks,
Laszlo