Re: [PATCH v2 0/8] vhost-user-blk: dynamically resize config space based on features

[Raphael Norwitz]

> ping

Apologies for the late review - busy week. First pass looks good but will review
comprehensively tomorrow or over the weekend.

Re: [PATCH v8 3/7] block: add block layer APIs resembling Linux ZonedBlockDevice ioctls

[Damien Le Moal]

On 9/1/22 23:57, Markus Armbruster wrote:
> Sam Li  writes:
> 
>> Markus Armbruster  于2022年8月31日周三 16:35写道：
>>>
>>> Sam Li  writes:
>>>
 Markus Armbruster  于2022年8月30日周二 19:57写道：
>
> Sam Li  writes:
>
>> By adding zone management operations in BlockDriver, storage controller
>> emulation can use the new block layer APIs including Report Zone and
>> four zone management operations (open, close, finish, reset).
>>
>> Add zoned storage commands of the device: zone_report(zrp), 
>> zone_open(zo),
>> zone_close(zc), zone_reset(zrs), zone_finish(zf).
>>
>> For example, to test zone_report, use following command:
>> $ ./build/qemu-io --image-opts driver=zoned_host_device, 
>> filename=/dev/nullb0
>> -c "zrp offset nr_zones"
>>
>> Signed-off-by: Sam Li 
>> Reviewed-by: Hannes Reinecke 
>
> [...]
>
>> diff --git a/block/file-posix.c b/block/file-posix.c
>> index 0a8b4b426e..e3efba6db7 100644
>> --- a/block/file-posix.c
>> +++ b/block/file-posix.c
>
> [...]
>
>> @@ -3752,6 +4025,54 @@ static BlockDriver bdrv_host_device = {
>>  #endif
>>  };
>>
>> +#if defined(CONFIG_BLKZONED)
>> +static BlockDriver bdrv_zoned_host_device = {
>> +.format_name = "zoned_host_device",
>
> Indentation should be 4, not 8.
>
>> +.protocol_name = "zoned_host_device",
>> +.instance_size = sizeof(BDRVRawState),
>> +.bdrv_needs_filename = true,
>> +.bdrv_probe_device  = hdev_probe_device,
>> +.bdrv_file_open = hdev_open,
>> +.bdrv_close = raw_close,
>> +.bdrv_reopen_prepare = raw_reopen_prepare,
>> +.bdrv_reopen_commit  = raw_reopen_commit,
>> +.bdrv_reopen_abort   = raw_reopen_abort,
>> +.bdrv_co_create_opts = bdrv_co_create_opts_simple,
>> +.create_opts = _create_opts_simple,
>> +.mutable_opts= mutable_opts,
>> +.bdrv_co_invalidate_cache = raw_co_invalidate_cache,
>> +.bdrv_co_pwrite_zeroes = hdev_co_pwrite_zeroes,
>> +
>> +.bdrv_co_preadv = raw_co_preadv,
>> +.bdrv_co_pwritev= raw_co_pwritev,
>> +.bdrv_co_flush_to_disk  = raw_co_flush_to_disk,
>> +.bdrv_co_pdiscard   = hdev_co_pdiscard,
>> +.bdrv_co_copy_range_from = raw_co_copy_range_from,
>> +.bdrv_co_copy_range_to  = raw_co_copy_range_to,
>> +.bdrv_refresh_limits = raw_refresh_limits,
>> +.bdrv_io_plug = raw_aio_plug,
>> +.bdrv_io_unplug = raw_aio_unplug,
>> +.bdrv_attach_aio_context = raw_aio_attach_aio_context,
>> +
>> +.bdrv_co_truncate   = raw_co_truncate,
>> +.bdrv_getlength = raw_getlength,
>> +.bdrv_get_info = raw_get_info,
>> +.bdrv_get_allocated_file_size
>> += raw_get_allocated_file_size,
>> +.bdrv_get_specific_stats = hdev_get_specific_stats,
>> +.bdrv_check_perm = raw_check_perm,
>> +.bdrv_set_perm   = raw_set_perm,
>> +.bdrv_abort_perm_update = raw_abort_perm_update,
>> +.bdrv_probe_blocksizes = hdev_probe_blocksizes,
>> +.bdrv_probe_geometry = hdev_probe_geometry,
>> +.bdrv_co_ioctl = hdev_co_ioctl,
>> +
>> +/* zone management operations */
>> +.bdrv_co_zone_report = raw_co_zone_report,
>> +.bdrv_co_zone_mgmt = raw_co_zone_mgmt,
>> +};
>
> Differences to bdrv_host_device:
>
> * .bdrv_parse_filename is not set
>
> * .bdrv_co_ioctl is not set
>
> * .bdrv_co_zone_report and .bdrv_co_zone_mgmt are set

 As Stefan mentioned, zoned_host_device is a new driver that doesn't
 work with string filenames. .bdrv_parse_filename() helps legacy
 drivers strip the optional protocol prefix off the filename and no use
 here. Therefore it can be dropped.
>>>
>>> Makes sense.
>>>
 .bdrv_co_ioctl is set actually.
>>>
>>> You're right; I diffed the two and misread the result.
>>>
 Zoned_host_device is basically host_device + zone operations. It
 serves for a simple purpose: if the host device is zoned, register
 zoned_host_device driver; else, register host_device.
>>>
>>> Why would I ever want to use host_device instead of zoned_host_device?
>>>
>>> To answer this question, we need to understand how their behavior
>>> differs.
>>>
>>> We can ignore the legacy protocol prefix / string filename part.
>>>
>>> All that's left seems to be "if the host device is zoned, then using the
>>> zoned_host_device driver gets you the zoned features, whereas using the
>>> host_device driver doesn't".  What am I missing?
>>
>> I think that's basically what users need to know about.
> 
> Now answer my previous question, please: why would 

Re: [PATCH v7 00/10] parallels: Refactor the code of images checks and fix a bug

[Denis V. Lunev]

On 29.08.2022 12:12, Alexander Ivanov wrote:

Fix image inflation when offset in BAT is out of image.

Replace whole BAT syncing by flushing only dirty blocks.

Move all the checks outside the main check function in
separate functions

Use WITH_QEMU_LOCK_GUARD for simplier code.

v7:
1,2: Fix string lengths in the commit messages.
3: Fix a typo in the commit message.

v6:
1: Move the error check inside the loop. Move file size getting
to the function beginning. Skip out-of-image offsets.
2: A new patch - don't let high_off be more than the end of the last cluster.
3: Set data_end without any condition.
7: Move data_end setting to parallels_check_outside_image().
8: Remove s->data_end setting from parallels_check_leak().
Fix 'i' type.

v5:
2: Change the way of data_end fixing.
6,7: Move data_end check to parallels_check_leak().

v4:
1: Move s->data_end fix to parallels_co_check(). Split the check
in parallels_open() and the fix in parallels_co_check() to two patches.
2: A new patch - a part of the patch 1.
Add a fix for data_end to parallels_co_check().
3: Move offset convertation to parallels_set_bat_entry().
4: Fix 'ret' rewriting by bdrv_co_flush() results.
7: Keep 'i' as uint32_t.

v3:

1-8: Fix commit message.

v2:

2: A new patch - a part of the splitted patch 2.
3: Patch order was changed so the replacement is done in parallels_co_check.
Now we use a helper to set BAT entry and mark the block dirty.
4: Revert the condition with s->header_unclean.
5: Move unrelated helper parallels_set_bat_entry creation to a separate patch.
7: Move fragmentation counting code to this function too.
8: Fix an incorrect usage of WITH_QEMU_LOCK_GUARD.


Alexander Ivanov (10):
   parallels: Out of image offset in BAT leads to image inflation
   parallels: Fix high_off calculation in parallels_co_check()
   parallels: Fix data_end after out-of-image check
   parallels: create parallels_set_bat_entry_helper() to assign BAT value
   parallels: Use generic infrastructure for BAT writing in
 parallels_co_check()
   parallels: Move check of unclean image to a separate function
   parallels: Move check of cluster outside image to a separate function
   parallels: Move check of leaks to a separate function
   parallels: Move statistic collection to a separate function
   parallels: Replace qemu_co_mutex_lock by WITH_QEMU_LOCK_GUARD

  block/parallels.c | 193 +-
  1 file changed, 138 insertions(+), 55 deletions(-)


queued, thanks

Re: [PATCH v8 3/7] block: add block layer APIs resembling Linux ZonedBlockDevice ioctls

[Markus Armbruster]

Markus Armbruster  writes:

> Sam Li  writes:
>
>> Markus Armbruster  于2022年8月31日周三 16:35写道：
>>>
>>> Sam Li  writes:
>>>
>>> > Markus Armbruster  于2022年8月30日周二 19:57写道：
>>> >>
>>> >> Sam Li  writes:
>>> >>
>>> >> > By adding zone management operations in BlockDriver, storage controller
>>> >> > emulation can use the new block layer APIs including Report Zone and
>>> >> > four zone management operations (open, close, finish, reset).
>>> >> >
>>> >> > Add zoned storage commands of the device: zone_report(zrp), 
>>> >> > zone_open(zo),
>>> >> > zone_close(zc), zone_reset(zrs), zone_finish(zf).
>>> >> >
>>> >> > For example, to test zone_report, use following command:
>>> >> > $ ./build/qemu-io --image-opts driver=zoned_host_device, 
>>> >> > filename=/dev/nullb0
>>> >> > -c "zrp offset nr_zones"
>>> >> >
>>> >> > Signed-off-by: Sam Li 
>>> >> > Reviewed-by: Hannes Reinecke 
>>> >>
>>> >> [...]
>>> >>
>>> >> > diff --git a/block/file-posix.c b/block/file-posix.c
>>> >> > index 0a8b4b426e..e3efba6db7 100644
>>> >> > --- a/block/file-posix.c
>>> >> > +++ b/block/file-posix.c
>>> >>
>>> >> [...]
>>> >>
>>> >> > @@ -3752,6 +4025,54 @@ static BlockDriver bdrv_host_device = {
>>> >> >  #endif
>>> >> >  };
>>> >> >
>>> >> > +#if defined(CONFIG_BLKZONED)
>>> >> > +static BlockDriver bdrv_zoned_host_device = {
>>> >> > +.format_name = "zoned_host_device",
>>> >>
>>> >> Indentation should be 4, not 8.
>>> >>
>>> >> > +.protocol_name = "zoned_host_device",
>>> >> > +.instance_size = sizeof(BDRVRawState),
>>> >> > +.bdrv_needs_filename = true,
>>> >> > +.bdrv_probe_device  = hdev_probe_device,
>>> >> > +.bdrv_file_open = hdev_open,
>>> >> > +.bdrv_close = raw_close,
>>> >> > +.bdrv_reopen_prepare = raw_reopen_prepare,
>>> >> > +.bdrv_reopen_commit  = raw_reopen_commit,
>>> >> > +.bdrv_reopen_abort   = raw_reopen_abort,
>>> >> > +.bdrv_co_create_opts = bdrv_co_create_opts_simple,
>>> >> > +.create_opts = _create_opts_simple,
>>> >> > +.mutable_opts= mutable_opts,
>>> >> > +.bdrv_co_invalidate_cache = raw_co_invalidate_cache,
>>> >> > +.bdrv_co_pwrite_zeroes = hdev_co_pwrite_zeroes,
>>> >> > +
>>> >> > +.bdrv_co_preadv = raw_co_preadv,
>>> >> > +.bdrv_co_pwritev= raw_co_pwritev,
>>> >> > +.bdrv_co_flush_to_disk  = raw_co_flush_to_disk,
>>> >> > +.bdrv_co_pdiscard   = hdev_co_pdiscard,
>>> >> > +.bdrv_co_copy_range_from = raw_co_copy_range_from,
>>> >> > +.bdrv_co_copy_range_to  = raw_co_copy_range_to,
>>> >> > +.bdrv_refresh_limits = raw_refresh_limits,
>>> >> > +.bdrv_io_plug = raw_aio_plug,
>>> >> > +.bdrv_io_unplug = raw_aio_unplug,
>>> >> > +.bdrv_attach_aio_context = raw_aio_attach_aio_context,
>>> >> > +
>>> >> > +.bdrv_co_truncate   = raw_co_truncate,
>>> >> > +.bdrv_getlength = raw_getlength,
>>> >> > +.bdrv_get_info = raw_get_info,
>>> >> > +.bdrv_get_allocated_file_size
>>> >> > += raw_get_allocated_file_size,
>>> >> > +.bdrv_get_specific_stats = hdev_get_specific_stats,
>>> >> > +.bdrv_check_perm = raw_check_perm,
>>> >> > +.bdrv_set_perm   = raw_set_perm,
>>> >> > +.bdrv_abort_perm_update = raw_abort_perm_update,
>>> >> > +.bdrv_probe_blocksizes = hdev_probe_blocksizes,
>>> >> > +.bdrv_probe_geometry = hdev_probe_geometry,
>>> >> > +.bdrv_co_ioctl = hdev_co_ioctl,
>>> >> > +
>>> >> > +/* zone management operations */
>>> >> > +.bdrv_co_zone_report = raw_co_zone_report,
>>> >> > +.bdrv_co_zone_mgmt = raw_co_zone_mgmt,
>>> >> > +};
>>> >>
>>> >> Differences to bdrv_host_device:
>>> >>
>>> >> * .bdrv_parse_filename is not set
>>> >>
>>> >> * .bdrv_co_ioctl is not set
>>> >>
>>> >> * .bdrv_co_zone_report and .bdrv_co_zone_mgmt are set
>>> >
>>> > As Stefan mentioned, zoned_host_device is a new driver that doesn't
>>> > work with string filenames. .bdrv_parse_filename() helps legacy
>>> > drivers strip the optional protocol prefix off the filename and no use
>>> > here. Therefore it can be dropped.
>>>
>>> Makes sense.
>>>
>>> > .bdrv_co_ioctl is set actually.
>>>
>>> You're right; I diffed the two and misread the result.
>>>
>>> > Zoned_host_device is basically host_device + zone operations. It
>>> > serves for a simple purpose: if the host device is zoned, register
>>> > zoned_host_device driver; else, register host_device.
>>>
>>> Why would I ever want to use host_device instead of zoned_host_device?
>>>
>>> To answer this question, we need to understand how their behavior
>>> differs.
>>>
>>> We can ignore the legacy protocol prefix / string filename part.
>>>
>>> All that's left seems to be "if the host device is zoned, then using the
>>> zoned_host_device driver gets you the zoned features, whereas using the
>>> host_device 

Re: [PATCH v8 3/7] block: add block layer APIs resembling Linux ZonedBlockDevice ioctls

[Markus Armbruster]

Sam Li  writes:

> Markus Armbruster  于2022年8月31日周三 16:35写道：
>>
>> Sam Li  writes:
>>
>> > Markus Armbruster  于2022年8月30日周二 19:57写道：
>> >>
>> >> Sam Li  writes:
>> >>
>> >> > By adding zone management operations in BlockDriver, storage controller
>> >> > emulation can use the new block layer APIs including Report Zone and
>> >> > four zone management operations (open, close, finish, reset).
>> >> >
>> >> > Add zoned storage commands of the device: zone_report(zrp), 
>> >> > zone_open(zo),
>> >> > zone_close(zc), zone_reset(zrs), zone_finish(zf).
>> >> >
>> >> > For example, to test zone_report, use following command:
>> >> > $ ./build/qemu-io --image-opts driver=zoned_host_device, 
>> >> > filename=/dev/nullb0
>> >> > -c "zrp offset nr_zones"
>> >> >
>> >> > Signed-off-by: Sam Li 
>> >> > Reviewed-by: Hannes Reinecke 
>> >>
>> >> [...]
>> >>
>> >> > diff --git a/block/file-posix.c b/block/file-posix.c
>> >> > index 0a8b4b426e..e3efba6db7 100644
>> >> > --- a/block/file-posix.c
>> >> > +++ b/block/file-posix.c
>> >>
>> >> [...]
>> >>
>> >> > @@ -3752,6 +4025,54 @@ static BlockDriver bdrv_host_device = {
>> >> >  #endif
>> >> >  };
>> >> >
>> >> > +#if defined(CONFIG_BLKZONED)
>> >> > +static BlockDriver bdrv_zoned_host_device = {
>> >> > +.format_name = "zoned_host_device",
>> >>
>> >> Indentation should be 4, not 8.
>> >>
>> >> > +.protocol_name = "zoned_host_device",
>> >> > +.instance_size = sizeof(BDRVRawState),
>> >> > +.bdrv_needs_filename = true,
>> >> > +.bdrv_probe_device  = hdev_probe_device,
>> >> > +.bdrv_file_open = hdev_open,
>> >> > +.bdrv_close = raw_close,
>> >> > +.bdrv_reopen_prepare = raw_reopen_prepare,
>> >> > +.bdrv_reopen_commit  = raw_reopen_commit,
>> >> > +.bdrv_reopen_abort   = raw_reopen_abort,
>> >> > +.bdrv_co_create_opts = bdrv_co_create_opts_simple,
>> >> > +.create_opts = _create_opts_simple,
>> >> > +.mutable_opts= mutable_opts,
>> >> > +.bdrv_co_invalidate_cache = raw_co_invalidate_cache,
>> >> > +.bdrv_co_pwrite_zeroes = hdev_co_pwrite_zeroes,
>> >> > +
>> >> > +.bdrv_co_preadv = raw_co_preadv,
>> >> > +.bdrv_co_pwritev= raw_co_pwritev,
>> >> > +.bdrv_co_flush_to_disk  = raw_co_flush_to_disk,
>> >> > +.bdrv_co_pdiscard   = hdev_co_pdiscard,
>> >> > +.bdrv_co_copy_range_from = raw_co_copy_range_from,
>> >> > +.bdrv_co_copy_range_to  = raw_co_copy_range_to,
>> >> > +.bdrv_refresh_limits = raw_refresh_limits,
>> >> > +.bdrv_io_plug = raw_aio_plug,
>> >> > +.bdrv_io_unplug = raw_aio_unplug,
>> >> > +.bdrv_attach_aio_context = raw_aio_attach_aio_context,
>> >> > +
>> >> > +.bdrv_co_truncate   = raw_co_truncate,
>> >> > +.bdrv_getlength = raw_getlength,
>> >> > +.bdrv_get_info = raw_get_info,
>> >> > +.bdrv_get_allocated_file_size
>> >> > += raw_get_allocated_file_size,
>> >> > +.bdrv_get_specific_stats = hdev_get_specific_stats,
>> >> > +.bdrv_check_perm = raw_check_perm,
>> >> > +.bdrv_set_perm   = raw_set_perm,
>> >> > +.bdrv_abort_perm_update = raw_abort_perm_update,
>> >> > +.bdrv_probe_blocksizes = hdev_probe_blocksizes,
>> >> > +.bdrv_probe_geometry = hdev_probe_geometry,
>> >> > +.bdrv_co_ioctl = hdev_co_ioctl,
>> >> > +
>> >> > +/* zone management operations */
>> >> > +.bdrv_co_zone_report = raw_co_zone_report,
>> >> > +.bdrv_co_zone_mgmt = raw_co_zone_mgmt,
>> >> > +};
>> >>
>> >> Differences to bdrv_host_device:
>> >>
>> >> * .bdrv_parse_filename is not set
>> >>
>> >> * .bdrv_co_ioctl is not set
>> >>
>> >> * .bdrv_co_zone_report and .bdrv_co_zone_mgmt are set
>> >
>> > As Stefan mentioned, zoned_host_device is a new driver that doesn't
>> > work with string filenames. .bdrv_parse_filename() helps legacy
>> > drivers strip the optional protocol prefix off the filename and no use
>> > here. Therefore it can be dropped.
>>
>> Makes sense.
>>
>> > .bdrv_co_ioctl is set actually.
>>
>> You're right; I diffed the two and misread the result.
>>
>> > Zoned_host_device is basically host_device + zone operations. It
>> > serves for a simple purpose: if the host device is zoned, register
>> > zoned_host_device driver; else, register host_device.
>>
>> Why would I ever want to use host_device instead of zoned_host_device?
>>
>> To answer this question, we need to understand how their behavior
>> differs.
>>
>> We can ignore the legacy protocol prefix / string filename part.
>>
>> All that's left seems to be "if the host device is zoned, then using the
>> zoned_host_device driver gets you the zoned features, whereas using the
>> host_device driver doesn't".  What am I missing?
>
> I think that's basically what users need to know about.

Now answer my previous question, please: why would I 

[PATCH 3/3] qemu-img: Speed up checksum

[Nir Soffer]

Add coroutine based loop inspired by `qemu-img convert` design.

Changes compared to `qemu-img convert`:

- State for the entire image is kept in ImgChecksumState

- State for single worker coroutine is kept in ImgChecksumworker.

- "Writes" are always in-order, ensured using a queue.

- Calling block status once per image extent, when the current extent is
  consumed by the workers.

- Using 1m buffer size - testings shows that this gives best read
  performance both with buffered and direct I/O.

- Number of coroutines is not configurable. Testing does not show
  improvement when using more than 8 coroutines.

- Progress include entire image, not only the allocated state.

Comparing to the simple read loop shows that this version is up to 4.67
times faster when computing a checksum for an image full of zeroes. For
real images it is 1.59 times faster with direct I/O, and with buffered
I/O there is no difference.

Test results on Dell PowerEdge R640 in a CentOS Stream 9 container:

| image| size | i/o   | before | after  | change |
|--|--|---||||
| zero [1] |   6g | buffered  | 1.600s ±0.014s | 0.342s ±0.016s |  x4.67 |
| zero |   6g | direct| 4.684s ±0.093s | 2.211s ±0.009s |  x2.12 |
| real [2] |   6g | buffered  | 1.841s ±0.075s | 1.806s ±0.036s |  x1.02 |
| real |   6g | direct| 3.094s ±0.079s | 1.947s ±0.017s |  x1.59 |
| nbd  [3] |   6g | buffered  | 2.455s ±0.183s | 1.808s ±0.016s |  x1.36 |
| nbd  |   6g | direct| 3.540s ±0.020s | 1.749s ±0.018s |  x2.02 |

[1] raw image full of zeroes
[2] raw fedora 35 image with additional random data, 50% full
[3] image [2] exported by qemu-nbd via unix socket

Signed-off-by: Nir Soffer 
---
 qemu-img.c | 343 +
 1 file changed, 270 insertions(+), 73 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 7edcfe4bc8..bfa8e2862f 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1613,48 +1613,288 @@ out:
 qemu_vfree(buf2);
 blk_unref(blk2);
 out2:
 blk_unref(blk1);
 out3:
 qemu_progress_end();
 return ret;
 }
 
 #ifdef CONFIG_BLKHASH
+
+#define CHECKSUM_COROUTINES 8
+#define CHECKSUM_BUF_SIZE (1 * MiB)
+#define CHECKSUM_ZERO_SIZE MIN(16 * GiB, SIZE_MAX)
+
+typedef struct ImgChecksumState ImgChecksumState;
+
+typedef struct ImgChecksumWorker {
+QTAILQ_ENTRY(ImgChecksumWorker) entry;
+ImgChecksumState *state;
+Coroutine *co;
+uint8_t *buf;
+
+/* The current chunk. */
+int64_t offset;
+int64_t length;
+bool zero;
+
+/* Always true for zero extent, false for data extent. Set to true
+ * when reading the chunk completes. */
+bool ready;
+} ImgChecksumWorker;
+
+struct ImgChecksumState {
+const char *filename;
+BlockBackend *blk;
+BlockDriverState *bs;
+int64_t total_size;
+
+/* Current extent, modified in checksum_co_next. */
+int64_t offset;
+int64_t length;
+bool zero;
+
+int running_coroutines;
+CoMutex lock;
+ImgChecksumWorker workers[CHECKSUM_COROUTINES];
+
+/* Ensure in-order updates. Update are scheduled at the tail of the
+ * queue and processed from the head of the queue when a worker is
+ * ready. */
+QTAILQ_HEAD(, ImgChecksumWorker) update_queue;
+
+struct blkhash *hash;
+int ret;
+};
+
+static int checksum_block_status(ImgChecksumState *s)
+{
+int64_t length;
+int status;
+
+/* Must be called when current extent is consumed. */
+assert(s->length == 0);
+
+status = bdrv_block_status_above(s->bs, NULL, s->offset,
+ s->total_size - s->offset, , NULL,
+ NULL);
+if (status < 0) {
+error_report("Error checking status at offset %" PRId64 " for %s",
+ s->offset, s->filename);
+s->ret = status;
+return -1;
+}
+
+assert(length > 0);
+
+s->length = length;
+s->zero = !!(status & BDRV_BLOCK_ZERO);
+
+return 0;
+}
+
+/**
+ * Grab the next chunk from the current extent, getting the next extent if
+ * needed, and schecule the next update at the end fo the update queue.
+ *
+ * Retrun true if the worker has work to do, false if the worker has
+ * finished or there was an error getting the next extent.
+ */
+static coroutine_fn bool checksum_co_next(ImgChecksumWorker *w)
+{
+ImgChecksumState *s = w->state;
+
+qemu_co_mutex_lock(>lock);
+
+if (s->offset == s->total_size || s->ret != -EINPROGRESS) {
+qemu_co_mutex_unlock(>lock);
+return false;
+}
+
+if (s->length == 0 && checksum_block_status(s)) {
+qemu_co_mutex_unlock(>lock);
+return false;
+}
+
+/* Grab one chunk from current extent. */
+w->offset = s->offset;
+w->length = MIN(s->length,
+s->zero ?  CHECKSUM_ZERO_SIZE : CHECKSUM_BUF_SIZE);
+w->zero = s->zero;
+w->ready = s->zero;
+
+/* 

[PATCH 2/3] iotests: Test qemu-img checksum

[Nir Soffer]

Add simple tests creating an image with all kinds of extents, different
formats, different backing chain, different protocol, and different
image options. Since all images have the same guest visible content they
must have the same checksum.

To help debugging in case of failures, the output includes a json map of
every test image.

Signed-off-by: Nir Soffer 
---
 tests/qemu-iotests/tests/qemu-img-checksum| 149 ++
 .../qemu-iotests/tests/qemu-img-checksum.out  |  74 +
 2 files changed, 223 insertions(+)
 create mode 100755 tests/qemu-iotests/tests/qemu-img-checksum
 create mode 100644 tests/qemu-iotests/tests/qemu-img-checksum.out

diff --git a/tests/qemu-iotests/tests/qemu-img-checksum 
b/tests/qemu-iotests/tests/qemu-img-checksum
new file mode 100755
index 00..3a85ba33f2
--- /dev/null
+++ b/tests/qemu-iotests/tests/qemu-img-checksum
@@ -0,0 +1,149 @@
+#!/usr/bin/env python3
+# group: rw auto quick
+#
+# Test cases for qemu-img checksum.
+#
+# Copyright (C) 2022 Red Hat, Inc.
+#
+# 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 .
+
+import re
+
+import iotests
+
+from iotests import (
+filter_testfiles,
+qemu_img,
+qemu_img_log,
+qemu_io,
+qemu_nbd_popen,
+)
+
+
+def checksum_available():
+out = qemu_img("--help").stdout
+return re.search(r"\bchecksum .+ filename\b", out) is not None
+
+
+if not checksum_available():
+iotests.notrun("checksum command not available")
+
+iotests.script_initialize(
+supported_fmts=["raw", "qcow2"],
+supported_cache_modes=["none", "writeback"],
+supported_protocols=["file", "nbd"],
+required_fmts=["raw", "qcow2"],
+)
+
+print()
+print("=== Test images ===")
+print()
+
+disk_raw = iotests.file_path('raw')
+qemu_img("create", "-f", "raw", disk_raw, "10m")
+qemu_io("-f", "raw",
+"-c", "write -P 0x1 0 2m",  # data
+"-c", "write -P 0x0 2m 2m", # data with zeroes
+"-c", "write -z 4m 2m", # zero allocated
+"-c", "write -z -u 6m 2m",  # zero hole
+# unallocated
+disk_raw)
+print(filter_testfiles(disk_raw))
+qemu_img_log("map", "--output", "json", disk_raw)
+
+disk_qcow2 = iotests.file_path('qcow2')
+qemu_img("create", "-f", "qcow2", disk_qcow2, "10m")
+qemu_io("-f", "qcow2",
+"-c", "write -P 0x1 0 2m",  # data
+"-c", "write -P 0x0 2m 2m", # data with zeroes
+"-c", "write -z 4m 2m", # zero allocated
+"-c", "write -z -u 6m 2m",  # zero hole
+# unallocated
+disk_qcow2)
+print(filter_testfiles(disk_qcow2))
+qemu_img_log("map", "--output", "json", disk_qcow2)
+
+disk_compressed = iotests.file_path('compressed')
+qemu_img("convert", "-f", "qcow2", "-O", "qcow2", "-c",
+ disk_qcow2, disk_compressed)
+print(filter_testfiles(disk_compressed))
+qemu_img_log("map", "--output", "json", disk_compressed)
+
+disk_base = iotests.file_path('base')
+qemu_img("create", "-f", "raw", disk_base, "10m")
+qemu_io("-f", "raw",
+"-c", "write -P 0x1 0 2m",
+"-c", "write -P 0x0 2m 2m",
+disk_base)
+print(filter_testfiles(disk_base))
+qemu_img_log("map", "--output", "json", disk_base)
+
+disk_top = iotests.file_path('top')
+qemu_img("create", "-f", "qcow2", "-b", disk_base, "-F", "raw",
+ disk_top)
+qemu_io("-f", "qcow2",
+"-c", "write -z 4m 2m",
+"-c", "write -z -u 6m 2m",
+disk_top)
+print(filter_testfiles(disk_top))
+qemu_img_log("map", "--output", "json", disk_top)
+
+print()
+print("=== Checksums - file ===")
+print()
+
+qemu_img_log("checksum", disk_raw)
+qemu_img_log("checksum", disk_qcow2)
+qemu_img_log("checksum", disk_compressed)
+qemu_img_log("checksum", disk_top)
+qemu_img_log("checksum", disk_base)
+
+print()
+print("=== Checksums - nbd ===")
+print()
+
+nbd_sock = iotests.file_path("nbd.sock", base_dir=iotests.sock_dir)
+nbd_uri = f"nbd+unix:///{{}}?socket={nbd_sock}"
+
+with qemu_nbd_popen("-k", nbd_sock, "-f", "raw", "-x", "raw", disk_raw):
+qemu_img_log("checksum", nbd_uri.format("raw"))
+
+with qemu_nbd_popen("-k", nbd_sock, "-f", "qcow2", "-x", "qcow2", disk_qcow2):
+qemu_img_log("checksum", nbd_uri.format("qcow2"))
+
+with qemu_nbd_popen("-k", nbd_sock, "-f", "qcow2", "-x", "compressed", 
disk_compressed):
+qemu_img_log("checksum", 

[PATCH 1/3] qemu-img: Add checksum command

[Nir Soffer]

The checksum command compute a checksum for disk image content using the
blkhash library[1]. The blkhash library is not packaged yet, but it is
available via copr[2].

Example run:

$ ./qemu-img checksum -p fedora-35.qcow2
6e5c00c995056319d52395f8d91c7f84725ae3da69ffcba4de4c7d22cff713a5  
fedora-35.qcow2

The block checksum is constructed by splitting the image to fixed sized
blocks and computing a digest of every block. The image checksum is the
digest of the all block digests.

The checksum uses internally the "sha256" algorithm but it cannot be
compared with checksums created by other tools such as `sha256sum`.

The blkhash library supports sparse images, zero detection, and
optimizes zero block hashing (they are practically free). The library
uses multiple threads to speed up the computation.

Comparing to `sha256sum`, `qemu-img checksum` is 3.5-4800[3] times
faster, depending on the amount of data in the image:

$ ./qemu-img info /scratch/50p.raw
file format: raw
virtual size: 6 GiB (6442450944 bytes)
disk size: 2.91 GiB

$ hyperfine -w2 -r5 -p "sleep 1" "./qemu-img checksum /scratch/50p.raw" \
 "sha256sum /scratch/50p.raw"
Benchmark 1: ./qemu-img checksum /scratch/50p.raw
  Time (mean ± σ):  1.849 s ±  0.037 s[User: 7.764 s, System: 0.962 
s]
  Range (min … max):1.813 s …  1.908 s5 runs

Benchmark 2: sha256sum /scratch/50p.raw
  Time (mean ± σ): 14.585 s ±  0.072 s[User: 13.537 s, System: 
1.003 s]
  Range (min … max):   14.501 s … 14.697 s5 runs

Summary
  './qemu-img checksum /scratch/50p.raw' ran
7.89 ± 0.16 times faster than 'sha256sum /scratch/50p.raw'

The new command is available only when `blkhash` is available during
build. To test the new command please install the `blkhash-devel`
package:

$ dnf copr enable nsoffer/blkhash
$ sudo dnf install blkhash-devel

[1] https://gitlab.com/nirs/blkhash
[2] https://copr.fedorainfracloud.org/coprs/nsoffer/blkhash/
[3] Computing checksum for 8T empty image: qemu-img checksum: 3.7s,
sha256sum (estimate): 17,749s

Signed-off-by: Nir Soffer 
---
 docs/tools/qemu-img.rst |  22 +
 meson.build |  10 ++-
 meson_options.txt   |   2 +
 qemu-img-cmds.hx|   8 ++
 qemu-img.c  | 191 
 5 files changed, 232 insertions(+), 1 deletion(-)

diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
index 85a6e05b35..8be9c45cbf 100644
--- a/docs/tools/qemu-img.rst
+++ b/docs/tools/qemu-img.rst
@@ -347,20 +347,42 @@ Command description:
 Check completed, image is corrupted
   3
 Check completed, image has leaked clusters, but is not corrupted
   63
 Checks are not supported by the image format
 
   If ``-r`` is specified, exit codes representing the image state refer to the
   state after (the attempt at) repairing it. That is, a successful ``-r all``
   will yield the exit code 0, independently of the image state before.
 
+.. option:: checksum [--object OBJECTDEF] [--image-opts] [-f FMT] [-T 
SRC_CACHE] [-p] FILENAME
+
+  Print a checksum for image *FILENAME* guest visible content. Images with
+  different format or settings wil have the same checksum.
+
+  The format is probed unless you specify it by ``-f``.
+
+  The checksum is computed for guest visible content. Allocated areas full of
+  zeroes, zero clusters, and unallocated areas are read as zeros so they will
+  have the same checksum. Images with single or multiple files or backing files
+  will have the same checksums if the guest will see the same content when
+  reading the image.
+
+  Image metadata that is not visible to the guest such as dirty bitmaps does
+  not affect the checksum.
+
+  Computing a checksum requires a read-only image. You cannot compute a
+  checksum of an active image used by a guest, but you can compute a checksum
+  of a guest during pull mode incremental backup using NBD URL.
+
+  The checksum is not compatible with other tools such as *sha256sum*.
+
 .. option:: commit [--object OBJECTDEF] [--image-opts] [-q] [-f FMT] [-t 
CACHE] [-b BASE] [-r RATE_LIMIT] [-d] [-p] FILENAME
 
   Commit the changes recorded in *FILENAME* in its base image or backing file.
   If the backing file is smaller than the snapshot, then the backing file will 
be
   resized to be the same size as the snapshot.  If the snapshot is smaller than
   the backing file, the backing file will not be truncated.  If you want the
   backing file to match the size of the smaller snapshot, you can safely 
truncate
   it yourself once the commit operation successfully completes.
 
   The image *FILENAME* is emptied after the operation has succeeded. If you do
diff --git a/meson.build b/meson.build
index 20fddbd707..56b648d8a7 100644
--- a/meson.build
+++ b/meson.build
@@ -727,20 +727,24 @@ if not get_option('curl').auto() or have_block
 kwargs: static_kwargs)
 endif
 

[PATCH 0/3] Add qemu-img checksum command using blkhash

[Nir Soffer]

Since blkhash is available only via copr now, the new command is added as
optional feature, built only if blkhash-devel package is installed.

Nir Soffer (3):
  qemu-img: Add checksum command
  iotests: Test qemu-img checksum
  qemu-img: Speed up checksum

 docs/tools/qemu-img.rst   |  22 +
 meson.build   |  10 +-
 meson_options.txt |   2 +
 qemu-img-cmds.hx  |   8 +
 qemu-img.c| 388 ++
 tests/qemu-iotests/tests/qemu-img-checksum| 149 +++
 .../qemu-iotests/tests/qemu-img-checksum.out  |  74 
 7 files changed, 652 insertions(+), 1 deletion(-)
 create mode 100755 tests/qemu-iotests/tests/qemu-img-checksum
 create mode 100644 tests/qemu-iotests/tests/qemu-img-checksum.out

-- 
2.37.2

Re: [PATCH v5 00/13] Instantiate VT82xx functions in host device

[Bernhard Beschow]

On Thu, Sep 1, 2022 at 1:41 PM Bernhard Beschow  wrote:

> v5:
> * Add patch "Inline vt82c686b_southbridge_init() and remove it" (Zoltan)
> * Use machine parameter when creating rtc-time alias (Zoltan)
>
> Testing done: Same as in v3.
>
> v4:
> * Fix in comment: AC97 Modem -> MC97 Modem (Zoltan)
> * Introduce TYPE_VT82C686B_USB_UHCI define (Zoltan)
> * Introduce TYPE_VIA_IDE define (for consistency)
>
> v3:
> * Replace pre increment by post increment in for loop (Zoltan)
> * Move class defines close to where the class is defined (Zoltan)
>
> Testing done:
> * `make check-avocado`
>   Passes for boot_linux_console.py for mips64el_fuloong2e
> * `qemu-system-ppc -machine pegasos2 -rtc base=localtime -device
> ati-vga,guest_hwcursor=true,romfile="" -cdrom morphos-3.17.iso -kernel
> morphos-3.17/boot.img`
>   Boots successfully and it is possible to open games and tools.
>
> v2:
> * Keep the call to pci_ide_create_devs() in board code for consistency
> (Zoltan)
> * Create rtc-time alias in board rather than in south bridge code
> * Remove stale comments about PCI functions (Zoltan)
>
> v1:
> This series instantiates all PCI functions of the VT82xx south bridges in
> the south bridges themselves.
> For the IDE function this is especially important since its interrupt
> routing is configured in the
> ISA function, hence doesn't make sense to instantiate it as a
> "Frankenstein" device. The interrupt
> routing is currently hardcoded and changing that is currently not in the
> scope of this series.
>
> Testing done:
> * `qemu-system-ppc -machine pegasos2 -rtc base=localtime -device
> ati-vga,guest_hwcursor=true,romfile="" -cdrom morphos-3.17.iso -kernel
> morphos-3.17/boot.img`
>   Boots successfully and it is possible to open games and tools.
>
> * I was unable to test the fuloong2e board even before this series since
> it seems to be unfinished [1].
>   A buildroot-baked kernel [2] booted but doesn't find its root partition,
> though the issues could be in the buildroot receipt I created.
>
> [1] https://osdn.net/projects/qmiga/wiki/SubprojectPegasos2
> [2] https://github.com/shentok/buildroot/commits/fuloong2e


Forgot to mention: All patches except  "hw/mips/fuloong2e: Inline
vt82c686b_southbridge_init() and remove it" are:

Reviewed-by: BALATON Zoltan 

Best regards,
Bernhard

>
> Bernhard Beschow (13):
>   hw/isa/vt82c686: Resolve chip-specific realize methods
>   hw/isa/vt82c686: Resolve unneeded attribute
>   hw/isa/vt82c686: Prefer pci_address_space() over get_system_memory()
>   hw/isa/vt82c686: Reuse errp
>   hw/isa/vt82c686: Introduce TYPE_VIA_IDE define
>   hw/isa/vt82c686: Instantiate IDE function in host device
>   hw/isa/vt82c686: Introduce TYPE_VT82C686B_USB_UHCI define
>   hw/isa/vt82c686: Instantiate USB functions in host device
>   hw/isa/vt82c686: Instantiate PM function in host device
>   hw/isa/vt82c686: Instantiate AC97 and MC97 functions in host device
>   hw/mips/fuloong2e: Inline vt82c686b_southbridge_init() and remove it
>   hw/isa/vt82c686: Embed RTCState in host device
>   hw/isa/vt82c686: Create rtc-time alias in boards instead
>
>  configs/devices/mips64el-softmmu/default.mak |   1 -
>  hw/ide/via.c |   2 +-
>  hw/isa/Kconfig   |   1 +
>  hw/isa/vt82c686.c| 120 +++
>  hw/mips/fuloong2e.c  |  39 +++---
>  hw/ppc/Kconfig   |   1 -
>  hw/ppc/pegasos2.c|  25 ++--
>  hw/usb/vt82c686-uhci-pci.c   |   4 +-
>  include/hw/isa/vt82c686.h|   4 +-
>  9 files changed, 126 insertions(+), 71 deletions(-)
>
> --
> 2.37.3
>
>

[PATCH v5 10/13] hw/isa/vt82c686: Instantiate AC97 and MC97 functions in host device

[Bernhard Beschow]

The AC97 function's wakeup status is wired to the PM function and both
the AC97 and MC97 interrupt routing is determined by the ISA function.

Signed-off-by: Bernhard Beschow 
---
 hw/isa/vt82c686.c   | 16 
 hw/mips/fuloong2e.c |  4 
 hw/ppc/pegasos2.c   |  5 -
 3 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index d048607079..91686e9570 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -553,6 +553,8 @@ struct ViaISAState {
 PCIIDEState ide;
 UHCIState uhci[2];
 ViaPMState pm;
+PCIDevice ac97;
+PCIDevice mc97;
 };
 
 static const VMStateDescription vmstate_via = {
@@ -572,6 +574,8 @@ static void via_isa_init(Object *obj)
 object_initialize_child(obj, "ide", >ide, TYPE_VIA_IDE);
 object_initialize_child(obj, "uhci1", >uhci[0], 
TYPE_VT82C686B_USB_UHCI);
 object_initialize_child(obj, "uhci2", >uhci[1], 
TYPE_VT82C686B_USB_UHCI);
+object_initialize_child(obj, "ac97", >ac97, TYPE_VIA_AC97);
+object_initialize_child(obj, "mc97", >mc97, TYPE_VIA_MC97);
 }
 
 static const TypeInfo via_isa_info = {
@@ -652,6 +656,18 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
 if (!qdev_realize(DEVICE(>pm), BUS(pci_bus), errp)) {
 return;
 }
+
+/* Function 5: AC97 Audio */
+qdev_prop_set_int32(DEVICE(>ac97), "addr", d->devfn + 5);
+if (!qdev_realize(DEVICE(>ac97), BUS(pci_bus), errp)) {
+return;
+}
+
+/* Function 6: MC97 Modem */
+qdev_prop_set_int32(DEVICE(>mc97), "addr", d->devfn + 6);
+if (!qdev_realize(DEVICE(>mc97), BUS(pci_bus), errp)) {
+return;
+}
 }
 
 /* TYPE_VT82C686B_ISA */
diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
index 377108d313..2d8723ab74 100644
--- a/hw/mips/fuloong2e.c
+++ b/hw/mips/fuloong2e.c
@@ -210,10 +210,6 @@ static void vt82c686b_southbridge_init(PCIBus *pci_bus, 
int slot, qemu_irq intc,
 
 dev = PCI_DEVICE(object_resolve_path_component(OBJECT(via), "pm"));
 *i2c_bus = I2C_BUS(qdev_get_child_bus(DEVICE(dev), "i2c"));
-
-/* Audio support */
-pci_create_simple(pci_bus, PCI_DEVFN(slot, 5), TYPE_VIA_AC97);
-pci_create_simple(pci_bus, PCI_DEVFN(slot, 6), TYPE_VIA_MC97);
 }
 
 /* Network support */
diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
index e32944ee2b..09fdb7557f 100644
--- a/hw/ppc/pegasos2.c
+++ b/hw/ppc/pegasos2.c
@@ -159,7 +159,6 @@ static void pegasos2_init(MachineState *machine)
 pci_bus = mv64361_get_pci_bus(pm->mv, 1);
 
 /* VIA VT8231 South Bridge (multifunction PCI device) */
-/* VT8231 function 0: PCI-to-ISA Bridge */
 via = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(12, 0), true,
   TYPE_VT8231_ISA);
 qdev_connect_gpio_out(DEVICE(via), 0,
@@ -173,10 +172,6 @@ static void pegasos2_init(MachineState *machine)
 spd_data = spd_data_generate(DDR, machine->ram_size);
 smbus_eeprom_init_one(i2c_bus, 0x57, spd_data);
 
-/* VT8231 function 5-6: AC97 Audio & Modem */
-pci_create_simple(pci_bus, PCI_DEVFN(12, 5), TYPE_VIA_AC97);
-pci_create_simple(pci_bus, PCI_DEVFN(12, 6), TYPE_VIA_MC97);
-
 /* other PC hardware */
 pci_vga_init(pci_bus);
 
-- 
2.37.3

[PATCH v5 08/13] hw/isa/vt82c686: Instantiate USB functions in host device

[Bernhard Beschow]

The USB functions can be enabled/disabled through the ISA function. Also
its interrupt routing can be influenced there.

Signed-off-by: Bernhard Beschow 
---
 hw/isa/vt82c686.c   | 12 
 hw/mips/fuloong2e.c |  3 ---
 hw/ppc/pegasos2.c   |  4 
 3 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 63c1e3b8ce..f05fd9948a 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -23,6 +23,7 @@
 #include "hw/intc/i8259.h"
 #include "hw/irq.h"
 #include "hw/dma/i8257.h"
+#include "hw/usb/hcd-uhci.h"
 #include "hw/timer/i8254.h"
 #include "hw/rtc/mc146818rtc.h"
 #include "migration/vmstate.h"
@@ -546,6 +547,7 @@ struct ViaISAState {
 qemu_irq *isa_irqs;
 ViaSuperIOState via_sio;
 PCIIDEState ide;
+UHCIState uhci[2];
 };
 
 static const VMStateDescription vmstate_via = {
@@ -563,6 +565,8 @@ static void via_isa_init(Object *obj)
 ViaISAState *s = VIA_ISA(obj);
 
 object_initialize_child(obj, "ide", >ide, TYPE_VIA_IDE);
+object_initialize_child(obj, "uhci1", >uhci[0], 
TYPE_VT82C686B_USB_UHCI);
+object_initialize_child(obj, "uhci2", >uhci[1], 
TYPE_VT82C686B_USB_UHCI);
 }
 
 static const TypeInfo via_isa_info = {
@@ -629,6 +633,14 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
 if (!qdev_realize(DEVICE(>ide), BUS(pci_bus), errp)) {
 return;
 }
+
+/* Functions 2-3: USB Ports */
+for (i = 0; i < ARRAY_SIZE(s->uhci); i++) {
+qdev_prop_set_int32(DEVICE(>uhci[i]), "addr", d->devfn + 2 + i);
+if (!qdev_realize(DEVICE(>uhci[i]), BUS(pci_bus), errp)) {
+return;
+}
+}
 }
 
 /* TYPE_VT82C686B_ISA */
diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
index 6b7370f2aa..dc92223b76 100644
--- a/hw/mips/fuloong2e.c
+++ b/hw/mips/fuloong2e.c
@@ -208,9 +208,6 @@ static void vt82c686b_southbridge_init(PCIBus *pci_bus, int 
slot, qemu_irq intc,
 dev = PCI_DEVICE(object_resolve_path_component(OBJECT(via), "ide"));
 pci_ide_create_devs(dev);
 
-pci_create_simple(pci_bus, PCI_DEVFN(slot, 2), TYPE_VT82C686B_USB_UHCI);
-pci_create_simple(pci_bus, PCI_DEVFN(slot, 3), TYPE_VT82C686B_USB_UHCI);
-
 dev = pci_create_simple(pci_bus, PCI_DEVFN(slot, 4), TYPE_VT82C686B_PM);
 *i2c_bus = I2C_BUS(qdev_get_child_bus(DEVICE(dev), "i2c"));
 
diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
index 70776558c9..85cca6f7a6 100644
--- a/hw/ppc/pegasos2.c
+++ b/hw/ppc/pegasos2.c
@@ -168,10 +168,6 @@ static void pegasos2_init(MachineState *machine)
 dev = PCI_DEVICE(object_resolve_path_component(OBJECT(via), "ide"));
 pci_ide_create_devs(dev);
 
-/* VT8231 function 2-3: USB Ports */
-pci_create_simple(pci_bus, PCI_DEVFN(12, 2), TYPE_VT82C686B_USB_UHCI);
-pci_create_simple(pci_bus, PCI_DEVFN(12, 3), TYPE_VT82C686B_USB_UHCI);
-
 /* VT8231 function 4: Power Management Controller */
 dev = pci_create_simple(pci_bus, PCI_DEVFN(12, 4), TYPE_VT8231_PM);
 i2c_bus = I2C_BUS(qdev_get_child_bus(DEVICE(dev), "i2c"));
-- 
2.37.3

[PATCH v5 06/13] hw/isa/vt82c686: Instantiate IDE function in host device

[Bernhard Beschow]

The IDE function is closely tied to the ISA function (e.g. the IDE
interrupt routing happens there), so it makes sense that the IDE
function is instantiated within the south bridge itself.

Signed-off-by: Bernhard Beschow 
---
 configs/devices/mips64el-softmmu/default.mak |  1 -
 hw/isa/Kconfig   |  1 +
 hw/isa/vt82c686.c| 17 +
 hw/mips/fuloong2e.c  |  8 
 hw/ppc/Kconfig   |  1 -
 hw/ppc/pegasos2.c|  9 -
 6 files changed, 26 insertions(+), 11 deletions(-)

diff --git a/configs/devices/mips64el-softmmu/default.mak 
b/configs/devices/mips64el-softmmu/default.mak
index c610749ac1..d5188f7ea5 100644
--- a/configs/devices/mips64el-softmmu/default.mak
+++ b/configs/devices/mips64el-softmmu/default.mak
@@ -1,7 +1,6 @@
 # Default configuration for mips64el-softmmu
 
 include ../mips-softmmu/common.mak
-CONFIG_IDE_VIA=y
 CONFIG_FULOONG=y
 CONFIG_LOONGSON3V=y
 CONFIG_ATI_VGA=y
diff --git a/hw/isa/Kconfig b/hw/isa/Kconfig
index d42143a991..20de7e9294 100644
--- a/hw/isa/Kconfig
+++ b/hw/isa/Kconfig
@@ -53,6 +53,7 @@ config VT82C686
 select I8254
 select I8257
 select I8259
+select IDE_VIA
 select MC146818RTC
 select PARALLEL
 
diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 37e37b3855..63c1e3b8ce 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -17,6 +17,7 @@
 #include "hw/isa/vt82c686.h"
 #include "hw/pci/pci.h"
 #include "hw/qdev-properties.h"
+#include "hw/ide/pci.h"
 #include "hw/isa/isa.h"
 #include "hw/isa/superio.h"
 #include "hw/intc/i8259.h"
@@ -544,6 +545,7 @@ struct ViaISAState {
 qemu_irq cpu_intr;
 qemu_irq *isa_irqs;
 ViaSuperIOState via_sio;
+PCIIDEState ide;
 };
 
 static const VMStateDescription vmstate_via = {
@@ -556,10 +558,18 @@ static const VMStateDescription vmstate_via = {
 }
 };
 
+static void via_isa_init(Object *obj)
+{
+ViaISAState *s = VIA_ISA(obj);
+
+object_initialize_child(obj, "ide", >ide, TYPE_VIA_IDE);
+}
+
 static const TypeInfo via_isa_info = {
 .name  = TYPE_VIA_ISA,
 .parent= TYPE_PCI_DEVICE,
 .instance_size = sizeof(ViaISAState),
+.instance_init = via_isa_init,
 .abstract  = true,
 .interfaces= (InterfaceInfo[]) {
 { INTERFACE_CONVENTIONAL_PCI_DEVICE },
@@ -583,6 +593,7 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
 {
 ViaISAState *s = VIA_ISA(d);
 DeviceState *dev = DEVICE(d);
+PCIBus *pci_bus = pci_get_bus(d);
 qemu_irq *isa_irq;
 ISABus *isa_bus;
 int i;
@@ -612,6 +623,12 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
 if (!qdev_realize(DEVICE(>via_sio), BUS(isa_bus), errp)) {
 return;
 }
+
+/* Function 1: IDE */
+qdev_prop_set_int32(DEVICE(>ide), "addr", d->devfn + 1);
+if (!qdev_realize(DEVICE(>ide), BUS(pci_bus), errp)) {
+return;
+}
 }
 
 /* TYPE_VT82C686B_ISA */
diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
index 44225fbe33..32605901e7 100644
--- a/hw/mips/fuloong2e.c
+++ b/hw/mips/fuloong2e.c
@@ -199,13 +199,13 @@ static void main_cpu_reset(void *opaque)
 static void vt82c686b_southbridge_init(PCIBus *pci_bus, int slot, qemu_irq 
intc,
I2CBus **i2c_bus)
 {
-PCIDevice *dev;
+PCIDevice *dev, *via;
 
-dev = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(slot, 0), true,
+via = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(slot, 0), true,
   TYPE_VT82C686B_ISA);
-qdev_connect_gpio_out(DEVICE(dev), 0, intc);
+qdev_connect_gpio_out(DEVICE(via), 0, intc);
 
-dev = pci_create_simple(pci_bus, PCI_DEVFN(slot, 1), TYPE_VIA_IDE);
+dev = PCI_DEVICE(object_resolve_path_component(OBJECT(via), "ide"));
 pci_ide_create_devs(dev);
 
 pci_create_simple(pci_bus, PCI_DEVFN(slot, 2), "vt82c686b-usb-uhci");
diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig
index 400511c6b7..18565e966b 100644
--- a/hw/ppc/Kconfig
+++ b/hw/ppc/Kconfig
@@ -74,7 +74,6 @@ config PEGASOS2
 bool
 select MV64361
 select VT82C686
-select IDE_VIA
 select SMBUS_EEPROM
 select VOF
 # This should come with VT82C686
diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
index 8039775f80..8bc528a560 100644
--- a/hw/ppc/pegasos2.c
+++ b/hw/ppc/pegasos2.c
@@ -102,7 +102,7 @@ static void pegasos2_init(MachineState *machine)
 CPUPPCState *env;
 MemoryRegion *rom = g_new(MemoryRegion, 1);
 PCIBus *pci_bus;
-PCIDevice *dev;
+PCIDevice *dev, *via;
 I2CBus *i2c_bus;
 const char *fwname = machine->firmware ?: PROM_FILENAME;
 char *filename;
@@ -160,13 +160,12 @@ static void pegasos2_init(MachineState *machine)
 
 /* VIA VT8231 South Bridge (multifunction PCI device) */
 /* VT8231 function 0: PCI-to-ISA Bridge */
-dev = pci_create_simple_multifunction(pci_bus, 

[PATCH v5 12/13] hw/isa/vt82c686: Embed RTCState in host device

[Bernhard Beschow]

Embed the rtc in the host device, analoguous to the other child devices
and analoguous to PIIX4.

Signed-off-by: Bernhard Beschow 
---
 hw/isa/vt82c686.c | 12 +++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 91686e9570..48cd4d0036 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -550,6 +550,7 @@ struct ViaISAState {
 qemu_irq cpu_intr;
 qemu_irq *isa_irqs;
 ViaSuperIOState via_sio;
+RTCState rtc;
 PCIIDEState ide;
 UHCIState uhci[2];
 ViaPMState pm;
@@ -571,6 +572,7 @@ static void via_isa_init(Object *obj)
 {
 ViaISAState *s = VIA_ISA(obj);
 
+object_initialize_child(obj, "rtc", >rtc, TYPE_MC146818_RTC);
 object_initialize_child(obj, "ide", >ide, TYPE_VIA_IDE);
 object_initialize_child(obj, "uhci1", >uhci[0], 
TYPE_VT82C686B_USB_UHCI);
 object_initialize_child(obj, "uhci2", >uhci[1], 
TYPE_VT82C686B_USB_UHCI);
@@ -624,7 +626,15 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
 isa_bus_irqs(isa_bus, s->isa_irqs);
 i8254_pit_init(isa_bus, 0x40, 0, NULL);
 i8257_dma_init(isa_bus, 0);
-mc146818_rtc_init(isa_bus, 2000, NULL);
+
+/* RTC */
+qdev_prop_set_int32(DEVICE(>rtc), "base_year", 2000);
+if (!qdev_realize(DEVICE(>rtc), BUS(isa_bus), errp)) {
+return;
+}
+object_property_add_alias(qdev_get_machine(), "rtc-time", OBJECT(>rtc),
+  "date");
+isa_connect_gpio_out(ISA_DEVICE(>rtc), 0, s->rtc.isairq);
 
 for (i = 0; i < PCI_CONFIG_HEADER_SIZE; i++) {
 if (i < PCI_COMMAND || i >= PCI_REVISION_ID) {
-- 
2.37.3

[PATCH v5 01/13] hw/isa/vt82c686: Resolve chip-specific realize methods

[Bernhard Beschow]

The object creation now happens in chip-specific init methods which
allows the realize methods to be consolidated into one method. Shifting
the logic into the init methods has the addidional advantage that the
parent object's init methods are called implicitly - like constructors
in object-oriented languages.

Signed-off-by: Bernhard Beschow 
---
 hw/isa/vt82c686.c | 33 ++---
 1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 8f656251b8..0217c98fe4 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -544,7 +544,7 @@ struct ViaISAState {
 qemu_irq cpu_intr;
 qemu_irq *isa_irqs;
 ISABus *isa_bus;
-ViaSuperIOState *via_sio;
+ViaSuperIOState via_sio;
 };
 
 static const VMStateDescription vmstate_via = {
@@ -602,6 +602,11 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
 d->wmask[i] = 0;
 }
 }
+
+/* Super I/O */
+if (!qdev_realize(DEVICE(>via_sio), BUS(s->isa_bus), errp)) {
+return;
+}
 }
 
 /* TYPE_VT82C686B_ISA */
@@ -615,7 +620,7 @@ static void vt82c686b_write_config(PCIDevice *d, uint32_t 
addr,
 pci_default_write_config(d, addr, val, len);
 if (addr == 0x85) {
 /* BIT(1): enable or disable superio config io ports */
-via_superio_io_enable(s->via_sio, val & BIT(1));
+via_superio_io_enable(>via_sio, val & BIT(1));
 }
 }
 
@@ -639,13 +644,11 @@ static void vt82c686b_isa_reset(DeviceState *dev)
 pci_conf[0x77] = 0x10; /* GPIO Control 1/2/3/4 */
 }
 
-static void vt82c686b_realize(PCIDevice *d, Error **errp)
+static void vt82c686b_init(Object *obj)
 {
-ViaISAState *s = VIA_ISA(d);
+ViaISAState *s = VIA_ISA(obj);
 
-via_isa_realize(d, errp);
-s->via_sio = VIA_SUPERIO(isa_create_simple(s->isa_bus,
-   TYPE_VT82C686B_SUPERIO));
+object_initialize_child(obj, "sio", >via_sio, TYPE_VT82C686B_SUPERIO);
 }
 
 static void vt82c686b_class_init(ObjectClass *klass, void *data)
@@ -653,7 +656,7 @@ static void vt82c686b_class_init(ObjectClass *klass, void 
*data)
 DeviceClass *dc = DEVICE_CLASS(klass);
 PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
-k->realize = vt82c686b_realize;
+k->realize = via_isa_realize;
 k->config_write = vt82c686b_write_config;
 k->vendor_id = PCI_VENDOR_ID_VIA;
 k->device_id = PCI_DEVICE_ID_VIA_82C686B_ISA;
@@ -670,6 +673,7 @@ static const TypeInfo vt82c686b_isa_info = {
 .name  = TYPE_VT82C686B_ISA,
 .parent= TYPE_VIA_ISA,
 .instance_size = sizeof(ViaISAState),
+.instance_init = vt82c686b_init,
 .class_init= vt82c686b_class_init,
 };
 
@@ -684,7 +688,7 @@ static void vt8231_write_config(PCIDevice *d, uint32_t addr,
 pci_default_write_config(d, addr, val, len);
 if (addr == 0x50) {
 /* BIT(2): enable or disable superio config io ports */
-via_superio_io_enable(s->via_sio, val & BIT(2));
+via_superio_io_enable(>via_sio, val & BIT(2));
 }
 }
 
@@ -703,13 +707,11 @@ static void vt8231_isa_reset(DeviceState *dev)
 pci_conf[0x6b] = 0x01; /* Fast IR I/O Base */
 }
 
-static void vt8231_realize(PCIDevice *d, Error **errp)
+static void vt8231_init(Object *obj)
 {
-ViaISAState *s = VIA_ISA(d);
+ViaISAState *s = VIA_ISA(obj);
 
-via_isa_realize(d, errp);
-s->via_sio = VIA_SUPERIO(isa_create_simple(s->isa_bus,
-   TYPE_VT8231_SUPERIO));
+object_initialize_child(obj, "sio", >via_sio, TYPE_VT8231_SUPERIO);
 }
 
 static void vt8231_class_init(ObjectClass *klass, void *data)
@@ -717,7 +719,7 @@ static void vt8231_class_init(ObjectClass *klass, void 
*data)
 DeviceClass *dc = DEVICE_CLASS(klass);
 PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
-k->realize = vt8231_realize;
+k->realize = via_isa_realize;
 k->config_write = vt8231_write_config;
 k->vendor_id = PCI_VENDOR_ID_VIA;
 k->device_id = PCI_DEVICE_ID_VIA_8231_ISA;
@@ -734,6 +736,7 @@ static const TypeInfo vt8231_isa_info = {
 .name  = TYPE_VT8231_ISA,
 .parent= TYPE_VIA_ISA,
 .instance_size = sizeof(ViaISAState),
+.instance_init = vt8231_init,
 .class_init= vt8231_class_init,
 };
 
-- 
2.37.3

[PATCH v5 00/13] Instantiate VT82xx functions in host device

[Bernhard Beschow]

v5:
* Add patch "Inline vt82c686b_southbridge_init() and remove it" (Zoltan)
* Use machine parameter when creating rtc-time alias (Zoltan)

Testing done: Same as in v3.

v4:
* Fix in comment: AC97 Modem -> MC97 Modem (Zoltan)
* Introduce TYPE_VT82C686B_USB_UHCI define (Zoltan)
* Introduce TYPE_VIA_IDE define (for consistency)

v3:
* Replace pre increment by post increment in for loop (Zoltan)
* Move class defines close to where the class is defined (Zoltan)

Testing done:
* `make check-avocado`
  Passes for boot_linux_console.py for mips64el_fuloong2e
* `qemu-system-ppc -machine pegasos2 -rtc base=localtime -device 
ati-vga,guest_hwcursor=true,romfile="" -cdrom morphos-3.17.iso -kernel 
morphos-3.17/boot.img`
  Boots successfully and it is possible to open games and tools.

v2:
* Keep the call to pci_ide_create_devs() in board code for consistency (Zoltan)
* Create rtc-time alias in board rather than in south bridge code
* Remove stale comments about PCI functions (Zoltan)

v1:
This series instantiates all PCI functions of the VT82xx south bridges in the 
south bridges themselves.
For the IDE function this is especially important since its interrupt routing 
is configured in the
ISA function, hence doesn't make sense to instantiate it as a "Frankenstein" 
device. The interrupt
routing is currently hardcoded and changing that is currently not in the scope 
of this series.

Testing done:
* `qemu-system-ppc -machine pegasos2 -rtc base=localtime -device 
ati-vga,guest_hwcursor=true,romfile="" -cdrom morphos-3.17.iso -kernel 
morphos-3.17/boot.img`
  Boots successfully and it is possible to open games and tools.

* I was unable to test the fuloong2e board even before this series since it 
seems to be unfinished [1].
  A buildroot-baked kernel [2] booted but doesn't find its root partition, 
though the issues could be in the buildroot receipt I created.

[1] https://osdn.net/projects/qmiga/wiki/SubprojectPegasos2
[2] https://github.com/shentok/buildroot/commits/fuloong2e

Bernhard Beschow (13):
  hw/isa/vt82c686: Resolve chip-specific realize methods
  hw/isa/vt82c686: Resolve unneeded attribute
  hw/isa/vt82c686: Prefer pci_address_space() over get_system_memory()
  hw/isa/vt82c686: Reuse errp
  hw/isa/vt82c686: Introduce TYPE_VIA_IDE define
  hw/isa/vt82c686: Instantiate IDE function in host device
  hw/isa/vt82c686: Introduce TYPE_VT82C686B_USB_UHCI define
  hw/isa/vt82c686: Instantiate USB functions in host device
  hw/isa/vt82c686: Instantiate PM function in host device
  hw/isa/vt82c686: Instantiate AC97 and MC97 functions in host device
  hw/mips/fuloong2e: Inline vt82c686b_southbridge_init() and remove it
  hw/isa/vt82c686: Embed RTCState in host device
  hw/isa/vt82c686: Create rtc-time alias in boards instead

 configs/devices/mips64el-softmmu/default.mak |   1 -
 hw/ide/via.c |   2 +-
 hw/isa/Kconfig   |   1 +
 hw/isa/vt82c686.c| 120 +++
 hw/mips/fuloong2e.c  |  39 +++---
 hw/ppc/Kconfig   |   1 -
 hw/ppc/pegasos2.c|  25 ++--
 hw/usb/vt82c686-uhci-pci.c   |   4 +-
 include/hw/isa/vt82c686.h|   4 +-
 9 files changed, 126 insertions(+), 71 deletions(-)

-- 
2.37.3

[PATCH v5 09/13] hw/isa/vt82c686: Instantiate PM function in host device

[Bernhard Beschow]

The PM controller has activity bits which monitor activity of other
built-in devices in the host device.

Signed-off-by: Bernhard Beschow 
---
 hw/isa/vt82c686.c | 13 +
 hw/mips/fuloong2e.c   |  2 +-
 hw/ppc/pegasos2.c |  3 +--
 include/hw/isa/vt82c686.h |  2 --
 4 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index f05fd9948a..d048607079 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -250,6 +250,8 @@ static const ViaPMInitInfo vt82c686b_pm_init_info = {
 .device_id = PCI_DEVICE_ID_VIA_82C686B_PM,
 };
 
+#define TYPE_VT82C686B_PM "vt82c686b-pm"
+
 static const TypeInfo vt82c686b_pm_info = {
 .name  = TYPE_VT82C686B_PM,
 .parent= TYPE_VIA_PM,
@@ -261,6 +263,8 @@ static const ViaPMInitInfo vt8231_pm_init_info = {
 .device_id = PCI_DEVICE_ID_VIA_8231_PM,
 };
 
+#define TYPE_VT8231_PM "vt8231-pm"
+
 static const TypeInfo vt8231_pm_info = {
 .name  = TYPE_VT8231_PM,
 .parent= TYPE_VIA_PM,
@@ -548,6 +552,7 @@ struct ViaISAState {
 ViaSuperIOState via_sio;
 PCIIDEState ide;
 UHCIState uhci[2];
+ViaPMState pm;
 };
 
 static const VMStateDescription vmstate_via = {
@@ -641,6 +646,12 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
 return;
 }
 }
+
+/* Function 4: Power Management */
+qdev_prop_set_int32(DEVICE(>pm), "addr", d->devfn + 4);
+if (!qdev_realize(DEVICE(>pm), BUS(pci_bus), errp)) {
+return;
+}
 }
 
 /* TYPE_VT82C686B_ISA */
@@ -683,6 +694,7 @@ static void vt82c686b_init(Object *obj)
 ViaISAState *s = VIA_ISA(obj);
 
 object_initialize_child(obj, "sio", >via_sio, TYPE_VT82C686B_SUPERIO);
+object_initialize_child(obj, "pm", >pm, TYPE_VT82C686B_PM);
 }
 
 static void vt82c686b_class_init(ObjectClass *klass, void *data)
@@ -746,6 +758,7 @@ static void vt8231_init(Object *obj)
 ViaISAState *s = VIA_ISA(obj);
 
 object_initialize_child(obj, "sio", >via_sio, TYPE_VT8231_SUPERIO);
+object_initialize_child(obj, "pm", >pm, TYPE_VT8231_PM);
 }
 
 static void vt8231_class_init(ObjectClass *klass, void *data)
diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
index dc92223b76..377108d313 100644
--- a/hw/mips/fuloong2e.c
+++ b/hw/mips/fuloong2e.c
@@ -208,7 +208,7 @@ static void vt82c686b_southbridge_init(PCIBus *pci_bus, int 
slot, qemu_irq intc,
 dev = PCI_DEVICE(object_resolve_path_component(OBJECT(via), "ide"));
 pci_ide_create_devs(dev);
 
-dev = pci_create_simple(pci_bus, PCI_DEVFN(slot, 4), TYPE_VT82C686B_PM);
+dev = PCI_DEVICE(object_resolve_path_component(OBJECT(via), "pm"));
 *i2c_bus = I2C_BUS(qdev_get_child_bus(DEVICE(dev), "i2c"));
 
 /* Audio support */
diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
index 85cca6f7a6..e32944ee2b 100644
--- a/hw/ppc/pegasos2.c
+++ b/hw/ppc/pegasos2.c
@@ -168,8 +168,7 @@ static void pegasos2_init(MachineState *machine)
 dev = PCI_DEVICE(object_resolve_path_component(OBJECT(via), "ide"));
 pci_ide_create_devs(dev);
 
-/* VT8231 function 4: Power Management Controller */
-dev = pci_create_simple(pci_bus, PCI_DEVFN(12, 4), TYPE_VT8231_PM);
+dev = PCI_DEVICE(object_resolve_path_component(OBJECT(via), "pm"));
 i2c_bus = I2C_BUS(qdev_get_child_bus(DEVICE(dev), "i2c"));
 spd_data = spd_data_generate(DDR, machine->ram_size);
 smbus_eeprom_init_one(i2c_bus, 0x57, spd_data);
diff --git a/include/hw/isa/vt82c686.h b/include/hw/isa/vt82c686.h
index e6f6dd4d43..eaa07881c5 100644
--- a/include/hw/isa/vt82c686.h
+++ b/include/hw/isa/vt82c686.h
@@ -4,10 +4,8 @@
 #include "hw/pci/pci.h"
 
 #define TYPE_VT82C686B_ISA "vt82c686b-isa"
-#define TYPE_VT82C686B_PM "vt82c686b-pm"
 #define TYPE_VT82C686B_USB_UHCI "vt82c686b-usb-uhci"
 #define TYPE_VT8231_ISA "vt8231-isa"
-#define TYPE_VT8231_PM "vt8231-pm"
 #define TYPE_VIA_AC97 "via-ac97"
 #define TYPE_VIA_IDE "via-ide"
 #define TYPE_VIA_MC97 "via-mc97"
-- 
2.37.3

[PATCH v5 02/13] hw/isa/vt82c686: Resolve unneeded attribute

[Bernhard Beschow]

Now that also the super io device is realized in the common realize method,
the isa_bus attribute can be turned into a temporary.

Signed-off-by: Bernhard Beschow 
---
 hw/isa/vt82c686.c | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 0217c98fe4..9d12e1cae4 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -543,7 +543,6 @@ struct ViaISAState {
 PCIDevice dev;
 qemu_irq cpu_intr;
 qemu_irq *isa_irqs;
-ISABus *isa_bus;
 ViaSuperIOState via_sio;
 };
 
@@ -585,17 +584,18 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
 ViaISAState *s = VIA_ISA(d);
 DeviceState *dev = DEVICE(d);
 qemu_irq *isa_irq;
+ISABus *isa_bus;
 int i;
 
 qdev_init_gpio_out(dev, >cpu_intr, 1);
 isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1);
-s->isa_bus = isa_bus_new(dev, get_system_memory(), pci_address_space_io(d),
+isa_bus = isa_bus_new(dev, get_system_memory(), pci_address_space_io(d),
   _fatal);
-s->isa_irqs = i8259_init(s->isa_bus, *isa_irq);
-isa_bus_irqs(s->isa_bus, s->isa_irqs);
-i8254_pit_init(s->isa_bus, 0x40, 0, NULL);
-i8257_dma_init(s->isa_bus, 0);
-mc146818_rtc_init(s->isa_bus, 2000, NULL);
+s->isa_irqs = i8259_init(isa_bus, *isa_irq);
+isa_bus_irqs(isa_bus, s->isa_irqs);
+i8254_pit_init(isa_bus, 0x40, 0, NULL);
+i8257_dma_init(isa_bus, 0);
+mc146818_rtc_init(isa_bus, 2000, NULL);
 
 for (i = 0; i < PCI_CONFIG_HEADER_SIZE; i++) {
 if (i < PCI_COMMAND || i >= PCI_REVISION_ID) {
@@ -604,7 +604,7 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
 }
 
 /* Super I/O */
-if (!qdev_realize(DEVICE(>via_sio), BUS(s->isa_bus), errp)) {
+if (!qdev_realize(DEVICE(>via_sio), BUS(isa_bus), errp)) {
 return;
 }
 }
-- 
2.37.3

[PATCH v5 13/13] hw/isa/vt82c686: Create rtc-time alias in boards instead

[Bernhard Beschow]

According to good QOM practice, an object should only deal with objects
of its own sub tree. Having devices create an alias on the machine
object doesn't respect this good practice. To resolve this, create the
alias in the machine's code.

Signed-off-by: Bernhard Beschow 
---
 hw/isa/vt82c686.c   | 2 --
 hw/mips/fuloong2e.c | 4 
 hw/ppc/pegasos2.c   | 4 
 3 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 48cd4d0036..3f9bd0c04d 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -632,8 +632,6 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
 if (!qdev_realize(DEVICE(>rtc), BUS(isa_bus), errp)) {
 return;
 }
-object_property_add_alias(qdev_get_machine(), "rtc-time", OBJECT(>rtc),
-  "date");
 isa_connect_gpio_out(ISA_DEVICE(>rtc), 0, s->rtc.isairq);
 
 for (i = 0; i < PCI_CONFIG_HEADER_SIZE; i++) {
diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
index 3c46215616..b478483706 100644
--- a/hw/mips/fuloong2e.c
+++ b/hw/mips/fuloong2e.c
@@ -295,6 +295,10 @@ static void mips_fuloong2e_init(MachineState *machine)
 pci_dev = pci_create_simple_multifunction(pci_bus,
   PCI_DEVFN(FULOONG2E_VIA_SLOT, 0),
   true, TYPE_VT82C686B_ISA);
+object_property_add_alias(OBJECT(machine), "rtc-time",
+  object_resolve_path_component(OBJECT(pci_dev),
+"rtc"),
+  "date");
 qdev_connect_gpio_out(DEVICE(pci_dev), 0, env->irq[5]);
 
 dev = DEVICE(object_resolve_path_component(OBJECT(pci_dev), "ide"));
diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
index 09fdb7557f..49b753c7cc 100644
--- a/hw/ppc/pegasos2.c
+++ b/hw/ppc/pegasos2.c
@@ -161,6 +161,10 @@ static void pegasos2_init(MachineState *machine)
 /* VIA VT8231 South Bridge (multifunction PCI device) */
 via = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(12, 0), true,
   TYPE_VT8231_ISA);
+object_property_add_alias(OBJECT(machine), "rtc-time",
+  object_resolve_path_component(OBJECT(via),
+"rtc"),
+  "date");
 qdev_connect_gpio_out(DEVICE(via), 0,
   qdev_get_gpio_in_named(pm->mv, "gpp", 31));
 
-- 
2.37.3

[PATCH v5 11/13] hw/mips/fuloong2e: Inline vt82c686b_southbridge_init() and remove it

[Bernhard Beschow]

The previous patches moved most of this function into the via-isa device
model such that it has become fairly trivial. So inline it for
simplicity.

Suggested-by: BALATON Zoltan 
Signed-off-by: Bernhard Beschow 
---
 hw/mips/fuloong2e.c | 28 ++--
 1 file changed, 10 insertions(+), 18 deletions(-)

diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
index 2d8723ab74..3c46215616 100644
--- a/hw/mips/fuloong2e.c
+++ b/hw/mips/fuloong2e.c
@@ -196,22 +196,6 @@ static void main_cpu_reset(void *opaque)
 }
 }
 
-static void vt82c686b_southbridge_init(PCIBus *pci_bus, int slot, qemu_irq 
intc,
-   I2CBus **i2c_bus)
-{
-PCIDevice *dev, *via;
-
-via = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(slot, 0), true,
-  TYPE_VT82C686B_ISA);
-qdev_connect_gpio_out(DEVICE(via), 0, intc);
-
-dev = PCI_DEVICE(object_resolve_path_component(OBJECT(via), "ide"));
-pci_ide_create_devs(dev);
-
-dev = PCI_DEVICE(object_resolve_path_component(OBJECT(via), "pm"));
-*i2c_bus = I2C_BUS(qdev_get_child_bus(DEVICE(dev), "i2c"));
-}
-
 /* Network support */
 static void network_init(PCIBus *pci_bus)
 {
@@ -308,8 +292,16 @@ static void mips_fuloong2e_init(MachineState *machine)
 pci_bus = bonito_init((qemu_irq *)&(env->irq[2]));
 
 /* South bridge -> IP5 */
-vt82c686b_southbridge_init(pci_bus, FULOONG2E_VIA_SLOT, env->irq[5],
-   );
+pci_dev = pci_create_simple_multifunction(pci_bus,
+  PCI_DEVFN(FULOONG2E_VIA_SLOT, 0),
+  true, TYPE_VT82C686B_ISA);
+qdev_connect_gpio_out(DEVICE(pci_dev), 0, env->irq[5]);
+
+dev = DEVICE(object_resolve_path_component(OBJECT(pci_dev), "ide"));
+pci_ide_create_devs(PCI_DEVICE(dev));
+
+dev = DEVICE(object_resolve_path_component(OBJECT(pci_dev), "pm"));
+smbus = I2C_BUS(qdev_get_child_bus(dev, "i2c"));
 
 /* GPU */
 if (vga_interface_type != VGA_NONE) {
-- 
2.37.3

[PATCH v5 04/13] hw/isa/vt82c686: Reuse errp

[Bernhard Beschow]

Rather than terminating abruptly, make use of the already present errp and
propagate the error to the caller.

Signed-off-by: Bernhard Beschow 
---
 hw/isa/vt82c686.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 5582c0b179..37e37b3855 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -590,7 +590,12 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
 qdev_init_gpio_out(dev, >cpu_intr, 1);
 isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1);
 isa_bus = isa_bus_new(dev, pci_address_space(d), pci_address_space_io(d),
-  _fatal);
+  errp);
+
+if (!isa_bus) {
+return;
+}
+
 s->isa_irqs = i8259_init(isa_bus, *isa_irq);
 isa_bus_irqs(isa_bus, s->isa_irqs);
 i8254_pit_init(isa_bus, 0x40, 0, NULL);
-- 
2.37.3

[PATCH v5 07/13] hw/isa/vt82c686: Introduce TYPE_VT82C686B_USB_UHCI define

[Bernhard Beschow]

Suggested-by: BALATON Zoltan 
Signed-off-by: Bernhard Beschow 
---
 hw/mips/fuloong2e.c| 4 ++--
 hw/ppc/pegasos2.c  | 4 ++--
 hw/usb/vt82c686-uhci-pci.c | 4 ++--
 include/hw/isa/vt82c686.h  | 1 +
 4 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
index 32605901e7..6b7370f2aa 100644
--- a/hw/mips/fuloong2e.c
+++ b/hw/mips/fuloong2e.c
@@ -208,8 +208,8 @@ static void vt82c686b_southbridge_init(PCIBus *pci_bus, int 
slot, qemu_irq intc,
 dev = PCI_DEVICE(object_resolve_path_component(OBJECT(via), "ide"));
 pci_ide_create_devs(dev);
 
-pci_create_simple(pci_bus, PCI_DEVFN(slot, 2), "vt82c686b-usb-uhci");
-pci_create_simple(pci_bus, PCI_DEVFN(slot, 3), "vt82c686b-usb-uhci");
+pci_create_simple(pci_bus, PCI_DEVFN(slot, 2), TYPE_VT82C686B_USB_UHCI);
+pci_create_simple(pci_bus, PCI_DEVFN(slot, 3), TYPE_VT82C686B_USB_UHCI);
 
 dev = pci_create_simple(pci_bus, PCI_DEVFN(slot, 4), TYPE_VT82C686B_PM);
 *i2c_bus = I2C_BUS(qdev_get_child_bus(DEVICE(dev), "i2c"));
diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
index 8bc528a560..70776558c9 100644
--- a/hw/ppc/pegasos2.c
+++ b/hw/ppc/pegasos2.c
@@ -169,8 +169,8 @@ static void pegasos2_init(MachineState *machine)
 pci_ide_create_devs(dev);
 
 /* VT8231 function 2-3: USB Ports */
-pci_create_simple(pci_bus, PCI_DEVFN(12, 2), "vt82c686b-usb-uhci");
-pci_create_simple(pci_bus, PCI_DEVFN(12, 3), "vt82c686b-usb-uhci");
+pci_create_simple(pci_bus, PCI_DEVFN(12, 2), TYPE_VT82C686B_USB_UHCI);
+pci_create_simple(pci_bus, PCI_DEVFN(12, 3), TYPE_VT82C686B_USB_UHCI);
 
 /* VT8231 function 4: Power Management Controller */
 dev = pci_create_simple(pci_bus, PCI_DEVFN(12, 4), TYPE_VT8231_PM);
diff --git a/hw/usb/vt82c686-uhci-pci.c b/hw/usb/vt82c686-uhci-pci.c
index 0bf2b72ff0..46a901f56f 100644
--- a/hw/usb/vt82c686-uhci-pci.c
+++ b/hw/usb/vt82c686-uhci-pci.c
@@ -31,7 +31,7 @@ static void usb_uhci_vt82c686b_realize(PCIDevice *dev, Error 
**errp)
 
 static UHCIInfo uhci_info[] = {
 {
-.name  = "vt82c686b-usb-uhci",
+.name  = TYPE_VT82C686B_USB_UHCI,
 .vendor_id = PCI_VENDOR_ID_VIA,
 .device_id = PCI_DEVICE_ID_VIA_UHCI,
 .revision  = 0x01,
@@ -45,7 +45,7 @@ static UHCIInfo uhci_info[] = {
 
 static const TypeInfo vt82c686b_usb_uhci_type_info = {
 .parent = TYPE_UHCI,
-.name   = "vt82c686b-usb-uhci",
+.name   = TYPE_VT82C686B_USB_UHCI,
 .class_init = uhci_data_class_init,
 .class_data = uhci_info,
 };
diff --git a/include/hw/isa/vt82c686.h b/include/hw/isa/vt82c686.h
index 87aca3e5bb..e6f6dd4d43 100644
--- a/include/hw/isa/vt82c686.h
+++ b/include/hw/isa/vt82c686.h
@@ -5,6 +5,7 @@
 
 #define TYPE_VT82C686B_ISA "vt82c686b-isa"
 #define TYPE_VT82C686B_PM "vt82c686b-pm"
+#define TYPE_VT82C686B_USB_UHCI "vt82c686b-usb-uhci"
 #define TYPE_VT8231_ISA "vt8231-isa"
 #define TYPE_VT8231_PM "vt8231-pm"
 #define TYPE_VIA_AC97 "via-ac97"
-- 
2.37.3

[PATCH v5 05/13] hw/isa/vt82c686: Introduce TYPE_VIA_IDE define

[Bernhard Beschow]

Establishes consistency with other (VIA) devices.

Signed-off-by: Bernhard Beschow 
---
 hw/ide/via.c  | 2 +-
 hw/mips/fuloong2e.c   | 2 +-
 hw/ppc/pegasos2.c | 2 +-
 include/hw/isa/vt82c686.h | 1 +
 4 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/ide/via.c b/hw/ide/via.c
index 82def819c4..e1a429405d 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -230,7 +230,7 @@ static void via_ide_class_init(ObjectClass *klass, void 
*data)
 }
 
 static const TypeInfo via_ide_info = {
-.name  = "via-ide",
+.name  = TYPE_VIA_IDE,
 .parent= TYPE_PCI_IDE,
 .class_init= via_ide_class_init,
 };
diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
index 5ee546f5f6..44225fbe33 100644
--- a/hw/mips/fuloong2e.c
+++ b/hw/mips/fuloong2e.c
@@ -205,7 +205,7 @@ static void vt82c686b_southbridge_init(PCIBus *pci_bus, int 
slot, qemu_irq intc,
   TYPE_VT82C686B_ISA);
 qdev_connect_gpio_out(DEVICE(dev), 0, intc);
 
-dev = pci_create_simple(pci_bus, PCI_DEVFN(slot, 1), "via-ide");
+dev = pci_create_simple(pci_bus, PCI_DEVFN(slot, 1), TYPE_VIA_IDE);
 pci_ide_create_devs(dev);
 
 pci_create_simple(pci_bus, PCI_DEVFN(slot, 2), "vt82c686b-usb-uhci");
diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
index 61f4263953..8039775f80 100644
--- a/hw/ppc/pegasos2.c
+++ b/hw/ppc/pegasos2.c
@@ -166,7 +166,7 @@ static void pegasos2_init(MachineState *machine)
   qdev_get_gpio_in_named(pm->mv, "gpp", 31));
 
 /* VT8231 function 1: IDE Controller */
-dev = pci_create_simple(pci_bus, PCI_DEVFN(12, 1), "via-ide");
+dev = pci_create_simple(pci_bus, PCI_DEVFN(12, 1), TYPE_VIA_IDE);
 pci_ide_create_devs(dev);
 
 /* VT8231 function 2-3: USB Ports */
diff --git a/include/hw/isa/vt82c686.h b/include/hw/isa/vt82c686.h
index 56ac141be3..87aca3e5bb 100644
--- a/include/hw/isa/vt82c686.h
+++ b/include/hw/isa/vt82c686.h
@@ -8,6 +8,7 @@
 #define TYPE_VT8231_ISA "vt8231-isa"
 #define TYPE_VT8231_PM "vt8231-pm"
 #define TYPE_VIA_AC97 "via-ac97"
+#define TYPE_VIA_IDE "via-ide"
 #define TYPE_VIA_MC97 "via-mc97"
 
 void via_isa_set_irq(PCIDevice *d, int n, int level);
-- 
2.37.3

[PATCH v5 03/13] hw/isa/vt82c686: Prefer pci_address_space() over get_system_memory()

[Bernhard Beschow]

Unlike get_system_memory(), pci_address_space() respects the memory tree
available to the parent device.

Signed-off-by: Bernhard Beschow 
---
 hw/isa/vt82c686.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 9d12e1cae4..5582c0b179 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -589,7 +589,7 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
 
 qdev_init_gpio_out(dev, >cpu_intr, 1);
 isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1);
-isa_bus = isa_bus_new(dev, get_system_memory(), pci_address_space_io(d),
+isa_bus = isa_bus_new(dev, pci_address_space(d), pci_address_space_io(d),
   _fatal);
 s->isa_irqs = i8259_init(isa_bus, *isa_irq);
 isa_bus_irqs(isa_bus, s->isa_irqs);
-- 
2.37.3

Re: [PATCH v2 0/8] vhost-user-blk: dynamically resize config space based on features

[Daniil Tatianin]

ping

Re: [PATCH 38/51] tests/qtest: {ahci, ide}-test: Open file in binary mode

[Marc-André Lureau]

Hi

On Wed, Aug 24, 2022 at 3:08 PM Bin Meng  wrote:

> From: Xuzhou Cheng 
>
> By default Windows opens file in text mode, while a POSIX compliant
> implementation treats text files and binary files the same.
>
> The fopen() 'mode' string can include the letter 'b' to indicate
> binary mode shall be used. POSIX spec says the character 'b' shall
> have no effect, but is allowed for ISO C standard conformance.
> Let's add the letter 'b' which works on both POSIX and Windows.
>
> Similar situation applies to the open() 'flags' where O_BINARY is
> used for binary mode.
>
> Signed-off-by: Xuzhou Cheng 
> Signed-off-by: Bin Meng 
> ---
>
>  tests/qtest/ahci-test.c | 2 +-
>  tests/qtest/ide-test.c  | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/tests/qtest/ahci-test.c b/tests/qtest/ahci-test.c
> index bce9ff770c..be11508c75 100644
> --- a/tests/qtest/ahci-test.c
> +++ b/tests/qtest/ahci-test.c
> @@ -1453,7 +1453,7 @@ static int prepare_iso(size_t size, unsigned char
> **buf, char **name)
>   * Close the file and reopen it.
>   */
>  close(fd);
> -fd = open(cdrom_path, O_WRONLY);
> +fd = open(cdrom_path, O_WRONLY | O_BINARY);
>  g_assert(fd != -1);
>

that should be gone in next iteration, with g_mkstemp() usage.


>  #endif
>
> diff --git a/tests/qtest/ide-test.c b/tests/qtest/ide-test.c
> index c5cad6c0be..ee03dea4fa 100644
> --- a/tests/qtest/ide-test.c
> +++ b/tests/qtest/ide-test.c
> @@ -892,7 +892,7 @@ static void cdrom_pio_impl(int nblocks)
>
>  /* Prepopulate the CDROM with an interesting pattern */
>  generate_pattern(pattern, patt_len, ATAPI_BLOCK_SIZE);
> -fh = fopen(tmp_path, "w+");
> +fh = fopen(tmp_path, "wb+");
>  ret = fwrite(pattern, ATAPI_BLOCK_SIZE, patt_blocks, fh);
>  g_assert_cmpint(ret, ==, patt_blocks);
>  fclose(fh);
> @@ -993,7 +993,7 @@ static void test_cdrom_dma(void)
>  prdt[0].size = cpu_to_le32(len | PRDT_EOT);
>
>  generate_pattern(pattern, ATAPI_BLOCK_SIZE * 16, ATAPI_BLOCK_SIZE);
> -fh = fopen(tmp_path, "w+");
> +fh = fopen(tmp_path, "wb+");
>  ret = fwrite(pattern, ATAPI_BLOCK_SIZE, 16, fh);
>  g_assert_cmpint(ret, ==, 16);
>  fclose(fh);
> --
> 2.34.1
>
>
>
ack this part,
Reviewed-by: Marc-André Lureau 


-- 
Marc-André Lureau

Re: [PATCH 33/51] tests/qtest: {ahci, ide}-test: Use relative path for temporary files

[Marc-André Lureau]

On Wed, Aug 24, 2022 at 2:55 PM Bin Meng  wrote:

> From: Bin Meng 
>
> These test cases uses "blkdebug:path/to/config:path/to/image" for
> testing. On Windows, absolute file paths contain the delimiter ':'
> which causes the blkdebug filename parser fail to parse filenames.
>
>
hmm.. maybe it should learn to escape paths..


Signed-off-by: Bin Meng 
> ---
>
>  tests/qtest/ahci-test.c | 19 ---
>  tests/qtest/ide-test.c  | 18 --
>  2 files changed, 32 insertions(+), 5 deletions(-)
>
> diff --git a/tests/qtest/ahci-test.c b/tests/qtest/ahci-test.c
> index 0e88cd0eef..bce9ff770c 100644
> --- a/tests/qtest/ahci-test.c
> +++ b/tests/qtest/ahci-test.c
> @@ -1848,7 +1848,7 @@ static void create_ahci_io_test(enum IOMode type,
> enum AddrMode addr,
>
>  int main(int argc, char **argv)
>  {
> -const char *arch;
> +const char *arch, *base;
>  int ret;
>  int fd;
>  int c;
> @@ -1886,8 +1886,21 @@ int main(int argc, char **argv)
>  return 0;
>  }
>
> +/*
> + * "base" stores the starting point where we create temporary files.
> + *
> + * On Windows, this is set to the relative path of current working
> + * directory, because the absolute path causes the blkdebug filename
> + * parser fail to parse "blkdebug:path/to/config:path/to/image".
> + */
> +#ifndef _WIN32
> +base = g_get_tmp_dir();
> +#else
> +base = ".";
> +#endif
>

Meanwhile, that seems reasonable. Perhaps chdir() to the temporary
directory first? (assuming other paths are absolute)


> +
>  /* Create a temporary image */
> -tmp_path = g_strdup_printf("%s/qtest.XX", g_get_tmp_dir());
> +tmp_path = g_strdup_printf("%s/qtest.XX", base);
>  fd = mkstemp(tmp_path);
>  g_assert(fd >= 0);
>  if (have_qemu_img()) {
> @@ -1905,7 +1918,7 @@ int main(int argc, char **argv)
>  close(fd);
>
>  /* Create temporary blkdebug instructions */
> -debug_path = g_strdup_printf("%s/qtest-blkdebug.XX",
> g_get_tmp_dir());
> +debug_path = g_strdup_printf("%s/qtest-blkdebug.XX", base);
>  fd = mkstemp(debug_path);
>  g_assert(fd >= 0);
>  close(fd);
> diff --git a/tests/qtest/ide-test.c b/tests/qtest/ide-test.c
> index ebbf8e0126..c5cad6c0be 100644
> --- a/tests/qtest/ide-test.c
> +++ b/tests/qtest/ide-test.c
> @@ -1011,17 +1011,31 @@ static void test_cdrom_dma(void)
>
>  int main(int argc, char **argv)
>  {
> +const char *base;
>  int fd;
>  int ret;
>
> +/*
> + * "base" stores the starting point where we create temporary files.
> + *
> + * On Windows, this is set to the relative path of current working
> + * directory, because the absolute path causes the blkdebug filename
> + * parser fail to parse "blkdebug:path/to/config:path/to/image".
> + */
> +#ifndef _WIN32
> +base = g_get_tmp_dir();
> +#else
> +base = ".";
> +#endif
> +
>  /* Create temporary blkdebug instructions */
> -debug_path = g_strdup_printf("%s/qtest-blkdebug.XX",
> g_get_tmp_dir());
> +debug_path = g_strdup_printf("%s/qtest-blkdebug.XX", base);
>  fd = mkstemp(debug_path);
>  g_assert(fd >= 0);
>  close(fd);
>
>  /* Create a temporary raw image */
> -tmp_path = g_strdup_printf("%s/qtest.XX", g_get_tmp_dir());
> +tmp_path = g_strdup_printf("%s/qtest.XX", base);
>  fd = mkstemp(tmp_path);
>  g_assert(fd >= 0);
>  ret = ftruncate(fd, TEST_IMAGE_SIZE);
> --
> 2.34.1
>
>
>

-- 
Marc-André Lureau

Re: [PATCH 32/51] tests/qtest: Fix ERROR_SHARING_VIOLATION for win32

[Marc-André Lureau]

Hi

On Wed, Aug 24, 2022 at 2:03 PM Bin Meng  wrote:

> From: Bin Meng 
>
> On Windows, the MinGW provided mkstemp() API opens the file with
> exclusive access, denying other processes to read/write the file.
> Such behavior prevents the QEMU executable from opening the file,
> (e.g.: CreateFile returns ERROR_SHARING_VIOLATION).
>

g_mkstemp() doesn't have this behaviour (after running a quick test). Use
it?


>
> This can be fixed by closing the file and reopening it.
>
> Signed-off-by: Bin Meng 
> ---
>
>  tests/qtest/ahci-test.c| 14 ++
>  tests/qtest/boot-serial-test.c | 13 +
>  2 files changed, 27 insertions(+)
>
> diff --git a/tests/qtest/ahci-test.c b/tests/qtest/ahci-test.c
> index f26cd6f86f..0e88cd0eef 100644
> --- a/tests/qtest/ahci-test.c
> +++ b/tests/qtest/ahci-test.c
> @@ -1443,6 +1443,20 @@ static int prepare_iso(size_t size, unsigned char
> **buf, char **name)
>  int fd = mkstemp(cdrom_path);
>
>  g_assert(fd != -1);
> +#ifdef _WIN32
> +/*
> + * On Windows, the MinGW provided mkstemp() API opens the file with
> + * exclusive access, denying other processes to read/write the file.
> + * Such behavior prevents the QEMU executable from opening the file,
> + * (e.g.: CreateFile returns ERROR_SHARING_VIOLATION).
> + *
> + * Close the file and reopen it.
> + */
> +close(fd);
> +fd = open(cdrom_path, O_WRONLY);
> +g_assert(fd != -1);
> +#endif
> +
>  g_assert(buf);
>  g_assert(name);
>  patt = g_malloc(size);
> diff --git a/tests/qtest/boot-serial-test.c
> b/tests/qtest/boot-serial-test.c
> index 404adcfa20..fb6c81bf35 100644
> --- a/tests/qtest/boot-serial-test.c
> +++ b/tests/qtest/boot-serial-test.c
> @@ -235,6 +235,19 @@ static void test_machine(const void *data)
>
>  ser_fd = mkstemp(serialtmp);
>  g_assert(ser_fd != -1);
> +#ifdef _WIN32
> +/*
> + * On Windows, the MinGW provided mkstemp() API opens the file with
> + * exclusive access, denying other processes to read/write the file.
> + * Such behavior prevents the QEMU executable from opening the file,
> + * (e.g.: CreateFile returns ERROR_SHARING_VIOLATION).
> + *
> + * Close the file and reopen it.
> + */
> +close(ser_fd);
> +ser_fd = open(serialtmp, O_RDONLY);
> +g_assert(ser_fd != -1);
> +#endif
>
>  if (test->kernel) {
>  code = test->kernel;
> --
> 2.34.1
>
>
>

-- 
Marc-André Lureau

Re: [PATCH 03/51] block: Unify the get_tmp_filename() implementation

[Bin Meng]

Hi Marc-André,

On Wed, Aug 31, 2022 at 8:54 PM Marc-André Lureau
 wrote:
>
> Hi Bin
>
> On Wed, Aug 24, 2022 at 1:42 PM Bin Meng  wrote:
>>
>> From: Bin Meng 
>>
>> At present get_tmp_filename() has platform specific implementations
>> to get the directory to use for temporary files. Switch over to use
>> g_get_tmp_dir() which works on all supported platforms.
>>
>
> It "works" quite differently though. Is this patch really necessary here?

Without this patch the qtest cases builds on Windows do not have any
problem. So it is optional. I put it in the same series as it has the
same context of using hardcoded /tmp directory name.

>
> If yes, please explain why.
>
> If not, I suggest you drop optional / rfc / "nice to have" patches from the 
> series. It will help to get it merged faster.

I can drop this single patch and send another single patch if this is
the desired practice.

>
> thanks

Regards,
Bin