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

[Markus Armbruster]

Laszlo Ersek  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 
>> Reviewed-by: Philippe Mathieu-Daudé 
>> Acked-by: Laszlo Ersek 
>> Reviewed-by: Michael S. Tsirkin 
>> 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
> .
> 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!

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 
> Reviewed-by: Philippe Mathieu-Daudé 
> Acked-by: Laszlo Ersek 
> Reviewed-by: Michael S. Tsirkin 
> 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
.
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

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

[Markus Armbruster]

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 
Reviewed-by: Philippe Mathieu-Daudé 
Acked-by: Laszlo Ersek 
Reviewed-by: Michael S. Tsirkin 
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(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index d8572d3c00..d71dc28ef6 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -2649,6 +2649,8 @@ static void pc_machine_initfn(Object *obj)
 pcms->smbus_enabled = true;
 pcms->sata_enabled = true;
 pcms->pit_enabled = true;
+
+pc_system_flash_create(pcms);
 }
 
 static void pc_machine_reset(void)
diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index 785123252c..c628540774 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -40,6 +40,17 @@
 
 #define BIOS_FILENAME "bios.bin"
 
+/*
+ * 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
+ * [0xFEE0..0xFEE01000] -- see IO_APIC_DEFAULT_ADDRESS --, leaving free
+ * only 18MB-4KB below 4G. For now, restrict the cumulative mapping to 8MB in
+ * size.
+ */
+#define FLASH_SIZE_LIMIT (8 * MiB)
+
+#define FLASH_SECTOR_SIZE 4096
+
 static void pc_isa_bios_init(MemoryRegion *rom_memory,
  MemoryRegion *flash_mem,
  int ram_size)
@@ -71,96 +82,118 @@ static void pc_isa_bios_init(MemoryRegion *rom_memory,
 memory_region_set_readonly(isa_bios, true);
 }
 
-#define FLASH_MAP_UNIT_MAX 2
+static PFlashCFI01 *pc_pflash_create(PCMachineState *pcms,
+ const char *name,
+ const char *alias_prop_name)
+{
+DeviceState *dev = qdev_create(NULL, TYPE_PFLASH_CFI01);
 
-/* 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
- *