PATCH: Increase System Firmware Max Size

PATCH: Increase System Firmware Max Size

[McMillan, Erich]

[-- Attachment #1: Type: text/plain, Size: 936 bytes --]

Hi all,

(this is my first Qemu patch submission, please let me know if my formatting/content needs to be fixed).
We have a need for increased firmware size, currently we are building Qemu with the following change to test our Uefi Firmware and it works well for us. Hope that this change can be made to open source. Thank you.
-------
Increase allowed system firmware size to 16M per comment suggesting up to 18M is permissible.

 Signed-off-by: Erich McMillan <erich.mcmillan@hp.com>

diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index b8d8ef59eb17c6ace8194fc69c3d27809becfbc0..f6f7cd744d0690cee0355fbd19ffdcdb71ea75ca 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -46,7 +46,7 @@
  * only 18MB-4KB below 4G. For now, restrict the cumulative mapping to 8MB in
  * size.
  */
-#define FLASH_SIZE_LIMIT (8 * MiB)
+#define FLASH_SIZE_LIMIT (16 * MiB)

 #define FLASH_SECTOR_SIZE 4096
-------


[-- Attachment #2: Type: text/html, Size: 2886 bytes --]

Re: PATCH: Increase System Firmware Max Size

[Laszlo Ersek]

+Markus, Dave, Phil

On 09/11/20 03:45, McMillan, Erich wrote:
> Hi all,
> 
> (this is my first Qemu patch submission, please let me know if my formatting/content needs to be fixed).
> We have a need for increased firmware size, currently we are building Qemu with the following change to test our Uefi Firmware and it works well for us. Hope that this change can be made to open source. Thank you.
> -------
> Increase allowed system firmware size to 16M per comment suggesting up to 18M is permissible.
> 
>  Signed-off-by: Erich McMillan <erich.mcmillan@hp.com>
> 
> diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
> index b8d8ef59eb17c6ace8194fc69c3d27809becfbc0..f6f7cd744d0690cee0355fbd19ffdcdb71ea75ca 100644
> --- a/hw/i386/pc_sysfw.c
> +++ b/hw/i386/pc_sysfw.c
> @@ -46,7 +46,7 @@
>   * only 18MB-4KB below 4G. For now, restrict the cumulative mapping to 8MB in
>   * size.
>   */
> -#define FLASH_SIZE_LIMIT (8 * MiB)
> +#define FLASH_SIZE_LIMIT (16 * MiB)
> 
>  #define FLASH_SECTOR_SIZE 4096
> -------
> 
> 

(1) This is not a trivial change, so qemu-trivial: please ignore.

(2) The comment has not been updated.

(3) I'm almost certain that *if* we want to change this, it needs to be
turned into a machine type (or some device model) property, for
migration compatibility.

(4) I feel we need much more justification for this change than just
"our firmware is larger than upstream OVMF".

In the upstream 4MB unified OVMF build, there's *plenty* of free room.
Of course that's not to say that we're willing to *squander* that space
-- whenever we include anything new in the upstream OVMF platform(s),
there must be a very good reason for it --, just that, given the
upstream OVMF status, the proposed pflash increase in QEMU is staggering.

Considering upstream OVMF and QEMU, it should take *years* to even get
close to filling the 4MB unified flash image of OVMF (hint: link-time
optimization, LZMA compression), let alone to exhaust the twice as large
(8MB) QEMU allowance.

Unless you are committed to open source your guest firmware too (maybe
as part of upstream edk2, maybe elsewhere), I seriously doubt we should
accommodate this use case in *upstream* QEMU. It complicates things
(minimally with regard to migration), and currently I don't see the
benefit to the upstream community.

Clearly, for building your firmware image, you must have minimally
modified the DSC and FDF files in OVMF too, or created an entirely
separate firmware platform -- DSC and FDF both.

If you are using your downstream OVMF build as a testbed for your
proprietary physical platform firmware, that's generally a use case that
we're mildly interested in not breaking in upstream OvmfPkg. But in
order to make me care for real (as an OvmfPkg co-maintainer), you'd need
to upstream your OVMF platform to edk2 (we already have Xen and --
recently added -- bhyve firmware platforms under OvmfPkg, not just
QEMU). You'd also have to offer long-term reviewership and testing for
that platform in edk2 (like the Xen and bhyve stake-holders do). Then we
could consider additional complexity in QEMU for booting that firmware
platform.

Thanks,
Laszlo

Re: PATCH: Increase System Firmware Max Size

[Dr. David Alan Gilbert]

* Laszlo Ersek (lersek@redhat.com) wrote:
> +Markus, Dave, Phil
> 
> On 09/11/20 03:45, McMillan, Erich wrote:
> > Hi all,
> > 
> > (this is my first Qemu patch submission, please let me know if my formatting/content needs to be fixed).
> > We have a need for increased firmware size, currently we are building Qemu with the following change to test our Uefi Firmware and it works well for us. Hope that this change can be made to open source. Thank you.
> > -------
> > Increase allowed system firmware size to 16M per comment suggesting up to 18M is permissible.
> > 
> >  Signed-off-by: Erich McMillan <erich.mcmillan@hp.com>
> > 
> > diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
> > index b8d8ef59eb17c6ace8194fc69c3d27809becfbc0..f6f7cd744d0690cee0355fbd19ffdcdb71ea75ca 100644
> > --- a/hw/i386/pc_sysfw.c
> > +++ b/hw/i386/pc_sysfw.c
> > @@ -46,7 +46,7 @@
> >   * only 18MB-4KB below 4G. For now, restrict the cumulative mapping to 8MB in
> >   * size.
> >   */
> > -#define FLASH_SIZE_LIMIT (8 * MiB)
> > +#define FLASH_SIZE_LIMIT (16 * MiB)
> > 
> >  #define FLASH_SECTOR_SIZE 4096
> > -------
> > 
> > 
> 
> (1) This is not a trivial change, so qemu-trivial: please ignore.
> 
> (2) The comment has not been updated.
> 
> (3) I'm almost certain that *if* we want to change this, it needs to be
> turned into a machine type (or some device model) property, for
> migration compatibility.

I'm missing what this constant exists for - why is the
check there at all  Is there something else that lives below this
address that we have to protect?
My reading of the code is that increasing that constant doesn't change
the guests view at all, as long as the guest was given the same flash
files - so if the guests view doesn't change, then why would we tie
it to the machine type?

> (4) I feel we need much more justification for this change than just
> "our firmware is larger than upstream OVMF".
> 
> In the upstream 4MB unified OVMF build, there's *plenty* of free room.
> Of course that's not to say that we're willing to *squander* that space
> -- whenever we include anything new in the upstream OVMF platform(s),
> there must be a very good reason for it --, just that, given the
> upstream OVMF status, the proposed pflash increase in QEMU is staggering.
> 
> Considering upstream OVMF and QEMU, it should take *years* to even get
> close to filling the 4MB unified flash image of OVMF (hint: link-time
> optimization, LZMA compression), let alone to exhaust the twice as large
> (8MB) QEMU allowance.
> 
> Unless you are committed to open source your guest firmware too (maybe
> as part of upstream edk2, maybe elsewhere), I seriously doubt we should
> accommodate this use case in *upstream* QEMU. It complicates things
> (minimally with regard to migration), and currently I don't see the
> benefit to the upstream community.
> 
> Clearly, for building your firmware image, you must have minimally
> modified the DSC and FDF files in OVMF too, or created an entirely
> separate firmware platform -- DSC and FDF both.
> 
> If you are using your downstream OVMF build as a testbed for your
> proprietary physical platform firmware, that's generally a use case that
> we're mildly interested in not breaking in upstream OvmfPkg. But in
> order to make me care for real (as an OvmfPkg co-maintainer), you'd need
> to upstream your OVMF platform to edk2 (we already have Xen and --
> recently added -- bhyve firmware platforms under OvmfPkg, not just
> QEMU). You'd also have to offer long-term reviewership and testing for
> that platform in edk2 (like the Xen and bhyve stake-holders do). Then we
> could consider additional complexity in QEMU for booting that firmware
> platform.

Our UEFI firmware is pretty sparse; it doesn't have any pretty graphics
or snazzy stuff, or have to survive configuring lots of hardware; also
I'm aware of other companies who are looking at putting big blobs
of stuff in firmware for open uses;  so I don't see a problem with
changing this limit from the QEMU side of things.

Dave

> 
> Thanks,
> Laszlo
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: PATCH: Increase System Firmware Max Size

[Laszlo Ersek]

On 09/11/20 10:34, Dr. David Alan Gilbert wrote:
> * Laszlo Ersek (lersek@redhat.com) wrote:
>> +Markus, Dave, Phil
>>
>> On 09/11/20 03:45, McMillan, Erich wrote:
>>> Hi all,
>>>
>>> (this is my first Qemu patch submission, please let me know if my formatting/content needs to be fixed).
>>> We have a need for increased firmware size, currently we are building Qemu with the following change to test our Uefi Firmware and it works well for us. Hope that this change can be made to open source. Thank you.
>>> -------
>>> Increase allowed system firmware size to 16M per comment suggesting up to 18M is permissible.
>>>
>>>  Signed-off-by: Erich McMillan <erich.mcmillan@hp.com>
>>>
>>> diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
>>> index b8d8ef59eb17c6ace8194fc69c3d27809becfbc0..f6f7cd744d0690cee0355fbd19ffdcdb71ea75ca 100644
>>> --- a/hw/i386/pc_sysfw.c
>>> +++ b/hw/i386/pc_sysfw.c
>>> @@ -46,7 +46,7 @@
>>>   * only 18MB-4KB below 4G. For now, restrict the cumulative mapping to 8MB in
>>>   * size.
>>>   */
>>> -#define FLASH_SIZE_LIMIT (8 * MiB)
>>> +#define FLASH_SIZE_LIMIT (16 * MiB)
>>>
>>>  #define FLASH_SECTOR_SIZE 4096
>>> -------
>>>
>>>
>>
>> (1) This is not a trivial change, so qemu-trivial: please ignore.
>>
>> (2) The comment has not been updated.
>>
>> (3) I'm almost certain that *if* we want to change this, it needs to be
>> turned into a machine type (or some device model) property, for
>> migration compatibility.
> 
> I'm missing what this constant exists for - why is the
> check there at all  Is there something else that lives below this
> address that we have to protect?

Yes, some MMIOs that I'm at least aware of are IO_APIC_DEFAULT_ADDRESS
(0xfec00000), TPM_PPI_ADDR_BASE (0xFED45000), APIC_DEFAULT_ADDRESS
(0xfee00000).

They are not directly adjacent with pflash; nor should they be.

On one hand, the current FLASH_SIZE_LIMIT is meant to be  sufficient for
a long time ("should be enough for anyone").

On the other hand, if we have a really strong reason to increase the
size limit, the current value is supposed to give us a safety margin, so
that we can satisfy the immediate need at that point *first*, and start
looking into (likely more intrusive) physical address map changes, to
restore the safety margin.

> My reading of the code is that increasing that constant doesn't change
> the guests view at all, as long as the guest was given the same flash
> files - so if the guests view doesn't change, then why would we tie
> it to the machine type?

If you increase the size limit (without tieing it to a machine type),
then, with an upgraded QEMU and the same (old) machine type, you can
start a guest with a larger-than-earlier (cumulative) flash size. Then,
when you try to migrate this to an old QEMU (but same machine type),
it's a bad surprise. I understand that backwards migration is not
universally supported (or expected), but I don't want this problem to
land on my desk *ever*.

Furthermore, un-enumerable / platform-MMIO devices tend to pop up time
after time. TPM_PPI_ADDR_BASE (0xFED45000) is a somewhat recent
addition, for example. It's not like we're never going to need more
address space up there.

> 
>> (4) I feel we need much more justification for this change than just
>> "our firmware is larger than upstream OVMF".
>>
>> In the upstream 4MB unified OVMF build, there's *plenty* of free room.
>> Of course that's not to say that we're willing to *squander* that space
>> -- whenever we include anything new in the upstream OVMF platform(s),
>> there must be a very good reason for it --, just that, given the
>> upstream OVMF status, the proposed pflash increase in QEMU is staggering.
>>
>> Considering upstream OVMF and QEMU, it should take *years* to even get
>> close to filling the 4MB unified flash image of OVMF (hint: link-time
>> optimization, LZMA compression), let alone to exhaust the twice as large
>> (8MB) QEMU allowance.
>>
>> Unless you are committed to open source your guest firmware too (maybe
>> as part of upstream edk2, maybe elsewhere), I seriously doubt we should
>> accommodate this use case in *upstream* QEMU. It complicates things
>> (minimally with regard to migration), and currently I don't see the
>> benefit to the upstream community.
>>
>> Clearly, for building your firmware image, you must have minimally
>> modified the DSC and FDF files in OVMF too, or created an entirely
>> separate firmware platform -- DSC and FDF both.
>>
>> If you are using your downstream OVMF build as a testbed for your
>> proprietary physical platform firmware, that's generally a use case that
>> we're mildly interested in not breaking in upstream OvmfPkg. But in
>> order to make me care for real (as an OvmfPkg co-maintainer), you'd need
>> to upstream your OVMF platform to edk2 (we already have Xen and --
>> recently added -- bhyve firmware platforms under OvmfPkg, not just
>> QEMU). You'd also have to offer long-term reviewership and testing for
>> that platform in edk2 (like the Xen and bhyve stake-holders do). Then we
>> could consider additional complexity in QEMU for booting that firmware
>> platform.
> 
> Our UEFI firmware is pretty sparse;

Yes, in part because I strive to keep it that way. I fight to keep out
all cruft that I can (at least by conditionalizing it) so that there is
room for the stuff that I cannot keep out. (And I always strive to set
expectations that flipping all possible build knobs in OVMF to "on" may
very well cause an "out of space" error.)

- I've made sure that PVSCSI_ENABLE and MPT_SCSI_ENABLE be stand-alone
config knobs. The use case behind them is valid, the drivers are open
source, but the use case is still niche, so they must be easy to keep out.

- I've made sure LSI_SCSI_ENABLE is a stand-alone config knob too (and
it even defaults to FALSE). The QEMU device that the driver drives is
obsolete / deprecated.

- If VMBus drivers are ever going to be contributed, they'll need a
config knob.

- Current Xen code in OVMF is supposed to be separated completely to the
new, dedicated XenPVH platform
<https://bugzilla.tianocore.org/show_bug.cgi?id=2122>.

- Bhyve support is a separate platform build.

- Capsule updates are not supported by OVMF, and if they will ever be,
they're going to have to be a separate firmware platform. Datacenter
virtualization has no use for capsule updates.

- The next big thing I expect to have to keep out of OVMF is Redfish
<https://en.wikipedia.org/wiki/Redfish_(specification)>. Libvirt,
OpenStack, Cockpit, Kubernetes already handle that area *underneath* the
guest, I believe. (It's OK to use OVMF for developing / testing Redfish;
it's not OK to expect that the current OVMF firmware platform contain
everything that it contains now *plus* Redfish.)

Sparse is *good*.

> it doesn't have any pretty graphics
> or snazzy stuff,

Which is arguably completely superfluous on every possible platform one
can imagine. On the other hand, if you want a real serial port on
workstation class hardware, you may have to order a separate part (if
you are lucky and you *can* order one). Serial-over-LAN is a complete
non-replacement.

The reason (should I say: excuse) for the firmware to exist is to (a)
boot operating systems, (b) abstract away some ugly platform-specific
hardware for OS runtime (by providing ACPI and SMBIOS descriptions, and
a *small* set of runtime services). We can maybe add (c) "root of trust".

In practice, physical firmware is becoming the hw vendor's OS before
(and under) your OS, one you cannot replace, and one whose updates can
brick your hardware. Permitting the same feature creep on virtual
platforms is wrong.

> or have to survive configuring lots of hardware; also
> I'm aware of other companies who are looking at putting big blobs
> of stuff in firmware for open uses;

Key term being "open uses". Let us see them.

> so I don't see a problem with
> changing this limit from the QEMU side of things.

I do. Software and data always expand to consume all available space,
and it's going to cause a conflict between UEFI features and platform
MMIO sooner or later. Then I'll get the privilege of shuffling around
the crap in OVMF (all of which is "indispensable" or course).

If we ever go down this route, it needs to benefit open source directly
and significantly.

Laszlo

Re: PATCH: Increase System Firmware Max Size

[Dr. David Alan Gilbert]

* Laszlo Ersek (lersek@redhat.com) wrote:
> On 09/11/20 10:34, Dr. David Alan Gilbert wrote:
> > * Laszlo Ersek (lersek@redhat.com) wrote:
> >> +Markus, Dave, Phil
> >>
> >> On 09/11/20 03:45, McMillan, Erich wrote:
> >>> Hi all,
> >>>
> >>> (this is my first Qemu patch submission, please let me know if my formatting/content needs to be fixed).
> >>> We have a need for increased firmware size, currently we are building Qemu with the following change to test our Uefi Firmware and it works well for us. Hope that this change can be made to open source. Thank you.
> >>> -------
> >>> Increase allowed system firmware size to 16M per comment suggesting up to 18M is permissible.
> >>>
> >>>  Signed-off-by: Erich McMillan <erich.mcmillan@hp.com>
> >>>
> >>> diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
> >>> index b8d8ef59eb17c6ace8194fc69c3d27809becfbc0..f6f7cd744d0690cee0355fbd19ffdcdb71ea75ca 100644
> >>> --- a/hw/i386/pc_sysfw.c
> >>> +++ b/hw/i386/pc_sysfw.c
> >>> @@ -46,7 +46,7 @@
> >>>   * only 18MB-4KB below 4G. For now, restrict the cumulative mapping to 8MB in
> >>>   * size.
> >>>   */
> >>> -#define FLASH_SIZE_LIMIT (8 * MiB)
> >>> +#define FLASH_SIZE_LIMIT (16 * MiB)
> >>>
> >>>  #define FLASH_SECTOR_SIZE 4096
> >>> -------
> >>>
> >>>
> >>
> >> (1) This is not a trivial change, so qemu-trivial: please ignore.
> >>
> >> (2) The comment has not been updated.
> >>
> >> (3) I'm almost certain that *if* we want to change this, it needs to be
> >> turned into a machine type (or some device model) property, for
> >> migration compatibility.
> > 
> > I'm missing what this constant exists for - why is the
> > check there at all  Is there something else that lives below this
> > address that we have to protect?
> 
> Yes, some MMIOs that I'm at least aware of are IO_APIC_DEFAULT_ADDRESS
> (0xfec00000), TPM_PPI_ADDR_BASE (0xFED45000), APIC_DEFAULT_ADDRESS
> (0xfee00000).
> 
> They are not directly adjacent with pflash; nor should they be.

Hmm those need explicitly checks adding somewhere against
FLASH_SIZE_LIMIT!

> On one hand, the current FLASH_SIZE_LIMIT is meant to be  sufficient for
> a long time ("should be enough for anyone").
> 
> On the other hand, if we have a really strong reason to increase the
> size limit, the current value is supposed to give us a safety margin, so
> that we can satisfy the immediate need at that point *first*, and start
> looking into (likely more intrusive) physical address map changes, to
> restore the safety margin.
> 
> > My reading of the code is that increasing that constant doesn't change
> > the guests view at all, as long as the guest was given the same flash
> > files - so if the guests view doesn't change, then why would we tie
> > it to the machine type?
> 
> If you increase the size limit (without tieing it to a machine type),
> then, with an upgraded QEMU and the same (old) machine type, you can
> start a guest with a larger-than-earlier (cumulative) flash size. Then,
> when you try to migrate this to an old QEMU (but same machine type),
> it's a bad surprise. I understand that backwards migration is not
> universally supported (or expected), but I don't want this problem to
> land on my desk *ever*.

I support backwards migration; but that migration wouldn't work anyway -
wouldn't that fail nicely with a mismatched RAMBlock size?

> Furthermore, un-enumerable / platform-MMIO devices tend to pop up time
> after time. TPM_PPI_ADDR_BASE (0xFED45000) is a somewhat recent
> addition, for example. It's not like we're never going to need more
> address space up there.
> 
> > 
> >> (4) I feel we need much more justification for this change than just
> >> "our firmware is larger than upstream OVMF".
> >>
> >> In the upstream 4MB unified OVMF build, there's *plenty* of free room.
> >> Of course that's not to say that we're willing to *squander* that space
> >> -- whenever we include anything new in the upstream OVMF platform(s),
> >> there must be a very good reason for it --, just that, given the
> >> upstream OVMF status, the proposed pflash increase in QEMU is staggering.
> >>
> >> Considering upstream OVMF and QEMU, it should take *years* to even get
> >> close to filling the 4MB unified flash image of OVMF (hint: link-time
> >> optimization, LZMA compression), let alone to exhaust the twice as large
> >> (8MB) QEMU allowance.
> >>
> >> Unless you are committed to open source your guest firmware too (maybe
> >> as part of upstream edk2, maybe elsewhere), I seriously doubt we should
> >> accommodate this use case in *upstream* QEMU. It complicates things
> >> (minimally with regard to migration), and currently I don't see the
> >> benefit to the upstream community.
> >>
> >> Clearly, for building your firmware image, you must have minimally
> >> modified the DSC and FDF files in OVMF too, or created an entirely
> >> separate firmware platform -- DSC and FDF both.
> >>
> >> If you are using your downstream OVMF build as a testbed for your
> >> proprietary physical platform firmware, that's generally a use case that
> >> we're mildly interested in not breaking in upstream OvmfPkg. But in
> >> order to make me care for real (as an OvmfPkg co-maintainer), you'd need
> >> to upstream your OVMF platform to edk2 (we already have Xen and --
> >> recently added -- bhyve firmware platforms under OvmfPkg, not just
> >> QEMU). You'd also have to offer long-term reviewership and testing for
> >> that platform in edk2 (like the Xen and bhyve stake-holders do). Then we
> >> could consider additional complexity in QEMU for booting that firmware
> >> platform.
> > 
> > Our UEFI firmware is pretty sparse;
> 
> Yes, in part because I strive to keep it that way.

But that's your choice, on our firmware implementation; that's not a
requirement of QEMU or q35.

> I fight to keep out
> all cruft that I can (at least by conditionalizing it) so that there is
> room for the stuff that I cannot keep out. (And I always strive to set
> expectations that flipping all possible build knobs in OVMF to "on" may
> very well cause an "out of space" error.)
> 
> - I've made sure that PVSCSI_ENABLE and MPT_SCSI_ENABLE be stand-alone
> config knobs. The use case behind them is valid, the drivers are open
> source, but the use case is still niche, so they must be easy to keep out.
> 
> - I've made sure LSI_SCSI_ENABLE is a stand-alone config knob too (and
> it even defaults to FALSE). The QEMU device that the driver drives is
> obsolete / deprecated.
> 
> - If VMBus drivers are ever going to be contributed, they'll need a
> config knob.
> 
> - Current Xen code in OVMF is supposed to be separated completely to the
> new, dedicated XenPVH platform
> <https://bugzilla.tianocore.org/show_bug.cgi?id=2122>.
> 
> - Bhyve support is a separate platform build.
> 
> - Capsule updates are not supported by OVMF, and if they will ever be,
> they're going to have to be a separate firmware platform. Datacenter
> virtualization has no use for capsule updates.
> 
> - The next big thing I expect to have to keep out of OVMF is Redfish
> <https://en.wikipedia.org/wiki/Redfish_(specification)>. Libvirt,
> OpenStack, Cockpit, Kubernetes already handle that area *underneath* the
> guest, I believe. (It's OK to use OVMF for developing / testing Redfish;
> it's not OK to expect that the current OVMF firmware platform contain
> everything that it contains now *plus* Redfish.)
> 
> Sparse is *good*.
> 
> > it doesn't have any pretty graphics
> > or snazzy stuff,
> 
> Which is arguably completely superfluous on every possible platform one
> can imagine. On the other hand, if you want a real serial port on
> workstation class hardware, you may have to order a separate part (if
> you are lucky and you *can* order one). Serial-over-LAN is a complete
> non-replacement.
> 
> The reason (should I say: excuse) for the firmware to exist is to (a)
> boot operating systems, (b) abstract away some ugly platform-specific
> hardware for OS runtime (by providing ACPI and SMBIOS descriptions, and
> a *small* set of runtime services). We can maybe add (c) "root of trust".
> 
> In practice, physical firmware is becoming the hw vendor's OS before
> (and under) your OS, one you cannot replace, and one whose updates can
> brick your hardware. Permitting the same feature creep on virtual
> platforms is wrong.

On the firmware you develop that choice is fine; but there's no reason
to force that restriction onto others.

> > or have to survive configuring lots of hardware; also
> > I'm aware of other companies who are looking at putting big blobs
> > of stuff in firmware for open uses;
> 
> Key term being "open uses". Let us see them.

Well yes, I think we know who we're speaking about here and we're
working on it.

> > so I don't see a problem with
> > changing this limit from the QEMU side of things.
> 
> I do. Software and data always expand to consume all available space,
> and it's going to cause a conflict between UEFI features and platform
> MMIO sooner or later. Then I'll get the privilege of shuffling around
> the crap in OVMF (all of which is "indispensable" or course).
> 
> If we ever go down this route, it needs to benefit open source directly
> and significantly.

Being able to use QEMU to let vendors debug their platform firmware is a
perfectly reasonable use of QEMU; maybe not of your OVMF build - we
need to keep the restrictions on the two separate.

Dave

> Laszlo
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: PATCH: Increase System Firmware Max Size

[McMillan, Erich via]

[-- Attachment #1: Type: text/plain, Size: 11058 bytes --]

Hi Dave, and Laszlo,

I'm not exactly following your email '>' formatting on this discussion so apologies if I've messed it up.


________________________________
From: Dr. David Alan Gilbert <dgilbert@redhat.com>
Sent: Friday, September 11, 2020 10:06 AM
To: Laszlo Ersek <lersek@redhat.com>
Cc: McMillan, Erich <erich.mcmillan@hp.com>; qemu-devel@nongnu.org <qemu-devel@nongnu.org>; mst@redhat.com <mst@redhat.com>; marcel.apfelbaum@gmail.com <marcel.apfelbaum@gmail.com>; qemu-trivial@nongnu.org <qemu-trivial@nongnu.org>; Markus Armbruster <armbru@redhat.com>; Philippe Mathieu-Daudé <philmd@redhat.com>
Subject: Re: PATCH: Increase System Firmware Max Size

>* Laszlo Ersek (lersek@redhat.com) wrote:
> On 09/11/20 10:34, Dr. David Alan Gilbert wrote:
> > * Laszlo Ersek (lersek@redhat.com) wrote:
> >> +Markus, Dave, Phil
> >>
> >> On 09/11/20 03:45, McMillan, Erich wrote:
> >>> Hi all,
> >>>
> >>> (this is my first Qemu patch submission, please let me know if my formatting/content needs to be fixed).
> >>> We have a need for increased firmware size, currently we are building Qemu with the following change to test our Uefi Firmware and it works well for us. Hope that this change can be made to open source. Thank you.
> >>> -------
> >>> Increase allowed system firmware size to 16M per comment suggesting up to 18M is permissible.
> >>>
> >>> Signed-off-by: Erich McMillan <erich.mcmillan@hp.com>
> >>>
> >>> diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
> >>> index b8d8ef59eb17c6ace8194fc69c3d27809becfbc0..f6f7cd744d0690cee0355fbd19ffdcdb71ea75ca 100644
> >>> --- a/hw/i386/pc_sysfw.c
> >>> +++ b/hw/i386/pc_sysfw.c
> >>> @@ -46,7 +46,7 @@
> >>> * only 18MB-4KB below 4G. For now, restrict the cumulative mapping to 8MB in
> >>> * size.
> >>> */
> >>> -#define FLASH_SIZE_LIMIT (8 * MiB)
> >>> +#define FLASH_SIZE_LIMIT (16 * MiB)
> >>>
> >>> #define FLASH_SECTOR_SIZE 4096
> >>> -------
> >>>
> >>>
> >>
> >> (1) This is not a trivial change, so qemu-trivial: please ignore.
> >>
> >> (2) The comment has not been updated.
> >>
> >> (3) I'm almost certain that *if* we want to change this, it needs to be
> >> turned into a machine type (or some device model) property, for
> >> migration compatibility.
> >
> > I'm missing what this constant exists for - why is the
> > check there at all Is there something else that lives below this
> > address that we have to protect?
>
> Yes, some MMIOs that I'm at least aware of are IO_APIC_DEFAULT_ADDRESS
> (0xfec00000), TPM_PPI_ADDR_BASE (0xFED45000), APIC_DEFAULT_ADDRESS
> (0xfee00000).
>
> They are not directly adjacent with pflash; nor should they be.

Hmm those need explicitly checks adding somewhere against
FLASH_SIZE_LIMIT!

> On one hand, the current FLASH_SIZE_LIMIT is meant to be sufficient for
> a long time ("should be enough for anyone").
>
> On the other hand, if we have a really strong reason to increase the
> size limit, the current value is supposed to give us a safety margin, so
> that we can satisfy the immediate need at that point *first*, and start
> looking into (likely more intrusive) physical address map changes, to
> restore the safety margin.
>
> > My reading of the code is that increasing that constant doesn't change
> > the guests view at all, as long as the guest was given the same flash
> > files - so if the guests view doesn't change, then why would we tie
> > it to the machine type?
>
> If you increase the size limit (without tieing it to a machine type),
> then, with an upgraded QEMU and the same (old) machine type, you can
> start a guest with a larger-than-earlier (cumulative) flash size. Then,
> when you try to migrate this to an old QEMU (but same machine type),
> it's a bad surprise. I understand that backwards migration is not
> universally supported (or expected), but I don't want this problem to
> land on my desk *ever*.

I support backwards migration; but that migration wouldn't work anyway -
wouldn't that fail nicely with a mismatched RAMBlock size?

> Furthermore, un-enumerable / platform-MMIO devices tend to pop up time
> after time. TPM_PPI_ADDR_BASE (0xFED45000) is a somewhat recent
> addition, for example. It's not like we're never going to need more
> address space up there.
>
> >
> >> (4) I feel we need much more justification for this change than just
> >> "our firmware is larger than upstream OVMF".
> >>
> >> In the upstream 4MB unified OVMF build, there's *plenty* of free room.
> >> Of course that's not to say that we're willing to *squander* that space
> >> -- whenever we include anything new in the upstream OVMF platform(s),
> >> there must be a very good reason for it --, just that, given the
> >> upstream OVMF status, the proposed pflash increase in QEMU is staggering.
> >>
> >> Considering upstream OVMF and QEMU, it should take *years* to even get
> >> close to filling the 4MB unified flash image of OVMF (hint: link-time
> >> optimization, LZMA compression), let alone to exhaust the twice as large
> >> (8MB) QEMU allowance.
> >>
> >> Unless you are committed to open source your guest firmware too (maybe
> >> as part of upstream edk2, maybe elsewhere), I seriously doubt we should
> >> accommodate this use case in *upstream* QEMU. It complicates things
> >> (minimally with regard to migration), and currently I don't see the
> >> benefit to the upstream community.
> >>
> >> Clearly, for building your firmware image, you must have minimally
> >> modified the DSC and FDF files in OVMF too, or created an entirely
> >> separate firmware platform -- DSC and FDF both.
> >>
> >> If you are using your downstream OVMF build as a testbed for your
> >> proprietary physical platform firmware, that's generally a use case that
> >> we're mildly interested in not breaking in upstream OvmfPkg. But in
> >> order to make me care for real (as an OvmfPkg co-maintainer), you'd need
> >> to upstream your OVMF platform to edk2 (we already have Xen and --
> >> recently added -- bhyve firmware platforms under OvmfPkg, not just
> >> QEMU). You'd also have to offer long-term reviewership and testing for
> >> that platform in edk2 (like the Xen and bhyve stake-holders do). Then we
> >> could consider additional complexity in QEMU for booting that firmware
> >> platform.
> >
> > Our UEFI firmware is pretty sparse;
>
> Yes, in part because I strive to keep it that way.

But that's your choice, on our firmware implementation; that's not a
requirement of QEMU or q35.

> I fight to keep out
> all cruft that I can (at least by conditionalizing it) so that there is
> room for the stuff that I cannot keep out. (And I always strive to set
> expectations that flipping all possible build knobs in OVMF to "on" may
> very well cause an "out of space" error.)
>
> - I've made sure that PVSCSI_ENABLE and MPT_SCSI_ENABLE be stand-alone
> config knobs. The use case behind them is valid, the drivers are open
> source, but the use case is still niche, so they must be easy to keep out.
>
> - I've made sure LSI_SCSI_ENABLE is a stand-alone config knob too (and
> it even defaults to FALSE). The QEMU device that the driver drives is
> obsolete / deprecated.
>
> - If VMBus drivers are ever going to be contributed, they'll need a
> config knob.
>
> - Current Xen code in OVMF is supposed to be separated completely to the
> new, dedicated XenPVH platform
> <https://bugzilla.tianocore.org/show_bug.cgi?id=2122<https://bugzilla.tianocore.org/show_bug.cgi?id=2122>>.
>
> - Bhyve support is a separate platform build.
>
> - Capsule updates are not supported by OVMF, and if they will ever be,
> they're going to have to be a separate firmware platform. Datacenter
> virtualization has no use for capsule updates.
>
> - The next big thing I expect to have to keep out of OVMF is Redfish
> <https://en.wikipedia.org/wiki/Redfish_(specification)<https://en.wikipedia.org/wiki/Redfish_(specification)>>. Libvirt,
> OpenStack, Cockpit, Kubernetes already handle that area *underneath* the
> guest, I believe. (It's OK to use OVMF for developing / testing Redfish;
> it's not OK to expect that the current OVMF firmware platform contain
> everything that it contains now *plus* Redfish.)
>
> Sparse is *good*.
>
> > it doesn't have any pretty graphics
> > or snazzy stuff,
>
> Which is arguably completely superfluous on every possible platform one
> can imagine. On the other hand, if you want a real serial port on
> workstation class hardware, you may have to order a separate part (if
> you are lucky and you *can* order one). Serial-over-LAN is a complete
> non-replacement.
>
> The reason (should I say: excuse) for the firmware to exist is to (a)
> boot operating systems, (b) abstract away some ugly platform-specific
> hardware for OS runtime (by providing ACPI and SMBIOS descriptions, and
> a *small* set of runtime services). We can maybe add (c) "root of trust".
>
> In practice, physical firmware is becoming the hw vendor's OS before
> (and under) your OS, one you cannot replace, and one whose updates can
> brick your hardware. Permitting the same feature creep on virtual
> platforms is wrong.

On the firmware you develop that choice is fine; but there's no reason
to force that restriction onto others.

I agree that fw has become the vendor OS, but at this point there's no going back.
Utilizing a virtual platform allows us to greatly increase the security of our code,
could we make this change a Qemu experimental flag, so that fw vendors could
use it at their own risk?

> > or have to survive configuring lots of hardware; also
> > I'm aware of other companies who are looking at putting big blobs
> > of stuff in firmware for open uses;
>
> Key term being "open uses". Let us see them.

Well yes, I think we know who we're speaking about here and we're
working on it.

> > so I don't see a problem with
> > changing this limit from the QEMU side of things.
>
> I do. Software and data always expand to consume all available space,
> and it's going to cause a conflict between UEFI features and platform
> MMIO sooner or later. Then I'll get the privilege of shuffling around
> the crap in OVMF (all of which is "indispensable" or course).
>
> If we ever go down this route, it needs to benefit open source directly
> and significantly.

Being able to use QEMU to let vendors debug their platform firmware is a
perfectly reasonable use of QEMU; maybe not of your OVMF build - we
need to keep the restrictions on the two separate.

I would hope that continued use of QEMU would encourage us vendors to contribute
more toward improving the open source project. As Dave pointed out, in no way
would anyone be forced (or able) to use vendor fw binaries unless we started to
release them as such, which would not be in our best interests, since we like to sell hardware.

Dave

> Laszlo
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[-- Attachment #2: Type: text/html, Size: 14424 bytes --]

Re: PATCH: Increase System Firmware Max Size

[Laszlo Ersek]

On 09/11/20 17:22, McMillan, Erich wrote:

> I agree that fw has become the vendor OS, but at this point there's no
> going back.
> Utilizing a virtual platform allows us to greatly increase the security
> of our code,
> could we make this change a Qemu experimental flag, so that fw vendors could
> use it at their own risk?

That would make me feel more comfortable, yes.

Daniel proposed "firmware-max-size" (I've now taken the liberty to
replace "_" with "-"; I believe that's the current rule for property names).

If we called it "x-firmware-max-size" and kept the default value
unchanged, I'd feel way safer. (Because then any feature request for
upstream OVMF that were based on changing "x-firmware-max-size" could be
refuted immediately with: "that property name starts with x-, sorry".)

Thanks,
Laszlo

Re: PATCH: Increase System Firmware Max Size

[Daniel P. Berrangé]

On Fri, Sep 11, 2020 at 04:06:02PM +0100, Dr. David Alan Gilbert wrote:
> * Laszlo Ersek (lersek@redhat.com) wrote:
> > On 09/11/20 10:34, Dr. David Alan Gilbert wrote:

> > > it doesn't have any pretty graphics
> > > or snazzy stuff,
> > 
> > Which is arguably completely superfluous on every possible platform one
> > can imagine. On the other hand, if you want a real serial port on
> > workstation class hardware, you may have to order a separate part (if
> > you are lucky and you *can* order one). Serial-over-LAN is a complete
> > non-replacement.
> > 
> > The reason (should I say: excuse) for the firmware to exist is to (a)
> > boot operating systems, (b) abstract away some ugly platform-specific
> > hardware for OS runtime (by providing ACPI and SMBIOS descriptions, and
> > a *small* set of runtime services). We can maybe add (c) "root of trust".
> > 
> > In practice, physical firmware is becoming the hw vendor's OS before
> > (and under) your OS, one you cannot replace, and one whose updates can
> > brick your hardware. Permitting the same feature creep on virtual
> > platforms is wrong.
> 
> On the firmware you develop that choice is fine; but there's no reason
> to force that restriction onto others.
> 
> > > or have to survive configuring lots of hardware; also
> > > I'm aware of other companies who are looking at putting big blobs
> > > of stuff in firmware for open uses;
> > 
> > Key term being "open uses". Let us see them.
> 
> Well yes, I think we know who we're speaking about here and we're
> working on it.

I don't think we need to dictate that firmware used with QEMU has to be
open source.

If someone wants to use a closed source firmware that is fine. They simply
can't expect us to help debug problems in QEMU when using the closed source
firmware, without first demonstrating it with an open source firmware we can
see.

> > > so I don't see a problem with
> > > changing this limit from the QEMU side of things.
> > 
> > I do. Software and data always expand to consume all available space,
> > and it's going to cause a conflict between UEFI features and platform
> > MMIO sooner or later. Then I'll get the privilege of shuffling around
> > the crap in OVMF (all of which is "indispensable" or course).
> > 
> > If we ever go down this route, it needs to benefit open source directly
> > and significantly.
> 
> Being able to use QEMU to let vendors debug their platform firmware is a
> perfectly reasonable use of QEMU; maybe not of your OVMF build - we
> need to keep the restrictions on the two separate.

Agreed, I can understand the motivation to not want to change the QEMU
defaults, but I don't see why we should have this as a hard coded
limit that is not runtime configurable.

IOW, why can't we keep our current default and provide a machine type
property "firmware_max_size" which users can opt-in to setting if
their particular firmware exceeds normal defaults. That won't impact
us for migration compat in any way, and lets users have flexibility t
do what they want.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: PATCH: Increase System Firmware Max Size

[Laszlo Ersek]

On 09/11/20 17:23, Daniel P. Berrangé wrote:

> I don't see why we should have this as a hard coded
> limit that is not runtime configurable.
> 
> IOW, why can't we keep our current default and provide a machine type
> property "firmware_max_size" which users can opt-in to setting if
> their particular firmware exceeds normal defaults. That won't impact
> us for migration compat in any way, and lets users have flexibility t
> do what they want.

Technically, this is fine, in my opinion.

My concerns (in distilled form, this time):

- The change increases maintenance burden.

- The change does not benefit most users of QEMU, as the intended guest
payload will not available to most of them at all (regardless of
licensing terms).

- The existence of the property may entice OVMF users to ask us to
enlarge the *current* OVMF firmware platform and to pack more stuff in
it. That is not OK. My counter-proposal ("please contribute a new
platform DSC/FDF under OvmfPkg, and assume co-reviewership for it")
would almost certainly not be acted upon.

That's all.

Thanks
Laszlo

Re: PATCH: Increase System Firmware Max Size

[Daniel P. Berrangé]

On Fri, Sep 11, 2020 at 06:06:27PM +0200, Laszlo Ersek wrote:
> On 09/11/20 17:23, Daniel P. Berrangé wrote:
> 
> > I don't see why we should have this as a hard coded
> > limit that is not runtime configurable.
> > 
> > IOW, why can't we keep our current default and provide a machine type
> > property "firmware_max_size" which users can opt-in to setting if
> > their particular firmware exceeds normal defaults. That won't impact
> > us for migration compat in any way, and lets users have flexibility t
> > do what they want.
> 
> Technically, this is fine, in my opinion.
> 
> My concerns (in distilled form, this time):
> 
> - The change increases maintenance burden.

I think that is so marginal that its lost in the noise compared to
everything else that's done in QEMU. Essentially someone can pick
a value that is so large that it tickles some other problem in QEMU
or their firmware. We're not promising to fix such problems if they
occurs. It'll be perfectly valid to say "dont do that" if someone
sets a value that breaks something else and we don't consider it
worth the time to investigate and fix.

> - The change does not benefit most users of QEMU, as the intended guest
> payload will not available to most of them at all (regardless of
> licensing terms).

I think that's only relevant if the change is imposing a significant
maint burden which needs to be justified. Not the case here IMHO.

> - The existence of the property may entice OVMF users to ask us to
> enlarge the *current* OVMF firmware platform and to pack more stuff in
> it. That is not OK. My counter-proposal ("please contribute a new
> platform DSC/FDF under OvmfPkg, and assume co-reviewership for it")
> would almost certainly not be acted upon.

I don't see this is relevant. If OVMF maintainers want to reject
feature proposals they have the right to do that regardless of what
QEMU sets for max image size. As you say earlier, the existing size
limit is already enourmous compared to what OVMF actually uses, so
if this was a real problem it'd already exist.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: PATCH: Increase System Firmware Max Size

[Laszlo Ersek]

On 09/11/20 18:21, Daniel P. Berrangé wrote:

> If OVMF maintainers want to reject
> feature proposals they have the right to do that regardless of what
> QEMU sets for max image size. As you say earlier, the existing size
> limit is already enourmous compared to what OVMF actually uses, so
> if this was a real problem it'd already exist.

Very good point -- I have indeed not realized this.

Laszlo

Re: PATCH: Increase System Firmware Max Size

[Laszlo Ersek]

On 09/11/20 17:06, Dr. David Alan Gilbert wrote:
> * Laszlo Ersek (lersek@redhat.com) wrote:
>> On 09/11/20 10:34, Dr. David Alan Gilbert wrote:

>>> I'm missing what this constant exists for - why is the
>>> check there at all  Is there something else that lives below this
>>> address that we have to protect?
>>
>> Yes, some MMIOs that I'm at least aware of are IO_APIC_DEFAULT_ADDRESS
>> (0xfec00000), TPM_PPI_ADDR_BASE (0xFED45000), APIC_DEFAULT_ADDRESS
>> (0xfee00000).
>>
>> They are not directly adjacent with pflash; nor should they be.
> 
> Hmm those need explicitly checks adding somewhere against
> FLASH_SIZE_LIMIT!

Yes, that would be nice. I don't know how it works. Maybe when adding
the next MemoryRegion there's an error or an assert(). No clue.

>> If you increase the size limit (without tieing it to a machine type),
>> then, with an upgraded QEMU and the same (old) machine type, you can
>> start a guest with a larger-than-earlier (cumulative) flash size. Then,
>> when you try to migrate this to an old QEMU (but same machine type),
>> it's a bad surprise. I understand that backwards migration is not
>> universally supported (or expected), but I don't want this problem to
>> land on my desk *ever*.
> 
> I support backwards migration; but that migration wouldn't work anyway -
> wouldn't that fail nicely with a mismatched RAMBlock size?

My point wasn't that the guest would be lost or corrupted, only that it
couldn't be migrated. We'd say "for this, you have to upgrade QEMU on
the destination host as well, or use a smaller firmware", and they'd say
"we don't want either of those things".

>>> Our UEFI firmware is pretty sparse;
>>
>> Yes, in part because I strive to keep it that way.
> 
> But that's your choice, on our firmware implementation; that's not a
> requirement of QEMU or q35.

Right; if we can keep regressions out (not just functional regressions,
but workflow / use case regressions too), then it's OK to support more
use cases.

By workflow / use case regressions I mean that it should not become more
difficult to maintain OVMF as a result of the patch. (It should not
imply that now people can stuff even more cruft into OVMF, because "hey
there's more room now".)

>> The reason (should I say: excuse) for the firmware to exist is to (a)
>> boot operating systems, (b) abstract away some ugly platform-specific
>> hardware for OS runtime (by providing ACPI and SMBIOS descriptions, and
>> a *small* set of runtime services). We can maybe add (c) "root of trust".
>>
>> In practice, physical firmware is becoming the hw vendor's OS before
>> (and under) your OS, one you cannot replace, and one whose updates can
>> brick your hardware. Permitting the same feature creep on virtual
>> platforms is wrong.
> 
> On the firmware you develop that choice is fine; but there's no reason
> to force that restriction onto others.

Technically, I agree. It's fine to run larger UEFI firmware as long as
the default size restriction for the default (or traditional) UEFI
firmware remains the same.

Turn the size limit into a property, keep the same default value,
implement the migration aspects; specify *very clearly* in the commit
message what particular firmware this knob is being introduced for.

... And I'd still be grumpy, because it increases maintenance burden for
QEMU (and possibly OVMF too -- "hey we got more room now!"; see above),
without open source users benefiting from the change. It's not like
we'll ever be able to run, or read the source code of, that 8MB+
firmware image, is it?

>>> or have to survive configuring lots of hardware; also
>>> I'm aware of other companies who are looking at putting big blobs
>>> of stuff in firmware for open uses;
>>
>> Key term being "open uses". Let us see them.
> 
> Well yes, I think we know who we're speaking about here and we're
> working on it.

Sorry, I wasn't clear enough.

I *know* we're going to see *those* "open uses" that you meant.

Precisely because they are "open uses", they have a chance at justifying
the churn.

My intent was to apply your (valid) argument to *this* proposal -- let
us see the "open uses" for *this* particular proposal.

Notice, in the thread starter:

"We have a need for increased firmware size", "our Uefi Firmware",
"change can be made to open source" --> it's obviously for the sake of a
proprietary platform firmware. Do you feel comfortable about taking on
more risks, reviews and maintenance for that?

(Note that I'm not singling out this particular proprietary guest
payload. I feel the exact same way when QEMU is being contorted for
Windows of OSX guests, but at least those guest payloads are universally
available to users, if the users are willing to comply with the
corresponding terms and conditions.)

> Being able to use QEMU to let vendors debug their platform firmware is a
> perfectly reasonable use of QEMU; maybe not of your OVMF build - we
> need to keep the restrictions on the two separate.

You as a QEMU maintainer / reviewer, and I as an OVMF maintainer /
reviewer, are going to pay with our time and effort for a proprietary
guest oriented change that normal QEMU users won't even be able to run,
let alone read, modify, distribute.

But yes, technically speaking, we can replace the size limit constant
with a property, I think.

Laszlo

Re: PATCH: Increase System Firmware Max Size

[Dr. David Alan Gilbert]

* Laszlo Ersek (lersek@redhat.com) wrote:
> On 09/11/20 17:06, Dr. David Alan Gilbert wrote:
> > * Laszlo Ersek (lersek@redhat.com) wrote:
> >> On 09/11/20 10:34, Dr. David Alan Gilbert wrote:
> 
> >>> I'm missing what this constant exists for - why is the
> >>> check there at all  Is there something else that lives below this
> >>> address that we have to protect?
> >>
> >> Yes, some MMIOs that I'm at least aware of are IO_APIC_DEFAULT_ADDRESS
> >> (0xfec00000), TPM_PPI_ADDR_BASE (0xFED45000), APIC_DEFAULT_ADDRESS
> >> (0xfee00000).
> >>
> >> They are not directly adjacent with pflash; nor should they be.
> > 
> > Hmm those need explicitly checks adding somewhere against
> > FLASH_SIZE_LIMIT!
> 
> Yes, that would be nice. I don't know how it works. Maybe when adding
> the next MemoryRegion there's an error or an assert(). No clue.
> 
> >> If you increase the size limit (without tieing it to a machine type),
> >> then, with an upgraded QEMU and the same (old) machine type, you can
> >> start a guest with a larger-than-earlier (cumulative) flash size. Then,
> >> when you try to migrate this to an old QEMU (but same machine type),
> >> it's a bad surprise. I understand that backwards migration is not
> >> universally supported (or expected), but I don't want this problem to
> >> land on my desk *ever*.
> > 
> > I support backwards migration; but that migration wouldn't work anyway -
> > wouldn't that fail nicely with a mismatched RAMBlock size?
> 
> My point wasn't that the guest would be lost or corrupted, only that it
> couldn't be migrated. We'd say "for this, you have to upgrade QEMU on
> the destination host as well, or use a smaller firmware", and they'd say
> "we don't want either of those things".

My point is that already breaks if you used a smaller firmware on the
source and the current size on the destination.

> >>> Our UEFI firmware is pretty sparse;
> >>
> >> Yes, in part because I strive to keep it that way.
> > 
> > But that's your choice, on our firmware implementation; that's not a
> > requirement of QEMU or q35.
> 
> Right; if we can keep regressions out (not just functional regressions,
> but workflow / use case regressions too), then it's OK to support more
> use cases.
> 
> By workflow / use case regressions I mean that it should not become more
> difficult to maintain OVMF as a result of the patch. (It should not
> imply that now people can stuff even more cruft into OVMF, because "hey
> there's more room now".)
> 
> >> The reason (should I say: excuse) for the firmware to exist is to (a)
> >> boot operating systems, (b) abstract away some ugly platform-specific
> >> hardware for OS runtime (by providing ACPI and SMBIOS descriptions, and
> >> a *small* set of runtime services). We can maybe add (c) "root of trust".
> >>
> >> In practice, physical firmware is becoming the hw vendor's OS before
> >> (and under) your OS, one you cannot replace, and one whose updates can
> >> brick your hardware. Permitting the same feature creep on virtual
> >> platforms is wrong.
> > 
> > On the firmware you develop that choice is fine; but there's no reason
> > to force that restriction onto others.
> 
> Technically, I agree. It's fine to run larger UEFI firmware as long as
> the default size restriction for the default (or traditional) UEFI
> firmware remains the same.
> 
> Turn the size limit into a property, keep the same default value,
> implement the migration aspects; specify *very clearly* in the commit
> message what particular firmware this knob is being introduced for.

I don't think there is a migration aspect here; and this knob is a
general knob - it's just Erich here is the poor unfortunate person
who wanted to tune that knob for the first time.

> ... And I'd still be grumpy, because it increases maintenance burden for
> QEMU (and possibly OVMF too -- "hey we got more room now!"; see above),
> without open source users benefiting from the change. It's not like
> we'll ever be able to run, or read the source code of, that 8MB+
> firmware image, is it?

Sorry, not QEMU's problem; we don't restrict RAM to 640k just because
it should be enough for anyone.

> >>> or have to survive configuring lots of hardware; also
> >>> I'm aware of other companies who are looking at putting big blobs
> >>> of stuff in firmware for open uses;
> >>
> >> Key term being "open uses". Let us see them.
> > 
> > Well yes, I think we know who we're speaking about here and we're
> > working on it.
> 
> Sorry, I wasn't clear enough.
> 
> I *know* we're going to see *those* "open uses" that you meant.
> 
> Precisely because they are "open uses", they have a chance at justifying
> the churn.
> 
> My intent was to apply your (valid) argument to *this* proposal -- let
> us see the "open uses" for *this* particular proposal.
> 
> Notice, in the thread starter:
> 
> "We have a need for increased firmware size", "our Uefi Firmware",
> "change can be made to open source" --> it's obviously for the sake of a
> proprietary platform firmware. Do you feel comfortable about taking on
> more risks, reviews and maintenance for that?

I'm not seeing any more risks, reviews or maintenance in QEMU due to it.
Indeed replacing an undocumented, unchecked constant comparison with
a proper property seems better.

> (Note that I'm not singling out this particular proprietary guest
> payload. I feel the exact same way when QEMU is being contorted for
> Windows of OSX guests, but at least those guest payloads are universally
> available to users, if the users are willing to comply with the
> corresponding terms and conditions.)
> 
> > Being able to use QEMU to let vendors debug their platform firmware is a
> > perfectly reasonable use of QEMU; maybe not of your OVMF build - we
> > need to keep the restrictions on the two separate.
> 
> You as a QEMU maintainer / reviewer, and I as an OVMF maintainer /
> reviewer, are going to pay with our time and effort for a proprietary
> guest oriented change that normal QEMU users won't even be able to run,
> let alone read, modify, distribute.

We have lots of complex hideous changes that I'm never going to use but
seem reasonable;  this is a tiny change that seems perfectly reasonable
both for open and closed firmware.
I realise herding OVMF developers is tricky, but that's not a reason
to nack a reasonable almost trivial change in QEMU.

Dave

> But yes, technically speaking, we can replace the size limit constant
> with a property, I think.
> 
> Laszlo
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: PATCH: Increase System Firmware Max Size

[Laszlo Ersek]

On 09/11/20 18:22, Dr. David Alan Gilbert wrote:

> We have lots of complex hideous changes that I'm never going to use but
> seem reasonable;  this is a tiny change that seems perfectly reasonable
> both for open and closed firmware.
> I realise herding OVMF developers is tricky, but that's not a reason
> to nack a reasonable almost trivial change in QEMU.

OK.

Laszlo

Re: PATCH: Increase System Firmware Max Size

[Dr. David Alan Gilbert]

* Laszlo Ersek (lersek@redhat.com) wrote:
> On 09/11/20 18:22, Dr. David Alan Gilbert wrote:
> 
> > We have lots of complex hideous changes that I'm never going to use but
> > seem reasonable;  this is a tiny change that seems perfectly reasonable
> > both for open and closed firmware.
> > I realise herding OVMF developers is tricky, but that's not a reason
> > to nack a reasonable almost trivial change in QEMU.
> 
> OK.

I'm OK with pushing it towards being a property though; that's much
prettier; so:

Erich: Can you redo this as a property as suggested in another part
of this thread, making it default to the current value?

Dave

> Laszlo
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: PATCH: Increase System Firmware Max Size

[McMillan, Erich via]

[-- Attachment #1: Type: text/plain, Size: 1191 bytes --]

I’d be happy to rewrite as a property.

Erich

Get Outlook for iOS<https://aka.ms/o0ukef>
________________________________
From: Dr. David Alan Gilbert <dgilbert@redhat.com>
Sent: Friday, September 11, 2020 11:59 AM
To: Laszlo Ersek
Cc: McMillan, Erich; qemu-devel@nongnu.org; mst@redhat.com; marcel.apfelbaum@gmail.com; qemu-trivial@nongnu.org; Markus Armbruster; Philippe Mathieu-Daudé
Subject: Re: PATCH: Increase System Firmware Max Size

* Laszlo Ersek (lersek@redhat.com) wrote:
> On 09/11/20 18:22, Dr. David Alan Gilbert wrote:
>
> > We have lots of complex hideous changes that I'm never going to use but
> > seem reasonable;  this is a tiny change that seems perfectly reasonable
> > both for open and closed firmware.
> > I realise herding OVMF developers is tricky, but that's not a reason
> > to nack a reasonable almost trivial change in QEMU.
>
> OK.

I'm OK with pushing it towards being a property though; that's much
prettier; so:

Erich: Can you redo this as a property as suggested in another part
of this thread, making it default to the current value?

Dave

> Laszlo
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK


[-- Attachment #2: Type: text/html, Size: 2334 bytes --]

Re: PATCH: Increase System Firmware Max Size

[McMillan, Erich]

[-- Attachment #1: Type: text/plain, Size: 6772 bytes --]

Hi all,

I've rewritten the FLASH_SIZE_LIMIT as a command line parameter as requested, but I'd like some feedback. My current concerns are:

  1.  I'm not too happy using an global variable in this manner, but I'm not sure the appropriate way to share this information between vl.c and pc_sysfw.c. Is there a way for other .c modules to query the QemuOpts, or is this not preferred.
  2.  Would appreciate feedback on the help strings, I think the formatting is correct based on -m (memory size) flag.
  3.  If global variable is acceptable then is it appropriate for it to be shared via loader.h, this seemed the most appropriate place to put it without adding new includes to either vl.c or pc_sysfw.c.

Thank you,
Erich

diff --git a/softmmu/vl.c b/softmmu/vl.c
index 05d1a4cb6bf863b6ac1df8f94694c073fa4f8d90..a34995819fa14ef728d82ab179ef3a2e468e2365 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -442,6 +442,20 @@ static QemuOptsList qemu_mem_opts = {
     },
 };

+static QemuOptsList qemu_max_fw_size_opts = {
+    .name = "maxfirmwaresize",
+    .implied_opt_name = "fwsize",
+    .head = QTAILQ_HEAD_INITIALIZER(qemu_max_fw_size_opts.head),
+    .merge_lists = true,
+    .desc = {
+        {
+            .name = "size",
+            .type = QEMU_OPT_SIZE,
+        },
+        { /* end of list */ }
+    },
+};
+
 static QemuOptsList qemu_icount_opts = {
     .name = "icount",
     .implied_opt_name = "shift",
@@ -2559,6 +2573,23 @@ static bool object_create_delayed(const char *type, QemuOpts *opts)
     return !object_create_initial(type, opts);
 }

+static void set_max_firmware_size(uint64_t *maxfwsize)
+{
+    const char *max_fw_size_str;
+    QemuOpts *opts = qemu_find_opts_singleton("maxfirmwaresize");
+
+    max_fw_size_str = qemu_opt_get(opts, "size");
+
+    if (max_fw_size_str) {
+        if (!*max_fw_size_str) {
+            error_report("missing 'size' option value");
+            exit(EXIT_FAILURE);
+        }
+
+        *maxfwsize = qemu_opt_get_size(opts, "size", 8 * MiB);
+    }
+}
+

 static bool set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size,
                                MachineClass *mc)
@@ -2887,6 +2918,7 @@ void qemu_init(int argc, char **argv, char **envp)
     qemu_add_opts(&qemu_machine_opts);
     qemu_add_opts(&qemu_accel_opts);
     qemu_add_opts(&qemu_mem_opts);
+    qemu_add_opts(&qemu_max_fw_size_opts);
     qemu_add_opts(&qemu_smp_opts);
     qemu_add_opts(&qemu_boot_opts);
     qemu_add_opts(&qemu_add_fd_opts);
@@ -3143,6 +3175,10 @@ void qemu_init(int argc, char **argv, char **envp)
                     exit(EXIT_FAILURE);
                 }
                 break;
+            case QEMU_OPTION_maxfirmwaresize:
+                opts = qemu_opts_parse_noisily(qemu_find_opts("maxfirmwaresize"),
+                                               optarg, true);
+                break;
 #ifdef CONFIG_TPM
             case QEMU_OPTION_tpmdev:
                 if (tpm_config_parse(qemu_find_opts("tpmdev"), optarg) < 0) {
@@ -3831,6 +3867,8 @@ void qemu_init(int argc, char **argv, char **envp)
     have_custom_ram_size = set_memory_options(&ram_slots, &maxram_size,
                                               machine_class);

+    set_max_firmware_size(&MaxCombinedFirmwareSize);
+
     os_daemonize();

     /*
diff --git a/include/hw/loader.h b/include/hw/loader.h
index a9eeea39521d2d5b5e9b73e0fcbd4d4ce9292046..9e982cd60f8f2173a3160f563167e48b7ca88aa9 100644
--- a/include/hw/loader.h
+++ b/include/hw/loader.h
@@ -318,4 +318,13 @@ int rom_add_option(const char *file, int32_t bootindex);
  * overflow on real hardware too. */
 #define UBOOT_MAX_GUNZIP_BYTES (64 << 20)

+/*
+ * 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
+ * [0xFEE00000..0xFEE01000] -- see IO_APIC_DEFAULT_ADDRESS --, leaving free
+ * only 18MB-4KB below 4G. For now, restrict the cumulative mapping to 8MB in
+ * size, but allow user to specify larger size via command line.
+ */
+uint64_t MaxCombinedFirmwareSize = (8 * MiB);
+
 #endif
diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index b8d8ef59eb17c6ace8194fc69c3d27809becfbc0..012c28a3b4de1c1618404faefd63a99267636935 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -39,14 +39,8 @@
 #include "hw/block/flash.h"
 #include "sysemu/kvm.h"

-/*
- * 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
- * [0xFEE00000..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)
+
+extern MaxCombinedFirmwareSize;

 #define FLASH_SECTOR_SIZE 4096

@@ -177,10 +171,10 @@ static void pc_system_flash_map(PCMachineState *pcms,
         }
         if ((hwaddr)size != size
             || total_size > HWADDR_MAX - size
-            || total_size + size > FLASH_SIZE_LIMIT) {
+            || total_size + size > MaxCombinedFirmwareSize) {
             error_report("combined size of system firmware exceeds "
                          "%" PRIu64 " bytes",
-                         FLASH_SIZE_LIMIT);
+                         MaxCombinedFirmwareSize);
             exit(1);
         }




________________________________
From: McMillan, Erich
Sent: Thursday, September 10, 2020 8:45 PM
To: qemu-devel@nongnu.org <qemu-devel@nongnu.org>
Cc: mst@redhat.com <mst@redhat.com>; marcel.apfelbaum@gmail.com <marcel.apfelbaum@gmail.com>; qemu-trivial@nongnu.org <qemu-trivial@nongnu.org>
Subject: PATCH: Increase System Firmware Max Size

Hi all,

(this is my first Qemu patch submission, please let me know if my formatting/content needs to be fixed).
We have a need for increased firmware size, currently we are building Qemu with the following change to test our Uefi Firmware and it works well for us. Hope that this change can be made to open source. Thank you.
-------
Increase allowed system firmware size to 16M per comment suggesting up to 18M is permissible.

 Signed-off-by: Erich McMillan <erich.mcmillan@hp.com>

diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index b8d8ef59eb17c6ace8194fc69c3d27809becfbc0..f6f7cd744d0690cee0355fbd19ffdcdb71ea75ca 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -46,7 +46,7 @@
  * only 18MB-4KB below 4G. For now, restrict the cumulative mapping to 8MB in
  * size.
  */
-#define FLASH_SIZE_LIMIT (8 * MiB)
+#define FLASH_SIZE_LIMIT (16 * MiB)

 #define FLASH_SECTOR_SIZE 4096
-------


[-- Attachment #2: Type: text/html, Size: 13074 bytes --]

Re: PATCH: Increase System Firmware Max Size

[McMillan, Erich via]

[-- Attachment #1: Type: text/plain, Size: 13250 bytes --]

Apologies, ignore previous patch. The relevant patch is below:

From 473daf6129debf8d158a9ae1aff788c5bdbbc799 Mon Sep 17 00:00:00 2001
From: Erich McMillan <erich.mcmillan@hp.com>
Date: Tue, 15 Sep 2020 13:23:25 -0500
Subject: [PATCH 2/2] Add max firmware size as optional parameter

Signed-off-by: Erich McMillan <erich.mcmillan@hp.com>
---
 hw/i386/pc_sysfw.c  | 13 ++-----------
 include/hw/loader.h |  9 +++++++++
 qemu-options.hx     |  8 ++++++++
 softmmu/vl.c        | 40 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 59 insertions(+), 11 deletions(-)

diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index b6c0822..ba6c99d 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -39,15 +39,6 @@
 #include "hw/block/flash.h"
 #include "sysemu/kvm.h"

-/*
- * 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
- * [0xFEE00000..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,
@@ -182,10 +173,10 @@ static void pc_system_flash_map(PCMachineState *pcms,
         }
         if ((hwaddr)size != size
             || total_size > HWADDR_MAX - size
-            || total_size + size > FLASH_SIZE_LIMIT) {
+            || total_size + size > MaxCombinedFirmwareSize) {
             error_report("combined size of system firmware exceeds "
                          "%" PRIu64 " bytes",
-                         FLASH_SIZE_LIMIT);
+                         MaxCombinedFirmwareSize);
             exit(1);
         }

diff --git a/include/hw/loader.h b/include/hw/loader.h
index a9eeea3..7898b63 100644
--- a/include/hw/loader.h
+++ b/include/hw/loader.h
@@ -318,4 +318,13 @@ int rom_add_option(const char *file, int32_t bootindex);
  * overflow on real hardware too. */
 #define UBOOT_MAX_GUNZIP_BYTES (64 << 20)

+/*
+ * 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
+ * [0xFEE00000..0xFEE01000] -- see IO_APIC_DEFAULT_ADDRESS --, leaving free
+ * only 18MB-4KB below 4G. For now, restrict the cumulative mapping to 8MB in
+ * size, but allow user to specify larger size via command line.
+ */
+extern uint64_t MaxCombinedFirmwareSize;
+
 #endif
diff --git a/qemu-options.hx b/qemu-options.hx
index b0f0205..32eed3a 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1377,6 +1377,14 @@ SRST
         |qemu_system_x86| -hda a -hdb b
 ERST

+DEF("maxfirmwaresize", HAS_ARG, QEMU_OPTION_maxfirmwaresize,
+    "-maxfirmwaresize [size=]megs  specify maximum combined firmware size, default is 8MiB. Known issues if value exceeds 16MiB.\n",
+    QEMU_ARCH_ALL)
+SRST
+``-maxfirmwaresize [size=]megs``
+    Specify maximum combined firmware size, default is 8MiB. Known issues if value exceeds 16MiB.
+ERST
+
 DEF("mtdblock", HAS_ARG, QEMU_OPTION_mtdblock,
     "-mtdblock file  use 'file' as on-board Flash memory image\n",
     QEMU_ARCH_ALL)
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 0cc86b0..fcf41d2 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -116,6 +116,8 @@

 #define MAX_VIRTIO_CONSOLES 1

+uint64_t MaxCombinedFirmwareSize = 8 * MiB;
+
 static const char *data_dir[16];
 static int data_dir_idx;
 const char *bios_name = NULL;
@@ -448,6 +450,20 @@ static QemuOptsList qemu_mem_opts = {
     },
 };

+static QemuOptsList qemu_max_fw_size_opts = {
+    .name = "maxfirmwaresize",
+    .implied_opt_name = "size",
+    .head = QTAILQ_HEAD_INITIALIZER(qemu_max_fw_size_opts.head),
+    .merge_lists = true,
+    .desc = {
+        {
+            .name = "size",
+            .type = QEMU_OPT_SIZE,
+        },
+        { /* end of list */ }
+    },
+};
+
 static QemuOptsList qemu_icount_opts = {
     .name = "icount",
     .implied_opt_name = "shift",
@@ -2576,6 +2592,23 @@ static bool object_create_delayed(const char *type, QemuOpts *opts)
     return !object_create_initial(type, opts);
 }

+static void set_max_firmware_size(uint64_t *maxfwsize)
+{
+    const char *max_fw_size_str;
+    QemuOpts *opts = qemu_find_opts_singleton("maxfirmwaresize");
+
+    max_fw_size_str = qemu_opt_get(opts, "size");
+
+    if (max_fw_size_str) {
+        if (!*max_fw_size_str) {
+            error_report("missing 'size' option value");
+            exit(EXIT_FAILURE);
+        }
+
+        *maxfwsize = qemu_opt_get_size(opts, "size", 8 * MiB);
+    }
+}
+

 static bool set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size,
                                MachineClass *mc)
@@ -2904,6 +2937,7 @@ void qemu_init(int argc, char **argv, char **envp)
     qemu_add_opts(&qemu_machine_opts);
     qemu_add_opts(&qemu_accel_opts);
     qemu_add_opts(&qemu_mem_opts);
+    qemu_add_opts(&qemu_max_fw_size_opts);
     qemu_add_opts(&qemu_smp_opts);
     qemu_add_opts(&qemu_boot_opts);
     qemu_add_opts(&qemu_add_fd_opts);
@@ -3160,6 +3194,10 @@ void qemu_init(int argc, char **argv, char **envp)
                     exit(EXIT_FAILURE);
                 }
                 break;
+            case QEMU_OPTION_maxfirmwaresize:
+                opts = qemu_opts_parse_noisily(qemu_find_opts("maxfirmwaresize"),
+                                               optarg, true);
+                break;
 #ifdef CONFIG_TPM
             case QEMU_OPTION_tpmdev:
                 if (tpm_config_parse(qemu_find_opts("tpmdev"), optarg) < 0) {
@@ -3845,6 +3883,8 @@ void qemu_init(int argc, char **argv, char **envp)
     have_custom_ram_size = set_memory_options(&ram_slots, &maxram_size,
                                               machine_class);

+    set_max_firmware_size(&MaxCombinedFirmwareSize);
+
     os_daemonize();
     rcu_disable_atfork();

--
2.25.1



________________________________
From: McMillan, Erich <erich.mcmillan@hp.com>
Sent: Tuesday, September 15, 2020 2:09 PM
To: qemu-devel@nongnu.org <qemu-devel@nongnu.org>
Cc: mst@redhat.com <mst@redhat.com>; marcel.apfelbaum@gmail.com <marcel.apfelbaum@gmail.com>; qemu-trivial@nongnu.org <qemu-trivial@nongnu.org>
Subject: Re: PATCH: Increase System Firmware Max Size

Hi all,

I've rewritten the FLASH_SIZE_LIMIT as a command line parameter as requested, but I'd like some feedback. My current concerns are:

  1.  I'm not too happy using an global variable in this manner, but I'm not sure the appropriate way to share this information between vl.c and pc_sysfw.c. Is there a way for other .c modules to query the QemuOpts, or is this not preferred.
  2.  Would appreciate feedback on the help strings, I think the formatting is correct based on -m (memory size) flag.
  3.  If global variable is acceptable then is it appropriate for it to be shared via loader.h, this seemed the most appropriate place to put it without adding new includes to either vl.c or pc_sysfw.c.

Thank you,
Erich

diff --git a/softmmu/vl.c b/softmmu/vl.c
index 05d1a4cb6bf863b6ac1df8f94694c073fa4f8d90..a34995819fa14ef728d82ab179ef3a2e468e2365 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -442,6 +442,20 @@ static QemuOptsList qemu_mem_opts = {
     },
 };

+static QemuOptsList qemu_max_fw_size_opts = {
+    .name = "maxfirmwaresize",
+    .implied_opt_name = "fwsize",
+    .head = QTAILQ_HEAD_INITIALIZER(qemu_max_fw_size_opts.head),
+    .merge_lists = true,
+    .desc = {
+        {
+            .name = "size",
+            .type = QEMU_OPT_SIZE,
+        },
+        { /* end of list */ }
+    },
+};
+
 static QemuOptsList qemu_icount_opts = {
     .name = "icount",
     .implied_opt_name = "shift",
@@ -2559,6 +2573,23 @@ static bool object_create_delayed(const char *type, QemuOpts *opts)
     return !object_create_initial(type, opts);
 }

+static void set_max_firmware_size(uint64_t *maxfwsize)
+{
+    const char *max_fw_size_str;
+    QemuOpts *opts = qemu_find_opts_singleton("maxfirmwaresize");
+
+    max_fw_size_str = qemu_opt_get(opts, "size");
+
+    if (max_fw_size_str) {
+        if (!*max_fw_size_str) {
+            error_report("missing 'size' option value");
+            exit(EXIT_FAILURE);
+        }
+
+        *maxfwsize = qemu_opt_get_size(opts, "size", 8 * MiB);
+    }
+}
+

 static bool set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size,
                                MachineClass *mc)
@@ -2887,6 +2918,7 @@ void qemu_init(int argc, char **argv, char **envp)
     qemu_add_opts(&qemu_machine_opts);
     qemu_add_opts(&qemu_accel_opts);
     qemu_add_opts(&qemu_mem_opts);
+    qemu_add_opts(&qemu_max_fw_size_opts);
     qemu_add_opts(&qemu_smp_opts);
     qemu_add_opts(&qemu_boot_opts);
     qemu_add_opts(&qemu_add_fd_opts);
@@ -3143,6 +3175,10 @@ void qemu_init(int argc, char **argv, char **envp)
                     exit(EXIT_FAILURE);
                 }
                 break;
+            case QEMU_OPTION_maxfirmwaresize:
+                opts = qemu_opts_parse_noisily(qemu_find_opts("maxfirmwaresize"),
+                                               optarg, true);
+                break;
 #ifdef CONFIG_TPM
             case QEMU_OPTION_tpmdev:
                 if (tpm_config_parse(qemu_find_opts("tpmdev"), optarg) < 0) {
@@ -3831,6 +3867,8 @@ void qemu_init(int argc, char **argv, char **envp)
     have_custom_ram_size = set_memory_options(&ram_slots, &maxram_size,
                                               machine_class);

+    set_max_firmware_size(&MaxCombinedFirmwareSize);
+
     os_daemonize();

     /*
diff --git a/include/hw/loader.h b/include/hw/loader.h
index a9eeea39521d2d5b5e9b73e0fcbd4d4ce9292046..9e982cd60f8f2173a3160f563167e48b7ca88aa9 100644
--- a/include/hw/loader.h
+++ b/include/hw/loader.h
@@ -318,4 +318,13 @@ int rom_add_option(const char *file, int32_t bootindex);
  * overflow on real hardware too. */
 #define UBOOT_MAX_GUNZIP_BYTES (64 << 20)

+/*
+ * 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
+ * [0xFEE00000..0xFEE01000] -- see IO_APIC_DEFAULT_ADDRESS --, leaving free
+ * only 18MB-4KB below 4G. For now, restrict the cumulative mapping to 8MB in
+ * size, but allow user to specify larger size via command line.
+ */
+uint64_t MaxCombinedFirmwareSize = (8 * MiB);
+
 #endif
diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index b8d8ef59eb17c6ace8194fc69c3d27809becfbc0..012c28a3b4de1c1618404faefd63a99267636935 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -39,14 +39,8 @@
 #include "hw/block/flash.h"
 #include "sysemu/kvm.h"

-/*
- * 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
- * [0xFEE00000..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)
+
+extern MaxCombinedFirmwareSize;

 #define FLASH_SECTOR_SIZE 4096

@@ -177,10 +171,10 @@ static void pc_system_flash_map(PCMachineState *pcms,
         }
         if ((hwaddr)size != size
             || total_size > HWADDR_MAX - size
-            || total_size + size > FLASH_SIZE_LIMIT) {
+            || total_size + size > MaxCombinedFirmwareSize) {
             error_report("combined size of system firmware exceeds "
                          "%" PRIu64 " bytes",
-                         FLASH_SIZE_LIMIT);
+                         MaxCombinedFirmwareSize);
             exit(1);
         }




________________________________
From: McMillan, Erich
Sent: Thursday, September 10, 2020 8:45 PM
To: qemu-devel@nongnu.org <qemu-devel@nongnu.org>
Cc: mst@redhat.com <mst@redhat.com>; marcel.apfelbaum@gmail.com <marcel.apfelbaum@gmail.com>; qemu-trivial@nongnu.org <qemu-trivial@nongnu.org>
Subject: PATCH: Increase System Firmware Max Size

Hi all,

(this is my first Qemu patch submission, please let me know if my formatting/content needs to be fixed).
We have a need for increased firmware size, currently we are building Qemu with the following change to test our Uefi Firmware and it works well for us. Hope that this change can be made to open source. Thank you.
-------
Increase allowed system firmware size to 16M per comment suggesting up to 18M is permissible.

 Signed-off-by: Erich McMillan <erich.mcmillan@hp.com>

diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index b8d8ef59eb17c6ace8194fc69c3d27809becfbc0..f6f7cd744d0690cee0355fbd19ffdcdb71ea75ca 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -46,7 +46,7 @@
  * only 18MB-4KB below 4G. For now, restrict the cumulative mapping to 8MB in
  * size.
  */
-#define FLASH_SIZE_LIMIT (8 * MiB)
+#define FLASH_SIZE_LIMIT (16 * MiB)

 #define FLASH_SECTOR_SIZE 4096
-------


[-- Attachment #2: Type: text/html, Size: 24515 bytes --]

Re: PATCH: Increase System Firmware Max Size

[Laszlo Ersek]

Hi Erich,

(1) this patch is really not trivial; please do not continue CC'ing
qemu-trivial

(2) Please do CC people that have given you feedback previously. I
primarily mean Daniel and David.

(3) Generally speaking, please post new versions of a patch stand-alone
(not in reply to another message) on the list.

(4) Please use git-send-email (or suitable wrapper utilities) for
sending your patch.

https://wiki.qemu.org/Contribute/SubmitAPatch


One non-meta comment below:

On 09/15/20 21:10, McMillan, Erich via wrote:
> Apologies, ignore previous patch. The relevant patch is below:
>
> From 473daf6129debf8d158a9ae1aff788c5bdbbc799 Mon Sep 17 00:00:00 2001
> From: Erich McMillan <erich.mcmillan@hp.com>
> Date: Tue, 15 Sep 2020 13:23:25 -0500
> Subject: [PATCH 2/2] Add max firmware size as optional parameter
>
> Signed-off-by: Erich McMillan <erich.mcmillan@hp.com>
> ---
>  hw/i386/pc_sysfw.c  | 13 ++-----------
>  include/hw/loader.h |  9 +++++++++
>  qemu-options.hx     |  8 ++++++++
>  softmmu/vl.c        | 40 ++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 59 insertions(+), 11 deletions(-)
>
> diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
> index b6c0822..ba6c99d 100644
> --- a/hw/i386/pc_sysfw.c
> +++ b/hw/i386/pc_sysfw.c
> @@ -39,15 +39,6 @@
>  #include "hw/block/flash.h"
>  #include "sysemu/kvm.h"
>
> -/*
> - * 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
> - * [0xFEE00000..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,
> @@ -182,10 +173,10 @@ static void pc_system_flash_map(PCMachineState *pcms,
>          }
>          if ((hwaddr)size != size
>              || total_size > HWADDR_MAX - size
> -            || total_size + size > FLASH_SIZE_LIMIT) {
> +            || total_size + size > MaxCombinedFirmwareSize) {
>              error_report("combined size of system firmware exceeds "
>                           "%" PRIu64 " bytes",
> -                         FLASH_SIZE_LIMIT);
> +                         MaxCombinedFirmwareSize);
>              exit(1);
>          }
>
> diff --git a/include/hw/loader.h b/include/hw/loader.h
> index a9eeea3..7898b63 100644
> --- a/include/hw/loader.h
> +++ b/include/hw/loader.h
> @@ -318,4 +318,13 @@ int rom_add_option(const char *file, int32_t bootindex);
>   * overflow on real hardware too. */
>  #define UBOOT_MAX_GUNZIP_BYTES (64 << 20)
>
> +/*
> + * 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
> + * [0xFEE00000..0xFEE01000] -- see IO_APIC_DEFAULT_ADDRESS --, leaving free
> + * only 18MB-4KB below 4G. For now, restrict the cumulative mapping to 8MB in
> + * size, but allow user to specify larger size via command line.
> + */
> +extern uint64_t MaxCombinedFirmwareSize;
> +
>  #endif
> diff --git a/qemu-options.hx b/qemu-options.hx
> index b0f0205..32eed3a 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -1377,6 +1377,14 @@ SRST
>          |qemu_system_x86| -hda a -hdb b
>  ERST
>
> +DEF("maxfirmwaresize", HAS_ARG, QEMU_OPTION_maxfirmwaresize,
> +    "-maxfirmwaresize [size=]megs  specify maximum combined firmware size, default is 8MiB. Known issues if value exceeds 16MiB.\n",
> +    QEMU_ARCH_ALL)
> +SRST
> +``-maxfirmwaresize [size=]megs``
> +    Specify maximum combined firmware size, default is 8MiB. Known issues if value exceeds 16MiB.
> +ERST
> +
>  DEF("mtdblock", HAS_ARG, QEMU_OPTION_mtdblock,
>      "-mtdblock file  use 'file' as on-board Flash memory image\n",
>      QEMU_ARCH_ALL)
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 0cc86b0..fcf41d2 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -116,6 +116,8 @@
>
>  #define MAX_VIRTIO_CONSOLES 1
>
> +uint64_t MaxCombinedFirmwareSize = 8 * MiB;
> +
>  static const char *data_dir[16];
>  static int data_dir_idx;
>  const char *bios_name = NULL;
> @@ -448,6 +450,20 @@ static QemuOptsList qemu_mem_opts = {
>      },
>  };
>
> +static QemuOptsList qemu_max_fw_size_opts = {
> +    .name = "maxfirmwaresize",
> +    .implied_opt_name = "size",
> +    .head = QTAILQ_HEAD_INITIALIZER(qemu_max_fw_size_opts.head),
> +    .merge_lists = true,
> +    .desc = {
> +        {
> +            .name = "size",
> +            .type = QEMU_OPT_SIZE,
> +        },
> +        { /* end of list */ }
> +    },
> +};
> +
>  static QemuOptsList qemu_icount_opts = {
>      .name = "icount",
>      .implied_opt_name = "shift",
> @@ -2576,6 +2592,23 @@ static bool object_create_delayed(const char *type, QemuOpts *opts)
>      return !object_create_initial(type, opts);
>  }
>
> +static void set_max_firmware_size(uint64_t *maxfwsize)
> +{
> +    const char *max_fw_size_str;
> +    QemuOpts *opts = qemu_find_opts_singleton("maxfirmwaresize");
> +
> +    max_fw_size_str = qemu_opt_get(opts, "size");
> +
> +    if (max_fw_size_str) {
> +        if (!*max_fw_size_str) {
> +            error_report("missing 'size' option value");
> +            exit(EXIT_FAILURE);
> +        }
> +
> +        *maxfwsize = qemu_opt_get_size(opts, "size", 8 * MiB);
> +    }
> +}
> +
>
>  static bool set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size,
>                                 MachineClass *mc)
> @@ -2904,6 +2937,7 @@ void qemu_init(int argc, char **argv, char **envp)
>      qemu_add_opts(&qemu_machine_opts);
>      qemu_add_opts(&qemu_accel_opts);
>      qemu_add_opts(&qemu_mem_opts);
> +    qemu_add_opts(&qemu_max_fw_size_opts);
>      qemu_add_opts(&qemu_smp_opts);
>      qemu_add_opts(&qemu_boot_opts);
>      qemu_add_opts(&qemu_add_fd_opts);
> @@ -3160,6 +3194,10 @@ void qemu_init(int argc, char **argv, char **envp)
>                      exit(EXIT_FAILURE);
>                  }
>                  break;
> +            case QEMU_OPTION_maxfirmwaresize:
> +                opts = qemu_opts_parse_noisily(qemu_find_opts("maxfirmwaresize"),
> +                                               optarg, true);
> +                break;
>  #ifdef CONFIG_TPM
>              case QEMU_OPTION_tpmdev:
>                  if (tpm_config_parse(qemu_find_opts("tpmdev"), optarg) < 0) {
> @@ -3845,6 +3883,8 @@ void qemu_init(int argc, char **argv, char **envp)
>      have_custom_ram_size = set_memory_options(&ram_slots, &maxram_size,
>                                                machine_class);
>
> +    set_max_firmware_size(&MaxCombinedFirmwareSize);
> +
>      os_daemonize();
>      rcu_disable_atfork();
>

(5) In my opinion (which could be wrong of course), we shouldn't
introduce a new command line option for this, but a new PC machine type
property called "x-firmware-max-size".

Please look at the object_class_property_add() calls in
pc_machine_class_init() [hw/i386/pc.c].

I think the PC_MACHINE_MAX_RAM_BELOW_4G property is a good example to
imitate:

- It has type "size".

- It comes with a getter and a setter, and an associated field in
PCMachineState ("max_ram_below_4g").

- It has a nice description.

Then in pc_system_flash_map() [hw/i386/pc_sysfw.c], I suggest replacing
FLASH_SIZE_LIMIT with "pcms->firmware_max_size".

(On a tangent: if the new property mattered for the recently added
"microvm" machine type too, i.e., not just i440fx (=pc) and q35, then
the function to modify would be the more abstract
x86_machine_class_init() [hw/i386/x86.c], rather than
pc_machine_class_init(). But the new property does not seem to matter
for "microvm".)

Thanks
Laszlo

Re: PATCH: Increase System Firmware Max Size

[Daniel P. Berrangé]

On Wed, Sep 16, 2020 at 11:52:41AM +0200, Laszlo Ersek wrote:
> Hi Erich,
> 
> (1) this patch is really not trivial; please do not continue CC'ing
> qemu-trivial
> 
> (2) Please do CC people that have given you feedback previously. I
> primarily mean Daniel and David.
> 
> (3) Generally speaking, please post new versions of a patch stand-alone
> (not in reply to another message) on the list.
> 
> (4) Please use git-send-email (or suitable wrapper utilities) for
> sending your patch.
> 
> https://wiki.qemu.org/Contribute/SubmitAPatch
> 
> 
> One non-meta comment below:
> 
> On 09/15/20 21:10, McMillan, Erich via wrote:
> > Apologies, ignore previous patch. The relevant patch is below:
> >
> > From 473daf6129debf8d158a9ae1aff788c5bdbbc799 Mon Sep 17 00:00:00 2001
> > From: Erich McMillan <erich.mcmillan@hp.com>
> > Date: Tue, 15 Sep 2020 13:23:25 -0500
> > Subject: [PATCH 2/2] Add max firmware size as optional parameter
> >
> > Signed-off-by: Erich McMillan <erich.mcmillan@hp.com>
> > ---
> >  hw/i386/pc_sysfw.c  | 13 ++-----------
> >  include/hw/loader.h |  9 +++++++++
> >  qemu-options.hx     |  8 ++++++++
> >  softmmu/vl.c        | 40 ++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 59 insertions(+), 11 deletions(-)
> >
> > diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
> > index b6c0822..ba6c99d 100644
> > --- a/hw/i386/pc_sysfw.c
> > +++ b/hw/i386/pc_sysfw.c
> > @@ -39,15 +39,6 @@
> >  #include "hw/block/flash.h"
> >  #include "sysemu/kvm.h"
> >
> > -/*
> > - * 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
> > - * [0xFEE00000..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,
> > @@ -182,10 +173,10 @@ static void pc_system_flash_map(PCMachineState *pcms,
> >          }
> >          if ((hwaddr)size != size
> >              || total_size > HWADDR_MAX - size
> > -            || total_size + size > FLASH_SIZE_LIMIT) {
> > +            || total_size + size > MaxCombinedFirmwareSize) {
> >              error_report("combined size of system firmware exceeds "
> >                           "%" PRIu64 " bytes",
> > -                         FLASH_SIZE_LIMIT);
> > +                         MaxCombinedFirmwareSize);
> >              exit(1);
> >          }
> >
> > diff --git a/include/hw/loader.h b/include/hw/loader.h
> > index a9eeea3..7898b63 100644
> > --- a/include/hw/loader.h
> > +++ b/include/hw/loader.h
> > @@ -318,4 +318,13 @@ int rom_add_option(const char *file, int32_t bootindex);
> >   * overflow on real hardware too. */
> >  #define UBOOT_MAX_GUNZIP_BYTES (64 << 20)
> >
> > +/*
> > + * 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
> > + * [0xFEE00000..0xFEE01000] -- see IO_APIC_DEFAULT_ADDRESS --, leaving free
> > + * only 18MB-4KB below 4G. For now, restrict the cumulative mapping to 8MB in
> > + * size, but allow user to specify larger size via command line.
> > + */
> > +extern uint64_t MaxCombinedFirmwareSize;
> > +
> >  #endif
> > diff --git a/qemu-options.hx b/qemu-options.hx
> > index b0f0205..32eed3a 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -1377,6 +1377,14 @@ SRST
> >          |qemu_system_x86| -hda a -hdb b
> >  ERST
> >
> > +DEF("maxfirmwaresize", HAS_ARG, QEMU_OPTION_maxfirmwaresize,
> > +    "-maxfirmwaresize [size=]megs  specify maximum combined firmware size, default is 8MiB. Known issues if value exceeds 16MiB.\n",
> > +    QEMU_ARCH_ALL)
> > +SRST
> > +``-maxfirmwaresize [size=]megs``
> > +    Specify maximum combined firmware size, default is 8MiB. Known issues if value exceeds 16MiB.
> > +ERST
> > +
> >  DEF("mtdblock", HAS_ARG, QEMU_OPTION_mtdblock,
> >      "-mtdblock file  use 'file' as on-board Flash memory image\n",
> >      QEMU_ARCH_ALL)
> > diff --git a/softmmu/vl.c b/softmmu/vl.c
> > index 0cc86b0..fcf41d2 100644
> > --- a/softmmu/vl.c
> > +++ b/softmmu/vl.c
> > @@ -116,6 +116,8 @@
> >
> >  #define MAX_VIRTIO_CONSOLES 1
> >
> > +uint64_t MaxCombinedFirmwareSize = 8 * MiB;
> > +
> >  static const char *data_dir[16];
> >  static int data_dir_idx;
> >  const char *bios_name = NULL;
> > @@ -448,6 +450,20 @@ static QemuOptsList qemu_mem_opts = {
> >      },
> >  };
> >
> > +static QemuOptsList qemu_max_fw_size_opts = {
> > +    .name = "maxfirmwaresize",
> > +    .implied_opt_name = "size",
> > +    .head = QTAILQ_HEAD_INITIALIZER(qemu_max_fw_size_opts.head),
> > +    .merge_lists = true,
> > +    .desc = {
> > +        {
> > +            .name = "size",
> > +            .type = QEMU_OPT_SIZE,
> > +        },
> > +        { /* end of list */ }
> > +    },
> > +};
> > +
> >  static QemuOptsList qemu_icount_opts = {
> >      .name = "icount",
> >      .implied_opt_name = "shift",
> > @@ -2576,6 +2592,23 @@ static bool object_create_delayed(const char *type, QemuOpts *opts)
> >      return !object_create_initial(type, opts);
> >  }
> >
> > +static void set_max_firmware_size(uint64_t *maxfwsize)
> > +{
> > +    const char *max_fw_size_str;
> > +    QemuOpts *opts = qemu_find_opts_singleton("maxfirmwaresize");
> > +
> > +    max_fw_size_str = qemu_opt_get(opts, "size");
> > +
> > +    if (max_fw_size_str) {
> > +        if (!*max_fw_size_str) {
> > +            error_report("missing 'size' option value");
> > +            exit(EXIT_FAILURE);
> > +        }
> > +
> > +        *maxfwsize = qemu_opt_get_size(opts, "size", 8 * MiB);
> > +    }
> > +}
> > +
> >
> >  static bool set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size,
> >                                 MachineClass *mc)
> > @@ -2904,6 +2937,7 @@ void qemu_init(int argc, char **argv, char **envp)
> >      qemu_add_opts(&qemu_machine_opts);
> >      qemu_add_opts(&qemu_accel_opts);
> >      qemu_add_opts(&qemu_mem_opts);
> > +    qemu_add_opts(&qemu_max_fw_size_opts);
> >      qemu_add_opts(&qemu_smp_opts);
> >      qemu_add_opts(&qemu_boot_opts);
> >      qemu_add_opts(&qemu_add_fd_opts);
> > @@ -3160,6 +3194,10 @@ void qemu_init(int argc, char **argv, char **envp)
> >                      exit(EXIT_FAILURE);
> >                  }
> >                  break;
> > +            case QEMU_OPTION_maxfirmwaresize:
> > +                opts = qemu_opts_parse_noisily(qemu_find_opts("maxfirmwaresize"),
> > +                                               optarg, true);
> > +                break;
> >  #ifdef CONFIG_TPM
> >              case QEMU_OPTION_tpmdev:
> >                  if (tpm_config_parse(qemu_find_opts("tpmdev"), optarg) < 0) {
> > @@ -3845,6 +3883,8 @@ void qemu_init(int argc, char **argv, char **envp)
> >      have_custom_ram_size = set_memory_options(&ram_slots, &maxram_size,
> >                                                machine_class);
> >
> > +    set_max_firmware_size(&MaxCombinedFirmwareSize);
> > +
> >      os_daemonize();
> >      rcu_disable_atfork();
> >
> 
> (5) In my opinion (which could be wrong of course), we shouldn't
> introduce a new command line option for this, but a new PC machine type
> property called "x-firmware-max-size".

Yeah, we definitely do not want a new top level CLI arg, just a
machine type property.  We don't need any "x-" prefix on it
though, just a plain "firmware-max-size" prop is fine.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: PATCH: Increase System Firmware Max Size

[Laszlo Ersek]

On 09/16/20 11:56, Daniel P. Berrangé wrote:
> On Wed, Sep 16, 2020 at 11:52:41AM +0200, Laszlo Ersek wrote:
>> Hi Erich,
>>
>> (1) this patch is really not trivial; please do not continue CC'ing
>> qemu-trivial
>>
>> (2) Please do CC people that have given you feedback previously. I
>> primarily mean Daniel and David.
>>
>> (3) Generally speaking, please post new versions of a patch stand-alone
>> (not in reply to another message) on the list.
>>
>> (4) Please use git-send-email (or suitable wrapper utilities) for
>> sending your patch.
>>
>> https://wiki.qemu.org/Contribute/SubmitAPatch
>>
>>
>> One non-meta comment below:
>>
>> On 09/15/20 21:10, McMillan, Erich via wrote:
>>> Apologies, ignore previous patch. The relevant patch is below:
>>>
>>> From 473daf6129debf8d158a9ae1aff788c5bdbbc799 Mon Sep 17 00:00:00 2001
>>> From: Erich McMillan <erich.mcmillan@hp.com>
>>> Date: Tue, 15 Sep 2020 13:23:25 -0500
>>> Subject: [PATCH 2/2] Add max firmware size as optional parameter
>>>
>>> Signed-off-by: Erich McMillan <erich.mcmillan@hp.com>
>>> ---
>>>  hw/i386/pc_sysfw.c  | 13 ++-----------
>>>  include/hw/loader.h |  9 +++++++++
>>>  qemu-options.hx     |  8 ++++++++
>>>  softmmu/vl.c        | 40 ++++++++++++++++++++++++++++++++++++++++
>>>  4 files changed, 59 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
>>> index b6c0822..ba6c99d 100644
>>> --- a/hw/i386/pc_sysfw.c
>>> +++ b/hw/i386/pc_sysfw.c
>>> @@ -39,15 +39,6 @@
>>>  #include "hw/block/flash.h"
>>>  #include "sysemu/kvm.h"
>>>
>>> -/*
>>> - * 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
>>> - * [0xFEE00000..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,
>>> @@ -182,10 +173,10 @@ static void pc_system_flash_map(PCMachineState *pcms,
>>>          }
>>>          if ((hwaddr)size != size
>>>              || total_size > HWADDR_MAX - size
>>> -            || total_size + size > FLASH_SIZE_LIMIT) {
>>> +            || total_size + size > MaxCombinedFirmwareSize) {
>>>              error_report("combined size of system firmware exceeds "
>>>                           "%" PRIu64 " bytes",
>>> -                         FLASH_SIZE_LIMIT);
>>> +                         MaxCombinedFirmwareSize);
>>>              exit(1);
>>>          }
>>>
>>> diff --git a/include/hw/loader.h b/include/hw/loader.h
>>> index a9eeea3..7898b63 100644
>>> --- a/include/hw/loader.h
>>> +++ b/include/hw/loader.h
>>> @@ -318,4 +318,13 @@ int rom_add_option(const char *file, int32_t bootindex);
>>>   * overflow on real hardware too. */
>>>  #define UBOOT_MAX_GUNZIP_BYTES (64 << 20)
>>>
>>> +/*
>>> + * 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
>>> + * [0xFEE00000..0xFEE01000] -- see IO_APIC_DEFAULT_ADDRESS --, leaving free
>>> + * only 18MB-4KB below 4G. For now, restrict the cumulative mapping to 8MB in
>>> + * size, but allow user to specify larger size via command line.
>>> + */
>>> +extern uint64_t MaxCombinedFirmwareSize;
>>> +
>>>  #endif
>>> diff --git a/qemu-options.hx b/qemu-options.hx
>>> index b0f0205..32eed3a 100644
>>> --- a/qemu-options.hx
>>> +++ b/qemu-options.hx
>>> @@ -1377,6 +1377,14 @@ SRST
>>>          |qemu_system_x86| -hda a -hdb b
>>>  ERST
>>>
>>> +DEF("maxfirmwaresize", HAS_ARG, QEMU_OPTION_maxfirmwaresize,
>>> +    "-maxfirmwaresize [size=]megs  specify maximum combined firmware size, default is 8MiB. Known issues if value exceeds 16MiB.\n",
>>> +    QEMU_ARCH_ALL)
>>> +SRST
>>> +``-maxfirmwaresize [size=]megs``
>>> +    Specify maximum combined firmware size, default is 8MiB. Known issues if value exceeds 16MiB.
>>> +ERST
>>> +
>>>  DEF("mtdblock", HAS_ARG, QEMU_OPTION_mtdblock,
>>>      "-mtdblock file  use 'file' as on-board Flash memory image\n",
>>>      QEMU_ARCH_ALL)
>>> diff --git a/softmmu/vl.c b/softmmu/vl.c
>>> index 0cc86b0..fcf41d2 100644
>>> --- a/softmmu/vl.c
>>> +++ b/softmmu/vl.c
>>> @@ -116,6 +116,8 @@
>>>
>>>  #define MAX_VIRTIO_CONSOLES 1
>>>
>>> +uint64_t MaxCombinedFirmwareSize = 8 * MiB;
>>> +
>>>  static const char *data_dir[16];
>>>  static int data_dir_idx;
>>>  const char *bios_name = NULL;
>>> @@ -448,6 +450,20 @@ static QemuOptsList qemu_mem_opts = {
>>>      },
>>>  };
>>>
>>> +static QemuOptsList qemu_max_fw_size_opts = {
>>> +    .name = "maxfirmwaresize",
>>> +    .implied_opt_name = "size",
>>> +    .head = QTAILQ_HEAD_INITIALIZER(qemu_max_fw_size_opts.head),
>>> +    .merge_lists = true,
>>> +    .desc = {
>>> +        {
>>> +            .name = "size",
>>> +            .type = QEMU_OPT_SIZE,
>>> +        },
>>> +        { /* end of list */ }
>>> +    },
>>> +};
>>> +
>>>  static QemuOptsList qemu_icount_opts = {
>>>      .name = "icount",
>>>      .implied_opt_name = "shift",
>>> @@ -2576,6 +2592,23 @@ static bool object_create_delayed(const char *type, QemuOpts *opts)
>>>      return !object_create_initial(type, opts);
>>>  }
>>>
>>> +static void set_max_firmware_size(uint64_t *maxfwsize)
>>> +{
>>> +    const char *max_fw_size_str;
>>> +    QemuOpts *opts = qemu_find_opts_singleton("maxfirmwaresize");
>>> +
>>> +    max_fw_size_str = qemu_opt_get(opts, "size");
>>> +
>>> +    if (max_fw_size_str) {
>>> +        if (!*max_fw_size_str) {
>>> +            error_report("missing 'size' option value");
>>> +            exit(EXIT_FAILURE);
>>> +        }
>>> +
>>> +        *maxfwsize = qemu_opt_get_size(opts, "size", 8 * MiB);
>>> +    }
>>> +}
>>> +
>>>
>>>  static bool set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size,
>>>                                 MachineClass *mc)
>>> @@ -2904,6 +2937,7 @@ void qemu_init(int argc, char **argv, char **envp)
>>>      qemu_add_opts(&qemu_machine_opts);
>>>      qemu_add_opts(&qemu_accel_opts);
>>>      qemu_add_opts(&qemu_mem_opts);
>>> +    qemu_add_opts(&qemu_max_fw_size_opts);
>>>      qemu_add_opts(&qemu_smp_opts);
>>>      qemu_add_opts(&qemu_boot_opts);
>>>      qemu_add_opts(&qemu_add_fd_opts);
>>> @@ -3160,6 +3194,10 @@ void qemu_init(int argc, char **argv, char **envp)
>>>                      exit(EXIT_FAILURE);
>>>                  }
>>>                  break;
>>> +            case QEMU_OPTION_maxfirmwaresize:
>>> +                opts = qemu_opts_parse_noisily(qemu_find_opts("maxfirmwaresize"),
>>> +                                               optarg, true);
>>> +                break;
>>>  #ifdef CONFIG_TPM
>>>              case QEMU_OPTION_tpmdev:
>>>                  if (tpm_config_parse(qemu_find_opts("tpmdev"), optarg) < 0) {
>>> @@ -3845,6 +3883,8 @@ void qemu_init(int argc, char **argv, char **envp)
>>>      have_custom_ram_size = set_memory_options(&ram_slots, &maxram_size,
>>>                                                machine_class);
>>>
>>> +    set_max_firmware_size(&MaxCombinedFirmwareSize);
>>> +
>>>      os_daemonize();
>>>      rcu_disable_atfork();
>>>
>>
>> (5) In my opinion (which could be wrong of course), we shouldn't
>> introduce a new command line option for this, but a new PC machine type
>> property called "x-firmware-max-size".
> 
> Yeah, we definitely do not want a new top level CLI arg, just a
> machine type property.  We don't need any "x-" prefix on it
> though, just a plain "firmware-max-size" prop is fine.

According to the previous discussion, I'd like to request that we add
the x- prefix (showing that either the property is experimental, or that
it is intended in support of experimental use cases).

If you disagree, I'll accept that though.

Thanks
Laszlo

Re: PATCH: Increase System Firmware Max Size

[Daniel P. Berrangé]

On Wed, Sep 16, 2020 at 01:31:05PM +0200, Laszlo Ersek wrote:
> On 09/16/20 11:56, Daniel P. Berrangé wrote:
> > On Wed, Sep 16, 2020 at 11:52:41AM +0200, Laszlo Ersek wrote:
> >> (5) In my opinion (which could be wrong of course), we shouldn't
> >> introduce a new command line option for this, but a new PC machine type
> >> property called "x-firmware-max-size".
> > 
> > Yeah, we definitely do not want a new top level CLI arg, just a
> > machine type property.  We don't need any "x-" prefix on it
> > though, just a plain "firmware-max-size" prop is fine.
> 
> According to the previous discussion, I'd like to request that we add
> the x- prefix (showing that either the property is experimental, or that
> it is intended in support of experimental use cases).
> 
> If you disagree, I'll accept that though.

I don't see a reason to call the property itself experimental unless
someone is proposing that there's likely to be a better way to enlarge
the max firmware size that will replace it later.

The use case is definitely not something I'd call experimental either.
The user providing their own arbirary firmware is not going to be common,
but when they do I'd consider it a valid supported use case. QEMU is
agnostic about what firmware impl is used, as long as the firmware can
support the expected services/interfaces.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: PATCH: Increase System Firmware Max Size

[Laszlo Ersek]

On 09/16/20 11:52, Laszlo Ersek wrote:

> (5) In my opinion (which could be wrong of course), we shouldn't
> introduce a new command line option for this, but a new PC machine type
> property called "x-firmware-max-size".
> 
> Please look at the object_class_property_add() calls in
> pc_machine_class_init() [hw/i386/pc.c].
> 
> I think the PC_MACHINE_MAX_RAM_BELOW_4G property is a good example to
> imitate:
> 
> - It has type "size".
> 
> - It comes with a getter and a setter, and an associated field in
> PCMachineState ("max_ram_below_4g").
> 
> - It has a nice description.
> 
> Then in pc_system_flash_map() [hw/i386/pc_sysfw.c], I suggest replacing
> FLASH_SIZE_LIMIT with "pcms->firmware_max_size".

The default value (8MB) should be set in pc_machine_initfn() [hw/i386/pc.c].

Thanks
Laszlo