| Message ID | CS1PR8401MB0327EF9D532330BA44257AFCF3240@CS1PR8401MB0327.NAMPRD84.PROD.OUTLOOK.COM (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | PATCH: Increase System Firmware Max Size | expand |
+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
* 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
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
* 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
Hi Dave, and Laszlo, I'm not exactly following your email '>' formatting on this discussion so apologies if I've messed it up.
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
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
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
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
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
* 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
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
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
* 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
I’d be happy to rewrite as a property. Erich Get Outlook for iOS<https://aka.ms/o0ukef>
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);
}
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
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
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
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
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
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
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 -------
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>