[PATCH 0/1] hw/arm/aspeed: Automatically zero-extend flash images

[PATCH 0/1] hw/arm/aspeed: Automatically zero-extend flash images

[Peter Delevoryas]

I've been using this patch for a long time so that I don't have to use
dd to zero-extend stuff all the time. It's just doing what people are
doing already, right? I hope it's not controversial.

One note: I couldn't figure out how to make it work without changing the
permissions on the block device to allow truncation. If somebody knows
how to avoid the `blk_get_perm`, `blk_set_perm` calls here, let me know!

Thanks,
Peter

Peter Delevoryas (1):
  hw/arm/aspeed: Automatically zero-extend flash images

 hw/arm/aspeed.c | 40 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

-- 
2.38.1

[PATCH 1/1] hw/arm/aspeed: Automatically zero-extend flash images

[Peter Delevoryas]

One thing that's really annoying about the Aspeed machines is that you
have to provide a flash image that is the same size as the SPI-NOR flash
device the board uses, or something larger. If you don't, you'll get
this obscure error message:

    qemu-system-aarch64: failed to read the initial flash content

Which is just because the file isn't the right size. Zero-extending the
file to 128MB (the largest SPI-NOR flash size) fixes this.

This commit just performs the zero-extend automatically, so people don't
have to maintain bash scripts for it. And if your bash script does this
already, it will be a no-op. And, if your firmware image is larger than
the SPI-NOR device, then it will not truncate it.

Signed-off-by: Peter Delevoryas <peter@pjd.dev>
---
 hw/arm/aspeed.c | 40 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 55f114ef72..26450d90db 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -260,6 +260,30 @@ static void write_boot_rom(DriveInfo *dinfo, hwaddr addr, size_t rom_size,
     rom_add_blob_fixed("aspeed.boot_rom", storage, rom_size, addr);
 }
 
+static int zero_extend_block_device(BlockBackend *blk, uint64_t size,
+                                    Error **errp)
+{
+    uint64_t perm, shared_perm;
+
+    blk_get_perm(blk, &perm, &shared_perm);
+
+    if (blk_set_perm(blk, BLK_PERM_ALL, BLK_PERM_ALL, errp)) {
+        error_append_hint(errp, "Unable to change permissions on block device");
+        return -1;
+    }
+    if (blk_truncate(blk, size, true, PREALLOC_MODE_OFF, BDRV_REQ_ZERO_WRITE,
+                     errp)) {
+        error_append_hint(errp, "Unable to zero-extend block device");
+        return -1;
+    }
+    if (blk_set_perm(blk, perm, shared_perm, errp)) {
+        error_append_hint(errp,
+                          "Unable to restore permissions on block device");
+        /* Ignore error since we successfully extended the device already */
+    }
+    return 0;
+}
+
 void aspeed_board_init_flashes(AspeedSMCState *s, const char *flashtype,
                                       unsigned int count, int unit0)
 {
@@ -273,10 +297,24 @@ void aspeed_board_init_flashes(AspeedSMCState *s, const char *flashtype,
         DriveInfo *dinfo = drive_get(IF_MTD, 0, unit0 + i);
         qemu_irq cs_line;
         DeviceState *dev;
+        AspeedSMCFlash *flash = &s->flashes[i];
+        uint64_t flash_size = memory_region_size(&flash->mmio);
 
         dev = qdev_new(flashtype);
         if (dinfo) {
-            qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(dinfo));
+            BlockBackend *blk = blk_by_legacy_dinfo(dinfo);
+            int64_t blk_size = blk_getlength(blk);
+
+            if (blk_size > 0 && blk_size < (int64_t)flash_size) {
+                Error *err = NULL;
+
+                zero_extend_block_device(blk, flash_size, &err);
+                if (err) {
+                    warn_reportf_err(err, "Error zero-extending MTD drive[%d] "
+                                     "to flash size", i);
+                }
+            }
+            qdev_prop_set_drive(dev, "drive", blk);
         }
         qdev_realize_and_unref(dev, BUS(s->spi), &error_fatal);
 
-- 
2.38.1

Re: [PATCH 0/1] hw/arm/aspeed: Automatically zero-extend flash images

[Peter Maydell]

On Mon, 14 Nov 2022 at 19:08, Peter Delevoryas <peter@pjd.dev> wrote:
>
> I've been using this patch for a long time so that I don't have to use
> dd to zero-extend stuff all the time. It's just doing what people are
> doing already, right? I hope it's not controversial.

We just had a thread about this kind of thing for one of the
riscv boards (although there the attempted approach was to
change the size of the flash device to match the provided
file, rather than changing the file to match the flash device):
https://lore.kernel.org/qemu-devel/20221107130217.2243815-1-sunilvl@ventanamicro.com/

The short summary is (a) yes, it's controversial and
(b) if we do something for this we need to have a standard
approach that we do on all boards, not "some boards do
some weird magic in different ways from everything else"...

thanks
-- PMM

Re: [PATCH 0/1] hw/arm/aspeed: Automatically zero-extend flash images

[Peter Delevoryas]

On Tue, Nov 15, 2022 at 10:48:40AM +0000, Peter Maydell wrote:
> On Mon, 14 Nov 2022 at 19:08, Peter Delevoryas <peter@pjd.dev> wrote:
> >
> > I've been using this patch for a long time so that I don't have to use
> > dd to zero-extend stuff all the time. It's just doing what people are
> > doing already, right? I hope it's not controversial.
> 
> We just had a thread about this kind of thing for one of the
> riscv boards (although there the attempted approach was to
> change the size of the flash device to match the provided
> file, rather than changing the file to match the flash device):
> https://lore.kernel.org/qemu-devel/20221107130217.2243815-1-sunilvl@ventanamicro.com/

Thanks for linking this!

> 
> The short summary is (a) yes, it's controversial and
> (b) if we do something for this we need to have a standard
> approach that we do on all boards, not "some boards do
> some weird magic in different ways from everything else"...

I see, that's ok, I'll investigate option (b) then.

Thanks,
Peter

> 
> thanks
> -- PMM

Re: [PATCH 0/1] hw/arm/aspeed: Automatically zero-extend flash images

[Cédric Le Goater]

Hello Peter,

On 11/14/22 20:08, Peter Delevoryas wrote:
> I've been using this patch for a long time so that I don't have to use
> dd to zero-extend stuff all the time. It's just doing what people are
> doing already, right? I hope it's not controversial.

I simply run :

    truncate --size <size>

on the FW file when needed and it is rare because most FW image builders
know the flash size of the target.

However, the current error message is confusing and the following could
be an improvement :

@@ -1606,6 +1606,13 @@ static void m25p80_realize(SSIPeripheral
      if (s->blk) {
          uint64_t perm = BLK_PERM_CONSISTENT_READ |
                          (blk_supports_write_perm(s->blk) ? BLK_PERM_WRITE : 0);
+
+        if (blk_getlength(s->blk) != s->size) {
+            error_setg(errp, "backend file is too small for flash device %s (%d MB)",
+                       object_class_get_name(OBJECT_CLASS(mc)), s->size >> 20);
+            return;
+        }
+
          ret = blk_set_perm(s->blk, perm, BLK_PERM_ALL, errp);
          if (ret < 0) {
              return;

I can send a patch for the above.


<hack>

I mostly run the QEMU machines with -snapshot, this hack  :

   blk_set_allow_write_beyond_eof(s->blk, true);

makes it work also ...

</hack>


Thanks,

C.

> One note: I couldn't figure out how to make it work without changing the
> permissions on the block device to allow truncation. If somebody knows
> how to avoid the `blk_get_perm`, `blk_set_perm` calls here, let me know!
>
> Thanks,
> Peter
> 
> Peter Delevoryas (1):
>    hw/arm/aspeed: Automatically zero-extend flash images
> 
>   hw/arm/aspeed.c | 40 +++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 39 insertions(+), 1 deletion(-)
> 

Re: [PATCH 0/1] hw/arm/aspeed: Automatically zero-extend flash images

[Philippe Mathieu-Daudé]

On 15/11/22 14:06, Cédric Le Goater wrote:
> Hello Peter,
> 
> On 11/14/22 20:08, Peter Delevoryas wrote:
>> I've been using this patch for a long time so that I don't have to use
>> dd to zero-extend stuff all the time. It's just doing what people are
>> doing already, right? I hope it's not controversial.
> 
> I simply run :
> 
>     truncate --size <size>
> 
> on the FW file when needed and it is rare because most FW image builders
> know the flash size of the target.
> 
> However, the current error message is confusing and the following could
> be an improvement :
> 
> @@ -1606,6 +1606,13 @@ static void m25p80_realize(SSIPeripheral
>       if (s->blk) {
>           uint64_t perm = BLK_PERM_CONSISTENT_READ |
>                           (blk_supports_write_perm(s->blk) ? 
> BLK_PERM_WRITE : 0);
> +
> +        if (blk_getlength(s->blk) != s->size) {

'!=' -> '<' ?

> +            error_setg(errp, "backend file is too small for flash 
> device %s (%d MB)",
> +                       object_class_get_name(OBJECT_CLASS(mc)), s->size 
>  >> 20);
> +            return;
> +        }
> +
>           ret = blk_set_perm(s->blk, perm, BLK_PERM_ALL, errp);
>           if (ret < 0) {
>               return;
> 
> I can send a patch for the above.
> 
> 
> <hack>
> 
> I mostly run the QEMU machines with -snapshot, this hack  :
> 
>    blk_set_allow_write_beyond_eof(s->blk, true);
> 
> makes it work also ...
> 
> </hack>
> 
> 
> Thanks,
> 
> C.

Re: [PATCH 0/1] hw/arm/aspeed: Automatically zero-extend flash images

[Cédric Le Goater]

On 11/15/22 15:06, Philippe Mathieu-Daudé wrote:
> On 15/11/22 14:06, Cédric Le Goater wrote:
>> Hello Peter,
>>
>> On 11/14/22 20:08, Peter Delevoryas wrote:
>>> I've been using this patch for a long time so that I don't have to use
>>> dd to zero-extend stuff all the time. It's just doing what people are
>>> doing already, right? I hope it's not controversial.
>>
>> I simply run :
>>
>>     truncate --size <size>
>>
>> on the FW file when needed and it is rare because most FW image builders
>> know the flash size of the target.
>>
>> However, the current error message is confusing and the following could
>> be an improvement :
>>
>> @@ -1606,6 +1606,13 @@ static void m25p80_realize(SSIPeripheral
>>       if (s->blk) {
>>           uint64_t perm = BLK_PERM_CONSISTENT_READ |
>>                           (blk_supports_write_perm(s->blk) ? BLK_PERM_WRITE : 0);
>> +
>> +        if (blk_getlength(s->blk) != s->size) {
> 
> '!=' -> '<' ?

Hey. yes :)

I will send a patch. I am not sure this is 7.2 material though.

Thanks,

C.

Re: [PATCH 0/1] hw/arm/aspeed: Automatically zero-extend flash images

[Peter Delevoryas]

On Tue, Nov 15, 2022 at 02:06:57PM +0100, Cédric Le Goater wrote:
> Hello Peter,
> 
> On 11/14/22 20:08, Peter Delevoryas wrote:
> > I've been using this patch for a long time so that I don't have to use
> > dd to zero-extend stuff all the time. It's just doing what people are
> > doing already, right? I hope it's not controversial.
> 
> I simply run :
> 
>    truncate --size <size>

Ohhh I always forget about that command. That's certainly easier
than what I was doing.

> 
> on the FW file when needed and it is rare because most FW image builders
> know the flash size of the target.

welllllll my yocto builds don't, I'm not sure we would want them to either?

Because that would be extra data with no value to transfer to the BMC/etc.

flashcp erases the whole flash initially, which is why I don't worry
about providing a firmware image that is only covering the first 30 MB of
flash.

> 
> However, the current error message is confusing and the following could
> be an improvement :
> 
> @@ -1606,6 +1606,13 @@ static void m25p80_realize(SSIPeripheral
>      if (s->blk) {
>          uint64_t perm = BLK_PERM_CONSISTENT_READ |
>                          (blk_supports_write_perm(s->blk) ? BLK_PERM_WRITE : 0);
> +
> +        if (blk_getlength(s->blk) != s->size) {
> +            error_setg(errp, "backend file is too small for flash device %s (%d MB)",
> +                       object_class_get_name(OBJECT_CLASS(mc)), s->size >> 20);
> +            return;
> +        }
> +
>          ret = blk_set_perm(s->blk, perm, BLK_PERM_ALL, errp);
>          if (ret < 0) {
>              return;
> 
> I can send a patch for the above.

Ohhh yay! Thanks!

> 
> <hack>
> 
> I mostly run the QEMU machines with -snapshot, this hack  :
> 
>   blk_set_allow_write_beyond_eof(s->blk, true);
> 
> makes it work also ...

!!! Ohhh! interesting...that seems more sketchy but certainly simpler.

> 
> </hack>
> 
> 
> Thanks,
> 
> C.
> 
> > One note: I couldn't figure out how to make it work without changing the
> > permissions on the block device to allow truncation. If somebody knows
> > how to avoid the `blk_get_perm`, `blk_set_perm` calls here, let me know!
> > 
> > Thanks,
> > Peter
> > 
> > Peter Delevoryas (1):
> >    hw/arm/aspeed: Automatically zero-extend flash images
> > 
> >   hw/arm/aspeed.c | 40 +++++++++++++++++++++++++++++++++++++++-
> >   1 file changed, 39 insertions(+), 1 deletion(-)
> > 
>