|
From: | Philippe Mathieu-Daudé |
Subject: | Re: [PATCH v2] pflash: Only read non-zero parts of backend image |
Date: | Tue, 20 Dec 2022 10:30:43 +0100 |
User-agent: | Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.5.1 |
[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;
[Prev in Thread] | Current Thread | [Next in Thread] |