Re: [Qemu-arm] [Qemu-devel] [PATCH v7 2/2] hw/arm: Add arm SBSA reference machine, devices part

[Markus Armbruster]

Peter Maydell <address@hidden> writes:

> On Thu, 18 Apr 2019 at 05:05, Hongbo Zhang <address@hidden> wrote:
>>
>> Following the previous patch, this patch adds peripheral devices to the
>> newly introduced SBSA-ref machine.
>>
>> Signed-off-by: Hongbo Zhang <address@hidden>
>> ---
>>  hw/arm/sbsa-ref.c | 451 
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 451 insertions(+)
>
> Some fairly minor comments on this one.
>
>> +static void create_flash(const SBSAMachineState *vms,
>> +                         MemoryRegion *sysmem,
>> +                         MemoryRegion *secure_sysmem)
>> +{
>> +    /*
>> +     * Create one secure and nonsecure flash devices to fill SBSA_FLASH
>> +     * space in the memmap, file passed via -bios goes in the first one.
>> +     */
>> +    hwaddr flashsize = vms->memmap[SBSA_FLASH].size / 2;
>> +    hwaddr flashbase = vms->memmap[SBSA_FLASH].base;
>> +
>> +    create_one_flash("sbsa-ref.flash0", flashbase, flashsize,
>> +                     bios_name, secure_sysmem);
>> +    create_one_flash("sbsa-ref.flash1", flashbase + flashsize, flashsize,
>> +                     NULL, sysmem);
>> +}
>
> I think Markus might have an opinion on the best way to create
> flash devices on a new board model. Is "just create two flash
> devices the way the virt board does" the right thing?

Short answer: create flash devices the way the ARM virt board does now,
after commit e0561e60f17, merged into master today.  Possibly less
backward compatibility stuff you don't need.  As is, your patch creates
them the way the ARM virt board did before commit e0561e60f17.  Please
consider updating.

Longer answer:

The old way to configure block backends is -drive.

The newer -blockdev is more flexible.  Libvirt is in the process of
transitioning from -drive to -blockdev entirely.  Other users with
similar needs for flexibility may do the same.  We hope to deprecate
-drive eventually.

The traditional way to configure onboard flash is -drive if=pflash.
Works, but we need a way to configure with -blockdev for full
flexibility, and to support libvirt ditching -drive entirely.

I recently improved the i386 PC machine types (commit ebc29e1beab) and
the ARM virt machine types (commit e0561e60f17) to support flash
configuration with -blockdev.

I recommend new boards support flash configuration with -blockdev from
the start.

Questions?