Laszlo Ersek <ler...@redhat.com> 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 <arm...@redhat.com> >> Reviewed-by: Philippe Mathieu-Daudé <phi...@redhat.com> >> Acked-by: Laszlo Ersek <ler...@redhat.com> >> Reviewed-by: Michael S. Tsirkin <m...@redhat.com> >> Message-Id: <87ftrtux81....@dusky.pond.sub.org> >> --- >> 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!
