Re: [Qemu-devel] [PULL 26/27] pc: Support firmware configuration with -blockdev

[Markus Armbruster]

Laszlo Ersek <address@hidden> writes:

> Hi Markus,
>
> (+Michal, Peter)
>
> On 03/11/19 23:08, 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.  It also makes upgrading
>> firmware on the host easier.  Below the hood, it creates 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.
>> 
>> Mapping -drive if=none,... to -blockdev is a solved problem.  With
>> if=T other than if=none, -drive additionally configures a block device
>> frontend.  For non-onboard devices, that part maps to -device.  Also a
>> solved problem.  For onboard devices such as PC flash memory, we have
>> an unsolved problem.
>> 
>> 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.  One of them is
>> -drive, which we'd rather deprecate, but can't until we have suitable
>> replacements for all its uses.
>> 
>> Sadly, I can't attack the wider problem today.  So back to the narrow
>> problem.
>> 
>> My first idea was to reduce it to its solved buddy by using pluggable
>> instead of onboard devices for the flash memory.  Workable, but it
>> requires some extra smarts in firmware descriptors and libvirt.  Paolo
>> had an idea that is simpler for libvirt: keep the devices onboard, and
>> add machine properties for their block backends.
>> 
>> The implementation is less than straightforward, I'm afraid.
>> 
>> First, block backend properties are *qdev* properties.  Machines can't
>> have those, as they're not devices.  I could duplicate these qdev
>> properties as QOM properties, but I hate that.
>> 
>> More seriously, the properties do not belong to the machine, they
>> belong to the onboard flash devices.  Adding them to the machine would
>> then require bad magic to somehow transfer them to the flash devices.
>> Fortunately, QOM provides the means to handle exactly this case: add
>> alias properties to the machine that forward to the onboard devices'
>> properties.
>> 
>> Properties need to be created in .instance_init() methods.  For PC
>> machines, that's pc_machine_initfn().  To make alias properties work,
>> we need to create the onboard flash devices there, too.  Requires
>> several bug fixes, in the previous commits.  We also have to realize
>> the devices.  More on that below.
>> 
>> If the user sets pflash0, firmware resides in flash memory.
>> pc_system_firmware_init() maps and realizes the flash devices.
>> 
>> Else, firmware resides in ROM.  The onboard flash devices aren't used
>> then.  pc_system_firmware_init() destroys them unrealized, along with
>> the alias properties.
>> 
>> The existing code to pick up drives defined with -drive if=pflash is
>> replaced by code to desugar into the machine properties.
>> 
>> Signed-off-by: Markus Armbruster <address@hidden>
>> Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
>> Acked-by: Laszlo Ersek <address@hidden>
>> Reviewed-by: Michael S. Tsirkin <address@hidden>
>> Message-Id: <address@hidden>
>> ---
>>  hw/i386/pc.c         |   2 +
>>  hw/i386/pc_sysfw.c   | 233 ++++++++++++++++++++++++++++---------------
>>  include/hw/i386/pc.h |   3 +
>>  3 files changed, 156 insertions(+), 82 deletions(-)
>
> the next patch in the series -- which is now commit e33763be7cd3 --
> updates the command line recommendation regardless of system emulation
> target / machine type. But, the series (i.e., this patch, ultimately)
> implements support for the new command line only for the PC machine types.
>
> I think the same interface should be implemented for (arm|aarch64)/virt
> too, because those machine types are covered by
> "docs/interop/firmware.json" similarly, and once Michal does the libvirt
> work for "pflash via -blockdev", libvirt will want to use uniform
> cmdline options for both sets of machine types.
>
> (Search "hw/arm/virt.c" for IF_PFLASH / create_one_flash().)
>
> ... Yes, this omission is in fact "reviewer fail" (if you allow me to
> steal that term); I should have noticed this *much* earlier. I've only
> realized now, from
> <https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg07070.html>.
> Mea culpa :(
>
> (It's just as well that this is not a "correctness" but "completeness"
> kind of problem -- whatever has been committed thus far should be OK.)

I'll look into it.  Thanks!