Re: [PATCH v2] pflash: Only read non-zero parts of backend image

[Philippe Mathieu-Daudé]

[Extending to people using UEFI VARStore on Virt machines]

On 20/12/22 09:42, Gerd Hoffmann wrote:
> From: Xiang Zheng <zhengxiang9@huawei.com>
>
> Currently we fill the VIRT_FLASH memory space with two 64MB NOR images
> when using persistent UEFI variables on virt board. Actually we only use
> a very small(non-zero) part of the memory while the rest significant
> large(zero) part of memory is wasted.
>
> So this patch checks the block status and only writes the non-zero part
> into memory. This requires pflash devices to use sparse files for
> backends.

I like the idea, but I'm not sure how to relate with NOR flash devices.

From the block layer, we get BDRV_BLOCK_ZERO when a block is fully
filled by zeroes ('\0').

We don't want to waste host memory, I get it.

Now what "sees" the guest? Is the UEFI VARStore filled with zeroes?
If so, is it a EDK2 specific case for all virt machines? This would
be a virtualization optimization and in that case, this patch would
work.

On hardware the NOR flash "erased state" is filled of '\xff'. If
EDK2 requires a 64MiB VARStore on NOR flash, I'd expect the non-used
area to be filled with \xff, at least up to the sector size. Otherwise
it is sub-optimal use of persistent storage on hardware.

But instead of keeping insisting on that, I'd like to step back a little
and discuss. What is the use case?

* Either you want to test UEFI on real hardware and a NOR flash makes
  sense,

* or you are trying to optimize paravirtualized guests. In that case
  why insist with emulated NOR devices? Why not have EDK2 directly
  use a paravirtualized block driver which we can optimize / tune
  without interfering with emulated models?

Keeping insisting on optimizing guests using the QEMU pflash device
seems wrong to me. I'm pretty sure we can do better optimizing clouds
payloads.

> Signed-off-by: Xiang Zheng <zhengxiang9@huawei.com>
>
> [ kraxel: rebased to latest master ]
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>   hw/block/block.c | 36 +++++++++++++++++++++++++++++++++++-
>   1 file changed, 35 insertions(+), 1 deletion(-)
>
> diff --git a/hw/block/block.c b/hw/block/block.c
> index f9c4fe67673b..142ebe4267e4 100644
> --- a/hw/block/block.c
> +++ b/hw/block/block.c
> @@ -14,6 +14,40 @@
>   #include "qapi/error.h"
>   #include "qapi/qapi-types-block.h"
>   
> +/*
> + * Read the non-zeroes parts of @blk into @buf
> + * Reading all of the @blk is expensive if the zeroes parts of @blk
> + * is large enough. Therefore check the block status and only write
> + * the non-zeroes block into @buf.
> + *
> + * Return 0 on success, non-zero on error.
> + */
> +static int blk_pread_nonzeroes(BlockBackend *blk, hwaddr size, void *buf)
> +{
> +    int ret;
> +    int64_t bytes, offset = 0;
> +    BlockDriverState *bs = blk_bs(blk);
> +
> +    for (;;) {
> +        bytes = MIN(size - offset, BDRV_REQUEST_MAX_SECTORS);
> +        if (bytes <= 0) {
> +            return 0;
> +        }
> +        ret = bdrv_block_status(bs, offset, bytes, &bytes, NULL, NULL);
> +        if (ret < 0) {
> +            return ret;
> +        }
> +        if (!(ret & BDRV_BLOCK_ZERO)) {
> +            ret = bdrv_pread(bs->file, offset, bytes,
> +                             (uint8_t *) buf + offset, 0);
> +            if (ret < 0) {
> +                return ret;
> +            }
> +        }
> +        offset += bytes;
> +    }
> +}
> +
>   /*
>    * Read the entire contents of @blk into @buf.
>    * @blk's contents must be @size bytes, and @size must be at most
> @@ -53,7 +87,7 @@ bool blk_check_size_and_read_all(BlockBackend *blk, void 
> *buf, hwaddr size,
>        * block device and read only on demand.
>        */
>       assert(size <= BDRV_REQUEST_MAX_BYTES);
> -    ret = blk_pread(blk, 0, size, buf, 0);
> +    ret = blk_pread_nonzeroes(blk, size, buf);
>       if (ret < 0) {
>           error_setg_errno(errp, -ret, "can't read block backend");
>           return false;