Re: [PATCH v3 1/6] block: add bitmap-populate job
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
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
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, &bs, 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(&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;
[PATCH v3 2/6] blockdev: combine DriveBackupState and BlockdevBackupState
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
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 WITH
[PATCH v3 3/6] qmp: expose block-dirty-bitmap-populate
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, &bs, 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, +.u.block_dirty_b
[PATCH v3 1/6] block: add bitmap-populate job
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
[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
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(&s->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
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 *)&n->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 *)&n->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
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
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
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(&lock); > > > +instance = yank_find_instance(instance_name); > > > +assert(instance); > > > + > > > +entry = g_slice_new(struct YankFuncAndParam); > > > +entry->func = func; > > > +entry->opaque = opaque; > > > + > > > +QLIST_INSERT_HEAD(&instance->yankfns, entry, next); > > > +qemu_mutex_unlock(&lock); > > > +} > > > + > > > +void yank_unregister_function(char *instance_name, YankFn *func, void > > > *opaque) > > > +{ > > > +struct YankInstance *instance; > > > +struct YankFuncAndParam *entry; > > > + > > > +qemu_mutex_lock(&lock); > > > +instance = yank_find_instance(instance_name); > > > +assert(instance); > > > + > > > +QLIST_FOREACH(entry, &instance->yankfns, next) { > > > +if (entry->func == func && entry->opaque == opaque) { > > > +QLIST_REMOVE(entry, next); > > > +g_slice_free(struct YankFuncAndParam, entry); > > > +qemu_mutex_unlock(&lock); > > > +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
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
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
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
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
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
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(&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(&lock); > > > > > +return; > > > > > +} > > > > > +} > > > > > +tmp = instances; > > > > > +for (; tmp; tmp = tmp->next) { > > > > > +instance = yank_find_instance(tmp->value); > > > > > +assert(instance); > > > > > +QLIST_FOREACH(entry, &instance->yankfns, next) { > > > > > +entry->func(entry->opaque); > > > > > +} > > > > > +} > > > > > +qemu_mutex_unlock(&lock); > > > > > +} > > > > > > > > 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
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
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
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
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
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
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
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(&lock); > > +instance = yank_find_instance(instance_name); > > +assert(instance); > > + > > +entry = g_slice_new(struct YankFuncAndParam); > > +entry->func = func; > > +entry->opaque = opaque; > > + > > +QLIST_INSERT_HEAD(&instance->yankfns, entry, next); > > +qemu_mutex_unlock(&lock); > > +} > > + > > +void yank_unregister_function(char *instance_name, YankFn *func, void > > *opaque) > > +{ > > +struct YankInstance *instance; > > +struct YankFuncAndParam *entry; > > + > > +qemu_mutex_lock(&lock); > > +instance = yank_find_instance(instance_name); > > +assert(instance); > > + > > +QLIST_FOREACH(entry, &instance->yankfns, next) { > > +if (entry->func == func && entry->opaque == opaque) { > > +QLIST_REMOVE(entry, next); > > +g_slice_free(struct YankFuncAndParam, entry); > > +qemu_mutex_unlock(&lock); > > +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
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
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 0x560caa8efb
Re: [PATCH 0/2] qemu-storage-daemon: memory leak and --object opts fixes
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
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 /usr/lib64/clang/10.0.0/lib/li
Re: [PATCH v10 00/12] acpi: i386 tweaks
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 > /usr/bin/ld:
Re: [PATCH 0/2] qemu-storage-daemon: memory leak and --object opts fixes
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 `__i
Re: [PATCH for-5.1] qcow2: Don't open images with a backing file and the data-file-raw bit
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
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: commo
[PATCH 1/2] qcow2: Force preallocation with data-file-raw
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 0/2] qcow2: Force preallocation with data-file-raw
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] iotests/244: Test preallocation for data-file-raw
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 2/2] qemu-storage-daemon: add missing cleanup calls
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
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(&qemu_object_opts); qemu_add_opts(&qemu_trace_opts); qcrypto_init(&error_fatal); bdrv_init(); -- 2.26.2
[PATCH 0/2] qemu-storage-daemon: memory leak and --object opts fixes
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
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
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(&pool->waiters); } -void coroutine_fn aio_task_pool_wait_one(AioTaskPool *pool) -{ -assert(pool->busy_tasks > 0); - -qemu_co_queue_wait(&pool->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(&pool->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(&pool->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
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(&pool->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(&pool->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(&pool->waiters); return pool; } -- 2.17.1
[PATCH 1/6] migration/savevm: respect qemu_fclose() error code in save_snapshot()
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 = &sn1, old_sn1, *old_sn = &old_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
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, &qiov, 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, &t->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 @@ bdrv_co_rw_vmstate(Block
[PATCH 4/6] block/block-backend: remove always true check from blk_save_vmstate
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
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
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: /usr/lib64/clang/10.0.0/lib/linux/libclang
[PATCH v10 05/12] floppy: move cmos_get_fd_drive_type() from pc
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.
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()
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, &ambiguous); +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
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&KfgmB7Wac^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+&N4%p;5Dzm0BSUmU zfT6K769dogG08W@jfL Kwb_laR004i*&G%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&KfgmB7U!;3lKb3{wbFHMxz+Yw}9Q`ogG08W@jfL eCEzBb&kVD~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&C?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&c&j 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&t^#`2N?hd0uLns delta 152 zcmeCTxoN}Y66_KZDaXLTShA68K4ZNDyIy>-Q+$B4r$Ka+Gn;3yV?0N^qe~DE1A{z6 zbVGn=P
[PATCH v10 03/12] acpi: move aml builder code for floppy device
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, &maxc, &maxh, &maxs); + +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 = &vmstate_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, &maxc, &maxh, &maxs); - -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()
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()
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
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
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, &ambiguous); -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 = &vmstate_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
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
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
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
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
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
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