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

[Laszlo Ersek]

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.)

Thanks,
Laszlo