Re: [PATCH v3 1/6] block: add bitmap-populate job

[Vladimir Sementsov-Ogievskiy]

19.06.2020 22:56, Eric Blake wrote:

From: John Snow 

This job copies the allocation map into a bitmap. It's a job because
there's no guarantee that allocation interrogation will be quick (or
won't hang), so it cannot be retrofitted into block-dirty-bitmap-merge.

It was designed with different possible population patterns in mind,
but only top layer allocation was implemented for now.

Signed-off-by: John Snow 
Signed-off-by: Eric Blake 
---
  qapi/block-core.json  |  48 +
  qapi/job.json |   6 +-
  include/block/block_int.h |  21 
  block/bitmap-populate.c   | 207 ++
  blockjob.c|   3 +-
  MAINTAINERS   |   1 +
  block/Makefile.objs   |   1 +
  7 files changed, 285 insertions(+), 2 deletions(-)
  create mode 100644 block/bitmap-populate.c

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 0e1c6a59f228..a1bcdba04423 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2211,6 +2211,54 @@
{ 'command': 'block-dirty-bitmap-merge',
  'data': 'BlockDirtyBitmapMerge' }

+##
+# @BitmapPattern:
+#
+# An enumeration of possible patterns that can be written into a bitmap.
+#
+# @allocation-top: The allocation status of the top layer
+#  of the attached storage node.
+#
+# Since: 5.1
+##
+{ 'enum': 'BitmapPattern',
+  'data': ['allocation-top'] }
+
+##
+# @BlockDirtyBitmapPopulate:
+#
+# @job-id: identifier for the newly-created block job.
+#
+# @pattern: What pattern should be written into the bitmap?
+#
+# @on-error: the action to take if an error is encountered on a bitmap's
+#attached node, default 'report'.
+#'stop' and 'enospc' can only be used if the block device supports
+#io-status (see BlockInfo).
+#
+# @auto-finalize: When false, this job will wait in a PENDING state after it 
has
+# finished its work, waiting for @block-job-finalize before
+# making any block graph changes.
+# When true, this job will automatically
+# perform its abort or commit actions.
+# Defaults to true.
+#
+# @auto-dismiss: When false, this job will wait in a CONCLUDED state after it
+#has completely ceased all work, and awaits @block-job-dismiss.
+#When true, this job will automatically disappear from the 
query
+#list without user intervention.
+#Defaults to true.
+#
+# Since: 5.1
+##
+{ 'struct': 'BlockDirtyBitmapPopulate',
+  'base': 'BlockDirtyBitmap',
+  'data': { 'job-id': 'str',
+'pattern': 'BitmapPattern',
+'*on-error': 'BlockdevOnError',
+'*auto-finalize': 'bool',
+'*auto-dismiss': 'bool' } }
+


Peter said about a possibility of populating several target bitmaps 
simultaneously.

What about such a generalized semantics:

Merge all sources to each target

@targets: list of bitmaps to be populated by the job
{ 'struct': 'BlockDirtyBitmapPopulate',
  'data': { ,
'targets': ['BlockDirtyBitmap'],
'sources': ['BitmapPopulateSource'] } }


@bitmap: specify dirty bitmap to be merged to target bitamp(s)
@node: specify a node name, which allocation-map is to be merged to target 
bitmap(s)
{ 'alternate': 'BitmapPopulateSource',
  'data': { 'bitmap': 'BlockDirtyBitmap',
'node': 'str' } }


- so, we can merge several bitmaps together with several allocation maps into 
several target bitmaps.
(I remember, we also said about a possibility of starting several populating 
jobs, populating into
 same bitmap, I think it may be substituted by one job with several sources. 
Still, it's not hard to
 allow to use target bitmaps in a several jobs simultaneously and this is not 
about the QAPI interface)

Will this simplify things in libvirt?

--
Best regards,
Vladimir

Re: [PATCH 4/6] block/block-backend: remove always true check from blk_save_vmstate

[Vladimir Sementsov-Ogievskiy]

19.06.2020 13:07, Denis V. Lunev wrote:

bdrv_save_vmstate() returns either error with negative return value or
size. Thus this check is useless.

Signed-off-by: Denis V. Lunev
Suggested-by: Eric Blake
CC: Kevin Wolf
CC: Max Reitz
CC: Stefan Hajnoczi
CC: Fam Zheng
CC: Juan Quintela
CC: "Dr. David Alan Gilbert"
CC: Vladimir Sementsov-Ogievskiy
CC: Denis Plotnikov


Reviewed-by: Vladimir Sementsov-Ogievskiy 

--
Best regards,
Vladimir

[PATCH v3 6/6] bitmaps: Use x- prefix for block-dirty-bitmap-popluate

[Eric Blake]

Give ourselves an out if we need to tweak the interface, in order to
gain more experience with what works when libvirt experiments with
using it.

Signed-off-by: Eric Blake 
---
 qapi/block-core.json   |   6 +-
 qapi/transaction.json  |   4 +-
 blockdev.c |  14 ++--
 tests/qemu-iotests/298 |   2 +-
 tests/qemu-iotests/298.out | 128 ++---
 5 files changed, 77 insertions(+), 77 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 313583b47c16..dcf6b907e45c 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2260,7 +2260,7 @@
 '*auto-dismiss': 'bool' } }

 ##
-# @block-dirty-bitmap-populate:
+# @x-block-dirty-bitmap-populate:
 #
 # Creates a new job that writes a pattern into a dirty bitmap.
 #
@@ -2268,13 +2268,13 @@
 #
 # Example:
 #
-# -> { "execute": "block-dirty-bitmap-populate",
+# -> { "execute": "x-block-dirty-bitmap-populate",
 #  "arguments": { "node": "drive0", "target": "bitmap0",
 # "job-id": "job0", "pattern": "allocate-top" } }
 # <- { "return": {} }
 #
 ##
-{ 'command': 'block-dirty-bitmap-populate', 'boxed': true,
+{ 'command': 'x-block-dirty-bitmap-populate', 'boxed': true,
   'data': 'BlockDirtyBitmapPopulate' }

 ##
diff --git a/qapi/transaction.json b/qapi/transaction.json
index 21be59faae56..3277e948f321 100644
--- a/qapi/transaction.json
+++ b/qapi/transaction.json
@@ -50,7 +50,7 @@
 # - @block-dirty-bitmap-enable: since 4.0
 # - @block-dirty-bitmap-disable: since 4.0
 # - @block-dirty-bitmap-merge: since 4.0
-# - @block-dirty-bitmap-populate: since 5.1
+# - @x-block-dirty-bitmap-populate: since 5.1
 # - @blockdev-backup: since 2.3
 # - @blockdev-snapshot: since 2.5
 # - @blockdev-snapshot-internal-sync: since 1.7
@@ -68,7 +68,7 @@
'block-dirty-bitmap-enable': 'BlockDirtyBitmap',
'block-dirty-bitmap-disable': 'BlockDirtyBitmap',
'block-dirty-bitmap-merge': 'BlockDirtyBitmapMerge',
-   'block-dirty-bitmap-populate': 'BlockDirtyBitmapPopulate',
+   'x-block-dirty-bitmap-populate': 'BlockDirtyBitmapPopulate',
'blockdev-backup': 'BlockdevBackup',
'blockdev-snapshot': 'BlockdevSnapshot',
'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal',
diff --git a/blockdev.c b/blockdev.c
index d072519e7b91..b86ef5b7f281 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2188,8 +2188,8 @@ static void 
block_dirty_bitmap_populate_prepare(BlkActionState *common,
 int job_flags = JOB_DEFAULT;

 assert(common->action->type ==
-   TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_POPULATE);
-bitpop = common->action->u.block_dirty_bitmap_populate.data;
+   TRANSACTION_ACTION_KIND_X_BLOCK_DIRTY_BITMAP_POPULATE);
+bitpop = common->action->u.x_block_dirty_bitmap_populate.data;

 bmap = block_dirty_bitmap_lookup(bitpop->node, bitpop->name, , errp);
 if (!bmap) {
@@ -2317,7 +2317,7 @@ static const BlkActionOps actions[] = {
 .commit = block_dirty_bitmap_remove_commit,
 .abort = block_dirty_bitmap_remove_abort,
 },
-[TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_POPULATE] = {
+[TRANSACTION_ACTION_KIND_X_BLOCK_DIRTY_BITMAP_POPULATE] = {
 .instance_size = sizeof(BlockJobActionState),
 .prepare = block_dirty_bitmap_populate_prepare,
 .commit = blockdev_backup_commit,
@@ -2443,12 +2443,12 @@ void qmp_block_passwd(bool has_device, const char 
*device,
"Setting block passwords directly is no longer supported");
 }

-void qmp_block_dirty_bitmap_populate(BlockDirtyBitmapPopulate *bitpop,
- Error **errp)
+void qmp_x_block_dirty_bitmap_populate(BlockDirtyBitmapPopulate *bitpop,
+   Error **errp)
 {
 TransactionAction action = {
-.type = TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_POPULATE,
-.u.block_dirty_bitmap_populate.data = bitpop,
+.type = TRANSACTION_ACTION_KIND_X_BLOCK_DIRTY_BITMAP_POPULATE,
+.u.x_block_dirty_bitmap_populate.data = bitpop,
 };
 blockdev_do_action(, errp);
 }
diff --git a/tests/qemu-iotests/298 b/tests/qemu-iotests/298
index 4bfcecd3bc88..2a3df2de85db 100755
--- a/tests/qemu-iotests/298
+++ b/tests/qemu-iotests/298
@@ -49,7 +49,7 @@ class Drive:
 def block_dirty_bitmap_populate(vm, node, bitmap, job_id, pattern, **kwargs):
 # Strip any arguments explicitly nulled by the caller:
 kwargs = {key: val for key, val in kwargs.items() if val is not None}
-result = vm.qmp_log('block-dirty-bitmap-populate',
+result = vm.qmp_log('x-block-dirty-bitmap-populate',
 node=node,
 name=bitmap,
 job_id=job_id,
diff --git a/tests/qemu-iotests/298.out b/tests/qemu-iotests/298.out
index 7c0afc71920c..8b75f0e516c0 100644
--- a/tests/qemu-iotests/298.out
+++ b/tests/qemu-iotests/298.out
@@ -33,7 +33,7 @@
 expecting 0 dirty sectors; have 0. 

[PATCH v3 2/6] blockdev: combine DriveBackupState and BlockdevBackupState

[Eric Blake]

From: John Snow 

They have the same fields -- rename it BlockJobActionState.

Signed-off-by: John Snow 
Signed-off-by: Eric Blake 
---
 blockdev.c | 30 --
 1 file changed, 12 insertions(+), 18 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 72df193ca73b..6d80af903c55 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1655,11 +1655,11 @@ static void external_snapshot_clean(BlkActionState 
*common)
 aio_context_release(aio_context);
 }

-typedef struct DriveBackupState {
+typedef struct BlockJobActionState {
 BlkActionState common;
 BlockDriverState *bs;
 BlockJob *job;
-} DriveBackupState;
+} BlockJobActionState;

 static BlockJob *do_backup_common(BackupCommon *backup,
   BlockDriverState *bs,
@@ -1669,7 +1669,7 @@ static BlockJob *do_backup_common(BackupCommon *backup,

 static void drive_backup_prepare(BlkActionState *common, Error **errp)
 {
-DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common);
+BlockJobActionState *state = DO_UPCAST(BlockJobActionState, common, 
common);
 DriveBackup *backup;
 BlockDriverState *bs;
 BlockDriverState *target_bs;
@@ -1806,7 +1806,7 @@ out:

 static void drive_backup_commit(BlkActionState *common)
 {
-DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common);
+BlockJobActionState *state = DO_UPCAST(BlockJobActionState, common, 
common);
 AioContext *aio_context;

 aio_context = bdrv_get_aio_context(state->bs);
@@ -1820,7 +1820,7 @@ static void drive_backup_commit(BlkActionState *common)

 static void drive_backup_abort(BlkActionState *common)
 {
-DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common);
+BlockJobActionState *state = DO_UPCAST(BlockJobActionState, common, 
common);

 if (state->job) {
 AioContext *aio_context;
@@ -1836,7 +1836,7 @@ static void drive_backup_abort(BlkActionState *common)

 static void drive_backup_clean(BlkActionState *common)
 {
-DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common);
+BlockJobActionState *state = DO_UPCAST(BlockJobActionState, common, 
common);
 AioContext *aio_context;

 if (!state->bs) {
@@ -1851,15 +1851,9 @@ static void drive_backup_clean(BlkActionState *common)
 aio_context_release(aio_context);
 }

-typedef struct BlockdevBackupState {
-BlkActionState common;
-BlockDriverState *bs;
-BlockJob *job;
-} BlockdevBackupState;
-
 static void blockdev_backup_prepare(BlkActionState *common, Error **errp)
 {
-BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, 
common);
+BlockJobActionState *state = DO_UPCAST(BlockJobActionState, common, 
common);
 BlockdevBackup *backup;
 BlockDriverState *bs;
 BlockDriverState *target_bs;
@@ -1907,7 +1901,7 @@ static void blockdev_backup_prepare(BlkActionState 
*common, Error **errp)

 static void blockdev_backup_commit(BlkActionState *common)
 {
-BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, 
common);
+BlockJobActionState *state = DO_UPCAST(BlockJobActionState, common, 
common);
 AioContext *aio_context;

 aio_context = bdrv_get_aio_context(state->bs);
@@ -1921,7 +1915,7 @@ static void blockdev_backup_commit(BlkActionState *common)

 static void blockdev_backup_abort(BlkActionState *common)
 {
-BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, 
common);
+BlockJobActionState *state = DO_UPCAST(BlockJobActionState, common, 
common);

 if (state->job) {
 AioContext *aio_context;
@@ -1937,7 +1931,7 @@ static void blockdev_backup_abort(BlkActionState *common)

 static void blockdev_backup_clean(BlkActionState *common)
 {
-BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, 
common);
+BlockJobActionState *state = DO_UPCAST(BlockJobActionState, common, 
common);
 AioContext *aio_context;

 if (!state->bs) {
@@ -2209,14 +2203,14 @@ static const BlkActionOps actions[] = {
 .clean = external_snapshot_clean,
 },
 [TRANSACTION_ACTION_KIND_DRIVE_BACKUP] = {
-.instance_size = sizeof(DriveBackupState),
+.instance_size = sizeof(BlockJobActionState),
 .prepare = drive_backup_prepare,
 .commit = drive_backup_commit,
 .abort = drive_backup_abort,
 .clean = drive_backup_clean,
 },
 [TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP] = {
-.instance_size = sizeof(BlockdevBackupState),
+.instance_size = sizeof(BlockJobActionState),
 .prepare = blockdev_backup_prepare,
 .commit = blockdev_backup_commit,
 .abort = blockdev_backup_abort,
-- 
2.27.0

[PATCH v3 4/6] iotests: move bitmap helpers into their own file

[Eric Blake]

From: John Snow 

Signed-off-by: John Snow 
Message-Id: <20200514034922.24834-5-js...@redhat.com>
Signed-off-by: Eric Blake 
---
 tests/qemu-iotests/257| 110 +---
 tests/qemu-iotests/bitmaps.py | 131 ++
 2 files changed, 132 insertions(+), 109 deletions(-)
 create mode 100644 tests/qemu-iotests/bitmaps.py

diff --git a/tests/qemu-iotests/257 b/tests/qemu-iotests/257
index 004a433b8be2..2a81f9e30c56 100755
--- a/tests/qemu-iotests/257
+++ b/tests/qemu-iotests/257
@@ -24,120 +24,12 @@ import os

 import iotests
 from iotests import log, qemu_img
+from bitmaps import EmulatedBitmap, GROUPS

 SIZE = 64 * 1024 * 1024
 GRANULARITY = 64 * 1024


-class Pattern:
-def __init__(self, byte, offset, size=GRANULARITY):
-self.byte = byte
-self.offset = offset
-self.size = size
-
-def bits(self, granularity):
-lower = self.offset // granularity
-upper = (self.offset + self.size - 1) // granularity
-return set(range(lower, upper + 1))
-
-
-class PatternGroup:
-"""Grouping of Pattern objects. Initialize with an iterable of Patterns."""
-def __init__(self, patterns):
-self.patterns = patterns
-
-def bits(self, granularity):
-"""Calculate the unique bits dirtied by this pattern grouping"""
-res = set()
-for pattern in self.patterns:
-res |= pattern.bits(granularity)
-return res
-
-
-GROUPS = [
-PatternGroup([
-# Batch 0: 4 clusters
-Pattern('0x49', 0x000),
-Pattern('0x6c', 0x010),   # 1M
-Pattern('0x6f', 0x200),   # 32M
-Pattern('0x76', 0x3ff)]), # 64M - 64K
-PatternGroup([
-# Batch 1: 6 clusters (3 new)
-Pattern('0x65', 0x000),   # Full overwrite
-Pattern('0x77', 0x00f8000),   # Partial-left (1M-32K)
-Pattern('0x72', 0x2008000),   # Partial-right (32M+32K)
-Pattern('0x69', 0x3fe)]), # Adjacent-left (64M - 128K)
-PatternGroup([
-# Batch 2: 7 clusters (3 new)
-Pattern('0x74', 0x001),   # Adjacent-right
-Pattern('0x69', 0x00e8000),   # Partial-left  (1M-96K)
-Pattern('0x6e', 0x2018000),   # Partial-right (32M+96K)
-Pattern('0x67', 0x3fe,
-2*GRANULARITY)]), # Overwrite [(64M-128K)-64M)
-PatternGroup([
-# Batch 3: 8 clusters (5 new)
-# Carefully chosen such that nothing re-dirties the one cluster
-# that copies out successfully before failure in Group #1.
-Pattern('0xaa', 0x001,
-3*GRANULARITY),   # Overwrite and 2x Adjacent-right
-Pattern('0xbb', 0x00d8000),   # Partial-left (1M-160K)
-Pattern('0xcc', 0x2028000),   # Partial-right (32M+160K)
-Pattern('0xdd', 0x3fc)]), # New; leaving a gap to the right
-]
-
-
-class EmulatedBitmap:
-def __init__(self, granularity=GRANULARITY):
-self._bits = set()
-self.granularity = granularity
-
-def dirty_bits(self, bits):
-self._bits |= set(bits)
-
-def dirty_group(self, n):
-self.dirty_bits(GROUPS[n].bits(self.granularity))
-
-def clear(self):
-self._bits = set()
-
-def clear_bits(self, bits):
-self._bits -= set(bits)
-
-def clear_bit(self, bit):
-self.clear_bits({bit})
-
-def clear_group(self, n):
-self.clear_bits(GROUPS[n].bits(self.granularity))
-
-@property
-def first_bit(self):
-return sorted(self.bits)[0]
-
-@property
-def bits(self):
-return self._bits
-
-@property
-def count(self):
-return len(self.bits)
-
-def compare(self, qmp_bitmap):
-"""
-Print a nice human-readable message checking that a bitmap as reported
-by the QMP interface has as many bits set as we expect it to.
-"""
-
-name = qmp_bitmap.get('name', '(anonymous)')
-log("= Checking Bitmap {:s} =".format(name))
-
-want = self.count
-have = qmp_bitmap['count'] // qmp_bitmap['granularity']
-
-log("expecting {:d} dirty sectors; have {:d}. {:s}".format(
-want, have, "OK!" if want == have else "ERROR!"))
-log('')
-
-
 class Drive:
 """Represents, vaguely, a drive attached to a VM.
 Includes format, graph, and device information."""
diff --git a/tests/qemu-iotests/bitmaps.py b/tests/qemu-iotests/bitmaps.py
new file mode 100644
index ..522fc25171d1
--- /dev/null
+++ b/tests/qemu-iotests/bitmaps.py
@@ -0,0 +1,131 @@
+# Bitmap-related helper utilities
+#
+# Copyright (c) 2020 John Snow for 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 

[PATCH v3 3/6] qmp: expose block-dirty-bitmap-populate

[Eric Blake]

From: John Snow 

This is a new job-creating command.

Signed-off-by: John Snow 
Signed-off-by: Eric Blake 
---
 qapi/block-core.json  | 18 +++
 qapi/transaction.json |  2 ++
 blockdev.c| 74 +++
 3 files changed, 94 insertions(+)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index a1bcdba04423..313583b47c16 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2259,6 +2259,24 @@
 '*auto-finalize': 'bool',
 '*auto-dismiss': 'bool' } }

+##
+# @block-dirty-bitmap-populate:
+#
+# Creates a new job that writes a pattern into a dirty bitmap.
+#
+# Since: 5.1
+#
+# Example:
+#
+# -> { "execute": "block-dirty-bitmap-populate",
+#  "arguments": { "node": "drive0", "target": "bitmap0",
+# "job-id": "job0", "pattern": "allocate-top" } }
+# <- { "return": {} }
+#
+##
+{ 'command': 'block-dirty-bitmap-populate', 'boxed': true,
+  'data': 'BlockDirtyBitmapPopulate' }
+
 ##
 # @BlockDirtyBitmapSha256:
 #
diff --git a/qapi/transaction.json b/qapi/transaction.json
index b6c11158f0b6..21be59faae56 100644
--- a/qapi/transaction.json
+++ b/qapi/transaction.json
@@ -50,6 +50,7 @@
 # - @block-dirty-bitmap-enable: since 4.0
 # - @block-dirty-bitmap-disable: since 4.0
 # - @block-dirty-bitmap-merge: since 4.0
+# - @block-dirty-bitmap-populate: since 5.1
 # - @blockdev-backup: since 2.3
 # - @blockdev-snapshot: since 2.5
 # - @blockdev-snapshot-internal-sync: since 1.7
@@ -67,6 +68,7 @@
'block-dirty-bitmap-enable': 'BlockDirtyBitmap',
'block-dirty-bitmap-disable': 'BlockDirtyBitmap',
'block-dirty-bitmap-merge': 'BlockDirtyBitmapMerge',
+   'block-dirty-bitmap-populate': 'BlockDirtyBitmapPopulate',
'blockdev-backup': 'BlockdevBackup',
'blockdev-snapshot': 'BlockdevSnapshot',
'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal',
diff --git a/blockdev.c b/blockdev.c
index 6d80af903c55..d072519e7b91 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2177,6 +2177,63 @@ static void 
block_dirty_bitmap_remove_commit(BlkActionState *common)
 bdrv_release_dirty_bitmap(state->bitmap);
 }

+static void block_dirty_bitmap_populate_prepare(BlkActionState *common,
+Error **errp)
+{
+BlockJobActionState *state = DO_UPCAST(BlockJobActionState, common, 
common);
+BlockDirtyBitmapPopulate *bitpop;
+BlockDriverState *bs;
+AioContext *aio_context;
+BdrvDirtyBitmap *bmap = NULL;
+int job_flags = JOB_DEFAULT;
+
+assert(common->action->type ==
+   TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_POPULATE);
+bitpop = common->action->u.block_dirty_bitmap_populate.data;
+
+bmap = block_dirty_bitmap_lookup(bitpop->node, bitpop->name, , errp);
+if (!bmap) {
+return;
+}
+
+aio_context = bdrv_get_aio_context(bs);
+aio_context_acquire(aio_context);
+state->bs = bs;
+
+/* Paired with .clean() */
+bdrv_drained_begin(state->bs);
+
+if (!bitpop->has_on_error) {
+bitpop->on_error = BLOCKDEV_ON_ERROR_REPORT;
+}
+if (!bitpop->has_auto_finalize) {
+bitpop->auto_finalize = true;
+}
+if (!bitpop->has_auto_dismiss) {
+bitpop->auto_dismiss = true;
+}
+
+if (!bitpop->auto_finalize) {
+job_flags |= JOB_MANUAL_FINALIZE;
+}
+if (!bitpop->auto_dismiss) {
+job_flags |= JOB_MANUAL_DISMISS;
+}
+
+state->job = bitpop_job_create(
+bitpop->job_id,
+bs,
+bmap,
+bitpop->pattern,
+bitpop->on_error,
+job_flags,
+NULL, NULL,
+common->block_job_txn,
+errp);
+
+aio_context_release(aio_context);
+}
+
 static void abort_prepare(BlkActionState *common, Error **errp)
 {
 error_setg(errp, "Transaction aborted using Abort action");
@@ -2260,6 +2317,13 @@ static const BlkActionOps actions[] = {
 .commit = block_dirty_bitmap_remove_commit,
 .abort = block_dirty_bitmap_remove_abort,
 },
+[TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_POPULATE] = {
+.instance_size = sizeof(BlockJobActionState),
+.prepare = block_dirty_bitmap_populate_prepare,
+.commit = blockdev_backup_commit,
+.abort = blockdev_backup_abort,
+.clean = blockdev_backup_clean,
+},
 /* Where are transactions for MIRROR, COMMIT and STREAM?
  * Although these blockjobs use transaction callbacks like the backup job,
  * these jobs do not necessarily adhere to transaction semantics.
@@ -2379,6 +2443,16 @@ void qmp_block_passwd(bool has_device, const char 
*device,
"Setting block passwords directly is no longer supported");
 }

+void qmp_block_dirty_bitmap_populate(BlockDirtyBitmapPopulate *bitpop,
+ Error **errp)
+{
+TransactionAction action = {
+.type = TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_POPULATE,
+

[PATCH v3 1/6] block: add bitmap-populate job

[Eric Blake]

From: John Snow 

This job copies the allocation map into a bitmap. It's a job because
there's no guarantee that allocation interrogation will be quick (or
won't hang), so it cannot be retrofitted into block-dirty-bitmap-merge.

It was designed with different possible population patterns in mind,
but only top layer allocation was implemented for now.

Signed-off-by: John Snow 
Signed-off-by: Eric Blake 
---
 qapi/block-core.json  |  48 +
 qapi/job.json |   6 +-
 include/block/block_int.h |  21 
 block/bitmap-populate.c   | 207 ++
 blockjob.c|   3 +-
 MAINTAINERS   |   1 +
 block/Makefile.objs   |   1 +
 7 files changed, 285 insertions(+), 2 deletions(-)
 create mode 100644 block/bitmap-populate.c

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 0e1c6a59f228..a1bcdba04423 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2211,6 +2211,54 @@
   { 'command': 'block-dirty-bitmap-merge',
 'data': 'BlockDirtyBitmapMerge' }

+##
+# @BitmapPattern:
+#
+# An enumeration of possible patterns that can be written into a bitmap.
+#
+# @allocation-top: The allocation status of the top layer
+#  of the attached storage node.
+#
+# Since: 5.1
+##
+{ 'enum': 'BitmapPattern',
+  'data': ['allocation-top'] }
+
+##
+# @BlockDirtyBitmapPopulate:
+#
+# @job-id: identifier for the newly-created block job.
+#
+# @pattern: What pattern should be written into the bitmap?
+#
+# @on-error: the action to take if an error is encountered on a bitmap's
+#attached node, default 'report'.
+#'stop' and 'enospc' can only be used if the block device supports
+#io-status (see BlockInfo).
+#
+# @auto-finalize: When false, this job will wait in a PENDING state after it 
has
+# finished its work, waiting for @block-job-finalize before
+# making any block graph changes.
+# When true, this job will automatically
+# perform its abort or commit actions.
+# Defaults to true.
+#
+# @auto-dismiss: When false, this job will wait in a CONCLUDED state after it
+#has completely ceased all work, and awaits @block-job-dismiss.
+#When true, this job will automatically disappear from the 
query
+#list without user intervention.
+#Defaults to true.
+#
+# Since: 5.1
+##
+{ 'struct': 'BlockDirtyBitmapPopulate',
+  'base': 'BlockDirtyBitmap',
+  'data': { 'job-id': 'str',
+'pattern': 'BitmapPattern',
+'*on-error': 'BlockdevOnError',
+'*auto-finalize': 'bool',
+'*auto-dismiss': 'bool' } }
+
 ##
 # @BlockDirtyBitmapSha256:
 #
diff --git a/qapi/job.json b/qapi/job.json
index 5e658281f5c4..33ff3500f794 100644
--- a/qapi/job.json
+++ b/qapi/job.json
@@ -19,10 +19,14 @@
 #
 # @create: image creation job type, see "blockdev-create" (since 3.0)
 #
+# @bitmap-populate: drive bitmap population job type, see
+#   "block-dirty-bitmap-populate" (since 5.1)
+#
 # Since: 1.7
 ##
 { 'enum': 'JobType',
-  'data': ['commit', 'stream', 'mirror', 'backup', 'create'] }
+  'data': ['commit', 'stream', 'mirror', 'backup', 'create',
+   'bitmap-populate'] }

 ##
 # @JobStatus:
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 791de6a59c2c..93fb886e7e97 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -1231,6 +1231,27 @@ BlockJob *backup_job_create(const char *job_id, 
BlockDriverState *bs,
 BlockCompletionFunc *cb, void *opaque,
 JobTxn *txn, Error **errp);

+/*
+ * bitpop_job_create: Create a new bitmap population job.
+ *
+ * @job_id: The id of the newly-created job.
+ * @bs: Block device associated with the @target_bitmap.
+ * @target_bitmap: The bitmap to populate.
+ * @on_error: What to do if an error on @bs is encountered.
+ * @creation_flags: Flags that control the behavior of the Job lifetime.
+ *  See @BlockJobCreateFlags
+ * @cb: Completion function for the job.
+ * @opaque: Opaque pointer value passed to @cb.
+ * @txn: Transaction that this job is part of (may be NULL).
+ */
+BlockJob *bitpop_job_create(const char *job_id, BlockDriverState *bs,
+BdrvDirtyBitmap *target_bitmap,
+BitmapPattern pattern,
+BlockdevOnError on_error,
+int creation_flags,
+BlockCompletionFunc *cb, void *opaque,
+JobTxn *txn, Error **errp);
+
 BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,
   const char *child_name,
   const BdrvChildClass *child_class,
diff --git a/block/bitmap-populate.c b/block/bitmap-populate.c
new file mode 100644
index 

[PATCH v3 0/6] block: add block-dirty-bitmap-populate job

[Eric Blake]

[From John's original cover letter:]
This is a new (very small) block job that writes a pattern into a
bitmap. The only pattern implemented is the top allocation information.

This can be used to "recover" an incremental bitmap chain if an external
snapshot was taken without creating a new bitmap first: any writes made
to the image will be reflected by the allocation status and can be
written back into a bitmap.

This is useful for e.g. libvirt managing backup chains if a user creates
an external snapshot outside of libvirt.

v3:
 - Addressed a bit more feedback
 - Make it easier to decide if we want an x- prefix if we think there
 are more tweaks to be made to the interface
 - Drop dependency on John's JobRunner iotest series
 - Renumber the new iotest

I know there was a lot of discussion about whether there are
optimizations to be made with populating directly into the target
bitmap rather than into a temporary that then gets merged in at the
completion of the job, but the QMP aspect seems fairly stable.  Even
so, we may still want to consider using an x- prefix until we know for
sure whether libvirt can make decent use of the interface.

Eric Blake (1):
  bitmaps: Use x- prefix for block-dirty-bitmap-popluate

John Snow (5):
  block: add bitmap-populate job
  blockdev: combine DriveBackupState and BlockdevBackupState
  qmp: expose block-dirty-bitmap-populate
  iotests: move bitmap helpers into their own file
  iotests: add 298 for block-dirty-bitmap-populate

 qapi/block-core.json  |   66 +
 qapi/job.json |6 +-
 qapi/transaction.json |2 +
 include/block/block_int.h |   21 +
 block/bitmap-populate.c   |  207 ++
 blockdev.c|  104 +-
 blockjob.c|3 +-
 MAINTAINERS   |1 +
 block/Makefile.objs   |1 +
 tests/qemu-iotests/257|  110 +-
 tests/qemu-iotests/298|  232 ++
 tests/qemu-iotests/298.out| 4544 +
 tests/qemu-iotests/bitmaps.py |  131 +
 tests/qemu-iotests/group  |1 +
 14 files changed, 5300 insertions(+), 129 deletions(-)
 create mode 100644 block/bitmap-populate.c
 create mode 100755 tests/qemu-iotests/298
 create mode 100644 tests/qemu-iotests/298.out
 create mode 100644 tests/qemu-iotests/bitmaps.py

-- 
2.27.0

Re: [PATCH v4 2/4] block/nbd.c: Add yank feature

[Lukas Straub]

On Wed, 17 Jun 2020 16:09:09 +0100
Stefan Hajnoczi  wrote:

> On Mon, May 25, 2020 at 05:44:26PM +0200, Lukas Straub wrote:
> > @@ -1395,6 +1407,15 @@ static int nbd_client_reopen_prepare(BDRVReopenState 
> > *state,
> >  return 0;
> >  }
> > 
> > +static void nbd_yank(void *opaque)
> > +{
> > +BlockDriverState *bs = opaque;
> > +BDRVNBDState *s = (BDRVNBDState *)bs->opaque;
> > +
> > +qio_channel_shutdown(QIO_CHANNEL(s->sioc), QIO_CHANNEL_SHUTDOWN_BOTH, 
> > NULL);  
> 
> qio_channel_shutdown() is not guaranteed to be thread-safe. Please
> document new assumptions that are being introduced.
> 
> Today we can more or less get away with it (although TLS sockets are a
> little iffy) because it boils down the a shutdown(2) system call. I
> think it would be okay to update the qio_channel_shutdown() and
> .io_shutdown() documentation to clarify that this is thread-safe.

Good idea, especially since the migration code already assumes this. I will do 
this in the next version.

> > +atomic_set(>state, NBD_CLIENT_QUIT);  
> 
> docs/devel/atomics.rst says:
> 
>   No barriers are implied by ``atomic_read`` and ``atomic_set`` in either 
> Linux
>   or QEMU.
> 
> Other threads might not see the latest value of s->state because this is
> a weakly ordered memory access.
> 
> I haven't audited the NBD code in detail, but if you want the other
> threads to always see NBD_CLIENT_QUIT then s->state should be set before
> calling qio_channel_shutdown() using a stronger atomics API like
> atomic_load_acquire()/atomic_store_release().

You are right, I will change that in the next version.

Thanks,
Lukas Straub


pgpaWZt0OZPuh.pgp
Description: OpenPGP digital signature

Re: [PATCH RESEND v2 2/2] nvme: allow cmb and pmr to be enabled on same device

[Andrzej Jakowski]

On 6/18/20 2:25 AM, Klaus Jensen wrote:
> On Jun 16 22:18, Andrzej Jakowski wrote:
>> So far it was not possible to have CMB and PMR emulated on the same
>> device, because BAR2 was used exclusively either of PMR or CMB. This
>> patch places CMB at BAR4 offset so it not conflicts with MSI-X vectors.
>>
>> Signed-off-by: Andrzej Jakowski 
>> ---
>>  hw/block/nvme.c  | 122 ---
>>  hw/block/nvme.h  |   5 +-
>>  include/block/nvme.h |   4 +-
>>  3 files changed, 86 insertions(+), 45 deletions(-)
>>
>> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
>> index 9f11f3e9da..62681966b9 100644
>> --- a/hw/block/nvme.c
>> +++ b/hw/block/nvme.c
>> @@ -22,12 +22,12 @@
>>   *  [pmrdev=,] \
>>   *  max_ioqpairs=
>>   *
>> - * Note cmb_size_mb denotes size of CMB in MB. CMB is assumed to be at
>> - * offset 0 in BAR2 and supports only WDS, RDS and SQS for now.
>> + * Note cmb_size_mb denotes size of CMB in MB. CMB when configured is 
>> assumed
>> + * to be resident in BAR4 at certain offset - this is because BAR4 is also
>> + * used for storing MSI-X table that is available at offset 0 in BAR4.
> 
> I would still really like a R-b by a more PCI-competent reviewer to
> ensure that it is sane to have the MSI-X table here in prefetchable
> 64-bit address space.

Having Reviewed-by for that make sense to me. And let me offer some
evidences of real devices having MSI-X in prefetchable region and 
non-prefetchable region. Based on those examples I don't think it matters
where you place your MSI-X vector.

Why do you think it may not be sane to place MSI-x table in prefetchable
region?

Device with MSI-X in non-prefetchable region:
Region 0: Memory at fb42c000 (64-bit, non-prefetchable) [size=16K]
Capabilities: [80] MSI-X: Enable+ Count=1 Masked-
Vector table: BAR=0 offset=2000
PBA: BAR=0 offset=3000

Device with MSI-X in prefetchable region
Region 0: Memory at fbc0 (64-bit, prefetchable) [size=2M]
Region 2: I/O ports at e020 [size=32]
Region 4: Memory at fbe04000 (64-bit, prefetchable) [size=16K]
Capabilities: [40] Power Management version 3
Flags: PMEClk- DSI+ D1- D2- AuxCurrent=0mA 
PME(D0+,D1-,D2-,D3hot+,D3cold+)
Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=1 PME-
Capabilities: [50] MSI: Enable- Count=1/1 Maskable+ 64bit+
Address:   Data: 
Masking:   Pending: 
Capabilities: [70] MSI-X: Enable+ Count=64 Masked-
Vector table: BAR=4 offset=
PBA: BAR=4 offset=2000



> 
>>   *
>> - * cmb_size_mb= and pmrdev= options are mutually exclusive due to limitation
>> - * in available BAR's. cmb_size_mb= will take precedence over pmrdev= when
>> - * both provided.
>> + * pmrdev is assumed to be resident in BAR2/BAR3. When configured it 
>> consumes
>> + * whole BAR2/BAR3 exclusively.
>>   * Enabling pmr emulation can be achieved by pointing to 
>> memory-backend-file.
>>   * For example:
>>   * -object memory-backend-file,id=,share=on,mem-path=, \
>> @@ -69,18 +69,19 @@
>>  
>>  static void nvme_process_sq(void *opaque);
>>  
>> -static bool nvme_addr_is_cmb(NvmeCtrl *n, hwaddr addr)
>> +static bool nvme_addr_is_cmb(NvmeCtrl *n, hwaddr addr, int size)
>>  {
>> -hwaddr low = n->ctrl_mem.addr;
>> -hwaddr hi  = n->ctrl_mem.addr + int128_get64(n->ctrl_mem.size);
>> +hwaddr low = n->bar4.addr + n->cmb_offset;
>> +hwaddr hi  = low + NVME_CMBSZ_GETSIZE(n->bar.cmbsz);
> 
> Isn't it better to use n->bar4.size?

My understanding is that cmb doesn't necessarily need to occupy whole BAR,
which is required to be power-of-two in size.

> 
>>  
>> -return addr >= low && addr < hi;
>> +return addr >= low && (addr + size) < hi;
>>  }
> 
> I think nvme_addr_is_cmb should do what it says on the tin - that is,
> check that addr is within the CMB. The size check belongs in
> nvme_addr_read.

Yep, that was my intention. New check confirms that start and end of requested
addresses are within CMB range. While previous code only checked start address.
You may end up with situation where end adress could be outside of CMB range 
while
start in CMB range.

> 
>>  
>>  static void nvme_addr_read(NvmeCtrl *n, hwaddr addr, void *buf, int size)
>>  {
>> -if (n->bar.cmbsz && nvme_addr_is_cmb(n, addr)) {
>> -memcpy(buf, (void *)>cmbuf[addr - n->ctrl_mem.addr], size);
>> +hwaddr cmb_addr = n->bar4.addr + n->cmb_offset;
>> +if (n->cmbsz && nvme_addr_is_cmb(n, addr, size)) {
>> +memcpy(buf, (void *)>cmbuf[addr - cmb_addr], size);
>>  return;
>>  }
>>  
>> @@ -167,17 +168,18 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg, 
>> QEMUIOVector *iov, uint64_t prp1,
>>   uint64_t prp2, uint32_t len, NvmeCtrl *n)
>>  {
>>  hwaddr trans_len = n->page_size - (prp1 % n->page_size);
>> 

Re: [PATCH 0/7] python: create installable package

[John Snow]

On 6/19/20 12:44 PM, Kevin Wolf wrote:
> Am 19.06.2020 um 17:04 hat John Snow geschrieben:
>> On 6/18/20 5:23 AM, Kevin Wolf wrote:
>>> Am 17.06.2020 um 22:27 hat John Snow geschrieben:
> In the Avocado project, we have a `make develop` rule that does that
> for the main setup.py file, and for all plugins we carry on the same
> tree, which is similar in some regards to the "not at the project root
> directory" situation here with "qemu/python/setup.py".
>

 Ah, yeah. If we're going this far, I'd prefer using a VENV over
 modifying the user's environment. That way you can blast it all away
 with a `make distclean`.

 Maybe the "make develop" target could even use the presence of a .venv
 directory to know when it needs to make the environment or not ...
>>> [..]
 For QEMU developers, installing with develop is going to be the smart
 way to go. When your git tree is updated, your package will be updated
 along with it. You can do it once and then probably forget about it.
>>>
>>> I don't think we can make this a manual step at all. Building QEMU
>>> requires running some Python scripts (e.g. the QAPI generator), so the
>>> setup needs to be done either in configure or in a Makefile target that
>>> is specified as a dependency of any rule that would run a Python script.
>>> Building QEMU once would then be enough.
>>
>> I am imagining that we might treat "building" and "testing" separately
>> -- as it is, builds require python3.5 and tests requires 3.6 which
>> definitely necessitates two distinct environments.
> 
> I'm not sure what the exact definition of "end of life" of a distro is
> that we're using. I seem to remember that the reason for using Python
> 3.5 was Debian Stretch. Its official end of life is in about three
> weeks, but then there is still some LTS thing with reduced support done
> by a different group.
> 
> If we read our policy literally and use the regular end of life, I guess
> we could just move QEMU to 3.6 for everything.
> 

I think we have kinda-sorta-agreed to exclude the third-party LTS
support. I think we will be able to move to Python 3.6 shortly.

That'd solve one problem.

>> I will admit that I haven't constructed a full, coherent vision of
>> python management that encapsulates both building ad testing yet. For
>> example, should configure/make expect to be run inside of a venv, or
>> should they expect to create and then enter the venv? That's not clear
>> to me yet. I'm simultaneously trying to work out with Peter Maydell how
>> the sphinx dependency should work. Sphinx is presently our only python
>> dependency for our build environment.)
> 
> It's kind of obvious that this can't require user interaction because we
> want ./configure; make to work. So I guess this means the venv needs to
> be set up automatically by configure/make?
> 
>> Perhaps starting with the testing step is a good starting point and we
>> can use an implicit dependency on a `make develop` style step so it
>> happens automatically.
>>
>> (But perhaps keeping it as a standalone target that CAN be invoked
>> manually would be nice if you want to do some more intensive debugging
>> or development of new tests.)
> 
> Yes. And you'll have many dependencies on it, so it would be a separate
> target anyway.
> 
>>> Doing it automatically also means that we have to keep things local to
>>> the QEMU directory rather than installing them globally into the user
>>> directory. This is desirable anyway: Most of us deal with more than one
>>> QEMU source tree, so conflicts would be inevitable.
>>
>> I think it should be easy enough to put the VENV in the build directory
>> to prevent cross-contamination.
> 
> Sure. I'm not overly familiar with all of this, but I guess my point was
> just that a venv is needed rather than a global installation into the
> user directory. If nobody ever suggested the latter, blame the
> misunderstanding on my non-existent experience with more complex Python
> setups.
> 

Python doesn't make it easy to understand, I think.

I'll head along in this direction: We want testing to use a venv that
exists in the build directory and we want to automate its creation and
usage.

I am still working out the role of Python/VENVs at configure time with
Peter Maydell in another thread which may answer the other half of the
equation for me.

--js

> Kevin
> 

Re: [PATCH v4 1/4] Introduce yank feature

[Lukas Straub]

On Fri, 19 Jun 2020 17:52:40 +0100
Daniel P. Berrangé  wrote:

> On Fri, Jun 19, 2020 at 06:29:24PM +0200, Lukas Straub wrote:
> > On Wed, 17 Jun 2020 16:12:40 +0100
> > Stefan Hajnoczi  wrote:
> >   
> > > On Mon, May 25, 2020 at 05:44:23PM +0200, Lukas Straub wrote:  
> > > > +static struct YankInstance *yank_find_instance(char *name)
> > > 
> > > There are const char * -> char * casts in later patches. Please use
> > > const char * where possible. Callers shouldn't need to cast away const.  
> > 
> > nbd and chardev generate the instance name dynamically so it
> > needs to be char *, but in migration it's hardcoded.  
> 
> I think you're looking at it from the wrong perspective.
> 
> The yank_find_instance() method never modifies the 'name' paramater
> that it receives. Therefore it should be "const char *". Likewise
> for the other yank_*()  methods in fact.
> 
> The caller can have a char *, or a const char * as suits its needs.
> Either can be passed into the yank_* methods and will gain const-ness
> from the POV of yank code.

Makes sense, I will change it in the next version.

Thanks,
Lukas Straub

> 
> Regards,
> Daniel



pgphQiooTBERI.pgp
Description: OpenPGP digital signature

Re: [PATCH v4 1/4] Introduce yank feature

[Daniel P . Berrangé]

On Fri, Jun 19, 2020 at 04:23:50PM +0200, Lukas Straub wrote:
> On Tue, 16 Jun 2020 15:39:57 +0100
> Daniel P. Berrangé  wrote:
> 
> > On Mon, May 25, 2020 at 05:44:23PM +0200, Lukas Straub wrote:
> > > The yank feature allows to recover from hanging qemu by "yanking"
> > > at various parts. Other qemu systems can register themselves and
> > > multiple yank functions. Then all yank functions for selected
> > > instances can be called by the 'yank' out-of-band qmp command.
> > > Available instances can be queried by a 'query-yank' oob command.
> > > 
> > > Signed-off-by: Lukas Straub 
> > > ---
> > >  qapi/misc.json |  45 +
> > >  yank.c | 174 +
> > >  yank.h |  67 +++
> > >  3 files changed, 286 insertions(+)
> > >  create mode 100644 yank.c
> > >  create mode 100644 yank.h  
> > 
> > > +void yank_register_function(char *instance_name, YankFn *func, void 
> > > *opaque)
> > > +{
> > > +struct YankInstance *instance;
> > > +struct YankFuncAndParam *entry;
> > > +
> > > +qemu_mutex_lock();
> > > +instance = yank_find_instance(instance_name);
> > > +assert(instance);
> > > +
> > > +entry = g_slice_new(struct YankFuncAndParam);
> > > +entry->func = func;
> > > +entry->opaque = opaque;
> > > +
> > > +QLIST_INSERT_HEAD(>yankfns, entry, next);
> > > +qemu_mutex_unlock();
> > > +}
> > > +
> > > +void yank_unregister_function(char *instance_name, YankFn *func, void 
> > > *opaque)
> > > +{
> > > +struct YankInstance *instance;
> > > +struct YankFuncAndParam *entry;
> > > +
> > > +qemu_mutex_lock();
> > > +instance = yank_find_instance(instance_name);
> > > +assert(instance);
> > > +
> > > +QLIST_FOREACH(entry, >yankfns, next) {
> > > +if (entry->func == func && entry->opaque == opaque) {
> > > +QLIST_REMOVE(entry, next);
> > > +g_slice_free(struct YankFuncAndParam, entry);
> > > +qemu_mutex_unlock();
> > > +return;
> > > +}
> > > +}
> > > +
> > > +abort();
> > > +}  
> > 
> > Since the NBD impl no longer needs to register multiple different functions
> > on the same insance_nane, these methods could be be simplified, to only
> > accept a single function, instead of keeping a whole list. This would avoid
> > need to pass a function into the unregister() method at all.
> 
> Multiple yank functions are still needed for multifd migration.

Oh I missed that subtlety, so fine to ignore my suggestion.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [PATCH v4 1/4] Introduce yank feature

[Daniel P . Berrangé]

On Fri, Jun 19, 2020 at 06:29:24PM +0200, Lukas Straub wrote:
> On Wed, 17 Jun 2020 16:12:40 +0100
> Stefan Hajnoczi  wrote:
> 
> > On Mon, May 25, 2020 at 05:44:23PM +0200, Lukas Straub wrote:
> > > +static struct YankInstance *yank_find_instance(char *name)  
> > 
> > There are const char * -> char * casts in later patches. Please use
> > const char * where possible. Callers shouldn't need to cast away const.
> 
> nbd and chardev generate the instance name dynamically so it
> needs to be char *, but in migration it's hardcoded.

I think you're looking at it from the wrong perspective.

The yank_find_instance() method never modifies the 'name' paramater
that it receives. Therefore it should be "const char *". Likewise
for the other yank_*()  methods in fact.

The caller can have a char *, or a const char * as suits its needs.
Either can be passed into the yank_* methods and will gain const-ness
from the POV of yank code.


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [PATCH 1/2] qcow2: Force preallocation with data-file-raw

[Alberto Garcia]

On Fri 19 Jun 2020 12:40:11 PM CEST, Max Reitz wrote:
> +if (qcow2_opts->data_file_raw &&
> +qcow2_opts->preallocation == PREALLOC_MODE_OFF)
> +{
> +/*
> + * data-file-raw means that "the external data file can be
> + * read as a consistent standalone raw image without looking
> + * at the qcow2 metadata."  It does not say that the metadata
> + * must be ignored, though (and the qcow2 driver in fact does
> + * not ignore it), so the L1/L2 tables must be present and
> + * give a 1:1 mapping, so you get the same result regardless
> + * of whether you look at the metadata or whether you ignore
> + * it.
> + */
> +qcow2_opts->preallocation = PREALLOC_MODE_METADATA;

I'm not convinced by this, but your comment made me think of another
possible alternative: in qcow2_get_cluster_offset(), if the cluster is
unallocated and we are using a raw data file then we return _ZERO_PLAIN:

--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -654,6 +654,10 @@ out:
 assert(bytes_available - offset_in_cluster <= UINT_MAX);
 *bytes = bytes_available - offset_in_cluster;
 
+if (type == QCOW2_CLUSTER_UNALLOCATED && data_file_is_raw(bs)) {
+type = QCOW2_CLUSTER_ZERO_PLAIN;
+}
+
 return type;

You could even add a '&& bs->backing' to the condition and emit a
warning to make it more explicit.

Berto

Re: [PATCH 0/7] python: create installable package

[Kevin Wolf]

Am 19.06.2020 um 17:04 hat John Snow geschrieben:
> On 6/18/20 5:23 AM, Kevin Wolf wrote:
> > Am 17.06.2020 um 22:27 hat John Snow geschrieben:
> >>> In the Avocado project, we have a `make develop` rule that does that
> >>> for the main setup.py file, and for all plugins we carry on the same
> >>> tree, which is similar in some regards to the "not at the project root
> >>> directory" situation here with "qemu/python/setup.py".
> >>>
> >>
> >> Ah, yeah. If we're going this far, I'd prefer using a VENV over
> >> modifying the user's environment. That way you can blast it all away
> >> with a `make distclean`.
> >>
> >> Maybe the "make develop" target could even use the presence of a .venv
> >> directory to know when it needs to make the environment or not ...
> > [..]
> >> For QEMU developers, installing with develop is going to be the smart
> >> way to go. When your git tree is updated, your package will be updated
> >> along with it. You can do it once and then probably forget about it.
> > 
> > I don't think we can make this a manual step at all. Building QEMU
> > requires running some Python scripts (e.g. the QAPI generator), so the
> > setup needs to be done either in configure or in a Makefile target that
> > is specified as a dependency of any rule that would run a Python script.
> > Building QEMU once would then be enough.
> 
> I am imagining that we might treat "building" and "testing" separately
> -- as it is, builds require python3.5 and tests requires 3.6 which
> definitely necessitates two distinct environments.

I'm not sure what the exact definition of "end of life" of a distro is
that we're using. I seem to remember that the reason for using Python
3.5 was Debian Stretch. Its official end of life is in about three
weeks, but then there is still some LTS thing with reduced support done
by a different group.

If we read our policy literally and use the regular end of life, I guess
we could just move QEMU to 3.6 for everything.

> I will admit that I haven't constructed a full, coherent vision of
> python management that encapsulates both building ad testing yet. For
> example, should configure/make expect to be run inside of a venv, or
> should they expect to create and then enter the venv? That's not clear
> to me yet. I'm simultaneously trying to work out with Peter Maydell how
> the sphinx dependency should work. Sphinx is presently our only python
> dependency for our build environment.)

It's kind of obvious that this can't require user interaction because we
want ./configure; make to work. So I guess this means the venv needs to
be set up automatically by configure/make?

> Perhaps starting with the testing step is a good starting point and we
> can use an implicit dependency on a `make develop` style step so it
> happens automatically.
> 
> (But perhaps keeping it as a standalone target that CAN be invoked
> manually would be nice if you want to do some more intensive debugging
> or development of new tests.)

Yes. And you'll have many dependencies on it, so it would be a separate
target anyway.

> > Doing it automatically also means that we have to keep things local to
> > the QEMU directory rather than installing them globally into the user
> > directory. This is desirable anyway: Most of us deal with more than one
> > QEMU source tree, so conflicts would be inevitable.
> 
> I think it should be easy enough to put the VENV in the build directory
> to prevent cross-contamination.

Sure. I'm not overly familiar with all of this, but I guess my point was
just that a venv is needed rather than a global installation into the
user directory. If nobody ever suggested the latter, blame the
misunderstanding on my non-existent experience with more complex Python
setups.

Kevin

Re: [PATCH 0/2] block: propagate discard alignment from format drivers to the guest

[Denis V. Lunev]

On 6/19/20 7:20 PM, Eduardo Habkost wrote:
> On Thu, Jun 11, 2020 at 08:16:06PM +0300, Denis V. Lunev wrote:
>> Nowaday SCSI drivers in guests are able to align UNMAP requests before
>> sending to the device. Right now QEMU provides an ability to set
>> this via "discard_granularity" property of the block device which could
>> be used by management layer.
>>
>> Though, in particular, from the point of QEMU, there is
>> pdiscard_granularity on the format driver level, f.e. on QCOW2 or iSCSI.
>> It would be beneficial to pass this value as a default for this
>> property.
> I assume the value is visible to the guest.  What is supposed to
> happen if live migrating and the block backend is a different one
> on the destination?
>
> Also, don't we have mechanisms to change the block backend change
> at run time?  What should happen in that case?
First of all, I think that this should be very rare case.
Though nothing bad is expected. The quest will see
old value, i.e. one negotiated at guest startup.

Let us assume that block backend has been changed
and discard alignment is
- less than set. In this case the guest will continue to
  send larger than possible requests, i.e. some blocks
  will not be discarded as it could happen. This will
  happen until the guest restarts and see smaller
  alignment. First re-trim will discard all non-discarded
  so far blocks
- greater than set. The code will work like now, i.e.
  some extra requests will be sent.

Den

Re: [PATCH v4 1/4] Introduce yank feature

[Lukas Straub]

On Wed, 17 Jun 2020 16:12:40 +0100
Stefan Hajnoczi  wrote:

> On Mon, May 25, 2020 at 05:44:23PM +0200, Lukas Straub wrote:
> > +static struct YankInstance *yank_find_instance(char *name)  
> 
> There are const char * -> char * casts in later patches. Please use
> const char * where possible. Callers shouldn't need to cast away const.

nbd and chardev generate the instance name dynamically so it needs to be char 
*, but in migration it's hardcoded.

Thanks,
Lukas Straub


pgpLRvpG4Txdl.pgp
Description: OpenPGP digital signature

Re: [PATCH v2 1/4] Introduce yank feature

[Lukas Straub]

On Wed, 17 Jun 2020 15:39:36 +0100
Stefan Hajnoczi  wrote:

> On Thu, May 21, 2020 at 04:48:06PM +0100, Daniel P. Berrangé wrote:
> > On Thu, May 21, 2020 at 05:42:41PM +0200, Lukas Straub wrote:  
> > > On Thu, 21 May 2020 16:03:35 +0100
> > > Stefan Hajnoczi  wrote:
> > >   
> > > > On Wed, May 20, 2020 at 11:05:39PM +0200, Lukas Straub wrote:  
> > > > > +void yank_generic_iochannel(void *opaque)
> > > > > +{
> > > > > +QIOChannel *ioc = QIO_CHANNEL(opaque);
> > > > > +
> > > > > +qio_channel_shutdown(ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
> > > > > +}
> > > > > +
> > > > > +void qmp_yank(strList *instances, Error **errp)
> > > > > +{
> > > > > +strList *tmp;
> > > > > +struct YankInstance *instance;
> > > > > +struct YankFuncAndParam *entry;
> > > > > +
> > > > > +qemu_mutex_lock();
> > > > > +tmp = instances;
> > > > > +for (; tmp; tmp = tmp->next) {
> > > > > +instance = yank_find_instance(tmp->value);
> > > > > +if (!instance) {
> > > > > +error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
> > > > > +  "Instance '%s' not found", tmp->value);
> > > > > +qemu_mutex_unlock();
> > > > > +return;
> > > > > +}
> > > > > +}
> > > > > +tmp = instances;
> > > > > +for (; tmp; tmp = tmp->next) {
> > > > > +instance = yank_find_instance(tmp->value);
> > > > > +assert(instance);
> > > > > +QLIST_FOREACH(entry, >yankfns, next) {
> > > > > +entry->func(entry->opaque);
> > > > > +}
> > > > > +}
> > > > > +qemu_mutex_unlock();
> > > > > +}
> > > > 
> > > > From docs/devel/qapi-code-gen.txt:
> > > > 
> > > >   An OOB-capable command handler must satisfy the following conditions:
> > > > 
> > > >   - It terminates quickly.  
> > > Check.
> > >   
> > > >   - It does not invoke system calls that may block.  
> > > brk/sbrk (malloc and friends):
> > > The manpage doesn't say anything about blocking, but malloc is already 
> > > used while handling the qmp command.
> > > 
> > > shutdown():
> > > The manpage doesn't say anything about blocking, but this is already used 
> > > in migration oob qmp commands.  
> > 
> > It just marks the socket state in local kernel side. It doesn't involve
> > any blocking roundtrips over the wire, so this is fine.
> >   
> > > 
> > > There are no other syscalls involved to my knowledge.
> > >   
> > > >   - It does not access guest RAM that may block when userfaultfd is
> > > > enabled for postcopy live migration.  
> > > Check.
> > >   
> > > >   - It takes only "fast" locks, i.e. all critical sections protected by
> > > > any lock it takes also satisfy the conditions for OOB command
> > > > handler code.  
> > > 
> > > The lock in yank.c satisfies this requirement.
> > > 
> > > qio_channel_shutdown doesn't take any locks.  
> > 
> > Agreed, I think the yank code is compliant with all the points
> > listed above.  
> 
> The code does not document this in any way. In fact, it's an interface
> for registering arbitrary callback functions which execute in this
> special environment.
> 
> If you don't document this explicitly it will break when someone changes
> the code, even if it works right now.
> 
> Please document this in the yank APIs and also in the implementation.

Hi,
It is documented in yank.h:

/**
 * yank_register_function: Register a yank function
 *
 * This registers a yank function. All limitations of qmp oob commands apply
 * to the yank function as well.
 *
 * This function is thread-safe.
 *
 * @instance_name: The name of the instance
 * @func: The yank function
 * @opaque: Will be passed to the yank function
 */

Thanks,
Lukas Straub

> For example, QemuMutex yank has the priority inversion problem: no other
> function may violate the oob rules while holding the mutex, otherwise
> the oob function will hang while waiting for the lock when the other
> function is blocked.
> 
> Stefan



pgpwKMtGXuUKI.pgp
Description: OpenPGP digital signature

Re: [PATCH v4 2/4] block/nbd.c: Add yank feature

[Lukas Straub]

On Tue, 16 Jun 2020 15:44:06 +0100
Daniel P. Berrangé  wrote:

> On Mon, May 25, 2020 at 05:44:26PM +0200, Lukas Straub wrote:
> > Register a yank function which shuts down the socket and sets
> > s->state = NBD_CLIENT_QUIT. This is the same behaviour as if an
> > error occured.
> > 
> > Signed-off-by: Lukas Straub 
> > ---
> >  Makefile.objs |   1 +
> >  block/nbd.c   | 101 --
> >  2 files changed, 65 insertions(+), 37 deletions(-)
> > 
> > diff --git a/Makefile.objs b/Makefile.objs
> > index a7c967633a..8e403b81f3 100644
> > --- a/Makefile.objs
> > +++ b/Makefile.objs
> > @@ -18,6 +18,7 @@ block-obj-y += block.o blockjob.o job.o
> >  block-obj-y += block/ scsi/
> >  block-obj-y += qemu-io-cmds.o
> >  block-obj-$(CONFIG_REPLICATION) += replication.o
> > +block-obj-y += yank.o  
> 
> Oh, I see this is repeated for migration and chardev code too.
> 
> Instead of doing this and relying on linker to merge duplicates,
> I think we should put yank.c into util/ and built it into util-obj-y,
> so it gets added to everything.

Ok, I will do this in the next version.

Thanks,
Lukas Straub

> Regards,
> Daniel



pgpcCKUVeaDXO.pgp
Description: OpenPGP digital signature

Re: [PATCH 0/2] block: propagate discard alignment from format drivers to the guest

[Eduardo Habkost]

On Thu, Jun 11, 2020 at 08:16:06PM +0300, Denis V. Lunev wrote:
> Nowaday SCSI drivers in guests are able to align UNMAP requests before
> sending to the device. Right now QEMU provides an ability to set
> this via "discard_granularity" property of the block device which could
> be used by management layer.
> 
> Though, in particular, from the point of QEMU, there is
> pdiscard_granularity on the format driver level, f.e. on QCOW2 or iSCSI.
> It would be beneficial to pass this value as a default for this
> property.

I assume the value is visible to the guest.  What is supposed to
happen if live migrating and the block backend is a different one
on the destination?

Also, don't we have mechanisms to change the block backend change
at run time?  What should happen in that case?

> 
> Technically this should reduce the amount of use less UNMAP requests
> from the guest to the host. Basic test confirms this. Fedora 31 guest
> during 'fstrim /' on 32 Gb disk has issued 401/415 requests with/without
> proper alignment to QEMU.
> 
> Changes from v2:
> - 172 iotest fixed
> 
> Changes from v1:
> - fixed typos in description
> - added machine type compatibility layer as suggested by Kevin
> 
> Signed-off-by: Denis V. Lunev 
> CC: Kevin Wolf 
> CC: Max Reitz 
> CC: Eduardo Habkost 
> CC: Marcel Apfelbaum 
> CC: John Snow 
> CC: Paolo Bonzini 
> CC: Fam Zheng 
> 
> 

-- 
Eduardo

Re: [PATCH 0/7] python: create installable package

[John Snow]

On 6/19/20 11:15 AM, Philippe Mathieu-Daudé wrote:
> On 6/17/20 10:27 PM, John Snow wrote:
>>
>>
>> On 6/17/20 3:52 PM, Cleber Rosa wrote:
>>> On Tue, Jun 02, 2020 at 08:15:16PM -0400, John Snow wrote:
> [...]
>>> Are you proposing that we have, say, "python-qemu" version 10, being
>>> the 10th API version, without any regard to the QEMU version
>>> supported?  Or version 10.5.3 would mean 10th API version, intended
>>> to support QEMU 5.3?
>>>
>>
>> I am proposing only that we use semver to track the API version of the
>> SDK itself.
>>
>> So that could be:
>>
>> A) 1.x, 2.x, 3.x (etc) with absolutely no connection to the intended
>> QEMU support version. It either works or it doesn't. It might not work
>> very spectacularly. Major semver bumps indicate a breaking change to the
>> library API.
> 
> Major changes occurs between QEMU releases. If there is no QEMU release,
> it is pointless to release the python-qemu package, right?
> 

There might be fixes to the package that might be worth releasing
out-of-band as point fixes. I don't intend to do this if I can help it,
but I wanted to consider the possibility that unforeseen circumstances
might force our hand.

>>
>> B) 1.5.0.0, 1.5.1.0, 1.5.2.0 (etc) where the major version still
>> describes the API, but the remainder of the version describes the
>> intended target QEMU.
>>
>> Or, we could do:
>>
>> C) 5.0.0, 5.1.0, 5.2.0, etc. where it tracks the QEMU version verbatim,
>> end of story.
> 
> At least it KISS.
> 

Simple at a glance.

I have some concerns about how Python packages are normally specified in
e.g. requirements.txt where there's a habit of saying:

package>=3.0.0, <4.0.0

There is a fairly common belief in the ecosystem that semver is being
used. QEMU does not use semver.

This leads us to a strange development paradigm in-tree where the Python
package can have breaking changes from 5.x to 6.x, which are not
otherwise special releases for QEMU itself.

Combined with our deprecation policy, it means that we start adopting a
policy like:

- Features get deprecated for at least two releases
- But can only be changed for a new "major" release.

That mismatch against the QEMU versioning paradigm does not sound like
KISS to me.

>>
>> I don't like (C) very much, because it violates some prevailing idioms
>> about python package versioning. A or B seem better, but do run us into
>> potential trouble with people having mismatched versions.
> 
> Which is why I prefer (C).
> 

Keep in mind that even though it looks more obvious, it still doesn't
enforce the pairing. Problems with mismatched versions are just as
likely to occur because of misconfigurations with requirements.txt.

I guess my big concerns here are:

1. Using 1:1 QEMU versions (starting at 5.x) might imply API stability
for the package, and I would like to avoid committing to that. We *can*
declare the package as Alpha/Beta in PyPI, but in practice I am not sure
that information is consulted as readily as version numbers are.

2. Python world (Citation Needed?) expects semver. We can ignore that if
we choose; there aren't semver police. What are the consequences of
doing that?

3. No matter what we do, the relationship between the Python package and
the QEMU version is only superficial and isn't enforced anywhere. (And,
I think, shouldn't be enforced.)

>>
>> I'd take A or B. (B) is a little chatty but gives some good information
>> and allows you to pin versions effectively, so I think I'm leaning
>> towards that one right now.
>>
>> Well, whatever we do right now, I think I do really want to make sure we
>> are publishing under 0.x to really give the illustration that we are NOT
>> promising even the illusion of stability right now.
> 

It sounds like Avocado might be one of the biggest users of this, so I'd
like to get some more feedback from Cleber, Wainer, et al.

--js

Re: [PATCH 0/7] python: create installable package

[Philippe Mathieu-Daudé]

On 6/17/20 10:27 PM, John Snow wrote:
> 
> 
> On 6/17/20 3:52 PM, Cleber Rosa wrote:
>> On Tue, Jun 02, 2020 at 08:15:16PM -0400, John Snow wrote:
[...]
>> Are you proposing that we have, say, "python-qemu" version 10, being
>> the 10th API version, without any regard to the QEMU version
>> supported?  Or version 10.5.3 would mean 10th API version, intended
>> to support QEMU 5.3?
>>
> 
> I am proposing only that we use semver to track the API version of the
> SDK itself.
> 
> So that could be:
> 
> A) 1.x, 2.x, 3.x (etc) with absolutely no connection to the intended
> QEMU support version. It either works or it doesn't. It might not work
> very spectacularly. Major semver bumps indicate a breaking change to the
> library API.

Major changes occurs between QEMU releases. If there is no QEMU release,
it is pointless to release the python-qemu package, right?

> 
> B) 1.5.0.0, 1.5.1.0, 1.5.2.0 (etc) where the major version still
> describes the API, but the remainder of the version describes the
> intended target QEMU.
> 
> Or, we could do:
> 
> C) 5.0.0, 5.1.0, 5.2.0, etc. where it tracks the QEMU version verbatim,
> end of story.

At least it KISS.

> 
> I don't like (C) very much, because it violates some prevailing idioms
> about python package versioning. A or B seem better, but do run us into
> potential trouble with people having mismatched versions.

Which is why I prefer (C).

> 
> I'd take A or B. (B) is a little chatty but gives some good information
> and allows you to pin versions effectively, so I think I'm leaning
> towards that one right now.
> 
> Well, whatever we do right now, I think I do really want to make sure we
> are publishing under 0.x to really give the illustration that we are NOT
> promising even the illusion of stability right now.

Re: [PATCH 0/7] python: create installable package

[Philippe Mathieu-Daudé]

On 6/18/20 11:23 AM, Kevin Wolf wrote:
> Am 17.06.2020 um 22:27 hat John Snow geschrieben:
>>> In the Avocado project, we have a `make develop` rule that does that
>>> for the main setup.py file, and for all plugins we carry on the same
>>> tree, which is similar in some regards to the "not at the project root
>>> directory" situation here with "qemu/python/setup.py".
>>>
>>
>> Ah, yeah. If we're going this far, I'd prefer using a VENV over
>> modifying the user's environment. That way you can blast it all away
>> with a `make distclean`.
>>
>> Maybe the "make develop" target could even use the presence of a .venv
>> directory to know when it needs to make the environment or not ...
> [..]
>> For QEMU developers, installing with develop is going to be the smart
>> way to go. When your git tree is updated, your package will be updated
>> along with it. You can do it once and then probably forget about it.
> 
> I don't think we can make this a manual step at all. Building QEMU
> requires running some Python scripts (e.g. the QAPI generator), so the
> setup needs to be done either in configure or in a Makefile target that
> is specified as a dependency of any rule that would run a Python script.
> Building QEMU once would then be enough.
> 
> Doing it automatically also means that we have to keep things local to
> the QEMU directory rather than installing them globally into the user
> directory. This is desirable anyway: Most of us deal with more than one
> QEMU source tree, so conflicts would be inevitable.

Indeed. Each of the source tree I use has its own virtual environment.
I personally stopped using the distribution packages, they don't make
sense when you develop, the tree changes too quickly.

Distributions use stable releases, so IMO it only makes sense to
generate a package along with releases. Else use venv.

Re: [PATCH 0/7] python: create installable package

[John Snow]

On 6/18/20 5:23 AM, Kevin Wolf wrote:
> Am 17.06.2020 um 22:27 hat John Snow geschrieben:
>>> In the Avocado project, we have a `make develop` rule that does that
>>> for the main setup.py file, and for all plugins we carry on the same
>>> tree, which is similar in some regards to the "not at the project root
>>> directory" situation here with "qemu/python/setup.py".
>>>
>>
>> Ah, yeah. If we're going this far, I'd prefer using a VENV over
>> modifying the user's environment. That way you can blast it all away
>> with a `make distclean`.
>>
>> Maybe the "make develop" target could even use the presence of a .venv
>> directory to know when it needs to make the environment or not ...
> [..]
>> For QEMU developers, installing with develop is going to be the smart
>> way to go. When your git tree is updated, your package will be updated
>> along with it. You can do it once and then probably forget about it.
> 
> I don't think we can make this a manual step at all. Building QEMU
> requires running some Python scripts (e.g. the QAPI generator), so the
> setup needs to be done either in configure or in a Makefile target that
> is specified as a dependency of any rule that would run a Python script.
> Building QEMU once would then be enough.
> 

I am imagining that we might treat "building" and "testing" separately
-- as it is, builds require python3.5 and tests requires 3.6 which
definitely necessitates two distinct environments.

I will admit that I haven't constructed a full, coherent vision of
python management that encapsulates both building ad testing yet. For
example, should configure/make expect to be run inside of a venv, or
should they expect to create and then enter the venv? That's not clear
to me yet. I'm simultaneously trying to work out with Peter Maydell how
the sphinx dependency should work. Sphinx is presently our only python
dependency for our build environment.)

Perhaps starting with the testing step is a good starting point and we
can use an implicit dependency on a `make develop` style step so it
happens automatically.

(But perhaps keeping it as a standalone target that CAN be invoked
manually would be nice if you want to do some more intensive debugging
or development of new tests.)

> Doing it automatically also means that we have to keep things local to
> the QEMU directory rather than installing them globally into the user
> directory. This is desirable anyway: Most of us deal with more than one
> QEMU source tree, so conflicts would be inevitable.
> 

I think it should be easy enough to put the VENV in the build directory
to prevent cross-contamination.

> Kevin
> 

Re: [PATCH v4 1/4] Introduce yank feature

[Lukas Straub]

On Tue, 16 Jun 2020 15:39:57 +0100
Daniel P. Berrangé  wrote:

> On Mon, May 25, 2020 at 05:44:23PM +0200, Lukas Straub wrote:
> > The yank feature allows to recover from hanging qemu by "yanking"
> > at various parts. Other qemu systems can register themselves and
> > multiple yank functions. Then all yank functions for selected
> > instances can be called by the 'yank' out-of-band qmp command.
> > Available instances can be queried by a 'query-yank' oob command.
> > 
> > Signed-off-by: Lukas Straub 
> > ---
> >  qapi/misc.json |  45 +
> >  yank.c | 174 +
> >  yank.h |  67 +++
> >  3 files changed, 286 insertions(+)
> >  create mode 100644 yank.c
> >  create mode 100644 yank.h  
> 
> > +void yank_register_function(char *instance_name, YankFn *func, void 
> > *opaque)
> > +{
> > +struct YankInstance *instance;
> > +struct YankFuncAndParam *entry;
> > +
> > +qemu_mutex_lock();
> > +instance = yank_find_instance(instance_name);
> > +assert(instance);
> > +
> > +entry = g_slice_new(struct YankFuncAndParam);
> > +entry->func = func;
> > +entry->opaque = opaque;
> > +
> > +QLIST_INSERT_HEAD(>yankfns, entry, next);
> > +qemu_mutex_unlock();
> > +}
> > +
> > +void yank_unregister_function(char *instance_name, YankFn *func, void 
> > *opaque)
> > +{
> > +struct YankInstance *instance;
> > +struct YankFuncAndParam *entry;
> > +
> > +qemu_mutex_lock();
> > +instance = yank_find_instance(instance_name);
> > +assert(instance);
> > +
> > +QLIST_FOREACH(entry, >yankfns, next) {
> > +if (entry->func == func && entry->opaque == opaque) {
> > +QLIST_REMOVE(entry, next);
> > +g_slice_free(struct YankFuncAndParam, entry);
> > +qemu_mutex_unlock();
> > +return;
> > +}
> > +}
> > +
> > +abort();
> > +}  
> 
> Since the NBD impl no longer needs to register multiple different functions
> on the same insance_nane, these methods could be be simplified, to only
> accept a single function, instead of keeping a whole list. This would avoid
> need to pass a function into the unregister() method at all.

Multiple yank functions are still needed for multifd migration.

> I don't consider this a blocker though, so you pick whether to do this
> or not.
> 
> 
> > diff --git a/yank.h b/yank.h
> > new file mode 100644
> > index 00..f1c8743b72
> > --- /dev/null
> > +++ b/yank.h
> > @@ -0,0 +1,67 @@
> > +  
> 
> Missing license header

I will fix that in the next version.

Thanks,
Lukas Straub

> 
> > +#ifndef YANK_H
> > +#define YANK_H
> > +
> > +typedef void (YankFn) (void *opaque);
> > +
> > +/**
> > + * yank_register_instance: Register a new instance.
> > + *
> > + * This registers a new instance for yanking. Must be called before any 
> > yank
> > + * function is registered for this instance.
> > + *
> > + * This function is thread-safe.
> > + *
> > + * @instance_name: The globally unique name of the instance.
> > + */
> > +void yank_register_instance(char *instance_name);
> > +
> > +/**
> > + * yank_unregister_instance: Unregister a instance.
> > + *
> > + * This unregisters a instance. Must be called only after every yank 
> > function
> > + * of the instance has been unregistered.
> > + *
> > + * This function is thread-safe.
> > + *
> > + * @instance_name: The name of the instance.
> > + */
> > +void yank_unregister_instance(char *instance_name);
> > +
> > +/**
> > + * yank_register_function: Register a yank function
> > + *
> > + * This registers a yank function. All limitations of qmp oob commands 
> > apply
> > + * to the yank function as well.
> > + *
> > + * This function is thread-safe.
> > + *
> > + * @instance_name: The name of the instance
> > + * @func: The yank function
> > + * @opaque: Will be passed to the yank function
> > + */
> > +void yank_register_function(char *instance_name, YankFn *func, void 
> > *opaque);
> > +
> > +/**
> > + * yank_unregister_function: Unregister a yank function
> > + *
> > + * This unregisters a yank function.
> > + *
> > + * This function is thread-safe.
> > + *
> > + * @instance_name: The name of the instance
> > + * @func: func that was passed to yank_register_function
> > + * @opaque: opaque that was passed to yank_register_function
> > + */
> > +void yank_unregister_function(char *instance_name, YankFn *func, void 
> > *opaque);
> > +
> > +/**
> > + * yank_unregister_function: Generic yank function for iochannel
> > + *
> > + * This is a generic yank function which will call qio_channel_shutdown on 
> > the
> > + * provided QIOChannel.
> > + *
> > + * @opaque: QIOChannel to shutdown
> > + */
> > +void yank_generic_iochannel(void *opaque);
> > +#endif  
> 
> 
> 
> Regards,
> Daniel



pgp_UVHIYRggx.pgp
Description: OpenPGP digital signature

Re: [PATCH 0/2] qemu-storage-daemon: memory leak and --object opts fixes

[Eric Blake]

On 6/19/20 5:11 AM, Stefan Hajnoczi wrote:

Small fixes for qemu-storage-daemon.

Stefan Hajnoczi (2):
   qemu-storage-daemon: remember to add qemu_object_opts
   qemu-storage-daemon: add missing cleanup calls


For both patches,
Reviewed-by: Eric Blake 



  qemu-storage-daemon.c | 5 +
  1 file changed, 5 insertions(+)



--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: virtio-scsi and another complex AioContext issue

[Stefan Hajnoczi]

On Thu, Jun 11, 2020 at 10:36:22AM +0200, Sergio Lopez wrote:
> Hi,
> 
> While debugging BZ#1844343, I managed to reproduce the issue which
> leads to crash with a backtrace like this one:
> 
> < snip >
> Thread 2 (Thread 0x7fe208463f00 (LWP 1659571)):
> #0  0x7fe2033b78ed in __lll_lock_wait () at /lib64/libpthread.so.0
> #1  0x7fe2033b0bd4 in pthread_mutex_lock () at /lib64/libpthread.so.0
> #2  0x560caa8f1e6d in qemu_mutex_lock_impl
> (mutex=0x560cacc68a10, file=0x560caaa9797f "util/async.c", line=521) at 
> util/qemu-thread-posix.c:78
> #3  0x560caa82414d in bdrv_set_aio_context_ignore
> (bs=bs@entry=0x560cacc73570, 
> new_context=new_context@entry=0x560cacc5fed0, 
> ignore=ignore@entry=0x7ffe388b1cc0) at block.c:6192
> #4  0x560caa824503 in bdrv_child_try_set_aio_context
> (bs=bs@entry=0x560cacc73570, ctx=0x560cacc5fed0, ignore_child= out>, errp=)
> at block.c:6272
> #5  0x560caa859e6b in blk_do_set_aio_context
> (blk=0x560cacecf370, new_context=0x560cacc5fed0, 
> update_root_node=update_root_node@entry=true, errp=errp@entry=0x0) at 
> block/block-backend.c:1989
> #6  0x560caa85c501 in blk_set_aio_context
> (blk=, new_context=, errp=errp@entry=0x0) 
> at block/block-backend.c:2010
> #7  0x560caa61db30 in virtio_scsi_hotunplug
> (hotplug_dev=0x560cadaafbd0, dev=0x560cacec1210, errp=0x7ffe388b1d80)
> at 
> /usr/src/debug/qemu-kvm-4.2.0-22.module+el8.2.1+6758+cb8d64c2.x86_64/hw/scsi/virtio-scsi.c:869
> #8  0x560caa6ccd1e in qdev_unplug (dev=0x560cacec1210, 
> errp=errp@entry=0x7ffe388b1db8)
> at qdev-monitor.c:872
> #9  0x560caa6ccd9e in qmp_device_del (id=, 
> errp=errp@entry=0x7ffe388b1db8)
> at qdev-monitor.c:884
> #10 0x560caa7ec4d3 in qmp_marshal_device_del
> (args=, ret=, errp=0x7ffe388b1e18) at 
> qapi/qapi-commands-qdev.c:99
> #11 0x560caa8a45ec in do_qmp_dispatch
> (errp=0x7ffe388b1e10, allow_oob=, request=, 
> cmds=0x560cab1928a0 ) at qapi/qmp-dispatch.c:132
> #12 0x560caa8a45ec in qmp_dispatch
> (cmds=0x560cab1928a0 , request=, 
> allow_oob=)
> at qapi/qmp-dispatch.c:175
> #13 0x560caa7c2521 in monitor_qmp_dispatch (mon=0x560cacca2f00, 
> req=)
> at monitor/qmp.c:145
> #14 0x560caa7c2bba in monitor_qmp_bh_dispatcher (data=) at 
> monitor/qmp.c:234
> #15 0x560caa8ec716 in aio_bh_call (bh=0x560cacbd80e0) at util/async.c:117
> #16 0x560caa8ec716 in aio_bh_poll (ctx=ctx@entry=0x560cacbd6da0) at 
> util/async.c:117
> #17 0x560caa8efb04 in aio_dispatch (ctx=0x560cacbd6da0) at 
> util/aio-posix.c:459
> #18 0x560caa8ec5f2 in aio_ctx_dispatch
> (source=, callback=, user_data= out>) at util/async.c:260
> #19 0x7fe2078d167d in g_main_context_dispatch () at 
> /lib64/libglib-2.0.so.0
> #20 0x560caa8eebb8 in glib_pollfds_poll () at util/main-loop.c:219
> #21 0x560caa8eebb8 in os_host_main_loop_wait (timeout=) at 
> util/main-loop.c:242
> #22 0x560caa8eebb8 in main_loop_wait (nonblocking=) at 
> util/main-loop.c:518
> #23 0x560caa6cfe51 in main_loop () at vl.c:1828
> #24 0x560caa57b322 in main (argc=, argv=, 
> envp=)
> at vl.c:4504
> 
> Thread 1 (Thread 0x7fe1fb059700 (LWP 1659573)):
> #0  0x7fe20301b70f in raise () at /lib64/libc.so.6
> #1  0x7fe203005b25 in abort () at /lib64/libc.so.6
> #2  0x7fe2030059f9 in _nl_load_domain.cold.0 () at /lib64/libc.so.6
> #3  0x7fe203013cc6 in .annobin_assert.c_end () at /lib64/libc.so.6
> #4  0x560caa85bfe4 in blk_get_aio_context (blk=0x560cacecf370) at 
> block/block-backend.c:1968
> #5  0x560caa85bfe4 in blk_get_aio_context (blk=0x560cacecf370) at 
> block/block-backend.c:1962
> #6  0x560caa61d79c in virtio_scsi_ctx_check (s=0x560cadaafbd0, 
> s=0x560cadaafbd0, d=0x560cacec1210)
> at 
> /usr/src/debug/qemu-kvm-4.2.0-22.module+el8.2.1+6758+cb8d64c2.x86_64/hw/scsi/virtio-scsi.c:250
> #7  0x560caa61d79c in virtio_scsi_handle_cmd_req_prepare 
> (req=0x7fe1ec013880, s=0x560cadaafbd0)
> at 
> /usr/src/debug/qemu-kvm-4.2.0-22.module+el8.2.1+6758+cb8d64c2.x86_64/hw/scsi/virtio-scsi.c:569
> #8  0x560caa61d79c in virtio_scsi_handle_cmd_vq
> (s=s@entry=0x560cadaafbd0, vq=vq@entry=0x7fe1f82ac140)
> at 
> /usr/src/debug/qemu-kvm-4.2.0-22.module+el8.2.1+6758+cb8d64c2.x86_64/hw/scsi/virtio-scsi.c:612
> #9  0x560caa61e48e in virtio_scsi_data_plane_handle_cmd (vdev= out>, vq=0x7fe1f82ac140)
> at 
> /usr/src/debug/qemu-kvm-4.2.0-22.module+el8.2.1+6758+cb8d64c2.x86_64/hw/scsi/virtio-scsi-dataplane.c:60
> #10 0x560caa62bfbe in virtio_queue_notify_aio_vq (vq=)
> at 
> /usr/src/debug/qemu-kvm-4.2.0-22.module+el8.2.1+6758+cb8d64c2.x86_64/hw/virtio/virtio.c:2243
> #11 0x560caa8ef046 in run_poll_handlers_once
> (ctx=ctx@entry=0x560cacc689b0, timeout=timeout@entry=0x7fe1fb058658) at 
> util/aio-posix.c:517
> #12 0x560caa8efbc5 in try_poll_mode (timeout=0x7fe1fb058658, 
> ctx=0x560cacc689b0)
> at util/aio-posix.c:607
> #13 

Re: [PATCH 0/2] qemu-storage-daemon: memory leak and --object opts fixes

[Stefan Hajnoczi]

On Fri, Jun 19, 2020 at 12:15 PM  wrote:
> /tmp/qemu-test/src/tests/qht-bench.c:287:29: error: implicit conversion from 
> 'unsigned long' to 'double' changes value from 18446744073709551615 to 
> 18446744073709551616 [-Werror,-Wimplicit-int-float-conversion]
> *threshold = rate * UINT64_MAX;
>   ~ ^~
> /usr/include/stdint.h:130:23: note: expanded from macro 'UINT64_MAX'

Unrelated failure.

Stefan

Re: [PATCH 0/2] qcow2: Force preallocation with data-file-raw

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200619104012.235977-1-mre...@redhat.com/



Hi,

This series failed the asan build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC  qga/commands.o
  CC  qga/guest-agent-command-state.o
  CC  qga/main.o
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  CC  qga/commands-posix.o
  CC  qga/channel-posix.o
  CC  qga/qapi-generated/qga-qapi-types.o
---
  GEN docs/interop/qemu-ga-ref.html
  GEN docs/interop/qemu-ga-ref.txt
  GEN docs/interop/qemu-ga-ref.7
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  AS  pc-bios/optionrom/linuxboot.o
  CC  pc-bios/optionrom/linuxboot_dma.o
  AS  pc-bios/optionrom/kvmvapic.o
---
  BUILD   pc-bios/optionrom/pvh.raw
  LINKqemu-keymap
  SIGNpc-bios/optionrom/pvh.bin
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  LINKivshmem-client
  LINKivshmem-server
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  LINKqemu-nbd
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  LINKqemu-storage-daemon
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  LINKqemu-img
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  LINKqemu-io
  LINKqemu-edid
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  LINKfsdev/virtfs-proxy-helper
  LINKscsi/qemu-pr-helper
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  LINKqemu-bridge-helper
  LINKvirtiofsd
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  LINKvhost-user-input
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 

Re: [PATCH v10 00/12] acpi: i386 tweaks

[Michael S. Tsirkin]

On Fri, Jun 19, 2020 at 02:51:51AM -0700, no-re...@patchew.org wrote:
> Patchew URL: 
> https://patchew.org/QEMU/20200619091905.21676-1-kra...@redhat.com/
> 
> 
> 
> Hi,
> 
> This series failed the asan build test. Please find the testing commands and
> their output below. If you have Docker installed, you can probably reproduce 
> it
> locally.
> 
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> export ARCH=x86_64
> make docker-image-fedora V=1 NETWORK=1
> time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1
> === TEST SCRIPT END ===
> 
>   GEN docs/interop/qemu-qmp-ref.txt
>   GEN docs/interop/qemu-qmp-ref.7
>   CC  qga/commands.o
> /usr/bin/ld: 
> /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
>  warning: common of `__interception::real_vfork' overridden by definition 
> from 
> /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
>   CC  qga/guest-agent-command-state.o
>   CC  qga/main.o
>   CC  qga/commands-posix.o
> ---
>   GEN docs/interop/qemu-ga-ref.html
>   GEN docs/interop/qemu-ga-ref.txt
>   GEN docs/interop/qemu-ga-ref.7
> /usr/bin/ld: 
> /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
>  warning: common of `__interception::real_vfork' overridden by definition 
> from 
> /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
>   LINKqemu-keymap
>   LINKivshmem-client
> /usr/bin/ld: 
> /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
>  warning: common of `__interception::real_vfork' overridden by definition 
> from 
> /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
> /usr/bin/ld: 
> /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
>  warning: common of `__interception::real_vfork' overridden by definition 
> from 
> /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
>   LINKivshmem-server
>   AS  pc-bios/optionrom/multiboot.o
>   AS  pc-bios/optionrom/linuxboot.o
>   CC  pc-bios/optionrom/linuxboot_dma.o
>   AS  pc-bios/optionrom/kvmvapic.o
> /usr/bin/ld: 
> /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
>  warning: common of `__interception::real_vfork' overridden by definition 
> from 
> /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
>   LINKqemu-nbd
>   AS  pc-bios/optionrom/pvh.o
>   CC  pc-bios/optionrom/pvh_main.o
> ---
>   BUILD   pc-bios/optionrom/multiboot.raw
>   BUILD   pc-bios/optionrom/linuxboot.raw
>   BUILD   pc-bios/optionrom/linuxboot_dma.raw
> /usr/bin/ld: 
> /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
>  warning: common of `__interception::real_vfork' overridden by definition 
> from 
> /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
>   BUILD   pc-bios/optionrom/kvmvapic.raw
>   SIGNpc-bios/optionrom/multiboot.bin
>   LINKqemu-storage-daemon
> ---
>   SIGNpc-bios/optionrom/linuxboot_dma.bin
>   SIGNpc-bios/optionrom/kvmvapic.bin
>   BUILD   pc-bios/optionrom/pvh.img
> /usr/bin/ld: 
> /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
>  warning: common of `__interception::real_vfork' overridden by definition 
> from 
> /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
>   LINKqemu-img
>   BUILD   pc-bios/optionrom/pvh.raw
>   SIGNpc-bios/optionrom/pvh.bin
>   LINKqemu-io
> /usr/bin/ld: 
> /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
>  warning: common of `__interception::real_vfork' overridden by definition 
> from 
> /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
>   LINKqemu-edid
> /usr/bin/ld: 
> /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
>  warning: common of `__interception::real_vfork' overridden by definition 
> from 
> /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
> /usr/bin/ld: 
> /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
>  warning: common of `__interception::real_vfork' overridden by definition 
> from 
> /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
>   LINKfsdev/virtfs-proxy-helper
>   LINKscsi/qemu-pr-helper
> /usr/bin/ld: 
> /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
>  warning: common of `__interception::real_vfork' overridden by definition 
> from 
> /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
>   LINKqemu-bridge-helper
>   LINKvirtiofsd
> 

Re: [PATCH 0/2] qemu-storage-daemon: memory leak and --object opts fixes

[no-reply]

Patchew URL: 
https://patchew.org/QEMU/20200619101132.2401756-1-stefa...@redhat.com/



Hi,

This series failed the asan build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1
=== TEST SCRIPT END ===

  GEN docs/interop/qemu-qmp-ref.html
  GEN docs/interop/qemu-qmp-ref.txt
  GEN docs/interop/qemu-qmp-ref.7
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  CC  qga/commands.o
  CC  qga/guest-agent-command-state.o
  CC  qga/main.o
---
  CC  qemu-img.o
  AR  libvhost-user.a
  GEN docs/interop/qemu-ga-ref.html
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  GEN docs/interop/qemu-ga-ref.txt
  GEN docs/interop/qemu-ga-ref.7
  LINKqemu-keymap
  LINKivshmem-client
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  AS  pc-bios/optionrom/multiboot.o
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  CC  pc-bios/optionrom/linuxboot_dma.o
  AS  pc-bios/optionrom/linuxboot.o
  AS  pc-bios/optionrom/kvmvapic.o
  LINKivshmem-server
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  LINKqemu-nbd
  AS  pc-bios/optionrom/pvh.o
  CC  pc-bios/optionrom/pvh_main.o
---
  BUILD   pc-bios/optionrom/kvmvapic.img
  BUILD   pc-bios/optionrom/multiboot.raw
  BUILD   pc-bios/optionrom/linuxboot.raw
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  BUILD   pc-bios/optionrom/linuxboot_dma.raw
  LINKqemu-storage-daemon
  BUILD   pc-bios/optionrom/kvmvapic.raw
---
  BUILD   pc-bios/optionrom/pvh.raw
  SIGNpc-bios/optionrom/pvh.bin
  LINKqemu-edid
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  LINKfsdev/virtfs-proxy-helper
  LINKscsi/qemu-pr-helper
  LINKqemu-bridge-helper
  LINKvirtiofsd
  LINKvhost-user-input
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of 

Re: [PATCH for-5.1] qcow2: Don't open images with a backing file and the data-file-raw bit

[Alberto Garcia]

On Fri 19 Jun 2020 09:57:27 AM CEST, Max Reitz wrote:
>> If two images have the same contents but then you compare them
>> changing the backing file of one of them you can also get a content
>> mismatch. How is this different?
>
> It’s different in that files with data-file-raw can’t have backing
> files at all.  So maybe users shouldn’t be allowed to give them
> backing files at runtime either.

I understand that. Ideally it should be forbidden. Perhaps that could be
fixed by turning drv->supports_backing into a function.

My point however is that forcing a different backing file is something
that is going to cause breakage unless the user really knows that
they're doing. And we don't generally forbid that, we just let the user
take responsibility. So I'm not too worried about this case.

> Or at least, if we have data-file-raw, *all* data visible on such an
> image should be taken from the raw data file, never from any backing
> file.

It should be easy to handle in qcow2_co_preadv_part() and
qcow2_co_copy_range_from(), but it feels hackish and error prone.

> With preallocation, we’d ensure that we always take all data from the
> raw data file.  So we’d always ignore any potential backing file.

Preallocation has its problems (and we would also have to handle it
differently if there are subclusters, but I think that should be
easy). But I don't have a strong opinion.

Berto

Re: [PATCH v5 0/6] block: seriously improve savevm performance

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200619100708.30440-1-...@openvz.org/



Hi,

This series failed the asan build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1
=== TEST SCRIPT END ===

  GEN docs/interop/qemu-qmp-ref.html
  GEN docs/interop/qemu-qmp-ref.txt
  GEN docs/interop/qemu-qmp-ref.7
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `  CC  qga/commands.o
  CC  qga/guest-agent-command-state.o
  CC  qga/main.o
  CC  qga/commands-posix.o
---
  LINKelf2dmp
  CC  qemu-img.o
  AR  libvhost-user.a
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  AS  pc-bios/optionrom/linuxboot.o
  CC  pc-bios/optionrom/linuxboot_dma.o
  AS  pc-bios/optionrom/kvmvapic.o
---
  SIGNpc-bios/optionrom/pvh.bin
  LINKqemu-ga
  LINKqemu-keymap
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  LINKivshmem-client
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  LINKivshmem-server
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  LINKqemu-nbd
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  LINKqemu-storage-daemon
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  LINKqemu-img
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  LINKqemu-io
  LINKqemu-edid
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  LINKfsdev/virtfs-proxy-helper
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  LINKscsi/qemu-pr-helper
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  LINKqemu-bridge-helper
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  LINKvirtiofsd
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  LINKvhost-user-input
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: 

[PATCH 1/2] qcow2: Force preallocation with data-file-raw

[Max Reitz]

Setting the qcow2 data-file-raw bit means that you can ignore the
qcow2 metadata when reading from the external data file.  It does not
mean that you have to ignore it, though.  Therefore, the data read must
be the same regardless of whether you interpret the metadata or whether
you ignore it, and thus the L1/L2 tables must all be present and give a
1:1 mapping.

This patch changes 244's output: First, the qcow2 file is larger right
after creation, because of metadata preallocation.  Second, the qemu-img
map output changes: Everything that was not explicitly discarded or
zeroed is now a data area.

Signed-off-by: Max Reitz 
---
 block/qcow2.c  | 22 ++
 tests/qemu-iotests/244.out |  9 -
 2 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 0cd2e6757e..2a588d8091 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3460,6 +3460,28 @@ qcow2_co_create(BlockdevCreateOptions *create_options, 
Error **errp)
 ret = -EINVAL;
 goto out;
 }
+if (qcow2_opts->data_file_raw &&
+qcow2_opts->preallocation == PREALLOC_MODE_OFF)
+{
+/*
+ * data-file-raw means that "the external data file can be
+ * read as a consistent standalone raw image without looking
+ * at the qcow2 metadata."  It does not say that the metadata
+ * must be ignored, though (and the qcow2 driver in fact does
+ * not ignore it), so the L1/L2 tables must be present and
+ * give a 1:1 mapping, so you get the same result regardless
+ * of whether you look at the metadata or whether you ignore
+ * it.
+ */
+qcow2_opts->preallocation = PREALLOC_MODE_METADATA;
+
+/*
+ * Cannot use preallocation with backing files, but giving a
+ * backing file when specifying data_file_raw is an error
+ * anyway.
+ */
+assert(!qcow2_opts->has_backing_file);
+}
 
 if (qcow2_opts->data_file) {
 if (version < 3) {
diff --git a/tests/qemu-iotests/244.out b/tests/qemu-iotests/244.out
index dbab7359a9..24f02363dd 100644
--- a/tests/qemu-iotests/244.out
+++ b/tests/qemu-iotests/244.out
@@ -83,7 +83,7 @@ qcow2 file size after I/O: 327680
 === Standalone image with external data file (valid raw) ===
 
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 
data_file=TEST_DIR/t.IMGFMT.data data_file_raw=on
-qcow2 file size before I/O: 196616
+qcow2 file size before I/O: 327680
 
 wrote 4194304/4194304 bytes at offset 1048576
 4 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
@@ -93,11 +93,10 @@ wrote 3145728/3145728 bytes at offset 3145728
 3 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 No errors were found on the image.
 
-[{ "start": 0, "length": 1048576, "depth": 0, "zero": true, "data": false},
-{ "start": 1048576, "length": 1048576, "depth": 0, "zero": false, "data": 
true, "offset": 1048576},
+[{ "start": 0, "length": 2097152, "depth": 0, "zero": false, "data": true, 
"offset": 0},
 { "start": 2097152, "length": 2097152, "depth": 0, "zero": true, "data": 
false},
-{ "start": 4194304, "length": 1048576, "depth": 0, "zero": true, "data": 
false, "offset": 4194304},
-{ "start": 5242880, "length": 61865984, "depth": 0, "zero": true, "data": 
false}]
+{ "start": 4194304, "length": 2097152, "depth": 0, "zero": true, "data": 
false, "offset": 4194304},
+{ "start": 6291456, "length": 60817408, "depth": 0, "zero": false, "data": 
true, "offset": 6291456}]
 
 read 1048576/1048576 bytes at offset 0
 1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-- 
2.26.2

[PATCH 2/2] iotests/244: Test preallocation for data-file-raw

[Max Reitz]

Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/244 | 65 ++
 tests/qemu-iotests/244.out | 23 ++
 2 files changed, 88 insertions(+)

diff --git a/tests/qemu-iotests/244 b/tests/qemu-iotests/244
index efe3c0428b..c2fdeab0c7 100755
--- a/tests/qemu-iotests/244
+++ b/tests/qemu-iotests/244
@@ -217,6 +217,71 @@ $QEMU_IMG amend -f $IMGFMT -o 
"data_file=blkdebug::$TEST_IMG.data" "$TEST_IMG"
 $QEMU_IMG convert -f $IMGFMT -O $IMGFMT -n -C "$TEST_IMG.src" "$TEST_IMG"
 $QEMU_IMG compare -f $IMGFMT -F $IMGFMT "$TEST_IMG.src" "$TEST_IMG"
 
+echo
+echo '=== Preallocation with data-file-raw ==='
+
+echo
+echo '--- Using a non-zeroed data file ---'
+
+# Using data-file-raw must enforce at least metadata preallocation so
+# that it does not matter whether one reads the raw file or the qcow2
+# file
+
+# The real test we would like to do here is to use an existing block
+# device with some random data on it as the external data file.
+# When creating the qcow2 file, it would not be overwritten and its
+# data would stay as it is.  However, using an existing block device
+# is a bit cumbersome in a test, so we are going to cheat by using a
+# normal regular file.
+
+# Unfortunately, this will O_CREAT | O_TRUNC that regular file, so
+# there is no point in creating it beforehand and filling it with
+# random data:
+_make_test_img -o "data_file=$TEST_IMG.data,data_file_raw=on" 1M
+
+# So now comes the cheating: We write directly into the data file.
+# That is actually unsupported, but it works for this test.
+# (As written above, the actual case would be to use a block device
+# as the external data file.  Such a device would not be emptied when
+# the qcow2 file is created, so its data would persist that step.)
+$QEMU_IO -f raw -c 'write -P 42 0 1M' "$TEST_IMG.data" | _filter_qemu_io
+
+echo
+echo 'Comparing pattern:'
+
+# Reading from either the qcow2 file or the data file should return
+# the same result:
+$QEMU_IO -f raw -c 'read -P 42 0 1M' "$TEST_IMG.data" | _filter_qemu_io
+$QEMU_IO -f $IMGFMT -c 'read -P 42 0 1M' "$TEST_IMG" | _filter_qemu_io
+
+# For good measure
+$QEMU_IMG compare -f raw "$TEST_IMG.data" "$TEST_IMG"
+
+echo
+echo '--- Giving a backing file at runtime ---'
+
+# qcow2 files with data-file-raw cannot have backing files given by
+# their image header, but qemu will allow you to set a backing node at
+# runtime -- it should not have any effect, though (because reading
+# from the qcow2 node should return the same data as reading from the
+# raw node).
+
+_make_test_img -o "data_file=$TEST_IMG.data,data_file_raw=on" 1M
+TEST_IMG="$TEST_IMG.base" _make_test_img 1M
+
+# Write something that is not zero into the base image
+$QEMU_IO -c 'write -P 42 0 1M' "$TEST_IMG.base" | _filter_qemu_io
+
+echo
+echo 'Comparing qcow2 image and raw data file:'
+
+# $TEST_IMG and $TEST_IMG.data must show the same data at all times;
+# that is, the qcow2 node must not fall through to the backing image
+# at any point
+$QEMU_IMG compare --image-opts \
+"driver=raw,file.filename=$TEST_IMG.data"  \
+"file.filename=$TEST_IMG,backing.file.filename=$TEST_IMG.base"
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/244.out b/tests/qemu-iotests/244.out
index 24f02363dd..34d6b0e626 100644
--- a/tests/qemu-iotests/244.out
+++ b/tests/qemu-iotests/244.out
@@ -130,4 +130,27 @@ Offset  Length  Mapped to   File
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 
data_file=TEST_DIR/t.IMGFMT.data
 Images are identical.
 Images are identical.
+
+=== Preallocation with data-file-raw ===
+
+--- Using a non-zeroed data file ---
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 
data_file=TEST_DIR/t.IMGFMT.data data_file_raw=on
+wrote 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+Comparing pattern:
+read 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Images are identical.
+
+--- Giving a backing file at runtime ---
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 
data_file=TEST_DIR/t.IMGFMT.data data_file_raw=on
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=1048576
+wrote 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+Comparing qcow2 image and raw data file:
+Images are identical.
 *** done
-- 
2.26.2

[PATCH 0/2] qcow2: Force preallocation with data-file-raw

[Max Reitz]

Hi,

As discussed here:

https://lists.nongnu.org/archive/html/qemu-block/2020-02/msg00644.html
https://lists.nongnu.org/archive/html/qemu-block/2020-04/msg00329.html
https://lists.nongnu.org/archive/html/qemu-block/2020-06/msg00240.html

I think that qcow2 images with data-file-raw should always have
preallocated 1:1 L1/L2 tables, so that the image always looks the same
whether you respect or ignore the qcow2 metadata.  The easiest way to
achieve that is to enforce at least metadata preallocation whenever
data-file-raw is given.


Max Reitz (2):
  qcow2: Force preallocation with data-file-raw
  iotests/244: Test preallocation for data-file-raw

 block/qcow2.c  | 22 +
 tests/qemu-iotests/244 | 65 ++
 tests/qemu-iotests/244.out | 32 ---
 3 files changed, 114 insertions(+), 5 deletions(-)

-- 
2.26.2

[PATCH 2/2] qemu-storage-daemon: add missing cleanup calls

[Stefan Hajnoczi]

Several components used by qemu-storage-daemon have cleanup functions
that aren't called. Keep the "valgrind --leak-check=full" as clean as
possible by invoking the necessary cleanup functions.

Signed-off-by: Stefan Hajnoczi 
---
 qemu-storage-daemon.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/qemu-storage-daemon.c b/qemu-storage-daemon.c
index a01cbd6371..7e9b0e0d3f 100644
--- a/qemu-storage-daemon.c
+++ b/qemu-storage-daemon.c
@@ -335,5 +335,9 @@ int main(int argc, char *argv[])
 main_loop_wait(false);
 }
 
+monitor_cleanup();
+qemu_chr_cleanup();
+user_creatable_cleanup();
+
 return EXIT_SUCCESS;
 }
-- 
2.26.2

[PATCH 1/2] qemu-storage-daemon: remember to add qemu_object_opts

[Stefan Hajnoczi]

The --object option is supported by qemu-storage-daemon but the
qemu_object_opts QemuOptsList wasn't being added. As a result calls to
qemu_find_opts("object") failed with "There is no option group
'object'".

This patch fixes the object-del QMP command.

Signed-off-by: Stefan Hajnoczi 
---
 qemu-storage-daemon.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/qemu-storage-daemon.c b/qemu-storage-daemon.c
index 9e7adfe3a6..a01cbd6371 100644
--- a/qemu-storage-daemon.c
+++ b/qemu-storage-daemon.c
@@ -316,6 +316,7 @@ int main(int argc, char *argv[])
 
 module_call_init(MODULE_INIT_QOM);
 module_call_init(MODULE_INIT_TRACE);
+qemu_add_opts(_object_opts);
 qemu_add_opts(_trace_opts);
 qcrypto_init(_fatal);
 bdrv_init();
-- 
2.26.2

[PATCH 0/2] qemu-storage-daemon: memory leak and --object opts fixes

[Stefan Hajnoczi]

Small fixes for qemu-storage-daemon.

Stefan Hajnoczi (2):
  qemu-storage-daemon: remember to add qemu_object_opts
  qemu-storage-daemon: add missing cleanup calls

 qemu-storage-daemon.c | 5 +
 1 file changed, 5 insertions(+)

-- 
2.26.2

[PATCH v5 0/6] block: seriously improve savevm performance

[Denis V. Lunev]

This series do standard basic things:
- it creates intermediate buffer for all writes from QEMU migration code
  to QCOW2 image,
- this buffer is sent to disk asynchronously, allowing several writes to
  run in parallel.

In general, migration code is fantastically inefficent (by observation),
buffers are not aligned and sent with arbitrary pieces, a lot of time
less than 100 bytes at a chunk, which results in read-modify-write
operations with non-cached operations. It should also be noted that all
operations are performed into unallocated image blocks, which also suffer
due to partial writes to such new clusters.

This patch series is an implementation of idea discussed in the RFC
posted by Denis Plotnikov
https://lists.gnu.org/archive/html/qemu-devel/2020-04/msg01925.html
Results with this series over NVME are better than original code
original rfcthis
cached:  1.79s  2.38s   1.27s
non-cached:  3.29s  1.31s   0.81s

Changes from v4:
- added patch 4 with blk_save_vmstate() cleanup
- added R-By
- bdrv_flush_vmstate -> bdrv_finalize_vmstate
- fixed return code of bdrv_co_do_save_vmstate
- fixed typos in comments (Eric, thanks!)
- fixed patchew warnings

Changes from v3:
- rebased to master
- added patch 3 which removes aio_task_pool_wait_one()
- added R-By to patch 1
- patch 4 is rewritten via bdrv_run_co
- error path in blk_save_vmstate() is rewritten to call bdrv_flush_vmstate
  unconditionally
- added some comments
- fixes initialization in bdrv_co_vmstate_save_task_entry as suggested

Changes from v2:
- code moved from QCOW2 level to generic block level
- created bdrv_flush_vmstate helper to fix 022, 029 tests
- added recursive for bs->file in bdrv_co_flush_vmstate (fix 267)
- fixed blk_save_vmstate helper
- fixed coroutine wait as Vladimir suggested with waiting fixes from me

Changes from v1:
- patchew warning fixed
- fixed validation that only 1 waiter is allowed in patch 1

Signed-off-by: Denis V. Lunev 
CC: Kevin Wolf 
CC: Max Reitz 
CC: Stefan Hajnoczi 
CC: Fam Zheng 
CC: Juan Quintela 
CC: "Dr. David Alan Gilbert" 
CC: Vladimir Sementsov-Ogievskiy 
CC: Denis Plotnikov 

[PATCH 3/6] block/aio_task: drop aio_task_pool_wait_one() helper

[Denis V. Lunev]

It is not used outside the module.

Actually there are 2 kind of waiters:
- for a slot and
- for all tasks to finish
This patch limits external API to listed types.

Signed-off-by: Denis V. Lunev 
Suggested-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
CC: Kevin Wolf 
CC: Max Reitz 
CC: Stefan Hajnoczi 
CC: Fam Zheng 
CC: Juan Quintela 
CC: "Dr. David Alan Gilbert" 
CC: Denis Plotnikov 
---
 block/aio_task.c | 13 ++---
 include/block/aio_task.h |  1 -
 2 files changed, 2 insertions(+), 12 deletions(-)

diff --git a/block/aio_task.c b/block/aio_task.c
index cf62e5c58b..7ba15ff41f 100644
--- a/block/aio_task.c
+++ b/block/aio_task.c
@@ -54,26 +54,17 @@ static void coroutine_fn aio_task_co(void *opaque)
 qemu_co_queue_restart_all(>waiters);
 }
 
-void coroutine_fn aio_task_pool_wait_one(AioTaskPool *pool)
-{
-assert(pool->busy_tasks > 0);
-
-qemu_co_queue_wait(>waiters, NULL);
-
-assert(pool->busy_tasks < pool->max_busy_tasks);
-}
-
 void coroutine_fn aio_task_pool_wait_slot(AioTaskPool *pool)
 {
 while (pool->busy_tasks >= pool->max_busy_tasks) {
-aio_task_pool_wait_one(pool);
+qemu_co_queue_wait(>waiters, NULL);
 }
 }
 
 void coroutine_fn aio_task_pool_wait_all(AioTaskPool *pool)
 {
 while (pool->busy_tasks > 0) {
-aio_task_pool_wait_one(pool);
+qemu_co_queue_wait(>waiters, NULL);
 }
 }
 
diff --git a/include/block/aio_task.h b/include/block/aio_task.h
index 50bc1e1817..50b1c036c5 100644
--- a/include/block/aio_task.h
+++ b/include/block/aio_task.h
@@ -48,7 +48,6 @@ bool aio_task_pool_empty(AioTaskPool *pool);
 void coroutine_fn aio_task_pool_start_task(AioTaskPool *pool, AioTask *task);
 
 void coroutine_fn aio_task_pool_wait_slot(AioTaskPool *pool);
-void coroutine_fn aio_task_pool_wait_one(AioTaskPool *pool);
 void coroutine_fn aio_task_pool_wait_all(AioTaskPool *pool);
 
 #endif /* BLOCK_AIO_TASK_H */
-- 
2.17.1

[PATCH 2/6] block/aio_task: allow start/wait task from any coroutine

[Denis V. Lunev]

From: Vladimir Sementsov-Ogievskiy 

Currently, aio task pool assumes that there is a main coroutine, which
creates tasks and wait for them. Let's remove the restriction by using
CoQueue. Code becomes clearer, interface more obvious.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Denis V. Lunev 
CC: Kevin Wolf 
CC: Max Reitz 
CC: Stefan Hajnoczi 
CC: Fam Zheng 
CC: Juan Quintela 
CC: "Dr. David Alan Gilbert" 
CC: Vladimir Sementsov-Ogievskiy 
CC: Denis Plotnikov 
---
 block/aio_task.c | 21 ++---
 1 file changed, 6 insertions(+), 15 deletions(-)

diff --git a/block/aio_task.c b/block/aio_task.c
index 88989fa248..cf62e5c58b 100644
--- a/block/aio_task.c
+++ b/block/aio_task.c
@@ -27,11 +27,10 @@
 #include "block/aio_task.h"
 
 struct AioTaskPool {
-Coroutine *main_co;
 int status;
 int max_busy_tasks;
 int busy_tasks;
-bool waiting;
+CoQueue waiters;
 };
 
 static void coroutine_fn aio_task_co(void *opaque)
@@ -52,31 +51,23 @@ static void coroutine_fn aio_task_co(void *opaque)
 
 g_free(task);
 
-if (pool->waiting) {
-pool->waiting = false;
-aio_co_wake(pool->main_co);
-}
+qemu_co_queue_restart_all(>waiters);
 }
 
 void coroutine_fn aio_task_pool_wait_one(AioTaskPool *pool)
 {
 assert(pool->busy_tasks > 0);
-assert(qemu_coroutine_self() == pool->main_co);
 
-pool->waiting = true;
-qemu_coroutine_yield();
+qemu_co_queue_wait(>waiters, NULL);
 
-assert(!pool->waiting);
 assert(pool->busy_tasks < pool->max_busy_tasks);
 }
 
 void coroutine_fn aio_task_pool_wait_slot(AioTaskPool *pool)
 {
-if (pool->busy_tasks < pool->max_busy_tasks) {
-return;
+while (pool->busy_tasks >= pool->max_busy_tasks) {
+aio_task_pool_wait_one(pool);
 }
-
-aio_task_pool_wait_one(pool);
 }
 
 void coroutine_fn aio_task_pool_wait_all(AioTaskPool *pool)
@@ -98,8 +89,8 @@ AioTaskPool *coroutine_fn aio_task_pool_new(int 
max_busy_tasks)
 {
 AioTaskPool *pool = g_new0(AioTaskPool, 1);
 
-pool->main_co = qemu_coroutine_self();
 pool->max_busy_tasks = max_busy_tasks;
+qemu_co_queue_init(>waiters);
 
 return pool;
 }
-- 
2.17.1

[PATCH 1/6] migration/savevm: respect qemu_fclose() error code in save_snapshot()

[Denis V. Lunev]

qemu_fclose() could return error, f.e. if bdrv_co_flush() will return
the error.

This validation will become more important once we will start waiting of
asynchronous IO operations, started from bdrv_write_vmstate(), which are
coming soon.

Signed-off-by: Denis V. Lunev 
Reviewed-by: "Dr. David Alan Gilbert" 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
CC: Kevin Wolf 
CC: Max Reitz 
CC: Stefan Hajnoczi 
CC: Fam Zheng 
CC: Juan Quintela 
CC: Denis Plotnikov 
---
 migration/savevm.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index b979ea6e7f..da3dead4e9 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2628,7 +2628,7 @@ int save_snapshot(const char *name, Error **errp)
 {
 BlockDriverState *bs, *bs1;
 QEMUSnapshotInfo sn1, *sn = , old_sn1, *old_sn = _sn1;
-int ret = -1;
+int ret = -1, ret2;
 QEMUFile *f;
 int saved_vm_running;
 uint64_t vm_state_size;
@@ -2712,10 +2712,14 @@ int save_snapshot(const char *name, Error **errp)
 }
 ret = qemu_savevm_state(f, errp);
 vm_state_size = qemu_ftell(f);
-qemu_fclose(f);
+ret2 = qemu_fclose(f);
 if (ret < 0) {
 goto the_end;
 }
+if (ret2 < 0) {
+ret = ret2;
+goto the_end;
+}
 
 /* The bdrv_all_create_snapshot() call that follows acquires the AioContext
  * for itself.  BDRV_POLL_WHILE() does not support nested locking because
-- 
2.17.1

[PATCH 6/6] block/io: improve savevm performance

[Denis V. Lunev]

This patch does 2 standard basic things:
- it creates intermediate buffer for all writes from QEMU migration code
  to block driver,
- this buffer is sent to disk asynchronously, allowing several writes to
  run in parallel.

Thus bdrv_vmstate_write() is becoming asynchronous. All pending operations
completion are performed in newly invented bdrv_finalize_vmstate().

In general, migration code is fantastically inefficent (by observation),
buffers are not aligned and sent with arbitrary pieces, a lot of time
less than 100 bytes at a chunk, which results in read-modify-write
operations if target file descriptor is opened with O_DIRECT. It should
also be noted that all operations are performed into unallocated image
blocks, which also suffer due to partial writes to such new clusters
even on cached file descriptors.

Snapshot creation time (2 GB Fedora-31 VM running over NVME storage):
original fixed
cached:  1.79s   1.27s
non-cached:  3.29s   0.81s

The difference over HDD would be more significant :)

Signed-off-by: Denis V. Lunev 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
CC: Kevin Wolf 
CC: Max Reitz 
CC: Stefan Hajnoczi 
CC: Fam Zheng 
CC: Juan Quintela 
CC: "Dr. David Alan Gilbert" 
CC: Denis Plotnikov 
---
 block/io.c| 126 +-
 include/block/block_int.h |   8 +++
 2 files changed, 132 insertions(+), 2 deletions(-)

diff --git a/block/io.c b/block/io.c
index 1f69268361..71a696deb7 100644
--- a/block/io.c
+++ b/block/io.c
@@ -26,6 +26,7 @@
 #include "trace.h"
 #include "sysemu/block-backend.h"
 #include "block/aio-wait.h"
+#include "block/aio_task.h"
 #include "block/blockjob.h"
 #include "block/blockjob_int.h"
 #include "block/block_int.h"
@@ -33,6 +34,7 @@
 #include "qapi/error.h"
 #include "qemu/error-report.h"
 #include "qemu/main-loop.h"
+#include "qemu/units.h"
 #include "sysemu/replay.h"
 
 /* Maximum bounce buffer for copy-on-read and write zeroes, in bytes */
@@ -2640,6 +2642,103 @@ typedef struct BdrvVmstateCo {
 boolis_read;
 } BdrvVmstateCo;
 
+typedef struct BdrvVMStateTask {
+AioTask task;
+
+BlockDriverState *bs;
+int64_t offset;
+void *buf;
+size_t bytes;
+} BdrvVMStateTask;
+
+typedef struct BdrvSaveVMState {
+AioTaskPool *pool;
+BdrvVMStateTask *t;
+} BdrvSaveVMState;
+
+
+static coroutine_fn int bdrv_co_vmstate_save_task_entry(AioTask *task)
+{
+int err = 0;
+BdrvVMStateTask *t = container_of(task, BdrvVMStateTask, task);
+
+if (t->bytes != 0) {
+QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, t->buf, t->bytes);
+
+bdrv_inc_in_flight(t->bs);
+err = t->bs->drv->bdrv_save_vmstate(t->bs, , t->offset);
+bdrv_dec_in_flight(t->bs);
+}
+
+qemu_vfree(t->buf);
+return err;
+}
+
+static BdrvVMStateTask *bdrv_vmstate_task_create(BlockDriverState *bs,
+ int64_t pos, size_t size)
+{
+BdrvVMStateTask *t = g_new(BdrvVMStateTask, 1);
+
+*t = (BdrvVMStateTask) {
+.task.func = bdrv_co_vmstate_save_task_entry,
+.buf = qemu_blockalign(bs, size),
+.offset = pos,
+.bs = bs,
+};
+
+return t;
+}
+
+static int bdrv_co_do_save_vmstate(BlockDriverState *bs, QEMUIOVector *qiov,
+   int64_t pos)
+{
+BdrvSaveVMState *state = bs->savevm_state;
+BdrvVMStateTask *t;
+size_t buf_size = MAX(bdrv_get_cluster_size(bs), 1 * MiB);
+size_t to_copy, off;
+
+if (state == NULL) {
+state = g_new(BdrvSaveVMState, 1);
+*state = (BdrvSaveVMState) {
+.pool = aio_task_pool_new(BDRV_VMSTATE_WORKERS_MAX),
+.t = bdrv_vmstate_task_create(bs, pos, buf_size),
+};
+
+bs->savevm_state = state;
+}
+
+if (aio_task_pool_status(state->pool) < 0) {
+/*
+ * The operation as a whole is unsuccessful. Prohibit all futher
+ * operations. If we clean here, new useless ops will come again.
+ * Thus we rely on caller for cleanup here.
+ */
+return aio_task_pool_status(state->pool);
+}
+
+t = state->t;
+if (t->offset + t->bytes != pos) {
+/* Normally this branch is not reachable from migration */
+return bs->drv->bdrv_save_vmstate(bs, qiov, pos);
+}
+
+off = 0;
+while (1) {
+to_copy = MIN(qiov->size - off, buf_size - t->bytes);
+qemu_iovec_to_buf(qiov, off, t->buf + t->bytes, to_copy);
+t->bytes += to_copy;
+if (t->bytes < buf_size) {
+return 0;
+}
+
+aio_task_pool_start_task(state->pool, >task);
+
+pos += to_copy;
+off += to_copy;
+state->t = t = bdrv_vmstate_task_create(bs, pos, buf_size);
+}
+}
+
 static int coroutine_fn
 bdrv_co_rw_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos,
bool is_read)
@@ -2655,7 +2754,7 @@ 

[PATCH 4/6] block/block-backend: remove always true check from blk_save_vmstate

[Denis V. Lunev]

bdrv_save_vmstate() returns either error with negative return value or
size. Thus this check is useless.

Signed-off-by: Denis V. Lunev 
Suggested-by: Eric Blake 
CC: Kevin Wolf 
CC: Max Reitz 
CC: Stefan Hajnoczi 
CC: Fam Zheng 
CC: Juan Quintela 
CC: "Dr. David Alan Gilbert" 
CC: Vladimir Sementsov-Ogievskiy 
CC: Denis Plotnikov 
---
 block/block-backend.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 6936b25c83..1c6e53bbde 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -2188,7 +2188,7 @@ int blk_save_vmstate(BlockBackend *blk, const uint8_t 
*buf,
 return ret;
 }
 
-if (ret == size && !blk->enable_write_cache) {
+if (!blk->enable_write_cache) {
 ret = bdrv_flush(blk_bs(blk));
 }
 
-- 
2.17.1

[PATCH 5/6] block, migration: add bdrv_finalize_vmstate helper

[Denis V. Lunev]

Right now bdrv_fclose() is just calling bdrv_flush().

The problem is that migration code is working inefficiently from block
layer terms and are frequently called for very small pieces of
unaligned data. Block layer is capable to work this way, but this is very
slow.

This patch is a preparation for the introduction of the intermediate
buffer at block driver state. It would be beneficial to separate
conventional bdrv_flush() from closing QEMU file from migration code.

The patch also forces bdrv_finalize_vmstate() operation inside
synchronous blk_save_vmstate() operation. This helper is used from
qemu-io only.

Signed-off-by: Denis V. Lunev 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
CC: Kevin Wolf 
CC: Max Reitz 
CC: Stefan Hajnoczi 
CC: Fam Zheng 
CC: Juan Quintela 
CC: "Dr. David Alan Gilbert" 
CC: Denis Plotnikov 
---
 block/block-backend.c |  6 +-
 block/io.c| 15 +++
 include/block/block.h |  5 +
 migration/savevm.c|  4 
 4 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 1c6e53bbde..5bb11c8e01 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -2177,16 +2177,20 @@ int blk_truncate(BlockBackend *blk, int64_t offset, 
bool exact,
 int blk_save_vmstate(BlockBackend *blk, const uint8_t *buf,
  int64_t pos, int size)
 {
-int ret;
+int ret, ret2;
 
 if (!blk_is_available(blk)) {
 return -ENOMEDIUM;
 }
 
 ret = bdrv_save_vmstate(blk_bs(blk), buf, pos, size);
+ret2 = bdrv_finalize_vmstate(blk_bs(blk));
 if (ret < 0) {
 return ret;
 }
+if (ret2 < 0) {
+return ret2;
+}
 
 if (!blk->enable_write_cache) {
 ret = bdrv_flush(blk_bs(blk));
diff --git a/block/io.c b/block/io.c
index df8f2a98d4..1f69268361 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2724,6 +2724,21 @@ int bdrv_readv_vmstate(BlockDriverState *bs, 
QEMUIOVector *qiov, int64_t pos)
 return bdrv_rw_vmstate(bs, qiov, pos, true);
 }
 
+static int coroutine_fn bdrv_co_finalize_vmstate(BlockDriverState *bs)
+{
+return 0;
+}
+
+static int coroutine_fn bdrv_finalize_vmstate_co_entry(void *opaque)
+{
+return bdrv_co_finalize_vmstate(opaque);
+}
+
+int bdrv_finalize_vmstate(BlockDriverState *bs)
+{
+return bdrv_run_co(bs, bdrv_finalize_vmstate_co_entry, bs);
+}
+
 /**/
 /* async I/Os */
 
diff --git a/include/block/block.h b/include/block/block.h
index 25e299605e..ab2c962094 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -572,6 +572,11 @@ int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t 
*buf,
 
 int bdrv_load_vmstate(BlockDriverState *bs, uint8_t *buf,
   int64_t pos, int size);
+/*
+ * bdrv_finalize_vmstate() is mandatory to commit vmstate changes if
+ * bdrv_save_vmstate() was ever called.
+ */
+int bdrv_finalize_vmstate(BlockDriverState *bs);
 
 void bdrv_img_create(const char *filename, const char *fmt,
  const char *base_filename, const char *base_fmt,
diff --git a/migration/savevm.c b/migration/savevm.c
index da3dead4e9..798a4cb402 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -150,6 +150,10 @@ static ssize_t block_get_buffer(void *opaque, uint8_t 
*buf, int64_t pos,
 
 static int bdrv_fclose(void *opaque, Error **errp)
 {
+int err = bdrv_finalize_vmstate(opaque);
+if (err < 0) {
+return err;
+}
 return bdrv_flush(opaque);
 }
 
-- 
2.17.1

Re: [PATCH v10 00/12] acpi: i386 tweaks

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200619091905.21676-1-kra...@redhat.com/



Hi,

This series failed the asan build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1
=== TEST SCRIPT END ===

  GEN docs/interop/qemu-qmp-ref.txt
  GEN docs/interop/qemu-qmp-ref.7
  CC  qga/commands.o
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  CC  qga/guest-agent-command-state.o
  CC  qga/main.o
  CC  qga/commands-posix.o
---
  GEN docs/interop/qemu-ga-ref.html
  GEN docs/interop/qemu-ga-ref.txt
  GEN docs/interop/qemu-ga-ref.7
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  LINKqemu-keymap
  LINKivshmem-client
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  LINKivshmem-server
  AS  pc-bios/optionrom/multiboot.o
  AS  pc-bios/optionrom/linuxboot.o
  CC  pc-bios/optionrom/linuxboot_dma.o
  AS  pc-bios/optionrom/kvmvapic.o
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  LINKqemu-nbd
  AS  pc-bios/optionrom/pvh.o
  CC  pc-bios/optionrom/pvh_main.o
---
  BUILD   pc-bios/optionrom/multiboot.raw
  BUILD   pc-bios/optionrom/linuxboot.raw
  BUILD   pc-bios/optionrom/linuxboot_dma.raw
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  BUILD   pc-bios/optionrom/kvmvapic.raw
  SIGNpc-bios/optionrom/multiboot.bin
  LINKqemu-storage-daemon
---
  SIGNpc-bios/optionrom/linuxboot_dma.bin
  SIGNpc-bios/optionrom/kvmvapic.bin
  BUILD   pc-bios/optionrom/pvh.img
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  LINKqemu-img
  BUILD   pc-bios/optionrom/pvh.raw
  SIGNpc-bios/optionrom/pvh.bin
  LINKqemu-io
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  LINKqemu-edid
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  LINKfsdev/virtfs-proxy-helper
  LINKscsi/qemu-pr-helper
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  LINKqemu-bridge-helper
  LINKvirtiofsd
/usr/bin/ld: 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o):
 warning: common of `__interception::real_vfork' overridden by definition from 
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
/usr/bin/ld: 

[PATCH v10 05/12] floppy: move cmos_get_fd_drive_type() from pc

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann 
Reviewed-by: Philippe Mathieu-Daudé 
Acked-by: John Snow 
---
 include/hw/block/fdc.h |  1 +
 include/hw/i386/pc.h   |  1 -
 hw/block/fdc.c | 26 +-
 hw/i386/pc.c   | 25 -
 4 files changed, 26 insertions(+), 27 deletions(-)

diff --git a/include/hw/block/fdc.h b/include/hw/block/fdc.h
index 5d71cf972268..479cebc0a330 100644
--- a/include/hw/block/fdc.h
+++ b/include/hw/block/fdc.h
@@ -16,5 +16,6 @@ void sun4m_fdctrl_init(qemu_irq irq, hwaddr io_base,
DriveInfo **fds, qemu_irq *fdc_tc);
 
 FloppyDriveType isa_fdc_get_drive_type(ISADevice *fdc, int i);
+int cmos_get_fd_drive_type(FloppyDriveType fd0);
 
 #endif
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 8d764f965cd3..5e3b19ab78fc 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -176,7 +176,6 @@ typedef void (*cpu_set_smm_t)(int smm, void *arg);
 void pc_i8259_create(ISABus *isa_bus, qemu_irq *i8259_irqs);
 
 ISADevice *pc_find_fdc0(void);
-int cmos_get_fd_drive_type(FloppyDriveType fd0);
 
 /* port92.c */
 #define PORT92_A20_LINE "a20"
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 5a634ab46302..ffe650b17cd5 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -32,7 +32,6 @@
 #include "qapi/error.h"
 #include "qemu/error-report.h"
 #include "qemu/timer.h"
-#include "hw/i386/pc.h"
 #include "hw/acpi/aml-build.h"
 #include "hw/irq.h"
 #include "hw/isa/isa.h"
@@ -2809,6 +2808,31 @@ static Aml *build_fdinfo_aml(int idx, FloppyDriveType 
type)
 return dev;
 }
 
+int cmos_get_fd_drive_type(FloppyDriveType fd0)
+{
+int val;
+
+switch (fd0) {
+case FLOPPY_DRIVE_TYPE_144:
+/* 1.44 Mb 3"5 drive */
+val = 4;
+break;
+case FLOPPY_DRIVE_TYPE_288:
+/* 2.88 Mb 3"5 drive */
+val = 5;
+break;
+case FLOPPY_DRIVE_TYPE_120:
+/* 1.2 Mb 5"5 drive */
+val = 2;
+break;
+case FLOPPY_DRIVE_TYPE_NONE:
+default:
+val = 0;
+break;
+}
+return val;
+}
+
 static void fdc_isa_build_aml(ISADevice *isadev, Aml *scope)
 {
 Aml *dev;
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index ec39741c87ac..7e0ed987b164 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -386,31 +386,6 @@ static uint64_t ioportF0_read(void *opaque, hwaddr addr, 
unsigned size)
 
 #define REG_EQUIPMENT_BYTE  0x14
 
-int cmos_get_fd_drive_type(FloppyDriveType fd0)
-{
-int val;
-
-switch (fd0) {
-case FLOPPY_DRIVE_TYPE_144:
-/* 1.44 Mb 3"5 drive */
-val = 4;
-break;
-case FLOPPY_DRIVE_TYPE_288:
-/* 2.88 Mb 3"5 drive */
-val = 5;
-break;
-case FLOPPY_DRIVE_TYPE_120:
-/* 1.2 Mb 5"5 drive */
-val = 2;
-break;
-case FLOPPY_DRIVE_TYPE_NONE:
-default:
-val = 0;
-break;
-}
-return val;
-}
-
 static void cmos_init_hd(ISADevice *s, int type_ofs, int info_ofs,
  int16_t cylinders, int8_t heads, int8_t sectors)
 {
-- 
2.18.4

[PATCH v10 11/12] acpi: q35: drop _SB.PCI0.ISA.LPCD opregion.

[Gerd Hoffmann]

Seems to be unused.

ich9 DSDT changes:

 Scope (_SB.PCI0)
 {
 Device (ISA)
 {
 Name (_ADR, 0x001F)  // _ADR: Address
 OperationRegion (PIRQ, PCI_Config, 0x60, 0x0C)
-OperationRegion (LPCD, PCI_Config, 0x80, 0x02)
-Field (LPCD, AnyAcc, NoLock, Preserve)
-{
-COMA,   3,
-,   1,
-COMB,   3,
-Offset (0x01),
-LPTD,   2
-}
 }
 }

Signed-off-by: Gerd Hoffmann 
Reviewed-by: Igor Mammedov 
---
 hw/i386/acpi-build.c | 11 ---
 1 file changed, 11 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 59f1b4d89000..378515df66c5 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1340,7 +1340,6 @@ static void build_q35_isa_bridge(Aml *table)
 {
 Aml *dev;
 Aml *scope;
-Aml *field;
 
 scope =  aml_scope("_SB.PCI0");
 dev = aml_device("ISA");
@@ -1350,16 +1349,6 @@ static void build_q35_isa_bridge(Aml *table)
 aml_append(dev, aml_operation_region("PIRQ", AML_PCI_CONFIG,
  aml_int(0x60), 0x0C));
 
-aml_append(dev, aml_operation_region("LPCD", AML_PCI_CONFIG,
- aml_int(0x80), 0x02));
-field = aml_field("LPCD", AML_ANY_ACC, AML_NOLOCK, AML_PRESERVE);
-aml_append(field, aml_named_field("COMA", 3));
-aml_append(field, aml_reserved_field(1));
-aml_append(field, aml_named_field("COMB", 3));
-aml_append(field, aml_reserved_field(1));
-aml_append(field, aml_named_field("LPTD", 2));
-aml_append(dev, field);
-
 aml_append(scope, dev);
 aml_append(table, scope);
 }
-- 
2.18.4

[PATCH v10 08/12] acpi: simplify build_isa_devices_aml()

[Gerd Hoffmann]

x86 machines can have a single ISA bus only.

Signed-off-by: Gerd Hoffmann 
Reviewed-by: Igor Mammedov 
Reviewed-by: Philippe Mathieu-Daudé 
---
 hw/i386/acpi-build.c | 16 ++--
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 19e9c298dc8f..d27cecc877c4 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -979,18 +979,14 @@ static void build_isa_devices_aml(Aml *table)
 {
 VMBusBridge *vmbus_bridge = vmbus_bridge_find();
 bool ambiguous;
-
-Aml *scope = aml_scope("_SB.PCI0.ISA");
 Object *obj = object_resolve_path_type("", TYPE_ISA_BUS, );
+Aml *scope;
 
-if (ambiguous) {
-error_report("Multiple ISA busses, unable to define IPMI ACPI data");
-} else if (!obj) {
-error_report("No ISA bus, unable to define IPMI ACPI data");
-} else {
-build_acpi_ipmi_devices(scope, BUS(obj), "\\_SB.PCI0.ISA");
-isa_build_aml(ISA_BUS(obj), scope);
-}
+assert(obj && !ambiguous);
+
+scope = aml_scope("_SB.PCI0.ISA");
+build_acpi_ipmi_devices(scope, BUS(obj), "\\_SB.PCI0.ISA");
+isa_build_aml(ISA_BUS(obj), scope);
 
 if (vmbus_bridge) {
 aml_append(scope, build_vmbus_device_aml(vmbus_bridge));
-- 
2.18.4

[PATCH v10 12/12] tests/acpi: update expected data files

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann 
---
 tests/data/acpi/pc/DSDT   | Bin 5014 -> 4934 bytes
 tests/data/acpi/pc/DSDT.acpihmat  | Bin 6338 -> 6258 bytes
 tests/data/acpi/pc/DSDT.bridge| Bin 6873 -> 6793 bytes
 tests/data/acpi/pc/DSDT.cphp  | Bin 5477 -> 5397 bytes
 tests/data/acpi/pc/DSDT.dimmpxm   | Bin 6667 -> 6587 bytes
 tests/data/acpi/pc/DSDT.ipmikcs   | Bin 5086 -> 5006 bytes
 tests/data/acpi/pc/DSDT.memhp | Bin 6373 -> 6293 bytes
 tests/data/acpi/pc/DSDT.numamem   | Bin 5020 -> 4940 bytes
 tests/data/acpi/q35/DSDT  | Bin 7752 -> 7678 bytes
 tests/data/acpi/q35/DSDT.acpihmat | Bin 9076 -> 9002 bytes
 tests/data/acpi/q35/DSDT.bridge   | Bin 7769 -> 7695 bytes
 tests/data/acpi/q35/DSDT.cphp | Bin 8215 -> 8141 bytes
 tests/data/acpi/q35/DSDT.dimmpxm  | Bin 9405 -> 9331 bytes
 tests/data/acpi/q35/DSDT.ipmibt   | Bin 7827 -> 7753 bytes
 tests/data/acpi/q35/DSDT.memhp| Bin 9111 -> 9037 bytes
 tests/data/acpi/q35/DSDT.mmio64   | Bin 8882 -> 8808 bytes
 tests/data/acpi/q35/DSDT.numamem  | Bin 7758 -> 7684 bytes
 tests/data/acpi/q35/DSDT.tis  | Bin 8357 -> 8283 bytes
 18 files changed, 0 insertions(+), 0 deletions(-)

diff --git a/tests/data/acpi/pc/DSDT b/tests/data/acpi/pc/DSDT
index 
384a82dbb3cb0e9f47db6f4d08945631c2b72b56..6d0aaf729ac7d64cf966621adf276534de5cc555
 100644
GIT binary patch
delta 62
zcmbQHeoT$aCDoQ7eL^rC%>49{BR537k=rgeU1i1P!GFUJ$J3E3H%+5|g
JYOx!m5C9(`8>;{S

diff --git a/tests/data/acpi/pc/DSDT.acpihmat b/tests/data/acpi/pc/DSDT.acpihmat
index 
47ddfdb027b06dc2daa46be711c3f4640ce68320..2e5e02400b1bd2842989d395c573fc593f45503b
 100644
GIT binary patch
delta 63
zcmX?P_{o6FCDZZv3Dbv7^9k+UVN}qe1Nm3L3ERpXRu>DN4%p;5D!qEA-W;J
R#K4(}D}jq;^E5^saR3*D4`~1Z

delta 123
zcmexlaLAC$CDF-B8Wz4&0K_yA{5gXkv7fCxilj(A6xARcB0MuzBy
z07GMECI+tm0uHQ5%A8py>oQ7eL^rC%>49{BR537k=rgeU1i1P!GFUJ$J3E3H%+5|g
KYO@=ojyM2sP8?SN

diff --git a/tests/data/acpi/pc/DSDT.bridge b/tests/data/acpi/pc/DSDT.bridge
index 
d1e2fa9fb8c75160fc1fa46deed6a6a9cb515559..623c4c03585c47d4d28adc611823b7cce8f4a5c7
 100644
GIT binary patch
delta 63
zcmca<+G)z=66_MvDaF9R$gz=2j8RQZFFx2QKET=2Ai7D)GuSbnBi_*^hzBUo5Zw@9
RV^ad$BmnX}4$S}n

delta 123
zcmeA)y=ltj66_LkQ;LCs@%%oQ7eL^rC%>49{BR537k=rgeU1i1P!GFUJ$J3E3H%+5|g
KYO@=ovm^kEs~o!k

diff --git a/tests/data/acpi/pc/DSDT.cphp b/tests/data/acpi/pc/DSDT.cphp
index 
54f481faf1e336c0bbf5e774cd67220fe06e951b..e0a43ccdadae150c0f39599c85e4e21ed8fff2a4
 100644
GIT binary patch
delta 63
zcmaE=HC2ntCDDN4%p;5D!qEA-W;J
R#K4(}D}jq;^EAfu!T{rs4-5bR

delta 123
zcmbQL^;CoQ7eL^rC%>49{BR537k=rgeU1i1P!GFUJ$J3E3H%+5|g
KYO@>Td0_x-S{(WS

diff --git a/tests/data/acpi/pc/DSDT.dimmpxm b/tests/data/acpi/pc/DSDT.dimmpxm
index 
5d98016ae571cde04ff96d58212e0faf9aaf50e6..21eb065a0ee3bd96f1a2e7601aa83fefa833349a
 100644
GIT binary patch
delta 63
zcmeA+*=@|_66_MPTatl+QDGyO7^9k+UVN}qe1Nm3L3ERpXRu>DN4%p;5D!qEA-W;J
R#K4(}D}jq;^EAd%2>|(!4=4Zt

delta 123
zcmdmO+-<_;66_MfEycjV_-rGW7^A7GUVN}qe1Nm3L3ER3K!l+%p;5Dzm0BSUmU
zfT6K769dogG08W@jfL
Kwb_laR004i*%C

diff --git a/tests/data/acpi/pc/DSDT.ipmikcs b/tests/data/acpi/pc/DSDT.ipmikcs
index 
57b78358744a5bb13639ccddb887be2721240807..b8f08f266b5735fe6967d4e105ee6b3662dad7e6
 100644
GIT binary patch
delta 88
zcmcbo-lxvx66_MvC(OXW7`%~7jL}?8FFx2QKET=2Ai7D)GuSbnBi_*^hzBUo5Zw@9
jV!;3lKb3{wbFHMxz+Yw}9Q`ogG08W@jfL
eCEzBb~GuSbH@_fdJlOq{DHa9Tw2?794F(Zip

diff --git a/tests/data/acpi/pc/DSDT.memhp b/tests/data/acpi/pc/DSDT.memhp
index 
8cb90ef14e13be85995c6fe3d3f6d12f4d939504..9a9418f4bde5fb18883c244ea956122e371ff01a
 100644
GIT binary patch
delta 63
zcmaEAIMtBLCD
R69Z=^t^_WY?kD#Q_+=51Ie~

delta 123
zcmbPg_|%ZgCDoQ7eL^rC%>49{BR537k=rgeU1i1P!GFUJ$J3E3H%+5|g
KYO@=ozc>JL)EtNa

diff --git a/tests/data/acpi/pc/DSDT.numamem b/tests/data/acpi/pc/DSDT.numamem
index 
f194bc639482eb839a875d493857526f85f1a9e0..6eec385c2ec00544c6eaa7e19d32b2ccd5a51915
 100644
GIT binary patch
delta 63
zcmbQEenySUCD@oA7^9k+UVN}qe1Nm3L3ERpXRu>DN4%p;5D!qEA-W;J
R#K4(}D}jq;^E5^_Apr0P4vzo;

delta 123
zcmX@3HboQ7eL^rC%>49{BR537k=rgeU1i1P!GFUJ$J3E3H%+5|g
KYO@<7n-BmV+#7uW

diff --git a/tests/data/acpi/q35/DSDT b/tests/data/acpi/q35/DSDT
index 
6a5e4dd85a7d9a95f7ad0fb95e6a4fa7a8d91adb..e63676d7a63afec714debeb465ee478ea4714337
 100644
GIT binary patch
delta 63
zcmX?M^Us>gCDB|5BVXhFs

delta 152
zcmexoeZq#zCDgNCodb_^bFp7k5NGe0GZ(>WdHyG

diff --git a/tests/data/acpi/q35/DSDT.acpihmat 
b/tests/data/acpi/q35/DSDT.acpihmat
index 
c1dd7773f3386a946fcb4a9a3bf9ad3a33ddbbe9..cd97b819824e4140d087e465d179b71775d6a494
 100644
GIT binary patch
delta 63
zcmez3w#tpmCD(TV^lo)6ss

delta 152
zcmZ4G_Qj3MCDgNCodb_^bFp7kI_dF0FK)wf

diff --git a/tests/data/acpi/q35/DSDT.bridge b/tests/data/acpi/q35/DSDT.bridge
index 
2ef1e894a35b9e85fe07e2678bd2456f5ec40dc6..8b0fb497dbbaeba18e9d0e1503de4396f1c230b0
 100644
GIT binary patch
delta 63
zcmca<({ID&66_MfFUP>Z=(Uk+KBJnNUVN}qe1Nm3L3ERpXRu>DN4%p;5Dx=`JVSIt
SfM-x36ITKk^#`2N?hd0uLns

delta 152
zcmeCTxoN}Y66_KZDaXLTShA68K4ZNDyIy>-Q+$B4r$Ka+Gn;3yV?0N^qe~DE1A{z6

[PATCH v10 03/12] acpi: move aml builder code for floppy device

[Gerd Hoffmann]

DSDT change: isa device order changes in case MI1 (ipmi) is present.

Signed-off-by: Gerd Hoffmann 
Reviewed-by: Igor Mammedov 
---
 hw/block/fdc.c   | 83 
 hw/i386/acpi-build.c | 83 
 stubs/cmos.c |  7 
 stubs/Makefile.objs  |  1 +
 4 files changed, 91 insertions(+), 83 deletions(-)
 create mode 100644 stubs/cmos.c

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 8528b9a3c722..c92436772292 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -32,6 +32,8 @@
 #include "qapi/error.h"
 #include "qemu/error-report.h"
 #include "qemu/timer.h"
+#include "hw/i386/pc.h"
+#include "hw/acpi/aml-build.h"
 #include "hw/irq.h"
 #include "hw/isa/isa.h"
 #include "hw/qdev-properties.h"
@@ -2765,6 +2767,85 @@ void isa_fdc_get_drive_max_chs(FloppyDriveType type,
 (*maxc)--;
 }
 
+static Aml *build_fdinfo_aml(int idx, FloppyDriveType type)
+{
+Aml *dev, *fdi;
+uint8_t maxc, maxh, maxs;
+
+isa_fdc_get_drive_max_chs(type, , , );
+
+dev = aml_device("FLP%c", 'A' + idx);
+
+aml_append(dev, aml_name_decl("_ADR", aml_int(idx)));
+
+fdi = aml_package(16);
+aml_append(fdi, aml_int(idx));  /* Drive Number */
+aml_append(fdi,
+aml_int(cmos_get_fd_drive_type(type)));  /* Device Type */
+/*
+ * the values below are the limits of the drive, and are thus independent
+ * of the inserted media
+ */
+aml_append(fdi, aml_int(maxc));  /* Maximum Cylinder Number */
+aml_append(fdi, aml_int(maxs));  /* Maximum Sector Number */
+aml_append(fdi, aml_int(maxh));  /* Maximum Head Number */
+/*
+ * SeaBIOS returns the below values for int 0x13 func 0x08 regardless of
+ * the drive type, so shall we
+ */
+aml_append(fdi, aml_int(0xAF));  /* disk_specify_1 */
+aml_append(fdi, aml_int(0x02));  /* disk_specify_2 */
+aml_append(fdi, aml_int(0x25));  /* disk_motor_wait */
+aml_append(fdi, aml_int(0x02));  /* disk_sector_siz */
+aml_append(fdi, aml_int(0x12));  /* disk_eot */
+aml_append(fdi, aml_int(0x1B));  /* disk_rw_gap */
+aml_append(fdi, aml_int(0xFF));  /* disk_dtl */
+aml_append(fdi, aml_int(0x6C));  /* disk_formt_gap */
+aml_append(fdi, aml_int(0xF6));  /* disk_fill */
+aml_append(fdi, aml_int(0x0F));  /* disk_head_sttl */
+aml_append(fdi, aml_int(0x08));  /* disk_motor_strt */
+
+aml_append(dev, aml_name_decl("_FDI", fdi));
+return dev;
+}
+
+static void fdc_isa_build_aml(ISADevice *isadev, Aml *scope)
+{
+Aml *dev;
+Aml *crs;
+int i;
+
+#define ACPI_FDE_MAX_FD 4
+uint32_t fde_buf[5] = {
+0, 0, 0, 0, /* presence of floppy drives #0 - #3 */
+cpu_to_le32(2)  /* tape presence (2 == never present) */
+};
+
+crs = aml_resource_template();
+aml_append(crs, aml_io(AML_DECODE16, 0x03F2, 0x03F2, 0x00, 0x04));
+aml_append(crs, aml_io(AML_DECODE16, 0x03F7, 0x03F7, 0x00, 0x01));
+aml_append(crs, aml_irq_no_flags(6));
+aml_append(crs,
+aml_dma(AML_COMPATIBILITY, AML_NOTBUSMASTER, AML_TRANSFER8, 2));
+
+dev = aml_device("FDC0");
+aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0700")));
+aml_append(dev, aml_name_decl("_CRS", crs));
+
+for (i = 0; i < MIN(MAX_FD, ACPI_FDE_MAX_FD); i++) {
+FloppyDriveType type = isa_fdc_get_drive_type(isadev, i);
+
+if (type < FLOPPY_DRIVE_TYPE_NONE) {
+fde_buf[i] = cpu_to_le32(1);  /* drive present */
+aml_append(dev, build_fdinfo_aml(i, type));
+}
+}
+aml_append(dev, aml_name_decl("_FDE",
+   aml_buffer(sizeof(fde_buf), (uint8_t *)fde_buf)));
+
+aml_append(scope, dev);
+}
+
 static const VMStateDescription vmstate_isa_fdc ={
 .name = "fdc",
 .version_id = 2,
@@ -2798,11 +2879,13 @@ static Property isa_fdc_properties[] = {
 static void isabus_fdc_class_init(ObjectClass *klass, void *data)
 {
 DeviceClass *dc = DEVICE_CLASS(klass);
+ISADeviceClass *isa = ISA_DEVICE_CLASS(klass);
 
 dc->realize = isabus_fdc_realize;
 dc->fw_name = "fdc";
 dc->reset = fdctrl_external_reset_isa;
 dc->vmsd = _isa_fdc;
+isa->build_aml = fdc_isa_build_aml;
 device_class_set_props(dc, isa_fdc_properties);
 set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
 }
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 900f786d08de..45297d9a90e7 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -938,85 +938,6 @@ static void build_hpet_aml(Aml *table)
 aml_append(table, scope);
 }
 
-static Aml *build_fdinfo_aml(int idx, FloppyDriveType type)
-{
-Aml *dev, *fdi;
-uint8_t maxc, maxh, maxs;
-
-isa_fdc_get_drive_max_chs(type, , , );
-
-dev = aml_device("FLP%c", 'A' + idx);
-
-aml_append(dev, aml_name_decl("_ADR", aml_int(idx)));
-
-fdi = aml_package(16);
-aml_append(fdi, aml_int(idx));  /* Drive Number */
-aml_append(fdi,
-

[PATCH v10 10/12] acpi: drop build_piix4_pm()

[Gerd Hoffmann]

The _SB.PCI0.PX13.P13C opregion (holds isa device enable bits)
is not used any more, remove it from DSDT.

piix4 DSDT changes:

 Scope (_SB.PCI0)
 {
-Device (PX13)
-{
-Name (_ADR, 0x00010003)  // _ADR: Address
-OperationRegion (P13C, PCI_Config, Zero, 0xFF)
-}
-}
-
-Scope (_SB.PCI0)
-{
 Device (ISA)
 {
 Name (_ADR, 0x0001)  // _ADR: Address
 OperationRegion (P40C, PCI_Config, 0x60, 0x04)
 }
 }

Signed-off-by: Gerd Hoffmann 
Reviewed-by: Igor Mammedow 
---
 hw/i386/acpi-build.c | 16 
 1 file changed, 16 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index ffbdbee51aa8..59f1b4d89000 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1364,21 +1364,6 @@ static void build_q35_isa_bridge(Aml *table)
 aml_append(table, scope);
 }
 
-static void build_piix4_pm(Aml *table)
-{
-Aml *dev;
-Aml *scope;
-
-scope =  aml_scope("_SB.PCI0");
-dev = aml_device("PX13");
-aml_append(dev, aml_name_decl("_ADR", aml_int(0x00010003)));
-
-aml_append(dev, aml_operation_region("P13C", AML_PCI_CONFIG,
- aml_int(0x00), 0xff));
-aml_append(scope, dev);
-aml_append(table, scope);
-}
-
 static void build_piix4_isa_bridge(Aml *table)
 {
 Aml *dev;
@@ -1530,7 +1515,6 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
 aml_append(dsdt, sb_scope);
 
 build_hpet_aml(dsdt);
-build_piix4_pm(dsdt);
 build_piix4_isa_bridge(dsdt);
 build_isa_devices_aml(dsdt);
 build_piix4_pci_hotplug(dsdt);
-- 
2.18.4

[PATCH v10 07/12] acpi: factor out fw_cfg_add_acpi_dsdt()

[Gerd Hoffmann]

Add helper function to add fw_cfg device,
also move code to hw/i386/fw_cfg.c.

Signed-off-by: Gerd Hoffmann 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Igor Mammedov 
---
 hw/i386/fw_cfg.h |  1 +
 hw/i386/acpi-build.c | 24 +---
 hw/i386/fw_cfg.c | 28 
 3 files changed, 30 insertions(+), 23 deletions(-)

diff --git a/hw/i386/fw_cfg.h b/hw/i386/fw_cfg.h
index 9e742787792b..275f15c1c5e8 100644
--- a/hw/i386/fw_cfg.h
+++ b/hw/i386/fw_cfg.h
@@ -25,5 +25,6 @@ FWCfgState *fw_cfg_arch_create(MachineState *ms,
uint16_t apic_id_limit);
 void fw_cfg_build_smbios(MachineState *ms, FWCfgState *fw_cfg);
 void fw_cfg_build_feature_control(MachineState *ms, FWCfgState *fw_cfg);
+void fw_cfg_add_acpi_dsdt(Aml *scope, FWCfgState *fw_cfg);
 
 #endif
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 13113e83dfe2..19e9c298dc8f 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1802,30 +1802,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
 
 /* create fw_cfg node, unconditionally */
 {
-/* when using port i/o, the 8-bit data register *always* overlaps
- * with half of the 16-bit control register. Hence, the total size
- * of the i/o region used is FW_CFG_CTL_SIZE; when using DMA, the
- * DMA control register is located at FW_CFG_DMA_IO_BASE + 4 */
-uint8_t io_size = object_property_get_bool(OBJECT(x86ms->fw_cfg),
-   "dma_enabled", NULL) ?
-  ROUND_UP(FW_CFG_CTL_SIZE, 4) + sizeof(dma_addr_t) :
-  FW_CFG_CTL_SIZE;
-
 scope = aml_scope("\\_SB.PCI0");
-dev = aml_device("FWCF");
-
-aml_append(dev, aml_name_decl("_HID", aml_string("QEMU0002")));
-
-/* device present, functioning, decoding, not shown in UI */
-aml_append(dev, aml_name_decl("_STA", aml_int(0xB)));
-
-crs = aml_resource_template();
-aml_append(crs,
-aml_io(AML_DECODE16, FW_CFG_IO_BASE, FW_CFG_IO_BASE, 0x01, io_size)
-);
-aml_append(dev, aml_name_decl("_CRS", crs));
-
-aml_append(scope, dev);
+fw_cfg_add_acpi_dsdt(scope, x86ms->fw_cfg);
 aml_append(dsdt, scope);
 }
 
diff --git a/hw/i386/fw_cfg.c b/hw/i386/fw_cfg.c
index da60ada59462..c55abfb01abb 100644
--- a/hw/i386/fw_cfg.c
+++ b/hw/i386/fw_cfg.c
@@ -15,6 +15,7 @@
 #include "qemu/osdep.h"
 #include "sysemu/numa.h"
 #include "hw/acpi/acpi.h"
+#include "hw/acpi/aml-build.h"
 #include "hw/firmware/smbios.h"
 #include "hw/i386/fw_cfg.h"
 #include "hw/timer/hpet.h"
@@ -179,3 +180,30 @@ void fw_cfg_build_feature_control(MachineState *ms, 
FWCfgState *fw_cfg)
 *val = cpu_to_le64(feature_control_bits | FEATURE_CONTROL_LOCKED);
 fw_cfg_add_file(fw_cfg, "etc/msr_feature_control", val, sizeof(*val));
 }
+
+void fw_cfg_add_acpi_dsdt(Aml *scope, FWCfgState *fw_cfg)
+{
+/*
+ * when using port i/o, the 8-bit data register *always* overlaps
+ * with half of the 16-bit control register. Hence, the total size
+ * of the i/o region used is FW_CFG_CTL_SIZE; when using DMA, the
+ * DMA control register is located at FW_CFG_DMA_IO_BASE + 4
+ */
+Object *obj = OBJECT(fw_cfg);
+uint8_t io_size = object_property_get_bool(obj, "dma_enabled", NULL) ?
+ROUND_UP(FW_CFG_CTL_SIZE, 4) + sizeof(dma_addr_t) :
+FW_CFG_CTL_SIZE;
+Aml *dev = aml_device("FWCF");
+Aml *crs = aml_resource_template();
+
+aml_append(dev, aml_name_decl("_HID", aml_string("QEMU0002")));
+
+/* device present, functioning, decoding, not shown in UI */
+aml_append(dev, aml_name_decl("_STA", aml_int(0xB)));
+
+aml_append(crs,
+aml_io(AML_DECODE16, FW_CFG_IO_BASE, FW_CFG_IO_BASE, 0x01, io_size));
+
+aml_append(dev, aml_name_decl("_CRS", crs));
+aml_append(scope, dev);
+}
-- 
2.18.4

[PATCH v10 09/12] acpi: drop serial/parallel enable bits from dsdt

[Gerd Hoffmann]

The _STA methods for COM+LPT used to reference them,
but that isn't the case any more.

piix4 DSDT changes:

 Scope (_SB.PCI0)
 {
 Device (ISA)
 {
 Name (_ADR, 0x0001)  // _ADR: Address
 OperationRegion (P40C, PCI_Config, 0x60, 0x04)
-Field (^PX13.P13C, AnyAcc, NoLock, Preserve)
-{
-Offset (0x5F),
-,   7,
-LPEN,   1,
-Offset (0x67),
-,   3,
-CAEN,   1,
-,   3,
-CBEN,   1
-}
 }
 }

ich9 DSDT changes:

 Scope (_SB.PCI0)
 {
 Device (ISA)
 {
 Name (_ADR, 0x001F)  // _ADR: Address
 OperationRegion (PIRQ, PCI_Config, 0x60, 0x0C)
 OperationRegion (LPCD, PCI_Config, 0x80, 0x02)
 Field (LPCD, AnyAcc, NoLock, Preserve)
 {
 COMA,   3,
 ,   1,
 COMB,   3,
 Offset (0x01),
 LPTD,   2
 }
-
-OperationRegion (LPCE, PCI_Config, 0x82, 0x02)
-Field (LPCE, AnyAcc, NoLock, Preserve)
-{
-CAEN,   1,
-CBEN,   1,
-LPEN,   1
-}
 }
 }

Signed-off-by: Gerd Hoffmann 
Reviewed-by: Igor Mammedov 
---
 hw/i386/acpi-build.c | 23 ---
 1 file changed, 23 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index d27cecc877c4..ffbdbee51aa8 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1360,15 +1360,6 @@ static void build_q35_isa_bridge(Aml *table)
 aml_append(field, aml_named_field("LPTD", 2));
 aml_append(dev, field);
 
-aml_append(dev, aml_operation_region("LPCE", AML_PCI_CONFIG,
- aml_int(0x82), 0x02));
-/* enable bits */
-field = aml_field("LPCE", AML_ANY_ACC, AML_NOLOCK, AML_PRESERVE);
-aml_append(field, aml_named_field("CAEN", 1));
-aml_append(field, aml_named_field("CBEN", 1));
-aml_append(field, aml_named_field("LPEN", 1));
-aml_append(dev, field);
-
 aml_append(scope, dev);
 aml_append(table, scope);
 }
@@ -1392,7 +1383,6 @@ static void build_piix4_isa_bridge(Aml *table)
 {
 Aml *dev;
 Aml *scope;
-Aml *field;
 
 scope =  aml_scope("_SB.PCI0");
 dev = aml_device("ISA");
@@ -1401,19 +1391,6 @@ static void build_piix4_isa_bridge(Aml *table)
 /* PIIX PCI to ISA irq remapping */
 aml_append(dev, aml_operation_region("P40C", AML_PCI_CONFIG,
  aml_int(0x60), 0x04));
-/* enable bits */
-field = aml_field("^PX13.P13C", AML_ANY_ACC, AML_NOLOCK, AML_PRESERVE);
-/* Offset(0x5f),, 7, */
-aml_append(field, aml_reserved_field(0x2f8));
-aml_append(field, aml_reserved_field(7));
-aml_append(field, aml_named_field("LPEN", 1));
-/* Offset(0x67),, 3, */
-aml_append(field, aml_reserved_field(0x38));
-aml_append(field, aml_reserved_field(3));
-aml_append(field, aml_named_field("CAEN", 1));
-aml_append(field, aml_reserved_field(3));
-aml_append(field, aml_named_field("CBEN", 1));
-aml_append(dev, field);
 
 aml_append(scope, dev);
 aml_append(table, scope);
-- 
2.18.4

[PATCH v10 06/12] acpi: move aml builder code for i8042 (kbd+mouse) device

[Gerd Hoffmann]

DSDT change: isa device order changes in case MI1 (ipmi) is present.

Signed-off-by: Gerd Hoffmann 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Igor Mammedov 
---
 hw/i386/acpi-build.c | 39 ---
 hw/input/pckbd.c | 31 +++
 2 files changed, 31 insertions(+), 39 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 45297d9a90e7..13113e83dfe2 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -938,42 +938,6 @@ static void build_hpet_aml(Aml *table)
 aml_append(table, scope);
 }
 
-static Aml *build_kbd_device_aml(void)
-{
-Aml *dev;
-Aml *crs;
-
-dev = aml_device("KBD");
-aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0303")));
-
-aml_append(dev, aml_name_decl("_STA", aml_int(0xf)));
-
-crs = aml_resource_template();
-aml_append(crs, aml_io(AML_DECODE16, 0x0060, 0x0060, 0x01, 0x01));
-aml_append(crs, aml_io(AML_DECODE16, 0x0064, 0x0064, 0x01, 0x01));
-aml_append(crs, aml_irq_no_flags(1));
-aml_append(dev, aml_name_decl("_CRS", crs));
-
-return dev;
-}
-
-static Aml *build_mouse_device_aml(void)
-{
-Aml *dev;
-Aml *crs;
-
-dev = aml_device("MOU");
-aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0F13")));
-
-aml_append(dev, aml_name_decl("_STA", aml_int(0xf)));
-
-crs = aml_resource_template();
-aml_append(crs, aml_irq_no_flags(12));
-aml_append(dev, aml_name_decl("_CRS", crs));
-
-return dev;
-}
-
 static Aml *build_vmbus_device_aml(VMBusBridge *vmbus_bridge)
 {
 Aml *dev;
@@ -1019,9 +983,6 @@ static void build_isa_devices_aml(Aml *table)
 Aml *scope = aml_scope("_SB.PCI0.ISA");
 Object *obj = object_resolve_path_type("", TYPE_ISA_BUS, );
 
-aml_append(scope, build_kbd_device_aml());
-aml_append(scope, build_mouse_device_aml());
-
 if (ambiguous) {
 error_report("Multiple ISA busses, unable to define IPMI ACPI data");
 } else if (!obj) {
diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c
index 60a41303203a..29d633ca9478 100644
--- a/hw/input/pckbd.c
+++ b/hw/input/pckbd.c
@@ -26,6 +26,7 @@
 #include "qemu/log.h"
 #include "hw/isa/isa.h"
 #include "migration/vmstate.h"
+#include "hw/acpi/aml-build.h"
 #include "hw/input/ps2.h"
 #include "hw/irq.h"
 #include "hw/input/i8042.h"
@@ -561,12 +562,42 @@ static void i8042_realizefn(DeviceState *dev, Error 
**errp)
 qemu_register_reset(kbd_reset, s);
 }
 
+static void i8042_build_aml(ISADevice *isadev, Aml *scope)
+{
+Aml *kbd;
+Aml *mou;
+Aml *crs;
+
+crs = aml_resource_template();
+aml_append(crs, aml_io(AML_DECODE16, 0x0060, 0x0060, 0x01, 0x01));
+aml_append(crs, aml_io(AML_DECODE16, 0x0064, 0x0064, 0x01, 0x01));
+aml_append(crs, aml_irq_no_flags(1));
+
+kbd = aml_device("KBD");
+aml_append(kbd, aml_name_decl("_HID", aml_eisaid("PNP0303")));
+aml_append(kbd, aml_name_decl("_STA", aml_int(0xf)));
+aml_append(kbd, aml_name_decl("_CRS", crs));
+
+crs = aml_resource_template();
+aml_append(crs, aml_irq_no_flags(12));
+
+mou = aml_device("MOU");
+aml_append(mou, aml_name_decl("_HID", aml_eisaid("PNP0F13")));
+aml_append(mou, aml_name_decl("_STA", aml_int(0xf)));
+aml_append(mou, aml_name_decl("_CRS", crs));
+
+aml_append(scope, kbd);
+aml_append(scope, mou);
+}
+
 static void i8042_class_initfn(ObjectClass *klass, void *data)
 {
 DeviceClass *dc = DEVICE_CLASS(klass);
+ISADeviceClass *isa = ISA_DEVICE_CLASS(klass);
 
 dc->realize = i8042_realizefn;
 dc->vmsd = _kbd_isa;
+isa->build_aml = i8042_build_aml;
 set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
 }
 
-- 
2.18.4

[PATCH v10 04/12] floppy: make isa_fdc_get_drive_max_chs static

[Gerd Hoffmann]

acpi aml generator needs this, but it is in floppy code now
so we can make the function static.

Signed-off-by: Gerd Hoffmann 
Reviewed-by: Igor Mammedov 
Reviewed-by: Philippe Mathieu-Daudé 
Acked-by: John Snow 
---
 include/hw/block/fdc.h | 2 --
 hw/block/fdc.c | 4 ++--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/include/hw/block/fdc.h b/include/hw/block/fdc.h
index c15ff4c62315..5d71cf972268 100644
--- a/include/hw/block/fdc.h
+++ b/include/hw/block/fdc.h
@@ -16,7 +16,5 @@ void sun4m_fdctrl_init(qemu_irq irq, hwaddr io_base,
DriveInfo **fds, qemu_irq *fdc_tc);
 
 FloppyDriveType isa_fdc_get_drive_type(ISADevice *fdc, int i);
-void isa_fdc_get_drive_max_chs(FloppyDriveType type,
-   uint8_t *maxc, uint8_t *maxh, uint8_t *maxs);
 
 #endif
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index c92436772292..5a634ab46302 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -2744,8 +2744,8 @@ FloppyDriveType isa_fdc_get_drive_type(ISADevice *fdc, 
int i)
 return isa->state.drives[i].drive;
 }
 
-void isa_fdc_get_drive_max_chs(FloppyDriveType type,
-   uint8_t *maxc, uint8_t *maxh, uint8_t *maxs)
+static void isa_fdc_get_drive_max_chs(FloppyDriveType type, uint8_t *maxc,
+  uint8_t *maxh, uint8_t *maxs)
 {
 const FDFormat *fdf;
 
-- 
2.18.4

[PATCH v10 02/12] acpi: bios-tables-test: show more context on asl diffs

[Gerd Hoffmann]

Makes it easier to create good commit messages from the logs.

Signed-off-by: Gerd Hoffmann 
Reviewed-by: Philippe Mathieu-Daudé 
---
 tests/qtest/bios-tables-test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
index b482f76c03d4..c315156858f4 100644
--- a/tests/qtest/bios-tables-test.c
+++ b/tests/qtest/bios-tables-test.c
@@ -469,7 +469,7 @@ static void test_acpi_asl(test_data *data)
 fflush(stderr);
 if (getenv("V")) {
 const char *diff_env = getenv("DIFF");
-const char *diff_cmd = diff_env ? diff_env : "diff -u";
+const char *diff_cmd = diff_env ? diff_env : "diff -U 16";
 char *diff = g_strdup_printf("%s %s %s", diff_cmd,
  exp_sdt->asl_file, 
sdt->asl_file);
 int out = dup(STDOUT_FILENO);
-- 
2.18.4

[PATCH v10 01/12] qtest: allow DSDT acpi table changes

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann 
---
 tests/qtest/bios-tables-test-allowed-diff.h | 18 ++
 1 file changed, 18 insertions(+)

diff --git a/tests/qtest/bios-tables-test-allowed-diff.h 
b/tests/qtest/bios-tables-test-allowed-diff.h
index dfb8523c8bf4..8992f1f12b77 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1 +1,19 @@
 /* List of comma-separated changed AML files to ignore */
+"tests/data/acpi/pc/DSDT",
+"tests/data/acpi/pc/DSDT.acpihmat",
+"tests/data/acpi/pc/DSDT.bridge",
+"tests/data/acpi/pc/DSDT.cphp",
+"tests/data/acpi/pc/DSDT.dimmpxm",
+"tests/data/acpi/pc/DSDT.ipmikcs",
+"tests/data/acpi/pc/DSDT.memhp",
+"tests/data/acpi/pc/DSDT.numamem",
+"tests/data/acpi/q35/DSDT",
+"tests/data/acpi/q35/DSDT.acpihmat",
+"tests/data/acpi/q35/DSDT.bridge",
+"tests/data/acpi/q35/DSDT.cphp",
+"tests/data/acpi/q35/DSDT.dimmpxm",
+"tests/data/acpi/q35/DSDT.ipmibt",
+"tests/data/acpi/q35/DSDT.memhp",
+"tests/data/acpi/q35/DSDT.mmio64",
+"tests/data/acpi/q35/DSDT.numamem",
+"tests/data/acpi/q35/DSDT.tis",
-- 
2.18.4

[PATCH v10 00/12] acpi: i386 tweaks

[Gerd Hoffmann]

First batch of microvm patches, some generic acpi stuff.
Split the acpi-build.c monster, specifically split the
pc and q35 and pci bits into a separate file which we
can skip building at some point in the future.

v2 changes: leave acpi-build.c largely as-is, move useful
bits to other places to allow them being reused, specifically:

 * move isa device generator functions to individual isa devices.
 * move fw_cfg generator function to fw_cfg.c

v3 changes: fix rtc, support multiple lpt devices.

v4 changes:
 * drop merged patches.
 * split rtc crs change to separata patch.
 * added two cleanup patches.
 * picked up ack & review tags.

v5 changes:
 * add comment for rtc crs update.
 * add even more cleanup patches.
 * picked up ack & review tags.

v6 changes:
 * floppy: move cmos_get_fd_drive_type.
 * picked up ack & review tags.

v7 changes:
 * rebased to mst/pci branch, resolved stubs conflict.
 * dropped patches already queued up in mst/pci.
 * added missing sign-off.
 * picked up ack & review tags.

v8 changes:
 * (re-)add patch to allow acpi table changes

v9 changes:
 * add asl changes to commit messages.
 * update acpi test data.

v10 changes:
 * move acpi test data update to separate commit.

take care,
  Gerd

Gerd Hoffmann (12):
  qtest: allow DSDT acpi table changes
  acpi: bios-tables-test: show more context on asl diffs
  acpi: move aml builder code for floppy device
  floppy: make isa_fdc_get_drive_max_chs static
  floppy: move cmos_get_fd_drive_type() from pc
  acpi: move aml builder code for i8042 (kbd+mouse) device
  acpi: factor out fw_cfg_add_acpi_dsdt()
  acpi: simplify build_isa_devices_aml()
  acpi: drop serial/parallel enable bits from dsdt
  acpi: drop build_piix4_pm()
  acpi: q35: drop _SB.PCI0.ISA.LPCD opregion.
  tests/acpi: update expected data files

 hw/i386/fw_cfg.h|   1 +
 include/hw/block/fdc.h  |   3 +-
 include/hw/i386/pc.h|   1 -
 tests/qtest/bios-tables-test-allowed-diff.h |  18 ++
 hw/block/fdc.c  | 111 ++-
 hw/i386/acpi-build.c| 210 +---
 hw/i386/fw_cfg.c|  28 +++
 hw/i386/pc.c|  25 ---
 hw/input/pckbd.c|  31 +++
 stubs/cmos.c|   7 +
 tests/qtest/bios-tables-test.c  |   2 +-
 stubs/Makefile.objs |   1 +
 tests/data/acpi/pc/DSDT | Bin 5014 -> 4934 bytes
 tests/data/acpi/pc/DSDT.acpihmat| Bin 6338 -> 6258 bytes
 tests/data/acpi/pc/DSDT.bridge  | Bin 6873 -> 6793 bytes
 tests/data/acpi/pc/DSDT.cphp| Bin 5477 -> 5397 bytes
 tests/data/acpi/pc/DSDT.dimmpxm | Bin 6667 -> 6587 bytes
 tests/data/acpi/pc/DSDT.ipmikcs | Bin 5086 -> 5006 bytes
 tests/data/acpi/pc/DSDT.memhp   | Bin 6373 -> 6293 bytes
 tests/data/acpi/pc/DSDT.numamem | Bin 5020 -> 4940 bytes
 tests/data/acpi/q35/DSDT| Bin 7752 -> 7678 bytes
 tests/data/acpi/q35/DSDT.acpihmat   | Bin 9076 -> 9002 bytes
 tests/data/acpi/q35/DSDT.bridge | Bin 7769 -> 7695 bytes
 tests/data/acpi/q35/DSDT.cphp   | Bin 8215 -> 8141 bytes
 tests/data/acpi/q35/DSDT.dimmpxm| Bin 9405 -> 9331 bytes
 tests/data/acpi/q35/DSDT.ipmibt | Bin 7827 -> 7753 bytes
 tests/data/acpi/q35/DSDT.memhp  | Bin 9111 -> 9037 bytes
 tests/data/acpi/q35/DSDT.mmio64 | Bin 8882 -> 8808 bytes
 tests/data/acpi/q35/DSDT.numamem| Bin 7758 -> 7684 bytes
 tests/data/acpi/q35/DSDT.tis| Bin 8357 -> 8283 bytes
 30 files changed, 203 insertions(+), 235 deletions(-)
 create mode 100644 stubs/cmos.c

-- 
2.18.4

Re: pls consider this is [v3] Re: [PATCH 0/2] block: propagate discard alignment from format drivers to the guest

[Denis V. Lunev]

On 6/11/20 8:21 PM, Denis V. Lunev wrote:
> On 6/11/20 8:16 PM, Denis V. Lunev wrote:
>> Nowaday SCSI drivers in guests are able to align UNMAP requests before
>> sending to the device. Right now QEMU provides an ability to set
>> this via "discard_granularity" property of the block device which could
>> be used by management layer.
>>
>> Though, in particular, from the point of QEMU, there is
>> pdiscard_granularity on the format driver level, f.e. on QCOW2 or iSCSI.
>> It would be beneficial to pass this value as a default for this
>> property.
>>
>> Technically this should reduce the amount of use less UNMAP requests
>> from the guest to the host. Basic test confirms this. Fedora 31 guest
>> during 'fstrim /' on 32 Gb disk has issued 401/415 requests with/without
>> proper alignment to QEMU.
>>
>> Changes from v2:
>> - 172 iotest fixed
>>
>> Changes from v1:
>> - fixed typos in description
>> - added machine type compatibility layer as suggested by Kevin
>>
>> Signed-off-by: Denis V. Lunev 
>> CC: Kevin Wolf 
>> CC: Max Reitz 
>> CC: Eduardo Habkost 
>> CC: Marcel Apfelbaum 
>> CC: John Snow 
>> CC: Paolo Bonzini 
>> CC: Fam Zheng 
>>
>>
> Sorry for missed v3 tag in the subject :(
ping

Re: [PATCH for-5.1] qcow2: Don't open images with a backing file and the data-file-raw bit

[Max Reitz]

On 18.06.20 14:03, Alberto Garcia wrote:
> On Wed 03 Jun 2020 03:53:03 PM CEST, Max Reitz wrote:
>> Sorry for the long delay. :/
> 
> And sorry for my long delay as well.
> 
>> The patch itself looks good, but I’m not sure whether it is extensive
>> enough.  Let me just jump straight to the problem:
>>
>> $ ./qemu-img create -f qcow2 \
>> -o data_file=foo.qcow2.raw,data_file_raw=on \
>> foo.qcow2 64M
>> (Create some file empty foo.qcow2 with external data file that’s raw)
>>
>> $ ./qemu-img create -f qcow2 backing.qcow2 64M
>> $ ./qemu-io -c 'write -P 42 0 64M' backing.qcow2
>> (Create some file filled with 42s)
>>
>> $ ./qemu-img compare foo.qcow2 foo.qcow2.raw
>> Images are identical.
>> (As expected, foo.qcow2 is identical to its raw data file)
>>
>> $ ./qemu-img compare --image-opts \
>> file.filename=foo.qcow2,backing.file.filename=backing.qcow2 \
>> file.filename=foo.qcow2.raw
>> Content mismatch at offset 0!
>> (Oops.)
> 
> If two images have the same contents but then you compare them changing
> the backing file of one of them you can also get a content mismatch. How
> is this different?

It’s different in that files with data-file-raw can’t have backing files
at all.  So maybe users shouldn’t be allowed to give them backing files
at runtime either.

Or at least, if we have data-file-raw, *all* data visible on such an
image should be taken from the raw data file, never from any backing file.

> Regardless of how we should ideally handle bs->backing and data-file-raw
> (and yes I agree that it would be nice that QEMU would say "you cannot
> have a backing file and an external raw file" in all cases) I think that
> if the problem is that the user can override the backing file name and
> get different contents, then that's not a problem that we should be
> worried about.

Well, not really be worried about, but I do think it’s indicative of
some problem, though I’m not sure whether the problem is error
reporting.  I think it’s rather the fact that data-file-raw should imply
metadata preallocation.

With preallocation, we’d ensure that we always take all data from the
raw data file.  So we’d always ignore any potential backing file.

(It makes sense to guard users against giving images with data-file-raw
a backing file, and to consider such images corrupt, as done by this
patch.  But if users can force a backing file at runtime, I think
showing its contents is another bug.)

Max



signature.asc
Description: OpenPGP digital signature