Re: [PATCH v4 02/15] hw/display/apple-gfx: Introduce ParavirtualizedGraphics.Framework support

[Phil Dennis-Jordan]

On Sat, 26 Oct 2024 at 06:40, Akihiko Odaki 
wrote:

> On 2024/10/26 4:43, Phil Dennis-Jordan wrote:
> >
> >
> > On Fri, 25 Oct 2024 at 08:03, Akihiko Odaki  > > wrote:
> >
> > On 2024/10/24 19:28, Phil Dennis-Jordan wrote:
> >  > +/* For running PVG memory-mapping requests in the AIO
> context */
> >  > +QemuCond job_cond;
> >  > +QemuMutex job_mutex;
> >
> > Use: QemuEvent
> >
> >
> > Hmm. I think if we were to use that, we would need to create a new
> > QemuEvent for every job and destroy it afterward, which seems expensive.
> > We can't rule out multiple concurrent jobs being submitted, and the
> > QemuEvent system only supports a single producer as far as I can tell.
> >
> > You can probably sort of hack around it with just one QemuEvent by
> > putting the qemu_event_wait into a loop and turning the job.done flag
> > into an atomic (because it would now need to be checked outside the
> > lock) but this all seems unnecessarily complicated considering the
> > QemuEvent uses the same mechanism QemuCond/QemuMutex internally on macOS
> > (the only platform relevant here), except we can use it as intended with
> > QemuCond/QemuMutex rather than having to work against the abstraction.
>
> I don't think it's going to be used concurrently. It would be difficult
> to reason even for the framework if it performs memory
> unmapping/mapping/reading operations concurrently.


I've just performed a very quick test by wrapping the job submission/wait
in the 2 mapMemory callbacks and the 1 readMemory callback with atomic
counters and logging whenever a counter went above 1.

 * Overall, concurrent callbacks across all types were common (many per
second when the VM is busy). It's not exactly a "thundering herd" (I never
saw >2) but it's probably not a bad idea to use a separate condition
variable for each job type. (task map, surface map, memory read)
 * While I did not observe any concurrent memory mapping operations *within*
a type of memory map (2 task mappings or 2 surface mappings) I did see very
occasional concurrent memory *read* callbacks. These would, as far as I can
tell, not be safe with QemuEvents, unless we placed the event inside the
job struct and init/destroyed it on every callback (which seems like
excessive overhead).

My recommendation would be to split it up into 3 pairs of mutex/cond; this
will almost entirely remove any contention, but continue to be safe for
when it does occur. I don't think QemuEvent is a realistic option (too
tricky to get right) for the observed-concurrent readMemory callback. I'm
nervous about assuming the mapMemory callbacks will NEVER be called
concurrently, but at a push I'll acquiesce to switching those to QemuEvent
in the absence of evidence of concurrency.


> PGDevice.h also notes
> raiseInterrupt needs to be thread-safe while it doesn't make such notes
> for memory operations. This actually makes sense.
>
> If it's ever going to be used concurrently, it's better to have
> QemuEvent for each job to avoid the thundering herd problem.
>
> >
> >  > +
> >  > +dispatch_queue_t render_queue;
> >  > +/* The following fields should only be accessed from the
> BQL: */
> >
> > Perhaps it may be better to document fields that can be accessed
> > *without* the BQL; most things in QEMU implicitly require the BQL.
> >
> >  > +bool gfx_update_requested;
> >  > +bool new_frame_ready;
> >  > +bool using_managed_texture_storage;
> >  > +} AppleGFXState;
> >  > +
> >  > +void apple_gfx_common_init(Object *obj, AppleGFXState *s, const
> > char* obj_name);
> >  > +void apple_gfx_common_realize(AppleGFXState *s,
> > PGDeviceDescriptor *desc,
> >  > +  Error **errp);
> >  > +uintptr_t apple_gfx_host_address_for_gpa_range(uint64_t
> > guest_physical,
> >  > +   uint64_t length,
> > bool read_only);
> >  > +void apple_gfx_await_bh_job(AppleGFXState *s, bool
> *job_done_flag);
> >  > +
> >  > +#endif
> >  > +
> >  > diff --git a/hw/display/apple-gfx.m b/hw/display/apple-gfx.m
> >  > new file mode 100644
> >  > index 000..46be9957f69
> >  > --- /dev/null
> >  > +++ b/hw/display/apple-gfx.m
> >  > @@ -0,0 +1,713 @@
> >  > +/*
> >  > + * QEMU Apple ParavirtualizedGraphics.framework device
> >  > + *
> >  > + * Copyright © 2023 Amazon.com, Inc. or its affiliates. All
> > Rights Reserved.
> >  > + *
> >  > + * This work is licensed under the terms of the GNU GPL, version
> > 2 or later.
> >  > + * See the COPYING file in the top-level directory.
> >  > + *
> >  > + * ParavirtualizedGraphics.framework is a set of libraries that
> > macOS provides
> >  > + * which implements 3d graphics passthrough to the host as well
> as a
> >  > + * proprietary guest communication cha

Re: [PATCH 0/2] arm: Add collie and sx functional tests

[Cédric Le Goater]

On 10/26/24 07:54, Guenter Roeck wrote:

On 10/25/24 21:47, Philippe Mathieu-Daudé wrote:

On 25/10/24 12:25, Jan Lübbe wrote:

On Fri, 2024-10-25 at 06:59 -0700, Guenter Roeck wrote:

On 10/25/24 02:57, Jan Lübbe wrote:

On Fri, 2024-10-25 at 08:55 +0200, Cédric Le Goater wrote:

On 10/24/24 19:59, Philippe Mathieu-Daudé wrote:

Cc'ing Jan.

On 22/10/24 12:04, Guenter Roeck wrote:


I have no idea how this is supposed to work, and I don't think it works
as implemented. I'll revert commit e32ac563b83 in my local copy of qemu.
Jan, can you have a look at this bug report please? Otherwise I'll
have a look during soft freeze.


Guenter, just to make sure: In your case [1], you had "boot-emmc" (see
aspeed_machine_ast2600_class_emmc_init) enabled, right? Otherwise the


I tried both enabled and disabled.


boot partition size would be 0, meaning that the eMMC has no boot
partitions.


That is what I would have expected, but it does not reflect reality.


In that configuration, your backing file needs to have space for the
boot partitions at the start (2*1MiB) and the rootfs starts at the 2
MiB offset.



boot-emmc doesn't make a difference, because

  if (emmc) {
  qdev_prop_set_uint64(card, "boot-partition-size", 1 * MiB);
  qdev_prop_set_uint8(card, "boot-config",
  boot_emmc ? 0x1 << 3 : 0x0);
  }

in hw/arm/aspeed.c sets the boot partition size independently of the
boot-emmc flag.


Ah, yes. :/

So you can have SD, eMMC with boot from boot partition *disabled* or
eMMC with boot from boot partition *enabled*.


I suspect that the expectation is that I always provide
an emmc image with a 2 MB gap at the beginning, but in my opinion that is
really hyper-clumsy, and I simply won't do it, sorry.


I was surprised that the boot partitions' contents are stored in the
same backing file as the main area (instead of being separate files).
But I've not researched why it was designed this way.


Yeah I'd have preferred separate files too, but when it seemed
to be simpler for the single user case:
https://lore.kernel.org/qemu-devel/d48b6357-c839-4971-aa28-bdbd5b1ba...@kaod.org/



I don't mind a single file. What bothers me is that the partitioning is made
mandatory for ast2600 even if not used.


Our only use case, in 2019, was to boot QEMU ast2600 machines from an
eMMC device using an OpenBMC FW image like the ones we find on IBM
Power10 Rainier systems. I agree we only focused on this scenario.
Most of the support should be there for other use cases, and it's now
a question of finding the right tunables for the user.

I did a quick experiment using 2 patches,

one on hw/sd/sd.c to fix c8cb19876d3e ("hw/sd/sdcard: Support boot
area in emmc image")

@@ -826,7 +826,9 @@ static void sd_reset(DeviceState *dev)
 sect = 0;
 }
 size = sect << HWBLOCK_SHIFT;
-size -= sd_bootpart_offset(sd);
+if (sd_is_emmc(sd)) {
+size -= sd->boot_part_size * 2;
+}
 
 sect = sd_addr_to_wpnum(size) + 1;


and another on hw/arm/aspeed.c to remove the setting of the eMMC
device properties when it is not bootable :
  
@@ -338,7 +338,7 @@ static void sdhci_attach_drive(SDHCIStat

 return;
 }
 card = qdev_new(emmc ? TYPE_EMMC : TYPE_SD_CARD);
-if (emmc) {
+if (emmc && boot_emmc) {
 qdev_prop_set_uint64(card, "boot-partition-size", 1 * MiB);
 qdev_prop_set_uint8(card, "boot-config",
 boot_emmc ? 0x1 << 3 : 0x0);
  
(I am not saying this is correct)


I then generated an eMMC image from a raw disk image, and without the
extra 2M for the boot areas (for U-Boot binaries) :

$ fdisk -l   obmc-phosphor-image-rainier.wic
Disk obmc-phosphor-image-rainier.wic: 14.13 GiB, 15167689728 bytes, 
29624394 sectors
Units: sectors of 1 * 512 = 512 bytes
Sector size (logical/physical): 512 bytes / 512 bytes
I/O size (minimum/optimal): 512 bytes / 512 bytes
Disklabel type: gpt
Disk identifier: DB9A8B4B-4BB8-4D5E-83A3-67A3D4F64614

Device  Start  End  Sectors Size Type

obmc-phosphor-image-rainier.wic1   40 2087 2048   1M Microsoft 
basic data
obmc-phosphor-image-rainier.wic2 2088   133159   131072  64M Linux 
filesystem
obmc-phosphor-image-rainier.wic3   133160   264231   131072  64M Linux 
filesystem
obmc-phosphor-image-rainier.wic4   264232  2361383  2097152   1G Linux 
filesystem
obmc-phosphor-image-rainier.wic5  2361384  4458535  2097152   1G Linux 
filesystem
obmc-phosphor-image-rainier.wic6  4458536 19138599 14680064   7G Linux 
filesystem
obmc-phosphor-image-rainier.wic7 19138600 29624359 10485760   5G Linux 
filesystem

and used that to boot an ast2600-evb from flash with an emmc device.
  
/path/to/qemu-system-arm -M ast2600-evb -net nic,netdev=net0 -netdev id=n

[PATCH v4 4/5] block: allow commit to unmap zero blocks

[Vincent Vanlaer]

Non-active block commits do not discard blocks only containing zeros,
causing images to lose sparseness after the commit. This commit fixes
that by writing zero blocks using blk_co_pwrite_zeroes rather than
writing them out as any other arbitrary data.

Signed-off-by: Vincent Vanlaer 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 block/commit.c | 38 +-
 1 file changed, 29 insertions(+), 9 deletions(-)

diff --git a/block/commit.c b/block/commit.c
index 5c24c8b80a..ca1a367b41 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -149,19 +149,39 @@ static int commit_iteration(CommitBlockJob *s, int64_t 
offset,
 }
 
 if (ret & BDRV_BLOCK_ALLOCATED) {
-assert(bytes < SIZE_MAX);
+if (ret & BDRV_BLOCK_ZERO) {
+/*
+ * If the top (sub)clusters are smaller than the base
+ * (sub)clusters, this will not unmap unless the underlying device
+ * does some tracking of these requests. Ideally, we would find
+ * the maximal extent of the zero clusters.
+ */
+ret = blk_co_pwrite_zeroes(s->base, offset, bytes,
+   BDRV_REQ_MAY_UNMAP);
+if (ret < 0) {
+error_in_source = false;
+goto fail;
+}
+} else {
+assert(bytes < SIZE_MAX);
 
-ret = blk_co_pread(s->top, offset, bytes, buf, 0);
-if (ret < 0) {
-goto fail;
-}
+ret = blk_co_pread(s->top, offset, bytes, buf, 0);
+if (ret < 0) {
+goto fail;
+}
 
-ret = blk_co_pwrite(s->base, offset, bytes, buf, 0);
-if (ret < 0) {
-error_in_source = false;
-goto fail;
+ret = blk_co_pwrite(s->base, offset, bytes, buf, 0);
+if (ret < 0) {
+error_in_source = false;
+goto fail;
+}
 }
 
+/*
+ * Whether zeroes actually end up on disk depends on the details of
+ * the underlying driver. Therefore, this might rate limit more than
+ * is necessary.
+ */
 block_job_ratelimit_processed_bytes(&s->common, bytes);
 }
 
-- 
2.44.1

[PATCH v4 3/5] block: refactor error handling of commit_iteration

[Vincent Vanlaer]

Signed-off-by: Vincent Vanlaer 
---
 block/commit.c | 61 --
 1 file changed, 34 insertions(+), 27 deletions(-)

diff --git a/block/commit.c b/block/commit.c
index 078e54f51f..5c24c8b80a 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -129,51 +129,58 @@ static void commit_clean(Job *job)
 }
 
 static int commit_iteration(CommitBlockJob *s, int64_t offset,
-int64_t *n, void *buf)
+int64_t *requested_bytes, void *buf)
 {
+int64_t bytes = *requested_bytes;
 int ret = 0;
-bool copy;
 bool error_in_source = true;
 
 /* Copy if allocated above the base */
 WITH_GRAPH_RDLOCK_GUARD() {
 ret = bdrv_co_common_block_status_above(blk_bs(s->top),
 s->base_overlay, true, true, offset, COMMIT_BUFFER_SIZE,
-n, NULL, NULL, NULL);
+&bytes, NULL, NULL, NULL);
 }
 
-copy = (ret >= 0 && ret & BDRV_BLOCK_ALLOCATED);
-trace_commit_one_iteration(s, offset, *n, ret);
-if (copy) {
-assert(*n < SIZE_MAX);
+trace_commit_one_iteration(s, offset, bytes, ret);
 
-ret = blk_co_pread(s->top, offset, *n, buf, 0);
-if (ret >= 0) {
-ret = blk_co_pwrite(s->base, offset, *n, buf, 0);
-if (ret < 0) {
-error_in_source = false;
-}
-}
-}
 if (ret < 0) {
-BlockErrorAction action = block_job_error_action(&s->common,
- s->on_error,
- error_in_source,
- -ret);
-if (action == BLOCK_ERROR_ACTION_REPORT) {
-return ret;
-} else {
-*n = 0;
-return 0;
+goto fail;
+}
+
+if (ret & BDRV_BLOCK_ALLOCATED) {
+assert(bytes < SIZE_MAX);
+
+ret = blk_co_pread(s->top, offset, bytes, buf, 0);
+if (ret < 0) {
+goto fail;
 }
+
+ret = blk_co_pwrite(s->base, offset, bytes, buf, 0);
+if (ret < 0) {
+error_in_source = false;
+goto fail;
+}
+
+block_job_ratelimit_processed_bytes(&s->common, bytes);
 }
+
 /* Publish progress */
-job_progress_update(&s->common.job, *n);
 
-if (copy) {
-block_job_ratelimit_processed_bytes(&s->common, *n);
+job_progress_update(&s->common.job, bytes);
+
+*requested_bytes = bytes;
+
+return 0;
+fail:;
+BlockErrorAction action = block_job_error_action(&s->common, s->on_error,
+ error_in_source, -ret);
+if (action == BLOCK_ERROR_ACTION_REPORT) {
+return ret;
 }
 
+*requested_bytes = 0;
+
 return 0;
 }
 
-- 
2.44.1

Re: [PATCH 0/2] arm: Add collie and sx functional tests

[Guenter Roeck]

On 10/26/24 03:02, Cédric Le Goater wrote:
[ ... ]


I don't mind a single file. What bothers me is that the partitioning is made
mandatory for ast2600 even if not used.


Our only use case, in 2019, was to boot QEMU ast2600 machines from an
eMMC device using an OpenBMC FW image like the ones we find on IBM
Power10 Rainier systems. I agree we only focused on this scenario.
Most of the support should be there for other use cases, and it's now
a question of finding the right tunables for the user.

I did a quick experiment using 2 patches,

one on hw/sd/sd.c to fix c8cb19876d3e ("hw/sd/sdcard: Support boot
area in emmc image")

     @@ -826,7 +826,9 @@ static void sd_reset(DeviceState *dev)
  sect = 0;
  }
  size = sect << HWBLOCK_SHIFT;
     -    size -= sd_bootpart_offset(sd);
     +    if (sd_is_emmc(sd)) {
     +    size -= sd->boot_part_size * 2;
     +    }
  sect = sd_addr_to_wpnum(size) + 1;

and another on hw/arm/aspeed.c to remove the setting of the eMMC
device properties when it is not bootable :
     @@ -338,7 +338,7 @@ static void sdhci_attach_drive(SDHCIStat
  return;
  }
  card = qdev_new(emmc ? TYPE_EMMC : TYPE_SD_CARD);
     -    if (emmc) {
     +    if (emmc && boot_emmc) {
  qdev_prop_set_uint64(card, "boot-partition-size", 1 * MiB);
  qdev_prop_set_uint8(card, "boot-config",
  boot_emmc ? 0x1 << 3 : 0x0);
(I am not saying this is correct)



Works for me, though, and it is much better than mandating the existence
of boot partitions.

If you end up submitting those patches, please feel free to add

Tested-by: Guenter Roeck 

Thanks,
Guenter

Re: [PATCH v4 15/15] hw/vmapple/vmapple: Add vmapple machine type

[Phil Dennis-Jordan]

On Sat, 26 Oct 2024 at 08:21, Akihiko Odaki 
wrote:

> On 2024/10/24 19:28, Phil Dennis-Jordan wrote:
> > From: Alexander Graf 
> >
> > Apple defines a new "vmapple" machine type as part of its proprietary
> > macOS Virtualization.Framework vmm. This machine type is similar to the
> > virt one, but with subtle differences in base devices, a few special
> > vmapple device additions and a vastly different boot chain.
> >
> > This patch reimplements this machine type in QEMU. To use it, you
> > have to have a readily installed version of macOS for VMApple,
> > run on macOS with -accel hvf, pass the Virtualization.Framework
> > boot rom (AVPBooter) in via -bios, pass the aux and root volume as pflash
> > and pass aux and root volume as virtio drives. In addition, you also
> > need to find the machine UUID and pass that as -M vmapple,uuid=
> parameter:
> >
> > $ qemu-system-aarch64 -accel hvf -M vmapple,uuid=0x1234 -m 4G \
> >  -bios
> /System/Library/Frameworks/Virtualization.framework/Versions/A/Resources/AVPBooter.vmapple2.bin
> >  -drive file=aux,if=pflash,format=raw \
> >  -drive file=root,if=pflash,format=raw \
> >  -drive file=aux,if=none,id=aux,format=raw \
> >  -device vmapple-virtio-aux,drive=aux \
> >  -drive file=root,if=none,id=root,format=raw \
> >  -device vmapple-virtio-root,drive=root
> >
> > With all these in place, you should be able to see macOS booting
> > successfully.
> >
> > Known issues:
> >   - Keyboard and mouse/tablet input is laggy. The reason for this is
> > either that macOS's XHCI driver is broken when the device/platform
> > does not support MSI/MSI-X, or there's some unfortunate interplay
> > with Qemu's XHCI implementation in this scenario.
> >   - Currently only macOS 12 guests are supported. The boot process for
> > 13+ will need further investigation and adjustment.
> >
> > Signed-off-by: Alexander Graf 
> > Co-authored-by: Phil Dennis-Jordan 
> > Signed-off-by: Phil Dennis-Jordan 
> > ---
> > v3:
> >   * Rebased on latest upstream, updated affinity and NIC creation
> > API usage
> >   * Included Apple-variant virtio-blk in build dependency
> >   * Updated API usage for setting 'redist-region-count' array-typed
> property on GIC.
> >   * Switched from virtio HID devices (for which macOS 12 does not
> contain drivers) to an XHCI USB controller and USB HID devices.
> >
> > v4:
> >   * Fixups for v4 changes to the other patches in the set.
> >   * Corrected the assert macro to use
> >   * Removed superfluous endian conversions corresponding to cfg's.
> >   * Init error handling improvement.
> >   * No need to select CPU type on TCG, as only HVF is supported.
> >   * Machine type version bumped to 9.2
> >   * #include order improved
> >
> >   MAINTAINERS |   1 +
> >   docs/system/arm/vmapple.rst |  63 
> >   docs/system/target-arm.rst  |   1 +
> >   hw/vmapple/Kconfig  |  20 ++
> >   hw/vmapple/meson.build  |   1 +
> >   hw/vmapple/vmapple.c| 652 
> >   6 files changed, 738 insertions(+)
> >   create mode 100644 docs/system/arm/vmapple.rst
> >   create mode 100644 hw/vmapple/vmapple.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 104813ed85f..f44418b4a95 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -2739,6 +2739,7 @@ R: Phil Dennis-Jordan 
> >   S: Maintained
> >   F: hw/vmapple/*
> >   F: include/hw/vmapple/*
> > +F: docs/system/arm/vmapple.rst
> >
> >   Subsystems
> >   --
> > diff --git a/docs/system/arm/vmapple.rst b/docs/system/arm/vmapple.rst
> > new file mode 100644
> > index 000..acb921ffb35
> > --- /dev/null
> > +++ b/docs/system/arm/vmapple.rst
> > @@ -0,0 +1,63 @@
> > +VMApple machine emulation
> >
> +
> > +
> > +VMApple is the device model that the macOS built-in hypervisor called
> "Virtualization.framework"
> > +exposes to Apple Silicon macOS guests. The "vmapple" machine model in
> QEMU implements the same
> > +device model, but does not use any code from Virtualization.Framework.
> > +
> > +Prerequisites
> > +-
> > +
> > +To run the vmapple machine model, you need to
> > +
> > + * Run on Apple Silicon
> > + * Run on macOS 12.0 or above
> > + * Have an already installed copy of a Virtualization.Framework macOS
> 12 virtual machine. I will
> > +   assume that you installed it using the macosvm CLI.
> > +
> > +First, we need to extract the UUID from the virtual machine that you
> installed. You can do this
> > +by running the following shell script:
> > +
> > +.. code-block:: bash
> > +  :caption: uuid.sh script to extract the UUID from a macosvm.json file
> > +
> > +  #!/bin/bash
> > +
> > +  MID=$(cat "$1" | python3 -c 'import
> json,sys;obj=json.load(sys.stdin);print(obj["machineId"]);')
> > +  echo "$MID" | base64 -d | plutil -extract ECID raw -
>
> I prefer it to be written entirely in Python instead of a

[PATCH v4 0/5] block: allow commit to unmap zero blocks

[Vincent Vanlaer]

This patch series adds support for zero blocks in non-active commits.
The first three patches in the series refactor the relevant code, patch
four makes the actual changes, and the last patch adds a test for the
new functionality.

---

Changes since v3:
- minor reformating based on checkpatch.pl
- moved tracepoint in commit_iteration before first possible return on
  error
- renamed the handle_error label in commit_iteration to fail and
  prevented the happy path from passing through this label
- moved test script to the tests/qemu-iotests/tests folder and named it
  commit-zero-blocks

Changes since v2:
- moved main loop of commit_run to a separate function and refactored
  the error handling.
- call blk_co_pwrite_zero even if the size of the zero region does not
  align with the sectors of the base image. This removes the need for
  the CommitMethod enum

Changes since v1:
- split up the implementation in three separate commits
- removed accidentally left over includes from testing

Vincent Vanlaer (5):
  block: get type of block allocation in commit_run
  block: move commit_run loop to separate function
  block: refactor error handling of commit_iteration
  block: allow commit to unmap zero blocks
  block: add test non-active commit with zeroed data

 block/commit.c| 116 +-
 tests/qemu-iotests/tests/commit-zero-blocks   |  96 +++
 .../qemu-iotests/tests/commit-zero-blocks.out |  54 
 3 files changed, 232 insertions(+), 34 deletions(-)
 create mode 100755 tests/qemu-iotests/tests/commit-zero-blocks
 create mode 100644 tests/qemu-iotests/tests/commit-zero-blocks.out

-- 
2.44.1

[PATCH v4 2/5] block: move commit_run loop to separate function

[Vincent Vanlaer]

Signed-off-by: Vincent Vanlaer 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 block/commit.c | 89 +-
 1 file changed, 52 insertions(+), 37 deletions(-)

diff --git a/block/commit.c b/block/commit.c
index 8dee25b313..078e54f51f 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -128,6 +128,55 @@ static void commit_clean(Job *job)
 blk_unref(s->top);
 }
 
+static int commit_iteration(CommitBlockJob *s, int64_t offset,
+int64_t *n, void *buf)
+{
+int ret = 0;
+bool copy;
+bool error_in_source = true;
+
+/* Copy if allocated above the base */
+WITH_GRAPH_RDLOCK_GUARD() {
+ret = bdrv_co_common_block_status_above(blk_bs(s->top),
+s->base_overlay, true, true, offset, COMMIT_BUFFER_SIZE,
+n, NULL, NULL, NULL);
+}
+
+copy = (ret >= 0 && ret & BDRV_BLOCK_ALLOCATED);
+trace_commit_one_iteration(s, offset, *n, ret);
+if (copy) {
+assert(*n < SIZE_MAX);
+
+ret = blk_co_pread(s->top, offset, *n, buf, 0);
+if (ret >= 0) {
+ret = blk_co_pwrite(s->base, offset, *n, buf, 0);
+if (ret < 0) {
+error_in_source = false;
+}
+}
+}
+if (ret < 0) {
+BlockErrorAction action = block_job_error_action(&s->common,
+ s->on_error,
+ error_in_source,
+ -ret);
+if (action == BLOCK_ERROR_ACTION_REPORT) {
+return ret;
+} else {
+*n = 0;
+return 0;
+}
+}
+/* Publish progress */
+job_progress_update(&s->common.job, *n);
+
+if (copy) {
+block_job_ratelimit_processed_bytes(&s->common, *n);
+}
+
+return 0;
+}
+
 static int coroutine_fn commit_run(Job *job, Error **errp)
 {
 CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
@@ -158,9 +207,6 @@ static int coroutine_fn commit_run(Job *job, Error **errp)
 buf = blk_blockalign(s->top, COMMIT_BUFFER_SIZE);
 
 for (offset = 0; offset < len; offset += n) {
-bool copy;
-bool error_in_source = true;
-
 /* Note that even when no rate limit is applied we need to yield
  * with no pending I/O here so that bdrv_drain_all() returns.
  */
@@ -168,42 +214,11 @@ static int coroutine_fn commit_run(Job *job, Error **errp)
 if (job_is_cancelled(&s->common.job)) {
 break;
 }
-/* Copy if allocated above the base */
-WITH_GRAPH_RDLOCK_GUARD() {
-ret = bdrv_co_common_block_status_above(blk_bs(s->top),
-s->base_overlay, true, true, offset, COMMIT_BUFFER_SIZE,
-&n, NULL, NULL, NULL);
-}
 
-copy = (ret >= 0 && ret & BDRV_BLOCK_ALLOCATED);
-trace_commit_one_iteration(s, offset, n, ret);
-if (copy) {
-assert(n < SIZE_MAX);
-
-ret = blk_co_pread(s->top, offset, n, buf, 0);
-if (ret >= 0) {
-ret = blk_co_pwrite(s->base, offset, n, buf, 0);
-if (ret < 0) {
-error_in_source = false;
-}
-}
-}
-if (ret < 0) {
-BlockErrorAction action =
-block_job_error_action(&s->common, s->on_error,
-   error_in_source, -ret);
-if (action == BLOCK_ERROR_ACTION_REPORT) {
-return ret;
-} else {
-n = 0;
-continue;
-}
-}
-/* Publish progress */
-job_progress_update(&s->common.job, n);
+ret = commit_iteration(s, offset, &n, buf);
 
-if (copy) {
-block_job_ratelimit_processed_bytes(&s->common, n);
+if (ret < 0) {
+return ret;
 }
 }
 
-- 
2.44.1

[PATCH v4 5/5] block: add test non-active commit with zeroed data

[Vincent Vanlaer]

Signed-off-by: Vincent Vanlaer 
Tested-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/tests/commit-zero-blocks   | 96 +++
 .../qemu-iotests/tests/commit-zero-blocks.out | 54 +++
 2 files changed, 150 insertions(+)
 create mode 100755 tests/qemu-iotests/tests/commit-zero-blocks
 create mode 100644 tests/qemu-iotests/tests/commit-zero-blocks.out

diff --git a/tests/qemu-iotests/tests/commit-zero-blocks 
b/tests/qemu-iotests/tests/commit-zero-blocks
new file mode 100755
index 00..de00273e72
--- /dev/null
+++ b/tests/qemu-iotests/tests/commit-zero-blocks
@@ -0,0 +1,96 @@
+#!/usr/bin/env bash
+# group: rw quick
+#
+# Test for commit of discarded blocks
+#
+# This tests committing a live snapshot where some of the blocks that
+# are present in the base image are discarded in the intermediate image.
+# This intends to check that these blocks are also discarded in the base
+# image after the commit.
+#
+# Copyright (C) 2024 Vincent Vanlaer.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+# creator
+owner=libvirt-e6954...@volkihar.be
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+status=1   # failure is the default!
+
+_cleanup()
+{
+_cleanup_qemu
+_rm_test_img "${TEST_IMG}.base"
+_rm_test_img "${TEST_IMG}.mid"
+_cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+cd ..
+. ./common.rc
+. ./common.filter
+. ./common.qemu
+
+_supported_fmt qcow2
+_supported_proto file
+
+size="1M"
+
+TEST_IMG="$TEST_IMG.base" _make_test_img $size
+TEST_IMG="$TEST_IMG.mid" _make_test_img -b "$TEST_IMG.base" -F $IMGFMT $size
+_make_test_img -b "${TEST_IMG}.mid" -F $IMGFMT $size
+
+$QEMU_IO -c "write -P 0x01 64k 128k" "$TEST_IMG.base" | _filter_qemu_io
+$QEMU_IO -c "discard 64k 64k" "$TEST_IMG.mid" | _filter_qemu_io
+
+echo
+echo "=== Base image info before commit ==="
+TEST_IMG="${TEST_IMG}.base" _img_info | _filter_img_info
+$QEMU_IMG map --output=json "$TEST_IMG.base" | _filter_qemu_img_map
+
+echo
+echo "=== Middle image info before commit ==="
+TEST_IMG="${TEST_IMG}.mid" _img_info | _filter_img_info
+$QEMU_IMG map --output=json "$TEST_IMG.mid" | _filter_qemu_img_map
+
+echo
+echo === Running QEMU Live Commit Test ===
+echo
+
+qemu_comm_method="qmp"
+_launch_qemu -drive file="${TEST_IMG}",if=virtio,id=test
+h=$QEMU_HANDLE
+
+_send_qemu_cmd $h "{ 'execute': 'qmp_capabilities' }" "return"
+
+_send_qemu_cmd $h "{ 'execute': 'block-commit',
+ 'arguments': { 'device': 'test',
+ 'top': '"${TEST_IMG}.mid"',
+ 'base': '"${TEST_IMG}.base"'} }" '"status": 
"null"'
+
+_cleanup_qemu
+
+echo
+echo "=== Base image info after commit ==="
+TEST_IMG="${TEST_IMG}.base" _img_info | _filter_img_info
+$QEMU_IMG map --output=json "$TEST_IMG.base" | _filter_qemu_img_map
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/tests/commit-zero-blocks.out 
b/tests/qemu-iotests/tests/commit-zero-blocks.out
new file mode 100644
index 00..85bdc46aaf
--- /dev/null
+++ b/tests/qemu-iotests/tests/commit-zero-blocks.out
@@ -0,0 +1,54 @@
+QA output created by commit-zero-blocks
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=1048576
+Formatting 'TEST_DIR/t.IMGFMT.mid', fmt=IMGFMT size=1048576 
backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=IMGFMT
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 
backing_file=TEST_DIR/t.IMGFMT.mid backing_fmt=IMGFMT
+wrote 131072/131072 bytes at offset 65536
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+discard 65536/65536 bytes at offset 65536
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+=== Base image info before commit ===
+image: TEST_DIR/t.IMGFMT.base
+file format: IMGFMT
+virtual size: 1 MiB (1048576 bytes)
+[{ "start": 0, "length": 65536, "depth": 0, "present": false, "zero": true, 
"data": false, "compressed": false},
+{ "start": 65536, "length": 131072, "depth": 0, "present": true, "zero": 
false, "data": true, "compressed": false, "offset": OFFSET},
+{ "start": 196608, "length": 851968, "depth": 0, "present": false, "zero": 
true, "data": false, "compressed": false}]
+
+=== Middle image info before commit ===
+image: TEST_DIR/t.IMGFMT.mid
+file format: IMGFMT
+virtual

[PATCH v4 1/5] block: get type of block allocation in commit_run

[Vincent Vanlaer]

bdrv_co_common_block_status_above not only returns whether the block is
allocated, but also if it contains zeroes.

Signed-off-by: Vincent Vanlaer 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 block/commit.c | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/block/commit.c b/block/commit.c
index 7c3fdcb0ca..8dee25b313 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -15,6 +15,8 @@
 #include "qemu/osdep.h"
 #include "qemu/cutils.h"
 #include "trace.h"
+#include "block/block-common.h"
+#include "block/coroutines.h"
 #include "block/block_int.h"
 #include "block/blockjob_int.h"
 #include "qapi/error.h"
@@ -167,9 +169,13 @@ static int coroutine_fn commit_run(Job *job, Error **errp)
 break;
 }
 /* Copy if allocated above the base */
-ret = blk_co_is_allocated_above(s->top, s->base_overlay, true,
-offset, COMMIT_BUFFER_SIZE, &n);
-copy = (ret > 0);
+WITH_GRAPH_RDLOCK_GUARD() {
+ret = bdrv_co_common_block_status_above(blk_bs(s->top),
+s->base_overlay, true, true, offset, COMMIT_BUFFER_SIZE,
+&n, NULL, NULL, NULL);
+}
+
+copy = (ret >= 0 && ret & BDRV_BLOCK_ALLOCATED);
 trace_commit_one_iteration(s, offset, n, ret);
 if (copy) {
 assert(n < SIZE_MAX);
-- 
2.44.1