[PATCH 0/2] hw/arm: allow flash images being smaller than the available space

[PATCH 0/2] hw/arm: allow flash images being smaller than the available space

[Gerd Hoffmann]

Gerd Hoffmann (2):
  hw/arm: allow flash images being smaller than the available space
  docs: add no-padding firmware feature

 hw/arm/virt.c              | 16 ++++++++++++++++
 docs/interop/firmware.json |  5 ++++-
 2 files changed, 20 insertions(+), 1 deletion(-)

-- 
2.38.1

[PATCH 1/2] hw/arm: allow flash images being smaller than the available space

[Gerd Hoffmann]

Query block device backing flash for size and use that instead of
requiring the block device being exactly 64M in size.  This allows
to use edk2 firmware builds without padding, i.e. use QEMU_EFI.fd
(which is /way/ smaller than 64M) as-is.

-rw-r--r--. 1 root root 67108864 Dec 12 23:45 QEMU_EFI-pflash.raw
-rw-r--r--. 1 root root  2097152 Dec 12 23:45 QEMU_EFI.fd

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/arm/virt.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index b87135085610..c71ae2cd73f7 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -43,6 +43,7 @@
 #include "hw/vfio/vfio-amd-xgbe.h"
 #include "hw/display/ramfb.h"
 #include "net/net.h"
+#include "sysemu/block-backend.h"
 #include "sysemu/device_tree.h"
 #include "sysemu/numa.h"
 #include "sysemu/runstate.h"
@@ -1134,6 +1135,21 @@ static void virt_flash_map1(PFlashCFI01 *flash,
                             MemoryRegion *sysmem)
 {
     DeviceState *dev = DEVICE(flash);
+    BlockBackend *blk;
+
+    blk = pflash_cfi01_get_blk(flash);
+    if (blk) {
+        hwaddr blksize = blk_getlength(blk);
+
+        if (blksize == 0 || blksize > size ||
+            !QEMU_IS_ALIGNED(size, VIRT_FLASH_SECTOR_SIZE)) {
+            error_report("system firmware block device %s"
+                         " has invalid size %" PRId64,
+                         blk_name(blk), size);
+            exit(1);
+        }
+        size = blksize;
+    }
 
     assert(QEMU_IS_ALIGNED(size, VIRT_FLASH_SECTOR_SIZE));
     assert(size / VIRT_FLASH_SECTOR_SIZE <= UINT32_MAX);
-- 
2.38.1

Re: [PATCH 1/2] hw/arm: allow flash images being smaller than the available space

[Peter Maydell]

On Fri, 16 Dec 2022 at 10:12, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> Query block device backing flash for size and use that instead of
> requiring the block device being exactly 64M in size.  This allows
> to use edk2 firmware builds without padding, i.e. use QEMU_EFI.fd
> (which is /way/ smaller than 64M) as-is.
>
> -rw-r--r--. 1 root root 67108864 Dec 12 23:45 QEMU_EFI-pflash.raw
> -rw-r--r--. 1 root root  2097152 Dec 12 23:45 QEMU_EFI.fd
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/arm/virt.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index b87135085610..c71ae2cd73f7 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -43,6 +43,7 @@
>  #include "hw/vfio/vfio-amd-xgbe.h"
>  #include "hw/display/ramfb.h"
>  #include "net/net.h"
> +#include "sysemu/block-backend.h"
>  #include "sysemu/device_tree.h"
>  #include "sysemu/numa.h"
>  #include "sysemu/runstate.h"
> @@ -1134,6 +1135,21 @@ static void virt_flash_map1(PFlashCFI01 *flash,
>                              MemoryRegion *sysmem)
>  {
>      DeviceState *dev = DEVICE(flash);
> +    BlockBackend *blk;
> +
> +    blk = pflash_cfi01_get_blk(flash);
> +    if (blk) {
> +        hwaddr blksize = blk_getlength(blk);
> +
> +        if (blksize == 0 || blksize > size ||
> +            !QEMU_IS_ALIGNED(size, VIRT_FLASH_SECTOR_SIZE)) {
> +            error_report("system firmware block device %s"
> +                         " has invalid size %" PRId64,
> +                         blk_name(blk), size);
> +            exit(1);
> +        }
> +        size = blksize;
> +    }
>
>      assert(QEMU_IS_ALIGNED(size, VIRT_FLASH_SECTOR_SIZE));
>      assert(size / VIRT_FLASH_SECTOR_SIZE <= UINT32_MAX);
> --
> 2.38.1

We've had at least three threads about this already, attempting
various approaches. Please read up on them and the discussions
that ensued from those patches before having another go at it.

The problem with this idea is that the size of the flash device
exposed to the guest should not depend on the size of the backing
file the user provides -- it's a fact about the machine and
also if it varies it's easy for the user to back themselves into
a corner where they can't migrate to a destination where the
backing file is larger, or they can't add new variables to the
EFI store because the backing file is too small.

thanks
-- PMM

[PATCH 2/2] docs: add no-padding firmware feature

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 docs/interop/firmware.json | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/docs/interop/firmware.json b/docs/interop/firmware.json
index 56814f02b3c0..74f404d745b0 100644
--- a/docs/interop/firmware.json
+++ b/docs/interop/firmware.json
@@ -191,6 +191,8 @@
 #                  PL011 UART. @verbose-static is mutually exclusive
 #                  with @verbose-dynamic.
 #
+# @no-padding: The (arm/aarch64) firmware images are not padded to 64M.
+#
 # Since: 3.0
 ##
 { 'enum' : 'FirmwareFeature',
@@ -198,7 +200,8 @@
              'amd-sev', 'amd-sev-es', 'amd-sev-snp',
              'intel-tdx',
              'enrolled-keys', 'requires-smm', 'secure-boot',
-             'verbose-dynamic', 'verbose-static' ] }
+             'verbose-dynamic', 'verbose-static',
+             'no-padding' ] }
 
 ##
 # @FirmwareFlashFile:
-- 
2.38.1

Re: [PATCH 2/2] docs: add no-padding firmware feature

[Kashyap Chamarthy]

On Fri, Dec 16, 2022 at 11:12:34AM +0100, Gerd Hoffmann wrote:

Hi,

> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  docs/interop/firmware.json | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/interop/firmware.json b/docs/interop/firmware.json
> index 56814f02b3c0..74f404d745b0 100644
> --- a/docs/interop/firmware.json
> +++ b/docs/interop/firmware.json
> @@ -191,6 +191,8 @@
>  #                  PL011 UART. @verbose-static is mutually exclusive
>  #                  with @verbose-dynamic.
>  #
> +# @no-padding: The (arm/aarch64) firmware images are not padded to 64M.
> +#
>  # Since: 3.0
>  ##
>  { 'enum' : 'FirmwareFeature',
> @@ -198,7 +200,8 @@
>               'amd-sev', 'amd-sev-es', 'amd-sev-snp',
>               'intel-tdx',
>               'enrolled-keys', 'requires-smm', 'secure-boot',
> -             'verbose-dynamic', 'verbose-static' ] }
> +             'verbose-dynamic', 'verbose-static',
> +             'no-padding' ] }

If you're re-spinning, please consider adding a sentence or two (for
those of us who're not familiar) a bit more about the "no-padding"
feature to the commit message.

IIUC, I found the use of the padding feature reading an old email
response[1] from Dan Berrangé:

(quote)
    ...
    If there's a risk that newer firmware will be larger than old firmware
    there's only really two options:
    
      - Keep all firmware images forever, each with a unique versioned
        filename. This ensures target QEMU will always load the original
        smaller firmware
    
      - Add padding to the firmware images. IOW, if the firmware is 2 MB,
        add zero-padding to the end of the image to round it upto 4 MB
        (whatever you anticipate the largest size wil be in future).
    ...
(/quote)


[1] https://edk2.groups.io/g/devel/message/54758

Regardless:

Reviewed-by: Kashyap Chamarthy <kchamart@redhat.com>

>  ##
>  # @FirmwareFlashFile:
> -- 
> 2.38.1
> 

-- 
/kashyap